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

allow SameSite cookie attribute to be configured #11210

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Feb 3, 2025

What this PR does / why we need it:

This PR sets the SameSite cookie attribute to "Lax", which is more secure than "None" and documents how to change it to "Strict".

Which issue(s) this PR closes:

Special notes for your reviewer:

None.

Suggestions on how to test this:

Confirm the former and new attributes for the JSESSIONID cookie's attributes.

Follow the docs and try to change the SameSite value. Changing the SameSite value to "Strict" for example is more easily done in a classic environment than in Docker because "Lax" is baked into the base image. It's possible to change the value and rebuild the base image. The file to edit is modules/container-base/src/main/docker/Dockerfile. Then, to rebuild, you follow https://guides.dataverse.org/en/6.5/container/base-image.html#build-instructions

Here's "before" and "after":

Before: SameSite attribute is absent (Dataverse 6.5 and earlier)

% curl -s -I http://localhost:8080 | grep JSESSIONID
Set-Cookie: JSESSIONID=6574324d75aebeb86dc96ecb3bb0; Path=/

After: SameSite attribute is present (twice) (this pull request)

Note: we suspect that the repeated SameSite attribute is a bug in Payara. We should retest after we've upgraded Payara in #11128 before reporting it upstream.

% curl -s -I http://localhost:8080 | grep JSESSIONID
Set-Cookie: JSESSIONID=6574324d75aebeb86dc96ecb3bb0; Path=/;SameSite=Lax;SameSite=Lax

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No.

Is there a release notes update needed for this change?:

Yes, included.

Additional documentation:

Preview docs at https://dataverse-guide--11210.org.readthedocs.build/en/11210/installation/config.html#samesite-cookie-attribute

@pdurbin pdurbin added Size: 10 A percentage of a sprint. 7 hours. FY25 Sprint 16 FY25 Sprint 16 (2025-01-29 - 2025-02-12) labels Feb 3, 2025

See :ref:`samesite-cookie-attribute` for context.

The Dataverse installer configures the Payara **server-config.network-config.protocols.protocol.http-listener-1.http.cookie-same-site-value** setting to "Lax". From `Payara's documentation <https://docs.payara.fish/community/docs/6.2024.6/Technical%20Documentation/Payara%20Server%20Documentation/General%20Administration/Administering%20HTTP%20Connectivity.html>`_, the other possible values are "Strict" or "None". To change this to "Strict", for example, you could run the following command...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value for this appears to do nothing if the http.cookie-same-site-enabled isn't set to true.

Conversely, setting http.cookie-same-site-enabled=true without setting this one ends up sending None as the default which is worse than not having a SameSite setting at all. I think the docs should warn people about these issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't end up documenting this (or testing it) but I'm happy to. Let me know.

Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bin/asadmin work fine on an existing installation. I did suggest some doc changes to clarify that installations are not less secure now that our recommendation of going to Lax, and to stress that using None on purpose, or accidentally increases risks.

Also - not sure why the build failed - from the log, it doesn't look related to the PR to me.

@qqmyers
Copy link
Member

qqmyers commented Feb 6, 2025

Another note - I think Strict breaks remote login - at least ORCID which is what I tested. Lax works as expected.

@pdurbin pdurbin assigned qqmyers and unassigned pdurbin Feb 11, 2025
@@ -281,6 +281,18 @@ provides. These are mostly based on environment variables (very common with cont
- See :ref:`:ApplicationServerSettings` ``http.request-timeout-seconds``.

*Note:* can also be set using any other `MicroProfile Config Sources`_ available via ``dataverse.http.timeout``.
* - ``DATAVERSE_COOKIE_SAME_SITE_VALUE``
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I try to change DATAVERSE_COOKIE_SAME_SITE_VALUE to "Strict" in the compose file, it doesn't seem to have an effect. I still see "Lax" when I check the cookie with curl.

I opened a thread in Zulip about this: https://dataverse.zulipchat.com/#narrow/channel/375812-containers/topic/DATAVERSE_HTTP_TIMEOUT.20not.20having.20an.20effect/near/499094581

I'm not sure why it isn't working. 🤷 I'm hoping @poikilotherm will save me! ❤️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you also set DATAVERSE_COOKIE_SAME_SITE_ENABLED?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's already set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Through the Dockerfile, I mean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold on, I need to add something to the Dockerfile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bah. It doesn't work. As discussed at standup, I switched Docker to be hard coded to Lax and updated the docs to say you have to rebuild the base image if you want Strict or whatever. See d62ac2f

Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Odd that SameSite is repeated in curl. FWIW: if I look at the first call to Dataverse in the Chrome or Edge console, the Set-Cookie entry in the response header only shows one copy (even selecting 'raw' for the response headers). Perhaps they are de-duplicating even for raw?

@pdurbin pdurbin assigned pdurbin and unassigned qqmyers Feb 12, 2025
Also explain that we'd like it to be configurable through MPCONFIG
but it doesn't seem to be working.
@pdurbin pdurbin removed their assignment Feb 12, 2025
@cmbz cmbz added the FY25 Sprint 17 FY25 Sprint 17 (2025-02-12 - 2025-02-26) label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 16 FY25 Sprint 16 (2025-01-29 - 2025-02-12) FY25 Sprint 17 FY25 Sprint 17 (2025-02-12 - 2025-02-26) Size: 10 A percentage of a sprint. 7 hours.
Projects
Status: Ready for QA ⏩
Development

Successfully merging this pull request may close these issues.

3 participants