Repository navigation
mbedtls: add more informative logging - #14444
jmaggard10 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
This change seems unrelated and possibly even wrong?
There was a problem hiding this comment.
This is related and correct. The verifypeer check is now done inside the callback function. If verifypeer is false then mbed_verify() will clear the flags the same way that mbed_no_verify did.
We log the cert info inside the callback function now; otherwise we can't log it if cert validation fails, or if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is not enabled.
There was a problem hiding this comment.
What about the previous check for if(!conn_config->verifyhost). Can you just remove it? Also seems to be a change that's not about logging.
There was a problem hiding this comment.
We don't need the verifyhost check here any more. verifyhost and verifypeer are both handled inside the callback function now.
|
About 44 TLS related tests fail. |
091dbea to
241c98c
Compare
@bagder , should all be fixed now. Did a bunch of testing with ASan as well on both mbedtls 2.28 and 3.6 and both look good. |
abfc8bb to
57e8e1c
Compare
There was a problem hiding this comment.
Any particular reason for this value? It would be good to add a note as to how/why you picked it.
There was a problem hiding this comment.
I didn't actually pick 16384. I just kept this value from the original code which I moved from around line 1127. It just needs to be large enough to hold all the cert info in a format that looks like this:
cert. version : 3
serial number : 03:A0:74:D1:87:FE:21:A3:B0:79:06:B7:6C:63:83:59:DF:B1
issuer name : C=US, O=Let's Encrypt, CN=R10
subject name : CN=curl.se
issued on : 2024-06-16 02:36:02
expires on : 2024-09-14 02:36:01
signed using : RSA with SHA-256
RSA key size : 2048 bits
basic constraints : CA=false
subject alt name :
dNSName : curl.se
key usage : Digital Signature, Key Encipherment
ext key usage : TLS Web Server Authentication, TLS Web Client Authentication
certificate policies : ???
16KB does seem larger than necessary. I think we can reduce this if you'd prefer. mbedtls_x509_crt_info() should return MBEDTLS_ERR_X509_BUFFER_TOO_SMALL (-0x2980) and leave the buffer with truncated info if the buffer is too small, so we can also just go ahead and print whatever we have instead of complaining that we're unable to get a dump. Let me know which route you'd prefer and I can make the changes.
After TLS handshare, indicate which TLS version was negotiated in addition to the cipher in the handshake completed log message. Also use the verify callback for certificate logging and collection. This allows things to work even when MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled in the mbedtls library. And lastly, catch certificate validation errors later so we can give the user more informative error messages that indicate what the failure was from certificate validation. Tested on both current LTS versions (2.28 and 3.6).
81c3cfb to
45aff56
Compare
|
Thanks! |
2 participants
After TLS handshare, indicate which TLS version was negotiated in addition to the cipher in the handshake completed log message.
Also use the verify callback for certificate logging and collection. This allows things to work even when
MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled in the mbedtls library.
And lastly, catch certificate validation errors later so we can give the user more informative error messages that indicate what the failure was from certificate validation.
These changes also happen to fix an issue where setting CURLOPT_SSL_VERIFYHOST to 0 did not take effect on MbedTLS 3.6 when TLS 1.3 was enabled.
Tested on both current LTS versions (2.28 and 3.6).