-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(http source): Digest access authentication #22101
base: master
Are you sure you want to change the base?
feat(http source): Digest access authentication #22101
Conversation
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 @average-gary for this contribution. Since there's no issue attached, can you explain the motivation for this PR in the summary? E.g. what use case does this change enable?
} | ||
|
||
tokio::time::timeout(inputs.timeout, client.send(request)) | ||
.then({ |
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.
Can you extract this into a separate function? Or alternatively two, an extra one for the new is_digest
block.
/// Digest authentication. | ||
/// | ||
/// requires a round trip to the server to get the challenge and then send the response | ||
Digest { |
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 is identical to Basic
, I wonder if we can add a new field digest: bool
that defaults to false
. cc @jszwedko for UX review 🙏
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.
👍 The fields are identical, but given users are likely to think about these mechanisms differently (rather than digest authentication being a specialized form of basic authentication) I think having them separate makes sense.
} | ||
let ha1 = format!("{:x}", md5::Md5::digest(format!("{}:{}:{}", username_inner, realm, user_password_inner.inner()))); | ||
let ha2 = format!("{:x}", md5::Md5::digest(format!("GET:{}", uri.path()))); | ||
let cnonce = "00000001"; // TODO: use rng for client nonce |
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.
Do you plan to address this in this 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.
Thank you for pointing this out. I will update to add this for this PR.
I have thousands of machines whose firmware uses this auth digest method and I wanted to see if I could make vector a viable option for scraping data/metrics from the machine. This PR was my PoC at enabling it. |
@@ -295,6 +299,20 @@ pub enum Auth { | |||
/// The bearer authentication token. | |||
token: SensitiveString, | |||
}, | |||
/// Digest authentication. | |||
/// | |||
/// requires a round trip to the server to get the challenge and then send the response |
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.
/// requires a round trip to the server to get the challenge and then send the response | |
/// Challenge and response authentication as specified by [RFC7616](https://httpwg.org/specs/rfc7616.html). |
Summary
Adding rfc2617 to http source
Change Type
Is this a breaking change?
How did you test this PR?
The datacenter/compute infrastructure I manage uses this authentication method for its endpoint. I successfully ran vector against one of these devices and received expected results back.
Does this PR include user facing changes?
Checklist
Cargo.lock
), pleaserun
dd-rust-license-tool write
to regenerate the license inventory and commit the changes (if any). More details here.References
None.