Make s3 repository sensitive settings use secure settings by rjernst · Pull Request #22479 · elastic/elasticsearch

rjernst · 2017-01-06T22:01:58Z

This change converts repository-s3 to use the new secure settings. In
order to support the multiple ways we allow aws creds to be configured,
it also moves the main methods for the keystore wrapper into a
SecureSettings interface, in order to allow settings prefixing to work.

This change converts repository-s3 to use the new secure settings. In order to support the multiple ways we allow aws creds to be configured, it also moves the main methods for the keystore wrapper into a SecureSettings interface, in order to allow settings prefixing to work.

rjernst · 2017-01-06T22:02:56Z

I still need to figure out some tests for this, but I wanted to get this up here to get some feedback on how it looks.

s1monw

LGTM I don't really know how we should test this. I think the simplest would be to have a class that you can override that adds factory methods for StaticCredentialsProvider etc. instead of using a static method.?

s1monw · 2017-01-09T20:29:25Z

core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java

-        if (entry instanceof KeyStore.SecretKeyEntry == false) {
-            throw new IllegalStateException("Secret setting " + setting + " is not a string");
+    @Override
+    public SecureString getString(String setting) {


can't we throw GeneralSecurityException do we have to wrap?

Sure, we could throw it. I was thinking ahead to Vault integration, but I guess that could make GeneralSecurityExceptions?

I take it back. Unfortunately, GeneralSecureException is not the only thing that can be thrown. Not all keystore apis throw a subclass of this. There are like 3 different exceptions. :(

Oops, nevermind. Done!

rjernst · 2017-01-10T06:10:44Z

@s1monw I found the existing tests for the old settings, made them work with deprecation log checks, and duplicated them to use new secure settings.

rjernst · 2017-01-10T08:32:50Z

retest this please

s1monw

LGTM

rjernst · 2017-01-11T01:07:45Z

retest this please

* Settings: Make s3 repository sensitive settings use secure settings This change converts repository-s3 to use the new secure settings. In order to support the multiple ways we allow aws creds to be configured, it also moves the main methods for the keystore wrapper into a SecureSettings interface, in order to allow settings prefixing to work.

… props, and profile files This is a follow up to elastic#22479, where storing credentials secure way was added.

dadoonet · 2017-01-13T11:44:01Z

@rjernst I tried some first tests today:

With elasticsearch 5.1.1

In my config/elasticsearch.yml file:

cloud.aws.access_key: "ACCESS-KEY"
cloud.aws.secret_key: "SECRET-KEY"
repositories.s3.bucket: "test.eu-west-1.elasticsearch.org"
repositories.s3.region: "eu-west-1"

Then I start elasticsearch and run:

curl -XPOST 'http://localhost:9200/_snapshot/backups?pretty=true' -d '{ "type":"s3" }'

This gives:

{
  "acknowledged" : true
}

With elasticsearch 6.0.0

bin/elasticsearch-keystore create
bin/elasticsearch-keystore add cloud.aws.secret_key
# Entering here ACCESS-KEY
bin/elasticsearch-keystore add cloud.aws.access_key
# Entering here SECRET-KEY

In elasticsearch.yml:

repositories.s3.bucket: "test.eu-west-1.elasticsearch.org"
repositories.s3.region: "eu-west-1"

Then I start elasticsearch and run:

curl -XPOST 'http://localhost:9200/_snapshot/backups?pretty=true' -d '{ "type":"s3" }'

This gives:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "repository_verification_exception",
        "reason" : "[backups] path  is not accessible on master node"
      }
    ],
    "type" : "repository_verification_exception",
    "reason" : "[backups] path  is not accessible on master node",
    "caused_by" : {
      "type" : "i_o_exception",
      "reason" : "Unable to upload object tests-Z8dZ3RULTSKNXfoVU222Rw/master.dat-temp",
      "caused_by" : {
        "type" : "amazon_s3_exception",
        "reason" : "The AWS Access Key Id you provided does not exist in our records. (Service: Amazon S3; Status Code: 403; Error Code: InvalidAccessKeyId; Request ID: FE35B1FCEC30375A)"
      }
    }
  },
  "status" : 500
}

May be I forgot something though.
I'm going to look at it in debug mode and will update here with findings.

dadoonet · 2017-01-13T14:25:07Z

Apparently I'm getting back an empty String when running

