ssh-key: non-UTF-8 comment support (#347)

OpenSSH private keys contain a comment field in the binary serialization
encoded as an RFC4251 `string` type, which is described as follows:

    Arbitrary length binary string.  Strings are allowed to contain
    arbitrary binary data, including null characters and 8-bit
    characters

Previously this field was stored using Rust's `String` type which
constrains the allowed encoding of the comments to UTF-8, leading to a
failure to parse keys containing such comments.

This has been changed to store a `Vec<u8>`, with helper methods
`comment_str` and `comment_str_lossy` to obtain `&str` references, and
`comment` bytes to get the raw value. The original `comment` methods
have been deprecated.

The lossy algorithm decodes as much of the string as possible as UTF-8.

The public key serialization, which encodes the key as a one-line
string, uses the lossy encoding of the comment so as to still remain
`String`-compatible. If there's desire to correct this, it will require
more substantial changes to the entire public key decoder/encoder.

Fixes #331

Author: tarcieri

ssh-key/src/ppk.rs

@@ -446,7 +446,7 @@ impl PpkContainer {
         let keypair_data =
             decode_private_key_as(&mut private_key_cursor, public_key.clone(), ppk.algorithm)?;
 
-        public_key.comment = comment.unwrap_or_default();
+        public_key.comment = comment.unwrap_or_default().into();
 
         Ok(PpkContainer {
             public_key,

ssh-key/src/private.rs

@@ -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<String>) -> Result<Self> {
+    pub fn new(key_data: KeypairData, comment: impl Into<Vec<u8>>) -> Result<Self> {
         if key_data.is_encrypted() {
             return Err(Error::Encrypted);
         }
@@ -464,8 +464,44 @@ 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.public_key.comment()
+        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()
     }
 
     /// Cipher algorithm (a.k.a. `ciphername`).
@@ -539,7 +575,7 @@ impl PrivateKey {
 
     /// Set the comment on the key.
     #[cfg(feature = "alloc")]
-    pub fn set_comment(&mut self, comment: impl Into<String>) {
+    pub fn set_comment(&mut self, comment: impl Into<Vec<u8>>) {
         self.public_key.set_comment(comment);
     }
 
@@ -645,7 +681,7 @@ impl PrivateKey {
         checkint.encode(writer)?;
         checkint.encode(writer)?;
         self.key_data.encode(writer)?;
-        self.comment().encode(writer)?;
+        self.comment_bytes().encode(writer)?;
         writer.write(&PADDING_BYTES[..padding_len])?;
         Ok(())
     }
@@ -668,7 +704,7 @@ impl PrivateKey {
         [
             8, // 2 x uint32 checkints,
             self.key_data.encoded_len()?,
-            self.comment().encoded_len()?,
+            self.comment_bytes().encoded_len()?,
         ]
         .checked_sum()
     }

ssh-key/src/public.rs

@@ -30,7 +30,7 @@ pub use self::{ecdsa::EcdsaPublicKey, sk::SkEcdsaSha2NistP256};
 pub(crate) use self::ssh_format::SshFormat;
 
 use crate::{Algorithm, Error, Fingerprint, HashAlg, Result};
-use core::str::FromStr;
+use core::str::{self, FromStr};
 use encoding::{Base64Reader, Decode, Reader};
 
 #[cfg(feature = "alloc")]
@@ -93,16 +93,21 @@ pub struct PublicKey {
     pub(crate) key_data: KeyData,
 
     /// Comment on the key (e.g. email address)
+    ///
+    /// Note that when a [`PublicKey`] is serialized in a private key, the
+    /// comment is encoded as an RFC4251 `string` which may contain arbitrary
+    /// binary data, so `Vec<u8>` is used to store the comment to ensure keys
+    /// containing such comments successfully round-trip.
     #[cfg(feature = "alloc")]
-    pub(crate) comment: String,
+    pub(crate) comment: Vec<u8>,
 }
 
 impl PublicKey {
     /// Create a new public key with the given comment.
     ///
     /// On `no_std` platforms, use `PublicKey::from(key_data)` instead.
     #[cfg(feature = "alloc")]
-    pub fn new(key_data: KeyData, comment: impl Into<String>) -> Self {
+    pub fn new(key_data: KeyData, comment: impl Into<Vec<u8>>) -> Self {
         Self {
             key_data,
             comment: comment.into(),
@@ -129,7 +134,7 @@ impl PublicKey {
         let public_key = Self {
             key_data,
             #[cfg(feature = "alloc")]
-            comment: encapsulation.comment.to_owned(),
+            comment: encapsulation.comment.to_owned().into(),
         };
 
         Ok(reader.finish(public_key)?)
@@ -144,19 +149,23 @@ impl PublicKey {
 
     /// Encode OpenSSH-formatted public key.
     pub fn encode_openssh<'o>(&self, out: &'o mut [u8]) -> Result<&'o str> {
-        SshFormat::encode(
-            self.algorithm().as_str(),
-            &self.key_data,
-            self.comment(),
-            out,
-        )
+        #[cfg(not(feature = "alloc"))]
+        let comment = "";
+        #[cfg(feature = "alloc")]
+        let comment = self.comment_str_lossy();
+
+        SshFormat::encode(self.algorithm().as_str(), &self.key_data, comment, out)
     }
 
     /// Encode an OpenSSH-formatted public key, allocating a [`String`] for
     /// the result.
     #[cfg(feature = "alloc")]
     pub fn to_openssh(&self) -> Result<String> {
-        SshFormat::encode_string(self.algorithm().as_str(), &self.key_data, self.comment())
+        SshFormat::encode_string(
+            self.algorithm().as_str(),
+            &self.key_data,
+            self.comment_str_lossy(),
+        )
     }
 
     /// Serialize SSH public key as raw bytes.
@@ -249,17 +258,48 @@ impl PublicKey {
     }
 
     /// Comment on the key (e.g. email address).
-    #[cfg(not(feature = "alloc"))]
+    ///
+    /// This is a deprecated alias for [`PublicKey::comment_str_lossy`].
+    #[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).
+    ///
+    /// 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(&self) -> &str {
+    pub fn comment_bytes(&self) -> &[u8] {
         &self.comment
     }
 
+    /// 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> {
+        str::from_utf8(&self.comment)
+    }
+
+    /// 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 {
+        for i in (1..=self.comment.len()).rev() {
+            if let Ok(s) = str::from_utf8(&self.comment[..i]) {
+                return s;
+            }
+        }
+
+        ""
+    }
+
     /// Public key data.
     pub fn key_data(&self) -> &KeyData {
         &self.key_data
@@ -274,7 +314,7 @@ impl PublicKey {
 
     /// Set the comment on the key.
     #[cfg(feature = "alloc")]
-    pub fn set_comment(&mut self, comment: impl Into<String>) {
+    pub fn set_comment(&mut self, comment: impl Into<Vec<u8>>) {
         self.comment = comment.into();
     }
 
@@ -290,7 +330,7 @@ impl PublicKey {
     /// Decode comment (e.g. email address)
     #[cfg(feature = "alloc")]
     pub(crate) fn decode_comment(&mut self, reader: &mut impl Reader) -> Result<()> {
-        self.comment = String::decode(reader)?;
+        self.comment = Vec::decode(reader)?;
         Ok(())
     }
 }
@@ -300,7 +340,7 @@ impl From<KeyData> for PublicKey {
         PublicKey {
             key_data,
             #[cfg(feature = "alloc")]
-            comment: String::new(),
+            comment: Vec::new(),
         }
     }
 }

ssh-key/tests/authorized_keys.rs

@@ -15,16 +15,16 @@ fn read_example_file() {
         authorized_keys[0].public_key().to_string(),
         "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILM+rvN+ot98qgEN796jTiQfZfG1KaT0PtFDJ/XFSqti"
     );
-    assert_eq!(authorized_keys[0].public_key().comment(), "");
+    assert_eq!(authorized_keys[0].public_key().comment_bytes(), b"");
 
     assert_eq!(
         authorized_keys[1].config_opts().to_string(),
         "command=\"/usr/bin/date\""
     );
     assert_eq!(authorized_keys[1].public_key().to_string(), "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBHwf2HMM5TRXvo2SQJjsNkiDD5KqiiNjrGVv3UUh+mMT5RHxiRtOnlqvjhQtBq0VpmpCV/PwUdhOig4vkbqAcEc= user2@example.com");
     assert_eq!(
-        authorized_keys[1].public_key().comment(),
-        "user2@example.com"
+        authorized_keys[1].public_key().comment_bytes(),
+        b"user2@example.com"
     );
 
     assert_eq!(
@@ -33,8 +33,8 @@ fn read_example_file() {
     );
     assert_eq!(authorized_keys[2].public_key().to_string(), "ssh-dss AAAAB3NzaC1kc3MAAACBANw9iSUO2UYhFMssjUgW46URqv8bBrDgHeF8HLBOWBvKuXF2Rx2J/XyhgX48SOLMuv0hcPaejlyLarabnF9F2V4dkpPpZSJ+7luHmxEjNxwhsdtg8UteXAWkeCzrQ6MvRJZHcDBjYh56KGvslbFnJsGLXlI4PQCyl6awNImwYGilAAAAFQCJGBU3hZf+QtP9Jh/nbfNlhFu7hwAAAIBHObOQioQVRm3HsVb7mOy3FVKhcLoLO3qoG9gTkd4KeuehtFAC3+rckiX7xSCnE/5BBKdL7VP9WRXac2Nlr9Pwl3e7zPut96wrCHt/TZX6vkfXKkbpUIj5zSqfvyNrWKaYJkfzwAQwrXNS1Hol676Ud/DDEn2oatdEhkS3beWHXAAAAIBgQqaz/YYTRMshzMzYcZ4lqgvgmA55y6v0h39e8HH2A5dwNS6sPUw2jyna+le0dceNRJifFld1J+WYM0vmquSr11DDavgEidOSaXwfMvPPPJqLmbzdtT16N+Gij9U9STQTHPQcQ3xnNNHgQAStzZJbhLOVbDDDo5BO7LMUALDfSA== user3@example.com");
     assert_eq!(
-        authorized_keys[2].public_key().comment(),
-        "user3@example.com"
+        authorized_keys[2].public_key().comment_bytes(),
+        b"user3@example.com"
     );
 
     assert_eq!(
@@ -43,13 +43,13 @@ fn read_example_file() {
     );
     assert_eq!(authorized_keys[3].public_key().to_string(), "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQC0WRHtxuxefSJhpIxGq4ibGFgwYnESPm8C3JFM88A1JJLoprenklrd7VJ+VH3Ov/bQwZwLyRU5dRmfR/SWTtIPWs7tToJVayKKDB+/qoXmM5ui/0CU2U4rCdQ6PdaCJdC7yFgpPL8WexjWN06+eSIKYz1AAXbx9rRv1iasslK/KUqtsqzVliagI6jl7FPO2GhRZMcso6LsZGgSxuYf/Lp0D/FcBU8GkeOo1Sx5xEt8H8bJcErtCe4Blb8JxcW6EXO3sReb4z+zcR07gumPgFITZ6hDA8sSNuvo/AlWg0IKTeZSwHHVknWdQqDJ0uczE837caBxyTZllDNIGkBjCIIOFzuTT76HfYc/7CTTGk07uaNkUFXKN79xDiFOX8JQ1ZZMZvGOTwWjuT9CqgdTvQRORbRWwOYv3MH8re9ykw3Ip6lrPifY7s6hOaAKry/nkGPMt40m1TdiW98MTIpooE7W+WXu96ax2l2OJvxX8QR7l+LFlKnkIEEJd/ItF1G22UmOjkVwNASTwza/hlY+8DoVvEmwum/nMgH2TwQT3bTQzF9s9DOJkH4d8p4Mw4gEDjNx0EgUFA91ysCAeUMQQyIvuR8HXXa+VcvhOOO5mmBcVhxJ3qUOJTyDBsT0932Zb4mNtkxdigoVxu+iiwk0vwtvKwGVDYdyMP5EAQeEIP1t0w== user4@example.com");
     assert_eq!(
-        authorized_keys[3].public_key().comment(),
-        "user4@example.com"
+        authorized_keys[3].public_key().comment_bytes(),
+        b"user4@example.com"
     );
 
     assert_eq!(authorized_keys[4].public_key().to_string(), "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBN76zuqnjypL54/w4763l7q1Sn3IBYHptJ5wcYfEWkzeNTvpexr05Z18m2yPT2SWRd1JJ8Aj5TYidG9MdSS5J78= hello world this is a long comment");
     assert_eq!(
-        authorized_keys[4].public_key().comment(),
-        "hello world this is a long comment"
+        authorized_keys[4].public_key().comment_bytes(),
+        b"hello world this is a long comment"
     );
 }

ssh-key/tests/dot_ssh.rs

@@ -20,7 +20,7 @@ fn path_round_trip() {
 #[test]
 fn private_keys() {
     let dot_ssh = dot_ssh();
-    assert_eq!(dot_ssh.private_keys().unwrap().count(), 21);
+    assert_eq!(dot_ssh.private_keys().unwrap().count(), 22);
 }
 
 #[test]

ssh-key/tests/examples/non_utf8_comment

@@ -0,0 +1,7 @@
+-----BEGIN OPENSSH PRIVATE KEY-----
+b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW
+QyNTUxOQAAACD2TvUAi4NJBWXBvDkzbLiIjgjeMzhTz9cUleJn2zRWKAAAAJi1GedGtRnn
+RgAAAAtzc2gtZWQyNTUxOQAAACD2TvUAi4NJBWXBvDkzbLiIjgjeMzhTz9cUleJn2zRWKA
+AAAEDBi49uVXZzDhN8JohiYkBdezFWbCAw6iCS2JRA4J0ujfZO9QCLg0kFZcG8OTNsuIiO
+CN4zOFPP1xSV4mfbNFYoAAAAFHN0YXJfQLLcyPHI8cjxtcS158TUAQ==
+-----END OPENSSH PRIVATE KEY-----

ssh-key/tests/known_hosts.rs

@@ -19,7 +19,7 @@ fn read_example_file() {
         known_hosts[0].public_key().to_string(),
         "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILM+rvN+ot98qgEN796jTiQfZfG1KaT0PtFDJ/XFSqti"
     );
-    assert_eq!(known_hosts[0].public_key().comment(), "");
+    assert_eq!(known_hosts[0].public_key().comment_bytes(), b"");
 
     assert_eq!(known_hosts[1].marker(), None);
     assert_eq!(
@@ -31,7 +31,7 @@ fn read_example_file() {
         ])
     );
     assert_eq!(known_hosts[1].public_key().to_string(), "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBHwf2HMM5TRXvo2SQJjsNkiDD5KqiiNjrGVv3UUh+mMT5RHxiRtOnlqvjhQtBq0VpmpCV/PwUdhOig4vkbqAcEc= example.com");
-    assert_eq!(known_hosts[1].public_key().comment(), "example.com");
+    assert_eq!(known_hosts[1].public_key().comment_bytes(), b"example.com");
 
     assert_eq!(known_hosts[2].marker(), Some(&Marker::Revoked));
     assert_eq!(
@@ -48,7 +48,7 @@ fn read_example_file() {
         }
     );
     assert_eq!(known_hosts[2].public_key().to_string(), "ssh-dss AAAAB3NzaC1kc3MAAACBANw9iSUO2UYhFMssjUgW46URqv8bBrDgHeF8HLBOWBvKuXF2Rx2J/XyhgX48SOLMuv0hcPaejlyLarabnF9F2V4dkpPpZSJ+7luHmxEjNxwhsdtg8UteXAWkeCzrQ6MvRJZHcDBjYh56KGvslbFnJsGLXlI4PQCyl6awNImwYGilAAAAFQCJGBU3hZf+QtP9Jh/nbfNlhFu7hwAAAIBHObOQioQVRm3HsVb7mOy3FVKhcLoLO3qoG9gTkd4KeuehtFAC3+rckiX7xSCnE/5BBKdL7VP9WRXac2Nlr9Pwl3e7zPut96wrCHt/TZX6vkfXKkbpUIj5zSqfvyNrWKaYJkfzwAQwrXNS1Hol676Ud/DDEn2oatdEhkS3beWHXAAAAIBgQqaz/YYTRMshzMzYcZ4lqgvgmA55y6v0h39e8HH2A5dwNS6sPUw2jyna+le0dceNRJifFld1J+WYM0vmquSr11DDavgEidOSaXwfMvPPPJqLmbzdtT16N+Gij9U9STQTHPQcQ3xnNNHgQAStzZJbhLOVbDDDo5BO7LMUALDfSA==");
-    assert_eq!(known_hosts[2].public_key().comment(), "");
+    assert_eq!(known_hosts[2].public_key().comment_bytes(), b"");
 
     assert_eq!(known_hosts[3].marker(), Some(&Marker::CertAuthority));
     assert_eq!(
@@ -57,7 +57,7 @@ fn read_example_file() {
     );
     assert_eq!(known_hosts[3].public_key().to_string(), "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQC0WRHtxuxefSJhpIxGq4ibGFgwYnESPm8C3JFM88A1JJLoprenklrd7VJ+VH3Ov/bQwZwLyRU5dRmfR/SWTtIPWs7tToJVayKKDB+/qoXmM5ui/0CU2U4rCdQ6PdaCJdC7yFgpPL8WexjWN06+eSIKYz1AAXbx9rRv1iasslK/KUqtsqzVliagI6jl7FPO2GhRZMcso6LsZGgSxuYf/Lp0D/FcBU8GkeOo1Sx5xEt8H8bJcErtCe4Blb8JxcW6EXO3sReb4z+zcR07gumPgFITZ6hDA8sSNuvo/AlWg0IKTeZSwHHVknWdQqDJ0uczE837caBxyTZllDNIGkBjCIIOFzuTT76HfYc/7CTTGk07uaNkUFXKN79xDiFOX8JQ1ZZMZvGOTwWjuT9CqgdTvQRORbRWwOYv3MH8re9ykw3Ip6lrPifY7s6hOaAKry/nkGPMt40m1TdiW98MTIpooE7W+WXu96ax2l2OJvxX8QR7l+LFlKnkIEEJd/ItF1G22UmOjkVwNASTwza/hlY+8DoVvEmwum/nMgH2TwQT3bTQzF9s9DOJkH4d8p4Mw4gEDjNx0EgUFA91ysCAeUMQQyIvuR8HXXa+VcvhOOO5mmBcVhxJ3qUOJTyDBsT0932Zb4mNtkxdigoVxu+iiwk0vwtvKwGVDYdyMP5EAQeEIP1t0w== authority@example.com");
     assert_eq!(
-        known_hosts[3].public_key().comment(),
-        "authority@example.com"
+        known_hosts[3].public_key().comment_bytes(),
+        b"authority@example.com"
     );
 }