Consistent Secure Settings #40416

albertzaharovits · 2019-03-25T15:41:59Z

Introduces a new ConsistentSecureSettingsValidatorService service that exposes a single public method, namely allSecureSettingsConsistent . The method returns true if the local node's secure settings (inside the keystore) are equal to the master's, and false otherwise. Technically, the local node has to have exactly the same secure settings - setting names should not be missing or in surplus - for all SecureSetting instances that are flagged with the newly introduced Property.Consistent. It is worth highlighting that the allSecureSettingsConsistent is not a consensus view across the cluster, but rather the local node's perspective in relation to the master.

Notes on implementation:

Hashes published in the cluster state by the master, against which the other nodes cross check, are salted and the salt (by definition) is unique. Consequently, for the other nodes to have a chance to validate the hash, they must retain the original values. Caching those values in plaintext across their lifetime is perilous, so instead the value that gets cached is another hash, but this one is unsalted. The unsalted hash is an obfuscation of secret values that still preserves the required bijective function property for the values that need to be checked for equally across the cluster.

Does not yet contain any tests. I would like to get a preliminary approval of the approach before adding the tests and making it ready for review proper.

Limitations:

this is not designed for dynamic secure settings (hashes are fixed and computed at node start-up) but in theory the service can be retrofitted and ConsistentSecureSettingsValidatorService can be injected in the reload handler to acknowledge a dynamic change.
It is not possible to wait for the settings to become consistent because, right now, the node has to be restarted to change a consistent secure setting.

elasticmachine · 2019-03-25T15:42:01Z

Pinging @elastic/es-core-infra

jaymode

The idea behind this seems like it will work. I think what is missing is how a consumer of a secure setting will use this. Currently I only see a global consistent or not consistent; do you really want to fail all features if a single setting is inconsistent?

Regarding the lack of dealing with reloaded secure settings; is that going to be addressed as part of completion of this feature?

...rc/main/java/org/elasticsearch/common/settings/ConsistentSecureSettingsValidatorService.java

server/src/main/java/org/elasticsearch/common/settings/Setting.java

...rc/main/java/org/elasticsearch/common/settings/ConsistentSecureSettingsValidatorService.java

server/src/main/java/org/elasticsearch/node/Node.java

albertzaharovits · 2019-04-03T13:16:20Z

Thanks for reviewing @jaymode ! I will ping again when I have added tests and refine the client interface.

The idea behind this seems like it will work.

This is an important validation for me!

Currently I only see a global consistent or not consistent; do you really want to fail all features if a single setting is inconsistent?

No, Indeed the API should allow for checking consistency for a subset of the settings! I will add this.

Regarding the lack of dealing with reloaded secure settings; is that going to be addressed as part of completion of this feature?

When I started working on it I was NOT considering this. This is because right now we consider all secure settings final, unless otherwise pointed out in the docs, and until the need arose I was reluctant to do any work. I will re-evaluate this as it progresses, with this approach it does not sound complicated to add it.

...rc/main/java/org/elasticsearch/common/settings/ConsistentSecureSettingsValidatorService.java

server/src/main/java/org/elasticsearch/common/settings/Setting.java

...rc/main/java/org/elasticsearch/common/settings/ConsistentSecureSettingsValidatorService.java

did not mean to click approve yet!

albertzaharovits · 2019-06-18T10:02:29Z

@rjernst @jaymode may I ask for another pass over this one? Apologies for the the long time interval and the incurred cache thrashing!

rjernst

This looks pretty good. I have just a few more comments, mostly minor.

...tools/keystore-cli/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java

server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java

rjernst · 2019-06-20T20:51:51Z

server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java

