From 91a4e5bbb946883e829d791a4e6f93c9ab3a4572 Mon Sep 17 00:00:00 2001 From: John Rood Date: Tue, 7 Jun 2022 21:59:45 +0200 Subject: [PATCH] Propagate JWT parse error, fixes https://github.com/vmware-archive/gangway/issues/73. Also correct the JWT signatures in the now failing handler tests. --- CHANGELOG.md | 2 +- cmd/gangway/handlers_test.go | 10 +++++----- internal/oidc/token.go | 4 ++-- internal/oidc/token_test.go | 25 +++++++++++++++++++++++-- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e8038f33..c9e9607ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,7 @@ Fixes https://github.com/vmware-archive/gangway/issues/71 * BREAKING - corrected ENV variable name of `customHTMLTemplatesDir` to `CUSTOM_HTML_TEMPLATES_DIR`, this was (incorrectly) `CUSTOM_HTTP_TEMPLATES_DIR` * Config option `showClaims` to show/hide received claims - +* Propagate JWT parse error (https://github.com/vmware-archive/gangway/issues/73) ## v3.3.0 (2021-07-15) diff --git a/cmd/gangway/handlers_test.go b/cmd/gangway/handlers_test.go index 0fa99737f..151bc12b8 100644 --- a/cmd/gangway/handlers_test.go +++ b/cmd/gangway/handlers_test.go @@ -146,7 +146,7 @@ func TestCommandLineHandler(t *testing.T) { "default": { params: map[string]string{ "state": "Uv38ByGCZU8WP18PmmIdcpVmx00QA3xNe7sEB9Hixkk=", - "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.zNG4Dnxr76J0p4phfsAUYWunioct0krkMiunMynlQsU", + "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.QxBLa0qPxN2GgrUKc22BTBTks_TUs5xxDDtCKbRtmBg", "refresh_token": "bar", "code": "0cj0VQzNl36e4P2L&state=jdep4ov52FeUuzWLDDtSXaF4b5%2F%2FCUJ52xlE69ehnQ8%3D", }, @@ -158,7 +158,7 @@ func TestCommandLineHandler(t *testing.T) { "incorrect username claim": { params: map[string]string{ "state": "Uv38ByGCZU8WP18PmmIdcpVmx00QA3xNe7sEB9Hixkk=", - "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.zNG4Dnxr76J0p4phfsAUYWunioct0krkMiunMynlQsU", + "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.QxBLa0qPxN2GgrUKc22BTBTks_TUs5xxDDtCKbRtmBg", "refresh_token": "bar", "code": "0cj0VQzNl36e4P2L&state=jdep4ov52FeUuzWLDDtSXaF4b5%2F%2FCUJ52xlE69ehnQ8%3D", }, @@ -169,7 +169,7 @@ func TestCommandLineHandler(t *testing.T) { "no email claim": { params: map[string]string{ "state": "Uv38ByGCZU8WP18PmmIdcpVmx00QA3xNe7sEB9Hixkk=", - "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.zNG4Dnxr76J0p4phfsAUYWunioct0krkMiunMynlQsU", + "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.QxBLa0qPxN2GgrUKc22BTBTks_TUs5xxDDtCKbRtmBg", "refresh_token": "bar", "code": "0cj0VQzNl36e4P2L&state=jdep4ov52FeUuzWLDDtSXaF4b5%2F%2FCUJ52xlE69ehnQ8%3D", }, @@ -277,7 +277,7 @@ func TestKubeconfigHandler(t *testing.T) { ClientSecret: "someClientSecret", }, params: map[string]string{ - "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.zNG4Dnxr76J0p4phfsAUYWunioct0krkMiunMynlQsU", + "id_token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.a5ug38N-hzHYsrFMx3puWfCSD_44lFUUugTsr8J9vH0", "refresh_token": "bar", }, expectedStatusCode: http.StatusOK, @@ -286,7 +286,7 @@ func TestKubeconfigHandler(t *testing.T) { expectedAuthInfoAuthProviderConfig: map[string]string{ "client-id": "someClientID", "client-secret": "someClientSecret", - "id-token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.zNG4Dnxr76J0p4phfsAUYWunioct0krkMiunMynlQsU", + "id-token": "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJHYW5nd2F5VGVzdCIsImlhdCI6MTU0MDA0NjM0NywiZXhwIjoxODg3MjAxNTQ3LCJhdWQiOiJnYW5nd2F5LmhlcHRpby5jb20iLCJzdWIiOiJnYW5nd2F5QGhlcHRpby5jb20iLCJHaXZlbk5hbWUiOiJHYW5nIiwiU3VybmFtZSI6IldheSIsIkVtYWlsIjoiZ2FuZ3dheUBoZXB0aW8uY29tIiwiR3JvdXBzIjoiZGV2LGFkbWluIn0.a5ug38N-hzHYsrFMx3puWfCSD_44lFUUugTsr8J9vH0", "refresh-token": "bar", "idp-issuer-url": "GangwayTest", "idp-certificate-authority-data": "", diff --git a/internal/oidc/token.go b/internal/oidc/token.go index fde54d6a1..7fb0f63b1 100644 --- a/internal/oidc/token.go +++ b/internal/oidc/token.go @@ -33,14 +33,14 @@ type Token struct { // ParseToken returns a jwt token from an idToken, returns error if it cannot parse func ParseToken(idToken, clientSecret string) (*jwt.Token, error) { - token, _ := jwt.Parse(idToken, func(token *jwt.Token) (interface{}, error) { + token, err := jwt.Parse(idToken, func(token *jwt.Token) (interface{}, error) { if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) } return []byte(clientSecret), nil }) - return token, nil + return token, err } // Exchange takes an oauth2 auth token and exchanges for an id_token diff --git a/internal/oidc/token_test.go b/internal/oidc/token_test.go index c9dc7e321..d71c10346 100644 --- a/internal/oidc/token_test.go +++ b/internal/oidc/token_test.go @@ -55,7 +55,7 @@ func TestParseToken(t *testing.T) { "rsa": { idToken: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.EkN-DOsnsuRjRO6BxXemmJDm3HbxrbRzXglbN2S4sOkopdU4IsDxTI8jO19W_A4K8ZPJijNLis4EZsHeY559a4DFOd50_OqgHGuERTqYZyuhtF39yxJPAjUESwxk2J5k_4zM3O-vtd1Ghyo4IbqKKSy6J9mTniYJPenn5-HIirE", clientSecret: "", - expectError: false, + expectError: true, want: &jwt.Token{ Raw: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiYWRtaW4iOnRydWV9.EkN-DOsnsuRjRO6BxXemmJDm3HbxrbRzXglbN2S4sOkopdU4IsDxTI8jO19W_A4K8ZPJijNLis4EZsHeY559a4DFOd50_OqgHGuERTqYZyuhtF39yxJPAjUESwxk2J5k_4zM3O-vtd1Ghyo4IbqKKSy6J9mTniYJPenn5-HIirE", Method: jwt.SigningMethodRS256, @@ -72,6 +72,25 @@ func TestParseToken(t *testing.T) { Valid: false, }, }, + "invalid": { + idToken: "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJmb28iOiJiYXIiLCJleHAiOjE1MDAwLCJpc3MiOiJ0ZXN0In0.HE7fK0xOQwFEr4WDgRWj4teRPZ6i3GLwD5YCm6Pwu_c", + clientSecret: "", + expectError: true, + want: nil, + }, + "alg-none": { + // uses {"alg":"none","typ":"JWT"} + idToken: "eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0K.eyJmb28iOiJiYXIiLCJleHAiOjE1MDAwLCJpc3MiOiJ0ZXN0aiIsImlhdCI6MTQyMjc3OTYzOH0K.", + clientSecret: "", + expectError: true, + want: nil, + }, + "abc": { + idToken: "I know my ABCs", + clientSecret: "", + expectError: true, + want: nil, + }, } for name, tc := range tests { @@ -82,8 +101,10 @@ func TestParseToken(t *testing.T) { // If we expect an error, check that it's thrown if tc.expectError { if err == nil { - t.Fatalf("Error was returned but not expected: %v", err) + t.Fatalf("No error was returned but one expected") } + } else if err != nil { + t.Fatalf("Error was returned but not expected: %v", err) } else { // We don't expect an error, check the result if got.Valid != tc.want.Valid {