On Wed, 2025-09-03 at 10:21 +0200, Sabrina Dubroca wrote: > 2025-09-02, 22:50:53 +0000, Wilfred Mallawa wrote: > > On Tue, 2025-09-02 at 18:07 +0200, Sabrina Dubroca wrote: > > > 2025-09-02, 13:38:10 +1000, Wilfred Mallawa wrote: > > > > From: Wilfred Mallawa <wilfred.mallawa@xxxxxxx> > > Hey Sabrina, > > > A selftest would be nice (tools/testing/selftests/net/tls.c), but > > > I'm > > > not sure what we could do on the "RX" side to check that we are > > > respecting the size restriction. Use a basic TCP socket and try > > > to > > > parse (and then discard without decrypting) records manually out > > > of > > > the stream and see if we got the length we wanted? > > > > > So far I have just been using an NVMe TCP Target with TLS enabled > > and > > checking that the targets RX record sizes are <= negotiated size in > > tls_rx_one_record(). I didn't check for this patch and the bug > > below > > got through...my bad! > > > > Is it possible to get the exact record length into the testing > > layer? > > Not really, unless we come up with some mechanism using probes. I > wouldn't go that route unless we don't have any other choice. > > > Wouldn't the socket just return N bytes received which doesn't > > necessarily correlate to a record size? > > Yes. That's why I suggested only using ktls on one side of the test, > and parsing the records out of the raw stream of bytes on the RX > side. > Ah okay I see. > Actually, control records don't get aggregated on read, so sending a > large non-data buffer should result in separate limit-sized reads. > But > this makes me wonder if this limit is supposed to apply to control > records, and how the userspace library/application is supposed to > deal > with the possible splitting of those records? > Good point, from the spec, "When the "record_size_limit" extension is negotiated, an endpoint MUST NOT generate a protected record with plaintext that is larger than the RecordSizeLimit value it receives from its peer. Unprotected messages are not subject to this limit." [1] >From what I understand, as long as it in encrypted. It must respect the record size limit? In regards to user-space, do you mean for TX or RX? For TX, there shouldn't need to be any changes as record splitting occurs in the kernel. For RX, I am not too sure, but this patch shouldn't change anything for that case? [1] https://datatracker.ietf.org/doc/html/rfc8449#section-4 > > Here's a rough example of what I had in mind. The hardcoded cipher > overhead is a bit ugly but I don't see a way around it. Sanity check > at the end is probably not needed. I didn't write the loop because I > haven't had enough coffee yet to get that right :) > Ha! Great! Thanks for the example. I am not too familiar with the self tests in the kernel. But will try to it for the next round. Thanks, Wilfred > > TEST(tx_record_size) > { > struct tls_crypto_info_keys tls12; > int cfd, ret, fd, len, overhead; > char buf[1000], buf2[2000]; > __u16 limit = 100; > bool notls; > > tls_crypto_info_init(TLS_1_2_VERSION, > TLS_CIPHER_AES_CCM_128, > &tls12, 0); > > ulp_sock_pair(_metadata, &fd, &cfd, ¬ls); > > if (notls) > exit(KSFT_SKIP); > > /* Don't install keys on fd, we'll parse raw records */ > ret = setsockopt(cfd, SOL_TLS, TLS_TX, &tls12, tls12.len); > ASSERT_EQ(ret, 0); > > ret = setsockopt(cfd, SOL_TLS, TLS_TX_RECORD_SIZE_LIM, > &limit, sizeof(limit)); > ASSERT_EQ(ret, 0); > > EXPECT_EQ(send(cfd, buf, sizeof(buf), 0), sizeof(buf)); > close(cfd); > > ret = recv(fd, buf2, sizeof(buf2), 0); > memcpy(&len, buf2 + 3, 2); > len = htons(len); > > /* 16B tag + 8B IV -- record header (5B) is not counted but > we'll need it to walk the record stream */ > overhead = 16 + 8; > > // TODO should be <= limit since we may not have filled > every > // record (especially the last one), and loop over all the > // records we got > // next record starts at buf2 + (limit + overhead + 5) > ASSERT_EQ(len, limit + overhead); > /* sanity check that it's a TLS header for application data > */ > ASSERT_EQ(buf2[0], 23); > ASSERT_EQ(buf2[1], 0x3); > ASSERT_EQ(buf2[2], 0x3); > > close(fd); > } >
