Refactor s2n_x509_validator_validate_cert_chain to support an async callback #3500
Conversation
tls/s2n_x509_validator.h
Outdated
| INIT, | ||
| PRE_VALIDATE, | ||
| VALIDATED, |
There was a problem hiding this comment.
I don't love this name. How about "PARSED"? Or "CERT_READ"?
There was a problem hiding this comment.
I chose something like PRE_VALIDATE, because this stage kind of does more than just parse the cert chain. It also calls s2n_verify_host_information, and will also call s2n_crl_lookup in the next PR. How about CERT_CHAIN_PROCESSED? I added this in a9c024e. Let me know if you don't like this name.
Also, switching to the PRE_VALIDATE state sort of implied that skip_cert_validation was false, so I checked for this before switching states. With CERT_CHAIN_PROCESSED, this isn't clear, So in a9c024e I made it always enter this state and skip the verify parts in s2n_x509_validator_verify_cert_chain.
There was a problem hiding this comment.
Maybe "READY_TO_VALIDATE"? Or just "READY"? Names are hard :\
There was a problem hiding this comment.
READY_TO_VALIDATE makes sense. I switched it to READY_TO_VERIFY, to keep the state name the same as s2n_x509_validator_verify_cert_chain, which does the actual X509_verify_cert call now. 876a33b
tls/s2n_x509_validator.c
Outdated
| if (conn->actual_protocol_version >= S2N_TLS13) { | ||
| s2n_parsed_extensions_list parsed_extensions_list = { 0 }; | ||
| RESULT_GUARD_POSIX(s2n_extension_list_parse(&cert_chain_in_stuffer, &parsed_extensions_list)); | ||
|
|
||
| /* RFC 8446: if an extension applies to the entire chain, it SHOULD be included in the first CertificateEntry */ | ||
| *first_certificate_extensions = parsed_extensions_list; | ||
| } |
There was a problem hiding this comment.
Not directly related to your change, and I know we already only processed the extensions from the first certificate, but it still feels like very wrong behavior to me. Can we put a comment somewhere (either here or down where the extensions are processed) making this behavior very clear and mentioning that it's an s2n-tls choice not specified by the RFC?
There was a problem hiding this comment.
I added a comment where the extensions are processed: 2b5f26e
I also moved the existing RFC SHOULD comment to here.
Resolved issues:
Part of #3499
Description of changes:
This PR refactors
s2n_x509_validator_validate_cert_chainto support adding the async CRL callback.A new validator state has been added,
PRE_VALIDATE, which splits this function into two stages:INIT, andPRE_VALIDATE. In theINITstage, when the function is first called, the cert chain is read, the host name is validated, and the CRL callback will be triggered. After triggering the CRL callback, the function may returned blocked. After unblocking, the function will continue and enter thePRE_VALIDATEstage, which will skip everything in theINITstage and proceed to validate the certificate chain.Splitting up this function into stages ensures that work is not duplicated after blocking due to invoking the async callback.
Call-outs:
A lot of
pkey_types were set toS2N_PKEY_TYPE_UNKNOWNin this PR. This is because the default value fors2n_pkey_typeisS2N_PKEY_TYPE_RSA, which is the value being checked in the tests. Setting the value to unknown ensures this value is actually changing.Additionally, checks for
S2N_PKEY_TYPE_RSAwere removed in tests wheres2n_x509_validator_validate_cert_chainis expected to fail. This is becausepublic_key_outis now set later in this function, and is no longer set before this function fails in these tests.Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.