Closed Jusshersmith closed 2 years ago
Merging #316 (f92247f) into main (a1b1b74) will increase coverage by
0.82%
. The diff coverage is92.10%
.
@@ Coverage Diff @@
## main #316 +/- ##
==========================================
+ Coverage 62.73% 63.56% +0.82%
==========================================
Files 58 58
Lines 4286 4825 +539
==========================================
+ Hits 2689 3067 +378
- Misses 1382 1544 +162
+ Partials 215 214 -1
Impacted Files | Coverage Δ | |
---|---|---|
internal/pkg/sessions/session_state.go | 85.00% <ø> (+0.38%) |
:arrow_up: |
internal/proxy/configuration.go | 90.90% <ø> (+1.10%) |
:arrow_up: |
internal/proxy/oauthproxy.go | 55.85% <0.00%> (+1.58%) |
:arrow_up: |
internal/proxy/proxy.go | 16.98% <0.00%> (+0.98%) |
:arrow_up: |
internal/proxy/proxy_config.go | 78.50% <78.57%> (+2.28%) |
:arrow_up: |
internal/pkg/sessions/cookie_store.go | 85.71% <91.66%> (+0.25%) |
:arrow_up: |
internal/auth/authenticator.go | 86.89% <100.00%> (+1.15%) |
:arrow_up: |
internal/auth/configuration.go | 51.54% <100.00%> (+7.85%) |
:arrow_up: |
internal/auth/mux.go | 80.35% <100.00%> (+5.35%) |
:arrow_up: |
internal/auth/options.go | 88.03% <100.00%> (+2.92%) |
:arrow_up: |
... and 57 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update a1b1b74...f92247f. Read the comment docs.
Although in certain situations having this flexibility may be useful, we've opted, at least for now, not to ship this.
A change like this has potentially wide yet obscure impacts on the security posture of this application, so we would prefer to avoid this route unless absolutely required.
Problem
The SameSite Cookie attribute allows you to determine whether a cookie is able to be used in a cross-site context, or if it's restricted to a first-party context.
SameSite=None
, allowing cookies to be used in a cross-site context.SameSite=Lax
, restricting cookies to a first-party or same-site context.This has no impact on typical workflows, however is a requirement for others.
Solution
~Add a
session_cookie_samesite
configuration variable ("none"
,"lax"
, or"strict"
) to bothsso_proxy
andsso_auth
, which is then stored in the session cookie store and set on any created cookies.~The above solution stored the
SameSite
variable in the cookie store and set the value on all created cookies. The new solution (below) allowssso_proxy
to read the setting from upstream configurations, instead of globally, but it does also cause some less-appealing workflows (like having to pass it explicitly in calls toSetCSRF
, for example).sso_auth
session_cookie_samesite
global configuration variable ("none"
,"lax"
, or"strict"
), which is stored in theAuthenticator
object, and subsequently, the session itself.sso_proxy
cookie_samesite
upstream configuration variable ("none"
,"lax"
, or"strict"
), which is then stored in theOAuthProxy
object, and subsequently, the session itself.session pkg
SameSite
has been added to the session state, which is then referenced in chained calls to set the session cookie.SetCSRF
always setshttp.SameSiteDefaultMode
.ClearSession
, andClearCSRF
both sethttp.SameSiteDefaultMode
-- I don't think (I might be wrong!) this has much of an effect, but this felt like the safest setting?)Notes
SameSiteDefaultMode
is used. (https://golang.org/src/net/http/cookie.go)SameSite
is set toNone
, thenSecure
must be enabled.