feat: implement support for http digest auth (resolve #352) #553
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,8 +15,10 @@ package config | |
|
|
||
| import ( | ||
| "context" | ||
| "crypto/md5" | ||
| "crypto/tls" | ||
| "crypto/x509" | ||
| "encoding/hex" | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
|
|
@@ -77,7 +79,7 @@ var invalidHTTPClientConfigs = []struct { | |
| }, | ||
| { | ||
| httpClientConfigFile: "testdata/http.conf.empty.bad.yml", | ||
| errMsg: "at most one of basic_auth, digest_auth, oauth2, bearer_token & bearer_token_file must be configured", | ||
| }, | ||
| { | ||
| httpClientConfigFile: "testdata/http.conf.basic-auth.too-much.bad.yaml", | ||
|
|
@@ -97,11 +99,15 @@ var invalidHTTPClientConfigs = []struct { | |
| }, | ||
| { | ||
| httpClientConfigFile: "testdata/http.conf.basic-auth-and-auth-creds.too-much.bad.yaml", | ||
| errMsg: "at most one of basic_auth, digest_auth, oauth2 & authorization must be configured", | ||
| }, | ||
| { | ||
| httpClientConfigFile: "testdata/http.conf.basic-auth-and-oauth2.too-much.bad.yaml", | ||
| errMsg: "at most one of basic_auth, digest_auth, oauth2 & authorization must be configured", | ||
| }, | ||
| { | ||
| httpClientConfigFile: "testdata/http.conf.basic-auth-and-digest.too-much.bad.yaml", | ||
| errMsg: "at most one of basic_auth, digest_auth, oauth2 & authorization must be configured", | ||
| }, | ||
| { | ||
| httpClientConfigFile: "testdata/http.conf.auth-creds-no-basic.bad.yaml", | ||
|
|
@@ -312,6 +318,31 @@ func TestNewClientFromConfig(t *testing.T) { | |
| fmt.Fprint(w, ExpectedMessage) | ||
| } | ||
| }, | ||
| }, { | ||
| clientConfig: HTTPClientConfig{ | ||
| BasicAuth: &BasicAuth{ | ||
| Username: ExpectedUsername, | ||
| Password: ExpectedPassword, | ||
| }, | ||
| TLSConfig: TLSConfig{ | ||
| CAFile: TLSCAChainPath, | ||
| CertFile: ClientCertificatePath, | ||
| KeyFile: ClientKeyNoPassPath, | ||
| ServerName: "", | ||
| InsecureSkipVerify: false}, | ||
| }, | ||
| handler: func(w http.ResponseWriter, r *http.Request) { | ||
| username, password, ok := r.BasicAuth() | ||
| if !ok { | ||
| fmt.Fprintf(w, "The Authorization header wasn't set") | ||
| } else if ExpectedUsername != username { | ||
| fmt.Fprintf(w, "The expected username (%s) differs from the obtained username (%s).", ExpectedUsername, username) | ||
| } else if ExpectedPassword != password { | ||
| fmt.Fprintf(w, "The expected password (%s) differs from the obtained password (%s).", ExpectedPassword, password) | ||
| } else { | ||
| fmt.Fprint(w, ExpectedMessage) | ||
| } | ||
| }, | ||
| }, { | ||
| clientConfig: HTTPClientConfig{ | ||
| Authorization: &Authorization{ | ||
|
|
@@ -335,7 +366,7 @@ func TestNewClientFromConfig(t *testing.T) { | |
| }, | ||
| }, { | ||
| clientConfig: HTTPClientConfig{ | ||
| DigestAuth: &DigestAuth{ | ||
| Username: ExpectedUsername, | ||
| Password: ExpectedPassword, | ||
| }, | ||
|
|
@@ -347,14 +378,61 @@ func TestNewClientFromConfig(t *testing.T) { | |
| InsecureSkipVerify: false}, | ||
| }, | ||
| handler: func(w http.ResponseWriter, r *http.Request) { | ||
| // Example server response header: | ||
| // WWW-Authenticate: Digest realm="prometheus", nonce="43568ca162f46c3bcc57ecae193b3159", qop="auth", opaque="3bc9f19d8195721e24469ff255750f8c", algorithm=MD5, stale=FALSE | ||
| // | ||
| // Example client request header: | ||
| // Authorization: Digest username="foo", realm="prometheus", nonce="43568ca162f46c3bcc57ecae193b3159", uri="/", cnonce="NDA2M2JmYzQ2YTQ4OTQ0OTQ1NzE0NmI3ZmYyY2YyNzU=", nc=00000001, qop=auth, response="fe543d7eeb2d2f0aba8d100a1f076909", opaque="3bc9f19d8195721e24469ff255750f8c", algorithm=MD5 | ||
|
|
||
| const ( | ||
| nonce = "43568ca162f46c3bcc57ecae193b3159" | ||
| realm = "prometheus" | ||
| ) | ||
|
|
||
| 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) | ||
|
Comment on lines
+392
to
+395
There was a problem hiding this comment. can we refactor this test to allow varying the inputs?
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. |
||
| } else { | ||
| // second, authenticated request | ||
| if !strings.HasPrefix(authHeader, "Digest") { | ||
| fmt.Fprint(w, "Request does not contain a valid digest auth header") | ||
| return | ||
| } | ||
|
|
||
| digestComponents := make(map[string]string) | ||
| for _, p := range strings.Split(authHeader, ", ")[1:] { | ||
| kvParts := strings.Split(p, "=") | ||
| digestComponents[kvParts[0]] = strings.TrimSpace(strings.Trim(kvParts[1], "\"")) | ||
| } | ||
|
|
||
| if v := digestComponents["realm"]; v != realm { | ||
| fmt.Fprintf(w, "Digest auth with wrong realm (%s)", v) | ||
| return | ||
| } | ||
| if v := digestComponents["nonce"]; v != nonce { | ||
| fmt.Fprintf(w, "Digest auth with wrong nonce (%s)", v) | ||
| return | ||
| } | ||
|
|
||
| hashMD5 := func(s string) string { | ||
| hasher := md5.New() | ||
| hasher.Write([]byte(s)) | ||
| return hex.EncodeToString(hasher.Sum(nil)) | ||
| } | ||
|
|
||
| hash1Str := fmt.Sprintf("%s:%s:%s", ExpectedUsername, realm, ExpectedPassword) | ||
| hash1 := hashMD5(hash1Str) | ||
| hash2Str := fmt.Sprintf("GET:%s", digestComponents["uri"]) | ||
| hash2 := hashMD5(hash2Str) | ||
| responseStr := fmt.Sprintf("%s:%s:%s:%s:%s:%s", hash1, nonce, digestComponents["nc"], digestComponents["cnonce"], digestComponents["qop"], hash2) | ||
| response := hashMD5(responseStr) | ||
|
|
||
| if response != digestComponents["response"] { | ||
| fmt.Fprintf(w, "Digest auth failed, response hashes didn't match") | ||
| return | ||
| } | ||
|
|
||
| fmt.Fprint(w, ExpectedMessage) | ||
| } | ||
| }, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| basic_auth: | ||
| username: user | ||
| password: foo | ||
| digest_auth: | ||
| username: user | ||
| password: foo |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is a maintainer and could eventually be responsible for merging my PR?
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.
?
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'm sorry but I'm not maintainer here, just a contributor
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.
Okay, sorry to bother you then. Who can I tag here to finally (!) get this finished?!
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.
Hello??? @SuperQ
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.
?????
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 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.