[FEATURE] Use STS to receive a temporary credentials role session #12
[FEATURE] Use STS to receive a temporary credentials role session #12MurraySpeight wants to merge 3 commits intokeitaroinc:mainfrom
Conversation
Configuring a role will assume an IAM role by using AWS Security Token Service. This is more secure than using a long-term password or access key credentials. A session has a limited duration, which reduces the risk if the credentials are compromised.
MurraySpeight
changed the title
|
Hey @MurraySpeight, thank you for your contribution, could you please check and fix the code quality and also please add some tests that cover the new functionality? |
| RoleArn=self.role, | ||
| RoleSessionName="CkanExtS3Session") | ||
| credentials = assumed_role_object['Credentials'] | ||
| return boto3.session.Session( |
There was a problem hiding this comment.
Perhaps assign this to session (which is about to be returned) instead of returning it directly, so there's only a single exit point?
Or change line 71 into a guard clause, "if no role specified then return immediately".
There was a problem hiding this comment.
I think you've missed the part where the initial session is used to communicate with STS if there is a role configured. So in this case both sessions are needed but only the one that has the assumed role is returned to be used for communicating with S3.
There was a problem hiding this comment.
No, I didn't miss that. I'm just suggesting that it's more readable to assign and then return, instead of having two separate return statements.
(Having only one exit from a function is more of a guideline than a hard rule, but it's not a bad guideline when it's convenient.)
I've fixed the lint issue. I can't see how this test would be written based on what is there so I'm not going to be able to add a test case for this. |
Perhaps you could mock out the |
3 participants
#11