Skip to content

Commit

Permalink
fix: incorrect oidc cookie consumption
Browse files Browse the repository at this point in the history
  • Loading branch information
mistahj67 committed Jan 16, 2025
1 parent 3765722 commit b5534ea
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 42 deletions.
32 changes: 14 additions & 18 deletions cmd/api/src/api/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,36 +296,34 @@ func (s authenticator) ValidateRequestSignature(tokenID uuid.UUID, request *http
}

func DeleteBrowserCookie(request *http.Request, response http.ResponseWriter, name string) {
SetSecureBrowserCookie(request, response, name, "", time.Now().UTC(), false)
SetSecureBrowserCookie(request, response, name, "", time.Now().UTC(), false, 0)
}

func SetSecureBrowserCookie(request *http.Request, response http.ResponseWriter, name, value string, expires time.Time, httpOnly bool) {
func SetSecureBrowserCookie(request *http.Request, response http.ResponseWriter, name, value string, expires time.Time, httpOnly bool, sameSite http.SameSite) {
var (
hostURL = *ctx.FromRequest(request).Host
sameSiteValue = http.SameSiteDefaultMode
domainValue string
hostURL = *ctx.FromRequest(request).Host
isHttps = hostURL.Scheme == "https"
)

if hostURL.Scheme == "https" {
sameSiteValue = http.SameSiteStrictMode
// If sameSite is not explicitly set, we want to rely on the host scheme
if sameSite == 0 && isHttps {
sameSite = http.SameSiteStrictMode
}

// NOTE: Set-Cookie should generally have the Domain field blank to ensure the cookie is only included with requests against the host, excluding subdomains; however,
// most browsers will ignore Set-Cookie headers from localhost responses if the Domain field is not set explicitly.
if strings.Contains(hostURL.Hostname(), "localhost") {
domainValue = hostURL.Hostname()
// NOTE: Browsers will not set localhost cookies with sameSite set to None. This is a local network SSO workaround
if strings.Contains(hostURL.Hostname(), "localhost") && sameSite == http.SameSiteNoneMode {
sameSite = http.SameSiteDefaultMode
}

// Set the token cookie
http.SetCookie(response, &http.Cookie{
Name: name,
Value: value,
Expires: expires,
Secure: hostURL.Scheme == "https",
Secure: isHttps,
HttpOnly: httpOnly,
SameSite: sameSiteValue,
SameSite: sameSite,
Path: "/",
Domain: domainValue,
})
}

Expand Down Expand Up @@ -356,8 +354,6 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res
auditLogFields["oidc_provider_id"] = ssoProvider.OIDCProvider.ID
authProvider = *ssoProvider.OIDCProvider
}
DeleteBrowserCookie(request, response, AuthStateCookieName)
DeleteBrowserCookie(request, response, AuthPKCECookieName)
}

// Generate commit ID for audit logging
Expand Down Expand Up @@ -404,8 +400,8 @@ func (s authenticator) CreateSSOSession(request *http.Request, response http.Res

locationURL := URLJoinPath(hostURL, UserInterfacePath)

// Set the token cookie
SetSecureBrowserCookie(request, response, AuthTokenCookieName, sessionJWT, time.Now().UTC().Add(s.cfg.AuthSessionTTL()), false)
// Set the token cookie, httpOnly must be false for the UI to pick up and store token
SetSecureBrowserCookie(request, response, AuthTokenCookieName, sessionJWT, time.Now().UTC().Add(s.cfg.AuthSessionTTL()), false, 0)

// Redirect back to the UI landing page
response.Header().Add(headers.Location.String(), locationURL.String())
Expand Down
6 changes: 4 additions & 2 deletions cmd/api/src/api/constant.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,16 @@ const (
AuthorizationSchemeBHESignature = "bhesignature"
AuthorizationSchemeBearer = "bearer"

// Form parameters
FormParameterState = "state"
FormParameterCode = "code"

// Query parameters
QueryParameterSortBy = "sort_by"
QueryParameterHydrateCounts = "counts"
QueryParameterHydrateDomains = "hydrate_domains"
QueryParameterHydrateOUs = "hydrate_ous"
QueryParameterScope = "scope"
QueryParameterState = "state"
QueryParameterCode = "code"

// URI path parameters
URIPathVariableApplicationConfigurationParameter = "parameter"
Expand Down
41 changes: 19 additions & 22 deletions cmd/api/src/api/v2/auth/oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,54 +183,51 @@ func (s ManagementResource) OIDCLoginHandler(response http.ResponseWriter, reque
verifier := oauth2.GenerateVerifier()

// Store PKCE on web browser in secure cookie for retrieval in callback
api.SetSecureBrowserCookie(request, response, api.AuthPKCECookieName, verifier, time.Now().UTC().Add(time.Minute*7), true)
api.SetSecureBrowserCookie(request, response, api.AuthPKCECookieName, verifier, time.Now().UTC().Add(time.Minute*7), true, http.SameSiteNoneMode)

// Store State on web browser in secure cookie for retrieval in callback
api.SetSecureBrowserCookie(request, response, api.AuthStateCookieName, state, time.Now().UTC().Add(time.Minute*7), true)

// Redirect user to consent page to ask for permission for the scopes specified above.
redirectURL := conf.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.S256ChallengeOption(verifier))
api.SetSecureBrowserCookie(request, response, api.AuthStateCookieName, state, time.Now().UTC().Add(time.Minute*7), true, http.SameSiteNoneMode)

// Redirect user to consent page to ask for permission for the scopes specified above and specify POST callback
redirectURL := conf.AuthCodeURL(state, oauth2.AccessTypeOffline, oauth2.S256ChallengeOption(verifier), oauth2.SetAuthURLParam("response_mode", "form_post"))
response.Header().Add(headers.Location.String(), redirectURL)
response.WriteHeader(http.StatusFound)
}
}

func (s ManagementResource) OIDCCallbackHandler(response http.ResponseWriter, request *http.Request, ssoProvider model.SSOProvider) {
var (
queryParams = request.URL.Query()
state = queryParams[api.QueryParameterState]
code = queryParams[api.QueryParameterCode]
state = request.FormValue(api.FormParameterState)
code = request.FormValue(api.FormParameterCode)
)

// No matter what happens, wipe the auth cookies
api.DeleteBrowserCookie(request, response, api.AuthStateCookieName)
api.DeleteBrowserCookie(request, response, api.AuthPKCECookieName)

if ssoProvider.OIDCProvider == nil {
// SSO misconfiguration scenario
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)
queryParams.Del(api.QueryParameterState)
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
} else if code == "" {
log.Warnf("[OIDC] auth code is missing")
api.RedirectToLoginURL(response, request, "We’re having trouble connecting. Please check your internet and try again.")
} else if state == "" {
log.Warnf("[OIDC] state parameter is missing")
// Missing state parameter - treat as technical issue
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
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
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)
} else if stateCookie, err := request.Cookie(api.AuthStateCookieName); err != nil || stateCookie.Value != state {
log.Warnf(fmt.Sprintf("[OIDC] state cookie does not match %v", err))
// Invalid state - treat as technical issue or misconfiguration
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
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 {
} else if claims, err := getOIDCClaims(request.Context(), provider, ssoProvider, pkceVerifier, code); err != nil {
log.Warnf("[OIDC] %v", err)
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
Expand Down

0 comments on commit b5534ea

Please sign in to comment.