feat(authN): Redesign JWT token auth #372 #394
Conversation
michalkrzyz
requested review from
drochow,
MR2011 and
dorneanu
as code owners
michalkrzyz
force-pushed
the
mikrzyz/issue-239
branch
from
6072be7
to
0b45461
Compare
Redesign JWT token authentication middleware to support additional/alternative authentication method
michalkrzyz
force-pushed
the
mikrzyz/issue-239
branch
2 times, most recently
from
7240fe3
to
6a63744
Compare
| logger Logger | ||
| secret []byte | ||
| } | ||
|
|
There was a problem hiding this comment.
A Docstring would probably be good to say:
- Verifies token presence in header
- Validates token by parsing
- Checks token validity (signature)
- Verifies expiration time
Due to implementation constraints, we intentionally do:
- No validation of the token issuer (iss claim)
- No validation of the "not before" time (nbf claim)
- No audience validation (aud claim)
- No token signing algorithm verification before parsing
There was a problem hiding this comment.
I am not a big fan of commenting the code ("good code documents itself" :) ). Would it be ok if I enclose each check in single responsibility functions like:
checkHeaderInRequest
verifyTokenStructure
checkTokenSignature
verifyTokenExpiration
etc.
There was a problem hiding this comment.
@drochow please check the last commit. I have added some functions to make checks/gets more readable.
Split verify function for JWT token auth to make it more readable
michalkrzyz
force-pushed
the
mikrzyz/issue-239
branch
from
eb0f0b3
to
7e776e4
Compare
| func NewAuth(cfg *util.Config) Auth { | ||
| l := newLogger() | ||
| type Auth struct { | ||
| chain []AuthMethod |
There was a problem hiding this comment.
Maybe I'm missing the full picture, but why exactly do we need a chain of auth methods?
There was a problem hiding this comment.
We want to have the same API access via JWT token (scanner) as well as for OIDC (user)
There was a problem hiding this comment.
Hmmm.. Even if OIDC uses JWTs as part of its specification, we do need to treat both auth mechanisms differently. I mean, you also have different fields (inside the JWT token payload, see this example). Or how is this supposed to work? Maybe we need some call to clarify this.
|
In the description I cannot see the "related issues" nor the ones which the PR should close. |
Redesign JWT token authentication middleware to support additional/alternative authentication method
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Added to documentation?
Checklist