On 1/13/2016 1:06 AM, Thomas Glanzmann wrote:
> Manuel has answered:
>
>> Ok, so I'll start with the easy things: we do support both ID-based
>> (old-style) and ticket-based (RFC 5077) session resumption. From the
>> client perspective, the API for both is exactly the same (the only
>> difference being the size of the state stored): you first save a
>> session with mbedtls_ssl_get_session() then load it to another context
>> using mbedtls_ssl_set_session() before you perform a handshake with
>> that new context, and the handshake will resume the previous session
>> if the server is OK with that. For an example, you can look at what is
>> done in ssl_client2.c with saved_session. In practice before you
>> resume, instead of using mbedtls_ssl_session_reset() as we do, if
>> you're using a fresh ssl_context you'll just call mbedtls_ssl_setup()
>> before mbedtls_ssl_set_session().

I thought we already do this [1], I'm at a loss for why session resume
doesn't work if in fact it doesn't. I recall it did appear to resume for
me, probably I had a different google server than you.

[1]:
https://github.com/bagder/curl/blob/curl-7_46_0/lib/vtls/mbedtls.c#L351-L354

>> Regarding the peer certificate issue, currently the documentation for
>> mbedtls_ssl_get_session() says the peer's certificate is lost in the
>> operation which is no longer correct, though it used to be (I think
>> the new behaviour was introduced in 1.3.0). The current behaviour is
>> that the peer's end entity certificate is saved, though the rest of
>> the chain is not. For HPKP this might be a problem as the pinned key
>> could be anywhere in the chain. OTOH, out of the top of my head, I'm
>> really not sure you need to check the pinning again on resumed
>> sessions: a man-in-the-middle wouldn't be able to resume the session
>> anyway. So you probably want to check skip HPKP verification for
>> resumed sessions, which make the lack of cert chain a non-issue.

Ok we should be fine then since 1.3.0 is when it was polarssl not
mbedtls. As far as checking each key in the chain, and your reply in
that thread [2] where you said you would implement pinning not only for
the peer cert (I assume you want to for every cert in the chain), note
that is not the way it is implemented for the other backends.

The CURLOPT_PINNEDPUBLICKEY documentation [3] says "the server sends a
certificate indicating its identity. A public key is extracted from this
certificate and if it does not exactly match the public key provided to
this option, curl will abort the connection before sending or receiving
any data. " Therefore I'd bring up chain pinning as a separate
discussion if you want to see it implemented.

Further, to your comments in a previous e-mail about pinning, it was
intended that the pinning check takes place even if verifypeer is
disabled. Though this is not explicitly documented and there's no test
for it (I'm working on that, haven't forgotten) that is the way it
should be implemented across all backends including OpenSSL.
Specifically in reference to the line you cited in openssl.c [4] there's
no return there so I think you misread it.

[2]: https://tls.mbed.org/discussions/generic/resumed-tls-handshake
[3]: http://curl.haxx.se/libcurl/c/CURLOPT_PINNEDPUBLICKEY.html
[4]: https://github.com/bagder/curl/blob/master/lib/vtls/openssl.c#L2654

>> Finally, regarding the fact that mbedtls_pk_write_pubkey_der() expects
>> a non-const pk_context, I think it is a bug on our side: that function
>> should work well with a const public key, and I'm afraid we just
>> forgot the const in the prototype :( So I think it should work if you
>> just cast the const away. If you want a more correct solution, then
>> the easiest thing to make a non-const copy of is the certificate, by
>> calling mbedtls_x509_crt_parse_der() on c->raw.p, c->raw.len if c is
>> the pointer returned by mbedtls_ssl_get_peer_cert().

I'd play it safe and do the copy.

I'd also play it safe with regard to his comment in the thread about no
different cert on session resume. I don't know that that's certain and
it seems he doesn't either. Another certificate could be provided in the
meantime during the handshake resume before we get to the pinned key
verify point (I think..). So I think we should not remove the check
unless we know for certain. Downside is a slight performance hit on
resume because we're verifying the pinned peer key again.

Basically my takeaway here is the change I've proposed is fine should
you want to go that way, and as a separate issue that session resume
needs to be checked to make sure it's working properly because of the
issue you observed.

Thanks to Manuel for his thorough reply, and thanks to you Thomas for
the work you are doing.

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2016-01-13