openpgp: Enforce that KeyAmalgamations have a binding signature.

  - ValidKeyIter enforces that KeyAmalgamations have a valid binding
    signature.

  - Enforce this constraint when constructing a KeyAmalgamation and
    when modifying a KeyAmalgamation's policy.

6 files changed, 90 insertions, 51 deletions

diff --git a/openpgp-ffi/src/cert.rs b/openpgp-ffi/src/cert.rs
index dc62a7f1..241453c9 100644
--- a/openpgp-ffi/src/cert.rs
+++ b/openpgp-ffi/src/cert.rs
@@ -749,19 +749,17 @@ pub extern "C" fn pgp_cert_valid_key_iter_revoked<'a>(
     iter_wrapper.iter = tmp.revoked(Some(revoked));
 }
 
-/// Returns the next key.  Returns NULL if there are no more elements.
+/// Returns the next valid key.  Returns NULL if there are no more
+/// elements.
 ///
-/// If sigo is not NULL, stores the current self-signature (if any) in
-/// *sigo.  (Note: subkeys always have signatures, but a primary key
-/// may not have a direct signature, and there might not be any user
-/// ids.)
+/// If sigo is not NULL, stores the current self-signature.
 ///
 /// If rso is not NULL, this stores the key's revocation status in
 /// *rso.
 #[::sequoia_ffi_macros::extern_fn] #[no_mangle]
 pub extern "C" fn pgp_cert_valid_key_iter_next<'a>(
     iter_wrapper: *mut ValidKeyIterWrapper<'a>,
-    sigo: Option<&mut Maybe<Signature>>,
+    sigo: Option<&mut *mut Signature>,
     rso: Option<&mut *mut RevocationStatus<'a>>)
     -> Maybe<Key>
 {
diff --git a/openpgp-ffi/src/parse/stream.rs b/openpgp-ffi/src/parse/stream.rs
index 262ff596..67ef6c71 100644
--- a/openpgp-ffi/src/parse/stream.rs
+++ b/openpgp-ffi/src/parse/stream.rs
@@ -184,7 +184,7 @@ fn $fn_name<'a>(
     sig_r: Maybe<*mut Signature>,
     cert_r: Maybe<*mut Cert>,
     key_r: Maybe<*mut Key>,
-    binding_r: Maybe<Maybe<Signature>>,
+    binding_r: Maybe<*mut Signature>,
     revocation_status_r:
     Maybe<*mut RevocationStatus<'a>>)
     -> bool
diff --git a/openpgp/src/cert/builder.rs b/openpgp/src/cert/builder.rs
index 24172b0b..c5adc427 100644
--- a/openpgp/src/cert/builder.rs
+++ b/openpgp/src/cert/builder.rs
@@ -678,15 +678,15 @@ mod tests {
             .for_signing()
             .nth(0).unwrap();
         assert!(ka.alive().is_ok());
-        assert!(ka.clone().set_time(now + 290 * s).alive().is_ok());
-        assert!(! ka.clone().set_time(now + 310 * s).alive().is_ok());
+        assert!(ka.clone().set_time(now + 290 * s).unwrap().alive().is_ok());
+        assert!(! ka.clone().set_time(now + 310 * s).unwrap().alive().is_ok());
 
         let ka = cert.keys().policy(now).alive().revoked(false)
             .for_authentication()
             .nth(0).unwrap();
         assert!(ka.alive().is_ok());
-        assert!(ka.clone().set_time(now + 590 * s).alive().is_ok());
-        assert!(! ka.clone().set_time(now + 610 * s).alive().is_ok());
+        assert!(ka.clone().set_time(now + 590 * s).unwrap().alive().is_ok());
+        assert!(! ka.clone().set_time(now + 610 * s).unwrap().alive().is_ok());
     }
 
     #[test]
@@ -708,7 +708,7 @@ mod tests {
         assert_eq!(cert.keys().policy(None).count(), 2);
         for ka in cert.keys().policy(None) {
             assert_eq!(ka.key().creation_time(), UNIX_EPOCH);
-            assert_eq!(ka.binding_signature().unwrap()
+            assert_eq!(ka.binding_signature()
                        .signature_creation_time().unwrap(), UNIX_EPOCH);
         }
 
diff --git a/openpgp/src/cert/key_amalgamation.rs b/openpgp/src/cert/key_amalgamation.rs
index 85695f2e..8c510a18 100644
--- a/openpgp/src/cert/key_amalgamation.rs
+++ b/openpgp/src/cert/key_amalgamation.rs
@@ -32,33 +32,49 @@ enum KeyAmalgamationBinding<'a, P: key::KeyParts> {
 #[derive(Debug, Clone)]
 pub struct KeyAmalgamation<'a, P: key::KeyParts> {
     cert: &'a Cert,
-    time: SystemTime,
     binding: KeyAmalgamationBinding<'a, P>,
+
+    // The reference time.
+    time: SystemTime,
+    // The binding siganture at time `time`.  (This is just a cache.)
+    binding_signature: &'a Signature,
 }
 
-impl<'a, P> From<(&'a Cert, SystemTime)>
+impl<'a, P> TryFrom<(&'a Cert, SystemTime)>
     for KeyAmalgamation<'a, P>
     where P: key::KeyParts
 {
-    fn from(x: (&'a Cert, SystemTime)) -> Self {
-        KeyAmalgamation {
+    type Error = failure::Error;
+
+    fn try_from(x: (&'a Cert, SystemTime)) -> Result<Self> {
+        Ok(KeyAmalgamation {
             cert: x.0,
             binding: KeyAmalgamationBinding::Primary(),
             time: x.1,
-        }
+            binding_signature:
+                x.0.primary_key_signature(x.1)
+                    .ok_or_else(|| Error::NoBindingSignature(x.1))?,
+        })
     }
 }
 
-impl<'a, P> From<(&'a Cert, &'a KeyBinding<P, key::SubordinateRole>, SystemTime)>
+impl<'a, P> TryFrom<(&'a Cert, &'a KeyBinding<P, key::SubordinateRole>, SystemTime)>
     for KeyAmalgamation<'a, P>
     where P: key::KeyParts
 {
-    fn from(x: (&'a Cert, &'a KeyBinding<P, key::SubordinateRole>, SystemTime)) -> Self {
-        KeyAmalgamation {
+    type Error = failure::Error;
+
+    fn try_from(x: (&'a Cert, &'a KeyBinding<P, key::SubordinateRole>, SystemTime))
+        -> Result<Self>
+    {
+        Ok(KeyAmalgamation {
             cert: x.0,
             binding: KeyAmalgamationBinding::Subordinate(x.1),
             time: x.2,
-        }
+            binding_signature:
+                x.1.binding_signature(x.2)
+                    .ok_or_else(|| Error::NoBindingSignature(x.2))?,
+        })
     }
 }
 
@@ -73,21 +89,25 @@ impl<'a> From<KeyAmalgamation<'a, key::PublicParts>>
                 cert,
                 binding: KeyAmalgamationBinding::Primary(),
                 time,
+                binding_signature,
             } =>
                 KeyAmalgamation {
                     cert,
                     binding: KeyAmalgamationBinding::Primary(),
                     time,
+                    binding_signature,
                 },
             KeyAmalgamation {
                 cert,
                 binding: KeyAmalgamationBinding::Subordinate(binding),
                 time,
+                binding_signature,
             } =>
                 KeyAmalgamation {
                     cert,
                     binding: KeyAmalgamationBinding::Subordinate(binding.into()),
                     time,
+                    binding_signature,
                 },
         }
     }
@@ -101,22 +121,26 @@ impl<'a> From<KeyAmalgamation<'a, key::SecretParts>>
             KeyAmalgamation {
                 cert,
                 binding: KeyAmalgamationBinding::Primary(),
-                time
+                time,
+                binding_signature,
             } =>
                 KeyAmalgamation {
                     cert,
                     binding: KeyAmalgamationBinding::Primary(),
                     time,
+                    binding_signature,
                 },
             KeyAmalgamation {
                 cert,
                 binding: KeyAmalgamationBinding::Subordinate(binding),
-                time
+                time,
+                binding_signature,
             } =>
                 KeyAmalgamation {
                     cert,
                     binding: KeyAmalgamationBinding::Subordinate(binding.into()),
                     time,
+                    binding_signature,
                 },
         }
     }
