From e0db59fd090486e90a44880c2b9d9549fe00d3a7 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 22 Oct 2021 10:23:21 -0700 Subject: [PATCH] More small updates based on PR feedback --- .../oidc_upstream_watcher.go | 9 +++- .../oidc_upstream_watcher_test.go | 43 +++++++++++++++++++ internal/oidc/token/token_handler.go | 5 +-- internal/oidc/token/token_handler_test.go | 2 +- pkg/oidcclient/login.go | 2 +- 5 files changed, 55 insertions(+), 6 deletions(-) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 34e81631..0e4399ea 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -60,6 +60,7 @@ const ( reasonUnreachable = "Unreachable" reasonInvalidResponse = "InvalidResponse" reasonDisallowedParameterName = "DisallowedParameterName" + allParamNamesAllowedMsg = "additionalAuthorizeParameters parameter names are allowed" // Errors that are generated by our reconcile process. errOIDCFailureStatus = constable.Error("OIDCIdentityProvider has a failing condition") @@ -221,7 +222,6 @@ func (c *oidcWatcherController) validateUpstream(ctx controllerlib.Context, upst c.validateIssuer(ctx.Context, upstream, &result), } if len(rejectedAuthcodeAuthorizeParameters) > 0 { - // This condition probably isn't important enough to report when it is successful, so just report errors. conditions = append(conditions, &v1alpha1.Condition{ Type: typeAdditionalAuthorizeParametersValid, Status: v1alpha1.ConditionFalse, @@ -229,6 +229,13 @@ func (c *oidcWatcherController) validateUpstream(ctx controllerlib.Context, upst Message: fmt.Sprintf("the following additionalAuthorizeParameters are not allowed: %s", strings.Join(rejectedAuthcodeAuthorizeParameters, ",")), }) + } else { + conditions = append(conditions, &v1alpha1.Condition{ + Type: typeAdditionalAuthorizeParametersValid, + Status: v1alpha1.ConditionTrue, + Reason: upstreamwatchers.ReasonSuccess, + Message: allParamNamesAllowedMsg, + }) } c.updateStatus(ctx.Context, upstream, conditions) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index dc61c63d..57942c02 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -119,6 +119,16 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { require.NoError(t, err) wrongCABase64 := base64.StdEncoding.EncodeToString(wrongCA.Bundle()) + happyAdditionalAuthorizeParametersValidCondition := v1alpha1.Condition{ + Type: "AdditionalAuthorizeParametersValid", + Status: "True", + Reason: "Success", + Message: "additionalAuthorizeParameters parameter names are allowed", + LastTransitionTime: now, + } + happyAdditionalAuthorizeParametersValidConditionEarlier := happyAdditionalAuthorizeParametersValidCondition + happyAdditionalAuthorizeParametersValidConditionEarlier.LastTransitionTime = earlier + var ( testNamespace = "test-namespace" testName = "test-name" @@ -162,6 +172,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="secret \"test-client-secret\" not found" "reason"="SecretNotFound" "status"="False" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="secret \"test-client-secret\" not found" "name"="test-name" "namespace"="test-namespace" "reason"="SecretNotFound" "type"="ClientCredentialsValid"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -170,6 +181,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "False", @@ -207,6 +219,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "reason"="SecretWrongType" "status"="False" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "name"="test-name" "namespace"="test-namespace" "reason"="SecretWrongType" "type"="ClientCredentialsValid"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -215,6 +228,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "False", @@ -251,6 +265,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "reason"="SecretMissingKeys" "status"="False" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "name"="test-name" "namespace"="test-namespace" "reason"="SecretMissingKeys" "type"="ClientCredentialsValid"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -259,6 +274,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "False", @@ -298,6 +314,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7" "reason"="InvalidTLSConfig" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -306,6 +323,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", @@ -345,6 +363,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="spec.certificateAuthorityData is invalid: no certificates found" "reason"="InvalidTLSConfig" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="spec.certificateAuthorityData is invalid: no certificates found" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -353,6 +372,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", @@ -390,6 +410,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="Get \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": unsupported protocol scheme \"\"" "issuer"="invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" "name"="test-name" "namespace"="test-namespace"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": unsupported protocol [truncated 9 chars]" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": unsupported protocol [truncated 9 chars]" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -398,6 +419,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", @@ -437,6 +459,7 @@ Get "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-na `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="Get \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": x509: certificate signed by unknown authority" "issuer"="` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" "name"="test-name" "namespace"="test-namespace"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": x509: certificate signed by unknown authority" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": x509: certificate signed by unknown authority" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -445,6 +468,7 @@ Get "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-na Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", @@ -483,6 +507,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to parse authorization endpoint URL: parse \"%\": invalid URL escape \"%\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse authorization endpoint URL: parse \"%\": invalid URL escape \"%\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -491,6 +516,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", @@ -528,6 +554,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL scheme must be \"https\", not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -536,6 +563,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", @@ -584,6 +612,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ &oidctestutil.TestUpstreamOIDCIdentityProvider{ @@ -603,6 +632,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "discovered issuer configuration"}, }, @@ -622,6 +652,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidConditionEarlier, {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, }, @@ -635,6 +666,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ &oidctestutil.TestUpstreamOIDCIdentityProvider{ @@ -654,6 +686,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", Conditions: []v1alpha1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, @@ -676,6 +709,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidConditionEarlier, {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, }, @@ -689,6 +723,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ &oidctestutil.TestUpstreamOIDCIdentityProvider{ @@ -708,6 +743,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", Conditions: []v1alpha1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, @@ -732,6 +768,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidConditionEarlier, {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, }, @@ -745,6 +782,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ &oidctestutil.TestUpstreamOIDCIdentityProvider{ @@ -764,6 +802,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Ready", Conditions: []v1alpha1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, }, @@ -841,6 +880,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="oidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "issuer"="` + testIssuerURL + `/ends-with-slash" "name"="test-name" "namespace"="test-namespace"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/ends-with-slash\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/ends-with-slash\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -849,6 +889,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", @@ -888,6 +929,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="oidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "issuer"="` + testIssuerURL + `/" "name"="test-name" "namespace"="test-namespace"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, @@ -896,6 +938,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs Status: v1alpha1.OIDCIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, { Type: "ClientCredentialsValid", Status: "True", diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 724ee0aa..30956524 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -156,9 +156,8 @@ func findOIDCProviderByNameAndValidateUID( for _, p := range providerCache.GetOIDCIdentityProviders() { if p.GetName() == s.ProviderName { if p.GetResourceUID() != s.ProviderUID { - return nil, errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Provider %q of type %q from upstream session data has changed its resource UID since authentication.", - s.ProviderName, s.ProviderType)) + return nil, errorsx.WithStack(errUpstreamRefreshError.WithHint( + "Provider from upstream session data has changed its resource UID since authentication.")) } return p, nil } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 968944d7..3b6be1ff 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1432,7 +1432,7 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Provider 'some-oidc-idp' of type 'oidc' from upstream session data has changed its resource UID since authentication." + "error_description": "Error during upstream refresh. Provider from upstream session data has changed its resource UID since authentication." } `), }, diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 46319490..fad28e10 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -813,7 +813,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype refreshed, err := upstreamOIDCIdentityProvider.PerformRefresh(ctx, refreshToken.Token) if err != nil { // Ignore errors during refresh, but return nil which will trigger the full login flow. - h.logger.V(debugLogLevel).Info("Pinniped: Refresh failed.") + h.logger.V(debugLogLevel).Info("Pinniped: Refresh failed.", "err", err) return nil, nil }