diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index f2d3aa9f..4669cdc3 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package oidcupstreamwatcher implements a controller which watches OIDCIdentityProviders. @@ -324,6 +324,11 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 } } + _, issuerURLCondition := validateHTTPSURL(upstream.Spec.Issuer, "issuer", reasonUnreachable) + if issuerURLCondition != nil { + return issuerURLCondition + } + discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer) if err != nil { const klogLevelTrace = 6 @@ -359,46 +364,35 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 } } if additionalDiscoveryClaims.RevocationEndpoint != "" { - // Found a revocation URL. Try to parse it. - revocationURL, err := url.Parse(additionalDiscoveryClaims.RevocationEndpoint) - if err != nil { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf("failed to parse revocation endpoint URL: %v", err), - } - } - // Don't want to send refresh tokens to an insecure revocation endpoint, so require that it use https. - if revocationURL.Scheme != "https" { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf(`revocation endpoint URL scheme must be "https", not %q`, revocationURL.Scheme), - } + // Found a revocation URL. Validate it. + revocationURL, revocationURLCondition := validateHTTPSURL( + additionalDiscoveryClaims.RevocationEndpoint, + "revocation endpoint", + reasonInvalidResponse, + ) + if revocationURLCondition != nil { + return revocationURLCondition } // Remember the URL for later use. result.RevocationURL = revocationURL } - // Parse out and validate the discovered authorize endpoint. - authURL, err := url.Parse(discoveredProvider.Endpoint().AuthURL) - if err != nil { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf("failed to parse authorization endpoint URL: %v", err), - } + _, authorizeURLCondition := validateHTTPSURL( + discoveredProvider.Endpoint().AuthURL, + "authorization endpoint", + reasonInvalidResponse, + ) + if authorizeURLCondition != nil { + return authorizeURLCondition } - if authURL.Scheme != "https" { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf(`authorization endpoint URL scheme must be "https", not %q`, authURL.Scheme), - } + + _, tokenURLCondition := validateHTTPSURL( + discoveredProvider.Endpoint().TokenURL, + "token endpoint", + reasonInvalidResponse, + ) + if tokenURLCondition != nil { + return tokenURLCondition } // If everything is valid, update the result and set the condition to true. @@ -489,3 +483,32 @@ func truncateMostLongErr(err error) string { return msg[:max] + fmt.Sprintf(" [truncated %d chars]", len(msg)-max) } + +func validateHTTPSURL(maybeHTTPSURL, endpointType, reason string) (*url.URL, *v1alpha1.Condition) { + parsedURL, err := url.Parse(maybeHTTPSURL) + if err != nil { + return nil, &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf("failed to parse %s URL: %v", endpointType, truncateMostLongErr(err)), + } + } + if parsedURL.Scheme != "https" { + return nil, &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf(`%s URL '%s' must have "https" scheme, not %q`, endpointType, maybeHTTPSURL, parsedURL.Scheme), + } + } + if len(parsedURL.Query()) != 0 || parsedURL.Fragment != "" { + return nil, &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf(`%s URL '%s' cannot contain query or fragment component`, endpointType, maybeHTTPSURL), + } + } + return parsedURL, nil +} diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 72fa284f..62e308e2 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidcupstreamwatcher @@ -399,7 +399,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Spec: v1alpha1.OIDCIdentityProviderSpec{ - Issuer: "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + Issuer: "%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", Client: v1alpha1.OIDCClient{SecretName: testSecretName}, }, }}, @@ -410,11 +410,10 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `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"="failed to parse issuer URL: parse \"%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": invalid URL escape \"%in\"" "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"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse issuer URL: parse \"%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": invalid URL escape \"%in\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -435,8 +434,145 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: "False", LastTransitionTime: now, Reason: "Unreachable", - Message: `failed to perform OIDC discovery against "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee": -Get "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration": unsupported protocol [truncated 9 chars]`, + Message: `failed to parse issuer URL: parse "%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee": invalid URL escape "%in"`, + }, + }, + }, + }}, + }, + { + name: "issuer is insecure http URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: strings.Replace(testIssuerURL, "https", "http", 1), + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + 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{ + `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"="issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have \"https\" scheme, not \"http\"" "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"="issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have "https" scheme, not "http"`, + }, + }, + }, + }}, + }, + { + name: "issuer contains a query param", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "?sub=foo", + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + 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{ + `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"="issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component" "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"="issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component`, + }, + }, + }, + }}, + }, + { + name: "issuer contains a fragment", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "#fragment", + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + 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{ + `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"="issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component" "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"="issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component`, }, }, }, @@ -603,9 +739,9 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), 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"="authorization endpoint URL 'http://example.com/authorize' must have \"https\" scheme, 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"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL 'http://example.com/authorize' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -626,7 +762,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: "False", LastTransitionTime: now, Reason: "InvalidResponse", - Message: `authorization endpoint URL scheme must be "https", not "http"`, + Message: `authorization endpoint URL 'http://example.com/authorize' must have "https" scheme, not "http"`, }, }, }, @@ -650,9 +786,9 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), 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"="revocation 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"="revocation endpoint URL 'http://example.com/revoke' must have \"https\" scheme, 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"="revocation endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="revocation endpoint URL 'http://example.com/revoke' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -673,7 +809,148 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: "False", LastTransitionTime: now, Reason: "InvalidResponse", - Message: `revocation endpoint URL scheme must be "https", not "http"`, + Message: `revocation endpoint URL 'http://example.com/revoke' must have "https" scheme, not "http"`, + }, + }, + }, + }}, + }, + { + name: "issuer returns insecure token URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/insecure-token-url", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + 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{ + `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"="token endpoint URL 'http://example.com/token' must have \"https\" scheme, 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"="token endpoint URL 'http://example.com/token' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidResponse", + Message: `token endpoint URL 'http://example.com/token' must have "https" scheme, not "http"`, + }, + }, + }, + }}, + }, + { + name: "issuer returns no token URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/missing-token-url", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + 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{ + `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"="token endpoint URL '' must have \"https\" scheme, not \"\"" "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"="token endpoint URL '' must have \"https\" scheme, not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidResponse", + Message: `token endpoint URL '' must have "https" scheme, not ""`, + }, + }, + }, + }}, + }, + { + name: "issuer returns no auth URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/missing-auth-url", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + 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{ + `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 '' must have \"https\" scheme, not \"\"" "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 '' must have \"https\" scheme, not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidResponse", + Message: `authorization endpoint URL '' must have "https" scheme, not ""`, }, }, }, @@ -1253,6 +1530,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL, AuthURL: "https://example.com/authorize", RevocationURL: "https://example.com/revoke", + TokenURL: "https://example.com/token", }) }) @@ -1263,6 +1541,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL + "/valid-without-revocation", AuthURL: "https://example.com/authorize", RevocationURL: "", // none + TokenURL: "https://example.com/token", }) }) @@ -1270,8 +1549,9 @@ func newTestIssuer(t *testing.T) (string, string) { mux.HandleFunc("/invalid/.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 + "/invalid", - AuthURL: "%", + Issuer: testURL + "/invalid", + AuthURL: "%", + TokenURL: "https://example.com/token", }) }) @@ -1282,6 +1562,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL + "/invalid-revocation-url", AuthURL: "https://example.com/authorize", RevocationURL: "%", + TokenURL: "https://example.com/token", }) }) @@ -1289,18 +1570,52 @@ func newTestIssuer(t *testing.T) (string, string) { mux.HandleFunc("/insecure/.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 + "/insecure", - AuthURL: "http://example.com/authorize", + Issuer: testURL + "/insecure", + AuthURL: "http://example.com/authorize", + TokenURL: "https://example.com/token", }) }) - // At "/insecure", serve an issuer that returns an insecure authorization URL (not https://). + // At "/insecure-revocation-url", serve an issuer that returns an insecure revocation URL (not https://). mux.HandleFunc("/insecure-revocation-url/.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 + "/insecure-revocation-url", AuthURL: "https://example.com/authorize", RevocationURL: "http://example.com/revoke", + TokenURL: "https://example.com/token", + }) + }) + + // At "/insecure-token-url", serve an issuer that returns an insecure token URL (not https://). + mux.HandleFunc("/insecure-token-url/.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 + "/insecure-token-url", + AuthURL: "https://example.com/authorize", + RevocationURL: "https://example.com/revoke", + TokenURL: "http://example.com/token", + }) + }) + + // At "/missing-token-url", serve an issuer that returns no token URL (is required by the spec unless it's an idp which only supports + // implicit flow, which we don't support). So for our purposes we need to always get a token url + mux.HandleFunc("/missing-token-url/.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 + "/missing-token-url", + AuthURL: "https://example.com/authorize", + RevocationURL: "https://example.com/revoke", + }) + }) + + // At "/missing-auth-url", serve an issuer that returns no auth URL, which is required by the spec. + mux.HandleFunc("/missing-auth-url/.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 + "/missing-auth-url", + RevocationURL: "https://example.com/revoke", + TokenURL: "https://example.com/token", }) }) @@ -1316,6 +1631,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL + "/ends-with-slash/", AuthURL: "https://example.com/authorize", RevocationURL: "https://example.com/revoke", + TokenURL: "https://example.com/token", }) }) diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index 2623ea5f..5b503371 100644 --- a/internal/fositestorage/accesstoken/accesstoken_test.go +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package accesstoken @@ -53,7 +53,7 @@ func TestAccessTokenStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/access-token", @@ -122,7 +122,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/access-token", diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 1d678ec5..e7c6655b 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package authorizationcode @@ -370,29 +370,33 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ "providerName": "ʼn2ƋŢ觛ǂ焺nŐǛ", "providerType": "ɥ闣ʬ橳(ý綃ʃʚƟ覣k眐4", "oidc": { - "upstreamRefreshToken": "tC嵽痊w" + "upstreamRefreshToken": "tC嵽痊w", + "upstreamSubject": "a紽ǒ|鰽ŋ猊I", + "upstreamIssuer": "妬\u003e6鉢緋uƴŤȱʀ" }, "ldap": { - "userDN": "Ź榨Q|ôɵt毇妬\u003e6鉢緋", + "userDN": "Â?墖\u003cƬb獭潜Ʃ饾k|鬌R蜚蠣", "extraRefreshAttributes": { - "ď逳鞪?3)藵睋邔\u0026Ű惫蜀Ģ¡圔": "墀jMʥ", - "ƍ蛊ʚ£:設虝27": "b獭潜Ʃ饾k|鬌R蜚蠣麹概", - "藚ɏ¬Ê蒭堜]ȗ韚ʫ": "鷞aŚB碠k9帴ʘ赱" + "ȱ藚ɏ¬Ê蒭堜]ȗ韚ʫ": "鷞aŚB碠k9帴ʘ赱" } }, "activedirectory": { - "userDN": "0D餹sêĝɓ", + "userDN": "瑹xȢ~1Įx欼笝?úT妼", "extraRefreshAttributes": { - "摱ì": "bEǎ儯惝Io" + "iYn": "麹Œ颛", + "İ\u003e×1飞O+î艔垎0OƉǢIȽ齤士": "ȐĨf跞@)¿,ɭS隑ip偶" } } } }, "requestedAudience": [ - "Ł" + "應,Ɣ鬅X¤", + "¤.岵骘胲ƤkǦ" ], "grantedAudience": [ - "r" + "鸖I¶媁y衑拁Ȃ", + "社Vƅȭǝ*擦28Dž 甍 ć", + "bņ抰蛖a³2ʫ承dʬ)ġ,TÀqy_" ] }, "version": "2" diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 006ed61f..72c4cfba 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package authorizationcode @@ -65,7 +65,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/authcode", @@ -84,7 +84,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/authcode", @@ -389,6 +389,7 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { validSessionJSONBytes, err := json.MarshalIndent(validSession, "", "\t") require.NoError(t, err) authorizeCodeSessionJSONFromFuzzing := string(validSessionJSONBytes) + t.Log(authorizeCodeSessionJSONFromFuzzing) // the fuzzed session and storage session should have identical JSON require.JSONEq(t, authorizeCodeSessionJSONFromFuzzing, authorizeCodeSessionJSONFromStorage) diff --git a/internal/fositestorage/openidconnect/openidconnect_test.go b/internal/fositestorage/openidconnect/openidconnect_test.go index 1ceafa9e..ac680a60 100644 --- a/internal/fositestorage/openidconnect/openidconnect_test.go +++ b/internal/fositestorage/openidconnect/openidconnect_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package openidconnect @@ -52,7 +52,7 @@ func TestOpenIdConnectStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/oidc", diff --git a/internal/fositestorage/pkce/pkce_test.go b/internal/fositestorage/pkce/pkce_test.go index e21dda33..b84798ce 100644 --- a/internal/fositestorage/pkce/pkce_test.go +++ b/internal/fositestorage/pkce/pkce_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package pkce @@ -52,7 +52,7 @@ func TestPKCEStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/pkce", diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index ae7a1226..ce74793a 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package refreshtoken @@ -52,7 +52,7 @@ func TestRefreshTokenStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/refresh-token", @@ -122,7 +122,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/refresh-token", diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 046f1849..2a2a689e 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -14,11 +14,12 @@ import ( reflect "reflect" gomock "github.com/golang/mock/gomock" + oauth2 "golang.org/x/oauth2" + types "k8s.io/apimachinery/pkg/types" + nonce "go.pinniped.dev/pkg/oidcclient/nonce" oidctypes "go.pinniped.dev/pkg/oidcclient/oidctypes" pkce "go.pinniped.dev/pkg/oidcclient/pkce" - oauth2 "golang.org/x/oauth2" - types "k8s.io/apimachinery/pkg/types" ) // MockUpstreamOIDCIdentityProviderI is a mock of UpstreamOIDCIdentityProviderI interface. @@ -230,16 +231,16 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0 } // ValidateToken mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce) (*oidctypes.Token, error) { +func (m *MockUpstreamOIDCIdentityProviderI) ValidateTokenAndMergeWithUserInfo(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ValidateToken", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "ValidateTokenAndMergeWithUserInfo", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*oidctypes.Token) ret1, _ := ret[1].(error) return ret0, ret1 } // ValidateToken indicates an expected call of ValidateToken. -func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateToken), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateTokenAndMergeWithUserInfo", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateTokenAndMergeWithUserInfo), arg0, arg1, arg2, arg3) } diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index f9760825..b08d39bc 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package auth provides a handler for the OIDC authorization endpoint. @@ -191,6 +191,20 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, ) } + upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + // Return a user-friendly error for this case which is entirely within our control. + return writeAuthorizeError(w, oauthHelper, authorizeRequester, + fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, + ) + } + upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + // Return a user-friendly error for this case which is entirely within our control. + return writeAuthorizeError(w, oauthHelper, authorizeRequester, + fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, + ) + } customSessionData := &psession.CustomSessionData{ ProviderUID: oidcUpstream.GetResourceUID(), @@ -198,6 +212,8 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: token.RefreshToken.Token, + UpstreamIssuer: upstreamIssuer, + UpstreamSubject: upstreamSubject, }, } return makeDownstreamSessionAndReturnAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, subject, username, groups, customSessionData) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 274ca0e9..845e39b4 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package auth @@ -470,6 +470,8 @@ func TestAuthorizationEndpoint(t *testing.T) { ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: oidcPasswordGrantUpstreamRefreshToken, + UpstreamSubject: oidcUpstreamSubject, + UpstreamIssuer: oidcUpstreamIssuer, }, } diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 22f37a55..1a5a3aee 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package callback provides a handler for the OIDC callback endpoint. @@ -83,12 +83,23 @@ func NewHandler( return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } + upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) + if err != nil { + return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) + } + upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) + if err != nil { + return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) + } + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, &psession.CustomSessionData{ ProviderUID: upstreamIDPConfig.GetResourceUID(), ProviderName: upstreamIDPConfig.GetName(), ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: token.RefreshToken.Token, + UpstreamSubject: upstreamSubject, + UpstreamIssuer: upstreamIssuer, }, }) diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 48e04afd..d14c159f 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package callback @@ -77,7 +77,11 @@ var ( ProviderUID: happyUpstreamIDPResourceUID, ProviderName: happyUpstreamIDPName, ProviderType: psession.ProviderTypeOIDC, - OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamRefreshToken}, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: oidcUpstreamRefreshToken, + UpstreamIssuer: oidcUpstreamIssuer, + UpstreamSubject: oidcUpstreamSubject, + }, } ) diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 5f0cf5f0..265ed5c1 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package downstreamsession provides some shared helpers for creating downstream OIDC sessions. @@ -89,11 +89,11 @@ func getSubjectAndUsernameFromUpstreamIDToken( ) (string, string, error) { // The spec says the "sub" claim is only unique per issuer, // so we will prepend the issuer string to make it globally unique. - upstreamIssuer, err := extractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), idTokenClaims) + upstreamIssuer, err := ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), idTokenClaims) if err != nil { return "", "", err } - upstreamSubject, err := extractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), idTokenClaims) + upstreamSubject, err := ExtractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), idTokenClaims) if err != nil { return "", "", err } @@ -128,7 +128,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( } } - username, err := extractStringClaimValue(usernameClaimName, upstreamIDPConfig.GetName(), idTokenClaims) + username, err := ExtractStringClaimValue(usernameClaimName, upstreamIDPConfig.GetName(), idTokenClaims) if err != nil { return "", "", err } @@ -136,7 +136,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( return subject, username, nil } -func extractStringClaimValue(claimName string, upstreamIDPName string, idTokenClaims map[string]interface{}) (string, error) { +func ExtractStringClaimValue(claimName string, upstreamIDPName string, idTokenClaims map[string]interface{}) (string, error) { value, ok := idTokenClaims[claimName] if !ok { plog.Warning( diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 2c34dcbe..24b821e7 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package provider @@ -71,10 +71,10 @@ type UpstreamOIDCIdentityProviderI interface { // RevokeRefreshToken will attempt to revoke the given token, if the provider has a revocation endpoint. RevokeRefreshToken(ctx context.Context, refreshToken string) error - // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response + // ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response // into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated // tokens, or an error. - ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) + ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) } type UpstreamLDAPIdentityProviderI interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index afaee0b1..e0fc4408 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package token provides a handler for the OIDC token endpoint. @@ -6,6 +6,7 @@ package token import ( "context" + "errors" "net/http" "github.com/ory/fosite" @@ -88,7 +89,7 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, switch customSessionData.ProviderType { case psession.ProviderTypeOIDC: - return upstreamOIDCRefresh(ctx, customSessionData, providerCache) + return upstreamOIDCRefresh(ctx, session, providerCache) case psession.ProviderTypeLDAP: return upstreamLDAPRefresh(ctx, providerCache, session) case psession.ProviderTypeActiveDirectory: @@ -98,7 +99,8 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, } } -func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister) error { +func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, providerCache oidc.UpstreamIdentityProvidersLister) error { + s := session.Custom if s.OIDC == nil || s.OIDC.UpstreamRefreshToken == "" { return errorsx.WithStack(errMissingUpstreamSessionInternalError) } @@ -122,17 +124,43 @@ func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, pro // "the response body is the Token Response of Section 3.1.3.3 except that it might not contain an id_token." // https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse _, hasIDTok := refreshedTokens.Extra("id_token").(string) - if hasIDTok { - // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at - // least some providers do not include one, so we skip the nonce validation here (but not other validations). - _, err = p.ValidateToken(ctx, refreshedTokens, "") - if err != nil { + + // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at + // least some providers do not include one, so we skip the nonce validation here (but not other validations). + validatedTokens, err := p.ValidateTokenAndMergeWithUserInfo(ctx, refreshedTokens, "", hasIDTok) + if err != nil { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh returned an invalid ID token or UserInfo response.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + + claims := validatedTokens.IDToken.Claims + // if we have any claims at all, we better have a subject, and it better match the previous value. + // but it's possible that we don't because both returning a new refresh token on refresh and having a userinfo + // endpoint are optional. + if len(validatedTokens.IDToken.Claims) != 0 { //nolint:nestif + newSub, hasSub := getString(claims, oidc.IDTokenSubjectClaim) + if !hasSub { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh returned an invalid ID token.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh not found")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + if s.OIDC.UpstreamSubject != newSub { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + usernameClaim := p.GetUsernameClaim() + newUsername, hasUsername := getString(claims, usernameClaim) + oldUsername := session.Fosite.Claims.Extra[oidc.DownstreamUsernameClaim] + // its possible this won't be returned. + // but if it is, verify that it hasn't changed. + if hasUsername && oldUsername != newUsername { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + newIssuer, hasIssuer := getString(claims, oidc.IDTokenIssuerClaim) + if hasIssuer && s.OIDC.UpstreamIssuer != newIssuer { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("issuer in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } - } else { - plog.Debug("upstream refresh request did not return a new ID token", - "providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID) } // Upstream refresh may or may not return a new refresh token. If we got a new refresh token, then update it in @@ -147,6 +175,11 @@ func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, pro return nil } +func getString(m map[string]interface{}, key string) (string, bool) { + val, ok := m[key].(string) + return val, ok +} + func findOIDCProviderByNameAndValidateUID( s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister, diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 0c021de1..2a8f039a 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package token @@ -52,10 +52,12 @@ import ( "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" + "go.pinniped.dev/pkg/oidcclient/oidctypes" ) const ( goodIssuer = "https://some-issuer.com" + goodUpstreamSubject = "some-subject" goodClient = "pinniped-cli" goodRedirectURI = "http://127.0.0.1/callback" goodPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" @@ -909,6 +911,8 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: oidcUpstreamInitialRefreshToken, + UpstreamSubject: goodUpstreamSubject, + UpstreamIssuer: goodIssuer, }, } } @@ -981,7 +985,6 @@ func TestRefreshGrant(t *testing.T) { want := happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(wantCustomSessionDataStored) // Should always try to perform an upstream refresh. want.wantUpstreamRefreshCall = happyOIDCUpstreamRefreshCall() - // Should only try to ValidateToken when there was an id token returned by the upstream refresh. if expectToValidateToken != nil { want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken) } @@ -1046,7 +1049,37 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant with openid scope granted (id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( + upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + refreshedUpstreamTokensWithIDAndRefreshTokens(), + ), + }, + }, + { + name: "refresh grant with unchanged username claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": goodUpstreamSubject, + "username-claim": goodUsername, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1062,7 +1095,11 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant without openid scope granted (no id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, @@ -1089,7 +1126,11 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant when the upstream refresh does not return a new ID token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1098,14 +1139,20 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), - nil, // expect ValidateToken is *not* called + refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateTokenAndMergeWithUserInfo is called ), }, }, { name: "happy path refresh grant when the upstream refresh does not return a new refresh token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1121,7 +1168,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request adds a new scope to the list of requested scopes then it is ignored", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1140,7 +1193,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request removes a scope which was originally granted from the list of requested scopes then it is granted anyway", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience") }, @@ -1170,7 +1229,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request does not include a scope param then it gets all the same scopes as the original authorization request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": goodUpstreamSubject, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1529,7 +1594,7 @@ func TestRefreshGrant(t *testing.T) { name: "when the upstream refresh returns an invalid ID token during the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). - // This is the current format of the errors returned by the production code version of ValidateToken, see ValidateToken in upstreamoidc.go + // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go WithValidateTokenError(httperr.Wrap(http.StatusBadRequest, "some validate error", errors.New("some validate cause"))). Build()), authcodeExchange: authcodeExchangeInputs{ @@ -1545,7 +1610,130 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token." + "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token or UserInfo response." + } + `), + }, + }, + }, + { + name: "when the upstream refresh returns an ID token with a different subject than the original", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). + WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). + // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go + WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "something-different", + }, + }, + }). + Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, + { + name: "refresh grant with claims but not the subject claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, + { + name: "refresh grant with changed username claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": goodUpstreamSubject, + "username-claim": "some-changed-username", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, + { + name: "refresh grant with changed issuer claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": goodUpstreamSubject, + "iss": "some-changed-issuer", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." } `), }, diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index d7fd47df..bbcc4317 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package psession @@ -62,6 +62,8 @@ const ( // OIDCSessionData is the additional data needed by Pinniped when the upstream IDP is an OIDC provider. type OIDCSessionData struct { UpstreamRefreshToken string `json:"upstreamRefreshToken"` + UpstreamSubject string `json:"upstreamSubject"` + UpstreamIssuer string `json:"upstreamIssuer"` } // LDAPSessionData is the additional data needed by Pinniped when the upstream IDP is an LDAP provider. diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 88e54a73..aa682081 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -83,6 +83,12 @@ type ValidateTokenArgs struct { ExpectedIDTokenNonce nonce.Nonce } +type ValidateRefreshArgs struct { + Ctx context.Context + Tok *oauth2.Token + StoredAttributes provider.StoredRefreshAttributes +} + type TestUpstreamLDAPIdentityProvider struct { Name string ResourceUID types.UID @@ -312,7 +318,7 @@ func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenArgs(call int) *Rev return u.revokeRefreshTokenArgs[call] } -func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { +func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { if u.validateTokenArgs == nil { u.validateTokenArgs = make([]*ValidateTokenArgs, 0) } @@ -533,10 +539,10 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken( } } require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, - "should have been exactly one call to ValidateToken() by all OIDC upstreams", + "should have been exactly one call to ValidateTokenAndMergeWithUserInfo() by all OIDC upstreams", ) require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall, - "ValidateToken() was called on the wrong OIDC upstream", + "ValidateTokenAndMergeWithUserInfo() was called on the wrong OIDC upstream", ) require.Equal(t, expectedArgs, actualArgs) } @@ -548,7 +554,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.validateTokenCallCount } require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, - "expected exactly zero calls to ValidateToken()", + "expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()", ) } diff --git a/internal/testutil/psession.go b/internal/testutil/psession.go index 28e65968..83aacb13 100644 --- a/internal/testutil/psession.go +++ b/internal/testutil/psession.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testutil @@ -29,6 +29,8 @@ func NewFakePinnipedSession() *psession.PinnipedSession { ProviderName: "fake-provider-name", OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: "fake-upstream-refresh-token", + UpstreamSubject: "some-subject", + UpstreamIssuer: "some-issuer", }, }, } diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 437eb6ef..b9d371ed 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package upstreamoidc implements an abstraction of upstream OIDC provider interactions. @@ -12,6 +12,7 @@ import ( "net/http" "net/url" "strings" + "time" coreosoidc "github.com/coreos/go-oidc/v3/oidc" "golang.org/x/oauth2" @@ -112,7 +113,7 @@ func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.C // There is no nonce to validate for a resource owner password credentials grant because it skips using // the authorize endpoint and goes straight to the token endpoint. const skipNonceValidation nonce.Nonce = "" - return p.ValidateToken(ctx, tok, skipNonceValidation) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true) } func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) { @@ -126,7 +127,7 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, return nil, err } - return p.ValidateToken(ctx, tok, expectedIDTokenNonce) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true) } func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) { @@ -236,36 +237,27 @@ func (p *ProviderConfig) tryRevokeRefreshToken( } } -// ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, +// ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. -func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { - idTok, hasIDTok := tok.Extra("id_token").(string) - if !hasIDTok { - return nil, httperr.New(http.StatusBadRequest, "received response missing ID token") - } - validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) +func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { + var validatedClaims = make(map[string]interface{}) + + var idTokenExpiry time.Time + // if we require the id token, make sure we have it. + // also, if it exists but wasn't required, still make sure it passes these checks. + idTokenExpiry, idTok, err := p.validateIDToken(ctx, tok, expectedIDTokenNonce, validatedClaims, requireIDToken) if err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - if validated.AccessTokenHash != "" { - if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - } - if expectedIDTokenNonce != "" { - if err := expectedIDTokenNonce.Validate(validated); err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) - } + return nil, err } - var validatedClaims map[string]interface{} - if err := validated.Claims(&validatedClaims); err != nil { - return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) - } - maybeLogClaims("claims from ID token", p.Name, validatedClaims) + idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) - if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { - return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) + if len(idTokenSubject) > 0 || !requireIDToken { + // only fetch userinfo if the ID token has a subject or if we are ignoring the id token completely. + // otherwise, defer to existing ID token validation + if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken); err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) + } } return &oidctypes.Token{ @@ -279,58 +271,111 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e }, IDToken: &oidctypes.IDToken{ Token: idTok, - Expiry: metav1.NewTime(validated.Expiry), + Expiry: metav1.NewTime(idTokenExpiry), Claims: validatedClaims, }, }, nil } -func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}) error { +func (p *ProviderConfig) validateIDToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, validatedClaims map[string]interface{}, requireIDToken bool) (time.Time, string, error) { + idTok, hasIDTok := tok.Extra("id_token").(string) + if !hasIDTok && !requireIDToken { + return time.Time{}, "", nil // exit early + } + + var idTokenExpiry time.Time + if !hasIDTok { + return time.Time{}, "", httperr.New(http.StatusBadRequest, "received response missing ID token") + } + validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) + if err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } + if validated.AccessTokenHash != "" { + if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } + } + if expectedIDTokenNonce != "" { + if err := expectedIDTokenNonce.Validate(validated); err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) + } + } + if err := validated.Claims(&validatedClaims); err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) + } + maybeLogClaims("claims from ID token", p.Name, validatedClaims) + idTokenExpiry = validated.Expiry // keep track of the id token expiry if we have an id token. Otherwise, it'll just be the zero value. + return idTokenExpiry, idTok, nil +} + +func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool) error { idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string) - if len(idTokenSubject) == 0 { - return nil // defer to existing ID token validation - } - providerJSON := &struct { - UserInfoURL string `json:"userinfo_endpoint"` - }{} - if err := p.Provider.Claims(providerJSON); err != nil { - // this should never happen because we should have already parsed these claims at an earlier stage - return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err) + userInfo, err := p.maybeFetchUserInfo(ctx, tok) + if err != nil { + return err } - - // implementing the user info endpoint is not required, skip this logic when it is absent - if len(providerJSON.UserInfoURL) == 0 { + if userInfo == nil { return nil } - userInfo, err := p.Provider.UserInfo(coreosoidc.ClientContext(ctx, p.Client), oauth2.StaticTokenSource(tok)) - if err != nil { - return httperr.Wrap(http.StatusInternalServerError, "could not get user info", err) - } - // The sub (subject) Claim MUST always be returned in the UserInfo Response. - // // NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not // guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in // the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, // the UserInfo Response values MUST NOT be used. // // http://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse - if len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject { + // If there is no ID token and it is not required, we must assume that the caller is performing other checks + // to ensure the subject is correct. + checkIDToken := requireIDToken || len(idTokenSubject) > 0 + if checkIDToken && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) { return httperr.Newf(http.StatusUnprocessableEntity, "userinfo 'sub' claim (%s) did not match id_token 'sub' claim (%s)", userInfo.Subject, idTokenSubject) } + // keep track of the issuer from the ID token + idTokenIssuer := claims["iss"] + // merge existing claims with user info claims if err := userInfo.Claims(&claims); err != nil { return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal user info claims", err) } + // The OIDC spec for the UserInfo response does not make any guarantees about the iss claim's existence or validity: + // "If signed, the UserInfo Response SHOULD contain the Claims iss (issuer) and aud (audience) as members. The iss value SHOULD be the OP's Issuer Identifier URL." + // See https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse + // So we just ignore it and use it the version from the id token, which has stronger guarantees. + delete(claims, "iss") + if idTokenIssuer != nil { + claims["iss"] = idTokenIssuer + } maybeLogClaims("claims from ID token and userinfo", p.Name, claims) return nil } +func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token) (*coreosoidc.UserInfo, error) { + providerJSON := &struct { + UserInfoURL string `json:"userinfo_endpoint"` + }{} + if err := p.Provider.Claims(providerJSON); err != nil { + // this should never happen because we should have already parsed these claims at an earlier stage + return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err) + } + + // implementing the user info endpoint is not required, skip this logic when it is absent + if len(providerJSON.UserInfoURL) == 0 { + return nil, nil + } + + userInfo, err := p.Provider.UserInfo(coreosoidc.ClientContext(ctx, p.Client), oauth2.StaticTokenSource(tok)) + if err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "could not get user info", err) + } + return userInfo, nil +} + func maybeLogClaims(msg, name string, claims map[string]interface{}) { if plog.Enabled(plog.LevelAll) { // log keys and values at all level data, _ := json.Marshal(claims) // nothing we can do if it fails, but it really never should diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index f8906562..90dc6f1f 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamoidc @@ -584,8 +584,327 @@ func TestProviderConfig(t *testing.T) { if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) + } + }) + } + }) + + t.Run("ValidateTokenAndMergeWithUserInfo", func(t *testing.T) { + expiryTime := time.Now().Add(42 * time.Second) + testTokenWithoutIDToken := &oauth2.Token{ + AccessToken: "test-access-token", + // the library sets the original refresh token into the result, even though the server did not return that + RefreshToken: "test-initial-refresh-token", + TokenType: "test-token-type", + Expiry: expiryTime, + } + // generated from jwt.io + // sub: some-subject + // iss: some-issuer + // nonce: some-nonce + goodIDToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJub25jZSI6InNvbWUtbm9uY2UiLCJpc3MiOiJzb21lLWlzc3VlciJ9.eGvzOihLUqzn3M4k6fHsToedgy7Fu89_Xu_u4mwMgRlIyRWZqmEMV76RVLnZd9Ihm9j_VpvrpirIkaj4JM9eRNfLX1n328cmBivBwnTKAzHuTm17dUKO5EvdTmQzmwnN0WZ8nWk4GfR7RzcvE1V8G9tIiWD8FkO3Dr-NR_zTun3N37onAazVLCmF0SDtATDfUH1ETqviHEp8xGx5HD5mv5T3HEjOuer5gxTEnfncef0LurBH3po-C0tXHKu74PD8x88CMJ1DLsRdCalnctwa850slKPkBSTP-ssh0JVg7cdMXoosVpwiXtKYaBkrhu8VS018aFP-cBbW0mYwsHmt3g" //nolint:gosec + + tests := []struct { + name string + tok *oauth2.Token + nonce nonce.Nonce + requireIDToken bool + userInfo *oidc.UserInfo + rawClaims []byte + userInfoErr error + wantErr string + wantMergedTokens *oidctypes.Token + }{ + { + name: "token with id, access and refresh tokens, valid nonce, and no userinfo", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + }, + }, + }, + }, + { + name: "id token not required but is provided", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, + }, + { + name: "token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, + }, + { + name: "claims from userinfo override id token claims", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA", + Claims: map[string]interface{}{ + "iss": "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable. + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, + }, + { + name: "token with id, access and refresh tokens and valid nonce, but userinfo has a different issuer", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable. + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, + }, + { + name: "token with no id token but valid userinfo", + tok: testTokenWithoutIDToken, + nonce: "", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "", + Claims: map[string]interface{}{ + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, + }, + { + name: "token with neither id token nor userinfo", + tok: testTokenWithoutIDToken, + nonce: "", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }, + }, + { + name: "token with id, access and refresh tokens, valid nonce, and userinfo subject that doesn't match", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), + wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + }, + { + name: "id token not required but is provided, and subjects don't match", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), + wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + }, + { + name: "invalid id token", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-an-id-token"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), + wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", + }, + { + name: "invalid nonce", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), + wantErr: "received ID token with invalid nonce: invalid nonce (expected \"some-other-nonce\", got \"some-nonce\")", + }, + { + name: "expected to have id token, but doesn't", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), + wantErr: "received response missing ID token", + }, + { + name: "mismatched access token hash", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), + wantErr: "received response missing ID token", + }, + { + name: "id token missing subject, skip userinfo check", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ", + Claims: map[string]interface{}{ + "iss": "some-issuer", + "name": "John Doe", + "nonce": "some-nonce", + }, + }, + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + p := ProviderConfig{ + Name: "test-name", + UsernameClaim: "test-username-claim", + GroupsClaim: "test-groups-claim", + Config: &oauth2.Config{ + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://example.com", + TokenURL: "https://example.com", + AuthStyle: oauth2.AuthStyleInParams, + }, + Scopes: []string{"scope1", "scope2"}, + }, + Provider: &mockProvider{ + rawClaims: tt.rawClaims, + userInfo: tt.userInfo, + userInfoErr: tt.userInfoErr, + }, + } + gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) + if tt.wantErr != "" { + require.Error(t, err) + require.Equal(t, tt.wantErr, err.Error()) } else { require.NoError(t, err) + require.Equal(t, tt.wantMergedTokens, gotTok) } }) } diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 470ed3dd..325ec4a4 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -822,7 +822,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at least // some providers do not include one, so we skip the nonce validation here (but not other validations). - return upstreamOIDCIdentityProvider.ValidateToken(ctx, refreshed, "") + return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true) } func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 2bf4620c..e55f8e03 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -406,7 +406,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). @@ -453,7 +453,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(nil, fmt.Errorf("some validation error")) mock.EXPECT(). PerformRefresh(gomock.Any(), "test-refresh-token-returning-invalid-id-token"). @@ -1648,7 +1648,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index b10f650e..29a46e79 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -87,10 +87,8 @@ func TestSupervisorLogin(t *testing.T) { }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { - customSessionData := pinnipedSession.Custom - require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) - require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) - customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" + pinnipedSessionData := pinnipedSession.Custom + pinnipedSessionData.OIDC.UpstreamIssuer = "wrong-issuer" }, // the ID token Subject should include the upstream user ID after the upstream issuer name wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", @@ -124,10 +122,8 @@ func TestSupervisorLogin(t *testing.T) { }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { - customSessionData := pinnipedSession.Custom - require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) - require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) - customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Extra["username"] = "some-incorrect-username" }, wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" },