Implement sigsum (transparency log) verification for the relay list

[kl]

This is a draft PR for adding the sigsum verification logic for the relay list.

This code will only work in stagemole for now but should successfuly verify the relay list.

It replaces the current api call for fetching the relay list with two new calls:

1. /trl/v0/timestamps/latest fetches the latest sigsum signature and digest data for the relay list.
2. /trl/v0/data/ fetches the actual relay list given a sigsum digest.

I have added a bunch of TODOs in places I was unsure how we should do things, so please give your feedback.

---

This change is 

[linear]

DROID-2436 Implement verify relay list

[kl]

@kl made 1 comment.
Reviewable status: 0 of 11 files reviewed, all discussions resolved.

---

mullvad-update/Cargo.toml line 17 at r1 (raw file):

chrono = { workspace = true, features = ["now", "serde"] }
clap = { workspace = true, optional = true }
ed25519-dalek = { version = "2.2", features = ["rand_core", "zeroize"] }

This bump was needed to be compatible with the sigsum crate.

[faern]

@faern made 1 comment.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on @kl).

---

mullvad-api/src/relay_list.rs line 160 at r1 (raw file):

/// it manually.
#[derive(Debug)]
pub struct ServerRelayListSigsum {

Would it be possible to put all the sigsum related stuff into its own file submodule? This file is already as is over 500 LOC and pretty complex, and sigsum adds another layer of complexity.

[MarkusPettersson98]

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

---

mullvad-api/src/relay_list.rs line 137 at r1 (raw file):

            Ok(relay_list_sigsum)
        }
    }

AFAICT this can be re-written as an async function directly

Code quote (i):

    pub fn relay_list_sigsum(
        &self,
    ) -> impl Future<Output = Result<ServerRelayListSigsum, rest::Error>> {
        let request = self.relay_list_sigsum_response();

        async move {
            let response = request.await?;

            let relay_list_sigsum = response
                .body()
                .await
                .and_then(|body| {
                    str::from_utf8(&body)
                        .map_err(|_| rest::Error::InvalidUtf8Error)
                        .and_then(ServerRelayListSigsum::parse_from_server_response)
                })
                .inspect_err(|_err| {
                    log::error!("Failed to deserialize API response of relay list sigsum")
                })?;

            Ok(relay_list_sigsum)
        }
    }

Code snippet (ii):

pub async fn relay_list_sigsum(&self) -> Result<ServerRelayListSigsum, rest::Error> {
    let relay_list_sigsum = self
        .relay_list_sigsum_response()
        .await?
        .body()
        .await
        .and_then(|body| {
            str::from_utf8(&body)
                .map_err(|_| rest::Error::InvalidUtf8Error)
                .and_then(ServerRelayListSigsum::parse_from_server_response)
        })
        .inspect_err(|_err| {
            log::error!("Failed to deserialize API response of relay list sigsum")
        })?;

    Ok(relay_list_sigsum)
}

---

mullvad-api/src/relay_list.rs line 42 at r1 (raw file):

        &self,
        prev_etag: Option<ETag>,
        digest: &str,

Can we introduce a newtype here to encode that it is a sigsum digest? Would be very nice

Code quote:

digest: &str,

---

mullvad-api/src/relay_list.rs line 152 at r1 (raw file):

            service.request(request).await
        }
    }

AFAICT this can be re-written as an async function directly

Code quote (i):

    fn relay_list_sigsum_response(
        &self,
    ) -> impl Future<Output = Result<rest::Response<Incoming>, rest::Error>> {
        let service = self.handle.service.clone();
        let request = self.handle.factory.get("trl/v0/timestamps/latest");

        async move {
            let request = request?
                .timeout(RELAY_LIST_TIMEOUT)
                .expected_status(&[StatusCode::NOT_MODIFIED, StatusCode::OK]);

            service.request(request).await
        }
    }

Code snippet (ii):

async fn relay_list_sigsum_response(&self) -> Result<rest::Response<Incoming>, rest::Error> {
    let service = self.handle.service.clone();
    let request = self.handle.factory.get("trl/v0/timestamps/latest");

    let request = request?
        .timeout(RELAY_LIST_TIMEOUT)
        .expected_status(&[StatusCode::NOT_MODIFIED, StatusCode::OK]);

    service.request(request).await
}

[MarkusPettersson98]

@MarkusPettersson98 made 1 comment.
Reviewable status: 3 of 11 files reviewed, 5 unresolved discussions (waiting on @kl).

---

mullvad-daemon/Cargo.toml line 41 at r1 (raw file):

serde_json = { workspace = true, features = ["std"] }
# TODO: change to crates.io when new release is available
sigsum = { git = "https://github.com/mullvad/sigsum-rs.git" }

⛏️ Put this in the workspace Crates.toml from the get-go:)

Code quote:

