From 15339bf5a79656affcccc424919bea8b67a0f770 Mon Sep 17 00:00:00 2001 From: Przemek Pokrywka <12400578+dekiel@users.noreply.github.com> Date: Fri, 25 Oct 2024 15:37:08 +0200 Subject: [PATCH] Verify oidc token against extended expiration time (#12218) * Allow 10 minutes of grace period before the token expire * test * Split token and claims verification Added function for checking extended expiration time * Use extended expiration when standard expiration was to short * Mask raw token in debug logs Configure extended expiration time through flag * Align with the code structure and test update revert to Token align with Token * Accept ClaimsReader interface in ValidateClaims instead a concrete type Check if expiration timestamp is in the future Test VerifyExtendedExpiration method * cleanup TokenProcessor tests --------- Co-authored-by: Patryk Dobrowolski --- cmd/oidc-token-verifier/main.go | 55 ++++++++++++++------ pkg/oidc/oidc.go | 62 ++++++++++++++++------ pkg/oidc/oidc_test.go | 91 ++++++++++++++++++++------------- 3 files changed, 141 insertions(+), 67 deletions(-) diff --git a/cmd/oidc-token-verifier/main.go b/cmd/oidc-token-verifier/main.go index 9ae348842799..993ae9bc82bb 100644 --- a/cmd/oidc-token-verifier/main.go +++ b/cmd/oidc-token-verifier/main.go @@ -1,9 +1,11 @@ package main import ( + "errors" "fmt" "os" + "github.com/coreos/go-oidc/v3/oidc" "github.com/kyma-project/test-infra/pkg/logging" tioidc "github.com/kyma-project/test-infra/pkg/oidc" "github.com/spf13/cobra" @@ -20,13 +22,14 @@ type Logger interface { } type options struct { - token string - clientID string - outputPath string - publicKeyPath string - newPublicKeysVarName string - trustedWorkflows []string - debug bool + token string + clientID string + outputPath string + publicKeyPath string + newPublicKeysVarName string + trustedWorkflows []string + debug bool + oidcTokenExpirationTime int // OIDC token expiration time in minutes } var ( @@ -53,6 +56,7 @@ func NewRootCmd() *cobra.Command { rootCmd.PersistentFlags().StringVarP(&opts.clientID, "client-id", "c", "image-builder", "OIDC token client ID, this is used to verify the audience claim in the token. The value should be the same as the audience claim value in the token.") rootCmd.PersistentFlags().StringVarP(&opts.publicKeyPath, "public-key-path", "p", "", "Path to the cached public keys directory") rootCmd.PersistentFlags().BoolVarP(&opts.debug, "debug", "d", false, "Enable debug mode") + rootCmd.PersistentFlags().IntVarP(&opts.oidcTokenExpirationTime, "oidc-token-expiration-time", "e", 10, "OIDC token expiration time in minutes") return rootCmd } @@ -103,8 +107,10 @@ func isTokenProvided(logger Logger, opts *options) error { // It uses OIDC discovery to get the identity provider public keys. func (opts *options) extractClaims() error { var ( - zapLogger *zap.Logger - err error + zapLogger *zap.Logger + err error + tokenExpiredError *oidc.TokenExpiredError + token *tioidc.Token ) if opts.debug { zapLogger, err = zap.NewDevelopment() @@ -161,18 +167,35 @@ func (opts *options) extractClaims() error { verifier := provider.NewVerifier(logger, verifyConfig) logger.Infow("New verifier created") - // claims will store the extracted claim values from the token. - claims := tioidc.NewClaims(logger) - // Verifies the token and check if the claims have expected values. - // Verifies custom claim values too. - // Extract the claim values from the token into the claims struct. - // It provides a final result if the token is valid and the claims have expected values. - err = tokenProcessor.VerifyAndExtractClaims(ctx, &verifier, &claims) + // Verify the token + token, err = verifier.Verify(ctx, opts.token) + if errors.As(err, &tokenExpiredError) { + err = verifier.VerifyExtendedExpiration(err.(*oidc.TokenExpiredError).Expiry, opts.oidcTokenExpirationTime) + if err != nil { + return err + } + verifyConfig.SkipExpiryCheck = false + verifierWithoutExpiration := provider.NewVerifier(logger, verifyConfig) + token, err = verifierWithoutExpiration.Verify(ctx, opts.token) + } if err != nil { return err } logger.Infow("Token verified successfully") + // Create claims + claims := tioidc.NewClaims(logger) + logger.Infow("Verifying token claims") + + // Pass the token to ValidateClaims + err = tokenProcessor.ValidateClaims(&claims, token) + + if err != nil { + return err + } + logger.Infow("Token claims expectations verified successfully") + logger.Infow("All token checks passed successfully") + return nil } diff --git a/pkg/oidc/oidc.go b/pkg/oidc/oidc.go index c54227fca8c8..b105df761bab 100644 --- a/pkg/oidc/oidc.go +++ b/pkg/oidc/oidc.go @@ -7,6 +7,7 @@ package oidc import ( "errors" "fmt" + "time" "github.com/coreos/go-oidc/v3/oidc" "github.com/go-jose/go-jose/v4" @@ -64,7 +65,7 @@ type VerifierProvider interface { type ClaimsInterface interface { // Validate(jwt.Expected) error - ValidateExpectations(Issuer) error + validateExpectations(Issuer) error } type LoggerInterface interface { @@ -154,6 +155,13 @@ type TokenVerifier struct { Logger LoggerInterface } +func maskToken(token string) string { + if len(token) < 15 { + return "********" + } + return token[:2] + "********" + token[len(token)-2:] +} + // NewVerifierConfig creates a new VerifierConfig. // It verifies the clientID is not empty. func NewVerifierConfig(logger LoggerInterface, clientID string, options ...VerifierConfigOption) (VerifierConfig, error) { @@ -197,20 +205,39 @@ func NewVerifierConfig(logger LoggerInterface, clientID string, options ...Verif // Verify verifies the raw OIDC token. // It returns a Token struct which contains the verified token if successful. -func (tokenVerifier *TokenVerifier) Verify(ctx context.Context, rawToken string) (Token, error) { +func (tokenVerifier *TokenVerifier) Verify(ctx context.Context, rawToken string) (*Token, error) { logger := tokenVerifier.Logger logger.Debugw("Verifying token") - logger.Debugw("Got raw token value", "rawToken", rawToken) + logger.Debugw("Got raw token value", "rawToken", maskToken(rawToken)) idToken, err := tokenVerifier.Verifier.Verify(ctx, rawToken) if err != nil { token := Token{} - return token, fmt.Errorf("failed to verify token: %w", err) + return &token, fmt.Errorf("failed to verify token: %w", err) } logger.Debugw("Token verified successfully") token := Token{ Token: idToken, } - return token, nil + return &token, nil +} + +// VerifyExtendedExpiration checks the OIDC token expiration timestamp against the provided expiration time. +// It allows to accept tokens after the token original expiration time elapsed. +// The other aspects of the token must be verified separately with expiration check disabled. +func (tokenVerifier *TokenVerifier) VerifyExtendedExpiration(expirationTimestamp time.Time, gracePeriodMinutes int) error { + logger := tokenVerifier.Logger + logger.Debugw("Verifying token expiration time", "expirationTimestamp", expirationTimestamp, "gracePeriodMinutes", gracePeriodMinutes) + now := time.Now() + // check if expirationTimestamp is in the future + if expirationTimestamp.After(now) { + return fmt.Errorf("token expiration time is in the future: %v", expirationTimestamp) + } + elapsed := now.Sub(expirationTimestamp) + gracePeriod := time.Minute + if elapsed <= gracePeriod { + return nil + } + return fmt.Errorf("token expired more than %v ago", gracePeriod) } // Claims gets the claims from the token and unmarshal them into the provided claims struct. @@ -225,9 +252,9 @@ func NewClaims(logger LoggerInterface) Claims { } } -// ValidateExpectations validates the claims against the trusted issuer expected values. +// validateExpectations validates the claims against the trusted issuer expected values. // It checks audience, issuer, and job_workflow_ref claims. -func (claims *Claims) ValidateExpectations(issuer Issuer) error { +func (claims *Claims) validateExpectations(issuer Issuer) error { logger := claims.LoggerInterface logger.Debugw("Validating job_workflow_ref claim against expected value", "job_workflow_ref", claims.JobWorkflowRef, "expected", issuer.ExpectedJobWorkflowRef) if claims.JobWorkflowRef != issuer.ExpectedJobWorkflowRef { @@ -284,7 +311,7 @@ func NewTokenProcessor( tokenProcessor.logger = logger tokenProcessor.rawToken = rawToken - logger.Debugw("Added raw token to token processor", "rawToken", rawToken) + logger.Debugw("Added raw token to token processor", "rawToken", maskToken(rawToken)) tokenProcessor.verifierConfig = config logger.Debugw("Added Verifier config to token processor", @@ -375,25 +402,28 @@ func (tokenProcessor *TokenProcessor) Issuer() string { return tokenProcessor.issuer.IssuerURL } -// VerifyAndExtractClaims verify and parse the token to get the token claims. +// ValidateClaims verify and parse the token to get the token claims. // It uses the provided verifier to verify the token signature and expiration time. // It verifies if the token claims have expected values. // It unmarshal the claims into the provided claims struct. -func (tokenProcessor *TokenProcessor) VerifyAndExtractClaims(ctx context.Context, verifier TokenVerifierInterface, claims ClaimsInterface) error { +func (tokenProcessor *TokenProcessor) ValidateClaims(claims ClaimsInterface, token ClaimsReader) error { logger := tokenProcessor.logger - token, err := verifier.Verify(ctx, tokenProcessor.rawToken) - if err != nil { - return fmt.Errorf("failed to verify token: %w", err) + + // Ensure that the token is initialized + if token == nil { + return fmt.Errorf("token cannot be nil") } + logger.Debugw("Getting claims from token") - err = token.Claims(claims) + err := token.Claims(claims) if err != nil { return fmt.Errorf("failed to get claims from token: %w", err) } logger.Debugw("Got claims from token", "claims", fmt.Sprintf("%+v", claims)) - err = claims.ValidateExpectations(tokenProcessor.issuer) + + err = claims.validateExpectations(tokenProcessor.issuer) if err != nil { - return fmt.Errorf("failed to validate claims: %w", err) + return fmt.Errorf("expecations validation failed: %w", err) } return nil } diff --git a/pkg/oidc/oidc_test.go b/pkg/oidc/oidc_test.go index 643e4589fae8..48a91d111635 100644 --- a/pkg/oidc/oidc_test.go +++ b/pkg/oidc/oidc_test.go @@ -3,12 +3,13 @@ package oidc_test import ( "errors" "fmt" - - // "time" + "time" // "fmt" "os" + // "time" + "github.com/coreos/go-oidc/v3/oidc" "github.com/go-jose/go-jose/v4/jwt" tioidc "github.com/kyma-project/test-infra/pkg/oidc" @@ -22,8 +23,8 @@ import ( var _ = Describe("OIDC", func() { var ( - err error - ctx context.Context + err error + logger *zap.SugaredLogger trustedIssuers map[string]tioidc.Issuer rawToken []byte @@ -90,7 +91,6 @@ var _ = Describe("OIDC", func() { JWKSURL: "https://fakedings.dev-gcp.nais.io/fake/jwks", }, } - ctx = context.Background() }) When("issuer is trusted", func() { It("should return a new TokenProcessor", func() { @@ -158,7 +158,6 @@ var _ = Describe("OIDC", func() { Describe("TokenProcessor", func() { var ( - verifier *oidcmocks.MockTokenVerifierInterface Token oidcmocks.MockTokenInterface claims tioidc.Claims token tioidc.Token @@ -172,12 +171,6 @@ var _ = Describe("OIDC", func() { rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") Expect(err).NotTo(HaveOccurred()) - tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), verifierConfig) - Expect(err).NotTo(HaveOccurred()) - Expect(tokenProcessor).NotTo(BeNil()) - - ctx = context.Background() - trustedIssuers = map[string]tioidc.Issuer{ "https://fakedings.dev-gcp.nais.io/fake": { Name: "github", @@ -186,9 +179,13 @@ var _ = Describe("OIDC", func() { ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main", }, } + + tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), verifierConfig) + Expect(err).NotTo(HaveOccurred()) + Expect(tokenProcessor).NotTo(BeNil()) + token = tioidc.Token{} mockToken = oidcmocks.MockClaimsReader{} - verifier = &oidcmocks.MockTokenVerifierInterface{} }) Describe("Issuer", func() { It("should return the issuer", func() { @@ -196,7 +193,7 @@ var _ = Describe("OIDC", func() { Expect(issuer).To(Equal("https://fakedings.dev-gcp.nais.io/fake")) }) }) - Describe("VerifyAndExtractClaims", func() { + Describe("ValidateClaims", func() { BeforeEach(func() { claims = tioidc.NewClaims(logger) }) @@ -212,10 +209,9 @@ var _ = Describe("OIDC", func() { }, ).Return(nil) token.Token = &mockToken - verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(token, nil) // Run - err = tokenProcessor.VerifyAndExtractClaims(ctx, verifier, &claims) + err = tokenProcessor.ValidateClaims(&claims, &token) // Verify Expect(err).NotTo(HaveOccurred()) @@ -235,35 +231,23 @@ var _ = Describe("OIDC", func() { arg.JobWorkflowRef = "kyma-project/test-infra/.github/workflows/unexpected.yml@refs/heads/main" }, ).Return(nil) + expectedError := fmt.Errorf("expecations validation failed: %w", fmt.Errorf("job_workflow_ref claim expected value validation failed, expected: kyma-project/test-infra/.github/workflows/unexpected.yml@refs/heads/main, provided: kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main")) token.Token = &mockToken - verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(token, nil) - - // Run - err = tokenProcessor.VerifyAndExtractClaims(ctx, verifier, &claims) - - // Verify - Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("failed to validate claims: job_workflow_ref claim expected value validation failed, expected: kyma-project/test-infra/.github/workflows/unexpected.yml@refs/heads/main, provided: kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main")) - }) - It("should return an error when token was not verified", func() { - verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(token, fmt.Errorf("token validation failed")) // Run - err = tokenProcessor.VerifyAndExtractClaims(ctx, verifier, &claims) + err = tokenProcessor.ValidateClaims(&claims, &token) // Verify Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("failed to verify token: token validation failed")) - Expect(claims).To(Equal(tioidc.NewClaims(logger))) + Expect(err).To(MatchError(expectedError)) }) It("should return an error when claims are not set", func() { mockToken.On("Claims", &claims).Return(fmt.Errorf("claims are not set")) token.Token = &mockToken - verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(token, nil) Token.On("Claims", &claims).Return(fmt.Errorf("claims are not set")) // Run - err = tokenProcessor.VerifyAndExtractClaims(ctx, verifier, &claims) + err = tokenProcessor.ValidateClaims(&claims, &token) // Verify Expect(err).To(HaveOccurred()) @@ -278,7 +262,7 @@ var _ = Describe("OIDC", func() { tokenVerifier tioidc.TokenVerifier verifier *oidcmocks.MockVerifier ctx context.Context - token tioidc.Token + token *tioidc.Token ) BeforeEach(func() { verifier = &oidcmocks.MockVerifier{} @@ -295,7 +279,45 @@ var _ = Describe("OIDC", func() { verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(&oidc.IDToken{}, nil) token, err = tokenVerifier.Verify(ctx, string(rawToken)) Expect(err).NotTo(HaveOccurred()) - Expect(token).To(BeAssignableToTypeOf(tioidc.Token{})) + Expect(token).To(BeAssignableToTypeOf(&tioidc.Token{})) + }) + + It("should return an error when the token is invalid", func() { + verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(&oidc.IDToken{}, errors.New("invalid token")) + token, err = tokenVerifier.Verify(ctx, string(rawToken)) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError("failed to verify token: invalid token")) + Expect(token).To(Equal(&tioidc.Token{})) + }) + }) + Describe("VerifyExtendedExpiration", func() { + var ( + expirationTimestamp time.Time + gracePeriodMinutes int + ) + + BeforeEach(func() { + gracePeriodMinutes = 1 + }) + + It("should return no error when the token is within the grace period", func() { + expirationTimestamp = time.Now().Add(-30 * time.Second) + err := tokenVerifier.VerifyExtendedExpiration(expirationTimestamp, gracePeriodMinutes) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should return an error when the token is expired beyond the grace period", func() { + expirationTimestamp = time.Now().Add(-2 * time.Minute) + err := tokenVerifier.VerifyExtendedExpiration(expirationTimestamp, gracePeriodMinutes) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError("token expired more than 1m0s ago")) + }) + + It("should return an error when the token expiration timestamp is in the future", func() { + expirationTimestamp = time.Now().Add(1 * time.Minute) + err := tokenVerifier.VerifyExtendedExpiration(expirationTimestamp, gracePeriodMinutes) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(fmt.Sprintf("token expiration time is in the future: %v", expirationTimestamp))) }) }) }) @@ -311,7 +333,6 @@ var _ = Describe("OIDC", func() { provider = tioidc.Provider{ VerifierProvider: oidcProvider, } - ctx = context.Background() verifierConfig, err = tioidc.NewVerifierConfig(logger, clientID) Expect(err).NotTo(HaveOccurred()) })