UB: Never expiring creds bug

[okruitho]

Issue #, if available:

Description of changes:
Fix non-expiring CRT credentials being incorrectly reported as expired by CrtCredentialsProvider.

CRT documents UINT64_MAX as “no expiration”. Previously, the provider converted this sentinel into a DateTime as though it were an ordinary timestamp, exceeding the representable range and causing IsExpired() and IsExpiredOrEmpty() to return true.

For gcc this works fine; when using clang the casts from uint64_t to double to uint64_t give undefined behaviour, depending on compliler optimisation.

Downstream impact

We encountered this in DuckDB’s AWS extension, which uses this check before copying credentials into an S3 secret. Our toolchain is clang based. The incorrect expiration status caused valid credentials to be omitted, surfacing in its MinIO integration test.

Changes

• Preserve AWSCredentials’ default non-expiring value when CRT returns UINT64_MAX.
• Use the uint64_t seconds constructor for finite expiration timestamps, avoiding the floating-point conversion.
• Add regression coverage for non-expiring credentials, ensuring all three state checks return false and subsequent retrieval uses cached credentials.
• Add coverage confirming finite expiration timestamps are preserved.

Check all that applies:

• Did a review by yourself.
• Added proper tests to cover this PR. (If tests are not applicable, explain.)
• Checked if this PR is a breaking (APIs have been changed) change.
• Checked if this PR will not introduce cross-platform inconsistent behavior.
• Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

• Linux
• Windows
• Android
• MacOS
• IOS
• Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[sbiscigl]

so looking at the DateTime class

/**
   * Initializes time point to epoch time in seconds with a millis mantissa,
   *
   * i.e. 1.1 would be 1100 milliseconds
   */
DateTime(double secondsSinceEpoch);

/**
   * Initializes time point to epoch time in seconds
   */
DateTime(uint64_t secondsSinceEpoch);

since GetExpirationTimepointInSeconds returns a uint64_t is the double cast even needed? could have just been a oversight in the initial implementation. removing the cast seems like a better idea than comparing numeric limits.

looking at the original implementation that was written at roughly the same time as the uint64_t constructor was added. so that timeline likely tangled up this issue. if you update to use the uint64_t constructor, i see no issue in merging this.

[okruitho]

Unfortunately, when assigning an MAX ULONG to an datetime it also overflows..

With regards to the code, this is done here. Therefore, I don't see a path to fix this without overhauling more code.

[sbiscigl]

so from my understanding std::chrono::system_clock::time_point is only meant to hold at max std::chrono::system_clock::duration:::max(). which on platforms, as you mention is lower than std::numeric_limits<uint64_t>::max and gives you this overflow. We should fix this in the datetime class and not at the call site. the call site should call with a uint64_t no casting, then in the datetime constructor for uint64_t we should floor it with maximum representable time to closes represent what we are doing. creating a date time class with the longest expiration possible given the platform.

so we should update the call site to use uint64_t then update the constructor to do something like

DateTime::DateTime(uint64_t secondsSinceEpoch) : m_valid(true)
{
  using sys_dur = std::chrono::system_clock::duration;
  auto max_secs = std::chrono::duration_cast<std::chrono::seconds>(sys_dur::max()).count();
  std::chrono::seconds secs{ std::min<uint64_t>(secondsSinceEpoch, max_secs) };
  m_time = std::chrono::system_clock::time_point(secs);
}

constructing a DateTime with overflowing uint64_t is something that we likely dont want to do, and want to avoid at construction.

[okruitho]

I applied your suggestion 👍

[sbiscigl]

thank you so much, for CI and procedure reasons i need to pull this PR onto a branch owned by the team. will merge it from there, your commit will still credit you, thanks for contributing!

[sbiscigl]

Created a PR with your commit we intend on merging #3924