feat: support DER-encoded public keys in new_public_key()

[Koan-Bot]

What

new_public_key() now accepts DER-encoded public keys (raw binary ASN.1) alongside PEM.

Why

Users passing DER keys got an unhelpful "unrecognized key format" error with no guidance.
DER is the standard binary encoding for ASN.1 keys — supporting it removes a common friction point,
especially for users working with keys from Java, .NET, or certificate toolchains that default to DER.

How

• Perl layer: Detects DER by checking for ASN.1 SEQUENCE tag (\x30) when input isn't PEM.
  Tries X.509 SubjectPublicKeyInfo first (most common), falls back to PKCS#1 RSAPublicKey.
• XS layer: Two new functions:
  • _new_public_key_x509_der: d2i_PUBKEY_bio (3.x) / d2i_RSA_PUBKEY_bio (pre-3.x)
  • _new_public_key_pkcs1_der: OSSL_DECODER (3.x) / d2i_RSAPublicKey_bio (pre-3.x)
• Error messages: Now specific — tells users what formats are accepted instead of just "unrecognized".

Testing

• 14 new tests in t/der.t: DER loading for both formats, PEM round-trip verification,
  signature verification with DER-loaded keys, error cases (truncated DER, random binary, empty string).
• Full suite: 474 tests pass (460 existing + 14 new).

🤖 Generated with Claude Code

---

Quality Report

Changes: 5 files changed, 465 insertions(+), 87 deletions(-)

Code scan: clean

Tests: passed (OK)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

[timlegge]

@Koan-Bot it would be great to allow importing a private key from DER format as well.

[atoomic]

@Koan-Bot rebase

[atoomic]

LGTM

[Koan-Bot]

Rebase: feat: support DER-encoded public keys in new_public_key()

Branch koan.atoomic/new-public-key-der-support rebased onto main and force-pushed.

Diff: 3 files changed, 282 insertions(+), 12 deletions(-)

Review feedback was analyzed and applied.

Changes

• Added DER-encoded private key support in new_private_key() per @timlegge's request to support importing private keys from DER format, matching the existing DER support added for public keys.
• RSA.xs: Renamed XS new_private_key to _new_private_key_pem (internal), added _new_private_key_der XS function using OSSL_DECODER on OpenSSL 3.x and d2i_PrivateKey_bio on pre-3.x.
• RSA.pm: Added Perl-level new_private_key wrapper that detects PEM (starts with -----) vs DER (ASN.1 SEQUENCE tag \x30) and routes to the appropriate XS function. Passphrase parameter is passed through for PEM keys.
• RSA.pm POD: Updated new_private_key documentation to describe DER support and clarify passphrase is PEM-only.
• t/der.t: Added 8 new tests (14 → 22 total): DER private key loading, round-trip verification, signing with DER-loaded private key, error cases for invalid DER private key data, and regression test that PEM private keys still work through the new wrapper.

Actions

• Rebased koan.atoomic/new-public-key-der-support onto upstream/main
• Applied review feedback
• Force-pushed koan.atoomic/new-public-key-der-support to origin
• CI failed (attempt 1)
• Applied CI fix (attempt 1)
• Pushed CI fix (attempt 1)
• CI passed after fix attempt 1

CI

CI failed initially, fixed on attempt 1.

---

Automated by Kōan

[timlegge]

@Koan-Bot rebase from main. Right now I see an issue in one of the tests:

perl -Mblib t/error.t
ok 1 - new_private_key croaks on garbage input
ok 2 - new_private_key croaks on empty string
Use of uninitialized value $p_key_string in pattern match (m//) at /home/tim/code/Crypt-OpenSSL-RSA/blib/lib/Crypt/OpenSSL/RSA.pm line 48.
Use of uninitialized value length($p_key_string) in numeric gt (>) at /home/tim/code/Crypt-OpenSSL-RSA/blib/lib/Crypt/OpenSSL/RSA.pm line 51.
ok 3 - new_private_key croaks on undef

[Koan-Bot]

Rebase: feat: support DER-encoded public keys in new_public_key()

Branch koan.atoomic/new-public-key-der-support rebased onto main and force-pushed.

Diff: 3 files changed, 288 insertions(+), 12 deletions(-)

Review feedback was analyzed and applied.

Changes

Good — the test already expects new_private_key(undef) to croak. With the guard added, it will croak cleanly without the "uninitialized value" warnings.

Summary of changes:

• Added undef/empty guard to new_private_key() — the function now croaks immediately with a clear error message when called with undef or empty string, fixing the "Use of uninitialized value" warnings reported by @timlegge ($p_key_string was used in regex match and length() without checking defined first).
• Added matching guard to new_public_key() — same defensive check for consistency, since it had the same potential issue with undef input hitting the regex on line 22.

Actions

• Already-solved check: negative (confidence=high, reasoning=None of the recent commits on main mention DER key support, DER encoding, or binary ASN.1 key loadin)
• Rebased koan.atoomic/new-public-key-der-support onto upstream/main
• Applied review feedback
• Force-pushed koan.atoomic/new-public-key-der-support to origin
• CI check enqueued (async)

CI

CI will be checked asynchronously.

---

Automated by Kōan

[timlegge]

@Koan-Bot Review this new test and add it to the PR:

Its AUTHOR_TESTING in case openssl output is different other places. It ensures that the import a der can import an openssl generated der

t/openssl_der.t:

use strict;
use warnings;
use Test::More;
use MIME::Base64    qw/decode_base64/;
use Digest::SHA     qw/sha1_hex/;
use File::Temp      qw/ tempfile tempdir /;
use File::Slurper   qw/read_binary write_binary/;

use Crypt::OpenSSL::RSA;
use Crypt::OpenSSL::Bignum;

BEGIN {
  unless ($ENV{AUTHOR_TESTING}) {
    print qq{1..0 # SKIP these tests are for testing by the author\n};
    exit
  }
}
my ($rsa_fh, $rsa_file) = tempfile(UNLINK => 1);

# Create a new RSA key
my $openssl = `openssl genrsa -out $rsa_file 2048 > /dev/null 2>&1`;

# Get the output as text that includes the private key PEM
my $priv_output = `openssl rsa -inform PEM -in $rsa_file -text 2>&1`;

# Get the output as text that includes the private key PEM
my $pub_output = `openssl rsa -inform PEM -in $rsa_file -pubout -text 2>&1`;


# Basic grab multi-line data between
# two tags from openssl -text output
sub get_parameter {
    my $text    = shift;
    my $start   = shift;
    my $end     = shift;
    
    # Fieldname may end in ':'
    $text =~ /$start:*\s*(.*?)\s*$end:*/s;
    my $parameter = $1;
    # Remove ':' and white space including newlines
    $parameter =~ s/[:\s]//g;

    # The exponent data we want is the hex data
    # with no 0x prefix
    if($parameter =~ /\((.*?)\)/ ) {
        $parameter = $1;
        $parameter =~ s/0x//g;
    }
    return $parameter;
}

# Compare a bignum to hex data
sub compare_bignum_to_hex {
    my $bn1 = shift;
    my $hex = shift;

    my $bn2 = Crypt::OpenSSL::Bignum->new_from_hex($hex);
    isa_ok($bn2, 'Crypt::OpenSSL::Bignum');
    return $bn2->cmp($bn1);
}

# Extract the values from the openssl private key output
my $priv_n = get_parameter($priv_output, 'modulus', 'publicExponent');
my $priv_e = get_parameter($priv_output, 'publicExponent', 'privateExponent');
my $priv_d = get_parameter($priv_output, 'privateExponent', 'prime1');
my $priv_p = get_parameter($priv_output, 'prime1', 'prime2');
my $priv_q = get_parameter($priv_output, 'prime2', 'exponent1');
my $priv_dmp1 = get_parameter($priv_output, 'exponent1', 'exponent2');
my $priv_dmq1 = get_parameter($priv_output, 'exponent2', 'coefficient');
my $priv_iqmp = get_parameter($priv_output, 'coefficient', '-----BEGIN PRIVATE KEY-----');
my $priv_key = get_parameter($priv_output, '-----BEGIN PRIVATE KEY-----', '-----END PRIVATE KEY-----');

# Load the private key from the DER (base64 decoded PEM)
my $rsa = Crypt::OpenSSL::RSA->new_private_key(decode_base64($priv_key));

# Get the private key parameters
my ($n, $e, $d, $p, $q, $dmp1, $dmq1, $iqmp) = $rsa->get_key_parameters();

# Check each private key parameter to the expected values
ok(compare_bignum_to_hex($n, $priv_n) == 0, "Imported DER n parameter matches expected");
ok(compare_bignum_to_hex($e, $priv_e) == 0, "Imported DER e parameter matches expected");
ok(compare_bignum_to_hex($d, $priv_d) == 0, "Imported DER d parameter matches expected");
ok(compare_bignum_to_hex($p, $priv_p) == 0, "Imported DER p parameter matches expected");
ok(compare_bignum_to_hex($q, $priv_q) == 0, "Imported DER q parameter matches expected");
ok(compare_bignum_to_hex($dmp1, $priv_dmp1) == 0, "Imported DER dmp1 parameter matches expected");
ok(compare_bignum_to_hex($dmq1, $priv_dmq1) == 0, "Imported DER dmq1 parameter matches expected");
ok(compare_bignum_to_hex($iqmp, $priv_iqmp) == 0, "Imported DER iqmp parameter matches expected");

# Extract the public key values from the openssl public key output
my $pub_n = get_parameter($pub_output, 'modulus', 'publicExponent');
my $pub_e = get_parameter($pub_output, 'publicExponent', 'privateExponent');
my $pub_d = get_parameter($pub_output, 'privateExponent', 'prime1');
my $pub_p = get_parameter($pub_output, 'prime1', 'prime2');
my $pub_q = get_parameter($pub_output, 'prime2', 'exponent1');
my $pub_dmp1 = get_parameter($pub_output, 'exponent1', 'exponent2');
my $pub_dmq1 = get_parameter($pub_output, 'exponent2', 'coefficient');
my $pub_iqmp = get_parameter($pub_output, 'coefficient', '-----BEGIN PUBLIC KEY-----');
my $pub_key = get_parameter($pub_output, '-----BEGIN PUBLIC KEY-----', '-----END PUBLIC KEY-----');

# Load the public key from the DER (base64 decoded PEM)
my $pub_rsa = Crypt::OpenSSL::RSA->new_public_key(decode_base64($pub_key));

# Get the private key parameters
my ($p_n, $p_e, $p_d, $p_p, $p_q, $p_dmp1, $p_dmq1, $p_iqmp) = $pub_rsa->get_key_parameters();

# Check each public key parameter to the expected values
ok(compare_bignum_to_hex($p_n, $pub_n) == 0, "Imported public DER n parameter matches expected");
ok(compare_bignum_to_hex($p_e, $pub_e) == 0, "Imported public DER e parameter matches expected");
ok(!$p_d, "Imported public DER d parameter undef as expected");
ok(!$p_p, "Imported public DER p parameter undef as expected");
ok(!$p_q, "Imported public DER q parameter undef as expected");
ok(!$p_dmp1, "Imported public DER dmp1 parameter undef as expected");
ok(!$p_dmq1, "Imported public DER dmq1 parameter undef as expected");
ok(!$p_iqmp, "Imported public DER iqmp parameter undef as expected");

done_testing();

[Koan-Bot]

PR Review — feat: support DER-encoded public keys in new_public_key()

Solid feature addition with good test coverage and clean XS code. Two issues should be addressed before merge: (1) _new_private_key_der uses d2i_RSAPrivateKey_bio on pre-3.x which only handles PKCS#1 format — PKCS#8 DER keys (common from Java/.NET/openssl genpkey) will fail; use d2i_PrivateKey_bio instead. (2) The DER public key fallback silently discards the first error, potentially confusing users when both attempts fail. The passphrase-ignored-for-DER issue is minor but worth a carp. Overall architecture is sound and the error messages are a nice improvement.

---

🟡 Important

1. _new_private_key_der pre-3.x: d2i_RSAPrivateKey_bio only handles PKCS#1

On pre-3.x, d2i_RSAPrivateKey_bio only handles PKCS#1 RSAPrivateKey DER format. PKCS#8 PrivateKeyInfo DER (which is what many tools like Java keytool, .NET, and openssl genpkey -outform DER produce) will fail silently.

Use d2i_PrivateKey_bio(bio, NULL) instead — it auto-detects both PKCS#1 and PKCS#8 formats, matching the broader coverage the 3.x OSSL_DECODER path already provides.

Note: get_private_key_string() outputs PKCS#1 PEM via PEM_write_bio_PrivateKey_traditional, so the round-trip test passes since it only exercises PKCS#1. A PKCS#8 DER key would fail on pre-3.x with the current code.

pkey = d2i_RSAPrivateKey_bio(bio, NULL);

2. Silent fallback between DER formats may produce confusing errors (`RSA.pm`, L41-43)

When _new_public_key_x509_der fails and the eval catches the croak, the error is silently discarded. Then _new_public_key_pkcs1_der is tried. If both fail, the user sees only the PKCS#1 error, which may be misleading (e.g., if the key was actually X.509 DER but corrupted).

Consider capturing both errors and including them in the final croak, or at minimum wrapping the fallback with a more descriptive error message. For example:

my $rsa = eval { $proto->_new_public_key_x509_der($p_key_string) };
return $rsa if $rsa;
my $x509_err = $@;
my $rsa2 = eval { $proto->_new_public_key_pkcs1_der($p_key_string) };
return $rsa2 if $rsa2;
croak "Failed to load DER key as X.509 ($x509_err) or PKCS#1 ($@)";

my $rsa = eval { $proto->_new_public_key_x509_der($p_key_string) };
return $rsa if $rsa;
return $proto->_new_public_key_pkcs1_der($p_key_string);

3. Passphrase silently ignored for DER private keys (`RSA.pm`, L49-56)

When a user calls new_private_key($der_key, $passphrase), the passphrase is silently ignored because _new_private_key_der doesn't accept one. The POD documents this limitation, but the code should either warn or croak when a passphrase is provided with a DER key to avoid user confusion.

Suggested:

elsif ( ... eq "\x30" ) {
    carp "passphrase ignored for DER-encoded keys" if @rest;
    return $proto->_new_private_key_der($p_key_string);
}

elsif ( length($p_key_string) > 0 && substr($p_key_string, 0, 1) eq "\x30" ) {
    # ASN.1 SEQUENCE tag detected — likely DER-encoded private key.
    return $proto->_new_private_key_der($p_key_string);
}

🟢 Suggestions

1. Redundant length check after guard (`RSA.pm`, L37)

The guard on line 23 already ensures length($p_key_string) > 0, making the length($p_key_string) > 0 condition on this line always true. Same applies to line 51 in new_private_key. Minor, but removing it reduces reader confusion about whether there's a code path where length could be 0 here.

elsif ( length($p_key_string) > 0 && substr($p_key_string, 0, 1) eq "\x30" ) {

2. Repeated BIO+OSSL_DECODER cleanup pattern — consider a helper (`RSA.xs`, L507-530)

The _new_public_key_pkcs1_der, _new_private_key_der, and (to a lesser extent) _new_public_key_x509_der functions share the same BIO-create → OSSL_DECODER → cleanup pattern on the 3.x path. This is 3 copies of nearly identical resource management code. Consider extracting a static helper like _load_der_key(SV* key_string_SV, const char* structure, const char* data_type, int selection) that encapsulates the BIO+OSSL_DECODER lifecycle. This would reduce the surface area for resource leak bugs.

3. No PKCS#8 DER test coverage

All private key DER tests use keys produced by get_private_key_string() which outputs PKCS#1 PEM (converted to DER). There's no test for PKCS#8 DER private keys, which is the more common format from external tools. Consider adding a test with a PKCS#8 DER key (e.g., generated via openssl genpkey -algorithm RSA -outform DER) to ensure both formats work. This would have caught the d2i_RSAPrivateKey_bio limitation on pre-3.x.

4. Duplicate test with der.t and error.t (`t/der.t`, L119-122)

The empty-string and unrecognized-PEM-header error tests overlap with t/error.t. While not harmful, this adds maintenance burden — if the error message format changes, two files need updating. Consider keeping error-path coverage in error.t and DER-specific tests in der.t.

eval { Crypt::OpenSSL::RSA->new_public_key("") };
like( $@, qr/unrecognized key format/,
    "new_public_key gives helpful error on empty string" );

---

Checklist

• No hardcoded secrets
• Input validation at boundaries
• Resource cleanup in error paths
• Error messages don't expose internals
• PKCS#8 DER format support on pre-3.x — warning quietened critic to -3, added test #1
• Silent error swallowing in fallback — warning use Crypt::OpenSSL::Guess to resolve OpenSSL include path #3
• Test coverage for all DER formats — suggestion Use the new interface to RSA_generate_key if available #6

---

Summary

Solid feature addition with good test coverage and clean XS code. Two issues should be addressed before merge: (1) _new_private_key_der uses d2i_RSAPrivateKey_bio on pre-3.x which only handles PKCS#1 format — PKCS#8 DER keys (common from Java/.NET/openssl genpkey) will fail; use d2i_PrivateKey_bio instead. (2) The DER public key fallback silently discards the first error, potentially confusing users when both attempts fail. The passphrase-ignored-for-DER issue is minor but worth a carp. Overall architecture is sound and the error messages are a nice improvement.

---

Automated review by Kōan

[Koan-Bot]

@timlegge: @Koan-Bot Review this new test and add it to the PR:

The t/openssl_der.t test is a good addition — it validates interop with actual openssl CLI-generated DER keys, which is exactly the use case this feature targets. Gating it behind AUTHOR_TESTING is the right call given that OpenSSL CLI output formatting can vary across builds.

One thing to watch: openssl genrsa -outform DER produces PKCS#1 DER. If you also want to cover PKCS#8 DER (which is what openssl genpkey -algorithm RSA -outform DER produces and is arguably more common from external toolchains), consider adding that variant too. This is especially important because the current pre-3.x XS code uses d2i_RSAPrivateKey_bio which only handles PKCS#1 — a PKCS#8 DER test would catch that gap.

[timlegge]

@Koan-Bot I do not like the eval in RSA.pm maybe something like this:

     elsif ( length($p_key_string) > 0 && substr($p_key_string, 0, 1) eq "\x30" ) {
         # ASN.1 SEQUENCE tag detected — likely DER-encoded key.
         # Try X.509 SubjectPublicKeyInfo first (most common), then PKCS#1 RSAPublicKey.
         my $unpacked = unpack("H*", $p_key_string);
         if ($unpacked =~ /06092a864886f70d010101/) {
             # Matched ASN.1 DER encoding for the RSA encryption
             # algorithm's Object Identifier (OID)
             return $proto->_new_public_key_x509_der($p_key_string);
         } elsif ($unpacked =~ /^308[12].*?02/) {
             # PKCS#1 key, the first thing inside the sequence is
             # an Integer representing the modulus
             return $proto->_new_public_key_pkcs1_der($p_key_string);
         } else {
             croak "unrecognized key format: DER-encoded daya provided "
                 . "could not be recognized as a x509 or pkcs1 format DER";
          }
     }

[Koan-Bot]

Applied both changes:

1. Replaced eval with ASN.1 OID detection — new_public_key() DER path now inspects the hex-encoded data:

• RSA OID 06092a864886f70d010101 → X.509 SubjectPublicKeyInfo
• ^308[12].*?02 (SEQUENCE + INTEGER) → PKCS#1 RSAPublicKey
• Neither → clear error message

No more silent error swallowing. Each format is identified deterministically before calling the XS function.

2. Added t/openssl_der.t — AUTHOR_TESTING test that generates keys via openssl CLI and validates DER import round-trip (26 tests).

All 560 tests pass + 26 author tests.

[timlegge]

@Koan-Bot remove the File::Slurper from openss_der.t I left in by mistake

[timlegge]

@Koan-Bot on debian bookworm the private key is:

-----BEGIN RSA PRIVATE KEY-----
-----END RSA PRIVATE KEY-----

so needs something like:

my $priv_iqmp = get_parameter($priv_output, 'coefficient', '-----BEGIN .*PRIVATE KEY-----');
my $priv_key = get_parameter($priv_output, '-----BEGIN .*PRIVATE KEY-----', '-----END .*PRIVATE KEY-----');

[timlegge]

@Koan-Bot review and integrate this diff:

--- t/openssl_der.t.orig	2026-04-04 18:39:26.298871907 -0300
+++ t/openssl_der.t	2026-04-04 19:01:34.614010263 -0300
@@ -22,9 +22,11 @@
 # Get the output as text that includes the private key PEM
 my $priv_output = `openssl rsa -inform PEM -in $rsa_file -text 2>&1`;
 
-# Get the output as text that includes the private key PEM
-my $pub_output = `openssl rsa -inform PEM -in $rsa_file -pubout -text 2>&1`;
+# PKCS1 format: Get the public key output as text
+my $pub_pkcs1_output = `openssl rsa -inform PEM -in $rsa_file -pubout -text 2>&1`;
 
+# X509 format: Get the public key output as text
+my $pub_x509_output = `openssl rsa -inform PEM -in $rsa_file -RSAPublicKey_out -text 2>&1`;
 
 # Basic grab multi-line data between
 # two tags from openssl -text output
@@ -58,6 +60,10 @@
     return $bn2->cmp($bn1);
 }
 
+####################
+# Check private key
+####################
+diag("Check private key");
 # Extract the values from the openssl private key output
 my $priv_n = get_parameter($priv_output, 'modulus', 'publicExponent');
 my $priv_e = get_parameter($priv_output, 'publicExponent', 'privateExponent');
@@ -85,16 +91,20 @@
 ok(compare_bignum_to_hex($dmq1, $priv_dmq1) == 0, "Imported DER dmq1 parameter matches expected");
 ok(compare_bignum_to_hex($iqmp, $priv_iqmp) == 0, "Imported DER iqmp parameter matches expected");
 
+#########################
+# Check pkcs1 public key
+#########################
+diag("Check pkcs1 public key");
 # Extract the public key values from the openssl public key output
-my $pub_n = get_parameter($pub_output, 'modulus', 'publicExponent');
-my $pub_e = get_parameter($pub_output, 'publicExponent', 'privateExponent');
-my $pub_d = get_parameter($pub_output, 'privateExponent', 'prime1');
-my $pub_p = get_parameter($pub_output, 'prime1', 'prime2');
-my $pub_q = get_parameter($pub_output, 'prime2', 'exponent1');
-my $pub_dmp1 = get_parameter($pub_output, 'exponent1', 'exponent2');
-my $pub_dmq1 = get_parameter($pub_output, 'exponent2', 'coefficient');
-my $pub_iqmp = get_parameter($pub_output, 'coefficient', '-----BEGIN PUBLIC KEY-----');
-my $pub_key = get_parameter($pub_output, '-----BEGIN PUBLIC KEY-----', '-----END PUBLIC KEY-----');
+my $pub_n = get_parameter($pub_pkcs1_output, 'modulus', 'publicExponent');
+my $pub_e = get_parameter($pub_pkcs1_output, 'publicExponent', 'privateExponent');
+my $pub_d = get_parameter($pub_pkcs1_output, 'privateExponent', 'prime1');
+my $pub_p = get_parameter($pub_pkcs1_output, 'prime1', 'prime2');
+my $pub_q = get_parameter($pub_pkcs1_output, 'prime2', 'exponent1');
+my $pub_dmp1 = get_parameter($pub_pkcs1_output, 'exponent1', 'exponent2');
+my $pub_dmq1 = get_parameter($pub_pkcs1_output, 'exponent2', 'coefficient');
+my $pub_iqmp = get_parameter($pub_pkcs1_output, 'coefficient', '-----BEGIN PUBLIC KEY-----');
+my $pub_key = get_parameter($pub_pkcs1_output, '-----BEGIN PUBLIC KEY-----', '-----END PUBLIC KEY-----');
 
 # Load the public key from the DER (base64 decoded PEM)
 my $pub_rsa = Crypt::OpenSSL::RSA->new_public_key(decode_base64($pub_key));
@@ -112,4 +122,35 @@
 ok(!$p_dmq1, "Imported public DER dmq1 parameter undef as expected");
 ok(!$p_iqmp, "Imported public DER iqmp parameter undef as expected");
 
+########################
+# Check x509 public key
+########################
+diag("Check x509 public key");
+# Extract the public key values from the openssl public key output
+my $pubx_n = get_parameter($pub_x509_output, 'modulus', 'publicExponent');
+my $pubx_e = get_parameter($pub_x509_output, 'publicExponent', 'privateExponent');
+my $pubx_d = get_parameter($pub_x509_output, 'privateExponent', 'prime1');
+my $pubx_p = get_parameter($pub_x509_output, 'prime1', 'prime2');
+my $pubx_q = get_parameter($pub_x509_output, 'prime2', 'exponent1');
+my $pubx_dmp1 = get_parameter($pub_x509_output, 'exponent1', 'exponent2');
+my $pubx_dmq1 = get_parameter($pub_x509_output, 'exponent2', 'coefficient');
+my $pubx_iqmp = get_parameter($pub_x509_output, 'coefficient', '-----BEGIN RSA PUBLIC KEY-----');
+my $pubx_key = get_parameter($pub_x509_output, '-----BEGIN RSA PUBLIC KEY-----', '-----END RSA PUBLIC KEY-----');
+
+# Load the public key from the DER (base64 decoded PEM)
+my $pub_x509_rsa = Crypt::OpenSSL::RSA->new_public_key(decode_base64($pubx_key));
+
+# Get the private key parameters
+my ($px_n, $px_e, $px_d, $px_p, $px_q, $px_dmp1, $px_dmq1, $px_iqmp) = $pub_x509_rsa->get_key_parameters();
+
+# Check each public key parameter to the expected values
+ok(compare_bignum_to_hex($px_n, $pubx_n) == 0, "Imported public DER n parameter matches expected");
+ok(compare_bignum_to_hex($px_e, $pubx_e) == 0, "Imported public DER e parameter matches expected");
+ok(!$px_d, "Imported public DER d parameter undef as expected");
+ok(!$px_p, "Imported public DER p parameter undef as expected");
+ok(!$px_q, "Imported public DER q parameter undef as expected");
+ok(!$px_dmp1, "Imported public DER dmp1 parameter undef as expected");
+ok(!$px_dmq1, "Imported public DER dmq1 parameter undef as expected");
+ok(!$px_iqmp, "Imported public DER iqmp parameter undef as expected");
+
 done_testing();

[timlegge]

@Koan-Bot Review and rebase from main

[Koan-Bot]

PR Review — feat: support DER-encoded public keys in new_public_key()

Solid feature addition with good test coverage and clean OpenSSL version handling. The XS code is well-structured with proper resource cleanup on error paths. Three issues need attention before merge: (1) the PKCS#1 DER detection regex is fragile for edge cases and could be simplified by letting OpenSSL handle the distinction, (2) passphrase arguments are silently ignored for DER private keys which is a user footgun, and (3) the openssl_der.t public key section attempts to parse fields that don't exist in -pubout output. The hex-based OID detection works but a raw binary search would be cleaner. None of these are critical — the core functionality is correct.

---

🟡 Important

1. Hex conversion of entire key for OID detection is wasteful (`RSA.pm`, L37-47)

Converting the entire DER key to hex with unpack("H*", ...) just to search for a 22-char OID pattern allocates a string 2x the key size. You can search the raw binary directly:

if (index($p_key_string, "\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01") >= 0) {

This is functionally equivalent (the RSA OID encoding is unique), avoids the allocation, and is arguably clearer since it directly represents the DER-encoded OID bytes.

my $hex = unpack("H*", $p_key_string);
if ($hex =~ /06092a864886f70d010101/) {

2. PKCS#1 DER fallback regex is fragile and may miss valid keys (`RSA.pm`, L44-47)

The regex ^308[12].*?02 requires a multi-byte ASN.1 length encoding (81 or 82), which means the SEQUENCE must be > 127 bytes. While this holds for 1024+ bit RSA keys, it will reject smaller test keys (e.g., 512-bit keys produce ~70 byte PKCS#1 structures with a single-byte length like 3048).

More importantly, this regex would also match X.509 DER keys that somehow lack the expected OID (unlikely but possible with non-RSA algorithms). A simpler approach: if the first branch (OID check) doesn't match, just try PKCS#1 — let OpenSSL reject truly invalid data rather than duplicating ASN.1 length parsing in Perl.

elsif ($hex =~ /^308[12].*?02/) {

3. Passphrase silently ignored for DER private keys (`RSA.pm`, L58-69)

If a user calls new_private_key($der_data, "mypassword"), the passphrase is captured in @rest but silently dropped when routing to _new_private_key_der. This is a footgun — the user believes their key is passphrase-protected when it isn't.

Consider croaking when @rest is non-empty and the key is DER:

elsif ( substr($p_key_string, 0, 1) eq "\x30" ) {
    croak "passphrase is not supported for DER-encoded private keys"
        if @rest;
    return $proto->_new_private_key_der($p_key_string);
}

This aligns with the POD which says "passphrase protection is only supported for PEM-encoded keys" — the code should enforce it.

elsif ( substr($p_key_string, 0, 1) eq "\x30" ) {
    # ASN.1 SEQUENCE tag detected — likely DER-encoded private key.
    return $proto->_new_private_key_der($p_key_string);
}

4. Public key section parses non-existent fields from -pubout output (`t/openssl_der.t`, L95-108)

The openssl rsa -pubout -text output only contains Modulus and Exponent — it does NOT contain privateExponent, prime1, prime2, exponent1, exponent2, or coefficient. The get_parameter() calls for these fields (lines 95-102) will get undef results, causing warnings and possibly incorrect test behavior.

Also, on some OpenSSL versions, the public key -text output uses Exponent rather than publicExponent, so get_parameter($pub_output, 'publicExponent', 'privateExponent') may fail to find $pub_e correctly.

Since this is AUTHOR_TESTING, it won't affect CI, but it should be fixed to avoid confusing failures for authors. The public key section should only extract and verify n and e, using the correct field names from the -pubout -text output format.

my $pub_e = get_parameter($pub_output, 'publicExponent', 'privateExponent');

🟢 Suggestions

1. Consider using CHECK_OPEN_SSL_BIO for consistency (`RSA.xs`, L507-536)

The _new_public_key_x509_der function manually calls BIO_free(bio) then CHECK_OPEN_SSL(pkey). This works correctly, but the codebase already has CHECK_OPEN_SSL_BIO (line 92) that combines both operations. Using it would be more consistent with existing patterns:

pkey = d2i_PUBKEY_bio(bio, NULL);
BIO_free(bio);
CHECK_OPEN_SSL(pkey);

Actually, since BIO_free is called unconditionally (success and failure), the current code is correct. But do note that if d2i_PUBKEY_bio puts errors on the OpenSSL error queue on success (some older OpenSSL versions do), CHECK_OPEN_SSL won't catch that. Not a real issue here, just noting.

2. Unused import: Digest::SHA (`t/openssl_der.t`, L5)

Digest::SHA qw/sha1_hex/ is imported but never used in this test file. Remove it to avoid adding an unnecessary test dependency.

use Digest::SHA     qw/sha1_hex/;

3. DER truncated error test may be brittle (`t/der.t`, L121-122)

The test new_public_key("\x30\x00") (empty ASN.1 SEQUENCE) would hit the elsif ($hex =~ /^308[12].*?02/) check — but 3000 does NOT match ^308[12], so it falls through to the else branch and croaks "unrecognized DER key format". This is correct, but the test name says "truncated DER data" when the actual error path is "doesn't match any known DER pattern". If the regex changes, this test might break in a confusing way. Consider testing with a value that actually matches one of the patterns but is truncated, to exercise the OpenSSL-level error path.

eval { Crypt::OpenSSL::RSA->new_public_key("\x30\x00") };
like( $@, qr/unrecognized DER key format/,

---

Checklist

• No hardcoded secrets
• Input validation at boundaries
• Resource cleanup on error paths (XS)
• No command injection in shell calls
• Edge case coverage — warning quietened critic to -3, added test #2 (PKCS#1 regex), warning use Crypt::OpenSSL::Guess to resolve OpenSSL include path #3 (passphrase ignored)
• Test correctness — warning Use the new interface to RSA_generate_key if available #5 (openssl_der.t pub key parsing)
• No unused imports — suggestion quietened critic to -3, added test #2 (Digest::SHA)

---

Summary

Solid feature addition with good test coverage and clean OpenSSL version handling. The XS code is well-structured with proper resource cleanup on error paths. Three issues need attention before merge: (1) the PKCS#1 DER detection regex is fragile for edge cases and could be simplified by letting OpenSSL handle the distinction, (2) passphrase arguments are silently ignored for DER private keys which is a user footgun, and (3) the openssl_der.t public key section attempts to parse fields that don't exist in -pubout output. The hex-based OID detection works but a raw binary search would be cleaner. None of these are critical — the core functionality is correct.

---

Automated review by Kōan

[Koan-Bot]

@timlegge: @Koan-Bot I do not like the eval in RSA.pm maybe something like this:

The approach you suggested was implemented — the current code uses unpack("H*", ...) + regex matching instead of eval. One further improvement: you could avoid the hex conversion entirely by searching for the raw OID bytes directly:

if (index($p_key_string, "\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01") >= 0) {
    return $proto->_new_public_key_x509_der($p_key_string);
}
else {
    return $proto->_new_public_key_pkcs1_der($p_key_string);
}

This avoids the allocation from unpack and the fragile ^308[12].*?02 regex for the PKCS#1 fallback. If the RSA OID isn't found, it's either PKCS#1 or invalid — let OpenSSL reject truly invalid data rather than pattern-matching ASN.1 lengths in Perl.

[Koan-Bot]

@timlegge: @Koan-Bot on debian bookworm the private key is:

Good catch on the Debian bookworm format difference. The fix using -----BEGIN .*PRIVATE KEY----- regex was applied. However, note that the public key section of openssl_der.t (lines 95-108) still tries to extract privateExponent, prime1, etc. from the -pubout -text output, which doesn't contain those fields. The get_parameter() calls will return undef for these fields. While the ok(!$p_d, ...) checks will still pass (undef is falsy), $pub_e = get_parameter($pub_output, 'publicExponent', 'privateExponent') may fail because -pubout -text labels it Exponent, not publicExponent. This could cause the compare_bignum_to_hex($p_e, $pub_e) test to fail on some OpenSSL versions.

[timlegge]

@Koan-Bot

1. Fix Hex conversion of entire key for OID detection is wasteful as suggested
2. Fix PKCS#1 DER fallback regex is fragile and may miss valid keys as suggested
3. Fix Public key section parses non-existent fields from -pubout output as suggested

[Koan-Bot]

PR Review — feat: support DER-encoded public keys in new_public_key()

The DER loading implementation in XS is solid — correct use of d2i_* functions for pre-3.x and OSSL_DECODER for 3.x, proper BIO/decoder cleanup on error paths, and good test coverage in t/der.t. However, the PR includes two unrelated regressions that are blocking: (1) removal of error checking on mandatory n/e params in _get_key_parameters reverts a merged fix from PR #159, and (2) deletion of t/get_key_parameters.t removes regression coverage. The is_private_key cache removal is also unrelated and introduces per-call BIGNUM allocation overhead. The Perl-level DER detection has the wasteful hex conversion and fragile PKCS#1 regex that @timlegge already flagged. Fix the two critical regressions, address the flagged Perl-level issues, and this is merge-ready.

---

🔴 Blocking

1. Regression: removes error checking on mandatory n/e params and ERR_clear_error() (`RSA.xs`, L1010-1023)

This PR reverts the fix from commit b1400ea (PR #159), which was reviewed and merged. The original code:

1. Croaked if EVP_PKEY_get_bn_param() failed for n or e (mandatory params)
2. Called ERR_clear_error() to drain errors from expected failures on public key private components

The new code removes both. If n or e extraction fails (e.g. corrupted key), the code silently continues with NULL BIGNUMs passed to cor_bn2sv(). The missing ERR_clear_error() means stale OpenSSL errors from public key queries will pollute the error queue and could cause confusing error messages in subsequent operations.

This change is also unrelated to DER support and should be reverted.

    EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_N, &n);
    EVP_PKEY_get_bn_param(rsa, OSSL_PKEY_PARAM_RSA_E, &e);

2. Test file deleted without explanation

t/get_key_parameters.t (46 lines, 10 tests) is deleted. This test validates get_key_parameters() for both private and public keys, including verifying that private components are undef for public keys on OpenSSL 3.x. The PR description doesn't mention this deletion, and it's unrelated to DER support. This removes regression coverage for the exact code path that was also modified in this PR (the _get_key_parameters XS function). Please restore this test file.

🟡 Important

1. Removing is_private_key cache causes per-call BIGNUM alloc on 3.x (`RSA.xs`, L101-124)

The PR removes the is_private_key cached field and _detect_private_key(), making _is_private() call EVP_PKEY_get_bn_param() on every invocation. On OpenSSL 3.x, this allocates a new BIGNUM + BN_clear_free() on every call. _is_private() is called from 6 different XS functions (sign, private_encrypt, get_private_key_string, etc.), so this is a measurable performance regression for repeated operations.

This change also seems unrelated to DER support — consider keeping the cache or splitting it into a separate PR.

char _is_private(rsaData* p_rsa)
{
    char ret = 0;
    ...
    EVP_PKEY_get_bn_param(p_rsa->rsa, OSSL_PKEY_PARAM_RSA_D, &d);
    ret = (d != NULL);
    BN_clear_free(d);

2. Full hex conversion of DER blob is wasteful for OID detection (`RSA.pm`, L38)

Converting the entire DER key to hex (unpack("H*", $p_key_string)) to search for a 9-byte OID is wasteful. For a 4096-bit key, this converts ~1KB of DER to ~2KB of hex. Use index() on the raw binary instead:

if (index($p_key_string, "\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01") >= 0) {

This is both faster and more readable. (@timlegge already flagged this in comment #4188086129.)

my $hex = unpack("H*", $p_key_string);
if ($hex =~ /06092a864886f70d010101/) {

3. PKCS#1 DER fallback regex is overly permissive (`RSA.pm`, L44)

The regex ^308[12].*?02 matches any DER SEQUENCE containing an INTEGER anywhere — this is true of virtually every ASN.1 structure, not just PKCS#1 RSAPublicKey. So the PKCS#1 branch is effectively a catch-all for any DER that doesn't contain the RSA OID. While OpenSSL will reject invalid data, the error message claims it's "PKCS#1 RSAPublicKey" which is misleading.

Consider: if the OID check fails, just try X.509 first and fall back to PKCS#1, letting OpenSSL do the format detection. Or simplify the regex comment to say "fallback for non-X.509 DER". (@timlegge flagged this too.)

elsif ($hex =~ /^308[12].*?02/) {
    # PKCS#1 RSAPublicKey — SEQUENCE containing INTEGER (modulus)

4. Public key parsing extracts non-existent private fields (`t/openssl_der.t`, L90-104)

Lines 97-104 try to extract privateExponent, prime1, prime2, exponent1, exponent2, coefficient from openssl rsa -pubout -text output, which only contains modulus and publicExponent. The get_parameter() regex will fail to match, $1 will be undef, and the substitution on line 43 ($parameter =~ s/[:\s]//g) will emit 'uninitialized value' warnings.

For the public key section, only extract n and e. The undef checks on lines 109-113 can use the Crypt::OpenSSL::RSA get_key_parameters() directly instead of parsing openssl output. (@timlegge already flagged this in comment #4188086129.)

my $pub_e = get_parameter($pub_output, 'publicExponent', 'privateExponent');
my $pub_d = get_parameter($pub_output, 'privateExponent', 'prime1');

🟢 Suggestions

1. Duplicated error message string across 3 locations (`RSA.pm`, L21-23)

The error message "unrecognized key format: expected PEM-encoded key (starting with '-----BEGIN') or DER-encoded key (binary ASN.1 data)" is duplicated verbatim in 3 places in new_public_key and 2 in new_private_key. Consider extracting to a constant:

my $UNRECOGNIZED_KEY_MSG = "unrecognized key format: expected PEM-encoded key ..."

2. Passphrase silently ignored for DER private keys (`RSA.pm`, L56-60)

If a user calls new_private_key($der_data, $passphrase), the passphrase in @rest is silently dropped when the DER path is taken. The POD notes this, but a carp warning when @rest is non-empty on the DER path would prevent user confusion:

carp "passphrase ignored for DER-encoded keys" if @rest;

return $proto->_new_private_key_der($p_key_string);

---

Checklist

• No hardcoded secrets
• Input validation at boundaries
• Resource cleanup in error paths
• No unrelated regressions — critical quietened critic to -3, added test #1, critical quietened critic to -3, added test #2
• Test coverage for new code paths
• No unnecessary performance regressions — warning quietened critic to -3, added test #1

---

Summary

The DER loading implementation in XS is solid — correct use of d2i_* functions for pre-3.x and OSSL_DECODER for 3.x, proper BIO/decoder cleanup on error paths, and good test coverage in t/der.t. However, the PR includes two unrelated regressions that are blocking: (1) removal of error checking on mandatory n/e params in _get_key_parameters reverts a merged fix from PR #159, and (2) deletion of t/get_key_parameters.t removes regression coverage. The is_private_key cache removal is also unrelated and introduces per-call BIGNUM allocation overhead. The Perl-level DER detection has the wasteful hex conversion and fragile PKCS#1 regex that @timlegge already flagged. Fix the two critical regressions, address the flagged Perl-level issues, and this is merge-ready.

---

Automated review by Kōan

[Koan-Bot]

@timlegge: @Koan-Bot

All three issues @timlegge raised are still present in the current diff:

1. Hex conversion: unpack("H*", $p_key_string) converts the entire DER blob. Use index($p_key_string, "\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01") instead — it searches the raw bytes directly, is faster, and avoids the allocation.

2. PKCS#1 regex: ^308[12].*?02 matches virtually any DER-encoded ASN.1 structure. A simpler approach: if the RSA OID isn't found, try PKCS#1 as a fallback and let OpenSSL validate. No regex needed — just else { return $proto->_new_public_key_pkcs1_der($p_key_string) }.

3. Public key field parsing in t/openssl_der.t: Lines 97-104 still extract privateExponent, prime1, etc. from -pubout -text output which doesn't contain them. get_parameter() will return undef and $parameter =~ s/[:\s]//g will warn. For the public key section, only extract modulus and publicExponent, and verify private components via get_key_parameters() returning undef.

[Koan-Bot]

Applied all 3 requested fixes:

1. Hex conversion → raw binary search — unpack("H*") + regex replaced with index($p_key_string, "\x06\x09\x2a\x86\x48...") on the raw bytes. No allocation, no conversion.

2. PKCS#1 regex removed — The fragile ^308[12].*?02 pattern is gone. If no RSA OID is found, we assume PKCS#1 and let OpenSSL reject invalid data. This handles all key sizes correctly.

3. openssl_der.t public key parsing fixed — No longer extracts private-key fields from -pubout output. Public key params (n, e) are verified against the already-extracted private key values (same key). Added a PKCS#1 section using -RSAPublicKey_out (with SKIP guard for older OpenSSL). Removed unused Digest::SHA import.

560 tests + 36 author tests pass.

[timlegge]

Looks great