add support for PEM Parameters without ASN1 hooks #1831

samuel40791765 · 2024-09-06T00:28:31Z

This reverts part of commit 32fdc51.

Issues:

Addresses CryptoAlg-2546

Description of changes:

commit 32fdc51 removed the function pointers for param_decode and param_encode and removed PEM_read_bio_Parameters and PEM_write_bio_Parameters along with it (which was the only API consuming these hooks). This reverts part of the commit to add these back.

Call-outs:

1. This is in draft for the time being until we have proper support for EVP_PKEY_DH ASN1 serialization.
2. Do we even need function pointers if only 3 types of EVP_PKEYs consume these? Considering if we can just pull the logic into the API instead of adding a function pointer for each EVP_PKEY.

Ultimately decided to leave the ASN1 logic in PEM_read/write_bio_Parameters which allows us to continue decoupling the pem logic from |EVP_PKEY|. These correspond to the historical param_decode and param_encode EVP_PKEY_ASN1_METHOD hooks in OpenSSL.

Testing:

Round trip serde tests for each possible EVP_PKEY type
Sample test DH Parameter file from Ruby 3.1: https://github.com/ruby/ruby/blob/ruby_3_1/ext/openssl/lib/openssl/ssl.rb#L33-L50

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 · 2024-09-06T00:50:50Z

Codecov Report

Attention: Patch coverage is 75.49020% with 25 lines in your changes missing coverage. Please review.

Project coverage is 78.52%. Comparing base (e4092fb) to head (d4637d1).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
crypto/pem/pem_pkey.c	62.12%	25 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1831      +/-   ##
==========================================
+ Coverage   78.47%   78.52%   +0.04%     
==========================================
  Files         583      583              
  Lines       98480    98601     +121     
  Branches    14125    14137      +12     
==========================================
+ Hits        77287    77425     +138     
+ Misses      20564    20549      -15     
+ Partials      629      627       -2

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

crypto/pem/pem_pkey.c

samuel40791765

Not sure if the original comments still apply when this was in "Draft" and unfinished. The logic has changed considerably since then.
I think we can get away with not implementing the EVP_PKEY_DH ASN1 hooks with this method going forward, feel free to leave any comments again if the original ones still apply.

justsmth · 2024-09-10T17:49:17Z

crypto/pem/pem_lib.c

+
  // Permit older strings



NP: The remainder of this function does not have any test coverage.

I'm not sure if the remainder are related to PEM functions we're implementing for this PR. This function is a generic function for parsing the first line of PEM files and the rest below apply to other supported formats.

justsmth · 2024-09-10T18:03:06Z

crypto/pem/pem_pkey.c

+  char *nm = NULL;
+  unsigned char *data = NULL;
+  long len;
+  if (!PEM_bytes_read_bio(&data, &len, &nm, PEM_STRING_PARAMETERS, bio, 0,


NP: We have zero documentation for PEM_bytes_read_bio.

Added documentation

crypto/pem/pem_pkey.c

justsmth · 2024-09-10T18:22:23Z

include/openssl/pem.h

+// |EVP_PKEY|. If |*pkey| is non-null, the original |*pkey| is freed and the
+// returned |EVP_PKEY| is also written to |*pkey|. This is only supported with


I know it's implied from this statement, but it would be good to explicitly say that *pkey must be initialized (either to NULL or to an allocated value) prior to being passed in. (Passing in an uninitialized pointer would be UB.)

Added clarification

justsmth · 2024-09-10T18:27:32Z

crypto/pem/pem_pkey.c

+int PEM_write_bio_Parameters(BIO *bio, EVP_PKEY *pkey) {
+  if (bio == NULL || pkey == NULL || pkey->ameth == NULL) {


Why the check for pkey->ameth? Should this be a check for pkey->type?

Good point, this is a remnant of the original way OpenSSL had implemented these. I tried checking for pkey->type, but it's an int and not a pointer.
The switch statement below should help take care of that.

@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

justsmth reviewed Sep 6, 2024

View reviewed changes

crypto/pem/pem_pkey.c Outdated Show resolved Hide resolved

justsmth reviewed Sep 6, 2024

View reviewed changes

crypto/pem/pem_pkey.c Outdated Show resolved Hide resolved

crypto/pem/pem_pkey.c Outdated Show resolved Hide resolved

samuel40791765 force-pushed the pem-read-bio branch 2 times, most recently from b11cf54 to fb39681 Compare September 9, 2024 22:57

samuel40791765 changed the title ~~Revert "Remove param_decode and param_encode EVP_PKEY hooks."~~ add support for PEM Parameters without ASN1 hooks Sep 9, 2024

samuel40791765 force-pushed the pem-read-bio branch 2 times, most recently from 435cb6d to e5ab72d Compare September 9, 2024 23:08

add support for PEM Parameters without ASN1 hooks

2618c46

samuel40791765 force-pushed the pem-read-bio branch from e5ab72d to 2618c46 Compare September 9, 2024 23:09

samuel40791765 commented Sep 9, 2024

View reviewed changes

samuel40791765 marked this pull request as ready for review September 9, 2024 23:11

samuel40791765 requested a review from a team as a code owner September 9, 2024 23:11

justsmth reviewed Sep 10, 2024

View reviewed changes

PR comments; documentation, checks, and fix CFI CI

d4637d1

samuel40791765 requested a review from skmcgrail September 10, 2024 23:05

skmcgrail approved these changes Sep 12, 2024

View reviewed changes

justsmth approved these changes Sep 12, 2024

View reviewed changes

samuel40791765 merged commit 95ec546 into aws:main Sep 12, 2024
108 of 109 checks passed

samuel40791765 deleted the pem-read-bio branch September 12, 2024 18:34

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

add support for PEM Parameters without ASN1 hooks #1831

add support for PEM Parameters without ASN1 hooks #1831

samuel40791765 commented Sep 6, 2024 •

edited

Loading

codecov-commenter commented Sep 6, 2024 •

edited

Loading

samuel40791765 left a comment

justsmth Sep 10, 2024

samuel40791765 Sep 10, 2024

justsmth Sep 10, 2024

samuel40791765 Sep 10, 2024

justsmth Sep 10, 2024

samuel40791765 Sep 10, 2024

justsmth Sep 10, 2024

samuel40791765 Sep 10, 2024

		// \|EVP_PKEY\|. If \|pkey\| is non-null, the original \|pkey\| is freed and the
		// returned \|EVP_PKEY\| is also written to \|*pkey\|. This is only supported with

		int PEM_write_bio_Parameters(BIO bio, EVP_PKEY pkey) {
		if (bio == NULL \|\| pkey == NULL \|\| pkey->ameth == NULL) {

add support for PEM Parameters without ASN1 hooks #1831

add support for PEM Parameters without ASN1 hooks #1831

Conversation

samuel40791765 commented Sep 6, 2024 • edited Loading

Issues:

Description of changes:

Call-outs:

Testing:

codecov-commenter commented Sep 6, 2024 • edited Loading

Codecov Report

samuel40791765 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel40791765 commented Sep 6, 2024 •

edited

Loading

codecov-commenter commented Sep 6, 2024 •

edited

Loading