Parse SOL_TLS control message, closes #2064 #2065
Conversation
There was a problem hiding this comment.
This looks like a cool feature. I've long wanted to play with ktls myself (though in my case, the FreeBSD version), but haven't found an excuse. But before we can accept this, you need to move all of the C-API definitions into the libc crate. And you should also add a CHANGELOG entry.
src/sys/socket/mod.rs
Outdated
| /// `SOL_TLS` messages of type `TLS_GET_RECORD_TYPE`, containing the TLS message content type, | ||
| /// normally one of change_cipher_spec(20), alert(21), handshake(22) (for TLS 1.3 | ||
| /// resumption tickets), application_data(23) | ||
| TlsGetRecordType(u8), |
There was a problem hiding this comment.
I briefly considered it but, for it to be future-proof it'd need an "Unknown" variant. I don't mind it happening, but I feel like there'd be many back-and-forths on the PR until everyone's happy with it.
There was a problem hiding this comment.
I don't think this is good enough. Matching against magic numbers isn't Rusty at all. I think you should add the various types, including an Unknown variant.
There was a problem hiding this comment.
As you wish — there's now an enum with the variants I know/care about.
linux: add a few kTLS defintions related: nix-rust/nix#2065 i think android has support, too, but if I put it in `linux-like` does that imply emscripten support?
fasterthanlime
force-pushed
the
sol_tls
branch
from
004d31c to
964cebf
Compare
|
@asomers twelve years later, the requisite changes have been merged into libc (and released to crates.io), so this PR now bumps the dependency and uses the constants from there. |
|
@asomers is there anything I can do to help move this along? rustls/ktls#24 has been blocked on it for a while (..mostly while waiting for libc to merge+release, but the ball's in nix's court now). |
|
Update: I'm maintaining a fork of just this bit of nix now: https://crates.io/crates/ktls-recvmsg (I'd like to deprecate that fork eventually) |
src/sys/socket/mod.rs
Outdated
| @@ -1015,6 +1020,11 @@ impl ControlMessageOwned { | |||
| let dl = ptr::read_unaligned(p as *const libc::sockaddr_in6); | |||
| ControlMessageOwned::Ipv6OrigDstAddr(dl) | |||
| }, | |||
| #[cfg(all(target_os = "linux"))] | |||
There was a problem hiding this comment.
This should also include android.
There was a problem hiding this comment.
Fine, but be warned I have not tested it on Android at all nor do I plan on maintaining/supporting that platform personally. My general opinion is that it's better to wait for someone who needs it, and then they can be a point of contact re: getting it working on that platform. That person isn't me for Android.
There was a problem hiding this comment.
I had to revert this, since the libc constants for this are not available on android (my fault, since I did that PR too, but I'm not willing to block this PR on this).
There was a problem hiding this comment.
Oops. I thought the constant was available on Android in libc. I guess I grepped wrong. Sorry.
src/sys/socket/mod.rs
Outdated
| /// `SOL_TLS` messages of type `TLS_GET_RECORD_TYPE`, containing the TLS message content type, | ||
| /// normally one of change_cipher_spec(20), alert(21), handshake(22) (for TLS 1.3 | ||
| /// resumption tickets), application_data(23) | ||
| TlsGetRecordType(u8), |
There was a problem hiding this comment.
I don't think this is good enough. Matching against magic numbers isn't Rusty at all. I think you should add the various types, including an Unknown variant.
fasterthanlime
force-pushed
the
sol_tls
branch
from
8bfb971 to
981b07e
Compare
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | ||
| #[repr(u8)] | ||
| #[non_exhaustive] | ||
| pub enum TlsGetRecordType { |
There was a problem hiding this comment.
Edit: nvm, I confused the content type with the handshake type.
There was a problem hiding this comment.
fwiw, re: your original comment:
That said, and I know @asomers asked for it, but I don't think this belongs in the nix crate because it's about TLS, not libc or Linux.
I agree with this, but I'm willing to do whatever to get this PR merged at this point. I agree the API boundaries are kinda unclear right now, after all libc itself is a grab bag 🤷
...since the corresponding libc constant isn't gated for Android.
|
Please see |
|
Thanks! |
cf. #2064 - I've added the absolute minimum I need. We could swap that u8 with something more strongly-typed, but since I'm clearly the first person using ktls & the nix crate together, I'm treading lightly in terms of added lines.