From 7c37624defa650c73366cbfcc22a0eded5fb536f Mon Sep 17 00:00:00 2001 From: wassafshahzad Date: Sat, 26 Oct 2024 02:49:07 +0200 Subject: [PATCH 1/4] Added check to ensure 'sub' is present in claim before parsing and use token subjact if sub is not present in claims Signed-off-by: wassafshahzad --- filters/auth/oidc_introspection.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/filters/auth/oidc_introspection.go b/filters/auth/oidc_introspection.go index 8d21dc4ec1..6a1760571b 100644 --- a/filters/auth/oidc_introspection.go +++ b/filters/auth/oidc_introspection.go @@ -139,8 +139,13 @@ func (filter *oidcIntrospectionFilter) Request(ctx filters.FilterContext) { return } - sub := token.Claims["sub"].(string) - authorized(ctx, sub) + sub, ok := token.Claims["sub"] + if ok { + authorized(ctx, sub.(string)) + } else { + sub := token.Subject + authorized(ctx, sub) + } } func (filter *oidcIntrospectionFilter) Response(filters.FilterContext) {} From 6957cd7e7da72733c38ac51d719365d6bbd09372 Mon Sep 17 00:00:00 2001 From: wassafshahzad Date: Mon, 18 Nov 2024 03:08:08 +0100 Subject: [PATCH 2/4] Fixes issue with sub in token claims Signed-off-by: wassafshahzad --- filters/auth/oidc_introspection.go | 12 ++++++------ filters/auth/oidc_introspection_test.go | 24 ++++++++++++++++++------ filters/auth/oidc_test.go | 11 ++++++++--- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/filters/auth/oidc_introspection.go b/filters/auth/oidc_introspection.go index 6a1760571b..84827704ed 100644 --- a/filters/auth/oidc_introspection.go +++ b/filters/auth/oidc_introspection.go @@ -139,13 +139,13 @@ func (filter *oidcIntrospectionFilter) Request(ctx filters.FilterContext) { return } - sub, ok := token.Claims["sub"] - if ok { - authorized(ctx, sub.(string)) - } else { - sub := token.Subject - authorized(ctx, sub) + sub, ok := token.Claims["sub"].(string) + if !ok { + unauthorized(ctx, sub, invalidSub, "", "") + return } + + authorized(ctx, sub) } func (filter *oidcIntrospectionFilter) Response(filters.FilterContext) {} diff --git a/filters/auth/oidc_introspection_test.go b/filters/auth/oidc_introspection_test.go index c20a7d7482..1a4186ee0a 100644 --- a/filters/auth/oidc_introspection_test.go +++ b/filters/auth/oidc_introspection_test.go @@ -139,11 +139,12 @@ func TestCreateOIDCQueryClaimsFilter(t *testing.T) { func TestOIDCQueryClaimsFilter(t *testing.T) { for _, tc := range []struct { - msg string - path string - expected int - expectErr bool - args []interface{} + msg string + path string + expected int + expectErr bool + args []interface{} + removeClaims []string }{ { msg: "secure sub/path not permitted", @@ -165,6 +166,17 @@ func TestOIDCQueryClaimsFilter(t *testing.T) { expected: 200, expectErr: false, }, + { + msg: "secure sub/path is not permitted", + args: []interface{}{ + "/login:groups.#[==\"AppX-Test-Users\"]", + "/:@_:email%\"*@example.org\"", + }, + path: "/login/page", + expected: 401, + expectErr: false, + removeClaims: []string{"sub"}, + }, { msg: "generic user path permitted", args: []interface{}{ @@ -292,7 +304,7 @@ func TestOIDCQueryClaimsFilter(t *testing.T) { t.Errorf("Failed to parse url %s: %v", proxy.URL, err) } reqURL.Path = tc.path - oidcServer := createOIDCServer(proxy.URL+"/redirect", validClient, "mysec", jwt.MapClaims{"groups": []string{"CD-Administrators", "Purchasing-Department", "AppX-Test-Users", "white space"}}) + oidcServer := createOIDCServer(proxy.URL+"/redirect", validClient, "mysec", jwt.MapClaims{"groups": []string{"CD-Administrators", "Purchasing-Department", "AppX-Test-Users", "white space"}}, tc.removeClaims) defer oidcServer.Close() t.Logf("oidc/auth server URL: %s", oidcServer.URL) // create filter diff --git a/filters/auth/oidc_test.go b/filters/auth/oidc_test.go index 771fde01fe..0b1b3839e3 100644 --- a/filters/auth/oidc_test.go +++ b/filters/auth/oidc_test.go @@ -127,7 +127,7 @@ var testOpenIDConfig = `{ // returns a localhost instance implementation of an OpenID Connect // server with configendpoint, tokenendpoint, authenticationserver endpoint, userinfor // endpoint, jwks endpoint -func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims) *httptest.Server { +func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims, removeClaims []string) *httptest.Server { var oidcServer *httptest.Server oidcServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { @@ -233,6 +233,11 @@ func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims for k, v := range extraClaims { claims[k] = v } + + for _, k := range removeClaims { + delete(claims, k) + } + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) privKey, err := os.ReadFile(keyPath) @@ -557,7 +562,7 @@ func TestNewOidc(t *testing.T) { } func TestCreateFilterOIDC(t *testing.T) { - oidcServer := createOIDCServer("", "", "", nil) + oidcServer := createOIDCServer("", "", "", nil, nil) defer oidcServer.Close() for _, tt := range []struct { @@ -900,7 +905,7 @@ func TestOIDCSetup(t *testing.T) { t.Logf("redirect URL: %s", redirectURL.String()) - oidcServer := createOIDCServer(redirectURL.String(), "valid-client", "mysec", tc.extraClaims) + oidcServer := createOIDCServer(redirectURL.String(), "valid-client", "mysec", tc.extraClaims, nil) defer oidcServer.Close() t.Logf("oidc server URL: %s", oidcServer.URL) From b523e87180c3af08d3a4651d48b99e895b53e067 Mon Sep 17 00:00:00 2001 From: wassafshahzad Date: Sat, 8 Feb 2025 21:22:37 +0100 Subject: [PATCH 3/4] Addresses feedback Signed-off-by: wassafshahzad --- filters/auth/oidc_introspection.go | 2 +- filters/auth/oidc_introspection_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/filters/auth/oidc_introspection.go b/filters/auth/oidc_introspection.go index 84827704ed..73c74d52af 100644 --- a/filters/auth/oidc_introspection.go +++ b/filters/auth/oidc_introspection.go @@ -141,7 +141,7 @@ func (filter *oidcIntrospectionFilter) Request(ctx filters.FilterContext) { sub, ok := token.Claims["sub"].(string) if !ok { - unauthorized(ctx, sub, invalidSub, "", "") + unauthorized(ctx, r.Host, invalidSub, "", "") return } diff --git a/filters/auth/oidc_introspection_test.go b/filters/auth/oidc_introspection_test.go index 1a4186ee0a..34647d802f 100644 --- a/filters/auth/oidc_introspection_test.go +++ b/filters/auth/oidc_introspection_test.go @@ -167,7 +167,7 @@ func TestOIDCQueryClaimsFilter(t *testing.T) { expectErr: false, }, { - msg: "secure sub/path is not permitted", + msg: "missing sub claim is not permitted", args: []interface{}{ "/login:groups.#[==\"AppX-Test-Users\"]", "/:@_:email%\"*@example.org\"", From 6077f54a25782daaa740ddfb93ee7f6b922ab577 Mon Sep 17 00:00:00 2001 From: wassafshahzad Date: Mon, 24 Mar 2025 05:17:47 +0100 Subject: [PATCH 4/4] Updated function signature Signed-off-by: wassafshahzad --- filters/auth/oidc_introspection.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filters/auth/oidc_introspection.go b/filters/auth/oidc_introspection.go index 73c74d52af..4dbcd20175 100644 --- a/filters/auth/oidc_introspection.go +++ b/filters/auth/oidc_introspection.go @@ -141,7 +141,7 @@ func (filter *oidcIntrospectionFilter) Request(ctx filters.FilterContext) { sub, ok := token.Claims["sub"].(string) if !ok { - unauthorized(ctx, r.Host, invalidSub, "", "") + unauthorized(ctx, fmt.Sprint(filter.typ), invalidSub, r.Host, "Invalid Subject") return }