Skip to content
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

pluto: handle auth info in https_proxy #3639

Merged
merged 1 commit into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions sources/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions sources/api/pluto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ snafu = "0.7"
tokio = { version = "~1.32", default-features = false, features = ["macros", "rt-multi-thread"] } # LTS
tokio-retry = "0.3"
tokio-rustls = "0.23"
url = "2"

[build-dependencies]
bottlerocket-variant = { version = "0.1", path = "../../bottlerocket-variant" }
Expand Down
25 changes: 23 additions & 2 deletions sources/api/pluto/src/hyper_proxy/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

mod stream;
mod tunnel;
use futures_util::future::TryFutureExt;
use headers::{authorization::Credentials, Authorization, HeaderMapExt, ProxyAuthorization};
use http::header::HeaderMap;
use hyper::{service::Service, Uri};

use futures_util::future::TryFutureExt;
use std::{fmt, io, sync::Arc};
use std::{
future::Future,
Expand All @@ -27,7 +27,12 @@ type BoxError = Box<dyn std::error::Error + Send + Sync>;

/// The Intercept enum to filter connections
#[derive(Debug, Clone)]
#[allow(dead_code)]
pub enum Intercept {
/// Only https connections will go through proxy
Https,
/// No connection will go through this proxy
None,
/// A custom intercept
Custom(Custom),
}
Expand Down Expand Up @@ -84,7 +89,9 @@ impl Intercept {
/// A function to check if given `Uri` is proxied
pub fn matches<D: Dst>(&self, uri: &D) -> bool {
match (self, uri.scheme()) {
(&Intercept::Https, Some("https")) => true,
(&Intercept::Custom(Custom(ref f)), _) => f(uri.scheme(), uri.host(), uri.port()),
_ => false,
}
}
}
Expand Down Expand Up @@ -116,6 +123,20 @@ impl Proxy {
force_connect: false,
}
}

/// Set `Proxy` authorization
pub fn set_authorization<C: Credentials + Clone>(&mut self, credentials: Authorization<C>) {
match self.intercept {
Intercept::Https => {
self.headers.typed_insert(ProxyAuthorization(credentials.0));
}
_ => {
self.headers
.typed_insert(Authorization(credentials.0.clone()));
self.headers.typed_insert(ProxyAuthorization(credentials.0));
}
}
}
Comment on lines +127 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to set the authorization headers in the _ (specifically, None) case where connections aren't going through the proxy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was lifted from hyper_proxy source and it's not super clear to me either. In any case, we shouldn't be reaching this path since we're only proxying HTTPS traffic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some digging, we've determined that the Auth header here will never get sent if the proxy doesn't "match" with the destination URL. We'll go ahead and simplify this logic here so it only contains the case we care about, i.e. proxying HTTPS traffic.

}

/// A wrapper around `Proxy`s with a connector.
Expand Down
22 changes: 21 additions & 1 deletion sources/api/pluto/src/proxy.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use crate::hyper_proxy::{Proxy, ProxyConnector};
use headers::Authorization;
use hyper::Uri;
use hyper_rustls::HttpsConnectorBuilder;
use snafu::{ResultExt, Snafu};
use std::env;
use url::Url;

#[derive(Debug, Snafu)]
pub(super) enum Error {
Expand All @@ -12,6 +14,12 @@ pub(super) enum Error {
source: hyper::http::uri::InvalidUri,
},

#[snafu(display("Unable to parse '{}' as URL: {}", input, source))]
UrlParse {
input: String,
source: url::ParseError,
},

#[snafu(display("Failed to create proxy creator: {}", source))]
ProxyConnector { source: std::io::Error },
}
Expand Down Expand Up @@ -75,7 +83,19 @@ pub(crate) fn setup_http_client(
input: &https_proxy,
})?;
}
let proxy = Proxy::new(intercept, proxy_uri);
let mut proxy = Proxy::new(intercept, proxy_uri);
// Parse https_proxy as URL to extract out auth information if any
let proxy_url = Url::parse(&https_proxy).context(UrlParseSnafu {
input: &https_proxy,
})?;

if !proxy_url.username().is_empty() || proxy_url.password().is_some() {
proxy.set_authorization(Authorization::basic(
proxy_url.username(),
proxy_url.password().unwrap_or_default(),
));
}

let https_connector = HttpsConnectorBuilder::new()
.with_native_roots()
.https_or_http()
Expand Down