Refactor RSA_METHOD and expand API #1790

smittals2 · 2024-08-22T00:04:30Z

Issues

CryptoAlg-2482

Description of changes:

This PR is the last in a series of three (prev PRs: 1st, 2nd) to refactor and expand ENGINE, EC_KEY_METHOD and RSA_METHOD. This PR:

Expands the RSA_METHOD struct and changes the function signatures for encrypt, decrypt, sign_raw, and verify_raw to match OpenSSL 1.1.1.
Makes the struct definition for RSA_METHOD internal. This means static allocation of this struct is no longer possible.
Adds various functions to get/set, create, free, and manipulate RSA_METHOD

Callout:

This PR includes the commits for the other 2 PRs. They should be reviewed and merged before this one.

Testing:

Tested with various fields initialized in RSA_METHOD to ensure that custom functionality is correctly invoked.

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.

…support *group_order_size custom functionality anymore

codecov-commenter · 2024-08-22T00:27:56Z

Codecov Report

Attention: Patch coverage is 77.64706% with 38 lines in your changes missing coverage. Please review.

Project coverage is 78.52%. Comparing base (b925242) to head (b9a609f).
Report is 43 commits behind head on main.

Files with missing lines	Patch %	Lines
crypto/fipsmodule/rsa/rsa.c	66.66%	30 Missing ⚠️
crypto/rsa_extra/rsa_crypt.c	71.42%	4 Missing ⚠️
crypto/engine/engine.c	66.66%	2 Missing ⚠️
crypto/fipsmodule/rsa/rsa_impl.c	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1790      +/-   ##
==========================================
+ Coverage   78.41%   78.52%   +0.11%     
==========================================
  Files         582      585       +3     
  Lines       97573    98482     +909     
  Branches    13990    14126     +136     
==========================================
+ Hits        76510    77332     +822     
- Misses      20441    20524      +83     
- Partials      622      626       +4

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

### Description of changes: 1. Breaking changes to the ENGINE API - made the function signatures match OpenSSL. Removed method_size as a parameter since the method can no longer be statically allocated. 2. Got rid of old code supporting reference counting for METHOD structs as it never existed in OpenSSL and was no-ops/unused in BoringSSL 3. Removed static allocation enforcement for METHOD structs. Now consumers can only dynamically allocate these structs via funcs like EC_KEY_METHOD_new and RSA_METHOD_new which are added [here](#1785) and [here](#1790). ### Call-outs: First PR in a series of 3 to refactor and expand support for ENGINE, RSA_METHOD, and EC_KEY_METHOD structs ### Testing: No significant tests can be written yet. The memory model will be tested further in the PRs that follow. 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.

…ntegrity failure

crypto/fipsmodule/rsa/rsa.c

crypto/fipsmodule/rsa/internal.h

crypto/fipsmodule/rsa/rsa.c

include/openssl/rsa.h

tests/ci/integration/run_openssh_integration.sh

tests/ci/integration/openvpn_patch/aws-lc-openvpn2-6-x.patch

crypto/fipsmodule/ec/internal.h

crypto/fipsmodule/rsa/internal.h

include/openssl/rsa.h

crypto/fipsmodule/rsa/internal.h

crypto/rsa_extra/rsa_crypt.c

include/openssl/rsa.h

tests/ci/integration/run_openssh_integration.sh

crypto/fipsmodule/rsa/rsa.c

crypto/engine/engine.c

include/openssl/engine.h

include/openssl/rsa.h

crypto/fipsmodule/rsa/rsa.c

nebeid

Were the patch files removed?

include/openssl/rsa.h

smittals2 · 2024-09-05T18:36:05Z

Were the patch files removed?

No we still need some patches to work with OpenVPN

crypto/fipsmodule/rsa/internal.h

crypto/fipsmodule/rsa/rsa.c

nebeid · 2024-09-06T16:33:11Z

include/openssl/rsa.h

 OPENSSL_EXPORT int RSA_meth_set0_app_data(RSA_METHOD *meth, void *app_data);


+// RSA_meth_set_sign sets |sign| on |meth|. The function |sign| should return


Do we need more documentation for |sign|? Sould we say that type is a hash_nid and it should create a PKCS1 prefix as in the usage justsmth pointed to https://github.com/aws/aws-lc/blame/612c200ca1ff0f4068fcd47d4869035d16e08bb0/crypto/fipsmodule/rsa/rsa.c#L818? I didn't dive into the implementation to provide an accurate documentation, tbh.

This sign functionality is provided by the consumer. So the implementation details are upto them. We document the default sign functionality :)

include/openssl/rsa.h

@andrewhop

## What's Changed * Use OPENSSL_STATIC_ASSERT which handles all the platform/compiler/C s… by @andrewhop in #1791 * ML-KEM refactor by @dkostic in #1763 * ML-KEM-IPD to ML-KEM as defined in FIPS 203 by @dkostic in #1796 * Add KDA OneStep testing to ACVP by @skmcgrail in #1792 * Updating erroneous documentation for BIO_get_mem_data and subsequent usage by @smittals2 in #1752 * No-op impls for several EVP_PKEY_CTX functions by @justsmth in #1759 * Drop "ipd" suffix from ML-KEM related code by @dkostic in #1797 * Upstream merge 2024 08 19 by @skmcgrail in #1781 * ML-KEM move to the FIPS module by @dkostic in #1802 * Reduce collision probability for variable names by @torben-hansen in #1804 * Refactor ENGINE API and memory around METHOD structs by @smittals2 in #1776 * bn: Move x86-64 argument-based dispatching of bn_mul_mont to C. by @justsmth in #1795 * Check at runtime that the tool is loading the same libcrypto it was built with by @andrewhop in #1716 * Avoid matching prefixes of a symbol as arm registers by @torben-hansen in #1807 * Add CI for FreeBSD by @justsmth in #1787 * Move curve25519 implementations to fips module except spake25519 by @torben-hansen in #1809 * Add CAST for SP 800-56Cr2 One-Step function by @skmcgrail in #1803 * Remove custom PKCS7 ASN1 functions, add new structs by @WillChilds-Klein in #1726 * NASM use default debug format by @justsmth in #1747 * Add KDF in counter mode ACVP Testing by @skmcgrail in #1810 * add support for OCSP_request_verify by @samuel40791765 in #1778 * Fix GitHub/CodeBuild Purge Lambda by @justsmth in #1808 * KBKDF_ctr_hmac FIPS Service Indicator by @skmcgrail in #1798 * Update x509 tool to write all output to common BIO which is a file or stdout by @andrewhop in #1800 * Add ML-KEM to speed.cc, bump AWSLC_API_VERSION to 30 by @andrewhop in #1817 * Add EVP_PKEY_asn1_* functions by @justsmth in #1751 * Improve portability of CI integration script by @torben-hansen in #1815 * Upstream merge 2024 08 23 by @justsmth in #1799 * Replace ECDSA_METHOD with EC_KEY_METHOD and add the associated API by @smittals2 in #1785 * Cherrypick "Add some barebones support for DH in EVP" by @samuel40791765 in #1813 * Add KDA OneStep (SSKDF_digest and SSKDF_hmac) to FIPS indicator by @skmcgrail in #1793 * Add EVP_Digest one-shot test XOFs by @WillChilds-Klein in #1820 * Wire-up ACVP Testing for SHA3 Signatures with RSA by @skmcgrail in #1805 * Make SHA3 (not SHAKE) Approved for EVP_DigestSign/Verify, RSA and ECDSA. by @nebeid in #1821 * Begin tracking RelWithDebInfo library statistics by @andrewhop in #1822 * Move EVP ed25519 function table under FIPS module by @torben-hansen in #1826 * Avoid C11 Atomics on Windows by @justsmth in #1824 * Improve pre-sandbox setup by @torben-hansen in #1825 * Add OCSP round trip integration test with minor fixes by @samuel40791765 in #1811 * Add various PKCS7 getters and setters by @WillChilds-Klein in #1780 * Run clang-format on pkcs7 code by @WillChilds-Klein in #1830 * Move KEM API and ML-KEM definitions to FIPS module by @torben-hansen in #1828 * fix socat integration CI by @samuel40791765 in #1833 * Retire out-of-module KEM folder by @torben-hansen in #1832 * Refactor RSA_METHOD and expand API by @smittals2 in #1790 * Update benchmark documentation in tool/readme.md by @andrewhop in #1812 * Pre jail unit test by @torben-hansen in #1835 * Move EVP KEM implementation to in-module and correct OID by @torben-hansen in #1838 * More minor symbols Ruby depends on by @samuel40791765 in #1837 * ED25519 Power-on Self Test / CAST / KAT by @skmcgrail in #1834 * ACVP ML-KEM testing by @skmcgrail in #1840 * ACVP ECDSA SHA3 Digest Testing by @skmcgrail in #1819 * ML-KEM Service Indicator for EVP_PKEY_keygen, EVP_PKEY_encapsulate, EVP_PKEY_decapsulate by @skmcgrail in #1844 * Add ML-KEM CAST for KeyGen, Encaps, and Decaps by @skmcgrail in #1846 * ED25519 Service Indicator by @skmcgrail in #1829 * Update Allowed RSA KeySize Generation to FIPS 186-5 specification by @skmcgrail in #1823 * Add ED25519 ACVP Testing by @skmcgrail in #1818 * Make EDDSA/Ed25519 POST lazy initalized by @skmcgrail in #1848 * add support for PEM Parameters without ASN1 hooks by @samuel40791765 in #1831 * Add OpenVPN tip of main to CI by @smittals2 in #1843 * Ensure SSE2 is enabled when using optimized assembly for 32-bit x86 by @graebm in #1841 * Add support for `EVP_PKEY_CTX_ctrl_str` - Step #1 by @justsmth in #1842 * Added SHA3/SHAKE XOF functionality by @jakemas in #1839 * Migrated ML-KEM SHA3/SHAKE usage to fipsmodule by @jakemas in #1851 * AVX-512 support for RSA Signing by @pittma in #1273

smittals2 and others added 15 commits August 19, 2024 00:17

refactor engine API and memory around METHOD structs

9cb198e

added check in RSA_free and removed old comment

9c1b425

updated documentation

d9ce001

removed ECDSA_METHOD and first batch of EC_KEY_METHOD replacement

534e93e

updated documentation for EC_KEY_METHOD api

76633ab

refactored EC_KEY_is_opaque and updated documentation

b458ac2

refactored ECDSA_size as a result of removing ECDSA_METHOD. We don't …

e236789

…support *group_order_size custom functionality anymore

changed param type from unsigned int to int to match OpenSSL

ca46765

added function to set flags on EC_KEY_METHOD

0630da8

changed func sig

9b17db0

test for EC_KEY_METHOD

0686e44

added static default eckey meth

43a7315

cleaned up comments and code, changed static allocation

3429da0

added more assertions

f7fe3b4

Merge branch 'main' into eckeymethod

8a68735

changed default init for EC_KEY_METHOD for deterministic FIPS hash

9bda4bb

smittals2 force-pushed the rsaMethod branch from bb6ee46 to 4f6f77f Compare August 22, 2024 00:40

smittals2 added 3 commits August 21, 2024 18:34

fixed memory leak in test

2e2d6f3

removed cast for string

3ce0ec8

using ASSERT_STREQ for string literals instead of comapring pointers

f885475

smittals2 force-pushed the rsaMethod branch from 4f6f77f to 9c24285 Compare August 22, 2024 02:28

smittals2 mentioned this pull request Aug 22, 2024

Refactor ENGINE API and memory around METHOD structs #1776

