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

RAS Authentication Updates #291

Merged
merged 10 commits into from
Nov 17, 2023
Merged

RAS Authentication Updates #291

merged 10 commits into from
Nov 17, 2023

Conversation

utku-ozturk
Copy link
Member

@utku-ozturk utku-ozturk commented Oct 31, 2023

This PR extends the value that is stored as Redis (key, value) pair:

Old format: jwtToken
New format: jwtToken:email (since jwtToken is base64 encoded, :(colon as the separator makes sense.

ToDo

From Researcher Auth Service (RAS) Project Partner Developer Guide:
To comply with GA4GH, only the following signing algorithm is supported: RS256.

While the data portals' Auth0 implementation works with HS256, RAS only uses RS256. Current JWT encoding/decoding functions throw the exception below since RS256 requires public/private keys to sign tokens. We bypassed the decoding's verify_signature by setting False for now, which should be True in production.

ValueError: ('Could not deserialize key data. The data may be in an incorrect format, it may be encrypted with an unsupported algorithm, or it may be an unsupported key type (e.g. EC curves with explicit parameters).', [_OpenSSLErrorWithText(code=75497580, lib=9, reason=108, reason_text=b'error:0480006C:PEM routines::no start line')])

Related PRs:

  1. RAS Authentication Updates snovault#273
  2. RAS Authentication Updates fourfront#1864
  3. RAS Authentication Updates shared-portal-components#238

Copy link
Member

@willronchetti willronchetti left a comment

Choose a reason for hiding this comment

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

These changes generally look good but may merit some unit testing/fixes in existing tests, happy to take that on if needed. We also need to enable signature verification.

return jwt.decode(self.jwt, secret, audience=audience, leeway=leeway,
options={'verify_signature': True}, algorithms=['HS256'])
options={'verify_signature': False}, algorithms=algorithms)
Copy link
Member

Choose a reason for hiding this comment

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

Does this still not work when verify_signature = True?

@coveralls
Copy link

coveralls commented Nov 8, 2023

Pull Request Test Coverage Report for Build 6908964577

  • 13 of 14 (92.86%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 78.597%

Changes Missing Coverage Covered Lines Changed/Added Lines %
dcicutils/redis_tools.py 12 13 92.31%
Totals Coverage Status
Change from base Build 6736812910: 0.004%
Covered Lines: 9346
Relevant Lines: 11891

💛 - Coveralls

@utku-ozturk utku-ozturk merged commit 1de66d4 into master Nov 17, 2023
4 checks passed
@utku-ozturk utku-ozturk deleted the ras_updates branch November 17, 2023 20:57
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.

3 participants