From aefc8b19e401ed6a94a582dc91dd83ff1a94143a Mon Sep 17 00:00:00 2001 From: Justus Winter Date: Tue, 12 Dec 2023 15:07:28 +0100 Subject: openpgp: Add Cert::into_packets2, TSK::into_packets. - Cert::into_packet is problematic because it does not protect from accidentally leaking secret key material. The documentation even warns about that, but it still happened. Hence, this is a violation of our safe-by-default principle guiding the API, and we should fix it. - The replacement, Cert::into_packets2, strips secret key material just as serializing a cert does. To convert to a sequence of packets while keeping the secret key material, a new function is added: TSK::into_packets, analogous to how TSK serializes secret key material. --- openpgp/NEWS | 2 + openpgp/src/cert.rs | 148 +++++++++++++++++++++++++++++++++++++ openpgp/src/serialize/cert.rs | 42 +++++++++++ openpgp/tests/for-each-artifact.rs | 31 +++++++- 4 files changed, 221 insertions(+), 2 deletions(-) diff --git a/openpgp/NEWS b/openpgp/NEWS index c68fd0d8..76123c2d 100644 --- a/openpgp/NEWS +++ b/openpgp/NEWS @@ -18,6 +18,8 @@ - CertBuilder::set_exportable - UserID::from_static_bytes - Error::ShortKeyID + - Cert::into_packets2 + - TSK::into_packets * Changes in 1.17.0 ** Notable fixes - Sequoia now ignores some formatting errors when reading secret diff --git a/openpgp/src/cert.rs b/openpgp/src/cert.rs index f5579605..b3dfa833 100644 --- a/openpgp/src/cert.rs +++ b/openpgp/src/cert.rs @@ -1425,6 +1425,58 @@ impl Cert { .chain(self.bad.into_iter().map(|s| s.into())) } + /// Converts the certificate into an iterator over a sequence of + /// packets. + /// + /// This function strips secrets from the keys, similar to how + /// serializing a [`Cert`] would not serialize secret keys. This + /// behavior makes it harder to accidentally leak secret key + /// material. + /// + /// If you do want to preserve secret key material, use + /// [`Cert::into_tsk`] to opt-in to getting the secret key + /// material, then use [`TSK::into_packets`] to convert to a + /// packet stream. + /// + /// # Examples + /// + /// ``` + /// # use sequoia_openpgp as openpgp; + /// # use openpgp::cert::prelude::*; + /// # + /// # fn main() -> openpgp::Result<()> { + /// # let (cert, _) = + /// # CertBuilder::general_purpose(None, Some("alice@example.org")) + /// # .generate()?; + /// assert!(cert.is_tsk()); + /// // But: + /// assert!(! Cert::from_packets(cert.into_packets2())?.is_tsk()); + /// # Ok(()) } + /// ``` + pub fn into_packets2(self) -> impl Iterator + Send + Sync { + /// Strips the secret key material. + fn rewrite(mut p: impl Iterator + Send + Sync) + -> impl Iterator + Send + Sync + { + let k: Packet = match p.next().unwrap() { + Packet::PublicKey(k) => + Packet::PublicKey(k.take_secret().0), + Packet::PublicSubkey(k) => + Packet::PublicSubkey(k.take_secret().0), + _ => unreachable!(), + }; + + std::iter::once(k).chain(p) + } + + rewrite(self.primary.into_packets()) + .chain(self.userids.into_iter().flat_map(|b| b.into_packets())) + .chain(self.user_attributes.into_iter().flat_map(|b| b.into_packets())) + .chain(self.subkeys.into_iter().flat_map(|b| rewrite(b.into_packets()))) + .chain(self.unknowns.into_iter().flat_map(|b| b.into_packets())) + .chain(self.bad.into_iter().map(|s| s.into())) + } + /// Returns the first certificate found in the sequence of packets. /// /// If the sequence of packets does not start with a certificate @@ -3456,6 +3508,102 @@ impl Cert { } } +use crate::serialize::TSK; +impl<'a> TSK<'a> { + /// Converts the certificate into an iterator over a sequence of + /// packets. + /// + /// This function emits secret key packets, modulo the keys that + /// are filtered (see [`TSK::set_filter`]). If requested, missing + /// secret key material is replaced by stubs (see + /// [`TSK::emit_secret_key_stubs`]. + /// + /// # Examples + /// + /// ``` + /// # use sequoia_openpgp as openpgp; + /// # use openpgp::cert::prelude::*; + /// # use openpgp::serialize::{Serialize, SerializeInto}; + /// # + /// # fn main() -> openpgp::Result<()> { + /// # let (cert, _) = + /// # CertBuilder::general_purpose(None, Some("alice@example.org")) + /// # .generate()?; + /// assert!(cert.is_tsk()); + /// let a = cert.as_tsk().to_vec()?; + /// let mut b = Vec::new(); + /// cert.into_tsk().into_packets() + /// .for_each(|p| p.serialize(&mut b).unwrap()); + /// assert_eq!(a, b); + /// # Ok(()) } + /// ``` + pub fn into_packets(self) -> impl Iterator + 'a { + /// Strips the secret key material if the filter rejects it, + /// and optionally inserts secret key stubs. + use std::sync::Arc; + fn rewrite<'a>( + filter: Arc bool + 'a>>, + emit_secret_key_stubs: bool, + mut p: impl Iterator + Send + Sync) + -> impl Iterator + Send + Sync + { + let k: Packet = match p.next().unwrap() { + Packet::PublicKey(mut k) => { + if ! k.role_as_unspecified().parts_as_secret() + .map(|k| (filter)(k)) + .unwrap_or(false) + { + k = k.take_secret().0; + } + + if ! k.has_secret() && emit_secret_key_stubs { + k = TSK::add_stub(k).into(); + } + + if k.has_secret() { + Packet::SecretKey(k.parts_into_secret().unwrap()) + } else { + Packet::PublicKey(k) + } + } + Packet::PublicSubkey(mut k) => { + if ! k.role_as_unspecified().parts_as_secret() + .map(|k| (filter)(k)) + .unwrap_or(false) + { + k = k.take_secret().0; + } + + if ! k.has_secret() && emit_secret_key_stubs { + k = TSK::add_stub(k).into(); + } + + if k.has_secret() { + Packet::SecretSubkey(k.parts_into_secret().unwrap()) + } else { + Packet::PublicSubkey(k) + } + } + _ => unreachable!(), + }; + + std::iter::once(k).chain(p) + } + + let (cert, filter, emit_secret_key_stubs) = self.decompose(); + let filter = Arc::new(filter); + let cert = cert.into_owned(); + + rewrite(filter.clone(), emit_secret_key_stubs, cert.primary.into_packets()) + .chain(cert.userids.into_iter().flat_map(|b| b.into_packets())) + .chain(cert.user_attributes.into_iter().flat_map(|b| b.into_packets())) + .chain(cert.subkeys.into_iter().flat_map( + move |b| rewrite(filter.clone(), emit_secret_key_stubs, b.into_packets()))) + .chain(cert.unknowns.into_iter().flat_map(|b| b.into_packets())) + .chain(cert.bad.into_iter().map(|s| s.into())) + } +} + impl TryFrom> for Cert { type Error = anyhow::Error; diff --git a/openpgp/src/serialize/cert.rs b/openpgp/src/serialize/cert.rs index 0b8c05c6..2006b122 100644 --- a/openpgp/src/serialize/cert.rs +++ b/openpgp/src/serialize/cert.rs @@ -372,6 +372,15 @@ impl<'a> TSK<'a> { } } + /// Decomposes the TSK. + pub(crate) fn decompose(self) + -> (Cow<'a, Cert>, + Box bool>, + bool) + { + (self.cert, self.filter, self.emit_stubs) + } + /// Returns whether we're emitting secret (sub)key packets when /// serializing. /// @@ -1124,6 +1133,39 @@ mod test { cert.as_tsk().armored().to_vec()?); assert_eq!(cert.armored().to_vec()?, tsk.as_tsk().set_filter(no_secrets).armored().to_vec()?); + + Ok(()) + } + + #[test] + fn into_packets() -> Result<()> { + let cert = Cert::from_bytes(crate::tests::key("testy-private.pgp"))?; + + fn t(tsk: TSK) { + let a = tsk.to_vec().unwrap(); + let b = { + let mut b = Vec::new(); + tsk.into_packets().for_each(|p| p.serialize(&mut b).unwrap()); + b + }; + assert_eq!(a, b); + } + + let tsk = cert.clone().into_tsk(); + t(tsk); + + let tsk = cert.clone().into_tsk().set_filter(|_| false); + t(tsk); + + let tsk = cert.clone().into_tsk() + .set_filter(|k| k.fingerprint() == cert.fingerprint()); + t(tsk); + + let tsk = cert.clone().into_tsk() + .set_filter(|k| k.fingerprint() == cert.fingerprint()) + .emit_secret_key_stubs(true); + t(tsk); + Ok(()) } } diff --git a/openpgp/tests/for-each-artifact.rs b/openpgp/tests/for-each-artifact.rs index 4d22840d..cb729dc5 100644 --- a/openpgp/tests/for-each-artifact.rs +++ b/openpgp/tests/for-each-artifact.rs @@ -118,8 +118,8 @@ mod for_each_artifact { if p != q { eprintln!("roundtripping {:?} failed", src); - let p_: Vec<_> = p.clone().into_packets().collect(); - let q_: Vec<_> = q.clone().into_packets().collect(); + let p_: Vec<_> = p.clone().as_tsk().into_packets().collect(); + let q_: Vec<_> = q.clone().as_tsk().into_packets().collect(); eprintln!("original: {} packets; roundtripped: {} packets", p_.len(), q_.len()); @@ -141,6 +141,33 @@ mod for_each_artifact { assert_eq!(v, w, "Serialize and SerializeInto disagree on {:?}", p); + // Check that Cert::into_packets2() and Cert::to_vec() + // agree. (Cert::into_packets2() returns no secret keys if + // secret key material is present; Cert::to_vec only ever + // returns public keys.) + let v = p.to_vec()?; + let mut buf = Vec::new(); + for p in p.clone().into_packets2() { + p.serialize(&mut buf)?; + } + if let Err(_err) = diff_serialized(&buf, &v) { + panic!("Checking that \ + Cert::into_packets2() \ + and Cert::to_vec() agree."); + } + + // Check that Cert::as_tsk().into_packets() and + // Cert::as_tsk().to_vec() agree. + let v = p.as_tsk().to_vec()?; + let mut buf = Vec::new(); + for p in p.as_tsk().into_packets() { + p.serialize(&mut buf)?; + } + if let Err(_err) = diff_serialized(&buf, &v) { + panic!("Checking that Cert::as_tsk().into_packets() \ + and Cert::as_tsk().to_vec() agree."); + } + // Check that // Cert::strip_secret_key_material().into_packets() and // Cert::to_vec() agree. (Cert::into_packets() returns -- cgit v1.2.3