[Authz] Allow update settings action for system user #34030

bizybot · 2018-09-25T06:15:04Z

When the cluster.routing.allocation.disk.watermark.flood_stage watermark
is breached, DiskThresholdMonitor marks the indices as read-only. This
failed when x-pack security was present as system user does not have the privilege
for update settings action("indices:admin/settings/update").
This commit adds the required privilege for the system user. Also added missing
debug logs when access is denied to help future debugging.
An assert statement is added to catch any missed privileges required for
system user.

Closes #33119

When the `cluster.routing.allocation.disk.watermark.flood_stage` watermark is breached, DiskThresholdMonitor marks the indices as read only. This failed when x-pack security was present as System user does not have privilege for update settings action("indices:admin/settings/update"). This commit adds the required privilege for System user. Also added missing debug logs when access is denied to help future debugging. An assert statement is added to catch any missed privileges required for system user. Closes elastic#33119

elasticmachine · 2018-09-25T06:15:06Z

Pinging @elastic/es-security

elasticmachine · 2018-09-25T06:19:50Z

Pinging @elastic/es-distributed

tvernum

Seems about right to me, with a few minor comments.

tvernum · 2018-09-26T02:08:02Z

...ore/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java

@@ -23,7 +23,8 @@
            "indices:admin/mapping/put", // needed for recovery and shrink api
            "indices:admin/template/put", // needed for the TemplateUpgradeService
            "indices:admin/template/delete", // needed for the TemplateUpgradeService
-            "indices:admin/seq_no/global_checkpoint_sync*" // needed for global checkpoint syncs
+            "indices:admin/seq_no/global_checkpoint_sync*", // needed for global checkpoint syncs
+            "indices:admin/settings/update" // needed for DiskThreasholdMonitor.markIndicesReadOnly


Typo in comment -> DiskThresholdMonitor

Corrected this, Thank you.

...core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java

tvernum · 2018-09-26T02:10:41Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

@@ -154,7 +154,10 @@ public void authorize(Authentication authentication, String action, TransportReq
                auditTrail.accessGranted(authentication, action, request, new String[] { SystemUser.ROLE_NAME });
                return;
            }
-            throw denial(authentication, action, request, new String[] { SystemUser.ROLE_NAME });
+            auditTrail.accessDenied(authentication, action, request, new String[] { SystemUser.ROLE_NAME });
+            ElasticsearchSecurityException e = denialException(authentication, action);


Why did we need to expand denial(..) into 2 statements?

I had some different code here and then changed it later, I think I missed to correct this afterwards. Thank you.

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

jasontedor

I left a comment. I will be traveling until Thursday. I defer to @danielmitterdorfer and @rjernst in my place so that my absence does not block progress.

jasontedor · 2018-09-26T03:27:39Z

...security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java

        verifyNoMoreInteractions(auditTrail);
    }

