Skip to content

Commit 1c2bfef

Browse files
etretr
committed
TASK-136: Make ACME retry expiry validation deterministic
Add a private per-registry selection clock, retained by immutable credential snapshots and defaulting to real system time. Advance test time only after HelloRetryRequest; verify expired fresh lookup and handshake rejection while the pinned challenge handshake completes with its original certificate, digest and ALPN. Reproduce the scheduler-pause failure before repair and confirm deterministic GREEN plus a clock-routing negative control. Validate the full task diff with eight initial reviewers and seven impacted closure reviewers; all final approvals recorded. Post-repair C++20 TLS-on 15/15 and TLS-off 4/4 focused executables, native linkage, check-local, lint, complexity, size and whitespace gates pass. ASan/UBSan and TSan each pass 56 tests and 1816 checks. Update evidence documentation and persist one nonblocking minor review finding. Nonlocal platform checks remain assigned to CI and the v3 PR.
1 parent 03ef8c0 commit 1c2bfef

6 files changed

Lines changed: 62 additions & 12 deletions

File tree

‎docs/task-136-acme-evidence.md‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,38 @@ unrelated challenges. Retirement tests observe weak snapshot/context ownership,
8181
a terminal claim that cannot be taken twice, zero pending raw children and zero
8282
owner completions after executor drain. Established ordinary application data
8383
still exchanges after publication/removal. The retry-expiry test first observes
84-
an actual HelloRetryRequest, then yields until its explicit deadline with a
85-
bounded steady-clock stop; it does not use a scheduling sleep as a race oracle.
84+
an actual HelloRetryRequest, then advances its private selection clock exactly
85+
to the challenge deadline. A fresh lookup and fresh ClientHello reject the
86+
expired challenge while the selected handshake completes with its pinned leaf.
87+
The internal registry copies its selection clock into each immutable snapshot;
88+
the default remains `system_clock::now`. Publication and certificate validation
89+
continue to use real time and the fixture's ten-minute deadline. The test has no
90+
one-second setup window, wall-clock wait or scheduling sleep as a race oracle.
91+
92+
## Validation repair: deterministic retry expiry
93+
94+
The old test failed publication when an external process paused it with
95+
`SIGSTOP` for 1.2 seconds during setup. Behavioral RED is recorded in
96+
`/private/tmp/task136-repair-scheduler-red.log`. The revised test also failed
97+
the fresh-handshake and no-certificate assertions when ClientHello still used
98+
real time, despite the lookup clock being advanced; that separate RED is in
99+
`/private/tmp/task136-repair-clock-red.log`. Routing ClientHello through the
100+
snapshot's internal clock makes those assertions pass without changing the
101+
pinned-handshake certificate, digest, ALPN or certificate-message checks.
102+
103+
The same 1.2-second scheduler pause now passes at setup offsets 0.6 ms and 1 ms
104+
(each confirmed by more than 1.2 seconds elapsed inside the retry test), recorded
105+
in `/private/tmp/task136-repair-scheduler-green.log`. The focused six-executable
106+
TLS selection, credentials, rotation and ACME lifetime run passes **28 tests /
107+
1,051 checks** in `/private/tmp/task136-repair-focused-green.log`, with per-suite
108+
receipts in `build-on/test/*.log`. Changed-file cpplint, changed-source CCN <= 10,
109+
source file-size and `git diff --check` also pass; receipts are
110+
`/private/tmp/task136-repair-{lint,complexity,size}.log`.
111+
The existing sanitizer script rebuilt the full affected native object closure;
112+
all nine focused executables pass with **56 tests / 1,816 checks** under each of
113+
ASan/UBSan and TSan, with no diagnostics. The receipt is
114+
`/private/tmp/task136-repair-sanitizers.log`; provider and system libraries remain
115+
uninstrumented as described below.
86116

87117
## Reproduction
88118