# TODO: change to crates.io when new release is available
sigsum = { git = "https://github.com/mullvad/sigsum-rs.git" }

[faern]

@faern made 1 comment.
Reviewable status: 3 of 11 files reviewed, 6 unresolved discussions (waiting on @kl).

---

mullvad-daemon/src/sigsum.rs line 8 at r1 (raw file):

use std::sync::LazyLock;

// TODO: where/how should we store the signatures?

To be able to track the trusted public keys easier with git, and to separate data from code, I think we should store them in separate files, and include them with include_str!. The same system we have for the trusted public keys of desktop releases in

mullvadvpn-app/mullvad-update/src/defaults.rs

Lines 27 to 28 in 060d8c4

	pub static TRUSTED_METADATA_SIGNING_PUBKEYS: LazyLock<Vec1<VerifyingKey>> =
	LazyLock::new(|| parse_keys(include_str!("../trusted-metadata-signing-pubkeys")));

and the the bundled TLS cert in mullvad-api:

mullvadvpn-app/mullvad-api/src/tls_stream.rs

Line 18 in 060d8c4

const LE_ROOT_CERT: &[u8] = include_bytes!("../le_root_cert.pem");

Doing so would also make it easier to swap them out for test builds and similar.

[faern]

@faern made 1 comment.
Reviewable status: 3 of 11 files reviewed, 6 unresolved discussions (waiting on @kl).

---

mullvad-update/Cargo.toml line 17 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

This bump was needed to be compatible with the sigsum crate.

But 2.1 and 2.2 are semver compatible. The dependency definitions in sigsum should automatically bring in ^2.2 without any change in what we specify as a lower bound on what we depend on? Saying "2.1" here just means that our code works from 2.1 and all the way up to (not including) 3.0

[faern]

@faern made 1 comment.
Reviewable status: 3 of 11 files reviewed, 7 unresolved discussions (waiting on @kl).

---

mullvad-daemon/src/sigsum.rs line 59 at r1 (raw file):

#[cfg(test)]
mod test {

Can we maybe put these tests in separate files? Since they are very verbose and full of data that are not related to the actual implementation 🤔 I am actually in general more and more falling into the camp of "why the hell did Rust allow inline tests?" 😅

[kl]

@kl made 7 comments and resolved 3 discussions.
Reviewable status: 3 of 11 files reviewed, 4 unresolved discussions (waiting on @faern).

---

mullvad-api/src/relay_list.rs line 42 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Can we introduce a newtype here to encode that it is a sigsum digest? Would be very nice

Done

---

mullvad-api/src/relay_list.rs line 137 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

AFAICT this can be re-written as an async function directly

Done

---

mullvad-api/src/relay_list.rs line 152 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

AFAICT this can be re-written as an async function directly

Done

---

mullvad-api/src/relay_list.rs line 160 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Would it be possible to put all the sigsum related stuff into its own file submodule? This file is already as is over 500 LOC and pretty complex, and sigsum adds another layer of complexity.

This code is in the mullvad-api crate and the code that uses this is in mullvad-daemon which has a dependency on mullvad-api (but not the other way around). So I don't think we could put all sigsum related code in one place without creating a new crate?

---

mullvad-daemon/src/sigsum.rs line 8 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

To be able to track the trusted public keys easier with git, and to separate data from code, I think we should store them in separate files, and include them with include_str!. The same system we have for the trusted public keys of desktop releases in

mullvadvpn-app/mullvad-update/src/defaults.rs

Lines 27 to 28 in 060d8c4

