From 35479e2978c567adf885bef404ee0a56b65f5dd6 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Mon, 3 May 2021 14:06:49 -0400 Subject: [PATCH] cred req: disallow lossy user info translations Signed-off-by: Monis Khan --- internal/registry/credentialrequest/rest.go | 18 +++++++- .../registry/credentialrequest/rest_test.go | 45 +++++++++++++++++-- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index 9c9165d5..6e5b44b4 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -101,7 +101,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation traceFailureWithError(t, "token authentication", err) return failureResponse(), nil } - if userInfo == nil || userInfo.GetName() == "" { + if ok := isUserInfoValid(userInfo); !ok { traceSuccess(t, userInfo, false) return failureResponse(), nil } @@ -169,13 +169,29 @@ func validateRequest(ctx context.Context, obj runtime.Object, createValidation r return credentialRequest, nil } +func isUserInfoValid(userInfo user.Info) bool { + switch { + case userInfo == nil, // must be non-nil + len(userInfo.GetName()) == 0, // must have a username, groups are optional + len(userInfo.GetUID()) != 0, // certs cannot assert UID + len(userInfo.GetExtra()) != 0: // certs cannot assert extra + return false + + default: + return true + } +} + func traceSuccess(t *trace.Trace, userInfo user.Info, authenticated bool) { userID := "" + hasExtra := false if userInfo != nil { userID = userInfo.GetUID() + hasExtra = len(userInfo.GetExtra()) > 0 } t.Step("success", trace.Field{Key: "userID", Value: userID}, + trace.Field{Key: "hasExtra", Value: hasExtra}, trace.Field{Key: "authenticated", Value: authenticated}, ) } diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 78d5dd73..f7a7cf7b 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -86,7 +86,6 @@ func TestCreate(t *testing.T) { requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). Return(&user.DefaultInfo{ Name: "test-user", - UID: "test-user-uid", Groups: []string{"test-group-1", "test-group-2"}, }, nil) @@ -118,7 +117,7 @@ func TestCreate(t *testing.T) { }, }, }) - requireOneLogStatement(r, logger, `"success" userID:test-user-uid,authenticated:true`) + requireOneLogStatement(r, logger, `"success" userID:,hasExtra:false,authenticated:true`) }) it("CreateFailsWithValidTokenWhenCertIssuerFails", func() { @@ -154,7 +153,7 @@ func TestCreate(t *testing.T) { response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - requireOneLogStatement(r, logger, `"success" userID:,authenticated:false`) + requireOneLogStatement(r, logger, `"success" userID:,hasExtra:false,authenticated:false`) }) it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookFails", func() { @@ -184,7 +183,45 @@ func TestCreate(t *testing.T) { response, err := callCreate(context.Background(), storage, req) requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) - requireOneLogStatement(r, logger, `"success" userID:,authenticated:false`) + requireOneLogStatement(r, logger, `"success" userID:,hasExtra:false,authenticated:false`) + }) + + it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookReturnsAUserWithUID", func() { + req := validCredentialRequest() + + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). + Return(&user.DefaultInfo{ + Name: "test-user", + UID: "test-uid", + Groups: []string{"test-group-1", "test-group-2"}, + }, nil) + + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}) + + response, err := callCreate(context.Background(), storage, req) + + requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) + requireOneLogStatement(r, logger, `"success" userID:test-uid,hasExtra:false,authenticated:false`) + }) + + it("CreateSucceedsWithAnUnauthenticatedStatusWhenWebhookReturnsAUserWithExtra", func() { + req := validCredentialRequest() + + requestAuthenticator := credentialrequestmocks.NewMockTokenCredentialRequestAuthenticator(ctrl) + requestAuthenticator.EXPECT().AuthenticateTokenCredentialRequest(gomock.Any(), req). + Return(&user.DefaultInfo{ + Name: "test-user", + Groups: []string{"test-group-1", "test-group-2"}, + Extra: map[string][]string{"test-key": {"test-val-1", "test-val-2"}}, + }, nil) + + storage := NewREST(requestAuthenticator, nil, schema.GroupResource{}) + + response, err := callCreate(context.Background(), storage, req) + + requireSuccessfulResponseWithAuthenticationFailureMessage(t, err, response) + requireOneLogStatement(r, logger, `"success" userID:,hasExtra:true,authenticated:false`) }) it("CreateFailsWhenGivenTheWrongInputType", func() {