Skip to content
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.

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

Sending DurationSeconds in assumeRole request in SharedIniFileCredentials #2909

Conversation

soyelmnd
Copy link
Contributor

@soyelmnd soyelmnd commented Oct 16, 2019

tldr; as in title, respect duration_seconds of a chained profile if any (by passing it to the assume role request in SharedIniFileCredentials)

Checklist
  • npm run test passes
  • changelog is added, npm run add-change
  • run bundle exec rake docs:api and inspect doc/latest/index.html if documentation is changed

--

The unit test should hopefully reflect the use case; but for more context, we're running into the situation where a chained profile's duration_seconds doesn't get picked up by aws-sdk-js and therefore ends up having the 1h limit by default

[base-profile]
aws_access_key_id = access
aws_secret_access_key = secret

[default]
source_profile = base-profile
duration_seconds = 7200

// And this below credentials would last for just 1h instead of 2h as expected
new AWS.SharedIniFileCredentials({
  profile: 'default'
  disableAssumeRole: false,
});

// The workaround that we do now is to manually craft that Credentials
// (ie. create `baseProfile` credentials, assumeRole `default` with the
// correct `durationSeconds`, etc. literally what we have inside
// `SharedIniFileCredentials#loadRoleProfile()`), which is a bit cumbersome

@soyelmnd
Copy link
Contributor Author

Who could review this guys, @lsegal @jeskew @AllanFly120 @jstewmon maybe? (I see your names on the git blame 😃 apologize if I made a wrong tag). Thanks a lot

@jstewmon
Copy link
Contributor

I'm just a contributor, but these changes LGTM FWIW.

nit: I would consider this a feature, not a bugfix for the changelog entry.

@AllanZhengYP
Copy link
Contributor

Hi @soyelmnd, thank you for the contribution. Is the duration_seconds specified in any AWS documents or is there other language AWS SDK supports it?

@soyelmnd
Copy link
Contributor Author

soyelmnd commented Oct 22, 2019

Is the duration_seconds specified in any AWS documents

Yup, as @jstewmon linked above (thanks Jonathan) ☝️ the cli config, which I believe should be considered the source of truth for SharedIniFileCredentials, shouldn't it?

... or is there other language AWS SDK supports it?

That's a good question, not what I know of and might need to check. Though aws-cli has supported it nicely (from a quick glance, seems like since 1.16.73 thanks to this boto/botocore#1600, so my gut feeling is boto3 would support it too, but haven't tested)

I would consider this a feature, not a bugfix for the changelog entry.

Sounds good @jstewmon 👍 thanks

@soyelmnd
Copy link
Contributor Author

soyelmnd commented Nov 6, 2019

Could I kindly confirm if there's any other thing you want me to clarify on this please @AllanFly120, hope the above is clear enough?

@soyelmnd
Copy link
Contributor Author

soyelmnd commented Nov 15, 2019

A kind very kind ping on this small one please 👋

@soyelmnd
Copy link
Contributor Author

Hey guys, last ping of the year please, any feedbacks/updates on this tiny? 👋

@ajredniwja
Copy link
Contributor

@soyelmnd can the change log be updated?

@ajredniwja ajredniwja added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Jun 16, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #2909 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2909   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files         300      300           
  Lines        8992     8993    +1     
  Branches     1677     1678    +1     
=======================================
+ Hits         8719     8720    +1     
  Misses        273      273           
Impacted Files Coverage Δ
lib/credentials/shared_ini_file_credentials.js 97.64% <100.00%> (+0.02%) ⬆️

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 fd804e1...64d57c8. Read the comment docs.

@soyelmnd
Copy link
Contributor Author

Just done @ajredniwja, thanks for encouraging 👍 anything else needs to be done and who could review this long pending tiny one please?

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-v2-github
  • Commit ID: 64d57c8
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Jun 17, 2020
@soyelmnd
Copy link
Contributor Author

Kindly ping on this 1+ yo small PR friends 👋 any suggestions who can review it pls?

@soyelmnd
Copy link
Contributor Author

soyelmnd commented Aug 30, 2021

Hey guys 👋 it's going to be the 2 year anniversary for this small PR soon, any chance I can get at least some feedback on what you think we should do with this? Cheers

@andphe
Copy link

andphe commented Sep 16, 2021

+1

Botocore already supports this https://github.com/boto/boto3/blob/master/CHANGELOG.rst#1962 , and Ruby SDK as well https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-core/CHANGELOG.md#3630-2019-08-15

@soyelmnd
Copy link
Contributor Author

soyelmnd commented May 30, 2022

Hey guys, ping on this simple yet very long pending one 👋 @AllanZhengYP could you please suggest if this can get reviewed and/or who can review it? @aws/aws-sdk-js-team @trivikr maybe?

@github-actions
Copy link

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 31, 2023
@soyelmnd
Copy link
Contributor Author

Hey friends, it's 2023 now, with the change still looks relevant tho it hasn't received any feedback so far.

@ajredniwja @AllanZhengYP, unless I'm mistaken, all questions have been addressed, just wondering if you guys have any suggestion on how to move this trivial change forward?

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 1, 2023
@siddsriv siddsriv merged commit dd0206f into aws:master Jul 5, 2023
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.

9 participants