Fix the bug that validate flag is not set when the SAN(SubjectAltName) matching is performed

[tyxia]

When SAN(SubjectAltName) matching with the provided matchers is the only verification performed and matching succeeds, the validated flag is not updated properly(i.e. left as NotValidated) and the function return wrong status code.

Add the unit test:

1. Create DefaultValidator object with NULL certificateValidationContext (i.e no validation context). This ensures that the certificate verification later will be only performed on the SAN matcher arg provided in step#3
2. Create the cert from test data file.
3. Provide the SAN matcher as only input and perform the certificate verification with correct SAN regex matcher and incorrect SAN exact matcher(negative test).

Risk Level: Low
Testing:
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Tianyu Xia tyxia@google.com

[repokitteh-read-only]

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.

🐱

Caused by: #16816 was opened by tyxia.

see: more, trace.

[rojkov]

Thank you for fix it! Could you please add a test case for it?

[tyxia]

Thank you for fix it! Could you please add a test case for it?

Thank you for the review!
This code path is currently not tested in default_validator_test unit test. In order to test this path quickly, I coded up a small test to : 1) set up TLS ClientContext 2) load the certificate yaml test data (with customization for SAN matcher) 3) call the function with SSL-specific structs.
The code works as expected, but I found an interesting thing:
The caller site (around line#225 in this file, which normally will be called from boringssl) only cares if the validate flag is Envoy::Ssl::ClientValidationStatus::Failed or not.

I think it is because:

1. This function(i.e. function DefaultCertValidator::verifyCertificate) is used to verify context configuration like subject name, hash etc, but not for trust_ca.
2. only trust_ca is specified but no SAN list/macther, hash, etc is a valid validation context configuration. This matches the code in DefaultCertValidator::initializeSslContexts and document here
3. And as we can see in doVerifyCertChain function, trust_ca path is validated separately.

So, the validate flag in this function I changed only represents that it is not validated for SAN, hash etc but doesn't mean that it is not validated because we could have only trust_ca case and it is validated in the code above.

In short, I think the fix is technically correct but doesn't make the difference in practice based on current code. Also, this code path is already tested in many integration tests(not its unit test though). Thus, we may not need a new test case here.

Please let me know what do you think.

[rojkov]

Yes, I have no doubt the code is correct. But a unit test could help with making it future-proof and to pinpoint a problem better.

We know that in case of DefaultCertValidator its method verifyCertificate() should return either Validated or Failed status. So for the sake of future refactorings it'd be beneficial to check this invariant in a unit test.

For example the function could be simplified even farther if we dropped validated completely and returned Envoy::Ssl::ClientValidationStatus::Validated in the last return.

[tyxia]

Yes, I have no doubt the code is correct. But a unit test could help with making it future-proof and to pinpoint a problem better.

We know that in case of DefaultCertValidator its method verifyCertificate() should return either Validated or Failed status. So for the sake of future refactorings it'd be beneficial to check this invariant in a unit test.

For example the function could be simplified even farther if we dropped validated completely and returned Envoy::Ssl::ClientValidationStatus::Validated in the last return.

[tyxia]

Yes, I have no doubt the code is correct. But a unit test could help with making it future-proof and to pinpoint a problem better.
We know that in case of DefaultCertValidator its method verifyCertificate() should return either Validated or Failed status. So for the sake of future refactorings it'd be beneficial to check this invariant in a unit test.
For example the function could be simplified even farther if we dropped validated completely and returned Envoy::Ssl::ClientValidationStatus::Validated in the last return.

Thank you for the inputs! I have some local test code but I have one thing want to discuss first:
Should we treat Envoy::Ssl::ClientValidationStatus::NotValidated as a OK return code at caller site (line#225 in this file)? (i.e. always ignore NotValidated and return Validated in the last return).

Without full background info of the code but based on my code reading, I think there is one corner case not being handled right now : trusted_ca = false and no hash or SAN matcher .
Basically, it means no validation context is provided so no certificate validation is performed. Current code will still return OK for this case but strictly speaking, technically it seems not correct that verifyCertificate callback get called but no certificate validation_context provided?

I quickly tested one change at line 225 to return ERROR (i.e. 0) for this case
change from:

return allow_untrusted_certificate_ ? 1
                                      : (validated != Envoy::Ssl::ClientValidationStatus::Failed);

to

return allow_untrusted_certificate_ ? 1 : verify_trusted_ca_ ? validated != Envoy::Ssl::ClientValidationStatus::Failed : validated == Envoy::Ssl::ClientValidationStatus::Validated;

And it seems reveal one potential bug in sds_dynamic_integration_test's QUIC path(The only failure caused by change above). TCP path of this test works fine.
Basically, the problem is QUIC client side register and then will invoke the verifyCertificate() callback because of its cert_verify_mode is set to SSL_VERIFY_PEER. But the test doesn't provide the validation_context. Thus, it will run into NotValidated case and my change return it as ERROR.

The quick experimental fix for that could be query the true verify_mode that is initialized in DefaultCertValidator::initializeSslContexts, it will be SSL_VERIFY_NONE for this case because there is no validation_context config provided.
And then similar to openSSL logic, we can treat the the error as non-fatal if mode is SSL_VERIFY_NONE.
I have a draft PR (not the final fix) #16982 as the reference to what are changes I am making.

I feel this might not be a critical issue at this moment but it might also doesn't hurt to get to the bottom of it :)
Thanks for your time and review!

[rojkov]

This sounds reasonable. To me NotValidated feels like an initial state that needs to be driven to either Failed or Validated eventually. Consequently treating it as OK in functions like doVerifyCertChain() is rather wrong. But that fix can be done in a follow-up PR to keep this one small.

[tyxia]

This sounds reasonable. To me NotValidated feels like an initial state that needs to be driven to either Failed or Validated eventually. Consequently treating it as OK in functions like doVerifyCertChain() is rather wrong. But that fix can be done in a follow-up PR to keep this one small.

Yes, I agreed. I will split those fixes into another PR.

The unit test is added. PTAL!

[rojkov]

Thank you! LGTM modulo one more check.

[tyxia]

Also, cc @htuch for visibility.

[rojkov]

Thank you! Looks good to me.
@yanavlasov I think it's ready for a second pass.