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

Replace usage of RS256 JWT signing with simpler HS384 #1622

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

spwoodcock
Copy link
Member

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

#1574
#1539

Describe this PR

  • Initially I thought that RS256 would be the best encryption algorithm for us: it's the stronger choice and has some advantages.
  • However I neglected to consider ease of use and deployment:
    • Uses need to generate a pub/priv key pair - while we can automate this a bit, it's not ideal.
    • Generated RSA keys are a bit of a pain to use in CI variables (they could be base64 encoded to get around this).
  • The symmetric algo HS384 is absolutely fine and secure enough for our use case.
  • I chose this over HS256 as a good middle ground of bit length and future proofing (overkill still).
  • It's supported by pyjwt and electric-sql πŸ‘
  • We can re-use the ENCRYPTION_KEY variable we already have in FMTM for the database encryption πŸŽ‰

Note the ENCRYPTION_KEY is 42-bits and we ideally need a 48-bit secret for HS384, but it's not a huge dealbreaker.
We could add a migration in future to update the tokens to more bits if needed.

Review Guide

Try and sign in.

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@spwoodcock spwoodcock added the backend Related to backend code label Jul 3, 2024
@spwoodcock spwoodcock requested a review from Sujanadh July 3, 2024 15:39
@spwoodcock spwoodcock self-assigned this Jul 3, 2024
@github-actions github-actions bot added docs Improvements or additions to documentation frontend Related to frontend code devops Related to deployment or configuration labels Jul 3, 2024
@spwoodcock
Copy link
Member Author

Merging this early so I can test it with the mapper frontend / electric-sql.

Added @Sujanadh in review as a sanity check - let me know if any issues πŸ‘

@spwoodcock spwoodcock merged commit bbfe82c into development Jul 3, 2024
6 of 7 checks passed
@spwoodcock spwoodcock deleted the fix/HS384-over-RS256 branch July 3, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code devops Related to deployment or configuration docs Improvements or additions to documentation frontend Related to frontend code
Projects
Development

Successfully merging this pull request may close these issues.

1 participant