ssh-key: extract a Comment type (#360)

Extracts a type which models the concerns of comments, namely that they
can be encoded as an RFC4251 `string` type which can hold arbitrary
binary data when stored in the binary serialization of a private key.

This replaces the various accessor methods on `PublicKey` and
`PrivateKey` for handling the various conversions.

Author: tarcieri

ssh-key/src/comment.rs

@@ -0,0 +1,158 @@
+//! SSH key comment support.
+
+use alloc::{borrow::ToOwned, boxed::Box, string::String, vec::Vec};
+use core::{
+    convert::Infallible,
+    fmt,
+    str::{self, FromStr},
+};
+use encoding::{Decode, Encode, Error, Reader, Writer};
+
+/// SSH key comment (e.g. email address of owner)
+///
+/// Comments may be found in both the binary serialization of  [`PrivateKey`] as well as the text
+/// serialization of [`PublicKey`].
+///
+/// The binary serialization of [`PrivateKey`] stores the comment encoded as an [RFC4251]
+/// `string` type which can contain arbitrary binary data and does not necessarily represent valid
+/// UTF-8. To support round trip encoding of such comments.
+///
+/// To support round-trip encoding of such comments, this type also supports arbitrary binary data.
+///
+/// [RFC4251]: https://datatracker.ietf.org/doc/html/rfc4251#section-5
+/// [`PrivateKey`]: crate::PrivateKey
+/// [`PublicKey`]: crate::PublicKey
+#[derive(Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub struct Comment(Box<[u8]>);
+
+impl AsRef<[u8]> for Comment {
+    fn as_ref(&self) -> &[u8] {
+        self.as_bytes()
+    }
+}
+
+impl AsRef<str> for Comment {
+    fn as_ref(&self) -> &str {
+        self.as_str_lossy()
+    }
+}
+
+impl Decode for Comment {
+    type Error = Error;
+
+    fn decode(reader: &mut impl Reader) -> encoding::Result<Self> {
+        Vec::<u8>::decode(reader).map(Into::into)
+    }
+}
+
+impl Encode for Comment {
+    fn encoded_len(&self) -> Result<usize, Error> {
+        self.0.encoded_len()
+    }
+
+    fn encode(&self, writer: &mut impl Writer) -> Result<(), Error> {
+        self.0.encode(writer)
+    }
+}
+
+impl FromStr for Comment {
+    type Err = Infallible;
+
+    fn from_str(s: &str) -> Result<Comment, Infallible> {
+        Ok(s.into())
+    }
+}
+
+impl From<&str> for Comment {
+    fn from(s: &str) -> Comment {
+        s.to_owned().into()
+    }
+}
+
+impl From<String> for Comment {
+    fn from(s: String) -> Self {
+        s.into_bytes().into()
+    }
+}
+
+impl From<&[u8]> for Comment {
+    fn from(bytes: &[u8]) -> Comment {
+        bytes.to_owned().into()
+    }
+}
+
+impl From<Vec<u8>> for Comment {
+    fn from(vec: Vec<u8>) -> Self {
+        Self(vec.into_boxed_slice())
+    }
+}
+
+impl From<Comment> for Vec<u8> {
+    fn from(comment: Comment) -> Vec<u8> {
+        comment.0.into()
+    }
+}
+
+impl TryFrom<Comment> for String {
+    type Error = Error;
+
+    fn try_from(comment: Comment) -> Result<String, Error> {
+        comment.as_str().map(ToOwned::to_owned)
+    }
+}
+
+impl fmt::Display for Comment {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(self.as_str_lossy())
+    }
+}
+
+impl Comment {
+    /// Interpret the comment as raw binary data.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Interpret the comment as a UTF-8 string.
+    pub fn as_str(&self) -> Result<&str, Error> {
+        Ok(str::from_utf8(&self.0)?)
+    }
+
+    /// Interpret the comment as a UTF-8 string.
+    ///
+    /// This is the maximal prefix of the comment which can be interpreted as valid UTF-8.
+    // TODO(tarcieri): precompute and store the offset which represents this prefix?
+    #[cfg(feature = "alloc")]
+    pub fn as_str_lossy(&self) -> &str {
+        for i in (1..=self.len()).rev() {
+            if let Ok(s) = str::from_utf8(&self.0[..i]) {
+                return s;
+            }
+        }
+
+        ""
+    }
+
+    /// Is the comment empty?
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+
+    /// Get the length of this comment in bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::Comment;
+
+    #[test]
+    fn as_str_lossy_ignores_non_utf8_data() {
+        const EXAMPLE: &[u8] = b"hello world\xc3\x28";
+
+        let comment = Comment::from(EXAMPLE);
+        assert_eq!(comment.as_str_lossy(), "hello world");
+    }
+}

ssh-key/src/lib.rs

@@ -51,7 +51,7 @@
 //!
 //! // Key attributes
 //! assert_eq!(public_key.algorithm(), ssh_key::Algorithm::Ed25519);
-//! assert_eq!(public_key.comment(), "[email protected]");
+//! assert_eq!(public_key.comment().as_bytes(), b"[email protected]");
 //!
 //! // Key data: in this example an Ed25519 key
 //! if let Some(ed25519_public_key) = public_key.key_data().ed25519() {
@@ -100,7 +100,7 @@
 //!
 //! // Key attributes
 //! assert_eq!(private_key.algorithm(), ssh_key::Algorithm::Ed25519);
-//! assert_eq!(private_key.comment(), "[email protected]");
+//! assert_eq!(private_key.comment().as_bytes(), b"[email protected]");
 //!
 //! // Key data: in this example an Ed25519 key
 //! if let Some(ed25519_keypair) = private_key.key_data().ed25519() {
@@ -156,6 +156,8 @@ mod error;
 mod fingerprint;
 mod kdf;
 
+#[cfg(feature = "alloc")]
+mod comment;
 #[cfg(feature = "std")]
 mod dot_ssh;
 #[cfg(feature = "ppk")]
@@ -183,6 +185,7 @@ pub use {
     crate::{
         algorithm::AlgorithmName,
         certificate::Certificate,
+        comment::Comment,
         known_hosts::KnownHosts,
         signature::{Signature, SigningKey},
         sshsig::SshSig,

ssh-key/src/private.rs

@@ -124,7 +124,7 @@ pub use self::{
 
 #[cfg(feature = "alloc")]
 pub use crate::{
-    SshSig,
+    Comment, SshSig,
     private::{
         dsa::{DsaKeypair, DsaPrivateKey},
         opaque::{OpaqueKeypair, OpaqueKeypairBytes, OpaquePrivateKeyBytes},
@@ -212,7 +212,7 @@ impl PrivateKey {
     ///
     /// On `no_std` platforms, use `PrivateKey::from(key_data)` instead.
     #[cfg(feature = "alloc")]
-    pub fn new(key_data: KeypairData, comment: impl Into<Vec<u8>>) -> Result<Self> {
+    pub fn new(key_data: KeypairData, comment: impl Into<Comment>) -> Result<Self> {
         if key_data.is_encrypted() {
             return Err(Error::Encrypted);
         }
@@ -465,43 +465,8 @@ impl PrivateKey {
 
     /// Comment on the key (e.g. email address).
     #[cfg(feature = "alloc")]
-    #[deprecated(
-        since = "0.7.0",
-        note = "please use `comment_bytes`, `comment_str`, or `comment_str_lossy` instead"
-    )]
-    pub fn comment(&self) -> &str {
-        self.comment_str_lossy()
-    }
-
-    /// Comment on the key (e.g. email address).
-    #[cfg(not(feature = "alloc"))]
-    pub fn comment_bytes(&self) -> &[u8] {
-        b""
-    }
-
-    /// Comment on the key (e.g. email address).
-    ///
-    /// Since comments can contain arbitrary binary data when decoded from a
-    /// private key, this returns the raw bytes of the comment.
-    #[cfg(feature = "alloc")]
-    pub fn comment_bytes(&self) -> &[u8] {
-        self.public_key.comment_bytes()
-    }
-
-    /// Comment on the key (e.g. email address).
-    ///
-    /// This returns a UTF-8 interpretation of the comment when valid.
-    #[cfg(feature = "alloc")]
-    pub fn comment_str(&self) -> core::result::Result<&str, str::Utf8Error> {
-        self.public_key.comment_str()
-    }
-
-    /// Comment on the key (e.g. email address).
-    ///
-    /// This returns as much data as can be interpreted as valid UTF-8.
-    #[cfg(feature = "alloc")]
-    pub fn comment_str_lossy(&self) -> &str {
-        self.public_key.comment_str_lossy()
+    pub fn comment(&self) -> &Comment {
+        self.public_key.comment()
     }
 
     /// Cipher algorithm (a.k.a. `ciphername`).
@@ -575,7 +540,7 @@ impl PrivateKey {
 
     /// Set the comment on the key.
     #[cfg(feature = "alloc")]
-    pub fn set_comment(&mut self, comment: impl Into<Vec<u8>>) {
+    pub fn set_comment(&mut self, comment: impl Into<Comment>) {
         self.public_key.set_comment(comment);
     }
 
@@ -681,7 +646,13 @@ impl PrivateKey {
         checkint.encode(writer)?;
         checkint.encode(writer)?;
         self.key_data.encode(writer)?;
-        self.comment_bytes().encode(writer)?;
+
+        // Serialize comment
+        #[cfg(not(feature = "alloc"))]
+        b"".encode(writer)?;
+        #[cfg(feature = "alloc")]
+        self.comment().encode(writer)?;
+
         writer.write(&PADDING_BYTES[..padding_len])?;
         Ok(())
     }
@@ -701,10 +672,15 @@ impl PrivateKey {
         // This method is intended for use with unencrypted keys only
         debug_assert!(!self.is_encrypted(), "called on encrypted key");
 
+        #[cfg(not(feature = "alloc"))]
+        let comment_len = 0;
+        #[cfg(feature = "alloc")]
+        let comment_len = self.comment().encoded_len()?;
+
         [
             8, // 2 x uint32 checkints,
             self.key_data.encoded_len()?,
-            self.comment_bytes().encoded_len()?,
+            comment_len,
         ]
         .checked_sum()
     }