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

enhancement: Enhance Security by Allowing Same-Site Cookie Value Modification #131

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Mar 21, 2024

This commit introduces a significant enhancement to the security of our application by allowing the modification of the 'SameSite' attribute of cookies from the consuming application.

The 'SameSite' attribute is a security measure that browsers use to restrict how cookies are sent with cross-site requests.

By default, the Identity Provider (IDP) should be reachable from multiple domains; hence the 'SameSite' attribute is set to 'None'.

This allows cookies to be sent in all requests, irrespective of the site that the requests are being made from.

However, there are scenarios where the IDP should only be reachable from the same domain. In such cases, the 'SameSite' attribute needs to be set to 'Strict'.

This restricts the browser from sending cookies with any cross-site requests, thereby limiting the exposure of the user's session and mitigating the risk of Cross-Site Request Forgery (CSRF) attacks.

By allowing the 'SameSite' attribute to be modifiable, we provide the flexibility to tighten security measures based on the specific requirements and threat models of the consuming application.

This change does not impact existing functionality but provides an additional layer of security where needed.

https://www.authelia.com/configuration/session/introduction/#same_site-1
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie

Copy link
Collaborator

@longsleep longsleep left a comment

Choose a reason for hiding this comment

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

Thanks, can you please explain in the commit message why this is needed / what the use case is / what it means for security.

It is not clear to me in why the lico cookies would need something else than "None" as the intention is explicitly to not block anyone from using the IdP from other domains if they have a registered client or are implicitly allowed.

I assume the idea is to somehow improve the security by allowing to specify cookies as SameSite=Strict?

…fication

This commit introduces a significant enhancement to the security of our application by allowing the modification of the 'SameSite' attribute of cookies from the consuming application.
The 'SameSite' attribute is a security measure that browsers use to restrict how cookies are sent with cross-site requests. By default, the Identity Provider (IDP) should be reachable from multiple domains; hence the 'SameSite' attribute is set to 'None'.
This allows cookies to be sent in all requests, irrespective of the site that the requests are being made from.

However, there are scenarios where the IDP should only be reachable from the same domain. In such cases, the 'SameSite' attribute needs to be set to 'Strict'.

This restricts the browser from sending cookies with any cross-site requests, thereby limiting the exposure of the user's session and mitigating the risk of Cross-Site Request Forgery (CSRF) attacks.
By allowing the 'SameSite' attribute to be modifiable, we provide the flexibility to tighten security measures based on the specific requirements and threat models of the consuming application.
This change does not impact existing functionality but provides an additional layer of security where needed.
@fschade fschade changed the title enhancement: cookie same site setting enhancement: Enhance Security by Allowing Same-Site Cookie Value Modification Mar 22, 2024
@fschade fschade requested a review from longsleep March 22, 2024 09:52
Copy link
Collaborator

@longsleep longsleep left a comment

Choose a reason for hiding this comment

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

Thanks for the details, lgtm 👍

@longsleep longsleep merged commit 72cf922 into libregraph:master Mar 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants