diff --git a/review_r92_test.go b/review_r92_test.go new file mode 100644 index 0000000..55cd9ac --- /dev/null +++ b/review_r92_test.go @@ -0,0 +1,54 @@ +package traefikoidc + +import ( + "encoding/base64" + "encoding/json" + "testing" + "time" +) + +// makeTestJWT builds a syntactically valid 3-part JWT with the given claims. +// The signature segment is not cryptographically validated by the code under +// test (accessTokenUnexpired only parses claims), so a static value is fine. +func makeTestJWT(t *testing.T, claims map[string]interface{}) string { + t.Helper() + hdr, err := json.Marshal(map[string]string{"alg": "RS256", "typ": "JWT"}) + if err != nil { + t.Fatalf("marshal header: %v", err) + } + pl, err := json.Marshal(claims) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + return base64.RawURLEncoding.EncodeToString(hdr) + "." + + base64.RawURLEncoding.EncodeToString(pl) + ".c2ln" +} + +// R92 regression: on the lenient-audience access-token path, +// accessTokenUnexpired previously re-checked only 'exp' after jwt.Verify +// short-circuited at the aud check, leaving 'nbf' (and 'iat') unvalidated. +// A token with a not-yet-reached nbf (valid exp) was therefore trusted for +// authorization before it was valid. It must now reject not-yet-valid +// tokens just like the main verification path (verifyNotBefore). +func TestR92LenientPathRejectsFutureNbf(t *testing.T) { + tm := &TraefikOidc{} + now := time.Now() + + notYetValid := makeTestJWT(t, map[string]interface{}{ + "exp": float64(now.Add(time.Hour).Unix()), + "iat": float64(now.Add(-time.Minute).Unix()), + "nbf": float64(now.Add(10 * time.Minute).Unix()), // not yet valid + }) + if tm.accessTokenUnexpired(notYetValid) { + t.Error("access token with future nbf was accepted on the lenient-audience path (not-yet-valid token trusted)") + } + + // Control: a token with no nbf and a valid exp must still be accepted. + valid := makeTestJWT(t, map[string]interface{}{ + "exp": float64(now.Add(time.Hour).Unix()), + "iat": float64(now.Add(-time.Minute).Unix()), + }) + if !tm.accessTokenUnexpired(valid) { + t.Error("token with valid exp and no nbf should be accepted") + } +} diff --git a/token_validation_rs.go b/token_validation_rs.go index 314065d..2eace8b 100644 --- a/token_validation_rs.go +++ b/token_validation_rs.go @@ -38,21 +38,44 @@ func (t *TraefikOidc) validateGoogleTokensRS(rs *requestState) (bool, bool, bool return t.validateStandardTokensRS(rs) } -// accessTokenUnexpired reports whether a signature-verified access token's exp -// claim is still in the future. Used on the lenient-audience path where -// jwt.Verify short-circuits at the aud check before validating exp, so expiry -// must be (re)checked here before the token's claims are trusted. Honors the -// same clock-skew tolerance as normal expiration verification. +// accessTokenUnexpired reports whether a signature-verified access +// token's time claims still hold. Used on the lenient-audience path where +// jwt.Verify short-circuits at the aud check BEFORE reaching exp/iat/nbf +// validation, leaving all time claims unvalidated. Re-apply the same +// clock-skew-aware checks the main path applies (verifyExpiration, +// verifyIssuedAt with iat required, and verifyNotBefore when present) so a +// token that is expired, used before its issue time, or not yet valid (nbf) +// is not trusted for authorization here. func (t *TraefikOidc) accessTokenUnexpired(token string) bool { parsed, err := parseJWT(token) if err != nil { return false } - exp, ok := parsed.Claims["exp"].(float64) + claims := parsed.Claims + + exp, ok := claims["exp"].(float64) if !ok { return false } - return time.Now().Before(time.Unix(int64(exp), 0).Add(ClockSkewToleranceFuture)) + if err := verifyExpiration(exp); err != nil { + return false + } + + iat, ok := claims["iat"].(float64) + if !ok { + return false + } + if err := verifyIssuedAt(iat); err != nil { + return false + } + + if nbf, ok := claims["nbf"].(float64); ok { + if err := verifyNotBefore(nbf); err != nil { + return false + } + } + + return true } // validateTokenExpiryRS is the requestState-aware variant of validateTokenExpiry. @@ -235,8 +258,9 @@ func (t *TraefikOidc) validateStandardTokensRS(rs *requestState) (bool, bool, bo // there is no ID token to compare against. jwt.Verify returns at // the aud check BEFORE reaching the exp check, so expiry was never // validated on this path — an expired, wrong-audience token would - // otherwise be accepted as authenticated. Require the token to be - // still unexpired before trusting its claims for authorization. + // otherwise be accepted as authenticated. Require the token's time + // claims (exp, iat, nbf) to all still hold before trusting its + // claims for authorization. if !t.accessTokenUnexpired(rs.accessToken) { if rs.refreshToken != "" { return false, true, false