Phase 2 support for operator privileges: Cluster settings #66684

ywangd · 2020-12-21T10:45:15Z

Phase 2 of operator privileges is to support operator-only settings. This includes:

Prevent operator-only setting from being updated with cluster settings API unless the user is an operator
Skip operator-only setting during restore unless the authenticating user is an operator.

For the restore process, even if the user is not an operator, the restore will not fail, but rather succeeds by skipping any operator-only settings. This means, if the current cluster state has values for an operator-only setting, it will have the same value after restore. When the user is an operator, the restore behaviour is essentially the same as of today.
(EDIT: after discussions, the restore behaviour for operator-only settings will be the same for either operator or non-operator user. When operator privileges are enabled, operator-only settings will not be restored. Otherwise (if the feature is disabled), the behaviour is the same as of today.

Operator and non-operator settings are differentiated with a new DynamicOperator property. The Setting constructor enforces that Dynamic and DynamicOperator cannot be both specified.

I spent quite some time exploring how the restore code should be made aware of operator privileges. The challenge is that they are located in different packages. There are a few options, but at the end I decided to let the SnapshotRestoreRequest carry a flag to indicate whether or not to filter operator-only settings. This flag is set in AuthorizationService where operator privileges are enforced. This is similar to how RequestInterceptor works except it is for a cluster action instead of index actions. I personally think it is a reasonable, lightweight approach that does the job to separate the concerns of security and snapshot/restore. We could choose a more formal and heavier approach by introducing a new Plugin method (or even interface) to allow the main to pull settings filters from plugins. But I think it's an overkill just for this purpose.

The PR does have javaRestTest to test overall features, but otherwise lacks unit tests. I'll be adding them once the overall approach is approved.

…-phase-two-settings

ywangd · 2020-12-23T13:51:17Z

@tvernum As talked during the meeting, for this draft PR, I am seeking feedback mainly around the approach that makes the snapshot code aware of the information provided by operator privileges check. As said in the PR description, it is done by having the security code flag a new field on the SnapshotRestoreRequest object. This is one of the two major code level design decisions we need to make. The other is how we label operator-only and non-operator settings, for which I chose to use a new Setting Property as suggested by @henningandersen .

Of course besides the above main issue, any other comments are always welcome. Thanks!

henningandersen · 2020-12-24T09:55:09Z

When the user is an operator, the restore behaviour is essentially the same as of today.

I am curious about whether this is ever needed? If the operator controls some settings, I would think we never want to restore those. For instance, if the operator restores an env from production to dev every weekend, I would think the production settings may not be valid for the dev environment (like rate-limiter or autoscaling policies)? Are there examples where this is desired behavior?

ywangd · 2020-12-31T00:24:12Z

When the user is an operator, the restore behaviour is essentially the same as of today.

I am curious about whether this is ever needed? If the operator controls some settings, I would think we never want to restore those. For instance, if the operator restores an env from production to dev every weekend, I would think the production settings may not be valid for the dev environment (like rate-limiter or autoscaling policies)? Are there examples where this is desired behavior?

I think this behaviour is useful when moving cluster to different zones or provision (replicate) a cluster from existing ones. Also this behaviour is backward compatible when operator privileges are enabled on Cloud, i.e. there is no new logic (on Cloud side) needed for any downstream/subsequent actions after restore. With this being said, I'll forward this question to Cloud folks to get more definitive answers.

tvernum · 2020-12-31T00:33:24Z

I'd start from the question of what should the behaviour be when operator privileges are not enabled (remembering that it's an Enterprise feature, so many on-prem customers will not have it).

I think it would be surprising (to the point of being incorrect) if restoring a cluster from a snapshot did not restore IP Filters (as an example). For example, you attempt a version upgrade, it goes wrong, so you wipe your cluster and restore from snapshot but your IP Filters aren't restored & your clusters is now open. That seems wrong.

I would argue that the same is true if I restore a cluster as an operator, when operator privileges are enabled, because

The same reasoning applies (though not as strongly because the cluster admin has opted-in to operator privileges, so we can argue that they should be aware of these consequences)
Consistency is better. If operator privileges are about stopping non-operators from doing operator-things, then what's the argument for stopping operators from doing things that are possible on a regular cluster?

…-phase-two-settings

elasticmachine · 2021-02-01T06:03:29Z

Pinging @elastic/es-security (Team:Security)

ywangd · 2021-02-01T06:07:00Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java


+        public void maybeInterceptRequest(ThreadContext threadContext, TransportRequest request) {
+            if (request instanceof RestoreSnapshotRequest) {
+                ((RestoreSnapshotRequest) request).skipOperatorOnly(shouldProcess());


The code always sets the value of skipOperatorOnly because:

Similar to other authorization decision, the value is computed and local to every node

The value is always set properly so that a transport client cannot override this field (it is not exposed to Rest)

ywangd · 2021-02-01T06:07:35Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java

+        @Override
+        public void maybeInterceptRequest(ThreadContext threadContext, TransportRequest request) {
+            if (request instanceof RestoreSnapshotRequest) {
+                ((RestoreSnapshotRequest) request).skipOperatorOnly(false);


Similarly, this field is always set even when operator privileges are not enabled to retain the default behaviour. See also above.

ywangd · 2021-02-01T06:12:38Z

@tvernum @henningandersen This PR is now ready for review.
@droberts195 Could you please confirm that the ML settings that have been labelled as "operator-only" correctly reflect what the ML team has in mind?

Thanks!

droberts195 · 2021-02-01T11:13:10Z

@droberts195 Could you please confirm that the ML settings that have been labelled as "operator-only" correctly reflect what the ML team has in mind?

Yes, the 6 that are changed in this PR are good - thanks!

tvernum

LGTM

...in/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java

tvernum · 2021-02-03T06:39:52Z

...erTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesSingleNodeTests.java

    }

-    public void testOutcomeOfSuperuserPerformingOperatorOnlyActionWillDependOnWhetherFeatureIsEnabled() {
+    public void testOperatorOnlyActionOrSettingWillNotBeActionableByNormalSuperuser() {


What value do we get from combining these into 1 test case? It seems more natural to have 2 separate cases for action and settings.

No real value. I split it and also the corresponding one that test the operator user.

tvernum · 2021-02-03T06:44:26Z

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

+    /**
+     * Returns <code>true</code> if this setting is dynamically updateable by operators, otherwise <code>false</code>
+     */
+    public final boolean isDynamicOperator() {


I wonder whether this should just be isOperatorOnly()
I think it would read better in OperatorOnlyRegistry.checkClusterUpdateSettings (because that code is really focused on "is this an operator setting" rather than "is this dynamic, but only updateable by operators")

It makes senses since operator settings must be dynamic in the first place, though this assumes some understanding of the concept. But overall I agree it reads better in the call sites.

henningandersen

LGTM, thanks @ywangd , I added a few minor things to address.

henningandersen · 2021-02-03T09:02:56Z

...ts/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java

+        }
+    }
+
+    @SuppressWarnings("unchecked")


I did not spot the place where this is needed, perhaps it can be removed or alternatively moved to the specific line?

Good catch! It is indeed a copy/paste artifact ~~

henningandersen · 2021-02-03T09:04:22Z

...ts/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java

+    }
+
+    @SuppressWarnings("unchecked")
+    public void testSnapshotRestoreBehaviourOfOperatorSettings() throws IOException {


I think we may want the opposite test too - checking that without operator privileges enabled, we do restore properties marked as operator?

I thought these would be covered by existing tests already, but I was wrong. The settings are from xpack and most snapshot related tests are in core. I added a new yml test to cover this and PUT _cluster/settings.

henningandersen · 2021-02-03T09:11:46Z

...ts/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java

        assertTrue((boolean) operatorPrivileges.get("enabled"));
    }
+
+    public void testUpdateOperatorSettings() throws IOException {


I think we want a test to validate that we can set operator settings when operator privileges are disabled too. Could also be in the single node test rather than rest test, if easier.

Added a yml test for it. See also above.

henningandersen · 2021-02-03T09:45:57Z

...in/java/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java

    private boolean includeAliases = true;
    private Settings indexSettings = EMPTY_SETTINGS;
    private String[] ignoreIndexSettings = Strings.EMPTY_ARRAY;
+    private boolean skipOperatorOnly = false; // this field does not get serialised because it is always set locally by authz


nit: this is not included in toString, would be nice to add it for debugging purposes.

The toString method relies on toXContent, which I intentionally left out for this new field. To support toString, I extracted the internals of the toXContent method into a new toXContentFragment method and use it for the string building.

henningandersen · 2021-02-03T10:09:04Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

                        if (request.includeGlobalState()) {
                            if (metadata.persistentSettings() != null) {
                                Settings settings = metadata.persistentSettings();
                                clusterSettings.validateUpdate(settings);


Hopefully this never causes an issue, but I think this line should go below the modification of the settings below to ensure the updated settings are valid.

A good catch, thanks! It may not cause real issue like you said, nevertheless it is better to be safe.

henningandersen · 2021-02-03T10:18:46Z

...va/org/elasticsearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java

+        XContentParser parser = XContentType.JSON.xContent().createParser(
+            NamedXContentRegistry.EMPTY, null, BytesReference.bytes(builder).streamInput());
+        Map<String, Object> map = parser.mapOrdered();
+        assertFalse(map.containsKey("skip_operator_only"));


I wonder if it was nicer to verify that toXContent gives same result regardless of the skipOperatorOnly flag? I.e., invoke it on two instances with different skipOperatorOnly flag and verify the maps are identical.

…apshots/restore/RestoreSnapshotRequest.java Co-authored-by: Tim Vernum <[email protected]>

ywangd · 2021-02-04T03:39:00Z

@elasticmachine update branch

ywangd · 2021-02-04T09:30:25Z

@droberts195 Could you please confirm that the ML settings that have been labelled as "operator-only" correctly reflect what the ML team has in mind?

Yes, the 6 that are changed in this PR are good - thanks!

Thanks @droberts195 But somehow I counted 9 of them:

xpack.ml.node_concurrent_job_allocations
xpack.ml.max_machine_memory_percent
xpack.ml.use_auto_machine_memory_percent
xpack.ml.max_lazy_ml_nodes
xpack.ml.process_connect_timeout
xpack.ml.nightly_maintenance_requests_per_second
xpack.ml.max_ml_node_size
xpack.ml.enable_config_migration
xpack.ml.persist_results_max_retries

Could you please advise? Thanks!

droberts195 · 2021-02-04T10:09:52Z

But somehow I counted 9 of them

Yes, sorry, you are right that there are 9. I'm happy with all of those.

) Add a new OperatorDynamic enum to differentiate between operator-only and regular dynamic cluster settings. The Setting constructor validates that Dynamic and OperatorDynamic cannot be both specified. Operator-only settings behave as the follows: * When the feature is enabled, operator-only settings cannot be updated with PUT cluster settings API unless the user is an operator. * The restore behaviour for operator-only settings will be identical for either operator or non-operator user. That is, when operator privileges are enabled, operator-only settings will not be restored. Otherwise (if the feature is disabled), the behaviour is the same as of today.

…68563) Add a new OperatorDynamic enum to differentiate between operator-only and regular dynamic cluster settings. The Setting constructor validates that Dynamic and OperatorDynamic cannot be both specified. Operator-only settings behave as the follows: * When the feature is enabled, operator-only settings cannot be updated with PUT cluster settings API unless the user is an operator. * The restore behaviour for operator-only settings will be identical for either operator or non-operator user. That is, when operator privileges are enabled, operator-only settings will not be restored. Otherwise (if the feature is disabled), the behaviour is the same as of today.

Add documentation for operator privilegs. The docs cover features delivered by phase 1 (#65256) and 2 (#66684). Co-authored-by: Tim Vernum <[email protected]> Co-authored-by: lcawl <[email protected]> Co-authored-by: Adam Locke <[email protected]>

Add documentation for operator privilegs. The docs cover features delivered by phase 1 (elastic#65256) and 2 (elastic#66684). Co-authored-by: Tim Vernum <[email protected]> Co-authored-by: lcawl <[email protected]> Co-authored-by: Adam Locke <[email protected]>

Add documentation for operator privilegs. The docs cover features delivered by phase 1 (#65256) and 2 (#66684). Co-authored-by: Tim Vernum <[email protected]> Co-authored-by: lcawl <[email protected]> Co-authored-by: Adam Locke <[email protected]>

ywangd added 3 commits December 21, 2020 17:24

wip

6b8d317

Skip operator settings in restore when user is not operator

6b104bd

tweak

5f18f66

ywangd added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.12.0 labels Dec 21, 2020

ywangd requested review from henningandersen and tvernum December 21, 2020 10:45

ywangd added 3 commits December 22, 2020 11:02

Merge remote-tracking branch 'origin/master' into operator-privileges…

e0a20b7

…-phase-two-settings

Merge remote-tracking branch 'origin/master' into operator-privileges…

50f63df

…-phase-two-settings

Add ml settings for operator only

2933b68

ywangd added 5 commits January 28, 2021 22:18

Merge remote-tracking branch 'origin/master' into operator-privileges…

f1ffe0c

…-phase-two-settings

Update according to discussion email

0f225e0

tweak

57f518c

Merge remote-tracking branch 'origin/master' into operator-privileges…

924d505

…-phase-two-settings

add tests

8050479

ywangd marked this pull request as ready for review February 1, 2021 06:03

elasticmachine added the Team:Security Meta label for security team label Feb 1, 2021

ywangd commented Feb 1, 2021

View reviewed changes

forbidden api fix

2b9aea9

tvernum approved these changes Feb 3, 2021

View reviewed changes

henningandersen approved these changes Feb 3, 2021

View reviewed changes

Update server/src/main/java/org/elasticsearch/action/admin/cluster/sn…

8c79807

…apshots/restore/RestoreSnapshotRequest.java Co-authored-by: Tim Vernum <[email protected]>

elasticmachine and others added 8 commits February 3, 2021 19:39

Merge branch 'master' into operator-privileges-phase-two-settings

2b9b6aa

Address feedback

0a6ca70

fix names

45b6e4d

more names

d51630f

address more feedback

cb8e8a2

more feedback

6d5ad52

fix test

c9406af

add teardown

67840a9

ywangd merged commit 8e49ba9 into elastic:master Feb 4, 2021

ywangd mentioned this pull request Feb 15, 2021

Documentation for operator privileges #68964

Merged

stevejgordon mentioned this pull request Feb 22, 2021

7.12.0 Meta Ticket elastic/elasticsearch-net#5337

Closed

34 tasks

ywangd mentioned this pull request Feb 24, 2021

Prevent misuse of IP filtering #51178

Closed

This was referenced Mar 10, 2021

Documentation for operator privileges (#68964) #70193

Merged

Documentation for operator privileges (#68964) #70194

Merged

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

Phase 2 support for operator privileges: Cluster settings #66684

Phase 2 support for operator privileges: Cluster settings #66684

Uh oh!

Conversation

ywangd commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywangd commented Dec 23, 2020

Uh oh!

henningandersen commented Dec 24, 2020

Uh oh!

ywangd commented Dec 31, 2020

Uh oh!

tvernum commented Dec 31, 2020

Uh oh!

elasticmachine commented Feb 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd commented Feb 1, 2021

Uh oh!

droberts195 commented Feb 1, 2021

Uh oh!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

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!

henningandersen left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

ywangd commented Feb 4, 2021

Uh oh!

ywangd commented Feb 4, 2021

Uh oh!

droberts195 commented Feb 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ywangd commented Dec 21, 2020 •

edited

Loading