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

[Issue #2809] Handle parsing the jwt we created, and connect to a user #2959

Merged
merged 34 commits into from
Nov 21, 2024

Conversation

chouinar
Copy link
Collaborator

Summary

Fixes #2809

Time to review: 15 mins

Changes proposed

Setup logic to process the jwt we created in #2898

Setup a method to automatically generate a key for local development in a secure way via an override file

Context for reviewers

The core part of this PR is pretty straightforward, we parse the JWT, do some validation, raise specific error messages for certain scenarios, and have tests for that behavior.

For the auth token in the API request header, instead of using Bearer .. I left it as a dedicated header field. The bearer format doesn't let you specify the header name and if we ever need multiple tokens supported in an endpoint will lead to more headache.


Where things got complex was setting up the private/public key for the API. These just need to be stored in env vars, but putting them directly in our local.env file isn't ideal - even though the key will be distinctly local-only, it will always be something flagged in security scans and just generally look problematic.

To work around this fun problem, I realized I could solve another annoyance at the same time, Docker as of January 2024 allows you to specify multiple env files + make them optional. So - I used that. I setup a script that creates an override.env file that you can freely modify and won't be checked in, and more importantly, automatically contains secrets like those public/private keys we didn't want to check in. (Note - if you're wondering why I didn't use Docker secrets, they're far more complex and this PR would've been 20+ files to make that half-work).

Additional information

Locally I confirmed we can set tokens in the swagger docs and they work - we don't yet have an endpoint that uses this outside of the unit test I setup, but I temporarily modified the healthcheck endpoint to validate things work outside of tests as well.

Screenshot 2024-11-20 at 4 06 48 PM

The override file we generate looks like this (with the relevant key info removed):

# override.env
#
# Any environment variables written to this file
# will take precedence over those defined in local.env
#
# This file will not be checked into github and it is safe
# to store secrets here, however you should still follow caution
# with using any secrets locally if they cause the app to interact
# with external systems.
#
# This file was generated by running:
#    make setup-env-overrides
#
# Which runs as part of our "make init" flow.
#
# If you would like to re-generate this file, please run:
#    make setup-env-overrides --recreate
#
# Note that this will completely erase any existing configuration you may have


############################
# Authentication
############################

API_JWT_PRIVATE_KEY="-----BEGIN RSA PRIVATE KEY-----
...
-----END RSA PRIVATE KEY-----"

API_JWT_PUBLIC_KEY="-----BEGIN PUBLIC KEY-----
...
-----END PUBLIC KEY-----"

@chouinar chouinar requested a review from coilysiren November 20, 2024 21:08
@chouinar chouinar requested a review from mdragon as a code owner November 20, 2024 21:08
mdragon
mdragon previously approved these changes Nov 20, 2024
coilysiren
coilysiren previously approved these changes Nov 20, 2024
env_file:
- path: ./local.env
required: true
- path: ./override.env
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool! It's just a list so lower ones override, nice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, when I came across it in the docs I realized it was exactly what I'd been looking for in env var management from Docker for years

#
# Any environment variables written to this file
# will take precedence over those defined in local.env
#
Copy link
Collaborator

Choose a reason for hiding this comment

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

fancy docs!

Base automatically changed from chouinar/2808-create-a-jwt to main November 21, 2024 15:23
@chouinar chouinar dismissed stale reviews from coilysiren and mdragon November 21, 2024 15:23

The base branch was changed.

coilysiren
coilysiren previously approved these changes Nov 21, 2024
mdragon
mdragon previously approved these changes Nov 21, 2024
Copy link
Collaborator

@mdragon mdragon left a comment

Choose a reason for hiding this comment

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

Looks good

@chouinar chouinar dismissed stale reviews from mdragon and coilysiren via c4631ae November 21, 2024 16:46
@chouinar chouinar requested a review from mdragon November 21, 2024 16:46
@chouinar chouinar merged commit 487d217 into main Nov 21, 2024
@chouinar chouinar deleted the chouinar/2809-parse-a-jwt branch November 21, 2024 17:18
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.

Create a new auth implementation that validates a JWT token and attaches a user to the global context
4 participants