Pad WgData payloads to a multiple of 16 bytes

[hulthe]

ToDo

• Document how implementation conforms to spec
• Tests

This change is 

[hulthe]

@hulthe reviewed 2 files and all commit messages.
Reviewable status: 2 of 5 files reviewed, all discussions resolved.

[MarkusPettersson98]

@MarkusPettersson98 reviewed 6 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hulthe).

---

gotatun/src/noise/mod.rs line 131 at r4 (raw file):

        if let Some(tun_mtu) = tun_mtu {
            packet = pad_to_x16(packet, tun_mtu);
        }

Much better than previous solution 🧑‍🍳 💋

Code quote:

        if let Some(tun_mtu) = tun_mtu {
            packet = pad_to_x16(packet, tun_mtu);
        }

---

gotatun/src/noise/mod.rs line 384 at r4 (raw file):

///
/// The padding is clamped to not exceed `tun_mtu`.
fn pad_to_x16(mut packet: Packet, tun_mtu: &mut MtuWatcher) -> Packet {

It would be very cool if we could track that this operation has taken place in the type system. Not today, though.

Code quote:

/// Try to pad `packet` with `0`s such that `packet.len().is_multiple_of(16)`.
///
/// The padding is clamped to not exceed `tun_mtu`.
fn pad_to_x16(mut packet: Packet, tun_mtu: &mut MtuWatcher) -> Packet {

[MarkusPettersson98]

@MarkusPettersson98 reviewed 3 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hulthe).

[hulthe]

@hulthe reviewed 16 files and all commit messages, made 5 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98).

---

gotatun/src/noise/mod.rs line 131 at r4 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Much better than previous solution 🧑‍🍳 💋

🤗

---

gotatun/src/noise/mod.rs line 384 at r4 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

It would be very cool if we could track that this operation has taken place in the type system. Not today, though.

Indeed

---

gotatun/src/tun/channel.rs line 101 at r7 (raw file):

            std::future::pending::<Infallible>().await;
            unreachable!();
        };

todo: maybe return None instead and make Device::handle_outgoing handle this case.

---

gotatun/src/udp/channel.rs line 363 at r7 (raw file):

        .expect_left("packet is ipv4")
        .try_into_udp()
        .expect("packet is udp")

Might be premature optimizaiton, but:
Would be nice if we could disable these checks in release-builds.

---

gotatun/src/udp/channel.rs line 410 at r7 (raw file):

        .expect_right("packet is ipv6")
        .try_into_udp()
        .expect("packet is udp")

Might be premature optimizaiton, but:
Would be nice if we could disable these checks in release-builds.

[MarkusPettersson98]

@MarkusPettersson98 reviewed 11 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @hulthe).

---

gotatun/src/tun/channel.rs line 101 at r7 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

todo: maybe return None instead and make Device::handle_outgoing handle this case.

That would align the API with for example how tokio::sync::mpsc::recv works. Which would be nice.

---

gotatun/src/device/mod.rs line 681 at r9 (raw file):

            let mut n = 0;
            for packet in packets {
                n += 1;

If tun_rx.recv would return None (or an empty iterator, or a special error ..) in case there are no more packets to receive, we could feasibly skip this counter. Therefore, I propose a lil' bit of refactoring. WDYT?

Code quote:

            let mut n = 0;
            for packet in packets {
                n += 1;

---

gotatun/src/udp/channel.rs line 156 at r9 (raw file):

        let (b_tx, a_rx) = mpsc::channel(capacity);
        [Self { tx: a_tx, rx: a_rx }, Self { tx: b_tx, rx: b_rx }]
    }

Could we return a tuple of Self from this function instead?:)

Code quote:

    pub(crate) fn new_pair(capacity: usize) -> [Self; 2] {
        let (a_tx, b_rx) = mpsc::channel(capacity);
        let (b_tx, a_rx) = mpsc::channel(capacity);
        [Self { tx: a_tx, rx: a_rx }, Self { tx: b_tx, rx: b_rx }]
    }

[MarkusPettersson98]

@MarkusPettersson98 reviewed 2 files and made 4 comments.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @hulthe).

---

gotatun/src/device/tests.rs line 75 at r9 (raw file):

#[tokio::test]
#[test_log::test]
async fn wg_data_length_is_x16() {

I was curious to see if this test does what you set out to prove, and it is indeed excellent! I changed the implementation of pad_to_x16 to pad data packets to the next multiple of 15 instead of 16, and this test no longer passed. Amazing work ✨

Code quote:

async fn wg_data_length_is_x16()

---

gotatun/src/device/tests.rs line 70 at r9 (raw file):

    })
    .await
}

Would there be any point in adding a test that asserts that certain packets are send in the correct order? Section 5 of the paper starts with talking about a certain ordering restriction.

Code quote:

async fn one_keepalive() {
    test_device_pair(async |eve| {
        let keepalive_count = eve
            .wg_data()
            .filter(|wg_data| ready(wg_data.is_keepalive()))
            .count()
            .await;

        assert_eq!(keepalive_count, 1);
    })
    .await
}

---

gotatun/src/device/tests.rs line 96 at r9 (raw file):

/// Helper method to test that packets can be sent from one [`Device`] to another.
/// Use `eavesdrop` to sniff wireguard packets and assert things about the connection.
async fn test_device_pair(eavesdrop: impl AsyncFnOnce(MockEavesdropper) + Send) {

This is mega cool! 💯 🔥

Code quote:

/// Helper method to test that packets can be sent from one [`Device`] to another.
/// Use `eavesdrop` to sniff wireguard packets and assert things about the connection.
async fn test_device_pair(eavesdrop: impl AsyncFnOnce(MockEavesdropper) + Send) {

---

gotatun/src/device/tests/mock.rs line 238 at r9 (raw file):

            _ => None,
        })
    }

Love these combinators<3

Code quote:

    /// Get as stream of all sniffed WireGuard packets. [Read more](Self::ip)
    pub fn wg(&self) -> impl Stream<Item = WgKind> + use<> {
        self.udp()
            .map(|udp| udp.into_payload().try_into_wg().unwrap())
    }

    /// Get as stream of all sniffed [`WgData`] packets. [Read more](Self::ip)
    pub fn wg_data(&self) -> impl Stream<Item = Packet<WgData>> + use<> {
        self.wg().filter_map(async |wg| match wg {
            WgKind::Data(data) => Some(data),
            _ => None,
        })
    }

    /// Get as stream of all sniffed [`WgHandshakeInit`] packets. [Read more](Self::ip)
    pub fn wg_handshake_init(&self) -> impl Stream<Item = Packet<WgHandshakeInit>> + use<> {
        self.wg().filter_map(async |wg| match wg {
            WgKind::HandshakeInit(data) => Some(data),
            _ => None,
        })
    }

    /// Get as stream of all sniffed [`WgHandshakeResp`] packets. [Read more](Self::ip)
    pub fn wg_handshake_resp(&self) -> impl Stream<Item = Packet<WgHandshakeResp>> + use<> {
        self.wg().filter_map(async |wg| match wg {
            WgKind::HandshakeResp(data) => Some(data),
            _ => None,
        })
    }