From cf063291d81a8ffdf2d9eb9f1da4d6a5b795e087 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 26 Jun 2024 09:43:05 +0200 Subject: [PATCH] revert: remove ttl information from datagram (#1940) https://github.com/mozilla/neqo/pull/1568 introduced TTL information to datagrams. On the input path it would take the ttl information, but not act on it. On the output path it would set only "the default TTL on many OSes". https://github.com/mozilla/neqo/blob/66504908e5fa070a8a5fa67d8b5a201d2c9a5cc5/neqo-transport/src/path.rs#L576 This commit removes the ttl information from `Datagram`, thus reverting a subset of https://github.com/mozilla/neqo/pull/1568. This is partially motivated by https://github.com/mozilla/neqo/pull/1920, introducing `quinn-udp` as the IO library of choice, which does not support reading and writing TTL. See also discussion in https://github.com/mozilla/neqo/pull/1920#discussion_r1644113749. --- fuzz/fuzz_targets/client_initial.rs | 8 +---- fuzz/fuzz_targets/server_initial.rs | 8 +---- neqo-bin/src/udp.rs | 2 -- neqo-common/src/datagram.rs | 20 ++--------- neqo-transport/src/connection/mod.rs | 1 - .../src/connection/tests/handshake.rs | 6 ++-- .../src/connection/tests/migration.rs | 10 ++---- neqo-transport/src/path.rs | 5 +-- neqo-transport/src/server.rs | 10 ++---- neqo-transport/tests/connection.rs | 4 --- neqo-transport/tests/retry.rs | 33 +++---------------- neqo-transport/tests/server.rs | 26 ++------------- test-fixture/src/lib.rs | 12 ++----- 13 files changed, 22 insertions(+), 123 deletions(-) diff --git a/fuzz/fuzz_targets/client_initial.rs b/fuzz/fuzz_targets/client_initial.rs index 8e982b37ce..d0dfcdeac8 100644 --- a/fuzz/fuzz_targets/client_initial.rs +++ b/fuzz/fuzz_targets/client_initial.rs @@ -57,13 +57,7 @@ fuzz_target!(|data: &[u8]| { &mut ciphertext, (header_enc.len() - 1)..header_enc.len(), ); - let fuzzed_ci = Datagram::new( - ci.source(), - ci.destination(), - ci.tos(), - ci.ttl(), - ciphertext, - ); + let fuzzed_ci = Datagram::new(ci.source(), ci.destination(), ci.tos(), ciphertext); let mut server = default_server(); let _response = server.process(Some(&fuzzed_ci), now()); diff --git a/fuzz/fuzz_targets/server_initial.rs b/fuzz/fuzz_targets/server_initial.rs index f264619fe7..fd72ede445 100644 --- a/fuzz/fuzz_targets/server_initial.rs +++ b/fuzz/fuzz_targets/server_initial.rs @@ -63,13 +63,7 @@ fuzz_target!(|data: &[u8]| { &mut ciphertext, (header_enc.len() - 1)..header_enc.len(), ); - let fuzzed_si = Datagram::new( - si.source(), - si.destination(), - si.tos(), - si.ttl(), - ciphertext, - ); + let fuzzed_si = Datagram::new(si.source(), si.destination(), si.tos(), ciphertext); let _response = client.process(Some(&fuzzed_si), now()); }); diff --git a/neqo-bin/src/udp.rs b/neqo-bin/src/udp.rs index 0c4b034dc8..e211128aed 100644 --- a/neqo-bin/src/udp.rs +++ b/neqo-bin/src/udp.rs @@ -117,7 +117,6 @@ impl Socket { meta.addr, *local_address, meta.ecn.map(|n| IpTos::from(n as u8)).unwrap_or_default(), - None, // TODO: get the real TTL https://github.com/quinn-rs/quinn/issues/1749 d, ) }) @@ -141,7 +140,6 @@ mod tests { sender.local_addr()?, receiver.local_addr()?, IpTos::from((IpTosDscp::Le, IpTosEcn::Ect1)), - None, "Hello, world!".as_bytes().to_vec(), ); diff --git a/neqo-common/src/datagram.rs b/neqo-common/src/datagram.rs index 2dd946ffc0..1a5a63f9b5 100644 --- a/neqo-common/src/datagram.rs +++ b/neqo-common/src/datagram.rs @@ -13,23 +13,15 @@ pub struct Datagram { src: SocketAddr, dst: SocketAddr, tos: IpTos, - ttl: Option, d: Vec, } impl Datagram { - pub fn new>>( - src: SocketAddr, - dst: SocketAddr, - tos: IpTos, - ttl: Option, - d: V, - ) -> Self { + pub fn new>>(src: SocketAddr, dst: SocketAddr, tos: IpTos, d: V) -> Self { Self { src, dst, tos, - ttl, d: d.into(), } } @@ -49,11 +41,6 @@ impl Datagram { self.tos } - #[must_use] - pub fn ttl(&self) -> Option { - self.ttl - } - pub fn set_tos(&mut self, tos: IpTos) { self.tos = tos; } @@ -71,9 +58,8 @@ impl std::fmt::Debug for Datagram { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { write!( f, - "Datagram {:?} TTL {:?} {:?}->{:?}: {}", + "Datagram {:?} {:?}->{:?}: {}", self.tos, - self.ttl, self.src, self.dst, hex_with_len(&self.d) @@ -89,6 +75,6 @@ fn fmt_datagram() { let d = datagram([0; 1].to_vec()); assert_eq!( &format!("{d:?}"), - "Datagram IpTos(Cs0, Ect0) TTL Some(128) [fe80::1]:443->[fe80::1]:443: [1]: 00" + "Datagram IpTos(Cs0, Ect0) [fe80::1]:443->[fe80::1]:443: [1]: 00" ); } diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index f396782477..0a49bfae77 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1231,7 +1231,6 @@ impl Connection { d.source(), d.destination(), d.tos(), - d.ttl(), &d[d.len() - remaining..], ) } else { diff --git a/neqo-transport/src/connection/tests/handshake.rs b/neqo-transport/src/connection/tests/handshake.rs index ea2ed9b717..4cab1696e7 100644 --- a/neqo-transport/src/connection/tests/handshake.rs +++ b/neqo-transport/src/connection/tests/handshake.rs @@ -618,7 +618,7 @@ fn corrupted_initial() { .find(|(_, &v)| v != 0) .unwrap(); corrupted[idx] ^= 0x76; - let dgram = Datagram::new(d.source(), d.destination(), d.tos(), d.ttl(), corrupted); + let dgram = Datagram::new(d.source(), d.destination(), d.tos(), corrupted); server.process_input(&dgram, now()); // The server should have received two packets, // the first should be dropped, the second saved. @@ -714,7 +714,7 @@ fn extra_initial_invalid_cid() { let mut copy = hs.to_vec(); assert_ne!(copy[5], 0); // The DCID should be non-zero length. copy[6] ^= 0xc4; - let dgram_copy = Datagram::new(hs.destination(), hs.source(), hs.tos(), hs.ttl(), copy); + let dgram_copy = Datagram::new(hs.destination(), hs.source(), hs.tos(), copy); let nothing = client.process(Some(&dgram_copy), now).dgram(); assert!(nothing.is_none()); } @@ -836,7 +836,6 @@ fn drop_initial_packet_from_wrong_address() { SocketAddr::new(IpAddr::V6(Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 2)), 443), p.destination(), p.tos(), - p.ttl(), &p[..], ); @@ -864,7 +863,6 @@ fn drop_handshake_packet_from_wrong_address() { SocketAddr::new(IpAddr::V6(Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 2)), 443), p.destination(), p.tos(), - p.ttl(), &p[..], ); diff --git a/neqo-transport/src/connection/tests/migration.rs b/neqo-transport/src/connection/tests/migration.rs index 779cc78c53..2304917246 100644 --- a/neqo-transport/src/connection/tests/migration.rs +++ b/neqo-transport/src/connection/tests/migration.rs @@ -53,7 +53,7 @@ fn loopback() -> SocketAddr { } fn change_path(d: &Datagram, a: SocketAddr) -> Datagram { - Datagram::new(a, a, d.tos(), d.ttl(), &d[..]) + Datagram::new(a, a, d.tos(), &d[..]) } fn new_port(a: SocketAddr) -> SocketAddr { @@ -62,13 +62,7 @@ fn new_port(a: SocketAddr) -> SocketAddr { } fn change_source_port(d: &Datagram) -> Datagram { - Datagram::new( - new_port(d.source()), - d.destination(), - d.tos(), - d.ttl(), - &d[..], - ) + Datagram::new(new_port(d.source()), d.destination(), d.tos(), &d[..]) } /// As these tests use a new path, that path often has a non-zero RTT. diff --git a/neqo-transport/src/path.rs b/neqo-transport/src/path.rs index 2aecd209b1..7d40d6b138 100644 --- a/neqo-transport/src/path.rs +++ b/neqo-transport/src/path.rs @@ -534,8 +534,6 @@ pub struct Path { rtt: RttEstimate, /// A packet sender for the path, which includes congestion control and a pacer. sender: PacketSender, - /// The IP TTL to use for outgoing packets on this path. - ttl: u8, /// The number of bytes received on this path. /// Note that this value might saturate on a long-lived connection, @@ -573,7 +571,6 @@ impl Path { challenge: None, rtt: RttEstimate::default(), sender, - ttl: 64, // This is the default TTL on many OSes. received_bytes: 0, sent_bytes: 0, ecn_info: EcnInfo::default(), @@ -711,7 +708,7 @@ impl Path { // with the current value. let tos = self.tos(); self.ecn_info.on_packet_sent(); - Datagram::new(self.local, self.remote, tos, Some(self.ttl), payload) + Datagram::new(self.local, self.remote, tos, payload) } /// Get local address as `SocketAddr` diff --git a/neqo-transport/src/server.rs b/neqo-transport/src/server.rs index eabb10e25b..1489b8ebff 100644 --- a/neqo-transport/src/server.rs +++ b/neqo-transport/src/server.rs @@ -348,13 +348,8 @@ impl Server { &initial.dst_cid, ); if let Ok(p) = packet { - let retry = Datagram::new( - dgram.destination(), - dgram.source(), - dgram.tos(), - dgram.ttl(), - p, - ); + let retry = + Datagram::new(dgram.destination(), dgram.source(), dgram.tos(), p); Some(retry) } else { qerror!([self], "unable to encode retry, dropping packet"); @@ -603,7 +598,6 @@ impl Server { dgram.destination(), dgram.source(), dgram.tos(), - dgram.ttl(), vn, )); } diff --git a/neqo-transport/tests/connection.rs b/neqo-transport/tests/connection.rs index 35167d0abd..becef53f01 100644 --- a/neqo-transport/tests/connection.rs +++ b/neqo-transport/tests/connection.rs @@ -42,7 +42,6 @@ fn truncate_long_packet() { dupe.source(), dupe.destination(), dupe.tos(), - dupe.ttl(), &dupe[..(dupe.len() - tail)], ); let hs_probe = client.process(Some(&truncated), now()).dgram(); @@ -114,7 +113,6 @@ fn reorder_server_initial() { server_initial.source(), server_initial.destination(), server_initial.tos(), - server_initial.ttl(), packet, ); @@ -160,7 +158,6 @@ fn set_payload(server_packet: &Option, client_dcid: &[u8], payload: &[ server_initial.source(), server_initial.destination(), server_initial.tos(), - server_initial.ttl(), packet, ) } @@ -262,7 +259,6 @@ fn overflow_crypto() { server_initial.source(), server_initial.destination(), server_initial.tos(), - server_initial.ttl(), packet, ); client.process_input(&dgram, now()); diff --git a/neqo-transport/tests/retry.rs b/neqo-transport/tests/retry.rs index 07eee6635d..5f8d0f5700 100644 --- a/neqo-transport/tests/retry.rs +++ b/neqo-transport/tests/retry.rs @@ -156,13 +156,7 @@ fn retry_different_ip() { let dgram = dgram.unwrap(); let other_v4 = IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)); let other_addr = SocketAddr::new(other_v4, 443); - let from_other = Datagram::new( - other_addr, - dgram.destination(), - dgram.tos(), - dgram.ttl(), - &dgram[..], - ); + let from_other = Datagram::new(other_addr, dgram.destination(), dgram.tos(), &dgram[..]); let dgram = server.process(Some(&from_other), now()).dgram(); assert!(dgram.is_none()); } @@ -183,13 +177,7 @@ fn new_token_different_ip() { // Now rewrite the source address. let d = dgram.unwrap(); let src = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 2)), d.source().port()); - let dgram = Some(Datagram::new( - src, - d.destination(), - d.tos(), - d.ttl(), - &d[..], - )); + let dgram = Some(Datagram::new(src, d.destination(), d.tos(), &d[..])); let dgram = server.process(dgram.as_ref(), now()).dgram(); // Retry assert!(dgram.is_some()); assertions::assert_retry(dgram.as_ref().unwrap()); @@ -214,13 +202,7 @@ fn new_token_expired() { let the_future = now() + Duration::from_secs(60 * 60 * 24 * 30); let d = dgram.unwrap(); let src = SocketAddr::new(d.source().ip(), d.source().port() + 1); - let dgram = Some(Datagram::new( - src, - d.destination(), - d.tos(), - d.ttl(), - &d[..], - )); + let dgram = Some(Datagram::new(src, d.destination(), d.tos(), &d[..])); let dgram = server.process(dgram.as_ref(), the_future).dgram(); // Retry assert!(dgram.is_some()); assertions::assert_retry(dgram.as_ref().unwrap()); @@ -281,13 +263,7 @@ fn retry_bad_integrity() { let mut tweaked = retry.to_vec(); tweaked[retry.len() - 1] ^= 0x45; // damage the auth tag - let tweaked_packet = Datagram::new( - retry.source(), - retry.destination(), - retry.tos(), - retry.ttl(), - tweaked, - ); + let tweaked_packet = Datagram::new(retry.source(), retry.destination(), retry.tos(), tweaked); // The client should ignore this packet. let dgram = client.process(Some(&tweaked_packet), now()).dgram(); @@ -454,7 +430,6 @@ fn mitm_retry() { client_initial2.source(), client_initial2.destination(), client_initial2.tos(), - client_initial2.ttl(), notoken_packet, ); qdebug!("passing modified Initial to the main server"); diff --git a/neqo-transport/tests/server.rs b/neqo-transport/tests/server.rs index 36a49a48cf..39d04fb681 100644 --- a/neqo-transport/tests/server.rs +++ b/neqo-transport/tests/server.rs @@ -158,7 +158,6 @@ fn duplicate_initial_new_path() { SocketAddr::new(initial.source().ip(), initial.source().port() ^ 23), initial.destination(), initial.tos(), - initial.ttl(), &initial[..], ); @@ -373,13 +372,7 @@ fn new_token_different_port() { // Now rewrite the source port, which should not change that the token is OK. let d = dgram.unwrap(); let src = SocketAddr::new(d.source().ip(), d.source().port() + 1); - let dgram = Some(Datagram::new( - src, - d.destination(), - d.tos(), - d.ttl(), - &d[..], - )); + let dgram = Some(Datagram::new(src, d.destination(), d.tos(), &d[..])); let dgram = server.process(dgram.as_ref(), now()).dgram(); // Retry assert!(dgram.is_some()); assertions::assert_initial(dgram.as_ref().unwrap(), false); @@ -434,13 +427,7 @@ fn bad_client_initial() { &mut ciphertext, (header_enc.len() - 1)..header_enc.len(), ); - let bad_dgram = Datagram::new( - dgram.source(), - dgram.destination(), - dgram.tos(), - dgram.ttl(), - ciphertext, - ); + let bad_dgram = Datagram::new(dgram.source(), dgram.destination(), dgram.tos(), ciphertext); // The server should reject this. let response = server.process(Some(&bad_dgram), now()); @@ -522,13 +509,7 @@ fn bad_client_initial_connection_close() { &mut ciphertext, (header_enc.len() - 1)..header_enc.len(), ); - let bad_dgram = Datagram::new( - dgram.source(), - dgram.destination(), - dgram.tos(), - dgram.ttl(), - ciphertext, - ); + let bad_dgram = Datagram::new(dgram.source(), dgram.destination(), dgram.tos(), ciphertext); // The server should ignore this and go to Draining. let mut now = now(); @@ -551,7 +532,6 @@ fn version_negotiation_ignored() { dgram.source(), dgram.destination(), dgram.tos(), - dgram.ttl(), input.clone(), ); let vn = server.process(Some(&damaged), now()).dgram(); diff --git a/test-fixture/src/lib.rs b/test-fixture/src/lib.rs index 2fe0daf932..54fdaa6ebd 100644 --- a/test-fixture/src/lib.rs +++ b/test-fixture/src/lib.rs @@ -96,13 +96,7 @@ pub const DEFAULT_ADDR_V4: SocketAddr = addr_v4(); // Create a default datagram with the given data. #[must_use] pub fn datagram(data: Vec) -> Datagram { - Datagram::new( - DEFAULT_ADDR, - DEFAULT_ADDR, - IpTosEcn::Ect0.into(), - Some(128), - data, - ) + Datagram::new(DEFAULT_ADDR, DEFAULT_ADDR, IpTosEcn::Ect0.into(), data) } /// Create a default socket address. @@ -362,8 +356,8 @@ fn split_packet(buf: &[u8]) -> (&[u8], Option<&[u8]>) { pub fn split_datagram(d: &Datagram) -> (Datagram, Option) { let (a, b) = split_packet(&d[..]); ( - Datagram::new(d.source(), d.destination(), d.tos(), d.ttl(), a), - b.map(|b| Datagram::new(d.source(), d.destination(), d.tos(), d.ttl(), b)), + Datagram::new(d.source(), d.destination(), d.tos(), a), + b.map(|b| Datagram::new(d.source(), d.destination(), d.tos(), b)), ) }