Skip to content

fix(core): Drop fractional seconds from formatted timestamps used for S3 signed-URL generation (fixes #1131). #1128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 26, 2025
Merged

fix(core): Drop fractional seconds from formatted timestamps used for S3 signed-URL generation (fixes #1131). #1128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion components/core/src/clp/aws/AwsAuthenticationSigner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ auto is_unreserved_characters(char c) -> bool {

auto get_formatted_timestamp_string(std::chrono::system_clock::time_point const& timestamp)
-> string {
return fmt::format("{:%Y%m%dT%H%M%SZ}", timestamp);
auto const timestamp_secs = std::chrono::time_point_cast<std::chrono::seconds>(timestamp);
return fmt::format("{:%Y%m%dT%H%M%SZ}", timestamp_secs);
Comment on lines +94 to +95
Copy link
Member

@kirkrodrigues kirkrodrigues Jul 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai Can you open a bug report corresponding to this fix?

Copy link
Contributor

@coderabbitai coderabbitai bot Jul 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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 AwsAuthenticationSigner class methods for generating presigned URLs, signing keys, and signatures.
  • The S3Url class constructor parsing S3 URLs and throwing OperationFailed exceptions on errors.
  • Utility functions for hashing and encoding used in signing.
  • The exception class OperationFailed used 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.

}

auto get_formatted_date_string(std::chrono::system_clock::time_point const& timestamp) -> string {
Expand Down
Loading