Remove PGP and SSH signature support (#220)

Author: casey

DESIGN.md

@@ -8,18 +8,6 @@ Open Questions
   since deriving a key with an explicit context seems like good practice, but
   ed25519-dalek doesn't support it.
 
-- *Should filepack attempt to support PGP v5 (LibraPGP) or PGP v6 signatures?*
-  In the case of v5, no additional data is needed, we would only be generating
-  different message bytes. In the case of v6, we need an additional 32-byte
-  salt. Because the bech32m PGP signature string is already `signature1ap4…`,
-  it would be easy to add v5 and v6 variants.
-
-- *Should filepack attempt to support other signature schemes, like signify and
-  minisign?* Any existing signature scheme which uses Ed25519 and SHA-512 and
-  signs a superset of the message signed by the core filepack scheme could be
-  supported and treated interchangeably. I think it's really only a question of
-  demand.
-
 - *Should filepack bech32m signature strings include the public key?* This
   would allow them to be self-describing and potentially easier to handle. If
   we did this, the map of public keys to signatures in the notes struct would
@@ -35,55 +23,15 @@ Open Questions
   will be detectable by failing to verify, but I want to make sure my
   understanding is correct.
 
-- *Will signatures made with existing PGP and SSH keys pass strict
-  verification?* We use ed25519-dalek's strict verification of signatures. This
-  prevents certain kind of signature malleability issues, however it was not
-  part of the original RFC, and I wonder if there are any users with existing
-  PGP and SSH keys which fail strict verification.
-
 Closed Questions
-
-- *Should filepack signatures include an additional byte specifying the hashing
-  algorithm?* Currently, three filepack signature types are supported,
-  filepack, PGP, and SSH. All use Ed25519 as the signature scheme. The filepack
-  signature scheme signs an unhashed message, so no other hashing algorithms
-  are in use. SSH on the other hand signs a message including a hash of the
-  message, and PGP signs a hash of an outer message that includes the inner
-  message. Currently, we only allow SHA-512 as the hash algorithm in SSH and
-  PGP signatures. This is both because it is the default algorithm that
-  `ssh-keygen` and `gpg` use when creating Ed25519 signatures, and because
-  Ed25519 uses SHA-512 internally, so using any other hashing algorithm
-  introduces a new dependency. So, and here we finally get to the question,
-  should we add a byte to the `signature1a…` bech32 representation of
-  signatures which represents the hashing algorithm? This would allow us to
-  extend the existing signature encoding scheme to support multiple hashing
-  algorithms, although we would only do that if absolutely necessary, since
-  SHA-512 is a perfectly good default, and the only reason to add additional
-  algorithms would be something like supporting existing signing devices which
-  do not support SHA-512. **Conclusion: This seems a very common degree of
-  freedom in signature schemes, so I added it. Filepack, PGP, and SSH
-  signatures now start with `af0q`, `ap4p`, and, `as0p`. The last character is
-  the hashing algorithm, filepack does not hash the message, so it's `q`, which
-  is the 0th bech32m element, PGP and SSH are restricted to SHA-512, which is
-  `p`, or the 1st element. No support exists for alternative combinations of
-  signature scheme and hash function, this is only in case we have to add
-  support in the future.**
-
 ----------------
 
-- *Should filepack re-use an existing signature system, like SSH or PGP?* I
-  looked into both, but the formats and algorithms are incredibly complex, and
-  allow a huge number of unnecessary degrees of freedom. **Conclusion: I added
-  support for multiple signature schemes, filepack, GPG v4, and SSH. Filepack
-  is the simple scheme which filepack uses when signing messages using keys it
-  manages. The GPG and SSH schemes are compatible with GPG and SSH, but
-  restricted to Ed25519 as the signature scheme, and SHA-512 as the hash
-  algorithm. This allows signatures to be created using GPG or SSH, using
-  existing keys, key management, and signers, and imported into filepack. All
-  signature schemes sign the same message, wrapped in the case of GPG and SSH,
-  and the resulting message and public key are always Ed25519 keys, and aside
-  from the difference in message bytes, all signature schemes are verified in
-  the same way.**
+- *Should filepack support SSH or PGP Ed25519 signatures?* Supporting SSH and
+  PGP signatures would let developers reuse existing Ed25519 keys, and make use
+  of existing key managment systems and hardware signers **Conclusion: I added
+  support for SSH and PGP signatures, but ultimately decided to remove it. This
+  is something that adds complexity, so I want to do it based on actual demand,
+  concrete benefits, and actual use-cases. **
 
 - *Should Bech32m be used for fingerprints, public keys, private keys, and
   signatures?* This would make them distinct and easy to identify, and give

bin/generate-gpg-test-files

@@ -7,13 +7,20 @@ pub(crate) struct Bech32Decoder<'a> {
 }
 
 impl<'a> Bech32Decoder<'a> {
-  pub(crate) fn byte_array<const LEN: usize>(&mut self) -> Result<[u8; LEN], Bech32Error> {
+  pub(crate) fn byte_array<const LEN: usize>(mut self) -> Result<[u8; LEN], Bech32Error> {
     let mut array = [0; LEN];
 
     for (slot, byte) in array.iter_mut().zip(self.bytes(LEN)?) {
       *slot = byte;
     }
 
+    let excess = self.data.len() - self.i;
+
+    ensure! {
+      excess == 0,
+      bech32_error::Overlong { excess, ty: self.ty },
+    }
+
     Ok(array)
   }
 
@@ -43,39 +50,6 @@ impl<'a> Bech32Decoder<'a> {
     Ok(fes.fes_to_bytes())
   }
 
-  pub(crate) fn done(self) -> Result<(), Bech32Error> {
-    let excess = self.data.len() - self.i;
-
-    ensure! {
-      excess == 0,
-      bech32_error::Overlong { excess, ty: self.ty },
-    }
-
-    Ok(())
-  }
-
-  pub(crate) fn fe(&mut self) -> Result<Fe32, Bech32Error> {
-    let next = self
-      .data
-      .get(self.i)
-      .map(|c| Fe32::from_char_unchecked(*c))
-      .context(bech32_error::Truncated { ty: self.ty })?;
-
-    self.i += 1;
-
-    Ok(next)
-  }
-
-  pub(crate) fn fe_array<const LEN: usize>(&mut self) -> Result<[Fe32; LEN], Bech32Error> {
-    let mut array = [Fe32::Q; LEN];
-
-    for slot in &mut array {
-      *slot = self.fe()?;
-    }
-
-    Ok(array)
-  }
-
   fn fes(
     &mut self,
     len: usize,
@@ -93,11 +67,6 @@ impl<'a> Bech32Decoder<'a> {
     Ok(fes.iter().map(|c| Fe32::from_char_unchecked(*c)))
   }
 
-  pub(crate) fn into_bytes(mut self) -> Result<Vec<u8>, Bech32Error> {
-    let fes = self.data.len() - self.i;
-    Ok(self.bytes(fes * 5 / 8)?.collect())
-  }
-
   pub(crate) fn new(ty: Bech32Type, s: &'a str) -> Result<Self, Bech32Error> {
     let hrp_string =
       CheckedHrpstring::new::<bech32::Bech32m>(s).context(bech32_error::Decode { ty })?;
@@ -125,3 +94,49 @@ impl<'a> Bech32Decoder<'a> {
     })
   }
 }
+
+#[cfg(test)]
+mod tests {
+  use super::*;
+
+  #[test]
+  fn length() {
+    #[track_caller]
+    fn case(s: &str) {
+      use bech32::Checksum;
+      assert!(s.len() <= bech32::Bech32m::CODE_LENGTH);
+    }
+
+    case(test::FINGERPRINT);
+    case(test::PUBLIC_KEY);
+    case(test::PRIVATE_KEY);
+    case(test::SIGNATURE);
+  }
+
+  #[test]
+  fn private_key_round_trip() {
+    assert_eq!(
+      test::PRIVATE_KEY
+        .parse::<PrivateKey>()
+        .unwrap()
+        .display_secret()
+        .to_string(),
+      test::PRIVATE_KEY,
+    );
+  }
+
+  #[test]
+  fn round_trip() {
+    #[track_caller]
+    fn case<T: FromStr + ToString>(s: &str)
+    where
+      T::Err: fmt::Debug,
+    {
+      assert_eq!(s.parse::<T>().unwrap().to_string(), s);
+    }
+
+    case::<Fingerprint>(test::FINGERPRINT);
+    case::<PublicKey>(test::PUBLIC_KEY);
+    case::<Signature>(test::SIGNATURE);
+  }
+}

bin/generate-ssh-test-files

@@ -10,10 +10,6 @@ impl Bech32Encoder {
     self.fes.extend(bytes.iter().copied().bytes_to_fes());
   }
 
-  pub(crate) fn fes(&mut self, fes: &[Fe32]) {
-    self.fes.extend_from_slice(fes);
-  }
-
   pub(crate) fn new(ty: Bech32Type) -> Self {
     Self {
       fes: vec![BECH32_VERSION],

src/bech32_decoder.rs

@@ -23,9 +23,8 @@ impl FromStr for Fingerprint {
   type Err = Bech32Error;
 
   fn from_str(s: &str) -> Result<Self, Self::Err> {
-    let mut decoder = Bech32Decoder::new(Bech32Type::Fingerprint, s)?;
+    let decoder = Bech32Decoder::new(Bech32Type::Fingerprint, s)?;
     let inner = decoder.byte_array()?;
-    decoder.done()?;
     Ok(Self(inner.into()))
   }
 }

src/bech32_encoder.rs

@@ -54,8 +54,6 @@ use {
     path_error::PathError,
     public_key_error::PublicKeyError,
     sign_options::SignOptions,
-    signature_error::SignatureError,
-    signature_scheme::SignatureSchemeType,
     style::Style,
     subcommand::Subcommand,
     tag::Tag,
@@ -94,9 +92,7 @@ use {
     sync::LazyLock,
     time::{SystemTime, SystemTimeError, UNIX_EPOCH},
   },
-  strum::{
-    EnumDiscriminants, EnumIter, EnumString, IntoDiscriminant, IntoEnumIterator, IntoStaticStr,
-  },
+  strum::{EnumDiscriminants, EnumIter, EnumString, IntoEnumIterator, IntoStaticStr},
   url::Url,
   usized::IntoU64,
   walkdir::WalkDir,
@@ -106,11 +102,11 @@ pub use self::{
   directory::Directory, entry::Entry, error::Error, file::File, fingerprint::Fingerprint,
   hash::Hash, manifest::Manifest, message::Message, note::Note, private_key::PrivateKey,
   public_key::PublicKey, relative_path::RelativePath, serialized_message::SerializedMessage,
-  signature::Signature, signature_scheme::SignatureScheme,
+  signature::Signature,
 };
 
 #[cfg(test)]
-use std::collections::HashSet;
+use {std::collections::HashSet, strum::IntoDiscriminant};
 
 #[cfg(test)]
 fn tempdir() -> tempfile::TempDir {
@@ -134,6 +130,19 @@ macro_rules! assert_matches {
   }
 }
 
+#[cfg(test)]
+macro_rules! assert_matches_regex {
+  ($haystack:expr, $re:expr $(,)?) => {{
+    let haystack = $haystack;
+    let re = Regex::new(&$re).unwrap();
+    assert!(
+      re.is_match(&haystack),
+      "assertion failed: `{haystack:?}` does not match `{}`",
+      re.as_str(),
+    );
+  }};
+}
+
 mod arguments;
 mod bech32_decoder;
 mod bech32_encoder;
@@ -187,8 +196,6 @@ mod relative_path;
 mod serialized_message;
 mod sign_options;
 mod signature;
-mod signature_error;
-mod signature_scheme;
 mod style;
 mod subcommand;
 mod tag;

src/fingerprint.rs

@@ -65,10 +65,11 @@ mod tests {
       test::PUBLIC_KEY,
       test::SIGNATURE,
     );
-    assert_eq!(
+
+    assert_matches_regex! {
       serde_json::from_str::<Note>(&json).unwrap_err().to_string(),
-      "invalid entry: found duplicate key at line 1 column 405",
-    );
+      r"invalid entry: found duplicate key at line 1 column \d+",
+    }
   }
 
   #[test]