	pub static TRUSTED_METADATA_SIGNING_PUBKEYS: LazyLock<Vec1<VerifyingKey>> =
	LazyLock::new(|| parse_keys(include_str!("../trusted-metadata-signing-pubkeys")));

and the the bundled TLS cert in mullvad-api:

mullvadvpn-app/mullvad-api/src/tls_stream.rs

Line 18 in 060d8c4

const LE_ROOT_CERT: &[u8] = include_bytes!("../le_root_cert.pem");

Doing so would also make it easier to swap them out for test builds and similar.

I put them in a new module sigsum in the file trusted-sigsum-signing-pubkeys

---

mullvad-daemon/src/sigsum.rs line 59 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can we maybe put these tests in separate files? Since they are very verbose and full of data that are not related to the actual implementation 🤔 I am actually in general more and more falling into the camp of "why the hell did Rust allow inline tests?" 😅

Done

---

mullvad-update/Cargo.toml line 17 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

But 2.1 and 2.2 are semver compatible. The dependency definitions in sigsum should automatically bring in ^2.2 without any change in what we specify as a lower bound on what we depend on? Saying "2.1" here just means that our code works from 2.1 and all the way up to (not including) 3.0

I got a version conflict here that was solved by bumping to 2.2 because that changed some things in the lockfile. cargo update would probably have fixed it as well, but wasn't sure if it was ok to run that.
I can revert this line if you want?

[faern]

@faern made 1 comment.
Reviewable status: 1 of 17 files reviewed, 4 unresolved discussions (waiting on @kl and @MarkusPettersson98).

---

mullvad-api/src/relay_list.rs line 160 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

This code is in the mullvad-api crate and the code that uses this is in mullvad-daemon which has a dependency on mullvad-api (but not the other way around). So I don't think we could put all sigsum related code in one place without creating a new crate?

I meant putting it in mullvad-api/src/relay_list/transparency_log.rs. Not a git submodule if that is how you interpreted my comment 😅

[faern]

@faern made 1 comment and resolved 1 discussion.
Reviewable status: 1 of 17 files reviewed, 3 unresolved discussions (waiting on @kl and @MarkusPettersson98).

---

mullvad-update/Cargo.toml line 17 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

I got a version conflict here that was solved by bumping to 2.2 because that changed some things in the lockfile. cargo update would probably have fixed it as well, but wasn't sure if it was ok to run that.
I can revert this line if you want?

Cargo should automatically pull in a newer version of a crate if needed, without us bumping this here. You just cannot build it with --locked, because that will prevent the upgrade.

Yes, doing a full cargo update of all crates is not recommended! (At least not as part of a PR not specifically upgrading everything for some reason). You can also specifically upgrade one crate with cargo update -p ed25519-dalek if you want to, but again, I do not think that should be needed if you drop --locked 🤔

[kl]

@kl made 2 comments.
Reviewable status: 1 of 17 files reviewed, 3 unresolved discussions (waiting on @faern and @MarkusPettersson98).

---

mullvad-api/src/relay_list.rs line 160 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I meant putting it in mullvad-api/src/relay_list/transparency_log.rs. Not a git submodule if that is how you interpreted my comment 😅

Gotcha! Makes sense. I thought you mean put all sigsum related code across all crates in one module. I put the TL related types in a new ``mullvad-api/src/relay_list/transparency_log.rs`. Let me know if you think more things should live there as well.

---

mullvad-update/Cargo.toml line 17 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Cargo should automatically pull in a newer version of a crate if needed, without us bumping this here. You just cannot build it with --locked, because that will prevent the upgrade.

Yes, doing a full cargo update of all crates is not recommended! (At least not as part of a PR not specifically upgrading everything for some reason). You can also specifically upgrade one crate with cargo update -p ed25519-dalek if you want to, but again, I do not think that should be needed if you drop --locked 🤔

Ah I see now, I had defined a clippy alias that used --locked, that's why I got that error. Thanks for the heads up.

[Rawa]

@Rawa partially reviewed 18 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faern and @kl).

---

mullvad-daemon/src/sigsum/mod.rs line 16 at r3 (raw file):

        .map(|key| {
            let key_hex = hex::decode(key).expect("invalid hex");
            let key_bytes: [u8; 32] = key_hex.as_slice().try_into().expect("invalid pubkey");

Right now this file does not contain a new line at the end, I presume this is part of spec that it should not have that? Because otherwise this parsing would fail. Not sure if CI would catch it?

[faern]

@faern made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl).

---

mullvad-api/src/relay_list.rs line 160 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

Gotcha! Makes sense. I thought you mean put all sigsum related code across all crates in one module. I put the TL related types in a new ``mullvad-api/src/relay_list/transparency_log.rs`. Let me know if you think more things should live there as well.

Looks like you named it transparency_list. Let's take the exact naming AFK as I feel we could bounce a few iterations and doing so in code pushes is not efficient.

[faern]

@faern made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kl).

---

a discussion (no related file):
This PR will need to add documentation to the repository around us now verifying that the relay list has been logged in a transparency log. I'm not sure exactly where it makes the most sense. docs/security.md is one good candidate where it should at least be mentioned. Maybe a separate document about just transparency logging is justified, with references to it in other documentation.

[kl]

@kl made 1 comment and resolved 1 discussion.
Reviewable status: 10 of 18 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98 and @Rawa).

---

mullvad-daemon/src/sigsum/mod.rs line 16 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Right now this file does not contain a new line at the end, I presume this is part of spec that it should not have that? Because otherwise this parsing would fail. Not sure if CI would catch it?

Made the parsing more flexible so it ignores leading and trailing whitespace and accepts comments with #

[Rawa]

@Rawa partially reviewed 6 files.
Reviewable status: 12 of 20 files reviewed, 3 unresolved discussions.

[kl]

@kl made 1 comment and resolved 1 discussion.
Reviewable status: 11 of 20 files reviewed, 2 unresolved discussions (waiting on @Rawa).

---

mullvad-api/src/relay_list.rs line 160 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Looks like you named it transparency_list. Let's take the exact naming AFK as I feel we could bounce a few iterations and doing so in code pushes is not efficient.

Moved to a new module mullvad-api/src/relay_list_transparency