[release-0.31] Wait for authenticators to be initialized

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 {

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),
 	)