-
Notifications
You must be signed in to change notification settings - Fork 271
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
linkerd_identity: split linkerd_identity::Id
into DNS and URI variants
#2538
Changes from 5 commits
ca41400
d2242f6
3e31ee8
0359465
a187e19
2defa19
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 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -30,6 +30,7 @@ linkerd-meshtls-rustls = { path = "../../meshtls/rustls", optional = true } | |||||||||||
linkerd-proxy-client-policy = { path = "../../proxy/client-policy" } | ||||||||||||
linkerd-tonic-watch = { path = "../../tonic-watch" } | ||||||||||||
linkerd2-proxy-api = { version = "0.12", features = ["inbound"] } | ||||||||||||
linkerd-identity = { path = "../../identity" } | ||||||||||||
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. This shouldn't be necessary: app-inbound and app-outbound pick up the vast majority of their dependencies via reexport from app-core: linkerd2-proxy/linkerd/app/core/src/lib.rs Lines 50 to 54 in 31b2aea
|
||||||||||||
once_cell = "1" | ||||||||||||
parking_lot = "0.12" | ||||||||||||
rangemap = "1" | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ use linkerd_app_core::{ | |
tls, | ||
transport::{ClientAddr, OrigDstAddr, Remote}, | ||
}; | ||
use linkerd_identity as id; | ||
use linkerd_idle_cache::Cached; | ||
pub use linkerd_proxy_server_policy::{ | ||
authz::Suffix, | ||
|
@@ -150,15 +151,7 @@ impl AllowPolicy { | |
} | ||
} | ||
|
||
fn is_authorized( | ||
authz: &Authorization, | ||
client_addr: Remote<ClientAddr>, | ||
tls: &tls::ConditionalServerTls, | ||
) -> bool { | ||
if !authz.networks.iter().any(|n| n.contains(&client_addr.ip())) { | ||
return false; | ||
} | ||
|
||
fn is_tls_authorized(tls: &tls::ConditionalServerTls, authz: &Authorization) -> bool { | ||
match authz.authentication { | ||
Authentication::Unauthenticated => true, | ||
|
||
|
@@ -176,15 +169,30 @@ fn is_authorized( | |
tls::ConditionalServerTls::Some(tls::ServerTls::Established { | ||
client_id: Some(tls::server::ClientId(ref id)), | ||
.. | ||
}) => { | ||
identities.contains(&*id.to_str()) | ||
|| suffixes.iter().any(|s| s.contains(&id.to_str())) | ||
} | ||
}) => match id { | ||
id::Id::Uri(_) => identities.contains(&*id.to_str()), | ||
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. does that make sense for now ? |
||
id::Id::Dns(_) => { | ||
identities.contains(&*id.to_str()) | ||
|| suffixes.iter().any(|s| s.contains(&id.to_str())) | ||
} | ||
}, | ||
_ => false, | ||
}, | ||
} | ||
} | ||
|
||
fn is_authorized( | ||
authz: &Authorization, | ||
client_addr: Remote<ClientAddr>, | ||
tls: &tls::ConditionalServerTls, | ||
) -> bool { | ||
if !authz.networks.iter().any(|n| n.contains(&client_addr.ip())) { | ||
return false; | ||
} | ||
|
||
is_tls_authorized(tls, authz) | ||
} | ||
|
||
// === impl Permit === | ||
|
||
impl ServerPermit { | ||
|
@@ -199,3 +207,119 @@ impl ServerPermit { | |
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::is_tls_authorized; | ||
use super::Meta; | ||
use super::Suffix; | ||
use super::{Authentication, Authorization}; | ||
use linkerd_app_core::tls; | ||
use std::collections::BTreeSet; | ||
use std::str::FromStr; | ||
use std::sync::Arc; | ||
|
||
fn authorization(identities: BTreeSet<String>, suffixes: Vec<Suffix>) -> Authorization { | ||
Authorization { | ||
networks: vec![], | ||
meta: Arc::new(Meta::Default { | ||
name: "name".into(), | ||
}), | ||
authentication: Authentication::TlsAuthenticated { | ||
identities, | ||
suffixes, | ||
}, | ||
} | ||
} | ||
|
||
fn server_tls(identity: &str) -> tls::ConditionalServerTls { | ||
let client_id = tls::ClientId::from_str(identity).expect("should parse id"); | ||
tls::ConditionalServerTls::Some(tls::ServerTls::Established { | ||
client_id: Some(client_id), | ||
negotiated_protocol: None, | ||
}) | ||
} | ||
|
||
#[test] | ||
fn is_authorized_for_matching_spiffe_ids() { | ||
let tls = server_tls("spiffe://some-root/some-workload"); | ||
let authz = authorization( | ||
BTreeSet::from(["spiffe://some-root/some-workload".into()]), | ||
vec![], | ||
); | ||
assert!(is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_not_authorized_for_non_matching_spiffe_ids() { | ||
let tls = server_tls("spiffe://some-root/some-workload-1"); | ||
let authz = authorization( | ||
BTreeSet::from(["spiffe://some-root/some-workload-2".into()]), | ||
vec![], | ||
); | ||
assert!(!is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_authorized_for_matching_dns_ids() { | ||
let tls = server_tls("some.id.local"); | ||
let authz = authorization(BTreeSet::from(["some.id.local".into()]), vec![]); | ||
assert!(is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_not_authorized_for_non_matching_dns_ids() { | ||
let tls = server_tls("some.id.local.one"); | ||
let authz = authorization(BTreeSet::from(["some.id.local.two".into()]), vec![]); | ||
assert!(!is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_authorized_for_matching_dns_suffixes_ids() { | ||
let tls = server_tls("some.id.local"); | ||
let authz = authorization(BTreeSet::new(), vec![Suffix::new("id.local")]); | ||
assert!(is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_not_authorized_for_non_matching_suffixes_ids() { | ||
let tls = server_tls("some.id.local"); | ||
let authz = authorization(BTreeSet::new(), vec![Suffix::new("another-id.local")]); | ||
assert!(!is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_not_authorized_for_suffixes_and_spiffe_id() { | ||
let tls = server_tls("spiffe://some-root/some-workload-1"); | ||
let authz = authorization(BTreeSet::new(), vec![Suffix::new("some-workload-1")]); | ||
assert!(!is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_authorized_for_one_matching_spiffe_id() { | ||
let tls = server_tls("spiffe://some-root/some-workload-1"); | ||
let authz = authorization( | ||
BTreeSet::from([ | ||
"spiffe://some-root/some-workload-1".into(), | ||
"some.workload.one".into(), | ||
"some.workload.two".into(), | ||
]), | ||
vec![], | ||
); | ||
assert!(is_tls_authorized(&tls, &authz)) | ||
} | ||
|
||
#[test] | ||
fn is_authorized_for_one_matching_dns_id() { | ||
let tls = server_tls("some.workload.one"); | ||
let authz = authorization( | ||
BTreeSet::from([ | ||
"spiffe://some-root/some-workload-1".into(), | ||
"some.workload.one".into(), | ||
"some.workload.two".into(), | ||
]), | ||
vec![], | ||
); | ||
assert!(is_tls_authorized(&tls, &authz)) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -353,14 +353,19 @@ | |
self.metadata | ||
.identity() | ||
.cloned() | ||
.map(move |server_id| { | ||
.map(move |client_tls| { | ||
let alpn = if use_transport_header { | ||
use linkerd_app_core::transport_header::PROTOCOL; | ||
Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()])) | ||
} else { | ||
None | ||
}; | ||
tls::ConditionalClientTls::Some(tls::ClientTls::new(server_id, alpn)) | ||
|
||
tls::ConditionalClientTls::Some(tls::ClientTls::new( | ||
client_tls.server_id, | ||
client_tls.server_name, | ||
alpn, | ||
)) | ||
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 .map(move |mut client_tls| {
client_tls.alpn = if use_transport_header {
use linkerd_app_core::transport_header::PROTOCOL;
Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()]))
} else {
None
};
tls::ConditionalClientTls::Some(client_tls) 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. alternately -- we could add a .map(move |client_tls| {
tls::ConditionalClientTls::Some(client_tls.with_alpn(if use_transport_header {
use linkerd_app_core::transport_header::PROTOCOL;
Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()]))
} else {
None
})) |
||
}) | ||
.unwrap_or(tls::ConditionalClientTls::None( | ||
tls::NoClientTls::NotProvidedByServiceDiscovery, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,5 @@ | ||||||
use futures::{future, TryFutureExt}; | ||||||
use linkerd_app_core::{dns, identity, svc, tls, Conditional, Error}; | ||||||
use linkerd_app_core::{identity, svc, tls, Conditional, Error}; | ||||||
use std::task::{Context, Poll}; | ||||||
use thiserror::Error; | ||||||
use tracing::{debug, trace}; | ||||||
|
@@ -59,8 +59,7 @@ impl<S> RequireIdentity<S> { | |||||
#[inline] | ||||||
fn extract_id<B>(req: &mut http::Request<B>) -> Option<identity::Id> { | ||||||
let v = req.headers_mut().remove(HEADER_NAME)?; | ||||||
let n = v.to_str().ok()?.parse::<dns::Name>().ok()?; | ||||||
Some(n.into()) | ||||||
v.to_str().ok()?.parse::<identity::Id>().ok() | ||||||
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 -- the explicit annotation is no longer needed because we're not using an intermediate type.
Suggested change
|
||||||
} | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,14 +283,18 @@ | |
self.metadata | ||
.identity() | ||
.cloned() | ||
.map(move |server_id| { | ||
.map(move |client_tls| { | ||
let alpn = if use_transport_header { | ||
use linkerd_app_core::transport_header::PROTOCOL; | ||
Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()])) | ||
} else { | ||
None | ||
}; | ||
tls::ConditionalClientTls::Some(tls::ClientTls::new(server_id, alpn)) | ||
tls::ConditionalClientTls::Some(tls::ClientTls::new( | ||
zaharidichev marked this conversation as resolved.
Show resolved
Hide resolved
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. same suggestion as above could apply here |
||
client_tls.server_id, | ||
client_tls.server_name, | ||
alpn, | ||
)) | ||
}) | ||
.unwrap_or(tls::ConditionalClientTls::None( | ||
tls::NoClientTls::NotProvidedByServiceDiscovery, | ||
|
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.
I looked into this and (1) it appears that trust-dns has rebranded as hickory-dns, so we're going to have to be aware of that when we upgrade trust-dns; (2) and the latest version is on idna v0.4.
Normally, we'd follow up this PR with an upstream PR to update hickory-dns to idna 0.5 so that we can be sure to resolve this when we take the next hickory udpate.
These exceptions are fine temporarily, but each skip here means we're double-building dependencies. We try to keep this overhead manageable over time.