Skip to content

fix(auth): Vector does not put the Proxy-Authorization header on the wire (#17353) #17363

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.

Sign up for GitHub

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

Merged
jszwedko merged 1 commit into from
Jun 20, 2023
Merged

fix(auth): Vector does not put the Proxy-Authorization header on the wire (#17353) #17363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions lib/vector-core/src/config/proxy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,12 @@ impl ProxyConfig {
mod tests {
use base64::prelude::{Engine as _, BASE64_STANDARD};
use env_test_util::TempEnvVar;
use http::{HeaderValue, Uri};
use http::{
header::{AUTHORIZATION, PROXY_AUTHORIZATION},
HeaderName, HeaderValue, Uri,
};

const PROXY_HEADERS: [HeaderName; 2] = [AUTHORIZATION, PROXY_AUTHORIZATION];

use super::*;

Expand Down Expand Up @@ -341,20 +346,18 @@ mod tests {
Some(first.uri()),
Uri::try_from("http://user:pass@1.2.3.4:5678").as_ref().ok()
);
assert_eq!(
first.headers().get("authorization"),
expected_header_value.as_ref().ok()
);
for h in &PROXY_HEADERS {
assert_eq!(first.headers().get(h), expected_header_value.as_ref().ok());
}
assert_eq!(
Some(second.uri()),
Uri::try_from("https://user:pass@2.3.4.5:9876")
.as_ref()
.ok()
);
assert_eq!(
second.headers().get("authorization"),
expected_header_value.as_ref().ok()
);
for h in &PROXY_HEADERS {
assert_eq!(second.headers().get(h), expected_header_value.as_ref().ok());
}
}

#[ignore]
Expand All @@ -371,10 +374,8 @@ mod tests {
.expect("should not be None");
let encoded_header = format!("Basic {}", BASE64_STANDARD.encode("user:P@ssw0rd"));
let expected_header_value = HeaderValue::from_str(encoded_header.as_str());

assert_eq!(
first.headers().get("authorization"),
expected_header_value.as_ref().ok()
);
for h in &PROXY_HEADERS {
assert_eq!(first.headers().get(h), expected_header_value.as_ref().ok());
}
}
}
27 changes: 23 additions & 4 deletions src/http.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,12 @@ impl HttpError {
}

pub type HttpClientFuture = <HttpClient as Service<http::Request<Body>>>::Future;
type HttpProxyConnector = ProxyConnector<HttpsConnector<HttpConnector>>;

pub struct HttpClient<B = Body> {
client: Client<ProxyConnector<HttpsConnector<HttpConnector>>, B>,
client: Client<HttpProxyConnector, B>,
user_agent: HeaderValue,
proxy_connector: HttpProxyConnector,
}

impl<B> HttpClient<B>
Expand All @@ -77,14 +79,18 @@ where
proxy_config: &ProxyConfig,
client_builder: &mut client::Builder,
) -> Result<HttpClient<B>, HttpError> {
let proxy = build_proxy_connector(tls_settings.into(), proxy_config)?;
let client = client_builder.build(proxy);
let proxy_connector = build_proxy_connector(tls_settings.into(), proxy_config)?;
let client = client_builder.build(proxy_connector.clone());

let version = crate::get_version();
let user_agent = HeaderValue::from_str(&format!("Vector/{}", version))
.expect("Invalid header value for version!");

Ok(HttpClient { client, user_agent })
Ok(HttpClient {
client,
user_agent,
proxy_connector,
})
}

pub fn send(
Expand All @@ -95,6 +101,7 @@ where
let _enter = span.enter();

default_request_headers(&mut request, &self.user_agent);
self.maybe_add_proxy_headers(&mut request);

emit!(http_client::AboutToSendHttpRequest { request: &request });

Expand Down Expand Up @@ -135,6 +142,17 @@ where

Box::pin(fut)
}

fn maybe_add_proxy_headers(&self, request: &mut Request<B>) {
if let Some(proxy_headers) = self.proxy_connector.http_headers(request.uri()) {
for (k, v) in proxy_headers {
let request_headers = request.headers_mut();
Copy link
Member

@bruceg bruceg Jun 19, 2023

Choose a reason for hiding this comment

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

I suggest moving this out of the loop, to avoid calling it repeatedly.

Copy link
Contributor Author

@syedriko syedriko Jun 20, 2023

Choose a reason for hiding this comment

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

Absolutely, my oversight.

if !request_headers.contains_key(k) {
Copy link
Member

@bruceg bruceg Jun 19, 2023

Choose a reason for hiding this comment

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

Do we need to check if it exists, or can we just insert the new header? HTTP headers can contain multiple values.

Copy link
Contributor Author

@syedriko syedriko Jun 20, 2023

Choose a reason for hiding this comment

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

I'm going by RFC 7235, https://github.com/hyperium/headers/blob/f01cc90cf8d601a716856bc9d29f47df92b779e4/src/common/authorization.rs#L9 and https://github.com/hyperium/headers/blob/master/src/common/proxy_authorization.rs#L3. My read of RFC 7235 is that it doesn't leave room for >1 sets of credentials in the Authorization and Proxy-Authorization headers.
The reason I added the existence check -

vector/src/http.rs

Line 293 in 53e1785

pub fn apply_headers_map(&self, map: &mut HeaderMap) {
can also set the Authorization header, which I didn't want to overwrite.

request_headers.insert(k, v.into());
}
}
}
}
}

pub fn build_proxy_connector(
Expand Down Expand Up @@ -216,6 +234,7 @@ impl<B> Clone for HttpClient<B> {
Self {
client: self.client.clone(),
user_agent: self.user_agent.clone(),
proxy_connector: self.proxy_connector.clone(),
}
}
}
Expand Down