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

net/http: support partitioned cookies #62490

Open
islishude opened this issue Sep 7, 2023 · 30 comments · May be fixed by #62499
Open

net/http: support partitioned cookies #62490

islishude opened this issue Sep 7, 2023 · 30 comments · May be fixed by #62499

Comments

@islishude
Copy link

islishude commented Sep 7, 2023

Since Chrome drops support for SameSite=None cookies, aka third-party cookies, net/http package needs to add Partitioned field to the type Cookie, and add a deprecated message for SameSiteNoneMode

ref:

https://developer.chrome.com/docs/privacy-sandbox/third-party-cookie-phase-out/

@gopherbot gopherbot added this to the Proposal milestone Sep 7, 2023
@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

It would help a lot if you could say more about exactly what should change in the Go package. Thanks.

@islishude islishude changed the title proposal: net/http: deprecated SameSite=None cookies and add partitioned cookies proposal: net/http: deprecated third-party cookies and add partitioned cookies Sep 7, 2023
@islishude
Copy link
Author

Sure, updated.

islishude added a commit to islishude/go that referenced this issue Sep 7, 2023
@islishude islishude linked a pull request Sep 7, 2023 that will close this issue
@gopherbot
Copy link

Change https://go.dev/cl/526435 mentions this issue: net/http: add paritioned attribute to cookie type

@neild
Copy link
Contributor

neild commented Sep 7, 2023

We don't want a deprecation notice on SameSiteNoneMode. Browsers may not support it, but the net/http package should support sending SameSite=None if the user wants to.

Also, partitioned cookies use SameSite=None, so browsers haven't actually deprecated this value.

Adding a Partitioned field to http.Cookie seems reasonable:

type Cookie { // contains existing fields
  Partitioned bool
}

islishude added a commit to islishude/go that referenced this issue Sep 8, 2023
Fixes golang#62490

net/http: fix test case for TestCookieValid

net/http: add missing continue for readSetCookies
@islishude
Copy link
Author

@neild I'm agree with you. my pr just adds a note for the SameSiteNoneMode.

@seankhliao seankhliao changed the title proposal: net/http: deprecated third-party cookies and add partitioned cookies proposal: net/http: support partitioned cookies Sep 10, 2023
@iamdlfl
Copy link

iamdlfl commented Oct 31, 2023

Any updates on this proposal? It would be useful for something I'm trying to work on.

@gregwebs
Copy link

gregwebs commented Nov 3, 2023

What more needs to be done to have this proposal accepted? The associated commit explains the changes clearly.

@nightlyone
Copy link
Contributor

Supporting the partitioned cookie attribute without actually implementing the cookie jar changes implementing the semantics of the partitioning this attribute signals sounds at least incomplete or even dangerous from a security perspective to me.

So a more complete proposal should outline the full semantics and amount of changes to implement the support. Not only the encoding and decoding, but also creating the key to address the cookie jar elements and storing as well as accessing them in http.Client.Jar to reap all the benefits of such cookies.

@islishude
Copy link
Author

but also creating the key to address the cookie jar elements and storing as well as accessing them in http.Client.Jar to reap all the benefits of such cookies.

I can't understand why it's related to http.Client.Jar.

the cookies could be parsed by http.readSetCookies function

@gregwebs
Copy link

This proposal implements server-side support. Client-side support is intended for browsers to block cross-site tracking. It's unclear if this use case would be meaningful for a Go HTTP client. If someone made an end user web browser in Go there is a lot they would have to add on to the existing http client such as this and CORS.

@islishude
Copy link
Author

I think it's not feasible to add it to http.Client.Jar.

image

In the example above where https://support.chat.example is embedded on https://retail.example, the top-level URL is https://retail.example. https://developer.chrome.com/docs/privacy-sandbox/chips/

there should no scenes like above in the go http client.

@maxmoehl
Copy link

Is there a way to prioritise this proposal since it is time critical?

We are already receiving reports on broken websites for our router. In our scenario we read a cookie from the response provided by the backend to set an additional cookie for sticky sessions (to route further requests to the same backend). If we now rely on the Cookie.Unparsed slice to contain the Partitioned attribute we are at risk of breaking if the field is parsed in the future (and therefore removed from that slice).

@gregwebs
Copy link

@maxmoehl it seems like that is already the situation someone is in if they want to support older Go versions. A function could be made that would look for it in Unparsed and then in the future would use the attribute. It's not easy to write code to check for an attribute though- that requires reflection rather than just an interface check. So it would help with compatibility to provide getter and setter functions for new attributes like this. The getter and setter could be marked as deprecated in a future release.

@geofffranks
Copy link

@ianlancetaylor @neild - Looks like the PR for this is on hold until this proposal is accepted. However it's unclear to me what if anything is required to get this proposal accepted. Can you elaborate the next steps needed to move this along?

Thanks!

@aeneasr
Copy link

aeneasr commented Feb 29, 2024

This is becoming more and more urgent as 3P cookies are being phased out by all major vendors including Google, and CHIPS is a cross-platform solution solving this issue:

@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

Is the proposal simply to add a field as in #62490 (comment)?
When is it used or set?

@iamdlfl
Copy link

iamdlfl commented Mar 1, 2024

@rsc I'm not the original author but from what I can tell, yes. It also includes changes to relevant functions (String(), Valid(), etc.) https://go.dev/cl/526435

I would use this in an LMS plugin my team manages. Our tool is displayed in an iframe in the LMS and so needs to use Partitioned to set cookies.

@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@aeneasr
Copy link

aeneasr commented Mar 4, 2024

Thank you @rsc! It is indeed only a boolean flag on a cookie which omits / includes this tag in the Set-Cookie directive

@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

Have all remaining concerns about this proposal been addressed?

The proposal is to add a new Partitioned bool field to the Cookie struct, set and used as follows:

In the Cookie parser, if the cookie says “; Partitioned”, the bool is set to true.

In Cookie.String, if Partitioned is true, the string adds “; Partitioned”.

In Cookie.Valid, if Partitioned is true and the cookie is not secure or the path is not /, Valid returns an error.

@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add a new Partitioned bool field to the Cookie struct, set and used as follows:

In the Cookie parser, if the cookie says “; Partitioned”, the bool is set to true.

In Cookie.String, if Partitioned is true, the string adds “; Partitioned”.

In Cookie.Valid, if Partitioned is true and the cookie is not secure or the path is not /, Valid returns an error.

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

We did not end up deciding what needs to happen in net/http/cookiejar or http.CookieJar interface.

At the least, net/http/cookiejar needs to be updated to preserve the Partitioned bool when given cookies and returning them, right?

It is unclear to me whether the http.CookieJar interface also needs semantic changes. Does it?

@rsc
Copy link
Contributor

rsc commented Mar 27, 2024

Another option, perhaps the right one, would be to add Partition to cookie for use on server side, and then defer what to do on client side for another day. Are we okay with only addressing the server side? What are the consequences of adding Partition on the server side but not the client side?

@neild
Copy link
Contributor

neild commented Mar 27, 2024

Currently, net/http/cookiejar.Jar ignores the Partitioned field entirely. If we add a Partitioned bool to Cookie and do nothing else, cookiejar.Jar will continue to ignore the field. This seems like a strict improvement in the state of the world: Users can now write CookieJar implementations that make use of the Partitioned attribute, and cookiejar.Jar is no worse off than it is now. Since this just adds an attribute, it also seems very safe.

We could also add support to cookiejar.Jar for CHIPS, making it aware of the Partitioned attribute. I haven't attempted to implement this and perhaps there are surprises I'm not aware of, but this seems fairly straightforward. This will be a behavioral change, and so conceivably could cause a regression for some users, but since the only purpose of setting the Partitioned attribute is to opt-in to CHIPS/cookie partitioning, this seems fairly safe as well.

We should add cookie.Partitioned. I see no downsides to this.

I think we should also add CHIPS support to cookiejar. The risks are small (it seems unlikely that anyone is relying on partitioned cookies not being partitioned), and it would be strange to support the Partitioned attribute but not honor it in our implementation.

@ianlancetaylor
Copy link
Contributor

Can we add CHIPS support to net/http/cookiejar without changing the http.CookieJar interface? How do we tell it which partition to use?

@gregwebs
Copy link

Yes, defer the client cookie jar. This proposal implements server-side support. Client-side support is intended for browsers to block cross-site tracking, which is not a normal use case for a Go client. Let's stay focused on just delivering the server side for this proposal since we know how to do that without breaking anything and there are a lot of users of this server side.

A new issue can be opened for the client CookieJar.

@maxmoehl
Copy link

What are the consequences of adding Partition on the server side but not the client side?

I can't think of any that would have an impact. The client would not send the cookie from the embedded page if the parent page isn't the same as it was when the cookie was set (see: CHIPS). But the entire concept of embedded pages is missing from the go http client.

The only thing a client could do is preserve the attribute when sending back the cookie, but that shouldn't have any real impact as the attribute is intended for the client to act upon, not the server.

@neild
Copy link
Contributor

neild commented Mar 27, 2024

Can we add CHIPS support to net/http/cookiejar without changing the http.CookieJar interface? How do we tell it which partition to use?

I believe we can add CHIPS support without changing the interface. The partition is "the site (scheme and registrable domain) of the top-level URL the browser was visiting at the start of the request to the endpoint that set the cookie" (https://developers.google.com/privacy-sandbox/3pcd/chips#cookie_partitioning_technical_design), and CookieJar.SetCookies takes the request URL as a parameter:

SetCookies(u *url.URL, cookies []*Cookie)

Perhaps I'm missing something, though.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2024

It sounds like the "likely accept" version above is still okay. Thanks for the clarifications.

@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add a new Partitioned bool field to the Cookie struct, set and used as follows:

In the Cookie parser, if the cookie says “; Partitioned”, the bool is set to true.

In Cookie.String, if Partitioned is true, the string adds “; Partitioned”.

In Cookie.Valid, if Partitioned is true and the cookie is not secure or the path is not /, Valid returns an error.

@rsc rsc changed the title proposal: net/http: support partitioned cookies net/http: support partitioned cookies Apr 4, 2024
@rsc rsc modified the milestones: Proposal, Backlog Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.