Add `SetSockOpt` definitions related to Linux Kernel TLS. #2175

Arnavion · 2023-11-03T05:49:24Z

kTLS is implemented as a TCP ULP, so add a SetSockOpt impl for it.
kTLS parameters are pushed to the kernel using setsockopt(SOL_TLS),
so add SetSockOpt impls for them.

Other test fixes:

Change setsockopt tests to bind to port 0 so that
they bind to an unbound port and don't affect each other.
Fix skip! macro used to skip tests to generate its own block so that
it can be used as an expr.

What does this PR do

Implement additional definitions needed to set up kTLS.

Checklist:

I have read CONTRIBUTING.md
I have written necessary tests and rustdoc comments
A change log has been added if this PR modifies nix's API

Arnavion · 2023-11-03T05:59:37Z

The Linux CI did fail with EADDRINUSE. I did notice when I was writing the test that all the setsockopt tests that did bind were using the same port, and I wondered how it was that they never stepped on each other. I guess it was just luck that nobody hit it before.

Should I change my test and all the others to use unique ports?

asomers

Yes, you should change the port number. Also, would it be possible to add some docs? It isn't obvious how to use this. And is there any overlap with PR #2065 ?

Arnavion · 2023-11-04T16:55:18Z

Yes, you should change the port number.

Will do.

Also, would it be possible to add some docs? It isn't obvious how to use this.

The documentation is https://docs.kernel.org/networking/tls.html . I'll add comments explaining which setsockopt call each of the structs maps to.

And is there any overlap with PR #2065 ?

No. The steps of using kTLS are:

Set up TCP connection.
Do TLS handshake however you want, say using rustls or openssl.
Enable the TLS ULP on the TCP socket. (setsockopt(TcpUlp, b"tls\0") from this PR.)
Import the TLS secrets for sending and receiving to the TCP socket. (setsockopt(TcpTlsTx, ...) and setsockopt(TcpTlsRx, ...) from this PR.)
Read and write to the TCP socket using regular socket syscalls. Reading should use recvmsg because the kernel will send cmsg's with level == libc::SOL_TLS and type == libc::TLS_GET_RECORD_TYPE whenever there is a non-application data / post-handshake record. (The recvmsg changes in Parse SOL_TLS control message, closes #2064 #2065 )

asomers · 2023-11-04T23:35:38Z

There are a bunch of tests that don't pass under cross. If you're sure that this test failure is cross-induced, you could skip the test in CI by adding #[cfg_attr(qemu, ignore)].

Arnavion · 2023-11-04T23:37:42Z

There are a bunch of tests that don't pass under cross. If you're sure that this test failure is cross-induced, you could skip the test in CI by adding #[cfg_attr(qemu, ignore)].

Yes, that is what I had been battling with. The last commit I pushed finally fixes them.

src/sys/socket/sockopt.rs

Cargo.toml

SteveLauC · 2023-12-04T03:01:24Z

Hi @Arnavion, we now use the libc dependency from git, rebasing your branch should make things work

Arnavion · 2023-12-04T03:35:16Z

Done, and verified with my test program.

asomers · 2023-12-08T19:04:07Z

src/sys/socket/sockopt.rs

+/// ```ignore,rust
+/// setsockopt(sock, TcpUlp::default(), b"tls\0");
+/// ```
+#[cfg(target_os = "linux")]


The TCP_ULP and SOL_TCP symbols are defined on Android as well as Linux. So here and elsewhere, you should do

Suggested change

#[cfg(target_os = "linux")]

#[cfg(linux_android)]

Those two symbols are also defined on Fuchsia, but stuff like tls12_crypto_info_aes_gcm_128 isn't, so I guess you should leave Fuchsia out.

The TLS ULP is useless without being able to do TLS_TX and TLS_RX, and I only added all this in libc for linux. Someone who uses Android should extend it to Android.

Ah, but you're right that there's no harm in enabling other ULPs at least. Done.

asomers · 2023-12-08T19:24:36Z

test/sys/test_sockopt.rs

+        // Some architectures running under cross don't support `setsockopt(SOL_TCP, TCP_ULP)`
+        // because the cross image is based on Ubuntu 16.04 which predates TCP ULP support
+        // (it was added in kernel v4.13 released in 2017). For these architectures,
+        // the `setsockopt(SOL_TCP, TCP_ULP, "tls", sizeof("tls"))` call succeeds
+        // but the subsequent `setsockopt(SOL_TLS, TLS_TX, ...)` call fails with `ENOPROTOOPT`.
+        // It's as if the first `setsockopt` call enabled some other option, not `TCP_ULP`.
+        // For example, `strace` says:
+        //
+        //     [pid   813] setsockopt(4, SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4) = 0
+        //
+        // It's not clear why `setsockopt(SOL_TCP, TCP_ULP)` succeeds if the container image libc doesn't support it,
+        // but in any case we can't run the test on such an architecture, so skip it.
+        Err(nix::Error::ENOPROTOOPT) => skip!("TCP ULP are not supported."),


It might be simpler to delete this part, and just add #[cfg_attr(qemu, ignore)] to the top of the function.

SteveLauC · 2023-12-16T12:04:48Z

src/sys/socket/sockopt.rs

+/// ... and the `nix` equivalent is:
+///
+/// ```ignore,rust
+/// setsockopt(sock, TcpUlp::default(), b"tls\0");


I am curious, in the C interface, does this "tls" argument always have the type C string (having a NUL at the end) or it can be some random bytes

The value is the name of the ULP. You can look at the ones you have available with cat /proc/sys/net/ipv4/tcp_available_ulp.

