openpgp: Make Cert::into_packets() and to_vec() agree on sig order.

  - The signatures are ordered from authenticated and most important
    to not authenticated and most likely to be abused.  The order is:

      - Self revocations first.  They are authenticated and the most
        important information.
      - Self signatures.  They are authenticated.
      - Other signatures.  They are not authenticated at this point.
      - Other revocations.  They are not authenticated, and likely not
        well supported in other implementations, hence the least
        reliable way of revoking keys and therefore least useful and
        most likely to be abused.

4 files changed, 81 insertions, 27 deletions

diff --git a/openpgp/src/cert/components.rs b/openpgp/src/cert/components.rs
index 6351fef9..5258c493 100644
--- a/openpgp/src/cert/components.rs
+++ b/openpgp/src/cert/components.rs
@@ -306,16 +306,29 @@ impl<C> ComponentBinding<C> {
         }
     }
 
-    // Converts the component into an iterator over the contained
-    // packets.
+    /// Converts the component into an iterator over the contained
+    /// packets.
+    ///
+    /// The signatures are ordered from authenticated and most
+    /// important to not authenticated and most likely to be abused.
+    /// The order is:
+    ///
+    ///   - Self revocations first.  They are authenticated and the
+    ///     most important information.
+    ///   - Self signatures.  They are authenticated.
+    ///   - Other signatures.  They are not authenticated at this point.
+    ///   - Other revocations.  They are not authenticated, and likely
+    ///     not well supported in other implementations, hence the
+    ///     least reliable way of revoking keys and therefore least
+    ///     useful and most likely to be abused.
     pub(crate) fn into_packets<'a>(self) -> impl Iterator<Item=Packet>
         where Packet: From<C>
     {
         let p : Packet = self.component.into();
         std::iter::once(p)
+            .chain(self.self_revocations.into_iter().map(|s| s.into()))
             .chain(self.self_signatures.into_iter().map(|s| s.into()))
             .chain(self.certifications.into_iter().map(|s| s.into()))
-            .chain(self.self_revocations.into_iter().map(|s| s.into()))
             .chain(self.other_revocations.into_iter().map(|s| s.into()))
     }
 
diff --git a/openpgp/src/cert/mod.rs b/openpgp/src/cert/mod.rs
index c42ca5b3..1692d6f3 100644
--- a/openpgp/src/cert/mod.rs
+++ b/openpgp/src/cert/mod.rs
@@ -2999,6 +2999,25 @@ Pu1xwz57O4zo1VYf6TqHJzVC3OMvMUM2hhdecMUe5x6GorNaj6g=
         assert_eq!(cert.keys().unencrypted_secret().count(), 1);
     }
 
