From f75b964a6b46aa5046f11ea2b4a2ebfc793b6d43 Mon Sep 17 00:00:00 2001 From: Juliusz Chroboczek Date: Mon, 9 Jan 2023 15:48:09 +0100 Subject: [PATCH] Distinguish tokens with empty sub from no sub We now distinguish between tokens that specify an empty username (sub="") and tokens that don't specify sub. The latter are considered invalid for now. --- group/group.go | 9 +++++++-- token/token.go | 18 +++++++++--------- token/token_test.go | 39 +++++++++++++++++++++++++++++++++++---- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/group/group.go b/group/group.go index a5f0844..0737bad 100644 --- a/group/group.go +++ b/group/group.go @@ -1148,7 +1148,12 @@ func (g *Group) getPermission(creds ClientCredentials) (string, []string, error) log.Printf("Token authentication: %v", err) return "", nil, ErrNotAuthorised } - if !desc.AllowAnonymous && sub == "" { + if sub == nil { + log.Printf("Token authentication: token has no sub") + return "", nil, ErrNotAuthorised + } + username := *sub + if !desc.AllowAnonymous && username == "" { return "", nil, ErrAnonymousNotAuthorised } conf, err := GetConfiguration() @@ -1181,7 +1186,7 @@ func (g *Group) getPermission(creds ClientCredentials) (string, []string, error) if !ok { return "", nil, ErrNotAuthorised } - return sub, perms, nil + return username, perms, nil } func (g *Group) GetPermission(creds ClientCredentials) (string, []string, error) { diff --git a/token/token.go b/token/token.go index 88407a0..043f074 100644 --- a/token/token.go +++ b/token/token.go @@ -115,23 +115,23 @@ func toStringArray(a []interface{}) ([]string, bool) { return b, true } -func Valid(token string, keys []map[string]interface{}) (string, []string, []string, error) { +func Valid(token string, keys []map[string]interface{}) (*string, []string, []string, error) { tok, err := jwt.Parse(token, func(t *jwt.Token) (interface{}, error) { return getKey(t.Header, keys) }) if err != nil { - return "", nil, nil, err + return nil, nil, nil, err } claims := tok.Claims.(jwt.MapClaims) - var sub string + var sub *string if s, ok := claims["sub"]; ok && s != nil { ss, ok := s.(string) if !ok { - return "", nil, nil, + return nil, nil, nil, errors.New("invalid 'sub' field") } - sub = ss + sub = &ss } var aud []string @@ -142,11 +142,11 @@ func Valid(token string, keys []map[string]interface{}) (string, []string, []str case []interface{}: aud, ok = toStringArray(a) if !ok { - return "", nil, nil, + return nil, nil, nil, errors.New("invalid 'aud' field") } default: - return "", nil, nil, + return nil, nil, nil, errors.New("invalid 'aud' field") } } @@ -155,12 +155,12 @@ func Valid(token string, keys []map[string]interface{}) (string, []string, []str if p, ok := claims["permissions"]; ok && p != nil { pp, ok := p.([]interface{}) if !ok { - return "", nil, nil, + return nil, nil, nil, errors.New("invalid 'permissions' field") } perms, ok = toStringArray(pp) if !ok { - return "", nil, nil, + return nil, nil, nil, errors.New("invalid 'permissions' field") } } diff --git a/token/token_test.go b/token/token_test.go index 689f4a1..c6d7dee 100644 --- a/token/token_test.go +++ b/token/token_test.go @@ -70,11 +70,44 @@ func TestValid(t *testing.T) { goodToken := "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJqb2huIiwiYXVkIjoiaHR0cHM6Ly9nYWxlbmUub3JnOjg0NDMvZ3JvdXAvYXV0aC8iLCJwZXJtaXNzaW9ucyI6WyJwcmVzZW50Il0sImlhdCI6MTY0NTMxMDI5NCwiZXhwIjoyOTA2NzUwMjk0LCJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjEyMzQvIn0.6xXpgBkBMn4PSBpnwYHb-gRn_Q97Yq9DoKkAf2_6iwc" sub, aud, perms, err := Valid(goodToken, keys) - if err != nil { t.Errorf("Token invalid: %v", err) } else { - if sub != "john" { + if sub == nil || *sub != "john" { + t.Errorf("Unexpected sub: %v", sub) + } + if !reflect.DeepEqual(aud, []string{"https://galene.org:8443/group/auth/"}) { + t.Errorf("Unexpected aud: %v", aud) + } + if !reflect.DeepEqual(perms, []string{"present"}) { + t.Errorf("Unexpected perms: %v", perms) + } + } + + anonymousToken := "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIiLCJhdWQiOiJodHRwczovL2dhbGVuZS5vcmc6ODQ0My9ncm91cC9hdXRoLyIsInBlcm1pc3Npb25zIjpbInByZXNlbnQiXSwiaWF0IjoxNjQ1MzEwMjk0LCJleHAiOjI5MDY3NTAyOTQsImlzcyI6Imh0dHA6Ly9sb2NhbGhvc3Q6MTIzNC8ifQo.xwpHIRzKAIgiHKG1pVQyZlXcolmvRwNvBm6FN2gTwZw" + + sub, aud, perms, err = Valid(anonymousToken, keys) + if err != nil { + t.Errorf("Token invalid: %v", err) + } else { + if sub == nil || *sub != "" { + t.Errorf("Unexpected sub: %v", sub) + } + if !reflect.DeepEqual(aud, []string{"https://galene.org:8443/group/auth/"}) { + t.Errorf("Unexpected aud: %v", aud) + } + if !reflect.DeepEqual(perms, []string{"present"}) { + t.Errorf("Unexpected perms: %v", perms) + } + } + + noSubToken := "eyJhbGciOiJIUzI1NiJ9.eyJhdWQiOiJodHRwczovL2dhbGVuZS5vcmc6ODQ0My9ncm91cC9hdXRoLyIsInBlcm1pc3Npb25zIjpbInByZXNlbnQiXSwiaWF0IjoxNjQ1MzEwMjk0LCJleHAiOjI5MDY3NTAyOTQsImlzcyI6Imh0dHA6Ly9sb2NhbGhvc3Q6MTIzNC8ifQo.7LvoZEKPNVvsRe8SjLxmKa1TgjTA4ZQo2LMPJSXl-ro" + + sub, aud, perms, err = Valid(noSubToken, keys) + if err != nil { + t.Errorf("Token invalid: %v", err) + } else { + if sub != nil { t.Errorf("Unexpected sub: %v", sub) } if !reflect.DeepEqual(aud, []string{"https://galene.org:8443/group/auth/"}) { @@ -88,7 +121,6 @@ func TestValid(t *testing.T) { badToken := "eyJ0eXAiOiJKV1QiLCJhbGciOiJub25lIn0.eyJzdWIiOiJqb2huIiwiYXVkIjoiaHR0cHM6Ly9nYWxlbmUub3JnOjg0NDMvZ3JvdXAvYXV0aC8iLCJwZXJtaXNzaW9ucyI6WyJwcmVzZW50Il0sImlhdCI6MTY0NTMxMDQ2OSwiZXhwIjoyOTA2NzUwNDY5LCJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjEyMzQvIn0." _, _, _, err = Valid(badToken, keys) - var verr *jwt.ValidationError if !errors.As(err, &verr) { t.Errorf("Token should fail") @@ -97,7 +129,6 @@ func TestValid(t *testing.T) { expiredToken := "eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJqb2huIiwiYXVkIjoiaHR0cHM6Ly9nYWxlbmUub3JnOjg0NDMvZ3JvdXAvYXV0aC8iLCJwZXJtaXNzaW9ucyI6WyJwcmVzZW50Il0sImlhdCI6MTY0NTMxMDMyMiwiZXhwIjoxNjQ1MzEwMzUyLCJpc3MiOiJodHRwOi8vbG9jYWxob3N0OjEyMzQvIn0.jyqRhoV6iK54SvlP33Fy630aDo-sLNmKKi1kcfqs378" _, _, _, err = Valid(expiredToken, keys) - if !errors.As(err, &verr) { t.Errorf("Token should be expired") }