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("********")) + }) + }) + +})