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

feat!: Replace bearer with cookie-based authentication #1587

Merged
merged 8 commits into from
Jul 31, 2024

Conversation

dominik003
Copy link
Collaborator

@dominik003 dominik003 commented May 27, 2024

Breaking Changes

For Administrators

  • Support for non-compliant OIDC identity providers has been removed. If you've used OAuth without OIDC before, you can set up Keycloak as intermediate identity provider.
  • Since we no longer distinguish between different identity implementations, we have removed the providers abstraction in the values.yaml. This means that the providers field that supported azure or oauth as values has been removed and instead of oauth.xy or azure.xy xy is used directly (e.g. instead of oauth.endpoints.wellKnown it's endpoints.wellKnown).
  • The OIDC well-known endpoint is now a required configuration value and must be defined in endpoints.wellKnown. As a result of reading all endpoints from the well-known endpoint, the endpoints.token_issuance endpoint has been removed. The endpoints.authorization endpoint can still be used to overwrite the authorization endpoint from the well known configuration. It's needed for the OAuth mock, is most of the cases you can omit it.
  • The usernameClaim configuration was removed. Instead, a more fine-grained mapping is available. If you want to have the same behaviour as it was before the update, set idpIdentifier and username to the previous value of usernameClaim.
    claimMapping:
        idpIdentifier: sub # Maps the `sub` attribute in the JWT to the `idpIdentifier` attribute in the CCM. `idpIdentifier` is a new attribute to identify users uniquely. The attribute can be patched via the API for existing users.
        username: preferred_username # The username which is displayed in the UI and can be used to add users to projects.
        email: email # Optional email address.

For API Users

  • The PATCH /api/v1/users/{id}/roles route has been removed. Use the new PATCH /api/v1/users/{id} route and pass the new role as attribute in the payload.
  • The POST /api/v1/users endpoints requires a new attribute idp_identifier in the payload.
  • The GET /api/v1/sessions/{session_id}/connection returns the cookies as header (Set-Cookie) instead of an attribute in the payload.

Description

Generalized Authentication Provider

Until now the CCM had two types of authentication providers, namely Azure and OAuth. This distinction is no longer necessary because by implementing support for OIDC providers in general, we are also including Azure. Therefore, this PR merges both providers into one generalized provider.

Security Improvements

Previously, the CCM stored the identity token and the refresh token in a variable in the front-end authentication service. However, because the CCM is a single-page application, the client has access to the code, and storing these values as service variables is vulnerable to XSS attacks. The new approach now stores both tokens as secure, httponly, and samesite=strict cookies, which means that they are only appended to secure http requests, i.e., https, that they are automatically appended to all requests to endpoints that match a defined domain and path, and that there is no way to access the cookies from javascript.

In addition, we now use three different security options/parameters that are supported and recommended in OIDC.
First, we use a state parameter generated in the backend and validated by the frontend to protect against Cross-Site Request Forgery (CSRF) attacks. This is achieved by only requesting a token exchange from the backend if the state returned by the OIDC provider exists in local storage. Theoretically, this is still vulnerable to a combination of XSS and CSRF attacks, but as mentioned in the OWASP Cross-Site Request Forgery Prevention Cheat Sheet, this is a typical problem.
Second, we use a nonce parameter that is generated in the backend and passed to the OIDC provider, which then includes the value directly in the identity token. Then, when the backend receives the identity token during the token exchange, we validate that the nonce contained in the token matches the generated nonce.
Finally, we also use the Proof of Key Code Exchange (PKCE) extension to the OAuth/OIDC authorization code flow, where we generate a code verifier, append a hashed version of the code verifier, called the code challenge, and the hash method, called the code challenge method, to the initial authentication URL, and then send the generated code verifier during the token exchange. The OIDC provider then validates that the code challenge is equal to the hash of the sent code verifier. By using PKCE, we protect against Authorization Code Interception Attacks and we already comply with the OAuth 2.1 requirement to use PKCE.

We also removed the custom implementation of a keystore previously found in keystore.py and switched to a JWK client implementation provided by the same library used to validate the JWT tokens. This not only removes a lot of code, but also increases security by using a well-known open source implementation.

Removal of the ngx-cookie Package

Previously, we set session cookies in the frontend using the `ngx-cookie' package. This PR removes that package and now also sets the session cookies via set-cookie headers from the backend.

TODOs

  • Investigate whether we can remove the whole keystore.py file and replace it by the pyjwt PyJWKClient as seen in pyjwt oidc login flow
  • Find and replace important string comparisons such as state and nonce comparisons to use hmac.compare_digest to prevent time analysis.
  • Evaluate and discuss whether the current approach is vulnerable to Cross Site Request Forgery (CSRF). This includes discussing whether we want to set the SameSite attribute to strict or lax. For more information, see Same site cookie attribute prevents CSRF attacks.
  • Investigate a problem where after a successful login, the frontend displays an "Unauthorized" error coming from the users/current endpoint and the navigation bar is not displayed. The problem here is that the endpoint is called before the /tokens endpoint is called or its request has finished, so there is no identity token set yet.
  • Properly set the allow origins header to our frontend
  • Make sure the tests run without the oauth mock run
  • Write a proper PR description, including information about the breaking changes
  • Test against keycloak and write short documentation on how to use the CCM with keycloak
  • Evaluate which tests are needed/helpful and write them
  • Add removal of the patch /{user_id}/role route as breaking change and mention the new patch /{user_id} route
  • Fix the tests that are broken due to the refactoring
  • Test this PR in the local cluster. For this, first deploy the old version, i.e., without the database changes, and then redeploy with the changes in here.
  • Write down the breaking changes introduced in this PR
  • When merged, we need to adjust the PR description in feat!: Replace bearer with cookie-based authentication #1587
  • Before merging, we need to adjust the documentation added in feat!: Replace bearer with cookie-based authentication #1587 (e.g., the steps/configurations required to use Keycloak)

Potential follow-up issues

  • When implementing redis for caching, we should think about moving the nonce and code_verifier from the frontend to a proper backend session using redis. We can then also move the state back to the backend and add the redirecTo to the backend session object. This should further improve security as we are not exposing any information to the frontend that could be used for an attack.
  • Move the token refresh logic from the frontend to the backend, i.e. if we get a token expired error during token validation, we automatically try to refresh the token.

Resolves #1596, Resolves #1592

Copy link

github-actions bot commented May 27, 2024

A Storybook preview is available for commit 463dede.
View Storybook
View Chromatic build

@dominik003 dominik003 marked this pull request as draft May 27, 2024 16:46
@dominik003 dominik003 force-pushed the feat-rework-authentication branch 4 times, most recently from 870fece to a109d99 Compare June 10, 2024 16:40
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jun 10, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jun 10, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jun 10, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jun 10, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jun 10, 2024
Copy link
Member

@MoritzWeber0 MoritzWeber0 left a comment

Choose a reason for hiding this comment

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

Good improvements in regards to XSS protection and a more stable authentication system. I have some comments which we should implement before merging.

backend/capellacollab/config/models.py Show resolved Hide resolved
backend/capellacollab/core/authentication/flow.py Outdated Show resolved Hide resolved
backend/pyproject.toml Outdated Show resolved Hide resolved
backend/tests/users/test_tokens.py Show resolved Hide resolved
frontend/src/app/services/auth/auth.service.ts Outdated Show resolved Hide resolved
helm/config/backend.yaml Outdated Show resolved Hide resolved
@MoritzWeber0 MoritzWeber0 changed the title Feat rework authentication feat: Replace bearer with cookie-based authentication Jun 13, 2024
@MoritzWeber0 MoritzWeber0 changed the title feat: Replace bearer with cookie-based authentication feat!: Replace bearer with cookie-based authentication Jun 13, 2024
@dominik003 dominik003 force-pushed the feat-rework-authentication branch 4 times, most recently from 670ec16 to 8ca343b Compare June 17, 2024 16:16
@dominik003 dominik003 force-pushed the feat-rework-authentication branch 3 times, most recently from a9330d0 to 38acdc2 Compare July 1, 2024 11:41
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 36 lines in your changes missing coverage. Please review.

Project coverage is 83.97%. Comparing base (f366ae0) to head (e773ef1).
Report is 2 commits behind head on main.

Files Patch % Lines
...apellacollab/core/authentication/api_key_cookie.py 64.70% 17 Missing and 1 partial ⚠️
backend/capellacollab/core/authentication/oidc.py 91.52% 5 Missing ⚠️
...ackend/capellacollab/core/authentication/routes.py 93.24% 3 Missing and 2 partials ⚠️
backend/capellacollab/users/routes.py 80.00% 1 Missing and 2 partials ⚠️
...nd/capellacollab/core/authentication/basic_auth.py 66.66% 2 Missing ⚠️
backend/capellacollab/sessions/routes.py 0.00% 1 Missing and 1 partial ⚠️
backend/capellacollab/users/exceptions.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1587      +/-   ##
==========================================
+ Coverage   81.12%   83.97%   +2.85%     
==========================================
  Files         191      184       -7     
  Lines        6215     6086     -129     
  Branches      692      673      -19     
==========================================
+ Hits         5042     5111      +69     
+ Misses       1027      827     -200     
- Partials      146      148       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dominik003 dominik003 force-pushed the feat-rework-authentication branch 6 times, most recently from 0935596 to fc1f870 Compare July 8, 2024 11:47
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@MoritzWeber0 MoritzWeber0 force-pushed the feat-rework-authentication branch 2 times, most recently from fd0c755 to a56f41d Compare July 30, 2024 14:32
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
@MoritzWeber0 MoritzWeber0 force-pushed the feat-rework-authentication branch 2 times, most recently from 1e64fea to 2a41f63 Compare July 30, 2024 14:46
@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 30, 2024
Copy link

sonarcloud bot commented Jul 31, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
2 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@DSD-DBS DSD-DBS deleted a comment from github-actions bot Jul 31, 2024
@MoritzWeber0 MoritzWeber0 merged commit 497e033 into main Jul 31, 2024
30 checks passed
@MoritzWeber0 MoritzWeber0 deleted the feat-rework-authentication branch July 31, 2024 10:10
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.

Merge authentication provider Use cookie-based authentication
2 participants