openpgp: Fix Key conversions.

  - The public ABI has a bug.

  - The documentation for `Key` says that a `Key<SecretParts, R>`
    must have secret key material:

    > If P is key::SecretParts, then the key definitely contains
    > secret key material (although it is not guaranteed that the secret
    > key material is valid), and methods that require secret key
    > material are available.

    https://gitlab.com/sequoia-pgp/sequoia/-/blob/a9982139/openpgp/src/packet/mod.rs#L1249-1256

  - Unfortunately, we accidentally provided infallible conversions from
    `Key<PublicParts, R1>` to `Key<SecretsParts, R2>` and
    `Key<UnspecifiedParts, R1>` to `Key<SecretsParts, R2>` where
    `R1 != R2`.

  - These conversions are wrong, and useless: attempting to use a
    method on a `Key<SecretsParts, R2>` that expects the `Key` to have
    secret key material, but doesn't results in a panic.

  - Removing these methods converts run-time errors into compile-time
    errors.  As such, we do not consider this change to result in a
    change to the semantic version.

  - Make the conversions to `Key<key::SecretsParts, R>` fallible.

  - Fixes #740.

2 files changed, 75 insertions, 22 deletions

diff --git a/openpgp/src/packet/key/conversions.rs b/openpgp/src/packet/key/conversions.rs
index 7c8b8c8c..babdc192 100644
--- a/openpgp/src/packet/key/conversions.rs
+++ b/openpgp/src/packet/key/conversions.rs
@@ -337,6 +337,43 @@ macro_rules! create_conversions {
             }
         }
 
+        macro_rules! f_try {
+            ( <$from_parts:ty, $from_role:ty> -> <SecretParts, $to_role:ty> ) => {
+                impl<$($l ),*> TryFrom<$Key<$($l, )* $from_parts, $from_role>> for $Key<$($l, )* SecretParts, $to_role>
+                {
+                    type Error = anyhow::Error;
+                    fn try_from(p: $Key<$($l, )* $from_parts, $from_role>) -> Result<Self> {
+                        // First, just change the role.
+                        let k: $Key<$($l, )* $from_parts, $to_role> = p.into();
+                        // Now change the parts.
+                        k.try_into()
+                    }
+                }
+
+                impl<$($l ),*> TryFrom<&$($l)* $Key<$($l, )* $from_parts, $from_role>> for &$($l)* $Key<$($l, )* SecretParts, $to_role>
+                {
+                    type Error = anyhow::Error;
+                    fn try_from(p: &$($l)* $Key<$($l, )* $from_parts, $from_role>) -> Result<Self> {
+                        // First, just change the role.
+                        let k: &$($l)* $Key<$($l, )* $from_parts, $to_role> = p.into();
+                        // Now change the parts.
+                        k.try_into()
+                    }
+                }
+
+                impl<$($l ),*> TryFrom<&$($l)* mut $Key<$($l, )* $from_parts, $from_role>> for &$($l)* mut $Key<$($l, )* SecretParts, $to_role>
+                {
+                    type Error = anyhow::Error;
+                    fn try_from(p: &$($l)* mut $Key<$($l, )* $from_parts, $from_role>) -> Result<Self> {
+                        // First, just change the role.
+                        let k: &$($l)* mut $Key<$($l, )* $from_parts, $to_role> = p.into();
+                        // Now change the parts.
+                        k.try_into()
+                    }
+                }
+            }
+        }
+
         // The calls that are comment out are the calls for the
         // combinations where either the KeyParts or the KeyRole does not
         // change.
@@ -345,8 +382,8 @@ macro_rules! create_conversions {
         //f!(<PublicParts, PrimaryRole> -> <PublicParts, SubordinateRole>);
         //f!(<PublicParts, PrimaryRole> -> <PublicParts, UnspecifiedRole>);
         //f!(<PublicParts, PrimaryRole> -> <SecretParts, PrimaryRole>);
-        f!(<PublicParts, PrimaryRole> -> <SecretParts, SubordinateRole>);
-        f!(<PublicParts, PrimaryRole> -> <SecretParts, UnspecifiedRole>);
+        f_try!(<PublicParts, PrimaryRole> -> <SecretParts, SubordinateRole>);
+        f_try!(<PublicParts, PrimaryRole> -> <SecretParts, UnspecifiedRole>);
         //f!(<PublicParts, PrimaryRole> -> <UnspecifiedParts, PrimaryRole>);
         f!(<PublicParts, PrimaryRole> -> <UnspecifiedParts, SubordinateRole>);
         f!(<PublicParts, PrimaryRole> -> <UnspecifiedParts, UnspecifiedRole>);
@@ -354,9 +391,9 @@ macro_rules! create_conversions {
         //f!(<PublicParts, SubordinateRole> -> <PublicParts, PrimaryRole>);
         //f!(<PublicParts, SubordinateRole> -> <PublicParts, SubordinateRole>);
         //f!(<PublicParts, SubordinateRole> -> <PublicParts, UnspecifiedRole>);
-        f!(<PublicParts, SubordinateRole> -> <SecretParts, PrimaryRole>);
+        f_try!(<PublicParts, SubordinateRole> -> <SecretParts, PrimaryRole>);
         //f!(<PublicParts, SubordinateRole> -> <SecretParts, SubordinateRole>);
-        f!(<PublicParts, SubordinateRole> -> <SecretParts, UnspecifiedRole>);
+        f_try!(<PublicParts, SubordinateRole> -> <SecretParts, UnspecifiedRole>);
         f!(<PublicParts, SubordinateRole> -> <UnspecifiedParts, PrimaryRole>);
         //f!(<PublicParts, SubordinateRole> -> <UnspecifiedParts, SubordinateRole>);
         f!(<PublicParts, SubordinateRole> -> <UnspecifiedParts, UnspecifiedRole>);
@@ -364,8 +401,8 @@ macro_rules! create_conversions {
         //f!(<PublicParts, UnspecifiedRole> -> <PublicParts, PrimaryRole>);
         //f!(<PublicParts, UnspecifiedRole> -> <PublicParts, SubordinateRole>);
         //f!(<PublicParts, UnspecifiedRole> -> <PublicParts, UnspecifiedRole>);
-        f!(<PublicParts, UnspecifiedRole> -> <SecretParts, PrimaryRole>);
-        f!(<PublicParts, UnspecifiedRole> -> <SecretParts, SubordinateRole>);
+        f_try!(<PublicParts, UnspecifiedRole> -> <SecretParts, PrimaryRole>);
+        f_try!(<PublicParts, UnspecifiedRole> -> <SecretParts, SubordinateRole>);
         //f!(<PublicParts, UnspecifiedRole> -> <SecretParts, UnspecifiedRole>);
         f!(<PublicParts, UnspecifiedRole> -> <UnspecifiedParts, PrimaryRole>);
         f!(<PublicParts, UnspecifiedRole> -> <UnspecifiedParts, SubordinateRole>);
@@ -405,8 +442,8 @@ macro_rules! create_conversions {
         f!(<UnspecifiedParts, PrimaryRole> -> <PublicParts, SubordinateRole>);
         f!(<UnspecifiedParts, PrimaryRole> -> <PublicParts, UnspecifiedRole>);
         //f!(<UnspecifiedParts, PrimaryRole> -> <SecretParts, PrimaryRole>);
-        f!(<UnspecifiedParts, PrimaryRole> -> <SecretParts, SubordinateRole>);
-        f!(<UnspecifiedParts, PrimaryRole> -> <SecretParts, UnspecifiedRole>);
+        f_try!(<UnspecifiedParts, PrimaryRole> -> <SecretParts, SubordinateRole>);
+        f_try!(<UnspecifiedParts, PrimaryRole> -> <SecretParts, UnspecifiedRole>);
         //f!(<UnspecifiedParts, PrimaryRole> -> <UnspecifiedParts, PrimaryRole>);
         //f!(<UnspecifiedParts, PrimaryRole> -> <UnspecifiedParts, SubordinateRole>);
         //f!(<UnspecifiedParts, PrimaryRole> -> <UnspecifiedParts, UnspecifiedRole>);
@@ -414,9 +451,9 @@ macro_rules! create_conversions {
         f!(<UnspecifiedParts, SubordinateRole> -> <PublicParts, PrimaryRole>);
         //f!(<UnspecifiedParts, SubordinateRole> -> <PublicParts, SubordinateRole>);
         f!(<UnspecifiedParts, SubordinateRole> -> <PublicParts, UnspecifiedRole>);
-        f!(<UnspecifiedParts, SubordinateRole> -> <SecretParts, PrimaryRole>);
+        f_try!(<UnspecifiedParts, SubordinateRole> -> <SecretParts, PrimaryRole>);
         //f!(<UnspecifiedParts, SubordinateRole> -> <SecretParts, SubordinateRole>);
-        f!(<UnspecifiedParts, SubordinateRole> -> <SecretParts, UnspecifiedRole>);
+        f_try!(<UnspecifiedParts, SubordinateRole> -> <SecretParts, UnspecifiedRole>);
         //f!(<UnspecifiedParts, SubordinateRole> -> <UnspecifiedParts, PrimaryRole>);
         //f!(<UnspecifiedParts, SubordinateRole> -> <UnspecifiedParts, SubordinateRole>);
         //f!(<UnspecifiedParts, SubordinateRole> -> <UnspecifiedParts, UnspecifiedRole>);
@@ -424,8 +461,8 @@ macro_rules! create_conversions {
         f!(<UnspecifiedParts, UnspecifiedRole> -> <PublicParts, PrimaryRole>);
         f!(<UnspecifiedParts, UnspecifiedRole> -> <PublicParts, SubordinateRole>);
         //f!(<UnspecifiedParts, UnspecifiedRole> -> <PublicParts, UnspecifiedRole>);
-        f!(<UnspecifiedParts, UnspecifiedRole> -> <SecretParts, PrimaryRole>);
-        f!(<UnspecifiedParts, UnspecifiedRole> -> <SecretParts, SubordinateRole>);
+        f_try!(<UnspecifiedParts, UnspecifiedRole> -> <SecretParts, PrimaryRole>);
+        f_try!(<UnspecifiedParts, UnspecifiedRole> -> <SecretParts, SubordinateRole>);
         //f!(<UnspecifiedParts, UnspecifiedRole> -> <SecretParts, UnspecifiedRole>);
         //f!(<UnspecifiedParts, UnspecifiedRole> -> <UnspecifiedParts, PrimaryRole>);
         //f!(<UnspecifiedParts, UnspecifiedRole> -> <UnspecifiedParts, SubordinateRole>);
diff --git a/openpgp/src/serialize/cert.rs b/openpgp/src/serialize/cert.rs
index fe25c19a..30ed69a4 100644
--- a/openpgp/src/serialize/cert.rs
+++ b/openpgp/src/serialize/cert.rs
@@ -418,10 +418,13 @@ impl<'a> TSK<'a> {
 
         // Serializes public or secret key depending on the filter.
         let serialize_key =
-            |o: &mut dyn std::io::Write, key: &'a key::UnspecifiedSecret,
+            |o: &mut dyn std::io::Write, key: &'a key::UnspecifiedKey,
              tag_public, tag_secret|
         {
-            let tag = if key.has_secret() && (self.filter)(key) {
+            // We check for secrets here.
+            let tag = if key.has_secret()
+                && (self.filter)(key.try_into().expect("checked for secrets"))
+            {
                 tag_secret
             } else {
                 tag_public
@@ -465,10 +468,16 @@ impl<'a> TSK<'a> {
                     PacketRef::PublicKey(key.into()).serialize(o),
                 Tag::PublicSubkey =>
                     PacketRef::PublicSubkey(key.into()).serialize(o),
-                Tag::SecretKey =>
-                    PacketRef::SecretKey(key.into()).serialize(o),
-                Tag::SecretSubkey =>
-                    PacketRef::SecretSubkey(key.into()).serialize(o),
+                Tag::SecretKey => {
+                    PacketRef::SecretKey(
+                        key.try_into().expect("checked for secrets"))
+                        .serialize(o)
+                }
+                Tag::SecretSubkey => {
+                    PacketRef::SecretSubkey(
+                        key.try_into().expect("checked for secrets"))
+                    .serialize(o)
+                }
                 _ => unreachable!(),
             }
         };
@@ -568,9 +577,10 @@ impl<'a> MarshalInto for TSK<'a> {
 
         // Serializes public or secret key depending on the filter.
         let serialized_len_key
-            = |key: &'a key::UnspecifiedSecret, tag_public, tag_secret|
+            = |key: &'a key::UnspecifiedKey, tag_public, tag_secret|
         {
-            let tag = if key.has_secret() && (self.filter)(key) {
+            // We check for secrets here.
+            let tag = if key.has_secret() && (self.filter)(key.try_into().expect("have secrets")) {
                 tag_secret
             } else {
                 tag_public
@@ -589,8 +599,14 @@ impl<'a> MarshalInto for TSK<'a> {
             let packet = match tag {
                 Tag::PublicKey => PacketRef::PublicKey(key.into()),
                 Tag::PublicSubkey => PacketRef::PublicSubkey(key.into()),
-                Tag::SecretKey => PacketRef::SecretKey(key.into()),
-                Tag::SecretSubkey => PacketRef::SecretSubkey(key.into()),
+                Tag::SecretKey => {
+                    PacketRef::SecretKey(
+                        key.try_into().expect("checked for secrets"))
+                }
+                Tag::SecretSubkey => {
+                    PacketRef::SecretSubkey(
+                        key.try_into().expect("checked for secrets"))
+                }
                 _ => unreachable!(),
             };