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

VEGA-2105 Add JWT verification #minor #54

Merged
merged 19 commits into from
Nov 14, 2023
Merged

VEGA-2105 Add JWT verification #minor #54

merged 19 commits into from
Nov 14, 2023

Conversation

townxelliot
Copy link
Contributor

@townxelliot townxelliot commented Nov 14, 2023

This will parse an X-Jwt-Authorization header out of the incoming request and attempt to verify any JWT token value in it.

At the moment, we then log whether a token was verified, or reject the request with a 401 and log an error.

Elliot Smith added 14 commits November 14, 2023 11:55
Assumption at present is that the UID for MRLPA will be at least
a one character string.
* Add JWT_SECRET_KEY variable to Makefile
* Modify api-test/tester to send JWT header
* Use X-Jwt-Authorization header for now to avoid clashes with
  AWS Authorization header (added by signer)
* Expose JWT_SECRET_KEY as env var to create lambda
* Add method for parsing a value out of an incoming event header
* Log incoming JWT, whether verified or error
Hopefully makes it slightly more reusable.
@townxelliot townxelliot requested a review from a team as a code owner November 14, 2023 12:38
gregtyler
gregtyler previously approved these changes Nov 14, 2023
if err == nil {
l.logger.Print("Successfully parsed JWT from event header")
} else {
l.logger.Print(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we return 401 if the token is invalid? Could potentially also build this into the VerifyHeader return values.

Suggested change
l.logger.Print(err)
shared.Problem{
StatusCode: 401,
Code: "UNAUTHORISED",
Detail: "Invalid JWT",
}.Respond()

Copy link
Contributor Author

@townxelliot townxelliot Nov 14, 2023

Choose a reason for hiding this comment

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

The only reason I didn't is because the ticket doesn't mention enforcing authorisation, only checking. I'm happy to add it though.

NB it might be that this will break the tests in CI, if the environment isn't set up properly or the env var isn't picked up etc., so might turn into a load more work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, poorly worded ticket: by "simply check these claims" it was the claims I meant were simple, I think we might as well enforce it off the bat. A classic of the "don't say 'simple' or 'just' lessons".

We're getting "Successfully parsed JWT from event header" in the lambda logs, so CI should pass enforcement 🤞🏽

Makes it easier to figure out which test failed (if any).
@townxelliot townxelliot merged commit c5f2b60 into main Nov 14, 2023
16 checks passed
@townxelliot townxelliot deleted the VEGA2105_jwt branch November 14, 2023 16:06
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.

2 participants