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: Make IDP cookies same site strict #8716

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Mar 21, 2024

Description

To enhance the security of our application and prevent Cross-Site Request Forgery (CSRF) attacks, we have updated the SameSite attribute of the build in Identity Provider (IDP) cookies to Strict.

This change restricts the browser from sending these cookies with any cross-site requests,
thereby limiting the exposure of the user's session to potential threats.

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

Needs libregraph/lico#131

https://www.authelia.com/configuration/session/introduction/#same_site-1

Related Issue

  • Fixes enterprise issue xxx

Motivation and Context

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value

How Has This Been Tested?

  • local installation

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Wait for upstream merge and get rid of the mod replace
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@fschade
Copy link
Contributor Author

fschade commented Mar 21, 2024

@rhafer not sure if this is bogus.... can we have a look together?
services/idp/pkg/backends/cs3/bootstrap/cs3.go

@fschade fschade self-assigned this Mar 21, 2024
@fschade fschade added Topic:Security Status:Needs-Review Needs review from a maintainer labels Mar 21, 2024
To enhance the security of our application and prevent Cross-Site Request Forgery (CSRF) attacks, we have updated the
SameSite attribute of the build in Identity Provider (IDP) cookies to Strict.
@fschade fschade changed the title enhancement: same site strict cookies enhancement: Make IDP cookies same site strict Mar 23, 2024
@fschade fschade marked this pull request as ready for review March 23, 2024 08:35
Copy link

sonarcloud bot commented Mar 23, 2024

@fschade fschade requested review from dragonchaser and removed request for dragonchaser March 23, 2024 09:30
@fschade fschade removed the Backport label Mar 25, 2024
@fschade
Copy link
Contributor Author

fschade commented Mar 25, 2024

@dj4oC if i understood you correct no backport for that?

@@ -112,6 +113,7 @@ type Settings struct {

CookieBackendURI string
CookieNames []string
CookieSameSite http.SameSite
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should have an env-var & yaml annotation to configure that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think no, since the idp is meant for internal use only... am i right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but on the same site.... im not against it... 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you decide to have it, can u take over and add it? i expect load testing will consume most of my day

Copy link
Member

Choose a reason for hiding this comment

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

wait yes you are right... this does not affect external idp configurations afaik....

Copy link
Member

Choose a reason for hiding this comment

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

unless we provide the ability to authenticate extenal apps via the internal idp this is not needed....

@fschade
Copy link
Contributor Author

fschade commented Mar 25, 2024

@dj4oC if i understood you correct no backport for that?

quote @dj4oC:

FYI backport to 4x is not necessary. If we fix a finding with a later version this is totally fine.

... for later reference

@fschade fschade merged commit 6840de5 into owncloud:master Mar 25, 2024
4 checks passed
ownclouders pushed a commit that referenced this pull request Mar 25, 2024
To enhance the security of our application and prevent Cross-Site Request Forgery (CSRF) attacks, we have updated the
SameSite attribute of the build in Identity Provider (IDP) cookies to Strict.
@dj4oC
Copy link
Contributor

dj4oC commented Mar 25, 2024

Correct

@butonic
Copy link
Member

butonic commented Apr 10, 2024

I'll do a backport for stable 5

butonic pushed a commit that referenced this pull request Apr 10, 2024
To enhance the security of our application and prevent Cross-Site Request Forgery (CSRF) attacks, we have updated the
SameSite attribute of the build in Identity Provider (IDP) cookies to Strict.
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer Topic:Security
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants