Fix Cookie Attributes on Sticky Session Refresh#534
Fix Cookie Attributes on Sticky Session Refresh#534hoffmaen wants to merge 20 commits intocloudfoundry:developfrom
Conversation
…router when the JSESSIONID and __VCAP_ID__ cookies are set with the Partitioned attribute.
There was a problem hiding this comment.
I have some concerns when it comes to adding a second cookie:
- Yet another cookie which has to be correlated, can be duplicated, etc. Based on your description the meta cookie encodes its own attributes, that would at least allow gorouter to clear it. But if you suddenly have two of those but one regular VCAP cookie it gets weird again.1
- This is a bit more vague but there might be environments in which cookies are tracked to some extent and a new cookie could cause confusion / issues.
I'd prefer if we could evolve the format of the __VCAP_ID__ cookie instead:
- Add a feature flag for
vcap_id_cookie_v2which takes the valuesdisabled(default),accept, andemit. - On
disablednothing changes, gorouter behaves as before. - On
acceptgorouter tries to detect the new format and parse it, falling back to the current format if that fails. - On
emitit starts setting the VCAP_ID cookie according to the new format. - Add
version=2andvalue=$valueto the cookie value.
All names are for illustration purposes only.
Footnotes
-
Yes, I know, you can then just clear both. It still adds complexity. ↩
src/code.cloudfoundry.org/gorouter/proxy/round_tripper/proxy_round_tripper.go
Outdated
Show resolved
Hide resolved
src/code.cloudfoundry.org/gorouter/proxy/round_tripper/proxy_round_tripper.go
Outdated
Show resolved
Hide resolved
src/code.cloudfoundry.org/gorouter/proxy/round_tripper/proxy_round_tripper.go
Outdated
Show resolved
Hide resolved
1d3cfb0 to
191472e
Compare
I appreciate the idea, but I see a few drawbacks here:
|
| logger *slog.Logger, | ||
| ) { | ||
| sessionCookie, vcapCookie := getSessionCookiesFromResponse(response, stickySessionCookieNames) | ||
| if vcapCookie != nil { |
There was a problem hiding this comment.
A comment to explain the early return like "Sticky Session already established previously, nothing more to do" would be nice.
| } | ||
|
|
||
| // stickySessionCase identifies which of the three mutually exclusive sticky-session paths applies to the current request/response pair. | ||
| type stickySessionCase int |
There was a problem hiding this comment.
How about
| type stickySessionCase int | |
| type stickySessionScenario int |
Also: With the new simplified structure I'm not sure if we need it at all...
| // stickySessionAuthNegotiate: first request of a Kerberos/SPNEGO handshake; fixed cookie defaults apply. | ||
| stickySessionAuthNegotiate stickySessionCase = iota | ||
| // sessionCookieInAppResponse: application set a JSESSIONID in the response; attributes are copied from it. | ||
| sessionCookieInAppResponse |
There was a problem hiding this comment.
Maybe this (or sth similar) makes the scenario more clear?
| sessionCookieInAppResponse | |
| newStickySessionRequested |
| vcapCookie.MaxAge = sessionCookie.MaxAge | ||
| vcapCookie.Expires = sessionCookie.Expires | ||
| createMetaCookie = true | ||
| default: // case stickySessionStaleInstance |
There was a problem hiding this comment.
I know why we did this (to avoid the "stickySessionStaleInstance is always true" thing right?).
But a bit more information for others might be helpful.
| sameSite := http.SameSite(0) | ||
| expiry := time.Time{} | ||
| partitioned := false | ||
| // Create sticky session with default cookie attributes |
There was a problem hiding this comment.
| // Create sticky session with default cookie attributes | |
| // Create VCAP_ID cookie with default cookie attributes |
| expiry = v.Expires | ||
| partitioned = v.Partitioned | ||
| addCookieToResponse(response, vcapCookie) | ||
| if createMetaCookie { |
There was a problem hiding this comment.
Hey, since we're only setting this in the case sessionCookieInAppResponse, why not set it there directly and save ourselves the var createMetaCookie?
Edit: Because we would have to specify if secureCookies {vcapCookie.Secure = true} twice
Edit2: But we could just doo ....Secure = vcapCookie.Secure || secureCookies
There was a problem hiding this comment.
Setting attributes in one place, and adding cookies in another keeps the code structure clean. I think this is a legitimate reason to keep the few extra lines.
|
|
||
| break | ||
| // getSessionCookiesFromResponse returns the session cookie and __VCAP_ID__ cookie, when they exist in the response | ||
| func getSessionCookiesFromResponse(response *http.Response, stickySessionCookieNames config.StringSet) (sessionCookie *http.Cookie , vcapIdCookie *http.Cookie) { |
There was a problem hiding this comment.
| func getSessionCookiesFromResponse(response *http.Response, stickySessionCookieNames config.StringSet) (sessionCookie *http.Cookie , vcapIdCookie *http.Cookie) { | |
| func getSessionCookies(response *http.Response, stickySessionCookieNames config.StringSet) (sessionCookie *http.Cookie , vcapIdCookie *http.Cookie) { |
| if secureCookies { | ||
| secure = true | ||
| // parseVcapMeta deserializes the __VCAP_ID_META__ cookie value. | ||
| func parseVcapMeta(value string) (*vcapAttributes, error) { |
There was a problem hiding this comment.
Could we use some other kind of serialization that doesn't involve manual rules?
Option A: Intermediate JSON struct — keep vcapAttributes as-is, marshal/unmarshal via a separate vcapAttributesJSON with string fields and JSON tags. Simple, no custom marshaler needed.
Option B: JSON tags + custom marshaler on vcapAttributes — add MarshalJSON/UnmarshalJSON to vcapAttributes. More self-contained but more boilerplate.
Would probably be less performant though
binary serialization (e.g. encoding/gob, protobuf, msgpack) would be faster and more compact than JSON, but you'd still be slower than the current url.Values approach for a payload this small.
There was a problem hiding this comment.
As discussed: We'll leave it as is, but will consider adding valueless params for booleans (e.g. if params.Get("secure") { metaAttributesFromCookie.secure = true }
| for _, v := range response.Cookies() { | ||
| if _, ok := stickySessionCookieNames[v.Name]; ok { | ||
| shouldSetVCAPID = true | ||
| if secureCookies { |
There was a problem hiding this comment.
Comment: point to global config value and that it overrides the per-cookie prefernce
| return vcapMetaAttributes | ||
| } | ||
| } | ||
| logger.Info("vcap-id-meta-cookie-not-found") |
There was a problem hiding this comment.
Add comment that we expect to see these logs during the migration to the new cookie handling, but not after?
774d7a0 to
f98f2af
Compare
Summary
When an application sets a partitioned
JSESSIONIDcookie, Gorouter sets a matching partitioned__VCAP_ID__cookie. However, when a subsequent request references a stale application instance and the new instance does not respond with a newJSESSIONIDcookie, Gorouter had no way of knowing whether the client's existing__VCAP_ID__was partitioned — since cookie attributes are never transmitted from the client to the server. As a result, Gorouter always created a new unpartitioned__VCAP_ID__cookie in this case, leaving the client with both an orphaned partitioned and a new unpartitioned__VCAP_ID__cookie. This broke sticky session routing for partitioned cookies, as browsers such as Chrome would prefer the stale partitioned cookie over the new unpartitioned one.This Pull Request introduces a new metadata cookie,
__VCAP_ID_META__, which Gorouter sets alongside__VCAP_ID__whenever it creates a sticky session (i.e. the application responds with a matching session cookie such asJSESSIONID). The metadata cookie encodes all relevant__VCAP_ID__attributes —Partitioned,Secure,SameSite, andExpiry— as a compact&-separatedkey=valuestring (e.g.expiry=1748001600&secure=1&samesite=none&partitioned=1). In the stale instance scenario, when the new instance does not set a newJSESSIONIDcookie, Gorouter reads__VCAP_ID_META__from the incoming request and reconstructs the refreshed__VCAP_ID__with the original attributes preserved.The expiry lifetime is stored as an absolute Unix timestamp rather than a relative Max-Age value. This ensures that a stale instance refresh does not silently extend the user's session: the refreshed
__VCAP_ID__receives an Expires header set to the original absolute point in time, so the remaining session lifetime is preserved exactly.__VCAP_ID_META__is issued with the same attributes (Secure,SameSite,Partitioned,Expiry) as__VCAP_ID__itself, so the two cookies co-expire and share the same security constraints.Closes #529 (Problem 1).
Backward Compatibility
Breaking Change? No
All clients that use sticky sessions will now additionally receive the
__VCAP_ID_META__cookie alongside__VCAP_ID__. This is a net improvement: without it, sticky session routing for partitioned cookies in the stale instance scenario was already broken, and attributes such as Secure and SameSite were not preserved on refresh for any client. Clients that do not use sticky sessions are entirely unaffected.