-
Notifications
You must be signed in to change notification settings - Fork 528
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
Handle multiple www-authenticate headers #1075
Conversation
It might happen there are multiple `www-authenticate` headers returned from a registry, e.g. `Negotiate` and `Basic`. In the current code, in such situation the first challenge would be picked with no further checks, which would result eventually in `unrecognized challenge` error failing the whole build, even though `Basic` challenge could be used instead. `pickFromMultipleChallenges` function was added to loop through the challenges in search for a supported one.
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.
This seems like a code path that would benefit from having a unit test.
Is there a real-world scenario where you're seeing responses including multiple headers like this?
Seems related to kerberos? https://stackoverflow.com/questions/4265975/authentication-issues-with-www-authenticate-negotiate Reminds me of bazelbuild/rules_k8s#141 |
Thanks for the comment. I'll try to add a unit test later this week. So, yeah, it can be related to Kerberos. In fact, the real-world scenario I'm aware of is using SPNEGO NGINX module with basic authentication fallback - in such configuration two www-authenticate headers will be sent: |
I don't know much about kerberos, but would it make sense to attempt to support it? I assume not, but it sounds like it would be neat :) |
To be honest, I don't know much about Kerberos myself either. :( I'm also not sure how common it is to have a Kerberos (Negotiate) only registry, but I guess not that common. I don't think it's supported by Docker either (here?). |
Unit test added. |
// If we hit more than one, I'm not even sure what to do. | ||
wac := challenges[0] | ||
// If we hit more than one, let's try to find one that we know how to handle. | ||
wac := pickFromMultipleChallenges(challenges) |
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.
What happens if we don't know how to handle any of the challenges?
I feel like maybe we should filter out unrecognized challenges and make sure len(filtered) != 0
?
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.
If there's neither basic nor bearer challenge, the last challenge will be returned. This will lead to unrecognized challenge
error here.
I think I would leave checking if challenge can be handled outside ping.go. Here, we only have some preference as to which challenge to choose, but ultimately we can return one that's not basic nor bearer.
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.
I don't think it will return the last challenge because the wac
in for _, wac := range challenges
will shadow the outer wac
variable. Instead, it would return a zero-value Challenge.
Maybe if we don't encounter a useful challenge, we just return the first one? (line 148 currently)
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.
Ah, right, my bad, it should be for _, wac = range challenges
so wac
is not shadowed... But ok, I will change it so it returns the first challenge if neither basic nor bearer are found.
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.
Thanks for noticing that!
Codecov Report
@@ Coverage Diff @@
## main #1075 +/- ##
==========================================
+ Coverage 75.52% 75.55% +0.02%
==========================================
Files 107 107
Lines 5100 5105 +5
==========================================
+ Hits 3852 3857 +5
Misses 703 703
Partials 545 545
Continue to review full report at Codecov.
|
Can you fix the gofmt issue? |
Sorry for late response. I've run gofmt on ping.go. |
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.
Thanks for the PR!
It might happen there are multiple
www-authenticate
headers returned from a registry, e.g.Negotiate
andBasic
. In the current code, in such situation the first challenge would be picked with no further checks, which would result eventually inunrecognized challenge
error failing the whole build, even thoughBasic
challenge could be used instead.pickFromMultipleChallenges
function was added to loop through the challenges in search for a supported one.