Skip to content
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.

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

Add audit logging for bulk role APIs #110410

Merged
merged 9 commits into from
Jul 5, 2024

Conversation

jfreden
Copy link
Contributor

@jfreden jfreden commented Jul 3, 2024

This adds audit logging to the new role bulk put and delete apis added in: #110383 #109339

_Bulk Put Roles Audit Log - a single put role entry per add

{"type":"audit", "timestamp":"2567-07-05T13:38:39,716+0400", "cluster.name":"roAXTKDMZrpgEUGy", "host.name":"0.0.0.0", "host.ip":"0.0.0.0", "event.type":"security_config_change", "event.action":"put_role", "request.id":"HkONdJnzsupYtgj", "put":{"role":{"name":"null_role","role_descriptor":{"cluster":[],"indices":[],"applications":[],"run_as":[]}}}}
{"type":"audit", "timestamp":"2567-07-05T13:38:39,716+0400", "cluster.name":"roAXTKDMZrpgEUGy", "host.name":"0.0.0.0", "host.ip":"0.0.0.0", "event.type":"security_config_change", "event.action":"put_role", "request.id":"HkONdJnzsupYtgj", "put":{"role":{"name":"role_descriptor1","role_descriptor":{"cluster":["monitor"],"indices":[{"names":["test*"],"privileges":["read","create_index"],"field_security":{"grant":["grantedField1"]},"query":"{\"match_all\":{}}","allow_restricted_indices":true}],"applications":[],"run_as":[]}}}}

_Bulk Delete Roles Audit Log - a single delete role entry per delete

{"type":"audit", "timestamp":"2567-07-05T13:40:33,678+0400", "cluster.name":"roAXTKDMZrpgEUGy", "host.name":"0.0.0.0", "host.ip":"0.0.0.0", "event.type":"security_config_change", "event.action":"delete_role", "request.id":"HkONdJnzsupYtgj", "delete":{"role":{"name":"null_role"}}}
{"type":"audit", "timestamp":"2567-07-05T13:40:33,679+0400", "cluster.name":"roAXTKDMZrpgEUGy", "host.name":"0.0.0.0", "host.ip":"0.0.0.0", "event.type":"security_config_change", "event.action":"delete_role", "request.id":"HkONdJnzsupYtgj", "delete":{"role":{"name":"role_descriptor1"}}}

@jfreden jfreden marked this pull request as ready for review July 3, 2024 09:56
@jfreden jfreden added :Security/Audit X-Pack Audit logging >non-issue labels Jul 3, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Jul 3, 2024
@elasticsearchmachine
Copy link
Collaborator

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

Comment on lines 1190 to 1199
logEntry.with(EVENT_ACTION_FIELD_NAME, "bulk_put_role");
XContentBuilder builder = JsonXContent.contentBuilder().humanReadable(true);
builder.startObject().startObject("roles");

for (RoleDescriptor roleDescriptor : bulkPutRoleRequest.getRoles()) {
withRoleDescriptor(builder.field(roleDescriptor.getName()), roleDescriptor);
}

builder.endObject().endObject();
logEntry.with(CHANGE_CONFIG_FIELD_NAME, Strings.toString(builder));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good option.
Another option is to create separate audit entries for every role, maintaining the format of the individual audit entry. That's important because audit entries could be indexed.

I suggest hearing what the broader team has to say. Could you please raise it in the team meeting today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. Thanks for raising this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised this in the weekly security meeting and we agreed to change this to separate audit entries per role. Thanks for the suggestion @albertzaharovits !

@jfreden
Copy link
Contributor Author

jfreden commented Jul 5, 2024

I've changed approach to generate one log entry per bulk item. All the entries for a bulk request will now be under the same request id. I couldn't find anywhere else where we do that so wanted to discuss the possible side effects of that? Any thoughts @albertzaharovits ?

@albertzaharovits albertzaharovits self-requested a review July 5, 2024 09:27
@albertzaharovits
Copy link
Contributor

All the entries for a bulk request will now be under the same request id. I couldn't find anywhere else where we do that so wanted to discuss the possible side effects of that? Any thoughts @albertzaharovits ?

There should be no problem here. The requestId is exactly used to correlate multiple audit entries in relation to the same request. The fact that a single request generates multiple audit entries "at a time" should not matter.

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM

@jfreden
Copy link
Contributor Author

jfreden commented Jul 5, 2024

Thanks for confirming!

@jfreden jfreden merged commit d7d86b4 into elastic:main Jul 5, 2024
20 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

The backport operation could not be completed due to the following error:

There are no branches to backport to. Aborting.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 110410

jfreden added a commit to jfreden/elasticsearch that referenced this pull request Jul 5, 2024
* Add audit logging for bulk put role
elasticsearchmachine pushed a commit that referenced this pull request Jul 5, 2024
* Add audit logging for bulk put role
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants