From c12512bdd70c20efe8dad694bb2d66eecf77902d Mon Sep 17 00:00:00 2001 From: Max Kotliar Date: Tue, 10 Feb 2026 18:57:17 +0200 Subject: [PATCH] lib/jwt: address code review comments (#10428) ### Describe Your Changes Addressing code revoew comments from https://github.com/VictoriaMetrics/VictoriaMetrics/pull/10426, kept them separate to isolate copy-paste change from follow up changes ### Checklist The following checks are **mandatory**: - [ ] My change adheres to [VictoriaMetrics contributing guidelines](https://docs.victoriametrics.com/victoriametrics/contributing/#pull-request-checklist). - [ ] My change adheres to [VictoriaMetrics development goals](https://docs.victoriametrics.com/victoriametrics/goals/). --- lib/jwt/algo_ps.go | 2 +- lib/jwt/jwt.go | 23 ++++++++++++------- lib/jwt/jwt_test.go | 43 +++++++++++++++++++++++++++++++---- lib/jwt/verifier_pool_test.go | 2 +- 4 files changed, 56 insertions(+), 14 deletions(-) diff --git a/lib/jwt/algo_ps.go b/lib/jwt/algo_ps.go index ddb1da821a..e7afe7779c 100644 --- a/lib/jwt/algo_ps.go +++ b/lib/jwt/algo_ps.go @@ -5,7 +5,7 @@ import ( "crypto/rsa" ) -// newVerifierPS returns a new RSA-PSS-based signer. +// newVerifierPS returns a new RSA-PSS-based verifier. func newVerifierPS(alg Algorithm, key *rsa.PublicKey) (*psAlg, error) { if key == nil { return nil, ErrNilKey diff --git a/lib/jwt/jwt.go b/lib/jwt/jwt.go index 0822d95485..7da2c151a7 100644 --- a/lib/jwt/jwt.go +++ b/lib/jwt/jwt.go @@ -98,8 +98,15 @@ func NewToken(auth string, enforceAuthPrefix bool) (*Token, error) { if enforceAuthPrefix && (len(auth) < len(prefix) || !strings.EqualFold(auth[:len(prefix)], prefix)) { return nil, fmt.Errorf("wrong format, prefix: %s is missing", prefix) } - // remove prefix if it is present - auth = strings.TrimPrefix(auth, prefix) + + // While https://datatracker.ietf.org/doc/html/rfc6750#section-2.1 states that only Bearer prefix is allowed, + // it claims to be conformant to the generic syntax defined in https://datatracker.ietf.org/doc/html/rfc2617#section-1.2 + // which permits case-insensitive auth scheme. + // So we should be tolerant to different cases of "Bearer" prefix. + if len(auth) >= len(prefix) && strings.EqualFold(auth[:len(prefix)], prefix) { + auth = auth[len(prefix):] + } + jwt := strings.SplitN(auth, ".", 3) if len(jwt) != 3 { return nil, ErrBadTokenFormat @@ -203,9 +210,9 @@ func parseJWTBody(data string) (*body, error) { // expired at time unix_ts Exp int64 `json:"exp"` // issued at time unix_ts - Iat int64 `json:"iat"` - Jti string `json:"jti,omitempty"` - Scope *json.RawMessage `json:"scope,omitempty"` + Iat int64 `json:"iat"` + Jti string `json:"jti,omitempty"` + Scope json.RawMessage `json:"scope,omitempty"` // store as raw message to support different types VMAccess *json.RawMessage `json:"vm_access"` } @@ -240,9 +247,9 @@ func parseJWTBody(data string) (*body, error) { // some IDPs encode scope as a string and some as an array var scope string if tb.Scope != nil { - if err := json.Unmarshal(*tb.Scope, &scope); err != nil { + if err := json.Unmarshal(tb.Scope, &scope); err != nil { var scopeSlice []string - if err := json.Unmarshal(*tb.Scope, &scopeSlice); err != nil { + if err := json.Unmarshal(tb.Scope, &scopeSlice); err != nil { return nil, fmt.Errorf("cannot parse jwt body scope: %w", err) } scope = strings.Join(scopeSlice, " ") @@ -262,7 +269,7 @@ func parseJWTBody(data string) (*body, error) { func decodeB64(data []byte) ([]byte, error) { idx := bytes.IndexAny(data, "+/") // slow path, std base64 encoding convert it to url encoding - if idx > 0 { + if idx >= 0 { for idx, c := range data { switch c { case '+': diff --git a/lib/jwt/jwt_test.go b/lib/jwt/jwt_test.go index 074f0c7626..b4bbd283bb 100644 --- a/lib/jwt/jwt_test.go +++ b/lib/jwt/jwt_test.go @@ -21,7 +21,8 @@ func TestParseJWTHeader_Failure(t *testing.T) { if err.Error() != expectedErr { t.Errorf("unexpected error message: \ngot\n%s\nwant\n%s", err.Error(), expectedErr) } - return + } else { + t.Errorf("expecting non-nil error") } } @@ -97,7 +98,8 @@ func TestParseJWTBody_Failure(t *testing.T) { if err.Error() != expectedErr { t.Errorf("unexpected error message: \ngot\n%s\nwant\n%s", err.Error(), expectedErr) } - return + } else { + t.Errorf("expecting non-nil error") } } @@ -404,10 +406,10 @@ func TestNewTokenFromRequest_Success(t *testing.T) { t.Fatalf("NewTokenFromRequest() error: %s", err) } if !reflect.DeepEqual(result.body.VMAccess, resultExpected.body.VMAccess) { - t.Fatalf("unxpected token body VMAccess;\ngot\n%v\nwant\n%v", result.body.VMAccess, resultExpected.body.VMAccess) + t.Fatalf("unexpected token body VMAccess;\ngot\n%v\nwant\n%v", result.body.VMAccess, resultExpected.body.VMAccess) } if !reflect.DeepEqual(result.header, resultExpected.header) { - t.Fatalf("unxpected token header\ngot\n%v\nwant\n%v", result.header, resultExpected.header) + t.Fatalf("unexpected token header\ngot\n%v\nwant\n%v", result.header, resultExpected.header) } } @@ -444,6 +446,39 @@ func TestNewTokenFromRequest_Success(t *testing.T) { } f(r, resultExpected, true) + // parse ok with non-standard "BEARER" prefix + r = &http.Request{ + Header: map[string][]string{ + "Authorization": { + "BEARER eyJhbGciOiJSUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICJhQVpvQ0d2dUdiRm9mdFdIeFFaeVJTUWVuM3lYNFUwR1BsUDVvWk9RU3djIn0.eyJleHAiOjE2MTA5NzYxODksImlhdCI6MTYxMDk3NTg4OSwiYXV0aF90aW1lIjoxNjEwOTc1ODg5LCJqdGkiOiI5YjE5NDE4Ny02YmI3LTQyNDQtOWQxYi01NTllYWIyZWY3ZjMiLCJpc3MiOiJodHRwczovL2xvY2FsaG9zdDo4NDQzL2F1dGgvcmVhbG1zL3Rlc3QiLCJhdWQiOiJhY2NvdW50Iiwic3ViIjoiNDYwODU5NDEtYjkyYi00NzFhLWIwNWEtOTU5OWNhMjlkYTFlIiwidHlwIjoiQmVhcmVyIiwiYXpwIjoiZ3JhZmFuYSIsInNlc3Npb25fc3RhdGUiOiIxMzc3ZDEwMi03NTJiLTQ0ODYtOTlkYS1jMjA4MjRiODJkMzEiLCJhY3IiOiIxIiwiYWxsb3dlZC1vcmlnaW5zIjpbImh0dHA6Ly9sb2NhbGhvc3Q6MzAwMCJdLCJyZWFsbV9hY2Nlc3MiOnsicm9sZXMiOlsib2ZmbGluZV9hY2Nlc3MiLCJ1bWFfYXV0aG9yaXphdGlvbiJdfSwicmVzb3VyY2VfYWNjZXNzIjp7ImFjY291bnQiOnsicm9sZXMiOlsibWFuYWdlLWFjY291bnQiLCJtYW5hZ2UtYWNjb3VudC1saW5rcyIsInZpZXctcHJvZmlsZSJdfX0sInNjb3BlIjoib3BlbmlkIGVtYWlsIHByb2ZpbGUiLCJ2bV9hY2Nlc3MiOnsiZXh0cmFfbGFiZWxzIjp7InByb2plY3QiOiJkZXYiLCJ0ZWFtIjoibW9iaWxlIn0sInRlbmFudF9pZCI6eyJhY2NvdW50X2lkIjoxLCJwcm9qZWN0X2lkIjo1fX0sImVtYWlsX3ZlcmlmaWVkIjpmYWxzZSwibmFtZSI6InRnIHRnIiwicHJvamVjdCI6Im1vYmlsZSIsInByZWZlcnJlZF91c2VybmFtZSI6InRnIiwidGVhbSI6ImRldiIsImdpdmVuX25hbWUiOiJ0ZyIsImZhbWlseV9uYW1lIjoidGciLCJlbWFpbCI6InRnQGZnaHQubmV0In0.XErPkz-qL-EV8BBAR17MoFytc5ajYRz71f9_GOuG1AVcMnUsD6D3x4z5jL1dLyoGGm8OUW_RIVrjMpZf_xOfgQKRVHAMaJi64UtpwS8EF50mlOCDAdKl6wlzAS4laV3dW9W9QrTH7TMetG33WVsJGaD-MIwSYJ5peh6u__oniezsRavw8Qw3nLpZCQPb-NatT3Q1raj1ymLJErJPtUBSk3ieWCVpTMo4ZYKFIQt2wjHeOVOF_3suhPfhgEgXlN6aUq3xeYJ1aAtl_5Ao3pB2pto46kDSXIulQQuGdttsw7bSDOYqZ-tx3y7DBWNdIcghsO_iMvrA805j5hG4Nu84Sw", + }, + }, + } + resultExpected = &Token{ + body: &body{ + Exp: 1610889266, + Iat: 1610888966, + Jti: "09a058a2-0752-4ecd-a4e9-b65e85af423f", + Scope: "openid email profile", + VMAccess: &access{ + Tenant: TenantID{ + ProjectID: 5, + AccountID: 1, + }, + Labels: map[string]string{ + "project": "dev", + "team": "mobile", + }, + }, + }, + header: &header{ + Alg: "RS256", + Kid: "aAZoCGvuGbFoftWHxQZyRSQen3yX4U0GPlP5oZOQSwc", + Typ: "JWT", + }, + } + f(r, resultExpected, true) + // go-jwt r = &http.Request{ Header: map[string][]string{ diff --git a/lib/jwt/verifier_pool_test.go b/lib/jwt/verifier_pool_test.go index 38499bfb45..a111f3f3b7 100644 --- a/lib/jwt/verifier_pool_test.go +++ b/lib/jwt/verifier_pool_test.go @@ -10,7 +10,7 @@ func TestTokenVerify_Failure(t *testing.T) { token, err := NewToken(auth, true) if err != nil { - return + t.Fatalf("unexpected error parsing token: %s", err) } k, err := ParseKey([]byte(key))