Allow using password-protected private keys

[fghanmi]

PR Type

Enhancement

---

Description

• Add password support for encrypted private keys

• Update key parsing to handle password-protected keys

• Add password validation and error handling

• Update CLI commands to accept password arguments

---

Diagram Walkthrough

flowchart LR
  A["CLI Commands"] -- "password args" --> B["Key Source"]
  B -- "encrypted key + password" --> C["Key Parser"]
  C -- "decrypt_key()" --> D["Decrypted Key"]
  D --> E["Sign Operations"]

Loading

File Walkthrough

	Relevant files
Enhancement	15 files lib.rs Update SSM key parsing with password parameter                      +1/-1      key_source.rs Add password field to LocalKeySource struct                            +6/-2      sign.rs Implement key decryption and password parsing                        +36/-6    add_key_role.rs Add password arguments for key operations                                +31/-4    add_role.rs Add password support for role creation                                      +26/-4    create.rs Add password arguments to create command                                  +15/-2    create_role.rs Add password support for role creation                                      +16/-2    remove_key_role.rs Add password arguments for key removal                                      +16/-2    remove_role.rs Add password support for role removal                                        +15/-2    rhtas.rs Add password validation and encryption checks                        +44/-3    root.rs Add password support for root operations                                  +50/-9    source.rs Update key source parsing with password                                    +5/-2      transfer_metadata.rs Add password arguments for metadata transfer                          +15/-2    update.rs Add password support for update operations                              +15/-2    update_targets.rs Add password arguments for target updates                                +15/-2
Tests	5 files test.rs Add password field to LocalKeySource in tests                        +8/-2      repo_editor.rs Update LocalKeySource usage with password field                    +34/-7    target_path_safety.rs Add password field to LocalKeySource creation                        +1/-0      root_command.rs Add encrypted key tests and password handling                        +151/-47 snakeoil_3.pem Add encrypted test key file                                                            +30/-0
Error handling	2 files error.rs Add KeyDecryption error variant                                                    +3/-0      error.rs Add password-related error variants                                            +15/-0
Dependencies	1 files Cargo.toml Add pkcs8 and base64 dependencies                                                +2/-0
Documentation	1 files README.md Update documentation with password examples                            +7/-0

---

[sourcery-ai]

Reviewer's Guide

This PR extends key handling to support optional passwords for encrypted key files throughout the CLI and library, adding decryption logic, new error paths, CLI flags, and corresponding test updates.

Sequence diagram for key parsing with optional password

sequenceDiagram
    participant CLI
    participant LocalKeySource
    participant parse_key_source()
    participant parse_keypair()
    participant decrypt_key()

    CLI->>parse_key_source(): Provide key path and optional password
    parse_key_source->>LocalKeySource: Create LocalKeySource(path, password)
    LocalKeySource->>parse_keypair: Call with key bytes, password
    alt password provided and not empty
        parse_keypair->>decrypt_key: Decrypt key with password
        decrypt_key-->>parse_keypair: Return decrypted key
    else password not provided or empty
        parse_keypair: Use key bytes as-is
    end
    parse_keypair-->>LocalKeySource: Return SignKeyPair
    LocalKeySource-->>CLI: Return SignKeyPair

Loading

Class diagram for updated key handling with password support

classDiagram
    class LocalKeySource {
        +PathBuf path
        +Option<String> password
    }
    class KeySource {
        <<trait>>
        +async as_sign()
        +async write()
    }
    LocalKeySource --|> KeySource

    class Sign {
        <<trait>>
    }
    class SignKeyPair {
        +ED25519KeyPair
        +ECDSAKeyPair
        +RsaKeyPair
    }
    SignKeyPair --|> Sign

    class Error {
        +MorePasswords
        +MoreNewPasswords
        +EncryptedKeyNoPassword
        +InvalidPassword
        +KeyDecryption
    }

    class parse_keypair {
        +parse_keypair(key: &[u8], password: Option<&str>)
    }
    class decrypt_key {
        +decrypt_key(encrypted_key: &[u8], password: &str)
    }

    parse_keypair --> decrypt_key : uses for decryption
    LocalKeySource --> parse_keypair : uses for key parsing
    KeySource <|.. LocalKeySource : implemented by
    Error <.. parse_keypair : error paths