‎specs/unworked_review_issues/task-136_validation_deddc2cb6180efc4ef9219c43e929777.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# Unworked Review Issues
2+
3+
**Run:** 2026-10-07 02:10:41
4+
**Task:** TASK-136
5+
**Total:** 1 (0 critical, 0 major, 1 minor)
6+
7+
## Minor
8+
9+
1. [ ] **code-simplifier** | `test/unit/tls_acme_peer.hpp:133` | code-structure
10+
corrupt_name and corrupt_alpn duplicate the ClientHello extension framing walk, including the fixed-field cursor setup and extension bounds checks. That duplication makes future fixture changes easy to apply to only one mutation helper.
11+
*Recommendation:* Extract the shared ClientHello extension traversal into a small helper that finds a requested extension and applies a mutation callback, then keep the SNI and ALPN byte changes in their focused callers.

‎src/detail/tls_credentials.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,7 @@ http::outcome tls_credentials_registry::remove_acme(std::string_view host) {
302302
}
303303
http::outcome tls_credentials_registry::replace(const tls_credentials_config& config) {
304304
auto candidate = std::make_shared<tls_credentials_snapshot>();
305+
candidate->selection_clock_ = selection_clock_;
305306
try {
306307
require(!config.hosts.empty() && config.default_host < config.hosts.size());
307308
candidate->default_host_ = config.default_host;

‎src/detail/tls_session.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ struct tls_session::impl {
200200
bool select_acme(const std::string& name) {
201201
if (!selection.snapshot || !sole_acme_offer || name.empty() ||
202202
handshake_context.transport != tls_transport::tcp || handshake_context.local_port != 443) return false;
203-
auto challenge = selection.snapshot->select_acme(name, std::chrono::system_clock::now());
203+
auto challenge = selection.snapshot->select_acme(name);
204204
if (!challenge) return false;
205205
selection = std::move(*challenge);
206206
acme = true;

‎src/httpserver/detail/tls_credentials.hpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include <mutex>
2020
#include <optional>
2121
#include <string>
22+
#include <utility>
2223
#include <vector>
2324
#include <httpserver/detail/tls_io_backend.hpp>
2425
#include <httpserver/detail/tls_psk.hpp>
@@ -71,6 +72,7 @@ class tls_credentials_snapshot final : public std::enable_shared_from_this<tls_c
7172
tls_credentials_selection select(std::size_t host) const;
7273
// Exact canonical lookup; expiry also applies to previously acquired snapshots.
7374
std::optional<tls_credentials_selection> select_acme(std::string_view canonical_host, std::chrono::system_clock::time_point now) const;
75+
std::optional<tls_credentials_selection> select_acme(std::string_view canonical_host) const { return select_acme(canonical_host, selection_clock_()); }
7476
tls_credentials_selection select_default() const { return select(default_host_); }
7577

7678
private:
@@ -80,6 +82,7 @@ class tls_credentials_snapshot final : public std::enable_shared_from_this<tls_c
8082
std::chrono::system_clock::time_point expires_at;
8183
};
8284
std::map<std::string, acme_entry, std::less<>> challenges_;
85+
std::function<std::chrono::system_clock::time_point()> selection_clock_ = std::chrono::system_clock::now;
8386
std::uint64_t generation_ = 0;
8487
std::size_t default_host_ = 0;
8588
std::vector<tls_host_metadata> hosts_;
@@ -90,6 +93,9 @@ class tls_credentials_snapshot final : public std::enable_shared_from_this<tls_c
9093
// shared_ptr atomic operations need not be lock-free. Published data never mutates.
9194
class tls_credentials_registry final {
9295
public:
96+
// Internal selection-clock seam; publication validation still uses real time.
97+
explicit tls_credentials_registry(std::function<std::chrono::system_clock::time_point()> selection_clock = std::chrono::system_clock::now)
98+
: selection_clock_(std::move(selection_clock)) {}
9399
http::outcome replace(const tls_credentials_config& config);
94100
http::outcome publish_acme(const tls_acme_challenge& input);
95101
http::outcome remove_acme(std::string_view host);
@@ -99,6 +105,7 @@ class tls_credentials_registry final {
99105
http::outcome publish_locked(std::shared_ptr<tls_credentials_snapshot> candidate,
100106
std::shared_ptr<const tls_credentials_snapshot>& retired);
101107
std::shared_ptr<const tls_credentials_snapshot> active_;
108+
std::function<std::chrono::system_clock::time_point()> selection_clock_;
102109
std::mutex publication_;
103110
std::uint64_t generation_ = 0;
104111
};

‎test/unit/tls_acme_selection_test.cpp‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
#include <array>
77
#include <memory>
88
#include <string>
9-
#include <thread>
109
#include <utility>
1110
#include <vector>
1211
#include "./tls_acme_peer.hpp"
@@ -177,11 +176,13 @@ LT_BEGIN_AUTO_TEST(tls_acme_selection_suite, snapshotless_sessions_cannot_obtain
177176
}
178177
LT_END_AUTO_TEST(snapshotless_sessions_cannot_obtain_challenges_and_challenge_shutdown_remains_usable)
179178
LT_BEGIN_AUTO_TEST(tls_acme_selection_suite, selected_challenge_remains_pinned_through_hello_retry_even_after_deadline)
180-
hd::tls_credentials_registry registry;
181-
LT_ASSERT(registry.replace(tls_test::credentials()).ok());
182179
auto challenge = acme_test::challenge();
183-
challenge.expires_at = std::chrono::system_clock::now() + std::chrono::seconds(1);
180+
auto selection_time = challenge.expires_at - std::chrono::seconds(1);
181+
hd::tls_credentials_registry registry([&] { return selection_time; });
182+
LT_ASSERT(registry.replace(tls_test::credentials()).ok());
184183
LT_ASSERT(registry.publish_acme(challenge).ok());
184+
const auto snapshot = registry.acquire();
185+
LT_ASSERT(snapshot->select_acme("a.example").has_value());
185186
acme_test::connection peer(registry.acquire()->select_default(), acme_test::client(TLS1_3_VERSION), "a.example", {"acme-tls/1"});
186187
// Predict only X25519; the provider's stronger first group requires a retry.
187188
LT_ASSERT(SSL_set1_groups_list(peer.peer.get(), "X25519MLKEM768:*X25519") == 1);
@@ -195,11 +196,11 @@ LT_BEGIN_AUTO_TEST(tls_acme_selection_suite, selected_challenge_remains_pinned_t
195196
LT_ASSERT(error == SSL_ERROR_NONE || error == SSL_ERROR_WANT_READ || error == SSL_ERROR_WANT_WRITE);
196197
}
197198
LT_ASSERT(peer.hello_retries == 1);
198-
LT_ASSERT(std::chrono::system_clock::now() < challenge.expires_at);
199-
const auto stop = std::chrono::steady_clock::now() + std::chrono::seconds(2);
200-
while (std::chrono::system_clock::now() < challenge.expires_at && std::chrono::steady_clock::now() < stop) std::this_thread::yield();
201-
LT_ASSERT(std::chrono::system_clock::now() >= challenge.expires_at);
202-
LT_CHECK(!registry.acquire()->select_acme("a.example", std::chrono::system_clock::now()));
199+
selection_time = challenge.expires_at;
200+
LT_CHECK(!snapshot->select_acme("a.example"));
201+
acme_test::connection expired(registry.acquire()->select_default(), acme_test::client(TLS1_3_VERSION), "a.example", {"acme-tls/1"});
202+
LT_CHECK(!expired.connect());
203+
LT_CHECK(SSL_get0_peer_certificate(expired.peer.get()) == nullptr);
203204
LT_ASSERT(peer.connect());
204205
LT_CHECK_EQ(peer.serial(), std::int64_t{303});
205206
LT_CHECK_EQ(peer.alpn(), "acme-tls/1");

0 commit comments

Comments
 (0)