aws / aws/aws-sdk-cpp

AdjustClockSkew logs a "time skew / signature check failed" WARN on every non-2xx outcome (incl. 404/412), before the error-type / ShouldRetry check

Open
#3,862 2 comments 0 reactions 0 assignees View on GitHub
bug p2 response-requested
Dominant language
C++
Stars
2.2k
Forks
1.2k
Avg merge
3d 14h
Merged PRs (30d)
12

Description

### Describe the bug

In the retry loop, `AWSClient::AdjustClockSkew()` is called on **every** non-successful HTTP outcome — *before* `RetryStrategy::ShouldRetry()` and without any error-type gating — and it emits, **unconditionally at the top of the function**:

> `If the signature check failed. This could be because of a time skew. Attempting to adjust the signer.`

before it has determined whether the error is actually clock-skew / signature related. So ordinary, expected non-2xx responses that have nothing to do with signatures or clocks — `404 NoSuchKey` (HeadObject on a missing key), `412 PreconditionFailed` (conditional `PutObject If-None-Match`), etc. — each emit this alarming WARN and do a wasted `Date` header parse. Workloads that use conditional requests / existence checks as normal control flow produce huge volumes of this misleading log and it falsely suggests a signature/clock defect where none exists.

### Regression Issue

- [ ] Select this option if this issue appears to be a regression.

### Expected Behavior

The "…time skew… adjust the signer" WARN should be emitted only when a clock skew is actually detected (i.e. inside the branch where `diff` exceeds the threshold and the signer is adjusted). Non-skew errors (404, 412, …) should not produce a signature/clock-skew warning and ideally should not enter the skew-detection / Date-parsing path at all.

### Current Behavior

`src/aws-cpp-sdk-core/source/client/AWSClient.cpp` (identical in 1.11.771 and `main`):

```cpp
// retry loop — called on any error outcome, BEFORE ShouldRetry:
bool shouldSleep = !AdjustClockSkew(outcome, signerName) && !retryWithCorrectRegion;
if (!retryWithCorrectRegion && !m_retryStrategy->ShouldRetry(outcome.GetError(), retries))
break;

bool AWSClient::AdjustClockSkew(HttpResponseOutcome& outcome, const char* signerName) const {
if (m_enableClockSkewAdjustment) {
auto signer = GetSignerByName(signerName);
AWS_LOGSTREAM_WARN(AWS_CLIENT_LOG_TAG,
"If the signature check failed. This could be because of a time skew. Attempting to adjust the signer."); // <-- unconditional, before any skew check
DateTime serverTime = GetServerTimeFromError(outcome.GetError());
...
if (diff >= TIME_DIFF_MAX || diff <= TIME_DIFF_MIN) { /* real adjust + retry */ return true; }
}
return false;
}
```

With clocks in sync, a 404/412 logs the WARN, parses the Date header, computes `diff ≈ 0`, and returns `false` (no adjustment, no retry). Net effect: one misleading WARN + a wasted Date parse per non-2xx outcome.

### Reproduction Steps

Point an `S3Client` at any S3-compatible endpoint whose clock matches the client, enable logging at `Warn`, and HEAD a non-existent key (→404). The WARN is logged even though there is no skew.

```cpp
#include
#include
#include

int main() {
Aws::SDKOptions options;
options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Warn; // logs to aws_sdk_*.log
Aws::InitAPI(options);
{
Aws::Client::ClientConfiguration cfg;
cfg.region = "us-east-1";
// cfg.endpointOverride = "http://127.0.0.1:9000"; // any reachable S3 / S3-compatible endpoint
Aws::S3::S3Client s3(cfg);
Aws::S3::Model::HeadObjectRequest req;
req.SetBucket("an-existing-bucket");
req.SetKey("a-definitely-missing-key"); // -> 404 NoSuchKey
(void) s3.HeadObject(req); // logs "...time skew... Attempting to adjust the signer."
}
Aws::ShutdownAPI(options);
return 0;
}
```
A conditional `PutObject` with `If-None-Match: *` returning `412` reproduces it the same way.

### Possible Solution

Only log / enter the skew path when a skew is plausible or confirmed. Minimal fix — move the WARN into the branch that actually detects skew:

```cpp
if (diff >= TIME_DIFF_MAX || diff <= TIME_DIFF_MIN) {
AWS_LOGSTREAM_WARN(AWS_CLIENT_LOG_TAG,
"Signature check may have failed due to a clock skew of " << diff.count() << " ms; adjusting the signer.");
... // adjust
}
```
Better: gate `AdjustClockSkew` (or its body) on clock-skew-related conditions (e.g. `RequestTimeTooSkewed` / `RequestExpired` / signature errors) so unrelated errors (404/412/…) never parse the Date header or log.

### Additional Information/Context

Found in a high-conditional-request S3 workload (content-addressed storage: HEAD-before-PUT existence checks, `PUT If-None-Match` dedup → 412, `DELETE If-Match`). Every such non-2xx emitted the WARN; at debug/trace log level this was ~184k lines / ~98 MB in a single test run and initially looked like a signature/clock defect (it is not — the requests succeed, non-retryable errors break out without a retry). The impact is misleading logs + wasted Date parsing, not extra network retries.

### AWS CPP SDK version used

1.11.771 (also reproduces on `main`)

### Compiler and Version used

gcc (Ubuntu 14.2.0-19ubuntu2) 14.2.0

### Operating System and version

Ubuntu 25.04 (x86_64, kernel 6.14.0-37-generic)

Contributor guide

Open the contributing guide

Research direction

Start in src/aws-cpp-sdk-core/source/client/AWSClient.cpp at the retry loop and AWSClient::AdjustClockSkew(), then trace the existing error and retry handling. Reproduce the behavior with the documented S3 HeadObject request for a missing key; done means ordinary 404/412 responses no longer emit the clock-skew warning or enter unnecessary skew detection, while actual skew handling still works.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
api
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.