diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index a73695423..e5cc7acaf 100644 --- a/ssl/d1_both.cc +++ b/ssl/d1_both.cc @@ -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; } diff --git a/ssl/d1_lib.cc b/ssl/d1_lib.cc index c73a1029b..2c7a1cd8a 100644 --- a/ssl/d1_lib.cc +++ b/ssl/d1_lib.cc @@ -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; } diff --git a/ssl/internal.h b/ssl/internal.h index 8dcc097fa..a8ceb16a9 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -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; }; diff --git a/ssl/test/handshake_util.cc b/ssl/test/handshake_util.cc index 65219d847..d5e43e7fd 100644 --- a/ssl/test/handshake_util.cc +++ b/ssl/test/handshake_util.cc @@ -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: diff --git a/ssl/test/packeted_bio.cc b/ssl/test/packeted_bio.cc index f5e2a54c9..94065506d 100644 --- a/ssl/test/packeted_bio.cc +++ b/ssl/test/packeted_bio.cc @@ -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; +} diff --git a/ssl/test/packeted_bio.h b/ssl/test/packeted_bio.h index b99e97191..9b2e50e55 100644 --- a/ssl/test/packeted_bio.h +++ b/ssl/test/packeted_bio.h @@ -47,5 +47,8 @@ bssl::UniquePtr 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 diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 9dddd138a..9f9072225 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go @@ -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) diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 6ee603fa4..29c12310c 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -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, + }) } } } diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 4c3c1cc0c..6583b74fd 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -514,6 +514,7 @@ const Flag *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; diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 093d2410a..b4b290d2c 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -221,6 +221,7 @@ struct TestConfig { bool no_check_ecdsa_curve = false; int expect_selected_credential = -1; std::vector credentials; + int private_key_delay_ms = 0; std::vector handshaker_args;