Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

S3 Repository: Add back repository level credentials #24609

Merged
merged 4 commits into from May 11, 2017

Conversation

Projects
None yet
5 participants
@rjernst
Copy link
Member

commented May 11, 2017

Specifying s3 access and secret keys inside repository settings are not
secure. However, until there is a way to dynamically update secure
settings, this is the only way to dynamically add repositories with
credentials that are not known at node startup time. This commit adds
back access_key and secret_key s3 repository settings, but protects
it with a required system property allow_insecure_settings.

S3 Repository: Add back repository level credentials
Specifying s3 access and secret keys inside repository settings are not
secure. However, until there is a way to dynamically update secure
settings, this is the only way to dynamically add repositories with
credentials that are not known at node startup time. This commit adds
back `access_key` and `secret_key` s3 repository settings, but protects
it with a required system property `allow_insecure_settings`.
@dadoonet
Copy link
Member

left a comment

Left minor comments.
I think we may be need to warn when we start the node if allow_insecure_settings is set, no?

@@ -24,6 +24,8 @@
import java.util.EnumSet;
import java.util.Set;

import org.elasticsearch.cluster.routing.IllegalShardRoutingStateException;

This comment has been minimized.

Copy link
@dadoonet

dadoonet May 11, 2017

Member

Not needed?

This comment has been minimized.

Copy link
@rjernst

rjernst May 11, 2017

Author Member

Indeed, removed in ad4c625

* A setting which contains a sensitive string, but which for legacy reasons must be found outside secure settings.
* @see #secureString(String, Setting, Property...)
*/
public static Setting<SecureString> inecureString(String name) {

This comment has been minimized.

Copy link
@dadoonet

dadoonet May 11, 2017

Member

Wrong name for the method?

This comment has been minimized.

Copy link
@rjernst

rjernst May 11, 2017

Author Member

Ooops, thanks! Fixed in ad4c625

@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

I think we may be need to warn when we start the node if allow_insecure_settings is set

For what purpose? Every insecure setting is automatically marked as deprecated, so simply using the setting will cause a deprecation warning.

@dadoonet

This comment has been minimized.

Copy link
Member

commented May 11, 2017

Every insecure setting is automatically marked as deprecated, so simply using the setting will cause a deprecation warning.

Ha that's right. But as it's up to the user to use that insecure setting when he registers a new repository, the admin guy who actually starts elasticsearch server might not be aware at startup time that someone else might use an insecure setting at some point.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

@rjernst what happens if somebody has a insecure setting but then goes and sets the property to false will the node start? And if the node has started and recovers a cluster state with such a setting will it fail?

@rjernst

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

For both questions, constructing the S3Repository would fail. However, since the s3 repositories are not created within Node construction, I don't think the node will fail, just that repository would be unavailable.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

@rjernst LGTM then

@jasontedor
Copy link
Member

left a comment

I didn't do a full review, I just left one comment about the name of the system property. We have somewhat of a convention that we prefix these with es. that I think we should follow here.

/**
* A secure setting.
*
* This class allows access to settings from the Elasticsearch keystore.
*/
public abstract class SecureSetting<T> extends Setting<T> {

/** Determines whether legacy settings with sensitive values should be allowed. */
private static final boolean ALLOW_INSECURE_SETTINGS = Booleans.parseBoolean(System.getProperty("allow_insecure_settings", "false"));

This comment has been minimized.

Copy link
@jasontedor

jasontedor May 11, 2017

Member

I think this should be pre-fixed with es. like we try to do with other custom system properties that we use. I'm not sure if we're 100% consistent, but we should aim to be when adding new ones.

rjernst added some commits May 11, 2017

Ok, renamed the property as you suggested.

@rjernst rjernst merged commit 17d0155 into elastic:master May 11, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@rjernst rjernst deleted the rjernst:s3_repo_creds branch May 11, 2017

@clintongormley clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.