Skip to content

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

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.

Sign up for GitHub

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

Merged
bizybot merged 7 commits into from
Oct 4, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5c31415
[Authz] Allow update settings action for system user
Sep 24, 2018
f030090
Correct run as debug log statement
Sep 25, 2018
a118ab7
Fix line length
Sep 25, 2018
cddd75e
Address review comments from Tim
Sep 26, 2018
6549ceb
Address review comments, suggestions from Jay
Sep 27, 2018
bf477ea
Merge branch 'master' into fix/33119
Oct 1, 2018
27d96c8
Remove unwanted change.
Oct 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .../src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ public final class SystemPrivilege extends Privilege {
"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
Copy link
Contributor

@tvernum tvernum Sep 26, 2018

Choose a reason for hiding this comment

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

Typo in comment -> DiskThresholdMonitor

Copy link
Contributor Author

@bizybot bizybot Sep 26, 2018

Choose a reason for hiding this comment

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

Corrected this, Thank you.

), Automatons.patterns("internal:transport/proxy/*"))); // no proxy actions for system user!

private SystemPrivilege() {
Expand Down
1 change: 1 addition & 0 deletions ...e/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ public void testSystem() throws Exception {
assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync"), is(true));
assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync[p]"), is(true));
assertThat(predicate.test("indices:admin/seq_no/global_checkpoint_sync[r]"), is(true));
assertThat(predicate.test("indices:admin/settings/update"), is(true));
}

public void testManageCcrPrivilege() {
Expand Down
8 changes: 7 additions & 1 deletion ...n/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@tvernum tvernum Sep 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@bizybot bizybot Sep 26, 2018

Choose a reason for hiding this comment

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

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

assert false : e.getMessage();
throw e;
}

// get the roles of the authenticated user, which may be different than the effective
Expand Down Expand Up @@ -568,9 +571,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(),
Copy link
Member

@jaymode jaymode Oct 2, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@bizybot bizybot Oct 3, 2018

Choose a reason for hiding this comment

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

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.

authentication.getUser().principal());
return authorizationError("action [{}] is unauthorized for user [{}] run as [{}]", action, authUser.principal(),
authentication.getUser().principal());
}
logger.debug("action [{}] is unauthorized for user [{}]", action, authUser.principal());
return authorizationError("action [{}] is unauthorized for user [{}]", action, authUser.principal());
}

Expand Down
45 changes: 25 additions & 20 deletions ...urity/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,53 +240,58 @@ private void authorize(Authentication authentication, String action, TransportRe
future.actionGet();
}

public void testActionsSystemUserIsAuthorized() {
TransportRequest request = mock(TransportRequest.class);
public void testActionsForSystemUserIsAuthorized() {
final TransportRequest request = mock(TransportRequest.class);

// A failure would throw an exception
Authentication authentication = createAuthentication(SystemUser.INSTANCE);
authorize(authentication, "indices:monitor/whatever", request);
verify(auditTrail).accessGranted(authentication, "indices:monitor/whatever", request,
new String[]{SystemUser.ROLE_NAME});

authentication = createAuthentication(SystemUser.INSTANCE);
authorize(authentication, "internal:whatever", request);
verify(auditTrail).accessGranted(authentication, "internal:whatever", request, new String[]{SystemUser.ROLE_NAME});
final Authentication authentication = createAuthentication(SystemUser.INSTANCE);
final String[] actions = { "indices:monitor/whatever", "internal:whatever", "cluster:monitor/whatever", "cluster:admin/reroute",
"indices:admin/mapping/put", "indices:admin/template/put", "indices:admin/seq_no/global_checkpoint_sync",
"indices:admin/settings/update" };
for (String action : actions) {
authorize(authentication, action, request);
verify(auditTrail).accessGranted(authentication, action, request, new String[] { SystemUser.ROLE_NAME });
}

verifyNoMoreInteractions(auditTrail);
}

public void testIndicesActionsAreNotAuthorized() {
public void testIndicesActionsForSystemUserWhichAreNotAuthorized() {
final TransportRequest request = mock(TransportRequest.class);
final Authentication authentication = createAuthentication(SystemUser.INSTANCE);
assertThrowsAuthorizationException(
assertThrowsAssertionErrorWhenAccessDeniedToSystemUser(
() -> authorize(authentication, "indices:", request),
"indices:", SystemUser.INSTANCE.principal());
verify(auditTrail).accessDenied(authentication, "indices:", request, new String[]{SystemUser.ROLE_NAME});
verifyNoMoreInteractions(auditTrail);
}

public void testClusterAdminActionsAreNotAuthorized() {
public void testClusterAdminActionsForSystemUserWhichAreNotAuthorized() {
final TransportRequest request = mock(TransportRequest.class);
final Authentication authentication = createAuthentication(SystemUser.INSTANCE);
assertThrowsAuthorizationException(
assertThrowsAssertionErrorWhenAccessDeniedToSystemUser(
() -> authorize(authentication, "cluster:admin/whatever", request),
"cluster:admin/whatever", SystemUser.INSTANCE.principal());
verify(auditTrail).accessDenied(authentication, "cluster:admin/whatever", request,
new String[]{SystemUser.ROLE_NAME});
verify(auditTrail).accessDenied(authentication, "cluster:admin/whatever", request, new String[] { SystemUser.ROLE_NAME });
verifyNoMoreInteractions(auditTrail);
}

public void testClusterAdminSnapshotStatusActionIsNotAuthorized() {
public void testClusterAdminSnapshotStatusActionForSystemUserWhichIsNotAuthorized() {
final TransportRequest request = mock(TransportRequest.class);
final Authentication authentication = createAuthentication(SystemUser.INSTANCE);
assertThrowsAuthorizationException(
assertThrowsAssertionErrorWhenAccessDeniedToSystemUser(
() -> authorize(authentication, "cluster:admin/snapshot/status", request),
"cluster:admin/snapshot/status", SystemUser.INSTANCE.principal());
verify(auditTrail).accessDenied(authentication, "cluster:admin/snapshot/status", request,
new String[]{SystemUser.ROLE_NAME});
verify(auditTrail).accessDenied(authentication, "cluster:admin/snapshot/status", request, new String[] { SystemUser.ROLE_NAME });
verifyNoMoreInteractions(auditTrail);
}

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

@jasontedor jasontedor Sep 26, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@tvernum tvernum Sep 26, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@rjernst rjernst Sep 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@bizybot bizybot Sep 26, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@rjernst rjernst Sep 26, 2018

Choose a reason for hiding this comment

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

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)?

Copy link
Member

@jaymode jaymode Sep 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jasontedor jasontedor Sep 27, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jaymode jaymode Oct 1, 2018

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@bizybot bizybot Oct 1, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jasontedor jasontedor Oct 2, 2018

Choose a reason for hiding this comment

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

@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.

assertThat(assertionError.getMessage(), containsString("action ["+ action +"] is unauthorized for user ["+ user +"]"));
}

public void testAuthorizeUsingConditionalPrivileges() {
final DeletePrivilegesRequest request = new DeletePrivilegesRequest();
final Authentication authentication = createAuthentication(new User("user1", "role1"));
Expand Down