2024-04-10 pr feedback

Signed-off-by: Andrew Pan <[email protected]>

Author: tnytown

Cargo.toml

@@ -30,7 +30,7 @@ test-registry = []
 
 fulcio-native-tls = ["oauth-native-tls", "reqwest/native-tls", "fulcio"]
 fulcio-rustls-tls = ["oauth-rustls-tls", "reqwest/rustls-tls", "fulcio"]
-fulcio = []
+fulcio = ["serde_with"]
 
 oauth-native-tls = ["openidconnect/native-tls", "oauth"]
 oauth-rustls-tls = ["openidconnect/rustls-tls", "oauth"]
@@ -40,12 +40,12 @@ rekor-native-tls = ["reqwest/native-tls", "rekor"]
 rekor-rustls-tls = ["reqwest/rustls-tls", "rekor"]
 rekor = ["reqwest"]
 
-sigstore-trust-root = ["futures-util", "tough", "regex", "tokio/sync"]
-
-bundle = []
+bundle = ["sigstore_protobuf_specs"]
 sign = ["bundle"]
 verify = ["bundle"]
 
+sigstore-trust-root = ["bundle", "futures-util", "tough", "regex", "tokio/sync"]
+
 cosign-native-tls = [
   "oci-distribution/native-tls",
   "cert",
@@ -113,10 +113,10 @@ rsa = "0.9.2"
 scrypt = "0.11.0"
 serde = { version = "1.0.136", features = ["derive"] }
 serde_json = "1.0.79"
-serde_with = { version = "3.4.0", features = ["base64", "json"] }
+serde_with = { version = "3.4.0", features = ["base64", "json"], optional = true }
 sha2 = { version = "0.10.6", features = ["oid"] }
 signature = { version = "2.0" }
-sigstore_protobuf_specs = "0.3.2"
+sigstore_protobuf_specs = { version = "0.3.2", optional = true }
 thiserror = "1.0.30"
 tokio = { version = "1.17.0", features = ["rt"] }
 tokio-util = { version = "0.7.10", features = ["io-util"] }

src/crypto/verification_key.rs

@@ -331,7 +331,7 @@ impl CosignVerificationKey {
 
     /// Verify the signature provided has been actually generated by the given key
     /// when signing the provided prehashed message.
-    pub fn verify_prehash(&self, signature: Signature, msg: &[u8]) -> Result<()> {
+    pub(crate) fn verify_prehash(&self, signature: Signature, msg: &[u8]) -> Result<()> {
         let sig = match signature {
             Signature::Raw(data) => data.to_owned(),
             Signature::Base64Encoded(data) => BASE64_STD_ENGINE.decode(data)?,

src/errors.rs

@@ -109,9 +109,6 @@ pub enum SigstoreError {
     #[error("Certificate pool error: {0}")]
     CertificatePoolError(String),
 
-    #[error("Certificate invalid: {0}")]
-    InvalidCertError(String),
-
     #[error("Signing session expired")]
     ExpiredSigningSession(),
 

src/trust/sigstore/constants.rs

@@ -19,7 +19,7 @@ pub(crate) const SIGSTORE_TARGET_BASE: &str = "https://tuf-repo-cdn.sigstore.dev
 macro_rules! impl_static_resource {
     {$($name:literal,)+} => {
         #[inline]
-        pub(crate) fn static_resource(name: impl AsRef<str>) -> Option<&'static [u8]> {
+        pub(crate) fn static_resource<N>(name: N) -> Option<&'static [u8]> where N: AsRef<str> {
             match name.as_ref() {
             $(
                 $name => Some(include_bytes!(concat!(env!("CARGO_MANIFEST_DIR"), "/trust_root/prod/", $name)))

src/trust/sigstore/mod.rs

@@ -38,7 +38,7 @@
 /// ```
 use futures_util::TryStreamExt;
 use sha2::{Digest, Sha256};
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use tokio_util::bytes::BytesMut;
 
 use sigstore_protobuf_specs::dev::sigstore::{
@@ -62,7 +62,7 @@ pub struct SigstoreTrustRoot {
 
 impl SigstoreTrustRoot {
     /// Constructs a new trust repository established by a [tough::Repository].
-    pub async fn new(checkout_dir: Option<&Path>) -> Result<Self> {
+    pub async fn new(checkout_dir: Option<PathBuf>) -> Result<Self> {
         // These are statically defined and should always parse correctly.
         let metadata_base = url::Url::parse(constants::SIGSTORE_METADATA_BASE)?;
         let target_base = url::Url::parse(constants::SIGSTORE_TARGET_BASE)?;
@@ -77,8 +77,6 @@ impl SigstoreTrustRoot {
         .await
         .map_err(Box::new)?;
 
-        let checkout_dir = checkout_dir.map(ToOwned::to_owned);
-
         let trusted_root = {
             let data = Self::fetch_target(&repository, &checkout_dir, "trusted_root.json").await?;
             serde_json::from_slice(&data[..])?
@@ -105,12 +103,13 @@ impl SigstoreTrustRoot {
             }
         };
 
-        // Try reading the target from disk cache.
+        // First, try reading the target from disk cache.
         let data = if let Some(Ok(local_data)) = local_path.as_ref().map(std::fs::read) {
+            debug!("{}: reading from embedded resources", name.raw());
             local_data.to_vec()
         // Try reading the target embedded into the binary.
         } else if let Some(embedded_data) = constants::static_resource(name.raw()) {
-            debug!("read embedded target {}", name.raw());
+            debug!("{}: reading from remote", name.raw());
             embedded_data.to_vec()
         // If all else fails, read the data from the TUF repo.
         } else if let Ok(remote_data) = read_remote_target().await {
@@ -128,15 +127,15 @@ impl SigstoreTrustRoot {
         };
 
         let data = if Sha256::digest(&data)[..] != target.hashes.sha256[..] {
+            debug!("{}: out of date", name.raw());
             read_remote_target().await?.to_vec()
         } else {
             data
         };
 
-        // Write the up-to-date data back to the disk. This doesn't need to succeed, as we can
-        // always fetch the target again later.
+        // Write our updated data back to the disk.
         if let Some(local_path) = local_path {
-            let _ = std::fs::write(local_path, &data);
+            std::fs::write(local_path, &data)?;
         }
 
         Ok(data)

src/verify/policy.rs

@@ -109,7 +109,7 @@ impl<T: SingleX509ExtPolicy + const_oid::AssociatedOid> VerificationPolicy for T
 
         // Parse raw string without DER encoding.
         let val = std::str::from_utf8(ext.extn_value.as_bytes())
-            .expect("failed to parse constructed Extension!");
+            .or(Err(PolicyError::ExtensionNotFound))?;
 
         if val != self.value() {
             return Err(PolicyError::ExtensionCheckFailed {
@@ -172,7 +172,10 @@ pub struct AnyOf<'a> {
 }
 
 impl<'a> AnyOf<'a> {
-    pub fn new<I: IntoIterator<Item = &'a dyn VerificationPolicy>>(policies: I) -> Self {
+    pub fn new<I>(policies: I) -> Self
+    where
+        I: IntoIterator<Item = &'a dyn VerificationPolicy>,
+    {
         Self {
             children: policies.into_iter().collect(),
         }
@@ -200,7 +203,10 @@ pub struct AllOf<'a> {
 }
 
 impl<'a> AllOf<'a> {
-    pub fn new<I: IntoIterator<Item = &'a dyn VerificationPolicy>>(policies: I) -> Option<Self> {
+    pub fn new<I>(policies: I) -> Option<Self>
+    where
+        I: IntoIterator<Item = &'a dyn VerificationPolicy>,
+    {
         let children: Vec<_> = policies.into_iter().collect();
 
         // Without this, we'd be able to construct an `AllOf` containing an empty list of child

src/verify/verifier.rs

@@ -62,13 +62,16 @@ impl AsyncVerifier {
         })
     }
 
-    async fn verify_digest(
+    async fn verify_digest<P>(
         &self,
         input_digest: Sha256,
         bundle: Bundle,
-        policy: &impl VerificationPolicy,
+        policy: &P,
         offline: bool,
-    ) -> VerificationResult {
+    ) -> VerificationResult
+    where
+        P: VerificationPolicy,
+    {
         let input_digest = input_digest.finalize();
         let materials: CheckedBundle = bundle.try_into()?;
 
@@ -129,9 +132,9 @@ impl AsyncVerifier {
 
         // 4) Verify that the Rekor entry is consistent with the other signing
         //    materials
-        let Some(log_entry) = materials.tlog_entry(offline, &input_digest) else {
-            return Err(SignatureErrorKind::Transparency)?;
-        };
+        let log_entry = materials
+            .tlog_entry(offline, &input_digest)
+            .ok_or(SignatureErrorKind::Transparency)?;
         debug!("log entry is consistent with other materials");
 
         // 5) Verify the inclusion proof supplied by Rekor for this artifact,
@@ -166,13 +169,17 @@ impl AsyncVerifier {
 
     /// Verifies an input against the given Sigstore Bundle, ensuring conformance to the provided
     /// [`VerificationPolicy`].
-    pub async fn verify<R: AsyncRead + Unpin + Send>(
+    pub async fn verify<R, P>(
         &self,
         mut input: R,
         bundle: Bundle,
-        policy: &impl VerificationPolicy,
+        policy: &P,
         offline: bool,
-    ) -> VerificationResult {
+    ) -> VerificationResult
+    where
+        R: AsyncRead + Unpin + Send,
+        P: VerificationPolicy,
+    {
         // arbitrary buffer size, chosen to be a multiple of the digest size.
         let mut buf = [0u8; 1024];
         let mut hasher = Sha256::new();
@@ -228,13 +235,17 @@ impl Verifier {
 
     /// Verifies an input against the given Sigstore Bundle, ensuring conformance to the provided
     /// [`VerificationPolicy`].
-    pub fn verify<R: Read>(
+    pub fn verify<R, P>(
         &self,
         mut input: R,
         bundle: Bundle,
-        policy: &impl VerificationPolicy,
+        policy: &P,
         offline: bool,
-    ) -> VerificationResult {
+    ) -> VerificationResult
+    where
+        R: Read,
+        P: VerificationPolicy,
+    {
         let mut hasher = Sha256::new();
         io::copy(&mut input, &mut hasher).map_err(VerificationError::Input)?;