From 0c25dbad1325e8895b4cd07544a42423a1994426 Mon Sep 17 00:00:00 2001 From: David Bond Date: Thu, 6 Jun 2024 15:59:43 +0200 Subject: [PATCH] Use appropriate gRPC error codes for OIDC (#4877) This commit makes modifications to the OIDC request handlers to return appropriate gRPC status codes on errors. Before this change, boundary presents the internal error codes to clients. This causes problems for reverse proxies, as these status codes are mapped to non-standard HTTP response codes which are ultimately all turned into 500 responses when 4xx status codes are more appropriate and descriptive of the error returned. Some modifications in this commit include calls to `event.WriteError` so that the internal error is written to the controller log for visibility. Comments about potential duplicate logs are now removed as `event.WriteError` is not called when using functions from the `handlers` package. Signed-off-by: David Bond --- .../authmethods/authmethod_service.go | 10 ++--- .../controller/handlers/authmethods/oidc.go | 41 ++++++++----------- .../handlers/authmethods/oidc_test.go | 6 +-- 3 files changed, 26 insertions(+), 31 deletions(-) diff --git a/internal/daemon/controller/handlers/authmethods/authmethod_service.go b/internal/daemon/controller/handlers/authmethods/authmethod_service.go index a552cce65..f923c5534 100644 --- a/internal/daemon/controller/handlers/authmethods/authmethod_service.go +++ b/internal/daemon/controller/handlers/authmethods/authmethod_service.go @@ -1423,19 +1423,19 @@ func (s Service) ConvertInternalAuthTokenToApiAuthToken(ctx context.Context, tok func (s Service) convertToAuthenticateResponse(ctx context.Context, req *pbs.AuthenticateRequest, authResults *requestauth.VerifyResults, tok *pba.AuthToken) (*pbs.AuthenticateResponse, error) { const op = "authmethod.convertToAuthenticateResponse" if req == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Missing request.") + return nil, handlers.InvalidArgumentErrorf("Nil request.", nil) } if authResults == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Missing auth results.") + return nil, handlers.InvalidArgumentErrorf("Nil auth results.", nil) } if authResults.Scope == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Missing auth results scope.") + return nil, handlers.InvalidArgumentErrorf("Missing auth results scope.", nil) } if authResults.Scope.Id == "" { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Missing auth results scope ID.") + return nil, handlers.InvalidArgumentErrorf("Missing auth results scope ID.", nil) } if tok == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Missing auth token.") + return nil, handlers.InvalidArgumentErrorf("Missing auth token", nil) } res := &perms.Resource{ ScopeId: authResults.Scope.Id, diff --git a/internal/daemon/controller/handlers/authmethods/oidc.go b/internal/daemon/controller/handlers/authmethods/oidc.go index 7bb3c9df5..1285c2833 100644 --- a/internal/daemon/controller/handlers/authmethods/oidc.go +++ b/internal/daemon/controller/handlers/authmethods/oidc.go @@ -168,7 +168,7 @@ func (s Service) authenticateOidc(ctx context.Context, req *pbs.AuthenticateRequ func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.AuthenticateRequest) (*pbs.AuthenticateResponse, error) { const op = "authmethod_service.(Service).authenticateOidcStart" if req == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Nil request.") + return nil, handlers.InvalidArgumentErrorf("Nil request.", nil) } var opts []oidc.Option @@ -179,10 +179,8 @@ func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.Authenticat authUrl, tokenId, err := oidc.StartAuth(ctx, s.oidcRepoFn, req.GetAuthMethodId(), opts...) if err != nil { - // this event.WriteError(...) may cause a dup error to be emitted... - // it should be removed if that's the case. event.WriteError(ctx, op, err, event.WithInfoMsg("error starting the oidc authentication flow")) - return nil, errors.New(ctx, errors.Internal, op, "Error generating parameters for starting the OIDC flow. See the controller's log for more information.") + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Error generating parameters for starting the OIDC flow. See the controller's log for more information.") } return &pbs.AuthenticateResponse{ @@ -208,22 +206,22 @@ func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.Authenti // in the redirect URL once we start looking at the url used for this // request instead of requiring the API URL to be set on the auth method. if req == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Nil request.") + return nil, handlers.InvalidArgumentErrorf("Nil request.", nil) } repo, err := s.oidcRepoFn() if err != nil { - return nil, errors.Wrap(ctx, err, op) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, err.Error()) } am, err := repo.LookupAuthMethod(ctx, req.GetAuthMethodId()) if err != nil { - return nil, errors.Wrap(ctx, err, op) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, err.Error()) } if am == nil { - return nil, errors.New(ctx, errors.RecordNotFound, op, fmt.Sprintf("Auth method %s not found.", req.GetAuthMethodId())) + return nil, handlers.NotFoundErrorf("Auth method %s not found.", req.GetAuthMethodId()) } if am.GetApiUrl() == "" { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Auth method doesn't have API URL defined.") + return nil, handlers.InvalidArgumentErrorf("Auth method doesn't have API URL defined.", nil) } errRedirectBase := fmt.Sprintf(oidc.AuthenticationErrorsEndpoint, am.GetApiUrl()) @@ -232,7 +230,8 @@ func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.Authenti pbErr := handlers.ToApiError(err) out, err := handlers.JSONMarshaler().Marshal(pbErr) if err != nil { - return nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to marshal the error for callback")) + event.WriteError(ctx, op, err, event.WithInfoMsg("unable to marshal the error for callback")) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "unable to marshal the error for callback") } u.Add("error", string(out)) errRedirect := fmt.Sprintf("%s?%s", errRedirectBase, u.Encode()) @@ -278,38 +277,33 @@ func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.Authenti func (s Service) authenticateOidcToken(ctx context.Context, req *pbs.AuthenticateRequest, authResults *auth.VerifyResults) (*pbs.AuthenticateResponse, error) { const op = "authmethod_service.(Service).authenticateOidcToken" if req == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Nil request.") + return nil, handlers.InvalidArgumentErrorf("Nil request.", nil) } if authResults == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Nil auth results.") + return nil, handlers.InvalidArgumentErrorf("Nil auth results.", nil) } if req.GetOidcAuthMethodAuthenticateTokenRequest() == nil { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Nil request attributes.") + return nil, handlers.InvalidArgumentErrorf("Nil request attributes.", nil) } attrs := req.GetOidcAuthMethodAuthenticateTokenRequest() if attrs.TokenId == "" { - return nil, errors.New(ctx, errors.InvalidParameter, op, "Empty token ID in request attributes.") + return nil, handlers.InvalidArgumentErrorf("Empty token ID in request attributes.", nil) } token, err := oidc.TokenRequest(ctx, s.kms, s.atRepoFn, req.GetAuthMethodId(), attrs.TokenId) if err != nil { switch { case errors.Match(errors.T(errors.Forbidden), err): - return nil, errors.Wrap(ctx, err, op, errors.WithMsg("Forbidden.")) + return nil, handlers.ForbiddenError() case errors.Match(errors.T(errors.AuthAttemptExpired), err): - return nil, errors.Wrap(ctx, err, op, errors.WithMsg("Forbidden.")) + return nil, handlers.ForbiddenError() default: - // this event.WriteError(...) may cause a dup error to be emitted... - // it should be removed if that's the case. event.WriteError(ctx, op, err, event.WithInfoMsg("error generating parameters for token request")) - return nil, errors.Wrap(ctx, err, op, errors.WithMsg("Error generating parameters for token request. See the controller's log for more information.")) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Error generating parameters for token request. See the controller's log for more information.") } } if token == nil { - if err != nil { - return nil, errors.New(ctx, errors.Internal, op, "Error generating response attributes.", errors.WithWrap(err)) - } return &pbs.AuthenticateResponse{ Command: req.Command, Attrs: &pbs.AuthenticateResponse_OidcAuthMethodAuthenticateTokenResponse{ @@ -325,7 +319,8 @@ func (s Service) authenticateOidcToken(ctx context.Context, req *pbs.Authenticat token, ) if err != nil { - return nil, errors.New(ctx, errors.Internal, op, "Error converting response to proper format.", errors.WithWrap(err)) + event.WriteError(ctx, op, err, event.WithInfoMsg("error converting response to proper format.")) + return nil, handlers.ApiErrorWithCodeAndMessage(codes.Internal, "Error converting response to proper format. See the controller's log for more information.") } return s.convertToAuthenticateResponse(ctx, req, authResults, responseToken) } diff --git a/internal/daemon/controller/handlers/authmethods/oidc_test.go b/internal/daemon/controller/handlers/authmethods/oidc_test.go index a8399de5a..d75e0bdb6 100644 --- a/internal/daemon/controller/handlers/authmethods/oidc_test.go +++ b/internal/daemon/controller/handlers/authmethods/oidc_test.go @@ -1761,7 +1761,7 @@ func TestAuthenticate_OIDC_Token(t *testing.T) { Command: "token", AuthMethodId: s.authMethod.GetPublicId(), }, - wantErrMatch: errors.T(errors.InvalidParameter), + wantErr: handlers.ApiErrorWithCode(codes.InvalidArgument), }, { name: "success", @@ -1796,7 +1796,7 @@ func TestAuthenticate_OIDC_Token(t *testing.T) { }, }, }, - wantErrMatch: errors.T(errors.AuthAttemptExpired), + wantErr: handlers.ApiErrorWithCode(codes.PermissionDenied), }, { name: "invalid-token-id-not-encoded-encrypted-proto", @@ -1809,7 +1809,7 @@ func TestAuthenticate_OIDC_Token(t *testing.T) { }, }, }, - wantErrMatch: errors.T(errors.Unknown), + wantErr: handlers.ApiErrorWithCode(codes.Internal), }, } for _, tc := range cases {