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

ktls: recv alerts #4199

Merged
merged 7 commits into from
Sep 15, 2023
Merged

ktls: recv alerts #4199

merged 7 commits into from
Sep 15, 2023

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Sep 13, 2023

Resolved issues:

resolves #4169

Description of changes:

Add support for receiving alerts.

Unlike send, I didn't use the existing IO callback. Unlike send I can't make any assumptions about the record type, so I'd need to immediately implement a mechanism for passing the record type back from the IO callback as noted in my send PR. For now, this works.

ktls promises that a call to recvmsg will only return data from a single record type, but potentially from multiple records. But that's basically the same guarantee we have reading records normally too: one alert / handshake message can be split across records, or multiple can appear in the same record. With ktls, we just choose our own artificial record size for the read instead of whatever the peer chose :)

Call-outs:

Receiving might not be as straightforward to consolidate with quic support as sending. quic only needs handshake messages, so it can ensure a read of the exact correct size by reading the handshake header first. ktls must also handle application data and alerts, so can't make any assumptions about the size of the data. But I can check the current s2n-quic implementation and see how it'd work with less accurately sized reads-- if it's just reading from a big blob of handshake data, we should be fine. And minimally we can switch between a couple different read_full_record implementations.

Testing:

Unit and self-talk tests.

Locally, I also made a minimal change to s2n_ktls_read_full_record so that it could also be used to (very inefficiently) read application data from s2n_recv. Basically, that works if we assume any data remaining in conn->in when s2n_ktls_read_full_record is called again is application data (basically this logic). With that modification I ran some self-talk tests and they worked. So we could use this for application data, it'd just involve copying and buffering :)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Sep 13, 2023
Comment on lines +415 to +421
/* This method copies data into conn->in, so is intended for control messages
* rather than application data. However in some cases-- such as when attempting
* to read the close_notify alert during s2n_shutdown-- it may encounter application
* data. Set a reasonable conn->in size to avoid an excessive number of calls
* to recvmsg when reading a larger record.
*/
POSIX_GUARD(s2n_stuffer_resize_if_empty(&conn->in, S2N_DEFAULT_FRAGMENT_LENGTH));
Copy link
Contributor Author

@lrstewart lrstewart Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this.

This basically means that we'll read control messages in chunks of S2N_DEFAULT_FRAGMENT_LENGTH, as if we'd received them in records with fragment length == S2N_DEFAULT_FRAGMENT_LENGTH. We do this because without the record header (which ktls consumes) we don't actually know how much data is expected / available, except that ktls guarantees that it won't mix data from different record types.

A couple alternatives:

  • I could initially set the buffer / fragment size to S2N_ALERT_LENGTH and resize larger if application data or handshake messages are encountered. That would mean an additional small recv for application data and handshake messages, but would optimize for alerts?
  • I could make an initial recvmsg call with the MSG_PEEK | MSG_TRUNC flags set, which should let me know how much data is available without actually reading it / needing a correctly sized buffer (according to the documentation-- I haven't tested this with ktls). But this means that even in the happy case, where no application data is encountered, we'd make two system calls. It'd also likely make this implementation significantly harder to consolidate with quic support, because the recv callback has no mechanism to indicate peek vs a real read. MSG_TRUNC is also only available in later versions of Linux, but the same is true of ktls so that's probably not an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could an option be to read into a stack allocated buffer? Then, if the record type is application data, we just ignore it, and otherwise we can copy it into conn->in, since the alerts are small?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh now that is an interesting idea! In fact, we might be able to extend it to alerts and handshake messages too. conn->in usually needs to be allocated on the heap to handle the situation where the application provided output buffer isn't large enough to hold all the application data from a record, so we need to hold onto the rest until the next call. But if we're only interested in alerts and handshake messages, partial alerts are buffered in conn->alert_in and partial handshake messages are buffered in conn->post_handshake.in. So with ktls, conn->in should always be read completely before we call read_full_record again.

Let me play around with this and see if we can allocated conn->in on the stack for ktls. If it gets too complicated, I might make the change in a separate PR like I did for ktls send.

Copy link
Contributor Author

@lrstewart lrstewart Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so:

  • Using stack memory for all messages: not possible without major changes, and not worth the complexity. If we want to only replace read_full_record, then we need to end up with data in conn->in where the rest of our code expects it. We could set conn->in to use stack memory, but that'd need to happen outside of read_full_record to be accessible outside of read_full_record.
  • Using stack memory only to skip application data: not worth the complexity. To do this, we'd need to call recvmsg in a loop instead of just once. This significantly complicates this method, particularly because we don't know whether the next call to recvmsg actually will return more application data. If it doesn't, then we need to copy the control message back into conn->in. It also very arguably changes this from "read_full_record" to "read_multiple_full_records". Everyone calling this already calls it in a loop to process multiple records, so let's let that existing looping do the work.

But I did end up implementing the first option I suggested, the one from #4199 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, reverted for now. I think it'll make more sense if I optimize the memory for receiving app data and control messages at the same time. It might be controversial :)

@lrstewart lrstewart marked this pull request as ready for review September 13, 2023 18:16
tests/unit/s2n_self_talk_ktls_test.c Outdated Show resolved Hide resolved
Comment on lines +415 to +421
/* This method copies data into conn->in, so is intended for control messages
* rather than application data. However in some cases-- such as when attempting
* to read the close_notify alert during s2n_shutdown-- it may encounter application
* data. Set a reasonable conn->in size to avoid an excessive number of calls
* to recvmsg when reading a larger record.
*/
POSIX_GUARD(s2n_stuffer_resize_if_empty(&conn->in, S2N_DEFAULT_FRAGMENT_LENGTH));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could an option be to read into a stack allocated buffer? Then, if the record type is application data, we just ignore it, and otherwise we can copy it into conn->in, since the alerts are small?

tls/s2n_ktls_io.c Show resolved Hide resolved
tls/s2n_ktls_io.c Show resolved Hide resolved
tls/s2n_ktls_io.c Show resolved Hide resolved
tests/unit/s2n_ktls_io_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_self_talk_ktls_test.c Show resolved Hide resolved
@lrstewart lrstewart enabled auto-merge (squash) September 15, 2023 10:43
@lrstewart lrstewart merged commit 87f0c9e into aws:main Sep 15, 2023
@lrstewart lrstewart deleted the ktls_recv branch September 15, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ktls: alert/handshake data
3 participants