ssh-key: decode Certificate when found in KeyData (#449)

When a certificate key type is found in SSH binary format being decoded
by KeyData::decode, decode it into a Certificate, stored in a new variant of
the KeyData enum.

Previously, KeyData::decode attempted to decode certificates as an
opaque key, which triggered overflow and/or encoding errors, because the
OpaquePublicKeyBytes::decode function only handled the first
length-prefixed field of the certificate (expecting only binary key
data), and allowed the torn remainder of the certificate data to be
handled by whatever function called `Reader::read` next.

Added conversion to/from Certificate/KeyData, and integration tests for
decoding a Certificate from wire-format binary public key.

Signed-off-by: Ross Williams <ross@ross-williams.net>

Author: overhacked

ssh-key/src/certificate.rs

@@ -9,11 +9,10 @@ mod unix_time;
 pub use self::{builder::Builder, cert_type::CertType, field::Field, options_map::OptionsMap};
 
 use crate::{
-    Algorithm, Error, Fingerprint, HashAlg, Result, Signature,
+    Algorithm, Comment, Error, Fingerprint, HashAlg, Result, Signature,
     public::{KeyData, SshFormat},
 };
 use alloc::{
-    borrow::ToOwned,
     string::{String, ToString},
     vec::Vec,
 };
@@ -117,7 +116,7 @@ use {
 /// human-readable formats like JSON and TOML.
 ///
 /// [PROTOCOL.certkeys]: https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.certkeys?annotate=HEAD
-#[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[derive(Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
 pub struct Certificate {
     /// CA-provided random bitstring of arbitrary length
     /// (but typically 16 or 32 bytes).
@@ -160,7 +159,7 @@ pub struct Certificate {
     signature: Signature,
 
     /// Comment on the certificate.
-    comment: String,
+    comment: Comment,
 }
 
 impl Certificate {
@@ -182,7 +181,7 @@ impl Certificate {
             return Err(Error::AlgorithmUnknown);
         }
 
-        cert.comment = encapsulation.comment.to_owned();
+        cert.comment = Comment::from(encapsulation.comment);
         Ok(reader.finish(cert)?)
     }
 
@@ -229,7 +228,12 @@ impl Certificate {
 
     /// Get the comment on this certificate.
     pub fn comment(&self) -> &str {
-        self.comment.as_str()
+        self.comment.as_str_lossy()
+    }
+
+    /// Set the comment on this certificate.
+    pub fn set_comment(&mut self, comment: impl Into<Comment>) {
+        self.comment = comment.into();
     }
 
     /// Nonces are a CA-provided random bitstring of arbitrary length
@@ -470,7 +474,7 @@ impl Certificate {
             reserved: Vec::decode(reader)?,
             signature_key: reader.read_prefixed(KeyData::decode)?,
             signature: reader.read_prefixed(Signature::decode)?,
-            comment: String::new(),
+            comment: Comment::default(),
         })
     }
 }

ssh-key/src/certificate/builder.rs

@@ -1,7 +1,7 @@
 //! OpenSSH certificate builder.
 
 use super::{CertType, Certificate, Field, OptionsMap};
-use crate::{Result, Signature, SigningKey, public};
+use crate::{Comment, Result, Signature, SigningKey, public};
 use alloc::{string::String, vec::Vec};
 
 #[cfg(feature = "rand_core")]
@@ -88,7 +88,7 @@ pub struct Builder {
     valid_before: u64,
     critical_options: OptionsMap,
     extensions: OptionsMap,
-    comment: Option<String>,
+    comment: Option<Comment>,
 }
 
 impl Builder {
@@ -255,7 +255,7 @@ impl Builder {
     /// Add a comment to this certificate.
     ///
     /// Default `""`
-    pub fn comment(&mut self, comment: impl Into<String>) -> Result<&mut Self> {
+    pub fn comment(&mut self, comment: impl Into<Comment>) -> Result<&mut Self> {
         if self.comment.is_some() {
             return Err(Field::Comment.invalid_error());
         }

ssh-key/src/certificate/cert_type.rs

@@ -4,7 +4,7 @@ use crate::{Error, Result};
 use encoding::{Decode, Encode, Reader, Writer};
 
 /// Types of OpenSSH certificates: user or host.
-#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
+#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
 #[repr(u32)]
 #[derive(Default)]
 pub enum CertType {

ssh-key/src/certificate/options_map.rs

@@ -9,7 +9,7 @@ use core::{
 use encoding::{CheckedSum, Decode, Encode, Reader, Writer};
 
 /// Key/value map type used for certificate's critical options and extensions.
-#[derive(Clone, Debug, Default, Eq, PartialEq, PartialOrd, Ord)]
+#[derive(Clone, Debug, Default, Eq, Hash, PartialEq, PartialOrd, Ord)]
 pub struct OptionsMap(pub BTreeMap<String, String>);
 
 impl OptionsMap {

ssh-key/src/public/key_data.rs

@@ -5,7 +5,11 @@ use crate::{Algorithm, Error, Fingerprint, HashAlg, Result};
 use encoding::{CheckedSum, Decode, Encode, Reader, Writer};
 
 #[cfg(feature = "alloc")]
-use super::{DsaPublicKey, OpaquePublicKey, RsaPublicKey};
+use {
+    super::{DsaPublicKey, OpaquePublicKey, RsaPublicKey},
+    crate::Certificate,
+    alloc::boxed::Box,
+};
 
 #[cfg(feature = "ecdsa")]
 use super::{EcdsaPublicKey, SkEcdsaSha2NistP256};
@@ -40,6 +44,14 @@ pub enum KeyData {
     /// [PROTOCOL.u2f]: https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.u2f?annotate=HEAD
     SkEd25519(SkEd25519),
 
+    /// Certificate key as specified in [draft-miller-ssh-cert]
+    /// (and [PROTOCOL.certkeys], now deprecated in favor of the IETF draft).
+    ///
+    /// [draft-miller-ssh-cert]: https://datatracker.ietf.org/doc/draft-miller-ssh-cert
+    /// [PROTOCOL.certkeys]: https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.certkeys?annotate=HEAD
+    #[cfg(feature = "alloc")]
+    Certificate(Box<Certificate>),
+
     /// Opaque public key data.
     #[cfg(feature = "alloc")]
     Other(OpaquePublicKey),
@@ -60,6 +72,8 @@ impl KeyData {
             Self::SkEcdsaSha2NistP256(_) => Algorithm::SkEcdsaSha2NistP256,
             Self::SkEd25519(_) => Algorithm::SkEd25519,
             #[cfg(feature = "alloc")]
+            Self::Certificate(cert) => cert.algorithm(),
+            #[cfg(feature = "alloc")]
             Self::Other(key) => key.algorithm(),
         }
     }
@@ -133,6 +147,24 @@ impl KeyData {
         }
     }
 
+    /// Get the certificate if this key is the correct type.
+    #[cfg(feature = "alloc")]
+    pub fn certificate(&self) -> Option<&Certificate> {
+        match self {
+            Self::Certificate(certificate) => Some(certificate.as_ref()),
+            _ => None,
+        }
+    }
+
+    /// Get the certificate, consuming the [`KeyData`], if this key is the correct type.
+    #[cfg(feature = "alloc")]
+    pub fn into_certificate(self) -> Option<Certificate> {
+        match self {
+            Self::Certificate(certificate) => Some(*certificate),
+            _ => None,
+        }
+    }
+
     /// Is this key a DSA key?
     #[cfg(feature = "alloc")]
     pub fn is_dsa(&self) -> bool {
@@ -173,6 +205,12 @@ impl KeyData {
         matches!(self, Self::Other(_))
     }
 
+    /// Is this a certificate?
+    #[cfg(feature = "alloc")]
+    pub fn is_certificate(&self) -> bool {
+        matches!(self, Self::Certificate(_))
+    }
+
     /// Decode [`KeyData`] for the specified algorithm.
     pub fn decode_as(reader: &mut impl Reader, algorithm: Algorithm) -> Result<Self> {
         match algorithm {
@@ -198,6 +236,12 @@ impl KeyData {
         }
     }
 
+    /// Decode [`KeyData`] as a certificate with the specified algorithm.
+    #[cfg(feature = "alloc")]
+    pub fn decode_as_certificate(reader: &mut impl Reader, algorithm: Algorithm) -> Result<Self> {
+        Certificate::decode_as(reader, algorithm).map(|cert| Self::Certificate(Box::new(cert)))
+    }
+
     /// Get the encoded length of this key data without a leading algorithm
     /// identifier.
     pub(crate) fn encoded_key_data_len(&self) -> encoding::Result<usize> {
@@ -213,6 +257,8 @@ impl KeyData {
             Self::SkEcdsaSha2NistP256(sk) => sk.encoded_len(),
             Self::SkEd25519(sk) => sk.encoded_len(),
             #[cfg(feature = "alloc")]
+            Self::Certificate(cert) => cert.encoded_len(),
+            #[cfg(feature = "alloc")]
             Self::Other(other) => other.key.encoded_len(),
         }
     }
@@ -231,6 +277,8 @@ impl KeyData {
             Self::SkEcdsaSha2NistP256(sk) => sk.encode(writer),
             Self::SkEd25519(sk) => sk.encode(writer),
             #[cfg(feature = "alloc")]
+            Self::Certificate(cert) => cert.encode(writer),
+            #[cfg(feature = "alloc")]
             Self::Other(other) => other.key.encode(writer),
         }
     }
@@ -241,6 +289,12 @@ impl Decode for KeyData {
 
     fn decode(reader: &mut impl Reader) -> Result<Self> {
         let algorithm = Algorithm::decode(reader)?;
+        #[cfg(feature = "alloc")]
+        if let Algorithm::Other(name) = &algorithm {
+            if let Ok(certificate_algorithm) = Algorithm::new_certificate(name.as_str()) {
+                return Self::decode_as_certificate(reader, certificate_algorithm);
+            }
+        }
         Self::decode_as(reader, algorithm)
     }
 }
@@ -299,3 +353,10 @@ impl From<SkEd25519> for KeyData {
         Self::SkEd25519(public_key)
     }
 }
+
+#[cfg(feature = "alloc")]
+impl From<Certificate> for KeyData {
+    fn from(certificate: Certificate) -> KeyData {
+        Self::Certificate(Box::new(certificate))
+    }
+}

ssh-key/src/signature.rs

@@ -83,7 +83,7 @@ where
 /// [RFC5656]: https://datatracker.ietf.org/doc/html/rfc5656
 /// [RFC8032]: https://datatracker.ietf.org/doc/html/rfc8032
 /// [RFC8332]: https://datatracker.ietf.org/doc/html/rfc8332
-#[derive(Clone, Eq, PartialEq, PartialOrd, Ord)]
+#[derive(Clone, Eq, Hash, PartialEq, PartialOrd, Ord)]
 pub struct Signature {
     /// Signature algorithm.
     algorithm: Algorithm,

ssh-key/tests/certificate.rs

@@ -2,8 +2,9 @@
 
 #![cfg(feature = "alloc")]
 
+use encoding::Decode;
 use hex_literal::hex;
-use ssh_key::{Algorithm, Certificate};
+use ssh_key::{Algorithm, Certificate, public::KeyData};
 use std::str::FromStr;
 
 #[cfg(feature = "ecdsa")]
@@ -262,6 +263,44 @@ fn encode_rsa_4096_openssh() {
     );
 }
 
+fn decode_keydata(certificate_str: &str) {
+    // Decode certificate from OpenSSH format to bytes-on-wire
+    let cert = Certificate::from_str(certificate_str).unwrap();
+    let cert_encoded = cert.to_bytes().unwrap();
+
+    // Decode bytes-on-wire to KeyData
+    let mut reader = &cert_encoded[..];
+    let key_data = KeyData::decode(&mut reader).unwrap();
+
+    // Convert KeyData to Certificate and override comment from
+    // OpenSSH encapsulation format so input and output match
+    let mut cert_decoded = key_data.into_certificate().unwrap();
+    cert_decoded.set_comment(cert.comment());
+
+    // Write Certificate to OpenSSH format and compare to input
+    assert_eq!(
+        certificate_str.trim_end(),
+        &cert_decoded.to_openssh().unwrap()
+    );
+}
+
+#[cfg(feature = "ecdsa")]
+#[test]
+fn decode_ecdsa_keydata() {
+    decode_keydata(ECDSA_P256_CERT_EXAMPLE)
+}
+
+#[cfg(feature = "ed25519")]
+#[test]
+fn decode_ed25519_keydata() {
+    decode_keydata(ED25519_CERT_EXAMPLE)
+}
+
+#[test]
+fn decode_rsa_4096_keydata() {
+    decode_keydata(RSA_4096_CERT_EXAMPLE)
+}
+
 #[cfg(feature = "ed25519")]
 #[test]
 fn verify_ed25519_certificate_signature() {