Merged

updated patch file to account for sign_setup

fca2567

smittals2 force-pushed the rsaMethod branch from b3283bc to f8a3036 Compare August 23, 2024 00:27

smittals2 marked this pull request as ready for review August 26, 2024 07:40

smittals2 requested a review from a team as a code owner August 26, 2024 07:40

justsmth self-requested a review August 26, 2024 14:42

Merge branch 'main' into eckeymethod

83236f9

smittals2 added 6 commits August 29, 2024 10:34

moved default RSA_METHOD declaration and used macro to prevent FIPS i…

cc74cba

…ntegrity failure

changed str checks

6fc62a8

refactored ENGINE funcs and updated documentation

afd0062

updated return values

d21e674

updated rsa_method test with checks for out_len

ed46cde

updated comments and changed openssh integration script

bf49206

smittals2 force-pushed the rsaMethod branch from 9384489 to bf49206 Compare August 29, 2024 17:34

justsmth reviewed Aug 30, 2024

View reviewed changes

smittals2 and others added 3 commits August 30, 2024 10:39

Merge branch 'main' into rsaMethod

30791a8

updated patch file to enable mgmt interface with openvpn

76e6454

update documentation and added null checks

612c200

justsmth reviewed Sep 4, 2024

View reviewed changes

nebeid reviewed Sep 4, 2024

View reviewed changes

nebeid reviewed Sep 5, 2024

View reviewed changes

include/openssl/rsa.h Outdated Show resolved Hide resolved

removed additional space

c286ad7

updated documentation and added set_sign functionality

dd6cc75

justsmth previously approved these changes Sep 5, 2024

View reviewed changes

crypto/fipsmodule/rsa/internal.h Show resolved Hide resolved

crypto/fipsmodule/rsa/rsa.c Show resolved Hide resolved

crypto/fipsmodule/rsa/rsa.c Show resolved Hide resolved

nebeid reviewed Sep 6, 2024

View reviewed changes

added engine testing

685de40

smittals2 dismissed justsmth’s stale review via 685de40 September 6, 2024 17:07

updated documentation

b87e274

smittals2 requested review from nebeid and justsmth September 6, 2024 18:18

updated documentation for custom operations

b9a609f

nebeid approved these changes Sep 9, 2024

View reviewed changes

justsmth approved these changes Sep 9, 2024

View reviewed changes

smittals2 merged commit f506b4b into aws:main Sep 9, 2024
107 of 108 checks passed

smittals2 mentioned this pull request Sep 15, 2024

prepare for v1.35.0 release #1853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor RSA_METHOD and expand API #1790

Refactor RSA_METHOD and expand API #1790

smittals2 commented Aug 22, 2024 •

edited

Loading

codecov-commenter commented Aug 22, 2024 •

edited

Loading

nebeid left a comment

smittals2 commented Sep 5, 2024 •

edited

Loading

nebeid Sep 6, 2024

smittals2 Sep 6, 2024

		OPENSSL_EXPORT int RSA_meth_set0_app_data(RSA_METHOD meth, void app_data);


		// RSA_meth_set_sign sets \|sign\| on \|meth\|. The function \|sign\| should return

Refactor RSA_METHOD and expand API #1790

Refactor RSA_METHOD and expand API #1790

Conversation

smittals2 commented Aug 22, 2024 • edited Loading

Issues

Description of changes:

Callout:

Testing:

codecov-commenter commented Aug 22, 2024 • edited Loading

Codecov Report

nebeid left a comment

Choose a reason for hiding this comment

smittals2 commented Sep 5, 2024 • edited Loading

nebeid Sep 6, 2024

Choose a reason for hiding this comment

smittals2 Sep 6, 2024

Choose a reason for hiding this comment

smittals2 commented Aug 22, 2024 •

edited

Loading

codecov-commenter commented Aug 22, 2024 •

edited

Loading

smittals2 commented Sep 5, 2024 •

edited

Loading