Add ability to prompt for selected settings on startup #10918

jaymode · 2015-05-01T11:03:19Z

Some settings may be considered sensitive, such as passwords, and storing them
in the configuration file on disk is not good from a security perspective. This change
allows settings to have a special value, __prompt__, that indicates elasticsearch
should prompt the user for the actual value on startup. This only works when
started in the foreground. In cases where elasticsearch is started as a service or
in the background, an exception will be thrown.

Closes #10838

s1monw · 2015-05-04T08:23:40Z

src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java

I don't think this class should have a method with this kind of sideeffects. I thinks we should put all this stuff into
PromptPlaceholderResolver and have a method there with this signature:

public static Settings promtTerminal(Terminal terminal, Settings settings) { // this method never modifies a builder we take the settings and build a new one and return return newSettings; }

this way we don't need all the static maps in that class either and we don't need to access inner structures. In prepareSettings we call this as the last thing before we return the settings as the tuple.

does thsi make sense?

Yeah it does make sense. The static map was more for issues that occurred when creating a Node within Bootstrap and having system properties with __prompt__. We can workaround that by not re-reading the system properties when building the node.

s1monw · 2015-05-04T08:26:12Z

I left one comment. I also wonder if we should add a SecureString that fully owns it's character array that we can potentially wipe after the fact? maybe as a different PR?

uboness · 2015-05-04T08:31:36Z

How would you know when to wipe the array?

s1monw · 2015-05-04T08:33:02Z

so there are multiple ways to do this. IMO there should only be ONE consumer. once the string is consumed we can wipe it. The other option is to have this thing MD5 or SHAxyz the input and never keep it around.

uboness · 2015-05-04T08:38:51Z

I like the one consumer approach... It's then the responsibility of the consumer to properly handle it and as far as the settings is concerned it's been handled

s1monw · 2015-05-04T09:40:17Z

@uboness wanna open a new issue or @jaymode maybe?

jaymode · 2015-05-04T10:22:30Z

I will open the issue to add the SecureString

jaymode · 2015-05-04T10:43:05Z

@s1monw updated the PR and opened #10950

uboness · 2015-05-04T11:37:34Z

src/main/java/org/elasticsearch/common/property/PromptPlaceholderResolver.java

is that going to only be used for secrets? So it seems based on the code right now... in that case, I'd call it __promt_secret__ or something along those lines.

If the idea is to just keep it a generic prompt, then we'll need some sort of type indication here, like __prompt:secret__ vs. __prompt:text__ or __prompt:int__

I think we should keep it generic. I think adding __prompt:secret__ and __prompt:text__ is the right thing to do. However, I don't see the need for other types since we store settings internally as a string or maybe I am missing something here?

+1 for the two

s1monw · 2015-05-05T09:52:37Z

I left one cosmetic comment - I will leave the final LGTM to @uboness here once you guys sorted out the different prompt options.

jaymode · 2015-05-05T11:05:28Z

@uboness updated the PR to support prompting for text and secret values

s1monw · 2015-05-13T20:29:58Z

@jaymode what's the status of this?

jaymode · 2015-05-13T20:32:42Z

@uboness ping.

@s1monw still waiting on last LGTM

kimchy · 2015-05-13T21:25:55Z

src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

can we document why we do it (and make this a constant since we use it in several places now)?

kimchy · 2015-05-13T21:26:09Z

looks great!, left one small comment

jaymode · 2015-05-14T11:42:01Z

added the constant and rebased to pickup other changes in bootstrap

s1monw · 2015-05-28T07:45:44Z

@jaymode ping

uboness · 2015-05-28T11:25:48Z

src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java

so, I've been thinking about it. We already have property place holders in the form ${...}. I think it'd be more consistent if we use the same form except with special syntax inside... something like: ${prompt::text}. @clintongormley wdyt?

I'm good with that, and a strict YAML parser doesn't barf with this syntax, which is a bonus :)

sorry :).. good with what? the original (i.e. __prompt:secret__) or the dollar sign option (i.e. ${prompt::text})?

the dollar sign option :)

s1monw · 2015-05-29T09:17:05Z

@jaymode I think we are good to go here now?

jaymode · 2015-05-29T12:58:58Z

@s1monw or @uboness can one you of do a once over on the commit I just pushed? I renamed the placeholder and that led to some special casing being needed so I want to make sure that part is reviewed. After that, I'll get this rebased and push it.

uboness · 2015-05-29T13:21:23Z

I can live with pushing this for 1.6, just to have the functionality. But I would love to see this cleaned up and basically have all the placeholder replacement consolidated in the `replacePropertyPlaceholders' method.

would love to see the system placeholders follow the same convention and be in the form: ${env::foobar}

We can also introduce a new setting: config.ignore_placeholders:

will ignore all placeholder settings:

config.ignore_placeholders: true

will only ignore env placeholders:

config.ignore_placeholders: env

will only ignore prompt placeholders:

config.ignore_placeholders: prompt

will ignore both the env and prompt settings:

config.ignore_placeholders: env, prompt

It's a breaking change, so something to consider for 2.0?

jaymode · 2015-06-02T13:29:40Z

opened #11455 as discussion for these settings cleanups

Some settings may be considered sensitive, such as passwords, and storing them in the configuration file on disk is not good from a security perspective. This change allows settings to have a special value, `${prompt::text}` or `${prompt::secret}`, to indicate that elasticsearch should prompt the user for the actual value on startup. This only works when started in the foreground. In cases where elasticsearch is started as a service or in the background, an exception will be thrown. Closes elastic#10838

In elastic#10918, we introduced the prompt placeholders. These were had a different format than our existing placeholders. This changes the prompt placeholders to follow the format of the existing placeholders. Relates to elastic#11455

In #10918, we introduced the prompt placeholders. These were had a different format than our existing placeholders. This changes the prompt placeholders to follow the format of the existing placeholders. Relates to #11455

jaymode added v2.0.0-beta1 review :Core/Infra/Settings Settings infrastructure and APIs v1.6.0 labels May 1, 2015

s1monw reviewed May 4, 2015
View reviewed changes

s1monw self-assigned this May 4, 2015

jaymode mentioned this pull request May 4, 2015

Add SecureString for sensitive settings #10950

Closed

uboness reviewed May 4, 2015
View reviewed changes

kimchy reviewed May 13, 2015
View reviewed changes

jaymode force-pushed the password_prompt branch from 2743109 to ba6c3d1 Compare May 14, 2015 11:34

uboness reviewed May 28, 2015
View reviewed changes

clintongormley added the >feature label May 29, 2015

clintongormley changed the title ~~Settings: add ability to prompt for selected settings on startup~~ Add ability to prompt for selected settings on startup May 29, 2015

jaymode mentioned this pull request Jun 2, 2015

cleanup and standardize settings placeholders #11455

Closed

jaymode force-pushed the password_prompt branch from aed2c50 to f6191d0 Compare June 2, 2015 14:28

jaymode merged commit f6191d0 into elastic:master Jun 2, 2015

kevinkluge removed the review label Jun 2, 2015

jaymode mentioned this pull request Jun 5, 2015

Make prompt placeholders consistent with existing placeholders #11514

Merged

dadoonet mentioned this pull request Jul 24, 2015

Azure plugin certificate too dangerous ? elastic/elasticsearch-cloud-azure#68

Closed

Add ability to prompt for selected settings on startup #10918

Add ability to prompt for selected settings on startup #10918

Uh oh!

Conversation

jaymode commented May 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw commented May 4, 2015

Uh oh!

uboness commented May 4, 2015

Uh oh!

s1monw commented May 4, 2015

Uh oh!

uboness commented May 4, 2015

Uh oh!

s1monw commented May 4, 2015

Uh oh!

jaymode commented May 4, 2015

Uh oh!

jaymode commented May 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw commented May 5, 2015

Uh oh!

jaymode commented May 5, 2015

Uh oh!

s1monw commented May 13, 2015

Uh oh!

jaymode commented May 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimchy commented May 13, 2015

Uh oh!

jaymode commented May 14, 2015

Uh oh!

s1monw commented May 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1monw commented May 29, 2015

Uh oh!

jaymode commented May 29, 2015

Uh oh!

uboness commented May 29, 2015

Uh oh!

jaymode commented Jun 2, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants