-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
env_file: | ||
- path: ./local.env | ||
required: true | ||
- path: ./override.env |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fancy docs!
The base branch was changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
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.
The override file we generate looks like this (with the relevant key info removed):