Change 404 errors for aliases to 401s (#4575)

* Change 404 errors for alias resolution to 401s
---------

Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>

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)
 	}

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))

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) {