+    /// Tests that Cert::into_packets() and Cert::serialize(..) agree.
+    #[test]
+    fn test_into_packets() -> Result<()> {
+        use crate::serialize::SerializeInto;
+
+        let dkg = Cert::from_bytes(crate::tests::key("dkg.gpg"))?;
+        let mut buf = Vec::new();
+        for p in dkg.clone().into_packets() {
+            p.serialize(&mut buf)?;
+        }
+        let dkg = dkg.to_vec()?;
+        if false && buf != dkg {
+            std::fs::write("/tmp/buf", &buf)?;
+            std::fs::write("/tmp/dkg", &dkg)?;
+        }
+        assert_eq!(buf, dkg);
+        Ok(())
+    }
+
     #[test]
     fn test_canonicalization() -> Result<()> {
         use crate::types::Curve;
diff --git a/openpgp/src/serialize/cert.rs b/openpgp/src/serialize/cert.rs
index 2d3e0482..761fbb50 100644
--- a/openpgp/src/serialize/cert.rs
+++ b/openpgp/src/serialize/cert.rs
@@ -22,6 +22,19 @@ impl Cert {
     /// If `export` is true, then non-exportable signatures are not
     /// written, and components without any exportable binding
     /// signature or revocation are not exported.
+    ///
+    /// The signatures are ordered from authenticated and most
+    /// important to not authenticated and most likely to be abused.
+    /// The order is:
+    ///
+    ///   - Self revocations first.  They are authenticated and the
+    ///     most important information.
+    ///   - Self signatures.  They are authenticated.
+    ///   - Other signatures.  They are not authenticated at this point.
+    ///   - Other revocations.  They are not authenticated, and likely
+    ///     not well supported in other implementations, hence the
+    ///     least reliable way of revoking keys and therefore least
+    ///     useful and most likely to be abused.
     fn serialize_common(&self, o: &mut dyn std::io::Write, export: bool)
                         -> Result<()>
     {
@@ -43,18 +56,18 @@ impl Cert {
             Ok(())
         };
 
-        for s in primary.self_signatures() {
-            serialize_sig(o, s)?;
-        }
         for s in primary.self_revocations() {
             serialize_sig(o, s)?;
         }
-        for s in primary.other_revocations() {
+        for s in primary.self_signatures() {
             serialize_sig(o, s)?;
         }
         for s in primary.certifications() {
             serialize_sig(o, s)?;
         }
+        for s in primary.other_revocations() {
+            serialize_sig(o, s)?;
+        }
 
         for u in self.userids().bindings() {
             if export && ! u.self_signatures().iter().chain(u.self_revocations()).any(
@@ -71,10 +84,10 @@ impl Cert {
             for s in u.self_signatures() {
                 serialize_sig(o, s)?;
             }
-            for s in u.other_revocations() {
+            for s in u.certifications() {
                 serialize_sig(o, s)?;
             }
-            for s in u.certifications() {
+            for s in u.other_revocations() {
                 serialize_sig(o, s)?;
             }
         }
@@ -94,10 +107,10 @@ impl Cert {
             for s in u.self_signatures() {
                 serialize_sig(o, s)?;
             }
-            for s in u.other_revocations() {
+            for s in u.certifications() {
                 serialize_sig(o, s)?;
             }
-            for s in u.certifications() {
+            for s in u.other_revocations() {
                 serialize_sig(o, s)?;
             }
         }
@@ -117,10 +130,10 @@ impl Cert {
             for s in k.self_signatures() {
                 serialize_sig(o, s)?;
             }
-            for s in k.other_revocations() {
+            for s in k.certifications() {
                 serialize_sig(o, s)?;
             }
-            for s in k.certifications() {
+            for s in k.other_revocations() {
                 serialize_sig(o, s)?;
             }
         }
@@ -141,10 +154,10 @@ impl Cert {
             for s in u.self_signatures() {
                 serialize_sig(o, s)?;
             }
-            for s in u.other_revocations() {
+            for s in u.certifications() {
                 serialize_sig(o, s)?;
             }
-            for s in u.certifications() {
+            for s in u.other_revocations() {
                 serialize_sig(o, s)?;
             }
         }
@@ -164,18 +177,18 @@ impl SerializeInto for Cert {
         l += PacketRef::PublicKey(primary.key().mark_role_primary_ref())
             .serialized_len();
 
-        for s in primary.self_signatures() {
-            l += PacketRef::Signature(s).serialized_len();
-        }
         for s in primary.self_revocations() {
             l += PacketRef::Signature(s).serialized_len();
         }
-        for s in primary.other_revocations() {
+        for s in primary.self_signatures() {
             l += PacketRef::Signature(s).serialized_len();
         }
         for s in primary.certifications() {
             l += PacketRef::Signature(s).serialized_len();
         }
+        for s in primary.other_revocations() {
+            l += PacketRef::Signature(s).serialized_len();
+        }
 
         for u in self.userids().bindings() {
             l += PacketRef::UserID(u.userid()).serialized_len();
@@ -186,10 +199,10 @@ impl SerializeInto for Cert {
             for s in u.self_signatures() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in u.other_revocations() {
+            for s in u.certifications() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in u.certifications() {
+            for s in u.other_revocations() {
                 l += PacketRef::Signature(s).serialized_len();
             }
         }
@@ -203,10 +216,10 @@ impl SerializeInto for Cert {
             for s in u.self_signatures() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in u.other_revocations() {
+            for s in u.certifications() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in u.certifications() {
+            for s in u.other_revocations() {
                 l += PacketRef::Signature(s).serialized_len();
             }
         }
@@ -220,10 +233,10 @@ impl SerializeInto for Cert {
             for s in k.self_signatures() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in k.other_revocations() {
+            for s in k.certifications() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in k.certifications() {
+            for s in k.other_revocations() {
                 l += PacketRef::Signature(s).serialized_len();
             }
         }
@@ -237,10 +250,10 @@ impl SerializeInto for Cert {
             for s in u.self_signatures() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in u.other_revocations() {
+            for s in u.certifications() {
                 l += PacketRef::Signature(s).serialized_len();
             }
-            for s in u.certifications() {
+            for s in u.other_revocations() {
                 l += PacketRef::Signature(s).serialized_len();
             }
         }
diff --git a/openpgp/tests/for-each-artifact.rs b/openpgp/tests/for-each-artifact.rs
index 2d1b1822..5678bd79 100644
--- a/openpgp/tests/for-each-artifact.rs
+++ b/openpgp/tests/for-each-artifact.rs
@@ -69,6 +69,15 @@ mod for_each_artifact {
             let w = p.as_tsk().to_vec().unwrap();
             assert_eq!(v, w,
                        "Serialize and SerializeInto disagree on {:?}", p);
+
+            // Check that Cert::into_packets() and Cert::to_vec()
+            // agree.
+            let v = p.to_vec()?;
+            let mut buf = Vec::new();
+            for p in p.clone().into_packets() {
+                p.serialize(&mut buf)?;
+            }
+            assert_eq!(buf, v);
             Ok(())
         }).unwrap();
     }