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

Reload secure settings for plugins #31383

Merged
merged 44 commits into from
Jun 18, 2018
Merged

Conversation

albertzaharovits
Copy link
Contributor

This is the merge PR of the reload-secure-store-action feature branch, progress of which has been tracked in #29135. This is not intended to be reviewed as a whole, small incremental changes have been reviewed independently. The PR is opened to have CI run (several times).

CC @elastic/es-security
Closes #29135

albertzaharovits and others added 30 commits January 31, 2018 09:31
Cache of clients by name which can be cleared when
secure settings get updated.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Allow plugin install even if no azure settings are present atm
@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jun 17, 2018

Both the gradle build https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/11892 and the packaging tests https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+packaging-tests-sample/3495 have failed because this PR introduced stricter behavior of not allowing the installation of the azure repository plugin if its client settings are not already defined. This strictness has been reverted. After all, this new functionality allows one to defer specifying or changing the secure settings (e27f097)

@albertzaharovits
Copy link
Contributor Author

test this please

@albertzaharovits
Copy link
Contributor Author

The previous gradle build failure (https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/11893/console) was unrelated to the changes herein. I've taken care of it, see #30942 (comment)

@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jun 17, 2018

@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jun 17, 2018

@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jun 17, 2018

Diligence paid off.
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/11896/console is a true failure due to changes herein. The reproduction line is:

REPRODUCE WITH: ./gradlew :server:integTest -Dtests.seed=9045C467BBF675E4 -Dtests.class=org.elasticsearch.action.admin.ReloadSecureSettingsIT -Dtests.method="testMissingKeystoreFile" -Dtests.security.manager=true -Dtests.locale=es-PR -Dtests.timezone=America/Yellowknife
12:01:52 FAILURE 0.16s J0 | ReloadSecureSettingsIT.testMissingKeystoreFile <<< FAILURES!
12:01:52    > Throwable #1: java.lang.AssertionError: 
12:01:52    > Expected: <3>
12:01:52    >      but: was <2>
12:01:52    > 	at __randomizedtesting.SeedInfo.seed([9045C467BBF675E4:A532E37150607DE1]:0)
12:01:52    > 	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
12:01:52    > 	at org.elasticsearch.action.admin.ReloadSecureSettingsIT$1.onResponse(ReloadSecureSettingsIT.java:73)
12:01:52    > 	at org.elasticsearch.action.admin.ReloadSecureSettingsIT$1.onResponse(ReloadSecureSettingsIT.java:67)

It reproduces locally 80% of the time. I have pushed 56f741e to mitigate it.
The problem is explained in #31261 (comment) , although I was not right there.

In short, the problem is that the SecureString holding the password, which is used to decrypt the keystore, is cleared before it is broadcasted. This happens because the original nodes request, NodesReloadSecureSettingsRequest (holding the password) is passed by reference to each NodeRequest. The password is cleared for each node request, but if the request is not serialized and deserialized, i.e. it is submitted locally

private void sendLocalRequest(long requestId, final String action, final TransportRequest request, TransportRequestOptions options) {
, then clearing the password on the NodeRequest will clear the password on the original NodeSRequest.

@albertzaharovits
Copy link
Contributor Author

@albertzaharovits albertzaharovits merged commit 3378240 into master Jun 18, 2018
dnhatn added a commit that referenced this pull request Jun 19, 2018
* master:
  Add get stored script and delete stored script to high level REST API - post backport fix
  Add get stored script and delete stored script to high level REST API (#31355)
  Core: Combine Action and GenericAction (#31405)
  Fix reference to XContentBuilder.string() (#31337)
  Avoid sending duplicate remote failed shard requests (#31313)
  Fix defaults in GeoShapeFieldMapper output (#31302)
  RestAPI: Reject forcemerge requests with a body (#30792)
  Packaging: Remove windows bin files from the tar distribution (#30596)
  Docs: Use the default distribution to test docs (#31251)
  [DOCS] Adds testing for security APIs (#31345)
  Clarify that IP range data can be specified in CIDR notation. (#31374)
  Use system context for cluster state update tasks (#31241)
  Percentile/Ranks should return null instead of NaN when empty (#30460)
  REST high-level client: add validate query API (#31077)
  Move language analyzers from server to analysis-common module. (#31300)
  [Test] Fix :example-plugins:rest-handler on Windows
  Expose lucene's RemoveDuplicatesTokenFilter (#31275)
  Reload secure settings for plugins (#31383)
  Remove some cases in FieldTypeLookupTests that are no longer relevant. (#31381)
  Ensure we don't use a remote profile if cluster name matches (#31331)
  [TEST] Double write alias fault (#30942)
  [DOCS] Fix version in SQL JDBC Maven template
  [DOCS] Improve install and setup section for SQL JDBC
  SQL: Fix rest endpoint names in node stats (#31371)
  Support for remote path in reindex api - post backport fix Closes #22913
  [ML] Put ML filter API response should contain the filter (#31362)
  Support for remote path in reindex api (#31290)
  Add byte array pooling to nio http transport (#31349)
  Remove trial status info from start trial doc (#31365)
  [DOCS] Adds links to release notes and highlights
  add is-write-index flag to aliases (#30942)
  Add rollover-creation-date setting to rolled over index (#31144)
  [ML] Hold ML filter items in sorted set (#31338)
  [Tests] Fix edge case in ScriptedMetricAggregatorTests (#31357)
@albertzaharovits albertzaharovits deleted the reload-secure-store-action branch June 21, 2018 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants