KTLS: Enable KTLS for receiving as well in TLS 1.3 #16798

ueno · 2021-10-10T07:20:29Z

This series removes the restriction of not enabling KTLS for receiving in TLS 1.3. Some adjustments to ssl3_get_record are needed to handle unprotected records provided by the kernel properly.

Checklist

documentation is added or updated
tests are added or updated

This adds a couple of special cases in ssl3_get_record() for KTLS: first, record unpadding is handled by the kernel and the type and length fields of the received SSL3_RECORD should have the correct value; secondly, the record type check for TLS 1.3 records is not relevant in KTLS, as the kernel provides unprotected record content. Signed-off-by: Daiki Ueno <dueno@redhat.com>

This removes a guard condition that prevents KTLS being enabled for receiving in TLS 1.3. Signed-off-by: Daiki Ueno <dueno@redhat.com>

bsdjhb · 2021-11-15T19:05:17Z

Hmm, in the TLS < 1.3 case, there is an additional check to ensure that there aren't partially-received TLS records in the read-ahead cache of the read BIO. This is used to update the sequence number provided to the kernel as well as to reject KTLS if the read-ahead cache contains partially-read (vs complete) TLS records. I don't see a similar check here, so how are you handling that edge case? (For the < 1.3 case see the code to use count_unprocessed_records in t1_enc.c).

For the tests, I also wonder if we want a OPENSSL_NO_KTLS_RX_13 or not. I'd mostly like to avoid adding more mazes of #ifdef's in the tests, but we'd need to make the tests not consider it a failure if a given (cipher suite, direction) tuple fails to offload to avoid false negatives on older kernels.

bsdjhb · 2021-11-15T19:12:05Z

ssl/record/ssl3_record.c

