test: add key export error-path and cipher coverage

[Koan-Bot]

What

Add 15 new tests to format.t covering key export error paths and additional cipher algorithms.

Why

Key export error handling (get_private_key_string with bad cipher, missing passphrase, unsupported cipher name) and public key format detection (new_public_key with unrecognized headers) had zero test coverage. These are real code paths (RSA.xs lines 435-449, RSA.pm lines 22-30) that could silently break during refactoring.

How

Extended format.t with tests for:

• Additional ciphers: aes-256-cbc, aes-192-cbc (beyond existing des3 and aes-128-cbc)
• Special character passphrases
• Error paths: cipher without passphrase, unsupported cipher, public-only key export, wrong/missing passphrase on import, unrecognized PEM headers
• Cross-format verification: X509 public key from private key matches PKCS1

Testing

All 296 tests pass locally (OpenSSL 3.6.1, Perl 5.42.0, macOS).

---

🤖 Generated with Claude Code

[atoomic]

@Koan-Bot rebase

[atoomic]

@Koan-Bot rebase

[Koan-Bot]

Rebase: test: add key export error-path and cipher coverage

Branch koan.atoomic/test-format-error-paths rebased onto main and force-pushed.

Diff: 1 file changed, 53 insertions(+), 15 deletions(-)

Review feedback was analyzed and applied.

Actions

• Resolved merge conflicts (1 round(s))
• Rebased koan.atoomic/test-format-error-paths onto upstream/main
• Force-pushed koan.atoomic/test-format-error-paths to origin

---

Automated by Kōan

[Koan-Bot]

Rebase: test: add key export error-path and cipher coverage

Branch koan.atoomic/test-format-error-paths rebased onto main and force-pushed.

Diff: 1 file changed, 53 insertions(+), 15 deletions(-)

Review feedback was analyzed and applied.

Actions

• Rebased koan.atoomic/test-format-error-paths onto upstream/main
• Force-pushed koan.atoomic/test-format-error-paths to origin

---

Automated by Kōan

[atoomic]

@toddr-bot review

[timlegge]

The no passphrase hangs test. I would also worry a little about the like checks as the error may differ across systems but just remove the hanging test I guess

@Koan-Bot review

[timlegge]

@Koan-Bot On my system OOpenSSL 3.0.13 30 Jan 2024 this prompts for a passphrase and hangs until something is entered

[Koan-Bot]

Confirmed — this is the blocking issue. When new_private_key() is called on an encrypted PEM without a passphrase, OpenSSL's default password callback reads from stdin, hanging the test. The fix is to simply remove lines 129-130 (eval { ...new_private_key($encrypted_pem) }; ok($@, ...)) and adjust the plan from 36 to 35. The wrong-passphrase test on line 126 already covers the decrypt-failure error path, so no coverage is lost.

Regarding the like checks: the patterns (Passphrase is required for cipher, Unsupported cipher, unrecognized key format) all match error messages from RSA.pm itself, not from OpenSSL, so they should be stable across OpenSSL versions. That said, if you want extra safety, the patterns could be shortened (e.g., qr/Passphrase/i, qr/cipher/i).

[toddr-bot]

This has been fixed in the latest rebase — the test that called new_private_key($encrypted_pem) without a passphrase argument has been removed from the diff. The current version only tests with an explicit wrong passphrase ('wrong_passphrase'), which fails fast via OpenSSL's decrypt path without triggering the interactive stdin prompt.

[toddr-bot]

Confirmed the error message patterns are stable: "unrecognized key format" comes from RSA.pm line 29 (croak), "Passphrase is required for cipher" from RSA.xs line 441, and "Unsupported cipher: %s" from RSA.xs line 453. These are all module-controlled strings, not OpenSSL error strings, so the like() patterns are safe across OpenSSL versions.

[Koan-Bot]

PR Review — test: add key export error-path and cipher coverage

Good test coverage additions — the cipher round-trip tests, special-character passphrase, and public key format detection tests are all valuable. However, the test at line 129 (new_private_key on encrypted PEM without passphrase) hangs on OpenSSL 3.0.x because it triggers an interactive password prompt, as @timlegge already reported. This is a blocking issue that must be fixed before merge. Remove that test and adjust the plan count. The pass() on the public-only key export is weak but not blocking.

---

🔴 Blocking

1. Test hangs: encrypted key import without passphrase triggers interactive prompt (t/format.t, L129-130)
This was already flagged by @timlegge. When new_private_key is called on an encrypted PEM without a passphrase argument, OpenSSL's default password callback prompts on stdin, causing the test to hang indefinitely in non-interactive environments (CI, make test).

Fix: Remove this test entirely, or if you want to keep coverage, pass an empty string '' instead of omitting the passphrase (which will fail fast with a decryption error rather than blocking). Alternatively, wrap with an alarm timeout:

SKIP: {
    skip 'encrypted key without passphrase hangs on interactive prompt', 1;
    eval { Crypt::OpenSSL::RSA->new_private_key($encrypted_pem) };
    ok($@, 'new_private_key croaks on encrypted key without passphrase');
}

But skipping defeats the purpose. The cleanest fix is to just drop this test — the "wrong passphrase" test on line 126 already covers the error path. Adjust the plan from 36 to 35.

eval { Crypt::OpenSSL::RSA->new_private_key($encrypted_pem) };
ok($@, "new_private_key croaks on encrypted key without passphrase");

🟡 Important

