Fix a nil dereference crash in rest.go

cmd/local-user-authenticator/main.go

@@ -135,7 +135,7 @@ func (w *webhook) ServeHTTP(rsp http.ResponseWriter, req *http.Request) {
 		[]byte(password),
 	) == nil
 	if !passwordMatches {
-		klog.InfoS("invalid password in request")
+		klog.InfoS("authentication failed: wrong password")
 		respondWithUnauthenticated(rsp)
 		return
 	}

cmd/local-user-authenticator/main_test.go

@@ -41,13 +41,15 @@ import (
 func TestWebhook(t *testing.T) {
 	t.Parallel()
 
+	const namespace = "local-user-authenticator"
+
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	user, otherUser, colonUser, noGroupUser, oneGroupUser, passwordUndefinedUser, emptyPasswordUser, underfinedGroupsUser :=
+	user, otherUser, colonUser, noGroupUser, oneGroupUser, passwordUndefinedUser, emptyPasswordUser, invalidPasswordHashUser, undefinedGroupsUser :=
-		"some-user", "other-user", "colon-user", "no-group-user", "one-group-user", "password-undefined-user", "empty-password-user", "undefined-groups-user"
+		"some-user", "other-user", "colon-user", "no-group-user", "one-group-user", "password-undefined-user", "empty-password-user", "invalid-password-hash-user", "undefined-groups-user"
-	uid, otherUID, colonUID, noGroupUID, oneGroupUID, passwordUndefinedUID, emptyPasswordUID, underfinedGroupsUID :=
+	uid, otherUID, colonUID, noGroupUID, oneGroupUID, passwordUndefinedUID, emptyPasswordUID, invalidPasswordHashUID, undefinedGroupsUID :=
-		"some-uid", "other-uid", "colon-uid", "no-group-uid", "one-group-uid", "password-undefined-uid", "empty-password-uid", "undefined-groups-uid"
+		"some-uid", "other-uid", "colon-uid", "no-group-uid", "one-group-uid", "password-undefined-uid", "empty-password-uid", "invalid-password-hash-uid", "undefined-groups-uid"
 	password, otherPassword, colonPassword, noGroupPassword, oneGroupPassword, undefinedGroupsPassword :=
 		"some-password", "other-password", "some-:-password", "no-group-password", "one-group-password", "undefined-groups-password"
 
@@ -66,7 +68,7 @@ func TestWebhook(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			UID:       types.UID(passwordUndefinedUID),
 			Name:      passwordUndefinedUser,
-			Namespace: "local-user-authenticator",
+			Namespace: namespace,
 		},
 		Data: map[string][]byte{
 			"groups": []byte(groups),
@@ -78,15 +80,27 @@ func TestWebhook(t *testing.T) {
 
 	require.NoError(t, kubeClient.Tracker().Add(&corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			UID:       types.UID(underfinedGroupsUID),
+			UID:       types.UID(undefinedGroupsUID),
-			Name:      underfinedGroupsUser,
+			Name:      undefinedGroupsUser,
-			Namespace: "local-user-authenticator",
+			Namespace: namespace,
 		},
 		Data: map[string][]byte{
 			"passwordHash": undefinedGroupsUserPasswordHash,
 		},
 	}))
 
+	require.NoError(t, kubeClient.Tracker().Add(&corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			UID:       types.UID(invalidPasswordHashUID),
+			Name:      invalidPasswordHashUser,
+			Namespace: namespace,
+		},
+		Data: map[string][]byte{
+			"groups":       []byte(groups),
+			"passwordHash": []byte("not a valid password hash"),
+		},
+	}))
+
 	secretInformer := createSecretInformer(t, kubeClient)
 
 	certProvider, caBundle, serverName := newCertProvider(t)
@@ -166,17 +180,27 @@ func TestWebhook(t *testing.T) {
 			wantHeaders: map[string][]string{"Content-Type": {"application/json"}},
 			wantBody:    unauthenticatedResponseJSON(),
 		},
