-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Updatable API keys - logging audit trail event #88276
Conversation
Pinging @elastic/es-security (Team:Security) |
Hi @n1v0lg, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We document all events belongs to the security_config_change
event type. Do you plan to have it as a separate PR? I think we can have it here since it is a relative small change and does not belong to the main API doc.
...security/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java
Outdated
Show resolved
Hide resolved
withRoleDescriptor(builder, roleDescriptor); | ||
} | ||
builder.endArray(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an oversight that metadata
is not included in the auditing message because metadata
support was added after audit for security-config-change. Our view has contiuned to be that audit message should be as complete as possible. It can get very verbose. But we generally consider the leaving out information without clear context is more risky. Taking the API key metadata as an example, Fleet uses it to identify the host where an agent runs on and this is actually an important piece of information.
That said, I am happy for it to be a separate PR. In fact, I can take this one so that you can focus on the main tasks.
["read","maintenance"]},{"names":["in*","alias*"],"privileges":["read"], | ||
"field_security":{"grant":["field1*","@timestamp"],"except":["field11"]}}], | ||
"applications":[],"run_as":[]},{"cluster":["all"],"indices":[{"names": | ||
["index-b*"],"privileges":["all"]}],"applications":[],"run_as":[]}]}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: no metadata here - will need to add it when we add metadata to the actual event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -1228,6 +1244,18 @@ private void withRequestBody(XContentBuilder builder, CreateApiKeyRequest create | |||
.endObject(); // apikey | |||
} | |||
|
|||
private void withRequestBody(final XContentBuilder builder, final UpdateApiKeyRequest updateApiKeyRequest) throws IOException { | |||
builder.startObject("apikey").field("id", updateApiKeyRequest.getId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking how this should play when we have the bulk update API. My intuition would be having a separate apikeys
schema to avoid having both id
and ids
in the same schema. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup separate schema makes sense. The apikeys
schema already exists for invalidation so we could re-use that for instance.
* upstream/master: (38 commits) Simplify map copying (elastic#88432) Make DiffableUtils.diff implementation agnostic (elastic#88403) Ingest: Start separating Metadata from IngestSourceAndMetadata (elastic#88401) Move runtime fields base scripts out of scripting fields api package. (elastic#88488) Enable TRACE Logging for test and increase timeout (elastic#88477) Mute ReactiveStorageIT#testScaleDuringSplitOrClone (elastic#88480) Track the count of failed invocations since last successful policy snapshot (elastic#88398) Avoid noisy exceptions on data nodes when aborting snapshots (elastic#88476) Fix ReactiveStorageDeciderServiceTests testNodeSizeForDataBelowLowWatermark (elastic#88452) INFO logging of snapshot restore and completion (elastic#88257) unmute test (elastic#88454) Updatable API keys - noop check (elastic#88346) Corrected an incomplete sentence. (elastic#86542) Use consistent shard map type in IndexService (elastic#88465) Stop registering TestGeoShapeFieldMapperPlugin in ESIntegTestCase (elastic#88460) TSDB: RollupShardIndexer logging improvements (elastic#88416) Audit API key ID when create or grant API keys (elastic#88456) Bound random negative size test in SearchSourceBuilderTests#testNegativeSizeErrors (elastic#88457) Updatable API keys - logging audit trail event (elastic#88276) Polish reworked LoggedExec task (elastic#88424) ... # Conflicts: # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/RollupShardIndexer.java
This PR adds a new audit trail event for when API keys are updated.