diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 509b7ac09..fef22fd6d 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -417,12 +417,11 @@ func (c *jwtCacheFillerController) validateProviderJWKSURL(provider *coreosoidc. return pJSON.JWKSURL, conditions, fmt.Errorf("%s", msg) } - msg := fmt.Sprintf("jwks_uri (%s) is a valid URL", parsedJWKSURL) conditions = append(conditions, &metav1.Condition{ Type: typeJWKSURLValid, Status: metav1.ConditionTrue, Reason: reasonSuccess, - Message: msg, + Message: "jwks_uri is a valid URL", }) return pJSON.JWKSURL, conditions, nil } @@ -474,12 +473,11 @@ func (c *jwtCacheFillerController) validateIssuer(issuer string, conditions []*m return nil, conditions, false } - msg := fmt.Sprintf("spec.issuer (%s) is a valid URL", issuer) conditions = append(conditions, &metav1.Condition{ Type: typeIssuerURLValid, Status: metav1.ConditionTrue, Reason: reasonSuccess, - Message: msg, + Message: "issuer is a valid URL", }) return issuerURL, conditions, true } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index e5e08a704..ae4ffe990 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -291,14 +291,14 @@ func TestController(t *testing.T) { } } - happyIssuerURLValid := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition { + happyIssuerURLValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { return metav1.Condition{ Type: "IssuerURLValid", Status: "True", ObservedGeneration: observedGeneration, LastTransitionTime: time, Reason: "Success", - Message: fmt.Sprintf("spec.issuer (%s) is a valid URL", issuer), + Message: "issuer is a valid URL", } } sadIssuerURLValidInvalid := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition { @@ -393,16 +393,14 @@ func TestController(t *testing.T) { } } - happyJWKSURLValid := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition { - parsed, err := url.Parse(issuer) - require.NoError(t, err) + happyJWKSURLValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { return metav1.Condition{ Type: "JWKSURLValid", Status: "True", ObservedGeneration: observedGeneration, LastTransitionTime: time, Reason: "Success", - Message: fmt.Sprintf("jwks_uri (https://%s/jwks.json) is a valid URL", parsed.Host), + Message: "jwks_uri is a valid URL", } } unknownJWKSURLValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { @@ -440,8 +438,8 @@ func TestController(t *testing.T) { return status.SortConditionsByType([]metav1.Condition{ happyAuthenticatorValid(someTime, observedGeneration), happyDiscoveryURLValid(someTime, observedGeneration), - happyIssuerURLValid(issuer, someTime, observedGeneration), - happyJWKSURLValid(issuer, someTime, observedGeneration), + happyIssuerURLValid(someTime, observedGeneration), + happyJWKSURLValid(someTime, observedGeneration), happyReadyCondition(someTime, observedGeneration), happyTLSConfigurationValid(someTime, observedGeneration), }) @@ -784,7 +782,7 @@ func TestController(t *testing.T) { wantStatusConditions: status.ReplaceConditions( allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0), []metav1.Condition{ - happyIssuerURLValid(someOtherLocalhostIssuer, frozenMetav1Now, 0), + happyIssuerURLValid(frozenMetav1Now, 0), sadReadyCondition(frozenMetav1Now, 0), sadDiscoveryURLValidConnectionRefused(someOtherLocalhostIssuer, frozenMetav1Now, 0), unknownAuthenticatorValid(frozenMetav1Now, 0), @@ -812,7 +810,7 @@ func TestController(t *testing.T) { wantStatusConditions: status.ReplaceConditions( allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0), []metav1.Condition{ - happyIssuerURLValid(badIssuerInvalidJWKSURI, frozenMetav1Now, 0), + happyIssuerURLValid(frozenMetav1Now, 0), sadReadyCondition(frozenMetav1Now, 0), unknownAuthenticatorValid(frozenMetav1Now, 0), sadJWKSURLValidParseURI("https://.café .com/café/café/café/coffee/jwks.json", frozenMetav1Now, 0), @@ -834,7 +832,7 @@ func TestController(t *testing.T) { wantStatusConditions: status.ReplaceConditions( allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0), []metav1.Condition{ - happyIssuerURLValid(badIssuerInvalidJWKSURIScheme, frozenMetav1Now, 0), + happyIssuerURLValid(frozenMetav1Now, 0), sadReadyCondition(frozenMetav1Now, 0), unknownAuthenticatorValid(frozenMetav1Now, 0), sadJWKSURLValidScheme("http://.café.com/café/café/café/coffee/jwks.json", frozenMetav1Now, 0), diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index e606336bb..544e2f075 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -67,8 +67,15 @@ func TestSuccessfulCredentialRequest_Browser(t *testing.T) { }, }, { - name: "jwt authenticator", - authenticator: testlib.CreateTestJWTAuthenticatorForCLIUpstream, + name: "jwt authenticator", + authenticator: func(ctx context.Context, t *testing.T) corev1.TypedLocalObjectReference { + authenticator := testlib.CreateTestJWTAuthenticatorForCLIUpstream(ctx, t) + return corev1.TypedLocalObjectReference{ + APIGroup: &auth1alpha1.SchemeGroupVersion.Group, + Kind: "JWTAuthenticator", + Name: authenticator.Name, + } + }, token: func(t *testing.T) (string, string, []string) { pinnipedExe := testlib.PinnipedCLIPath(t) credOutput, _ := runPinnipedLoginOIDC(ctx, t, pinnipedExe) diff --git a/test/integration/concierge_jwtauthenticator_status_test.go b/test/integration/concierge_jwtauthenticator_status_test.go new file mode 100644 index 000000000..8d9f61f00 --- /dev/null +++ b/test/integration/concierge_jwtauthenticator_status_test.go @@ -0,0 +1,297 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "context" + "encoding/base64" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1" + "go.pinniped.dev/test/testlib" +) + +// Never run this test in parallel since deleting all federation domains is disruptive, see main_test.go. +func TestConciergeJWTAuthenticatorStatus_Disruptive(t *testing.T) { + env := testlib.IntegrationEnv(t) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + t.Cleanup(cancel) + + tests := []struct { + name string + run func(t *testing.T) + }{ + { + name: "valid spec with no errors and all good status conditions and phase", + run: func(t *testing.T) { + jwtAuthenticator := testlib.CreateTestJWTAuthenticator(ctx, t, v1alpha1.JWTAuthenticatorSpec{ + Issuer: env.SupervisorUpstreamOIDC.Issuer, + Audience: "some-fake-audience", + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)), + }, + }, v1alpha1.JWTAuthenticatorPhaseReady) + + testlib.WaitForJWTAuthenticatorStatusConditions( + ctx, t, + jwtAuthenticator.Name, + allSuccessfulJWTAuthenticatorConditions()) + }, + }, { + name: "invalid with bad issuer", + run: func(t *testing.T) { + fakeIssuerURL := "https://127.0.0.1:443/some-fake-issuer" + jwtAuthenticator := testlib.CreateTestJWTAuthenticator(ctx, t, v1alpha1.JWTAuthenticatorSpec{ + Issuer: fakeIssuerURL, + Audience: "some-fake-audience", + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)), + }, + }, v1alpha1.JWTAuthenticatorPhaseError) + + testlib.WaitForJWTAuthenticatorStatusConditions( + ctx, t, + jwtAuthenticator.Name, + replaceSomeConditions( + allSuccessfulJWTAuthenticatorConditions(), + []metav1.Condition{ + { + Type: "Ready", + Status: "False", + Reason: "NotReady", + Message: "the JWTAuthenticator is not ready: see other conditions for details", + }, { + Type: "AuthenticatorValid", + Status: "Unknown", + Reason: "UnableToValidate", + Message: "unable to validate; other issues present", + }, { + Type: "JWKSURLValid", + Status: "Unknown", + Reason: "UnableToValidate", + Message: "unable to validate; other issues present", + }, { + Type: "DiscoveryURLValid", + Status: "False", + Reason: "InvalidDiscoveryProbe", + Message: fmt.Sprintf(`could not perform oidc discovery on provider issuer: Get "%s/.well-known/openid-configuration": dial tcp 127.0.0.1:443: connect: connection refused`, fakeIssuerURL), + }, + }, + )) + }, + }, + } + for _, test := range tests { + tt := test + t.Run(tt.name, func(t *testing.T) { + tt.run(t) + }) + } +} + +func TestConciergeJWTAuthenticatorCRDValidations_Parallel(t *testing.T) { + jwtAuthenticatorClient := testlib.NewConciergeClientset(t).AuthenticationV1alpha1().JWTAuthenticators() + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + t.Cleanup(cancel) + + objectMeta := testlib.ObjectMetaWithRandomName(t, "jwt-authenticator") + tests := []struct { + name string + jwtAuthenticator *v1alpha1.JWTAuthenticator + wantErr string + // some tests change the environment (api group suffix pinniped.dev->walrus.tld) so + // we need to be able to compare against several error strings + wantOneOfErr []string + }{ + { + name: "issuer can not be empty string", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: objectMeta, + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "", + Audience: "fake-audience", + }, + }, + wantOneOfErr: []string{ + `JWTAuthenticator.authentication.concierge.pinniped.dev "` + objectMeta.Name + `" is invalid: ` + + `spec.issuer: Invalid value: "": spec.issuer in body should be at least 1 chars long`, + `JWTAuthenticator.authentication.concierge.walrus.tld "` + objectMeta.Name + `" is invalid: ` + + `spec.issuer: Invalid value: "": spec.issuer in body should be at least 1 chars long`, + }, + }, + { + name: "audience can not be empty string", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: objectMeta, + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://example.com", + Audience: "", + }, + }, + wantOneOfErr: []string{ + `JWTAuthenticator.authentication.concierge.pinniped.dev "` + objectMeta.Name + `" is invalid: ` + + `spec.audience: Invalid value: "": spec.audience in body should be at least 1 chars long`, + `JWTAuthenticator.authentication.concierge.walrus.tld "` + objectMeta.Name + `" is invalid: ` + + `spec.audience: Invalid value: "": spec.audience in body should be at least 1 chars long`, + }, + }, + { + name: "issuer must be https", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: objectMeta, + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "http://www.example.com", + Audience: "foo", + }, + }, + wantOneOfErr: []string{ + `JWTAuthenticator.authentication.concierge.pinniped.dev "` + objectMeta.Name + `" is invalid: ` + + `spec.issuer: Invalid value: "http://www.example.com": spec.issuer in body should match '^https://'`, + `JWTAuthenticator.authentication.concierge.walrus.tld "` + objectMeta.Name + `" is invalid: ` + + `spec.issuer: Invalid value: "http://www.example.com": spec.issuer in body should match '^https://'`, + }, + }, + { + name: "minimum valid authenticator", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: testlib.ObjectMetaWithRandomName(t, "jwtauthenticator"), + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://www.example.com", + Audience: "foo", + }, + }, + }, + { + name: "minimum valid authenticator can have empty claims block", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: testlib.ObjectMetaWithRandomName(t, "jwtauthenticator"), + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://www.example.com", + Audience: "foo", + Claims: v1alpha1.JWTTokenClaims{}, + }, + }, + }, + { + name: "minimum valid authenticator can have empty group claim and empty username claim", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: testlib.ObjectMetaWithRandomName(t, "jwtauthenticator"), + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://www.example.com", + Audience: "foo", + Claims: v1alpha1.JWTTokenClaims{ + Groups: "", + Username: "", + }, + }, + }, + }, + { + name: "minimum valid authenticator can have empty TLS block", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: testlib.ObjectMetaWithRandomName(t, "jwtauthenticator"), + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://www.example.com", + Audience: "foo", + Claims: v1alpha1.JWTTokenClaims{ + Groups: "", + Username: "", + }, + TLS: &v1alpha1.TLSSpec{}, + }, + }, + }, + { + name: "minimum valid authenticator can have empty TLS CertificateAuthorityData", + jwtAuthenticator: &v1alpha1.JWTAuthenticator{ + ObjectMeta: testlib.ObjectMetaWithRandomName(t, "jwtauthenticator"), + Spec: v1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://www.example.com", + Audience: "foo", + Claims: v1alpha1.JWTTokenClaims{ + Groups: "", + Username: "", + }, + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: "pretend-this-is-a-certificate", + }, + }, + }, + }, + } + for _, test := range tests { + tt := test + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, createErr := jwtAuthenticatorClient.Create(ctx, tt.jwtAuthenticator, metav1.CreateOptions{}) + + t.Cleanup(func() { + // delete if it exists + delErr := jwtAuthenticatorClient.Delete(ctx, tt.jwtAuthenticator.Name, metav1.DeleteOptions{}) + if !errors.IsNotFound(delErr) { + require.NoError(t, delErr) + } + }) + + if tt.wantErr != "" && tt.wantOneOfErr != nil { + require.NoError(t, fmt.Errorf("test '%s' should not use both tt.wantErr and tt.wantOneOfErr", tt.name)) + } + + if tt.wantErr == "" && tt.wantOneOfErr == nil { + require.NoError(t, createErr) + } + + if tt.wantErr != "" { + wantErr := tt.wantErr + require.EqualError(t, createErr, wantErr) + } + if tt.wantOneOfErr != nil { + wantOneOfErr := tt.wantOneOfErr + require.Contains(t, wantOneOfErr, createErr.Error()) + } + }) + } +} + +func allSuccessfulJWTAuthenticatorConditions() []metav1.Condition { + return []metav1.Condition{{ + Type: "AuthenticatorValid", + Status: "True", + Reason: "Success", + Message: "authenticator initialized", + }, { + Type: "DiscoveryURLValid", + Status: "True", + Reason: "Success", + Message: "discovery performed successfully", + }, { + Type: "IssuerURLValid", + Status: "True", + Reason: "Success", + Message: "issuer is a valid URL", + }, { + + Type: "JWKSURLValid", + Status: "True", + Reason: "Success", + Message: "jwks_uri is a valid URL", + }, { + Type: "Ready", + Status: "True", + Reason: "Success", + Message: "the JWTAuthenticator is ready", + }, { + Type: "TLSConfigurationValid", + Status: "True", + Reason: "Success", + Message: "valid TLS configuration", + }} +} diff --git a/test/testlib/client.go b/test/testlib/client.go index 57d7a6fd5..396c734f4 100644 --- a/test/testlib/client.go +++ b/test/testlib/client.go @@ -204,13 +204,12 @@ func CreateTestWebhookAuthenticator(ctx context.Context, t *testing.T) corev1.Ty } } -// CreateTestJWTAuthenticatorForCLIUpstream creates and returns a test JWTAuthenticator in -// $PINNIPED_TEST_CONCIERGE_NAMESPACE, which will be automatically deleted at the end of the current -// test's lifetime. It returns a corev1.TypedLocalObjectReference which describes the test JWT -// authenticator within the test namespace. +// CreateTestJWTAuthenticatorForCLIUpstream creates and returns a test JWTAuthenticator which will be automatically +// deleted at the end of the current test's lifetime. It returns a corev1.TypedLocalObjectReference which describes +// the test JWT authenticator within the test namespace. // // CreateTestJWTAuthenticatorForCLIUpstream gets the OIDC issuer info from IntegrationEnv().CLIUpstreamOIDC. -func CreateTestJWTAuthenticatorForCLIUpstream(ctx context.Context, t *testing.T) corev1.TypedLocalObjectReference { +func CreateTestJWTAuthenticatorForCLIUpstream(ctx context.Context, t *testing.T) *auth1alpha1.JWTAuthenticator { t.Helper() testEnv := IntegrationEnv(t) spec := auth1alpha1.JWTAuthenticatorSpec{ @@ -228,7 +227,8 @@ func CreateTestJWTAuthenticatorForCLIUpstream(ctx context.Context, t *testing.T) CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(testEnv.CLIUpstreamOIDC.CABundle)), } } - return CreateTestJWTAuthenticator(ctx, t, spec, auth1alpha1.JWTAuthenticatorPhaseReady) + authenticator := CreateTestJWTAuthenticator(ctx, t, spec, auth1alpha1.JWTAuthenticatorPhaseReady) + return authenticator } // CreateTestJWTAuthenticator creates and returns a test JWTAuthenticator which will be automatically deleted @@ -238,7 +238,7 @@ func CreateTestJWTAuthenticator( ctx context.Context, t *testing.T, spec auth1alpha1.JWTAuthenticatorSpec, - expectedStatus auth1alpha1.JWTAuthenticatorPhase) corev1.TypedLocalObjectReference { + expectedStatus auth1alpha1.JWTAuthenticatorPhase) *auth1alpha1.JWTAuthenticator { t.Helper() client := NewConciergeClientset(t) @@ -265,11 +265,7 @@ func CreateTestJWTAuthenticator( WaitForJWTAuthenticatorStatusPhase(ctx, t, jwtAuthenticator.Name, expectedStatus) - return corev1.TypedLocalObjectReference{ - APIGroup: &auth1alpha1.SchemeGroupVersion.Group, - Kind: "JWTAuthenticator", - Name: jwtAuthenticator.Name, - } + return jwtAuthenticator } func WaitForJWTAuthenticatorStatusPhase(ctx context.Context, t *testing.T, jwtAuthenticatorName string, expectPhase auth1alpha1.JWTAuthenticatorPhase) { @@ -283,6 +279,32 @@ func WaitForJWTAuthenticatorStatusPhase(ctx context.Context, t *testing.T, jwtAu }, 60*time.Second, 1*time.Second, "expected the JWTAuthenticator to have status %q", expectPhase) } +func WaitForJWTAuthenticatorStatusConditions(ctx context.Context, t *testing.T, jwtAuthenticatorName string, expectConditions []metav1.Condition) { + t.Helper() + jwtAuthenticatorClient := NewConciergeClientset(t).AuthenticationV1alpha1().JWTAuthenticators() + RequireEventuallyf(t, func(requireEventually *require.Assertions) { + fd, err := jwtAuthenticatorClient.Get(ctx, jwtAuthenticatorName, metav1.GetOptions{}) + requireEventually.NoError(err) + + requireEventually.Lenf(fd.Status.Conditions, len(expectConditions), + "wanted status conditions: %#v", expectConditions) + + for i, wantCond := range expectConditions { + actualCond := fd.Status.Conditions[i] + + // This is a cheat to avoid needing to make equality assertions on these fields. + requireEventually.NotZero(actualCond.LastTransitionTime) + wantCond.LastTransitionTime = actualCond.LastTransitionTime + requireEventually.NotZero(actualCond.ObservedGeneration) + wantCond.ObservedGeneration = actualCond.ObservedGeneration + + requireEventually.Equalf(wantCond, actualCond, + "wanted status conditions: %#v\nactual status conditions were: %#v\nnot equal at index %d", + expectConditions, fd.Status.Conditions, i) + } + }, 60*time.Second, 1*time.Second, "wanted JWTAuthenticator conditions") +} + // CreateTestFederationDomain creates and returns a test FederationDomain in the // $PINNIPED_TEST_SUPERVISOR_NAMESPACE, which will be automatically deleted at the end of the // current test's lifetime.