-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Proxy: Add TLS client infrastructure. #1158
Changes from 5 commits
592a976
677e28b
57dc99a
b90eff6
9fa7d6d
ba9d2e6
d90dad9
548e28c
b005c80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
use std; | ||
|
||
/// Like `std::option::Option<C>` but `None` carries a reason why the value | ||
/// isn't available. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From that description, this basically sounds like a I understand why you didn't want to use a type which implies one case is an "error" here, though... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also don't like the introduction of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think "a reason we were not able to do TLS" falls into a (sufficiently broad) definition of "error";
I would be fine with that as well, but (coming at it from another direction) I think |
||
#[derive(Clone, Debug)] | ||
pub enum Conditional<C, R> where | ||
C: Clone + std::fmt::Debug, | ||
R: Clone + std::fmt::Debug | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't block this review, but the formatting used here is used nowhere else in the project as far as i can tell. We already have a smattering of styles, and I'd prefer not to introduce an N+1th form. Perhaps just run rustfmt over the file before merging? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did the rustfmt. |
||
{ | ||
Some(C), | ||
None(R) | ||
} | ||
|
||
impl<C, R> Copy for Conditional<C, R> where | ||
C: Copy + Clone + std::fmt::Debug, | ||
R: Copy + Clone + std::fmt::Debug {} | ||
|
||
impl<C, R> Eq for Conditional<C, R> where | ||
C: Eq + Clone + std::fmt::Debug, | ||
R: Eq + Clone + std::fmt::Debug {} | ||
|
||
impl<C, R> PartialEq for Conditional<C, R> where | ||
C: PartialEq + Clone + std::fmt::Debug, | ||
R: PartialEq + Clone + std::fmt::Debug { | ||
fn eq(&self, other: &Conditional<C, R>) -> bool { | ||
use self::Conditional::*; | ||
match (self, other) { | ||
(Some(a), Some(b)) => a.eq(b), | ||
(None(a), None(b)) => a.eq(b), | ||
_ => false, | ||
} | ||
} | ||
} | ||
|
||
impl<C, R> std::hash::Hash for Conditional<C, R> where | ||
C: std::hash::Hash + Clone + std::fmt::Debug, | ||
R: std::hash::Hash + Clone + std::fmt::Debug { | ||
fn hash<H: std::hash::Hasher>(&self, state: &mut H) { | ||
match self { | ||
Conditional::Some(c) => c.hash(state), | ||
Conditional::None(r) => r.hash(state), | ||
} | ||
} | ||
} | ||
|
||
impl<C, R> Conditional<C, R> where | ||
C: Clone + std::fmt::Debug, | ||
R: Copy + Clone + std::fmt::Debug | ||
{ | ||
pub fn to_empty(&self) -> Conditional<(), R> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tioli, but this would be more clearly (to me) named as |
||
match self { | ||
Conditional::Some(_) => Conditional::Some(()), | ||
Conditional::None(r) => Conditional::None(*r), | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,32 +9,34 @@ use tokio::{ | |
net::{TcpListener, TcpStream, ConnectFuture}, | ||
reactor::Handle, | ||
}; | ||
|
||
use conditional::Conditional; | ||
use ctx::transport::TlsStatus; | ||
use config::Addr; | ||
use transport::{GetOriginalDst, Io, tls}; | ||
|
||
pub type PlaintextSocket = TcpStream; | ||
|
||
pub struct BoundPort { | ||
inner: std::net::TcpListener, | ||
local_addr: SocketAddr, | ||
} | ||
|
||
/// Initiates a client connection to the given address. | ||
pub fn connect(addr: &SocketAddr) -> Connecting { | ||
Connecting { | ||
inner: PlaintextSocket::connect(addr), | ||
// TODO: when we can open TLS client connections, this is where we will | ||
// indicate that for telemetry. | ||
tls_status: TlsStatus::Disabled, | ||
pub fn connect(addr: &SocketAddr, | ||
tls: tls::ConditionalConnectionConfig<tls::ClientConfig>) | ||
-> Connecting | ||
{ | ||
Connecting::Plaintext { | ||
connect: TcpStream::connect(addr), | ||
tls: Some(tls), | ||
} | ||
} | ||
|
||
/// A socket that is in the process of connecting. | ||
pub struct Connecting { | ||
inner: ConnectFuture, | ||
tls_status: TlsStatus, | ||
pub enum Connecting { | ||
Plaintext { | ||
connect: ConnectFuture, | ||
tls: Option<tls::ConditionalConnectionConfig<tls::ClientConfig>>, | ||
}, | ||
UpgradeToTls(tls::UpgradeClientToTls), | ||
} | ||
|
||
/// Abstracts a plaintext socket vs. a TLS decorated one. | ||
|
@@ -138,7 +140,7 @@ impl BoundPort { | |
// libraries don't have the necessary API for that, so just | ||
// do it here. | ||
set_nodelay_or_warn(&socket); | ||
let tls_status = if let Some((_identity, config_watch)) = &tls { | ||
let why_no_tls = if let Some((_identity, config_watch)) = &tls { | ||
// TODO: use `identity` to differentiate between TLS | ||
// that the proxy should terminate vs. TLS that should | ||
// be passed through. | ||
|
@@ -150,12 +152,12 @@ impl BoundPort { | |
return Either::A(f); | ||
} else { | ||
// No valid TLS configuration. | ||
TlsStatus::NoConfig | ||
tls::ReasonForNoTls::NoConfig | ||
} | ||
} else { | ||
TlsStatus::Disabled | ||
tls::ReasonForNoTls::Disabled | ||
}; | ||
let conn = Connection::new(socket, tls_status); | ||
let conn = Connection::plain(socket, why_no_tls); | ||
Either::B(future::ok((conn, remote_addr))) | ||
}) | ||
.then(|r| { | ||
|
@@ -181,28 +183,50 @@ impl Future for Connecting { | |
type Error = io::Error; | ||
|
||
fn poll(&mut self) -> Poll<Self::Item, Self::Error> { | ||
let socket = try_ready!(self.inner.poll()); | ||
set_nodelay_or_warn(&socket); | ||
Ok(Async::Ready(Connection::new(socket, self.tls_status))) | ||
loop { | ||
let new_state = match self { | ||
Connecting::Plaintext { connect, tls } => { | ||
let plaintext_stream = try_ready!(connect.poll()); | ||
set_nodelay_or_warn(&plaintext_stream); | ||
match tls.take().expect("Polled after ready") { | ||
Conditional::Some(config) => { | ||
let upgrade_to_tls = tls::Connection::connect( | ||
plaintext_stream, &config.identity, config.config); | ||
Some(Connecting::UpgradeToTls(upgrade_to_tls)) | ||
}, | ||
Conditional::None(why) => { | ||
return Ok(Async::Ready(Connection::plain(plaintext_stream, why))); | ||
}, | ||
} | ||
}, | ||
Connecting::UpgradeToTls(upgrading) => { | ||
let tls_stream = try_ready!(upgrading.poll()); | ||
return Ok(Async::Ready(Connection::tls(tls_stream))); | ||
}, | ||
}; | ||
if let Some(s) = new_state { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the case where a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't find any case where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. originally I had a different (broken) implementation and I didn't clean it up enough. Fixed now. |
||
std::mem::replace(self, s); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could also just be if let Some(s) = new_state {
*self = s;
} Unless there's some particular reason that didn't work in this context? |
||
} | ||
} | ||
} | ||
} | ||
|
||
// ===== impl Connection ===== | ||
|
||
impl Connection { | ||
fn new<I: Io + 'static>(io: I, tls_status: TlsStatus) -> Self { | ||
fn plain(io: TcpStream, why_no_tls: tls::ReasonForNoTls) -> Self { | ||
Connection { | ||
io: Box::new(io), | ||
peek_buf: BytesMut::new(), | ||
tls_status, | ||
tls_status: Conditional::None(why_no_tls), | ||
} | ||
} | ||
|
||
fn tls(tls: tls::Connection) -> Self { | ||
Connection { | ||
fn tls<S: tls::Session + std::fmt::Debug + 'static>(tls: tls::Connection<S>) -> Self { | ||
Connection { | ||
io: Box::new(tls), | ||
peek_buf: BytesMut::new(), | ||
tls_status: TlsStatus::Success, | ||
tls_status: Conditional::Some(()), | ||
} | ||
} | ||
|
||
|
@@ -312,7 +336,7 @@ impl<T: Peek> Future for PeekFuture<T> { | |
|
||
// Misc. | ||
|
||
fn set_nodelay_or_warn(socket: &PlaintextSocket) { | ||
fn set_nodelay_or_warn(socket: &TcpStream) { | ||
if let Err(e) = socket.set_nodelay(true) { | ||
warn!( | ||
"could not set TCP_NODELAY on {:?}/{:?}: {}", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,16 +10,20 @@ | |
use config; | ||
use std::time::SystemTime; | ||
use std::sync::Arc; | ||
use transport::tls; | ||
|
||
pub mod http; | ||
pub mod transport; | ||
|
||
/// Describes a single running proxy instance. | ||
#[derive(Clone, Debug, PartialEq, Eq)] | ||
#[derive(Clone, Debug)] | ||
pub struct Process { | ||
/// Identifies the Kubernetes namespace in which this proxy is process. | ||
pub scheduled_namespace: String, | ||
|
||
pub start_time: SystemTime, | ||
|
||
tls_client_config: tls::ClientConfigWatch, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if the process context is the right place to put configurations. As per @olix0r in #1050 (comment), the On the other hand, I'm not sure where else we'd put this, besides passing it around everywhere... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While it's true that this was initially intended to be "just the metadata" -- every ctx object throughout the proxy ends up with a reference back to this thing -- i don't yet have an opinion on whether there are better approaches for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right. If we don't put it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. at the very least, let's not let this block things There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, that's fair, I just wanted to bring it up since Oliver mentioned something similar while reviewing my PR. |
||
} | ||
|
||
/// Indicates the orientation of traffic, relative to a sidecar proxy. | ||
|
@@ -29,27 +33,30 @@ pub struct Process { | |
/// local instance. | ||
/// - The _outbound_ proxy receives traffic from the local instance and forwards it to a | ||
/// remove service. | ||
#[derive(Clone, Debug, PartialEq, Eq)] | ||
#[derive(Clone, Debug)] | ||
pub enum Proxy { | ||
Inbound(Arc<Process>), | ||
Outbound(Arc<Process>), | ||
} | ||
|
||
impl Process { | ||
#[cfg(test)] | ||
// Test-only, but we can't use `#[cfg(test)]` because it is used by the | ||
// benchmarks | ||
pub fn test(ns: &str) -> Arc<Self> { | ||
Arc::new(Self { | ||
scheduled_namespace: ns.into(), | ||
start_time: SystemTime::now(), | ||
tls_client_config: tls::ClientConfig::no_tls(), | ||
}) | ||
} | ||
|
||
/// Construct a new `Process` from environment variables. | ||
pub fn new(config: &config::Config) -> Arc<Self> { | ||
pub fn new(config: &config::Config, tls_client_config: tls::ClientConfigWatch) -> Arc<Self> { | ||
let start_time = SystemTime::now(); | ||
Arc::new(Self { | ||
scheduled_namespace: config.namespaces.pod.clone(), | ||
start_time, | ||
tls_client_config, | ||
}) | ||
} | ||
} | ||
|
@@ -73,6 +80,12 @@ impl Proxy { | |
pub fn is_outbound(&self) -> bool { | ||
!self.is_inbound() | ||
} | ||
|
||
pub fn tls_client_config_watch(&self) -> &tls::ClientConfigWatch { | ||
match self { | ||
Proxy::Inbound(process) | Proxy::Outbound(process) => &process.tls_client_config | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
@@ -82,7 +95,6 @@ pub mod test_util { | |
fmt, | ||
net::SocketAddr, | ||
sync::Arc, | ||
time::SystemTime, | ||
}; | ||
|
||
use ctx; | ||
|
@@ -94,10 +106,7 @@ pub mod test_util { | |
} | ||
|
||
pub fn process() -> Arc<ctx::Process> { | ||
Arc::new(ctx::Process { | ||
scheduled_namespace: "test".into(), | ||
start_time: SystemTime::now(), | ||
}) | ||
ctx::Process::test("test") | ||
} | ||
|
||
pub fn server( | ||
|
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 subtle enough that it probably warrants a comment -- iiuc we don't actually pass the tls configuration into the client, since it's not needed (we only need to indicate the presence of TLS as a boolean). It's only the Connect impl below that actually needs the TLS config?
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 now recast as
TlsStatus::from(&tls),
;TlsStatus::from
replaces theto_empty()
function.