Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BED-5311 fix: incorrect oidc cookie consumption #1083

Merged
merged 1 commit into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading