Make oidc auth callback redirect on most errors (#1105)

api/authmethods/oidc_auth_method_authenticate_start_response.gen.go

@@ -2,7 +2,6 @@
 package authmethods
 
 type OidcAuthMethodAuthenticateStartResponse struct {
-	AuthUrl  string `json:"auth_url,omitempty"`
-	TokenUrl string `json:"token_url,omitempty"`
-	TokenId  string `json:"token_id,omitempty"`
+	AuthUrl string `json:"auth_url,omitempty"`
+	TokenId string `json:"token_id,omitempty"`
 }

internal/auth/oidc/repository_auth_method_read.go

@@ -61,7 +61,7 @@ func (r *Repository) lookupAuthMethod(ctx context.Context, authMethodId string,
 
 // getAuthMethods allows the caller to either lookup a specific AuthMethod via
 // its id or search for a set AuthMethods within a set of scopes.  Passing both
-// scopeIds and a authMethodId is an error. The WithUnauthenticatedUser,
+// scopeIds and a authMethod is an error. The WithUnauthenticatedUser,
 // WithLimit and WithOrder options are supported and all other options are
 // ignored.
 //

internal/auth/oidc/service.go

@@ -24,20 +24,15 @@ const (
 
 	// FinalRedirectEndpoint is the endpoint that the oidc callback redirect
 	// client to after the callback is complete.
-	FinalRedirectEndpoint = "%s/authentication-complete" // TODO jimlambrt 2/2021 get redirect from FE before PR
+	FinalRedirectEndpoint = "%s/authentication-complete"
 
 	// AuthenticationErrorsEndpoint is the endpoint that will returned as the final redirect
 	// from the callback when there are auth errors
-	AuthenticationErrorsEndpoint = "%s/authentication-error" // TODO jimlambrt 3/2021 get endpoint from FE before PR
+	AuthenticationErrorsEndpoint = "%s/authentication-error"
 
 	// CallbackEndpoint is the endpoint for the oidc callback which will be
 	// included in the auth URL returned when an authen attempted is kicked off.
-	CallbackEndpoint = "%s/v1/auth-methods/%s:authenticate:callback" // TODO jimlambrt 2/2021 get endpoint from Todd before PR
-
-	// TokenEndpoint is the endpoint the client will poll to see if their
-	// authentication attempt has completed and if successful, they'll
-	// retrieve their Boundary token.
-	TokenEndpoint = "%s/v1/auth-methods/%s:authenticate:token" // TODO jimlambrt 2/2021 get endpoint from Todd before PR
+	CallbackEndpoint = "%s/v1/auth-methods/%s:authenticate:callback"
 )
 
 type (
@@ -162,7 +157,7 @@ func unwrapMessage(ctx context.Context, encodedWrappedMsg string) (*request.Wrap
 // both a Request.State and Request.Token.
 //
 // It first checks the cache of derived wrappers. If it's not found in the cache, it
-// generates a key based on the scope's oidc DEK, using the scopeId and authMethodId
+// generates a key based on the scope's oidc DEK, using the scopeId and authMethod
 // as salt and info for derivation, creates a wrapper for the new key, adds that wrapper
 // to the cache and the returns the wrapper for the new key.
 //
@@ -193,7 +188,7 @@ func requestWrappingWrapper(ctx context.Context, k *kms.Kms, scopeId, authMethod
 		return derivedWrapper.(*aead.Wrapper), nil
 	}
 
-	// okay, I guess we need to derive a new key for this combo of oidcWrapper and authMethodId
+	// okay, I guess we need to derive a new key for this combo of oidcWrapper and authMethod
 	reader, err := kms.NewDerivedReader(oidcWrapper, 32, []byte(authMethodId), []byte(scopeId))
 	if err != nil {
 		return nil, errors.Wrap(err, op)

internal/auth/oidc/service_callback.go

@@ -46,7 +46,7 @@ func Callback(
 	oidcRepoFn OidcRepoFactory,
 	iamRepoFn IamRepoFactory,
 	atRepoFn AuthTokenRepoFactory,
-	authMethodId,
+	am *AuthMethod,
 	state, code string) (finalRedirect string, e error) {
 	const op = "oidc.Callback"
 	if oidcRepoFn == nil {
@@ -58,8 +58,8 @@ func Callback(
 	if atRepoFn == nil {
 		return "", errors.New(errors.InvalidParameter, op, "missing auth token repository function")
 	}
-	if authMethodId == "" {
-		return "", errors.New(errors.InvalidParameter, op, "missing auth method id")
+	if am == nil {
+		return "", errors.New(errors.InvalidParameter, op, "missing auth method")
 	}
 	if state == "" {
 		return "", errors.New(errors.InvalidParameter, op, "missing state")
@@ -82,17 +82,10 @@ func Callback(
 	if err != nil {
 		return "", errors.Wrap(err, op)
 	}
-	am, err := r.lookupAuthMethod(ctx, authMethodId)
-	if err != nil {
-		return "", errors.Wrap(err, op)
-	}
-	if am == nil {
-		return "", errors.New(errors.RecordNotFound, op, fmt.Sprintf("auth method %s not found", authMethodId))
-	}
 
 	// state is a proto request.Wrapper msg, which contains a cipher text field, so
 	// we need the derived wrapper that was used to encrypt it.
-	requestWrapper, err := requestWrappingWrapper(ctx, r.kms, am.ScopeId, authMethodId)
+	requestWrapper, err := requestWrappingWrapper(ctx, r.kms, am.ScopeId, am.GetPublicId())
 	if err != nil {
 		return "", errors.Wrap(err, op)
 	}
@@ -103,8 +96,8 @@ func Callback(
 
 	// it appears the authentication request was started with one auth method
 	// and the callback came to a different auth method.
-	if stateWrapper.AuthMethodId != authMethodId {
-		return "", errors.New(errors.InvalidParameter, op, fmt.Sprintf("%s auth method id does not match request wrapper auth method id: %s", authMethodId, stateWrapper))
+	if stateWrapper.AuthMethodId != am.GetPublicId() {
+		return "", errors.New(errors.InvalidParameter, op, fmt.Sprintf("%s auth method id does not match request wrapper auth method id: %s", am.GetPublicId(), stateWrapper))
 	}
 
 	stateBytes, err := decryptMessage(ctx, requestWrapper, stateWrapper)

internal/auth/oidc/service_callback_test.go

@@ -120,7 +120,6 @@ func Test_Callback(t *testing.T) {
 		iamRepoFn            IamRepoFactory       // returns a new iam repo
 		atRepoFn             AuthTokenRepoFactory // returns a new auth token repo
 		am                   *AuthMethod          // the authmethod for the test
-		authMethodIdOverride *string              // an override of the authmethod.PublcId as a callback parameters
 		state                string               // state parameter for test provider and Callback(...)
 		code                 string               // code parameter for test provider and Callback(...)
 		wantSubject          string               // sub claim from id token
@@ -224,35 +223,23 @@ func Test_Callback(t *testing.T) {
 			wantErrContains: "missing code",
 		},
 		{
-			name:            "missing-auth-method-id",
+			name:            "missing-auth-method",
 			oidcRepoFn:      repoFn,
 			iamRepoFn:       iamRepoFn,
 			atRepoFn:        atRepoFn,
-			am:              func() *AuthMethod { am := AllocAuthMethod(); return &am }(),
+			am:              func() *AuthMethod { return nil }(),
 			state:           testState(t, testAuthMethod, kmsCache, testTokenRequestId, 2000*time.Second, "https://testcontroler.com/hi-alice", testConfigHash, testNonce),
 			code:            "simple",
 			wantErrMatch:    errors.T(errors.InvalidParameter),
 			wantErrContains: "missing auth method",
 		},
 		{
-			name:            "bad-auth-method-id",
-			oidcRepoFn:      repoFn,
-			iamRepoFn:       iamRepoFn,
-			atRepoFn:        atRepoFn,
-			am:              func() *AuthMethod { am := AllocAuthMethod(); am.PublicId = "not-valid"; return &am }(),
-			state:           testState(t, testAuthMethod, kmsCache, testTokenRequestId, 2000*time.Second, "https://testcontroler.com/hi-alice", testConfigHash, testNonce),
-			code:            "simple",
-			wantErrMatch:    errors.T(errors.RecordNotFound),
-			wantErrContains: "auth method not-valid not found",
-		},
-		{
-			name:                 "mismatch-auth-method-id", // must remain the first test
+			name:                 "mismatch-auth-method",
 			oidcRepoFn:           repoFn,
 			iamRepoFn:            iamRepoFn,
 			atRepoFn:             atRepoFn,
 			am:                   testAuthMethod,
-			authMethodIdOverride: &testAuthMethod2.PublicId,
-			state:                testState(t, testAuthMethod, kmsCache, testTokenRequestId, 2000*time.Second, "https://testcontroler.com/hi-alice", testConfigHash, testNonce),
+			state:                testState(t, testAuthMethod2, kmsCache, testTokenRequestId, 2000*time.Second, "https://testcontroler.com/hi-alice", testConfigHash, testNonce),
 			code:                 "simple",
 			wantErrMatch:         errors.T(errors.InvalidParameter),
 			wantErrContains:      "auth method id does not match request wrapper auth method id",
@@ -339,18 +326,12 @@ func Test_Callback(t *testing.T) {
 			if len(info) > 0 {
 				tp.SetUserInfoReply(info)
 			}
-			var authMethodId string
-			if tt.authMethodIdOverride != nil {
-				authMethodId = *tt.authMethodIdOverride
-			} else {
-				authMethodId = tt.am.PublicId
-			}
 
 			gotRedirect, err := Callback(ctx,
 				tt.oidcRepoFn,
 				tt.iamRepoFn,
 				tt.atRepoFn,
-				authMethodId,
+				tt.am,
 				tt.state,
 				tt.code,
 			)
@@ -370,7 +351,11 @@ func Test_Callback(t *testing.T) {
 				var entries []oplog.Entry
 				err = rw.SearchWhere(ctx, &entries, "1=?", []interface{}{1})
 				require.NoError(err)
-				require.Equalf(0, len(entries), "should not have found oplog entry for %s", tt.am.PublicId)
+				amId := ""
+				if tt.am != nil {
+					amId = tt.am.PublicId
+				}
+				require.Equalf(0, len(entries), "should not have found oplog entry for %s", amId)
 				return
 			}
 			require.NoError(err)
@@ -476,7 +461,7 @@ func Test_Callback(t *testing.T) {
 			repoFn,
 			iamRepoFn,
 			atRepoFn,
-			testAuthMethod.PublicId,
+			testAuthMethod,
 			state,
 			"simple",
 		)
@@ -488,7 +473,7 @@ func Test_Callback(t *testing.T) {
 			repoFn,
 			iamRepoFn,
 			atRepoFn,
-			testAuthMethod.PublicId,
+			testAuthMethod,
 			state,
 			"simple",
 		)
@@ -563,9 +548,9 @@ func Test_StartAuth_to_Callback(t *testing.T) {
 
 		// the test controller is stateful and needs to know what auth method id
 		// it's suppose to operate on
-		controller.SetAuthMethodId(endToEndAuthMethod.PublicId)
+		controller.SetAuthMethod(endToEndAuthMethod)
 
-		authUrl, _, _, err := StartAuth(ctx, repoFn, endToEndAuthMethod.PublicId)
+		authUrl, _, err := StartAuth(ctx, repoFn, endToEndAuthMethod.PublicId)
 		require.NoError(err)
 
 		authParams, err := url.ParseQuery(authUrl.RawQuery)

internal/auth/oidc/service_start_auth.go

@@ -30,33 +30,33 @@ import (
 // WithRoundTripPayload(string) provides an option for a client roundtrip
 // payload. This payload will be added to the final redirect as a query
 // parameter.
-func StartAuth(ctx context.Context, oidcRepoFn OidcRepoFactory, authMethodId string, opt ...Option) (authUrl *url.URL, tokenUrl *url.URL, tokenId string, e error) {
+func StartAuth(ctx context.Context, oidcRepoFn OidcRepoFactory, authMethodId string, opt ...Option) (authUrl *url.URL, tokenId string, e error) {
 	const op = "oidc.StartAuth"
 	if authMethodId == "" {
-		return nil, nil, "", errors.New(errors.InvalidParameter, op, "missing auth method id")
+		return nil, "", errors.New(errors.InvalidParameter, op, "missing auth method id")
 	}
 	if oidcRepoFn == nil {
-		return nil, nil, "", errors.New(errors.InvalidParameter, op, "missing oidc repo function")
+		return nil, "", errors.New(errors.InvalidParameter, op, "missing oidc repo function")
 	}
 	r, err := oidcRepoFn()
 	if err != nil {
-		return nil, nil, "", errors.Wrap(err, op)
+		return nil, "", errors.Wrap(err, op)
 	}
 	am, err := r.lookupAuthMethod(ctx, authMethodId)
 	if err != nil {
-		return nil, nil, "", errors.Wrap(err, op)
+		return nil, "", errors.Wrap(err, op)
 	}
 	if am == nil {
-		return nil, nil, "", errors.New(errors.RecordNotFound, op, fmt.Sprintf("auth method %s not found", authMethodId))
+		return nil, "", errors.New(errors.RecordNotFound, op, fmt.Sprintf("auth method %s not found", authMethodId))
 	}
 	if am.OperationalState == string(InactiveState) {
-		return nil, nil, "", errors.New(errors.AuthMethodInactive, op, "not allowed to start authentication attempt")
+		return nil, "", errors.New(errors.AuthMethodInactive, op, "not allowed to start authentication attempt")
 	}
 
 	// get the provider from the cache (if possible)
 	provider, err := providerCache().get(ctx, am)
 	if err != nil {
-		return nil, nil, "", errors.Wrap(err, op)
+		return nil, "", errors.Wrap(err, op)
 	}
 	callbackRedirect := fmt.Sprintf(CallbackEndpoint, am.GetApiUrl(), authMethodId)
 
@@ -72,15 +72,15 @@ func StartAuth(ctx context.Context, oidcRepoFn OidcRepoFactory, authMethodId str
 	exp := timestamppb.New(now.Add(AttemptExpiration).Truncate(time.Second))
 	tokenRequestId, err := authtoken.NewAuthTokenId()
 	if err != nil {
-		return nil, nil, "", errors.Wrap(err, op)
+		return nil, "", errors.Wrap(err, op)
 	}
 	nonce, err := oidc.NewID()
 	if err != nil {
-		return nil, nil, "", errors.New(errors.Unknown, op, "unable to generate nonce", errors.WithWrap(err))
+		return nil, "", errors.New(errors.Unknown, op, "unable to generate nonce", errors.WithWrap(err))
 	}
 	hash, err := provider.ConfigHash()
 	if err != nil {
-		return nil, nil, "", errors.New(errors.Unknown, op, "unable to get provider config hash", errors.WithWrap(err))
+		return nil, "", errors.New(errors.Unknown, op, "unable to get provider config hash", errors.WithWrap(err))
 	}
 	st := &request.State{
 		TokenRequestId:     tokenRequestId,
@@ -93,11 +93,11 @@ func StartAuth(ctx context.Context, oidcRepoFn OidcRepoFactory, authMethodId str
 
 	requestWrapper, err := requestWrappingWrapper(ctx, r.kms, am.ScopeId, authMethodId)
 	if err != nil {
-		return nil, nil, "", errors.Wrap(err, op)
+		return nil, "", errors.Wrap(err, op)
 	}
 	encodedEncryptedSt, err := encryptMessage(ctx, requestWrapper, am, st)
 	if err != nil {
-		return nil, nil, "", errors.Wrap(err, op)
+		return nil, "", errors.Wrap(err, op)
 	}
 	oidcOpts := []oidc.Option{
 		oidc.WithState(string(encodedEncryptedSt)),
@@ -119,16 +119,16 @@ func StartAuth(ctx context.Context, oidcRepoFn OidcRepoFactory, authMethodId str
 		callbackRedirect,
 		oidcOpts...)
 	if err != nil {
-		return nil, nil, "", errors.New(errors.Unknown, op, "unable to create oidc request", errors.WithWrap(err))
+		return nil, "", errors.New(errors.Unknown, op, "unable to create oidc request", errors.WithWrap(err))
 	}
 
 	u, err := provider.AuthURL(ctx, oidcReq)
 	if err != nil {
-		return nil, nil, "", errors.New(errors.Unknown, op, "unable to get auth url from provider", errors.WithWrap(err))
+		return nil, "", errors.New(errors.Unknown, op, "unable to get auth url from provider", errors.WithWrap(err))
 	}
 	authUrl, err = url.Parse(u)
 	if err != nil {
-		return nil, nil, "", errors.New(errors.Unknown, op, "unable to parse auth url", errors.WithWrap(err))
+		return nil, "", errors.New(errors.Unknown, op, "unable to parse auth url", errors.WithWrap(err))
 	}
 
 	t := &request.Token{
@@ -137,12 +137,7 @@ func StartAuth(ctx context.Context, oidcRepoFn OidcRepoFactory, authMethodId str
 	}
 	encodedEncryptedTk, err := encryptMessage(ctx, requestWrapper, am, t)
 	if err != nil {
-		return nil, nil, "", errors.Wrap(err, op)
+		return nil, "", errors.Wrap(err, op)
 	}
-	tokenUrl, err = url.Parse(fmt.Sprintf(TokenEndpoint, am.GetApiUrl(), authMethodId))
-	if err != nil {
-		return nil, nil, "", errors.New(errors.Unknown, op, "unable to generate token URL", errors.WithWrap(err))
-	}
-
-	return authUrl, tokenUrl, encodedEncryptedTk, nil
+	return authUrl, encodedEncryptedTk, nil
 }

internal/auth/oidc/service_start_auth_test.go

@@ -158,18 +158,16 @@ func Test_StartAuth(t *testing.T) {
 			}
 
 			now := time.Now()
-			authUrl, tokenUrl, tokenId, err := StartAuth(ctx, tt.repoFn, tt.authMethod.PublicId, WithRoundtripPayload(tt.roundTripper))
+			authUrl, tokenId, err := StartAuth(ctx, tt.repoFn, tt.authMethod.PublicId, WithRoundtripPayload(tt.roundTripper))
 			if tt.wantErrMatch != nil {
 				require.Error(err)
 				assert.Nil(authUrl)
-				assert.Nil(tokenUrl)
 				assert.Truef(errors.Match(tt.wantErrMatch, err), "want err code: %q got: %q", tt.wantErrMatch, err)
 				assert.Contains(err.Error(), tt.wantErrContains)
 				return
 			}
 			require.NoError(err)
 			require.NotNil(authUrl)
-			require.NotNil(tokenUrl)
 
 			authParams, err := url.ParseQuery(authUrl.RawQuery)
 			require.NoError(err)

internal/auth/oidc/testing.go

@@ -339,12 +339,12 @@ func TestPendingToken(
 type testControllerSrv struct {
 	mu sync.RWMutex
 
-	authMethodId string
-	oidcRepoFn   OidcRepoFactory
-	iamRepoFn    IamRepoFactory
-	atRepoFn     AuthTokenRepoFactory
-	httpServer   *httptest.Server
-	t            *testing.T
+	authMethod *AuthMethod
+	oidcRepoFn OidcRepoFactory
+	iamRepoFn  IamRepoFactory
+	atRepoFn   AuthTokenRepoFactory
+	httpServer *httptest.Server
+	t          *testing.T
 }
 
 // startTestControllerSrv returns a running testControllerSrv
@@ -365,22 +365,22 @@ func startTestControllerSrv(t *testing.T, oidcRepoFn OidcRepoFactory, iamRepoFn
 	return s
 }
 
-// SetAuthMethodId will set the auth method id used for various
+// SetAuthMethod will set the auth method id used for various
 // operations
-func (s *testControllerSrv) SetAuthMethodId(authMethodId string) {
+func (s *testControllerSrv) SetAuthMethod(authMethod *AuthMethod) {
 	s.t.Helper()
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.authMethodId = authMethodId
+	s.authMethod = authMethod
 }
 
-// AuthMethodId returns the auth method id currently being used
+// AuthMethod returns the auth method id currently being used
 // for various operations
-func (s *testControllerSrv) AuthMethodId() string {
+func (s *testControllerSrv) AuthMethod() *AuthMethod {
 	s.t.Helper()
 	s.mu.RLock()
 	defer s.mu.RUnlock()
-	return s.authMethodId
+	return s.authMethod
 }
 
 // Addr returns the scheme.host.port of the running srv.
@@ -401,8 +401,8 @@ func (s *testControllerSrv) FinalRedirectUrl() string {
 func (s *testControllerSrv) CallbackUrl() string {
 	s.t.Helper()
 	require := require.New(s.t)
-	require.NotEmptyf(s.authMethodId, "auth method id was missing")
-	return fmt.Sprintf(CallbackEndpoint, s.Addr(), s.authMethodId)
+	require.NotNil(s.authMethod, "auth method was missing")
+	return fmt.Sprintf(CallbackEndpoint, s.Addr(), s.authMethod.GetPublicId())
 }
 
 // ServeHTTP satisfies the http.Handler interface
@@ -410,7 +410,7 @@ func (s *testControllerSrv) ServeHTTP(w http.ResponseWriter, req *http.Request)
 	s.t.Helper()
 	require := require.New(s.t)
 	switch req.URL.Path {
-	case fmt.Sprintf("/v1/auth-methods/%s:authenticate:callback", s.authMethodId):
+	case fmt.Sprintf("/v1/auth-methods/%s:authenticate:callback", s.authMethod.GetPublicId()):
 		err := req.ParseForm()
 		require.NoErrorf(err, "%s: internal error: %w", "callback", err)
 		state := req.FormValue("state")
@@ -427,7 +427,7 @@ func (s *testControllerSrv) ServeHTTP(w http.ResponseWriter, req *http.Request)
 				s.oidcRepoFn,
 				s.iamRepoFn,
 				s.atRepoFn,
-				s.authMethodId,
+				s.authMethod,
 				state,
 				code,
 			)

internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto

@@ -134,9 +134,6 @@ message OidcAuthMethodAuthenticateStartResponse {
 	// The returned authentication URL
 	string auth_url = 10 [json_name="auth_url"];
 
-	// The returned token URL
-	string token_url = 20 [json_name="token_url"];
-
 	// The returned token ID
 	string token_id = 30 [json_name="token_id"];
 }

internal/servers/controller/handlers/authmethods/oidc.go

@@ -150,9 +150,9 @@ func (s Service) authenticateOidc(ctx context.Context, req *pbs.AuthenticateRequ
 	}
 	switch req.GetCommand() {
 	case startCommand:
-		return s.authenticateOidcStart(ctx, req, authResults)
+		return s.authenticateOidcStart(ctx, req)
 	case callbackCommand:
-		return s.authenticateOidcCallback(ctx, req, authResults)
+		return s.authenticateOidcCallback(ctx, req)
 	case tokenCommand:
 		return s.authenticateOidcToken(ctx, req, authResults)
 	}
@@ -160,79 +160,106 @@ func (s Service) authenticateOidc(ctx context.Context, req *pbs.AuthenticateRequ
 	return &pbs.AuthenticateResponse{Command: req.GetCommand(), Attributes: nil}, nil
 }
 
-func (s Service) authenticateOidcStart(ctx context.Context, req *pbs.AuthenticateRequest, authResults *auth.VerifyResults) (*pbs.AuthenticateResponse, error) {
+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(errors.InvalidParameter, op, "Nil request.")
 	}
-	if authResults == nil {
-		return nil, errors.New(errors.InvalidParameter, op, "Nil auth results.")
-	}
 
 	var opts []oidc.Option
-	if req.GetAttributes() != nil {
-		attrs := new(pbs.OidcStartAttributes)
-		if err := handlers.StructToProto(req.GetAttributes(), attrs); err != nil {
-			return nil, errors.New(errors.InvalidParameter, op, "Error parsing request attributes.")
-		}
-		if attrs.GetCachedRoundtripPayload() != "" {
-			opts = append(opts, oidc.WithRoundtripPayload(attrs.GetCachedRoundtripPayload()))
-		}
+	attrs := new(pbs.OidcStartAttributes)
+	if err := handlers.StructToProto(req.GetAttributes(), attrs); err != nil {
+		return nil, errors.New(errors.InvalidParameter, op, "Error parsing request attributes.")
+	}
+	if attrs.GetCachedRoundtripPayload() != "" {
+		opts = append(opts, oidc.WithRoundtripPayload(attrs.GetCachedRoundtripPayload()))
 	}
 
-	authUrl, tokenUrl, tokenId, err := oidc.StartAuth(ctx, s.oidcRepoFn, req.GetAuthMethodId(), opts...)
+	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.")
 	}
 
-	resp := &pb.OidcAuthMethodAuthenticateStartResponse{
-		AuthUrl:  authUrl.String(),
-		TokenUrl: tokenUrl.String(),
-		TokenId:  tokenId,
+	respAttrs := &pb.OidcAuthMethodAuthenticateStartResponse{
+		AuthUrl: authUrl.String(),
+		TokenId: tokenId,
 	}
-
-	attrs, err := handlers.ProtoToStruct(resp)
-	if err != nil {
+	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 &pbs.AuthenticateResponse{Command: req.GetCommand(), Attributes: attrs}, nil
+	return resp, nil
 }
 
-func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.AuthenticateRequest, authResults *auth.VerifyResults) (*pbs.AuthenticateResponse, error) {
+// authenticateOidcCallback behaves differently than other service methods.
+// Because of the way it this is called by the end user, it should only return
+// an error if we are unable to lookup the auth method or the request
+// parameters were invalid.  All other errors should be returned back through
+// the response as a finalRedirectUrl to an endpoint that can properly show the
+// error details.
+func (s Service) authenticateOidcCallback(ctx context.Context, req *pbs.AuthenticateRequest) (*pbs.AuthenticateResponse, error) {
 	const op = "authmethod_service.(Service).authenticateOidcCallback"
 	if req == nil {
 		return nil, errors.New(errors.InvalidParameter, op, "Nil request.")
 	}
-	if authResults == nil {
-		return nil, errors.New(errors.InvalidParameter, op, "Nil auth results.")
+
+	repo, err := s.oidcRepoFn()
+	if err != nil {
+		return nil, errors.Wrap(err, op)
+	}
+	am, err := repo.LookupAuthMethod(ctx, req.GetAuthMethodId())
+	if err != nil {
+		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()))
+	}
+	if am.GetApiUrl() == "" {
+		return nil, errors.New(errors.InvalidParameter, op, "auth method doesn't have api url defined")
+	}
+
+	errRedirectBase := fmt.Sprintf(oidc.AuthenticationErrorsEndpoint, am.GetApiUrl())
+	errResponse := func(err error) *pbs.AuthenticateResponse {
+		u := make(url.Values)
+		// TODO: Decide how to format the domain error to match OIDC error format
+		u.Add("error", err.Error())
+		errRedirect := fmt.Sprintf("%s?%s", errRedirectBase, u.Encode())
+		return &pbs.AuthenticateResponse{Command: callbackCommand, Attributes: &structpb.Struct{
+			Fields: map[string]*structpb.Value{
+				"final_redirect_url": structpb.NewStringValue(errRedirect),
+			},
+		}}
 	}
 
 	attrs := new(pb.OidcAuthMethodAuthenticateCallbackRequest)
 	// Note that this conversion has already happened in the validate call so we don't expect errors here.
 	if err := handlers.StructToProto(req.GetAttributes(), attrs, handlers.WithDiscardUnknownFields(true)); err != nil {
-		return nil, err
+		return errResponse(err), nil
 	}
 
-	finalRedirectUrl, err := oidc.Callback(
+	var finalRedirectUrl string
+	if attrs.GetError() != "" {
+		// TODO: Package the OIDC error into the redirectUrl
+		return errResponse(fmt.Errorf("Error: %q, Details: %q", attrs.GetError(), attrs.GetErrorDescription())), nil
+	}
+	finalRedirectUrl, err = oidc.Callback(
 		ctx,
 		s.oidcRepoFn,
 		oidc.IamRepoFactory(s.iamRepoFn),
 		s.atRepoFn,
-		req.GetAuthMethodId(),
+		am,
 		attrs.GetState(),
 		attrs.GetCode())
 	if err != nil {
-		// TODO: Log something more meaningful
-		return nil, errors.New(errors.InvalidParameter, op, "Callback validation failed.", errors.WithWrap(err))
+		return errResponse(errors.New(errors.InvalidParameter, op, "Callback validation failed.", errors.WithWrap(err))), nil
 	}
 
 	respAttrs, err := handlers.ProtoToStruct(&pb.OidcAuthMethodAuthenticateCallbackResponse{
 		FinalRedirectUrl: finalRedirectUrl,
 	})
 	if err != nil {
-		return nil, errors.New(errors.Internal, op, "Error marshaling parameters")
+		return errResponse(errors.New(errors.Internal, op, "Error marshaling parameters")), nil
 	}
 
 	return &pbs.AuthenticateResponse{Command: req.GetCommand(), Attributes: respAttrs}, nil
@@ -339,12 +366,7 @@ func validateAuthenticateOidcRequest(req *pbs.AuthenticateRequest) error {
 			break
 		}
 
-		if attrs.GetError() != "" {
-			// TODO: Log more info.
-			return handlers.ApiErrorWithCodeAndMessage(codes.Unauthenticated, "OIDC provider returned an error.")
-		}
-
-		if attrs.GetCode() == "" {
+		if attrs.GetCode() == "" && attrs.GetError() == "" {
 			badFields[codeField] = "Code field not supplied in callback request."
 		}
 

internal/servers/controller/handlers/authmethods/oidc_test.go

@@ -1373,8 +1373,6 @@ func TestAuthenticate_OIDC_Start(t *testing.T) {
 			require.NotNil(m)
 			require.Contains(m, "auth_url")
 			require.NotEmpty(m["auth_url"])
-			require.Contains(m, "token_url")
-			require.NotEmpty(m["token_url"])
 			require.Contains(m, "token_id")
 			require.NotEmpty(m["token_id"])
 		})