From 78d5f2b4dcd14401e3ee08bacc1f6bc608f22164 Mon Sep 17 00:00:00 2001 From: Mistah J <26472282+mistahj67@users.noreply.github.com> Date: Thu, 9 Jan 2025 09:07:50 -1000 Subject: [PATCH 1/3] fix: missing error handling during sso login --- cmd/api/src/api/auth.go | 12 +++++++----- cmd/api/src/api/constant.go | 1 + cmd/api/src/api/url.go | 18 ++++++++++++++++++ cmd/api/src/api/v2/auth/oidc.go | 23 +++++++++++------------ cmd/api/src/api/v2/auth/saml.go | 23 +++++++++++------------ cmd/api/src/api/v2/helpers.go | 16 ---------------- 6 files changed, 48 insertions(+), 45 deletions(-) diff --git a/cmd/api/src/api/auth.go b/cmd/api/src/api/auth.go index f7d5a77eb4..61bb35e35d 100644 --- a/cmd/api/src/api/auth.go +++ b/cmd/api/src/api/auth.go @@ -363,7 +363,7 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res // Generate commit ID for audit logging if commitID, err = uuid.NewV4(); err != nil { log.Warnf("Error generating commit ID for login: %s", err) - WriteErrorResponse(requestCtx, BuildErrorResponse(http.StatusInternalServerError, "audit log creation failure", request), response) + RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") return } @@ -378,14 +378,15 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res if user, err = s.db.LookupUser(requestCtx, principalNameOrEmail); err != nil { auditLogFields["error"] = err if !errors.Is(err, database.ErrNotFound) { - HandleDatabaseError(request, response, err) + log.Errorf("[SSO] Error looking up user: %v", err) + RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else { - WriteErrorResponse(requestCtx, BuildErrorResponse(http.StatusForbidden, "user is not allowed", request), response) + RedirectToLoginURL(response, request, "Your user is not allowed, please contact your Administrator") } } else { if !user.SSOProviderID.Valid || ssoProvider.ID != user.SSOProviderID.Int32 { auditLogFields["error"] = ErrUserNotAuthorizedForProvider - WriteErrorResponse(requestCtx, BuildErrorResponse(http.StatusForbidden, "user is not allowed", request), response) + RedirectToLoginURL(response, request, "Your user is not allowed, please contact your Administrator") return } @@ -395,7 +396,8 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res response.Header().Add(headers.Location.String(), locationURL.String()) response.WriteHeader(http.StatusFound) } else { - WriteErrorResponse(requestCtx, BuildErrorResponse(http.StatusInternalServerError, "session creation failure", request), response) + log.Errorf("[SSO] session creation failure %v", err) + RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } } else { auditLogOutcome = model.AuditLogStatusSuccess diff --git a/cmd/api/src/api/constant.go b/cmd/api/src/api/constant.go index d39612a4cd..1fe172b81b 100644 --- a/cmd/api/src/api/constant.go +++ b/cmd/api/src/api/constant.go @@ -27,6 +27,7 @@ const ( // UserInterfacePath is the static path to the UI landing page UserInterfacePath = "/ui" + UserLoginPath = "/ui/login" UserDisabledPath = "/ui/user-disabled" // Authorization schemes diff --git a/cmd/api/src/api/url.go b/cmd/api/src/api/url.go index 080e040a79..09ffd9664b 100644 --- a/cmd/api/src/api/url.go +++ b/cmd/api/src/api/url.go @@ -17,8 +17,12 @@ package api import ( + "net/http" "net/url" "strings" + + "github.com/specterops/bloodhound/headers" + "github.com/specterops/bloodhound/src/ctx" ) func NewJoinedURL(base string, extensions ...string) (string, error) { @@ -49,3 +53,17 @@ func URLJoinPath(target url.URL, extensions ...string) url.URL { return target } + +func RedirectToLoginURL(response http.ResponseWriter, request *http.Request, errorMessage string) { + hostURL := *ctx.FromRequest(request).Host + redirectURL := URLJoinPath(hostURL, UserLoginPath) + + // Optionally, include the error message as a query parameter or in session storage + query := redirectURL.Query() + query.Set("error", errorMessage) + redirectURL.RawQuery = query.Encode() + + // Redirect to the login page + response.Header().Add(headers.Location.String(), redirectURL.String()) + response.WriteHeader(http.StatusFound) +} diff --git a/cmd/api/src/api/v2/auth/oidc.go b/cmd/api/src/api/v2/auth/oidc.go index 5f397a0e1b..69fb644f21 100644 --- a/cmd/api/src/api/v2/auth/oidc.go +++ b/cmd/api/src/api/v2/auth/oidc.go @@ -34,7 +34,6 @@ import ( "github.com/specterops/bloodhound/log" "github.com/specterops/bloodhound/mediatypes" "github.com/specterops/bloodhound/src/api" - "github.com/specterops/bloodhound/src/api/v2" "github.com/specterops/bloodhound/src/config" "github.com/specterops/bloodhound/src/ctx" "github.com/specterops/bloodhound/src/database" @@ -161,16 +160,16 @@ func (s ManagementResource) OIDCLoginHandler(response http.ResponseWriter, reque if ssoProvider.OIDCProvider == nil { // SSO misconfiguration scenario - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else if state, err := config.GenerateRandomBase64String(77); err != nil { log.Warnf("[OIDC] Failed to generate state: %v", err) // Technical issues scenario - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else if provider, err := oidc.NewProvider(request.Context(), ssoProvider.OIDCProvider.Issuer); err != nil { log.Warnf("[OIDC] Failed to create OIDC provider: %v", err) // SSO misconfiguration or technical issue // Treat this as a misconfiguration scenario - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else { conf := &oauth2.Config{ ClientID: ssoProvider.OIDCProvider.ClientID, @@ -206,7 +205,7 @@ func (s ManagementResource) OIDCCallbackHandler(response http.ResponseWriter, re if ssoProvider.OIDCProvider == nil { // SSO misconfiguration scenario - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else if len(code) == 0 { // Don't want to log state but do want to know if state was present hasState := queryParams.Has(api.QueryParameterState) @@ -214,29 +213,29 @@ func (s ManagementResource) OIDCCallbackHandler(response http.ResponseWriter, re log.Warnf("[OIDC] auth code is missing, has state %t %+v", hasState, queryParams) // Missing authorization code implies a credentials or form issue // Not explicitly covered, treat as technical issue - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else if pkceVerifier, err := request.Cookie(api.AuthPKCECookieName); err != nil { log.Warnf("[OIDC] pkce cookie is missing") // Missing PKCE verifier - likely a technical or config issue - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else if len(state) == 0 { log.Warnf("[OIDC] state parameter is missing") // Missing state parameter - treat as technical issue - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else if stateCookie, err := request.Cookie(api.AuthStateCookieName); err != nil || stateCookie.Value != state[0] { log.Warnf("[OIDC] state cookie does not match %v", err) // Invalid state - treat as technical issue or misconfiguration - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else if provider, err := oidc.NewProvider(request.Context(), ssoProvider.OIDCProvider.Issuer); err != nil { log.Warnf("[OIDC] Failed to create OIDC provider: %v", err) // SSO misconfiguration scenario - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else if claims, err := getOIDCClaims(request.Context(), provider, ssoProvider, pkceVerifier, code[0]); err != nil { log.Warnf("[OIDC] %v", err) - v2.RedirectToLoginPage(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") } else if email, err := getEmailFromOIDCClaims(claims); errors.Is(err, ErrEmailMissing) { // Note email claims are not always present so we will check different claim keys for possible email log.Warnf("[OIDC] Claims did not contain any valid email address") - v2.RedirectToLoginPage(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") } else { if ssoProvider.Config.AutoProvision.Enabled { if err := jitOIDCUserCreation(request.Context(), ssoProvider, email, claims, s.db); err != nil { diff --git a/cmd/api/src/api/v2/auth/saml.go b/cmd/api/src/api/v2/auth/saml.go index b623eebb5a..edf316a02a 100644 --- a/cmd/api/src/api/v2/auth/saml.go +++ b/cmd/api/src/api/v2/auth/saml.go @@ -379,12 +379,11 @@ func (s ManagementResource) ServeSigningCertificate(response http.ResponseWriter func (s ManagementResource) SAMLLoginHandler(response http.ResponseWriter, request *http.Request, ssoProvider model.SSOProvider) { if ssoProvider.SAMLProvider == nil { // SAML misconfiguration scenario - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") - + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else if serviceProvider, err := auth.NewServiceProvider(*ctx.Get(request.Context()).Host, s.config, *ssoProvider.SAMLProvider); err != nil { log.Warnf("[SAML] Service provider creation failed: %v", err) // Technical issues scenario - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else { var ( binding = saml.HTTPRedirectBinding @@ -400,14 +399,14 @@ func (s ManagementResource) SAMLLoginHandler(response http.ResponseWriter, reque log.Warnf("[SAML] Failed creating SAML authentication request: %v", err) // SAML misconfiguration or technical issue // Since this likely indicates a configuration problem, we treat it as a misconfiguration scenario - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else { switch binding { case saml.HTTPRedirectBinding: if redirectURL, err := authReq.Redirect("", &serviceProvider); err != nil { log.Warnf("[SAML] Failed to format a redirect for SAML provider %s: %v", serviceProvider.EntityID, err) // Likely a technical or configuration issue - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else { response.Header().Add(headers.Location.String(), redirectURL.String()) response.WriteHeader(http.StatusFound) @@ -421,13 +420,13 @@ func (s ManagementResource) SAMLLoginHandler(response http.ResponseWriter, reque if _, err := response.Write([]byte(fmt.Sprintf(authInitiationContentBodyFormat, authReq.Post("")))); err != nil { log.Warnf("[SAML] Failed to write response with HTTP POST binding: %v", err) // Technical issues scenario - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } default: log.Warnf("[SAML] Unhandled binding type %s", binding) // Treating unknown binding as a misconfiguration - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } } } @@ -437,15 +436,15 @@ func (s ManagementResource) SAMLLoginHandler(response http.ResponseWriter, reque func (s ManagementResource) SAMLCallbackHandler(response http.ResponseWriter, request *http.Request, ssoProvider model.SSOProvider) { if ssoProvider.SAMLProvider == nil { // SAML misconfiguration - v2.RedirectToLoginPage(response, request, "Your SSO Connection failed, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO Connection failed, please contact your Administrator") } else if serviceProvider, err := auth.NewServiceProvider(*ctx.Get(request.Context()).Host, s.config, *ssoProvider.SAMLProvider); err != nil { log.Warnf("[SAML] Service provider creation failed: %v", err) - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else if err := request.ParseForm(); err != nil { log.Warnf("[SAML] Failed to parse form POST: %v", err) // Technical issues or invalid form data // This is not covered by acceptance criteria directly; treat as technical issue - v2.RedirectToLoginPage(response, request, "We’re having trouble connecting. Please check your internet and try again.") + api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else if assertion, err := serviceProvider.ParseResponse(request, nil); err != nil { var typedErr *saml.InvalidResponseError switch { @@ -455,11 +454,11 @@ func (s ManagementResource) SAMLCallbackHandler(response http.ResponseWriter, re log.Warnf("[SAML] Failed to parse ACS response for provider %s: %v", ssoProvider.SAMLProvider.IssuerURI, err) } // SAML credentials issue scenario (authentication failed) - v2.RedirectToLoginPage(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") } else if principalName, err := ssoProvider.SAMLProvider.GetSAMLUserPrincipalNameFromAssertion(assertion); err != nil { log.Warnf("[SAML] Failed to lookup user for SAML provider %s: %v", ssoProvider.Name, err) // SAML credentials issue scenario again - v2.RedirectToLoginPage(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") + api.RedirectToLoginURL(response, request, "Your SSO was unable to authenticate your user, please contact your Administrator") } else { if ssoProvider.Config.AutoProvision.Enabled { if err := jitSAMLUserCreation(request.Context(), ssoProvider, principalName, assertion, s.db); err != nil { diff --git a/cmd/api/src/api/v2/helpers.go b/cmd/api/src/api/v2/helpers.go index 5e73403418..44c5fced30 100644 --- a/cmd/api/src/api/v2/helpers.go +++ b/cmd/api/src/api/v2/helpers.go @@ -26,9 +26,7 @@ import ( "time" "github.com/gorilla/mux" - "github.com/specterops/bloodhound/headers" "github.com/specterops/bloodhound/src/api" - "github.com/specterops/bloodhound/src/ctx" "github.com/specterops/bloodhound/src/model" "github.com/specterops/bloodhound/src/utils" ) @@ -39,20 +37,6 @@ func ErrBadQueryParameter(request *http.Request, key string, err error) *api.Err return api.BuildErrorResponse(http.StatusBadRequest, fmt.Sprintf("query parameter \"%s\" is malformed: %v", key, err), request) } -func RedirectToLoginPage(response http.ResponseWriter, request *http.Request, errorMessage string) { - hostURL := *ctx.FromRequest(request).Host - redirectURL := api.URLJoinPath(hostURL, api.UserInterfacePath) - - // Optionally, include the error message as a query parameter or in session storage - query := redirectURL.Query() - query.Set("error", errorMessage) - redirectURL.RawQuery = query.Encode() - - // Redirect to the login page - response.Header().Add(headers.Location.String(), redirectURL.String()) - response.WriteHeader(http.StatusFound) -} - func ParseIntQueryParameter(params url.Values, key string, defaultValue int) (int, error) { if param := params.Get(key); param != "" { return strconv.Atoi(param) From c4d77e9a0c5594c778f69151ae97e82ef93e526b Mon Sep 17 00:00:00 2001 From: Mistah J <26472282+mistahj67@users.noreply.github.com> Date: Thu, 9 Jan 2025 10:20:59 -1000 Subject: [PATCH 2/3] chore: swap to warn level --- cmd/api/src/api/auth.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/api/src/api/auth.go b/cmd/api/src/api/auth.go index 61bb35e35d..3f30c4afb3 100644 --- a/cmd/api/src/api/auth.go +++ b/cmd/api/src/api/auth.go @@ -362,7 +362,7 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res // Generate commit ID for audit logging if commitID, err = uuid.NewV4(); err != nil { - log.Warnf("Error generating commit ID for login: %s", err) + log.Warnf("[SSO] Error generating commit ID for login: %s", err) RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") return } @@ -378,7 +378,7 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res if user, err = s.db.LookupUser(requestCtx, principalNameOrEmail); err != nil { auditLogFields["error"] = err if !errors.Is(err, database.ErrNotFound) { - log.Errorf("[SSO] Error looking up user: %v", err) + log.Warnf("[SSO] Error looking up user: %v", err) RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } else { RedirectToLoginURL(response, request, "Your user is not allowed, please contact your Administrator") @@ -396,7 +396,7 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res response.Header().Add(headers.Location.String(), locationURL.String()) response.WriteHeader(http.StatusFound) } else { - log.Errorf("[SSO] session creation failure %v", err) + log.Warnf("[SSO] session creation failure %v", err) RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.") } } else { From b9778711ee896db9831bbe0394dfec5a2036fdb9 Mon Sep 17 00:00:00 2001 From: Mistah J <26472282+mistahj67@users.noreply.github.com> Date: Thu, 9 Jan 2025 11:01:05 -1000 Subject: [PATCH 3/3] fix: tests --- cmd/api/src/api/v2/auth/saml_internal_test.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/cmd/api/src/api/v2/auth/saml_internal_test.go b/cmd/api/src/api/v2/auth/saml_internal_test.go index 5c48e299db..3811e4309f 100644 --- a/cmd/api/src/api/v2/auth/saml_internal_test.go +++ b/cmd/api/src/api/v2/auth/saml_internal_test.go @@ -21,6 +21,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "testing" "time" @@ -120,7 +121,10 @@ func TestAuth_CreateSSOSession(t *testing.T) { testAuthenticator.CreateSSOSession(httpRequest, response, principalName, gothamSSO) - require.Equal(t, http.StatusForbidden, response.Code) + require.Equal(t, http.StatusFound, response.Code) + location, err := response.Result().Location() + require.Nil(t, err) + require.Equal(t, location.Query(), url.Values{"error": {"Your user is not allowed, please contact your Administrator"}}) }) t.Run("Forbidden 403 if user isn't associated with a SAML Provider", func(t *testing.T) { @@ -142,7 +146,10 @@ func TestAuth_CreateSSOSession(t *testing.T) { testAuthenticator.CreateSSOSession(httpRequest, response, principalName, gothamSSO) - require.Equal(t, http.StatusForbidden, response.Code) + require.Equal(t, http.StatusFound, response.Code) + location, err := response.Result().Location() + require.Nil(t, err) + require.Equal(t, location.Query(), url.Values{"error": {"Your user is not allowed, please contact your Administrator"}}) }) t.Run("Forbidden 403 if user isn't associated with specified SAML Provider", func(t *testing.T) { @@ -172,7 +179,10 @@ func TestAuth_CreateSSOSession(t *testing.T) { testAuthenticator.CreateSSOSession(httpRequest, response, principalName, gothamSSO) - require.Equal(t, http.StatusForbidden, response.Code) + require.Equal(t, http.StatusFound, response.Code) + location, err := response.Result().Location() + require.Nil(t, err) + require.Equal(t, location.Query(), url.Values{"error": {"Your user is not allowed, please contact your Administrator"}}) }) t.Run("Correctly fails with SAML assertion error if assertion is invalid", func(t *testing.T) {