Defer sending a KeyUpdate until after pending writes are complete#8773
Defer sending a KeyUpdate until after pending writes are complete#8773mattcaswell wants to merge 3 commits intoopenssl:masterfrom
Conversation
If we receive a KeyUpdate message (update requested) from the peer while we are in the middle of a write, we should defer sending the responding KeyUpdate message until after the current write is complete. We do this by waiting to send the KeyUpdate until the next time we write and there is no pending write data. This does imply a subtle change in behaviour. Firstly the responding KeyUpdate message won't be sent straight away as it is now. Secondly if the peer sends multiple KeyUpdates without us doing any writing then we will only send one response, as opposed to previously where we sent a response for each KeyUpdate received. Fixes openssl#8677
mattcaswell
added
branch: master
|
Does this mean that the key update response won't be sent if the application does not invoke SSL_write() after the key update request is received? |
|
Yes...although really shouldn't be a problem since if you don't do any writing then you're not using the key anyway. |
|
Appveyor failure is not related. |
Makes sense. |
|
I think this is no longer needed: openssl/ssl/statem/statem_lib.c Lines 650 to 652 in ad7e17d |
The "if" is still needed - but not the bit about handling SSL_SENT_SHUTDOWN since we'll only send a KeyUpdate if SSL_write() has been called which should fail if close_notify has already been sent. But perhaps that's what you meant - I wasn't sure. Anyway I removed the |
|
@kroeckx do you have any further comments on this PR? |
|
I will not review this.
|
Ok no problem. I'm assuming you have no fundamental objections to it. Ping @openssl/committers for review. |
kaduk
left a comment
kaduk
left a comment
There was a problem hiding this comment.
I think this is looks okay; it seems to be putting the logic in the right places and such. But I didn't get a chance to do the full review I'd normally like to do, tracing through the ancillary state machine interactions and such, so if we could get a third reviewer that would be good.
| NULL, NULL)) | ||
| || !TEST_true(create_ssl_connection(serverssl, clientssl, | ||
| SSL_ERROR_NONE))) | ||
| goto end; |
There was a problem hiding this comment.
side note(?) My personal preference would be for curly braces, even for one-line bodies, when the list of conditions is this long. I didn't double-check the precise wording in the policy, though.
| static int always_retry_gets(BIO *bp, char *buf, int size); | ||
| static int always_retry_puts(BIO *bp, const char *str); | ||
|
|
||
| const BIO_METHOD *bio_s_always_retry(void) |
There was a problem hiding this comment.
This method (and hot-swapping) is kind of a neat hack.
|
Pushed to master and 1.1.1. |
If we receive a KeyUpdate message (update requested) from the peer while we are in the middle of a write, we should defer sending the responding KeyUpdate message until after the current write is complete. We do this by waiting to send the KeyUpdate until the next time we write and there is no pending write data. This does imply a subtle change in behaviour. Firstly the responding KeyUpdate message won't be sent straight away as it is now. Secondly if the peer sends multiple KeyUpdates without us doing any writing then we will only send one response, as opposed to previously where we sent a response for each KeyUpdate received. Fixes #8677 Reviewed-by: Ben Kaduk <[email protected]> (Merged from #8773) (cherry picked from commit feb9e31)
Reviewed-by: Ben Kaduk <[email protected]> (Merged from #8773) (cherry picked from commit a77b4db)
If we receive a KeyUpdate message (update requested) from the peer while we are in the middle of a write, we should defer sending the responding KeyUpdate message until after the current write is complete. We do this by waiting to send the KeyUpdate until the next time we write and there is no pending write data. This does imply a subtle change in behaviour. Firstly the responding KeyUpdate message won't be sent straight away as it is now. Secondly if the peer sends multiple KeyUpdates without us doing any writing then we will only send one response, as opposed to previously where we sent a response for each KeyUpdate received. Fixes #8677 Reviewed-by: Ben Kaduk <[email protected]> (Merged from #8773)
Reviewed-by: Ben Kaduk <[email protected]> (Merged from #8773)
4 participants
If we receive a KeyUpdate message (update requested) from the peer while
we are in the middle of a write, we should defer sending the responding
KeyUpdate message until after the current write is complete. We do this
by waiting to send the KeyUpdate until the next time we write and there is
no pending write data.
This does imply a subtle change in behaviour. Firstly the responding
KeyUpdate message won't be sent straight away as it is now. Secondly if
the peer sends multiple KeyUpdates without us doing any writing then we
will only send one response, as opposed to previously where we sent a
response for each KeyUpdate received.
Fixes #8677