NotificationsYou must be signed in to change notification settings
Fork338
Star288

feat: implement support for http digest auth (resolve #352)#553

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

muety wants to merge2 commits intoprometheus:main

base:main

Choose a base branch

frommuety:digest-auth

Open

feat: implement support for http digest auth (resolve #352)#553

muety wants to merge2 commits intoprometheus:mainfrommuety:digest-auth

Conversation

Copy link

muety commentedDec 14, 2023•
edited
Loading

Implements#352. I'd need this in blackbox exporter. Let me know what you think! :-)

Digest auth can be tested againsthttps://httpbin.org/#/Auth.

muety force-pushed thedigest-auth branch 2 times, most recently from25b2e9f to1816b4bCompare

December 14, 2023 12:27

Copy link

Author

muety commentedDec 14, 2023

Not sure about the linting errors, perhaps someone could give me some assistance with that?

mmorel-35 reviewed

Dec 16, 2023

View reviewed changes

config/http_config.go OutdatedShow resolvedHide resolved

muety force-pushed thedigest-auth branch frombb438d7 to9a2e38cCompare

December 16, 2023 14:01

Copy link

Author

muety commentedDec 20, 2023

Any chances to get this merged? Should I tag someone in here?

SuperQ requested a review fromroidelapluie

December 20, 2023 10:34

Copy link

Member

SuperQ commentedDec 20, 2023

I don't see any major issues. I do wonder about ifgithub.com/icholy/digest is the best library option for this. I haven't done any evaluation of what the available options are.

Can you describe why you picked this implementation?

Copy link

Author

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 pickedicholy/digest, because it was the only client implementation I could find.

Copy link

Author

muety commentedDec 28, 2023

Is there someone I could tag here who is in charge of merging my PR?

SuperQ requested changes

Jan 1, 2024

View reviewed changes

Copy link

Member

SuperQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This needs a rebase.

muety added2 commits

January 1, 2024 16:59

feat: implement support for http digest auth (resolveprometheus#352)

667bba1

Signed-off-by: Ferdinand Mütsch <ferdinand@muetsch.io>

fix: linting errors

ba61de2

Signed-off-by: Ferdinand Mütsch <ferdinand@muetsch.io>

muety force-pushed thedigest-auth branch from9a2e38c toba61de2Compare

January 1, 2024 16:04

Copy link

Author

muety commentedJan 5, 2024

Rebase done, can this be merged now?

SuperQ mentioned this pull request

Jan 8, 2024

feat: HTTP Bearer Authorization for simple use casesprometheus/exporter-toolkit#193

Open

robbat2 reviewed

Jan 10, 2024

View reviewed changes

config/http_config_test.go

Comment on lines +392 to +395

		ifauthHeader:=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)

Copy link

robbat2Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Author

muetyJan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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. seehere). While we surely want to test that digest auth works ingeneral, I don't think it's our job to test the inner working of the library again.

robbat2 reviewed

Jan 10, 2024

View reviewed changes

config/http_config.go

Comment on lines +366 to +367

		ifc.BasicAuth!=nil\|\|c.OAuth2!=nil\|\|c.DigestAuth!=nil{
		returnfmt.Errorf("at most one of basic_auth,digest_auth,oauth2 & authorization must be configured")

Copy link

robbat2Jan 10, 2024

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.

auths_enabled := []bool{  c.BasicAuth != nil,  c.OAuth2 != nil,  c.DigestAuth != nil,  c.Authorization != nil,}if len(auths_enabled) > 1 {  //  error}

Copy link

Author

muetyJan 11, 2024

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.