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
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 all 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 DiskThresholdMonitor.markIndicesReadOnly
), Automatons.patterns("internal:transport/proxy/*"))); // no proxy actions for system user!

private SystemPrivilege() {
Expand Down
2 changes: 2 additions & 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,8 @@ 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));
assertThat(predicate.test("indices:admin/settings/foo"), is(false));
}

public void testManageCcrPrivilege() {
Expand Down
3 changes: 3 additions & 0 deletions ...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 @@ -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(),
Copy link
Member

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

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
33 changes: 16 additions & 17 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,22 +240,23 @@ 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(
Expand All @@ -265,25 +266,23 @@ public void testIndicesActionsAreNotAuthorized() {
verifyNoMoreInteractions(auditTrail);
}

public void testClusterAdminActionsAreNotAuthorized() {
public void testClusterAdminActionsForSystemUserWhichAreNotAuthorized() {
final TransportRequest request = mock(TransportRequest.class);
final Authentication authentication = createAuthentication(SystemUser.INSTANCE);
assertThrowsAuthorizationException(
() -> 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(
() -> 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);
}

Expand Down