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.

Sign up for GitHub

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

Jump to bottom

Cherrypick "Add some barebones support for DH in EVP" #1813

Merged
merged 4 commits into from
Aug 30, 2024
Merged

Cherrypick "Add some barebones support for DH in EVP" #1813

merged 4 commits into from
Aug 30, 2024

Conversation

samuel40791765
Copy link
Contributor

Description of changes:

We have multiple integration projects (Node and Ruby) that happen to depend on this as well. Cherrypicking the support for EVP_PKEY_DH over to AWS-LC as well.
There may be more that we need to add to support this better, but this is nice work that helps build something we can start upon.

Call-outs:

N/A

Testing:

Tests in commit

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.

@samuel40791765 samuel40791765 requested a review from a team as a code owner August 29, 2024 00:32
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 78.12500% with 42 lines in your changes missing coverage. Please review.

Project coverage is 78.41%. Comparing base (b925242) to head (fb59c08).

Files with missing lines Patch % Lines
crypto/evp_extra/p_dh_asn1.c 67.85% 18 Missing ⚠️
crypto/evp_extra/p_dh.c 81.25% 12 Missing ⚠️
crypto/evp_extra/evp_test.cc 72.72% 6 Missing and 3 partials ⚠️
crypto/fipsmodule/evp/evp.c 50.00% 2 Missing ⚠️
crypto/evp_extra/p_x25519.c 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             main    #1813    +/-   ##
========================================
  Coverage   78.41%   78.41%            
========================================
  Files         582      584     +2     
  Lines       97573    97740   +167     
  Branches    13990    14009    +19     
========================================
+ Hits        76510    76641   +131     
- Misses      20441    20476    +35     
- Partials      622      623     +1

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

@justsmth justsmth self-requested a review August 29, 2024 13:19
justsmth
justsmth previously approved these changes Aug 29, 2024
View reviewed changes
WillChilds-Klein
WillChilds-Klein previously approved these changes Aug 29, 2024
View reviewed changes
crypto/fipsmodule/evp/evp.c
}

DH *EVP_PKEY_get1_DH(const EVP_PKEY *pkey) {
SET_DIT_AUTO_DISABLE;
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, this isn't touching secret material. we didn't need this call here previously, did we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really good point, let me ask internally about the justification behind this and if I should add this back.

justsmth
justsmth previously approved these changes Aug 30, 2024
View reviewed changes
@justsmth justsmth dismissed stale reviews from WillChilds-Klein and themself via 3965b1c August 30, 2024 16:23
justsmth
justsmth previously approved these changes Aug 30, 2024
View reviewed changes
@davidben @samuel40791765
Remove some unnecessary dependencies on EVP_PKEY_set_type
23e5c29 
EVP_PKEY_set_type needs to pull in every supported EVP_PKEY type, but
most of our calls within the library already know what type they're
working with. Have them call evp_pkey_set_method directly.

Bug: 497
Change-Id: I17cb9a0dff0da55206686bce1d8e1df4773f6f4d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67127
Reviewed-by: Bob Beck <[email protected]>
Auto-Submit: David Benjamin <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit fea4c97491e43455063fda6967e4a2cc4234baf5)
smittals2
smittals2 previously approved these changes Aug 30, 2024
View reviewed changes
davidben and others added 3 commits August 30, 2024 18:09
@davidben @samuel40791765
Add some barebones support for DH in EVP
37b6e33 
OpenSSH needs this. Features that have been intentionally omitted for
now:

- X9.42-style Diffie-Hellman ("DHX"). We continue not to support this.
  Use ECDH or X25519 instead.

- SPKI and PKCS#8 serialization. Use ECDH or X25519 instead. The format
  is a bit ill-defined. Moreover, until we solve the serialization
  aspects of https://crbug.com/boringssl/497, adding them would put this
  legacy algorithm on path for every caller.

- Most of the random options like stapling a KDF, etc. Though I did add
  EVP_PKEY_CTX_set_dh_pad because it's the only way to undo OpenSSL's
  bug where they chop off leading zeros by default.

- Parameter generation. Diffie-Hellman parameters should not be
  generated at runtime.

This means you need to bootstrap with a DH object and then wrap it in an
EVP_PKEY. This matches the limitations of the EVP API in OpenSSL 1.1.x.
Unfortunately the OpenSSL 3.x APIs are unsuitable for many, many
reasons, so I expect when we get further along in
https://crbug.com/boringssl/535, we'll have established some patterns
here that we can apply to EVP_PKEY_DH too.

Change-Id: I34b4e8799afb266ea5602a70115cc2146f19c6a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67207
Reviewed-by: Theo Buehler <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
(cherry picked from commit e57ab142c0cabf30b6d4e85b8038003cc179716b)
@samuel40791765
Insert SET_DIT_AUTO_DISABLE in EVP_PKEY DH APIs
b547681
@samuel40791765
Update build files in generated-src
fb59c08
@samuel40791765 samuel40791765 merged commit f7f2082 into aws:main Aug 30, 2024
107 of 108 checks passed
@smittals2 smittals2 mentioned this pull request Sep 15, 2024
Merged
smittals2 added a commit that referenced this pull request Sep 17, 2024
@smittals2
prepare for v1.35.0 release (#1853)
d3a598c 
## 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
@samuel40791765 samuel40791765 deleted the cherrypick-dh branch December 13, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillChilds-Klein WillChilds-Klein WillChilds-Klein approved these changes

@smittals2 smittals2 smittals2 approved these changes

@justsmth justsmth justsmth left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@samuel40791765 @codecov-commenter @WillChilds-Klein @justsmth @smittals2 @davidben