From 56d316e8d31b5fe4246b0f7856ff3a183aefe713 Mon Sep 17 00:00:00 2001 From: Mo Khan Date: Mon, 10 May 2021 00:22:34 -0400 Subject: [PATCH] upstreamwatcher: do not truncate explicit oidc errors This change makes it easier to understand misconfigurations caused by issuers with extraneous trailing slashes. Signed-off-by: Mo Khan --- .../upstreamwatcher/upstreamwatcher.go | 7 +- .../upstreamwatcher/upstreamwatcher_test.go | 165 ++++++++++++++++++ test/integration/supervisor_upstream_test.go | 32 ++++ 3 files changed, 201 insertions(+), 3 deletions(-) diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go index 6d04e37d..d996492f 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go @@ -13,6 +13,7 @@ import ( "net/http" "net/url" "sort" + "strings" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -279,7 +280,7 @@ func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.OIDC Type: typeOIDCDiscoverySucceeded, Status: v1alpha1.ConditionFalse, Reason: reasonUnreachable, - Message: fmt.Sprintf("failed to perform OIDC discovery against %q:\n%s", upstream.Spec.Issuer, truncateErr(err)), + Message: fmt.Sprintf("failed to perform OIDC discovery against %q:\n%s", upstream.Spec.Issuer, truncateNonOIDCErr(err)), } } @@ -426,11 +427,11 @@ func computeScopes(additionalScopes []string) []string { return scopes } -func truncateErr(err error) string { +func truncateNonOIDCErr(err error) string { const max = 100 msg := err.Error() - if len(msg) <= max { + if len(msg) <= max || strings.HasPrefix(msg, "oidc:") { return msg } diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go index 1b1a2219..164c08ed 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go @@ -602,6 +602,151 @@ Get "invalid-url-that-is-really-really-long/.well-known/openid-configuration": u }, }}, }, + { + name: "existing valid upstream with trailing slash", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/ends-with-slash/", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, + Claims: v1alpha1.OIDCClaims{Groups: testGroupsClaim, Username: testUsernameClaim}, + }, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, + }, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantLogs: []string{ + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ + &oidctestutil.TestUpstreamOIDCIdentityProvider{ + Name: testName, + ClientID: testClientID, + AuthorizationURL: *testIssuerAuthorizeURL, + Scopes: testExpectedScopes, + UsernameClaim: testUsernameClaim, + GroupsClaim: testGroupsClaim, + }, + }, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + {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}, + }, + }, + }}, + }, + { + name: "issuer is invalid URL, missing trailing slash", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/ends-with-slash", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `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"`, + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `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"`, + `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{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `failed to perform OIDC discovery against "` + testIssuerURL + `/ends-with-slash": +oidc: issuer did not match the issuer returned by provider, expected "` + testIssuerURL + `/ends-with-slash" got "` + testIssuerURL + `/ends-with-slash/"`, + }, + }, + }, + }}, + }, + { + name: "issuer is invalid URL, extra trailing slash", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `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"`, + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `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"`, + `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{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `failed to perform OIDC discovery against "` + testIssuerURL + `/": +oidc: issuer did not match the issuer returned by provider, expected "` + testIssuerURL + `/" got "` + testIssuerURL + `"`, + }, + }, + }, + }}, + }, } for _, tt := range tests { tt := tt @@ -730,5 +875,25 @@ func newTestIssuer(t *testing.T) (string, string) { }) }) + // handle the four issuer with trailing slash configs + + // valid case in= out= + // handled above at the root of testURL + + // valid case in=/ out=/ + mux.HandleFunc("/ends-with-slash/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&providerJSON{ + Issuer: testURL + "/ends-with-slash/", + AuthURL: "https://example.com/authorize", + }) + }) + + // invalid case in= out=/ + // can be tested using /ends-with-slash/ endpoint + + // invalid case in=/ out= + // can be tested using root endpoint + return caBundlePEM, testURL } diff --git a/test/integration/supervisor_upstream_test.go b/test/integration/supervisor_upstream_test.go index fd7a1908..6dfd5503 100644 --- a/test/integration/supervisor_upstream_test.go +++ b/test/integration/supervisor_upstream_test.go @@ -43,6 +43,38 @@ Get "https://127.0.0.1:444444/issuer/.well-known/openid-configuration": dial tcp }) }) + t.Run("invalid issuer with trailing slash", func(t *testing.T) { + t.Parallel() + spec := v1alpha1.OIDCIdentityProviderSpec{ + Issuer: env.SupervisorTestUpstream.Issuer + "/", + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorTestUpstream.CABundle)), + }, + AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{ + AdditionalScopes: []string{"email", "profile"}, + }, + Client: v1alpha1.OIDCClient{ + SecretName: library.CreateClientCredsSecret(t, "test-client-id", "test-client-secret").Name, + }, + } + upstream := library.CreateTestOIDCIdentityProvider(t, spec, v1alpha1.PhaseError) + expectUpstreamConditions(t, upstream, []v1alpha1.Condition{ + { + Type: "ClientCredentialsValid", + Status: v1alpha1.ConditionTrue, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: v1alpha1.ConditionFalse, + Reason: "Unreachable", + Message: `failed to perform OIDC discovery against "` + env.SupervisorTestUpstream.Issuer + `/": +oidc: issuer did not match the issuer returned by provider, expected "` + env.SupervisorTestUpstream.Issuer + `/" got "` + env.SupervisorTestUpstream.Issuer + `"`, + }, + }) + }) + t.Run("valid", func(t *testing.T) { t.Parallel() spec := v1alpha1.OIDCIdentityProviderSpec{