-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
ECS audit logging #74640
ECS audit logging #74640
Conversation
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.
This is off to a great start! I have some nits and questions below you, but I like where this is going
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
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.
Took a first pass, looking good so far!
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
64234c1
to
20eefbc
Compare
@restrry This PR removes the audit events from the elastic search cluster client as we didn't find them useful enough and moving forwards expect solutions to be using the secure object client instead. We discussed this with @joshdover but wanted to give you heads up as you were on PTO when we had the discussion. Would be great to get your feedback. |
@thomheymann I thought we had decided:
|
You're right, I think we had another meeting with @legrego that you weren't part of regarding this, sorry for not looping you guys in. The issue with logging at ES client level is that we can't produce any useful audit events out of that as we don't have enough context. We'd essentially just be logging that an elastic search query has been made but not what actions were performed (CRUD). Maybe it would be possible to parse the elastic search query and work out user intention based on that but our thinking was that that's not worth the effort since only ML uses ES client directly and we'd like them to move to using saved objects moving forwards. Furthermore we want to avoid adding audit events that users start relying on and then having to support them over a lengthy deprecation process when we already know that we don't want to log at that low level. Does that sound reasonable to you? Do you have any additional considerations / thoughts? |
It's good as long as the current information suffices for audit logging. After removing the audit trail from elasticsearch, there is no other platform API using it. Therefore, the audit trail service can be removed from the core to reduce the core API surface and to keep all the audit capabilities in the Security plugin. Does it make sense to you?
The current implementation provides |
c234dc4
to
4f92ec1
Compare
Pinging @elastic/kibana-security (Team:Security) |
I'm not opposed to removing the service from core, but we might want to defer that decision until we have a "critical mass" of events audited. It's very likely that we can remove this from core, but I'd like to see how the next few categories of events play out before deciding this. I think we'd safely have this figured out before 8.0, so if you're comfortable leaving this in core for the time being, then we can make sure that we have a final decision by 8.0 so that we aren't carrying around a deprecated API for the next major version. |
2efe854
to
707ab34
Compare
I'm not a fan of keeping unnecessary code, but ok - let's get back to the matter when the audit logging MVP is implemented |
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.
ok for the platform changes
src/core/server/audit_trail/types.ts
Outdated
module?: string; | ||
dataset?: string; | ||
}; | ||
user: { |
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.
optional: I'd rather split them into a smaller interfaces.
type AuditEvent = Event & User & Session & ....
Co-authored-by: Larry Gregory <[email protected]>
603a1df
to
6ca583f
Compare
6ca583f
to
4cdd8a5
Compare
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.
Took another pass at this!
- RE: ECS audit logging #74640 (comment)
I think this might have been overlooked in the flurry of comments. It would be good to add assertions for the expected number of calls to the ECS audit logger in each unit test case. E.g.,
expect(clientOpts.auditLogger.log).toHaveBeenCalledTimes(1);
-
RE: ECS audit logging #74640 (comment)
I'll concede it's OK as-is if you prefer it this way 🙂 -
RE:
#80485
Glad to see there is a PR up for this.
Everything else LGTM!
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.
Just a couple strays, otherwise LGTM!
All done - Thanks a lot for your help! ❤️ |
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 pending green CI and resolved merge conflict. Great work @thomheymann!!
* master: (43 commits) [ML] Transforms: Fix tab ids for expanded row. (elastic#80666) server logs config paths to use for runner (elastic#52980) Fix audit logger logging to console even when disabled (elastic#80928) skip flaky suite (elastic#80929) Added Enterprise Search config to kibana-docker (elastic#80872) skip flaky suite (elastic#80914) [keystore_cli] parse values as JSON before adding to keystore (elastic#80848) [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742) ECS audit logging (elastic#74640) [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215) [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854) Move renderHeaderActions back into mount useEffect + update tests (elastic#80861) [Reporting] Document Network Policy configuration (elastic#80431) [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782) Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757) [Actions] Back Button on Add Connector Flyout (elastic#80160) removing `kibana_datatable` in favor of `datatable` (elastic#80548) [Alerting UI] Updating 'Add new' wording (elastic#80509) [Docs] Document Encrypted Saved Objects functionality. (elastic#80183) [Discover] fix auto-refresh (elastic#80635) ...
* master: (23 commits) [ML] Transforms: Fix tab ids for expanded row. (elastic#80666) server logs config paths to use for runner (elastic#52980) Fix audit logger logging to console even when disabled (elastic#80928) skip flaky suite (elastic#80929) Added Enterprise Search config to kibana-docker (elastic#80872) skip flaky suite (elastic#80914) [keystore_cli] parse values as JSON before adding to keystore (elastic#80848) [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742) ECS audit logging (elastic#74640) [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215) [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854) Move renderHeaderActions back into mount useEffect + update tests (elastic#80861) [Reporting] Document Network Policy configuration (elastic#80431) [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782) Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757) [Actions] Back Button on Add Connector Flyout (elastic#80160) removing `kibana_datatable` in favor of `datatable` (elastic#80548) [Alerting UI] Updating 'Add new' wording (elastic#80509) [Docs] Document Encrypted Saved Objects functionality. (elastic#80183) [Discover] fix auto-refresh (elastic#80635) ...
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
* ECS audit logging (#74640) * ECS audit logging * Apply suggestions from code review Co-authored-by: Larry Gregory <[email protected]> * Update x-pack/plugins/security/server/authentication/audit_events.ts Co-authored-by: Larry Gregory <[email protected]> * Update docs/settings/security-settings.asciidoc Co-authored-by: Larry Gregory <[email protected]> * remove audit trail service from core * fix test * Updated docs and added beta warning * Added dev docs * Tweaks * Plugin list changes * Apply suggestions from technical writers Co-authored-by: Kaarina Tungseth <[email protected]> * Added docs suggestion * Added api integration tests * Added suggestions from platform team * Update x-pack/plugins/security/server/audit/audit_service.test.ts Co-authored-by: Joe Portner <[email protected]> * Update x-pack/plugins/security/server/audit/audit_service.test.ts Co-authored-by: Joe Portner <[email protected]> * Update x-pack/plugins/security/server/audit/audit_service.test.ts Co-authored-by: Joe Portner <[email protected]> * Update docs/user/security/audit-logging.asciidoc Co-authored-by: Joe Portner <[email protected]> * Update docs/settings/security-settings.asciidoc Co-authored-by: Joe Portner <[email protected]> * Update x-pack/plugins/security/server/config.ts Co-authored-by: Joe Portner <[email protected]> * Added suggestions from PR * Grouped events table * Update x-pack/plugins/security/server/audit/audit_events.ts Co-authored-by: Larry Gregory <[email protected]> * Update x-pack/plugins/security/server/audit/audit_events.ts Co-authored-by: Larry Gregory <[email protected]> * Fixed ECS version number in docs Co-authored-by: Larry Gregory <[email protected]> * Added suggestions from code review * Removed beta * Added suggestions from code review Co-authored-by: Larry Gregory <[email protected]> Co-authored-by: Kaarina Tungseth <[email protected]> Co-authored-by: Joe Portner <[email protected]> # Conflicts: # x-pack/plugins/security/server/config.test.ts # x-pack/scripts/functional_tests.js * Fix tests * Fix audit logger logging to console even when disabled Co-authored-by: Kibana Machine <[email protected]>
💔 Build Failed
Failed CI StepsTest FailuresX-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/network_dns·ts.apis SecuritySolution Endpoints Network DNS With packetbeat Make sure that we get Dns data and sorting by uniqueDomains ascendingStandard Out
Stack Trace
Chrome UI Functional Tests.test/functional/apps/discover/_sidebar·js.discover app discover sidebar field filtering should reveal and hide the filter form when the toggle is clickedStandard Out
Stack Trace
Chrome UI Functional Tests.test/functional/apps/getting_started/_shakespeare·js.Getting Started Shakespeare should configure Terms aggregation on play_nameStandard Out
Stack Trace
Metrics [docs]distributable file count
History
To update your PR or re-run it, just comment with: |
Dev Docs
This PR adds new audit logging events and event filtering.
The following audit events will get logged when enabled:
user_login
http_request
saved_object_create
saved_object_get
saved_object_update
saved_object_delete
saved_object_find
saved_object_add_to_spaces
saved_object_delete_from_spaces
How to enable audit logging
CLI:
config:
Example Audit Log
Audit Service API
Checklist
Delete any items that are not applicable to this PR.
For maintainers