-                if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) {
+                if (SSL_IS_TLS13(s)
+                        && s->enc_read_ctx != NULL
+                        && !BIO_get_ktls_recv(s->rbio)) {
                    if (thisrr->type != SSL3_RT_APPLICATION_DATA
                            && (thisrr->type != SSL3_RT_CHANGE_CIPHER_SPEC
                                || !SSL_IS_FIRST_HANDSHAKE(s))


GitHub's UI won't let me add this on the correct line, but I believe that for KTLS we should adjust the TLS1.3 specific check below on like 396 as for KTLS the extra byte for the record length has been stripped and the length should be validated against SSL3_RT_MAX_ENCRYPTED_LENGTH instead?

bsdjhb · 2021-11-15T19:14:41Z

ssl/record/ssl3_record.c

+
+                thisrr->length = end;
+                thisrr->type = thisrr->data[end];
+                if (thisrr->type != SSL3_RT_APPLICATION_DATA


Do we still wish to do this check of the "real" record type even in the KTLS case? In my WIP for FreeBSD KTLS while I validate the outer record type, I am currently just returning the inner record type without performing additional checks on the type in the kernel. Does Linux perform validation of the inner record type in the kernel?

pasis · 2022-01-14T13:48:40Z

ssl/record/ssl3_record.c

            }
            if (s->msg_callback)
                s->msg_callback(0, s->version, SSL3_RT_INNER_CONTENT_TYPE,
-                                &thisrr->data[end], 1, s, s->msg_callback_arg);
+                                &thisrr->data[thisrr->length], 1, s,


Is this correct for KTLS? AFAIU, &thisrr->data[thisrr->length] won't contain the inner type.

I agree with @pasis. It should probably be &thisrr->type which will work for both KTLS and non-KTLS as the non-KTLS case has already updated thisrr->type at this point.

yangyangtiantianlonglong · 2022-01-18T07:52:24Z

ssl/tls13_enc.c

@@ -723,8 +722,10 @@ int tls13_change_cipher_state(SSL *s, int which)
        goto skip_ktls;


if (which & SSL3_CC_WRITE) rl_sequence = RECORD_LAYER_get_write_sequence(&s->rlayer); else rl_sequence = RECORD_LAYER_get_read_sequence(&s->rlayer); if (!ktls_configure_crypto(s, cipher, ciph_ctx, r1_sequence, &crypto_info, NULL, iv, key, NULL, 0))

I agree with @yangyangtiantianlonglong's suggestion.

yangyangtiantianlonglong · 2022-02-15T10:10:27Z

openssl/ssl/t1_enc.c

Lines 480 to 498 in 065121f

    
               if (which & SSL3_CC_READ) { 
        
           # ifndef OPENSSL_NO_KTLS_RX 
        
                   count_unprocessed = count_unprocessed_records(s); 
        
                   if (count_unprocessed < 0) 
        
                       goto skip_ktls; 
        
                   /* increment the crypto_info record sequence */ 
        
                   while (count_unprocessed) { 
        
                       for (bit = 7; bit >= 0; bit--) { /* increment */ 
        
                           ++rec_seq[bit]; 
        
                           if (rec_seq[bit] != 0) 
        
                               break; 
        
                       } 
        
                       count_unprocessed--; 
        
                   } 
        
           # else 
        
                   goto skip_ktls; 
        
           # endif 
        
               }

If read_ahead is set for receiving in TLS 1.3. The unprocess record needs to be performed ?

bsdjhb · 2022-02-23T01:26:16Z

openssl/ssl/t1_enc.c

Lines 480 to 498 in 065121f

if (which & SSL3_CC_READ) {

# ifndef OPENSSL_NO_KTLS_RX

count_unprocessed = count_unprocessed_records(s);

if (count_unprocessed < 0)

goto skip_ktls;

/* increment the crypto_info record sequence */

while (count_unprocessed) {

for (bit = 7; bit >= 0; bit--) { /* increment */

++rec_seq[bit];

if (rec_seq[bit] != 0)

break;

}

count_unprocessed--;

}

# else

goto skip_ktls;

# endif

}

If read_ahead is set for receiving in TLS 1.3. The unprocess record needs to be performed ?

I think so? That is what I was asking about earlier in my first comment. I guess I should see if this edge case can be provoked via a small test case.

bsdjhb · 2022-02-24T20:52:35Z

I've pushed a branch with fixes for the various issues here along with some other bugs I noticed (e.g. not using the right bio for RX) here: master...bsdjhb:ktls_rx_tls13

The history of that branch would probably need to be squashed a bit to be a better commit candidate.

pasis · 2022-03-03T16:23:43Z

@bsdjhb, both original PR and your branch have an issue when count_unprocessed_records() > 0. TLS1.2 RX works well, but TLS1.3 returns a corrupted buffer which doesn't start from GET/POST method in an HTTPS request.

I get this scenario in the most runs of NGINX with WRK benchmark. It's enough to run WRK with a single thread and single connection to reproduce.

It looks like for TLS1.3 openssl doesn't handle encrypted records that are received before KTLS configuration is finished. Unfortunately I don't have enough expertise to find the root cause.

bsdjhb · 2022-03-03T20:32:35Z

Hmm, I had tested my branch with nginx and had some extra syslog messages in place to log that I was hitting the count > 0 case. I might not have checked the nginx logs for errors though, just that the client (openssl s_time) wasn't reporting errors. I will check this some more on my end.

pasis · 2022-03-06T10:29:43Z

Hmm, I had tested my branch with nginx and had some extra syslog messages in place to log that I was hitting the count > 0 case. I might not have checked the nginx logs for errors though, just that the client (openssl s_time) wasn't reporting errors. I will check this some more on my end.

Main root cause is that unprocessed records (which are encrypted and in TLS1.3 format) are treated as TLS1.2 format due to insufficient condition. Check this diff:

diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c                                                                                                                             
index b92b74c5f5..6b30d5eb66 100644                                                                                                                                                          
--- a/ssl/record/ssl3_record.c                                                                                                                                                               
+++ b/ssl/record/ssl3_record.c                                                                                                                                                               
@@ -687,7 +691,7 @@ int ssl3_get_record(SSL *s)
              * content type, and padding is already removed and thisrr->type and
              * thisrr->length should have the correct values.
              */
-            if (!BIO_get_ktls_recv(s->rbio)) {
+            if (!BIO_get_ktls_recv(s->rbio) || is_ktls_left) {
                 size_t end;
 
                 if (thisrr->length == 0

Unprocessed records are in s->rlayer.rbuf during ssl3_get_record() call, they're decrypted, but because of the above mistake the TLS1.3 record is treated as TLS1.2 without stripping record type at the end.

pasis · 2022-03-07T10:05:05Z

With the above patch I don't see issues in NGINX scenario so far. @bsdjhb please apply it to your branch.

bsdjhb · 2022-03-08T01:22:36Z

@pasis thanks for tracking that down. I had reproduced the error on Friday but hadn't debugged it yet. I found some other places that also needed to be checking is_ktls_left so I decided to refactor ssl3_get_record a bit to add a using_ktls helper variable instead. I've also rewritten the history of the branch so that I think it is now a decent commit candidate.

pasis · 2022-03-09T17:35:22Z

@bsdjhb @ueno What are the next steps for this PR? Will you update current PR or create new one? Also could you ping responsible people to start review process?

I would like to see this feature in the next release if possible.

t8m · 2022-03-23T08:45:00Z

Closing as superceeded by #17942

ueno added 2 commits October 10, 2021 12:14

KTLS: Enable KTLS for receiving as well in TLS 1.3

e2dd40e

This removes a guard condition that prevents KTLS being enabled for receiving in TLS 1.3. Signed-off-by: Daiki Ueno <dueno@redhat.com>

ueno force-pushed the wip/dueno/ktls-recv-tls1.3 branch from 3bd87a7 to e2dd40e Compare October 10, 2021 10:16

t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Oct 11, 2021

ueno mentioned this pull request Oct 11, 2021

KTLS: Skip irrelevant logic in receiving TLS 1.3 record #16797

Closed

2 tasks

ueno marked this pull request as ready for review October 11, 2021 11:46

bsdjhb reviewed Nov 15, 2021

View reviewed changes

pasis reviewed Jan 14, 2022

View reviewed changes

yangyangtiantianlonglong suggested changes Jan 18, 2022

View reviewed changes

bsdjhb mentioned this pull request Mar 22, 2022

KTLS: TLS 1.3 receive offload #17942

Closed

1 task

t8m closed this Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KTLS: Enable KTLS for receiving as well in TLS 1.3 #16798

KTLS: Enable KTLS for receiving as well in TLS 1.3 #16798

ueno commented Oct 10, 2021 •

edited

bsdjhb commented Nov 15, 2021

bsdjhb Nov 15, 2021

bsdjhb Nov 15, 2021

pasis Jan 14, 2022

bsdjhb Jan 18, 2022

yangyangtiantianlonglong Jan 18, 2022

bsdjhb Jan 18, 2022

yangyangtiantianlonglong commented Feb 15, 2022

bsdjhb commented Feb 23, 2022

bsdjhb commented Feb 24, 2022

pasis commented Mar 3, 2022

bsdjhb commented Mar 3, 2022

pasis commented Mar 6, 2022

pasis commented Mar 7, 2022

bsdjhb commented Mar 8, 2022

pasis commented Mar 9, 2022

t8m commented Mar 23, 2022

		@@ -723,8 +722,10 @@ int tls13_change_cipher_state(SSL *s, int which)
		goto skip_ktls;

KTLS: Enable KTLS for receiving as well in TLS 1.3 #16798

KTLS: Enable KTLS for receiving as well in TLS 1.3 #16798

Conversation

ueno commented Oct 10, 2021 • edited

Checklist

bsdjhb commented Nov 15, 2021

bsdjhb Nov 15, 2021

Choose a reason for hiding this comment

bsdjhb Nov 15, 2021

Choose a reason for hiding this comment

pasis Jan 14, 2022

Choose a reason for hiding this comment

bsdjhb Jan 18, 2022

Choose a reason for hiding this comment

yangyangtiantianlonglong Jan 18, 2022

Choose a reason for hiding this comment

bsdjhb Jan 18, 2022

Choose a reason for hiding this comment

yangyangtiantianlonglong commented Feb 15, 2022

bsdjhb commented Feb 23, 2022

bsdjhb commented Feb 24, 2022

pasis commented Mar 3, 2022

bsdjhb commented Mar 3, 2022

pasis commented Mar 6, 2022

pasis commented Mar 7, 2022

bsdjhb commented Mar 8, 2022

pasis commented Mar 9, 2022

t8m commented Mar 23, 2022

ueno commented Oct 10, 2021 •

edited