Add production ed25519 keys for mullvad-api and installer-downloader

[dlon]

This adds the keys to use for verifying versions to mullvad-api and installer-downloader.

Fix DES-1930

---

This change is 

[linear]

DES-1930 Update pubkeys in mullvad-api/installer-downloader

[pinkisemils]

Reviewed 4 of 11 files at r1.
Reviewable status: 3 of 11 files reviewed, all discussions resolved

[pinkisemils]

Reviewed 1 of 1 files at r2.
Reviewable status: 4 of 11 files reviewed, all discussions resolved

[pinkisemils]

Reviewable status: 4 of 11 files reviewed, all discussions resolved

---

mullvad-update/prod-pubkeys line 8 at r2 (raw file):

299e8b06355031781623de7c9a013eb88b07aee80476e2938ce01a7b623d29d0
# albin
af4f7762e3e13af87f2e8c8af8ce5cce456cd3390af6d881b1eb6f802786cc3f

NIT: Missing newline at the end here.

[faern]

Reviewed 6 of 11 files at r1, all commit messages.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions

---

mullvad-update/src/keys.rs line 9 at r2 (raw file):

/// Pubkeys used to verify metadata from the Mullvad API (production)
pub static PRODUCTION_KEYS: LazyLock<Vec1<VerifyingKey>> =
    LazyLock::new(|| parse_keys(include_str!("../prod-pubkeys")));

Can we name this file trusted-metadata-signing-pubkeys please? It does not hurt to be specific, and this is a very important file. I do not find the current name very clear as to what it contains. The static variable should be named accordingly also for consistency and ease of reading.

I personally do not think "production" needs to be in the name, since no other keys should be in the tool by default(?). If we need to have some way to easily include staging/dev keys then we can also slam "production" onto the name also. But then we need to make sure those keys are not included in any release builds.

---

mullvad-update/src/keys.rs line 13 at r2 (raw file):

/// Pubkeys used to verify metadata from stagemole
pub static STAGING_KEYS: LazyLock<Vec1<VerifyingKey>> =
    LazyLock::new(|| parse_keys(include_str!("../stagemole-pubkey")));

This should be compiled out in production. We should not ship any staging/dev stuff to users. Especially not in so sensitive areas such as what signing keys to trust.

---

mullvad-update/meta/src/platform.rs line 235 at r2 (raw file):

        format::SignedResponse::deserialize_and_verify(
            &mullvad_update::keys::PRODUCTION_KEYS,

Nit, but we can refer to ourselves as crate::

[faern]

Reviewed 2 of 11 files at r1.
Reviewable status: 10 of 11 files reviewed, 3 unresolved discussions (waiting on @dlon)

[dlon]

Reviewable status: 7 of 11 files reviewed, 3 unresolved discussions (waiting on @faern and @pinkisemils)

---

mullvad-update/meta/src/platform.rs line 235 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nit, but we can refer to ourselves as crate::

Nope. That's a different crate

---

mullvad-update/src/keys.rs line 9 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Can we name this file trusted-metadata-signing-pubkeys please? It does not hurt to be specific, and this is a very important file. I do not find the current name very clear as to what it contains. The static variable should be named accordingly also for consistency and ease of reading.

I personally do not think "production" needs to be in the name, since no other keys should be in the tool by default(?). If we need to have some way to easily include staging/dev keys then we can also slam "production" onto the name also. But then we need to make sure those keys are not included in any release builds.

Done!

---

mullvad-update/src/keys.rs line 13 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This should be compiled out in production. We should not ship any staging/dev stuff to users. Especially not in so sensitive areas such as what signing keys to trust.

Solved by adding cfg(test). Which probably doesn't make sense, but it's only needed in the test below for now.

[dlon]

Reviewable status: 5 of 15 files reviewed, 3 unresolved discussions (waiting on @faern and @pinkisemils)

---

mullvad-update/src/keys.rs line 13 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Solved by adding cfg(test). Which probably doesn't make sense, but it's only needed in the test below for now.

Removed instead. Probably fine, since we have no convenient way to use staging anyway.

[faern]

Reviewed 6 of 10 files at r3, all commit messages.
Reviewable status: 11 of 15 files reviewed, 1 unresolved discussion (waiting on @pinkisemils)

---

mullvad-update/meta/src/platform.rs line 133 at r3 (raw file):

            pinned_certificate: Some(PINNED_CERTIFICATE.clone()),
            url,
            verifying_keys: mullvad_update::keys::TRUSTED_METADATA_SIGNING_PUBKEYS.clone(),

I sort of understand the design where the keys are given as an argument. To decouple the runtime value from the code. But this is critical to security, we are only ever going to have one set of keys, and yet this PR has to pass this constant in in four places. I feel this makes it harder to audit for correctness as well as having more duplication. Can't we make it so that the code path that fetch this one json file always are forced to validate it with this set of keys, no way around it? (Except maybe conditionally compilation allow overriding the keys in development builds)

[dlon]

Reviewable status: 11 of 17 files reviewed, 1 unresolved discussion (waiting on @faern and @pinkisemils)

---

mullvad-update/meta/src/platform.rs line 133 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I sort of understand the design where the keys are given as an argument. To decouple the runtime value from the code. But this is critical to security, we are only ever going to have one set of keys, and yet this PR has to pass this constant in in four places. I feel this makes it harder to audit for correctness as well as having more duplication. Can't we make it so that the code path that fetch this one json file always are forced to validate it with this set of keys, no way around it? (Except maybe conditionally compilation allow overriding the keys in development builds)

Done. Do you believe it's absolutely necessary to hide the other methods?

[faern]

Reviewable status: 11 of 17 files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

---

mullvad-update/src/lib.rs line 9 at r4 (raw file):

pub use client::*;

pub mod keys;

I'm not sure I think this should be public? Or at least there is no need for it to be public, is there? The API surface becomes larger for a bunch of stuff we don't need, which makes it harder to understand. I suggest we lock down everything that does not need to be public.

[faern]

Reviewed 1 of 11 files at r3, 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

[dlon]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

---

mullvad-update/src/lib.rs line 9 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I'm not sure I think this should be public? Or at least there is no need for it to be public, is there? The API surface becomes larger for a bunch of stuff we don't need, which makes it harder to understand. I suggest we lock down everything that does not need to be public.

This will be fixed in another PR.

[faern]

Reviewable status: complete! all files reviewed, all discussions resolved