-
Notifications
You must be signed in to change notification settings - Fork 122
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 various PKCS7 getters and setters #1780
Add various PKCS7 getters and setters #1780
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1780 +/- ##
==========================================
+ Coverage 78.40% 78.45% +0.04%
==========================================
Files 584 584
Lines 97777 98170 +393
Branches 14020 14087 +67
==========================================
+ Hits 76666 77016 +350
- Misses 20488 20530 +42
- Partials 623 624 +1 ☔ View full report in Codecov by Sentry. |
### Issues: Resolves #CryptoAlg-2493 ### Description of changes: This PR is the first in a series implementing Ruby's supported subset of the PKCS7 standard. To do this, we remove AWS-LC's customized ASN.1 serialization logic and delegate to `asn1.h` and `asn1t.h`. We also update the various `PKCS_type_is_*` no-op functions to actually check the input's type. The PKCS7 tests currently contain a test signedData structure that is encoded using BER. Because our `ASN1_get_object` currently [disallows](https://github.com/aws/aws-lc/blob/8080ce32b118746475c718d14a609bd1325166e1/crypto/asn1/asn1_lib.c#L136) indefinite-length BER, we need to modify `d2i_PKCS7` to detect and convert BER (if present) into DER before parsing. This allows us to retain backwards compatibility with BER-encoded PKCS7 objects. Please see the PR description on GitHub for more discussion on this topic. The next PR in this series will implement the various getters, setters, and allocation functions needed to support CRruby with testing for each. Please see PR #1780 for an idea of what that will look like.
dced1da
to
067a81f
Compare
067a81f
to
6748594
Compare
6748594
to
ca6af86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- A lot of the PKCS7 setters/getters seem to only apply to applicable or relevant types. Should we add which specific types are relevant in the function documentation?
- Nit: There are some remnants of OpenSSL's comment style in
crypto/pkcs7/pkcs7.c
that we should switch up. It'd also help to runclang-format
on the file since the brackets and spacing are a bit inconsistent.
ca6af86
to
1eb1ed9
Compare
sure, i can do that.
that's reasonable, although I'd like to do it in a separate PR that only applies |
1eb1ed9
to
a9056f1
Compare
a9056f1
to
a7723f0
Compare
ASSERT_TRUE(p7_dup); | ||
EXPECT_TRUE(PKCS7_type_is_signed(p7_dup.get())); | ||
|
||
p7_der = kPKCS7SignedWithSignerInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: is this necessary, seems like the pointer value hasn't changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you asking about the PKCS7_dup
call on L1207? if so, that's meant to exercise PKCS7_dup
and assert that a.) PKCS7_dup
duplicates the object without memory leaks and b.) that the duplicated object is of the expected content type.
if asking about the p7_der
assignment on L1211, then yes, and nice catch :) it's intentional. d2i_*
functions will advance the referenced input pointer during parsing (see comment here). this is why we assign a local pointer to the static address and reset it for relevant tests. OpenSSL documents the generic behavior in this type-specific man page.
d2i_TYPE() attempts to decode len bytes at *ppin. If successful a pointer to the TYPE structure is returned and *ppin is incremented to the byte following the parsed data.
thanks for the call-out, this needs to be documented in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes I was asking about p7_der
on L1211. It seemed like the d2i
functions did some pointer advancements, but the const
pointer made me question myself if anything had actually changed. Thanks for the clarification!!
a7723f0
to
402538c
Compare
ASSERT_TRUE(p7_dup); | ||
EXPECT_TRUE(PKCS7_type_is_signed(p7_dup.get())); | ||
|
||
p7_der = kPKCS7SignedWithSignerInfo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes I was asking about p7_der
on L1211. It seemed like the d2i
functions did some pointer advancements, but the const
pointer made me question myself if anything had actually changed. Thanks for the clarification!!
## 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
Issues:
Resolves CryptoAlg-2482
Description of changes:
Following up PR 1726, this PR adds various getter/setter functions needed to support ruby and provides test coverage for them. We also add test coverage for the various ASN.1 functions and
PKCS7_dup
added in PR 1726.This commit borrows a sample PKCS7 SignedData from https://lapo.it/asn1js/ that, unlike other test fixtures, contains SignerInfos. That sample object has been hand-edited to reduce its size (~10KiB of an inner EncapsulatedContentInfo object were consumed by filler). You can see a parsed illustration of the new fixture here.
The next PR in this series will implement PKCS7's BIO-based data initialization and finalization functions.
Call-outs:
Testing:
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.