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

[Entity Analytics] [Entity Store] Add audit logs #196847

Merged
merged 18 commits into from
Oct 30, 2024

Conversation

tiansivive
Copy link
Contributor

@tiansivive tiansivive commented Oct 18, 2024

Summary

This PR adds audit logs for the different actions that can be performed on the entity store engines.

@tiansivive tiansivive added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Theme: entity_analytics Team:Entity Analytics Security Entity Analytics Team labels Oct 18, 2024
@tiansivive tiansivive requested a review from a team as a code owner October 18, 2024 10:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-entity-analytics (Team:Entity Analytics)

@hop-dev hop-dev added v9.0.0 v8.16.0 backport:version Backport to applied version labels v8.17.0 and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 18, 2024
@tiansivive tiansivive self-assigned this Oct 22, 2024
Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

LGTM!

I left a few comments. Before merging the PR, I think we should answer all questions left as comments.

// This may change in the future, depending on the audit action.
const outcome = error ? AUDIT_OUTCOME.FAILURE : AUDIT_OUTCOME.UNKNOWN;

// QUESTION: For EXECUTE action: Maybe START is better: https://www.elastic.co/guide/en/ecs/8.11/ecs-allowed-values-event-type.html#ecs-event-type-start
Copy link
Member

Choose a reason for hiding this comment

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

Should we clarify al questions before merging the PR? Otherwise, we will have inconsistent audit logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We went through it at standup. I've removed the extra audit logs which makes the questions raised obsolete, so I've removed the comments.
Leaving the note one as I think that one is important for any future work here

const descriptor = await this.engineClient.get(entityType);
if (!options?.force && descriptor.status !== ENGINE_STATUS.STOPPED) {
throw new Error(
`In namespace ${this.options.namespace}: Cannot start Entity engine for ${entityType} when current status is: ${descriptor.status}`
`In namespace ${namespace}: Cannot start Entity engine for ${entityType} when current status is: ${descriptor.status}`
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: We should return the error message when the status is an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if we throw it just gets caught by any catch handlers in the requests, no?

const descriptor = await this.engineClient.get(entityType);

if (descriptor.status !== ENGINE_STATUS.STARTED) {
throw new Error(
`In namespace ${this.options.namespace}: Cannot stop Entity engine for ${entityType} when current status is: ${descriptor.status}`
`In namespace ${namespace}: Cannot stop Entity engine for ${entityType} when current status is: ${descriptor.status}`
Copy link
Member

Choose a reason for hiding this comment

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

same thing here and all other similar checks

@tiansivive tiansivive enabled auto-merge (squash) October 29, 2024 11:37
@tiansivive tiansivive merged commit 6c6ae68 into elastic:main Oct 30, 2024
44 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11602482232

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #78 / GenAI - Knowledge Base Entries APIs @ess Basic Security AI Assistant Knowledge Base Entries "before all" hook in "@ess Basic Security AI Assistant Knowledge Base Entries"

Metrics [docs]

✅ unchanged

History

cc @tiansivive

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.16 Backport failed because of merge conflicts
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 196847

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes Team:Entity Analytics Security Entity Analytics Team Theme: entity_analytics v8.16.0 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants