diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 0e4399ea..38b4119c 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -351,6 +351,44 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 c.validatorCache.putProvider(&upstream.Spec, discoveredProvider, httpClient) } + // Get the revocation endpoint, if there is one. Many providers do not offer a revocation endpoint. + var additionalDiscoveryClaims struct { + // "revocation_endpoint" is specified by https://datatracker.ietf.org/doc/html/rfc8414#section-2 + RevocationEndpoint string `json:"revocation_endpoint"` + } + if err := discoveredProvider.Claims(&additionalDiscoveryClaims); err != nil { + // This shouldn't actually happen because the above call to NewProvider() would have already returned this error. + return &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reasonInvalidResponse, + Message: fmt.Sprintf("failed to unmarshal OIDC discovery response from %q:\n%s", upstream.Spec.Issuer, truncateMostLongErr(err)), + } + } + 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), + } + } + // 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 { diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 57942c02..e37b6435 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -114,6 +114,8 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { testIssuerCABase64 := base64.StdEncoding.EncodeToString([]byte(testIssuerCA)) testIssuerAuthorizeURL, err := url.Parse("https://example.com/authorize") require.NoError(t, err) + testIssuerRevocationURL, err := url.Parse("https://example.com/revoke") + require.NoError(t, err) wrongCA, err := certauthority.New("foo", time.Hour) require.NoError(t, err) @@ -151,7 +153,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { inputSecrets []runtime.Object wantErr string wantLogs []string - wantResultingCache []provider.UpstreamOIDCIdentityProviderI + wantResultingCache []*oidctestutil.TestUpstreamOIDCIdentityProvider wantResultingUpstreams []v1alpha1.OIDCIdentityProvider }{ { @@ -175,7 +177,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="secret \"test-client-secret\" not found" "name"="test-name" "namespace"="test-namespace" "reason"="SecretNotFound" "type"="ClientCredentialsValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -222,7 +224,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "name"="test-name" "namespace"="test-namespace" "reason"="SecretWrongType" "type"="ClientCredentialsValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -268,7 +270,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "name"="test-name" "namespace"="test-namespace" "reason"="SecretMissingKeys" "type"="ClientCredentialsValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -317,7 +319,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -366,7 +368,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="spec.certificateAuthorityData is invalid: no certificates found" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -413,7 +415,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": unsupported protocol [truncated 9 chars]" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -462,7 +464,7 @@ Get "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-na `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"` + testIssuerURL + `/valid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": x509: certificate signed by unknown authority" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -510,7 +512,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse authorization endpoint URL: parse \"%\": invalid URL escape \"%\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -535,6 +537,53 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }, }}, }, + { + name: "issuer returns invalid revocation URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/invalid-revocation-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"="failed to parse revocation endpoint URL: parse \"%\": invalid URL escape \"%\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse revocation endpoint URL: parse \"%\": invalid URL escape \"%\"" "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: `failed to parse revocation endpoint URL: parse "%": invalid URL escape "%"`, + }, + }, + }, + }}, + }, { name: "issuer returns insecure authorize URL", inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ @@ -557,7 +606,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -582,6 +631,53 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }, }}, }, + { + name: "issuer returns insecure revocation URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/insecure-revocation-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"="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"="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"`, + }, + 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: `revocation endpoint URL scheme must be "https", not "http"`, + }, + }, + }, + }}, + }, { name: "upstream with error becomes valid", inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ @@ -614,11 +710,12 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ - &oidctestutil.TestUpstreamOIDCIdentityProvider{ + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { Name: testName, ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: testIssuerRevocationURL, Scopes: append(testExpectedScopes, "xyz"), // includes openid only once UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -668,11 +765,67 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ - &oidctestutil.TestUpstreamOIDCIdentityProvider{ + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { Name: testName, ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: testIssuerRevocationURL, + Scopes: testDefaultExpectedScopes, + UsernameClaim: testUsernameClaim, + GroupsClaim: testGroupsClaim, + AllowPasswordGrant: false, + AdditionalAuthcodeParams: map[string]string{}, + ResourceUID: testUID, + }, + }, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + {Type: "AdditionalAuthorizeParametersValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "additionalAuthorizeParameters parameter names are allowed", ObservedGeneration: 1234}, + {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234}, + }, + }, + }}, + }, + { + name: "existing valid upstream with no revocation endpoint in the discovery document", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/valid-without-revocation", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + Claims: v1alpha1.OIDCClaims{Groups: testGroupsClaim, Username: testUsernameClaim}, + }, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Ready", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidConditionEarlier, + {Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"}, + {Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"}, + }, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantLogs: []string{ + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { + Name: testName, + ClientID: testClientID, + AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: nil, // no revocation URL is set in the cached provider because none was returned by discovery Scopes: testDefaultExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -725,11 +878,12 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ - &oidctestutil.TestUpstreamOIDCIdentityProvider{ + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { Name: testName, ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: testIssuerRevocationURL, Scopes: testExpectedScopes, UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -784,11 +938,12 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{ - &oidctestutil.TestUpstreamOIDCIdentityProvider{ + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{ + { Name: testName, ClientID: testClientID, AuthorizationURL: *testIssuerAuthorizeURL, + RevocationURL: testIssuerRevocationURL, Scopes: testExpectedScopes, // does not include the default scopes UsernameClaim: testUsernameClaim, GroupsClaim: testGroupsClaim, @@ -845,7 +1000,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="the following additionalAuthorizeParameters are not allowed: response_type,scope,client_id,state,nonce,code_challenge,code_challenge_method,redirect_uri,hd" "reason"="DisallowedParameterName" "status"="False" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="the following additionalAuthorizeParameters are not allowed: response_type,scope,client_id,state,nonce,code_challenge,code_challenge_method,redirect_uri,hd" "name"="test-name" "namespace"="test-namespace" "reason"="DisallowedParameterName" "type"="AdditionalAuthorizeParametersValid"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testUID}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -883,7 +1038,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/ends-with-slash\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/ends-with-slash\" got \"` + testIssuerURL + `/ends-with-slash/\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -932,7 +1087,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"` + testIssuerURL + `/\":\noidc: issuer did not match the issuer returned by provider, expected \"` + testIssuerURL + `/\" got \"` + testIssuerURL + `\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, - wantResultingCache: []provider.UpstreamOIDCIdentityProviderI{}, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Status: v1alpha1.OIDCIdentityProviderStatus{ @@ -1010,6 +1165,7 @@ oidc: issuer did not match the issuer returned by provider, expected "` + testIs require.Equal(t, tt.wantResultingCache[i].AllowsPasswordGrant(), actualIDP.AllowsPasswordGrant()) require.Equal(t, tt.wantResultingCache[i].GetAdditionalAuthcodeParams(), actualIDP.GetAdditionalAuthcodeParams()) require.Equal(t, tt.wantResultingCache[i].GetResourceUID(), actualIDP.GetResourceUID()) + require.Equal(t, tt.wantResultingCache[i].GetRevocationURL(), actualIDP.GetRevocationURL()) require.ElementsMatch(t, tt.wantResultingCache[i].GetScopes(), actualIDP.GetScopes()) // We always want to use the proxy from env on these clients, so although the following assertions @@ -1067,18 +1223,30 @@ func newTestIssuer(t *testing.T) (string, string) { caBundlePEM, testURL := testutil.TLSTestServer(t, mux.ServeHTTP) type providerJSON struct { - Issuer string `json:"issuer"` - AuthURL string `json:"authorization_endpoint"` - TokenURL string `json:"token_endpoint"` - JWKSURL string `json:"jwks_uri"` + Issuer string `json:"issuer"` + AuthURL string `json:"authorization_endpoint"` + TokenURL string `json:"token_endpoint"` + RevocationURL string `json:"revocation_endpoint,omitempty"` + JWKSURL string `json:"jwks_uri"` } // At the root of the server, serve an issuer with a valid discovery response. mux.HandleFunc("/.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, - AuthURL: "https://example.com/authorize", + Issuer: testURL, + AuthURL: "https://example.com/authorize", + RevocationURL: "https://example.com/revoke", + }) + }) + + // At "/valid-without-revocation", serve an issuer with a valid discovery response which does not have a revocation endpoint. + mux.HandleFunc("/valid-without-revocation/.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 + "/valid-without-revocation", + AuthURL: "https://example.com/authorize", + RevocationURL: "", // none }) }) @@ -1091,6 +1259,16 @@ func newTestIssuer(t *testing.T) (string, string) { }) }) + // At "/invalid-revocation-url", serve an issuer that returns an invalid revocation URL (not parseable). + mux.HandleFunc("/invalid-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 + "/invalid-revocation-url", + AuthURL: "https://example.com/authorize", + RevocationURL: "%", + }) + }) + // At "/insecure", serve an issuer that returns an insecure authorization URL (not https://). mux.HandleFunc("/insecure/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") @@ -1100,6 +1278,16 @@ func newTestIssuer(t *testing.T) (string, string) { }) }) + // At "/insecure", serve an issuer that returns an insecure authorization 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", + }) + }) + // handle the four issuer with trailing slash configs // valid case in= out= @@ -1109,8 +1297,9 @@ func newTestIssuer(t *testing.T) (string, string) { mux.HandleFunc("/ends-with-slash/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&providerJSON{ - Issuer: testURL + "/ends-with-slash/", - AuthURL: "https://example.com/authorize", + Issuer: testURL + "/ends-with-slash/", + AuthURL: "https://example.com/authorize", + RevocationURL: "https://example.com/revoke", }) }) diff --git a/internal/controller/supervisorstorage/garbage_collector.go b/internal/controller/supervisorstorage/garbage_collector.go index beb5317f..ae6a3a1f 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -4,6 +4,7 @@ package supervisorstorage import ( + "errors" "time" v1 "k8s.io/api/core/v1" @@ -16,19 +17,32 @@ import ( pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/crud" + "go.pinniped.dev/internal/fositestorage/accesstoken" + "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/fositestorage/openidconnect" + "go.pinniped.dev/internal/fositestorage/pkce" + "go.pinniped.dev/internal/fositestorage/refreshtoken" + "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" ) const minimumRepeatInterval = 30 * time.Second type garbageCollectorController struct { + idpCache UpstreamOIDCIdentityProviderICache secretInformer corev1informers.SecretInformer kubeClient kubernetes.Interface clock clock.Clock timeOfMostRecentSweep time.Time } +// UpstreamOIDCIdentityProviderICache is a thread safe cache that holds a list of validated upstream OIDC IDP configurations. +type UpstreamOIDCIdentityProviderICache interface { + GetOIDCIdentityProviders() []provider.UpstreamOIDCIdentityProviderI +} + func GarbageCollectorController( + idpCache UpstreamOIDCIdentityProviderICache, clock clock.Clock, kubeClient kubernetes.Interface, secretInformer corev1informers.SecretInformer, @@ -46,6 +60,7 @@ func GarbageCollectorController( controllerlib.Config{ Name: "garbage-collector-controller", Syncer: &garbageCollectorController{ + idpCache: idpCache, secretInformer: secretInformer, kubeClient: kubeClient, clock: clock, @@ -102,6 +117,15 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { } if garbageCollectAfterTime.Before(frozenClock.Now()) { + storageType, isSessionStorage := secret.Labels[crud.SecretLabelKey] + if isSessionStorage { + err := c.maybeRevokeUpstreamOIDCRefreshToken(storageType, secret) + if err != nil { + // Log the error for debugging purposes, but still carry on to delete the Secret despite the error. + plog.DebugErr("garbage collector could not revoke upstream refresh token", err, logKV(secret)) + } + } + err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{ Preconditions: &metav1.Preconditions{ UID: &secret.UID, @@ -119,11 +143,76 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { return nil } +//nolint:godot // do not complain about the following to-do comment +// TODO write unit tests for all of the following cases. Note that there is already a test +// double implemented for the RevokeRefreshToken() method on the objects in the idpCache +// to help with the test mocking. See RevokeRefreshTokenCallCount() and RevokeRefreshTokenArgs(int) +// in TestUpstreamOIDCIdentityProvider (in file oidctestutil.go). This will allowing following +// the pattern used in other unit tests that fill the idpCache with mock providers using the builders +// from oidctestutil.go. +func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(storageType string, secret *v1.Secret) error { + // All session storage types hold upstream refresh tokens when the upstream IDP is an OIDC provider. + // However, some of them will be outdated because they are not updated by fosite after creation. + // Our goal below is to always revoke the latest upstream refresh token that we are holding for the + // session, and only the latest. + switch storageType { + case authorizationcode.TypeLabelValue: + // For authcode storage, check if the authcode was used. If the authcode was never used, then its + // storage must contain the latest upstream refresh token, so revoke it. + // TODO Use ReadFromSecret from the authorizationcode package under fositestorage to validate/parse the Secret, return any errors + // TODO return nil if the upstream type is not OIDC + // TODO return nil if the authcode is *NOT* active (meaning that it was already used) + // TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found + // TODO use the cached interface to revoke the refresh token, return any error + plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret)) + return nil + case accesstoken.TypeLabelValue: + // For access token storage, check if the "offline_access" scope was granted on the downstream + // session. If it was not, then the user could not possibly have performed a downstream refresh. + // In this case, the access token storage has the latest version of the upstream refresh token, + // so call the upstream issuer to revoke it. + // TODO Implement ReadFromSecret in the accesstoken package, similar to how it was done in the authorizationcode package + // TODO Use that the new ReadFromSecret func to validate/parse the Secret, return any errors + // TODO return nil if the upstream type is not OIDC + // TODO return nil if the "offline_access" scope was *NOT* granted on the downstream session + // TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found + // TODO use the cached interface to revoke the refresh token, return any error + plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret)) + return nil + case refreshtoken.TypeLabelValue: + // For refresh token storage, revoke its upstream refresh token. This refresh token storage could + // be the result of the authcode token exchange, or it could be the result of a downstream refresh. + // Either way, it always contains the latest upstream refresh token when it exists. + // TODO Implement ReadFromSecret in the refreshtoken package, similar to how it was done in the authorizationcode package + // TODO Use that new ReadFromSecret func to validate/parse the Secret, return any errors + // TODO return nil if the upstream type is not OIDC + // TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found + // TODO use the cached interface to always revoke the refresh token, return any error + plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret)) + return nil + case pkce.TypeLabelValue: + // For PKCE storage, its very existence means that the authcode was never exchanged, because these + // are deleted during authcode exchange. No need to do anything, since the upstream refresh token + // revocation is handled by authcode storage case above. + return nil + case openidconnect.TypeLabelValue: + // For OIDC storage, there is no need to do anything for reasons similar to the PKCE storage. + // These are not deleted during authcode exchange, probably due to a bug in fosite, even though it + // will never be read or updated again. However, the refresh token contained inside will be revoked + // by one of the other cases above. + return nil + default: + // There are no other storage types, so this should never happen in practice. + return errors.New("garbage collector saw invalid label on Secret when trying to determine if upstream revocation was needed") + } +} + func logKV(secret *v1.Secret) []interface{} { return []interface{}{ "secretName", secret.Name, "secretNamespace", secret.Namespace, "secretType", string(secret.Type), "garbageCollectAfter", secret.Annotations[crud.SecretLifetimeAnnotationKey], + "storageTypeLabelValue", secret.Labels[crud.SecretLabelKey], } } diff --git a/internal/controller/supervisorstorage/garbage_collector_test.go b/internal/controller/supervisorstorage/garbage_collector_test.go index 889de157..5451a918 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -39,6 +39,7 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) { observableWithInformerOption = testutil.NewObservableWithInformerOption() secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() _ = GarbageCollectorController( + nil, clock.RealClock{}, nil, secretsInformer, @@ -132,6 +133,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { var startInformersAndController = func() { // Set this at the last second to allow for injection of server override. subject = GarbageCollectorController( + nil, // TODO put an IDP cache here for these tests (use the builder like other controller tests) fakeClock, deleteOptionsRecorder, kubeInformers.Core().V1().Secrets(), diff --git a/internal/crud/crud.go b/internal/crud/crud.go index 4211ae10..57e73b2a 100644 --- a/internal/crud/crud.go +++ b/internal/crud/crud.go @@ -51,22 +51,20 @@ type JSON interface{} // document that we need valid JSON types func New(resource string, secrets corev1client.SecretInterface, clock func() time.Time, lifetime time.Duration) Storage { return &secretsStorage{ - resource: resource, - secretType: corev1.SecretType(fmt.Sprintf(secretTypeFormat, resource)), - secretVersion: []byte(secretVersion), - secrets: secrets, - clock: clock, - lifetime: lifetime, + resource: resource, + secretType: secretType(resource), + secrets: secrets, + clock: clock, + lifetime: lifetime, } } type secretsStorage struct { - resource string - secretType corev1.SecretType - secretVersion []byte - secrets corev1client.SecretInterface - clock func() time.Time - lifetime time.Duration + resource string + secretType corev1.SecretType + secrets corev1client.SecretInterface + clock func() time.Time + lifetime time.Duration } func (s *secretsStorage) Create(ctx context.Context, signature string, data JSON, additionalLabels map[string]string) (string, error) { @@ -86,28 +84,14 @@ func (s *secretsStorage) Get(ctx context.Context, signature string, data JSON) ( if err != nil { return "", fmt.Errorf("failed to get %s for signature %s: %w", s.resource, signature, err) } - if err := s.validateSecret(secret); err != nil { - return "", err - } - if err := json.Unmarshal(secret.Data[secretDataKey], data); err != nil { - return "", fmt.Errorf("failed to decode %s for signature %s: %w", s.resource, signature, err) + + err = FromSecret(s.resource, secret, data) + if err != nil { + return "", fmt.Errorf("error during get for signature %s: %w", signature, err) } return secret.ResourceVersion, nil } -func (s *secretsStorage) validateSecret(secret *corev1.Secret) error { - if secret.Type != s.secretType { - return fmt.Errorf("%w: %s must equal %s", ErrSecretTypeMismatch, secret.Type, s.secretType) - } - if labelResource := secret.Labels[SecretLabelKey]; labelResource != s.resource { - return fmt.Errorf("%w: %s must equal %s", ErrSecretLabelMismatch, labelResource, s.resource) - } - if !bytes.Equal(secret.Data[secretVersionKey], s.secretVersion) { - return ErrSecretVersionMismatch // TODO should this be fatal or not? - } - return nil -} - func (s *secretsStorage) Update(ctx context.Context, signature, resourceVersion string, data JSON) (string, error) { // Note: There may be a small bug here in that toSecret will move the SecretLifetimeAnnotationKey date forward // instead of keeping the storage resource's original SecretLifetimeAnnotationKey value. However, we only use @@ -154,6 +138,36 @@ func (s *secretsStorage) DeleteByLabel(ctx context.Context, labelName string, la return nil } +// FromSecret is similar to Get, but for when you already have a Secret in hand, e.g. from an informer. +// It validates and unmarshals the Secret. The data parameter is filled in as the result. +func FromSecret(resource string, secret *corev1.Secret, data JSON) error { + if err := validateSecret(resource, secret); err != nil { + return err + } + if err := json.Unmarshal(secret.Data[secretDataKey], data); err != nil { + return fmt.Errorf("failed to decode %s: %w", resource, err) + } + return nil +} + +func secretType(resource string) corev1.SecretType { + return corev1.SecretType(fmt.Sprintf(secretTypeFormat, resource)) +} + +func validateSecret(resource string, secret *corev1.Secret) error { + secretType := corev1.SecretType(fmt.Sprintf(secretTypeFormat, resource)) + if secret.Type != secretType { + return fmt.Errorf("%w: %s must equal %s", ErrSecretTypeMismatch, secret.Type, secretType) + } + if labelResource := secret.Labels[SecretLabelKey]; labelResource != resource { + return fmt.Errorf("%w: %s must equal %s", ErrSecretLabelMismatch, labelResource, resource) + } + if !bytes.Equal(secret.Data[secretVersionKey], []byte(secretVersion)) { + return ErrSecretVersionMismatch // TODO should this be fatal or not? + } + return nil +} + //nolint: gochecknoglobals var b32 = base32.StdEncoding.WithPadding(base32.NoPadding) @@ -190,7 +204,7 @@ func (s *secretsStorage) toSecret(signature, resourceVersion string, data JSON, }, Data: map[string][]byte{ secretDataKey: buf, - secretVersionKey: s.secretVersion, + secretVersionKey: []byte(secretVersion), }, Type: s.secretType, }, nil diff --git a/internal/crud/crud_test.go b/internal/crud/crud_test.go index 8a910b8a..59d328d0 100644 --- a/internal/crud/crud_test.go +++ b/internal/crud/crud_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package crud @@ -793,7 +793,8 @@ func TestStorage(t *testing.T) { require.Empty(t, rv1) require.Empty(t, out.Data) require.True(t, errors.Is(err, ErrSecretTypeMismatch)) - require.EqualError(t, err, "secret storage data has incorrect type: storage.pinniped.dev/candies-not must equal storage.pinniped.dev/candies") + require.EqualError(t, err, "error during get for signature 5aUhdNmfWLW3yKX8Zfz5ztS5IiiWBgu36Gja-o2xl0I: "+ + "secret storage data has incorrect type: storage.pinniped.dev/candies-not must equal storage.pinniped.dev/candies") return nil }, @@ -856,7 +857,8 @@ func TestStorage(t *testing.T) { require.Empty(t, rv1) require.Empty(t, out.Data) require.True(t, errors.Is(err, ErrSecretLabelMismatch)) - require.EqualError(t, err, "secret storage data has incorrect label: candies-are-bad must equal candies") + require.EqualError(t, err, "error during get for signature 5aUhdNmfWLW3yKX8Zfz5ztS5IiiWBgu36Gja-o2xl0I: "+ + "secret storage data has incorrect label: candies-are-bad must equal candies") return nil }, @@ -919,7 +921,8 @@ func TestStorage(t *testing.T) { require.Empty(t, rv1) require.Empty(t, out.Data) require.True(t, errors.Is(err, ErrSecretVersionMismatch)) - require.EqualError(t, err, "secret storage data has incorrect version") + require.EqualError(t, err, "error during get for signature 5aUhdNmfWLW3yKX8Zfz5ztS5IiiWBgu36Gja-o2xl0I: "+ + "secret storage data has incorrect version") return nil }, @@ -981,7 +984,8 @@ func TestStorage(t *testing.T) { rv1, err := storage.Get(ctx, signature, out) require.Empty(t, rv1) require.Empty(t, out.Data) - require.EqualError(t, err, "failed to decode candies for signature 5aUhdNmfWLW3yKX8Zfz5ztS5IiiWBgu36Gja-o2xl0I: invalid character '}' looking for beginning of value") + require.EqualError(t, err, "error during get for signature 5aUhdNmfWLW3yKX8Zfz5ztS5IiiWBgu36Gja-o2xl0I: "+ + "failed to decode candies: invalid character '}' looking for beginning of value") return nil }, @@ -1095,3 +1099,155 @@ func errString(err error) string { return err.Error() } + +func TestFromSecret(t *testing.T) { + fakeNow := time.Date(2030, time.January, 1, 0, 0, 0, 0, time.UTC) + lifetime := time.Minute * 10 + fakeNowPlusLifetimeAsString := metav1.Time{Time: fakeNow.Add(lifetime)}.Format(time.RFC3339) + + type testJSON struct { + Data string + } + + tests := []struct { + name string + resource string + secret *corev1.Secret + wantData *testJSON + wantErr string + }{ + { + name: "happy path", + resource: "candies", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-candies-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: "some-namespace", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "candies", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"snorlax"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/candies", + }, + wantData: &testJSON{Data: "snorlax"}, + }, + { + name: "can't unmarshal", + resource: "candies", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-candies-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: "some-namespace", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "candies", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`not-json`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/candies", + }, + wantData: &testJSON{Data: "snorlax"}, + wantErr: "failed to decode candies: invalid character 'o' in literal null (expecting 'u')", + }, + { + name: "wrong storage version", + resource: "candies", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-candies-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: "some-namespace", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "candies", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"snorlax"}`), + "pinniped-storage-version": []byte("wrong-version-here"), + }, + Type: "storage.pinniped.dev/candies", + }, + wantData: &testJSON{Data: "snorlax"}, + wantErr: "secret storage data has incorrect version", + }, + { + name: "wrong type label", + resource: "candies", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-candies-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: "some-namespace", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "candies", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"snorlax"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/not-candies", + }, + wantData: &testJSON{Data: "snorlax"}, + wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-candies must equal storage.pinniped.dev/candies", + }, + { + name: "wrong secret type", + resource: "candies", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-candies-lvzgyywdc2dhjdbgf5jvzfyphosigvhnsh6qlse3blumogoqhqhq", + Namespace: "some-namespace", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "candies", + }, + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": fakeNowPlusLifetimeAsString, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"Data":"snorlax"}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/not-candies", + }, + wantData: &testJSON{Data: "snorlax"}, + wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-candies must equal storage.pinniped.dev/candies", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + data := &testJSON{} + err := FromSecret("candies", tt.secret, data) + if tt.wantErr == "" { + require.NoError(t, err) + require.Equal(t, data, tt.wantData) + } else { + require.EqualError(t, err, tt.wantErr) + } + }) + } +} diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index b259e406..04ebe12d 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -11,6 +11,7 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -48,6 +49,23 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt return &authorizeCodeStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } +// ReadFromSecret reads the contents of a Secret as an AuthorizeCodeSession. +func ReadFromSecret(secret *v1.Secret) (*AuthorizeCodeSession, error) { + session := NewValidEmptyAuthorizeCodeSession() + err := crud.FromSecret(TypeLabelValue, secret, session) + if err != nil { + return nil, err + } + if session.Version != authorizeCodeStorageVersion { + return nil, fmt.Errorf("%w: authorization code session has version %s instead of %s", + ErrInvalidAuthorizeRequestVersion, session.Version, authorizeCodeStorageVersion) + } + if session.Request.ID == "" { + return nil, fmt.Errorf("malformed authorization code session: %w", ErrInvalidAuthorizeRequestData) + } + return session, nil +} + func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, signature string, requester fosite.Requester) error { // This conversion assumes that we do not wrap the default type in any way // i.e. we use the default fosite.OAuth2Provider.NewAuthorizeRequest implementation diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 50ac0c28..eadfd0a4 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -18,6 +18,7 @@ import ( fuzz "github.com/google/gofuzz" "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" + "github.com/ory/fosite/handler/openid" "github.com/pkg/errors" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" @@ -397,3 +398,120 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { // thus if AuthorizeRequest changes, we will detect it here (though we could possibly miss an omitempty field) require.JSONEq(t, ExpectedAuthorizeCodeSessionJSONFromFuzzing, authorizeCodeSessionJSONFromFuzzing) } + +func TestReadFromSecret(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + wantSession *AuthorizeCodeSession + wantErr string + }{ + { + name: "happy path", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "authcode", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","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"}}}},"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/authcode", + }, + wantSession: &AuthorizeCodeSession{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "abcd-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Fosite: &openid.DefaultSession{ + Username: "snorlax", + Subject: "panda", + }, + Custom: &psession.CustomSessionData{ + ProviderUID: "fake-provider-uid", + ProviderName: "fake-provider-name", + ProviderType: "fake-provider-type", + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + }, + }, + { + name: "wrong secret type", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "authcode", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1"},"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/not-authcode", + }, + wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-authcode must equal storage.pinniped.dev/authcode", + }, + { + name: "wrong session version", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "authcode", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1"},"version":"wrong-version-here","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/authcode", + }, + wantErr: "authorization request data has wrong version: authorization code session has version wrong-version-here instead of 2", + }, + { + name: "missing request", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-authcode-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "authcode", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/authcode", + }, + wantErr: "malformed authorization code session: authorization request data must be present", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + session, err := ReadFromSecret(tt.secret) + if tt.wantErr == "" { + require.NoError(t, err) + require.Equal(t, tt.wantSession, session) + } else { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, session) + } + }) + } +} diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index fd8b7dd9..046f1849 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -215,6 +215,20 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) PerformRefresh(arg0, ar return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PerformRefresh", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).PerformRefresh), arg0, arg1) } +// RevokeRefreshToken mocks base method. +func (m *MockUpstreamOIDCIdentityProviderI) RevokeRefreshToken(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RevokeRefreshToken", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// RevokeRefreshToken indicates an expected call of RevokeRefreshToken. +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RevokeRefreshToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).RevokeRefreshToken), arg0, arg1) +} + // ValidateToken mocks base method. func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce) (*oidctypes.Token, error) { m.ctrl.T.Helper() diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 88710f00..6278f790 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -68,6 +68,9 @@ type UpstreamOIDCIdentityProviderI interface { // validate the ID token. PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) + // 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 // into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated // tokens, or an error. diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 805c4d3e..c5e65a8b 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -124,6 +124,7 @@ func prepareControllers( NewManager(). WithController( supervisorstorage.GarbageCollectorController( + dynamicUpstreamIDPProvider, clock.RealClock{}, kubeClient, secretInformer, diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 90361ddd..07986df1 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -65,6 +65,13 @@ type PerformRefreshArgs struct { RefreshToken string } +// RevokeRefreshTokenArgs is used to spy on calls to +// TestUpstreamOIDCIdentityProvider.RevokeRefreshTokenArgsFunc(). +type RevokeRefreshTokenArgs struct { + Ctx context.Context + RefreshToken string +} + // ValidateTokenArgs is used to spy on calls to // TestUpstreamOIDCIdentityProvider.ValidateTokenFunc(). type ValidateTokenArgs struct { @@ -103,6 +110,7 @@ type TestUpstreamOIDCIdentityProvider struct { ClientID string ResourceUID types.UID AuthorizationURL url.URL + RevocationURL *url.URL UsernameClaim string GroupsClaim string Scopes []string @@ -124,6 +132,8 @@ type TestUpstreamOIDCIdentityProvider struct { PerformRefreshFunc func(ctx context.Context, refreshToken string) (*oauth2.Token, error) + RevokeRefreshTokenFunc func(ctx context.Context, refreshToken string) error + ValidateTokenFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) exchangeAuthcodeAndValidateTokensCallCount int @@ -132,6 +142,8 @@ type TestUpstreamOIDCIdentityProvider struct { passwordCredentialsGrantAndValidateTokensArgs []*PasswordCredentialsGrantAndValidateTokensArgs performRefreshCallCount int performRefreshArgs []*PerformRefreshArgs + revokeRefreshTokenCallCount int + revokeRefreshTokenArgs []*RevokeRefreshTokenArgs validateTokenCallCount int validateTokenArgs []*ValidateTokenArgs } @@ -158,6 +170,10 @@ func (u *TestUpstreamOIDCIdentityProvider) GetAuthorizationURL() *url.URL { return &u.AuthorizationURL } +func (u *TestUpstreamOIDCIdentityProvider) GetRevocationURL() *url.URL { + return u.RevocationURL +} + func (u *TestUpstreamOIDCIdentityProvider) GetScopes() []string { return u.Scopes } @@ -228,6 +244,18 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefresh(ctx context.Context, r return u.PerformRefreshFunc(ctx, refreshToken) } +func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshToken(ctx context.Context, refreshToken string) error { + if u.revokeRefreshTokenArgs == nil { + u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0) + } + u.revokeRefreshTokenCallCount++ + u.revokeRefreshTokenArgs = append(u.revokeRefreshTokenArgs, &RevokeRefreshTokenArgs{ + Ctx: ctx, + RefreshToken: refreshToken, + }) + return u.RevokeRefreshTokenFunc(ctx, refreshToken) +} + func (u *TestUpstreamOIDCIdentityProvider) PerformRefreshCallCount() int { return u.performRefreshCallCount } @@ -239,6 +267,17 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefreshArgs(call int) *Perform return u.performRefreshArgs[call] } +func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenCallCount() int { + return u.performRefreshCallCount +} + +func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenArgs(call int) *RevokeRefreshTokenArgs { + if u.revokeRefreshTokenArgs == nil { + u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0) + } + return u.revokeRefreshTokenArgs[call] +} + func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { if u.validateTokenArgs == nil { u.validateTokenArgs = make([]*ValidateTokenArgs, 0) @@ -477,6 +516,7 @@ type TestUpstreamOIDCIdentityProviderBuilder struct { authcodeExchangeErr error passwordGrantErr error performRefreshErr error + revokeRefreshTokenErr error validateTokenErr error } @@ -622,6 +662,9 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdent } return u.refreshedTokens, nil }, + RevokeRefreshTokenFunc: func(ctx context.Context, refreshToken string) error { + return u.revokeRefreshTokenErr + }, ValidateTokenFunc: func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { if u.validateTokenErr != nil { return nil, u.validateTokenErr diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 7d4006c8..7dfd4b4d 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -8,8 +8,10 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "net/url" + "strings" coreosoidc "github.com/coreos/go-oidc/v3/oidc" "golang.org/x/oauth2" @@ -40,6 +42,7 @@ type ProviderConfig struct { Client *http.Client AllowPasswordGrant bool AdditionalAuthcodeParams map[string]string + RevocationURL *url.URL // will commonly be nil: many providers do not offer this Provider interface { Verifier(*coreosoidc.Config) *coreosoidc.IDTokenVerifier Claims(v interface{}) error @@ -53,6 +56,10 @@ func (p *ProviderConfig) GetResourceUID() types.UID { return p.ResourceUID } +func (p *ProviderConfig) GetRevocationURL() *url.URL { + return p.RevocationURL +} + func (p *ProviderConfig) GetAdditionalAuthcodeParams() map[string]string { return p.AdditionalAuthcodeParams } @@ -130,6 +137,100 @@ func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string return p.Config.TokenSource(httpClientContext, &oauth2.Token{RefreshToken: refreshToken}).Token() } +// RevokeRefreshToken will attempt to revoke the given token, if the provider has a revocation endpoint. +func (p *ProviderConfig) RevokeRefreshToken(ctx context.Context, refreshToken string) error { + if p.RevocationURL == nil { + return nil + } + // First try using client auth in the request params. + tryAnotherClientAuthMethod, err := p.tryRevokeRefreshToken(ctx, refreshToken, false) + if tryAnotherClientAuthMethod { + // Try again using basic auth this time. Overwrite the first client auth error, + // which isn't useful anymore when retrying. + _, err = p.tryRevokeRefreshToken(ctx, refreshToken, true) + } + return err +} + +// tryRevokeRefreshToken will call the revocation endpoint using either basic auth or by including +// client auth in the request params. It will return an error when the request failed. If the +// request failed for a reason that might be due to bad client auth, then it will return true +// for the tryAnotherClientAuthMethod return value, indicating that it might be worth trying +// again using the other client auth method. +// RFC 7009 defines how to make a revocation request and how to interpret the response. +// See https://datatracker.ietf.org/doc/html/rfc7009#section-2.1 for details. +func (p *ProviderConfig) tryRevokeRefreshToken( + ctx context.Context, + refreshToken string, + useBasicAuth bool, +) (tryAnotherClientAuthMethod bool, err error) { + clientID := p.Config.ClientID + clientSecret := p.Config.ClientSecret + // Use the provided HTTP client to benefit from its CA, proxy, and other settings. + httpClient := p.Client + + params := url.Values{ + "token": []string{refreshToken}, + "token_type_hint": []string{"refresh_token"}, + } + if !useBasicAuth { + params["client_id"] = []string{clientID} + params["client_secret"] = []string{clientSecret} + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, p.RevocationURL.String(), strings.NewReader(params.Encode())) + if err != nil { + // This shouldn't really happen since we already know that the method and URL are legal. + return false, err + } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + if useBasicAuth { + req.SetBasicAuth(clientID, clientSecret) + } + + resp, err := httpClient.Do(req) + if err != nil { + // Couldn't connect to the server or some similar error. + return false, err + } + defer resp.Body.Close() + + switch resp.StatusCode { + case http.StatusOK: + // Success! + return false, nil + case http.StatusBadRequest: + // Bad request might be due to bad client auth method. Try to detect that. + body, err := io.ReadAll(resp.Body) + if err != nil { + return false, + fmt.Errorf("error reading response body on response with status code %d: %w", resp.StatusCode, err) + } + var parsedResp struct { + ErrorType string `json:"error"` + ErrorDescription string `json:"error_description"` + } + bodyStr := strings.TrimSpace(string(body)) // trimmed for logging purposes + err = json.Unmarshal(body, &parsedResp) + if err != nil { + return false, + fmt.Errorf("error parsing response body %q on response with status code %d: %w", bodyStr, resp.StatusCode, err) + } + err = fmt.Errorf("server responded with status %d with body: %s", resp.StatusCode, bodyStr) + if parsedResp.ErrorType != "invalid_client" { + // Got an error unrelated to client auth, so not worth trying again. + return false, err + } + // Got an "invalid_client" response, which might mean client auth failed, so it may be worth trying again + // using another client auth method. See https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 + return true, err + default: + // Any other error is probably not due to failed client auth. + return false, fmt.Errorf("server responded with status %d", resp.StatusCode) + } +} + // ValidateToken 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) { diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 342683fc..f8906562 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -10,6 +10,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "reflect" "testing" "time" @@ -454,6 +455,142 @@ func TestProviderConfig(t *testing.T) { } }) + t.Run("RevokeRefreshToken", func(t *testing.T) { + tests := []struct { + name string + nilRevocationURL bool + statusCodes []int + returnErrBodies []string + wantErr string + wantNumRequests int + }{ + { + name: "success without calling the server when there is no revocation URL set", + nilRevocationURL: true, + wantNumRequests: 0, + }, + { + name: "success when the server returns 200 OK on the first call", + statusCodes: []int{http.StatusOK}, + wantNumRequests: 1, + }, + { + name: "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call", + statusCodes: []int{http.StatusBadRequest, http.StatusOK}, + // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 defines this as the error for client auth failure + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`}, + wantNumRequests: 2, + }, + { + name: "error when the server returns 400 Bad Request on the first call due to client auth, then any 400 error on second call", + statusCodes: []int{http.StatusBadRequest, http.StatusBadRequest}, + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, `{ "error":"anything", "error_description":"unhappy" }`}, + wantErr: `server responded with status 400 with body: { "error":"anything", "error_description":"unhappy" }`, + wantNumRequests: 2, + }, + { + name: "error when the server returns 400 Bad Request with bad JSON body on the first call", + statusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{`invalid JSON body`}, + wantErr: `error parsing response body "invalid JSON body" on response with status code 400: invalid character 'i' looking for beginning of value`, + wantNumRequests: 1, + }, + { + name: "error when the server returns 400 Bad Request with empty body", + statusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{``}, + wantErr: `error parsing response body "" on response with status code 400: unexpected end of JSON input`, + wantNumRequests: 1, + }, + { + name: "error when the server returns 400 Bad Request on the first call due to client auth, then any other error on second call", + statusCodes: []int{http.StatusBadRequest, http.StatusForbidden}, + returnErrBodies: []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""}, + wantErr: "server responded with status 403", + wantNumRequests: 2, + }, + { + name: "error when server returns any other 400 error on first call", + statusCodes: []int{http.StatusBadRequest}, + returnErrBodies: []string{`{ "error":"anything_else", "error_description":"unhappy" }`}, + wantErr: `server responded with status 400 with body: { "error":"anything_else", "error_description":"unhappy" }`, + wantNumRequests: 1, + }, + { + name: "error when server returns any other error aside from 400 on first call", + statusCodes: []int{http.StatusForbidden}, + returnErrBodies: []string{""}, + wantErr: "server responded with status 403", + wantNumRequests: 1, + }, + } + for _, tt := range tests { + tt := tt + numRequests := 0 + t.Run(tt.name, func(t *testing.T) { + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + numRequests++ + require.LessOrEqual(t, numRequests, 2) + require.Equal(t, http.MethodPost, r.Method) + require.NoError(t, r.ParseForm()) + if numRequests == 1 { + // First request should use client_id/client_secret params. + require.Equal(t, 4, len(r.Form)) + require.Equal(t, "test-client-id", r.Form.Get("client_id")) + require.Equal(t, "test-client-secret", r.Form.Get("client_secret")) + require.Equal(t, "refresh_token", r.Form.Get("token_type_hint")) + require.Equal(t, "test-initial-refresh-token", r.Form.Get("token")) + } else { + // Second request, if there is one, should use basic auth. + require.Equal(t, 2, len(r.Form)) + require.Equal(t, "refresh_token", r.Form.Get("token_type_hint")) + require.Equal(t, "test-initial-refresh-token", r.Form.Get("token")) + username, password, hasBasicAuth := r.BasicAuth() + require.True(t, hasBasicAuth, "request should have had basic auth but did not") + require.Equal(t, "test-client-id", username) + require.Equal(t, "test-client-secret", password) + } + if tt.statusCodes[numRequests-1] != http.StatusOK { + w.Header().Set("content-type", "application/json") + http.Error(w, tt.returnErrBodies[numRequests-1], tt.statusCodes[numRequests-1]) + } + // Otherwise, responds with 200 OK and empty body by default. + })) + t.Cleanup(tokenServer.Close) + + tokenURL, err := url.Parse(tokenServer.URL) + require.NoError(t, err) + + p := ProviderConfig{ + Name: "test-name", + Config: &oauth2.Config{ + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + }, + RevocationURL: tokenURL, + Client: http.DefaultClient, + } + if tt.nilRevocationURL { + p.RevocationURL = nil + } + + err = p.RevokeRefreshToken( + context.Background(), + "test-initial-refresh-token", + ) + + require.Equal(t, tt.wantNumRequests, numRequests, + "did not make expected number of requests to revocation endpoint") + + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + }) + } + }) + t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) { tests := []struct { name string