@@ -132,7 +156,8 @@ impl<'a> TryFrom<KeyAmalgamation<'a, key::PublicParts>>
             KeyAmalgamation {
                 cert,
                 binding: KeyAmalgamationBinding::Primary(),
-                time
+                time,
+                binding_signature,
             } => {
                 // Error out if the primary key does not have secret
                 // key material.
@@ -142,17 +167,20 @@ impl<'a> TryFrom<KeyAmalgamation<'a, key::PublicParts>>
                     cert,
                     binding: KeyAmalgamationBinding::Primary(),
                     time,
+                    binding_signature,
                 }
             }
             KeyAmalgamation {
                 cert,
                 binding: KeyAmalgamationBinding::Subordinate(binding),
-                time
+                time,
+                binding_signature,
             } =>
                 KeyAmalgamation {
                     cert,
                     binding: KeyAmalgamationBinding::Subordinate(binding.try_into()?),
                     time,
+                    binding_signature,
                 },
         })
     }
@@ -202,23 +230,32 @@ impl<'a, P: 'a + key::KeyParts> KeyAmalgamation<'a, P> {
     /// Changes the amalgamation's reference time.
     ///
     /// If `time` is `None`, the current time is used.
-    pub fn set_time<T>(mut self, time: T) -> Self
+    pub fn set_time<T>(mut self, time: T) -> Result<Self>
         where T: Into<Option<time::SystemTime>>
     {
         self.time = time.into().unwrap_or_else(SystemTime::now);
-        self
+        self.binding_signature = match self {
+            KeyAmalgamation {
+                binding: KeyAmalgamationBinding::Primary(),
+                ..
+            } =>
+                self.cert.primary_key_signature(self.time)
+                    .ok_or_else(|| Error::NoBindingSignature(self.time))?,
+            KeyAmalgamation {
+                binding: KeyAmalgamationBinding::Subordinate(binding),
+                ..
+            } =>
+                binding.binding_signature(self.time)
+                    .ok_or_else(|| Error::NoBindingSignature(self.time))?,
+        };
+        Ok(self)
     }
 
     /// Returns the key's binding signature as of the reference time,
     /// if any.
