-
-
Notifications
You must be signed in to change notification settings - Fork 4
fix(auth): Require EdDSA and fix verification #232
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
Conversation
| if jwt_header.alg != Algorithm::EdDSA { | ||
| tracing::warn!( | ||
| "JWT signed with unexpected algorithm `{:?}`", | ||
| jwt_header.alg | ||
| ); | ||
| } |
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.
different algorithms require a different key constructor. if we see this happening a lot with InvalidKeyErrors, it's a hint that we need to update the match block below to handle more cases
i'd have just done that right now, but some algorithms have keys that come in multiple formats (pem or der, for example) and i don't want to arbitrarily pick one. if we really have to deal with this, it will probably be added to the key stuff in our config
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.
In our spec we said that we require EdDSA, so let's actually fail auth for now if a different algorithm is provided.
* main: feat(server): Implement the new Web API (#230)
| if jwt_header.alg != Algorithm::EdDSA { | ||
| tracing::warn!( | ||
| algorithm = ?jwt_header.alg, | ||
| "JWT signed with unexpected algorithm", | ||
| ); | ||
| let kind = jsonwebtoken::errors::ErrorKind::InvalidAlgorithm; | ||
| return Err(AuthError::ValidationFailure(kind.into())); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jan-auer
left a comment
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.
@matt-codecov I've made two changes to the PR:
- Require EdDSA. We need to restrict the supported algorithms as per spec.
- Log errors for invalid keys but skip them.
For a follow-up: We should process all keys during startup instead of parsing them again for each request. Then, AuthContext can instantiated with a map of ready keys and just has to perform validation.
There are many different functions you are to use for different types of keys. We are planning to require EdDSA, so we validate the algorithm and switch to using
from_ed_pem().Ref FS-160