Skip to content

Make Gitpod cookie "stricter" #16406

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

Closed
wants to merge 2 commits into from
Closed

Make Gitpod cookie "stricter" #16406

wants to merge 2 commits into from

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Feb 15, 2023

Description

This PR changes the way we're managing sameSite setting of the Gitpod cookie (aka session cookie).

We know, having them on lax mode in general comes with a reduction of browser based security mechanism. OTOH the current OAuth2 implementation relies on the session cookie being present on redirects. Also, during the OAuth2 flow, the mentioned browser security model might be neglected, at least there would be other measures possible to harden that process not relying on the browser agent. Given that, we can improve the security posture, by switching to lax mode for OAuth2 processes and back to strict mode when the process is done.

Related Issue(s)

Fixes #

How to test

  1. Try to login with GitHub and GitLab.
  2. Repeat after signing out of GitHub and GitLab to simulate a cold start of IdP session, which should lead to

Screenshot 2023-02-15 at 10 07 13

Release Notes

NONE

Documentation

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • leeway-no-cache
    leeway-target=components:all
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-slow-database
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-at-test-strict.1 because the annotations in the pull request description changed
(with .werft/ from main)

@AlexTugarev AlexTugarev changed the title At/test-strict Make Gitpod cookie "stricter" Feb 15, 2023
@AlexTugarev AlexTugarev marked this pull request as ready for review February 15, 2023 09:20
@AlexTugarev AlexTugarev requested a review from a team February 15, 2023 09:20
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 15, 2023
@AlexTugarev
Copy link
Member Author

@geropl, please have a look.

@geropl
Copy link
Member

geropl commented Feb 15, 2023

Ah, nice idea! 💡 ✨

I wonder if it's worth the effort. But will definitely have a closer look after lunch. 🙏

@AlexTugarev
Copy link
Member Author

/hold

I didn't test (re-)starting workspaces, thus it's not verified if that works with IDE at all.

@AlexTugarev
Copy link
Member Author

AlexTugarev commented Feb 15, 2023

/hold cancel

Starting workspaces ✔️
Restarting workspace (after logout in background) ✔️

@stale
Copy link

stale bot commented Mar 19, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta: stale This issue/PR is stale and will be closed soon release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants