From 33ff1a33ab871d58c8ba721918c338203087e97e Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Wed, 20 Jun 2018 07:40:49 -1000 Subject: [PATCH] Proxy: More carefully keep track of the reason TLS isn't used. (#1164) * Proxy: More carefully keep track of the reason TLS isn't used. There is only one case where we dynamically don't know whether we'll have an identity to construct a TLS connection configuration. Refactor the code with that in mind, better documenting all the reasons why an identity isn't available. Signed-off-by: Brian Smith --- proxy/benches/record.rs | 4 +- proxy/src/bind.rs | 17 +++++++- proxy/src/conditional.rs | 13 +++++++ proxy/src/control/destination/background.rs | 17 +++++--- proxy/src/control/destination/endpoint.rs | 3 +- proxy/src/control/destination/mod.rs | 12 ++++-- proxy/src/ctx/http.rs | 3 +- proxy/src/ctx/mod.rs | 5 ++- proxy/src/ctx/transport.rs | 2 +- proxy/src/lib.rs | 9 ++++- proxy/src/telemetry/metrics/labels.rs | 3 +- proxy/src/transparency/tcp.rs | 2 +- proxy/src/transport/connect.rs | 6 ++- proxy/src/transport/tls/config.rs | 43 +++++++++++++++------ proxy/src/transport/tls/mod.rs | 1 + 15 files changed, 106 insertions(+), 34 deletions(-) diff --git a/proxy/benches/record.rs b/proxy/benches/record.rs index 38a99eeca..e52f58183 100644 --- a/proxy/benches/record.rs +++ b/proxy/benches/record.rs @@ -49,7 +49,9 @@ where ctx::transport::Client::new( &proxy, &addr(), - destination::Metadata::new(metrics::DstLabels::new(labels), None), + destination::Metadata::new( + metrics::DstLabels::new(labels), + Conditional::None(tls::ReasonForNoIdentity::NotProvidedByServiceDiscovery)), TLS_DISABLED, ) } diff --git a/proxy/src/bind.rs b/proxy/src/bind.rs index 2120bf69d..28bbc156a 100644 --- a/proxy/src/bind.rs +++ b/proxy/src/bind.rs @@ -18,6 +18,7 @@ use transparency::{self, HttpBody, h1}; use transport; use tls; use ctx::transport::TlsStatus; +use conditional::Conditional; /// Binds a `Service` from a `SocketAddr`. /// @@ -208,8 +209,20 @@ where debug!("bind_stack endpoint={:?}, protocol={:?}", ep, protocol); let addr = ep.address(); - let tls = tls::current_connection_config(ep.tls_identity(), - &self.ctx.tls_client_config_watch()); + // Like `tls::current_connection_config()` with optional identity. + let tls = match ep.tls_identity() { + Conditional::Some(identity) => { + match *self.ctx.tls_client_config_watch().borrow() { + Some(ref config) => + Conditional::Some(tls::ConnectionConfig { + identity: identity.clone(), + config: config.clone() + }), + None => Conditional::None(tls::ReasonForNoTls::NoConfig), + } + }, + Conditional::None(why_no_identity) => Conditional::None(why_no_identity.into()), + }; let client_ctx = ctx::transport::Client::new( &self.ctx, diff --git a/proxy/src/conditional.rs b/proxy/src/conditional.rs index d2715dbd9..55c14d54e 100644 --- a/proxy/src/conditional.rs +++ b/proxy/src/conditional.rs @@ -53,3 +53,16 @@ where } } } + +impl Conditional +where + C: Clone + std::fmt::Debug, + R: Copy + Clone + std::fmt::Debug, +{ + pub fn as_ref<'a>(&'a self) -> Conditional<&'a C, R> { + match self { + Conditional::Some(c) => Conditional::Some(&c), + Conditional::None(r) => Conditional::None(*r), + } + } +} diff --git a/proxy/src/control/destination/background.rs b/proxy/src/control/destination/background.rs index cba981577..02394c117 100644 --- a/proxy/src/control/destination/background.rs +++ b/proxy/src/control/destination/background.rs @@ -40,6 +40,7 @@ use telemetry::metrics::DstLabels; use transport::{DnsNameAndPort, HostAndPort, LookupAddressAndConnect}; use timeout::Timeout; use transport::tls; +use conditional::Conditional; type DestinationServiceQuery = Remote; type UpdateRx = Receiver; @@ -78,6 +79,7 @@ pub(super) fn task( dns_resolver: dns::Resolver, namespaces: Namespaces, host_and_port: Option, + controller_tls: tls::ConditionalConnectionConfig ) -> impl Future { // Build up the Controller Client Stack @@ -85,7 +87,8 @@ pub(super) fn task( let scheme = http::uri::Scheme::from_shared(Bytes::from_static(b"http")).unwrap(); let authority = http::uri::Authority::from(&host_and_port); let connect = Timeout::new( - LookupAddressAndConnect::new(host_and_port.clone(), dns_resolver.clone()), + LookupAddressAndConnect::new(host_and_port.clone(), dns_resolver.clone(), + controller_tls), Duration::from_secs(3), ); @@ -581,18 +584,22 @@ fn pb_to_addr_meta( .collect::>(); labels.sort_by(|(k0, _), (k1, _)| k0.cmp(k1)); - let tls_identity = pb.tls_identity.and_then(|pb| { + let mut tls_identity = + Conditional::None(tls::ReasonForNoIdentity::NotProvidedByServiceDiscovery); + if let Some(pb) = pb.tls_identity { match tls::Identity::maybe_from_protobuf(tls_controller_namespace, pb) { - Ok(maybe_tls) => maybe_tls, + Ok(Some(identity)) => { + tls_identity = Conditional::Some(identity); + }, + Ok(None) => (), Err(e) => { error!("Failed to parse TLS identity: {:?}", e); // XXX: Wallpaper over the error and keep going without TLS. // TODO: Hard fail here once the TLS infrastructure has been // validated. - None }, } - }); + }; let meta = Metadata::new(DstLabels::new(labels.into_iter()), tls_identity); Some((addr, meta)) diff --git a/proxy/src/control/destination/endpoint.rs b/proxy/src/control/destination/endpoint.rs index 7981d60a3..6d45b0bbe 100644 --- a/proxy/src/control/destination/endpoint.rs +++ b/proxy/src/control/destination/endpoint.rs @@ -3,6 +3,7 @@ use std::net::SocketAddr; use telemetry::metrics::DstLabels; use super::Metadata; use tls; +use conditional::Conditional; /// An individual traffic target. /// @@ -35,7 +36,7 @@ impl Endpoint { self.metadata.dst_labels() } - pub fn tls_identity(&self) -> Option<&tls::Identity> { + pub fn tls_identity(&self) -> Conditional<&tls::Identity, tls::ReasonForNoIdentity> { self.metadata.tls_identity() } } diff --git a/proxy/src/control/destination/mod.rs b/proxy/src/control/destination/mod.rs index 733cc4e82..bfa452e94 100644 --- a/proxy/src/control/destination/mod.rs +++ b/proxy/src/control/destination/mod.rs @@ -48,6 +48,7 @@ mod endpoint; pub use self::endpoint::Endpoint; use config::Namespaces; +use conditional::Conditional; /// A handle to request resolutions from the background discovery task. #[derive(Clone, Debug)] @@ -95,7 +96,7 @@ pub struct Metadata { dst_labels: Option, /// How to verify TLS for the endpoint. - tls_identity: Option, + tls_identity: Conditional, } @@ -143,6 +144,7 @@ pub fn new( dns_resolver: dns::Resolver, namespaces: Namespaces, host_and_port: Option, + controller_tls: tls::ConditionalConnectionConfig, ) -> (Resolver, impl Future) { let (request_tx, rx) = mpsc::unbounded(); let disco = Resolver { request_tx }; @@ -151,6 +153,7 @@ pub fn new( dns_resolver, namespaces, host_and_port, + controller_tls, ); (disco, bg) } @@ -240,13 +243,14 @@ impl Metadata { Metadata { dst_labels: None, // If we have no metadata on an endpoint, assume it does not support TLS. - tls_identity: None, + tls_identity: + Conditional::None(tls::ReasonForNoIdentity::NotProvidedByServiceDiscovery), } } pub fn new( dst_labels: Option, - tls_identity: Option + tls_identity: Conditional ) -> Self { Metadata { dst_labels, @@ -259,7 +263,7 @@ impl Metadata { self.dst_labels.as_ref() } - pub fn tls_identity(&self) -> Option<&tls::Identity> { + pub fn tls_identity(&self) -> Conditional<&tls::Identity, tls::ReasonForNoIdentity> { self.tls_identity.as_ref() } } diff --git a/proxy/src/ctx/http.rs b/proxy/src/ctx/http.rs index 92616fc22..e2bae59e3 100644 --- a/proxy/src/ctx/http.rs +++ b/proxy/src/ctx/http.rs @@ -5,6 +5,7 @@ use ctx; use telemetry::metrics::DstLabels; use std::sync::atomic::Ordering; use transport::tls; +use conditional::Conditional; /// A `RequestId` can be mapped to a `u64`. No `RequestId`s will map to the @@ -76,7 +77,7 @@ impl Request { Arc::new(r) } - pub fn tls_identity(&self) -> Option<&tls::Identity> { + pub fn tls_identity(&self) -> Conditional<&tls::Identity, tls::ReasonForNoIdentity> { self.client.tls_identity() } diff --git a/proxy/src/ctx/mod.rs b/proxy/src/ctx/mod.rs index 8dde49599..76dbe024b 100644 --- a/proxy/src/ctx/mod.rs +++ b/proxy/src/ctx/mod.rs @@ -100,6 +100,8 @@ pub mod test_util { use ctx; use control::destination; use telemetry::metrics::DstLabels; + use tls; + use conditional::Conditional; fn addr() -> SocketAddr { ([1, 2, 3, 4], 5678).into() @@ -125,7 +127,8 @@ pub mod test_util { L: IntoIterator, S: fmt::Display, { - let meta = destination::Metadata::new(DstLabels::new(labels), None); + let meta = destination::Metadata::new(DstLabels::new(labels), + Conditional::None(tls::ReasonForNoIdentity::NotProvidedByServiceDiscovery)); ctx::transport::Client::new(&proxy, &addr(), meta, tls) } diff --git a/proxy/src/ctx/transport.rs b/proxy/src/ctx/transport.rs index b3b0be16b..47c46bb1d 100644 --- a/proxy/src/ctx/transport.rs +++ b/proxy/src/ctx/transport.rs @@ -126,7 +126,7 @@ impl Client { Arc::new(c) } - pub fn tls_identity(&self) -> Option<&tls::Identity> { + pub fn tls_identity(&self) -> Conditional<&tls::Identity, tls::ReasonForNoIdentity> { self.metadata.tls_identity() } diff --git a/proxy/src/lib.rs b/proxy/src/lib.rs index 04b891edf..d5cbb9ac9 100644 --- a/proxy/src/lib.rs +++ b/proxy/src/lib.rs @@ -97,6 +97,7 @@ use task::MainRuntime; use transparency::{HttpBody, Server}; pub use transport::{AddrInfo, GetOriginalDst, SoOriginalDst, tls}; use outbound::Outbound; +use conditional::Conditional; /// Runs a sidecar proxy. /// @@ -183,7 +184,7 @@ where let (tls_client_config, tls_server_config, tls_cfg_bg) = tls::watch_for_config_changes(self.config.tls_settings.as_ref()); - let process_ctx = ctx::Process::new(&self.config, tls_client_config); + let process_ctx = ctx::Process::new(&self.config, tls_client_config.clone()); let Main { config, @@ -225,6 +226,9 @@ where &taps, ); + let controller_tls = + Conditional::None(tls::ReasonForNoIdentity::NotImplementedForController.into()); // TODO + let (dns_resolver, dns_bg) = dns::Resolver::from_system_config_and_env(&config) .unwrap_or_else(|e| { // TODO: Make DNS configuration infallible. @@ -234,7 +238,8 @@ where let (resolver, resolver_bg) = control::destination::new( dns_resolver.clone(), config.namespaces.clone(), - control_host_and_port + control_host_and_port, + controller_tls, ); let (drain_tx, drain_rx) = drain::channel(); diff --git a/proxy/src/telemetry/metrics/labels.rs b/proxy/src/telemetry/metrics/labels.rs index 83610b664..223eb308e 100644 --- a/proxy/src/telemetry/metrics/labels.rs +++ b/proxy/src/telemetry/metrics/labels.rs @@ -382,8 +382,7 @@ impl fmt::Display for ctx::transport::TlsStatus { Conditional::Some(()) => f.pad(",tls=\"true\""), Conditional::None(tls::ReasonForNoTls::NoConfig) => f.pad(",tls=\"no_config\""), Conditional::None(tls::ReasonForNoTls::Disabled) | - Conditional::None(tls::ReasonForNoTls::NotImplementedForNonHttp) | - Conditional::None(tls::ReasonForNoTls::NotImplementedForControlPlane) => Ok(()), + Conditional::None(tls::ReasonForNoTls::NoIdentity(_)) => Ok(()), } } } diff --git a/proxy/src/transparency/tcp.rs b/proxy/src/transparency/tcp.rs index e9dc785d0..a28bd0ceb 100644 --- a/proxy/src/transparency/tcp.rs +++ b/proxy/src/transparency/tcp.rs @@ -60,7 +60,7 @@ impl Proxy { return future::Either::B(future::ok(())); }; - let tls = Conditional::None(tls::ReasonForNoTls::NotImplementedForNonHttp); // TODO + let tls = Conditional::None(tls::ReasonForNoIdentity::NotHttp.into()); // TODO let client_ctx = ClientCtx::new( &srv_ctx.proxy, diff --git a/proxy/src/transport/connect.rs b/proxy/src/transport/connect.rs index 875029d40..da45e3da1 100644 --- a/proxy/src/transport/connect.rs +++ b/proxy/src/transport/connect.rs @@ -11,7 +11,6 @@ use connection; use convert::TryFrom; use dns; use transport::tls; -use conditional::Conditional; #[derive(Debug, Clone)] pub struct Connect { @@ -51,6 +50,7 @@ pub enum HostAndPortError { pub struct LookupAddressAndConnect { host_and_port: HostAndPort, dns_resolver: dns::Resolver, + tls: tls::ConditionalConnectionConfig, } // ===== impl HostAndPort ===== @@ -129,10 +129,12 @@ impl LookupAddressAndConnect { pub fn new( host_and_port: HostAndPort, dns_resolver: dns::Resolver, + tls: tls::ConditionalConnectionConfig, ) -> Self { Self { host_and_port, dns_resolver, + tls, } } } @@ -145,7 +147,7 @@ impl tokio_connect::Connect for LookupAddressAndConnect { fn connect(&self) -> Self::Future { let port = self.host_and_port.port; let host = self.host_and_port.host.clone(); - let tls = Conditional::None(tls::ReasonForNoTls::NotImplementedForControlPlane); // TODO + let tls = tls::current_connection_config(&self.tls); let c = self.dns_resolver .resolve_one_ip(&self.host_and_port.host) .map_err(|_| { diff --git a/proxy/src/transport/tls/config.rs b/proxy/src/transport/tls/config.rs index acfbbf9ef..469afaa4f 100644 --- a/proxy/src/transport/tls/config.rs +++ b/proxy/src/transport/tls/config.rs @@ -91,13 +91,34 @@ pub enum ReasonForNoTls { /// TLS was enabled but the configuration isn't available (yet). NoConfig, - /// TLS isn't implemented for the connection between the proxy and the - /// control plane yet. - NotImplementedForControlPlane, + /// The endpoint's TLS identity is unknown. Without knowing its identity + /// we can't validate its certificate. + NoIdentity(ReasonForNoIdentity), +} - /// TLS is only enabled for HTTP (HTTPS) right now. - NotImplementedForNonHttp, +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub enum ReasonForNoIdentity { + /// The connection is a non-HTTP connection so we don't know anything + /// about the destination besides its address. + NotHttp, + /// The connection is for HTTP but the HTTP request doesn't have an + /// authority so we can't extract the identity from it. + NoAuthorityInHttpRequest, + + /// The destination service didn't give us the identity, which is its way + /// of telling us that we shouldn't do TLS for this endpoint. + NotProvidedByServiceDiscovery, + + /// We haven't implemented the mechanism to construct a TLS identity for + /// the controller yet. + NotImplementedForController, +} + +impl From for ReasonForNoTls { + fn from(r: ReasonForNoIdentity) -> Self { + ReasonForNoTls::NoIdentity(r) + } } pub type ConditionalConnectionConfig = Conditional, ReasonForNoTls>; @@ -318,21 +339,21 @@ impl ServerConfig { } } -pub fn current_connection_config(identity: Option<&Identity>, watch: &Watch>) +pub fn current_connection_config(watch: &ConditionalConnectionConfig>>) -> ConditionalConnectionConfig where C: Clone + std::fmt::Debug { - match identity { - Some(identity) => { - match *watch.borrow() { + match watch { + Conditional::Some(c) => { + match *c.config.borrow() { Some(ref config) => Conditional::Some(ConnectionConfig { - identity: identity.clone(), + identity: c.identity.clone(), config: config.clone() }), None => Conditional::None(ReasonForNoTls::NoConfig), } }, - None => Conditional::None(ReasonForNoTls::Disabled), + Conditional::None(r) => Conditional::None(*r), } } diff --git a/proxy/src/transport/tls/mod.rs b/proxy/src/transport/tls/mod.rs index 411948877..6f4c5a3c6 100755 --- a/proxy/src/transport/tls/mod.rs +++ b/proxy/src/transport/tls/mod.rs @@ -18,6 +18,7 @@ pub use self::{ ConditionalConnectionConfig, ConnectionConfig, ReasonForNoTls, + ReasonForNoIdentity, ServerConfig, ServerConfigWatch, current_connection_config,