Add support for cert-pinning on Windows and Mac. #702
Conversation
|
|
| }; | ||
|
|
||
| return http::client::details::verify_cert_chain_platform_specific(verifyCtx, utility::conversions::to_utf8string(host), chainFunc); | ||
| } |
There was a problem hiding this comment.
code cosmetics: indentation of closing bracket
| p_request_context->report_error(errorCode, build_error_msg(error_result)); | ||
| break; | ||
| } | ||
| case WINHTTP_CALLBACK_STATUS_SENDING_REQUEST: | ||
| { |
There was a problem hiding this comment.
This status does not seem to be hit on our system (Windows 7). We get the following notifications:
- WINHTTP_CALLBACK_STATUS_HANDLE_CREATED
- WINHTTP_CALLBACK_STATUS_SENDREQUEST_COMPLETE
- WINHTTP_CALLBACK_STATUS_HEADERS_AVAILABLE
- WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE
- WINHTTP_CALLBACK_STATUS_READ_COMPLETE
....
There was a problem hiding this comment.
Looks like the PR is missing a flag when calling WinHttpSetStatusCallback, we do have integration tests on our network manager but somehow i missed the flag when creating the diff from our repo.
Adding that that now.
…Callback and add two tests for cert-pinning windows.
| { | ||
| CertificateChain certificate_chain; | ||
| std::string host_name; | ||
| long certificate_error{ 0 }; |
There was a problem hiding this comment.
How do I interpret this error code? I suppose it's platform dependent?
Please add some description in the comments.
There was a problem hiding this comment.
Yes, the error code is platform dependent, we use it for debugging certificate issues.
Mac: https://developer.apple.com/documentation/security/sectrustresulttype
Windows: https://msdn.microsoft.com/en-us/library/windows/desktop/aa377590(v=vs.85).aspx
Will add a comment for that.
| return false; | ||
| } | ||
|
|
||
| auto info = std::make_shared<certificate_info>(host, get_X509_cert_chain_encoded_data(verifyCtx)); |
| auto info = std::make_shared<certificate_info>(host, get_X509_cert_chain_encoded_data(verifyCtx)); | ||
| info->verified = true; | ||
|
|
||
| return m_http_client->client_config().invoke_certificate_chain_callback(info); |
There was a problem hiding this comment.
On Linux this is called several times. I'm not exactly sure why, but I suppose it is due to the way OpenSSL handles certificate chains (cf. comments in this method). Do we need a similar workaround as for macOS and Android above?
There was a problem hiding this comment.
Yes, you would need to do the same flow on Linux as per the macOS flow above.
The "is_end_certificate_in_chain" check above should allow the connection until we get the end certificate, once we get that, then we get the OS to build the full cert chain, only after that, then we do the cert-pinning.
If you just wait for the end certificate and don't build the full chain from the OS, you will only have the certs from the SSL handshake, and more than likely that will not be the full chain.
| auto info = std::make_shared<http::client::certificate_info>(utility::conversions::to_utf8string(m_uri.host()), get_X509_cert_chain_encoded_data(verifyCtx)); | ||
| info->verified = true; | ||
|
|
||
| return m_config.invoke_certificate_chain_callback(info); |
There was a problem hiding this comment.
Wild guess: I would expect that this would also be invoked more than once per connection on Linux as it seems to be duplicated code referred to my comment above.
There was a problem hiding this comment.
Correct as the check "is_end_certificate_in_chain" isn't done on Linux.
FIX: Certificate Pinning Compilation on Linux
linux: invoke the certificate chain callback only once
|
We have had a number of requests for something like this -- we would be interested in adding such a feature, but unfortunately this one doesn't merge cleanly and appears to be incompatible with how we implemented Any chance you want to try to resolve the merges or should we close this? |
|
Given that this doesn't merge cleanly and is missing tests, I'm closing this for now. Anyone feel free to reopen with the conflicts fixed. (We aren't normally a super stickler about tests being added for each new feature, but this one is security sensitive. Doing it as an "outside test" that pings a server, like we do for our badssl tests, is OK) |
Pull request for the solution we ended up using from the first PR: #135
Again, this is a preliminary pull request to see if you guys are happy to add this to Casablanca.
Added a callback that will return the certificate_info with the cert chain to the consumer.
The certificate_info will contain:
Using this approach we leave the validation of the full certificate chain up to the consumer, so Casablanca does not have to support managing different types of certificate lists for the consumer to do cert-pinning.
The consumer can then use OpenSSL (Or whatever they want) to import the certificate chain and they will then decide on whether to accept or reject the connection.
Current changes are for Windows & Mac.