From 55fe70e643a7f7224a8f0cd7e7f464c62e42f52d Mon Sep 17 00:00:00 2001 From: Todd Date: Wed, 27 Mar 2024 16:14:49 -0700 Subject: [PATCH] Change 404 errors for aliases to 401s (#4575) * Change 404 errors for alias resolution to 401s --------- Co-authored-by: Johan Brandhorst-Satzkorn --- internal/daemon/controller/interceptor.go | 14 +++++++-- .../daemon/controller/interceptor_test.go | 10 +++---- internal/tests/api/targets/target_test.go | 30 ++++++++++++++++--- 3 files changed, 42 insertions(+), 12 deletions(-) diff --git a/internal/daemon/controller/interceptor.go b/internal/daemon/controller/interceptor.go index e0d2630af..373c048f2 100644 --- a/internal/daemon/controller/interceptor.go +++ b/internal/daemon/controller/interceptor.go @@ -335,10 +335,9 @@ func errorInterceptor( // For an field in the request to be considered for alias resolution, it must // be annotated with the Aliasable proto option. func aliasResolutionInterceptor( - ctx context.Context, + _ context.Context, aliasRepoFn common.AliasRepoFactory, ) grpc.UnaryServerInterceptor { - const op = "alias.ResolutionInterceptor" return func(interceptorCtx context.Context, req any, _ *grpc.UnaryServerInfo, @@ -355,7 +354,16 @@ func aliasResolutionInterceptor( } interceptorCtx, err = alias.ResolveRequestIds(interceptorCtx, reqMsg, r) if err != nil { - return nil, err + // Since this is intercepted prior to checking that the requester + // is authorized to make the request, returning a 404 here and a 401 + // when the request is checked in the handler would expose which + // aliases exist. Instead, we return a unauthenticated error here so there + // is no way to distinguish between a missing alias and an existing + // alias that points to a destination the requester is not allowed + // to perform an action on. + // handlers.UnauthenticatedError() turns into an http 401 status + // code "Unauthorized". + return nil, handlers.UnauthenticatedError() } return handler(interceptorCtx, req) } diff --git a/internal/daemon/controller/interceptor_test.go b/internal/daemon/controller/interceptor_test.go index 5ba30ccd5..895c4f46b 100644 --- a/internal/daemon/controller/interceptor_test.go +++ b/internal/daemon/controller/interceptor_test.go @@ -693,7 +693,7 @@ func Test_aliasResolutionInterceptor(t *testing.T) { req proto.Message wantModifiedReq proto.Message wantAlias *alias.Alias - errorContains string + errorIs error }{ { name: "non aliasable request", @@ -724,13 +724,13 @@ func Test_aliasResolutionInterceptor(t *testing.T) { name: "aliasable request with unknown alias", req: &pbs.GetTargetRequest{Id: "not.a.registered.alias"}, wantModifiedReq: &pbs.GetTargetRequest{Id: "not.a.registered.alias"}, - errorContains: "resource alias not found with value", + errorIs: handlers.UnauthenticatedError(), }, { name: "aliasable request with destinationless alias", req: &pbs.GetTargetRequest{Id: alWithoutDest.GetValue()}, wantModifiedReq: &pbs.GetTargetRequest{Id: alWithoutDest.GetValue()}, - errorContains: "resource not found for alias value", + errorIs: handlers.UnauthenticatedError(), }, } for _, tc := range cases { @@ -740,9 +740,9 @@ func Test_aliasResolutionInterceptor(t *testing.T) { req := proto.Clone(tc.req) retCtx, err := interceptor(ctx, req, info, returnCtxHandler) assert.Empty(t, cmp.Diff(tc.wantModifiedReq, req, protocmp.Transform())) - if tc.errorContains != "" { + if tc.errorIs != nil { require.Error(t, err) - require.Contains(t, err.Error(), tc.errorContains) + require.ErrorIs(t, err, tc.errorIs) return } ctxAlias := alias.FromContext(retCtx.(context.Context)) diff --git a/internal/tests/api/targets/target_test.go b/internal/tests/api/targets/target_test.go index 2c4a5d553..274c42ffd 100644 --- a/internal/tests/api/targets/target_test.go +++ b/internal/tests/api/targets/target_test.go @@ -675,13 +675,23 @@ func TestCrud(t *testing.T) { _, err = tarClient.Delete(tc.Context(), c.id) assert.NoError(err) + } - _, err = tarClient.Delete(tc.Context(), c.id) + t.Run("deleting unknown target is not found", func(t *testing.T) { + _, err = tarClient.Delete(tc.Context(), tar.Item.Id) assert.Error(err) apiErr := api.AsServerError(err) assert.NotNil(apiErr) assert.EqualValues(http.StatusNotFound, apiErr.Response().StatusCode()) - } + }) + + t.Run("deleting unknown alias is unauthorized", func(t *testing.T) { + _, err = tarClient.Delete(tc.Context(), al.GetValue()) + assert.Error(err) + apiErr := api.AsServerError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusUnauthorized, apiErr.Response().StatusCode()) + }) } func TestSet_Errors(t *testing.T) { @@ -699,6 +709,7 @@ func TestSet_Errors(t *testing.T) { tar, err := tarClient.Create(tc.Context(), "tcp", proj.GetPublicId(), targets.WithName("foo"), targets.WithTcpTargetDefaultPort(2)) require.NoError(err) assert.NotNil(tar) + validTargetId := tar.GetItem().Id // A malformed id is processed as the id and not a different path to the api. _, err = tarClient.Read(tc.Context(), fmt.Sprintf("%s/../", tar.Item.Id)) @@ -739,7 +750,7 @@ func TestSet_Errors(t *testing.T) { require.Error(err) apiErr = api.AsServerError(err) assert.NotNil(apiErr) - assert.EqualValues(http.StatusNotFound, apiErr.Response().StatusCode()) + assert.EqualValues(http.StatusUnauthorized, apiErr.Response().StatusCode()) // reading by alias with no destination id should fail rw := db.New(tc.DbConn()) @@ -748,7 +759,18 @@ func TestSet_Errors(t *testing.T) { require.Error(err) apiErr = api.AsServerError(err) assert.NotNil(apiErr) - assert.EqualValues(http.StatusNotFound, apiErr.Response().StatusCode()) + assert.EqualValues(http.StatusUnauthorized, apiErr.Response().StatusCode()) + noAliasApiErrReponse := apiErr.Response() + + client.SetToken("at_1234567890_madeupinvalidtoken") + tarClient = targets.NewClient(client) + _, err = tarClient.Read(tc.Context(), validTargetId) + apiErr = api.AsServerError(err) + assert.NotNil(apiErr) + assert.EqualValues(http.StatusUnauthorized, apiErr.Response().StatusCode()) + // We should not be able to tell the difference between an unauthorized request + // and a request that tries to resolve an alias that points to nothing. + assert.EqualValues(noAliasApiErrReponse.Body, apiErr.Response().Body) } func TestCreateTarget_WhitespaceInAddress(t *testing.T) {