Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore validation failures with NO_CERTIFICATE_VALIDATION #1728

Merged
merged 4 commits into from
Jul 28, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jun 16, 2021

I bump to this while working on dotnet/runtime#54302.
I'm not 100% sure what is exactly happening but when using custom client certificate the QueryContextAttributesW would fail. It is somewhat curious it fails with SEC_E_INVALID_HANDLE but since we are not asking to do validation at all, I decided to just ignore the error and move one.

In my test setup the peer certificate can still be retrieved and passed to validation callback
So when MsQuic/OS is not asked to do validation we would just log the error and move one.

I'm not sure if it make sense to even call the QueryContextAttributesW but since we pass result of it to the validation callback one can see it as pre-validation.

@wfurt wfurt requested a review from a team as a code owner June 16, 2021 23:02
@nibanks
Copy link
Member

nibanks commented Jun 16, 2021

@wfurt can you reproduce the issue? I'd like to have schannel take a look if possible to make sure they don't have a bug instead of working around (though we'd probably also take the work around).

@wfurt
Copy link
Member Author

wfurt commented Jun 17, 2021

I can. reproduce it with test added in dotnet/runtime#54302. But I don't know how easy it is for others to run. I'm planing to do also more investigation and possibly follow-up with Schannel about the case when client certificate is requested but client does not provide one. It would be nice if that behaves same as ssl.

@nibanks
Copy link
Member

nibanks commented Jun 17, 2021

I can. reproduce it with test added in dotnet/runtime#54302. But I don't know how easy it is for others to run. I'm planing to do also more investigation and possibly follow-up with Schannel about the case when client certificate is requested but client does not provide one. It would be nice if that behaves same as ssl.

Let's start an email thread with Schannel folks, along with their logs. If you use our log.ps1 with Full.Verbose profile, it will include them.

@wfurt
Copy link
Member Author

wfurt commented Jul 28, 2021

here is the answer

It appears that you have manual credential validation set. In that case, this attribute is not available. This essentially asking schannel to provide the result of its certificate validation while also asking it not to check the certificate at all. Manual credential validation is set if you request [ISC|ASC]_REQ_MANUAL_CRED_VALIDATION or SCH_CRED_MANUAL_CRED_VALIDATION or the schannel registry key “ManualCredValidation”.

Technically, it works to ignore the error if QUIC_CREDENTIAL_FLAG_NO_CERTIFICATE_VALIDATION is set. But better yet, don't call this attribute if QUIC_CREDENTIAL_FLAG_NO_CERTIFICATE_VALIDATION is set, if it does what I think it does.

417 [1] 2C2C.3F4C::07/28/21-11:56:01.9338011 [ssluser] UserCtxtAttr_cpp768 SpUserQueryContextAttributes() - QueryContextAttributes(SECPKG_ATTR_CERT_CHECK_RESULT_INPROC)
418 [1] 2C2C.3F4C::07/28/21-11:56:01.9338087 [ssluser] bulkencr_cpp2284 CSsl3TlsContext::QueryCertificateValidationResult() - Cannot query validation results as manual validation was set and validation bypassed.

That make sense to me. I updated the change accordingly. It also make sense why this works for MsQuic as the tests do not seems to use the QUIC_CREDENTIAL_FLAG_NO_CERTIFICATE_VALIDATION option.

If this looks good, can you please also get this to the 1.5 branch @nibanks?

@nibanks nibanks merged commit 8f74f85 into microsoft:main Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants