diff --git a/api/authmethods/oidc_auth_method_authenticate_start_response.gen.go b/api/authmethods/oidc_auth_method_authenticate_start_response.gen.go index 8573fa600..e06876b2a 100644 --- a/api/authmethods/oidc_auth_method_authenticate_start_response.gen.go +++ b/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"` } diff --git a/internal/auth/oidc/repository_auth_method_read.go b/internal/auth/oidc/repository_auth_method_read.go index 1701842aa..dde0f41ed 100644 --- a/internal/auth/oidc/repository_auth_method_read.go +++ b/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. // diff --git a/internal/auth/oidc/service.go b/internal/auth/oidc/service.go index 082f12a03..c17002204 100644 --- a/internal/auth/oidc/service.go +++ b/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) diff --git a/internal/auth/oidc/service_callback.go b/internal/auth/oidc/service_callback.go index 841bbfcc1..79f92c84e 100644 --- a/internal/auth/oidc/service_callback.go +++ b/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) diff --git a/internal/auth/oidc/service_callback_test.go b/internal/auth/oidc/service_callback_test.go index fc46fbe8e..9114dc409 100644 --- a/internal/auth/oidc/service_callback_test.go +++ b/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) diff --git a/internal/auth/oidc/service_start_auth.go b/internal/auth/oidc/service_start_auth.go index 9740e5d2c..9d9d89537 100644 --- a/internal/auth/oidc/service_start_auth.go +++ b/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 } diff --git a/internal/auth/oidc/service_start_auth_test.go b/internal/auth/oidc/service_start_auth_test.go index e46234867..f7095549d 100644 --- a/internal/auth/oidc/service_start_auth_test.go +++ b/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) diff --git a/internal/auth/oidc/testing.go b/internal/auth/oidc/testing.go index 714583fd2..10b88fea4 100644 --- a/internal/auth/oidc/testing.go +++ b/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, ) diff --git a/internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto b/internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto index 3d9a9938f..31c43cd8c 100644 --- a/internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto +++ b/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"]; } diff --git a/internal/servers/controller/handlers/authmethods/oidc.go b/internal/servers/controller/handlers/authmethods/oidc.go index 5b3964245..d47a33046 100644 --- a/internal/servers/controller/handlers/authmethods/oidc.go +++ b/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." } diff --git a/internal/servers/controller/handlers/authmethods/oidc_test.go b/internal/servers/controller/handlers/authmethods/oidc_test.go index 03b765e4a..5b85a1dad 100644 --- a/internal/servers/controller/handlers/authmethods/oidc_test.go +++ b/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"]) }) diff --git a/website/content/docs/concepts/security/permissions.mdx b/website/content/docs/concepts/security/permissions.mdx index 0760cb557..a62a98afc 100644 --- a/website/content/docs/concepts/security/permissions.mdx +++ b/website/content/docs/concepts/security/permissions.mdx @@ -214,11 +214,11 @@ wildcard or templated grant strings. @@ -226,15 +226,19 @@ wildcard or templated grant strings.
  • create: Create an account
    • - +
  • list: List accounts
    • - + @@ -245,23 +249,23 @@ wildcard or templated grant strings. @@ -269,38 +273,62 @@ wildcard or templated grant strings.
  • read: Read an account
    • - +
  • update: Update an account
    • - +
  • delete: Delete an account
    • - +
  • - set-password: Set a password on an account, without requiring the current password + set-password: Set a password on an account, without + requiring the current password
    • - +
  • - change-password: Change a password on an account given the current password + change-password: Change a password on an account given + the current password
    • - + @@ -318,11 +346,11 @@ wildcard or templated grant strings. @@ -330,15 +358,19 @@ wildcard or templated grant strings.
  • create: Create an auth method
    • - +
  • list: List auth methods
    • - + @@ -349,17 +381,17 @@ wildcard or templated grant strings. @@ -367,27 +399,35 @@ wildcard or templated grant strings.
  • read: Read an auth method
    • - +
  • update: Update an auth method
    • - +
  • delete: Delete an auth method
    • - +
  • authenticate: Authenticate to an auth method
    • - + @@ -405,11 +445,11 @@ wildcard or templated grant strings. @@ -417,9 +457,11 @@ wildcard or templated grant strings.
  • list: List auth tokens
    • - + @@ -430,17 +472,17 @@ wildcard or templated grant strings. @@ -448,15 +490,19 @@ wildcard or templated grant strings.
  • read: Read an auth token
    • - +
  • delete: Delete an auth token
    • - + @@ -475,11 +521,11 @@ wildcard or templated grant strings. @@ -487,15 +533,19 @@ wildcard or templated grant strings.
  • create: Create a group
    • - +
  • list: List groups
    • - + @@ -506,17 +556,17 @@ wildcard or templated grant strings. @@ -524,39 +574,51 @@ wildcard or templated grant strings.
  • read: Read a group
    • - +
  • update: Update a group
    • - +
  • delete: Delete a group
    • - +
  • add-members: Add members to a group
    • - +
  • set-members: Set the full set of members on a group
    • - +
  • remove-members: Remove members from a group
    • - + @@ -573,11 +635,11 @@ wildcard or templated grant strings. @@ -585,15 +647,19 @@ wildcard or templated grant strings.
  • create: Create a host
    • - +
  • list: List hosts
    • - + @@ -604,23 +670,23 @@ wildcard or templated grant strings. @@ -628,24 +694,36 @@ wildcard or templated grant strings.
  • read: Read a host
    • - +
  • update: Update a host
    • - +
  • delete: Delete a host
    • - + @@ -662,11 +740,11 @@ wildcard or templated grant strings. @@ -674,15 +752,19 @@ wildcard or templated grant strings.
  • create: Create a host catalog
    • - +
  • list: List host catalogs
    • - + @@ -693,17 +775,17 @@ wildcard or templated grant strings. @@ -711,21 +793,27 @@ wildcard or templated grant strings.
  • read: Read a host catalog
    • - +
  • update: Update a host catalog
    • - +
  • delete: Delete a host catalog
    • - + @@ -742,11 +830,11 @@ wildcard or templated grant strings. @@ -754,15 +842,19 @@ wildcard or templated grant strings.
  • create: Create a host set
    • - +
  • list: List host sets
    • - + @@ -773,23 +865,23 @@ wildcard or templated grant strings. @@ -797,45 +889,69 @@ wildcard or templated grant strings.
  • read: Read a host set
    • - +
  • update: Update a host set
    • - +
  • delete: Delete a host set
    • - +
  • add-hosts: Add hosts to a host-set
    • - +
  • set-hosts: Set the full set of hosts on a host set
    • - +
  • remove-hosts: Remove hosts from a host set
    • - + @@ -854,11 +970,11 @@ wildcard or templated grant strings. @@ -866,15 +982,19 @@ wildcard or templated grant strings.
  • create: Create a role
    • - +
  • list: List roles
    • - + @@ -885,17 +1005,17 @@ wildcard or templated grant strings. @@ -903,57 +1023,76 @@ wildcard or templated grant strings.
  • read: Read a role
    • - +
  • update: Update a role
    • - +
  • delete: Delete a role
    • - +
  • add-principals: Add principals to a role
    • - +
  • - set-principals: Set the full set of principals on a role + set-principals: Set the full set of principals on a + role
    • - +
  • remove-principals: Remove principals from a role
    • - +
  • add-grants: Add grants to a role
    • - +
  • set-grants: Set the full set of grants on a role
    • - +
  • remove-grants: Remove grants from a role
    • - + @@ -971,11 +1110,11 @@ wildcard or templated grant strings. @@ -983,15 +1122,19 @@ wildcard or templated grant strings.
  • create: Create a scope
    • - +
  • list: List scopes
    • - + @@ -1002,17 +1145,17 @@ wildcard or templated grant strings. @@ -1020,21 +1163,27 @@ wildcard or templated grant strings.
  • read: Read a scope
    • - +
  • update: Update a scope
    • - +
  • delete: Delete a scope
    • - + @@ -1051,11 +1200,11 @@ wildcard or templated grant strings. @@ -1063,9 +1212,11 @@ wildcard or templated grant strings.
  • list: List sessions
    • - + @@ -1076,17 +1227,17 @@ wildcard or templated grant strings. @@ -1094,27 +1245,37 @@ wildcard or templated grant strings.
  • read: Read a session
    • - +
  • cancel: Cancel a session
    • - +
  • - read:self: Read a session, which must be associated with the calling user + read:self: Read a session, which must be associated + with the calling user
    • - +
  • - cancel:self: Cancel a session, which must be associated with the calling user + cancel:self: Cancel a session, which must be associated + with the calling user
    • - + @@ -1131,11 +1292,11 @@ wildcard or templated grant strings. @@ -1143,15 +1304,19 @@ wildcard or templated grant strings.
  • create: Create a target
    • - +
  • list: List targets
    • - + @@ -1162,17 +1327,17 @@ wildcard or templated grant strings. @@ -1180,45 +1345,60 @@ wildcard or templated grant strings.
  • read: Read a target
    • - +
  • update: Update a target
    • - +
  • delete: Delete a target
    • - +
  • add-host-sets: Add host sets to a target
    • - +
  • - set-host-sets: Set the full set of host sets on a target + set-host-sets: Set the full set of host sets on a + target
    • - +
  • remove-host-sets: Remove host sets from a target
    • - +
  • authorize-session: Authorize a session via the target
    • - + @@ -1236,11 +1416,11 @@ wildcard or templated grant strings. @@ -1248,15 +1428,19 @@ wildcard or templated grant strings.
  • create: Create a user
    • - +
  • list: List users
    • - + @@ -1267,17 +1451,17 @@ wildcard or templated grant strings. @@ -1285,39 +1469,51 @@ wildcard or templated grant strings.
  • read: Read a user
    • - +
  • update: Update a user
    • - +
  • delete: Delete a user
    • - +
  • add-accounts: Add accounts to a user
    • - +
  • set-accounts: Set the full set of accounts on a user
    • - +
  • remove-accounts: Remove accounts from a user
    • - +