Allow specifying AWS role at runtime #81

rittneje · 2020-10-21T16:57:06Z

Fixes #80.

Testing

We have been using this feature on our CloudBees instance for several months now. We have not experienced any issues with it. We have been fetching credentials in two ways:

credentialsId, roleArn and roleSessionName provided
credentialsId, accessKeyVariable, and secretKeyVariable provided

Both have been working flawlessly.

In addition, when implementing this change, I did integration testing on our CloudBees instance to cover the following cases:

credentialsId, roleArn, roleSessionName, and roleSessionDurationSeconds
credentialsId and roleArn
credentialsId and roleArn, with roleSessionName as the empty string
credentialsId, with roleArn as the empty string
just credentialsId

All of these cases passed, as verified by calling sh 'aws sts get-caller-identity' within the withCredentials block.

rittneje · 2020-10-29T21:51:14Z

ping @jglick @escoem

jglick · 2020-10-29T22:01:34Z

I am not a maintainer.

rittneje · 2020-10-30T18:20:20Z

@jglick Any idea who the maintainers are?

jglick · 2020-10-30T18:42:49Z

@escoem perhaps.

rittneje · 2020-11-12T04:47:29Z

ping @escoem

rittneje · 2020-12-13T17:55:39Z

ping @escoem

rittneje · 2020-12-29T06:24:27Z

ping @escoem

rittneje · 2021-01-30T03:26:55Z

ping @escoem

rittneje · 2021-02-12T17:37:46Z

@escoem Is this project abandoned? This change is pretty important for us.

rittneje · 2021-02-25T22:02:54Z

ping @andresrc

alecharp

The big missing part of this PR is a test.
I don't know how much of code coverage we have here but changing code related with credentials without tests is a big risk.

The RoleSessionDurationSeconds limits problem I commented would have been detected with a test.

src/main/java/com/cloudbees/AWSCredentialsUtils.java

...n/java/com/cloudbees/jenkins/plugins/awscredentials/AmazonWebServicesCredentialsBinding.java

alecharp · 2021-03-03T09:33:42Z

...n/java/com/cloudbees/jenkins/plugins/awscredentials/AmazonWebServicesCredentialsBinding.java

+                .withStsClient(stsClient);
+
+        if (this.roleSessionDurationSeconds > 0) {
+            assumeRoleProviderBuilder = assumeRoleProviderBuilder
+                .withRoleSessionDurationSeconds(this.roleSessionDurationSeconds);
+        }


We might have a problem here.
The setter requires a value between 900 and 3600, or it throws an IllegalArgumentException. But we are not checking this.
However, the STSAssumeRoleSessionCredentialsProvider constructor will use DEFAULT_DURATION_SECONDS (900) if the value is equal to 0.

I would suggest to either have a doCheckX method in the descriptor to validate that the duration is truly between 900 and 3600. You can even have the default value to 900. This would not be used in pipeline setup.
Or you need to change the condition here.

(option 1)

Suggested change

.withStsClient(stsClient);

if (this.roleSessionDurationSeconds > 0) {

assumeRoleProviderBuilder = assumeRoleProviderBuilder

.withRoleSessionDurationSeconds(this.roleSessionDurationSeconds);

}

.withStsClient(stsClient)

.withRoleSessionDurationSeconds(this.roleSessionDurationSeconds);

(option 2)

Suggested change

.withStsClient(stsClient);

if (this.roleSessionDurationSeconds > 0) {

assumeRoleProviderBuilder = assumeRoleProviderBuilder

.withRoleSessionDurationSeconds(this.roleSessionDurationSeconds);

}

.withStsClient(stsClient);

if (this.roleSessionDurationSeconds >= 900 && this.roleSessionDurationSeconds <= 3600) {

assumeRoleProviderBuilder = assumeRoleProviderBuilder

.withRoleSessionDurationSeconds(this.roleSessionDurationSeconds);

}

My intent was to let the bounds just be controlled by the AWS SDK so that you don't have to keep the two in sync. (For example, a newer version of the SDK might change it so you can assume a role for up to 2 hours.) Similarly, I think it makes more sense to allow the default value to be chosen by the SDK, as it currently is.

I understand. The problem is that the "check" will be done by the library which throws an exception. I'm not sure this is the most clear way to handle the situation for users.

I'm a little confused. What exactly would we do in this code if the role session duration is out of bounds other than throw an exception ourselves?

rittneje · 2021-03-03T12:10:20Z

The big missing part of this PR is a test.
I don't know how much of code coverage we have here but changing code related with credentials without tests is a big risk.

The RoleSessionDurationSeconds limits problem I commented would have been detected with a test.

@alecharp I would need some guidance on how to write such tests. There don't appear to be any existing tests other than ConfigurationAsCodeTest.java (which is irrelevant).

jglick · 2021-03-03T13:28:36Z

Neither this plugin, nor aws-global-configuration, nor aws-java-sdk, have any tests which could actually exercise services.

