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

Add client handshake example #134

Open
kevinburke opened this issue Sep 14, 2021 · 4 comments
Open

Add client handshake example #134

kevinburke opened this issue Sep 14, 2021 · 4 comments

Comments

@kevinburke
Copy link
Contributor

kevinburke commented Sep 14, 2021

The current model in tests/client.c has a workflow that looks like:

  1. Create a rustls_connection (which does not involve reading or writing from the socket)
  2. Write unencrypted bytes to the connection
  3. In a for loop, alternate writes and reads based on the socket state and connection state. The handshake is performed implicitly, followed by the write of the unencrypted bytes.

Some of the C libraries I've seen have a different workflow - they have API's that look like

  1. Socket connection and TLS handshake (cr_connect_nonblocking in curl)
  2. API for writing data (cr_send in curl, pgtls_write in Postgres libpq/fe-secure.c)
  3. API for reading data (cr_recv in curl, pgtls_read in Postgres)

It might be helpful to add a second example, or break out the logic in the current example so the handshake is performed separately. Especially since as implemented in vtls/rustls.c, the logic to perform a handshake is not obvious (call the write method with an empty buffer), and there's no explicit "handshake" method in crustls.h

Even if it's the same workflow under the hood, conceptually this might help when porting code that currently exists in C.

@kevinburke
Copy link
Contributor Author

More broadly: I had just assumed that if I was implementing a method named write or send or tls_write, I would be sending data on the socket in one direction only. I'm not sure if I'm weird because I've never done C or socket programming before and most folks intuit this, or what the right fix for this would be to help folks mental models.

Some ideas though:

  • add a tls_handshake method to rustls-ffi that invokes read/write on the socket with an empty buffer, similar to what cr_connect_nonblocking is doing.
  • update the client docs to make clear that the reads/writes interleave a lot and not just if you are doing, like, a chunked download
  • update the crustls.h docs to make clear that rustls_connection_write_tls may be invoked with an empty buffer and will negotiate the handshake if it needs to.

@jsha
Copy link
Collaborator

jsha commented Nov 9, 2021

It seems like this a relevant comment in lib/vtls/rustls.c in the curl repo:

    /*
    * Connection has been established according to rustls. Set send/recv
    * handlers, and update the state machine.
    * This check has to come last because is_handshaking starts out false,
    * then becomes true when we first write data, then becomes false again
    * once the handshake is done.
    */

That is indeed pretty unintuitive behavior. We should at a minimum document that on rustls_connection_is_handshaking. And I think you're right - we should have a nice example in-repo of how to do it.

Weirdly, some of the other getters like negotiated_cipher_suite may have more intuitive behavior to check for handshake completion - they return None until the handshake is complete, rather than false, true, false.

add a tls_handshake method to rustls-ffi that invokes read/write on the socket with an empty buffer, similar to what cr_connect_nonblocking is doing.

I don't see where cr_connect_nonblocking writes to an empty buffer. Can you point it out to me?

@jsha
Copy link
Collaborator

jsha commented Nov 19, 2021

  • This check has to come last because is_handshaking starts out false,
  • then becomes true when we first write data, then becomes false again
  • once the handshake is done.

I checked, and this is not accurate. is_handshaking starts out true:

rustls-ffi/src/client.rs

Lines 615 to 620 in d7a8692

assert_eq!(rustls_connection::rustls_connection_wants_read(conn), false);
assert_eq!(rustls_connection::rustls_connection_wants_write(conn), true);
assert_eq!(
rustls_connection::rustls_connection_is_handshaking(conn),
true
);
.

I don't think that behavior changed. I suspect when I wrote that comment, I had just misinterpreted something I'd observed. Additionally, the comment is on a check that is actually first in its loop, so it's not even doing what it says. I'll send a PR.

@kevinburke
Copy link
Contributor Author

I don't see where cr_connect_nonblocking writes to an empty buffer. Can you point it out to me?

I think what I meant by this is, while it is reading and writing data, it's all metadata and connection overhead, it's not writing any of the data that the client wants to send to the server.

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

No branches or pull requests

2 participants