fix(core): Drop fractional seconds from formatted timestamps used for S3 signed-URL generation (fixes #1131). #1128
Conversation
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
Contributor
WalkthroughThe implementation of the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
haiqi96
changed the title
gibber9809
approved these changes
Jul 25, 2025
Contributor
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
LGTM. I'm also curious why this hasn't been an issue before.
For title since this is under src/clp/aws maybe it should be fix(clp) or fix(core)? Also the PR title should end with a period.
haiqi96
changed the title
Contributor
Author
Added another note in the description about a commit that is known to work (with this issue hidden) |
Member
|
Caused by #1054 which upgraded fmtlib. |
Comment on lines
+94
to
+95
| auto const timestamp_secs = std::chrono::time_point_cast<std::chrono::seconds>(timestamp); | ||
| return fmt::format("{:%Y%m%dT%H%M%SZ}", timestamp_secs); |
Member
There was a problem hiding this comment.
@coderabbitai Can you open a bug report corresponding to this fix?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@kirkrodrigues I'll create a bug report for this timestamp formatting issue.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the timestamp formatting bug
gh issue create \
--title "AWS S3 authentication fails due to fractional seconds in timestamps" \
--body "## Description
AWS S3 presigned URL generation was failing due to incorrect timestamp formatting that included fractional seconds. AWS requires timestamps to follow the ISO8601 Long Format \`yyyyMMdd'T'HHmmss'Z'\` without fractional seconds, but the current implementation was producing timestamps like \`2023-03-22T03-45-46.234232\`.
## Root Cause
The \`get_formatted_timestamp_string\` function in \`components/core/src/clp/aws/AwsAuthenticationSigner.cpp\` was formatting timestamps directly from \`std::chrono::system_clock::time_point\` without truncating to second precision, which could include sub-second precision depending on the fmt library version or C++ version used.
## Impact
- S3 authentication fails when using clp-s tool with timestamp keys
- Prevents successful compression operations with S3 storage
## Solution
The issue was resolved by explicitly casting the timepoint to second precision using \`std::chrono::time_point_cast<std::chrono::seconds>\` before formatting.
## References
- Fixed in PR: https://github.com/y-scope/clp/pull/1128
- Comment: https://github.com/y-scope/clp/pull/1128#discussion_r2233169218
## Validation
The fix was validated by:
1. Exporting AWS credentials
2. Building the clp-s tool
3. Running compression command with S3 authentication and timestamp key
4. Confirming the tool failed without the fix but succeeded after applying the change" \
--assignee kirkrodrigues
Length of output: 1575
@kirkrodrigues I've successfully created a bug report for this timestamp formatting issue. The issue has been opened at #1131 and assigned to you.
The bug report documents:
- The root cause of fractional seconds in AWS S3 authentication timestamps
- The impact on S3 authentication and compression operations
- The technical solution implemented in this PR
- References to both the PR and this comment thread
- The validation steps that confirmed the fix
🧬 Code Graph Analysis Results
Relevant snippets from components/core/src/clp/aws/AwsAuthenticationSigner.hpp (lines 24-31, 43, 46-52, 91-92, 102-103, 113-117, 128-133):
// Exception class for S3Url operation failures with error code, filename, line number, and optional message.
OperationFailed(
ErrorCode error_code,
char const* const filename,
int line_number,
std::string message = "S3Url operation failed"
)
: TraceableException{error_code, filename, line_number},
m_message{std::move(message)} {}
// Constructor for S3Url that parses an S3 HTTP URL string.
S3Url(std::string const& url);
// Accessors for S3Url components.
[[nodiscard]] auto get_region() const -> std::string_view { return m_region; }
[[nodiscard]] auto get_bucket() const -> std::string_view { return m_bucket; }
[[nodiscard]] auto get_key() const -> std::string_view { return m_key; }
[[nodiscard]] auto get_host() const -> std::string_view { return m_host; }
// Generates a presigned URL for the given S3Url, outputs to presigned_url, returns ErrorCode.
generate_presigned_url(S3Url const& s3_url, std::string& presigned_url) const -> ErrorCode;
// Returns the canonical query string for signing, given scope and timestamp.
get_canonical_query_string(std::string_view scope, std::string_view timestamp) const -> std::string;
// Computes the signing key for given region and date, outputs to signing_key vector, returns ErrorCode.
get_signing_key(
std::string_view region,
std::string_view date,
std::vector<unsigned char>& signing_key
) const -> ErrorCode;
// Computes the signature for given region, date, and string_to_sign, outputs to signature vector, returns ErrorCode.
get_signature(
std::string_view region,
std::string_view date,
std::string_view string_to_sign,
std::vector<unsigned char>& signature
) const -> ErrorCode;Relevant snippets from components/core/src/clp/hash_utils.hpp (lines 45, 57-61, 73-74):
// Converts a span of unsigned char to a hex string.
convert_to_hex_string(std::span<unsigned char> input) -> std::string;
// Computes HMAC-SHA256 hash of input using key, outputs to hash vector, returns ErrorCode.
get_hmac_sha256_hash(
std::span<unsigned char const> input,
std::span<unsigned char const> key,
std::vector<unsigned char>& hash
) -> ErrorCode;
// Computes SHA256 hash of input, outputs to hash vector, returns ErrorCode.
get_sha256_hash(std::span<unsigned char const> input, std::vector<unsigned char>& hash)
-> ErrorCode;Relevant snippets from components/core/src/clp_s/OperationFailed.hpp (multiple files):
// Exception class OperationFailed derived from TraceableException, constructed with error code, filename, and line number.
OperationFailed(ErrorCode error_code, char const* const filename, int line_number)
: TraceableException(error_code, filename, line_number) {}This exception is used in AwsAuthenticationSigner.cpp to throw errors on invalid URL format or endpoint.
These snippets provide context on:
- The
AwsAuthenticationSignerclass methods for generating presigned URLs, signing keys, and signatures. - The
S3Urlclass constructor parsing S3 URLs and throwingOperationFailedexceptions on errors. - Utility functions for hashing and encoding used in signing.
- The exception class
OperationFailedused for error handling.
This context is directly relevant to understanding the code in AwsAuthenticationSigner.cpp and to creating a bug report related to fixes in this file.
🧠 Learnings used
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: gibber9809
PR: y-scope/clp#504
File: components/core/src/clp_s/search/kql/CMakeLists.txt:29-29
Timestamp: 2024-10-22T15:36:04.655Z
Learning: When reviewing pull requests, focus on the changes within the PR and avoid commenting on issues outside the scope of the PR.
kirkrodrigues
approved these changes
Jul 26, 2025
Member
There was a problem hiding this comment.
Deferring to @gibber9809's review.
For the PR title, how about:
fix(core): Drop fractional seconds from formatted timestamps used for S3 signed-URL generation (fixes #1131).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
Aws requires that the timestamp in presigned Url strictly follow the ISO8601 Long Format "yyyyMMdd'T'HHmmss'Z'"
The current implementation of timestamp formatter 'S' will add fractional number and generate timestamp such as `2023-03-22T03-45-46.234232 which violates the requirement of AWS.
This PR resolves the issue by adding an explicit timepoint cast to convert the timestamp into second precision.
Note: I am not sure why this was not an issue in the previous commit. Perhaps it was some undefined behavior and recently we updated either fmt verison or c++ version that revealed this issue.
I have verified that commit 037cf10 doesn't have this issue, so perhaps we can do a binary search to find out the first commit revealing the issue if we want to understand the root cause
Checklist
breaking change.
Validation performed
./clp-s c output --print-archive-stats --auth s3 --timestamp-key 't.$date' https://yscope-log-compression-dataset-us-west-1.s3.us-west-1.amazonaws.com/mongodb-8gb/mongod.log.2023-03-22T03-45-46Summary by CodeRabbit