Loading

File-Level Changes

Change	Details	Files
Introduce optional password flags in CLI commands and propagate them through command handlers	Added --password option to root add-key, gen-rsa-key, sign, and other CLI subcommands Updated Command enum variants to include password fields and unwrap them in handlers Extended command implementations to pair each key_source with its matching password Adjusted RhtasArgs and other commands to validate password counts Updated README to document the new --password option	tuftool/src/root.rs tuftool/src/add_key_role.rs tuftool/src/add_role.rs tuftool/src/create_role.rs tuftool/src/remove_key_role.rs tuftool/src/remove_role.rs tuftool/src/transfer_metadata.rs tuftool/src/update.rs tuftool/src/update_targets.rs tuftool/src/create.rs tuftool/src/source.rs tuftool/src/rhtas.rs tuftool/README.md
Extend parse_key_source and LocalKeySource to carry passwords	Changed parse_key_source signature to accept an optional password Augmented LocalKeySource struct with a password field Updated KeySource trait implementation for LocalKeySource to pass password into parse_keypair Propagated None passwords in tough-ssm and test fixtures	tough/src/source.rs tough/src/key_source.rs tough-ssm/src/lib.rs tough/tests/repo_editor.rs tough/tests/target_path_safety.rs tough/src/editor/test.rs
Add decryption logic to parse and sign encrypted keys	Introduced decrypt_key helper using pkcs8 to decrypt PEM blocks Extended parse_keypair to accept an optional password and decrypt before parsing keypair Added check_key_encryption in RHTAS to validate encrypted key with provided passphrase Added new error variants for missing or invalid passwords Upgraded Cargo.toml with pkcs8 and base64 dependencies	tough/src/sign.rs tough/src/error.rs tough/Cargo.toml tuftool/src/rhtas.rs
Update tests to support encrypted key workflows	Modified root_command.rs tests to accept password argument on add-key and sign helpers Added new tests for creating and failing to use encrypted keys Updated LocalKeySource usage in tough tests to supply None password Added snakeoil_3.pem test fixture	tuftool/tests/root_command.rs tough/tests/repo_editor.rs tough/tests/target_path_safety.rs tough/tests/data/snakeoil_3.pem

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[fghanmi]

/review

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns Password handling: Multiple CLI paths now accept passwords and pass them around as owned Strings, which may remain in memory longer than necessary. Consider using more ephemeral handling (e.g., zeroize on drop) and avoiding cloning where possible. Encryption detection: The check_key_encryption function relies on substring matching "ENCRYPTED" and then uses OpenSSL to validate; this may miss other encrypted key formats or falsely detect. Prefer robust parsing (e.g., pkcs8::EncryptedPrivateKeyInfo) consistently. No other obvious injection or secret exposure in logs is visible, but ensure errors do not log passwords.

⚡ Recommended focus areas for review Possible Issue The new parse_keypair signature and call sites now accept an optional password, but error mapping for decryption uses a blanket catch-all and returns KeyDecryption without preserving context; also, the function constructs a PEM for decrypted PKCS8 bytes which may not be necessary and could break non-PEM PKCS8 inputs. Verify compatibility with existing unencrypted PKCS8 paths and that decryption failure surfaces actionable errors. /// Decrypts an RSA private key in PEM format using the given password. /// Returns the decrypted key in PKCS8 format pub fn decrypt_key( encrypted_key: &[u8], password: &str, ) -> std::result::Result<Vec<u8>, Box<dyn std::error::Error>> { let pem_str = std::str::from_utf8(encrypted_key)?; let pem = pem::parse(pem_str)?; let encrypted_private_key_document = pkcs8::EncryptedPrivateKeyInfo::from_der(pem.contents())?; let decrypted_private_key_document = encrypted_private_key_document.decrypt(password.as_bytes())?; let decrypted_key_bytes = decrypted_private_key_document.as_bytes(); let pem_output = pem::Pem::new("PRIVATE KEY".to_string(), decrypted_key_bytes.to_vec()); let pem_encoded = pem::encode(&pem_output); Ok(pem_encoded.as_bytes().to_vec()) } /// Parses a supplied keypair and if it is recognized, returns an object that /// implements the Sign trait /// Accepted Keys: ED25519 pkcs8, Ecdsa pkcs8, RSA pub fn parse_keypair(key: &[u8], password: Option<&str>) -> Result<impl Sign> { let decrypted_key = if let Some(pw) = password { if pw.is_empty() { key.to_vec() } else { decrypt_key(key, pw).map_err(|_| error::KeyDecryptionSnafu.build())? } } else { key.to_vec() }; let decrypted_key_slice: &[u8] = &decrypted_key; if let Ok(ed25519_key_pair) = Ed25519KeyPair::from_pkcs8(decrypted_key_slice) { Ok(SignKeyPair::ED25519(ed25519_key_pair)) } else if let Ok(ecdsa_key_pair) = EcdsaKeyPair::from_pkcs8( &aws_lc_rs::signature::ECDSA_P256_SHA256_ASN1_SIGNING, decrypted_key_slice, ) { Ok(SignKeyPair::ECDSA(ecdsa_key_pair)) } else if let Ok(pem) = pem::parse(decrypted_key_slice) { match pem.tag() { "PRIVATE KEY" => { if let Ok(rsa_key_pair) = RsaKeyPair::from_pkcs8(pem.contents()) { Ok(SignKeyPair::RSA(rsa_key_pair)) API Change Risk parse_key_source now requires a password parameter and returns LocalKeySource with that password; ensure all internal callers were updated. Any third-party callers or feature-gated SSM path must align with the new KeySource trait changes (added Any bound) and optional password handling. pub(crate) fn parse_key_source( input: &str, password: Option<String>, ) -> Result<Box<dyn KeySource>> { let path_or_url = parse_path_or_url(input)?; match path_or_url { PathOrUrl::Path(path) => Ok(Box::new(LocalKeySource { path, password })), PathOrUrl::Url(url) => { match url.scheme() { #[cfg(any(feature = "aws-sdk-rust", feature = "aws-sdk-rust-rustls"))] Security/Robustness check_key_encryption detects encryption by searching for the string "ENCRYPTED", which is brittle. Consider parsing PEM headers or attempting decryption via pkcs8 APIs consistently. Also, storing and passing passwords as plain strings may linger in memory; evaluate minimizing exposure or using zeroize if feasible. pub fn read_key_file(key: String) -> std::io::Result<String> { std::fs::read_to_string(key) } // Function to check if key is encrypted and validate password fn check_key_encryption(path: String, password: &str) -> Result<()> { let path_buf = PathBuf::from(&path); let key_content = read_key_file(path).context(error::FileReadSnafu { path: path_buf.clone(), })?; let is_encrypted = key_content.contains("ENCRYPTED"); if is_encrypted { if is_encrypted && password.is_empty() { return Err(error::EncryptedKeyNoPasswordSnafu { path: path_buf }.build()); } // Try parsing the encrypted key with the password PKey::private_key_from_pem_passphrase(key_content.as_bytes(), password.as_bytes()) .context(error::InvalidPasswordSnafu { path: path_buf })?; } Ok(()) }

