diff --git a/pkg/authentication/index.go b/pkg/authentication/index.go index 335664cd323..693024676b1 100644 --- a/pkg/authentication/index.go +++ b/pkg/authentication/index.go @@ -18,9 +18,14 @@ package authentication import ( "context" + "encoding/base64" "errors" "fmt" + "net/http" + "strings" + "time" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/klog/v2" kubeauthenticator "k8s.io/kubernetes/pkg/kubeapiserver/authenticator" @@ -30,6 +35,14 @@ import ( tenancyv1alpha1 "github.com/kcp-dev/sdk/apis/tenancy/v1alpha1" ) +// authenticatorSetupTimeout is duration to wait for an authenticator to +// be initialized, means it finished the discovery of its issuers and is +// ready to validate requests. +// +// 10s is the time upstream waits in the updateAuthenticationConfig when +// an authenticator is updated. +const authenticatorSetupTimeout = 10 * time.Second + // AuthenticatorIndex implements a mapping from workspace type to authenticator.Request. type AuthenticatorIndex interface { Lookup(wsType logicalcluster.Path) (authenticator.Request, bool) @@ -76,12 +89,11 @@ func buildAuthenticator( } ctx, cancel := context.WithCancelCause(lifecycleCtx) + logger := klog.FromContext(ctx).WithValues("controller", controllerName) authn, _, _, _, err := kubeAuthConfig.New(ctx) if err != nil { - logger := klog.FromContext(ctx).WithValues("controller", controllerName) logger.Error(err, "Failed to start workspace authenticator.") - cancel(fmt.Errorf("authenticator failed to start: %w", err)) return authenticatorState{}, err } @@ -92,9 +104,44 @@ func buildAuthenticator( return authenticatorState{}, errCauseEmpty } + if len(wac.Spec.JWT) > 0 { + // There shouldn't be an authenticator without at least one JWT + // at the very least because the spec validates this but better + // safe than sorry. + + // Wait for the authenticator to be initialized. + // + // The authenticators do have a healthcheck, however this healthcheck is not exposed + // and it is not easy to expose without intrusive changes. + // + // An alternative would've been to use the update function returned by kubeAuthConfig.New + // which _does_ wait until the authenticator's healthcheck are ready, however that would + // create each authenticator twice and just discard the first iteration. + // + // Instead the authenticator is validated by continually authenticating a dummy token + // created with the first issuer until the error is no longer "not initialized". + dummyToken := dummyTokenForIssuer(wac.Spec.JWT[0].Issuer.URL) + if err := wait.PollUntilContextTimeout(ctx, time.Second, authenticatorSetupTimeout, true, func(ctx context.Context) (bool, error) { + dummyReq, _ := http.NewRequestWithContext(ctx, http.MethodGet, "/", http.NoBody) + dummyReq.Header.Set("Authorization", "Bearer "+dummyToken) + _, _, err := authn.AuthenticateRequest(dummyReq) + return err == nil || !strings.Contains(err.Error(), "not initialized"), nil + }); err != nil { + logger.Error(err, "Failed to validate workspace authenticator.") + cancel(fmt.Errorf("authenticator failed to validate within %q: %w", authenticatorSetupTimeout, err)) + return authenticatorState{}, err + } + } + return authenticatorState{cancel: cancel, authenticator: authn}, nil } +func dummyTokenForIssuer(issuer string) string { + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"none"}`)) + payload := base64.RawURLEncoding.EncodeToString([]byte(fmt.Sprintf(`{"iss":%q}`, issuer))) + return header + "." + payload + "." +} + // wrapWithSecurityFilters wraps an authenticator with security filters // that prevent workspace-local auth from escalating to system-level access. func wrapWithSecurityFilters(authn authenticator.Request) authenticator.Request { diff --git a/test/e2e/authentication/workspace_test.go b/test/e2e/authentication/workspace_test.go index e77475e3183..350189d6168 100644 --- a/test/e2e/authentication/workspace_test.go +++ b/test/e2e/authentication/workspace_test.go @@ -21,6 +21,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/xrstf/mockoidc" @@ -35,7 +36,6 @@ import ( tenancyv1alpha1 "github.com/kcp-dev/sdk/apis/tenancy/v1alpha1" kcpclientset "github.com/kcp-dev/sdk/client/clientset/versioned/cluster" kcptesting "github.com/kcp-dev/sdk/testing" - kcptestinghelpers "github.com/kcp-dev/sdk/testing/helpers" "github.com/kcp-dev/kcp/pkg/authorization/bootstrap" "github.com/kcp-dev/kcp/test/e2e/fixtures/authfixtures" @@ -616,24 +616,20 @@ func TestWorkspaceOIDCTokenReview(t *testing.T) { }, } - var response *authenticationv1.TokenReview - kcptestinghelpers.Eventually(t, func() (bool, string) { - var err error - response, err = kubeClusterClient.Cluster(teamPath).AuthenticationV1().TokenReviews().Create(t.Context(), review, metav1.CreateOptions{}) - if err != nil { - return false, err.Error() - } - return response.Status.Authenticated, response.Status.Error - }, wait.ForeverTestTimeout, 500*time.Millisecond) - - require.Contains(t, response.Status.Audiences, kcpDefaultAudience) + // This check cannot happen in an .Eventually - when a valid request + // comes in it must succeed on the first try. + response, err := kubeClusterClient.Cluster(teamPath).AuthenticationV1().TokenReviews().Create(t.Context(), review, metav1.CreateOptions{}) + require.NoError(t, err) + // Ensure the token is valid and has the right audience + assert.True(t, response.Status.Authenticated, "token was not authenticated: %s", response.Status.Error) + assert.Contains(t, response.Status.Audiences, kcpDefaultAudience) // Ensure the correct clusters are mentioned in the TokenReview. - require.Equal(t, + assert.Equal(t, authenticationv1.ExtraValue{teamWs.Spec.Cluster}, response.Status.User.Extra["authentication.kcp.io/cluster-name"], ) - require.Contains(t, + assert.Contains(t, response.Status.User.Extra["authentication.kcp.io/scopes"], fmt.Sprintf("cluster:%s", teamWs.Spec.Cluster), )