Skip to content

Conversation

@rjernst
Copy link
Member

@rjernst rjernst commented Jan 6, 2017

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 rjernst added :Core/Infra/Settings Settings infrastructure and APIs v5.3.0 v6.0.0-alpha1 labels Jan 6, 2017
@rjernst
Copy link
Member Author

rjernst commented Jan 6, 2017

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.

@clintongormley clintongormley changed the title Settings: Make s3 repository sensitive settings use secure settings Make s3 repository sensitive settings use secure settings Jan 9, 2017
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, nevermind. Done!

@rjernst
Copy link
Member Author

rjernst commented Jan 10, 2017

@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
Copy link
Member Author

rjernst commented Jan 10, 2017

retest this please

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rjernst
Copy link
Member Author

rjernst commented Jan 11, 2017

retest this please

@rjernst rjernst merged commit 8015fbb into elastic:master Jan 11, 2017
@rjernst rjernst deleted the keystore2 branch January 11, 2017 19:19
rjernst added a commit that referenced this pull request Jan 11, 2017
* 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.
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Jan 11, 2017
… props, and profile files

This is a follow up to elastic#22479, where storing credentials secure way was
added.
@dadoonet
Copy link
Contributor

@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
Copy link
Contributor

Apparently I'm getting back an empty String when running

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

rjernst added a commit that referenced this pull request Jan 19, 2017
… 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.
rjernst added a commit that referenced this pull request Jan 19, 2017
… 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.
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Jan 24, 2017
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
rjernst added a commit that referenced this pull request Jan 27, 2017
* 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
rjernst added a commit that referenced this pull request Jan 27, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants