From 90ad4ee00d35b8bfbfe92cd8691862852cd51765 Mon Sep 17 00:00:00 2001 From: dekiel Date: Thu, 9 Jan 2025 14:19:47 +0100 Subject: [PATCH 1/6] Creating a verifier config with client id read from trusted issuer. This allow creating a config with different expected client id values, based on detected issuer in provided oidc token. Each client specific for oidc provider and application must use uniq audience value. --- cmd/oidc-token-verifier/main.go | 40 +++-- pkg/oidc/mocks/mock_ClaimsInterface.go | 74 ++------- pkg/oidc/mocks/mock_ClaimsReader.go | 2 +- pkg/oidc/mocks/mock_ProviderInterface.go | 2 +- pkg/oidc/mocks/mock_TokenInterface.go | 2 +- pkg/oidc/mocks/mock_TokenProcessorOption.go | 2 +- pkg/oidc/mocks/mock_TokenVerifierInterface.go | 6 +- pkg/oidc/mocks/mock_Verifier.go | 6 +- pkg/oidc/mocks/mock_VerifierConfigOption.go | 2 +- pkg/oidc/mocks/mock_VerifierProvider.go | 2 +- pkg/oidc/mocks/mock_loggerInterface.go | 2 +- pkg/oidc/oidc.go | 140 ++++++++++-------- pkg/oidc/oidc_test.go | 123 +++++++-------- pkg/oidc/oidc_unit_test.go | 42 +++++- 14 files changed, 231 insertions(+), 214 deletions(-) diff --git a/cmd/oidc-token-verifier/main.go b/cmd/oidc-token-verifier/main.go index 9f8c9a80917b..c1f750f919f5 100644 --- a/cmd/oidc-token-verifier/main.go +++ b/cmd/oidc-token-verifier/main.go @@ -21,7 +21,6 @@ type Logger interface { type options struct { token string - clientID string trustedWorkflows []string debug bool oidcTokenExpirationTime int // OIDC token expiration time in minutes @@ -47,7 +46,6 @@ func NewRootCmd() *cobra.Command { // if err != nil { // panic(err) // } - 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().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 @@ -119,29 +117,32 @@ func (opts *options) verifyToken() error { return err } - // Print used options values. - logger.Infow("Using the following trusted workflows", "trusted-workflows", opts.trustedWorkflows) - logger.Infow("Using the following client ID", "client-id", opts.clientID) - - // 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, tioidc.SkipExpiryCheck()) - if err != nil { - return err - } - logger.Infow("Verifier config created", "config", verifyConfig) - // Create a new token processor // It reads issuer from the token and verifies if the issuer is trusted. // The tokenProcessor is a main object that is used to verify the token and extract the claim values. // TODO(dekiel): add support for providing trusted issuers instead of using the value from the package. - tokenProcessor, err := tioidc.NewTokenProcessor(logger, tioidc.TrustedOIDCIssuers, opts.token, verifyConfig) + tokenProcessor, err := tioidc.NewTokenProcessor(logger, tioidc.TrustedOIDCIssuers, opts.token) if err != nil { return err } + logger.Infow("Token processor created for trusted issuer", "issuer", tokenProcessor.Issuer()) + + // TODO (dekiel): implement writing output data to the file. This will give us separated clear output for a data and logs. fmt.Printf("GITHUB_URL=%s\n", tokenProcessor.GetIssuer().GetGithubURL()) + // Create a new verifier config that will be used to verify the token. + // The standard expiration check is skipped. + // We use custom expiration time check to allow longer token expiration time than the value in the token. + // The extended expiration time is needed due to Azure DevOps delays in starting the pipeline. + // The delay was causing the token to expire before the pipeline started. + verifierConfig, err := tokenProcessor.NewVerifierConfig(tioidc.SkipExpiryCheck()) + if err != nil { + return err + } + + logger.Infow("Verifier config created") + ctx := context.Background() // Create a new provider using OIDC discovery to get the public keys. // It uses the issuer from the token to get the OIDC discovery endpoint. @@ -153,10 +154,15 @@ 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, err := provider.NewVerifier(logger, verifyConfig, tioidc.WithExtendedExpiration(opts.oidcTokenExpirationTime)) + // TODO (dekiel): Consider using verifier config as the only way to parametrize the verification process. + // The WithExtendedExpiration could be moved to the verifier config. + // The WithExtendedExpiration could disable the standard expiration check. + // This would allow to have a single place to configure the verification process. + verifier, err := provider.NewVerifier(logger, verifierConfig, tioidc.WithExtendedExpiration(opts.oidcTokenExpirationTime)) if err != nil { return err } + logger.Infow("New verifier created") // Verify the token @@ -164,10 +170,12 @@ func (opts *options) verifyToken() error { 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 diff --git a/pkg/oidc/mocks/mock_ClaimsInterface.go b/pkg/oidc/mocks/mock_ClaimsInterface.go index 353b3ad7acdc..119ecc4c05a7 100644 --- a/pkg/oidc/mocks/mock_ClaimsInterface.go +++ b/pkg/oidc/mocks/mock_ClaimsInterface.go @@ -1,12 +1,10 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks import ( - jwt "github.com/go-jose/go-jose/v4/jwt" - mock "github.com/stretchr/testify/mock" - oidc "github.com/kyma-project/test-infra/pkg/oidc" + mock "github.com/stretchr/testify/mock" ) // MockClaimsInterface is an autogenerated mock type for the ClaimsInterface type @@ -22,58 +20,12 @@ func (_m *MockClaimsInterface) EXPECT() *MockClaimsInterface_Expecter { return &MockClaimsInterface_Expecter{mock: &_m.Mock} } -// Validate provides a mock function with given fields: _a0 -func (_m *MockClaimsInterface) Validate(_a0 jwt.Expected) error { - ret := _m.Called(_a0) - - if len(ret) == 0 { - panic("no return value specified for Validate") - } - - var r0 error - if rf, ok := ret.Get(0).(func(jwt.Expected) error); ok { - r0 = rf(_a0) - } else { - r0 = ret.Error(0) - } - - return r0 -} - -// MockClaimsInterface_Validate_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Validate' -type MockClaimsInterface_Validate_Call struct { - *mock.Call -} - -// Validate is a helper method to define mock.On call -// - _a0 jwt.Expected -func (_e *MockClaimsInterface_Expecter) Validate(_a0 interface{}) *MockClaimsInterface_Validate_Call { - return &MockClaimsInterface_Validate_Call{Call: _e.mock.On("Validate", _a0)} -} - -func (_c *MockClaimsInterface_Validate_Call) Run(run func(_a0 jwt.Expected)) *MockClaimsInterface_Validate_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(jwt.Expected)) - }) - return _c -} - -func (_c *MockClaimsInterface_Validate_Call) Return(_a0 error) *MockClaimsInterface_Validate_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MockClaimsInterface_Validate_Call) RunAndReturn(run func(jwt.Expected) error) *MockClaimsInterface_Validate_Call { - _c.Call.Return(run) - return _c -} - -// ValidateExpectations provides a mock function with given fields: _a0 -func (_m *MockClaimsInterface) ValidateExpectations(_a0 oidc.Issuer) error { +// validateExpectations provides a mock function with given fields: _a0 +func (_m *MockClaimsInterface) validateExpectations(_a0 oidc.Issuer) error { ret := _m.Called(_a0) if len(ret) == 0 { - panic("no return value specified for ValidateExpectations") + panic("no return value specified for validateExpectations") } var r0 error @@ -86,30 +38,30 @@ func (_m *MockClaimsInterface) ValidateExpectations(_a0 oidc.Issuer) error { return r0 } -// MockClaimsInterface_ValidateExpectations_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ValidateExpectations' -type MockClaimsInterface_ValidateExpectations_Call struct { +// MockClaimsInterface_validateExpectations_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'validateExpectations' +type MockClaimsInterface_validateExpectations_Call struct { *mock.Call } -// ValidateExpectations is a helper method to define mock.On call +// validateExpectations is a helper method to define mock.On call // - _a0 oidc.Issuer -func (_e *MockClaimsInterface_Expecter) ValidateExpectations(_a0 interface{}) *MockClaimsInterface_ValidateExpectations_Call { - return &MockClaimsInterface_ValidateExpectations_Call{Call: _e.mock.On("ValidateExpectations", _a0)} +func (_e *MockClaimsInterface_Expecter) validateExpectations(_a0 interface{}) *MockClaimsInterface_validateExpectations_Call { + return &MockClaimsInterface_validateExpectations_Call{Call: _e.mock.On("validateExpectations", _a0)} } -func (_c *MockClaimsInterface_ValidateExpectations_Call) Run(run func(_a0 oidc.Issuer)) *MockClaimsInterface_ValidateExpectations_Call { +func (_c *MockClaimsInterface_validateExpectations_Call) Run(run func(_a0 oidc.Issuer)) *MockClaimsInterface_validateExpectations_Call { _c.Call.Run(func(args mock.Arguments) { run(args[0].(oidc.Issuer)) }) return _c } -func (_c *MockClaimsInterface_ValidateExpectations_Call) Return(_a0 error) *MockClaimsInterface_ValidateExpectations_Call { +func (_c *MockClaimsInterface_validateExpectations_Call) Return(_a0 error) *MockClaimsInterface_validateExpectations_Call { _c.Call.Return(_a0) return _c } -func (_c *MockClaimsInterface_ValidateExpectations_Call) RunAndReturn(run func(oidc.Issuer) error) *MockClaimsInterface_ValidateExpectations_Call { +func (_c *MockClaimsInterface_validateExpectations_Call) RunAndReturn(run func(oidc.Issuer) error) *MockClaimsInterface_validateExpectations_Call { _c.Call.Return(run) return _c } diff --git a/pkg/oidc/mocks/mock_ClaimsReader.go b/pkg/oidc/mocks/mock_ClaimsReader.go index 489be1efe73a..c9b948613f2b 100644 --- a/pkg/oidc/mocks/mock_ClaimsReader.go +++ b/pkg/oidc/mocks/mock_ClaimsReader.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks diff --git a/pkg/oidc/mocks/mock_ProviderInterface.go b/pkg/oidc/mocks/mock_ProviderInterface.go index 61b4618393b1..9e951ee701d3 100644 --- a/pkg/oidc/mocks/mock_ProviderInterface.go +++ b/pkg/oidc/mocks/mock_ProviderInterface.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks diff --git a/pkg/oidc/mocks/mock_TokenInterface.go b/pkg/oidc/mocks/mock_TokenInterface.go index 9ff95bbaa040..f0239d9e6077 100644 --- a/pkg/oidc/mocks/mock_TokenInterface.go +++ b/pkg/oidc/mocks/mock_TokenInterface.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks diff --git a/pkg/oidc/mocks/mock_TokenProcessorOption.go b/pkg/oidc/mocks/mock_TokenProcessorOption.go index 284373ec55f6..821b292ee325 100644 --- a/pkg/oidc/mocks/mock_TokenProcessorOption.go +++ b/pkg/oidc/mocks/mock_TokenProcessorOption.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks diff --git a/pkg/oidc/mocks/mock_TokenVerifierInterface.go b/pkg/oidc/mocks/mock_TokenVerifierInterface.go index 325fe88383f2..5694cb56ddde 100644 --- a/pkg/oidc/mocks/mock_TokenVerifierInterface.go +++ b/pkg/oidc/mocks/mock_TokenVerifierInterface.go @@ -1,12 +1,12 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks import ( - context "context" + mock "github.com/stretchr/testify/mock" + context "golang.org/x/net/context" oidc "github.com/kyma-project/test-infra/pkg/oidc" - mock "github.com/stretchr/testify/mock" ) // MockTokenVerifierInterface is an autogenerated mock type for the TokenVerifierInterface type diff --git a/pkg/oidc/mocks/mock_Verifier.go b/pkg/oidc/mocks/mock_Verifier.go index b0e509035ba8..b9a690d4612d 100644 --- a/pkg/oidc/mocks/mock_Verifier.go +++ b/pkg/oidc/mocks/mock_Verifier.go @@ -1,12 +1,12 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks import ( - context "context" + mock "github.com/stretchr/testify/mock" + context "golang.org/x/net/context" oidc "github.com/coreos/go-oidc/v3/oidc" - mock "github.com/stretchr/testify/mock" ) // MockVerifier is an autogenerated mock type for the Verifier type diff --git a/pkg/oidc/mocks/mock_VerifierConfigOption.go b/pkg/oidc/mocks/mock_VerifierConfigOption.go index 1cec5a37d1be..76b45dabff0a 100644 --- a/pkg/oidc/mocks/mock_VerifierConfigOption.go +++ b/pkg/oidc/mocks/mock_VerifierConfigOption.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks diff --git a/pkg/oidc/mocks/mock_VerifierProvider.go b/pkg/oidc/mocks/mock_VerifierProvider.go index 799850cf0f2b..223203525742 100644 --- a/pkg/oidc/mocks/mock_VerifierProvider.go +++ b/pkg/oidc/mocks/mock_VerifierProvider.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks diff --git a/pkg/oidc/mocks/mock_loggerInterface.go b/pkg/oidc/mocks/mock_loggerInterface.go index 192ad1097b88..750b2b409bbc 100644 --- a/pkg/oidc/mocks/mock_loggerInterface.go +++ b/pkg/oidc/mocks/mock_loggerInterface.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.38.0. DO NOT EDIT. +// Code generated by mockery v2.46.3. DO NOT EDIT. package oidcmocks diff --git a/pkg/oidc/oidc.go b/pkg/oidc/oidc.go index 409045e1e230..514f0c8b6461 100644 --- a/pkg/oidc/oidc.go +++ b/pkg/oidc/oidc.go @@ -26,6 +26,7 @@ var ( JWKSURL: "https://token.actions.githubusercontent.com/.well-known/jwks", ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/image-builder.yml@refs/heads/main", GithubURL: "https://github.com", + ClientID: "image-builder", } GithubToolsSAPOIDCIssuer = Issuer{ Name: "github-tools-sap", @@ -33,8 +34,12 @@ var ( JWKSURL: "https://github.tools.sap/_services/token/.well-known/jwks", ExpectedJobWorkflowRef: "kyma/oci-image-builder/.github/workflows/image-builder.yml@refs/heads/main", GithubURL: "https://github.tools.sap", + ClientID: "image-builder", + } + TrustedOIDCIssuers = map[string]Issuer{ + GithubOIDCIssuer.IssuerURL: GithubOIDCIssuer, + GithubToolsSAPOIDCIssuer.IssuerURL: GithubToolsSAPOIDCIssuer, } - TrustedOIDCIssuers = map[string]Issuer{GithubOIDCIssuer.IssuerURL: GithubOIDCIssuer, GithubToolsSAPOIDCIssuer.IssuerURL: GithubToolsSAPOIDCIssuer} ) // TODO(dekiel) interfaces need to be clenup up to remove redundancy. @@ -83,6 +88,8 @@ type Issuer struct { JWKSURL string `json:"jwks_url" yaml:"jwks_url"` ExpectedJobWorkflowRef string `json:"expected_job_workflow_ref" yaml:"expected_job_workflow_ref"` GithubURL string `json:"github_url" yaml:"github_url"` + // The clientID is used to verify the audience claim in the token. + ClientID string `json:"client_id" yaml:"client_id"` } func (i Issuer) GetGithubURL() string { @@ -107,8 +114,8 @@ type TokenProcessor struct { rawToken string trustedIssuers map[string]Issuer issuer Issuer - verifierConfig VerifierConfig - logger LoggerInterface + // verifierConfig VerifierConfig + logger LoggerInterface } func (tokenProcessor *TokenProcessor) GetIssuer() Issuer { @@ -194,47 +201,6 @@ func maskToken(token string) string { 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) { - if clientID == "" { - return VerifierConfig{}, fmt.Errorf("clientID is empty") - } - - verifierConfig := VerifierConfig{} - verifierConfig.ClientID = clientID - verifierConfig.SkipClientIDCheck = false - verifierConfig.SkipExpiryCheck = false - verifierConfig.SkipIssuerCheck = false - verifierConfig.InsecureSkipSignatureCheck = false - verifierConfig.SupportedSigningAlgs = SupportedSigningAlgorithms - - logger.Debugw("Created Verifier config with default values", - "clientID", clientID, - "SkipClientIDCheck", verifierConfig.SkipClientIDCheck, - "SkipExpiryCheck", verifierConfig.SkipExpiryCheck, - "SkipIssuerCheck", verifierConfig.SkipIssuerCheck, - "InsecureSkipSignatureCheck", verifierConfig.InsecureSkipSignatureCheck, - "SupportedSigningAlgs", verifierConfig.SupportedSigningAlgs, - ) - logger.Debugw("Applying VerifierConfigOptions") - for _, option := range options { - err := option(&verifierConfig) - if err != nil { - return VerifierConfig{}, fmt.Errorf("failed to apply VerifierConfigOption: %w", err) - } - } - logger.Debugw("Applied all VerifierConfigOptions", - "clientID", clientID, - "SkipClientIDCheck", verifierConfig.SkipClientIDCheck, - "SkipExpiryCheck", verifierConfig.SkipExpiryCheck, - "SkipIssuerCheck", verifierConfig.SkipIssuerCheck, - "InsecureSkipSignatureCheck", verifierConfig.InsecureSkipSignatureCheck, - "SupportedSigningAlgs", verifierConfig.SupportedSigningAlgs, - ) - return verifierConfig, nil -} - // 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. @@ -353,14 +319,13 @@ func (provider *Provider) NewVerifier(logger LoggerInterface, verifierConfig Ver return tokenVerifier, nil } -// NewTokenProcessor creates a new TokenProcessor for trusted issuers. -// It reads the token, gets the issuer from the token, and checks if the issuer is trusted. -// It verifies the VerifierConfig has a clientID. +// NewTokenProcessor creates a new TokenProcessor for a trusted issuer. +// It reads the issuer from the raw token and checks if the issuer is trusted. +// It verifies the trusted issuer has a non-empty clientID. func NewTokenProcessor( logger LoggerInterface, trustedIssuers map[string]Issuer, rawToken string, - config VerifierConfig, options ...TokenProcessorOption, ) (TokenProcessor, error) { logger.Debugw("Creating token processor") @@ -371,30 +336,31 @@ func NewTokenProcessor( tokenProcessor.rawToken = rawToken logger.Debugw("Added raw token to token processor", "rawToken", maskToken(rawToken)) - tokenProcessor.verifierConfig = config - logger.Debugw("Added Verifier config to token processor", - "clientID", config.ClientID, - "SkipClientIDCheck", config.SkipClientIDCheck, - "SkipExpiryCheck", config.SkipExpiryCheck, - "SkipIssuerCheck", config.SkipIssuerCheck, - "InsecureSkipSignatureCheck", config.InsecureSkipSignatureCheck, - "SupportedSigningAlgs", config.SupportedSigningAlgs, - ) - if tokenProcessor.verifierConfig.ClientID == "" { - return TokenProcessor{}, errors.New("verifierConfig clientID is empty") - } - tokenProcessor.trustedIssuers = trustedIssuers + logger.Debugw("Added trusted issuers to token processor", "trustedIssuers", trustedIssuers) + issuer, err := tokenProcessor.tokenIssuer(SupportedSigningAlgorithms) if err != nil { return TokenProcessor{}, fmt.Errorf("failed to get issuer from token: %w", err) } + logger.Debugw("Got issuer from token", "issuer", issuer) + trustedIssuer, err := tokenProcessor.isTrustedIssuer(issuer, tokenProcessor.trustedIssuers) if err != nil { return TokenProcessor{}, err } + + logger.Debugw("Matched issuer with trusted issuer", "trustedIssuer", trustedIssuer) + + if trustedIssuer.ClientID == "" { + return TokenProcessor{}, errors.New("trusted issuer clientID is empty") + } + + logger.Debugw("Verified trusted issuer clientID", "clientID", trustedIssuer.ClientID) + tokenProcessor.issuer = trustedIssuer + logger.Debugw("Added trusted issuer to TokenProcessor", "issuer", tokenProcessor.issuer) if len(options) > 0 { @@ -409,9 +375,59 @@ func NewTokenProcessor( } logger.Debugw("Created token processor", "issuer", tokenProcessor.issuer) + return tokenProcessor, nil } +// NewVerifierConfig creates a new VerifierConfig for trusted issuer. +// It verifies if the clientID in the tokenProcessor is not empty. +func (tokenProcessor *TokenProcessor) NewVerifierConfig(options ...VerifierConfigOption) (VerifierConfig, error) { + logger := tokenProcessor.logger + + if tokenProcessor.issuer.ClientID == "" { + return VerifierConfig{}, fmt.Errorf("clientID is empty") + } + + logger.Debugw("TokenProcessor clientID is not empty", "clientID", tokenProcessor.issuer.ClientID) + + verifierConfig := VerifierConfig{} + verifierConfig.ClientID = tokenProcessor.issuer.ClientID + verifierConfig.SkipClientIDCheck = false + verifierConfig.SkipExpiryCheck = false + verifierConfig.SkipIssuerCheck = false + verifierConfig.InsecureSkipSignatureCheck = false + verifierConfig.SupportedSigningAlgs = SupportedSigningAlgorithms + + logger.Debugw("Created Verifier config with default values", + "clientID", verifierConfig.ClientID, + "SkipClientIDCheck", verifierConfig.SkipClientIDCheck, + "SkipExpiryCheck", verifierConfig.SkipExpiryCheck, + "SkipIssuerCheck", verifierConfig.SkipIssuerCheck, + "InsecureSkipSignatureCheck", verifierConfig.InsecureSkipSignatureCheck, + "SupportedSigningAlgs", verifierConfig.SupportedSigningAlgs, + ) + + logger.Debugw("Applying VerifierConfigOptions") + + for _, option := range options { + err := option(&verifierConfig) + if err != nil { + return VerifierConfig{}, fmt.Errorf("failed to apply VerifierConfigOption: %w", err) + } + } + + logger.Debugw("Applied all VerifierConfigOptions", + "clientID", verifierConfig.ClientID, + "SkipClientIDCheck", verifierConfig.SkipClientIDCheck, + "SkipExpiryCheck", verifierConfig.SkipExpiryCheck, + "SkipIssuerCheck", verifierConfig.SkipIssuerCheck, + "InsecureSkipSignatureCheck", verifierConfig.InsecureSkipSignatureCheck, + "SupportedSigningAlgs", verifierConfig.SupportedSigningAlgs, + ) + + return verifierConfig, nil +} + // tokenIssuer gets the issuer from the token. // It doesn't verify the token, just parses its claims. // It's used to create a new TokenProcessor. @@ -486,4 +502,4 @@ func (tokenProcessor *TokenProcessor) ValidateClaims(claims ClaimsInterface, tok return fmt.Errorf("expecations validation failed: %w", err) } return nil -} +} \ No newline at end of file diff --git a/pkg/oidc/oidc_test.go b/pkg/oidc/oidc_test.go index 70f4d967308d..ff85489667db 100644 --- a/pkg/oidc/oidc_test.go +++ b/pkg/oidc/oidc_test.go @@ -30,7 +30,6 @@ var _ = Describe("OIDC", func() { rawToken []byte verifierConfig tioidc.VerifierConfig tokenProcessor tioidc.TokenProcessor - clientID string ) BeforeEach(func() { @@ -38,7 +37,16 @@ var _ = Describe("OIDC", func() { Expect(err).NotTo(HaveOccurred()) logger = l.Sugar() - clientID = "testClientID" + + trustedIssuers = map[string]tioidc.Issuer{ + "https://fakedings.dev-gcp.nais.io/fake": { + Name: "github", + IssuerURL: "https://fakedings.dev-gcp.nais.io/fake", + JWKSURL: "https://fakedings.dev-gcp.nais.io/fake/jwks", + ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main", + ClientID: "testClientID", + }, + } }) Describe("SkipExpiryCheck", func() { @@ -55,26 +63,33 @@ var _ = Describe("OIDC", func() { var ( verifierConfigOption tioidc.VerifierConfigOption ) - It("should return a new oidc.Config", func() { - verifierConfig, err := tioidc.NewVerifierConfig(logger, clientID) + + BeforeEach(func() { + rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") + Expect(err).NotTo(HaveOccurred()) + + tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) + Expect(err).NotTo(HaveOccurred()) + Expect(tokenProcessor).NotTo(BeNil()) + }) + + It("should create a new default VerifierConfig", func() { + verifierConfig, err := tokenProcessor.NewVerifierConfig() Expect(err).NotTo(HaveOccurred()) Expect(verifierConfig).To(BeAssignableToTypeOf(tioidc.VerifierConfig{})) - Expect(verifierConfig.ClientID).To(Equal(clientID)) + Expect(verifierConfig.ClientID).To(Equal(trustedIssuers["https://fakedings.dev-gcp.nais.io/fake"].ClientID)) Expect(verifierConfig.SupportedSigningAlgs).To(Equal(tioidc.SupportedSigningAlgorithms)) + Expect(verifierConfig.SkipExpiryCheck).To(BeFalse()) + Expect(verifierConfig.SkipClientIDCheck).To(BeFalse()) + Expect(verifierConfig.SkipIssuerCheck).To(BeFalse()) + Expect(verifierConfig.InsecureSkipSignatureCheck).To(BeFalse()) }) When("invalid VerifierConfigOption are provided", func() { It("should return an error", func() { verifierConfigOption = func(config *tioidc.VerifierConfig) error { return errors.New("invalid VerifierConfigOption") } - verifierConfig, err := tioidc.NewVerifierConfig(logger, clientID, verifierConfigOption) - Expect(err).To(HaveOccurred()) - Expect(verifierConfig).To(Equal(tioidc.VerifierConfig{})) - }) - }) - When("empty clientID is provided", func() { - It("should return an error", func() { - verifierConfig, err := tioidc.NewVerifierConfig(logger, "") + verifierConfig, err := tokenProcessor.NewVerifierConfig(verifierConfigOption) Expect(err).To(HaveOccurred()) Expect(verifierConfig).To(Equal(tioidc.VerifierConfig{})) }) @@ -90,32 +105,23 @@ var _ = Describe("OIDC", func() { // Read the token from the file in test-fixtures directory. rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") Expect(err).NotTo(HaveOccurred()) - - verifierConfig, err = tioidc.NewVerifierConfig(logger, clientID) - Expect(err).NotTo(HaveOccurred()) - - trustedIssuers = map[string]tioidc.Issuer{ - "https://fakedings.dev-gcp.nais.io/fake": { - Name: "github", - IssuerURL: "https://fakedings.dev-gcp.nais.io/fake", - JWKSURL: "https://fakedings.dev-gcp.nais.io/fake/jwks", - }, - } }) + When("issuer is trusted", func() { It("should return a new TokenProcessor", func() { - tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), verifierConfig) + tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) Expect(err).NotTo(HaveOccurred()) Expect(tokenProcessor).To(BeAssignableToTypeOf(tioidc.TokenProcessor{})) }) }) - When("empty verifierConfig is provided", func() { + When("issuer with empty clientID is provided", func() { It("should return an error", func() { - // Empty verifierConfig - verifierConfig = tioidc.VerifierConfig{} - tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), verifierConfig) + // Empty issuer clientID + trustedIssuers["https://fakedings.dev-gcp.nais.io/fake"].ClientID = "" + + tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("verifierConfig clientID is empty")) + Expect(err).To(MatchError("trusted issuer clientID is empty")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) }) }) @@ -123,14 +129,15 @@ var _ = Describe("OIDC", func() { It("should return an error", func() { // Untrusted issuer trustedIssuers = map[string]tioidc.Issuer{ - "https://untrusted.fakedings.dev-gcp.nais.io/fake": { + "https://other-trusted.fakedings.dev-gcp.nais.io/fake": { Name: "github", - IssuerURL: "https://untrusted.fakedings.dev-gcp.nais.io/fake", - JWKSURL: "https://untrusted.fakedings.dev-gcp.nais.io/fake/jwks", + IssuerURL: "https://other-trusted.fakedings.dev-gcp.nais.io/fake", + JWKSURL: "https://other-trusted.fakedings.dev-gcp.nais.io/fake/jwks", + ClientID: "testClientID", }, } - tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), verifierConfig) + tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) Expect(err).To(HaveOccurred()) Expect(err).To(MatchError("issuer https://fakedings.dev-gcp.nais.io/fake is not trusted")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) @@ -138,7 +145,7 @@ var _ = Describe("OIDC", func() { }) When("no trustedIssuers are provided", func() { It("should return an error", func() { - tokenProcessor, err := tioidc.NewTokenProcessor(logger, nil, string(rawToken), verifierConfig) + tokenProcessor, err := tioidc.NewTokenProcessor(logger, nil, string(rawToken)) Expect(err).To(HaveOccurred()) Expect(err).To(MatchError("issuer https://fakedings.dev-gcp.nais.io/fake is not trusted")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) @@ -150,7 +157,7 @@ var _ = Describe("OIDC", func() { return errors.New("invalid TokenProcessorOption") } - tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), verifierConfig, invalidTokenProcessorOption) + tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), invalidTokenProcessorOption) Expect(err).To(HaveOccurred()) Expect(err).To(MatchError("failed to apply TokenProcessorOption: invalid TokenProcessorOption")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) @@ -158,7 +165,7 @@ var _ = Describe("OIDC", func() { }) When("invalid raw token is provided", func() { It("should return an error", func() { - tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, "invalidToken", verifierConfig) + tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, "invalidToken") Expect(err).To(HaveOccurred()) Expect(err).To(MatchError("failed to get issuer from token: failed to parse oidc token: go-jose/go-jose: compact JWS format must have three parts")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) @@ -168,29 +175,16 @@ var _ = Describe("OIDC", func() { Describe("TokenProcessor", func() { var ( - Token oidcmocks.MockTokenInterface - claims tioidc.Claims - token tioidc.Token - mockToken oidcmocks.MockClaimsReader - tokenProcessor tioidc.TokenProcessor + Token oidcmocks.MockTokenInterface + claims tioidc.Claims + token tioidc.Token + mockToken oidcmocks.MockClaimsReader ) BeforeEach(func() { - verifierConfig, err = tioidc.NewVerifierConfig(logger, clientID) - Expect(err).NotTo(HaveOccurred()) - rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") Expect(err).NotTo(HaveOccurred()) - trustedIssuers = map[string]tioidc.Issuer{ - "https://fakedings.dev-gcp.nais.io/fake": { - Name: "github", - IssuerURL: "https://fakedings.dev-gcp.nais.io/fake", - JWKSURL: "https://fakedings.dev-gcp.nais.io/fake/jwks", - ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main", - }, - } - - tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken), verifierConfig) + tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) Expect(err).NotTo(HaveOccurred()) Expect(tokenProcessor).NotTo(BeNil()) @@ -280,7 +274,7 @@ var _ = Describe("OIDC", func() { Verifier: verifier, Logger: logger, } - verifierConfig, err = tioidc.NewVerifierConfig(logger, clientID) + // verifierConfig, err = tioidc.NewVerifierConfig(logger, clientID) Expect(err).NotTo(HaveOccurred()) ctx = context.Background() rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") @@ -443,16 +437,23 @@ var _ = Describe("OIDC", func() { Describe("Provider", func() { var ( - provider tioidc.Provider - oidcProvider *oidcmocks.MockVerifierProvider - verifierConfig tioidc.VerifierConfig + provider tioidc.Provider + oidcProvider *oidcmocks.MockVerifierProvider ) BeforeEach(func() { oidcProvider = &oidcmocks.MockVerifierProvider{} provider = tioidc.Provider{ VerifierProvider: oidcProvider, } - verifierConfig, err = tioidc.NewVerifierConfig(logger, clientID) + + rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") + Expect(err).NotTo(HaveOccurred()) + + tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) + Expect(err).NotTo(HaveOccurred()) + Expect(tokenProcessor).NotTo(BeNil()) + + verifierConfig, err = tokenProcessor.NewVerifierConfig() Expect(err).NotTo(HaveOccurred()) }) Describe("NewVerifier", func() { @@ -465,4 +466,4 @@ var _ = Describe("OIDC", func() { }) }) }) -}) +}) \ No newline at end of file diff --git a/pkg/oidc/oidc_unit_test.go b/pkg/oidc/oidc_unit_test.go index a4b996cc4fca..9060ffb57f18 100644 --- a/pkg/oidc/oidc_unit_test.go +++ b/pkg/oidc/oidc_unit_test.go @@ -3,8 +3,11 @@ package oidc // oidc_unit_test.go contains tests which require access to non-exported functions and variables. import ( + "os" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "go.uber.org/zap" ) var _ = Describe("OIDC", func() { @@ -34,4 +37,41 @@ var _ = Describe("OIDC", func() { }) }) -}) + Describe("NewVerifierConfig", func() { + var ( + tokenProcessor TokenProcessor + err error + logger *zap.SugaredLogger + trustedIssuers map[string]Issuer + rawToken []byte + ) + + BeforeEach(func() { + rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") + Expect(err).NotTo(HaveOccurred()) + + trustedIssuers = map[string]Issuer{ + "https://fakedings.dev-gcp.nais.io/fake": { + Name: "github", + IssuerURL: "https://fakedings.dev-gcp.nais.io/fake", + JWKSURL: "https://fakedings.dev-gcp.nais.io/fake/jwks", + ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main", + ClientID: "testClientID", + }, + } + + tokenProcessor, err = NewTokenProcessor(logger, trustedIssuers, string(rawToken)) + Expect(err).NotTo(HaveOccurred()) + Expect(tokenProcessor).NotTo(BeNil()) + }) + + When("empty clientID is provided", func() { + It("should return an error", func() { + tokenProcessor.issuer.ClientID = "" + verifierConfig, err := tokenProcessor.NewVerifierConfig() + Expect(err).To(HaveOccurred()) + Expect(verifierConfig).To(Equal(VerifierConfig{})) + }) + }) + }) +}) \ No newline at end of file From b143e0d606dd2ccce3e23fcd03bcfb2095928b2d Mon Sep 17 00:00:00 2001 From: dekiel Date: Thu, 9 Jan 2025 14:22:56 +0100 Subject: [PATCH 2/6] Fix compile error. --- pkg/oidc/oidc_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/oidc/oidc_test.go b/pkg/oidc/oidc_test.go index ff85489667db..2f688a71a4e2 100644 --- a/pkg/oidc/oidc_test.go +++ b/pkg/oidc/oidc_test.go @@ -117,7 +117,9 @@ var _ = Describe("OIDC", func() { When("issuer with empty clientID is provided", func() { It("should return an error", func() { // Empty issuer clientID - trustedIssuers["https://fakedings.dev-gcp.nais.io/fake"].ClientID = "" + issuer := trustedIssuers["https://fakedings.dev-gcp.nais.io/fake"] + issuer.ClientID = "" + trustedIssuers["https://fakedings.dev-gcp.nais.io/fake"] = issuer tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) Expect(err).To(HaveOccurred()) @@ -466,4 +468,4 @@ var _ = Describe("OIDC", func() { }) }) }) -}) \ No newline at end of file +}) From 5e2461d2d78f97944cfbb480fff322dffbcb4f83 Mon Sep 17 00:00:00 2001 From: dekiel Date: Thu, 9 Jan 2025 14:31:51 +0100 Subject: [PATCH 3/6] Fixed tests errors. --- pkg/oidc/oidc_test.go | 2 -- pkg/oidc/oidc_unit_test.go | 13 +++++++++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/oidc/oidc_test.go b/pkg/oidc/oidc_test.go index 2f688a71a4e2..fba1cf138778 100644 --- a/pkg/oidc/oidc_test.go +++ b/pkg/oidc/oidc_test.go @@ -276,8 +276,6 @@ 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()) diff --git a/pkg/oidc/oidc_unit_test.go b/pkg/oidc/oidc_unit_test.go index 9060ffb57f18..0b427ae82992 100644 --- a/pkg/oidc/oidc_unit_test.go +++ b/pkg/oidc/oidc_unit_test.go @@ -11,6 +11,16 @@ import ( ) var _ = Describe("OIDC", func() { + var ( + logger *zap.SugaredLogger + ) + BeforeEach(func() { + l, err := zap.NewDevelopment() + Expect(err).NotTo(HaveOccurred()) + + logger = l.Sugar() + }) + Describe("maskToken", func() { It("should mask the token if length is less than 15", func() { token := "shorttoken" @@ -41,7 +51,6 @@ var _ = Describe("OIDC", func() { var ( tokenProcessor TokenProcessor err error - logger *zap.SugaredLogger trustedIssuers map[string]Issuer rawToken []byte ) @@ -74,4 +83,4 @@ var _ = Describe("OIDC", func() { }) }) }) -}) \ No newline at end of file +}) From 80fc2004eb36e9e0b7c117ac3b35202689763296 Mon Sep 17 00:00:00 2001 From: dekiel Date: Thu, 9 Jan 2025 15:24:21 +0100 Subject: [PATCH 4/6] cleanup --- cmd/oidc-token-verifier/main.go | 7 ------- pkg/oidc/oidc.go | 1 - 2 files changed, 8 deletions(-) diff --git a/cmd/oidc-token-verifier/main.go b/cmd/oidc-token-verifier/main.go index c1f750f919f5..2517d6c247d0 100644 --- a/cmd/oidc-token-verifier/main.go +++ b/cmd/oidc-token-verifier/main.go @@ -21,7 +21,6 @@ type Logger interface { type options struct { token string - trustedWorkflows []string debug bool oidcTokenExpirationTime int // OIDC token expiration time in minutes } @@ -40,12 +39,6 @@ func NewRootCmd() *cobra.Command { It uses OIDC discovery to get the public keys and verify the token whenever the public keys are not cached or expired.`, } rootCmd.PersistentFlags().StringVarP(&opts.token, "token", "t", "", "OIDC token to verify") - // This flag should be enabled once we add support for it in the code. - // rootCmd.PersistentFlags().StringSliceVarP(&opts.trustedWorkflows, "trusted-workflows", "w", []string{}, "List of trusted workflows") - // err := rootCmd.MarkPersistentFlagRequired("trusted-workflows") - // if err != nil { - // panic(err) - // } 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 diff --git a/pkg/oidc/oidc.go b/pkg/oidc/oidc.go index 514f0c8b6461..0885be3ec56e 100644 --- a/pkg/oidc/oidc.go +++ b/pkg/oidc/oidc.go @@ -114,7 +114,6 @@ type TokenProcessor struct { rawToken string trustedIssuers map[string]Issuer issuer Issuer - // verifierConfig VerifierConfig logger LoggerInterface } From 2096b2e538e985f2bfdbd8a308dd7730d76e1c94 Mon Sep 17 00:00:00 2001 From: dekiel Date: Fri, 10 Jan 2025 14:55:28 +0100 Subject: [PATCH 5/6] Refactored NewTokenProcessor function. Extracted part responsible for validating and setting trusted issuer. The issuer validation code is now a method on issuer type. --- pkg/oidc/oidc.go | 121 ++++++++++++++++----- pkg/oidc/oidc_test.go | 8 +- pkg/oidc/oidc_unit_test.go | 209 +++++++++++++++++++++++++++++++++---- 3 files changed, 289 insertions(+), 49 deletions(-) diff --git a/pkg/oidc/oidc.go b/pkg/oidc/oidc.go index 0885be3ec56e..44d8a3735e17 100644 --- a/pkg/oidc/oidc.go +++ b/pkg/oidc/oidc.go @@ -7,6 +7,7 @@ package oidc import ( "errors" "fmt" + "net/url" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -26,7 +27,7 @@ var ( JWKSURL: "https://token.actions.githubusercontent.com/.well-known/jwks", ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/image-builder.yml@refs/heads/main", GithubURL: "https://github.com", - ClientID: "image-builder", + ClientID: "image-builder", } GithubToolsSAPOIDCIssuer = Issuer{ Name: "github-tools-sap", @@ -34,7 +35,7 @@ var ( JWKSURL: "https://github.tools.sap/_services/token/.well-known/jwks", ExpectedJobWorkflowRef: "kyma/oci-image-builder/.github/workflows/image-builder.yml@refs/heads/main", GithubURL: "https://github.tools.sap", - ClientID: "image-builder", + ClientID: "image-builder", } TrustedOIDCIssuers = map[string]Issuer{ GithubOIDCIssuer.IssuerURL: GithubOIDCIssuer, @@ -92,8 +93,63 @@ type Issuer struct { ClientID string `json:"client_id" yaml:"client_id"` } -func (i Issuer) GetGithubURL() string { - return i.GithubURL +func (issuer Issuer) GetGithubURL() string { + return issuer.GithubURL +} + +// validateIssuer checks if the issuer is valid. +func (issuer Issuer) validateIssuer(logger LoggerInterface) error { + logger.Debugw("Validating issuer", "issuer", issuer) + + if issuer.Name == "" { + return fmt.Errorf("issuer name is empty") + } + + if err := validateURL(logger, issuer.IssuerURL, "issuer URL"); err != nil { + return err + } + + if err := validateURL(logger, issuer.JWKSURL, "issuer JWKS URL"); err != nil { + return err + } + + if issuer.ClientID == "" { + return fmt.Errorf("issuer clientID is empty") + } + + logger.Debugw("Issuer validation successful", "issuer", issuer) + + return nil +} + +// validateURL checks if the URL is valid and uses https. +func validateURL(logger LoggerInterface, rawURL, urlType string) error { + logger.Debugw("Validating URL", "urlType", urlType, "rawURL", rawURL) + + if rawURL == "" { + return fmt.Errorf("%s is empty", urlType) + } + + parsedURL, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("failed parsing %s: %w", urlType, err) + } + + if parsedURL.Scheme == "" { + return fmt.Errorf("%s scheme is empty", urlType) + } + + if parsedURL.Scheme != "https" { + return fmt.Errorf("%s is not using https", urlType) + } + + if parsedURL.Host == "" { + return fmt.Errorf("%s host is empty", urlType) + } + + logger.Debugw("URL validation successful", "urlType", urlType, "parsedURL", parsedURL) + + return nil } // VerifierConfig is the configuration for a verifier. @@ -114,7 +170,7 @@ type TokenProcessor struct { rawToken string trustedIssuers map[string]Issuer issuer Issuer - logger LoggerInterface + logger LoggerInterface } func (tokenProcessor *TokenProcessor) GetIssuer() Issuer { @@ -329,37 +385,20 @@ func NewTokenProcessor( ) (TokenProcessor, error) { logger.Debugw("Creating token processor") tokenProcessor := TokenProcessor{} - tokenProcessor.logger = logger - tokenProcessor.rawToken = rawToken + logger.Debugw("Added raw token to token processor", "rawToken", maskToken(rawToken)) tokenProcessor.trustedIssuers = trustedIssuers - logger.Debugw("Added trusted issuers to token processor", "trustedIssuers", trustedIssuers) - issuer, err := tokenProcessor.tokenIssuer(SupportedSigningAlgorithms) - if err != nil { - return TokenProcessor{}, fmt.Errorf("failed to get issuer from token: %w", err) - } - - logger.Debugw("Got issuer from token", "issuer", issuer) + logger.Debugw("Added trusted issuers to token processor", "trustedIssuers", trustedIssuers) - trustedIssuer, err := tokenProcessor.isTrustedIssuer(issuer, tokenProcessor.trustedIssuers) + err := tokenProcessor.setIssuer() if err != nil { - return TokenProcessor{}, err + return TokenProcessor{}, fmt.Errorf("failed to set issuer: %w", err) } - logger.Debugw("Matched issuer with trusted issuer", "trustedIssuer", trustedIssuer) - - if trustedIssuer.ClientID == "" { - return TokenProcessor{}, errors.New("trusted issuer clientID is empty") - } - - logger.Debugw("Verified trusted issuer clientID", "clientID", trustedIssuer.ClientID) - - tokenProcessor.issuer = trustedIssuer - logger.Debugw("Added trusted issuer to TokenProcessor", "issuer", tokenProcessor.issuer) if len(options) > 0 { @@ -378,6 +417,36 @@ func NewTokenProcessor( return tokenProcessor, nil } +// setIssuer sets the issuer for the token processor. +// It reads the issuer from the raw token and checks if the issuer is trusted and valid. +func (tokenProcessor *TokenProcessor) setIssuer() error { + logger := tokenProcessor.logger + issuer, err := tokenProcessor.tokenIssuer(SupportedSigningAlgorithms) + if err != nil { + return fmt.Errorf("failed to get issuer from token: %w", err) + } + + logger.Debugw("Got issuer from token", "issuer", issuer) + + trustedIssuer, err := tokenProcessor.isTrustedIssuer(issuer, tokenProcessor.trustedIssuers) + if err != nil { + return err + } + + logger.Debugw("Matched issuer with trusted issuer", "trustedIssuer", trustedIssuer) + + err = trustedIssuer.validateIssuer(logger) + if err != nil { + return errors.New("trusted issuer clientID is empty") + } + + logger.Debugw("Verified trusted issuer clientID", "clientID", trustedIssuer.ClientID) + + tokenProcessor.issuer = trustedIssuer + + return nil +} + // NewVerifierConfig creates a new VerifierConfig for trusted issuer. // It verifies if the clientID in the tokenProcessor is not empty. func (tokenProcessor *TokenProcessor) NewVerifierConfig(options ...VerifierConfigOption) (VerifierConfig, error) { diff --git a/pkg/oidc/oidc_test.go b/pkg/oidc/oidc_test.go index fba1cf138778..621c749f23ca 100644 --- a/pkg/oidc/oidc_test.go +++ b/pkg/oidc/oidc_test.go @@ -123,7 +123,7 @@ var _ = Describe("OIDC", func() { tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("trusted issuer clientID is empty")) + Expect(err).To(MatchError("failed to set issuer: trusted issuer clientID is empty")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) }) }) @@ -141,7 +141,7 @@ var _ = Describe("OIDC", func() { tokenProcessor, err = tioidc.NewTokenProcessor(logger, trustedIssuers, string(rawToken)) Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("issuer https://fakedings.dev-gcp.nais.io/fake is not trusted")) + Expect(err).To(MatchError("failed to set issuer: issuer https://fakedings.dev-gcp.nais.io/fake is not trusted")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) }) }) @@ -149,7 +149,7 @@ var _ = Describe("OIDC", func() { It("should return an error", func() { tokenProcessor, err := tioidc.NewTokenProcessor(logger, nil, string(rawToken)) Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("issuer https://fakedings.dev-gcp.nais.io/fake is not trusted")) + Expect(err).To(MatchError("failed to set issuer: issuer https://fakedings.dev-gcp.nais.io/fake is not trusted")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) }) }) @@ -169,7 +169,7 @@ var _ = Describe("OIDC", func() { It("should return an error", func() { tokenProcessor, err := tioidc.NewTokenProcessor(logger, trustedIssuers, "invalidToken") Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError("failed to get issuer from token: failed to parse oidc token: go-jose/go-jose: compact JWS format must have three parts")) + Expect(err).To(MatchError("failed to set issuer: failed to get issuer from token: failed to parse oidc token: go-jose/go-jose: compact JWS format must have three parts")) Expect(tokenProcessor).To(Equal(tioidc.TokenProcessor{})) }) }) diff --git a/pkg/oidc/oidc_unit_test.go b/pkg/oidc/oidc_unit_test.go index 0b427ae82992..bc291330e565 100644 --- a/pkg/oidc/oidc_unit_test.go +++ b/pkg/oidc/oidc_unit_test.go @@ -3,6 +3,7 @@ package oidc // oidc_unit_test.go contains tests which require access to non-exported functions and variables. import ( + "fmt" "os" . "github.com/onsi/ginkgo/v2" @@ -47,39 +48,209 @@ var _ = Describe("OIDC", func() { }) }) - Describe("NewVerifierConfig", func() { + Describe("TokenProcessor", func() { var ( tokenProcessor TokenProcessor - err error trustedIssuers map[string]Issuer rawToken []byte + mockIssuer Issuer + err error ) BeforeEach(func() { - rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") - Expect(err).NotTo(HaveOccurred()) + mockIssuer = Issuer{ + Name: "github", + IssuerURL: "https://fakedings.dev-gcp.nais.io/fake", + JWKSURL: "https://fakedings.dev-gcp.nais.io/fake/jwks", + ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main", + ClientID: "testClientID", + } trustedIssuers = map[string]Issuer{ - "https://fakedings.dev-gcp.nais.io/fake": { - Name: "github", - IssuerURL: "https://fakedings.dev-gcp.nais.io/fake", - JWKSURL: "https://fakedings.dev-gcp.nais.io/fake/jwks", - ExpectedJobWorkflowRef: "kyma-project/test-infra/.github/workflows/verify-oidc-token.yml@refs/heads/main", - ClientID: "testClientID", - }, + mockIssuer.IssuerURL: mockIssuer, } - tokenProcessor, err = NewTokenProcessor(logger, trustedIssuers, string(rawToken)) + rawToken, err = os.ReadFile("test-fixtures/raw-oidc-token") Expect(err).NotTo(HaveOccurred()) - Expect(tokenProcessor).NotTo(BeNil()) + + tokenProcessor = TokenProcessor{ + logger: logger, + trustedIssuers: trustedIssuers, + rawToken: string(rawToken), + } + }) + + // This NewVerifierConfig scenario is tested here because it requires an access to the TokenProcessor issuer struct field. + Describe("NewVerifierConfig", func() { + When("empty clientID is provided", func() { + It("should return an error", func() { + tokenProcessor.issuer = mockIssuer + tokenProcessor.issuer.ClientID = "" + + verifierConfig, err := tokenProcessor.NewVerifierConfig() + + Expect(err).To(HaveOccurred(), "Expected an error to occur when clientID is empty, but no error occurred") + Expect(verifierConfig).To(Equal(VerifierConfig{}), "Expected verifierConfig to be an empty VerifierConfig struct, but got: %v", verifierConfig) + }) + }) + }) + + Describe("setIssuer", func() { + It("should set the issuer successfully", func() { + err := tokenProcessor.setIssuer() + Expect(err).NotTo(HaveOccurred(), "Expected no error, but got: %v", err) + Expect(tokenProcessor.issuer).To(Equal(mockIssuer), "Expected issuer to be set to mockIssuer, but got: %v", tokenProcessor.issuer) + }) + + It("should return an error if issuer is not trusted", func() { + tokenProcessor.trustedIssuers = map[string]Issuer{ + "https://untrusted.issuer": mockIssuer, + } + err := tokenProcessor.setIssuer() + Expect(err).To(HaveOccurred(), "Expected an error, but got none") + Expect(err).To(MatchError(fmt.Sprintf("issuer %s is not trusted", mockIssuer.IssuerURL), "Expected error message to match")) + }) + + It("should return an error if issuer is not valid", func() { + tokenProcessor.trustedIssuers[mockIssuer.IssuerURL] = Issuer{ + Name: "mock", + IssuerURL: "https://mock.issuer", + JWKSURL: "https://mock.issuer/jwks", + ClientID: "", + } + + err := tokenProcessor.setIssuer() + Expect(err).To(HaveOccurred(), "Expected an error, but got none") + Expect(err).To(MatchError("trusted issuer clientID is empty"), "Expected error message to match") + }) }) + }) + + Describe("Issuer", func() { + var ( + issuer Issuer + ) + + Describe("validateIssuer", func() { + When("the issuer name is empty", func() { + It("should return an error", func() { + issuer = Issuer{ + IssuerURL: "https://valid.url", + JWKSURL: "https://valid.url/jwks", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer name is empty, but got none") + Expect(err).To(MatchError("issuer name is empty"), "Expected error message to match") + }) + }) + + When("the issuer URL is empty", func() { + It("should return an error", func() { + issuer = Issuer{ + Name: "valid-name", + JWKSURL: "https://valid.url/jwks", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer URL is empty, but got none") + Expect(err).To(MatchError("issuer URL is empty"), "Expected error message to match") + }) + }) + + When("the issuer URL is not valid", func() { + It("should return an error", func() { + issuer = Issuer{ + Name: "valid-name", + IssuerURL: "invalid-url", + JWKSURL: "https://valid.url/jwks", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer URL is not valid, but got none") + Expect(err).To(MatchError("failed parsing issuer URL: parse \"invalid-url\": invalid URI for request"), "Expected error message to match") + }) + }) + + When("the issuer URL is not using https", func() { + It("should return an error", func() { + issuer = Issuer{ + Name: "valid-name", + IssuerURL: "http://valid.url", + JWKSURL: "https://valid.url/jwks", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer URL is not using https, but got none") + Expect(err).To(MatchError("issuer URL is not using https"), "Expected error message to match") + }) + }) + + When("the issuer JWKS URL is empty", func() { + It("should return an error", func() { + issuer = Issuer{ + Name: "valid-name", + IssuerURL: "https://valid.url", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer JWKS URL is empty, but got none") + Expect(err).To(MatchError("issuer JWKS URL is empty"), "Expected error message to match") + }) + }) + + When("the issuer JWKS URL is not valid", func() { + It("should return an error", func() { + issuer = Issuer{ + Name: "valid-name", + IssuerURL: "https://valid.url", + JWKSURL: "invalid-url", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer JWKS URL is not valid, but got none") + Expect(err).To(MatchError("failed parsing issuer JWKS URL: parse \"invalid-url\": invalid URI for request"), "Expected error message to match") + }) + }) + + When("the issuer JWKS URL is not using https", func() { + It("should return an error", func() { + issuer = Issuer{ + Name: "valid-name", + IssuerURL: "https://valid.url", + JWKSURL: "http://valid.url/jwks", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer JWKS URL is not using https, but got none") + Expect(err).To(MatchError("issuer JWKS URL is not using https"), "Expected error message to match") + }) + }) + + When("the issuer clientID is empty", func() { + It("should return an error", func() { + issuer = Issuer{ + Name: "valid-name", + IssuerURL: "https://valid.url", + JWKSURL: "https://valid.url/jwks", + } + err := issuer.validateIssuer(logger) + Expect(err).To(HaveOccurred(), "Expected an error when issuer clientID is empty, but got none") + Expect(err).To(MatchError("issuer clientID is empty"), "Expected error message to match") + }) + }) - When("empty clientID is provided", func() { - It("should return an error", func() { - tokenProcessor.issuer.ClientID = "" - verifierConfig, err := tokenProcessor.NewVerifierConfig() - Expect(err).To(HaveOccurred()) - Expect(verifierConfig).To(Equal(VerifierConfig{})) + When("all issuer fields are valid", func() { + It("should not return an error", func() { + issuer = Issuer{ + Name: "valid-name", + IssuerURL: "https://valid.url", + JWKSURL: "https://valid.url/jwks", + ClientID: "valid-client-id", + } + err := issuer.validateIssuer(logger) + Expect(err).NotTo(HaveOccurred(), "Expected no error when all issuer fields are valid, but got: %v", err) + }) }) }) }) From aadc0104523d700d3fdc9e3c451236613132ad9f Mon Sep 17 00:00:00 2001 From: dekiel Date: Fri, 10 Jan 2025 15:15:19 +0100 Subject: [PATCH 6/6] Fixed tests. Removed testing error message, just expecting an error. --- pkg/oidc/oidc_unit_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/oidc/oidc_unit_test.go b/pkg/oidc/oidc_unit_test.go index bc291330e565..ea34db90be2c 100644 --- a/pkg/oidc/oidc_unit_test.go +++ b/pkg/oidc/oidc_unit_test.go @@ -168,7 +168,6 @@ var _ = Describe("OIDC", func() { } err := issuer.validateIssuer(logger) Expect(err).To(HaveOccurred(), "Expected an error when issuer URL is not valid, but got none") - Expect(err).To(MatchError("failed parsing issuer URL: parse \"invalid-url\": invalid URI for request"), "Expected error message to match") }) }) @@ -209,7 +208,6 @@ var _ = Describe("OIDC", func() { } err := issuer.validateIssuer(logger) Expect(err).To(HaveOccurred(), "Expected an error when issuer JWKS URL is not valid, but got none") - Expect(err).To(MatchError("failed parsing issuer JWKS URL: parse \"invalid-url\": invalid URI for request"), "Expected error message to match") }) })