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

Implement secure cookies by default #2107

Merged

Conversation

kdp-cloud
Copy link
Collaborator

  • Use for secure cookies in a production environment
  • Force SSL in production environment

@kdp-cloud kdp-cloud added this to the 1.16.1 milestone Jan 8, 2025
@kdp-cloud kdp-cloud self-assigned this Jan 8, 2025
Copy link
Member

@stuzart stuzart left a comment

Choose a reason for hiding this comment

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

I'm not clear how this will affect running with docker and an nginx / apache front end forwarder. Will need some testing, possible ngnix configuration changes, and maybe an ENV variable to override.
For these reasons I think it might be better left for the main branch and next major version

@kdp-cloud kdp-cloud changed the base branch from seek-1.16 to main January 13, 2025 17:00
@kdp-cloud kdp-cloud modified the milestones: 1.16.1, 1.17.0 Jan 13, 2025
@kdp-cloud
Copy link
Collaborator Author

I'm not clear how this will affect running with docker and an nginx / apache front end forwarder. Will need some testing, possible ngnix configuration changes, and maybe an ENV variable to override. For these reasons I think it might be better left for the main branch and next major version

I changed the branch to main and the milestone to 1.17, but I'm not sure you should override this config in production since this implicates security issues.

@kdp-cloud kdp-cloud requested a review from stuzart January 13, 2025 17:33
@stuzart
Copy link
Member

stuzart commented Jan 15, 2025

I'm not clear how this will affect running with docker and an nginx / apache front end forwarder. Will need some testing, possible ngnix configuration changes, and maybe an ENV variable to override. For these reasons I think it might be better left for the main branch and next major version

I changed the branch to main and the milestone to 1.17, but I'm not sure you should override this config in production since this implicates security issues.

The docker containers always run in production, but are often used through http, e.g for testing, trying out seek, local installation behind a firewall or reverse proxy or load balancer.

I had a good look into this yesterday and some testing, and all is needed is the config.force_ssl, as this also sets the secure cookie https://api.rubyonrails.org/v7.2.2.1/classes/ActionDispatch/SSL.html, which also sets the HSTS (which I found afterwards blocked me from localhost:3000 and was a pain to remove, ending up needing to go to chrome://net-internals/#hsts and I'd be curious how this affects the http->https redirect as it being blocked by my browser )

You may also configure secure cookies separately through the apache or nginx config, which is what we do for https://fairdomhub.org - which may be preferable as it separates the application logic from deployment.

My conclusion is that it would be best to allow force_ssl to be turned on with an environment variable, but off by default, but update our docs and docker-compose file to recommend turning it on if deploying a production instance on the web, if the front end proxy hasn't already been configured instead. We can discuss this more in the meeting today.

Revert "Use secure cookies in production"

This reverts commit 3c718f8.

Revert "Make secure cookie implementation dependent on a environmental
variable"

This reverts commit 6913e51.
SameSite directive is set to lax. This is necessarry for exernal auth
@kdp-cloud
Copy link
Collaborator Author

As discussed:

  • HSTS directive will be set on proxy level
  • Secure cookies will be set on proxy level
  • Enabling HSTS and secure cookies will described in the documentation (PR)
  • The SameSite directive is defined application-side:
    • The session cookie is set to 'Lax' to permit external authentication like OAUTH2
    • Other cookies are set to 'Strict'

@kdp-cloud kdp-cloud merged commit 974a5b4 into seek4science:main Jan 24, 2025
10 checks passed
@kdp-cloud kdp-cloud deleted the implement-secure-cookies-by-default branch January 24, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Coded
Development

Successfully merging this pull request may close these issues.

2 participants