add support for X509_get_signature_info #1504
Conversation
samuel40791765
force-pushed
the
custom-sigs
branch
3 times, most recently
from
147682d
to
943da00
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1504 +/- ##
==========================================
+ Coverage 76.95% 76.97% +0.01%
==========================================
Files 425 425
Lines 71562 71602 +40
==========================================
+ Hits 55071 55113 +42
+ Misses 16491 16489 -2 ☔ View full report in Codecov by Sentry. |
| flags); | ||
| } | ||
|
|
||
| int x509_init_signature_info(X509 *x509) { |
There was a problem hiding this comment.
Don't recall if we were only adding the check on external functions first, but if we are also adding to same checks to internal functions then should this validated x509 != NULL?
There was a problem hiding this comment.
This did come up a long while back, I think the final consensus was to cover external functions. The justification being that there would be NULL checks in the external functions calling these internal functions, so the extra check would be overkill and add additional bloat.
crypto/x509/x509_set.c
Outdated
|
|
||
| int X509_get_signature_info(X509 *x509, int *digest_nid, int *pubkey_nid, | ||
| int *sec_bits, uint32_t *flags) { | ||
| if (!X509_check_purpose(x509, -1, -1)) { |
There was a problem hiding this comment.
It seems like the intent of this is to just trigger x509v3_cache_extensions, as it will bail out of the function afterwards due to id == -1. Would it be clearer to just call the cache function?
There was a problem hiding this comment.
Ahh nice catch, I was just following the original openssl implementation, but that makes sense. I'll also add this comment from OpenSSL to X509_check_purpose
There was a problem hiding this comment.
It seems like Postgres is also dependent on this function and the new behavior is causing the postgres CI to fail..
I'm guessing that X509_get_signature_info is just a information retrieval function, so the cert is allowed to "invalid extensions".. which explains why OpenSSL's implementation didn't check the return value. It's a bit of a pain, but I'll have to change this to be less error prone
There was a problem hiding this comment.
Nvm, the postgres CI was failing since the initial change. I'll have to figure out what's going on here
There was a problem hiding this comment.
I see all CI checks have passed. What ended up being the issue with the postgres checks?
EDIT: nevermind, i see it was our lack of PSS support in libssl.
There was a problem hiding this comment.
I forgot to mention I cut CryptoAlg-2393 to track PSS support in libssl. I'll add that in the commit/PR description.
samuel40791765
force-pushed
the
custom-sigs
branch
from
898adce
to
d56c5bd
Compare
Issues:
Resolves
CryptoAlg-2264Description of changes:
MySQL 8.2/8.3 depends on
X509_get_signature_info. This symbol originated from an OpenSSL 1.1.1 commit (openssl/openssl@786dd2c) implementing support for "custom signature parameters".The commit mainly allows the user to set and retrieve information about the signature type. There's also a custom ASN1 method handler that OpenSSL implemented in this commit.
We don't support custom
EVP_PKEYASN1 methods and setting your own signature information doesn't make much sense without the custom methods, so I've only implemented support for retrieving the signature type. MySQL only depends on retrieving the signature info, so this fits our use case.Call-outs:
CryptoAlg-2393was cut to track RSA PSS support in libssl, which was uncovered in the postgres CI.Testing:
New test
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.