Emit partial TPKs as revocation certificates.

- When emitting revocation certificates, emit the revocation
    signature with enough context so that it is a well-formed TPK,
    i.e. include the primary key, the component to be revoked (if
    revoking a user ID or subkey), and the revocation signature.

  - Having a partial TPK instead of a bare revocation makes handling
    it much easier, as it can be stored and transported like any
    cert.  It also gives the recipient of the certificate more
    context, and simplifies merging it into a database of certs.

  - Previously, there was a bug in sq where we would emit secret key
    material when emitting revocation certificates.  The reason for
    that was that the certificate was first converted to a packet
    stream, and then each packet serialized.  In contrast, if a
    Cert is serialized, no secrets are emitted unless the
    programmer opts in.  In a way, this is the more comprehensive fix
    for the problem, as it leverages sequoia-openpgp's mechanisms to
    protect secret key material.

  - See #160.

src/commands.rs

@@ -375,6 +375,7 @@ fn active_certification(config: &Config,
 // specified, or the specified User ID does not occur, then the
 // primary User ID is used and the specified User ID is added without
 // a binding signature.
+#[allow(dead_code)]
 pub fn cert_stub(cert: Cert,
                  policy: &dyn Policy,
                  timestamp: Option<SystemTime>,

src/commands/key/revoke.rs

@@ -17,7 +17,6 @@ use openpgp::Result;
 use crate::Config;
 use crate::cli::key::RevokeCommand;
 use crate::cli::types::FileOrStdout;
-use crate::commands::cert_stub;
 use crate::common::RevocationOutput;
 use crate::common::get_secret_signer;
 use crate::common::read_cert;
@@ -96,39 +95,17 @@ impl<'a> RevocationOutput for CertificateRevocation<'a> {
     ) -> Result<()> {
         let mut output = output.create_safe(force)?;
 
-        let (stub, packets): (Cert, Vec<Packet>) = {
+        // First, build a minimal revocation certificate containing
-            if self.first_party_issuer {
+        // the primary key, the revoked component, and the revocation
-                (self.cert.clone(), vec![self.revocation_packet.clone()])
+        // signature.
-            } else {
+        let rev_cert = Cert::from_packets(vec![
-                let cert_stub = match cert_stub(
+            self.cert.primary_key().key().clone().into(),
-                    self.cert.clone(),
+            self.revocation_packet.clone(),
-                    self.policy,
+        ].into_iter())?;
-                    self.time,
-                    None,
-                ) {
-                    Ok(stub) => stub,
-                    // We failed to create a stub.  Just use the original
-                    // certificate as is.
-                    Err(_) => self.cert.clone(),
-                };
-
-                (
-                    cert_stub.clone(),
-                    cert_stub
-                        .strip_secret_key_material()
-                        .insert_packets(self.revocation_packet.clone())?
-                        .into_packets()
-                        .collect(),
-                )
-            }
-        };
 
         if binary {
-            for packet in packets {
+            rev_cert.serialize(&mut output)
-                packet
+                .context("serializing revocation certificate")?;
-                    .serialize(&mut output)
-                    .context("serializing revocation certificate")?;
-            }
         } else {
             // Add some more helpful ASCII-armor comments.
             let mut more: Vec<String> = vec![];
@@ -140,21 +117,17 @@ impl<'a> RevocationOutput for CertificateRevocation<'a> {
                 // Then if it was issued by a third-party.
                 more.push("issued by".to_string());
                 more.push(self.secret.fingerprint().to_spaced_hex());
-                if let Ok(valid_cert) =
+                let issuer_uid = crate::best_effort_primary_uid(
-                    &stub.with_policy(self.policy, self.time)
+                    &self.secret, self.policy, self.time);
-                {
+                let issuer_uid = String::from_utf8_lossy(issuer_uid.value());
-                    if let Ok(uid) = valid_cert.primary_userid() {
+                // Truncate it, if it is too long.
-                        let uid = String::from_utf8_lossy(uid.value());
+                more.push(format!(
-                        // Truncate it, if it is too long.
+                    "{:?}",
-                        more.push(format!(
+                    issuer_uid.chars().take(70).collect::<String>()
-                            "{:?}",
+                ));
-                            uid.chars().take(70).collect::<String>()
-                        ));
-                    }
-                }
             }
 
-            let headers = &stub.armor_headers();
+            let headers = &self.cert.armor_headers();
             let headers: Vec<(&str, &str)> = headers
                 .iter()
                 .map(|s| ("Comment", s.as_str()))
@@ -163,11 +136,8 @@ impl<'a> RevocationOutput for CertificateRevocation<'a> {
 
             let mut writer =
                 Writer::with_headers(&mut output, Kind::PublicKey, headers)?;
-            for packet in packets {
+            rev_cert.serialize(&mut writer)
-                packet
+                .context("serializing revocation certificate")?;
-                    .serialize(&mut writer)
-                    .context("serializing revocation certificate")?;
-            }
             writer.finalize()?;
         }
 

src/commands/key/subkey.rs

@@ -9,9 +9,9 @@ use chrono::Utc;
 use sequoia_openpgp as openpgp;
 use openpgp::armor::Kind;
 use openpgp::armor::Writer;
-use openpgp::cert::amalgamation::ValidAmalgamation;
 use openpgp::cert::KeyBuilder;
 use openpgp::cert::SubkeyRevocationBuilder;
+use openpgp::packet::{Key, key};
 use openpgp::packet::signature::subpacket::NotationData;
 use openpgp::parse::Parse;
 use openpgp::policy::Policy;
@@ -29,7 +29,6 @@ use crate::cli::key::SubkeyAddCommand;
 use crate::cli::key::SubkeyCommand;
 use crate::cli::key::SubkeyRevokeCommand;
 use crate::cli::types::FileOrStdout;
-use crate::commands::cert_stub;
 use crate::commands::get_primary_keys;
 use crate::common::NULL_POLICY;
 use crate::common::RevocationOutput;
@@ -47,8 +46,7 @@ struct SubkeyRevocation<'a> {
     time: Option<SystemTime>,
     revocation_packet: Packet,
     first_party_issuer: bool,
-    subkey_packets: Vec<Packet>,
+    subkey: Key<key::PublicParts, key::SubordinateRole>,
-    subkey_as_hex: String,
 }
 
 impl<'a> SubkeyRevocation<'a> {
@@ -74,25 +72,16 @@ impl<'a> SubkeyRevocation<'a> {
 
         let first_party_issuer = secret.fingerprint() == cert.fingerprint();
 
-        let mut subkey_packets = vec![];
+        let (subkey, revocation_packet) = {
-        let mut subkey_as_hex = String::new();
-        let mut subkey = None;
-
-        let revocation_packet = {
             let valid_cert = cert.with_policy(NULL_POLICY, None)?;
 
-            for key in valid_cert.keys().subkeys() {
+            let keys = valid_cert.keys().subkeys()
-                if keyhandle.aliases(KeyHandle::from(key.fingerprint())) {
+                .key_handle(keyhandle.clone())
-                    subkey_packets.push(Packet::from(key.key().clone()));
+                .map(|skb| skb.key().clone())
-                    subkey_packets
+                .collect::<Vec<_>>();
-                        .push(Packet::from(key.binding_signature().clone()));
-                    subkey_as_hex.push_str(&key.fingerprint().to_spaced_hex());
-                    subkey = Some(key);
-                    break;
-                }
-            }
 
-            if let Some(ref subkey) = subkey {
+            if keys.len() == 1 {
+                let subkey = keys[0].clone();
                 let mut rev = SubkeyRevocationBuilder::new()
                     .set_reason_for_revocation(reason, message.as_bytes())?;
                 if let Some(time) = time {
@@ -106,12 +95,26 @@ impl<'a> SubkeyRevocation<'a> {
                         *critical,
                     )?;
                 }
-                let rev = rev.build(&mut signer, &cert, subkey.key(), None)?;
+                let rev = rev.build(&mut signer, &cert, &subkey, None)?;
-                Packet::Signature(rev)
+                (subkey.into(), Packet::Signature(rev))
+            } else if keys.len() > 1 {
+                wprintln!("Key ID {} does not uniquely identify a subkey, \
+                           please use a fingerprint instead.\nValid subkeys:",
+                          keyhandle);
+                for k in keys {
+                    wprintln!(
+                        "  - {} {}",
+                        k.fingerprint(),
+                        DateTime::<Utc>::from(k.creation_time()).date_naive()
+                    );
+                }
+                return Err(anyhow!(
+                    "Subkey is ambiguous."
+                ));
             } else {
                 wprintln!(
                     "Subkey {} not found.\nValid subkeys:",
-                    keyhandle.to_spaced_hex()
+                    keyhandle
                 );
                 let mut have_valid = false;
                 for k in valid_cert.keys().subkeys() {
@@ -139,8 +142,7 @@ impl<'a> SubkeyRevocation<'a> {
             time,
             revocation_packet,
             first_party_issuer,
-            subkey_packets,
+            subkey,
-            subkey_as_hex,
         })
     }
 }
@@ -155,40 +157,18 @@ impl<'a> RevocationOutput for SubkeyRevocation<'a> {
     ) -> Result<()> {
         let mut output = output.create_safe(force)?;
 
-        let (stub, packets): (Cert, Vec<Packet>) = {
+        // First, build a minimal revocation certificate containing
-            let mut cert_stub = match cert_stub(
+        // the primary key, the revoked component, and the revocation
-                self.cert.clone(),
+        // signature.
-                self.policy,
+        let rev_cert = Cert::from_packets(vec![
-                self.time,
+            self.cert.primary_key().key().clone().into(),
-                None,
+            self.subkey.clone().into(),
-            ) {
+            self.revocation_packet.clone(),
-                Ok(stub) => stub,
+        ].into_iter())?;
-                // We failed to create a stub.  Just use the original
-                // certificate as is.
-                Err(_) => self.cert.clone(),
-            };
-
-            if !self.subkey_packets.is_empty() {
-                cert_stub =
-                    cert_stub.insert_packets(self.subkey_packets.clone())?;
-            }
-
-            (
-                cert_stub.clone(),
-                cert_stub
-                    .strip_secret_key_material()
-                    .insert_packets(self.revocation_packet.clone())?
-                    .into_packets()
-                    .collect(),
-            )
-        };
 
         if binary {
-            for packet in packets {
+            rev_cert.serialize(&mut output)
-                packet
+                .context("serializing revocation certificate")?;
-                    .serialize(&mut output)
-                    .context("serializing revocation certificate")?;
-            }
         } else {
             // Add some more helpful ASCII-armor comments.
             let mut more: Vec<String> = vec![];
@@ -197,27 +177,23 @@ impl<'a> RevocationOutput for SubkeyRevocation<'a> {
             more.push(
                 "including a revocation to revoke the subkey".to_string(),
             );
-            more.push(self.subkey_as_hex.clone());
+            more.push(self.subkey.fingerprint().to_spaced_hex());
 
             if !self.first_party_issuer {
                 // Then if it was issued by a third-party.
                 more.push("issued by".to_string());
                 more.push(self.secret.fingerprint().to_spaced_hex());
-                if let Ok(valid_cert) =
+                let issuer_uid = crate::best_effort_primary_uid(
-                    &stub.with_policy(self.policy, self.time)
+                    &self.secret, self.policy, self.time);
-                {
+                let issuer_uid = String::from_utf8_lossy(issuer_uid.value());
-                    if let Ok(uid) = valid_cert.primary_userid() {
+                // Truncate it, if it is too long.
-                        let uid = String::from_utf8_lossy(uid.value());
+                more.push(format!(
-                        // Truncate it, if it is too long.
+                    "{:?}",
-                        more.push(format!(
+                    issuer_uid.chars().take(70).collect::<String>()
-                            "{:?}",
+                ));
-                            uid.chars().take(70).collect::<String>()
-                        ));
-                    }
-                }
             }
 
-            let headers = &stub.armor_headers();
+            let headers = &self.cert.armor_headers();
             let headers: Vec<(&str, &str)> = headers
                 .iter()
                 .map(|s| ("Comment", s.as_str()))
@@ -226,11 +202,8 @@ impl<'a> RevocationOutput for SubkeyRevocation<'a> {
 
             let mut writer =
                 Writer::with_headers(&mut output, Kind::PublicKey, headers)?;
-            for packet in packets {
+            rev_cert.serialize(&mut writer)
-                packet
+                .context("serializing revocation certificate")?;
-                    .serialize(&mut writer)
-                    .context("serializing revocation certificate")?;
-            }
             writer.finalize()?;
         }
         Ok(())

src/commands/key/userid.rs

@@ -29,7 +29,6 @@ use crate::Config;
 use crate::cli::key::UseridRevokeCommand;
 use crate::cli::types::FileOrStdout;
 use crate::cli;
-use crate::commands::cert_stub;
 use crate::commands::get_primary_keys;
 use crate::common::NULL_POLICY;
 use crate::common::RevocationOutput;
@@ -47,6 +46,7 @@ struct UserIDRevocation<'a> {
     revocation_packet: Packet,
     first_party_issuer: bool,
     userid: String,
+    uid: UserID,
 }
 
 impl<'a> UserIDRevocation<'a> {
@@ -72,6 +72,7 @@ impl<'a> UserIDRevocation<'a> {
         )?;
 
         let first_party_issuer = secret.fingerprint() == cert.fingerprint();
+        let uid = UserID::from(userid.as_str());
 
         let revocation_packet = {
             // Create a revocation for a User ID.
@@ -86,7 +87,7 @@ impl<'a> UserIDRevocation<'a> {
                 let valid_cert = cert.with_policy(NULL_POLICY, None)?;
                 let present = valid_cert
                     .userids()
-                    .any(|u| u.value() == userid.as_bytes());
+                    .any(|u| u.userid() == &uid);
 
                 if !present {
                     wprintln!(
@@ -127,7 +128,7 @@ impl<'a> UserIDRevocation<'a> {
             let rev = rev.build(
                 &mut signer,
                 &cert,
-                &UserID::from(userid.as_str()),
+                &uid,
                 None,
             )?;
             Packet::Signature(rev)
@@ -141,6 +142,7 @@ impl<'a> UserIDRevocation<'a> {
             revocation_packet,
             first_party_issuer,
             userid,
+            uid,
         })
     }
 }
@@ -155,35 +157,18 @@ impl<'a> RevocationOutput for UserIDRevocation<'a> {
     ) -> Result<()> {
         let mut output = output.create_safe(force)?;
 
-        let (stub, packets): (Cert, Vec<Packet>) = {
+        // First, build a minimal revocation certificate containing
-            let cert_stub = match cert_stub(
+        // the primary key, the revoked component, and the revocation
-                self.cert.clone(),
+        // signature.
-                self.policy,
+        let rev_cert = Cert::from_packets(vec![
-                self.time,
+            self.cert.primary_key().key().clone().into(),
-                Some(&UserID::from(self.userid.clone())),
+            self.uid.clone().into(),
-            ) {
+            self.revocation_packet.clone(),
-                Ok(stub) => stub,
+        ].into_iter())?;
-                // We failed to create a stub.  Just use the original
-                // certificate as is.
-                Err(_) => self.cert.clone(),
-            };
-
-            (
-                cert_stub.clone(),
-                cert_stub
-                    .strip_secret_key_material()
-                    .insert_packets(self.revocation_packet.clone())?
-                    .into_packets()
-                    .collect(),
-            )
-        };
 
         if binary {
-            for packet in packets {
+            rev_cert.serialize(&mut output)
-                packet
+                .context("serializing revocation certificate")?;
-                    .serialize(&mut output)
-                    .context("serializing revocation certificate")?;
-            }
         } else {
             // Add some more helpful ASCII-armor comments.
             let mut more: Vec<String> = vec![];
@@ -192,27 +177,23 @@ impl<'a> RevocationOutput for UserIDRevocation<'a> {
             more.push(
                 "including a revocation to revoke the User ID".to_string(),
             );
-            more.push(format!("{:?}", self.userid));
+            more.push(format!("{}", self.userid));
 
             if !self.first_party_issuer {
                 // Then if it was issued by a third-party.
                 more.push("issued by".to_string());
                 more.push(self.secret.fingerprint().to_spaced_hex());
-                if let Ok(valid_cert) =
+                let issuer_uid = crate::best_effort_primary_uid(
-                    &stub.with_policy(self.policy, self.time)
+                    &self.secret, self.policy, self.time);
-                {
+                let issuer_uid = String::from_utf8_lossy(issuer_uid.value());
-                    if let Ok(uid) = valid_cert.primary_userid() {
+                // Truncate it, if it is too long.
-                        let uid = String::from_utf8_lossy(uid.value());
+                more.push(format!(
-                        // Truncate it, if it is too long.
+                    "{:?}",
-                        more.push(format!(
+                    issuer_uid.chars().take(70).collect::<String>()
-                            "{:?}",
+                ));
-                            uid.chars().take(70).collect::<String>()
-                        ));
-                    }
-                }
             }
 
-            let headers = &stub.armor_headers();
+            let headers = &self.cert.armor_headers();
             let headers: Vec<(&str, &str)> = headers
                 .iter()
                 .map(|s| ("Comment", s.as_str()))
@@ -221,11 +202,8 @@ impl<'a> RevocationOutput for UserIDRevocation<'a> {
 
             let mut writer =
                 Writer::with_headers(&mut output, Kind::PublicKey, headers)?;
-            for packet in packets {
+            rev_cert.serialize(&mut writer)
-                packet
+                .context("serializing revocation certificate")?;
-                    .serialize(&mut writer)
-                    .context("serializing revocation certificate")?;
-            }
             writer.finalize()?;
         }
         Ok(())

tests/sq-key-revoke.rs

@@ -134,16 +134,16 @@ fn sq_key_revoke() -> Result<()> {
             );
         }
 
-        // We should get just a single signature packet.
+        // We should get the primary key and the revocation signature.
         let packet_pile = PacketPile::from_file(&revocation)?;
 
         assert_eq!(
             packet_pile.children().count(),
-            1,
+            2,
-            "expected a single packet"
+            "expected the primary key and the revocation signature"
         );
 
-        if let Some(Packet::Signature(sig)) = packet_pile.path_ref(&[0]) {
+        if let Some(Packet::Signature(sig)) = packet_pile.path_ref(&[1]) {
             // the issuer is the certificate owner
             assert_eq!(
                 sig.get_issuers().into_iter().next(),
@@ -311,11 +311,10 @@ fn sq_key_revoke_thirdparty() -> Result<()> {
         // read revocation cert
         let revocation_cert = Cert::from_file(&revocation)?;
         assert!(! revocation_cert.is_tsk());
-        let revocation_valid_cert = revocation_cert
-            .with_policy(STANDARD_POLICY, revocation_time.map(Into::into))?;
 
         // evaluate revocation status
-        let status = revocation_valid_cert.revocation_status();
+        let status = revocation_cert.revocation_status(
+            STANDARD_POLICY, revocation_time.map(Into::into));
         if let RevocationStatus::CouldBe(sigs) = status {
             // there is only one signature packet
             assert_eq!(sigs.len(), 1);

tests/sq-key-subkey.rs

@@ -239,8 +239,11 @@ fn sq_key_subkey_revoke() -> Result<()> {
         let mut found_revoked = false;
 
         // read revocation cert
-        let cert = Cert::from_file(&revocation)?;
+        let rev = Cert::from_file(&revocation)?;
-        assert!(! cert.is_tsk());
+        assert!(! rev.is_tsk());
+
+        // and merge it into the certificate.
+        let cert = cert.clone().merge_public(rev)?;
         let valid_cert =
             cert.with_policy(STANDARD_POLICY, revocation_time.map(Into::into))?;
         valid_cert
@@ -436,8 +439,11 @@ fn sq_key_subkey_revoke_thirdparty() -> Result<()> {
         let mut found_revoked = false;
 
         // read revocation cert
-        let cert = Cert::from_file(&revocation)?;
+        let rev = Cert::from_file(&revocation)?;
-        assert!(! cert.is_tsk());
+        assert!(! rev.is_tsk());
+
+        // and merge it into the certificate.
+        let cert = cert.clone().merge_public(rev)?;
         let valid_cert =
             cert.with_policy(STANDARD_POLICY, revocation_time.map(Into::into))?;
 

tests/sq-key-userid.rs

@@ -112,15 +112,12 @@ fn sq_key_userid_revoke() -> Result<()> {
         let mut found_revoked = false;
 
         // read revocation cert
-        let cert = Cert::from_file(&revocation)?;
+        let rev = Cert::from_file(&revocation)?;
-        assert!(! cert.is_tsk());
+        assert!(! rev.is_tsk());
+        let cert = cert.clone().merge_public(rev)?;
         let valid_cert =
             cert.with_policy(STANDARD_POLICY, revocation_time.map(Into::into))?;
 
-        // Make sure the certificate stub only contains the
-        // revoked User ID (the rest should be striped).
-        assert_eq!(valid_cert.userids().count(), 1);
-
         valid_cert.userids().for_each(|x| {
             if x.value() == userid_revoke.as_bytes() {
                 if let RevocationStatus::Revoked(sigs) = x.revocation_status(
@@ -174,6 +171,7 @@ fn sq_key_userid_revoke_thirdparty() -> Result<()> {
     // revoke the last userid
     let userid_revoke = userids.last().unwrap();
     let (tmpdir, path, _) = sq_key_generate(Some(userids))?;
+    let cert = Cert::from_file(&path)?;
 
     let (thirdparty_tmpdir, thirdparty_path, thirdparty_time) =
         sq_key_generate(Some(&["bob <bob@example.org"]))?;
@@ -269,15 +267,12 @@ fn sq_key_userid_revoke_thirdparty() -> Result<()> {
         let mut found_revoked = false;
 
         // read revocation cert
-        let revocation_cert = Cert::from_file(&revocation)?;
+        let rev = Cert::from_file(&revocation)?;
-        assert!(! revocation_cert.is_tsk());
+        assert!(! rev.is_tsk());
+        let revocation_cert = cert.clone().merge_public(rev)?;
         let revocation_valid_cert = revocation_cert
             .with_policy(STANDARD_POLICY, revocation_time.map(Into::into))?;
 
-        // Make sure the certificate stub only contains the
-        // revoked User ID (the rest should be stripped).
-        assert_eq!(revocation_valid_cert.userids().count(), 1);
-
         revocation_valid_cert.userids().for_each(|x| {
             if x.value() == userid_revoke.as_bytes() {
                 if let RevocationStatus::CouldBe(sigs) = x.revocation_status(