Conversation
roidelapluie
force-pushed
the
basicauth
branch
2 times, most recently
from
68ce902 to
1fe1da7
Compare
|
Thanks for working on this. What do you think about putting the user list into the same file as a first iteration. This would simplify the standard use case of config management deployed exporters having to manage multiple files. tls_config:
...
users:
alice: "$2y$BCRYPTHASH" |
roidelapluie
force-pushed
the
basicauth
branch
2 times, most recently
from
948241f to
f529d9c
Compare
|
ready for review. In particular: do we want to use bcrypt here? Pros:
|
|
Bcrypt is the current best practice, I'd be very wary of using anything else. You can adjust how long it takes by changing the number of rounds. How long are we actually talking here? |
SuperQ
left a comment
SuperQ
left a comment
There was a problem hiding this comment.
Minor nits.
Can you add an entry to the top level CHANGELOG?
* [FEATURE] Add basic authentication #1673
|
Yes, I'll also add WWW-Authenticate header |
70ms for 'test1234' with 10 rounds. |
|
70ms isn't too bad overall, maybe just mention it in the docs as a note? It's up to the user how many rounds to use when generating the hash. |
|
I agree, we should support bcrypt to start. Apache 2.4 htpasswd still defaults to 5 rounds, which is pretty small. It seems like basic auth is getting hashed on every scrape. Is there a way to cache this? Setting bcrypt cost to 16 causes each scrape to take 3.9s. We should document this in the README. |
|
Added README Added ability to use basic auth without TLS Added empty web config file support Use Strict Unmarshal ( fix #1691 ) |
roidelapluie
force-pushed
the
basicauth
branch
3 times, most recently
from
241cc97 to
06d6447
Compare
|
I have pushed further commits. |
roidelapluie
force-pushed
the
basicauth
branch
2 times, most recently
from
1e7beee to
3c513c6
Compare
|
I have squashed, rebased add added CHANGELOG entry. |
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
|
Another change I made is to allow to use empty usernames / passwords: curl -u : http://127.0.0.1:9100
<html>
<head><title>Node Exporter</title></head>
<body>
<h1>Node Exporter</h1>
<p><a href="/metrics">Metrics</a></p>
</body>
</html> |
brian-brazil
left a comment
brian-brazil
left a comment
There was a problem hiding this comment.
👍
We might also want a test for an empty username/password.
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
|
Done. We do not threat empty username/password differently than anything else. We can add a test later ; but it does not sound like a strong requirement for now. |
* Add basic authentication Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
* Add basic authentication Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
4 participants
WIP - To gather initial feedback.
Fully working.
Signed-off-by: Julien Pivotto roidelapluie@inuits.eu