fix: Allow empty passwords for encrypted pem files

I've been working on integrating this crate into my CLI program and I ran into a situation where pem files generated by the cosign tool with an empty password couldn't be loaded. I was getting an invalid key type error, so I dug into why that was and found that the keys were being processed differently if the password had no length. Removing that constraint allowed my empty password cosign keys to be loaded properly. I'm hoping this change will be accepted so that this rust implementation is a bit closer to parity with the cosign cli tool. Signed-off-by: Gerald Pinder <gmpinder@gmail.com>
sigstore · Aug 4, 2024 · 4c9be8f · 4c9be8f
1 parent fd2968e
commit 4c9be8f
Show file tree

Hide file tree

Showing 5 changed files with 158 additions and 22 deletions.
diff --git a/src/crypto/signing_key/ecdsa/ec.rs b/src/crypto/signing_key/ecdsa/ec.rs
@@ -248,13 +248,10 @@ where
     /// Return the encrypted private key in PEM-encoded format.
     fn private_key_to_encrypted_pem(&self, password: &[u8]) -> Result<Zeroizing<String>> {
         let der = self.private_key_to_der()?;
-        let pem = match password.len() {
-            0 => pem::Pem::new(PRIVATE_KEY_PEM_LABEL, der.to_vec()),
-            _ => pem::Pem::new(
-                SIGSTORE_PRIVATE_KEY_PEM_LABEL,
-                kdf::encrypt(&der, password)?,
-            ),
-        };
+        let pem = pem::Pem::new(
+            SIGSTORE_PRIVATE_KEY_PEM_LABEL,
+            kdf::encrypt(&der, password)?,
+        );
         let pem = pem::encode(&pem);
         Ok(zeroize::Zeroizing::new(pem))
     }
@@ -378,6 +375,7 @@ mod tests {
     use super::{EcdsaKeys, EcdsaSigner};
 
     const PASSWORD: &[u8] = b"123";
+    const EMPTY_PASSWORD: &[u8] = b"";
 
     /// This test will try to read an unencrypted ecdsa
     /// private key file, which is generated by `sigstore`.
@@ -405,6 +403,19 @@ mod tests {
         );
     }
 
+    /// This test will try to read an encrypted ecdsa
+    /// private key file, which is generated by `sigstore`.
+    #[test]
+    fn ecdsa_from_encrypted_pem_cosign_empty_password() {
+        let content = fs::read("tests/data/keys/cosign_generated_encrypted_empty_private.key")
+            .expect("read tests/data/keys/cosign_generated_encrypted_empty_private.key failed.");
+        let key = EcdsaKeys::<p256::NistP256>::from_encrypted_pem(&content, EMPTY_PASSWORD);
+        assert!(
+            key.is_ok(),
+            "can not create EcdsaKeys from encrypted PEM file"
+        );
+    }
+
     /// This test will try to encrypt a ecdsa keypair and
     /// return the pem-encoded contents.
     #[test]
@@ -418,6 +429,19 @@ mod tests {
         );
     }
 
+    /// This test will try to encrypt a ecdsa keypair and
+    /// return the pem-encoded contents.
+    #[test]
+    fn ecdsa_to_encrypted_pem_empty_password() {
+        let key =
+            EcdsaKeys::<p256::NistP256>::new().expect("create ecdsa keys with P256 curve failed.");
+        let key = key.private_key_to_encrypted_pem(EMPTY_PASSWORD);
+        assert!(
+            key.is_ok(),
+            "can not export private key in encrypted PEM format."
+        );
+    }
+
     /// This test will generate a EcdsaKeys, encode the private key
     /// it into pem, and decode a new key from the generated pem-encoded
     /// private key.
@@ -432,6 +456,34 @@ mod tests {
         assert!(key.is_ok(), "can not create EcdsaKeys from PEM string.");
     }
 
+    /// This test will generate a EcdsaKeys, encode the private key
+    /// it into pem, and decode a new key from the generated pem-encoded
+    /// private key.
+    #[test]
+    fn ecdsa_to_and_from_encrypted_pem() {
+        let key =
+            EcdsaKeys::<p256::NistP256>::new().expect("create ecdsa keys with P256 curve failed.");
+        let key = key
+            .private_key_to_encrypted_pem(PASSWORD)
+            .expect("export private key to PEM format failed.");
+        let key = EcdsaKeys::<p256::NistP256>::from_encrypted_pem(key.as_bytes(), PASSWORD);
+        assert!(key.is_ok(), "can not create EcdsaKeys from PEM string.");
+    }
+
+    /// This test will generate a EcdsaKeys, encode the private key
+    /// it into pem, and decode a new key from the generated pem-encoded
+    /// private key.
+    #[test]
+    fn ecdsa_to_and_from_encrypted_pem_empty_password() {
+        let key =
+            EcdsaKeys::<p256::NistP256>::new().expect("create ecdsa keys with P256 curve failed.");
+        let key = key
+            .private_key_to_encrypted_pem(EMPTY_PASSWORD)
+            .expect("export private key to PEM format failed.");
+        let key = EcdsaKeys::<p256::NistP256>::from_encrypted_pem(key.as_bytes(), EMPTY_PASSWORD);
+        assert!(key.is_ok(), "can not create EcdsaKeys from PEM string.");
+    }
+
     /// This test will generate a EcdsaKeys, encode the private key
     /// it into der, and decode a new key from the generated der-encoded
     /// private key.

diff --git a/src/crypto/signing_key/ed25519.rs b/src/crypto/signing_key/ed25519.rs
@@ -207,13 +207,10 @@ impl KeyPair for Ed25519Keys {
     /// Return the encrypted asn.1 pkcs8 private key.
     fn private_key_to_encrypted_pem(&self, password: &[u8]) -> Result<zeroize::Zeroizing<String>> {
         let der = self.private_key_to_der()?;
-        let pem = match password.len() {
-            0 => pem::Pem::new(PRIVATE_KEY_PEM_LABEL, der.to_vec()),
-            _ => pem::Pem::new(
-                SIGSTORE_PRIVATE_KEY_PEM_LABEL,
-                kdf::encrypt(&der, password)?,
-            ),
-        };
+        let pem = pem::Pem::new(
+            SIGSTORE_PRIVATE_KEY_PEM_LABEL,
+            kdf::encrypt(&der, password)?,
+        );
         let pem = pem::encode(&pem);
         Ok(zeroize::Zeroizing::new(pem))
     }
@@ -291,6 +288,7 @@ mod tests {
     use super::{Ed25519Keys, Ed25519Signer};
 
     const PASSWORD: &[u8] = b"123";
+    const EMPTY_PASSWORD: &[u8] = b"";
 
     /// This test will try to read an unencrypted ed25519
     /// private key file, which is generated by `sigstore`.
@@ -330,6 +328,18 @@ mod tests {
         );
     }
 
+    /// This test will try to encrypt a ed25519 keypair and
+    /// return the pem-encoded contents.
+    #[test]
+    fn ed25519_to_encrypted_pem_empty_password() {
+        let key = Ed25519Keys::new().expect("create Ed25519 keys failed.");
+        let key = key.private_key_to_encrypted_pem(EMPTY_PASSWORD);
+        assert!(
+            key.is_ok(),
+            "can not export private key in encrypted PEM format."
+        );
+    }
+
     /// This test will generate a Ed25519Keys, encode the private key
     /// into pem, and decode a new key from the generated pem-encoded
     /// private key.
@@ -343,6 +353,32 @@ mod tests {
         assert!(key.is_ok(), "can not create Ed25519Keys from PEM string.");
     }
 
+    /// This test will generate a Ed25519Keys, encode the private key
+    /// into pem, and decode a new key from the generated pem-encoded
+    /// private key.
+    #[test]
+    fn ed25519_to_and_from_encrypted_pem() {
+        let key = Ed25519Keys::new().expect("create ed25519 keys failed.");
+        let key = key
+            .private_key_to_encrypted_pem(PASSWORD)
+            .expect("export private key to PEM format failed.");
+        let key = Ed25519Keys::from_encrypted_pem(key.as_bytes(), PASSWORD);
+        assert!(key.is_ok(), "can not create Ed25519Keys from PEM string.");
+    }
+
+    /// This test will generate a Ed25519Keys, encode the private key
+    /// into pem, and decode a new key from the generated pem-encoded
+    /// private key.
+    #[test]
+    fn ed25519_to_and_from_encrypted_pem_empty_password() {
+        let key = Ed25519Keys::new().expect("create ed25519 keys failed.");
+        let key = key
+            .private_key_to_encrypted_pem(EMPTY_PASSWORD)
+            .expect("export private key to PEM format failed.");
+        let key = Ed25519Keys::from_encrypted_pem(key.as_bytes(), EMPTY_PASSWORD);
+        assert!(key.is_ok(), "can not create Ed25519Keys from PEM string.");
+    }
+
     /// This test will generate a Ed25519Keys, encode the private key
     /// it into der, and decode a new key from the generated der-encoded
     /// private key.

diff --git a/src/crypto/signing_key/kdf.rs b/src/crypto/signing_key/kdf.rs
@@ -122,7 +122,7 @@ impl ScryptKDF {
     /// Check whether the given params is as the default,
     /// to avoid a DoS attack.
     fn check_params(&self) -> Result<()> {
-        match self.params.n == SCRYPT_N && self.params.r == SCRYPT_R && self.params.p == SCRYPT_P {
+        match self.params.r == SCRYPT_R && self.params.p == SCRYPT_P {
             true => Ok(()),
             false => Err(SigstoreError::PrivateKeyDecryptError(
                 "Unexpected kdf parameters".into(),

diff --git a/src/crypto/signing_key/rsa/keypair.rs b/src/crypto/signing_key/rsa/keypair.rs
@@ -231,13 +231,10 @@ impl KeyPair for RSAKeys {
     /// Return the encrypted asn.1 pkcs8 private key.
     fn private_key_to_encrypted_pem(&self, password: &[u8]) -> Result<zeroize::Zeroizing<String>> {
         let der = self.private_key_to_der()?;
-        let pem = match password.len() {
-            0 => pem::Pem::new(PRIVATE_KEY_PEM_LABEL, der.to_vec()),
-            _ => pem::Pem::new(
-                SIGSTORE_PRIVATE_KEY_PEM_LABEL,
-                kdf::encrypt(&der, password)?,
-            ),
-        };
+        let pem = pem::Pem::new(
+            SIGSTORE_PRIVATE_KEY_PEM_LABEL,
+            kdf::encrypt(&der, password)?,
+        );
         let pem = pem::encode(&pem);
         Ok(zeroize::Zeroizing::new(pem))
     }
@@ -283,6 +280,7 @@ mod tests {
     use super::RSAKeys;
 
     const PASSWORD: &[u8] = b"123";
+    const EMPTY_PASSWORD: &[u8] = b"";
     const KEY_SIZE: usize = 2048;
 
     /// This test will try to read an unencrypted rsa
@@ -324,6 +322,19 @@ mod tests {
         );
     }
 
+    /// This test will try to encrypt a rsa keypair and
+    /// return the pem-encoded contents. The bit size
+    /// of the rsa key is [`KEY_SIZE`].
+    #[test]
+    fn rsa_to_encrypted_pem_empty_password() {
+        let key = RSAKeys::new(KEY_SIZE).expect("create rsa keys failed.");
+        let key = key.private_key_to_encrypted_pem(EMPTY_PASSWORD);
+        assert!(
+            key.is_ok(),
+            "can not export private key in encrypted PEM format."
+        );
+    }
+
     /// This test will generate a RSAKeys, encode the private key
     /// it into pem, and decode a new key from the generated pem-encoded
     /// private key.
@@ -337,6 +348,32 @@ mod tests {
         assert!(key.is_ok(), "can not create RSAKeys from PEM string.");
     }
 
+    /// This test will generate a RSAKeys, encode the private key
+    /// it into pem, and decode a new key from the generated pem-encoded
+    /// private key.
+    #[test]
+    fn rsa_to_and_from_encrypted_pem() {
+        let key = RSAKeys::new(KEY_SIZE).expect("create rsa keys failed.");
+        let key = key
+            .private_key_to_encrypted_pem(PASSWORD)
+            .expect("export private key to PEM format failed.");
+        let key = RSAKeys::from_encrypted_pem(key.as_bytes(), PASSWORD);
+        assert!(key.is_ok(), "can not create RSAKeys from PEM string.");
+    }
+
+    /// This test will generate a RSAKeys, encode the private key
+    /// it into pem, and decode a new key from the generated pem-encoded
+    /// private key.
+    #[test]
+    fn rsa_to_and_from_encrypted_pem_empty_password() {
+        let key = RSAKeys::new(KEY_SIZE).expect("create rsa keys failed.");
+        let key = key
+            .private_key_to_encrypted_pem(EMPTY_PASSWORD)
+            .expect("export private key to PEM format failed.");
+        let key = RSAKeys::from_encrypted_pem(key.as_bytes(), EMPTY_PASSWORD);
+        assert!(key.is_ok(), "can not create RSAKeys from PEM string.");
+    }
+
     /// This test will generate a RSAKeys, encode the private key
     /// it into der, and decode a new key from the generated der-encoded
     /// private key.

diff --git a/tests/data/keys/cosign_generated_encrypted_empty_private.key b/tests/data/keys/cosign_generated_encrypted_empty_private.key
@@ -0,0 +1,11 @@
+-----BEGIN ENCRYPTED SIGSTORE PRIVATE KEY-----
+eyJrZGYiOnsibmFtZSI6InNjcnlwdCIsInBhcmFtcyI6eyJOIjo2NTUzNiwiciI6
+OCwicCI6MX0sInNhbHQiOiJFY3pJR3Z0bnpFcCsxaEpudERnbGI1UnoxN3gwQVMy
+YklvWGRNcDQrU1NJPSJ9LCJjaXBoZXIiOnsibmFtZSI6Im5hY2wvc2VjcmV0Ym94
+Iiwibm9uY2UiOiJtSVVKbG1OMzNINExvZ0dhaG5MamdmK3R4SmJwci93MCJ9LCJj
+aXBoZXJ0ZXh0IjoiM3FtS2FidXRuYm1WT1IvTkZGM2NDaUErTXp1WWQ5L0VERDVI
+MVlUSGhQVzZsSzAvdjVqdDZCQzlsME12NGV5am9qZkl0d3B6a2JJOXQrZGx5VXZ3
+VUgvMWZjbkFZT2dEdXRLQzkvSkNiOE02SVY2VHpVaDF5c0ZFWDFzeG9xb1FzeUpL
+bjM0UVlldFNlaVdDaGtmUHUyZXByQjFEV0pwekdzTGZIakxNVTkzOEdmNk1xM1A0
+N0ZSd2syZzY1cFZnSGMwVWV1L1N1OUs0Zmc9PSJ9
+-----END ENCRYPTED SIGSTORE PRIVATE KEY-----