1. Always-pass test provides false confidence (t/format.t, L117-120)
The pass() call succeeds regardless of whether the eval died or succeeded. If get_private_key_string() segfaults (which is what you're guarding against), the pass() never runs anyway — Perl is already dead. And if it croaks, the eval catches it but you never inspect $@.

Consider making this test assert something meaningful:

eval { $pub_only->get_private_key_string() };
ok(!$@ || $@ =~ /private key/, "get_private_key_string on public-only key behaves gracefully");

Or at minimum add a comment explaining that you're only testing for non-crash (the current comment is good but the pass() is still misleading to anyone reading test output).

eval { $pub_only->get_private_key_string() };
pass("get_private_key_string on public-only key does not crash");

🟢 Suggestions

1. Typo in variable name: $passphase should be $passphrase (t/format.t, L105-106)
This is inherited from the existing code, not introduced by this PR, but since you're adding new uses of $passphase (lines 93, 95, 107, 110, 125), it's worth noting. Not blocking, but a rename in a follow-up would improve readability.

my $special_pass = q{p@ss!w0rd#$%^&*()};
ok( $private_key2 = Crypt::OpenSSL::RSA->new_private_key( $private_key->get_private_key_string($special_pass), $special_pass ), "passphrase with special characters round-trips" );

2. Error message pattern changed from original test (t/format.t, L109-110)
The original test matched qr/Passphrase is required/ but this now matches qr/Passphrase is required for cipher/. The longer pattern is fine if the module's error message is stable, but note this couples the test more tightly to the exact wording. The cipher was also changed from aes-128-cbc to des3 — not a problem, just noting the change isn't called out in the PR description.

eval { $private_key->get_private_key_string(undef, 'des3') };
like($@, qr/Passphrase is required for cipher/, "get_private_key_string croaks when cipher given without passphrase");

---

Checklist

• No hardcoded secrets
• Test isolation (no shared mutable state)
• Test plan count matches actual tests
• Tests don't hang or block in CI — critical quietened critic to -3, added test #1
• Error patterns match module messages (not OpenSSL-version-specific)

---

Summary

Good test coverage additions — the cipher round-trip tests, special-character passphrase, and public key format detection tests are all valuable. However, the test at line 129 (new_private_key on encrypted PEM without passphrase) hangs on OpenSSL 3.0.x because it triggers an interactive password prompt, as @timlegge already reported. This is a blocking issue that must be fixed before merge. Remove that test and adjust the plan count. The pass() on the public-only key export is weak but not blocking.

---

Automated review by Kōan

[atoomic]

@Koan-Bot rebase

[Koan-Bot]

Rebase: test: add key export error-path and cipher coverage

Branch koan.atoomic/test-format-error-paths rebased onto main and force-pushed.

Diff: 1 file changed, 50 insertions(+), 15 deletions(-)

Review feedback was analyzed and applied.

Actions

• Rebased koan.atoomic/test-format-error-paths onto upstream/main
• Applied review feedback
• Force-pushed koan.atoomic/test-format-error-paths to origin

---

Automated by Kōan

[toddr-bot]

PR Review — test: add key export error-path and cipher coverage

Clean test expansion with good coverage of cipher algorithms, special-character passphrases, and error paths. The previously-reported hanging test (encrypted key import without passphrase) has been correctly removed in the rebase. The like() patterns match error strings from RSA.pm (line 29) and RSA.xs (lines 441, 453), not from OpenSSL itself, so they are stable across OpenSSL versions. The only minor issue is the pass() on the public-only key export test, which always succeeds regardless of outcome — acceptable for a crash-guard test but could be slightly more informative. Test count (35) is correct. Merge-ready.

---

🟡 Important

1. pass() provides no meaningful assertion (t/format.t, L117-120)
The pass() call succeeds unconditionally — it doesn't distinguish between get_private_key_string() returning a PEM (LibreSSL/1.x) and dying (OpenSSL 3.x). If the goal is "doesn't segfault or hang", this works, but a slightly more informative pattern would document what actually happened:

my $pub_only = Crypt::OpenSSL::RSA->new_public_key($PUBLIC_KEY_PKCS1_STRING);
my $died = !eval { $pub_only->get_private_key_string(); 1 };
ok(1, "get_private_key_string on public-only key does not crash (died=" . ($died ? 'yes' : 'no') . ")");

This still always passes but logs whether the call died, which helps diagnose cross-platform differences. Not blocking since the current form is functionally correct for a crash-guard test.

my $pub_only = Crypt::OpenSSL::RSA->new_public_key($PUBLIC_KEY_PKCS1_STRING);
# Behavior varies: OpenSSL 3.x may croak, 1.x/LibreSSL returns a PEM
eval { $pub_only->get_private_key_string() };
pass("get_private_key_string on public-only key does not crash");

---

Checklist

• No hardcoded secrets
• Error message patterns match source code
• Test count matches plan
• No hanging/blocking test calls
• Test isolation (no shared mutable state)

---

Summary

Clean test expansion with good coverage of cipher algorithms, special-character passphrases, and error paths. The previously-reported hanging test (encrypted key import without passphrase) has been correctly removed in the rebase. The like() patterns match error strings from RSA.pm (line 29) and RSA.xs (lines 441, 453), not from OpenSSL itself, so they are stable across OpenSSL versions. The only minor issue is the pass() on the public-only key export test, which always succeeds regardless of outcome — acceptable for a crash-guard test but could be slightly more informative. Test count (35) is correct. Merge-ready.

---

Automated review by Kōan

[timlegge]

passes on 0.37. lgtm @atoomic