From 6a93de393102265ea9725ab97a42364cff0cd302 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 23 Jul 2020 16:01:55 -0700 Subject: [PATCH] More validations and error handling for create LoginRequest endpoint --- go.mod | 2 +- go.sum | 2 + pkg/registry/loginrequest/rest.go | 54 +++++-- pkg/registry/loginrequest/rest_test.go | 194 ++++++++++++++++--------- test/integration/loginrequest_test.go | 29 +++- 5 files changed, 192 insertions(+), 89 deletions(-) diff --git a/go.mod b/go.mod index f738f5b4..d3af2b83 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/google/go-cmp v0.4.0 github.com/spf13/cobra v1.0.0 github.com/stretchr/testify v1.6.1 - github.com/suzerain-io/placeholder-name-api v0.0.0-20200714184318-8ad91581433a + github.com/suzerain-io/placeholder-name-api v0.0.0-20200723201114-466aa86ace18 github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200714203950-a414963b4f95 golang.org/x/tools v0.0.0-20200707134715-9e0a013e855f // indirect k8s.io/api v0.19.0-rc.0 diff --git a/go.sum b/go.sum index d0a134d0..0575019c 100644 --- a/go.sum +++ b/go.sum @@ -521,6 +521,8 @@ github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/suzerain-io/placeholder-name-api v0.0.0-20200714184318-8ad91581433a h1:ycG6TufZM7ZDrgBklkXEMauvSyh44JQDvSUdJawAjGA= github.com/suzerain-io/placeholder-name-api v0.0.0-20200714184318-8ad91581433a/go.mod h1:bNHheAnmAISdW/ZYTnhCmg8QQKwA5WD64ZvPdsTrWjw= +github.com/suzerain-io/placeholder-name-api v0.0.0-20200723201114-466aa86ace18 h1:iR4hjldaCcIwDxld0OIG5EHIQrwnmJ+KVBqsEfXrvcI= +github.com/suzerain-io/placeholder-name-api v0.0.0-20200723201114-466aa86ace18/go.mod h1:bNHheAnmAISdW/ZYTnhCmg8QQKwA5WD64ZvPdsTrWjw= github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200714203950-a414963b4f95 h1:O1aoszdMJEekLXD7pNWP23vq0g8eXLp40AQgY8Hj+Sw= github.com/suzerain-io/placeholder-name-client-go v0.0.0-20200714203950-a414963b4f95/go.mod h1:NwirXEgd1VaA+6R0W7PYL/ae6wfPT3vA+tu7POAArjU= github.com/tdakkota/asciicheck v0.0.0-20200416190851-d7f85be797a2 h1:Xr9gkxfOP0KQWXKNqmwe8vEeSUiUj4Rlee9CMVX2ZUQ= diff --git a/pkg/registry/loginrequest/rest.go b/pkg/registry/loginrequest/rest.go index ca366263..9c55393a 100644 --- a/pkg/registry/loginrequest/rest.go +++ b/pkg/registry/loginrequest/rest.go @@ -16,6 +16,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/authentication/authenticator" "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/klog/v2" placeholderapi "github.com/suzerain-io/placeholder-name-api/pkg/apis/placeholder" ) @@ -53,7 +54,16 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation // TODO refactor all validation checks into a validation function in another package (e.g. see subjectaccessreqview api in k8s) - // TODO also validate .spec.type + if len(loginRequest.Spec.Type) == 0 { + errs := field.ErrorList{field.Required(field.NewPath("spec", "type"), "type must be supplied")} + return nil, apierrors.NewInvalid(placeholderapi.Kind(loginRequest.Kind), loginRequest.Name, errs) + } + + if loginRequest.Spec.Type != placeholderapi.TokenLoginCredentialType { + errs := field.ErrorList{field.Invalid(field.NewPath("spec", "type"), loginRequest.Spec.Type, "unrecognized type")} + return nil, apierrors.NewInvalid(placeholderapi.Kind(loginRequest.Kind), loginRequest.Name, errs) + } + token := loginRequest.Spec.Token if token == nil || len(token.Value) == 0 { errs := field.ErrorList{field.Required(field.NewPath("spec", "token", "value"), "token must be supplied")} @@ -93,22 +103,40 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation } }() - // TODO do the actual business logic of this endpoint here - - _, _, err := r.webhook.AuthenticateToken(cancelCtx, token.Value) + _, authenticated, err := r.webhook.AuthenticateToken(cancelCtx, token.Value) if err != nil { - return nil, fmt.Errorf("authenticate token failed: %w", err) + klog.Warningf("webhook authentication failure: %v", err) + return failureResponse(), nil } - // make a new object so that we do not return the original token in the response - out := &placeholderapi.LoginRequest{ - Status: placeholderapi.LoginRequestStatus{ - ExpirationTimestamp: nil, - Token: "snorlax", - ClientCertificateData: "", - ClientKeyData: "", - }, + var out *placeholderapi.LoginRequest + if authenticated { + out = successfulResponse() + } else { + out = failureResponse() } return out, nil } + +func successfulResponse() *placeholderapi.LoginRequest { + return &placeholderapi.LoginRequest{ + Status: placeholderapi.LoginRequestStatus{ + Credential: &placeholderapi.LoginRequestCredential{ + ExpirationTimestamp: nil, + Token: "snorlax", + ClientCertificateData: "", + ClientKeyData: "", + }, + }, + } +} + +func failureResponse() *placeholderapi.LoginRequest { + return &placeholderapi.LoginRequest{ + Status: placeholderapi.LoginRequestStatus{ + Credential: nil, + Message: "authentication failed", + }, + } +} diff --git a/pkg/registry/loginrequest/rest_test.go b/pkg/registry/loginrequest/rest_test.go index 47ba6164..27819b8d 100644 --- a/pkg/registry/loginrequest/rest_test.go +++ b/pkg/registry/loginrequest/rest_test.go @@ -32,6 +32,7 @@ type FakeToken struct { cancelled bool webhookStartedRunningNotificationChan chan bool returnErr error + returnUnauthenticated bool } func (f *FakeToken) AuthenticateToken(ctx context.Context, token string) (*authenticator.Response, bool, error) { @@ -47,7 +48,7 @@ func (f *FakeToken) AuthenticateToken(ctx context.Context, token string) (*authe case <-ctx.Done(): f.cancelled = true } - return &authenticator.Response{}, true, f.returnErr + return &authenticator.Response{}, !f.returnUnauthenticated, f.returnErr } func callCreate(ctx context.Context, storage *REST, loginRequest *placeholderapi.LoginRequest) (runtime.Object, error) { @@ -61,9 +62,13 @@ func callCreate(ctx context.Context, storage *REST, loginRequest *placeholderapi } func validLoginRequest() *placeholderapi.LoginRequest { + return validLoginRequestWithToken("a token") +} + +func validLoginRequestWithToken(token string) *placeholderapi.LoginRequest { return loginRequest(placeholderapi.LoginRequestSpec{ Type: placeholderapi.TokenLoginCredentialType, - Token: &placeholderapi.LoginRequestTokenCredential{Value: "a token"}, + Token: &placeholderapi.LoginRequestTokenCredential{Value: token}, }) } @@ -86,27 +91,66 @@ func requireAPIError(t *testing.T, response runtime.Object, err error, expectedE require.Contains(t, status.Status().Message, expectedErrorMessage) } -func TestCreateSucceedsWhenGivenAToken(t *testing.T) { - webhook := FakeToken{} +func TestCreateSucceedsWhenGivenATokenAndTheWebhookAuthenticatesTheToken(t *testing.T) { + webhook := FakeToken{ + returnUnauthenticated: false, + } storage := NewREST(&webhook) requestToken := "a token" - response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{ - Type: placeholderapi.TokenLoginCredentialType, - Token: &placeholderapi.LoginRequestTokenCredential{Value: requestToken}, - })) + + response, err := callCreate(context.Background(), storage, validLoginRequestWithToken(requestToken)) require.NoError(t, err) require.Equal(t, response, &placeholderapi.LoginRequest{ Status: placeholderapi.LoginRequestStatus{ - ExpirationTimestamp: nil, - Token: "snorlax", - ClientCertificateData: "", - ClientKeyData: "", + Credential: &placeholderapi.LoginRequestCredential{ + ExpirationTimestamp: nil, + Token: "snorlax", + ClientCertificateData: "", + ClientKeyData: "", + }, + Message: "", }, }) require.Equal(t, requestToken, webhook.calledWithToken) } +func TestCreateSucceedsWithAnUnauthenticatedStatusWhenGivenATokenAndTheWebhookDoesNotAuthenticateTheToken(t *testing.T) { + webhook := FakeToken{ + returnUnauthenticated: true, + } + storage := NewREST(&webhook) + requestToken := "a token" + + response, err := callCreate(context.Background(), storage, validLoginRequestWithToken(requestToken)) + + require.NoError(t, err) + require.Equal(t, response, &placeholderapi.LoginRequest{ + Status: placeholderapi.LoginRequestStatus{ + Credential: nil, + Message: "authentication failed", + }, + }) + require.Equal(t, requestToken, webhook.calledWithToken) +} + +func TestCreateSucceedsWithAnUnauthenticatedStatusWhenWebhookFails(t *testing.T) { + webhook := FakeToken{ + returnErr: errors.New("some webhook error"), + } + storage := NewREST(&webhook) + + response, err := callCreate(context.Background(), storage, validLoginRequest()) + + require.NoError(t, err) + require.Equal(t, response, &placeholderapi.LoginRequest{ + Status: placeholderapi.LoginRequestStatus{ + Credential: nil, + Message: "authentication failed", + }, + }) +} + func TestCreateDoesNotPassAdditionalContextInfoToTheWebhook(t *testing.T) { webhook := FakeToken{} storage := NewREST(&webhook) @@ -118,6 +162,74 @@ func TestCreateDoesNotPassAdditionalContextInfoToTheWebhook(t *testing.T) { require.Nil(t, webhook.calledWithContext.Value("context-key")) } +func TestCreateFailsWhenGivenTheWrongInputType(t *testing.T) { + notALoginRequest := runtime.Unknown{} + response, err := NewREST(&FakeToken{}).Create( + genericapirequest.NewContext(), + ¬ALoginRequest, + rest.ValidateAllObjectFunc, + &metav1.CreateOptions{}) + + requireAPIError(t, response, err, apierrors.IsBadRequest, "not a LoginRequest") +} + +func TestCreateFailsWhenTokenIsNilInRequest(t *testing.T) { + storage := NewREST(&FakeToken{}) + response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{ + Type: placeholderapi.TokenLoginCredentialType, + Token: nil, + })) + + requireAPIError(t, response, err, apierrors.IsInvalid, + `.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`) +} + +func TestCreateFailsWhenTypeInRequestIsMissing(t *testing.T) { + storage := NewREST(&FakeToken{}) + response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{ + Type: "", + Token: &placeholderapi.LoginRequestTokenCredential{Value: "a token"}, + })) + + requireAPIError(t, response, err, apierrors.IsInvalid, + `.placeholder.suzerain-io.github.io "request name" is invalid: spec.type: Required value: type must be supplied`) +} + +func TestCreateFailsWhenTypeInRequestIsNotLegal(t *testing.T) { + storage := NewREST(&FakeToken{}) + response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{ + Type: "this in an invalid type", + Token: &placeholderapi.LoginRequestTokenCredential{Value: "a token"}, + })) + + requireAPIError(t, response, err, apierrors.IsInvalid, + `.placeholder.suzerain-io.github.io "request name" is invalid: spec.type: Invalid value: "this in an invalid type": unrecognized type`) +} + +func TestCreateFailsWhenTokenValueIsEmptyInRequest(t *testing.T) { + storage := NewREST(&FakeToken{}) + response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{ + Type: placeholderapi.TokenLoginCredentialType, + Token: &placeholderapi.LoginRequestTokenCredential{Value: ""}, + })) + + requireAPIError(t, response, err, apierrors.IsInvalid, + `.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`) +} + +func TestCreateFailsWhenRequestOptionsDryRunIsNotEmpty(t *testing.T) { + response, err := NewREST(&FakeToken{}).Create( + genericapirequest.NewContext(), + validLoginRequest(), + rest.ValidateAllObjectFunc, + &metav1.CreateOptions{ + DryRun: []string{"some dry run flag"}, + }) + + requireAPIError(t, response, err, apierrors.IsInvalid, + `.placeholder.suzerain-io.github.io "request name" is invalid: dryRun: Unsupported value: []string{"some dry run flag"}`) +} + func TestCreateCancelsTheWebhookInvocationWhenTheCallToCreateIsCancelledItself(t *testing.T) { webhookStartedRunningNotificationChan := make(chan bool) webhook := FakeToken{ @@ -169,61 +281,3 @@ func TestCreateAllowsTheWebhookInvocationToFinishWhenTheCallToCreateIsNotCancell require.True(t, webhook.reachedTimeout) require.Equal(t, context.Canceled, webhook.calledWithContext.Err()) // the inner context is cancelled (in this case by the "defer") } - -func TestCreateFailsWhenWebhookFails(t *testing.T) { - webhook := FakeToken{ - returnErr: errors.New("some webhook error"), - } - storage := NewREST(&webhook) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - _, err := callCreate(ctx, storage, validLoginRequest()) - require.EqualError(t, err, "authenticate token failed: some webhook error") -} - -func TestCreateFailsWhenGivenTheWrongInputType(t *testing.T) { - notALoginRequest := runtime.Unknown{} - response, err := NewREST(&FakeToken{}).Create( - genericapirequest.NewContext(), - ¬ALoginRequest, - rest.ValidateAllObjectFunc, - &metav1.CreateOptions{}) - - requireAPIError(t, response, err, apierrors.IsBadRequest, "not a LoginRequest") -} - -func TestCreateFailsWhenTokenIsNilInRequest(t *testing.T) { - storage := NewREST(&FakeToken{}) - response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{ - Type: placeholderapi.TokenLoginCredentialType, - Token: nil, - })) - - requireAPIError(t, response, err, apierrors.IsInvalid, - `.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`) -} - -func TestCreateFailsWhenTokenValueIsEmptyInRequest(t *testing.T) { - storage := NewREST(&FakeToken{}) - response, err := callCreate(context.Background(), storage, loginRequest(placeholderapi.LoginRequestSpec{ - Type: placeholderapi.TokenLoginCredentialType, - Token: &placeholderapi.LoginRequestTokenCredential{Value: ""}, - })) - - requireAPIError(t, response, err, apierrors.IsInvalid, - `.placeholder.suzerain-io.github.io "request name" is invalid: spec.token.value: Required value: token must be supplied`) -} - -func TestCreateFailsWhenRequestOptionsDryRunIsNotEmpty(t *testing.T) { - response, err := NewREST(&FakeToken{}).Create( - genericapirequest.NewContext(), - validLoginRequest(), - rest.ValidateAllObjectFunc, - &metav1.CreateOptions{ - DryRun: []string{"some dry run flag"}, - }) - - requireAPIError(t, response, err, apierrors.IsInvalid, - `.placeholder.suzerain-io.github.io "request name" is invalid: dryRun: Unsupported value: []string{"some dry run flag"}`) -} diff --git a/test/integration/loginrequest_test.go b/test/integration/loginrequest_test.go index 9799aa14..5a3aefd9 100644 --- a/test/integration/loginrequest_test.go +++ b/test/integration/loginrequest_test.go @@ -40,20 +40,39 @@ func TestSuccessfulLoginRequest(t *testing.T) { require.NotEmptyf(t, tmcClusterToken, "must specify PLACEHOLDER_NAME_TMC_CLUSTER_TOKEN env var for integration tests") response, err := makeRequest(t, v1alpha1.LoginRequestSpec{ + Type: v1alpha1.TokenLoginCredentialType, Token: &v1alpha1.LoginRequestTokenCredential{Value: tmcClusterToken}, }) + require.NoError(t, err) + require.Empty(t, response.Status.Message) + + require.Empty(t, response.Spec) + require.NotNil(t, response.Status.Credential) + require.NotEmpty(t, response.Status.Credential.Token) + require.Empty(t, response.Status.Credential.ClientCertificateData) + require.Empty(t, response.Status.Credential.ClientKeyData) + require.Nil(t, response.Status.Credential.ExpirationTimestamp) +} + +func TestFailedLoginRequestWhenTheRequestIsValidButTheTokenDoesNotAuthenticateTheUser(t *testing.T) { + response, err := makeRequest(t, v1alpha1.LoginRequestSpec{ + Type: v1alpha1.TokenLoginCredentialType, + Token: &v1alpha1.LoginRequestTokenCredential{Value: "not a good token"}, + }) + require.NoError(t, err) require.Empty(t, response.Spec) - require.NotEmpty(t, response.Status.Token) - require.Empty(t, response.Status.ClientCertificateData) - require.Empty(t, response.Status.ClientKeyData) - require.Nil(t, response.Status.ExpirationTimestamp) + require.Nil(t, response.Status.Credential) + require.Equal(t, "authentication failed", response.Status.Message) } func TestLoginRequest_ShouldFailWhenRequestDoesNotIncludeToken(t *testing.T) { - _, err := makeRequest(t, v1alpha1.LoginRequestSpec{}) + _, err := makeRequest(t, v1alpha1.LoginRequestSpec{ + Type: v1alpha1.TokenLoginCredentialType, + Token: nil, + }) require.Error(t, err) statusError, isStatus := err.(*errors.StatusError)