ktls: mock send/recvmsg IO #4109
Conversation
toidiu
force-pushed
the
ak-ktls0_stufferIO
branch
6 times, most recently
from
c6ad9ce to
c1dc534
Compare
toidiu
force-pushed
the
ak-ktls0_stufferIO
branch
2 times, most recently
from
fcc56b2 to
950569f
Compare
toidiu
force-pushed
the
ak-ktls0_stufferIO
branch
2 times, most recently
from
55fea0c to
d400d46
Compare
toidiu
force-pushed
the
ak-ktls0_stufferIO
branch
2 times, most recently
from
97c5ac6 to
d11b6ab
Compare
tls/s2n_ktls_io.c
Outdated
| static ssize_t s2n_ktls_default_recvmsg(void *io_context, struct msghdr *msg, uint8_t *record_type) | ||
| { | ||
| POSIX_ENSURE_REF(io_context); | ||
| POSIX_ENSURE_REF(msg); | ||
| POSIX_ENSURE_REF(record_type); | ||
|
|
||
| int fd = 0; | ||
| const struct s2n_socket_read_io_context *peer_socket_ctx = io_context; | ||
| fd = peer_socket_ctx->fd; | ||
|
|
||
| return recvmsg(fd, msg, 0); | ||
| } |
There was a problem hiding this comment.
This still doesn't look quite right to me. You're not using record_type. Shouldn't the record type be parsed from msg by the caller? recvmsg only handles msg. If you put the logic for parsing msg in s2n_ktls_default_recvmsg, then you'll bypass it in all your unit tests.
You should only be mocking recvmsg, not any of your own logic. Same for sendmsg.
There was a problem hiding this comment.
This PR doesnt add a working definition of the default send/recvmsg; that comes in the following PRs. It only adds things necessary for mock stuffer impl.
There was a problem hiding this comment.
Yes, but shouldn't the only thing necessary for the mock be io_context and msg? record_type isn't relevant to recvmsg.
There was a problem hiding this comment.
technically recvmsg handles both data + ancillary. recv handles data only.
You are right, that its possible to write and parse the record type from msghdr but I can also mock that out and not deal with CMSG_* macro and allocation. I can also then test that functionality later in isolation.
Since this PR is large enough I am not inclined to add the writing/parsing CMSG logic to this PR. It makes more sense to add it in a following PR. Since this code is only used for testing, this should be ok.
There was a problem hiding this comment.
More importantly I think the reading/parsing CMSG deserves its own PR. Swapping this impl for the more complicated one will also serve as a good A/B test oracle.
There was a problem hiding this comment.
I agree that this mock interface doesn't seem correct. What's the purpose of passing the record type here? If it's just for the purpose of the mock impls that seems like the wrong path since it'll make the boundaries less clear. BTW you've already implemented setting/getting CMSG data since you have to do that anyway with the syscalls; just in reverse.
There was a problem hiding this comment.
I'll be handling this in a followup PR: https://github.com/aws/s2n-tls/pull/4109/files#diff-c9867e54a8d879980424cab3426b2d455215b438faedcaad673fbf33b1cd7aecR52
addressed in f53ac8d
There was a problem hiding this comment.
Had some more discussions and I have added clarification for why this PR chooses to represent msg_control as uint8_t . c051aae
Here is a rough idea of what the next PR will look like:
It adds the s2n_ktls_send and set_ancillary methods and starts to represent the data on the send side as cmsghdr: https://github.com/aws/s2n-tls/compare/main...toidiu:s2n-tls:ak-ktls0_cmsg_send1?expand=1.
After that there will be a PR for the recv code path, thus making incremental progress towards what the final mock will look like.
toidiu
force-pushed
the
ak-ktls0_stufferIO
branch
from
65e8914 to
e76e610
Compare
toidiu
force-pushed
the
ak-ktls0_stufferIO
branch
from
6d251a7 to
8b0e8d0
Compare
Description of changes:
kTLS uses sendmsg/recvmsg instead of send/recv like in normal TLS. Since kTLS has to be a dropin replacement for how we currently handle s2n_send and s2n_recv, we need to be able to test that functionality.
In this PR we mock out the sendmsg and recvmsg syscalls to give us full control over the IO operation and help us test that kTLS is infact a dropin replacement.
Callouts:
Note the main purpose of this PR is to add the testing framework for testing sendmsg and recvmsg.
Testing:
I have added some sanity testing but very specific operations (send/recv multiple records, partial reads, blocked) will be tested when in a following PR when I actually add the send/recvmsg operations.
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.