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 <brian@briansmith.org>
linkerd · Jun 20, 2018 · 45191f8 · 45191f8
1 parent 8ece9c4
commit 45191f8
Show file tree

Hide file tree

Showing 14 changed files with 101 additions and 34 deletions.
diff --git 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,21 @@ 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(tls::ReasonForNoTls::NoIdentity(why_no_identity)),
+        };
 
         let client_ctx = ctx::transport::Client::new(
             &self.ctx,

diff --git a/proxy/src/conditional.rs b/proxy/src/conditional.rs
@@ -53,3 +53,16 @@ where
         }
     }
 }
+
+impl<C, R> Conditional<C, R>
+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
@@ -40,6 +40,7 @@ use telemetry::metrics::DstLabels;
 use transport::{DnsNameAndPort, HostAndPort, LookupAddressAndConnect};
 use timeout::Timeout;
 use transport::tls;
+use conditional::Conditional;
 
 type DestinationServiceQuery<T> = Remote<PbUpdate, T>;
 type UpdateRx<T> = Receiver<PbUpdate, T>;
@@ -78,14 +79,16 @@ pub(super) fn task(
     dns_resolver: dns::Resolver,
     namespaces: Namespaces,
     host_and_port: Option<HostAndPort>,
+    controller_tls: tls::ConditionalConnectionConfig<tls::ClientConfigWatch>
 ) -> impl Future<Item = (), Error = ()>
 {
     // Build up the Controller Client Stack
     let mut client = host_and_port.map(|host_and_port| {
         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::<Vec<_>>();
     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
@@ -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
@@ -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<DstLabels>,
 
     /// How to verify TLS for the endpoint.
-    tls_identity: Option<tls::Identity>,
+    tls_identity: Conditional<tls::Identity, tls::ReasonForNoIdentity>,
 }
 
 
@@ -143,6 +144,7 @@ pub fn new(
     dns_resolver: dns::Resolver,
     namespaces: Namespaces,
     host_and_port: Option<HostAndPort>,
+    controller_tls: tls::ConditionalConnectionConfig<tls::ClientConfigWatch>,
 ) -> (Resolver, impl Future<Item = (), Error = ()>) {
     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<DstLabels>,
-        tls_identity: Option<tls::Identity>
+        tls_identity: Conditional<tls::Identity, tls::ReasonForNoIdentity>
     ) -> 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
@@ -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
@@ -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<Item=(S, S)>,
         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
@@ -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
@@ -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,10 @@ where
             &taps,
         );
 
+        let controller_tls =
+                Conditional::None(tls::ReasonForNoTls::NoIdentity(
+                    tls::ReasonForNoIdentity::NotImplementedForController)); // 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 +239,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
@@ -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
@@ -60,7 +60,8 @@ impl Proxy {
             return future::Either::B(future::ok(()));
         };
 
-        let tls = Conditional::None(tls::ReasonForNoTls::NotImplementedForNonHttp); // TODO
+        let tls = Conditional::None(
+            tls::ReasonForNoTls::NoIdentity(tls::ReasonForNoIdentity::NotHttp)); // TODO
 
         let client_ctx = ClientCtx::new(
             &srv_ctx.proxy,

diff --git 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<tls::ClientConfigWatch>,
 }
 
 // ===== impl HostAndPort =====
@@ -129,10 +129,12 @@ impl LookupAddressAndConnect {
     pub fn new(
         host_and_port: HostAndPort,
         dns_resolver: dns::Resolver,
+        tls: tls::ConditionalConnectionConfig<tls::ClientConfigWatch>,
     ) -> 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
@@ -91,13 +91,28 @@ 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,
-
-    /// TLS is only enabled for HTTP (HTTPS) right now.
-    NotImplementedForNonHttp,
+    /// The endpoint's TLS identity is unknown. Without knowing its identity
+    /// we can't validate its certificate.
+    NoIdentity(ReasonForNoIdentity),
+}
 
+#[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,
 }
 
 pub type ConditionalConnectionConfig<C> = Conditional<ConnectionConfig<C>, ReasonForNoTls>;
@@ -318,21 +333,21 @@ impl ServerConfig {
     }
 }
 
-pub fn current_connection_config<C>(identity: Option<&Identity>, watch: &Watch<Option<C>>)
+pub fn current_connection_config<C>(watch: &ConditionalConnectionConfig<Watch<Option<C>>>)
     -> ConditionalConnectionConfig<C> 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
@@ -18,6 +18,7 @@ pub use self::{
         ConditionalConnectionConfig,
         ConnectionConfig,
         ReasonForNoTls,
+        ReasonForNoIdentity,
         ServerConfig,
         ServerConfigWatch,
         current_connection_config,