Skip to content

Add Date.now() to expiry when passing it to Expiration#8254

Merged
Neon-White merged 1 commit into
noobaa:masterfrom
Neon-White:cast_sts_expiry
Sep 17, 2024
Merged

Add Date.now() to expiry when passing it to Expiration#8254
Neon-White merged 1 commit into
noobaa:masterfrom
Neon-White:cast_sts_expiry

Conversation

@Neon-White

@Neon-White Neon-White commented Aug 5, 2024

Copy link
Copy Markdown
Contributor

Explain the changes

  1. The Expiration field in the response to AssumeRole returned the value in milliseconds; it should be in seconds, and added to the current time; this is now fixed
  2. generate_session_token() passed expiry in milliseconds when make_auth_token expects seconds, this is now fixed
  3. It is now possible to provide an expiration in seconds by providing the AWS CLI flag --duration-seconds

Issues: Fixed #xxx / Gap #xxx

  1. Fixes BZ#2299801

Testing Instructions:

  1. Follow the STS test section here, observe the return value of Expiration:
  2. Try to utilize the credentials and observe whether they expire in the timestamp shown in the previous step
  • Doc added/updated
  • Tests updated

Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
@Neon-White Neon-White changed the title Cast expiry to string to prevent it from being cast to an epoch time Add Date.now() to expiry when passing it to Expiration Aug 5, 2024
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/test/unit_tests/test_sts.js Outdated
@liranmauda liranmauda requested a review from guymguym August 7, 2024 08:22
@liranmauda liranmauda requested a review from shirady August 19, 2024 09:54
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
@Neon-White Neon-White requested a review from shirady August 22, 2024 09:47
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Aug 22, 2024

@shirady shirady left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
@Neon-White Neon-White requested a review from guymguym August 27, 2024 12:09
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
@Neon-White Neon-White requested a review from guymguym September 4, 2024 11:28

@guymguym guymguym left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

final minor comments

Comment thread config.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Sep 5, 2024
@Neon-White Neon-White requested a review from guymguym September 5, 2024 08:36
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
Comment thread src/endpoint/sts/ops/sts_post_assume_role.js Outdated
@pull-request-size pull-request-size Bot removed the size/M label Sep 9, 2024

@guymguym guymguym left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code looks ok to me, but did you test it with aws sdk?

@Neon-White

Copy link
Copy Markdown
Contributor Author

@guymguym - Thanks for the reviews and input, I will make sure to verify it with AWS CLI and fix the unittest prior to merging

@Neon-White

Copy link
Copy Markdown
Contributor Author

@guymguym - The PR seems to work great with the AWS CLI, a few points I noticed:

  1. Permissions are enforced as expected
  2. The printed expiration timestamp is in GMT+0 regardless of the system time
  3. Sadly, the range validation errors don't seem to function as intended, since the CLI returns An error occurred (Unknown) when calling the AssumeRole operation: Unknown, even though the logs very clearly show that the correct exception was thrown -
[Endpoint/13] [ERROR] core.endpoint.sts.sts_rest:: STS ERROR <?xml version="1.0" encoding="UTF-8"?><Error><Code>ValidationError</Code><Message>Value 400000 for durationSeconds failed to satisfy constraint: Member must have value less than or equal to 43200</Message>

Any chance you already know why that'd happen?
If not, I suggest merging the PR in its current state and taking care of this problem in a separate PR (or creating an issue to track it)

Signed-off-by: Ben <belimele@redhat.com>
@Neon-White Neon-White merged commit b28bd87 into noobaa:master Sep 17, 2024
@Neon-White Neon-White deleted the cast_sts_expiry branch September 17, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants