search

rustls / rustls-ffi

Use Rustls from any language
Other
123 stars 31 forks source link

documentation of rustls_connection_is_handshaking() #427

Closed icing closed 1 month ago

icing commented 1 month ago

I think it would be helpful to document that rustls_connection_is_handshaking(rconn) in a client returns FALSE before the FINISHED message has been sent.

A client, assuming the handshake is complete and not sending more data to the peer, will timeout since the server continues to wait for FINISHED. With http: connections, this is never noticable since the client will always send a request that causes the FINISHED to be send also.

I debugged curl+rustls on an FTP passive connection, where only the server sends, and there the connection then stalls.

cpu commented 1 month ago

Thanks for opening an issue. Sorry to hear this sent you down a debugging rabbit hole :weary:

It looks like we're directly linking to the backing Rustls fn as the documentation for the rustls-ffi binding:

https://github.com/rustls/rustls-ffi/blob/2b0a45f791771427a17d9440593ecab818cd8f4e/src/rustls.h#L1510-L1513

I think that rustdoc documentation does try to communicate the detail you're noting here when it says:

After Connection::process_new_packets() has been called, this might start to return false while the final handshake packets still need to be extracted from the connection’s buffers.

Did you find that documentation and think it wasn't sufficient? I'm not opposed to lifting the important text directly into the rustls.h docstring but wanted to confirm the existing text in the upstream docs seems OK to you first.

icing commented 1 month ago

Ah, thanks for the pointer. I should have read that more carefully.

packets still need to be extracted from the connection’s buffers does not directly remind one to rustls_connection_write_tls(), so you'd need to know a bit of how rustls-ffi maps he rustls API.

I believe you could add to the link a bit in rustsls.h where it is not super-directly related. Just a thought.

cpu commented 1 month ago

I believe you could add to the link a bit in rustsls.h where it is not super-directly related. Just a thought.

I'll put up a PR with some documentation tweaking shortly :+1:

cpu commented 1 month ago

"shortly" ended up being optimistic, I had a diff laying around but forgot to open a PR :sweat_smile: - https://github.com/rustls/rustls-ffi/pull/430