ssh-key: decode Certificate when found in KeyData

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

@@ -17,7 +17,10 @@ use alloc::{
     string::{String, ToString},
     vec::Vec,
 };
-use core::str::FromStr;
+use core::{
+    hash::{Hash, Hasher},
+    str::FromStr,
+};
 use encoding::{Base64Reader, CheckedSum, Decode, Encode, Reader, Writer};
 use signature::Verifier;
 
@@ -160,7 +163,7 @@ pub struct Certificate {
     signature: Signature,
 
     /// Comment on the certificate.
-    comment: String,
+    pub comment: String,
 }
 
 impl Certificate {
@@ -511,6 +514,23 @@ impl Encode for Certificate {
     }
 }
 
+impl Hash for Certificate {
+    #[inline]
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.public_key.hash(state);
+        self.serial.hash(state);
+        self.cert_type.hash(state);
+        self.key_id.hash(state);
+        self.valid_principals.hash(state);
+        self.valid_after.hash(state);
+        self.valid_before.hash(state);
+        self.critical_options.hash(state);
+        self.extensions.hash(state);
+        self.signature_key.hash(state);
+        self.signature.hash(state);
+    }
+}
+
 impl FromStr for Certificate {
     type Err = Error;
 

ssh-key/src/certificate/cert_type.rs

@@ -1,10 +1,12 @@
 //! OpenSSH certificate types.
 
+use core::hash::Hash;
+
 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/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

@@ -2,7 +2,7 @@
 
 use crate::{Algorithm, EcdsaCurve, Error, Mpint, PrivateKey, PublicKey, Result, private, public};
 use alloc::vec::Vec;
-use core::fmt;
+use core::{fmt, hash::Hash};
 use encoding::{CheckedSum, Decode, Encode, Reader, Writer};
 use signature::{SignatureEncoding, Signer, Verifier};
 
@@ -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.comment = cert.comment().into();
+
+    // 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() {