From c436f84b3d5aa292634e0b2b6d4affae2b2e3188 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 11 Sep 2020 13:08:54 -0700 Subject: [PATCH] Fix a nil dereference crash in rest.go --- cmd/local-user-authenticator/main.go | 2 +- cmd/local-user-authenticator/main_test.go | 57 +++++++++++++------ internal/registry/credentialrequest/rest.go | 13 ++--- .../registry/credentialrequest/rest_test.go | 13 +++++ test/integration/credentialrequest_test.go | 10 ++-- 5 files changed, 63 insertions(+), 32 deletions(-) diff --git a/cmd/local-user-authenticator/main.go b/cmd/local-user-authenticator/main.go index 4d66ea4e..cf4cfae9 100644 --- a/cmd/local-user-authenticator/main.go +++ b/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 } diff --git a/cmd/local-user-authenticator/main_test.go b/cmd/local-user-authenticator/main_test.go index 837b3c2a..92b10dcd 100644 --- a/cmd/local-user-authenticator/main_test.go +++ b/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 := - "some-user", "other-user", "colon-user", "no-group-user", "one-group-user", "password-undefined-user", "empty-password-user", "undefined-groups-user" - uid, otherUID, colonUID, noGroupUID, oneGroupUID, passwordUndefinedUID, emptyPasswordUID, underfinedGroupsUID := - "some-uid", "other-uid", "colon-uid", "no-group-uid", "one-group-uid", "password-undefined-uid", "empty-password-uid", "undefined-groups-uid" + 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", "invalid-password-hash-user", "undefined-groups-user" + 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", "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), - Name: underfinedGroupsUser, - Namespace: "local-user-authenticator", + UID: types.UID(undefinedGroupsUID), + Name: undefinedGroupsUser, + 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, diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index b96a0538..6c12a6bf 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/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() == "" { - traceSuccess(t, authResponse.User, authenticated, false) + if !authenticated || authResponse == nil || authResponse.User == nil || authResponse.User.GetName() == "" { + 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 := "" - if user != nil { - userID = user.GetUID() + if response != nil && response.User != nil { + userID = response.User.GetUID() } t.Step("success", trace.Field{Key: "userID", Value: userID}, diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 935698c6..45b645d9 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/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:,idpAuthenticated:true,pinnipedAuthenticated:false`) + }) + it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookDoesNotReturnAnyUserInfo", func() { webhook := FakeToken{ returnResponse: &authenticator.Response{}, diff --git a/test/integration/credentialrequest_test.go b/test/integration/credentialrequest_test.go index cb9b0418..28557858 100644 --- a/test/integration/credentialrequest_test.go +++ b/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{})