+		{
+			name:        "when a user has an invalid password hash in the secret",
+			url:         goodURL,
+			method:      http.MethodPost,
+			headers:     goodRequestHeaders,
+			body:        func() (io.ReadCloser, error) { return newTokenReviewBody(invalidPasswordHashUser + ":foo") },
+			wantStatus:  http.StatusOK,
+			wantHeaders: map[string][]string{"Content-Type": {"application/json"}},
+			wantBody:    unauthenticatedResponseJSON(),
+		},
 		{
 			name:    "success for a user has no groups defined in the secret",
 			url:     goodURL,
 			method:  http.MethodPost,
 			headers: goodRequestHeaders,
 			body: func() (io.ReadCloser, error) {
-				return newTokenReviewBody(underfinedGroupsUser + ":" + undefinedGroupsPassword)
+				return newTokenReviewBody(undefinedGroupsUser + ":" + undefinedGroupsPassword)
 			},
 			wantStatus:  http.StatusOK,
 			wantHeaders: map[string][]string{"Content-Type": {"application/json"}},
-			wantBody:    authenticatedResponseJSON(underfinedGroupsUser, underfinedGroupsUID, []string{}),
+			wantBody:    authenticatedResponseJSON(undefinedGroupsUser, undefinedGroupsUID, []string{}),
 		},
 		{
 			name:        "when a user has empty string as their password",
@@ -389,15 +413,12 @@ func createSecretInformer(t *testing.T, kubeClient kubernetes.Interface) corev1i
 func newCertProvider(t *testing.T) (provider.DynamicTLSServingCertProvider, []byte, string) {
 	t.Helper()
 
-	ca, err := certauthority.New(pkix.Name{CommonName: "local-user-authenticator CA"}, time.Hour*24)
+	serverName := "local-user-authenticator"
+
+	ca, err := certauthority.New(pkix.Name{CommonName: serverName + " CA"}, time.Hour*24)
 	require.NoError(t, err)
 
-	serverName := "local-user-authenticator"
+	cert, err := ca.Issue(pkix.Name{CommonName: serverName}, []string{serverName}, time.Hour*24)
-	cert, err := ca.Issue(
-		pkix.Name{CommonName: serverName},
-		[]string{serverName},
-		time.Hour*24,
-	)
 	require.NoError(t, err)
 
 	certPEM, keyPEM, err := certauthority.ToPEM(cert)
@@ -482,7 +503,7 @@ func addSecretToFakeClientTracker(t *testing.T, kubeClient *kubernetesfake.Clien
 		ObjectMeta: metav1.ObjectMeta{
 			UID:       types.UID(uid),
 			Name:      username,
-			Namespace: "local-user-authenticator",
+			Namespace: namespace,
 		},
 		Data: map[string][]byte{
 			"passwordHash": passwordHash,

internal/registry/credentialrequest/rest.go

@@ -17,7 +17,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
-	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/registry/rest"
 	"k8s.io/utils/trace"
 
@@ -84,8 +83,8 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		traceFailureWithError(t, "webhook authentication", err)
 		return failureResponse(), nil
 	}
-	if !authenticated || authResponse.User == nil || authResponse.User.GetName() == "" {
+	if !authenticated || authResponse == nil || authResponse.User == nil || authResponse.User.GetName() == "" {
-		traceSuccess(t, authResponse.User, authenticated, false)
+		traceSuccess(t, authResponse, authenticated, false)
 		return failureResponse(), nil
 	}
 
@@ -105,7 +104,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		return failureResponse(), nil
 	}
 
-	traceSuccess(t, authResponse.User, authenticated, true)
+	traceSuccess(t, authResponse, authenticated, true)
 
 	return &pinnipedapi.CredentialRequest{
 		Status: pinnipedapi.CredentialRequestStatus{
@@ -171,10 +170,10 @@ func validateRequest(ctx context.Context, obj runtime.Object, createValidation r
 	return credentialRequest, nil
 }
 
-func traceSuccess(t *trace.Trace, user user.Info, webhookAuthenticated bool, pinnipedAuthenticated bool) {
+func traceSuccess(t *trace.Trace, response *authenticator.Response, webhookAuthenticated bool, pinnipedAuthenticated bool) {
 	userID := "<none>"
-	if user != nil {
+	if response != nil && response.User != nil {
-		userID = user.GetUID()
+		userID = response.User.GetUID()
 	}
 	t.Step("success",
 		trace.Field{Key: "userID", Value: userID},

internal/registry/credentialrequest/rest_test.go

@@ -194,6 +194,19 @@ func TestCreate(t *testing.T) {
 			requireOneLogStatement(r, logger, `"failure" failureType:webhook authentication,msg:some webhook error`)
 		})
 
+		it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookReturnsNilResponseWithAuthenticatedFalse", func() {
+			webhook := FakeToken{
+				returnResponse:        nil,
+				returnUnauthenticated: false,
+			}
+			storage := NewREST(&webhook, nil)
+
+			response, err := callCreate(context.Background(), storage, validCredentialRequest())
+
+			requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response)
+			requireOneLogStatement(r, logger, `"success" userID:<none>,idpAuthenticated:true,pinnipedAuthenticated:false`)
+		})
+
 		it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookDoesNotReturnAnyUserInfo", func() {
 			webhook := FakeToken{
 				returnResponse:        &authenticator.Response{},

test/integration/credentialrequest_test.go

@@ -37,11 +37,9 @@ func TestSuccessfulCredentialRequest(t *testing.T) {
 	response, err := makeRequest(t, validCredentialRequestSpecWithRealToken(t))
 	require.NoError(t, err)
 
-	// Note: If this assertion fails then your TMC token might have expired. Get a fresh one and try again.
-	require.Empty(t, response.Status.Message)
-
-	require.Empty(t, response.Spec)
 	require.NotNil(t, response.Status.Credential)
+	require.Empty(t, response.Status.Message)
+	require.Empty(t, response.Spec)
 	require.Empty(t, response.Status.Credential.Token)
 	require.NotEmpty(t, response.Status.Credential.ClientCertificateData)
 	require.Equal(t, testUsername, getCommonName(t, response.Status.Credential.ClientCertificateData))
@@ -81,7 +79,7 @@ func TestSuccessfulCredentialRequest(t *testing.T) {
 			},
 		})
 
-		// Use the client which is authenticated as the TMC user to list namespaces
+		// Use the client which is authenticated as the test user to list namespaces
 		var listNamespaceResponse *v1.NamespaceList
 		var canListNamespaces = func() bool {
 			listNamespaceResponse, err = clientWithCertFromCredentialRequest.CoreV1().Namespaces().List(ctx, metav1.ListOptions{})
@@ -112,7 +110,7 @@ func TestSuccessfulCredentialRequest(t *testing.T) {
 				},
 			})
 
-			// Use the client which is authenticated as the TMC group to list namespaces
+			// Use the client which is authenticated as the test user to list namespaces
 			var listNamespaceResponse *v1.NamespaceList
 			var canListNamespaces = func() bool {
 				listNamespaceResponse, err = clientWithCertFromCredentialRequest.CoreV1().Namespaces().List(ctx, metav1.ListOptions{})