Merge pull request #294 from thomastaylor312/feat/client_verification…

…_for_realz

feat(*): Adds required verification to the client

bin/client/main.rs

@@ -125,9 +125,24 @@ async fn run() -> std::result::Result<(), ClientError> {
         PickYourAuth::None(NoToken)
     };
 
+    let keyring_path: PathBuf = opts
+        .keyring
+        .unwrap_or_else(|| default_config_dir().join("keyring.toml"));
+
+    if let Some(p) = keyring_path.parent() {
+        tokio::fs::create_dir_all(p).await?;
+    }
+    let keyring = Arc::new(
+        keyring_path
+            .load()
+            .await
+            .unwrap_or_else(|_| KeyRing::default()),
+    );
+
     let bindle_client = ClientBuilder::default()
         .danger_accept_invalid_certs(opts.insecure)
-        .build(&opts.server_url, token)?;
+        .verification_strategy(opts.strategy)
+        .build(&opts.server_url, token, keyring)?;
 
     let local = bindle::provider::file::FileProvider::new(
         bindle_dir,
@@ -136,18 +151,6 @@ async fn run() -> std::result::Result<(), ClientError> {
     .await;
     let cache = DumbCache::new(bindle_client.clone(), local);
 
-    let keyring_path: PathBuf = opts
-        .keyring
-        .unwrap_or_else(|| default_config_dir().join("keyring.toml"));
-
-    if let Some(p) = keyring_path.parent() {
-        tokio::fs::create_dir_all(p).await?;
-    }
-    let mut keyring = keyring_path
-        .load()
-        .await
-        .unwrap_or_else(|_| KeyRing::default());
-
     match opts.subcmd {
         SubCommand::Info(info_opts) => {
             let inv = match info_opts.yanked {
@@ -362,7 +365,7 @@ async fn run() -> std::result::Result<(), ClientError> {
                         Err(e) if matches!(e.kind(), std::io::ErrorKind::NotFound) => {
                             println!("File {} does not exist. Creating it.", dir.display());
                             let mut keyfile = SecretKeyFile::default();
-                            let newkey = SecretKeyEntry::new(create_opts.label, roles);
+                            let newkey = SecretKeyEntry::new(&create_opts.label, roles);
                             keyfile.key.push(newkey.clone());
                             keyfile
                                 .save_file(dir)
@@ -381,7 +384,7 @@ async fn run() -> std::result::Result<(), ClientError> {
                             let mut keyfile = SecretKeyFile::load_file(&dir)
                                 .await
                                 .map_err(|e| ClientError::Other(e.to_string()))?;
-                            let newkey = SecretKeyEntry::new(create_opts.label, roles);
+                            let newkey = SecretKeyEntry::new(&create_opts.label, roles);
                             keyfile.key.push(newkey.clone());
                             keyfile
                                 .save_file(dir)
@@ -394,6 +397,10 @@ async fn run() -> std::result::Result<(), ClientError> {
                     };
 
                     if !create_opts.skip_keyring {
+                        let mut keyring = keyring_path
+                            .load()
+                            .await
+                            .unwrap_or_else(|_| KeyRing::default());
                         keyring.add_entry(key.try_into()?);
                         keyring_path
                             .save(&keyring)
@@ -402,6 +409,10 @@ async fn run() -> std::result::Result<(), ClientError> {
                     }
                 }
                 Keys::Add(opts) => {
+                    let mut keyring = keyring_path
+                        .load()
+                        .await
+                        .unwrap_or_else(|_| KeyRing::default());
                     // First, check that the key is actually valid
                     let raw = base64::decode(&opts.key)
                         .map_err(|_| SignatureError::CorruptKey(opts.key.clone()))?;

bin/client/opts.rs

@@ -1,3 +1,4 @@
+use bindle::VerificationStrategy;
 use clap::Parser;
 use std::path::PathBuf;
 
@@ -67,6 +68,13 @@ pub struct Opts {
     )]
     pub insecure: bool,
 
+    #[clap(
+        long = "strategy",
+        help = "The verification strategy to use when pulling the bindle",
+        default_value = "MultipleAttestationGreedy[Host]"
+    )]
+    pub strategy: VerificationStrategy,
+
     #[clap(subcommand)]
     pub subcmd: SubCommand,
 }

bin/server.rs

@@ -2,7 +2,7 @@ use std::net::SocketAddr;
 use std::path::PathBuf;
 
 use clap::Parser;
-use tracing::{info, warn};
+use tracing::{debug, info, warn};
 
 use bindle::{
     invoice::signature::{KeyRing, SignatureRole},
@@ -201,8 +201,14 @@ async fn main() -> anyhow::Result<()> {
     // - or config file signing-keys entry
     // - or $XDG_DATA/bindle/signing-keys.toml
     let signing_keys: PathBuf = match config.signing_file {
-        Some(keypath) => keypath,
-        None => ensure_signing_keys().await?,
+        Some(keypath) => {
+            debug!(path = %keypath.display(), "Signing keys file was set, loading...");
+            keypath
+        }
+        None => {
+            debug!("No signing key file set, attempting to load from default");
+            ensure_signing_keys().await?
+        }
     };
 
     // Map doesn't work here because we've already moved data out of opts
@@ -414,7 +420,7 @@ async fn ensure_signing_keys() -> anyhow::Result<PathBuf> {
                 "Creating a default host signing key and storing it in {}",
                 signing_keyfile.display()
             );
-            let key = SecretKeyEntry::new("Default host key".to_owned(), vec![SignatureRole::Host]);
+            let key = SecretKeyEntry::new("Default host key", vec![SignatureRole::Host]);
             default_keyfile.key.push(key);
             default_keyfile
                 .save_file(&signing_keyfile)

examples/client.rs

@@ -9,7 +9,11 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
     let root = std::env::var("CARGO_MANIFEST_DIR")?;
     let root_path = std::path::PathBuf::from(root);
 
-    let bindle_client = client::Client::new(&url, client::tokens::NoToken)?;
+    let bindle_client = client::Client::new(
+        &url,
+        client::tokens::NoToken,
+        std::sync::Arc::new(bindle::signature::KeyRing::default()),
+    )?;
 
     // Load an invoice manually and send it to the server
     println!("Creating invoice 1");

src/cache/lru.rs

@@ -411,7 +411,7 @@ mod test {
         // Make sure all the create operations pass through
         let provider = TestProvider::default();
         let cache = LruCache::new(10, provider.clone());
-        let sk = SecretKeyEntry::new("TEST".to_owned(), vec![SignatureRole::Proxy]);
+        let sk = SecretKeyEntry::new("TEST", vec![SignatureRole::Proxy]);
 
         let scaffold = testing::Scaffold::load("valid_v1").await;
         let verified = VerificationStrategy::MultipleAttestation(vec![])

src/client/mod.rs

@@ -7,6 +7,7 @@ pub mod tokens;
 
 use std::convert::TryInto;
 use std::path::Path;
+use std::sync::Arc;
 
 use reqwest::header::{self, HeaderMap};
 use reqwest::Client as HttpClient;
@@ -16,8 +17,9 @@ use tracing::{debug, info, instrument, trace};
 use url::Url;
 
 use crate::provider::{Provider, ProviderError};
-use crate::verification::Verified;
-use crate::{Id, Signed};
+use crate::signature::{KeyRing, SignatureRole};
+use crate::verification::{Verified, VerifiedInvoice};
+use crate::{Id, Invoice, Signed, VerificationStrategy};
 
 pub use error::ClientError;
 
@@ -36,6 +38,9 @@ pub struct Client<T> {
     client: HttpClient,
     base_url: Url,
     token_manager: T,
+    verification_strategy: VerificationStrategy,
+    // In an arc as the keyring could be fairly large and we don't want to clone it everywhere
+    keyring: Arc<KeyRing>,
 }
 
 /// The operation being performed against a Bindle server.
@@ -48,10 +53,22 @@ enum Operation {
 }
 
 /// A builder for for setting up a `Client`. Created using `Client::builder`
-#[derive(Default)]
 pub struct ClientBuilder {
     http2_prior_knowledge: bool,
     danger_accept_invalid_certs: bool,
+    verification_strategy: VerificationStrategy,
+}
+
+impl Default for ClientBuilder {
+    fn default() -> Self {
+        ClientBuilder {
+            http2_prior_knowledge: false,
+            danger_accept_invalid_certs: false,
+            verification_strategy: VerificationStrategy::MultipleAttestationGreedy(vec![
+                SignatureRole::Host,
+            ]),
+        }
+    }
 }
 
 impl ClientBuilder {
@@ -69,12 +86,30 @@ impl ClientBuilder {
         self
     }
 
-    /// Returns a new Client with the given URL and token manager, configured using the set options.
+    /// Sets the verification strategy this client should use.
+    ///
+    /// Defaults to MultipleAttestationGreedy(Host), which will validate that all signatures are
+    /// valid and will verify the signature of the host against the keyring
+    pub fn verification_strategy(mut self, verification_strategy: VerificationStrategy) -> Self {
+        self.verification_strategy = verification_strategy;
+        self
+    }
+
+    /// Returns a new Client with the given URL, token manager, and keyring, configured using the
+    /// set options.
     ///
     /// This URL should be the FQDN plus any namespacing (like `v1`). So if you were running a
     /// bindle server mounted at the v1 endpoint, your URL would look something like
     /// `http://my.bindle.com/v1/`. Will return an error if the URL is not valid
-    pub fn build<T>(self, base_url: &str, token_manager: T) -> Result<Client<T>> {
+    ///
+    /// This requires `KeyRing` to be wrapped in an `Arc` for efficiency sake. `KeyRing`s can be
+    /// quite large and so we require it to be wrapped in an Arc to avoid a large clone cost
+    pub fn build<T>(
+        self,
+        base_url: &str,
+        token_manager: T,
+        keyring: Arc<KeyRing>,
+    ) -> Result<Client<T>> {
         let (base_parsed, headers) = base_url_and_headers(base_url)?;
         let client = HttpClient::builder()
             .and_if(self.http2_prior_knowledge, |b| b.http2_prior_knowledge())
@@ -89,6 +124,8 @@ impl ClientBuilder {
             client,
             base_url: base_parsed,
             token_manager,
+            verification_strategy: self.verification_strategy,
+            keyring,
         })
     }
 }
@@ -113,8 +150,11 @@ impl<T: tokens::TokenManager> Client<T> {
     /// This URL should be the FQDN plus any namespacing (like `v1`). So if you were running a
     /// bindle server mounted at the v1 endpoint, your URL would look something like
     /// `http://my.bindle.com/v1/`. Will return an error if the URL is not valid
-    pub fn new(base_url: &str, token_manager: T) -> Result<Self> {
-        ClientBuilder::default().build(base_url, token_manager)
+    ///
+    /// This requires `KeyRing` to be wrapped in an `Arc` for efficiency sake. `KeyRing`s can be
+    /// quite large and so we require it to be wrapped in an Arc to avoid a large clone cost
+    pub fn new(base_url: &str, token_manager: T, keyring: Arc<KeyRing>) -> Result<Self> {
+        ClientBuilder::default().build(base_url, token_manager, keyring)
     }
 
     /// Returns a [`ClientBuilder`](ClientBuilder) configured with defaults
@@ -200,12 +240,16 @@ impl<T: tokens::TokenManager> Client<T> {
 
     //////////////// Get Invoice ////////////////
 
-    /// Returns the requested invoice from the bindle server if it exists. This can take any form
-    /// that can convert into the `Id` type, but generally speaking, this is the canonical name of
-    /// the bindle (e.g. `example.com/foo/1.0.0`). If you want to fetch a yanked invoice, use the
-    /// [`get_yanked_invoice`](Client::get_yanked_invoice) function
+    /// Returns the requested invoice from the bindle server if it exists.
+    ///
+    /// This can take any form that can convert into the `Id` type, but generally speaking, this is
+    /// the canonical name of the bindle (e.g. `example.com/foo/1.0.0`). If you want to fetch a
+    /// yanked invoice, use the [`get_yanked_invoice`](Client::get_yanked_invoice) function
+    ///
+    /// Once the invoice is fetched, it will be verified using the configured verification strategy
+    /// and keyring for this client
     #[instrument(level = "trace", skip(self, id), fields(invoice_id))]
-    pub async fn get_invoice<I>(&self, id: I) -> Result<crate::Invoice>
+    pub async fn get_invoice<I>(&self, id: I) -> Result<VerifiedInvoice<Invoice>>
     where
         I: TryInto<Id>,
         I::Error: Into<ClientError>,
@@ -221,7 +265,7 @@ impl<T: tokens::TokenManager> Client<T> {
 
     /// Same as `get_invoice` but allows you to fetch a yanked invoice
     #[instrument(level = "trace", skip(self, id), fields(invoice_id))]
-    pub async fn get_yanked_invoice<I>(&self, id: I) -> Result<crate::Invoice>
+    pub async fn get_yanked_invoice<I>(&self, id: I) -> Result<VerifiedInvoice<Invoice>>
     where
         I: TryInto<Id>,
         I::Error: Into<ClientError>,
@@ -235,13 +279,14 @@ impl<T: tokens::TokenManager> Client<T> {
         self.get_invoice_request(url).await
     }
 
-    async fn get_invoice_request(&self, url: Url) -> Result<crate::Invoice> {
+    async fn get_invoice_request(&self, url: Url) -> Result<VerifiedInvoice<Invoice>> {
         let req = self.client.get(url);
         let req = self.token_manager.apply_auth_header(req).await?;
         trace!(?req);
         let resp = req.send().await?;
         let resp = unwrap_status(resp, Endpoint::Invoice, Operation::Get).await?;
-        Ok(toml::from_slice(&resp.bytes().await?)?)
+        let inv: Invoice = toml::from_slice(&resp.bytes().await?)?;
+        Ok(self.verification_strategy.verify(inv, &self.keyring)?)
     }
 
     //////////////// Query Invoice ////////////////
@@ -495,6 +540,7 @@ impl<T: tokens::TokenManager + Send + Sync + 'static> Provider for Client<T> {
         self.get_yanked_invoice(parsed_id)
             .await
             .map_err(|e| e.into())
+            .map(|inv| inv.into())
     }
 
     async fn yank_invoice<I>(&self, id: I) -> crate::provider::Result<()>

src/invoice/mod.rs

@@ -455,8 +455,8 @@ mod test {
             .expect("If no signature, then this should verify fine");
 
         // Create two signing keys.
-        let signer_name1 = "Matt Butcher <[email protected]>".to_owned();
-        let signer_name2 = "Not Matt Butcher <[email protected]>".to_owned();
+        let signer_name1 = "Matt Butcher <[email protected]>";
+        let signer_name2 = "Not Matt Butcher <[email protected]>";
 
         let keypair1 = SecretKeyEntry::new(signer_name1, vec![SignatureRole::Creator]);
         let keypair2 = SecretKeyEntry::new(signer_name2, vec![SignatureRole::Proxy]);

src/invoice/signature.rs

@@ -165,6 +165,7 @@ impl<T: AsRef<Path> + Sync> KeyRingSaver for T {
             .await?;
 
         file.write_all(&toml::to_vec(keyring)?).await?;
+        file.flush().await?;
         Ok(())
     }
 }
@@ -341,12 +342,12 @@ pub struct SecretKeyEntry {
 }
 
 impl SecretKeyEntry {
-    pub fn new(label: String, roles: Vec<SignatureRole>) -> Self {
+    pub fn new(label: &str, roles: Vec<SignatureRole>) -> Self {
         let mut rng = rand::rngs::OsRng {};
         let rawkey = Keypair::generate(&mut rng);
         let keypair = base64::encode(rawkey.to_bytes());
         Self {
-            label,
+            label: label.to_owned(),
             keypair,
             roles,
         }
@@ -489,10 +490,8 @@ mod test {
     async fn test_secret_keys() {
         let mut kr = SecretKeyFile::default();
         assert_eq!(kr.key.len(), 0);
-        kr.key.push(SecretKeyEntry::new(
-            "test".to_owned(),
-            vec![SignatureRole::Proxy],
-        ));
+        kr.key
+            .push(SecretKeyEntry::new("test", vec![SignatureRole::Proxy]));
         assert_eq!(kr.key.len(), 1);
 
         let outdir = tempfile::tempdir().expect("created a temp dir");