OERV-RUNTIME/boringssl
2
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Schedule ACKs when we receive a partial flight

Browse Source 
This implements a very simple ACK policy: for every ACKable record (i.e.
contains no far-future fragments that we ignore), add it to a queue of
record numbers to ACK and set an ACK timer at 1/4 of the current
retransmission timeout.

RFC 9147 doesn't say a whole lot, but this is arguably slightly
different from the recommended policy. RFC 9147 has some text with
implies you're only meant to ACK the current flight and not arbitrarily
old message fragments. However, tracking that in the fully general case with
post-handshake messages is unclear. There's no harm in ACKing those
packets, so start with this. See discussion in
https://mailarchive.ietf.org/arch/msg/tls/kjJnquJOVaWxu5hUCmNzB35eqY0/

Something kind of fun is that this provision in the spec happens for
free:

> ACKs SHOULD NOT be sent for these flights unless the responding flight
> cannot be generated immediately. All other flights MUST be ACKed. In
> this case, implementations MAY send explicit ACKs for the complete
> received flight even though it will eventually also be implicitly
> acknowledged through the responding flight. A notable example for this
> is the case of client authentication in constrained environments,
> where generating the CertificateVerify message can take considerable
> time on the client.

If we generate the next flight before the ACK timer, it will be canceled
and we don't ACK. If the next flight is async and takes too long, the
ACK timer will win and we tell the peer not to retransmit.

