I/O Abstraction Layer

[alexeblee]

Currently, s2n is coupled with the transport layer and calls read/write directly on a file descriptor set by the parent application. Some applications manage I/O themselves, have restrictions on their TCP stack, use a custom transport layer, or cannot easily conform to a socket-style interface. I would like to add an abstraction layer to allow for additional customization for these types of applications.

Proposed Change

A minimally intrusive way to implement this would be with I/O callbacks. An application could define its own send and receive callbacks and register it with the s2n connection object:

int s2n_connection_set_send_callback(struct s2n_connection *conn, void *io_context, s2n_connection_send send_callback);
int s2n_connection_set_recv_callback(struct s2n_connection *conn, void *io_context, s2n_connection_recv recv_callback);

s2n_flush() and s2n_read_full_record() would be modified to use these callbacks. The callbacks themselves would be of the form:

typedef int s2n_connection_send(void *io_context, struct s2n_stuffer *stuffer, uint32_t len);
typedef int s2n_connection_recv(void *io_context, struct s2n_stuffer *stuffer, uint32_t len);

The format of the "io_context" is unrestricted and can be used to pass information to the custom callbacks. If the user does not explicitly set a callback, the current method of calling s2n_connection_set_fd() must be used. Behind the scenes, s2n_connection_set_fd() would be modified to create an "io_context" containing that file descriptor and register default send/recv callbacks.

[colmmacc]

I really love that this would probably make it easier to write end-to-end unit tests too. No having to wire up fds!

Just an open question to think about: Should we emulate send/recv/read/write or readv/writev instead? There are good reasons we might move to the latter. For example if we want to do low-memory in-place encryption, we might have separate iovec entries for the TLS header, IV, plaintext, and then mac/tag. On the other side though: definitely a bit more complicated as callback functions go.

[alexeblee]

I thought about the readv/writev case, and struggled to wrap my head around what would be an appropriate signature for the callback. The "io_context" object could be used to point to a structure defining the location (and number) of multiple "shared" buffers instead of a single buffer. The callback itself could use vector-based I/O if it wanted and would treat the context appropriately for each situation. Would we want to copy the signature of the readv/writev calls and have the callbacks take a iovec struct, or an array of stuffers? I'm inclined to say an array of stuffers but I could be missing something about the way we intend to use vector-based I/O.

[raycoll]

I like the approach. Clear path to falling back to our current behavior.

Questions:

• Is it necessary to expose s2n_stuffer in the public API? s2n_stuffer is strictly internal right now. It might be excessive to include it with this change.
• How do the callbacks communicate partial reads/writes back to s2n? We're looking into the system errno right now but that doesn't work for all I/O. I think we should re-use s2n_blocked_status . It's already in the public API so users writing callbacks should be comfortable with the pattern.
• How do we report failure back to s2n? The S2N_ERROR macros/error values won't be available to the application writing the callbacks. We could have the callbacks give -1 on any error and then have s2n set S2N_IO_CALLBACK_ERR for the caller.

Re writev/readv: Is there any benefit to making the callback functions iovec-aware? I'm interpreting the callbacks as a simple source/sink for wire data. We could have s2n perform the vectorization outside the callbacks and invoke the callbacks once per element of the vector.

[colmmacc]

I was trying to provoke discussion and it worked - but between the two of you I'm now thinking that alexEBlee's first take is the way to go. Keep it simple for now, and we can factor it into iovec later if we need/want to. Sounds like there's a good bit of subtlety there that we don't need to deal with right now.

[alexw91]

Is this resolved now that #360 has been merged?