-
Notifications
You must be signed in to change notification settings - Fork 118
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 PEM_write_bio_PrivateKey_traditional #1845
Conversation
7ce041a
to
98b3b46
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1845 +/- ##
==========================================
- Coverage 78.56% 78.34% -0.22%
==========================================
Files 583 584 +1
Lines 98807 99236 +429
Branches 14161 14188 +27
==========================================
+ Hits 77623 77743 +120
- Misses 20558 20864 +306
- Partials 626 629 +3 ☔ View full report in Codecov by Sentry. |
98b3b46
to
b251f0e
Compare
b251f0e
to
5e7b7f7
Compare
crypto/pem/pem_pkey.c
Outdated
return 0; | ||
} | ||
char pem_str[80]; | ||
BIO_snprintf(pem_str, 80, "%s PRIVATE KEY", x->ameth->pem_str); |
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
BIO_snprintf(pem_str, 80, "%s PRIVATE KEY", x->ameth->pem_str); | |
BIO_snprintf(pem_str, sizeof(pem_str), "%s PRIVATE KEY", x->ameth->pem_str); |
and how was 80 selected?
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.
No particular reason, this was taken from OpenSSL. They probably just chose a size large enough to fit the string with "pem_str" and PRIVATE KEY
.
Co-authored-by: Will Childs-Klein <willck93@gmail.com>
## What's Changed * More tweaks for Ruby integration by @samuel40791765 in #1852 * Implementation of EVP_PKEY_CTX_ctrl_str for various key types by @justsmth in #1850 * Add MLKEM768 Hybrid Groups to libssl by @alexw91 in #1849 * add support for PEM_write_bio_PrivateKey_traditional by @samuel40791765 in #1845 * Update s2n-bignum subtree by @torben-hansen in #1861 * Add asserts in testing to fix Coverity alert by @smittals2 in #1864 * Disable CRYPTO_is_AVX512IFMA_capable by @justsmth in #1858 **Full Changelog**: v1.35.0...v1.35.1 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.
Issues:
Resolves
CryptoAlg-2546
Description of changes:
The main difference between
PEM_write_bio_PrivateKey_traditional
and the modernPEM_write_bio_PrivateKey
is thatPEM_write_bio_PrivateKey
writes a PKCS#8 encoding, while the traditional one writes with DER encoding.OpenSSL's
PEM_write_bio_PrivateKey_traditional
calls intoi2d_PrivateKey
which calls into another additionalold_priv_encode
EVP_PKEY_ASN1_METHOD hook. We also need to make the call intoi2d_PrivateKey
, but luckily for us BoringSSL removed the hook and let the call point to the correspondingi2d_RSA/DSA/ECPrivateKey
methods instead. We can built on this directly to reimplementPEM_write_bio_PrivateKey_traditional
.aws-lc/crypto/x509/i2d_pr.c
Lines 65 to 79 in 51d9a8d
PEM_read_bio_PrivateKey
already has logic to parse the "traditional form" of these PEM files. I've also added tests to verify.Call-outs:
N/A
Testing:
Tests for each type writable/readable. A single test for DH, which shouldn't be writable.
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.