The string does not need to be NUL-terminated. This is leftover from when I was trying to make the CI work and I thought the lack of NUL terminator might be the reason armv7 was failing. I confirmed from the kernel code that it only copies the user-provided string up to the smaller of user-provided length and 15 bytes (TCP_ULP_NAME_MAX - 1) and adds its own NUL terminator, and it also works without the trailing NUL in my test program.

Removed the trailing NUL from this example and from the test.

I confirmed from the kernel code that it only copies the user-provided string up to the smaller of user-provided length and 15 bytes (TCP_ULP_NAME_MAX - 1) and adds its own NUL terminator.

Thanks for digging into the kernel code, do you think it would be great if we could have a comment in the code describing this behavior?

Thanks for doing it, and you don't need to squash commits as we will do squash merge.

SteveLauC

LGTM, but maybe @asomers also wants to finish his review

Arnavion · 2023-12-22T06:36:00Z

@asomers @SteveLauC Ping.

SteveLauC · 2023-12-23T03:23:39Z

Normally, asomers will maintain Nix on Friday/weekend

1. kTLS is implemented as a TCP ULP, so add a `SetSockOpt` impl for it. 2. kTLS parameters are pushed to the kernel using `setsockopt(SOL_TLS)`, so add `SetSockOpt` impls for them. Other test fixes: 1. Change `setsockopt` tests to bind to port 0 so that they bind to an unbound port and don't affect each other. 2. Fix `skip!` macro used to skip tests to generate its own block so that it can be used as an expr.

Arnavion · 2024-01-02T17:59:20Z

@SteveLauC Ping. This has been ready to merge for two weeks now, and I'm tired of fixing all the merge conflicts from the other newer PRs that keep getting merged :)

This is also blocking more fixes I want to make to the GetSockOpt / SetSockOpt traits.

SteveLauC · 2024-01-03T01:08:09Z

Understand, let's merge this!

asomers requested changes Nov 4, 2023

View reviewed changes

SteveLauC reviewed Nov 5, 2023

View reviewed changes

src/sys/socket/sockopt.rs Show resolved Hide resolved

Arnavion commented Nov 9, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

SteveLauC added the W-blocking-upstream label Nov 10, 2023

SteveLauC removed the W-blocking-upstream label Dec 4, 2023

Arnavion force-pushed the ktls branch from df6fb77 to 7102648 Compare December 4, 2023 03:23

Arnavion marked this pull request as ready for review December 4, 2023 03:35

Arnavion requested review from asomers and SteveLauC December 4, 2023 18:02

asomers requested changes Dec 8, 2023

View reviewed changes

Arnavion requested a review from asomers December 12, 2023 00:31

SteveLauC reviewed Dec 16, 2023

View reviewed changes

Arnavion force-pushed the ktls branch from 67ffadc to 7cb43da Compare December 16, 2023 19:34

Arnavion requested a review from SteveLauC December 16, 2023 22:18

SteveLauC approved these changes Dec 17, 2023

View reviewed changes

Arnavion force-pushed the ktls branch from d59b5f9 to 3bfa621 Compare December 31, 2023 16:59

SteveLauC added this pull request to the merge queue Jan 3, 2024

Merged via the queue into nix-rust:master with commit 57c611c Jan 3, 2024
35 checks passed

Arnavion deleted the ktls branch January 3, 2024 01:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SetSockOpt` definitions related to Linux Kernel TLS. #2175

Add `SetSockOpt` definitions related to Linux Kernel TLS. #2175

Arnavion commented Nov 3, 2023 •

edited

Arnavion commented Nov 3, 2023

asomers left a comment

Arnavion commented Nov 4, 2023 •

edited

asomers commented Nov 4, 2023

Arnavion commented Nov 4, 2023

SteveLauC commented Dec 4, 2023

Arnavion commented Dec 4, 2023

asomers Dec 8, 2023

Arnavion Dec 8, 2023

Arnavion Dec 8, 2023 •

edited

asomers Dec 8, 2023

Arnavion Dec 8, 2023

SteveLauC Dec 16, 2023 •

edited

Arnavion Dec 16, 2023

Arnavion Dec 16, 2023

SteveLauC Dec 17, 2023

Arnavion Dec 17, 2023

SteveLauC Dec 17, 2023

SteveLauC left a comment

Arnavion commented Dec 22, 2023

SteveLauC commented Dec 23, 2023

Arnavion commented Jan 2, 2024

SteveLauC commented Jan 3, 2024

Add SetSockOpt definitions related to Linux Kernel TLS. #2175

Add SetSockOpt definitions related to Linux Kernel TLS. #2175

Conversation

Arnavion commented Nov 3, 2023 • edited

What does this PR do

Checklist:

Arnavion commented Nov 3, 2023

asomers left a comment

Choose a reason for hiding this comment

Arnavion commented Nov 4, 2023 • edited

asomers commented Nov 4, 2023

Arnavion commented Nov 4, 2023

SteveLauC commented Dec 4, 2023

Arnavion commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arnavion Dec 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLauC Dec 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLauC left a comment

Choose a reason for hiding this comment

Arnavion commented Dec 22, 2023

SteveLauC commented Dec 23, 2023

Arnavion commented Jan 2, 2024

SteveLauC commented Jan 3, 2024

Add `SetSockOpt` definitions related to Linux Kernel TLS. #2175

Add `SetSockOpt` definitions related to Linux Kernel TLS. #2175

Arnavion commented Nov 3, 2023 •

edited

Arnavion commented Nov 4, 2023 •

edited

Arnavion Dec 8, 2023 •

edited

SteveLauC Dec 16, 2023 •

edited