📄 References No matching references available

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Weak encryption detection Description: The function check_key_encryption detects encryption by searching for the literal string "ENCRYPTED", which is brittle and may misclassify keys; relying on substring checks can allow bypass or false negatives for differently formatted encrypted PEMs. rhtas.rs [1149-1173] Referred Code pub fn read_key_file(key: String) -> std::io::Result<String> { std::fs::read_to_string(key) } // Function to check if key is encrypted and validate password fn check_key_encryption(path: String, password: &str) -> Result<()> { let path_buf = PathBuf::from(&path); let key_content = read_key_file(path).context(error::FileReadSnafu { path: path_buf.clone(), })?; let is_encrypted = key_content.contains("ENCRYPTED"); if is_encrypted { if is_encrypted && password.is_empty() { return Err(error::EncryptedKeyNoPasswordSnafu { path: path_buf }.build()); } // Try parsing the encrypted key with the password PKey::private_key_from_pem_passphrase(key_content.as_bytes(), password.as_bytes()) .context(error::InvalidPasswordSnafu { path: path_buf })?; ... (clipped 4 lines)
	Key material exposure Description: The decrypt_key function decodes PEM to DER and re-encodes the decrypted PKCS#8 as a PEM string stored in memory; decrypted private key material is held in heap buffers without zeroization, increasing risk of sensitive data lingering in memory. sign.rs [170-185] Referred Code /// Decrypts an RSA private key in PEM format using the given password. /// Returns the decrypted key in PKCS8 format pub fn decrypt_key( encrypted_key: &[u8], password: &str, ) -> std::result::Result<Vec<u8>, Box<dyn std::error::Error>> { let pem_str = std::str::from_utf8(encrypted_key)?; let pem = pem::parse(pem_str)?; let encrypted_private_key_document = pkcs8::EncryptedPrivateKeyInfo::from_der(pem.contents())?; let decrypted_private_key_document = encrypted_private_key_document.decrypt(password.as_bytes())?; let decrypted_key_bytes = decrypted_private_key_document.as_bytes(); let pem_output = pem::Pem::new("PRIVATE KEY".to_string(), decrypted_key_bytes.to_vec()); let pem_encoded = pem::encode(&pem_output); Ok(pem_encoded.as_bytes().to_vec()) }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
