Add support for AWS session tokens #30414

DaveCTurner · 2018-05-06T11:57:00Z

AWS supports the creation and use of credentials that are only valid for a
fixed period of time. These credentials comprise three parts: the usual access
key and secret key, together with a session token. This commit adds support for
these three-part credentials to the EC2 discovery plugin and the S3 repository
plugin.

Note that session tokens are only valid for a limited period of time and yet
there is no mechanism for refreshing or rotating them when they expire without
restarting Elasticsearch. Nonetheless, this feature is already useful for
nodes that need only run for a few days, such as for training, testing or
evaluation. #29135 tracks the work towards allowing these credentials to be
refreshed at runtime.

Resolves #16428

AWS supports the creation and use of credentials that are only valid for a fixed period of time. These credentials comprise three parts: the usual access key and secret key, together with a session token. This commit adds support for these three-part credentials to the EC2 discovery plugin and the S3 repository plugin. Note that session tokens are only valid for a limited period of time and yet there is no mechanism for refreshing or rotating them when they expire without restarting Elasticsearch. Nonetheless, this feature is already useful for nodes that need only run for a few days, such as for training, testing or evaluation. elastic#29135 tracks the work towards allowing these credentials to be refreshed at runtime. Resolves elastic#16428

elasticmachine · 2018-05-06T11:57:01Z

Pinging @elastic/es-distributed

elasticmachine · 2018-05-06T11:57:03Z

Pinging @elastic/es-security

tlrx

Thanks @DaveCTurner, this looks great. I'm curious to see how it will play with reloadable secured settings (#29135.

Anyway, do you think that we can have an integration test in :qa:amazon-s3 for this? That might be tricky to rotate tokens on infra, but I think it worth the trouble.

tlrx · 2018-05-22T15:16:56Z

...ins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java

+        assertThat(defaultSettings.proxyPassword, isEmptyString());
+        assertThat(defaultSettings.readTimeoutMillis, is(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT));
+        assertThat(defaultSettings.maxRetries, is(ClientConfiguration.DEFAULT_RETRY_POLICY.getMaxErrorRetry()));
+        assertThat(defaultSettings.throttleRetries, is(ClientConfiguration.DEFAULT_THROTTLE_RETRIES));


Thanks for adding these

tlrx · 2018-05-22T15:18:38Z

...ins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/S3ClientSettingsTests.java

+            () -> {
+                final MockSecureSettings secureSettings = new MockSecureSettings();
+                secureSettings.setString("s3.client.default.access_key", "aws_key");
+                S3ClientSettings.load(Settings.builder().setSecureSettings(secureSettings).build());


Nit: you can build the settings outside the expectThrows block, I find it easier to "spot" where the exception is expected to be thrown

++ I pushed ee41de6

tlrx · 2018-05-22T15:29:15Z

docs/plugins/discovery-ec2.asciidoc

@@ -46,6 +46,11 @@ Those that must be stored in the keystore are marked as `Secure`.

    An s3 secret key. The `access_key` setting must also be specified. (Secure)

+`session_token`::
+
+    An s3 session token. The `access_key` and `secret_key` settings must also


Nit: An -> A

This is tricky but I think "an" is preferred here: S3 is pronounced "ess three" which begins with a vowel. However, on reflection these docs should refer to EC2 and not S3, so I pushed c93e568.

Did I just correct an English about his English grammar? Yes I did. Was I right? Absolutly not.

Thanks :)

tlrx · 2018-05-22T15:29:21Z

docs/plugins/repository-s3.asciidoc

@@ -73,6 +73,11 @@ are marked as `Secure`.

    An s3 secret key. The `access_key` setting must also be specified. (Secure)

+`session_token`::
+
+    An s3 session token. The `access_key` and `secret_key` settings must also


Nit: An -> A

DaveCTurner · 2018-06-07T10:55:35Z

@tlrx I duplicated the QA tests as we discussed, and tightened up the auth checks in AmazonS3TestServer a bit in order to check that the right bucket is being accessed. This is ready for another look.

tlrx

