openpgp: Add issuers and sort subpackets when canonicalizing Certs.

  - This makes sure that merging a cert with itself does not change
    the cert.

  - Fixes #579.

3 files changed, 76 insertions, 8 deletions

diff --git a/openpgp/src/cert/bundle.rs b/openpgp/src/cert/bundle.rs
index 374334c1..46b15a29 100644
--- a/openpgp/src/cert/bundle.rs
+++ b/openpgp/src/cert/bundle.rs
@@ -637,11 +637,26 @@ impl<C> ComponentBundle<C> {
             }
         }
 
+        // If two signatures are merged, we also do some fixups.  Make
+        // sure we also do this to signatures that are not merged, so
+        // that `cert.merge(cert) == cert`.
+        fn sig_fixup(sig: &mut Signature) {
+            // Add missing issuer information.  This is a best effort
+            // though.  If the unhashed area is full, there is nothing
+            // we can do.
+            let _ = sig.add_missing_issuers();
+
+            // Merging Signatures sorts the unhashed subpacket area.
+            // Do the same.
+            sig.unhashed_area_mut().sort();
+        }
+
         self.self_signatures.sort_by(Signature::normalized_cmp);
         self.self_signatures.dedup_by(sig_merge);
         // Order self signatures so that the most recent one comes
         // first.
         self.self_signatures.sort_by(sig_cmp);
+        self.self_signatures.iter_mut().for_each(sig_fixup);
 
         self.certifications.sort_by(Signature::normalized_cmp);
         self.certifications.dedup_by(sig_merge);
@@ -651,14 +666,17 @@ impl<C> ComponentBundle<C> {
         // cert::test::signature_order checks that the signatures are
         // sorted.
         self.certifications.sort_by(sig_cmp);
+        self.certifications.iter_mut().for_each(sig_fixup);
 
         self.self_revocations.sort_by(Signature::normalized_cmp);
         self.self_revocations.dedup_by(sig_merge);
         self.self_revocations.sort_by(sig_cmp);
+        self.self_revocations.iter_mut().for_each(sig_fixup);
 
         self.other_revocations.sort_by(Signature::normalized_cmp);
         self.other_revocations.dedup_by(sig_merge);
         self.other_revocations.sort_by(sig_cmp);
+        self.other_revocations.iter_mut().for_each(sig_fixup);
     }
 }
 
diff --git a/openpgp/src/cert/mod.rs b/openpgp/src/cert/mod.rs
index f0402e7c..73790d59 100644
--- a/openpgp/src/cert/mod.rs
+++ b/openpgp/src/cert/mod.rs
@@ -5557,4 +5557,41 @@ Pu1xwz57O4zo1VYf6TqHJzVC3OMvMUM2hhdecMUe5x6GorNaj6g=
         assert_eq!(sig.embedded_signatures().count(), 2);
         Ok(())
     }
+
+    /// Checks that Cert::merge(cert, cert) == cert.
+    #[test]
+    fn issue_579() -> Result<()> {
+        use std::convert::TryFrom;
+        use crate::packet::signature::subpacket::SubpacketTag;
+
+        let mut pp = crate::PacketPile::from_bytes(crate::tests::key(
+            "emmelie-dorothea-dina-samantha-awina-ed25519.pgp"))?;
+        assert_eq!(pp.children().count(), 5);
+        // Drop issuer information from the unhashed areas.
+        if let Some(Packet::Signature(sig)) = pp.path_ref_mut(&[2]) {
+            sig.unhashed_area_mut().remove_all(SubpacketTag::Issuer);
+        } else {
+            panic!("expected a signature");
+        }
+        if let Some(Packet::Signature(sig)) = pp.path_ref_mut(&[4]) {
+            sig.unhashed_area_mut().remove_all(SubpacketTag::Issuer);
+        } else {
+            panic!("expected a signature");
+        }
+
+        let cert = Cert::try_from(pp)?;
+        assert_eq!(cert.clone().merge(cert.clone())?, cert);
+
+        // Specifically, the issuer information should have been added
+        // back by the canonicalization.
+        assert_eq!(
+            cert.userids().nth(0).unwrap().self_signatures()[0]
+                .unhashed_area().subpackets(SubpacketTag::Issuer).count(),
+            1);
+        assert_eq!(
+            cert.keys().subkeys().nth(0).unwrap().self_signatures()[0]
+                .unhashed_area().subpackets(SubpacketTag::Issuer).count(),
+            1);
+        Ok(())
+    }
 }
diff --git a/openpgp/src/packet/signature.rs b/openpgp/src/packet/signature.rs
index e5fe662d..3c217a85 100644
--- a/openpgp/src/packet/signature.rs
+++ b/openpgp/src/packet/signature.rs
@@ -3463,20 +3463,33 @@ mod test {
             panic!("expected a signature");
         }
 
-        // Parse into cert verifying the signatures.
-        use std::convert::TryFrom;
-        let cert = Cert::try_from(pp)?;
-        assert_eq!(cert.bad_signatures().len(), 0);
-        assert_eq!(cert.keys().subkeys().count(), 1);
-        let subkey = cert.keys().subkeys().nth(0).unwrap();
-        assert_eq!(subkey.self_signatures().len(), 1);
-        let sig = &subkey.self_signatures()[0];
+        // Verify the subkey binding without parsing into cert.
+        let primary_key =
+            if let Some(Packet::PublicKey(key)) = pp.path_ref(&[0]) {
+                key
+            } else {
+                panic!("Expected a primary key");
+            };
+        let subkey =
+            if let Some(Packet::PublicSubkey(key)) = pp.path_ref(&[3]) {
+                key
+            } else {
+                panic!("Expected a subkey");
+            };
+        let mut sig =
+            if let Some(Packet::Signature(sig)) = pp.path_ref(&[4]) {
+                sig.clone()
+            } else {
+                panic!("expected a signature");
+            };
+
 
         // The signature has only an issuer fingerprint.
         assert_eq!(sig.get_issuers().len(), 1);
         assert_eq!(sig.subpackets(SubpacketTag::Issuer).count(), 0);
         // But normalization after verification adds the missing
         // information.
+        sig.verify_subkey_binding(&primary_key, &primary_key, &subkey)?;
         let normalized_sig = sig.normalize();
         assert_eq!(normalized_sig.subpackets(SubpacketTag::Issuer).count(), 1);
         Ok(())