openpgp: Rework signature creation.

  - Move common logic to methods.  Do not reuse the direct key
    signature as template for user handle binding signatures, instead,
    use the new functions.

1 files changed, 51 insertions, 41 deletions

diff --git a/openpgp/src/cert/builder.rs b/openpgp/src/cert/builder.rs
index 7f65c5ab..d363f33a 100644
--- a/openpgp/src/cert/builder.rs
+++ b/openpgp/src/cert/builder.rs
@@ -9,7 +9,7 @@ use crate::packet::{
 };
 use crate::Result;
 use crate::packet::Signature;
-use crate::packet::signature;
+use crate::packet::signature::{self, SignatureBuilder};
 use crate::cert::prelude::*;
 use crate::Error;
 use crate::crypto::{Password, Signer};
@@ -975,42 +975,35 @@ impl CertBuilder<'_> {
                 }
                 primary
             }),
-            sig.clone().into(),
+            sig.into(),
         ])?;
 
-        let sig = signature::SignatureBuilder::from(sig)
-            .set_signature_creation_time(creation_time)?;
-
-        // Remove subpackets that needn't be copied into the binding
-        // signatures.
-        let sig = sig.set_revocation_key(vec![])?;
-
         let have_userids = !self.userids.is_empty();
 
         // Sign UserIDs.
         for (i, uid) in self.userids.into_iter().enumerate() {
-            let mut builder = sig.clone()
-                .set_type(SignatureType::PositiveCertification)
-                // GnuPG wants at least a 512-bit hash for P521 keys.
-                .set_hash_algo(HashAlgorithm::SHA512);
+            let sig =
+                SignatureBuilder::new(SignatureType::PositiveCertification);
+            let sig = Self::signature_common(sig, creation_time)?;
+            let mut sig = Self::add_primary_key_metadata(sig, &self.primary)?;
             if i == 0 {
-                builder = builder.set_primary_userid(true)?;
+                sig = sig.set_primary_userid(true)?;
             }
-            let signature = uid.bind(&mut signer, &cert, builder)?;
+            let signature = uid.bind(&mut signer, &cert, sig)?;
             cert = cert.insert_packets(
                 vec![Packet::from(uid), signature.into()])?;
         }
 
         // Sign UserAttributes.
         for (i, ua) in self.user_attributes.into_iter().enumerate() {
-            let mut builder = sig.clone()
-                .set_type(SignatureType::PositiveCertification)
-                 // GnuPG wants at least a 512-bit hash for P521 keys.
-                .set_hash_algo(HashAlgorithm::SHA512);
+            let sig =
+                SignatureBuilder::new(SignatureType::PositiveCertification);
+            let sig = Self::signature_common(sig, creation_time)?;
+            let mut sig = Self::add_primary_key_metadata(sig, &self.primary)?;
             if i == 0 && ! have_userids {
-                builder = builder.set_primary_userid(true)?;
+                sig = sig.set_primary_userid(true)?;
             }
-            let signature = ua.bind(&mut signer, &cert, builder)?;
+            let signature = ua.bind(&mut signer, &cert, sig)?;
             cert = cert.insert_packets(
                 vec![Packet::from(ua), signature.into()])?;
         }
@@ -1023,11 +1016,9 @@ impl CertBuilder<'_> {
                 .generate_key(flags)?;
             subkey.set_creation_time(creation_time)?;
 
-            let mut builder =
-                signature::SignatureBuilder::new(SignatureType::SubkeyBinding)
-                .set_signature_creation_time(creation_time)?
-                // GnuPG wants at least a 512-bit hash for P521 keys.
-                .set_hash_algo(HashAlgorithm::SHA512)
+            let sig = SignatureBuilder::new(SignatureType::SubkeyBinding);
+            let sig = Self::signature_common(sig, creation_time)?;
+            let mut builder = sig
                 .set_key_flags(flags.clone())?
                 .set_key_validity_period(blueprint.validity.or(self.primary.validity))?;
 
@@ -1074,21 +1065,9 @@ impl CertBuilder<'_> {
             .unwrap_or(self.ciphersuite)
             .generate_key(KeyFlags::empty().set_certification())?;
         key.set_creation_time(creation_time)?;
-        let mut sig = signature::SignatureBuilder::new(SignatureType::DirectKey)
-            // GnuPG wants at least a 512-bit hash for P521 keys.
-            .set_hash_algo(HashAlgorithm::SHA512)
-            .set_features(Features::sequoia())?
-            .set_key_flags(self.primary.flags.clone())?
-            .set_signature_creation_time(creation_time)?
-            .set_key_validity_period(self.primary.validity)?
-            .set_preferred_hash_algorithms(vec![
-                HashAlgorithm::SHA512,
-                HashAlgorithm::SHA256,
-            ])?
-            .set_preferred_symmetric_algorithms(vec![
-                SymmetricAlgorithm::AES256,
-                SymmetricAlgorithm::AES128,
-            ])?;
+        let sig = SignatureBuilder::new(SignatureType::DirectKey);
+        let sig = Self::signature_common(sig, creation_time)?;
+        let mut sig = Self::add_primary_key_metadata(sig, &self.primary)?;
 
         if let Some(ref revocation_keys) = self.revocation_keys {
             sig = sig.set_revocation_key(revocation_keys.clone())?;
@@ -1100,6 +1079,37 @@ impl CertBuilder<'_> {
 
         Ok((key, sig, Box::new(signer)))
     }
+
+    /// Common settings for generated signatures.
+    fn signature_common(builder: SignatureBuilder,
+                        creation_time: time::SystemTime)
+                        -> Result<SignatureBuilder>
+    {
+        builder
+            // GnuPG wants at least a 512-bit hash for P521 keys.
+            .set_hash_algo(HashAlgorithm::SHA512)
+            .set_signature_creation_time(creation_time)
+    }
+
+
+    /// Adds primary key metadata to the signature.
+    fn add_primary_key_metadata(builder: SignatureBuilder,
+                                primary: &KeyBlueprint)
+                                -> Result<SignatureBuilder>
+    {
+        builder
+            .set_features(Features::sequoia())?
+            .set_key_flags(primary.flags.clone())?
+            .set_key_validity_period(primary.validity)?
+            .set_preferred_hash_algorithms(vec![
+                HashAlgorithm::SHA512,
+                HashAlgorithm::SHA256,
+            ])?
+            .set_preferred_symmetric_algorithms(vec![
+                SymmetricAlgorithm::AES256,
+                SymmetricAlgorithm::AES128,
+            ])
+    }
 }
 
 #[cfg(test)]