Fix the issues in DefaultCertValidator::doVerifyCertChain and ContextImpl::verifyCertChain
#16982
Conversation
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
|
Hi @tyxia, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
|
Just to be aware, we've been making some improvements to Envoy tooling for more prompt reviews, but draft PRs are not included in the tooling. I'm updating the contribution guidelines to make that clear (https://github.com/envoyproxy/envoy/pull/17063/files) , but as this PR predates the repokitteh warning I just want to call out that draft PRs are likely not going to get fast turnaround. Please consider marking this as ready for review if it stalls, and you want the assignee to take a look! |
Thank you very much for information and pointers! I created this draft PR intentionally to trigger the CI run (as the sanity check):). I will mark it as ready-for-review when when it is ready. Please let me know if you have any other instructions. Thanks! |
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
source/extensions/transport_sockets/tls/cert_validator/default_validator.cc
Outdated
Show resolved
Hide resolved
tyxia
changed the title
DefaultCertValidator::doVerifyCertChain and ContextImpl::verifyCertChain
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
| // sure the verification for other validation context configurations doesn't fail (i.e. either | ||
| // `NotValidated` or `Validated`). If `trusted_ca` doesn't exist, we will need to make sure other | ||
| // configurations are verified and the verification succeed. | ||
| int validation_status = verify_trusted_ca_ |
There was a problem hiding this comment.
Thanks for review! I think either bool or int will work here as bool <-> int implicit conversion will be triggered, and it will be triggered in either way.
The reason I used int here is to align with value 1 in ternary operator in line 226 and the return type of this function (which is int).
Please let me know what do you think.
There was a problem hiding this comment.
I think int is fine given that the function returns the same error code as X509_verify_cert
|
/retest |
|
Retrying Azure Pipelines: |
…textImpl::verifyCertChain` (envoyproxy#16982) Signed-off-by: Tianyu Xia <tyxia@google.com>
trusted_ca = falseandno hash or SAN matcher. Basically, it means no validation context is provided so no certificate validation is performed. Current code will still returnOKfor this case.NotValidatedseems to be an initial state that needs to be updated to eitherFailedorValidatedeventually. If it stay asNotValidated, it should be treated asError.Basically, the problem is QUIC client side register and then will invoke the
verifyCertificate()callback because its cert_verify_mode is set toSSL_VERIFY_PEERbut no validation_context is provided. Thus, it will run intoNotValidatedcase I described in 1 and my change return it asERROR.Similar to openSSL logic, the error can be treated as non-fatal if mode is SSL_VERIFY_NONE. The proposed fix is in ContextImpl::verifyCertChain( ) (which is only used by QUIC path): Query the true verify_mode that is initialized in
DefaultCertValidator::initializeSslContexts(it will beSSL_VERIFY_NONEfor this case because there is no validation_context config provided).(PS: I discussed with QUIC folk, QUIC test could be investigated as well. But I feel the proposed fix is needed regardless)
Signed-off-by: Tianyu Xia tyxia@google.com
Risk Level: Low
Testing: Local test