artifact-manager-s3 has some Dockerized tests which run against MinIO; and some “live” tests which require AWS credentials and currently run only on a private server maintained by CloudBees, or on your local machine if you care to run them. In neither case would IAM/STS exotica like role assumption or MFAs be covered.

pipeline-cloudwatch-logs also has some live tests (currently not run in CI anywhere) and actually does do an AssumeRole call, but for specialized purposes and of course not covering the code in this PR.

Writing tests against live AWS is certainly possible, with some effort; making them exercise specific IAM/STS features is particularly hard, since the person/process running the test may not physically be permitted to use that feature, depending on how their account is set up. You can try to run Dockerized tests against OpenStack or various mock frameworks, but identity & security systems are likely to be the least accurately simulated, and anyway this is hardly proving that the code would actually work in AWS itself (though it could help detect regressions from routine refactorings not touching the basic design).

rittneje · 2021-03-08T16:08:45Z

ping @alecharp

rittneje · 2021-03-17T00:21:28Z

ping @alecharp

rittneje · 2021-03-24T17:31:43Z

ping @alecharp

rittneje · 2021-03-31T15:40:27Z

ping @alecharp

rittneje · 2021-04-09T15:28:08Z

ping @alecharp

…ration at runtime. Fixes #80. Signed-off-by: Jesse Rittner <rittneje@gmail.com>

rittneje · 2021-04-18T16:02:35Z

ping @alecharp

rittneje · 2021-04-26T15:25:03Z

ping @alecharp

rittneje · 2021-05-04T01:45:56Z

ping @alecharp

rittneje · 2021-05-10T16:46:46Z

ping @alecharp

rittneje · 2021-05-17T13:40:52Z

ping @alecharp

rittneje · 2021-05-25T03:02:48Z

ping @alecharp

rittneje · 2021-06-02T15:40:59Z

ping @alecharp

rittneje · 2021-06-07T22:47:32Z

ping @alecharp

rittneje · 2021-06-12T17:31:33Z

@alecharp This pull request has been open for almost 8 months. It has been over 3 months since you reviewed. Please merge this already. If you don't have the time to review it, then please tell me who can.

cc @escoem @andresrc

rittneje · 2021-06-21T01:17:11Z

@alecharp @escoem @andresrc

rittneje · 2021-06-29T05:03:08Z

@alecharp @escoem @andresrc

rittneje · 2021-07-06T19:49:56Z

@alecharp @escoem @andresrc

rittneje · 2021-07-12T15:21:02Z

@alecharp @escoem @andresrc

alecharp

LGTM. Seems we just have to wait a bit for @escoem to be available to merge and release this.

alecharp suggested changes Mar 3, 2021

View reviewed changes

rittneje requested a review from alecharp April 9, 2021 15:30

Allow specifying AWS role ARN, role session name, and role session du…

bda0dac

…ration at runtime. Fixes #80. Signed-off-by: Jesse Rittner <rittneje@gmail.com>

alecharp approved these changes Jul 29, 2021

View reviewed changes

escoem merged commit c748e81 into jenkinsci:master Aug 9, 2021

Allow specifying AWS role at runtime #81

Allow specifying AWS role at runtime #81

Conversation

rittneje commented Oct 21, 2020 • edited Loading

Testing

rittneje commented Oct 29, 2020

jglick commented Oct 29, 2020

rittneje commented Oct 30, 2020

jglick commented Oct 30, 2020

rittneje commented Nov 12, 2020

rittneje commented Dec 13, 2020

rittneje commented Dec 29, 2020

rittneje commented Jan 30, 2021

rittneje commented Feb 12, 2021

rittneje commented Feb 25, 2021

alecharp left a comment

Choose a reason for hiding this comment

alecharp Mar 3, 2021

Choose a reason for hiding this comment

rittneje Mar 3, 2021

Choose a reason for hiding this comment

alecharp Mar 3, 2021

Choose a reason for hiding this comment

rittneje Mar 3, 2021

Choose a reason for hiding this comment

rittneje commented Mar 3, 2021

jglick commented Mar 3, 2021

rittneje commented Mar 8, 2021

rittneje commented Mar 17, 2021

rittneje commented Mar 24, 2021

rittneje commented Mar 31, 2021

rittneje commented Apr 9, 2021

rittneje commented Apr 18, 2021

rittneje commented Apr 26, 2021

rittneje commented May 4, 2021

rittneje commented May 10, 2021

rittneje commented May 17, 2021

rittneje commented May 25, 2021

rittneje commented Jun 2, 2021

rittneje commented Jun 7, 2021

rittneje commented Jun 12, 2021

rittneje commented Jun 21, 2021

rittneje commented Jun 29, 2021

rittneje commented Jul 6, 2021

rittneje commented Jul 12, 2021

alecharp left a comment

Choose a reason for hiding this comment

rittneje commented Oct 21, 2020 •

edited

Loading