It looks great, I left more comment, mostly around the naming of the env vars (I'd like to keep the old names) and also allow the tests to be executed separatly.

tlrx · 2018-06-08T07:29:14Z

...tory-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3TestServer.java

        }

        final RequestHandler handler = handlers.retrieve(method + " " + path, params);
        if (handler != null) {
+            final String bucket = params.get("bucket");
+            if (bucket != null && permittedBucket.equals(bucket) == false) {
+                // allow a null bucket to support bucket-free APIs like ListBuckets?


allow a null bucket to support bucket-free APIs like ListBuckets?

I don't think it's needed, returning an error seems the right thing to do

It is needed, but it's not ListBuckets so I pushed 63caae2. It's the multi-object delete API, which puts the bucket name in the Host header rather than in a URL parameter, which I do not think is worth adding the extra plumbing for.

tlrx · 2018-06-08T07:32:19Z

plugins/repository-s3/qa/amazon-s3/build.gradle

-    s3Bucket = 'bucket_test'
-    s3BasePath = 'integration_test'
+// We test against two repositories, one which uses the usual two-part "permanent" credentials and
+// the other which uses three-part "temporary" or "session" credentials.


Hopefully we'll split this into two QA projects in a follow up PR, so for now I'd prefer to not rename the existing env vars.

I understand the permanent/temporary renaming for repositories or buckets but I still think that the env vars names should reflect the feature implemented, ie keeping amazon_s3_access_key for the default use case, and maybe amazon_s3_access_key_for_session_tokens.

I have reverted the changes to the names of the existing environment variables.

tlrx · 2018-06-08T07:36:22Z

plugins/repository-s3/qa/amazon-s3/build.gradle

+// If all these variables are missing then we are testing against the internal fixture instead, which has the following
+// credentials hard-coded in.
+
+if (!s3PermanentAccessKey && !s3PermanentSecretKey && !s3PermanentBucket && !s3PermanentBasePath


Can we set the defaults in separate condition blocks? So that if a set [amazon_s3_access_key/amazon_s3_secret_key/amazon_s3_base_path/amazon_s3_bucket] is set it runs against the real service whatever is set for amazon_s3_access_key_for_session_tokens

As it currently stands, I think it makes more sense to either test entirely against the fake fixture or entirely against the real service (and therefore not run the fake fixture). If there is a followup that allows these two cases to run separately then hopefully it's not too hard to disentangle this then.

tlrx · 2018-06-08T07:37:13Z

plugins/repository-s3/qa/amazon-s3/build.gradle

@@ -57,26 +78,34 @@ task s3Fixture(type: AntFixture) {
    dependsOn compileTestJava
    env 'CLASSPATH', "${ -> project.sourceSets.test.runtimeClasspath.asPath }"
    executable = new File(project.runtimeJavaHome, 'bin/java')
-    args 'org.elasticsearch.repositories.s3.AmazonS3Fixture', baseDir, s3Bucket
+    args 'org.elasticsearch.repositories.s3.AmazonS3Fixture', baseDir


I'd prefer if we continue to pass the bucket names to the fixture so that we can reuse it in different projects more easily.

Ok, I've reverted that. There is, however, already some other coupling (duplication of string values) between the build.gradle file and the fake S3 fixture that would need attention to use this fixture elsewhere.

tlrx · 2018-06-08T07:38:55Z

...ository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java

@@ -53,8 +53,8 @@
 public class AmazonS3Fixture {

    public static void main(String[] args) throws Exception {
-        if (args == null || args.length != 2) {
-            throw new IllegalArgumentException("AmazonS3Fixture <working directory> <bucket>");
+        if (args == null || args.length != 1) {


Can we revert this please?

We now pass the bucket names in on the command line again.

tlrx · 2018-06-08T07:39:23Z

...ository-s3/qa/amazon-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java

@@ -72,7 +72,8 @@ public static void main(String[] args) throws Exception {
            // Emulates S3
            final String storageUrl = "http://" + addressAndPort;
            final AmazonS3TestServer storageTestServer = new AmazonS3TestServer(storageUrl);
-            storageTestServer.createBucket(args[1]);
+            storageTestServer.createBucket("permanent_bucket_test");
+            storageTestServer.createBucket("temporary_bucket_test");


Can we revert this please, and pass the bucket names as program args?

We now pass the bucket names in on the command line again.

…-06-aws-session-token

…06-aws-session-token

…-06-aws-session-token

…06-aws-session-token - reverted conflicting changes and will re-make them by hand

... so that we can give them different accessibility.

DaveCTurner · 2018-06-25T17:16:22Z

@tlrx I have merged master, which was nontrivial thanks to #31383, and addressed some of your comments. I can't see how these two cases (permanent vs temporary credentials) are eventually going to work as two separate Gradle tasks, so I've held off on any changes that depends on that. I would prefer to merge this and proceed from there, but if you think anything here will prove obstructive then it'd be better not to.

tlrx

Sorry for the time it took me to review this.

This PR should be merged before #31688, in which the QA project is dropped.

As a follow ups:

we need to add a test for session tokens with discovery-ec2
this PR will prevent the current third party tests to run on CI until session tokens are injected to the Jenkins job (that's why I suggested to decorrelate env vars)
I think we can clean up test and have something similar to Fixture for Minio testing #31688, ie executing the same tests but with a different repository set up. We talked about this already and decided to do this as a follow up as this PR was already large enough. I can take care of this and mark you as a reviewer because there are other things I want to clean up.

AWS supports the creation and use of credentials that are only valid for a fixed period of time. These credentials comprise three parts: the usual access key and secret key, together with a session token. This commit adds support for these three-part credentials to the EC2 discovery plugin and the S3 repository plugin. Note that session tokens are only valid for a limited period of time and yet there is no mechanism for refreshing or rotating them when they expire without restarting Elasticsearch. Nonetheless, this feature is already useful for nodes that need only run for a few days, such as for training, testing or evaluation. #29135 tracks the work towards allowing these credentials to be refreshed at runtime. Resolves #16428

* 6.x: Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) (#31789) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) [ML] Limit ML filter items to 10K (#31731) Fixture for Minio testing (#31688) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Painless: Complete Removal of Painless Type (#31699) Consolidate watcher setting update registration (#31762) [DOCS] Adds empty 6.3.1 release notes page ingest: Introduction of a bytes processor (#31733) [test] don't run bats tests for suse boxes (#31749) Add analyze API to high-level rest client (#31577) Implemented XContent serialisation for GetIndexResponse (#31675) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) Support multiple system store types (#31650) Add write*Blob option to replace existing blob (#31729) Split CircuitBreaker-related tests (#31659) Painless: Add Context Docs (#31190) Docs: Remove missing reference Migrate scripted metric aggregation scripts to ScriptContext design (#30111) Watcher: Fix chain input toXcontent serialization (#31721) Remove _all example (#31711) rest-high-level: added get cluster settings (#31706) Docs: Match the examples in the description (#31710) [Docs] Correct typos (#31720) Extend allowed characters for grok field names (#21745) (#31653) (#31722) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Fix gradle4.8 deprecation warnings (#31654) Update numbers to reflect 4-byte UTF-8-encoded characters (#27083)

* master: [ML] Rate limit established model memory updates (#31768) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) Detach Transport from TransportService (#31727) [ML] Limit ML filter items to 10K (#31731) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) Fixture for Minio testing (#31688) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Painless: Complete Removal of Painless Type (#31699) Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) Consolidate watcher setting update registration (#31762) Build: re-enabled bwc (#31769) ingest: Introduction of a bytes processor (#31733) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Add analyze API to high-level rest client (#31577) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) Implemented XContent serialisation for GetIndexResponse (#31675) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) resolveHasher defaults to NOOP (#31723) Account for XContent overhead in in-flight breaker Split CircuitBreaker-related tests (#31659) Add write*Blob option to replace existing blob (#31729) Painless: Add Context Docs (#31190) Watcher: Fix chain input toXcontent serialization (#31721) Docs: Match the examples in the description (#31710) rest-high-level: added get cluster settings (#31706) [Docs] Correct typos (#31720) Clean up double semicolon code typos (#31687) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Revert long lines Fix TransportChangePasswordActionTests

Add PR number to changelog

0fbe039

tlrx self-requested a review May 7, 2018 09:02

tlrx reviewed May 22, 2018

View reviewed changes

DaveCTurner added 7 commits May 22, 2018 17:05

Merge branch 'master' into 2018-05-06-aws-session-token

0299a75

discovery-ec2 docs shouldn't refer to S3

c93e568

Reduce scope of expectThrows

ee41de6

Merge branch 'master' into 2018-05-06-aws-session-token

ed62728

Support injection of session token from the environment

85737f6

Extend QA tests to test using both temporary and permanent credentials

2eea829

Imports

d431f28

tlrx reviewed Jun 8, 2018

View reviewed changes

DaveCTurner added 9 commits June 25, 2018 11:28

Merge commit 'afc91e92fb697a3241b26a5bb7cdc0a3769f4085^' into 2018-05…

ff9f8a1

…-06-aws-session-token

Merge commit 'afc91e92fb697a3241b26a5bb7cdc0a3769f4085' into 2018-05-…

2602b32

…06-aws-session-token

Merge commit '3378240b2954fcf879ac84f440002685c4b30cc9^' into 2018-05…

9fd5d44

…-06-aws-session-token

Merge commit '3378240b2954fcf879ac84f440002685c4b30cc9' into 2018-05-…

9db4a59

…06-aws-session-token - reverted conflicting changes and will re-make them by hand

Merge branch 'master' into 2018-05-06-aws-session-token

c1e0a97

Add support for serving two buckets

552e852

... so that we can give them different accessibility.

Reinstate verification of credentials against the appropriate bucket

e583a0c

Update docs

87b6c58

Add discovery-ec2 session token setting

7eabb56

DaveCTurner added 7 commits June 25, 2018 16:33

Use session token in discovery-ec2 if provided

6915c47

Add assertions to Ec2DiscoveryPluginTests too

1475a26

Add support for session token to repository-s3 plugin

439b763

Extend QA tests to test both permanent and temporary credentials

2a93e94

Be pickier about auth verification in AmazonS3Fixture

60e7b83

Resolve merge conflicts manually

9de75c6

Be more certain

63caae2

DaveCTurner added 3 commits June 25, 2018 18:22

Merge branch 'master' into 2018-05-06-aws-session-token

be7b51c

Less hard-coding

d6abddc

Tidy imports

aafb0aa

tlrx approved these changes Jul 2, 2018

View reviewed changes

DaveCTurner added 4 commits July 2, 2018 13:49

Merge elastic#31601

ff29696

Merge elastic#31601 for temp creds too

86fa97a

Merge branch 'master' into 2018-05-06-aws-session-token

34ffc72

Merge branch 'master' into 2018-05-06-aws-session-token

da791b3

DaveCTurner merged commit 4108722 into elastic:master Jul 3, 2018

DaveCTurner deleted the 2018-05-06-aws-session-token branch July 3, 2018 13:12

ywelsch mentioned this pull request Jul 3, 2018

[TEST] Fix test "Register a repository with a non existing bucket" #31773

Closed

jpountz removed :Distributed/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Jan 29, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AWS session tokens #30414

Add support for AWS session tokens #30414

DaveCTurner commented May 6, 2018 •

edited

Loading

elasticmachine commented May 6, 2018

elasticmachine commented May 6, 2018

tlrx left a comment

tlrx May 22, 2018

tlrx May 22, 2018

DaveCTurner May 22, 2018

tlrx May 22, 2018

DaveCTurner May 22, 2018

tlrx May 22, 2018

tlrx May 22, 2018

DaveCTurner commented Jun 7, 2018

tlrx left a comment

tlrx Jun 8, 2018

DaveCTurner Jun 25, 2018

tlrx Jun 8, 2018

DaveCTurner Jun 25, 2018

tlrx Jun 8, 2018

DaveCTurner Jun 25, 2018

tlrx Jun 8, 2018

DaveCTurner Jun 25, 2018

tlrx Jun 8, 2018

DaveCTurner Jun 25, 2018

tlrx Jun 8, 2018

DaveCTurner Jun 25, 2018

DaveCTurner commented Jun 25, 2018 •

edited

Loading

tlrx left a comment

Add support for AWS session tokens #30414

Add support for AWS session tokens #30414

Conversation

DaveCTurner commented May 6, 2018 • edited Loading

elasticmachine commented May 6, 2018

elasticmachine commented May 6, 2018

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Jun 7, 2018

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Jun 25, 2018 • edited Loading

tlrx left a comment

Choose a reason for hiding this comment

DaveCTurner commented May 6, 2018 •

edited

Loading

DaveCTurner commented Jun 25, 2018 •

edited

Loading