From dbf898aea0d1c516e3d6f0c34a34b29c7903ad44 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Mon, 12 Apr 2021 15:42:07 +0000 Subject: [PATCH 1/6] Add a test for authority conversion --- linkerd/addr/src/lib.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/linkerd/addr/src/lib.rs b/linkerd/addr/src/lib.rs index 733d65f5d4..3ad02706fe 100644 --- a/linkerd/addr/src/lib.rs +++ b/linkerd/addr/src/lib.rs @@ -257,6 +257,22 @@ mod tests { assert_eq!(a.is_loopback(), *expected_result, "{:?}", host) } } + + #[test] + fn test_to_http_authority() { + let cases = &[ + "localhost:80", + "localhost.:80", + "LocalhOsT.:80", + "mlocalhost.:80", + "localhost1.:80", + "127.0.0.1:80", + "[::1]:80", + ]; + for host in cases { + Addr::from_str(host).unwrap().to_http_authority(); + } + } } #[cfg(fuzzing)] From 37b44a1f76c596c9427881cfa46f0d987b0d2207 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 12 Apr 2021 09:55:39 -0700 Subject: [PATCH 2/6] addr: add more `to_http_authority` ipv6 tests Signed-off-by: Eliza Weisman --- linkerd/addr/src/lib.rs | 47 +++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/linkerd/addr/src/lib.rs b/linkerd/addr/src/lib.rs index 3ad02706fe..87bd3c0acc 100644 --- a/linkerd/addr/src/lib.rs +++ b/linkerd/addr/src/lib.rs @@ -258,20 +258,55 @@ mod tests { } } + fn test_to_http_authority(cases: &[&str]) { + let width = cases.iter().map(|s| s.len()).max().unwrap_or(0); + for host in cases { + print!("trying {:width$?} ... ", host, width = width); + Addr::from_str(host).unwrap().to_http_authority(); + println!("ok"); + } + } + #[test] - fn test_to_http_authority() { - let cases = &[ + fn to_http_authority_ipv4_port_80() { + test_to_http_authority(&[ "localhost:80", "localhost.:80", "LocalhOsT.:80", "mlocalhost.:80", "localhost1.:80", "127.0.0.1:80", + ]) + } + + #[test] + fn to_http_authority_ipv4() { + test_to_http_authority(&[ + "localhost:9090", + "localhost.:9090", + "LocalhOsT.:9090", + "mlocalhost.:9090", + "localhost1.:9090", + "127.0.0.1:9090", + ]) + } + + #[test] + fn to_http_authority_ipv6_port_80() { + test_to_http_authority(&[ + "[2001:0db8:0000:0000:0000:8a2e:0370:7334]:80", + "[2001:db8::8a2e:370:7334]:80", "[::1]:80", - ]; - for host in cases { - Addr::from_str(host).unwrap().to_http_authority(); - } + ]) + } + + #[test] + fn to_http_authority_ipv6() { + test_to_http_authority(&[ + "[2001:0db8:0000:0000:0000:8a2e:0370:7334]:9090", + "[2001:db8::8a2e:370:7334]:9090", + "[::1]:9090", + ]) } } From 284a53c4e723a079baa8a51e600a9a46093de158 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 12 Apr 2021 09:58:21 -0700 Subject: [PATCH 3/6] nicer panic messages Signed-off-by: Eliza Weisman --- linkerd/addr/src/lib.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/linkerd/addr/src/lib.rs b/linkerd/addr/src/lib.rs index 87bd3c0acc..60e1739994 100644 --- a/linkerd/addr/src/lib.rs +++ b/linkerd/addr/src/lib.rs @@ -82,11 +82,15 @@ impl Addr { match self { Addr::Name(n) => n.as_http_authority(), Addr::Socket(ref a) if a.port() == 80 => { - http::uri::Authority::from_str(&a.ip().to_string()) - .expect("SocketAddr must be valid authority") + http::uri::Authority::from_str(&a.ip().to_string()).unwrap_or_else(|err| { + panic!("SocketAddr ({}) must be valid authority: {}", a, err) + }) + } + Addr::Socket(a) => { + http::uri::Authority::from_str(&a.to_string()).unwrap_or_else(|err| { + panic!("SocketAddr ({}) must be valid authority: {}", a, err) + }) } - Addr::Socket(a) => http::uri::Authority::from_str(&a.to_string()) - .expect("SocketAddr must be valid authority"), } } From 9dd6335a6fd11489ecdb53b4d52dc59aa5a6232f Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 12 Apr 2021 10:04:15 -0700 Subject: [PATCH 4/6] addr: fix `to_http_authority` panic with IPv6 Currently, the `Addr::to_http_authority` method panics when called on a `SocketAddr` which is an IPv6 address with port 80. This method does not panic when called with IPv4 addresses, or with IPv6 addresses whose ports are *not* port 80. This was initially caught by oss-fuzz; see [here][1] for details. The panic occurs because when an IPv6+ address occurs in an authority, it must be within square brackets, as per [RFC3986, Section 3.2][2]. The square brackets distinguish between colons in the IPv6 address and the colon separating the address and port. When the `SocketAddr`'s port is not port 80, we format it including the port, and the `fmt::Display` output from IPv6 `SocketAddr`s includes the square brackets as expected. However, when the socket's port *is* port 80, we have special logic for eliding the port from the authority. This works fine for IPv4, where we can just call `addr.ip().to_string()` to nicely format the address. However, with IPv6 addresses, this only formats the address itself, *not* the square brackets. According to RFC3986, square brackets are mandatory for *all* IPv6 addresses, even when port 80 is elided. This branch fixes the panic by changing `Addr::to_http_authority` to include square brackets when formatting IPv6 `SocketAddr`s with port 80. I've also improved on @olix0r's original test cases from linkerd/linkerd2-proxy@dbf898a to include IPv6 addrs with and without shorthand, and to test ports that are and are not port 80. These tests helped catch the panic, and may be useful to guard against future regressions. Fixes linkerd/linkerd2#6020 [1]: https://oss-fuzz.com/testcase-detail/6502844766224384 [2]: https://tools.ietf.org/html/rfc3986#section-3.2 Signed-off-by: Eliza Weisman --- linkerd/addr/src/lib.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/linkerd/addr/src/lib.rs b/linkerd/addr/src/lib.rs index 60e1739994..01ad3a69d8 100644 --- a/linkerd/addr/src/lib.rs +++ b/linkerd/addr/src/lib.rs @@ -82,7 +82,18 @@ impl Addr { match self { Addr::Name(n) => n.as_http_authority(), Addr::Socket(ref a) if a.port() == 80 => { - http::uri::Authority::from_str(&a.ip().to_string()).unwrap_or_else(|err| { + let ip = if a.is_ipv4() { + a.ip().to_string() + } else { + // When IPv6 or later addresses are used in an authority, + // they must be within square brackets. See + // https://tools.ietf.org/html/rfc3986#section-3.2 for + // details. The `fmt::Display` implementation of the + // `Ipv6Addr` type does not include brackets, so we must add + // them ourselves. + format!("[{}]", a.ip()) + }; + http::uri::Authority::from_str(&ip).unwrap_or_else(|err| { panic!("SocketAddr ({}) must be valid authority: {}", a, err) }) } From 1afaf401e51e742acb0396bf140322ceef574454 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 12 Apr 2021 10:18:58 -0700 Subject: [PATCH 5/6] fix formatting in test failure output Signed-off-by: Eliza Weisman --- linkerd/addr/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linkerd/addr/src/lib.rs b/linkerd/addr/src/lib.rs index 01ad3a69d8..d77d706095 100644 --- a/linkerd/addr/src/lib.rs +++ b/linkerd/addr/src/lib.rs @@ -276,7 +276,7 @@ mod tests { fn test_to_http_authority(cases: &[&str]) { let width = cases.iter().map(|s| s.len()).max().unwrap_or(0); for host in cases { - print!("trying {:width$?} ... ", host, width = width); + print!("trying {:1$} ... ", host, width); Addr::from_str(host).unwrap().to_http_authority(); println!("ok"); } From d584897d5bc219674f5ac7951309a37e3876b21f Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 12 Apr 2021 10:48:51 -0700 Subject: [PATCH 6/6] separate test cases with DNS names from ipv4 Signed-off-by: Eliza Weisman --- linkerd/addr/src/lib.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/linkerd/addr/src/lib.rs b/linkerd/addr/src/lib.rs index d77d706095..45d2e49dc9 100644 --- a/linkerd/addr/src/lib.rs +++ b/linkerd/addr/src/lib.rs @@ -283,29 +283,37 @@ mod tests { } #[test] - fn to_http_authority_ipv4_port_80() { + fn to_http_authority_names_port_80() { test_to_http_authority(&[ "localhost:80", "localhost.:80", "LocalhOsT.:80", "mlocalhost.:80", "localhost1.:80", - "127.0.0.1:80", ]) } #[test] - fn to_http_authority_ipv4() { + fn to_http_authority_names() { test_to_http_authority(&[ "localhost:9090", "localhost.:9090", "LocalhOsT.:9090", "mlocalhost.:9090", "localhost1.:9090", - "127.0.0.1:9090", ]) } + #[test] + fn to_http_authority_ipv4_port_80() { + test_to_http_authority(&["10.7.0.42:80", "127.0.0.1:80"]) + } + + #[test] + fn to_http_authority_ipv4() { + test_to_http_authority(&["10.7.0.42:9090", "127.0.0.1:9090"]) + } + #[test] fn to_http_authority_ipv6_port_80() { test_to_http_authority(&[