-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
ssl: add server cert's "sha256//" hash to verbose #430
Conversation
I was hoping to get an initial review on this change, and don't expect a merge at it's current state. With the proposed fix: $ ./curl --verbose --head --pinnedpubkey "sha256//daHR2E1iBqPT1X9qK/UISUZlkqW6MGJ7eBI+HDNtARs=" https://community.scaleway.com/
* Trying 212.47.225.64...
* Connected to community.scaleway.com (212.47.225.64) port 443 (#0)
* Cipher selection: ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
* TLSv1.0 (OUT), TLS handshake, Client hello (1):
* TLSv1.0 (IN), TLS handshake, Server hello (2):
* TLSv1.0 (IN), TLS handshake, Certificate (11):
* TLSv1.0 (IN), TLS handshake, Server key exchange (12):
* TLSv1.0 (IN), TLS handshake, Server finished (14):
* TLSv1.0 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.0 (OUT), TLS change cipher, Client hello (1):
* TLSv1.0 (OUT), TLS handshake, Finished (20):
* TLSv1.0 (IN), TLS change cipher, Client hello (1):
* TLSv1.0 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.0 / DHE-RSA-AES256-SHA
* Server certificate:
* subject: OU=GT30628797; OU=See www.rapidssl.com/resources/cps (c)15; OU=Domain Control Validated - RapidSSL(R); CN=*.scaleway.com
* start date: 2015-02-05 06:59:04 GMT
* expire date: 2017-02-07 02:23:01 GMT
* subjectAltName: community.scaleway.com matched
* issuer: C=US; O=GeoTrust Inc.; CN=RapidSSL SHA256 CA - G3
* SSL certificate verify ok.
* pinnedpubkey: sha256//wZfuNCfeEh0w6+eUazYO4VORGcsxqSACDPdqvnWBN/A=
* SSL: public key does not match pinned public key!
* Closing connection 0
* TLSv1.0 (OUT), TLS alert, Client hello (1):
curl: (90) SSL: public key does not match pinned public key! |
Looks clean and appropriate to me! |
The CI failure doesn't seem related to this change: 1801: data FAILED:
--- log/check-expected 2015-09-12 22:34:23.000000000 +0000
+++ log/check-generated 2015-09-12 22:34:23.000000000 +0000
@@ -1 +1,2 @@
HTTP/1.1 101 Switching![LF]
+[LF] Is there anything else I should add to this change? |
- Show how a certificate can be obtained using OpenSSL. Bug: #430 Reported-by: Daniel Hwang
FYI the reason for the sha256 hash mismatch in that output is because the public key you specified is from RapidSSL's generic certificate and the certificate curl received is a different certificate that's server-specific for scaleway. Likely what happened is someone made the hash you are passing in by retrieving the server certificate without specifying the server name. For OpenSSL's tool you would have to pass The correct hash appears to be the one your curl received, I like the idea of showing it when verbose. Why did you make a copy of sha256sumdigest? That looks unnecessary. |
You're right! It was unnecessary and I removed it. Thanks for the feedback! |
Great though now I notice something else. You did not free the memory allocated by the base64 encoding function. I fixed that and a few nits re style changes, and eliminated storing the return value from the base64 encoding function, instead testing it directly in an Branch is here: This could still be improved. Now that we are encoding the sha256 binary data to base64 we can just compare that with what is supplied by the user rather than what we are currently doing where we take each of the users base64 encoded Also regardless of opinions on that I think we should fail if @bagder I know you're assigned, not trying to step on your toes here. |
Add a "pinnedpubkey" section to the "Server Certificate" verbose Bug: #410 Reported-by: W. Mark Kubacki
I noticed that as well but thought it would be better to do that as a separate change. Nevertheless, it seems worthwhile to pursue in this commit, so I've updated the PR. I also included your style changes. |
@jay I assure you I feel nothing but gratitude when you step in and help out! |
Thanks, @jay! |
Daniel Hwang did most of the work so thanks to him :) One more minor thing, I've changed the designator we're using to show the server's pubkey hash from |
- Show how a certificate can be obtained using OpenSSL. Bug: curl#430 Reported-by: Daniel Hwang
Add a "pinnedpubkey" section to the "Server Certificate" verbose
Bug: #410
Reported-by: W. Mark Kubacki