Skip to content

Commit

Permalink
Require identity configuration (#1305)
Browse files Browse the repository at this point in the history
The proxy currently supports a mode where identity is disabled. This
proliferates complexity that isn't really needed: there doesn't appear
to be a real use case where disabling identity is necessary. And, if it
is really necessary, we should reintroduce it after decoupling TLS and
identity.

This change causes the proxy to error during startup if identity is
disabled by configuration.

Furthermore, the `linkerd-proxy-identity` crate now has a `test-util`
feature that makes it possible to build a `LocalCrtKey` identity from
credentials provided by the `linkerd-identity/test-util` feature. A
default set of credentials are used in inbound and outbound tests.
  • Loading branch information
olix0r authored Oct 8, 2021
1 parent 3bb7ec4 commit 49227fe
Show file tree
Hide file tree
Showing 52 changed files with 344 additions and 438 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,7 @@ dependencies = [
"linkerd-app-core",
"linkerd-app-test",
"linkerd-io",
"linkerd-proxy-identity",
"linkerd-server-policy",
"linkerd-tonic-watch",
"linkerd-tracing",
Expand Down Expand Up @@ -792,6 +793,7 @@ dependencies = [
"linkerd-http-retry",
"linkerd-identity",
"linkerd-io",
"linkerd-proxy-identity",
"linkerd-tracing",
"parking_lot",
"pin-project",
Expand Down
10 changes: 5 additions & 5 deletions linkerd/app/admin/src/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ struct Permitted {

#[derive(Clone)]
struct TlsParams {
identity: Option<LocalCrtKey>,
identity: LocalCrtKey,
}

const DETECT_TIMEOUT: Duration = Duration::from_secs(1);
Expand All @@ -74,7 +74,7 @@ impl Config {
self,
bind: B,
policy: impl inbound::policy::CheckPolicy,
identity: Option<LocalCrtKey>,
identity: LocalCrtKey,
report: R,
metrics: inbound::Metrics,
trace: trace::Handle,
Expand Down Expand Up @@ -153,7 +153,7 @@ impl Config {
}
})
.push(svc::ArcNewService::layer())
.push(tls::NewDetectTls::layer(TlsParams {
.push(tls::NewDetectTls::<LocalCrtKey, _, _>::layer(TlsParams {
identity,
}))
.into_inner();
Expand Down Expand Up @@ -240,9 +240,9 @@ impl<T> ExtractParam<tls::server::Timeout, T> for TlsParams {
}
}

impl<T> ExtractParam<Option<LocalCrtKey>, T> for TlsParams {
impl<T> ExtractParam<LocalCrtKey, T> for TlsParams {
#[inline]
fn extract_param(&self, _: &T) -> Option<LocalCrtKey> {
fn extract_param(&self, _: &T) -> LocalCrtKey {
self.identity.clone()
}
}
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/core/src/control.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl Config {
self,
dns: dns::Resolver,
metrics: metrics::ControlHttp,
identity: Option<L>,
identity: L,
) -> svc::ArcNewService<
(),
impl svc::Service<
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const DEFAULT_PORT: u16 = 80;

#[derive(Clone, Debug)]
pub struct ProxyRuntime {
pub identity: Option<proxy::identity::LocalCrtKey>,
pub identity: proxy::identity::LocalCrtKey,
pub metrics: metrics::Proxy,
pub tap: proxy::tap::Registry,
pub span_sink: http_tracing::OpenCensusSink,
Expand Down
13 changes: 4 additions & 9 deletions linkerd/app/gateway/src/gateway.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,11 @@ use tracing::{debug, warn};
#[derive(Clone, Debug)]
pub(crate) struct NewGateway<O> {
outbound: O,
local_id: Option<tls::LocalId>,
local_id: tls::LocalId,
}

#[derive(Clone, Debug)]
pub(crate) enum Gateway<O> {
NoIdentity,
BadDomain(dns::Name),
Outbound {
outbound: O,
Expand All @@ -37,11 +36,11 @@ pub(crate) type Target = (Option<profiles::Receiver>, HttpTarget);
// === impl NewGateway ===

impl<O> NewGateway<O> {
pub fn new(outbound: O, local_id: Option<tls::LocalId>) -> Self {
pub fn new(outbound: O, local_id: tls::LocalId) -> Self {
Self { outbound, local_id }
}

pub fn layer(local_id: Option<tls::LocalId>) -> impl layer::Layer<O, Service = Self> + Clone {
pub fn layer(local_id: tls::LocalId) -> impl layer::Layer<O, Service = Self> + Clone {
layer::mk(move |outbound| Self::new(outbound, local_id.clone()))
}
}
Expand All @@ -56,10 +55,7 @@ where
type Service = Gateway<O::Service>;

fn new_service(&self, (profile, http): Target) -> Self::Service {
let local_id = match self.local_id.clone() {
Some(id) => id,
None => return Gateway::NoIdentity,
};
let local_id = self.local_id.clone();
let profile = match profile {
Some(profile) => profile,
None => return Gateway::BadDomain(http.target.name().clone()),
Expand Down Expand Up @@ -198,7 +194,6 @@ where
tracing::debug!("Passing request to outbound");
Box::pin(outbound.call(request).map_err(Into::into))
}
Self::NoIdentity => Box::pin(future::err(GatewayIdentityRequired.into())),
Self::BadDomain(..) => Box::pin(future::err(GatewayDomainInvalid.into())),
}
}
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/gateway/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ where
dispatch_timeout,
..
} = inbound.config().proxy.clone();
let local_id = inbound.identity().map(|l| l.id().clone());
let local_id = inbound.identity().id().clone();

// For each gatewayed connection that is *not* HTTP, use the target from the
// transport header to lookup a service profile. If the profile includes a
Expand Down
4 changes: 2 additions & 2 deletions linkerd/app/gateway/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::*;
use linkerd_app_core::{
dns, identity as id, profiles, proxy::http, svc::NewService, tls, Error, NameAddr, NameMatch,
dns, profiles, proxy::http, svc::NewService, tls, Error, NameAddr, NameMatch,
};
use linkerd_app_inbound::{GatewayDomainInvalid, GatewayIdentityRequired, GatewayLoop};
use linkerd_app_test as support;
Expand Down Expand Up @@ -109,7 +109,7 @@ impl Test {
move |_: svc::Either<outbound::http::Logical, outbound::http::Endpoint>| {
outbound.clone()
},
Some(tls::LocalId(id::Name::from_str("gateway.id.test").unwrap())),
tls::LocalId("gateway.id.test".parse().unwrap()),
);

let t = HttpTarget {
Expand Down
3 changes: 2 additions & 1 deletion linkerd/app/inbound/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ libfuzzer-sys = { version = "0.4.2", features = ["arbitrary-derive"] }
hyper = { version = "0.14.13", features = ["http1", "http2"] }
linkerd-app-test = { path = "../test" }
linkerd-io = { path = "../../io", features = ["tokio-test"] }
linkerd-proxy-identity = { path = "../../proxy/identity", features = ["test-util"] }
linkerd-tracing = { path = "../../tracing", features = ["ansi"] }
tokio = { version = "1", features = ["full", "macros"] }
tokio-test = "0.4"
linkerd-tracing = { path = "../../tracing", features = ["ansi"] }
8 changes: 4 additions & 4 deletions linkerd/app/inbound/src/detect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ struct ConfigureHttpDetect;
#[derive(Clone)]
struct TlsParams {
timeout: tls::server::Timeout,
identity: Option<LocalCrtKey>,
identity: LocalCrtKey,
}

// === impl Inbound ===
Expand Down Expand Up @@ -135,7 +135,7 @@ impl<N> Inbound<N> {
.push_on_service(svc::MapTargetLayer::new(io::BoxedIo::new))
.into_inner(),
)
.push(tls::NewDetectTls::layer(TlsParams {
.push(tls::NewDetectTls::<LocalCrtKey, _, _>::layer(TlsParams {
timeout: tls::server::Timeout(detect_timeout),
identity: rt.identity.clone(),
}))
Expand Down Expand Up @@ -425,9 +425,9 @@ impl<T> svc::ExtractParam<tls::server::Timeout, T> for TlsParams {
}
}

impl<T> svc::ExtractParam<Option<LocalCrtKey>, T> for TlsParams {
impl<T> svc::ExtractParam<LocalCrtKey, T> for TlsParams {
#[inline]
fn extract_param(&self, _: &T) -> Option<LocalCrtKey> {
fn extract_param(&self, _: &T) -> LocalCrtKey {
self.identity.clone()
}
}
Expand Down
10 changes: 5 additions & 5 deletions linkerd/app/inbound/src/direct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub type GatewayIo<I> = io::EitherIo<FwdIo<I>, SensorIo<tls::server::Io<I>>>;
#[derive(Clone)]
struct TlsParams {
timeout: tls::server::Timeout,
identity: Option<WithTransportHeaderAlpn>,
identity: WithTransportHeaderAlpn,
}

impl<N> Inbound<N> {
Expand Down Expand Up @@ -186,9 +186,9 @@ impl<N> Inbound<N> {
// connection if it doesn't include an mTLS identity.
.push_request_filter(ClientInfo::try_from)
.push(svc::ArcNewService::layer())
.push(tls::NewDetectTls::layer(TlsParams {
.push(tls::NewDetectTls::<WithTransportHeaderAlpn, _, _>::layer(TlsParams {
timeout: tls::server::Timeout(detect_timeout),
identity: rt.identity.clone().map(WithTransportHeaderAlpn),
identity: WithTransportHeaderAlpn(rt.identity.clone()),
}))
.check_new_service::<T, I>()
.push_on_service(svc::BoxService::layer())
Expand Down Expand Up @@ -334,9 +334,9 @@ impl<T> ExtractParam<tls::server::Timeout, T> for TlsParams {
}
}

impl<T> ExtractParam<Option<WithTransportHeaderAlpn>, T> for TlsParams {
impl<T> ExtractParam<WithTransportHeaderAlpn, T> for TlsParams {
#[inline]
fn extract_param(&self, _: &T) -> Option<WithTransportHeaderAlpn> {
fn extract_param(&self, _: &T) -> WithTransportHeaderAlpn {
self.identity.clone()
}
}
Expand Down
6 changes: 3 additions & 3 deletions linkerd/app/inbound/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub struct Inbound<S> {
#[derive(Clone)]
struct Runtime {
metrics: Metrics,
identity: Option<LocalCrtKey>,
identity: LocalCrtKey,
tap: tap::Registry,
span_sink: OpenCensusSink,
drain: drain::Watch,
Expand Down Expand Up @@ -82,8 +82,8 @@ impl<S> Inbound<S> {
&self.config
}

pub fn identity(&self) -> Option<&LocalCrtKey> {
self.runtime.identity.as_ref()
pub fn identity(&self) -> &LocalCrtKey {
&self.runtime.identity
}

pub fn proxy_metrics(&self) -> &metrics::Proxy {
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/inbound/src/policy/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ impl Config {
self,
dns: dns::Resolver,
metrics: metrics::ControlHttp,
identity: Option<LocalCrtKey>,
identity: LocalCrtKey,
) -> Store {
match self {
Self::Fixed { default, ports } => {
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/inbound/src/test_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub fn runtime() -> (ProxyRuntime, drain::Signal) {
let (tap, _) = tap::new();
let (metrics, _) = metrics::Metrics::new(std::time::Duration::from_secs(10));
let runtime = ProxyRuntime {
identity: None,
identity: linkerd_proxy_identity::LocalCrtKey::default_for_test(),
metrics: metrics.proxy,
tap,
span_sink: None,
Expand Down
3 changes: 2 additions & 1 deletion linkerd/app/outbound/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ pin-project = "1"
hyper = { version = "0.14.13", features = ["http1", "http2"] }
linkerd-app-test = { path = "../test" }
linkerd-io = { path = "../../io", features = ["tokio-test"] }
linkerd-proxy-identity = { path = "../../proxy/identity", features = ["test-util"] }
linkerd-tracing = { path = "../../tracing", features = ["ansi"] }
tokio = { version = "1", features = ["full", "macros"] }
tokio-test = "0.4"
linkerd-tracing = { path = "../../tracing", features = ["ansi"] }
14 changes: 4 additions & 10 deletions linkerd/app/outbound/src/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ pub struct Endpoint<P> {

#[derive(Clone)]
pub struct FromMetadata {
pub identity_disabled: bool,
pub inbound_ips: Arc<HashSet<IpAddr>>,
}

Expand Down Expand Up @@ -184,15 +183,10 @@ impl<P: Copy + std::fmt::Debug> MapEndpoint<Concrete<P>, Metadata> for FromMetad
mut metadata: Metadata,
) -> Self::Out {
tracing::trace!(%addr, ?metadata, ?concrete, "Resolved endpoint");
let tls = if self.identity_disabled || self.inbound_ips.contains(&addr.ip()) {
let reason = if self.identity_disabled {
tls::NoClientTls::Disabled
} else {
metadata.clear_upgrade();
tracing::debug!(%addr, ?metadata, ?addr, ?self.inbound_ips, "Target is local");
tls::NoClientTls::Loopback
};
tls::ConditionalClientTls::None(reason)
let tls = if self.inbound_ips.contains(&addr.ip()) {
metadata.clear_upgrade();
tracing::debug!(%addr, ?metadata, ?addr, ?self.inbound_ips, "Target is local");
tls::ConditionalClientTls::None(tls::NoClientTls::Loopback)
} else {
Self::client_tls(&metadata, tls::NoClientTls::NotProvidedByServiceDiscovery)
};
Expand Down
2 changes: 0 additions & 2 deletions linkerd/app/outbound/src/http/logical.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,12 @@ impl<E> Outbound<E> {
let endpoint =
endpoint.instrument(|e: &Endpoint| debug_span!("endpoint", server.addr = %e.addr));

let identity_disabled = rt.identity.is_none();
let resolve = svc::stack(resolve.into_service())
.check_service::<ConcreteAddr>()
.push_request_filter(|c: Concrete| Ok::<_, Infallible>(c.resolve))
.push(svc::layer::mk(move |inner| {
map_endpoint::Resolve::new(
endpoint::FromMetadata {
identity_disabled,
inbound_ips: config.inbound_ips.clone(),
},
inner,
Expand Down
8 changes: 2 additions & 6 deletions linkerd/app/outbound/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub struct Outbound<S> {
#[derive(Clone, Debug)]
struct Runtime {
metrics: Metrics,
identity: Option<LocalCrtKey>,
identity: LocalCrtKey,
tap: tap::Registry,
span_sink: OpenCensusSink,
drain: drain::Watch,
Expand Down Expand Up @@ -126,11 +126,7 @@ impl<S> Outbound<S> {
}

fn no_tls_reason(&self) -> tls::NoClientTls {
if self.runtime.identity.is_none() {
tls::NoClientTls::Disabled
} else {
tls::NoClientTls::NotProvidedByServiceDiscovery
}
tls::NoClientTls::NotProvidedByServiceDiscovery
}

pub fn push<L: svc::Layer<S>>(self, layer: L) -> Outbound<L::Service> {
Expand Down
2 changes: 0 additions & 2 deletions linkerd/app/outbound/src/tcp/logical.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,12 @@ where
..
} = config.proxy;

let identity_disabled = rt.identity.is_none();
let resolve = svc::stack(resolve.into_service())
.check_service::<ConcreteAddr>()
.push_request_filter(|c: Concrete| Ok::<_, Infallible>(c.resolve))
.push(svc::layer::mk(move |inner| {
map_endpoint::Resolve::new(
endpoint::FromMetadata {
identity_disabled,
inbound_ips: config.inbound_ips.clone(),
},
inner,
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/outbound/src/test_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub(crate) fn runtime() -> (ProxyRuntime, drain::Signal) {
let (tap, _) = tap::new();
let (metrics, _) = metrics::Metrics::new(std::time::Duration::from_secs(10));
let runtime = ProxyRuntime {
identity: None,
identity: linkerd_proxy_identity::LocalCrtKey::default_for_test(),
metrics: metrics.proxy,
tap,
span_sink: None,
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/src/dst.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl Config {
self,
dns: dns::Resolver,
metrics: metrics::ControlHttp,
identity: Option<LocalCrtKey>,
identity: LocalCrtKey,
) -> Result<
Dst<
impl svc::Service<
Expand Down
Loading

0 comments on commit 49227fe

Please sign in to comment.