+    private static void assertThrowsAssertionErrorWhenAccessDeniedToSystemUser(ThrowingRunnable throwingRunnable,
+            String action, String user) {
+        AssertionError assertionError = expectThrows(AssertionError.class, throwingRunnable);


I do not think we should do this. For one, it’s asserting on behavior that would not happen in production. For another, someday I would like it that we run tests also with assertions disabled (because we have had problems in the past where our assertions were stateful and impacted production behavior). I think we need to find another way.

I don't think we really care whether it's an AssertionError or an ElasticsearchSecurityException (*) so you could just expectThrows(Throwable.class, and check the message.
We really only want to know that it failed for the right reason.

(*) well, I guess you could make an argument that do want to know that it's an assert if assertions are enabled, but checking whether assertions are enabled is hacky, and probably not worth worrying about.

I think Jason's point was we shouldn't be using assertions at all here. Why can't we expectThrows ElasticsearchSecurityException?

Earlier we used to do the same, but for the system user, we wanted to make sure that the privileges which it required are available so the assert statement in AuthorizationService. In the case of ES tests since assertions are enabled, we will come to know if someone missed on adding the required privileges for the system user. This is the reason we now have to verify the assertion error. As Tim mentioned we could add a check for assertion and based on that we could verify either AssertionError or ElasticsearchSecurityException.

If it is acceptable to check for ElasticsearchSecurityException if assertions are disabled, why is it not sufficient to check for ElasticsearchSecurityException always (and remove the assertion)?

I agree that we should not be catching AssertionError here. Instead if we expect to trip this assertion in a unit test, we could add a method that could be overridden for a test like we do in RestBuilderListener.

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/RestBuilderListener.java

Lines 50 to 54 in 80c5d30

// pkg private method that we can override for testing

boolean assertBuilderClosed(XContentBuilder xContentBuilder) {

assert xContentBuilder.generator().isClosed() : "callers should ensure the XContentBuilder is closed themselves";

return true;

}

For reference, this is the PR that introduced this method.

I do not even think the proposal to add an assertion here would have caught the issue that has led to this PR. This is because we run our standalone tests with the flood stage set to one byte (so that we never trip in CI, on our local machines, etc.):

elasticsearch/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy

Line 330 in 190ea9a

esConfig['cluster.routing.allocation.disk.watermark.flood_stage'] = '1b'

So we are adding something that is mildly contentious that would not have helped with the problem that got us here.

I think that we need to go back to the drawing board.

@jasontedor are you saying you do not think we should add an assertion here at all? I see your point about how this would not have caught the flood stage issue; do you have suggestions for a better alternative?

@bizybot I suggest we revert this assertion change from this PR, keep the unit test to validate the system user can perform the update settings action, and we move discussion about how to catch these problems to an issue for discussion.

Thanks, @jaymode, and @jasontedor. I can revert the assertion change from this PR and we can discuss for alternatives.

I have tested scenarios earlier where we would be able to annotate the code with the required privileges so that would ease the testing as we just need to verify whether the user was able to execute the code. But in our case, we would not be able to annotate code as the open source ES code cannot have x-pack annotations or something similar. I will keep thinking about the alternatives but just thought of sharing this one if someone has any ideas around it. Thank you.

@jaymode I agree that we should defer consideration on how to address this to a separate discussion. Let us focus on getting the fix in here for the update settings action.

- Corrected the spelling mistake - Added test case - Added comment for the assert

jaymode · 2018-09-26T19:55:27Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

-            throw denial(authentication, action, request, new String[] { SystemUser.ROLE_NAME });
+            ElasticsearchSecurityException e = denial(authentication, action, originalRequest, new String[] { SystemUser.ROLE_NAME });
+            // If this throws AssertionError during ES tests then you probably
+            // need to update system user's privileges


I would be careful with a comment like this. It may be that something should not run as the system user; there needs to be thought behind why something failed and what the correct thing to do is. A mechanical update system privileges is not what we should do

jaymode · 2018-09-26T20:06:27Z

...security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java

        verifyNoMoreInteractions(auditTrail);
    }

+    private static void assertThrowsAssertionErrorWhenAccessDeniedToSystemUser(ThrowingRunnable throwingRunnable,
+            String action, String user) {
+        AssertionError assertionError = expectThrows(AssertionError.class, throwingRunnable);


I agree that we should not be catching AssertionError here. Instead if we expect to trip this assertion in a unit test, we could add a method that could be overridden for a test like we do in RestBuilderListener.

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/RestBuilderListener.java

Lines 50 to 54 in 80c5d30

// pkg private method that we can override for testing

boolean assertBuilderClosed(XContentBuilder xContentBuilder) {

assert xContentBuilder.generator().isClosed() : "callers should ensure the XContentBuilder is closed themselves";

return true;

}

For reference, this is the PR that introduced this method.

-Removed the assertions and will create a issue for discussion. -Modified tests to verify exception.

jaymode · 2018-10-02T16:50:31Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

@@ -568,9 +568,12 @@ private ElasticsearchSecurityException denialException(Authentication authentica
        }
        // check for run as
        if (authentication.getUser().isRunAs()) {
+            logger.debug("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(),


I don't really see much value in having these log messages when we have an audit log. Can you explain how they help?

I think while debugging it is easier to see the logs than correlating with another file, also audit logging is disabled by default (yes no one would run prod without audit logs disabled but still).
If you still feel this is of no use I can remove it. Thank you.

jaymode

I am fine with the changes. Please make sure @jasontedor is ok now that the assertion changes have been reverted.

jasontedor

LGTM.

When the cluster.routing.allocation.disk.watermark.flood_stage watermark is breached, DiskThresholdMonitor marks the indices as read-only. This failed when x-pack security was present as system user does not have the privilege for update settings action("indices:admin/settings/update"). This commit adds the required privilege for the system user. Also added missing debug logs when access is denied to help future debugging. An assert statement is added to catch any missed privileges required for system user. Closes #33119

* master: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* rename-ccr-stats: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

When the cluster.routing.allocation.disk.watermark.flood_stage watermark is breached, DiskThresholdMonitor marks the indices as read-only. This failed when x-pack security was present as system user does not have the privilege for update settings action("indices:admin/settings/update"). This commit adds the required privilege for the system user. Also added missing debug logs when access is denied to help future debugging. An assert statement is added to catch any missed privileges required for system user. Closes #33119

bizybot added review v7.0.0 :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.5.0 labels Sep 25, 2018

bizybot requested review from tvernum and jaymode September 25, 2018 06:15

tvernum added the :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) label Sep 25, 2018

Yogesh Gaikwad added 2 commits September 25, 2018 17:43

Correct run as debug log statement

f030090

Fix line length

a118ab7

tvernum reviewed Sep 26, 2018

View reviewed changes

jasontedor requested changes Sep 26, 2018

View reviewed changes

Address review comments from Tim

cddd75e

- Corrected the spelling mistake - Added test case - Added comment for the assert

jaymode reviewed Sep 26, 2018

View reviewed changes

Yogesh Gaikwad added 3 commits October 1, 2018 15:34

Address review comments, suggestions from Jay

6549ceb

-Removed the assertions and will create a issue for discussion. -Modified tests to verify exception.

Merge branch 'master' into fix/33119

bf477ea

Remove unwanted change.

27d96c8

jaymode reviewed Oct 2, 2018

View reviewed changes

jaymode approved these changes Oct 3, 2018

View reviewed changes

jasontedor approved these changes Oct 4, 2018

View reviewed changes

bizybot merged commit 81227dc into elastic:master Oct 4, 2018

bizybot deleted the fix/33119 branch October 4, 2018 01:32

colings86 added >enhancement and removed :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) labels Oct 25, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

jasontedor mentioned this pull request Aug 29, 2019

Missing right to write transient cluster settings in custom plugin #46127

Closed

	// pkg private method that we can override for testing
	boolean assertBuilderClosed(XContentBuilder xContentBuilder) {
	assert xContentBuilder.generator().isClosed() : "callers should ensure the XContentBuilder is closed themselves";
	return true;
	}

[Authz] Allow update settings action for system user #34030

[Authz] Allow update settings action for system user #34030

Uh oh!

Conversation

bizybot commented Sep 25, 2018

Uh oh!

elasticmachine commented Sep 25, 2018

Uh oh!

elasticmachine commented Sep 25, 2018

Uh oh!

tvernum 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasontedor 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!

jaymode Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jaymode Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaymode Sep 26, 2018 •

edited

Loading

jaymode Sep 26, 2018 •

edited

Loading