rsa_crypt no longer needs public/private arguement

[timlegge]

move the max message length check and call it from the correct functions to simplify rsa_crypt

Things were getting complicated in rsa_crypt so I pulled the message length check out to check_max_message_length() and it is not called only from encrypt and private_encrypt (sign) .

That simplifies rsa_crypt.

In addition, the reason for the public argument was eliminated in 85fce04

@Koan-Bot and @toddr-bot review

[Koan-Bot]

Review — PR #165

Good refactor. The public parameter was only used for the public == is_encrypt length-check guard after 85fce04 removed its other use. Moving the check to the callers (encrypt, private_encrypt) makes rsa_crypt simpler and the call sites self-documenting. The equivalence is correct:

Call	Old: public == is_encrypt	New: explicit call
encrypt	1==1 → check ✓	check_max_message_length() called ✓
decrypt	0==1 → skip ✓	not called ✓
private_encrypt	0==0 → check ✓	check_max_message_length() called ✓
public_decrypt	1==0 → skip ✓	not called ✓

Issues

1. C89 mixed declarations — will break on older compilers (blocking)

void check_max_message_length(rsaData* p_rsa, int from_length) {
    int size;
    /* comment */
    size = EVP_PKEY_get_size(p_rsa->rsa);  // ← this is a statement
    int max_len = -1;                       // ← declaration after statement = C99
    const char *pad_name = NULL;            // ← same

Many Perl builds use C89 mode. All declarations must come before any statements:

void check_max_message_length(rsaData* p_rsa, int from_length) {
    int size;
    int max_len = -1;
    const char *pad_name = NULL;

    size = EVP_PKEY_get_size(p_rsa->rsa);
    /* ... */

2. SvCUR doesn't guarantee stringification (important)

check_max_message_length(p_rsa, SvCUR(p_plaintext));

SvCUR() returns the string length of the SV's PV slot, but if the SV hasn't been stringified yet (e.g., it's an integer), SvCUR may return 0 or garbage. The original code used SvPV(p_from, from_length) inside rsa_crypt() which forces stringification.

Two options:

• Use SvPV_nolen(p_plaintext) before SvCUR to force stringification, or
• Pass the length returned by SvPV (but that would mean duplicating the SvPV call)

In practice, Perl crypto data is almost always already a string, but the safety guarantee matters for an XS module.

3. from_length type should be STRLEN, not int

SvCUR returns STRLEN (which is size_t). The function parameter is int, which silently truncates on 64-bit. The original code had the same limitation (casting to (int) for the comparison), but since we're touching the signature anyway, STRLEN would be more correct.

Nits

• Commit message: "arguement" → "argument"
• The comment inside check_max_message_length() about "Only applies to encryption direction" is now redundant — the function is only called from encrypt/private_encrypt, so it's self-documenting. Could simplify to just the overhead calculation comments.

Summary

The refactor is sound. Fix the C89 declarations and the SvCUR issue, and this is good to merge.

[toddr-bot]

PR Review — rsa_crypt no longer needs public/private arguement

Sound refactor — the public parameter removal is correct and the call-site equivalence is verified (encrypt/private_encrypt get the check, decrypt/public_decrypt don't). Two blocking/important issues need fixing before merge: (1) C89 mixed declarations in check_max_message_length will fail on older compilers and MSVC, and (2) SvCUR on potentially non-stringified SVs is unsafe — use sv_len() or force stringification first. The STRLEN-vs-int type mismatch in the new function signature should also be addressed while the signature is being defined. After those fixes, this is ready to merge.

---

🔴 Blocking

1. C89 mixed declarations will break older Perl/compiler builds (`RSA.xs`, L392-397)

The int max_len and const char *pad_name declarations appear after the size = EVP_PKEY_get_size(...) statement. This is C99 syntax. Many Perl builds (especially on older platforms and MSVC) compile XS code in C89 mode, where all declarations must precede statements. Move all declarations to the top of the function:

void check_max_message_length(rsaData* p_rsa, int from_length) {
    int size;
    int max_len = -1;
    const char *pad_name = NULL;

    size = EVP_PKEY_get_size(p_rsa->rsa);
    ...

    size = EVP_PKEY_get_size(p_rsa->rsa);
    int max_len = -1;
    const char *pad_name = NULL;

🟡 Important

1. Function parameter type should be STRLEN, not int (`RSA.xs`, L392)

from_length is int but callers pass the result of SvCUR() which returns STRLEN (size_t). On 64-bit systems this silently truncates. Since you're defining a new function signature, use the correct type:

void check_max_message_length(rsaData* p_rsa, STRLEN from_length) {

This also removes the need for the (int)from_length cast in the comparison — you'd compare from_length > (STRLEN)max_len (after verifying max_len >= 0).

void check_max_message_length(rsaData* p_rsa, int from_length) {

2. SvCUR on potentially non-stringified SV (`RSA.xs`, L1139)

SvCUR(p_plaintext) returns the cached string length from the PV slot. If the SV has never been stringified (e.g., it was created as an IV), SvCUR can return 0 or stale data. The original code inside rsa_crypt used SvPV(p_from, from_length) which forces stringification.

Safest fix: call SvPV_nolen(p_plaintext) before using SvCUR, or pass sv_len(p_plaintext) instead (which handles all SV types correctly):

check_max_message_length(p_rsa, sv_len(p_plaintext));

sv_len() is the safe Perl API for getting SV length regardless of internal representation.

    check_max_message_length(p_rsa, SvCUR(p_plaintext));

🟢 Suggestions

1. Function should be declared static (`RSA.xs`, L392-415)

check_max_message_length is an internal helper with file-local scope. It should be declared static to avoid polluting the symbol namespace and to match the convention of other helpers in this file (e.g., _write_pkcs8_pem).

static void check_max_message_length(rsaData* p_rsa, int from_length) {

void check_max_message_length(rsaData* p_rsa, int from_length) {

2. Comment about encryption direction is now redundant (`RSA.xs`, L393-397)

The block comment explaining this only applies to encrypt/private_encrypt is no longer needed — the function is only called from those two XS functions, making it self-documenting. Consider removing or shortening to just document the overhead constants.

    /* Pre-validate plaintext length before calling OpenSSL.
       Only applies to encryption direction (encrypt, private_encrypt),
       not to decryption (decrypt, public_decrypt) where input is ciphertext. */

---

Checklist

• No hardcoded secrets
• Memory management correctness
• C standard compliance — critical quietened critic to -3, added test #1
• Input validation at boundaries — warning quietened critic to -3, added test #1, warning quietened critic to -3, added test #2
• Symbol visibility — suggestion quietened critic to -3, added test #1

---

Summary

Sound refactor — the public parameter removal is correct and the call-site equivalence is verified (encrypt/private_encrypt get the check, decrypt/public_decrypt don't). Two blocking/important issues need fixing before merge: (1) C89 mixed declarations in check_max_message_length will fail on older compilers and MSVC, and (2) SvCUR on potentially non-stringified SVs is unsafe — use sv_len() or force stringification first. The STRLEN-vs-int type mismatch in the new function signature should also be addressed while the signature is being defined. After those fixes, this is ready to merge.

---

Automated review by Kōan

[timlegge]

@Koan-Bot and @toddr-bot review the changes now. I still need to address a couple of issues but check the details of the oaep max length calculation

[Koan-Bot]

Follow-up review — OAEP max length calculation

The formula size - (2 * digest_length) - 2 is correct per RFC 8017 §7.1.1 (RSAES-OAEP). Good fix — the old hardcoded size - 42 assumed SHA-1 as the OAEP hash function, which broke when the default moved to SHA-256.

However, the current implementation has a compilation-breaking issue on pre-3.x OpenSSL, plus a memory leak.

🔴 Blocking: get_md_bynid() doesn't exist on pre-3.x — won't compile

check_max_message_length() is defined outside the #if OPENSSL_VERSION_NUMBER >= 0x30000000L guard (line ~392), but it calls get_md_bynid() which is only defined inside that guard (line ~207). This means on any OpenSSL < 3.0 build, the compiler will error on the undefined function.

Fix: Use get_digest_length(p_rsa->hashMode) instead. It's defined unconditionally (line 168), returns the digest length as an int directly from compile-time constants, and requires no allocation or cleanup:

if (p_rsa->padding == RSA_PKCS1_OAEP_PADDING) {
    digest_length = get_digest_length(p_rsa->hashMode);
    max_len = size - (2 * digest_length) - 2;
    pad_name = "OAEP";
}

This also eliminates the need for the EVP_MD *md_obj variable entirely.

🟡 Memory leak: fetched EVP_MD never freed (3.x only)

On OpenSSL 3.x, get_md_bynid() calls EVP_MD_fetch() which allocates a new EVP_MD object. The code never calls EVP_MD_free(md_obj) — it leaks on every call to check_max_message_length. This is moot if you switch to get_digest_length() as suggested above.

🟡 Stale comment

max_len = size - (2 * digest_length) - 2;  /* 2 * SHA1_DIGEST_LENGTH + 2 */

The code now uses the actual hash's digest length, not SHA-1's. The comment should say /* OAEP overhead: 2*hLen + 2 */ or similar.

Previous review items still open

• SvCUR(p_plaintext) — still unsafe for non-stringified SVs. Use sv_len(p_plaintext).
• from_length should be STRLEN, not int.
• Function should be static.

Test nit: t/rsa.t line 83

my $oaep_max = $rsa->size - (2 * $hash_size) - 2;

Should be $rsa->size() with parentheses — consistent with every other call in the file.

t/padding.t: hash filter is narrow

if ( $padding eq 'pkcs1_oaep' && $hash =~ /sha/ ) {

This only applies the hash-aware max_len for SHA variants. For MD5 with OAEP, the correct overhead is 2*16+2 = 34, but the test would still use the old $size - $pad (42). It won't fail (testing under the limit still works), but it's not testing the actual boundary. Consider applying the digest-aware formula for all OAEP hashes unconditionally.

Summary

The OAEP formula is correct. The main issue is using get_md_bynid() instead of get_digest_length() — the latter works on all OpenSSL versions, needs no allocation, and is the right tool for the job here. Switching to it fixes the compilation, leak, and variable clutter in one move.

[timlegge]

@Koan-Bot implement the get_digest_length() fix and fix the minor nit and the previous review items

[Koan-Bot]

Draft PR submitted: #166

[toddr-bot]

PR Review — rsa_crypt no longer needs public/private arguement

The refactor direction is sound — removing the public parameter and hoisting the length check to the call sites makes the code clearer. However, there are three blocking issues that must be fixed before merge: (1) check_max_message_length() calls get_md_bynid() which only exists on OpenSSL 3.x, so pre-3.x builds will fail to compile; (2) the EVP_MD* returned by get_md_bynid() (which calls EVP_MD_fetch()) is never freed, leaking memory on every encrypt/private_encrypt call; (3) C89 mixed declarations will break on older compilers and MSVC. The OAEP formula change from hardcoded size - 42 to size - (2 * digest_length) - 2 is correct per RFC 8017 and properly handles non-SHA1 hash modes. The SvCUR safety and STRLEN type issues should also be addressed while the signature is new.

---

🔴 Blocking

1. get_md_bynid() is 3.x-only — won't compile on pre-3.x OpenSSL (`RSA.xs`, L392-420)

check_max_message_length() is defined outside any #if guard but calls get_md_bynid(), which is only defined inside #if OPENSSL_VERSION_NUMBER >= 0x30000000L (line 205). This will produce a linker/compiler error on every pre-3.x build.

Options:

1. Add an #if/#else inside the function: on 3.x use get_md_bynid() + EVP_MD_size(), on pre-3.x use a simple switch on p_rsa->hashMode mapping NIDs to known digest sizes (e.g., NID_sha256 → 32).
2. Extract a small get_digest_length(int hashMode) helper that compiles on all versions and call that instead.

md_obj = get_md_bynid(p_rsa->hashMode);\ndigest_length = EVP_MD_size(md_obj);

2. EVP_MD memory leak from get_md_bynid() (`RSA.xs`, L404-406)

get_md_bynid() calls EVP_MD_fetch() which allocates a new EVP_MD*. The returned md_obj is used for EVP_MD_size() but never freed with EVP_MD_free(). Every call to encrypt() or private_encrypt() leaks an EVP_MD object.

Fix: add EVP_MD_free(md_obj); after the EVP_MD_size() call (or, better, avoid the allocation entirely by using a NID-to-size lookup table that works on all OpenSSL versions).

md_obj = get_md_bynid(p_rsa->hashMode);\ndigest_length = EVP_MD_size(md_obj);\n/* missing: EVP_MD_free(md_obj); */

3. C89 mixed declarations will break older compilers (`RSA.xs`, L392-397)

The EVP_MD *md_obj and int digest_length declarations appear after the size = EVP_PKEY_get_size(...) statement. Many Perl builds (MSVC, older gcc in C89 mode) reject declarations after statements. Move all declarations to the top of the function before any executable statements:

void check_max_message_length(rsaData* p_rsa, int from_length) {
    int size;
    int max_len = -1;
    const char *pad_name = NULL;
    int digest_length = 0;

    size = EVP_PKEY_get_size(p_rsa->rsa);
    ...

void check_max_message_length(rsaData* p_rsa, int from_length) {\n    int size;\n    int max_len = -1;\n    const char *pad_name = NULL;\n    EVP_MD *md_obj;\n    int digest_length = 0;\n    /* Pre-validate... */\n    size = EVP_PKEY_get_size(p_rsa->rsa);

🟡 Important

1. Parameter type should be STRLEN, not int (`RSA.xs`, L392)

from_length is int but callers pass the result of SvCUR() which returns STRLEN (size_t). On 64-bit systems this silently truncates. Since this is a new function signature, use the correct type:

void check_max_message_length(rsaData* p_rsa, STRLEN from_length) {

Then adjust the comparison: if (max_len >= 0 && from_length > (STRLEN)max_len).

void check_max_message_length(rsaData* p_rsa, int from_length) {

2. SvCUR on potentially non-stringified SV is unsafe (`RSA.xs`, L1137)

SvCUR(p_plaintext) returns the cached PV string length. If the SV hasn't been stringified (e.g., passed as an integer), SvCUR may return 0 or stale data. Use sv_len(p_plaintext) which handles all SV types correctly, or call SvPV_nolen(p_plaintext) first to force stringification.

Same issue at line 1183 (private_encrypt).

check_max_message_length(p_rsa, SvCUR(p_plaintext));

🟢 Suggestions

1. Function should be declared static (`RSA.xs`, L392-420)

check_max_message_length() is only used within RSA.xs. Declare it static to avoid polluting the symbol table and potential linker conflicts:

static void check_max_message_length(rsaData* p_rsa, STRLEN from_length) {

void check_max_message_length(rsaData* p_rsa, int from_length) {

2. Stale comment says SHA-1 but calculation uses SHA-256 (`t/error.t`, L89-90)

Line 90 has # SHA-1 OAEP overhead but the calculation uses $hash_size = 32 (SHA-256). Should be # SHA-256 OAEP overhead.

my $hash_size = 32;           # SHA256 (default is 32 bytes)\nmy $max_oaep = $rsa->size() - (2 * $hash_size) - 2;  # SHA-1 OAEP overhead

---

Checklist

• No hardcoded secrets
• Memory management correctness — critical quietened critic to -3, added test #2 — EVP_MD leak
• Cross-platform compilation — critical quietened critic to -3, added test #1 (pre-3.x) and use Crypt::OpenSSL::Guess to resolve OpenSSL include path #3 (C89)
• Input validation at boundaries — warning quietened critic to -3, added test #2 — SvCUR safety
• Type safety — warning quietened critic to -3, added test #1 — STRLEN vs int
• Test coverage for changes
• Test comments match code — suggestion use Crypt::OpenSSL::Guess to resolve OpenSSL include path #3 — stale SHA-1 comment

---

Summary

The refactor direction is sound — removing the public parameter and hoisting the length check to the call sites makes the code clearer. However, there are three blocking issues that must be fixed before merge: (1) check_max_message_length() calls get_md_bynid() which only exists on OpenSSL 3.x, so pre-3.x builds will fail to compile; (2) the EVP_MD* returned by get_md_bynid() (which calls EVP_MD_fetch()) is never freed, leaking memory on every encrypt/private_encrypt call; (3) C89 mixed declarations will break on older compilers and MSVC. The OAEP formula change from hardcoded size - 42 to size - (2 * digest_length) - 2 is correct per RFC 8017 and properly handles non-SHA1 hash modes. The SvCUR safety and STRLEN type issues should also be addressed while the signature is new.

---

Automated review by Kōan

[toddr-bot]

@timlegge: @Koan-Bot and @toddr-bot review the changes now. I still need to address a couple of issues but che...

The OAEP max length formula size - (2 * digest_length) - 2 is correct per RFC 8017 §7.1.1. The old hardcoded size - 42 assumed SHA-1 (digest_length=20, so 2*20+2=42), which was wrong once the default hash moved to SHA-256.

However, the implementation has two blocking issues with the digest length calculation:

1. get_md_bynid() is 3.x-only — it's defined inside #if OPENSSL_VERSION_NUMBER >= 0x30000000L but check_max_message_length() is outside any version guard. Pre-3.x builds will fail to link.

2. Memory leak — get_md_bynid() calls EVP_MD_fetch() which allocates. The returned EVP_MD* is never freed with EVP_MD_free().

Recommended fix: create a simple get_digest_length(int hashMode) helper that uses a switch statement mapping NIDs to known sizes (NID_sha1→20, NID_sha256→32, etc.). This works on all OpenSSL versions, avoids allocation, and avoids the leak. The values are constants defined by the hash specifications — no need to query OpenSSL at runtime for them.

[toddr-bot]

@timlegge: @Koan-Bot implement the get_digest_length() fix and fix the minor nit and the previous review items

Note for the implementation: when creating the get_digest_length() fix, make sure to also address the other items from both reviews in the same pass:

• static declaration for check_max_message_length()
• STRLEN parameter type instead of int
• sv_len() instead of SvCUR() at the call sites
• C89 declaration ordering (all declarations before statements)
• Fix the stale "SHA-1" comment in t/error.t line 90

[Koan-Bot]

❌ Permission denied. Only users with write access can trigger bot commands.

[timlegge]

@Koan-Bot @toddr-bot review

[toddr-bot]

PR Review — rsa_crypt no longer needs public/private arguement

The public parameter removal and call-site restructuring are correct — the refactor logic is sound. However, the OAEP overhead calculation in check_max_message_length() has a fundamental correctness bug: it uses p_rsa->hashMode (which controls signing hash) to compute OAEP overhead, but this module never sets the OAEP hash on the EVP_PKEY_CTX — OpenSSL always uses SHA-1 for OAEP. This makes the pre-validation check too strict for SHA-256+ (default) and too lenient for MD5. The test changes compound the problem by computing wrong boundary values, and t/error.t likely regresses (expects failure at 191 bytes but OpenSSL allows up to 214). There's also a C89 declaration-after-statement violation in the pre-3.x code path. Fix the OAEP overhead to use the fixed SHA-1 value (or implement actual OAEP hash selection), fix the C89 issue, and revert the test OAEP calculations.

---

🔴 Blocking

1. OAEP overhead uses hashMode but OAEP always uses SHA-1 (`RSA.xs`, L392-415)

The new check_max_message_length() computes OAEP overhead as 2 * get_digest_length(p_rsa->hashMode) + 2, but this module never sets the OAEP hash on the EVP_PKEY_CTX — there is no EVP_PKEY_CTX_set_rsa_oaep_md() call anywhere in rsa_crypt(). Both OpenSSL 3.x (default) and pre-3.x (RSA_public_encrypt with RSA_PKCS1_OAEP_PADDING) hardcode SHA-1 as the OAEP hash function.

This means hashMode (which controls signing/verification) has no bearing on OAEP overhead. The calculation is wrong in two directions:

• Too strict for SHA-256 (default): check allows size - 66, but OpenSSL accepts up to size - 42. Users encrypting data between 191–214 bytes with a 2048-bit key will get a spurious pre-validation error.
• Too lenient for MD5 (hLen=16): check allows size - 34, but OpenSSL rejects anything over size - 42. The pre-validation passes but OpenSSL fails — exactly what the pre-validation is supposed to prevent.

Either:

1. Keep using the hardcoded SHA-1 overhead (size - 42) since this module doesn't support OAEP hash selection, or
2. Actually implement OAEP hash selection by adding EVP_PKEY_CTX_set_rsa_oaep_md() in the 3.x encrypt path, and then use get_digest_length(hashMode) in the check.

Option 1 is simpler and matches the pre-existing behavior.

if (p_rsa->padding == RSA_PKCS1_OAEP_PADDING) {
    int digest_length = 0;
    digest_length = get_digest_length(p_rsa->hashMode);
    max_len = size - (2 * digest_length) - 2;
    pad_name = "OAEP";
}

2. C89 violation: declaration after statements in pre-3.x path (`RSA.xs`, L475-476)

In the #else (pre-3.x) code path, int size is declared after several statements (SvPV, the is_encrypt croak check). When only the #else branch is compiled, the function body has statements before this declaration, which is invalid C89. Many Perl builds (especially MSVC and older platforms) compile XS in C89 mode.

Fix: keep int size; in the top-level declarations of rsa_crypt() (where it was before), and just assign it in the #else block:

// At top of function:
int size;

// In #else block:
size = EVP_PKEY_get_size(p_rsa->rsa);

Or use it only where needed: the 3.x path doesn't reference size anymore, so you could guard the declaration with #if/#else at the declaration block, but that's more complex.

#else
    int size = EVP_PKEY_get_size(p_rsa->rsa);
    CHECK_NEW(to, size, UNSIGNED_CHAR);

🟡 Important

1. MD5 digest length is wrong; OAEP overhead values are incorrect (`t/padding.t`, L120-127)

MD5_DIGEST_LENGTH is 16, not 20. The comment "OpenSSL seems to require a minimum of 20" is masking the real issue: OAEP in this module always uses SHA-1 (digest length 20) regardless of the hash mode setting, so ALL entries should use 20 (or more precisely, the tests should use the fixed overhead of 42 as before).

Since hashMode doesn't affect OAEP overhead (see RSA.xs review comment), computing per-hash OAEP overhead is incorrect. The old hardcoded 'pad' => 42 was the correct value for all hashes because OAEP always uses SHA-1.

my %digest_lengths = (
    'md5'       => 20,  # OpenSSL seems to require a minimum of 20
    'sha1'      => 20,
    'sha224'     => 28,
    'sha256'     => 32,
    'sha384'     => 48,
    'sha512'     => 64
);

2. OAEP max calculation assumes hashMode affects OAEP (`t/crypto.t`, L18-22)

The new calculation $key_size - (2 * $hash_size) - 2 with $hash_size = 32 (SHA-256) gives oaep_max = 190, but the actual OAEP max with SHA-1 (which is what OpenSSL uses) is 256 - 42 = 214. The test still passes because 190 < 214, but it no longer tests the actual boundary — it tests 24 bytes below the boundary.

The old $key_size - 42 was correct. Keep it, or better, add a comment explaining that OAEP always uses SHA-1 in this module regardless of hashMode.

my $hash_size = 32;           # SHA-256 (default hash mode, 32 bytes)
my $oaep_max = $key_size - (2 * $hash_size) - 2;   # OAEP overhead: 2*hLen + 2

3. Same OAEP max miscalculation as crypto.t (`t/error.t`, L88-90)

Same issue: $rsa->size() - (2 * 32) - 2 = 190, but actual OAEP max is 214. The boundary test at line 95–98 ("exact max should work") encrypts 190 bytes, which is well below the real limit. The "too large" test at line 91 encrypts 191 bytes which also succeeds with OpenSSL (since real max is 214), so the test expectation ok($@, ...) would fail — OpenSSL won't reject 191 bytes.

This is a test regression: the old $rsa->size() - 42 correctly matched the OAEP boundary.

my $hash_size = 32;
my $max_oaep = $rsa->size() - (2 * $hash_size) - 2;

---

Checklist

• No hardcoded secrets
• Input validation at boundaries — critical quietened critic to -3, added test #1 — OAEP overhead mismatch
• C89/portability compliance — critical quietened critic to -3, added test #2 — declaration after statement
• Test boundary correctness — warning use Crypt::OpenSSL::Guess to resolve OpenSSL include path #3, Format code-like text elements in POD as code #4, Use the new interface to RSA_generate_key if available #5 — OAEP max values wrong
• Memory management (no leaks)
• Error handling cleanup paths

---

Summary

The public parameter removal and call-site restructuring are correct — the refactor logic is sound. However, the OAEP overhead calculation in check_max_message_length() has a fundamental correctness bug: it uses p_rsa->hashMode (which controls signing hash) to compute OAEP overhead, but this module never sets the OAEP hash on the EVP_PKEY_CTX — OpenSSL always uses SHA-1 for OAEP. This makes the pre-validation check too strict for SHA-256+ (default) and too lenient for MD5. The test changes compound the problem by computing wrong boundary values, and t/error.t likely regresses (expects failure at 191 bytes but OpenSSL allows up to 214). There's also a C89 declaration-after-statement violation in the pre-3.x code path. Fix the OAEP overhead to use the fixed SHA-1 value (or implement actual OAEP hash selection), fix the C89 issue, and revert the test OAEP calculations.

---

Automated review by Kōan

[toddr-bot]

@timlegge: @Koan-Bot @toddr-bot review

Reviewed. The previous review items (C89 declarations, sv_len(), static, STRLEN type) appear to be addressed in the current diff. However, there's a new critical issue: the OAEP overhead calculation in check_max_message_length() uses get_digest_length(p_rsa->hashMode) but this module never calls EVP_PKEY_CTX_set_rsa_oaep_md() — OAEP always uses SHA-1 internally regardless of hashMode. The old hardcoded size - 42 was actually correct. See detailed comments on RSA.xs and the test files.

[toddr-bot]

@timlegge: @Koan-Bot and @toddr-bot review the changes now. I still need to address a couple of issues but che...

The OAEP max length formula size - (2 * hLen) - 2 is correct per RFC 8017 §7.1.1 — that part is right. The problem is which hLen to use. In this module, hashMode controls the signing/verification hash, but OAEP encryption always uses SHA-1 because there's no EVP_PKEY_CTX_set_rsa_oaep_md() call in the encrypt path (and pre-3.x RSA_public_encrypt with RSA_PKCS1_OAEP_PADDING hardcodes SHA-1). So get_digest_length(p_rsa->hashMode) gives the wrong value — it should use SHA_DIGEST_LENGTH (20) for OAEP, giving the same size - 42 as before.

The test changes in crypto.t, error.t, and padding.t that use SHA-256's digest length (32) compute oaep_max = size - 66, which is 24 bytes below the actual OAEP limit. The t/error.t boundary test will likely regress because it expects OpenSSL to reject 191 bytes, but OpenSSL allows up to 214.

To make hashMode actually control OAEP, you'd need to add EVP_PKEY_CTX_set_rsa_oaep_md() in the 3.x encrypt path — but that's a feature addition, not part of this refactor.

[Koan-Bot]

❌ Permission denied. Only users with write access can trigger bot commands.

[timlegge]

@toddr-bot implement EVP_PKEY_CTX_set_rsa_oaep_md in a separate PR

[timlegge]

@Koan-Bot @toddr-bot final review

[toddr-bot]

Final review — PR #165

Previous review items are addressed: static declaration ✓, STRLEN parameter ✓, sv_len() at call sites ✓, C89 declarations in check_max_message_length() ✓, OAEP uses correct hardcoded size - 42 ✓. The public parameter removal equivalence is correct across all four call sites.

One blocking issue remains, plus one concern about the test changes.

---

🔴 Blocking

C89 declaration-after-statement in pre-3.x path (RSA.xs L476)

#else
    int size = EVP_PKEY_get_size(p_rsa->rsa);   /* ← C89 violation */
    CHECK_NEW(to, size, UNSIGNED_CHAR);

On pre-3.x builds, the compiler sees this declaration after the from = SvPV(...) and if(is_encrypt ...) statements earlier in rsa_crypt(). This is C99 syntax — strict C89 compilers (MSVC, older GCC) will reject it.

Fix: Add int size; to the top declarations block (around L427), guarded if desired:

#if OPENSSL_VERSION_NUMBER < 0x30000000L
    int size;
#endif

Then change L476 to just size = EVP_PKEY_get_size(p_rsa->rsa);.

(Note: the pre-existing C89 violations in the 3.x path — ctx, error, crypt_pad at L445-447 — are not introduced by this PR.)

---

🟡 Test deletion concern

The t/format.t changes delete the EC key and RSA-PSS key rejection tests (plan drops from 56 to 48). These tests were added in PR #141 to cover the EVP_PKEY_get_base_id() guard at L397, which prevents silent misuse of non-RSA keys on OpenSSL 3.x.

The runtime guard code is still present in this diff, but without tests it has no regression coverage. These tests are unrelated to the rsa_crypt refactor — was the deletion intentional? If so, why?

---

✅ Verified correct

Item	Status
public parameter removal equivalence	✓ All 4 call sites match old behavior
check_max_message_length() — static, C89 decls, STRLEN	✓
sv_len() instead of SvCUR()	✓
OAEP overhead hardcoded to SHA-1 (size - 42)	✓ Correct since no EVP_PKEY_CTX_set_rsa_oaep_md()
No check on decrypt/public_decrypt paths	✓ Input is ciphertext

[Koan-Bot]

Post-merge review — Kōan

The refactor is clean. Verified on macOS/OpenSSL 3.6.1 — builds and all tests pass.

What's good

• public parameter removal is correct. After 85fce04, is_encrypt alone determines padding logic. The public == is_encrypt plaintext-direction check is now at the call sites (encrypt, private_encrypt), which is clearer.
• C89 compliance is fine. The int size; declaration at L443-445 is inside #if < 0x30000000L, at the top of the function block before any statements. The final review concern about C89 violation was a false positive.
• check_max_message_length() is well-structured — C89 declarations at top, proper STRLEN parameter type, static linkage. Review feedback was addressed.
• OAEP size - 42 hardcoded correctly since this module never calls EVP_PKEY_CTX_set_rsa_oaep_md() — OAEP always uses SHA-1 internally.

Minor observations (non-blocking, for future reference)

1. private_encrypt with OAEP padding: check_max_message_length() will compute OAEP overhead (size - 42) and pass the check, but rsa_crypt(is_encrypt=0) won't force OAEP — on 3.x, EVP_PKEY_sign with OAEP will fail with an OpenSSL error. The pre-validation gives misleading feedback in this edge case. Low priority since the operation fails either way.

2. (int)from_length truncation in croak(): STRLEN is size_t (64-bit on LP64). The %d format + cast could truncate theoretically. Not a real concern since RSA plaintext is always tiny, but (int)from_length could be (unsigned long)from_length with %lu for pedantic correctness.

No blocking issues. 👍

🤖 Post-merge review by Kōan