🟢	No codebase code duplication found New Components Detected: - decrypt_key - check_key_encryption - create_root_encrypted_key - create_root_encrypted_key_invalid_password
Custom Compliance
⚪	No custom compliance provided Follow the guide to enable custom compliance check.

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `tuftool/src/rhtas.rs:1161` </location>
<code_context>
+        path: path_buf.clone(),
+    })?;
+
+    let is_encrypted = key_content.contains("ENCRYPTED");
+
+    if is_encrypted {
</code_context>

<issue_to_address>
**issue:** Key encryption detection via substring may be unreliable.

Parsing the PEM header or using a library function would provide a more reliable way to detect encrypted keys than substring matching.
</issue_to_address>

### Comment 2
<location> `tuftool/tests/root_command.rs:476-485` </location>
<code_context>
+// Ensure we can create and sign a root file with a password encrypted key
</code_context>

<issue_to_address>
**suggestion (testing):** Missing test for multiple encrypted keys with different passwords.

Please add a test that covers signing with multiple encrypted keys, each using a distinct password, to verify correct password-key matching.

Suggested implementation:

```rust
#[test]
// Ensure we can create and sign a root file with a password encrypted key
fn create_root_encrypted_key() {
    let out_dir = TempDir::new().unwrap();
    let root_json = out_dir.path().join("root.json");
    let key = test_utils::test_data().join("snakeoil_3.pem");

    // Password used to decrypt key
    let password = "test_password";
    // Create and initialise root.json
    initialize_root_json(root_json.to_str().unwrap());
    // Add key for all roles
}

// Test signing with multiple encrypted keys, each with a distinct password
#[test]
fn sign_root_with_multiple_encrypted_keys() {
    let out_dir = TempDir::new().unwrap();
    let root_json = out_dir.path().join("root.json");
    let key1 = test_utils::test_data().join("snakeoil_3.pem");
    let key2 = test_utils::test_data().join("snakeoil_4.pem");

    let password1 = "test_password_1";
    let password2 = "test_password_2";

    // Initialize root.json
    initialize_root_json(root_json.to_str().unwrap());

    // Add both keys for all roles
    add_key_to_root_json(root_json.to_str().unwrap(), key1.to_str().unwrap(), Some(password1));
    add_key_to_root_json(root_json.to_str().unwrap(), key2.to_str().unwrap(), Some(password2));

    // Try signing with correct passwords for both keys
    let sign_result = sign_root_json_with_multiple_keys(
        root_json.to_str().unwrap(),
        &[
            (key1.to_str().unwrap(), password1),
            (key2.to_str().unwrap(), password2),
        ],
    );
    assert!(sign_result.is_ok(), "Signing with correct passwords should succeed");

    // Try signing with wrong password for key1
    let sign_result_wrong1 = sign_root_json_with_multiple_keys(
        root_json.to_str().unwrap(),
        &[
            (key1.to_str().unwrap(), "wrong_password"),
            (key2.to_str().unwrap(), password2),
        ],
    );
    assert!(sign_result_wrong1.is_err(), "Signing with wrong password for key1 should fail");

    // Try signing with wrong password for key2
    let sign_result_wrong2 = sign_root_json_with_multiple_keys(
        root_json.to_str().unwrap(),
        &[
            (key1.to_str().unwrap(), password1),
            (key2.to_str().unwrap(), "wrong_password"),
        ],
    );
    assert!(sign_result_wrong2.is_err(), "Signing with wrong password for key2 should fail");

    // Try signing with wrong passwords for both keys
    let sign_result_wrong_both = sign_root_json_with_multiple_keys(
        root_json.to_str().unwrap(),
        &[
            (key1.to_str().unwrap(), "wrong_password"),
            (key2.to_str().unwrap(), "wrong_password"),
        ],
    );
    assert!(sign_result_wrong_both.is_err(), "Signing with wrong passwords for both keys should fail");
}

```

You will need to implement or adapt the following helper functions if they do not already exist:
- `add_key_to_root_json(root_path: &str, key_path: &str, password: Option<&str>)` to add a key to the root.json, using the password if provided.
- `sign_root_json_with_multiple_keys(root_path: &str, keys: &[(&str, &str)]) -> Result<(), Error>` to sign the root.json with multiple keys, each with its password.

If your test utilities use different conventions for adding keys or signing, adjust the function calls accordingly.
</issue_to_address>

### Comment 3
<location> `tuftool/tests/root_command.rs:517-522` </location>
<code_context>
+    let root_json = out_dir.path().join("root.json");
+    let key = test_utils::test_data().join("snakeoil_3.pem");
+
+    // Invalid password
+    let password = "invalid_password";
+    // Create and initialise root.json
+    initialize_root_json(root_json.to_str().unwrap());
+    // Add key to root role
+    let mut cmd = Command::cargo_bin("tuftool").unwrap();
+    cmd.args([
+        "root",
+        "add-key",
+        root_json.to_str().unwrap(),
+        "--role",
+        "root",
+    ])
+    .arg("-k")
+    .arg(key.to_str().unwrap());
+    if !password.is_empty() {
+        cmd.args(["--password", password]);
+    }
+    cmd.assert().failure();
+}
</code_context>

<issue_to_address>
**suggestion (testing):** Missing test for missing password with encrypted key.

Please add a test case where an encrypted key is used without supplying a password, and verify that the expected error is raised.

```suggestion
#[test]
// Add encrypted key with an invalid password
fn create_root_encrypted_key_invalid_password() {
    let out_dir = TempDir::new().unwrap();
    let root_json = out_dir.path().join("root.json");
    let key = test_utils::test_data().join("snakeoil_3.pem");

    // Invalid password
    let password = "invalid_password";
    // Create and initialise root.json
    initialize_root_json(root_json.to_str().unwrap());
    // Add key to root role
    let mut cmd = Command::cargo_bin("tuftool").unwrap();
    cmd.args([
        "root",
        "add-key",
        root_json.to_str().unwrap(),
        "--role",
        "root",
    ])
    .arg("-k")
    .arg(key.to_str().unwrap());
    if !password.is_empty() {
        cmd.args(["--password", password]);
    }
    cmd.assert().failure();
}

// Test: Add encrypted key without supplying a password
#[test]
fn create_root_encrypted_key_missing_password() {
    let out_dir = TempDir::new().unwrap();
    let root_json = out_dir.path().join("root.json");
    let key = test_utils::test_data().join("snakeoil_3.pem");

    // Create and initialise root.json
    initialize_root_json(root_json.to_str().unwrap());
    // Add key to root role without password
    let mut cmd = Command::cargo_bin("tuftool").unwrap();
    cmd.args([
        "root",
        "add-key",
        root_json.to_str().unwrap(),
        "--role",
        "root",
    ])
    .arg("-k")
    .arg(key.to_str().unwrap());
    // Do NOT supply --password
    let assert = cmd.assert().failure();
    // Optionally check for expected error message
    assert.stderr(predicates::str::contains("password required").or(predicates::str::contains("failed to decrypt")));
}
```
</issue_to_address>

### Comment 4
<location> `tuftool/tests/root_command.rs:482-491` </location>
<code_context>
+    // Password used to decrypt key
</code_context>

<issue_to_address>
**suggestion (testing):** Consider adding a test for empty password with encrypted key.

Testing this scenario will verify that the code properly handles and rejects empty passwords when decrypting keys.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[fghanmi]

/ask can we accept encrypted passwords instead of plain texts ?

[qodo-code-review]

Ask❓

can we accept encrypted passwords instead of plain texts ?

Answer:

Short answer: not with the current implementation. The PR accepts passwords as plain strings from CLI flags and threads them through to parsing/decryption, but it does not support receiving already-encrypted/encoded secrets nor any KMS/OS keyring integration.

Details observed:

• CLI surfaces passwords as Option<Vec> or Option (e.g., --password/--new-password flags across tuftool commands). These are passed as plain strings to parse_key_source(..., Some(password.to_string())) and ultimately to parse_keypair(data, password.as_deref()).
• The decryption flow in tough/src/sign.rs expects a cleartext passphrase:
  • decrypt_key(...) uses EncryptedPrivateKeyInfo::decrypt(password.as_bytes()) which requires the passphrase in plaintext bytes.
  • There’s no mechanism to decrypt a “password ciphertext” using a key management service or local key.
• Validation in tuftool (rhtas.rs) uses openssl::PKey::private_key_from_pem_passphrase with the provided plaintext password for verification.
• SSM path: tough-ssm calls parse_keypair(&data, None); there’s no way to fetch or pass a password from SSM yet.
• Source parser (tuftool/src/source.rs) only adds an Optional password field on LocalKeySource; there is no handler for secure references like env vars, files, keyring, or KMS-encrypted blobs.

What would be required to accept “encrypted passwords”:

• Define an input scheme for non-plaintext secrets, for example:
  • --password-file PATH (read passphrase from file/FD)
  • --password-env NAME (read from environment variable)
  • --password-cmd "cmd ..." (read from command output)
  • --password-ssm "aws-ssm://parameter" (retrieve from SSM)
  • --password-kms "kms://..." (decrypt with KMS and use result)
  • --password-b64 (base64-encoded passphrase, already supported trivially by the caller before passing)
• Implement corresponding retrieval/decryption logic before calling parse_keypair:
  • For KMS/SSM/keyring: add new KeySource-like secret providers or helper utilities.
  • For “encrypted password” blobs: we’d need a decryption key source (local KMS, age, gpg, etc.) and a format contract to decode to the cleartext passphrase.
• Wire through to all call sites that currently take Option<Vec> and ensure consistent mapping with keys (including length validations already present).
• Security improvements: prefer reading passphrases from stdin (no-echo) or files/FDs to avoid shell history; avoid logging; consider disabling --password plaintext flag or marking it deprecated.

Minimal, incremental path:

• Add flags: --password-file and --new-password-file (multi-valued, aligning by index with --key/--new-key).
• Implement reading passphrases from files and from stdin (e.g., --password-stdin to read one per key, separated by newlines).
• Optionally add --password-env to reference environment variables.
• Keep internal interfaces accepting Option but populate them from secure sources.

Conclusion: As-is, only plaintext passphrases are supported. To accept “encrypted passwords,” we need to introduce secret retrieval/decryption mechanisms and new CLI options, then decode/decrypt to plaintext before invoking parse_keypair/decrypt_key.

📄 References

• No matching references available

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Refactor password handling to avoid duplication To reduce code duplication in tuftool, create a shared helper function that handles the logic of parsing key sources and their corresponding optional passwords, returning a list of KeySource objects. Examples: tuftool/src/create.rs [87-100] let mut keys = Vec::new(); let default_password = String::new(); let passwords = match &self.passwords { Some(pws) => pws, None => &vec![], }; if passwords.len() > self.keys.len() { error::MorePasswordsSnafu.fail()?; } for (i, source) in self.keys.iter().enumerate() { ... (clipped 4 lines) tuftool/src/add_role.rs [126-139] let mut keys = Vec::new(); let default_password = String::new(); let passwords = match &self.passwords { Some(pws) => pws, None => &vec![], }; if passwords.len() > self.keys.len() { error::MorePasswordsSnafu.fail()?; } for (i, source) in self.keys.iter().enumerate() { ... (clipped 4 lines) Solution Walkthrough: Before: // In tuftool/src/create.rs async fn run(&self) -> Result<()> { let mut keys = Vec::new(); let default_password = String::new(); let passwords = match &self.passwords { Some(pws) => pws, None => &vec![], }; if passwords.len() > self.keys.len() { error::MorePasswordsSnafu.fail()?; } for (i, source) in self.keys.iter().enumerate() { let password = passwords.get(i).unwrap_or(&default_password); let key_source = parse_key_source(source, Some(password.to_string()))?; keys.push(key_source); } // ... use keys } After: // In a new tuftool/src/common.rs or similar pub(crate) fn parse_keys_and_passwords( key_sources: &[String], passwords: &Option<Vec<String>>, ) -> Result<Vec<Box<dyn KeySource>>> { // ... implementation to parse keys with passwords } // In tuftool/src/create.rs async fn run(&self) -> Result<()> { let keys = common::parse_keys_and_passwords(&self.keys, &self.passwords)?; // ... use keys } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies significant code duplication for password and key source handling across many files in tuftool, and proposing a helper function is a valid and impactful refactoring.	Medium
General	Remove redundant key decryption logic Remove the redundant check_key_encryption function and its call site, relying on the core tough library for key decryption and validation to ensure consistency. tuftool/src/rhtas.rs [1153-1173] -// Function to check if key is encrypted and validate password -fn check_key_encryption(path: String, password: &str) -> Result<()> { - let path_buf = PathBuf::from(&path); +// This function should be removed. +// The `rhtas` command's `update_metadata` function should be modified to handle +// errors from `key_source.as_sign()` and provide a specific error message +// for decryption failures. - let key_content = read_key_file(path).context(error::FileReadSnafu { - path: path_buf.clone(), - })?; +// In `update_metadata` function: +// ... +for (i, source) in self.keys.iter().enumerate() { + let password = passwords.get(i).unwrap_or(&default_password); + let key_source = parse_key_source(source, Some(password.to_string()))?; + // The call to check_key_encryption is removed. + // The error will be caught when `as_sign` is called on the key source later. + keys.push(key_source); +} +// ... - let is_encrypted = key_content.contains("ENCRYPTED"); - - if is_encrypted { - if is_encrypted && password.is_empty() { - return Err(error::EncryptedKeyNoPasswordSnafu { path: path_buf }.build()); - } - // Try parsing the encrypted key with the password - PKey::private_key_from_pem_passphrase(key_content.as_bytes(), password.as_bytes()) - .context(error::InvalidPasswordSnafu { path: path_buf })?; - } - - Ok(()) -} - [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies redundant and potentially inconsistent decryption logic in check_key_encryption and proposes removing it to rely on the single source of truth in the tough crate, which is a significant code quality improvement.	Medium
	Avoid unnecessary re-encoding of decrypted key Modify decrypt_key to return the raw decrypted key bytes directly, removing the unnecessary step of re-encoding them into PEM format. tough/src/sign.rs [172-185] /// Decrypts an RSA private key in PEM format using the given password. /// Returns the decrypted key in PKCS8 format pub fn decrypt_key( encrypted_key: &[u8], password: &str, ) -> std::result::Result<Vec<u8>, Box<dyn std::error::Error>> { let pem_str = std::str::from_utf8(encrypted_key)?; let pem = pem::parse(pem_str)?; let encrypted_private_key_document = pkcs8::EncryptedPrivateKeyInfo::from_der(pem.contents())?; let decrypted_private_key_document = encrypted_private_key_document.decrypt(password.as_bytes())?; - let decrypted_key_bytes = decrypted_private_key_document.as_bytes(); - let pem_output = pem::Pem::new("PRIVATE KEY".to_string(), decrypted_key_bytes.to_vec()); - let pem_encoded = pem::encode(&pem_output); - Ok(pem_encoded.as_bytes().to_vec()) + Ok(decrypted_private_key_document.as_bytes().to_vec()) } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that re-encoding the decrypted key to PEM format is unnecessary, and removing this step simplifies the code and avoids a minor, needless processing cycle.	Low
Possible issue	Improve key parsing logic for robustness Modify the key parsing logic to first attempt parsing without a password, even if one is provided, to gracefully handle unencrypted keys used with a password. tough/src/sign.rs [191-199] let decrypted_key = if let Some(pw) = password { - if pw.is_empty() { + // First, try to parse the key without a password. This handles cases where a + // password is provided for a key that is not encrypted. + if Ed25519KeyPair::from_pkcs8(key).is_ok() + || EcdsaKeyPair::from_pkcs8(&aws_lc_rs::signature::ECDSA_P256_SHA256_ASN1_SIGNING, key).is_ok() + || (pem::parse(key).is_ok() && RsaKeyPair::from_pkcs8(pem::parse(key).unwrap().contents()).is_ok()) + { key.to_vec() + } else if !pw.is_empty() { + decrypt_key(key, pw).map_err(|_| error::KeyDecryptionSnafu.build())? } else { - decrypt_key(key, pw).map_err(|_| error::KeyDecryptionSnafu.build())? + key.to_vec() } } else { key.to_vec() }; Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a scenario where providing a password for an unencrypted key would lead to a confusing error, and the proposed change improves robustness and user experience.	Low
More

[fghanmi]

/implement re-implement: allow using password-protected private keys

[fghanmi]

/implement make password a mandatory parameter

[fghanmi]

hello!

[fghanmi]

hello!

/implement make password a mandatory parameter

[fghanmi]

Password param

[fghanmi]

/implement make password a mandatory parameter

[qodo-code-review]

Code Implementation 🛠️

Implementation: Make the --password (-p) CLI parameter mandatory by marking it as required in the RhtasArgs struct definition.

Suggested change

	#[arg(short, long = "password")]
	#[arg(short, long = "password", required = true)]
	passwords: Option<Vec<String>>,

📄 References

• No matching references available

See review comment here

[fghanmi]

/implement make password mandatory.

[qodo-code-review]

Code Implementation 🛠️

Implementation: Make the password field in LocalKeySource mandatory by changing its type from Option to String. This enforces that a password must always be provided when constructing LocalKeySource instances.

Suggested change

	pub password: Option<String>,
	pub password: String,

📄 References

• No matching references available

See review comment here

[fghanmi]

/implement make all password parameters mandatory.