diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 1b2c4e238..f2d3aa9fd 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -344,6 +344,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 977ed35fa..d8f8a6cb1 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -115,6 +115,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) @@ -152,7 +154,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { inputSecrets []runtime.Object wantErr string wantLogs []string - wantResultingCache []provider.UpstreamOIDCIdentityProviderI + wantResultingCache []*oidctestutil.TestUpstreamOIDCIdentityProvider wantResultingUpstreams []v1alpha1.OIDCIdentityProvider }{ { @@ -176,7 +178,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{ @@ -223,7 +225,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{ @@ -269,7 +271,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{ @@ -318,7 +320,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{ @@ -367,7 +369,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{ @@ -414,7 +416,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{ @@ -463,7 +465,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{ @@ -511,7 +513,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{ @@ -536,6 +538,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{ @@ -558,7 +607,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{ @@ -583,6 +632,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{ @@ -615,11 +711,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, @@ -669,11 +766,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, @@ -726,11 +879,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, @@ -785,11 +939,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, @@ -846,7 +1001,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{ @@ -884,7 +1039,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{ @@ -933,7 +1088,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{ @@ -1011,6 +1166,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 @@ -1083,18 +1239,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 }) }) @@ -1107,6 +1275,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") @@ -1116,6 +1294,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= @@ -1125,8 +1313,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 beb5317f6..2ce06533d 100644 --- a/internal/controller/supervisorstorage/garbage_collector.go +++ b/internal/controller/supervisorstorage/garbage_collector.go @@ -4,31 +4,50 @@ package supervisorstorage import ( + "context" + "errors" + "fmt" "time" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/clock" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" + "k8s.io/utils/strings/slices" 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" + "go.pinniped.dev/internal/psession" ) 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 +65,7 @@ func GarbageCollectorController( controllerlib.Config{ Name: "garbage-collector-controller", Syncer: &garbageCollectorController{ + idpCache: idpCache, secretInformer: secretInformer, kubeClient: kubeClient, clock: clock, @@ -97,33 +117,163 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error { garbageCollectAfterTime, err := time.Parse(crud.SecretLifetimeAnnotationDateFormat, timeString) if err != nil { - plog.WarningErr("could not parse resource timestamp for garbage collection", err, logKV(secret)) + plog.WarningErr("could not parse resource timestamp for garbage collection", err, logKV(secret)...) continue } - if garbageCollectAfterTime.Before(frozenClock.Now()) { - err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{ - Preconditions: &metav1.Preconditions{ - UID: &secret.UID, - ResourceVersion: &secret.ResourceVersion, - }, - }) - if err != nil { - plog.WarningErr("failed to garbage collect resource", err, logKV(secret)) - continue - } - plog.Info("storage garbage collector deleted resource", logKV(secret)) + if !garbageCollectAfterTime.Before(frozenClock.Now()) { + // not old enough yet + continue } + + storageType, isSessionStorage := secret.Labels[crud.SecretLabelKey] + if isSessionStorage { + err := c.maybeRevokeUpstreamOIDCRefreshToken(ctx.Context, storageType, secret) + if err != nil { + plog.WarningErr("garbage collector could not revoke upstream refresh token", err, logKV(secret)...) + // If the error is of a type that is worth retrying, then do not delete the Secret right away. + // A future call to Sync will try revocation again for that secret. However, if the Secret is + // getting too old, then just delete it anyway. We don't want to extend the lifetime of these + // session Secrets by too much time, since the garbage collector is the only thing that is + // cleaning them out of etcd storage. + fourHoursAgo := frozenClock.Now().Add(-4 * time.Hour) + nowIsLessThanFourHoursBeyondSecretGCTime := garbageCollectAfterTime.After(fourHoursAgo) + if errors.As(err, &retryableRevocationError{}) && nowIsLessThanFourHoursBeyondSecretGCTime { + // Hasn't been very long since secret expired, so skip deletion to try revocation again later. + continue + } + } + } + + err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{ + Preconditions: &metav1.Preconditions{ + UID: &secret.UID, + ResourceVersion: &secret.ResourceVersion, + }, + }) + if err != nil { + plog.WarningErr("failed to garbage collect resource", err, logKV(secret)...) + continue + } + plog.Info("storage garbage collector deleted resource", logKV(secret)...) } return nil } +func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx context.Context, 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: + authorizeCodeSession, err := authorizationcode.ReadFromSecret(secret) + if err != nil { + return err + } + // Check if this downstream authcode was already used. If it was already used (i.e. not active anymore), then + // the latest upstream refresh token can be found in one of the other storage types handled below instead. + if !authorizeCodeSession.Active { + return nil + } + // When the downstream authcode was never used, then its storage must contain the latest upstream refresh token. + return c.revokeUpstreamOIDCRefreshToken(ctx, authorizeCodeSession.Request.Session.(*psession.PinnipedSession).Custom, secret) + + case accesstoken.TypeLabelValue: + // For access token storage, check if the "offline_access" scope was granted on the downstream session. + // If it was granted, then the latest upstream refresh token should be found in the refresh token storage instead. + // If it was not granted, then the user could not possibly have performed a downstream refresh, so the + // access token storage has the latest version of the upstream refresh token. + accessTokenSession, err := accesstoken.ReadFromSecret(secret) + if err != nil { + return err + } + pinnipedSession := accessTokenSession.Request.Session.(*psession.PinnipedSession) + if slices.Contains(accessTokenSession.Request.GetGrantedScopes(), coreosoidc.ScopeOfflineAccess) { + return nil + } + return c.revokeUpstreamOIDCRefreshToken(ctx, pinnipedSession.Custom, secret) + + case refreshtoken.TypeLabelValue: + // For refresh token storage, always revoke its upstream refresh token. This refresh token storage could + // be the result of the initial downstream authcode exchange, or it could be the result of a downstream + // refresh. Either way, it always contains the latest upstream refresh token when it exists. + refreshTokenSession, err := refreshtoken.ReadFromSecret(secret) + if err != nil { + return err + } + return c.revokeUpstreamOIDCRefreshToken(ctx, refreshTokenSession.Request.Session.(*psession.PinnipedSession).Custom, secret) + + 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 (c *garbageCollectorController) revokeUpstreamOIDCRefreshToken(ctx context.Context, customSessionData *psession.CustomSessionData, secret *v1.Secret) error { + // When session was for another upstream IDP type, e.g. LDAP, there is no upstream OIDC refresh token involved. + if customSessionData.ProviderType != psession.ProviderTypeOIDC { + return nil + } + + // Try to find the provider that was originally used to create the stored session. + var foundOIDCIdentityProviderI provider.UpstreamOIDCIdentityProviderI + for _, p := range c.idpCache.GetOIDCIdentityProviders() { + if p.GetName() == customSessionData.ProviderName && p.GetResourceUID() == customSessionData.ProviderUID { + foundOIDCIdentityProviderI = p + break + } + } + if foundOIDCIdentityProviderI == nil { + return fmt.Errorf("could not find upstream OIDC provider named %q with resource UID %q", customSessionData.ProviderName, customSessionData.ProviderUID) + } + + // Revoke the upstream refresh token. This is a noop if the upstream provider does not offer a revocation endpoint. + err := foundOIDCIdentityProviderI.RevokeRefreshToken(ctx, customSessionData.OIDC.UpstreamRefreshToken) + if err != nil { + // This could be a network failure, a 503 result which we should retry + // (see https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1), + // or any other non-200 response from the revocation endpoint. + // Regardless of which, it is probably worth retrying. + return retryableRevocationError{wrapped: err} + } + + plog.Trace("garbage collector successfully revoked upstream OIDC refresh token (or provider has no revocation endpoint)", logKV(secret)...) + return nil +} + +type retryableRevocationError struct { + wrapped error +} + +func (e retryableRevocationError) Error() string { + return fmt.Sprintf("retryable revocation error: %v", e.wrapped) +} + +func (e retryableRevocationError) Unwrap() error { + return e.wrapped +} + 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 889de157c..042c4d38f 100644 --- a/internal/controller/supervisorstorage/garbage_collector_test.go +++ b/internal/controller/supervisorstorage/garbage_collector_test.go @@ -5,10 +5,12 @@ package supervisorstorage import ( "context" + "encoding/json" "errors" "testing" "time" + "github.com/ory/fosite" "github.com/sclevine/spec" "github.com/sclevine/spec/report" "github.com/stretchr/testify/require" @@ -23,7 +25,14 @@ import ( kubetesting "k8s.io/client-go/testing" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/fositestorage/accesstoken" + "go.pinniped.dev/internal/fositestorage/authorizationcode" + "go.pinniped.dev/internal/fositestorage/refreshtoken" + "go.pinniped.dev/internal/oidc/clientregistry" + "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" + "go.pinniped.dev/internal/testutil/oidctestutil" ) func TestGarbageCollectorControllerInformerFilters(t *testing.T) { @@ -39,6 +48,7 @@ func TestGarbageCollectorControllerInformerFilters(t *testing.T) { observableWithInformerOption = testutil.NewObservableWithInformerOption() secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() _ = GarbageCollectorController( + nil, clock.RealClock{}, nil, secretsInformer, @@ -129,9 +139,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) { // Defer starting the informers until the last possible moment so that the // nested Before's can keep adding things to the informer caches. - var startInformersAndController = func() { + var startInformersAndController = func(idpCache provider.DynamicUpstreamIDPProvider) { // Set this at the last second to allow for injection of server override. subject = GarbageCollectorController( + idpCache, fakeClock, deleteOptionsRecorder, kubeInformers.Core().V1().Secrets(), @@ -183,7 +194,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { when("there are secrets without the garbage-collect-after annotation", func() { it("does not delete those secrets", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) require.Empty(t, kubeClient.Actions()) @@ -194,7 +205,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) - when("there are secrets with the garbage-collect-after annotation", func() { + when("there are any secrets with the garbage-collect-after annotation", func() { it.Before(func() { firstExpiredSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -236,7 +247,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("should delete any that are past their expiration", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.ElementsMatch( @@ -260,6 +271,724 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) }) + when("there are valid, expired authcode secrets", func() { + it.Before(func() { + activeOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + activeOIDCAuthcodeSessionJSON, err := json.Marshal(activeOIDCAuthcodeSession) + r.NoError(err) + activeOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "activeOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": activeOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(activeOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + + inactiveOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: false, + Request: &fosite.Request{ + ID: "request-id-2", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "other-fake-upstream-refresh-token", + }, + }, + }, + }, + } + inactiveOIDCAuthcodeSessionJSON, err := json.Marshal(inactiveOIDCAuthcodeSession) + r.NoError(err) + inactiveOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "inactiveOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-456", + ResourceVersion: "rv-456", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": inactiveOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(inactiveOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(inactiveOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(inactiveOIDCAuthcodeSessionSecret)) + }) + + it("should revoke upstream tokens only from the active authcode secrets and delete them all", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // The upstream refresh token is only revoked for the active authcode session. + idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // Both authcode session secrets are deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + testutil.NewPreconditions("uid-456", "rv-456"), + }, + *deleteOptions, + ) + }) + }) + + when("there is an invalid, expired authcode secret", func() { + it.Before(func() { + invalidOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "", // it is invalid for there to be a missing request ID + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + invalidOIDCAuthcodeSessionJSON, err := json.Marshal(invalidOIDCAuthcodeSession) + r.NoError(err) + invalidOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "invalidOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": invalidOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + r.NoError(kubeInformerClient.Tracker().Add(invalidOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(invalidOIDCAuthcodeSessionSecret)) + }) + + it("should remove the secret without revoking any upstream tokens", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // Nothing to revoke since we couldn't read the invalid secret. + idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t) + + // The invalid authcode session secrets is still deleted because it is expired. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "invalidOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there is a valid, expired authcode secret but its upstream name does not match any existing upstream", func() { + it.Before(func() { + wrongProviderNameOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name-will-not-match", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + wrongProviderNameOIDCAuthcodeSessionJSON, err := json.Marshal(wrongProviderNameOIDCAuthcodeSession) + r.NoError(err) + wrongProviderNameOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wrongProviderNameOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": wrongProviderNameOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(wrongProviderNameOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + }) + + it("should remove the secret without revoking any upstream tokens", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // Nothing to revoke since we couldn't find the upstream in the cache. + idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t) + + // The authcode session secrets is still deleted because it is expired. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "wrongProviderNameOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there is a valid, expired authcode secret but its upstream UID does not match any existing upstream", func() { + it.Before(func() { + wrongProviderNameOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid-will-not-match", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + wrongProviderNameOIDCAuthcodeSessionJSON, err := json.Marshal(wrongProviderNameOIDCAuthcodeSession) + r.NoError(err) + wrongProviderNameOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "wrongProviderNameOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": wrongProviderNameOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(wrongProviderNameOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret)) + }) + + it("should remove the secret without revoking any upstream tokens", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // Nothing to revoke since we couldn't find the upstream in the cache. + idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t) + + // The authcode session secrets is still deleted because it is expired. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "wrongProviderNameOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there is a valid, recently expired authcode secret but the upstream revocation fails", func() { + it.Before(func() { + activeOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + activeOIDCAuthcodeSessionJSON, err := json.Marshal(activeOIDCAuthcodeSession) + r.NoError(err) + activeOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "activeOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + // expired almost 4 hours ago, but not quite 4 hours + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add((-time.Hour * 4) + time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": activeOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(activeOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + }) + + it("keeps the secret for a while longer so the revocation can be retried on a future sync", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // Tried to revoke it, although this revocation will fail. + idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // The authcode session secrets is not deleted. + r.Empty(kubeClient.Actions()) + }) + }) + + when("there is a valid, long-since expired authcode secret but the upstream revocation fails", func() { + it.Before(func() { + activeOIDCAuthcodeSession := &authorizationcode.Session{ + Version: "2", + Active: true, + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + activeOIDCAuthcodeSessionJSON, err := json.Marshal(activeOIDCAuthcodeSession) + r.NoError(err) + activeOIDCAuthcodeSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "activeOIDCAuthcodeSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + // expired just over 4 hours ago + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add((-time.Hour * 4) - time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": authorizationcode.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": activeOIDCAuthcodeSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue, + } + _, err = authorizationcode.ReadFromSecret(activeOIDCAuthcodeSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid authcode secret") + r.NoError(kubeInformerClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret)) + }) + + it("deletes the secret because it has probably been retrying revocation for hours without success", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // Tried to revoke it, although this revocation will fail. + idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // The authcode session secrets is deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + + when("there are valid, expired access token secrets", func() { + it.Before(func() { + offlineAccessGrantedOIDCAccessTokenSession := &accesstoken.Session{ + Version: "2", + Request: &fosite.Request{ + GrantedScope: fosite.Arguments{"scope1", "scope2", "offline_access"}, + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "offline-access-granted-fake-upstream-refresh-token", + }, + }, + }, + }, + } + offlineAccessGrantedOIDCAccessTokenSessionJSON, err := json.Marshal(offlineAccessGrantedOIDCAccessTokenSession) + r.NoError(err) + offlineAccessGrantedOIDCAccessTokenSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "offlineAccessGrantedOIDCAccessTokenSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": accesstoken.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": offlineAccessGrantedOIDCAccessTokenSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + accesstoken.TypeLabelValue, + } + _, err = accesstoken.ReadFromSecret(offlineAccessGrantedOIDCAccessTokenSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid accesstoken secret") + r.NoError(kubeInformerClient.Tracker().Add(offlineAccessGrantedOIDCAccessTokenSessionSecret)) + r.NoError(kubeClient.Tracker().Add(offlineAccessGrantedOIDCAccessTokenSessionSecret)) + + offlineAccessNotGrantedOIDCAccessTokenSession := &accesstoken.Session{ + Version: "2", + Request: &fosite.Request{ + GrantedScope: fosite.Arguments{"scope1", "scope2"}, + ID: "request-id-2", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + offlineAccessNotGrantedOIDCAccessTokenSessionJSON, err := json.Marshal(offlineAccessNotGrantedOIDCAccessTokenSession) + r.NoError(err) + offlineAccessNotGrantedOIDCAccessTokenSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "offlineAccessNotGrantedOIDCAccessTokenSession", + Namespace: installedInNamespace, + UID: "uid-456", + ResourceVersion: "rv-456", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": accesstoken.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": offlineAccessNotGrantedOIDCAccessTokenSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + accesstoken.TypeLabelValue, + } + _, err = accesstoken.ReadFromSecret(offlineAccessNotGrantedOIDCAccessTokenSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid accesstoken secret") + r.NoError(kubeInformerClient.Tracker().Add(offlineAccessNotGrantedOIDCAccessTokenSessionSecret)) + r.NoError(kubeClient.Tracker().Add(offlineAccessNotGrantedOIDCAccessTokenSessionSecret)) + }) + + it("should revoke upstream tokens only from the active authcode secrets and delete them all", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // The upstream refresh token is only revoked for the downstream session which had offline_access granted. + idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // Both session secrets are deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession"), + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + testutil.NewPreconditions("uid-456", "rv-456"), + }, + *deleteOptions, + ) + }) + }) + + when("there are valid, expired refresh secrets", func() { + it.Before(func() { + oidcRefreshSession := &refreshtoken.Session{ + Version: "2", + Request: &fosite.Request{ + ID: "request-id-1", + Client: &clientregistry.Client{}, + Session: &psession.PinnipedSession{ + Custom: &psession.CustomSessionData{ + ProviderUID: "upstream-oidc-provider-uid", + ProviderName: "upstream-oidc-provider-name", + ProviderType: psession.ProviderTypeOIDC, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: "fake-upstream-refresh-token", + }, + }, + }, + }, + } + oidcRefreshSessionJSON, err := json.Marshal(oidcRefreshSession) + r.NoError(err) + oidcRefreshSessionSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oidcRefreshSession", + Namespace: installedInNamespace, + UID: "uid-123", + ResourceVersion: "rv-123", + Annotations: map[string]string{ + "storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339), + }, + Labels: map[string]string{ + "storage.pinniped.dev/type": refreshtoken.TypeLabelValue, + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": oidcRefreshSessionJSON, + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/" + refreshtoken.TypeLabelValue, + } + _, err = refreshtoken.ReadFromSecret(oidcRefreshSessionSecret) + r.NoError(err, "the test author accidentally formed an invalid refresh token secret") + r.NoError(kubeInformerClient.Tracker().Add(oidcRefreshSessionSecret)) + r.NoError(kubeClient.Tracker().Add(oidcRefreshSessionSecret)) + }) + + it("should revoke upstream tokens from the secrets and delete them all", func() { + happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder(). + WithName("upstream-oidc-provider-name"). + WithResourceUID("upstream-oidc-provider-uid"). + WithRevokeRefreshTokenError(nil) + idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build()) + + startInformersAndController(idpListerBuilder.Build()) + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + // The upstream refresh token is revoked. + idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t, + "upstream-oidc-provider-name", + &oidctestutil.RevokeRefreshTokenArgs{ + Ctx: syncContext.Context, + RefreshToken: "fake-upstream-refresh-token", + }, + ) + + // The secret is deleted. + r.ElementsMatch( + []kubetesting.Action{ + kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "oidcRefreshSession"), + }, + kubeClient.Actions(), + ) + r.ElementsMatch( + []metav1.DeleteOptions{ + testutil.NewPreconditions("uid-123", "rv-123"), + }, + *deleteOptions, + ) + }) + }) + when("very little time has passed since the previous sync call", func() { it.Before(func() { // Add a secret that will expire in 20 seconds. @@ -277,7 +1006,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("should do nothing to avoid being super chatty since it is called for every change to any Secret, until more time has passed", func() { - startInformersAndController() + startInformersAndController(nil) require.Empty(t, kubeClient.Actions()) // Run sync once with the current time set to frozenTime. @@ -342,7 +1071,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("does not delete that secret", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.ElementsMatch( @@ -391,7 +1120,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) { }) it("ignores the error and continues on to delete the next expired Secret", func() { - startInformersAndController() + startInformersAndController(nil) r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.ElementsMatch( diff --git a/internal/crud/crud.go b/internal/crud/crud.go index 4211ae108..57e73b2a8 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 8a910b8a3..59d328d04 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/accesstoken/accesstoken.go b/internal/fositestorage/accesstoken/accesstoken.go index dbd57fb35..792b76e71 100644 --- a/internal/fositestorage/accesstoken/accesstoken.go +++ b/internal/fositestorage/accesstoken/accesstoken.go @@ -10,6 +10,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" @@ -42,7 +43,7 @@ type accessTokenStorage struct { storage crud.Storage } -type session struct { +type Session struct { Request *fosite.Request `json:"request"` Version string `json:"version"` } @@ -51,6 +52,23 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt return &accessTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } +// ReadFromSecret reads the contents of a Secret as a Session. +func ReadFromSecret(secret *v1.Secret) (*Session, error) { + session := newValidEmptyAccessTokenSession() + err := crud.FromSecret(TypeLabelValue, secret, session) + if err != nil { + return nil, err + } + if session.Version != accessTokenStorageVersion { + return nil, fmt.Errorf("%w: access token session has version %s instead of %s", + ErrInvalidAccessTokenRequestVersion, session.Version, accessTokenStorageVersion) + } + if session.Request.ID == "" { + return nil, fmt.Errorf("malformed access token session: %w", ErrInvalidAccessTokenRequestData) + } + return session, nil +} + func (a *accessTokenStorage) RevokeAccessToken(ctx context.Context, requestID string) error { return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) } @@ -64,7 +82,7 @@ func (a *accessTokenStorage) CreateAccessTokenSession(ctx context.Context, signa _, err = a.storage.Create( ctx, signature, - &session{Request: request, Version: accessTokenStorageVersion}, + &Session{Request: request, Version: accessTokenStorageVersion}, map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, ) return err @@ -84,7 +102,7 @@ func (a *accessTokenStorage) DeleteAccessTokenSession(ctx context.Context, signa return a.storage.Delete(ctx, signature) } -func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) { +func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (*Session, string, error) { session := newValidEmptyAccessTokenSession() rv, err := a.storage.Get(ctx, signature, session) @@ -108,8 +126,8 @@ func (a *accessTokenStorage) getSession(ctx context.Context, signature string) ( return session, rv, nil } -func newValidEmptyAccessTokenSession() *session { - return &session{ +func newValidEmptyAccessTokenSession() *Session { + return &Session{ Request: &fosite.Request{ Client: &clientregistry.Client{}, Session: &psession.PinnipedSession{}, diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index dc4687427..5f3933df9 100644 --- a/internal/fositestorage/accesstoken/accesstoken_test.go +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" "github.com/pkg/errors" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -277,3 +278,119 @@ func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInt secrets := client.CoreV1().Secrets(namespace) return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) } + +func TestReadFromSecret(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + wantSession *Session + wantErr string + }{ + { + name: "happy path", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + 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/access-token", + }, + wantSession: &Session{ + Version: "2", + 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-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + 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-access-token", + }, + wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-access-token must equal storage.pinniped.dev/access-token", + }, + { + name: "wrong session version", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + 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/access-token", + }, + wantErr: "access token request data has wrong version: access token session has version wrong-version-here instead of 2", + }, + { + name: "missing request", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-access-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "access-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/access-token", + }, + wantErr: "malformed access token session: access token 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/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 3418f672b..50c35788e 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" @@ -38,7 +39,7 @@ type authorizeCodeStorage struct { storage crud.Storage } -type AuthorizeCodeSession struct { +type Session struct { Active bool `json:"active"` Request *fosite.Request `json:"request"` Version string `json:"version"` @@ -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 a Session. +func ReadFromSecret(secret *v1.Secret) (*Session, 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 @@ -70,7 +88,7 @@ func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, s // of the consent authorization request. It is used to identify the session. // signature for lookup in the DB - _, err = a.storage.Create(ctx, signature, &AuthorizeCodeSession{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil) + _, err = a.storage.Create(ctx, signature, &Session{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil) return err } @@ -108,7 +126,7 @@ func (a *authorizeCodeStorage) InvalidateAuthorizeCodeSession(ctx context.Contex return nil } -func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) (*AuthorizeCodeSession, string, error) { +func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) (*Session, string, error) { session := NewValidEmptyAuthorizeCodeSession() rv, err := a.storage.Get(ctx, signature, session) @@ -137,8 +155,8 @@ func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) return session, rv, nil } -func NewValidEmptyAuthorizeCodeSession() *AuthorizeCodeSession { - return &AuthorizeCodeSession{ +func NewValidEmptyAuthorizeCodeSession() *Session { + return &Session{ Request: &fosite.Request{ Client: &clientregistry.Client{}, Session: &psession.PinnipedSession{}, diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 50ac0c28f..fdb9653ed 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 *Session + 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: &Session{ + 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/fositestorage/refreshtoken/refreshtoken.go b/internal/fositestorage/refreshtoken/refreshtoken.go index c2d4336ce..8faddd5bc 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken.go +++ b/internal/fositestorage/refreshtoken/refreshtoken.go @@ -10,6 +10,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" @@ -42,7 +43,7 @@ type refreshTokenStorage struct { storage crud.Storage } -type session struct { +type Session struct { Request *fosite.Request `json:"request"` Version string `json:"version"` } @@ -51,6 +52,23 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt return &refreshTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)} } +// ReadFromSecret reads the contents of a Secret as a Session. +func ReadFromSecret(secret *v1.Secret) (*Session, error) { + session := newValidEmptyRefreshTokenSession() + err := crud.FromSecret(TypeLabelValue, secret, session) + if err != nil { + return nil, err + } + if session.Version != refreshTokenStorageVersion { + return nil, fmt.Errorf("%w: refresh token session has version %s instead of %s", + ErrInvalidRefreshTokenRequestVersion, session.Version, refreshTokenStorageVersion) + } + if session.Request.ID == "" { + return nil, fmt.Errorf("malformed refresh token session: %w", ErrInvalidRefreshTokenRequestData) + } + return session, nil +} + func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID string) error { return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID) } @@ -64,7 +82,7 @@ func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, sig _, err = a.storage.Create( ctx, signature, - &session{Request: request, Version: refreshTokenStorageVersion}, + &Session{Request: request, Version: refreshTokenStorageVersion}, map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()}, ) return err @@ -84,7 +102,7 @@ func (a *refreshTokenStorage) DeleteRefreshTokenSession(ctx context.Context, sig return a.storage.Delete(ctx, signature) } -func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) { +func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) (*Session, string, error) { session := newValidEmptyRefreshTokenSession() rv, err := a.storage.Get(ctx, signature, session) @@ -108,8 +126,8 @@ func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) return session, rv, nil } -func newValidEmptyRefreshTokenSession() *session { - return &session{ +func newValidEmptyRefreshTokenSession() *Session { + return &Session{ Request: &fosite.Request{ Client: &clientregistry.Client{}, Session: &psession.PinnipedSession{}, diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index e8a155c9f..92c728e94 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/ory/fosite" + "github.com/ory/fosite/handler/openid" "github.com/pkg/errors" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -277,3 +278,119 @@ func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInt secrets := client.CoreV1().Secrets(namespace) return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime) } + +func TestReadFromSecret(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + wantSession *Session + wantErr string + }{ + { + name: "happy path", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + 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/refresh-token", + }, + wantSession: &Session{ + Version: "2", + 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-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + 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-refresh-token", + }, + wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-refresh-token must equal storage.pinniped.dev/refresh-token", + }, + { + name: "wrong session version", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + 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/refresh-token", + }, + wantErr: "refresh token request data has wrong version: refresh token session has version wrong-version-here instead of 2", + }, + { + name: "missing request", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4", + ResourceVersion: "", + Labels: map[string]string{ + "storage.pinniped.dev/type": "refresh-token", + }, + }, + Data: map[string][]byte{ + "pinniped-storage-data": []byte(`{"version":"2","active": true}`), + "pinniped-storage-version": []byte("1"), + }, + Type: "storage.pinniped.dev/refresh-token", + }, + wantErr: "malformed refresh token session: refresh token 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 fd8b7dd91..046f18494 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 b5f2c2159..a054a4c84 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 85211f9a0..ae87639cf 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -125,6 +125,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 a1513011f..03603bdbb 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -68,6 +68,13 @@ type PerformRefreshArgs struct { ExpectedSubject 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 { @@ -137,6 +144,7 @@ type TestUpstreamOIDCIdentityProvider struct { ClientID string ResourceUID types.UID AuthorizationURL url.URL + RevocationURL *url.URL UsernameClaim string GroupsClaim string Scopes []string @@ -158,6 +166,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 @@ -166,6 +176,8 @@ type TestUpstreamOIDCIdentityProvider struct { passwordCredentialsGrantAndValidateTokensArgs []*PasswordCredentialsGrantAndValidateTokensArgs performRefreshCallCount int performRefreshArgs []*PerformRefreshArgs + revokeRefreshTokenCallCount int + revokeRefreshTokenArgs []*RevokeRefreshTokenArgs validateTokenCallCount int validateTokenArgs []*ValidateTokenArgs } @@ -192,6 +204,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 } @@ -262,6 +278,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 } @@ -273,6 +301,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) @@ -513,6 +552,43 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes ) } +func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeRefreshToken( + t *testing.T, + expectedPerformedByUpstreamName string, + expectedArgs *RevokeRefreshTokenArgs, +) { + t.Helper() + var actualArgs *RevokeRefreshTokenArgs + var actualNameOfUpstreamWhichMadeCall string + actualCallCountAcrossAllOIDCUpstreams := 0 + for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { + callCountOnThisUpstream := upstreamOIDC.revokeRefreshTokenCallCount + actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream + if callCountOnThisUpstream == 1 { + actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name + actualArgs = upstreamOIDC.revokeRefreshTokenArgs[0] + } + } + require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, + "should have been exactly one call to RevokeRefreshToken() by all OIDC upstreams", + ) + require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall, + "RevokeRefreshToken() was called on the wrong OIDC upstream", + ) + require.Equal(t, expectedArgs, actualArgs) +} + +func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeRefreshToken(t *testing.T) { + t.Helper() + actualCallCountAcrossAllOIDCUpstreams := 0 + for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { + actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.revokeRefreshTokenCallCount + } + require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, + "expected exactly zero calls to RevokeRefreshToken()", + ) +} + func NewUpstreamIDPListerBuilder() *UpstreamIDPListerBuilder { return &UpstreamIDPListerBuilder{} } @@ -534,6 +610,7 @@ type TestUpstreamOIDCIdentityProviderBuilder struct { authcodeExchangeErr error passwordGrantErr error performRefreshErr error + revokeRefreshTokenErr error validateTokenErr error } @@ -650,6 +727,11 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenError(err err return u } +func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeRefreshTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder { + u.revokeRefreshTokenErr = err + return u +} + func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdentityProvider { return &TestUpstreamOIDCIdentityProvider{ Name: u.name, @@ -679,6 +761,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 7d4006c88..437eb6efc 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,105 @@ 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 { + plog.Trace("RevokeRefreshToken() was called but upstream provider has no available revocation endpoint", "providerName", p.Name) + 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! + plog.Trace("RevokeRefreshToken() got 200 OK response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) + return false, nil + case http.StatusBadRequest: + // Bad request might be due to bad client auth method. Try to detect that. + plog.Trace("RevokeRefreshToken() got 400 Bad Request response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth) + 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 + plog.Trace("RevokeRefreshToken()'s 400 Bad Request response from provider's revocation endpoint was type 'invalid_client'", "providerName", p.Name, "usedBasicAuth", useBasicAuth) + return true, err + default: + // Any other error is probably not due to failed client auth. + plog.Trace("RevokeRefreshToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", resp.StatusCode) + 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 342683fce..f89065620 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