feat: implement support for http digest auth (resolve #352) #553
Conversation
muety
force-pushed
the
digest-auth
branch
2 times, most recently
from
25b2e9f
to
1816b4b
Compare
|
Not sure about the linting errors, perhaps someone could give me some assistance with that? |
|
Any chances to get this merged? Should I tag someone in here? |
|
I don't see any major issues. I do wonder about if Can you describe why you picked this implementation? |
|
Yeah, I was expecting a bit of discussion around this. I don't know about your requirements for pulling in new dependencies. I was hoping Go's standard lib had an implementation of digest auth, but turns out it doesn't. I picked |
|
Is there someone I could tag here who is in charge of merging my PR? |
Signed-off-by: Ferdinand Mütsch <ferdinand@muetsch.io>
Signed-off-by: Ferdinand Mütsch <ferdinand@muetsch.io>
|
Rebase done, can this be merged now? |
| if authHeader := r.Header.Get("Authorization"); authHeader == "" { | ||
| // first request | ||
| w.Header().Set("www-authenticate", "Digest realm=\""+realm+"\", nonce=\""+nonce+"\", qop=\"auth\", opaque=\"3bc9f19d8195721e24469ff255750f8c\", algorithm=MD5, stale=FALSE") | ||
| w.WriteHeader(401) |
There was a problem hiding this comment.
can we refactor this test to allow varying the inputs?
There's a couple of cases I think this needs to be able to test:
- qop=auth-int
- different algorithms
- algorithm=(alg)-sess vs algorithm=(alg) (all the way to include cnonces)
Later on, if we can keep some state: nextnonce support
There was a problem hiding this comment.
Thanks for your review! I could include additional tests for different combinations of these values if requested. But I'm not sure if that's the way to go. The digest auth client library already includes tests for these (e.g. see here). While we surely want to test that digest auth works in general, I don't think it's our job to test the inner working of the library again.
| if c.BasicAuth != nil || c.OAuth2 != nil || c.DigestAuth != nil { | ||
| return fmt.Errorf("at most one of basic_auth, digest_auth, oauth2 & authorization must be configured") |
There was a problem hiding this comment.
these scattered checks to ensure only one type of auth is enabled bug me: more places to go wrong.
Maybe refactor them to a function that ensures only one type of auth is configured, and drop them in as needed.
auths_enabled := []bool{
c.BasicAuth != nil,
c.OAuth2 != nil,
c.DigestAuth != nil,
c.Authorization != nil,
}
if len(auths_enabled) > 1 {
// error
}
There was a problem hiding this comment.
Agree! I found these checks a bot strange as well and would vote for refactoring them (willing to do that!). Only wondering if this refactoring shall be included here or rather as part of another PR, so we can get this one merged preferably soon? Let me know what you think is better.
There was a problem hiding this comment.
i'm not the maintainer of this codebase, but if I had a choice, I'd say should be a separate PR that merges before this one.
There was a problem hiding this comment.
Who is a maintainer and could eventually be responsible for merging my PR?
There was a problem hiding this comment.
I'm sorry but I'm not maintainer here, just a contributor
There was a problem hiding this comment.
Okay, sorry to bother you then. Who can I tag here to finally (!) get this finished?!
There was a problem hiding this comment.
This took me like half a day of work. Sorry, but I find it quite disrespectful that none of the maintainer even only bothers to reply.
|
After almost a year, I guess this is not gonna be merged then? |
Implements #352. I'd need this in blackbox exporter. Let me know what you think! :-)
Digest auth can be tested against https://httpbin.org/#/Auth.