auth: fix token verification chain #4415
Conversation
|
There is a regression in the latest release PTAL @thaJeztah @Jamstah 😬 Once this is merged in, I'll cut a patch beta release |
| } | ||
|
|
||
| return |
There was a problem hiding this comment.
thanks 🙏 (I hate named output vars combined with naked returns)
registry/auth/token/token.go
Outdated
| if err != nil { | ||
| // NOTE(milosgajdos): if the x5c header is missing | ||
| // the token may have been signed by a JWKS. | ||
| if err == jose.ErrMissingX5cHeader { |
There was a problem hiding this comment.
we should probably use errors.Is here
There was a problem hiding this comment.
I was considering that, too. Yeah, can change that.
| if signingKey, ok := verifyOpts.TrustedKeys[header.KeyID]; ok { | ||
| return signingKey, nil | ||
| } | ||
| return nil, fmt.Errorf("token signed by untrusted key with ID: %q", header.KeyID) |
There was a problem hiding this comment.
Not new, but I notice we return a "ErrInvalidToken" in other branches; is that error special / used as a sentinel error? (and in that case, must all invalid errors wrap that one somewhere?)
There was a problem hiding this comment.
ErrinvalidToken is a sentinel error that was in the original code (see v2.8 branch:
distribution/registry/auth/token/token.go
Line 31 in 5d5c60f
I think in this specific code we could maybe wrap it, but I'm not sure it's that important, because the other switch branches return different errors, too 🤔
There was a problem hiding this comment.
I am also wondering if we should ditch the switch clause here and replace it with if statements 🤔 might make the code a bit more readble
There was a problem hiding this comment.
Nah, I think a switch is nice; with a switch, all conditions are upfront and visible, with if/else if/else, they get buried deeper down the line, which (IMO) usually makes it less readable.
c6f686b to
eb469f1
Compare
There was a small regression introduced in distribution#4349. Specifically, if the certificate chain verification succeeds we should return immediately instead of following up with further token verification checks. This commit fixes that: we only follow up with further token verifications if x5c header is missing. We've also refactored this method so it's hopefully clearer. Co-authored-by: Kyle Squizzato <ksquizz@gmail.com> Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
eb469f1 to
70e0d88
Compare
This is not currently in any release (or even pushed image), right? I was trying out the 3.0.0-beta.1 release and ended up here after googling the problems we see |
|
That's correct, @calmh. We will do another patch release before the stable, but no ETA. |
There was a small regression introduced in
#4349.
Specifically, if the certificate chain verification succeeds we should return immediately instead of following up with further verification checks.
This commit fixes that: we only follow up with further token verifications if the
x5cheader is missing.We've also refactored this method so it's hopefully clearer:
Closes #4299