Bug: 42290594
Change-Id: I7974499f82ce2b2c7da91f02ca65886b0f82896c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72952
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
This commit is contained in:
David Benjamin
2024-11-18 22:47:35 +00:00
committed by Boringssl LUCI CQ
parent 7c3252a67b
commit dfd44900ff
10 changed files with 212 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ssl/d1_both.cc
+18 -3
View File
@@ -445,6 +445,22 @@ bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert,
if (!skipped_fragments) {
ssl->d1->records_to_ack.PushBack(record_number);
if (ssl_has_final_version(ssl) &&
ssl_protocol_version(ssl) >= TLS1_3_VERSION &&
!ssl->d1->ack_timer.IsSet()) {
// Schedule sending an ACK. The delay serves several purposes:
// - If there are more records to come, we send only one ACK.
// - If there are more records to come and the flight is now complete, we
// will send the reply (which implicitly ACKs the previous flight) and
// cancel the timer.
// - If there are more records to come, the flight is now complete, but
// generating the response is delayed (e.g. a slow, async private key),
// the timer will fire and we send an ACK anyway.
OPENSSL_timeval now = ssl_ctx_get_current_time(ssl->ctx.get());
ssl->d1->ack_timer.StartMicroseconds(
now, uint64_t{ssl->d1->timeout_duration_ms} * 1000 / 4);
}
}
return true;
@@ -998,10 +1014,8 @@ int dtls1_flush_flight(SSL *ssl, bool post_handshake) {
// to ACK previous records. This clears the ACK buffer slightly earlier than
// the specification suggests. See the discussion in
// https://mailarchive.ietf.org/arch/msg/tls/kjJnquJOVaWxu5hUCmNzB35eqY0/
//
// TODO(crbug.com/42290594): When we introduce the ACK timer, this should
// also stop the ACK timer.
ssl->d1->records_to_ack.Clear();
ssl->d1->ack_timer.Stop();
}
// Start the retransmission timer for the next flight (if any).
dtls1_start_timer(ssl);
@@ -1010,6 +1024,7 @@ int dtls1_flush_flight(SSL *ssl, bool post_handshake) {
int dtls1_send_ack(SSL *ssl) {
assert(ssl_protocol_version(ssl) >= TLS1_3_VERSION);
ssl->d1->ack_timer.Stop();
if (ssl->d1->records_to_ack.empty()) {
return 1;
}
ssl/d1_lib.cc
+27 -8
View File
@@ -247,6 +247,8 @@ int DTLSv1_get_timeout(const SSL *ssl, struct timeval *out) {
OPENSSL_timeval now = ssl_ctx_get_current_time(ssl->ctx.get());
uint64_t remaining_usec =
ssl->d1->retransmit_timer.MicrosecondsRemaining(now);
remaining_usec =
std::min(remaining_usec, ssl->d1->ack_timer.MicrosecondsRemaining(now));
if (remaining_usec == DTLSTimer::kNever) {
return 0; // No timeout is set.
}
@@ -274,17 +276,34 @@ int DTLSv1_handle_timeout(SSL *ssl) {
return -1;
}
// If no timer is expired, don't do anything.
OPENSSL_timeval now = ssl_ctx_get_current_time(ssl->ctx.get());
if (!ssl->d1->retransmit_timer.IsExpired(now)) {
if (!ssl->d1->ack_timer.IsSet() && !ssl->d1->retransmit_timer.IsSet()) {
// No timers are running. Don't bother querying the clock.
return 0;
}
if (!dtls1_check_timeout_num(ssl)) {
return -1;
OPENSSL_timeval now = ssl_ctx_get_current_time(ssl->ctx.get());
bool any_timer_expired = false;
if (ssl->d1->ack_timer.IsExpired(now)) {
any_timer_expired = true;
int ret = dtls1_send_ack(ssl);
if (ret <= 0) {
return ret;
}
}
dtls1_double_timeout(ssl);
dtls1_start_timer(ssl);
return dtls1_retransmit_outgoing_messages(ssl);
if (ssl->d1->retransmit_timer.IsExpired(now)) {
any_timer_expired = true;
if (!dtls1_check_timeout_num(ssl)) {
return -1;
}
dtls1_double_timeout(ssl);
dtls1_start_timer(ssl);
int ret = dtls1_retransmit_outgoing_messages(ssl);
if (ret <= 0) {
return ret;
}
}
return any_timer_expired ? 1 : 0;
}
ssl/internal.h
+3
View File
@@ -3613,6 +3613,9 @@ struct DTLS1_STATE {
// not hear from the peer.
DTLSTimer retransmit_timer;
// ack_timer tracks when to send an ACK.
DTLSTimer ack_timer;
// timeout_duration_ms is the timeout duration in milliseconds.
uint32_t timeout_duration_ms = 0;
};
ssl/test/handshake_util.cc
+23
View File
@@ -96,6 +96,29 @@ bool RetryAsync(SSL *ssl, int ret) {
test_state->early_callback_ready = true;
return true;
case SSL_ERROR_WANT_PRIVATE_KEY_OPERATION:
if (config->private_key_delay_ms != 0 &&
test_state->private_key_retries == 0) {
// The first time around, simulate the private key operation taking a
// long time to run.
if (test_state->packeted_bio == nullptr) {
fprintf(stderr, "-private-key-delay-ms requires DTLS.\n");
return false;
}
timeval *clock = PacketedBioGetClock(test_state->packeted_bio);
clock->tv_sec += config->private_key_delay_ms / 1000;
clock->tv_usec += config->private_key_delay_ms * 1000;
if (clock->tv_usec >= 1000000) {
clock->tv_usec -= 1000000;
clock->tv_sec++;
}
AsyncBioEnforceWriteQuota(test_state->async_bio, false);
int timeout_ret = DTLSv1_handle_timeout(ssl);
AsyncBioEnforceWriteQuota(test_state->async_bio, true);
if (timeout_ret < 0) {
fprintf(stderr, "Error retransmitting.\n");
return false;
}
}
test_state->private_key_retries++;
return true;
case SSL_ERROR_WANT_CERTIFICATE_VERIFY:
ssl/test/packeted_bio.cc
+8
View File
@@ -328,3 +328,11 @@ bool PacketedBioAdvanceClock(BIO *bio) {
OPENSSL_memset(&data->timeout, 0, sizeof(data->timeout));
return true;
}
timeval *PacketedBioGetClock(BIO *bio) {
PacketedBio *data = GetData(bio);
if (data == nullptr) {
return nullptr;
}
return data->clock;
}
ssl/test/packeted_bio.h
+3
View File
@@ -47,5 +47,8 @@ bssl::UniquePtr<BIO> PacketedBioCreate(
// pending timeout. Otherwise, it returns false.
bool PacketedBioAdvanceClock(BIO *bio);
// PacketedBioAdvanceClock return's |bio|'s clock.
timeval *PacketedBioGetClock(BIO *bio);
#endif // HEADER_PACKETED_BIO
ssl/test/runner/handshake_server.go
+14 -5
View File
@@ -1223,11 +1223,14 @@ ResendHelloRetryRequest:
// from the client certificate are sent over these keys.
c.useOutTrafficSecret(uint16(encryptionApplication), c.wireVersion, hs.suite, serverTrafficSecret)
if err := c.flushHandshake(); err != nil {
return err
}
if encryptedExtensions.extensions.hasEarlyData {
// In TLS, we need to consume EndOfEarlyData, and also test early data that
// was only partially written while reading the ServerHello. Both of these
// require flushing ServerHello first. Neither of these apply to DTLS, where
// we need to flush after installing handshake keys.
if encryptedExtensions.extensions.hasEarlyData && !c.isDTLS {
if err := c.flushHandshake(); err != nil {
return err
}
for _, expectedMsg := range config.Bugs.ExpectLateEarlyData {
if err := c.readRecord(recordTypeApplicationData); err != nil {
return err
@@ -1251,6 +1254,12 @@ ResendHelloRetryRequest:
return err
}
// DTLS testing requires this flush occur after installing handshake keys,
// so that we can process ACKs.
if err := c.flushHandshake(); err != nil {
return err
}
// If we sent an ALPS extension, the client must respond with a single EncryptedExtensions.
if encryptedExtensions.extensions.hasApplicationSettings || encryptedExtensions.extensions.hasApplicationSettingsOld {
clientEncryptedExtensions, err := readHandshakeType[clientEncryptedExtensionsMsg](c)
ssl/test/runner/runner.go
+114 -11
View File
@@ -11686,15 +11686,24 @@ func addDTLSRetransmitTests() {
DefaultCurves: []CurveID{}, // Force HelloRetryRequest.
Bugs: ProtocolBugs{
WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
if len(received) == 0 && next[0].Type == typeClientHello {
// Send the initial ClientHello as-is.
c.WriteFlight(next)
return
}
// Send a portion of the first message. The rest was lost.
msg := next[0]
split := len(msg.Data) / 2
c.WriteFragments([]DTLSFragment{msg.Fragment(0, split)})
// This is enough to ACK the previous flight. The shim
// should stop retransmitting and even stop the timer.
//
// TODO(crbug.com/42290594): The shim should send a partial
// ACK to request that we retransmit.
// After waiting the current timeout, the shim should ACK
// the partial flight.
c.ExpectNextTimeout(useTimeouts[0] / 4)
c.AdvanceClock(useTimeouts[0] / 4)
c.ReadACK(c.InEpoch())
// The partial flight is enough to ACK the previous flight.
// The shim should stop retransmitting and even stop the
// retransmit timer.
c.ExpectNoNextTimeout()
for _, t := range useTimeouts {
c.AdvanceClock(t)
@@ -11723,7 +11732,9 @@ func addDTLSRetransmitTests() {
WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
msg := next[0]
if msg.Type != typeServerHello {
// Leave the NewSessionTicket flight alone.
// TODO(crbug.com/42290594): Do not manipulate NewSessionTicket
// flights for now. The shim actually does now ACK those on a
// timer, but we'll need to test those more explicitly.
c.WriteFlight(next)
return
}
@@ -11749,15 +11760,16 @@ func addDTLSRetransmitTests() {
c.AdvanceClock(useTimeouts[1])
c.ReadRetransmit()
// Send EncryptedExtensions. The shim now knows the version
// and implicitly ACKs everything.
// Send EncryptedExtensions. The shim now knows the version.
c.WriteFragments([]DTLSFragment{next[1].Fragment(0, len(next[1].Data))})
// The shim should ACK the partial flight. The shim hasn't
// gotten to epoch 3 yet, so the ACK will come in epoch 2.
c.AdvanceClock(useTimeouts[2] / 4)
c.ReadACK(uint16(encryptionHandshake))
// This is enough to ACK the previous flight. The shim
// should stop retransmitting and even stop the timer.
//
// TODO(crbug.com/42290594): The shim should send a partial
// ACK to request that we retransmit.
c.ExpectNoNextTimeout()
for _, t := range useTimeouts[2:] {
c.AdvanceClock(t)
@@ -12318,6 +12330,97 @@ func addDTLSRetransmitTests() {
shouldFail: true,
expectedError: ":READ_TIMEOUT_EXPIRED:",
})
// If generating the reply to a flight takes time (generating a
// CertificateVerify for a client certificate), the shim should
// send an ACK.
testCases = append(testCases, testCase{
protocol: dtls,
name: "DTLS-Retransmit-SlowReplyGeneration" + suffix,
config: Config{
MaxVersion: vers.version,
ClientAuth: RequireAnyClientCert,
Bugs: ProtocolBugs{
WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
c.WriteFlight(next)
if next[0].Type == typeServerHello {
// The shim will reply with Certificate..Finished, but
// take time to do so. In that time, it should schedule
// an ACK so the runner knows not to retransmit.
c.ReadACK(c.InEpoch())
}
},
},
},
shimCertificate: &rsaCertificate,
// Simulate it taking time to generate the reply.
flags: slices.Concat(flags, []string{"-private-key-delay-ms", strconv.Itoa(int(useTimeouts[0].Milliseconds()))}),
})
// BoringSSL's ACK policy may schedule both retransmit and ACK
// timers in parallel.
//
// TODO(crbug.com/42290594): This is only possible during the
// handshake because we're willing to ACK old flights without
// trying to distinguish these cases. However, post-handshake
// messages will exercise this, so that may be a better version
// of this test. In-handshake, it's kind of a waste to ACK this,
// so maybe we should stop.
testCases = append(testCases, testCase{
protocol: dtls,
name: "DTLS-Retransmit-BothTimers" + suffix,
config: Config{
MaxVersion: vers.version,
Bugs: ProtocolBugs{
// Arrange for there to be two server flights.
SendHelloRetryRequestCookie: []byte("cookie"),
WriteFlightDTLS: func(c *DTLSController, prev, received, next []DTLSMessage, records []DTLSRecordNumberInfo) {
if next[0].Sequence == 0 || next[0].Type != typeServerHello {
// Send the first flight (HelloRetryRequest) as-is,
// as well as any post-handshake flights.
c.WriteFlight(next)
return
}
// The shim just send the ClientHello2 and is
// waiting for ServerHello..Finished. If it hears
// nothing, it will retransmit ClientHello2 on the
// assumption the packet was lost.
c.ExpectNextTimeout(useTimeouts[0])
// Retransmit a portion of HelloRetryRequest.
c.WriteFragments([]DTLSFragment{prev[0].Fragment(0, 1)})
// The shim does not actually need to ACK this,
// but BoringSSL does. Now both timers are active.
// Fire the first...
c.ExpectNextTimeout(useTimeouts[0] / 4)
c.AdvanceClock(useTimeouts[0] / 4)
c.ReadACK(0)
// ...followed by the second.
c.ExpectNextTimeout(3 * useTimeouts[0] / 4)
c.AdvanceClock(3 * useTimeouts[0] / 4)
c.ReadRetransmit()
// The shim is now set for the next retransmit.
c.ExpectNextTimeout(useTimeouts[1])
// Start the ACK timer again.
c.WriteFragments([]DTLSFragment{prev[0].Fragment(0, 1)})
c.ExpectNextTimeout(useTimeouts[1] / 4)
// Expire both timers at once.
c.AdvanceClock(useTimeouts[1])
c.ReadACK(0)
c.ReadRetransmit()
c.WriteFlight(next)
},
},
},
flags: flags,
})
}
}
}
ssl/test/test_config.cc
+1
View File
@@ -514,6 +514,7 @@ const Flag<TestConfig> *FindFlag(const char *name) {
&TestConfig::signed_cert_timestamps),
Base64Flag("-signed-cert-timestamps",
&CredentialConfig::signed_cert_timestamps)),
IntFlag("-private-key-delay-ms", &TestConfig::private_key_delay_ms),
};
std::sort(ret.begin(), ret.end(), FlagNameComparator{});
return ret;
ssl/test/test_config.h
+1
View File
@@ -221,6 +221,7 @@ struct TestConfig {
bool no_check_ecdsa_curve = false;
int expect_selected_credential = -1;
std::vector<CredentialConfig> credentials;
int private_key_delay_ms = 0;
std::vector<const char*> handshaker_args;