From ca57e50516383d6d5348ce36313d797dd2884892 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 13 Apr 2021 18:07:50 -0500 Subject: [PATCH] Add more OIDC error output. (#1108) Some of this is in the form of logging, and some via returning error messages to users. Also cleans a couple of linter complaints. --- internal/servers/controller/handler.go | 2 +- .../authmethods/authmethod_service.go | 12 +++++-- .../controller/handlers/authmethods/oidc.go | 35 ++++++++++--------- .../controller/handlers/option_test.go | 19 +++++++--- .../servers/controller/handlers/options.go | 18 ++++++++-- .../servers/controller/handlers/subtypes.go | 2 +- 6 files changed, 59 insertions(+), 29 deletions(-) diff --git a/internal/servers/controller/handler.go b/internal/servers/controller/handler.go index faf502ba8..91121913c 100644 --- a/internal/servers/controller/handler.go +++ b/internal/servers/controller/handler.go @@ -114,7 +114,7 @@ func handleGrpcGateway(c *Controller, props HandlerProperties) (http.Handler, er if err := services.RegisterAccountServiceHandlerServer(ctx, mux, accts); err != nil { return nil, fmt.Errorf("failed to register account service handler: %w", err) } - authMethods, err := authmethods.NewService(c.kms, c.PasswordAuthRepoFn, c.OidcRepoFn, c.IamRepoFn, c.AuthTokenRepoFn) + authMethods, err := authmethods.NewService(c.kms, c.PasswordAuthRepoFn, c.OidcRepoFn, c.IamRepoFn, c.AuthTokenRepoFn, handlers.WithLogger(c.logger.Named("authmethod_service"))) if err != nil { return nil, fmt.Errorf("failed to create auth method handler service: %w", err) } diff --git a/internal/servers/controller/handlers/authmethods/authmethod_service.go b/internal/servers/controller/handlers/authmethods/authmethod_service.go index 022c56c7a..1c7e458f5 100644 --- a/internal/servers/controller/handlers/authmethods/authmethod_service.go +++ b/internal/servers/controller/handlers/authmethods/authmethod_service.go @@ -27,6 +27,7 @@ import ( "github.com/hashicorp/boundary/internal/types/resource" "github.com/hashicorp/boundary/internal/types/scope" "github.com/hashicorp/boundary/sdk/strutil" + "github.com/hashicorp/go-hclog" "google.golang.org/grpc/codes" "google.golang.org/protobuf/types/known/structpb" "google.golang.org/protobuf/types/known/wrapperspb" @@ -97,10 +98,12 @@ type Service struct { oidcRepoFn common.OidcAuthRepoFactory iamRepoFn common.IamRepoFactory atRepoFn common.AuthTokenRepoFactory + + oidcLogger hclog.Logger } // NewService returns a auth method service which handles auth method related requests to boundary. -func NewService(kms *kms.Kms, pwRepoFn common.PasswordAuthRepoFactory, oidcRepoFn common.OidcAuthRepoFactory, iamRepoFn common.IamRepoFactory, atRepoFn common.AuthTokenRepoFactory) (Service, error) { +func NewService(kms *kms.Kms, pwRepoFn common.PasswordAuthRepoFactory, oidcRepoFn common.OidcAuthRepoFactory, iamRepoFn common.IamRepoFactory, atRepoFn common.AuthTokenRepoFactory, opt ...handlers.Option) (Service, error) { if kms == nil { return Service{}, stderrors.New("nil kms provided") } @@ -116,7 +119,12 @@ func NewService(kms *kms.Kms, pwRepoFn common.PasswordAuthRepoFactory, oidcRepoF if atRepoFn == nil { return Service{}, fmt.Errorf("nil auth token repository provided") } - return Service{kms: kms, pwRepoFn: pwRepoFn, oidcRepoFn: oidcRepoFn, iamRepoFn: iamRepoFn, atRepoFn: atRepoFn}, nil + s := Service{kms: kms, pwRepoFn: pwRepoFn, oidcRepoFn: oidcRepoFn, iamRepoFn: iamRepoFn, atRepoFn: atRepoFn} + opts := handlers.GetOpts(opt...) + if opts.WithLogger != nil { + s.oidcLogger = opts.WithLogger.Named("oidc") + } + return s, nil } var _ pbs.AuthMethodServiceServer = Service{} diff --git a/internal/servers/controller/handlers/authmethods/oidc.go b/internal/servers/controller/handlers/authmethods/oidc.go index d47a33046..45b08de68 100644 --- a/internal/servers/controller/handlers/authmethods/oidc.go +++ b/internal/servers/controller/handlers/authmethods/oidc.go @@ -26,7 +26,6 @@ const ( tokenCommand = "token" // token request/response fields - tokenField = "token" statusField = "status" // field names @@ -38,7 +37,6 @@ const ( callbackUrlField = "attributes.callback_url" apiUrlPrefixField = "attributes.api_url_prefix" caCertsField = "attributes.ca_certs" - maxAgeField = "attributes.max_age" signingAlgorithmField = "attributes.signing_algorithms" disableDiscoveredConfigValidationField = "attributes.disable_discovered_config_validation" roundtripPayloadAttributesField = "attributes.roundtrip_payload" @@ -65,7 +63,7 @@ func init() { type oidcState uint const ( - unknownState oidcState = iota + _ oidcState = iota inactiveState privateState publicState @@ -169,7 +167,7 @@ func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.Authenticat var opts []oidc.Option attrs := new(pbs.OidcStartAttributes) if err := handlers.StructToProto(req.GetAttributes(), attrs); err != nil { - return nil, errors.New(errors.InvalidParameter, op, "Error parsing request attributes.") + return nil, errors.New(errors.InvalidParameter, op, "Error parsing request attributes.", errors.WithWrap(err)) } if attrs.GetCachedRoundtripPayload() != "" { opts = append(opts, oidc.WithRoundtripPayload(attrs.GetCachedRoundtripPayload())) @@ -177,8 +175,10 @@ func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.Authenticat authUrl, tokenId, err := oidc.StartAuth(ctx, s.oidcRepoFn, req.GetAuthMethodId(), opts...) if err != nil { - // TODO: Log something - return nil, errors.New(errors.Internal, op, "Error generating parameters for starting the OIDC flow.") + if s.oidcLogger != nil { + s.oidcLogger.Error("error starting the oidc authentication flow", "op", op, "error", err) + } + return nil, errors.New(errors.Internal, op, "Error generating parameters for starting the OIDC flow. See the controller's log for more information.") } respAttrs := &pb.OidcAuthMethodAuthenticateStartResponse{ @@ -187,7 +187,7 @@ func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.Authenticat } resp := &pbs.AuthenticateResponse{Command: req.GetCommand()} if resp.Attributes, err = handlers.ProtoToStruct(respAttrs); err != nil { - return nil, errors.New(errors.Internal, op, "Error marshaling parameters.") + return nil, errors.New(errors.Internal, op, "Error marshaling parameters.", errors.WithWrap(err)) } return resp, nil } @@ -213,10 +213,10 @@ func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.Authenti return nil, errors.Wrap(err, op) } if am == nil { - return nil, errors.New(errors.RecordNotFound, op, fmt.Sprintf("auth method %s not found", req.GetAuthMethodId())) + return nil, errors.New(errors.RecordNotFound, op, fmt.Sprintf("Auth method %s not found.", req.GetAuthMethodId())) } if am.GetApiUrl() == "" { - return nil, errors.New(errors.InvalidParameter, op, "auth method doesn't have api url defined") + return nil, errors.New(errors.InvalidParameter, op, "Auth method doesn't have API URL defined.") } errRedirectBase := fmt.Sprintf(oidc.AuthenticationErrorsEndpoint, am.GetApiUrl()) @@ -259,7 +259,7 @@ func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.Authenti FinalRedirectUrl: finalRedirectUrl, }) if err != nil { - return errResponse(errors.New(errors.Internal, op, "Error marshaling parameters")), nil + return errResponse(errors.New(errors.Internal, op, "Error marshaling parameters after successful callback", errors.WithWrap(err))), nil } return &pbs.AuthenticateResponse{Command: req.GetCommand(), Attributes: respAttrs}, nil @@ -279,22 +279,24 @@ func (s Service) authenticateOidcToken(ctx context.Context, req *pbs.Authenticat attrs := new(pb.OidcAuthMethodAuthenticateTokenRequest) if err := handlers.StructToProto(req.GetAttributes(), attrs); err != nil { - return nil, errors.New(errors.InvalidParameter, op, "Error parsing request attributes.") + return nil, errors.New(errors.InvalidParameter, op, "Error parsing request attributes.", errors.WithWrap(err)) } if attrs.TokenId == "" { - return nil, errors.New(errors.InvalidParameter, op, "Empty token id request attributes.") + return nil, errors.New(errors.InvalidParameter, op, "Empty token ID in request attributes.") } token, err := oidc.TokenRequest(ctx, s.kms, s.atRepoFn, req.GetAuthMethodId(), attrs.TokenId) if err != nil { - // TODO: Log something so we don't lose the error's context and entire msg... switch { case errors.Match(errors.T(errors.Forbidden), err): return nil, errors.Wrap(err, op, errors.WithMsg("Forbidden.")) case errors.Match(errors.T(errors.AuthAttemptExpired), err): return nil, errors.Wrap(err, op, errors.WithMsg("Forbidden.")) default: - return nil, errors.Wrap(err, op, errors.WithMsg("Error generating parameters for token request.")) + if s.oidcLogger != nil { + s.oidcLogger.Error("error generating parameters for token request", "op", op, "error", err) + } + return nil, errors.Wrap(err, op, errors.WithMsg("Error generating parameters for token request. See the controller's log for more information.")) } } if token == nil { @@ -302,7 +304,7 @@ func (s Service) authenticateOidcToken(ctx context.Context, req *pbs.Authenticat statusField: "unknown", }) if err != nil { - return nil, errors.New(errors.Internal, op, "Error generating response attributes.") + return nil, errors.New(errors.Internal, op, "Error generating response attributes.", errors.WithWrap(err)) } return &pbs.AuthenticateResponse{ Command: req.Command, @@ -315,7 +317,7 @@ func (s Service) authenticateOidcToken(ctx context.Context, req *pbs.Authenticat token, ) if err != nil { - return nil, errors.Wrap(err, op) + return nil, errors.New(errors.Internal, op, "Error converting response to proper format.", errors.WithWrap(err)) } return s.convertToAuthenticateResponse(ctx, req, authResults, responseToken) } @@ -349,7 +351,6 @@ func validateAuthenticateOidcRequest(req *pbs.AuthenticateRequest) error { attrs.CachedRoundtripPayload = string(m) req.Attributes, err = handlers.ProtoToStruct(attrs) if err != nil { - // TODO: Logging, when we have a logger return fmt.Errorf("unable to convert map back to proto") } } diff --git a/internal/servers/controller/handlers/option_test.go b/internal/servers/controller/handlers/option_test.go index d7f734dde..03b35d9ca 100644 --- a/internal/servers/controller/handlers/option_test.go +++ b/internal/servers/controller/handlers/option_test.go @@ -3,6 +3,7 @@ package handlers import ( "testing" + "github.com/hashicorp/go-hclog" "github.com/stretchr/testify/assert" ) @@ -11,20 +12,28 @@ func Test_GetOpts(t *testing.T) { t.Parallel() t.Run("WithDiscardUnknownFields", func(t *testing.T) { assert := assert.New(t) - // test default of 0 - opts := getOpts() + + opts := GetOpts() testOpts := getDefaultOptions() - testOpts.withDiscardUnknownFields = false assert.Equal(opts, testOpts) - opts = getOpts(WithDiscardUnknownFields(true)) + opts = GetOpts(WithDiscardUnknownFields(true)) testOpts = getDefaultOptions() testOpts.withDiscardUnknownFields = true assert.Equal(opts, testOpts) - opts = getOpts(WithDiscardUnknownFields(false)) + opts = GetOpts(WithDiscardUnknownFields(false)) testOpts = getDefaultOptions() testOpts.withDiscardUnknownFields = false assert.Equal(opts, testOpts) }) + t.Run("WithLogger", func(t *testing.T) { + assert := assert.New(t) + + opts := GetOpts() + assert.Nil(opts.WithLogger) + + opts = GetOpts(WithLogger(hclog.New(nil))) + assert.NotNil(opts.WithLogger) + }) } diff --git a/internal/servers/controller/handlers/options.go b/internal/servers/controller/handlers/options.go index 38972f9df..d9fca4cb1 100644 --- a/internal/servers/controller/handlers/options.go +++ b/internal/servers/controller/handlers/options.go @@ -1,7 +1,9 @@ package handlers -// getOpts - iterate the inbound Options and return a struct -func getOpts(opt ...Option) options { +import "github.com/hashicorp/go-hclog" + +// GetOpts - iterate the inbound Options and return a struct +func GetOpts(opt ...Option) options { opts := getDefaultOptions() for _, o := range opt { o(&opts) @@ -9,12 +11,15 @@ func getOpts(opt ...Option) options { return opts } -// Option - how Options are passed as arguments +// Option - how Options are passed as arguments. Some of these are used for +// types within the handlers package, and some are for handlers to re-use across +// the various handler types. type Option func(*options) // options = how options are represented type options struct { withDiscardUnknownFields bool + WithLogger hclog.Logger } func getDefaultOptions() options { @@ -29,3 +34,10 @@ func WithDiscardUnknownFields(discard bool) Option { o.withDiscardUnknownFields = discard } } + +// WithLogger provides an option include a logger +func WithLogger(logger hclog.Logger) Option { + return func(o *options) { + o.WithLogger = logger + } +} diff --git a/internal/servers/controller/handlers/subtypes.go b/internal/servers/controller/handlers/subtypes.go index c49867328..c5261cdad 100644 --- a/internal/servers/controller/handlers/subtypes.go +++ b/internal/servers/controller/handlers/subtypes.go @@ -15,7 +15,7 @@ func StructToProto(fields *structpb.Struct, p proto.Message, opt ...Option) erro if err != nil { return err } - opts := getOpts(opt...) + opts := GetOpts(opt...) if opts.withDiscardUnknownFields { err = (protojson.UnmarshalOptions{DiscardUnknown: true}.Unmarshal(js, p)) } else {