+     */
+    public byte[] getSecretDigest(Settings settings) {
+        final SecureSettings secureSettings = settings.getSecureSettings();
+        if (secureSettings == null || false == secureSettings.getSettingNames().contains(getKey())) {


Why would we be looking up a digest for a setting that doesn't exist?

rjernst · 2019-06-20T20:55:51Z

server/src/main/java/org/elasticsearch/common/settings/SettingsModule.java

@@ -175,13 +175,29 @@ private void registerSetting(Setting<?> setting) {
                if (existingSetting != null) {
                    throw new IllegalArgumentException("Cannot register setting [" + setting.getKey() + "] twice");
                }
+                if (setting.isConsistent()) {
+                    if (setting instanceof Setting.AffixSetting<?>) {


If we are only going to support this setting property for secure settings, we should clarify that in the javadocs. But why couldn't this work for normal settings as well?

I had not thought about consistent ordinary node settings at all. My reasoning is that we have a better way of actually enforcing consistency with cluster settings for all the non-secure node settings.
If we think we need a form of detection of inconsistency of other node settings, I would think of making use of the cluster scoped settings infra, to publish the actual values, rather than hashes as is the case here. In that case we might automatically achieve consistency, without admin intervention.
Can we leave this for another PR? I have updated the javadocs, there were just a few places that alluded to this aspect.

server/src/test/java/org/elasticsearch/common/settings/ConsistentSettingsIT.java

rjernst · 2019-06-20T20:59:38Z

server/src/test/java/org/elasticsearch/common/settings/ConsistentSettingsIT.java

+    public void testAllConsistentSuccess() throws Exception {
+        internalCluster().getInstances(Environment.class).forEach(environment -> {
+            ClusterService clusterService = internalCluster().getInstance(ClusterService.class);
+            assertTrue(new ConsistentSettingsService(environment.settings(), clusterService, Collections.emptyList()).areAllConsistent());


These all seem like unit tests. Why couldn't they be in the Tests class instead of this IT?

No, they're legit integration tests IMO.
They couple the hashes publication with the cluster formation, also starting new nodes. They are end to end tests, where we publish from one node and verify on another.
I am a bit proud of them. The unit tests are in ConsistentSettingsServiceTests where we intercept the cluster state and do checks on that.

It doesn't seem like you are actually using the nodes really, except to do the creation of ctor args. I would expect an integ test to check the node as it is, but all these assertions construct a service that should be running inside the node. They look like unit tests where you want to tweak the state in eg clusterService through mocks, which is much lighter weight than spinning up even in memory nodes.

They do not tweak the cluster state. They look odd because there is no API for this functionality yet, so I have to construct the ConsistentSettingsService from the node's ClusterService as an API would need to do it. The ConsistentSettingsService ctr args are important to be on different nodes (the tests iterates over all of them).

Not sure what you mean about different nodes. The ctor args are always going to come from the local node. I don't think there is any reason all of the state that would be provided by different nodes couldn't be mocked. This would make the tests much faster, and easier to debug (no inter node communication).

We discussed on another channel and agreed that integration tests should be much thinner.

@rjernst I have pared down ConsistentSettingsIT; it contains a test for the success validation and another test for the failed validation, when a new node joins with different settings. The failed validation scenarios (of which there are two) are randomized, because all cases should be covered in ConsistentSettingsServiceTests.

rjernst · 2019-06-20T21:00:57Z

server/src/test/java/org/elasticsearch/common/settings/ConsistentSettingsIT.java

+            assertTrue(new ConsistentSettingsService(environment.settings(), clusterService, Collections.emptyList()).areAllConsistent());
+            assertTrue(new ConsistentSettingsService(environment.settings(), clusterService,
+                    Collections.singletonList(DUMMY_STRING_CONSISTENT_SETTING)).areAllConsistent());
+            assertTrue(new ConsistentSettingsService(environment.settings(), clusterService,


All of these assert true/false are very opaque. Could we have a helper assertion method that checks the consistency, and dumps the service state if it fails?

If by opaque you mean hard to troubleshoot when they go awry, then there should be WARN logs on the node that failed the verification, that show the setting name and the hash value. I think those are enough? Do you see a need to publish the hashes in the junit output too? I think these are pretty useless anyway...

Since you have been working on this, I'm sure you know exactly where to look in the logs. But some random dev that sees one of these tests fail has no idea when a true/false assertion fails with zero context in the test failure.

I have added messages to assertions which also prints the hashes map.

server/src/test/java/org/elasticsearch/common/settings/SettingsModuleTests.java

albertzaharovits · 2019-06-23T09:17:31Z

Hi Ryan, can you please take another look. I have simplified the integration tests.

rjernst

Looks ok for now, as long as we follow up with figuring out a consistent approach to whether regular settings can be consistent (maybe they shouldn't be allowed, but then we need to make sure documentation and appropriate errors occur).

rjernst · 2019-06-27T17:44:27Z

server/src/main/java/org/elasticsearch/common/settings/ConsistentSettingsService.java

+            } else if (setting instanceof SecureSetting<?>) {
+                secureSettingConsumer.accept((SecureSetting<?>) setting);
+            } else {
+                assert false : "Unrecognized consistent secure setting [" + setting.getKey() + "]";


Asserts aren't always enabled in production systems. This should probably be a normal exception, maybe an IllegalStateException? Same for above on asserting consistency? Doesn't have to be a normal exception for all assert cases, just something to think about whether it is a reachable path, which would be a silent error if asserts are disabled.

Introduces a new `ConsistentSecureSettingsValidatorService` service that exposes a single public method, namely `allSecureSettingsConsistent`. The method returns `true` if the local node's secure settings (inside the keystore) are equal to the master's, and `false` otherwise. Technically, the local node has to have exactly the same secure settings - setting names should not be missing or in surplus - for all `SecureSetting` instances that are flagged with the newly introduced `Property.Consistent`. It is worth highlighting that the `allSecureSettingsConsistent` is not a consensus view across the cluster, but rather the local node's perspective in relation to the master.

Nothing to see here.

albertzaharovits added 10 commits March 20, 2019 15:39

SettingsModule

3d934fe

SettingsModule looking sharp ~

8709b46

Metadata looking sharp

753fe03

SettingsModule compute public hashes

05c3249

Get setting hashes to ClusterSettings

f5d7ea3

WIP

7594ab2

ConsistentSecureSettingsValidator

c4cc9da

ConsistentSecureSettingsValidatorService

4547f9b

Base64 hash

beed446

Index and Node scoped settings...

933041b

albertzaharovits added >feature WIP :Core/Infra/Settings Settings infrastructure and APIs labels Mar 25, 2019

albertzaharovits self-assigned this Mar 25, 2019

albertzaharovits requested a review from jasontedor March 25, 2019 15:42

albertzaharovits added 2 commits March 25, 2019 18:10

Merge branch 'master' into consistent_settings

6caa042

Checkstyle

77711ed

jaymode self-requested a review March 25, 2019 18:17

jaymode requested changes Apr 1, 2019

View reviewed changes

jaymode reviewed Apr 3, 2019

View reviewed changes

...rc/main/java/org/elasticsearch/common/settings/ConsistentSecureSettingsValidatorService.java Outdated Show resolved Hide resolved

...rc/main/java/org/elasticsearch/common/settings/ConsistentSecureSettingsValidatorService.java Outdated Show resolved Hide resolved

Merge branch 'master' into consistent_settings

cfa87c5

albertzaharovits mentioned this pull request Apr 16, 2019

Backup .security #34454

Closed

3 tasks

rjernst previously approved these changes Apr 16, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/common/settings/Setting.java Show resolved Hide resolved

...rc/main/java/org/elasticsearch/common/settings/ConsistentSecureSettingsValidatorService.java Outdated Show resolved Hide resolved

albertzaharovits added 4 commits April 18, 2019 16:06

Merge branch 'master' into consistent_settings

2c7909f

WIP

0d2d634

WIP

ee6d259

byte[] getSHA256Digest(String setting) throws GeneralSecurityException

a118c8d

Merge branch 'master' into consistent_settings

4040829

albertzaharovits added the v8.0.0 label Jun 18, 2019

Change bwc strategy

d257388

rjernst requested changes Jun 20, 2019

View reviewed changes

albertzaharovits added 4 commits June 21, 2019 15:17

Merge branch 'master' into consistent_settings

f6181db

Review

208f27d

truncate ConsistentSettingsIT

f3dd7f0

KeyStoreWrapperTests

d38f7a5

albertzaharovits requested a review from rjernst June 23, 2019 09:17

afix -> affix fallout

7cee301

albertzaharovits mentioned this pull request Jun 25, 2019

Client Side Encrypted Snapshot Repositories #41910

Open

24 tasks

rjernst approved these changes Jun 27, 2019

View reviewed changes

albertzaharovits added 2 commits June 27, 2019 21:17

Merge branch 'master' into consistent_settings

e0600bb

Merge branch 'master' into consistent_settings

a8f60eb

albertzaharovits merged commit cdfc986 into elastic:master Jun 29, 2019

albertzaharovits deleted the consistent_settings branch June 29, 2019 12:17

albertzaharovits added the backport pending label Jun 29, 2019

albertzaharovits mentioned this pull request Jun 29, 2019

[BACKPORT] Consistent Secure Settings (#40416) #43786

Merged

albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 30, 2019

Reenable bwc tests after elastic#40416

d42a967

albertzaharovits mentioned this pull request Jun 30, 2019

Reenable bwc tests after #40416 #43794

Merged

albertzaharovits added a commit that referenced this pull request Jun 30, 2019

Reenable bwc tests after #40416 (#43794)

72d0fd4

Nothing to see here.

albertzaharovits removed the backport pending label Jul 1, 2019

jakelandis mentioned this pull request Sep 3, 2019

[ingest] Re-introduce the Hash Processor with consistent keys across all nodes #34085

Closed

gwbrown mentioned this pull request Dec 4, 2020

Create new secure setting type that is stored (encrypted) in cluster state #32727

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent Secure Settings #40416

Consistent Secure Settings #40416

albertzaharovits commented Mar 25, 2019

elasticmachine commented Mar 25, 2019

jaymode left a comment

albertzaharovits commented Apr 3, 2019

albertzaharovits commented Jun 18, 2019

rjernst left a comment

rjernst Jun 20, 2019

rjernst Jun 20, 2019

albertzaharovits Jun 21, 2019

rjernst Jun 20, 2019

albertzaharovits Jun 21, 2019

rjernst Jun 21, 2019

albertzaharovits Jun 21, 2019

rjernst Jun 21, 2019

albertzaharovits Jun 23, 2019

rjernst Jun 20, 2019

albertzaharovits Jun 21, 2019

rjernst Jun 21, 2019

albertzaharovits Jun 23, 2019

albertzaharovits commented Jun 23, 2019

rjernst left a comment

rjernst Jun 27, 2019

Consistent Secure Settings #40416

Consistent Secure Settings #40416

Conversation

albertzaharovits commented Mar 25, 2019

elasticmachine commented Mar 25, 2019

jaymode left a comment

Choose a reason for hiding this comment

albertzaharovits commented Apr 3, 2019

albertzaharovits commented Jun 18, 2019

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits commented Jun 23, 2019

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment