Skip to content
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

Update DAITA to v2 on Windows #7457

Merged
merged 44 commits into from
Jan 24, 2025
Merged

Update DAITA to v2 on Windows #7457

merged 44 commits into from
Jan 24, 2025

Conversation

dlon
Copy link
Member

@dlon dlon commented Jan 13, 2025

This PR updates Windows DAITA to v2 by reintroducing wireguard-go.

WIP

  • Sign and pack DLLs
  • Update build instructions
  • Make sure multihop handles network changes correctly. wgRebindTunnelSocket seems to affect all peers rather than only the entry.
  • Remove maybenot_machines (only from installer, since wgnt hasn't yet been removed)
  • Update changelog
  • Push tag, verify that dlls are signed
  • Set DAITA platform to wg-go for windows (currently not included)
  • Prepare build server before merging
  • Benchmarks

Out of scope for this PR

  • Remove wireguard-nt code for DAITA
  • Use the multihop implementation of wireguard-go instead of WireguardNT

This change is Reviewable

@dlon dlon changed the title Add DAITA v2 for Windows Update DAITA to v2 on Windows Jan 13, 2025
@dlon dlon force-pushed the win-daita-v2 branch 6 times, most recently from 8d2aae4 to 00670af Compare January 14, 2025 19:20
@dlon dlon force-pushed the win-daita-v2 branch 2 times, most recently from f165eaa to dc98308 Compare January 15, 2025 09:47
@dlon dlon marked this pull request as ready for review January 16, 2025 07:49
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 13 files at r1, 6 of 10 files at r3, 2 of 3 files at r4, 1 of 4 files at r6, 7 of 9 files at r7, 1 of 1 files at r8.
Reviewable status: 18 of 20 files reviewed, 2 unresolved discussions (waiting on @dlon)


talpid-wireguard/src/wireguard_go/mod.rs line 230 at r7 (raw file):

impl WgGoTunnel {
    #[cfg(all(not(target_os = "android"), unix))]

⛏️ Change to #cfg[any(target_os = "linux", target_os = "macos")]

Code quote:

#[cfg(all(not(target_os = "android"), unix))]

wireguard-go-rs/src/lib.rs line 148 at r8 (raw file):

            handle: code,
            assigned_name: assigned_name.to_owned(),
            // SAFETY: The LUID is guaranteed to be intialized by wgTurnOn

⛏️ // SAFETY: wgTurnOn succeeded and the LUID is guaranteed to be intialized by wgTurnOn.

Code quote:

// SAFETY: The LUID is guaranteed to be intialized by wgTurnOn

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 3 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 19 of 20 files reviewed, 4 unresolved discussions (waiting on @dlon)


wireguard-go-rs/build.rs line 147 at r10 (raw file):

            go_build.env("CC", "zig cc -target aarch64-windows");
        }
    }

⛏️ Please document that we use zig as a C-compiler, and therefore we explicitly set CC for cgo. Maybe we should even state that we use it instead of MinGW, but this might not be the correct place for that kind of documentation.

Code quote:

    let target_arch = target_arch()?;
    match target_arch {
        Arch::Amd64 => {
            go_build.env("CC", "zig cc -target x86_64-windows");
        }
        Arch::Arm64 => {
            go_build.env("CC", "zig cc -target aarch64-windows");
        }
    }

wireguard-go-rs/build.rs line 293 at r10 (raw file):

                dest = dest.to_str().unwrap()
            )
        })?;

I don't think we want to embed the whole path in the error message. It was done for debugging purposes. But maybe logging the filenames could be nice?

Code quote:

        fs::copy(&src, &dest).with_context(|| {
            format!(
                "Failed to copy {src} to {dest}",
                src = src.to_str().unwrap(),
                dest = dest.to_str().unwrap()
            )
        })?;

wireguard-go-rs/build.rs line 418 at r10 (raw file):

    for export in libwg_exports {
        writeln!(file, "\t{export}").context("Failed to output exported function")?;
    }

⛏️ Skip the intermediary libwg_exports

    for path in [
        "./libwg/libwg.go",
        "./libwg/libwg_windows.go",
        "./libwg/libwg_daita.go",
    ] {
        for export in gather_exports(path).context("Failed to find exports")? {
            writeln!(file, "\t{export}").context("Failed to output exported function")?;
        }
    }

Code quote:

    let mut libwg_exports = vec![];
    for path in &[
        "./libwg/libwg.go",
        "./libwg/libwg_windows.go",
        "./libwg/libwg_daita.go",
    ] {
        libwg_exports.extend(gather_exports(path).context("Failed to find exports")?);
    }

    for export in libwg_exports {
        writeln!(file, "\t{export}").context("Failed to output exported function")?;
    }

wireguard-go-rs/build.rs line 28 at r9 (raw file):

        Os::Linux => build_linux_static_lib(&out_dir)?,
        Os::MacOs => build_macos_static_lib(&out_dir)?,
        Os::Android => build_android_dynamic_lib()?,

⛏️ build_android_dynamic_lib should probably also take out_dir as an argument (just to be consistent)

Code quote:

Os::Android => build_android_dynamic_lib()?,

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 10 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dlon)


wireguard-go-rs/libwg/libwg_windows.go line 4 at r10 (raw file):

 *
 * Copyright (C) 2017-2019 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
 * Copyright (C) 2024 Mullvad VPN AB. All Rights Reserved.

⛏️ Copyright year should be updated!

Code quote:

 * Copyright (C) 2024 Mullvad VPN AB. All Rights Reserved.

Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 16 of 28 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


wireguard-go-rs/build.rs line 293 at r10 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I don't think we want to embed the whole path in the error message. It was done for debugging purposes. But maybe logging the filenames could be nice?

Done.

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 12 of 12 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon force-pushed the win-daita-v2 branch 2 times, most recently from 61f72e0 to c33e436 Compare January 20, 2025 08:53
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r15, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon merged commit 654de1c into main Jan 24, 2025
58 of 59 checks passed
@dlon dlon deleted the win-daita-v2 branch January 24, 2025 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants