Skip to content

Fix timestamp format for SSOCredentialFetcher#2250

Merged
nateprewitt merged 1 commit intoboto:developfrom
nateprewitt:timestamp_fix
Dec 17, 2020
Merged

Fix timestamp format for SSOCredentialFetcher#2250
nateprewitt merged 1 commit intoboto:developfrom
nateprewitt:timestamp_fix

Conversation

@nateprewitt
Copy link
Copy Markdown
Contributor

The timestamp format for SSO credentials was originally intended to be an RFC 3339 style timestamp in UTC. Due to a typo in the original AssumeRole implementation, the timezone is being converted to UTC instead of a literal Z. We're going to correct the SSO implementation since the current timestamp is incompatible with the other AWS SDKs. The AssumeRole timestamp will be left unchanged for now to preserve as much backwards compatibility as possible.

Customers that are upgrading should have their timestamp format updated on their next login or credential refresh with SSO.

@nateprewitt nateprewitt requested a review from kyleknap December 14, 2020 23:19
@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 14, 2020

Codecov Report

Merging #2250 (0429522) into develop (96f9740) will decrease coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2250      +/-   ##
===========================================
- Coverage    98.25%   97.86%   -0.40%     
===========================================
  Files           58       58              
  Lines        10857    10877      +20     
===========================================
- Hits         10668    10645      -23     
- Misses         189      232      +43     
Impacted Files Coverage Δ
botocore/credentials.py 98.64% <100.00%> (+<0.01%) ⬆️
botocore/compat.py 72.15% <0.00%> (-22.16%) ⬇️
botocore/hooks.py 95.18% <0.00%> (-0.81%) ⬇️
botocore/waiter.py 98.24% <0.00%> (-0.44%) ⬇️
botocore/awsrequest.py 97.22% <0.00%> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96f9740...0429522. Read the comment docs.

@benkehoe
Copy link
Copy Markdown

Technically, shouldn't _serialize_rfc3339_timestamp verify something like value.tzinfo == utc() before writing the zulu time format, and otherwise write it out with the offset from the tzinfo?

@miketheman
Copy link
Copy Markdown
Contributor

miketheman commented Dec 15, 2020

Technically, shouldn't _serialize_rfc3339_timestamp verify something like value.tzinfo == utc() before writing the zulu time format, and otherwise write it out with the offset from the tzinfo?

It appears that the value being passed in is generated from a timestamp, and the tz is being set as utc.
So long as this remains a private function, and is called with a value set correctly, then it should be fine, but it is good defensive practice to validate, or at least document in the function's docstring.

@nateprewitt
Copy link
Copy Markdown
Contributor Author

Agreed, we should add a docstring here for the private function so it's clear we're expecting the datetime to already be in UTC.

@nateprewitt nateprewitt force-pushed the timestamp_fix branch 3 times, most recently from 4dbf45a to b931eeb Compare December 17, 2020 22:13
Copy link
Copy Markdown
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Thanks! 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants