Skip to content

Update API to support OpenSSL v3 while keeping support for older API#43

Closed
timlegge wants to merge 27 commits into
cpan-authors:masterfrom
timlegge:openssl3-tim
Closed

Update API to support OpenSSL v3 while keeping support for older API#43
timlegge wants to merge 27 commits into
cpan-authors:masterfrom
timlegge:openssl3-tim

Conversation

@timlegge
Copy link
Copy Markdown
Member

@timlegge timlegge commented May 4, 2024

This is very much a work in progress and is more changes that I would like. I suspect I can further reduce the changes as I clean up. I would welcome assistance if anyone would like to send a pull request against https://github.com/timlegge/Crypt-OpenSSL-RSA/tree/openssl3-tim

ToDo:

  • More cleanups and fix the checks in the modules style
  • Fix the two remaining deprecated functions MD5 and RIPEMD160
  • Clean tests from the current github actions
  • Add additional OSs on the github actions
  • Actually verify the OPENSSL Versions - Likely just go with OPENSSL_VERSION_NUMBER >= 0x30000000L
  • Figure out the remaining test failures
  • - bignum.t - older openssl seemed to fill in missing values when a key was generated from data values
  • - format - opensslv3 public key format is not PKCS1 format
  • Performance testing
  • Create some compatibility tests - test values generated by old API that can be validated on the new API
  • other things

Issues:

  1. Need to be able to verify that data generated by the old module can be reproduced/validated by the new version
  2. Performance needs to be tested as openSSL 3 uses multiple calls to do things that the older version did. I chose not to keet the EVP_PKEY_CTX around which might be a performance mistake (but easily rectified) and OpenSSL does talk about digests and their impact on performance so having some tests would likely help

@timlegge timlegge changed the title DRAFT: Intial squashed work in progress DRAFT: Update API to support OpenSSL v3 while keeping support for older API May 4, 2024
@timlegge
Copy link
Copy Markdown
Member Author

timlegge commented May 4, 2024

The padding issues I saw are related to some openssl restrictions on padding:

https://www.openssl.org/docs/manmaster/man3/EVP_PKEY_CTX_set_rsa_padding.html states:

EVP_PKEY_CTX_set_rsa_padding() sets the RSA padding mode for ctx. The pad parameter can take the value RSA_PKCS1_PADDING for PKCS#1 padding, RSA_NO_PADDING for no padding, RSA_PKCS1_OAEP_PADDING for OAEP padding (encrypt and decrypt only), RSA_X931_PADDING for X9.31 padding (signature operations only), RSA_PKCS1_PSS_PADDING (sign and verify only) and RSA_PKCS1_WITH_TLS_PADDING for TLS RSA ClientKeyExchange message padding (decryption only).

Two RSA padding modes behave differently if EVP_PKEY_CTX_set_signature_md() is used. If this function is called for PKCS#1 padding the plaintext buffer is an actual digest value and is encapsulated in a DigestInfo structure according to PKCS#1 when signing and this structure is expected (and stripped off) when verifying. If this control is not used with RSA and PKCS#1 padding then the supplied data is used directly and not encapsulated. In the case of X9.31 padding for RSA the algorithm identifier byte is added or checked and removed if this control is called. If it is not called then the first byte of the plaintext buffer is expected to be the algorithm identifier byte.

The following patch would cause the code to croak if the an unsupported padding is requested. This does however break the existing tests.

diff --git a/RSA.xs b/RSA.xs
index 1cbf068..8a9fb70 100644
--- a/RSA.xs
+++ b/RSA.xs
@@ -1007,8 +1007,7 @@ sign(p_rsa, text_SV)
     ctx = EVP_PKEY_CTX_new(p_rsa->rsa, NULL /* no engine */);
     CHECK_OPEN_SSL(ctx);
     CHECK_OPEN_SSL(EVP_PKEY_sign_init(ctx));
-    /* FIXME: Issue setting padding in some cases */
-    EVP_PKEY_CTX_set_rsa_padding(ctx, p_rsa->padding);
+    CHECK_OPEN_SSL(EVP_PKEY_CTX_set_rsa_padding(ctx, p_rsa->padding));
 
     EVP_MD* md = get_md_bynid(p_rsa->hashMode);
     CHECK_OPEN_SSL(md != NULL);
@@ -1063,8 +1062,7 @@ PPCODE:
     ctx = EVP_PKEY_CTX_new(p_rsa->rsa, NULL /* no engine */);
     CHECK_OPEN_SSL(ctx);
     CHECK_OPEN_SSL(EVP_PKEY_verify_init(ctx) == 1);
-    /* FIXME: Issue setting padding in some cases */
-    EVP_PKEY_CTX_set_rsa_padding(ctx, p_rsa->padding);
+    CHECK_OPEN_SSL(EVP_PKEY_CTX_set_rsa_padding(ctx, p_rsa->padding));
 
     EVP_MD* md = get_md_bynid(p_rsa->hashMode);
     CHECK_OPEN_SSL(md != NULL);
diff --git a/t/rsa.t b/t/rsa.t
index d3e7f0b..676baa9 100644
--- a/t/rsa.t
+++ b/t/rsa.t
@@ -119,6 +119,8 @@ _check_for_croak(
 $plaintext .= $plaintext x 5;
 
 # check signature algorithms
+$rsa->use_pkcs1_padding();
+$rsa_pub->use_pkcs1_padding();
 $rsa->use_md5_hash();
 $rsa_pub->use_md5_hash();
 _Test_Sign_And_Verify( $plaintext, $rsa, $rsa_pub );

@timlegge timlegge force-pushed the openssl3-tim branch 2 times, most recently from 7ec10f0 to 5995094 Compare May 5, 2024 21:56
@toddr
Copy link
Copy Markdown
Member

toddr commented May 6, 2024

Thanks for your submission. I'll take a look soon.

@timlegge
Copy link
Copy Markdown
Member Author

timlegge commented May 6, 2024

Thanks for your submission. I'll take a look soon.

Thanks. I will leave the DRAFT in the title for now. Code-wise its in pretty good shape - well it works :-). Commit wise I may squash some of the commits when I look at it again tonight or tomorrow.

If you were receptive to a bump in version of ExtUtils::ParseXS to 3.51 it would improve the no blank lines between #if and #endif nonsense that is really annoying to look at and find.

I am working on some tests that will have base64 encoded encrypted values that can be compared across versions to ensure that everything works the same as in the past.

I think this needs some performance tests as well so I will see what I can do for it.

Happy to make changes as you see fit.

@toddr
Copy link
Copy Markdown
Member

toddr commented May 6, 2024

If you were receptive to a bump in version of ExtUtils::ParseXS to 3.51 it would improve the no blank lines between #if and #endif nonsense that is really annoying to look at and find.

I don't see that that modules is a dependency. Should it be?

@timlegge
Copy link
Copy Markdown
Member Author

timlegge commented May 6, 2024 via email

Comment thread RSA.xs Outdated
@toddr toddr marked this pull request as draft May 30, 2024 23:24
@toddr toddr changed the title DRAFT: Update API to support OpenSSL v3 while keeping support for older API Update API to support OpenSSL v3 while keeping support for older API May 30, 2024
@timlegge
Copy link
Copy Markdown
Member Author

timlegge commented Jul 1, 2024

@toddr I have been busy on dsully/perl-crypt-openssl-pkcs12#44 but I should be able to get back to this this week.

I believe I wrote the compatibility tests already and did some initial performance tests. OpenSSL 3.x is a little slower than older versions but did not seem significant.

@toddr
Copy link
Copy Markdown
Member

toddr commented Jul 1, 2024

thanks!

@timlegge timlegge force-pushed the openssl3-tim branch 2 times, most recently from b828200 to 5250805 Compare July 4, 2024 23:43
@timlegge
Copy link
Copy Markdown
Member Author

Back again... I was stuck in a loop testing/troubleshooting on HPUX - API calls that worked fine everywhere else failed there. It came down to a Crypt::OpenSSL::Random issue where it was linked to SSL 1.x and Crypt::OpenSSL::RSA was linked to OpenSSL 3.x. Since t/rsa.t loads Crypt::OpenSSL::Random before Crypt::OpenSSL::RSA it seemed to interfere with the calls. I sent a PR to Crypt::OpenSSL::Random with assistance from @Tux to link properly to the default openssl ssl and crypto libraries. If anyone else runs into it, load Crypt::OpenSSL::RSA first in the test and it should work fine.

Okay, back to work on cleaning up this PR

@toddr
Copy link
Copy Markdown
Member

toddr commented May 2, 2025

Replaced by #53

@toddr toddr closed this May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants