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

Add validation of keystore setting names #27626

Merged
merged 1 commit into from Dec 5, 2017

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Dec 1, 2017

This commit restricts settings added to the keystore to have a lowercase
ascii name. The java Keystore javadocs state that case sensitivity of
key alias names are implementation dependent. This ensures regardless of
case sensitivity in a jvm implementation, the keys will be stored as we
expect.

This commit restricts settings added to the keystore to have a lowercase
ascii name. The java Keystore javadocs state that case sensitivity of
key alias names are implementation dependent. This ensures regardless of
case sensitivity in a jvm implementation, the keys will be stored as we
expect.
@rjernst rjernst added the :Core/Infra/Settings Settings infrastructure and APIs label Dec 1, 2017
@rjernst
Copy link
Member Author

rjernst commented Dec 1, 2017

Note that this problem was originally found in https://discuss.elastic.co/t/keystore-is-tampered-with-or-corrupted-now-what. Without this change, trying to add a setting name with an uppercase letter results in a corrupt keystore (the wrapper metadata does not match with the keys in the actual keystore). While we don't have any secure setting names using uppercase letters, this ensures both the keystore will not be corrupted trying to store them, and anyone creating a SecureSetting will not be fooled into thinking they can exist.

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left one suggestion, otherwise LGTM.

@@ -46,6 +46,7 @@
private SecureSetting(String key, Property... properties) {
super(key, (String)null, null, ArrayUtils.concat(properties, FIXED_PROPERTIES, Property.class));
Copy link
Member

Choose a reason for hiding this comment

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

How about super(KeyStoreWrapper.validateSettingName(key), ...) and make validateSettingName return key if it passes validation?

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 don't think that buys anything here, since the object will not be fully constructed until the ctor exits anyways. And it makes the semantics of the validation method a little weird (eg could it mean the return value must be used?). I'm going to keep as is but happy to discuss and modify later if you insist.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I am fine with that.

@rjernst rjernst merged commit 8139e3a into elastic:master Dec 5, 2017
@rjernst rjernst deleted the keystore_valid_name branch December 5, 2017 22:30
rjernst added a commit that referenced this pull request Dec 5, 2017
This commit restricts settings added to the keystore to have a lowercase
ascii name. The java Keystore javadocs state that case sensitivity of
key alias names are implementation dependent. This ensures regardless of
case sensitivity in a jvm implementation, the keys will be stored as we
expect.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 6, 2017
* master:
  Add a new cluster setting to limit the total number of buckets returned by a request (elastic#27581)
  Allow index settings to be reset by wildcards (elastic#27671)
  Fix UpdateMappingIntegrationIT test failures
  Correct docs for binary fields and their default for doc values (elastic#27680)
  Clarify IntelliJ IDEA Jar Hell fix (elastic#27635)
  Add validation of keystore setting names (elastic#27626)
  Prevent constructing index template without patterns (elastic#27662)
  [DOCS] Fixed typos and broken attribute.
  Add support for filtering mappings fields (elastic#27603)
  [DOCS] Added link to upgrade guide and bumped the upgrade topic up to the top level (elastic#27621)
  [Geo] Add Well Known Text (WKT) Parsing Support to ShapeBuilders
  Fix up tests now that GeoDistance.*.calculate works (elastic#27541)
  [Docs] Fix parameter name (elastic#27656)
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.

None yet

3 participants