Skip to content
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

documentation of rustls_connection_is_handshaking() #427

Closed
icing opened this issue May 17, 2024 · 4 comments · Fixed by #430
Closed

documentation of rustls_connection_is_handshaking() #427

icing opened this issue May 17, 2024 · 4 comments · Fixed by #430

Comments

@icing
Copy link
Collaborator

icing commented May 17, 2024

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
Copy link
Member

cpu commented May 17, 2024

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

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

rustls-ffi/src/rustls.h

Lines 1510 to 1513 in 2b0a45f

/**
* <https://docs.rs/rustls/latest/rustls/struct.CommonState.html#method.is_handshaking>
*/
bool rustls_connection_is_handshaking(const struct rustls_connection *conn);

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
Copy link
Collaborator Author

icing commented May 17, 2024

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
Copy link
Member

cpu commented May 17, 2024

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 👍

@cpu
Copy link
Member

cpu commented May 29, 2024

"shortly" ended up being optimistic, I had a diff laying around but forgot to open a PR 😅 - #430

@cpu cpu closed this as completed in #430 May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants