Skip to content

Commit

Permalink
feedback
Browse files Browse the repository at this point in the history
Signed-off-by: Zahari Dichev <[email protected]>
  • Loading branch information
zaharidichev committed Dec 8, 2023
1 parent a187e19 commit 2defa19
Show file tree
Hide file tree
Showing 12 changed files with 25 additions and 52 deletions.
1 change: 0 additions & 1 deletion Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1208,7 +1208,6 @@ dependencies = [
"linkerd-app-test",
"linkerd-http-access-log",
"linkerd-http-metrics",
"linkerd-identity",
"linkerd-idle-cache",
"linkerd-io",
"linkerd-meshtls",
Expand Down
1 change: 0 additions & 1 deletion linkerd/app/inbound/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ 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" }
once_cell = "1"
parking_lot = "0.12"
rangemap = "1"
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/inbound/src/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ pub use self::{

pub use linkerd_app_core::metrics::ServerLabel;
use linkerd_app_core::{
identity as id,
metrics::{RouteAuthzLabels, ServerAuthzLabels},
tls,
transport::{ClientAddr, OrigDstAddr, Remote},
};
use linkerd_identity as id;
use linkerd_idle_cache::Cached;
pub use linkerd_proxy_server_policy::{
authz::Suffix,
Expand Down
10 changes: 3 additions & 7 deletions linkerd/app/outbound/src/http/concrete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,19 +353,15 @@ impl<T> svc::Param<tls::ConditionalClientTls> for Endpoint<T> {
self.metadata
.identity()
.cloned()
.map(move |client_tls| {
let alpn = if use_transport_header {
.map(move |mut client_tls| {
client_tls.alpn = if use_transport_header {

Check warning on line 357 in linkerd/app/outbound/src/http/concrete.rs

View check run for this annotation

Codecov / codecov/patch

linkerd/app/outbound/src/http/concrete.rs#L357

Added line #L357 was not covered by tests
use linkerd_app_core::transport_header::PROTOCOL;
Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()]))
} else {
None
};

tls::ConditionalClientTls::Some(tls::ClientTls::new(
client_tls.server_id,
client_tls.server_name,
alpn,
))
tls::ConditionalClientTls::Some(client_tls)

Check warning on line 364 in linkerd/app/outbound/src/http/concrete.rs

View check run for this annotation

Codecov / codecov/patch

linkerd/app/outbound/src/http/concrete.rs#L364

Added line #L364 was not covered by tests
})
.unwrap_or(tls::ConditionalClientTls::None(
tls::NoClientTls::NotProvidedByServiceDiscovery,
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/outbound/src/http/require_id_header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +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)?;
v.to_str().ok()?.parse::<identity::Id>().ok()
v.to_str().ok()?.parse().ok()
}
}

Expand Down
11 changes: 4 additions & 7 deletions linkerd/app/outbound/src/opaq/concrete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,18 +283,15 @@ impl<T> svc::Param<tls::ConditionalClientTls> for Endpoint<T> {
self.metadata
.identity()
.cloned()
.map(move |client_tls| {
let alpn = if use_transport_header {
.map(move |mut client_tls| {
client_tls.alpn = if use_transport_header {

Check warning on line 287 in linkerd/app/outbound/src/opaq/concrete.rs

View check run for this annotation

Codecov / codecov/patch

linkerd/app/outbound/src/opaq/concrete.rs#L287

Added line #L287 was not covered by tests
use linkerd_app_core::transport_header::PROTOCOL;
Some(tls::client::AlpnProtocols(vec![PROTOCOL.into()]))
} else {
None
};
tls::ConditionalClientTls::Some(tls::ClientTls::new(
client_tls.server_id,
client_tls.server_name,
alpn,
))

tls::ConditionalClientTls::Some(client_tls)

Check warning on line 294 in linkerd/app/outbound/src/opaq/concrete.rs

View check run for this annotation

Codecov / codecov/patch

linkerd/app/outbound/src/opaq/concrete.rs#L294

Added line #L294 was not covered by tests
})
.unwrap_or(tls::ConditionalClientTls::None(
tls::NoClientTls::NotProvidedByServiceDiscovery,
Expand Down
22 changes: 10 additions & 12 deletions linkerd/app/outbound/src/tcp/tagged_transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,13 @@ mod test {
fn param(&self) -> tls::ConditionalClientTls {
self.identity
.clone()
.map(|client_tls| {
.map(move |mut client_tls| {
let alpn = Some(tls::client::AlpnProtocols(vec![
transport_header::PROTOCOL.into()
]));
tls::ConditionalClientTls::Some(tls::ClientTls::new(
client_tls.server_id,
client_tls.server_name,
alpn,
))
client_tls.alpn = alpn;

tls::ConditionalClientTls::Some(client_tls)
})
.unwrap_or(tls::ConditionalClientTls::None(
tls::NoClientTls::NotProvidedByServiceDiscovery,
Expand Down Expand Up @@ -265,7 +263,7 @@ mod test {

let e = Endpoint {
port_override: Some(4143),
identity: Some(tls::ClientTls::new(server_id, server_name, None)),
identity: Some(tls::ClientTls::new(server_id, server_name)),
authority: None,
proto: None,
};
Expand All @@ -290,7 +288,7 @@ mod test {

let e = Endpoint {
port_override: Some(4143),
identity: Some(tls::ClientTls::new(server_id, server_name, None)),
identity: Some(tls::ClientTls::new(server_id, server_name)),
authority: Some(http::uri::Authority::from_str("foo.bar.example.com:5555").unwrap()),
proto: None,
};
Expand All @@ -315,7 +313,7 @@ mod test {

let e = Endpoint {
port_override: Some(4143),
identity: Some(tls::ClientTls::new(server_id, server_name, None)),
identity: Some(tls::ClientTls::new(server_id, server_name)),
authority: None,
proto: None,
};
Expand All @@ -340,7 +338,7 @@ mod test {

let e = Endpoint {
port_override: Some(4143),
identity: Some(tls::ClientTls::new(server_id, server_name, None)),
identity: Some(tls::ClientTls::new(server_id, server_name)),
authority: None,
proto: Some(SessionProtocol::Http1),
};
Expand All @@ -365,7 +363,7 @@ mod test {

let e = Endpoint {
port_override: Some(4143),
identity: Some(tls::ClientTls::new(server_id, server_name, None)),
identity: Some(tls::ClientTls::new(server_id, server_name)),
authority: Some(http::uri::Authority::from_str("foo.bar.example.com:5555").unwrap()),
proto: Some(SessionProtocol::Http1),
};
Expand All @@ -390,7 +388,7 @@ mod test {

let e = Endpoint {
port_override: Some(4143),
identity: Some(tls::ClientTls::new(server_id, server_name, None)),
identity: Some(tls::ClientTls::new(server_id, server_name)),
authority: None,
proto: Some(SessionProtocol::Http1),
};
Expand Down
1 change: 0 additions & 1 deletion linkerd/app/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,6 @@ pub fn parse_control_addr<S: Strings>(
identity: Conditional::Some(tls::ClientTls::new(
tls::ServerId(name.clone().into()),
tls::ServerName(name),

Check warning on line 1157 in linkerd/app/src/env.rs

View check run for this annotation

Codecov / codecov/patch

linkerd/app/src/env.rs#L1155-L1157

Added lines #L1155 - L1157 were not covered by tests
None,
)),
})),
_ => {
Expand Down
13 changes: 2 additions & 11 deletions linkerd/meshtls/tests/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,7 @@ pub async fn proxy_to_proxy_tls_works(mode: meshtls::Mode) {
let server_name = tls::ServerName(test_util::FOO_NS1.name.parse().unwrap());
let (client_result, server_result) = run_test(
client_tls.clone(),
Conditional::Some(tls::ClientTls::new(
server_id.clone(),
server_name.clone(),
None,
)),
Conditional::Some(tls::ClientTls::new(server_id.clone(), server_name.clone())),
|conn| write_then_read(conn, PING),
server_tls,
|(_, conn)| read_then_write(conn, PING.len(), PONG),
Expand All @@ -68,7 +64,6 @@ pub async fn proxy_to_proxy_tls_works(mode: meshtls::Mode) {
Some(Conditional::Some(tls::ClientTls::new(
server_id,
server_name,
None
)))
);
assert_eq!(&client_result.result.expect("pong")[..], PONG);
Expand All @@ -93,11 +88,7 @@ pub async fn proxy_to_proxy_tls_pass_through_when_identity_does_not_match(mode:

let (client_result, server_result) = run_test(
client_tls,
Conditional::Some(tls::ClientTls::new(
server_id.clone(),
server_name.clone(),
None,
)),
Conditional::Some(tls::ClientTls::new(server_id.clone(), server_name.clone())),
|conn| write_then_read(conn, PING),
server_tls,
|(_, conn)| read_then_write(conn, START_OF_TLS.len(), PONG),
Expand Down
8 changes: 1 addition & 7 deletions linkerd/meshtls/verifier/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,7 @@ fn extract_ids_from_cert(cert: &[u8]) -> Result<Vec<Id>> {
.iter()
.filter_map(|n| {
let id = match n {
GeneralName::DNSName(dns) => {
if *dns == "*" {
// Wildcards can perhaps be handled in a future path...
return None;
}
Id::parse_dns_name(dns)
}
GeneralName::DNSName(dns) => Id::parse_dns_name(dns),
GeneralName::URI(uri) => Id::parse_uri(uri),
_ => return None,
};
Expand Down
2 changes: 1 addition & 1 deletion linkerd/proxy/api-resolve/src/pb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ fn to_identity(pb: TlsIdentity) -> Option<ClientTls> {

let Strategy::DnsLikeIdentity(i) = pb.strategy?;
match (ServerId::from_str(&i.name), ServerName::from_str(&i.name)) {
(Ok(i), Ok(n)) => Some(ClientTls::new(i, n, None)),
(Ok(i), Ok(n)) => Some(ClientTls::new(i, n)),

Check warning on line 60 in linkerd/proxy/api-resolve/src/pb.rs

View check run for this annotation

Codecov / codecov/patch

linkerd/proxy/api-resolve/src/pb.rs#L59-L60

Added lines #L59 - L60 were not covered by tests
(_, _) => {
tracing::warn!("Ignoring invalid identity: {}", i.name);
None
Expand Down
4 changes: 2 additions & 2 deletions linkerd/tls/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,11 @@ pub struct ConnectMeta<M> {

impl ClientTls {
// XXX(ver) We'll have to change this when ServerIds are not necessarily DNS names.
pub fn new(server_id: ServerId, server_name: ServerName, alpn: Option<AlpnProtocols>) -> Self {
pub fn new(server_id: ServerId, server_name: ServerName) -> Self {
Self {
server_name,
server_id,
alpn,
alpn: None,
}
}
}
Expand Down

0 comments on commit 2defa19

Please sign in to comment.