feat: implement pgp signature validation

Uses the pgp crate to validate signatures on downloaded artifacts when they are available and warns if those are not valid.

Ref #2028

Author: dignifiedquire

Cargo.lock

@@ -54,6 +54,7 @@ toml = "0.5"
 url = "1"
 wait-timeout = "0.2"
 xz2 = "0.1.3"
+pgp = { version = "0.3.1", default-features = false}
 
 [dependencies.retry]
 version = "0.5"

Cargo.toml

@@ -7,6 +7,8 @@ use std::process::Command;
 use std::str::FromStr;
 use std::sync::Arc;
 
+use pgp::{Deserializable, SignedPublicKey};
+
 use crate::dist::{dist, temp};
 use crate::errors::*;
 use crate::notifications::*;
@@ -33,21 +35,24 @@ impl Display for OverrideReason {
     }
 }
 
+lazy_static::lazy_static! {
+    static ref BUILTIN_PGP_KEY: SignedPublicKey = pgp::SignedPublicKey::from_armor_single(
+        io::Cursor::new(&include_bytes!("rust-key.pgp.ascii")[..])
+    ).unwrap().0;
+}
+
 #[derive(Debug)]
 pub enum PgpPublicKey {
-    Builtin(&'static [u8]),
-    FromEnvironment(PathBuf, Vec<u8>),
-    FromConfiguration(PathBuf, Vec<u8>),
+    Builtin,
+    FromEnvironment(PathBuf, SignedPublicKey),
+    FromConfiguration(PathBuf, SignedPublicKey),
 }
 
 impl PgpPublicKey {
-    /// Retrieve the key data for this key
-    ///
-    /// This key might be ASCII Armored or may not, we make no
-    /// guarantees.
-    pub fn key_data(&self) -> &[u8] {
+    /// Retrieve the key.
+    pub fn key(&self) -> &SignedPublicKey {
         match self {
-            Self::Builtin(k) => k,
+            Self::Builtin => &*BUILTIN_PGP_KEY,
             Self::FromEnvironment(_, k) => &k,
             Self::FromConfiguration(_, k) => &k,
         }
@@ -57,7 +62,7 @@ impl PgpPublicKey {
 impl Display for PgpPublicKey {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            Self::Builtin(_) => write!(f, "builtin Rust release key"),
+            Self::Builtin => write!(f, "builtin Rust release key"),
             Self::FromEnvironment(p, _) => {
                 write!(f, "key specified in RUST_PGP_KEY ({})", p.display())
             }
@@ -98,18 +103,24 @@ impl Cfg {
         let download_dir = rustup_dir.join("downloads");
 
         // PGP keys
-        let mut pgp_keys: Vec<PgpPublicKey> =
-            vec![PgpPublicKey::Builtin(include_bytes!("rust-key.pgp.ascii"))];
-        if let Some(s_path) = env::var_os("RUSTUP_PGP_KEY") {
+        let mut pgp_keys: Vec<PgpPublicKey> = vec![PgpPublicKey::Builtin];
+
+        if let Some(ref s_path) = env::var_os("RUSTUP_PGP_KEY") {
             let path = PathBuf::from(s_path);
-            let content = utils::read_file_bytes("RUSTUP_PGP_KEY", &path)?;
-            pgp_keys.push(PgpPublicKey::FromEnvironment(path, content));
+            let file = utils::open_file("RUSTUP_PGP_KEY", &path)?;
+            let (key, _) = SignedPublicKey::from_armor_single(file)
+                .map_err(|error| ErrorKind::InvalidPgpKey(PathBuf::from(s_path), error))?;
+
+            pgp_keys.push(PgpPublicKey::FromEnvironment(path, key));
         }
         settings_file.with(|s| {
             if let Some(s) = &s.pgp_keys {
                 let path = PathBuf::from(s);
-                let content = utils::read_file_bytes("PGP Key from config", &path)?;
-                pgp_keys.push(PgpPublicKey::FromConfiguration(path, content));
+                let file = utils::open_file("PGP Key from config", &path)?;
+                let (key, _) = SignedPublicKey::from_armor_single(file)
+                    .map_err(|error| ErrorKind::InvalidPgpKey(PathBuf::from(s), error))?;
+
+                pgp_keys.push(PgpPublicKey::FromConfiguration(path, key));
             }
             Ok(())
         })?;

src/config.rs

@@ -844,6 +844,7 @@ fn try_update_from_dist_<'a>(
         update_hash,
         &download.temp_cfg,
         &download.notify_handler,
+        &download.pgp_keys,
     ) {
         Ok(None) => Ok(None),
         Ok(Some(hash)) => Ok(Some(hash)),

src/dist/dist.rs

@@ -1,7 +1,9 @@
+use crate::config::PgpPublicKey;
 use crate::dist::notifications::*;
 use crate::dist::temp;
 use crate::errors::*;
 use crate::utils::utils;
+
 use sha2::{Digest, Sha256};
 use url::Url;
 
@@ -17,6 +19,7 @@ pub struct DownloadCfg<'a> {
     pub temp_cfg: &'a temp::Cfg,
     pub download_dir: &'a PathBuf,
     pub notify_handler: &'a dyn Fn(Notification<'_>),
+    pub pgp_keys: &'a [PgpPublicKey],
 }
 
 pub struct File {
@@ -119,9 +122,50 @@ impl<'a> DownloadCfg<'a> {
         Ok(utils::read_file("hash", &hash_file).map(|s| s[0..64].to_owned())?)
     }
 
+    fn download_signature(&self, url: &str) -> Result<String> {
+        let sig_url = utils::parse_url(&(url.to_owned() + ".asc"))?;
+        let sig_file = self.temp_cfg.new_file()?;
+
+        utils::download_file(&sig_url, &sig_file, None, &|n| {
+            (self.notify_handler)(n.into())
+        })?;
+
+        Ok(utils::read_file("signature", &sig_file).map(|s| s.to_owned())?)
+    }
+
+    fn check_signature(&self, url: &str, file: &temp::File<'_>) -> Result<()> {
+        assert!(
+            !self.pgp_keys.is_empty(),
+            "At least the builtin key must be present"
+        );
+
+        let signature = self.download_signature(url).map_err(|e| {
+            e.chain_err(|| ErrorKind::SignatureVerificationFailed {
+                url: url.to_owned(),
+            })
+        })?;
+
+        let file_path: &Path = &file;
+        let content = std::fs::File::open(file_path).chain_err(|| ErrorKind::ReadingFile {
+            name: "channel data",
+            path: PathBuf::from(file_path),
+        })?;
+
+        if !crate::dist::signatures::verify_signature(content, &signature, &self.pgp_keys)? {
+            Err(ErrorKind::SignatureVerificationFailed {
+                url: url.to_owned(),
+            }
+            .into())
+        } else {
+            Ok(())
+        }
+    }
+
     /// Downloads a file, sourcing its hash from the same url with a `.sha256` suffix.
     /// If `update_hash` is present, then that will be compared to the downloaded hash,
     /// and if they match, the download is skipped.
+    /// Verifies the signature found at the same url with a `.asc` suffix, and prints a
+    /// warning when the signature does not verify, or is not found.
     pub fn download_and_check(
         &self,
         url_str: &str,
@@ -167,7 +211,13 @@ impl<'a> DownloadCfg<'a> {
             (self.notify_handler)(Notification::ChecksumValid(url_str));
         }
 
-        // TODO: Check the signature of the file
+        // No signatures for tarballs for now.
+        if !url_str.ends_with(".tar.gz")
+            && !url_str.ends_with(".tar.xz")
+            && self.check_signature(&url_str, &file).is_err()
+        {
+            (self.notify_handler)(Notification::SignatureInvalid(url_str));
+        }
 
         Ok(Some((file, partial_hash)))
     }

src/dist/download.rs

@@ -1,6 +1,7 @@
 //! Maintains a Rust installation by installing individual Rust
 //! platform components from a distribution server.
 
+use crate::config::PgpPublicKey;
 use crate::dist::component::{Components, Package, TarGzPackage, TarXzPackage, Transaction};
 use crate::dist::config::Config;
 use crate::dist::dist::{Profile, TargetTriple, DEFAULT_DIST_SERVER};
@@ -351,6 +352,7 @@ impl Manifestation {
         update_hash: Option<&Path>,
         temp_cfg: &temp::Cfg,
         notify_handler: &dyn Fn(Notification<'_>),
+        pgp_keys: &[PgpPublicKey],
     ) -> Result<Option<String>> {
         // If there's already a v2 installation then something has gone wrong
         if self.read_config()?.is_some() {
@@ -388,6 +390,7 @@ impl Manifestation {
             download_dir: &dld_dir,
             temp_cfg,
             notify_handler,
+            pgp_keys,
         };
 
         let dl = dlcfg.download_and_check(&url, update_hash, ".tar.gz")?;

src/dist/manifestation.rs

@@ -13,3 +13,4 @@ pub mod manifest;
 pub mod manifestation;
 pub mod notifications;
 pub mod prefix;
+pub mod signatures;

src/dist/mod.rs

@@ -35,6 +35,7 @@ pub enum Notification<'a> {
     ManifestChecksumFailedHack,
     ComponentUnavailable(&'a str, Option<&'a TargetTriple>),
     StrayHash(&'a Path),
+    SignatureInvalid(&'a str),
 }
 
 impl<'a> From<crate::utils::Notification<'a>> for Notification<'a> {
@@ -79,6 +80,7 @@ impl<'a> Notification<'a> {
             | ForcingUnavailableComponent(_)
             | StrayHash(_) => NotificationLevel::Warn,
             NonFatalError(_) => NotificationLevel::Error,
+            SignatureInvalid(_) => NotificationLevel::Warn,
         }
     }
 }
@@ -171,6 +173,7 @@ impl<'a> Display for Notification<'a> {
             ForcingUnavailableComponent(component) => {
                 write!(f, "Force-skipping unavailable component '{}'", component)
             }
+            SignatureInvalid(url) => write!(f, "Signature verification failed for '{}'", url),
         }
     }
 }

src/dist/notifications.rs

@@ -0,0 +1,45 @@
+//! Signature verification support for Rustup.
+
+// TODO: Determine whether we want external keyring support
+
+use pgp::types::KeyTrait;
+use pgp::{Deserializable, StandaloneSignature};
+
+use crate::config::PgpPublicKey;
+use crate::errors::*;
+
+use std::io::Read;
+
+fn squish_internal_err<E: std::fmt::Display>(err: E) -> Error {
+    ErrorKind::SignatureVerificationInternalError(format!("{}", err)).into()
+}
+
+pub fn verify_signature<T: Read>(
+    mut content: T,
+    signature: &str,
+    keys: &[PgpPublicKey],
+) -> Result<bool> {
+    let mut content_buf = Vec::new();
+    content.read_to_end(&mut content_buf)?;
+    let (signatures, _) =
+        StandaloneSignature::from_string_many(signature).map_err(squish_internal_err)?;
+
+    for signature in signatures {
+        let signature = signature.map_err(squish_internal_err)?;
+
+        for key in keys {
+            let actual_key = key.key();
+            if actual_key.is_signing_key() && signature.verify(actual_key, &content_buf).is_ok() {
+                return Ok(true);
+            }
+
+            for sub_key in &actual_key.public_subkeys {
+                if sub_key.is_signing_key() && signature.verify(sub_key, &content_buf).is_ok() {
+                    return Ok(true);
+                }
+            }
+        }
+    }
+
+    Ok(false)
+}

src/dist/signatures.rs

@@ -208,6 +208,16 @@ error_chain! {
                     expected,
                     calculated)
         }
+        SignatureVerificationInternalError(msg: String) {
+            description("internal error verifying signature")
+            display("internal error verifying signature: {}", msg)
+        }
+        SignatureVerificationFailed {
+            url: String,
+        } {
+            description("signature verification failed")
+            display("signature verification failed for {}", url)
+        }
         ComponentConflict {
             name: String,
             path: PathBuf,
@@ -348,6 +358,10 @@ error_chain! {
             description("bad path in tar")
             display("tar path '{}' is not supported", v.display())
         }
+        InvalidPgpKey(v: PathBuf, error: pgp::errors::Error) {
+            description("invalid PGP key"),
+            display("unable to read the PGP key '{}'", v.display())
+        }
     }
 }
 

src/errors.rs

@@ -159,6 +159,7 @@ impl<'a> Toolchain<'a> {
             temp_cfg: &self.cfg.temp_cfg,
             download_dir: &self.cfg.download_dir,
             notify_handler: &*self.dist_handler,
+            pgp_keys: self.cfg.get_pgp_keys(),
         }
     }
 

src/toolchain.rs

@@ -37,6 +37,13 @@ where
     })
 }
 
+pub fn open_file(name: &'static str, path: &Path) -> Result<fs::File> {
+    fs::File::open(path).chain_err(|| ErrorKind::ReadingFile {
+        name,
+        path: PathBuf::from(path),
+    })
+}
+
 pub fn read_file_bytes(name: &'static str, path: &Path) -> Result<Vec<u8>> {
     fs::read(path).chain_err(|| ErrorKind::ReadingFile {
         name,

src/utils/utils.rs

@@ -979,6 +979,12 @@ fn make_component_unavailable(config: &Config, name: &str, target: &TargetTriple
     let hash_path = manifest_path.with_extension("toml.sha256");
     println!("{}", hash_path.display());
     create_hash(&manifest_path, &hash_path);
+
+    // update that signature
+    use crate::mock::dist::{create_signature, write_file};
+    let signature = create_signature(manifest_str.as_bytes()).unwrap();
+    let sig_path = manifest_path.with_extension("toml.asc");
+    write_file(&sig_path, &signature);
 }
 
 #[test]
@@ -1255,3 +1261,31 @@ info: installing component 'rustc'
         );
     });
 }
+
+/// Invalidates the signature on the manifest of the nigthly channel.
+fn make_signature_invalid(config: &Config) {
+    let manifest_path = config.distdir.join("dist/channel-rust-nightly.toml");
+
+    // Set signature to sth bogus.
+    use crate::mock::dist::{create_signature, write_file};
+    let signature = create_signature(b"hello invalid").unwrap();
+    let sig_path = manifest_path.with_extension("toml.asc");
+    write_file(&sig_path, &signature);
+}
+
+#[test]
+fn warn_on_invalid_signature() {
+    setup(&|config| {
+        make_signature_invalid(config);
+        let manifest_path = config.distdir.join("dist/channel-rust-nightly.toml");
+
+        expect_stderr_ok(
+            config,
+            &["rustup", "update", "nightly", "--no-self-update"],
+            &format!(
+                "warning: Signature verification failed for 'file://{}'",
+                manifest_path.display()
+            ),
+        );
+    });
+}