From 65b056bb8b0704924bff2b94292f127aec66c3cc Mon Sep 17 00:00:00 2001 From: dekiel Date: Sun, 27 Oct 2024 20:02:44 +0100 Subject: [PATCH] Skip expiry check and run expiration check with 10 minutes expiration time by default for all tokens. Added options to set expiry check and expiration time values. Verify method runs own expiration check if upstream expiry check is skipped. Added expiration check verification. Applying options only if provided. This allows better logging. --- cmd/oidc-token-verifier/main.go | 25 ++--- pkg/oidc/oidc.go | 115 ++++++++++++++++------ pkg/oidc/oidc_test.go | 164 +++++++++++++++++++++++++++----- pkg/oidc/oidc_unit_test.go | 37 +++++++ 4 files changed, 271 insertions(+), 70 deletions(-) create mode 100644 pkg/oidc/oidc_unit_test.go diff --git a/cmd/oidc-token-verifier/main.go b/cmd/oidc-token-verifier/main.go index 09eb21530592..9f8c9a80917b 100644 --- a/cmd/oidc-token-verifier/main.go +++ b/cmd/oidc-token-verifier/main.go @@ -1,11 +1,9 @@ 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" @@ -102,10 +100,9 @@ func isTokenProvided(logger Logger, opts *options) error { // It uses OIDC discovery to get the identity provider public keys. func (opts *options) verifyToken() error { var ( - zapLogger *zap.Logger - err error - tokenExpiredError *oidc.TokenExpiredError - token *tioidc.Token + zapLogger *zap.Logger + err error + token *tioidc.Token ) if opts.debug { zapLogger, err = zap.NewDevelopment() @@ -128,7 +125,7 @@ func (opts *options) verifyToken() error { // Create a new verifier config that will be used to verify the token. // The clientID is used to verify the audience claim in the token. - verifyConfig, err := tioidc.NewVerifierConfig(logger, opts.clientID) + verifyConfig, err := tioidc.NewVerifierConfig(logger, opts.clientID, tioidc.SkipExpiryCheck()) if err != nil { return err } @@ -156,20 +153,14 @@ func (opts *options) verifyToken() error { // Create a new verifier using the provider and the verifier config. // The verifier is used to verify the token signature, expiration time and execute standard OIDC validation. - verifier := provider.NewVerifier(logger, verifyConfig) + verifier, err := provider.NewVerifier(logger, verifyConfig, tioidc.WithExtendedExpiration(opts.oidcTokenExpirationTime)) + if err != nil { + return err + } logger.Infow("New verifier created") // 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 } diff --git a/pkg/oidc/oidc.go b/pkg/oidc/oidc.go index 46f01599d847..409045e1e230 100644 --- a/pkg/oidc/oidc.go +++ b/pkg/oidc/oidc.go @@ -141,6 +141,16 @@ type GithubClaims struct { // VerifierConfigOption is a function that modifies the VerifierConfig. type VerifierConfigOption func(*VerifierConfig) error +// SkipExpiryCheck set verifier config to skip the token expiration check. +// It's used to allow longer token expiration time. +// The Verifier must run its own expiration check for extended expiration time. +func SkipExpiryCheck() VerifierConfigOption { + return func(config *VerifierConfig) error { + config.SkipExpiryCheck = true + return nil + } +} + // Provider is the OIDC provider. // It abstracts the provider implementation. // VerifierProvider provides the OIDC token verifier. @@ -152,16 +162,31 @@ type Provider struct { // Token is the OIDC token. // It abstracts the IDToken implementation. type Token struct { - Token ClaimsReader + Token ClaimsReader + IssuedAt time.Time } // TokenVerifier is the OIDC token verifier. // It abstracts the Verifier implementation. type TokenVerifier struct { - Verifier Verifier - Logger LoggerInterface + Config VerifierConfig + ExpirationTimeMinutes int + Verifier Verifier + Logger LoggerInterface +} + +// TokenVerifierOption is a function that modifies the TokenVerifier. +type TokenVerifierOption func(verifier *TokenVerifier) error + +// WithExtendedExpiration sets the custom expiration time used by the TokenVerifier. +func WithExtendedExpiration(expirationTimeMinutes int) TokenVerifierOption { + return func(verifier *TokenVerifier) error { + verifier.ExpirationTimeMinutes = expirationTimeMinutes + return nil + } } +// maskToken masks the token value. It's used for debug logging purposes. func maskToken(token string) string { if len(token) < 15 { return "********" @@ -212,42 +237,57 @@ 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. +// Verify allow checking extended expiration time for the token. +// It runs the token expiration check if the upstream verifier is configured to skip it. 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", maskToken(rawToken)) + logger.Debugw("verifying token", "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 nil, fmt.Errorf("failed to verify token: %w", err) } - logger.Debugw("Token verified successfully") + logger.Debugw("upstream verifier checks finished") token := Token{ - Token: idToken, + Token: idToken, + IssuedAt: idToken.IssuedAt, + } + logger.Debugw("checking if upstream verifier is configured to skip token expiration check", "SkipExpiryCheck", tokenVerifier.Config.SkipExpiryCheck) + if tokenVerifier.Config.SkipExpiryCheck { + logger.Debugw("upstream verifier configured to skip token expiration check, running our own check", "expirationTimeMinutes", tokenVerifier.ExpirationTimeMinutes, "tokenIssuedAt", token.IssuedAt) + err = token.IsTokenExpired(logger, tokenVerifier.ExpirationTimeMinutes) + logger.Debugw("finished token expiration check") } + if err != nil { + return nil, fmt.Errorf("failed to verify token: %w", err) + } + logger.Debugw("token verified successfully") 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) +// IsTokenExpired checks the OIDC token expiration timestamp against the provided expiration time. +// It allows accepting tokens after the token original expiration time elapsed. +// The other aspects of the token must be verified separately with the expiration check disabled. +func (token *Token) IsTokenExpired(logger LoggerInterface, expirationTimeMinutes int) error { + logger.Debugw("verifying token expiration time", "tokenIssuedAt", token.IssuedAt, "expirationTimeMinutes", expirationTimeMinutes) 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) + if token.IssuedAt.After(now) { + return fmt.Errorf("token issued in the future, tokenIssuedAt: %v, now: %v", token.IssuedAt, now) } - elapsed := now.Sub(expirationTimestamp) - gracePeriod := time.Minute - if elapsed <= gracePeriod { + logger.Debugw("token issued in the past") + expirationTime := token.IssuedAt.Add(time.Minute * time.Duration(expirationTimeMinutes)) + logger.Debugw("computed expiration time", "expirationTime", expirationTime) + if expirationTime.After(now) || expirationTime.Equal(now) { + logger.Debugw("token not expired") return nil } - return fmt.Errorf("token expired more than %v ago", gracePeriod) + return fmt.Errorf("token expired, tokenIssuedAt: %v, expired at %v", token.IssuedAt, expirationTime) } // Claims gets the claims from the token and unmarshal them into the provided claims struct. +// TODO: Should we have a tests for this method? +// +// We can test for returned error. +// We can test the claims struct is populated with the expected values. func (token *Token) Claims(claims interface{}) error { return token.Token.Claims(claims) } @@ -291,15 +331,26 @@ func NewProviderFromDiscovery(ctx context.Context, logger LoggerInterface, issue // NewVerifier creates a new TokenVerifier for provider. // It returns a TokenVerifier struct containing the new Verifier if successful. -func (provider *Provider) NewVerifier(logger LoggerInterface, verifierConfig VerifierConfig) TokenVerifier { +func (provider *Provider) NewVerifier(logger LoggerInterface, verifierConfig VerifierConfig, options ...TokenVerifierOption) (TokenVerifier, error) { logger.Debugw("Creating new verifier with config", "config", fmt.Sprintf("%+v", verifierConfig)) verifier := provider.VerifierProvider.Verifier(&verifierConfig.Config) tokenVerifier := TokenVerifier{ + Config: verifierConfig, Verifier: verifier, Logger: logger, } + if len(options) > 0 { + logger.Debugw("Applying TokenVerifierOptions") + for _, option := range options { + err := option(&tokenVerifier) + if err != nil { + return TokenVerifier{}, fmt.Errorf("failed to apply TokenVerifierOption: %w", err) + } + } + logger.Debugw("Applied all TokenVerifierOptions") + } logger.Debugw("Created new verifier") - return tokenVerifier + return tokenVerifier, nil } // NewTokenProcessor creates a new TokenProcessor for trusted issuers. @@ -346,14 +397,16 @@ func NewTokenProcessor( tokenProcessor.issuer = trustedIssuer logger.Debugw("Added trusted issuer to TokenProcessor", "issuer", tokenProcessor.issuer) - logger.Debugw("Applying TokenProcessorOptions") - for _, option := range options { - err := option(&tokenProcessor) - if err != nil { - return TokenProcessor{}, fmt.Errorf("failed to apply TokenProcessorOption: %w", err) + if len(options) > 0 { + logger.Debugw("Applying TokenProcessorOptions") + for _, option := range options { + err := option(&tokenProcessor) + if err != nil { + return TokenProcessor{}, fmt.Errorf("failed to apply TokenProcessorOption: %w", err) + } } + logger.Debugw("Applied all TokenProcessorOptions") } - logger.Debugw("Applied all TokenProcessorOptions") logger.Debugw("Created token processor", "issuer", tokenProcessor.issuer) return tokenProcessor, nil @@ -403,8 +456,8 @@ func (tokenProcessor *TokenProcessor) isTrustedIssuer(issuer string, trustedIssu return Issuer{}, fmt.Errorf("issuer %s is not trusted", issuer) } -// TODO(dekiel): This should return Issuer struct or has a different name. // Issuer returns the issuer of the token. +// TODO(dekiel): This should return Issuer struct or has a different name. func (tokenProcessor *TokenProcessor) Issuer() string { return tokenProcessor.issuer.IssuerURL } diff --git a/pkg/oidc/oidc_test.go b/pkg/oidc/oidc_test.go index 48a91d111635..70f4d967308d 100644 --- a/pkg/oidc/oidc_test.go +++ b/pkg/oidc/oidc_test.go @@ -41,6 +41,16 @@ var _ = Describe("OIDC", func() { clientID = "testClientID" }) + Describe("SkipExpiryCheck", func() { + It("should set SkipExpiryCheck to true", func() { + config := &tioidc.VerifierConfig{} + option := tioidc.SkipExpiryCheck() + err := option(config) + Expect(err).NotTo(HaveOccurred()) + Expect(config.SkipExpiryCheck).To(BeTrue()) + }) + }) + Describe("NewVerifierConfig", func() { var ( verifierConfigOption tioidc.VerifierConfigOption @@ -270,54 +280,163 @@ var _ = Describe("OIDC", func() { Verifier: verifier, Logger: logger, } + verifierConfig, err = tioidc.NewVerifierConfig(logger, clientID) + Expect(err).NotTo(HaveOccurred()) ctx = context.Background() rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") Expect(err).NotTo(HaveOccurred()) }) + + Describe("WithExtendedExpiration option", func() { + + It("should set expiration time", func() { + expirationTime := 1 + option := tioidc.WithExtendedExpiration(expirationTime) + err := option(&tokenVerifier) + Expect(err).NotTo(HaveOccurred()) + Expect(tokenVerifier.ExpirationTimeMinutes).To(Equal(expirationTime)) + }) + }) + Describe("Verify", func() { - It("should return Token when the token is valid", func() { - verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(&oidc.IDToken{}, nil) + It("should return Token when the token is valid with standard expiration time", func() { + // prepare + issuedAt := time.Now().Add(-4 * time.Minute) + idToken := &oidc.IDToken{IssuedAt: issuedAt} + tokenVerifier.Config.SkipExpiryCheck = false + tokenVerifier.ExpirationTimeMinutes = 10 + verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(idToken, nil) + + // execute token, err = tokenVerifier.Verify(ctx, string(rawToken)) + + // verify + Expect(err).NotTo(HaveOccurred()) + Expect(token).To(Equal(&tioidc.Token{Token: idToken, IssuedAt: issuedAt})) + }) + + It("should return Token when the token is valid with extended expiration time", func() { + // prepare + issuedAt := time.Now().Add(-9 * time.Minute) + idToken := &oidc.IDToken{IssuedAt: issuedAt} + verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(idToken, nil) + tokenVerifier.Config.SkipExpiryCheck = true + tokenVerifier.ExpirationTimeMinutes = 10 + + // execute + token, err = tokenVerifier.Verify(ctx, string(rawToken)) + + // verify Expect(err).NotTo(HaveOccurred()) - Expect(token).To(BeAssignableToTypeOf(&tioidc.Token{})) + Expect(token).To(Equal(&tioidc.Token{Token: idToken, IssuedAt: issuedAt})) }) - 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")) + It("should return an error when the token is invalid with standard expiration time", func() { + // prepare + verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(nil, errors.New("invalid token")) + tokenVerifier.Config.SkipExpiryCheck = false + tokenVerifier.ExpirationTimeMinutes = 10 + + // execute token, err = tokenVerifier.Verify(ctx, string(rawToken)) + + // verify Expect(err).To(HaveOccurred()) Expect(err).To(MatchError("failed to verify token: invalid token")) - Expect(token).To(Equal(&tioidc.Token{})) + Expect(token).To(BeNil()) + }) + + It("should return an error when the token is invalid with extended expiration time", func() { + // prepare + verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(nil, errors.New("invalid token")) + tokenVerifier.Config.SkipExpiryCheck = true + tokenVerifier.ExpirationTimeMinutes = 10 + + // execute + token, err = tokenVerifier.Verify(ctx, string(rawToken)) + + // verify + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError("failed to verify token: invalid token")) + Expect(token).To(BeNil()) + }) + + It("should return an error when token expired with standard expiration check", func() { + // prepare + // issuedAt := time.Now().Add(-6 * time.Minute) + // idToken := &oidc.IDToken{IssuedAt: issuedAt} + verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(nil, errors.New("token expired")) + tokenVerifier.Config.SkipExpiryCheck = false + tokenVerifier.ExpirationTimeMinutes = 10 + + // execute + token, err = tokenVerifier.Verify(ctx, string(rawToken)) + + // verify + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError("failed to verify token: token expired")) + Expect(token).To(BeNil()) + }) + + It("should return an error when token expired with extended expiration check", func() { + // prepare + issuedAt := time.Now().Add(-11 * time.Minute) + idToken := &oidc.IDToken{IssuedAt: issuedAt} + verifier.On("Verify", mock.AnythingOfType("backgroundCtx"), string(rawToken)).Return(idToken, nil) + tokenVerifier.Config.SkipExpiryCheck = true + tokenVerifier.ExpirationTimeMinutes = 10 + + // execute + token, err = tokenVerifier.Verify(ctx, string(rawToken)) + + // verify + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(fmt.Errorf("failed to verify token: %w", + fmt.Errorf("token expired, tokenIssuedAt: %v, expired at %v", issuedAt, issuedAt.Add(time.Minute*time.Duration(tokenVerifier.ExpirationTimeMinutes))), + ))) + Expect(token).To(BeNil()) }) }) - Describe("VerifyExtendedExpiration", func() { + }) + + Describe("Token", func() { + + Describe("IsTokenExpired", func() { var ( - expirationTimestamp time.Time - gracePeriodMinutes int + now time.Time + expirationTimeMinutes int ) BeforeEach(func() { - gracePeriodMinutes = 1 + now = time.Now() + expirationTimeMinutes = 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) + It("should return no error when the token is within the expiration time", func() { + token := tioidc.Token{ + IssuedAt: now, + } + err := token.IsTokenExpired(logger, expirationTimeMinutes) 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) + It("should return an error when the token is expired", func() { + token := tioidc.Token{ + IssuedAt: now.Add(-2 * time.Minute), // 2 minutes ago + } + extendedExpiration := token.IssuedAt.Add(time.Minute * time.Duration(expirationTimeMinutes)) + err := token.IsTokenExpired(logger, expirationTimeMinutes) Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("token expired more than 1m0s ago")) + Expect(err).To(MatchError(fmt.Errorf("token expired, tokenIssuedAt: %v, expired at %v", token.IssuedAt, extendedExpiration))) }) - 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) + It("should return an error when the token issued in the future", func() { + token := tioidc.Token{ + IssuedAt: now.Add(2 * time.Minute), // 2 minutes ago + } + err := token.IsTokenExpired(logger, expirationTimeMinutes) Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError(fmt.Sprintf("token expiration time is in the future: %v", expirationTimestamp))) + Expect(err).To(MatchError(MatchRegexp("token issued in the future, tokenIssuedAt: .+, now: .+"))) }) }) }) @@ -339,7 +458,8 @@ var _ = Describe("OIDC", func() { Describe("NewVerifier", func() { It("should return a new TokenVerifier", func() { oidcProvider.On("Verifier", &verifierConfig.Config).Return(&oidc.IDTokenVerifier{}) - verifier := provider.NewVerifier(logger, verifierConfig) + verifier, err := provider.NewVerifier(logger, verifierConfig) + Expect(err).NotTo(HaveOccurred()) Expect(verifier).NotTo(BeNil()) Expect(verifier).To(BeAssignableToTypeOf(tioidc.TokenVerifier{})) }) diff --git a/pkg/oidc/oidc_unit_test.go b/pkg/oidc/oidc_unit_test.go new file mode 100644 index 000000000000..a4b996cc4fca --- /dev/null +++ b/pkg/oidc/oidc_unit_test.go @@ -0,0 +1,37 @@ +package oidc + +// oidc_unit_test.go contains tests which require access to non-exported functions and variables. + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("OIDC", func() { + Describe("maskToken", func() { + It("should mask the token if length is less than 15", func() { + token := "shorttoken" + maskedToken := maskToken(token) + Expect(maskedToken).To(Equal("********")) + }) + + It("should mask the token if length is exactly 15", func() { + token := "123456789012345" + maskedToken := maskToken(token) + Expect(maskedToken).To(Equal("12********45")) + }) + + It("should mask the token if length is greater than 15", func() { + token := "12345678901234567890" + maskedToken := maskToken(token) + Expect(maskedToken).To(Equal("12********90")) + }) + + It("should mask the token if it's empty", func() { + token := "" + maskedToken := maskToken(token) + Expect(maskedToken).To(Equal("********")) + }) + }) + +})