refactor: ossl x509 parsing #4351
Conversation
| @@ -903,7 +903,7 @@ int main(int argc, char **argv) | |||
| EXPECT_SUCCESS(s2n_stuffer_copy(&input, &server->handshake.io, | |||
| s2n_stuffer_data_available(&input))); | |||
|
|
|||
| EXPECT_FAILURE_WITH_ERRNO(s2n_client_cert_recv(server), S2N_ERR_CERT_INVALID); | |||
| EXPECT_FAILURE_WITH_ERRNO(s2n_client_cert_recv(server), S2N_ERR_DECODE_CERTIFICATE); | |||
There was a problem hiding this comment.
The only behavior change here is that failing the openssl x509 parsing call returns a "decode certificate" error rather than the previous "cert invalid error". The same d2i_X509 failure is occurring that previously occurred.
server_cert = d2i_X509(NULL, &data, asn1_cert.size);
RESULT_ENSURE(server_cert, S2N_ERR_DECODE_CERTIFICATE);
I'm confused by this comment on the test case
Validate that the certificate chain we generated is parseable.
The certificate is "readable" by s2n_x509_validator_read_asn1_cert, but not parseable by openssl.
This commit introduces a helper method to standardize our openssl X509 parsing, and also restructures the validator and cert loading to remove some cert-reparsing that was occuring.
jmayclin
force-pushed
the
ossl-x509-refactor
branch
from
59fb211
to
92f3301
Compare
Co-authored-by: Sam Clark <[email protected]>
* add additional ENSURE_REFs * switch load_cert method to parse with validation * make asn1_cert_parse non-static and forward declare * rename server-cert to cert * explicitly zero-initialize variables
|
Thanks for fixing the duplicate parsing! |
| * | ||
| * |
There was a problem hiding this comment.
I'm aware of the option to hide whitespace, but I would suggest setting up your IDE so that it doesn't make these changes to lines you don't touch. Since it seems like none of our linters care, you're going to end up with some messy diffs otherwise.
There was a problem hiding this comment.
I have disabled for the time being, but this is the default preference of clang-format and I would be in favor of just adding a linter that does care 😄. Opened #4362 to track this
- remove unnecessary test header in openssl_x509_tests - RESULT_GUARD for s2n_pkey_zero_init - move version check to start of method - initialize public-key along with other structs - remove unnecessary pkey_zero_init - add extra line for trailing byte validation - use RESULT_GUARD_OSSL instead of manual check - rename s2n_pkey_x509... method
- remove param information from internal comments
- manually format function definition I really thought that clang format would handle this for me :'(
- use gte rather than equal for 1.3 check - use explicit ZERO_TO_DISABLE_DEFER_CLEANUP method
- the given preference was to address the TEST_DEBUG_PRINT issues systematically rather than in this specific instance
Resolved issues:
#4163
Description of changes:
This PR borrows work from #4176 and also applies it to local cert loading.
X509*is moved tos2n_openssl_x509_parse*methods.This makes s2n-tls of peer and local certificate more standard, and is necessary for #4339
Call-outs:
Historical Behavior: I would love to make all cert parsing enforce the trailing byte restriction, but it would be a behavior change, and therefore probably not a polite thing to do☹️
Parsing APIs: I don't love the multiple APIs for the openssl x509 parsing, but it seemed better than forcing people to needlessly declare a length variable or including a boolean toggle.
Whitespace: Sorry for the extra noise in the diff. My IDE removes all trailing whitespace. I'll open an issue for a linting step to just do this to our entire codebase.
Testing:
Added the unit tests from Sam's PR with some slight modifications, and all existing unit tests pass with the commented caveat in
s2n_certificate_test.cBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.