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

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

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

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

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

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

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 {