elasticsearch/plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

Line 131 in 8015fbb

try (SecureString key = getValue(repositorySettings, settings,

This is then trying to use environment variables, system properties or instance profile credentials. Then it causes that error message.

I don't understand why I'm getting an empty String while asking for repositories.s3.access_key and repositories.s3.secret_key.
Note that I also added those two settings with elasticsearch-keystore app:

$ bin/elasticsearch-keystore list
cloud.aws.access_key
cloud.aws.secret_key
repositories.s3.access_key
repositories.s3.secret_key

… props, and remove profile files (#22567) * S3 repository: Deprecate specifying credentials through env vars and sys props This is a follow up to #22479, where storing credentials secure way was added.

This change implements named configurations for s3 repository as proposed in elastic#22520. The access/secret key secure settings which were added in elastic#22479 are reverted, and the only secure settings are those with the new named configs. All other previously used settings for the connection are deprecated. closes elastic#22520

* S3 repository: Add named configurations This change implements named configurations for s3 repository as proposed in #22520. The access/secret key secure settings which were added in #22479 are reverted, and the only secure settings are those with the new named configs. All other previously used settings for the connection are deprecated. closes #22520

This change implements named configurations for s3 repository as proposed in #22520. The access/secret key secure settings which were added in #22479 are reverted, and the only secure settings are those with the new named configs. All other previously used settings for the connection are deprecated. closes #22520

rjernst added :Core/Infra/Settings Settings infrastructure and APIs v5.3.0 v6.0.0-alpha1 labels Jan 6, 2017

clintongormley changed the title ~~Settings: Make s3 repository sensitive settings use secure settings~~ Make s3 repository sensitive settings use secure settings Jan 9, 2017

clintongormley added the >enhancement label Jan 9, 2017

s1monw approved these changes Jan 9, 2017

View reviewed changes

rjernst added 4 commits January 9, 2017 14:54

Add throws for getString

15f1688

adding tests infra for mock secure settings and s3 tests

5437c6e

Merge branch 'master' into keystore2

035a158

Add tests

0e82e65

Simplify boolean return

ee41ab5

rjernst mentioned this pull request Jan 10, 2017

Simplify aws configuration #22520

Closed

s1monw approved these changes Jan 10, 2017

View reviewed changes

rjernst added 2 commits January 10, 2017 12:55

Fix checkstyle

3844bbc

Merge branch 'master' into keystore2

97d72be

fix more tests

5f42a6d

rjernst merged commit 8015fbb into elastic:master Jan 11, 2017

rjernst deleted the keystore2 branch January 11, 2017 19:19

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Jan 11, 2017

S3 repository: Deprecate specifying credentials through env vars, sys…

e3f06b1

… props, and profile files This is a follow up to elastic#22479, where storing credentials secure way was added.

rjernst mentioned this pull request Jan 11, 2017

Deprecate specifying credentials through env vars, sys props, and remove profile files #22567

Merged

rjernst mentioned this pull request Jan 24, 2017

S3 repository: Add named configurations #22762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make s3 repository sensitive settings use secure settings#22479

Make s3 repository sensitive settings use secure settings#22479
rjernst merged 9 commits intoelastic:masterfrom
rjernst:keystore2

rjernst commented Jan 6, 2017

Uh oh!

rjernst commented Jan 6, 2017

Uh oh!

s1monw left a comment

Uh oh!

s1monw Jan 9, 2017

Uh oh!

rjernst Jan 9, 2017

Uh oh!

rjernst Jan 9, 2017

Uh oh!

rjernst Jan 9, 2017

Uh oh!

rjernst commented Jan 10, 2017

Uh oh!

rjernst commented Jan 10, 2017

Uh oh!

s1monw left a comment

Uh oh!

rjernst commented Jan 11, 2017

Uh oh!

dadoonet commented Jan 13, 2017

Uh oh!

dadoonet commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rjernst commented Jan 6, 2017

Uh oh!

rjernst commented Jan 6, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

s1monw Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

rjernst Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

rjernst Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

rjernst Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jan 10, 2017

Uh oh!

rjernst commented Jan 10, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jan 11, 2017

Uh oh!

dadoonet commented Jan 13, 2017

With elasticsearch 5.1.1

With elasticsearch 6.0.0

Uh oh!

dadoonet commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants