Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for EVP_PKEY_CTX callback functions #1905

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

samuel40791765
Copy link
Contributor

Issues:

Resolves CryptoAlg-1716
Resolves CryptoAlg-2698

Description of changes:

We tried to no-op these functions, but it turns out Ruby depends on them pretty extensively as the interruption mechanism for threads. One of Ruby's tests depends on EVP_PKEY_CTX_get_app_data to return an actual value from the callback function, but we return NULL as a no-op. Ruby seems to depend on the EVP_PKEY callback function and relevant application data to correctly handle interruptions. Based on the relevant commit messages, the expectation is that the operation is interrupted, but AWS-LC continues resuming the operation and returns a generated RSA key. It looks like we may have to consider implementing functionality for these callback functions. This issue also applies to a test failure in test/openssl/test_pkey_dh.rb and test/openssl/test_pkey_dsa.rb. We probably aren't going to support DSA, but this will need to be applied to DH somewhere down the line.

Call-outs:

N/A

Testing:

new test that verifies this works with EVP_PKEY_RSA

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.51%. Comparing base (078ca2d) to head (8094218).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
crypto/fipsmodule/evp/evp_ctx.c 87.50% 3 Missing ⚠️
crypto/fipsmodule/evp/p_rsa.c 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1905      +/-   ##
==========================================
+ Coverage   78.47%   78.51%   +0.03%     
==========================================
  Files         585      585              
  Lines       99518    99681     +163     
  Branches    14243    14263      +20     
==========================================
+ Hits        78101    78260     +159     
- Misses      20780    20785       +5     
+ Partials      637      636       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 697 to +701
int EVP_PKEY_CTX_get_keygen_info(EVP_PKEY_CTX *ctx, int idx) {
// No-op
return 0;
GUARD_PTR(ctx);
if (idx == -1) {
return EVP_PKEY_CTX_KEYGEN_INFO_COUNT;
}
Copy link
Contributor

@justsmth justsmth Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Should this return 0 if ctx->operation != EVP_PKEY_OP_KEYGEN && ctx->operation != EVP_PKEY_OP_PARAMGEN?
  • EVP_PKEY_CTX_KEYGEN_INFO_COUNT is the maximum possible, but might the actual number of indices available differ by the type and the operation? Should there be a comment here relating to that possibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Sure, I think that makes sense. I initially tried returning 0 if the key type was not RSA or DH, but that didn't work because there was the possibility where EVP_PKEY_CTX did not have a set type yet. This should be a good way to enforce something similar.
  2. My interpretation of the array and how it's used in OpenSSL is that it's only used to map to the arguments available in BN_GENCB. Each operation that uses this only has 2 available arguments to configure, so I thought it was safe to lock this as 2.
    I did document some parts of this in EVP_PKEY_CTX_get_keygen_info, but forget to mention how I came to the conclusion for this variable. Will add!

bssl::UniquePtr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, nullptr));
ASSERT_TRUE(ctx);

// Check the initial values of |ctx->keygen_info|.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the keygen operation be initialized (EVP_PKEY_keygen_init) prior to this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial intention for the check here was to see if the new EVP_PKEY_CTX fields had been initialized correctly upon creating a new EVP_PKEY_CTX.
So the fields should have a readable value, regardless of EVP_PKEY_keygen_init was called.

@samuel40791765 samuel40791765 merged commit c64ec41 into aws:main Oct 7, 2024
114 of 115 checks passed
@samuel40791765 samuel40791765 deleted the evp-callback branch October 7, 2024 23:28
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Oct 22, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Oct 29, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Oct 30, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 1, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 4, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 6, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 16, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 18, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 18, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 25, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 25, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Nov 26, 2024
We tried to no-op these functions, but it turns out Ruby depends on them
pretty extensively as the interruption mechanism for threads. One of
Ruby's tests depends on `EVP_PKEY_CTX_get_app_data` to return an
actual value from the callback function, but we return NULL as a no-op. Ruby
seems to depend on the `EVP_PKEY` callback function and relevant
application data to correctly handle interruptions. Based on the relevant
commit messages, the expectation is that the operation is interrupted, but
AWS-LC continues resuming the operation and returns a generated RSA key.
It looks like we may have to consider implementing functionality for
these callback functions. This issue also applies to a test failure in
`test/openssl/test_pkey_dh.rb` and `test/openssl/test_pkey_dsa.rb`. We
probably aren't going to support DSA, but this will need to be applied
to DH somewhere down the line.

* Commits:
* ruby/openssl@88b90fb
* ruby/ruby@d3507e3

new test that verifies this works with `EVP_PKEY_RSA`

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
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.

4 participants