ssh-key: make PublicKey/PrivateKey fields private

ssh-key/README.md

@@ -17,9 +17,9 @@ and `authorized_keys` files.
 
 ## Features
 
-- [x] Constant-time Base64 decoding using the `base64ct` crate
+- [x] Constant-time Base64 decoding/encoding using the `base64ct` crate
 - [x] `no_std` support including support for "heapless" (no-`alloc`) targets
-- [x] Parsing OpenSSH-formatted public and private keys with the following algorithms:
+- [x] Decoding/encoding OpenSSH-formatted public & private keys:
   - [x] DSA (`no_std` + `alloc`)
   - [x] ECDSA (`no_std` "heapless")
   - [x] Ed25519 (`no_std` "heapless")
@@ -29,7 +29,6 @@ and `authorized_keys` files.
 
 #### TODO:
 
-- [ ] Encoder support (currently decode-only)
 - [ ] Encrypted private key support
 - [ ] Legacy SSH key (pre-OpenSSH) format support
 - [ ] Integrations with other RustCrypto crates (e.g. `ecdsa`, `ed25519`, `rsa`)

ssh-key/src/base64.rs

@@ -178,6 +178,21 @@ pub(crate) trait DecoderExt {
     fn decode_string(&mut self) -> Result<String> {
         String::from_utf8(self.decode_byte_vec()?).map_err(|_| Error::CharacterEncoding)
     }
+
+    /// Drain the given number of bytes from the decoder, discarding them.
+    fn drain(&mut self, n_bytes: usize) -> Result<()> {
+        let mut byte = [0];
+        for _ in 0..n_bytes {
+            self.decode_base64(&mut byte)?;
+        }
+        Ok(())
+    }
+
+    /// Decode a `u32` length prefix, and then drain the length of the body.
+    fn drain_prefixed(&mut self) -> Result<()> {
+        let n_bytes = self.decode_usize()?;
+        self.drain(n_bytes)
+    }
 }
 
 impl DecoderExt for Decoder<'_> {

ssh-key/src/lib.rs

@@ -21,21 +21,20 @@
 //!
 //! #### Example
 //!
-//! ```
+#![cfg_attr(feature = "std", doc = "```")]
+#![cfg_attr(not(feature = "std"), doc = "```ignore")]
 //! # fn main() -> Result<(), Box<dyn std::error::Error>> {
-//! # #[cfg(feature = "std")]
-//! # {
 //! use ssh_key::PublicKey;
 //!
 //! let encoded_key = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAILM+rvN+ot98qgEN796jTiQfZfG1KaT0PtFDJ/XFSqti user@example.com";
 //! let public_key = PublicKey::from_openssh(encoded_key)?;
 //!
 //! // Key attributes
 //! assert_eq!(public_key.algorithm(), ssh_key::Algorithm::Ed25519);
-//! assert_eq!(public_key.comment, "user@example.com");
+//! assert_eq!(public_key.comment(), "user@example.com");
 //!
 //! // Key data: in this example an Ed25519 key
-//! if let Some(ed25519_public_key) = public_key.key_data.ed25519() {
+//! if let Some(ed25519_public_key) = public_key.key_data().ed25519() {
 //!     assert_eq!(
 //!         ed25519_public_key.as_ref(),
 //!         [
@@ -45,7 +44,6 @@
 //!         ].as_ref()
 //!     );
 //! }
-//! # }
 //! # Ok(())
 //! # }
 //! ```
@@ -60,10 +58,9 @@
 //!
 //! #### Example
 //!
-//! ```
+#![cfg_attr(feature = "std", doc = " ```")]
+#![cfg_attr(not(feature = "std"), doc = " ```ignore")]
 //! # fn main() -> Result<(), Box<dyn std::error::Error>> {
-//! # #[cfg(feature = "std")]
-//! # {
 //! use ssh_key::PrivateKey;
 //!
 //! // WARNING: don't actually hardcode private keys in source code!!!
@@ -81,10 +78,10 @@
 //!
 //! // Key attributes
 //! assert_eq!(private_key.algorithm(), ssh_key::Algorithm::Ed25519);
-//! assert_eq!(private_key.comment, "user@example.com");
+//! assert_eq!(private_key.comment(), "user@example.com");
 //!
 //! // Key data: in this example an Ed25519 key
-//! if let Some(ed25519_keypair) = private_key.key_data.ed25519() {
+//! if let Some(ed25519_keypair) = private_key.key_data().ed25519() {
 //!     assert_eq!(
 //!         ed25519_keypair.public.as_ref(),
 //!         [
@@ -103,7 +100,6 @@
 //!         ].as_ref()
 //!     )
 //! }
-//! # }
 //! # Ok(())
 //! # }
 //! ```

ssh-key/src/private.rs

@@ -57,26 +57,37 @@ const UNIX_FILE_PERMISSIONS: u32 = 0o600;
 #[derive(Clone, Debug)]
 pub struct PrivateKey {
     /// Cipher algorithm (a.k.a. `ciphername`).
-    pub cipher_alg: CipherAlg,
+    cipher_alg: CipherAlg,
 
     /// KDF algorithm.
-    pub kdf_alg: KdfAlg,
+    kdf_alg: KdfAlg,
 
     /// KDF options.
-    pub kdf_options: KdfOptions,
+    kdf_options: KdfOptions,
 
     /// Key data.
-    pub key_data: KeypairData,
+    key_data: KeypairData,
 
     /// Comment on the key (e.g. email address).
     #[cfg(feature = "alloc")]
     #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
-    pub comment: String,
+    comment: String,
 }
 
 impl PrivateKey {
     /// Magic string used to identify keys in this format.
-    pub const AUTH_MAGIC: &'static [u8] = b"openssh-key-v1\0";
+    const AUTH_MAGIC: &'static [u8] = b"openssh-key-v1\0";
+
+    /// Create a new unencrypted private key with the given keypair data and comment.
+    ///
+    /// On `no_std` platforms, use `PrivateKey::from(key_data)` instead.
+    #[cfg(feature = "alloc")]
+    #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
+    pub fn new(key_data: KeypairData, comment: impl Into<String>) -> Self {
+        let mut private_key = Self::from(key_data);
+        private_key.comment = comment.into();
+        private_key
+    }
 
     /// Parse an OpenSSH-formatted PEM private key.
     ///
@@ -128,13 +139,7 @@ impl PrivateKey {
         let key_data = KeypairData::decode(&mut pem_decoder)?;
 
         #[cfg(not(feature = "alloc"))]
-        {
-            let len = pem_decoder.decode_usize()?;
-            for _ in 0..len {
-                let mut byte = [0];
-                pem_decoder.decode(&mut byte)?;
-            }
-        }
+        pem_decoder.drain_prefixed()?;
         #[cfg(feature = "alloc")]
         let comment = pem_decoder.decode_string()?;
 
@@ -191,23 +196,15 @@ impl PrivateKey {
         pem_encoder.encode_usize(public_key_data.encoded_len()?)?;
         public_key_data.encode(&mut pem_encoder)?;
 
-        // Get private key comment
-        // TODO(tarcieri): comment accessor method with consistent behavior
-        #[cfg(not(feature = "alloc"))]
-        let comment = "";
-        #[cfg(feature = "alloc")]
-        let comment = &self.comment;
-
         // Encode private key
         let padding_len = self.padding_len()?;
         debug_assert!(padding_len <= 7, "padding too long: {}", padding_len);
-
         pem_encoder.encode_usize(self.private_key_len()? + padding_len)?;
         let checkint = public_key_data.checkint();
         pem_encoder.encode_u32(checkint)?;
         pem_encoder.encode_u32(checkint)?;
         self.key_data.encode(&mut pem_encoder)?;
-        pem_encoder.encode_str(comment)?;
+        pem_encoder.encode_str(self.comment())?;
         pem_encoder.encode_base64(&PADDING_BYTES[..padding_len])?;
 
         let encoded_len = pem_encoder.finish()?;
@@ -260,6 +257,38 @@ impl PrivateKey {
         self.key_data.algorithm()
     }
 
+    /// Comment on the key (e.g. email address).
+    #[cfg(not(feature = "alloc"))]
+    pub fn comment(&self) -> &str {
+        ""
+    }
+
+    /// Comment on the key (e.g. email address).
+    #[cfg(feature = "alloc")]
+    pub fn comment(&self) -> &str {
+        &self.comment
+    }
+
+    /// Cipher algorithm (a.k.a. `ciphername`).
+    pub fn cipher_alg(&self) -> CipherAlg {
+        self.cipher_alg
+    }
+
+    /// KDF algorithm.
+    pub fn kdf_alg(&self) -> KdfAlg {
+        self.kdf_alg
+    }
+
+    /// KDF options.
+    pub fn kdf_options(&self) -> &KdfOptions {
+        &self.kdf_options
+    }
+
+    /// Keypair data.
+    pub fn key_data(&self) -> &KeypairData {
+        &self.key_data
+    }
+
     /// Get the [`PublicKey`] which corresponds to this private key.
     pub fn public_key(&self) -> PublicKey {
         PublicKey {
@@ -295,16 +324,10 @@ impl PrivateKey {
 
     /// Get the length of the private key data in bytes (not including padding).
     fn private_key_len(&self) -> Result<usize> {
-        // TODO(tarcieri): comment accessor method with consistent behavior
-        #[cfg(not(feature = "alloc"))]
-        let comment_len = 0;
-        #[cfg(feature = "alloc")]
-        let comment_len = self.comment.len();
-
         Ok(8 // 2 * checkints
-            + self.key_data.encoded_len()?
+            + self.key_data().encoded_len()?
             + 4 // comment length prefix
-            + comment_len)
+            + self.comment().len())
     }
 
     /// Get the number of padding bytes to add to this key (without padding).
@@ -325,6 +348,19 @@ impl PrivateKey {
     }
 }
 
+impl From<KeypairData> for PrivateKey {
+    fn from(key_data: KeypairData) -> PrivateKey {
+        PrivateKey {
+            cipher_alg: CipherAlg::None,
+            kdf_alg: KdfAlg::None,
+            kdf_options: KdfOptions::default(),
+            key_data,
+            #[cfg(feature = "alloc")]
+            comment: String::new(),
+        }
+    }
+}
+
 impl From<PrivateKey> for PublicKey {
     fn from(private_key: PrivateKey) -> PublicKey {
         private_key.public_key()
@@ -353,13 +389,14 @@ impl str::FromStr for PrivateKey {
 #[cfg_attr(docsrs, doc(cfg(feature = "subtle")))]
 impl ConstantTimeEq for PrivateKey {
     fn ct_eq(&self, other: &Self) -> Choice {
-        // TODO(tarcieri): comment accessor method with consistent behavior
-        #[cfg(not(feature = "alloc"))]
-        let comment_eq = Choice::from(1);
-        #[cfg(feature = "alloc")]
-        let comment_eq = self.comment.as_bytes().ct_eq(other.comment.as_bytes());
-
-        comment_eq & self.key_data.ct_eq(&other.key_data)
+        // Constant-time with respect to key data and comment
+        self.key_data.ct_eq(&other.key_data)
+            & self.comment.as_bytes().ct_eq(other.comment.as_bytes())
+            & Choice::from(
+                (self.cipher_alg == other.cipher_alg
+                    && self.kdf_alg == other.kdf_alg
+                    && self.kdf_options == other.kdf_options) as u8,
+            )
     }
 }
 

ssh-key/src/public.rs

@@ -33,15 +33,26 @@ use alloc::{
 #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
 pub struct PublicKey {
     /// Key data.
-    pub key_data: KeyData,
+    pub(crate) key_data: KeyData,
 
     /// Comment on the key (e.g. email address)
     #[cfg(feature = "alloc")]
-    #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
-    pub comment: String,
+    pub(crate) comment: String,
 }
 
 impl PublicKey {
+    /// Create a new public key with the given comment.
+    ///
+    /// On `no_std` platforms, use `PublicKey::from(key_data)` instead.
+    #[cfg(feature = "alloc")]
+    #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
+    pub fn new(key_data: KeyData, comment: impl Into<String>) -> Self {
+        Self {
+            key_data,
+            comment: comment.into(),
+        }
+    }
+
     /// Parse an OpenSSH-formatted public key.
     ///
     /// OpenSSH-formatted public keys look like the following:
@@ -72,19 +83,15 @@ impl PublicKey {
 
     /// Encode OpenSSH-formatted (PEM) public key.
     pub fn encode_openssh<'o>(&self, out: &'o mut [u8]) -> Result<&'o str> {
-        #[cfg(not(feature = "alloc"))]
-        let comment = "";
-        #[cfg(feature = "alloc")]
-        let comment = &self.comment;
-
-        openssh::Encapsulation::encode(out, self.algorithm().as_str(), comment, |encoder| {
+        openssh::Encapsulation::encode(out, self.algorithm().as_str(), self.comment(), |encoder| {
             self.key_data.encode(encoder)
         })
     }
 
     /// Encode an OpenSSH-formatted public key, allocating a [`String`] for
     /// the result.
     #[cfg(feature = "alloc")]
+    #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))]
     pub fn to_openssh(&self) -> Result<String> {
         let alg_len = self.algorithm().as_str().len();
         let key_data_len = base64::encoded_len(self.key_data.encoded_len()?);
@@ -101,6 +108,33 @@ impl PublicKey {
     pub fn algorithm(&self) -> Algorithm {
         self.key_data.algorithm()
     }
+
+    /// Comment on the key (e.g. email address).
+    #[cfg(not(feature = "alloc"))]
+    pub fn comment(&self) -> &str {
+        ""
+    }
+
+    /// Comment on the key (e.g. email address).
+    #[cfg(feature = "alloc")]
+    pub fn comment(&self) -> &str {
+        &self.comment
+    }
+
+    /// Private key data.
+    pub fn key_data(&self) -> &KeyData {
+        &self.key_data
+    }
+}
+
+impl From<KeyData> for PublicKey {
+    fn from(key_data: KeyData) -> PublicKey {
+        PublicKey {
+            key_data,
+            #[cfg(feature = "alloc")]
+            comment: String::new(),
+        }
+    }
 }
 
 impl FromStr for PublicKey {