ECS audit events for alerting #84113

thomheymann · 2020-11-23T16:18:13Z

Release Note

Adds ECS audit events for alerts and actions plugins.

Docs

https://kibana_84113.docs-preview.app.elstc.co/guide/en/kibana/master/xpack-security-audit-logging.html

Summary

The following events are now logged in ECS format when the new audit logger is enabled:

NOTE: @LeeDr - some of these event names changed, see https://www.elastic.co/guide/en/kibana/7.11/xpack-security-audit-logging.html

connector_create
connector_update
connector_delete
connector_get
connector_find
alert_rule_create
alert_rule_update
alert_rule_update_api_key
alert_rule_enable
alert_rule_disable
alert_rule_delete
alert_rule_get
alert_rule_find
alert_rule_mute
alert_rule_unmute
alert_instance_mute
alert_instance_unmute

Approach

Even though alerts are basically saved objects and the events share a lot of similarities with saved object CRUD operations we decided to create dedicated events for alert rules and connectors since they have their own authentication model and are hidden saved objects so won't be visible to users as a saved object in stack management screen.
The event names align with the alerting working group terminology, rather than the code. (e.g. action=connector, alert=alert_rule)
Write operations should be logged after any authorisation checks have been carried out but before the operation starts in order to ensure that a record is persisted even in case of a fatal crash. In order to log the saved object id alongside the create operation we are generating an ID using core generateSavedObjectId function directly within alerts and actions client (based on Allow predefined ids for encrypted saved objects #83482)

Known issues

Any event that is logged as part of a background task / fake request will not have the user field populated. While this is not the best user experience it does not create black spots in the audit trail since the user who created/updated the object can be looked up by querying the audit log for previous alert_rule_create or alert_rule_update events. This should be tackled as part of a separate PR once we worked out how we can support a concept of "authenticated fake requests". (Related to Scope-able elasticsearch clients #39430)

Out of scope

The legacy audit events are still logged. I will remove them as part of a separate PR together with the rest of the legacy audit logger.
connector_execute events are out of scope as the code path is currently different between direct execution and async execution (background tasks) which would cause duplicate or missing events. @gmmorris confirmed that the code can be refactored to support audit logging.

How to test

Enable audit logging:

xpack.security.audit.enabled: true
xpack.security.audit.appender:
  kind: file
  path: /kibana/audit.log
  layout:
    kind: json

Start Kibana and Elasticsearch in SSL mode and watch the audit log:

yarn es snapshot --license trial --ssl
yarn start --ssl
tail -f /kibana/audit.log

Create a connector / alert via Stack Management in Kibana

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-11-23T16:18:16Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

pgayvallet · 2020-11-24T20:50:11Z

src/core/server/index.ts

@@ -311,6 +311,7 @@ export {
  exportSavedObjectsToStream,
  importSavedObjectsFromStream,
  resolveSavedObjectsImportErrors,
+  generateSavedObjectId,


We should reduce non-contract APIs exposed from core to a minimum (the import/export static functions are remains from legacy that will be moved to the SO service mid term). Any reason this generateSavedObjectId is exposed as a static function instead of being provided by the savedObjects service contract?

It's a pure function that doesn't require any shared state so didn't make sense to me to add it to the SavedObjectsSerializer. Maybe we could add it as a static method to SavedObjectsServiceSetup if that addresses your concern?

cc @rudolf wdyt? is exposing generateSavedObjectId statically from the index acceptable to you, or do you think this should be exposed from the SO service contract?

Another option could be to put this into kbn-utils package.

Another option could be to put this into kbn-utils package.

I don't think we should. Separate by domain is one of our main principles, so it should belong to saved objects.
++ to add it as a setup/start contract property. id creation operation doesn't sound like serializer responsibility.
btw why other code in SO service still calls uuid to generate id?

kibana/src/core/server/saved_objects/service/lib/repository.ts

Line 350 in d4b2a51

if (object.id == null) object.id = uuid.v1();

However in that case, I'd say that the best option may be to stick to using uuid.v1 from the consuming code until we do implement this new API. @rudolf @restrry wdyt?

👍

However in that case, I'd say that the best option may be to stick to using uuid.v1 from the consuming code until we do implement this new API. @rudolf @restrry wdyt?

I believe the logic for ID generation is changing in the very near future (👀 @jportner), so I feel like it'd be safer to keep the logic consolidated in the static helper. Asking consumers to use uuid.v1 is leaking an implementation detail of the SO service, which feels worse to me than exposing a static function from the service.

I believe the logic for ID generation is changing in the very near future (👀 @jportner)

Sharing Saved Objects will implement ID (re-)generation when existing objects are converted, but I have no plans to change "regular" ID generation.

Let's keep it in SavedObjectsUtils then. it would be easier to track than greping for uuid anyway when we'll need to move that to the service contract.

All done. Please give it another parse.

This reverts commit 7d929fe.

gmmorris

(approved changes to Alerts & Actions)
LGTM, thanks for taking on this mammoth of a task!

It looks good and I'm happy to approve, but there's one thing that worries me (would be good to hear more from @elastic/kibana-alerting-services on this), which is that the audit logging concerns are yet another think the AlertsClient does.... I was really hoping we could hide that implementation inside of the AlertsAuthorization class (same thing fo Actions).

I'm happy to ago ahead and merge this with the hope that we find away of decoupling these things later... but I thought it might be worth flagging incase anyone on the team feels otherwise.

x-pack/plugins/actions/server/lib/audit_events.ts

gmmorris · 2020-11-30T12:32:43Z

x-pack/plugins/alerts/server/alerts_client/alerts_client.ts

+    this.auditLogger?.log(
+      alertRuleEvent({
+        action: AlertRuleAction.CREATE,
+        outcome: EventOutcome.UNKNOWN,


Is this "unknown" because we haven't actually tried to create yet? If so, shouldn't this be explicit about not having an unknown outcome as much as just not having reached an outcome yet?

I might be misunderstanding the reasons it's unknown 🤔
Just wondering if 'Unknown' is conceptually the right way of thinking about this event as it could be confusing if we see this in the log...

You're correct, the outcome for write operations is logged as unknown since we don't know whether the operation succeeded or failed at that point in time.

I wanted to be explicit about it so that it's clear to the user that the operation has not completed yet, which is also why the message is phrased in present progressive rather than past tense.

I also wanted to keep the option open to add logging the outcome as a separate event if users require that information for auditing purposes.

My concern with omitting the outcome field would be that users might not fully realise what the event means and when it was logged.

Fair enough, I guess the very existence of an unknown outcomes suggests to me that there was an outcome, we just couldn't identify it...
I can go either way, which is why I was happy to approve 👍

x-pack/plugins/alerts/server/alerts_client/audit_events.ts

mikecote · 2020-11-30T18:58:33Z

Hi @thomheymann, regarding the terminology, I know we've been back and forth on what terms to use so the alerting team had a discussion on what to use. We've decided against gradually introducing the new terms into our codebase (would cause increase in confusion vs changing everything at once) as it seems the new terminology won't be used just yet.

I hope this doesn't cause too many changes in your PR but please let me know if I can help out. I did a pass at the events in the description and came up with a map of what I think they would now become:

connector_create
connector_update
connector_delete
connector_get
connector_find
alert_create
alert_update
alert_update_api_key
alert_enable
alert_disable
alert_delete
alert_get
alert_find
alert_mute
alert_unmute
alert_instance_mute
alert_instance_unmute

mikecote

I started my review but ran short on time to finish. I finished looking at the implementation for actions plugin and left a few comments. I'm sure some of these would apply to the alerts plugin as well but I haven't looked at it yet.

x-pack/plugins/actions/server/lib/audit_events.ts

x-pack/plugins/actions/server/actions_client.ts

mikecote · 2020-12-01T16:34:53Z

x-pack/plugins/actions/server/actions_client.ts

+        this.preconfiguredActions.find((preconfiguredAction) => preconfiguredAction.id === id) !==
+        undefined
+      ) {
+        throw new PreconfiguredActionDisabledModificationError(


Is it intentional to capture the errors thrown when the action type is disabled? If so, there's a few other places (this.actionTypeRegistry.ensureActionTypeEnabled) that will throw similar errors.

When does this error occur? Is it role based or just some kind of global configuration?

I thought it was an authorisation error but looks like I made a mistake here.

When does this error occur? Is it role based or just some kind of global configuration?

There are two types of actions in the system, pre-configured and dynamic / user created actions. This code will throw whenever the update API is called for a pre-configured action. Since those actions are configured via kibana.yml, we don't support updates and throw the error.

So I think we may be ok here to skip auditing this error?

I've double checked with Oleg and we think this is audit worthy. As an auditor you'd want to know why someone is trying to update preconfigured connectors.

I'll make ensureActionTypeEnabled error are included in the audit log.

Are there any other errors I should include?

Actually, looking at the errors thrown inside ensureActionTypeEnabled, I'm not sure they're audit worthy.

The first type of error is thrown when an action type is disabled. That just feels like a validation error.

The second type of error is thrown when the license doesn't allow a certain type. That's definitely not security related / audit worthy.

I'd say let's leave these as is until we get a requirement to include those errors.

👍 works for me!

mikecote · 2020-12-01T16:35:30Z

x-pack/plugins/actions/server/actions_client.ts

+        this.preconfiguredActions.find((preconfiguredAction) => preconfiguredAction.id === id) !==
+        undefined
+      ) {
+        throw new PreconfiguredActionDisabledModificationError(


Same question here:

Is it intentional to capture the errors thrown when the action type is disabled? If so, there's a few other places (this.actionTypeRegistry.ensureActionTypeEnabled) that will throw similar errors.

flash1293

KibanaApp changes LGTM, just adding ids to the saved object migration tests which should have been there anyway

mikecote

I reviewed the remaining changes, LGTM once the requests below are complete! Great work!

Renaming of events: ECS audit events for alerting #84113 (comment)
Moving out the validation code discussed in ECS audit events for alerting #84113 (comment) and ECS audit events for alerting #84113 (comment)

jportner · 2020-12-03T17:12:49Z

@elasticmachine merge upstream

jportner

Looking great, mostly nits below!

src/core/server/saved_objects/service/lib/repository.test.js

jportner · 2020-12-03T18:49:22Z

src/core/server/saved_objects/service/lib/utils.ts

+  public static isRandomId(id: string | undefined) {
+    return typeof id === 'string' && UUID_REGEX.test(id);
+  }


Nit

Suggested change

public static isRandomId(id: string | undefined) {

return typeof id === 'string' && UUID_REGEX.test(id);

}

public static isRandomId(id: string | undefined) {

return typeof id === 'string' && UUID_REGEX.test(id.toLowerCase());

}

Alternatively you could change the regex to search for upper-case characters 🤷‍♂️

I don't think UUIDs are case insensitive: https://github.com/uuidjs/uuid/blob/master/src/validate.js

I'd prefer to use the package directly but upgrading to latest version is not trivial and should be a PR in its own right.

I think the uuid package is wrong. See the spec:

Each field is treated as an integer and has its value printed as a zero-filled hexadecimal digit string with the most significant digit first. The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.

But I suppose if we only ever care to validate IDs that were generated with the uuid package, the current implementation is correct 😄

jportner · 2020-12-03T20:34:24Z

x-pack/plugins/actions/server/actions_client.ts

+    ids.forEach((id) =>
+      this.auditLogger?.log(
+        connectorAuditEvent({
+          action: ConnectorAuditAction.GET,
+          savedObject: { type: 'action', id },
+        })
+      )
+    );


I know this is not intuitive, but when executing a bulk operation, the entire operation may succeed even though there are one or more errors for individual objects. For example, if an invalid type was specified (bad request), or if the object was not found.

To be consistent with our other access logs, we don't want to create an audit record for an object that wasn't actually accessed by the user. You could account for these cases with the following change:

Suggested change

ids.forEach((id) =>

this.auditLogger?.log(

connectorAuditEvent({

action: ConnectorAuditAction.GET,

savedObject: { type: 'action', id },

})

)

);

bulkGetResult.saved_objects.forEach(({ id, error }) => {

if (!error && this.auditLogger) {

this.auditLogger.log(

connectorAuditEvent({

action: ConnectorAuditAction.GET,

savedObject: { type: 'action', id },

})

);

}

});

Note, we should make the same change in x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts too.

jportner · 2020-12-03T21:10:33Z

...ugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts

+    const canSpecifyID =
+      (options.overwrite && options.version) || SavedObjectsUtils.isRandomId(options.id);
+    if (options.id && !canSpecifyID) {
      throw new Error(
-        `Predefined IDs are not allowed for encrypted saved objects of type "${type}".`
+        'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
      );
    }


Nit: the same code is used in two different places, could be abstracted out to a function like

private throwIfIdIsNotAllowed(id: string | undefined, overwrite: boolean, version: string) { const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id); if (id && !canSpecifyID) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.' ); } }

I have added a getValidId method that handles all that logic and also generates an id if none has been specified:
459e0c4#diff-06a76df7e9f18756a4bcd62574dd9d6fd505a9ed7c8f275a1bdabd52a81a4bb6R298-R320

jportner · 2020-12-03T21:14:41Z

src/core/server/saved_objects/service/lib/utils.ts

+  public static generateId() {
+    return uuid.v1();
+  }


Perhaps we should use uuid.v4() here instead, considering that the ESO wrapper now relies on this?

I know that you should not rely on UUIDs being hard to guess, but UUIDv1 is very predictable where UUIDv4 is not. I don't see any practical benefit to using v1 over v4.

@pgayvallet do you have any thoughts on this either way?

I believe we already had this discussion a while ago (probably with @rudolf), and I can't exactly remember the outcome, but to restart the conversation:

Objectively, v4 is better than v1

Still, I think v1 is good enough, it 'just work (tm)', so there is no urge to change it (or do you see one?)

I don't think we should switch from one to the other during a minor

Even if we do, it should probably not be in this PR

Do you want me to open an issue to discuss this change?

Still, I think v1 is good enough, it 'just work (tm)', so there is no urge to change it (or do you see one?)

My only justification is that the ESO wrapper used to generate its own IDs upon object creation, and it used v4. Now it's relying on the core generateId function which uses v1.

I think it's worth discussing further. I'm curious to know why we're hesitant to change to v4 in a minor.

x-pack/plugins/security/server/audit/audit_events.ts

x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts

pgayvallet

LGTM

pgayvallet · 2020-12-04T07:59:00Z

src/core/server/saved_objects/service/lib/utils.ts

+  public static generateId() {
+    return uuid.v1();
+  }


I believe we already had this discussion a while ago (probably with @rudolf), and I can't exactly remember the outcome, but to restart the conversation:

Objectively, v4 is better than v1

Still, I think v1 is good enough, it 'just work (tm)', so there is no urge to change it (or do you see one?)

I don't think we should switch from one to the other during a minor

Even if we do, it should probably not be in this PR

Do you want me to open an issue to discuss this change?

pgayvallet · 2020-12-04T08:03:04Z

src/core/server/saved_objects/service/lib/utils.ts

+  /**
+   * Validates that a saved object ID matches UUID format.
+   *
+   * @param {string} id The ID of a saved object.
+   * @todo Use `uuid.validate` once upgraded to v5.3+
+   */
+  public static isRandomId(id: string | undefined) {
+    return typeof id === 'string' && UUID_REGEX.test(id);
+  }


I think this naming is misleading. We are asserting that the id matches the uuid format, not that is was randomly generated (which is properly explained in the tsdoc, but not really reflected on the method name)

The purpose of the isRandomId method is to allow plugin developers to assert whether an ID has been randomly generated or whether it's a static ID. In order to do so the function checks whether a string matches UUID format but that's an implementation detail that consumers don't need to be aware of in the same way that they don't need to know that generateId generates a UUID.

Would isRandomlyGeneratedId be more explicit?

That would just be a rephrasing.

In that case, change Validates that a saved object ID matches UUID format. to Validates that a saved object ID has been randomly generated

jportner

LGTM, great stuff!

* ECS audit events for alerts plugin * added api changes * fixed linting and testing errors * fix test * Fixed linting errors after prettier update * Revert "Allow predefined ids for encrypted saved objects (#83482)" This reverts commit 7d929fe. * Added suggestions from code review * Fixed unit tests * Added suggestions from code review * Changed names of alert events * Changed naming as suggested in code review * Added suggestions from PR Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-01-15T18:52:18Z

⏳ Build in-progress, with failures

continuous-integration/kibana-ci/pull-request
Commit: 0ff3dfe
This comment will update when the build is complete

Failed CI Steps

History

💚 Build #92048 succeeded 0ff3dfe
💔 Build #91967 failed 459e0c4
💚 Build #91776 succeeded 5b91bb8
💚 Build #91323 succeeded 2873fe7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

ECS audit events for alerts plugin

51a784a

thomheymann added release_note:enhancement v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 labels Nov 23, 2020

thomheymann requested review from a team as code owners November 23, 2020 16:18

thomheymann added 5 commits November 24, 2020 09:19

added api changes

3652cf0

fixed linting and testing errors

ac215a0

Merge remote-tracking branch 'origin/master' into alerting/audit-logging

7abc2b0

fix test

6c3cb47

Merge remote-tracking branch 'origin/master' into alerting/audit-logging

5f10ab5

pgayvallet reviewed Nov 24, 2020

View reviewed changes

thomheymann added 5 commits November 25, 2020 10:22

Merge remote-tracking branch 'origin/master' into alerting/audit-logging

4e3d1a1

Fixed linting errors after prettier update

337a902

Revert "Allow predefined ids for encrypted saved objects (#83482)"

5f598d1

This reverts commit 7d929fe.

Added suggestions from code review

a2dd90f

Merge remote-tracking branch 'origin/master' into alerting/audit-logging

c9082be

thomheymann requested a review from a team as a code owner November 30, 2020 11:15

thomheymann requested a review from a team November 30, 2020 11:15

thomheymann requested a review from a team as a code owner November 30, 2020 11:15

thomheymann requested a review from pgayvallet November 30, 2020 11:19

gmmorris self-requested a review November 30, 2020 11:43

Fixed unit tests

d11ffc2

gmmorris approved these changes Nov 30, 2020

View reviewed changes

Added suggestions from code review

6e473a9

Merge remote-tracking branch 'origin/master' into alerting/audit-logging

fa5215a

mikecote self-requested a review December 1, 2020 16:01

mikecote reviewed Dec 1, 2020

View reviewed changes

Changed names of alert events

9014744

flash1293 approved these changes Dec 2, 2020

View reviewed changes

Changed naming as suggested in code review

2873fe7

thomheymann requested a review from mikecote December 2, 2020 11:26

mikecote approved these changes Dec 2, 2020

View reviewed changes

legrego requested a review from jportner December 3, 2020 14:52

Merge branch 'master' into alerting/audit-logging

5b91bb8

jportner reviewed Dec 3, 2020

View reviewed changes

legrego mentioned this pull request Dec 4, 2020

SavedObjects should support importing/exporting even if they're hidden from the API #82027

Closed

pgayvallet approved these changes Dec 4, 2020

View reviewed changes

Added suggestions from PR

459e0c4

jportner approved these changes Dec 4, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into alerting/audit-logging

0ff3dfe

thomheymann merged commit f413957 into master Dec 4, 2020

thomheymann deleted the alerting/audit-logging branch December 4, 2020 19:13

thomheymann mentioned this pull request Dec 5, 2020

[7.x] ECS audit events for alerting (#84113) #85093

Merged

marshallmain mentioned this pull request Dec 8, 2020

[Security Solution][Detections] Loosen UUID regex to accept uuidv1 or uuidv4 #85338

Merged

8 tasks

Pearson-k mentioned this pull request Feb 19, 2021

New audit options missing from kibana-docker #92046

Closed

jportner mentioned this pull request Nov 17, 2021

Kibana allows users to create a saved object with an empty ID #118957

Closed

ECS audit events for alerting #84113

ECS audit events for alerting #84113

Conversation

thomheymann commented Nov 23, 2020 • edited by LeeDr Loading

Release Note

Docs

Summary

Approach

Known issues

Out of scope

How to test

Checklist

For maintainers

elasticmachine commented Nov 23, 2020

pgayvallet Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmmorris left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikecote commented Nov 30, 2020

mikecote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

jportner commented Dec 3, 2020

jportner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 15, 2021 • edited Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

thomheymann commented Nov 23, 2020 •

edited by LeeDr

Loading

pgayvallet Nov 24, 2020 •

edited

Loading

gmmorris left a comment •

edited

Loading

kibanamachine commented Jan 15, 2021 •

edited

Loading