Skip to content

Bugfix/OIDC aud field check #1640

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

FracassandoCasualmente
Copy link

@FracassandoCasualmente FracassandoCasualmente commented Apr 30, 2025

Related issue(s) and PR(s)

This PR closes #1639.

Description

Resolves a vulnerability where a rogue server could impersonate any user who logs into it, enabling unauthorized access to any SDA instance using LS AAI.

How to test

  • Run make sda-s3-up
  • Log into the rogue service at http://localhost:8802/ as a dummy user
  • Get the access token from the rogue server and put it into a "token" variable in the terminal
  • Run curl -H "Authorization: Bearer $token" localhost:8090/files
  • Check that the api returns an error response
  • Download the submission credentials file from the rogue service
  • Change the "host_base" in the configuration to "http://localhost:18000"
  • Run sda-cli -config s3cmd.conf list and check that the inbox returns an error response
  • Repeat the same steps but by using the credentials from the real auth service at http://localhost:8801 and check that they both return a non-error response

Adds an alternative (evil) OIDC client that uses an access token of its own to impersonate a user in SDA
Avoids vulnerability where another OIDC client uses a user's token
to log into SDA, impersonating the latter's identity
this client was used to demonstrate a vulnerability caused by
not checking the 'aud' field in OIDC tokens
Copy link
Collaborator

@jbygdell jbygdell left a comment

Choose a reason for hiding this comment

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

Hi,

Thank you for your contribution.

We agree that this is something that needs addressing. Since the token authentication is used by several applications this will require changes to other parts of the code as well, mainly in the authenticator initiation and configurations. Which in turn requires changes to the helm chart and the test suite.

Removing unnecessary code
Still need to add audience verification using jwt.Parse

Co-authored-by: Joakim Bygdell <j.bygdell@gmail.com>
@FracassandoCasualmente FracassandoCasualmente marked this pull request as draft May 7, 2025 10:41
@FracassandoCasualmente
Copy link
Author

FracassandoCasualmente commented May 7, 2025

Hi,

Thank you for your contribution.

We agree that this is something that needs addressing. Since the token authentication is used by several applications this will require changes to other parts of the code as well, mainly in the authenticator initiation and configurations. Which in turn requires changes to the helm chart and the test suite.

Hello!
Thanks for reply and for the corrections.

Regarding the changes that are missing, I don't really have experience with helm charts nor have broad knowledge of the code base, so it would take me some time (1+ month?) to fix.
I'm happy to take the time to handle this, especially if your team feels short-staffed or overwhelmed. However, if you think it would take too long for the change to reach production, feel free to close this PR and handle it internally. :)

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.

[auth] OIDC authentication does not verify audience
2 participants