Skip to content

Commit 509b016

Browse files
Added check to handle token parsing with claims without "sub". (#3287)
### Description In the oidc_introspection.py `func (filter *oidcIntrospectionFilter) Request(ctx filters.FilterContext)` function get value form the Claim map using `sub` key and then casts the value into a string, but this would panic if no sub key is present. I added a check to ensure we only cast values if sub key is present. ### Linked Issue closes #3216 ### Approach to solution As describe in the following [comment](#3216 (comment)), I dug a bit deeper and found 2 suitable values to use if subject is not present in token. One is the Subject value in tokenContainer struct or UserInfo Struct. I decided to go with the tokenContainer struct. If this is not correct please do guide me if UserInfo Struct would be better. --------- Signed-off-by: wassafshahzad <wassafshahzad@gmail.com>
1 parent 91c0bae commit 509b016

3 files changed

Lines changed: 32 additions & 10 deletions

File tree

filters/auth/oidc_introspection.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,12 @@ func (filter *oidcIntrospectionFilter) Request(ctx filters.FilterContext) {
139139
return
140140
}
141141

142-
sub := token.Claims["sub"].(string)
142+
sub, ok := token.Claims["sub"].(string)
143+
if !ok {
144+
unauthorized(ctx, fmt.Sprint(filter.typ), invalidSub, r.Host, "Invalid Subject")
145+
return
146+
}
147+
143148
authorized(ctx, sub)
144149
}
145150

filters/auth/oidc_introspection_test.go

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,12 @@ func TestCreateOIDCQueryClaimsFilter(t *testing.T) {
139139

140140
func TestOIDCQueryClaimsFilter(t *testing.T) {
141141
for _, tc := range []struct {
142-
msg string
143-
path string
144-
expected int
145-
expectErr bool
146-
args []interface{}
142+
msg string
143+
path string
144+
expected int
145+
expectErr bool
146+
args []interface{}
147+
removeClaims []string
147148
}{
148149
{
149150
msg: "secure sub/path not permitted",
@@ -165,6 +166,17 @@ func TestOIDCQueryClaimsFilter(t *testing.T) {
165166
expected: 200,
166167
expectErr: false,
167168
},
169+
{
170+
msg: "missing sub claim is not permitted",
171+
args: []interface{}{
172+
"/login:groups.#[==\"AppX-Test-Users\"]",
173+
"/:@_:email%\"*@example.org\"",
174+
},
175+
path: "/login/page",
176+
expected: 401,
177+
expectErr: false,
178+
removeClaims: []string{"sub"},
179+
},
168180
{
169181
msg: "generic user path permitted",
170182
args: []interface{}{
@@ -292,7 +304,7 @@ func TestOIDCQueryClaimsFilter(t *testing.T) {
292304
t.Errorf("Failed to parse url %s: %v", proxy.URL, err)
293305
}
294306
reqURL.Path = tc.path
295-
oidcServer := createOIDCServer(proxy.URL+"/redirect", validClient, "mysec", jwt.MapClaims{"groups": []string{"CD-Administrators", "Purchasing-Department", "AppX-Test-Users", "white space"}})
307+
oidcServer := createOIDCServer(proxy.URL+"/redirect", validClient, "mysec", jwt.MapClaims{"groups": []string{"CD-Administrators", "Purchasing-Department", "AppX-Test-Users", "white space"}}, tc.removeClaims)
296308
defer oidcServer.Close()
297309
t.Logf("oidc/auth server URL: %s", oidcServer.URL)
298310
// create filter

filters/auth/oidc_test.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ var testOpenIDConfig = `{
141141
// returns a localhost instance implementation of an OpenID Connect
142142
// server with configendpoint, tokenendpoint, authenticationserver endpoint, userinfor
143143
// endpoint, jwks endpoint
144-
func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims) *httptest.Server {
144+
func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims, removeClaims []string) *httptest.Server {
145145
var oidcServer *httptest.Server
146146
oidcServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
147147
switch r.URL.Path {
@@ -247,6 +247,11 @@ func createOIDCServer(cb, client, clientsecret string, extraClaims jwt.MapClaims
247247
for k, v := range extraClaims {
248248
claims[k] = v
249249
}
250+
251+
for _, k := range removeClaims {
252+
delete(claims, k)
253+
}
254+
250255
token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims)
251256

252257
privKey, err := os.ReadFile(keyPath)
@@ -571,7 +576,7 @@ func TestNewOidc(t *testing.T) {
571576
}
572577

573578
func TestCreateFilterOIDC(t *testing.T) {
574-
oidcServer := createOIDCServer("", "", "", nil)
579+
oidcServer := createOIDCServer("", "", "", nil, nil)
575580
defer oidcServer.Close()
576581

577582
for _, tt := range []struct {
@@ -924,7 +929,7 @@ func TestOIDCSetup(t *testing.T) {
924929

925930
t.Logf("redirect URL: %s", redirectURL.String())
926931

927-
oidcServer := createOIDCServer(redirectURL.String(), "valid-client", "mysec", tc.extraClaims)
932+
oidcServer := createOIDCServer(redirectURL.String(), "valid-client", "mysec", tc.extraClaims, nil)
928933
defer oidcServer.Close()
929934
t.Logf("oidc server URL: %s", oidcServer.URL)
930935

0 commit comments

Comments
 (0)