Repository navigation
[Bugfix] Compute clock skew from response header arrival time - #3979
Merged
Merged
Conversation
Update clock skew correction to the revised clock skew SEP: the candidate skew is now serverTime - timeResponseReceived instead of the request round-trip midpoint, and timeResponseReceived is the raw client time when the response headers arrive, before the body is read. The midpoint attributed half of the server processing time to skew, so a 620 s request with in-sync clocks recorded +310 s and poisoned ClientSkew. - ClockSkew.h: new formula; Age, missing-Date and 15-minute discards unchanged. - HttpRequest: per-attempt ResponseHeadersReceivedTime, set by the curl, CRT and WinHTTP/WinINet clients; falls back to Now() when unset. - AWSClient (sync/async) and AwsSmithyClientBase clear it per attempt and use it as the received time. - Sync clock-skew-test-cases.json with the SEP's shared vectors and add a mock client test for a slow body.
Madrigal
reviewed
Nov 4, 2026
|
|
||
| pRequestCtx->m_timeResponseReceived = Aws::Utils::DateTime::Now(); | ||
| pRequestCtx->m_timeResponseReceived = | ||
| pRequestCtx->m_httpRequest ? Aws::Internal::GetTimeResponseReceived(*pRequestCtx->m_httpRequest) : Aws::Utils::DateTime::Now(); |
Contributor
There was a problem hiding this comment.
under which which conditions would this not have an httpRequest?
| const DateTime headersReceived = DateTime::Now(); | ||
| httpRequest->SetResponseHeadersReceivedTime(headersReceived); | ||
| firstResponse->AddHeader("Date", (headersReceived + std::chrono::hours(1)).ToGmtString(DateFormat::RFC822)); | ||
| std::this_thread::sleep_for(std::chrono::seconds(3)); |
Contributor
There was a problem hiding this comment.
I'd love if there was a way where we don't have to actually sleep on tests, but unfamiliar if there's any time mocking in cpp
| ASSERT_TRUE(signingTime.has_value()); | ||
| const int64_t skewMs = signingTime.value().Millis() - before.Millis(); | ||
| const int64_t hourMs = std::chrono::milliseconds(std::chrono::hours(1)).count(); | ||
| EXPECT_GE(skewMs, hourMs - 1100); |
Contributor
There was a problem hiding this comment.
why are we comparing -1100,500 range? maybe I'm missing something but it seems somewhat arbitrary
Madrigal
reviewed
Nov 4, 2026
| const auto& signingTime = requests.back().GetSigningTimestampOverride(); | ||
| ASSERT_TRUE(signingTime.has_value()); | ||
| const DateTime sendTime = signingTime.value() - std::chrono::hours(1); | ||
| EXPECT_GE(sendTime.Millis(), before.Millis()); |
Contributor
There was a problem hiding this comment.
we are just checking here that the value is greater than X, shouldn't we check that this is within Y range or exactly X?
Madrigal
approved these changes
Nov 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clock skew was computed as the server's
Dateminus the midpoint of the request round trip, with the received time taken after the body was read. A long-running request with in-sync clocks recorded a false skew (+310 s for a 620 s request), past the SigV4 window. This follows the updated clock skew SEP: skew is nowDateminus the time the response headers arrived.Changes:
ComputeClockSkewCandidateusesserverTime - timeResponseReceived. The Age, missing-Dateand 15-minute discards are unchanged.HttpRequestgets a per-attempt response-headers-received time, set by the curl, CRT and WinHTTP/WinINet clients before the body is read. Custom HTTP clients that don't set it fall back to the previous capture point.clock-skew-test-cases.jsonis synced with the SEP's shared vectors, including the new 620 s case, and a mock client test checks that a slow body doesn't affect the measured skew.Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.