-    pub fn binding_signature(&self) -> Option<&'a Signature>
+    pub fn binding_signature(&self) -> &'a Signature
     {
-        match self {
-            KeyAmalgamation { binding: KeyAmalgamationBinding::Primary(), .. } =>
-                self.cert.primary_key_signature(self.time()),
-            KeyAmalgamation { binding: KeyAmalgamationBinding::Subordinate(ref binding), .. } =>
-                binding.binding_signature(self.time()),
-        }
+        self.binding_signature
     }
 
     /// Returns the key's revocation status as of the amalgamation's
@@ -246,7 +283,7 @@ impl<'a, P: 'a + key::KeyParts> KeyAmalgamation<'a, P> {
     /// reference time.
     pub fn key_flags(&self) -> Option<KeyFlags>
     {
-        self.binding_signature().map(|sig| sig.key_flags())
+        Some(self.binding_signature.key_flags())
     }
 
     /// Returns whether the key has at least one of the specified key
@@ -308,11 +345,7 @@ impl<'a, P: 'a + key::KeyParts> KeyAmalgamation<'a, P> {
     /// Note: this does not return whether the certificate is valid.
     pub fn alive(&self) -> Result<()>
     {
-        if let Some(sig) = self.binding_signature() {
-            sig.key_alive(self.generic_key(), self.time())
-        } else {
-            Err(Error::NoBindingSignature(self.time()).into())
-        }
+        self.binding_signature.key_alive(self.generic_key(), self.time())
     }
 
     /// Returns whether the key contains secret key material.
diff --git a/openpgp/src/cert/keyiter.rs b/openpgp/src/cert/keyiter.rs
index a9a689a8..c40658eb 100644
--- a/openpgp/src/cert/keyiter.rs
+++ b/openpgp/src/cert/keyiter.rs
@@ -303,7 +303,7 @@ impl<'a, P: 'a + key::KeyParts, R: 'a + key::KeyRole> KeyIter<'a, P, R>
     {
         if let Some(cert) = self.cert.as_ref() {
             let time = time.into().unwrap_or_else(std::time::SystemTime::now);
-            let ka: KeyAmalgamation<'a, P> = (*cert, time).into();
+            let ka: KeyAmalgamation<'a, P> = (*cert, time).try_into()?;
             ka.alive()?;
             Ok(ka)
         } else {
@@ -474,9 +474,23 @@ impl<'a, P: 'a + key::KeyParts, R: 'a + key::KeyRole> ValidKeyIter<'a, P, R> {
         loop {
             let ka : KeyAmalgamation<'a, key::PublicParts> = if ! self.primary {
                 self.primary = true;
-                (cert, self.time).into()
+                match (cert, self.time).try_into() {
+                    Ok(ka) => ka,
+                    Err(err) => {
+                        // The primary key is bad.  Abort.
+                        t!("Getting primary key: {:?}", err);
+                        return None;
+                    }
+                }
             } else {
-                (cert, self.subkey_iter.next()?, self.time).into()
+                match (cert, self.subkey_iter.next()?, self.time).try_into() {
+                    Ok(ka) => ka,
+                    Err(err) => {
+                        // The subkey is bad, abort.
+                        t!("Getting subkey: {:?}", err);
+                        continue;
+                    }
+                }
             };
 
             let key = ka.key();
@@ -493,13 +507,7 @@ impl<'a, P: 'a + key::KeyParts, R: 'a + key::KeyRole> ValidKeyIter<'a, P, R> {
                 }
             }
 
-            let binding_signature
-                = if let Some(binding_signature) = ka.binding_signature() {
-                    binding_signature
-                } else {
-                    t!("No self-signature at time {:?}", self.time);
-                    continue
-                };
+            let binding_signature = ka.binding_signature();
 
             if let Some(flags) = self.flags.as_ref() {
                 if !ka.has_any_key_flag(flags) {
diff --git a/tool/src/commands/inspect.rs b/tool/src/commands/inspect.rs
index d56f4503..6cba96a1 100644
--- a/tool/src/commands/inspect.rs
+++ b/tool/src/commands/inspect.rs
@@ -141,7 +141,7 @@ fn inspect_cert(output: &mut dyn io::Write, cert: &openpgp::Cert,
     for ka in cert.keys().policy(None).skip(1) {
         writeln!(output, "         Subkey: {}", ka.key().fingerprint())?;
         inspect_revocation(output, "", ka.revoked())?;
-        inspect_key(output, "", ka.key(), ka.binding_signature(),
+        inspect_key(output, "", ka.key(), Some(ka.binding_signature()),
                     ka.binding().certifications(),
                     print_keygrips, print_certifications)?;
         writeln!(output)?;