Add logging implementation for AuditManager and audit more endpoints #15480

kfaraz · 2023-12-04T12:56:17Z

Description

This PR adds an implementation of AuditManager that simply logs the audit events instead of persisting them to metadata store.

Sample audit log

Setup

Authentication: none
druid.audit.manager.type=log

2023-12-08T14:20:07,403 WARN [qtp972079939-170] org.apache.druid.server.audit.AuditLogger - 
User[console], identity[allowAll], IP[127.0.0.1] performed action[rules]
 on key[inline_data] with comment[Load 2 replicas in the default tier].
 Request[null],
 payload[[{"tieredReplicants":{"_default_tier":2},"useDefaultTierForNull":true,"type":"loadForever"}]].

Changes

Add log implementation for AuditManager alongwith SQLAuditManager
LoggingAuditManager simply logs the audit event. Thus, it returns empty for all fetchAuditHistory calls
Add new config druid.audit.manager.type which can take values log, sql (default)
Add new config druid.audit.manager.logLevel which can take values DEBUG, INFO, WARN. This gets activated only if type is log.
Remove usage of ConfigSerde from AuditManager as audit is not just limited to configs
Add AuditSerdeHelper for a single implementation of serialization/deserialization of audit payload that can be used by different AuditManager implementations

How is audit performed

All existing and new audited REST API endpoints accept two headers X-Druid-Author and X-Druid-Comment which are being used to populate the audit info. The web-console currently passes console as the value for X-Druid-Author.

An alternative to this can be to extract the authentication result set as an attribute in the HttpServletRequest and invoke authenticationResult.getIdentity().

New audited endpoints

Post a task type=ingestion.batch
Mark segments as unused: type=markSegmentsAsUnused
Kill segments: type=killSegments
Create user in basic authenticator: type=basicAuth.createUser
Delete user in basic authenticator: type=basicAuth.deleteUser
Update user in basic authenticator: type=basicAuth.updateUserCreds

Release note

Add new config druid.audit.manager.type which can take values log, sql(default). This allows audited events to either be logged or persisted in metadata store (default behaviour).
Add new config druid.audit.manager.logLevel which allows users to set the log level of audit events and can take values DEBUG, INFO(default), WARN.

This PR has:

server/src/test/java/org/apache/druid/server/audit/SQLAuditManagerTest.java

server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java

    try {
      Response response =
-          dataSourcesResource.markAsUnusedAllSegmentsOrKillUnusedSegmentsInInterval("datasource", "true", "???");
+          dataSourcesResource.markAsUnusedAllSegmentsOrKillUnusedSegmentsInInterval("datasource", "true", "???", request);


...java/org/apache/druid/security/authentication/CoordinatorBasicAuthenticatorResourceTest.java

indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java

abhishekagarwal87 · 2023-12-05T09:20:49Z

...xing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java

@@ -900,7 +900,7 @@ public void testTaskPostDeniesDatasourceReadUser()
    Task task = NoopTask.forDatasource(Datasources.WIKIPEDIA);
    expectedException.expect(ForbiddenException.class);
    expectedException.expect(ForbiddenException.class);
-    overlordResource.taskPost(task, req);
+    overlordResource.taskPost(task, "", "", req);


maybe add a test with non-empty author to see that audit event does go through?

processing/src/test/java/org/apache/druid/audit/NoopAuditManager.java

server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java

server/src/main/java/org/apache/druid/server/audit/AuditSerdeHelper.java

server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java

abhishekagarwal87 · 2023-12-05T09:31:21Z

server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java

+        auditManager.doAudit(
+            AuditEvent.builder()
+                      .key(dataSourceName)
+                      .type("segments.markUnused")


can the type name be standardized? E.g. serviceName.api-path

Yeah, that makes sense to me.

So would something like the following work?

coordinator.markSegmentsUnused

coordinator.basicAuth.createUser

overlord.postTask

overlord.killSegments

Added a field request in AuditEntry to contain the details of path, uri, method and service.

abhishekagarwal87 · 2023-12-16T09:45:33Z

indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java

@@ -225,18 +222,29 @@ public Response taskPost(final Task task, @Context final HttpServletRequest req)
        taskQueue -> {
          try {
            taskQueue.add(task);
+
+            auditManager.doAudit(


can we filter task submissions that were initiated by the system itself? E.g. compaction duty, MSQ controller task, etc?

Yes, this is already happening inside the respective audit impls, such as SQLAuditManager and LoggingAuditManager. We check if an audited event was started by an internal service (by checking the author field) and then audit that event only if system event auditing is enabled via config (false by default).

This is better than checking at each call site if the request was initiated by an internal service.

abhishekagarwal87 · 2023-12-16T09:47:16Z

processing/src/main/java/org/apache/druid/audit/AuditManager.java

-  <T> void doAudit(String key, String type, AuditInfo auditInfo, T payload, ConfigSerde<T> configSerde);
+  default boolean isSystemRequest(AuditInfo auditInfo)
+  {
+    return AUTHOR_DRUID_SYSTEM.equals(auditInfo.getAuthor());


would this be valid for any auth extension?

Yes, because we would explicitly set this as the header value when using internal clients.

Currently, we are setting it only in OverlordClientImpl.runTask but we can choose do set it in other audited endpoints too, or better yet just always set it by default in any service client. What do you think?

So its possible for a user to bypass audit system if they set this header?

abhishekagarwal87 · 2023-12-16T09:49:01Z

server/src/main/java/org/apache/druid/guice/SQLMetadataStorageDruidModule.java

+    final MapBinder<String, AuditManager> auditManagerBinder
+        = PolyBind.optionBinder(binder, Key.get(AuditManager.class));
+    auditManagerBinder
+        .addBinding("log")
+        .to(LoggingAuditManager.class)
+        .in(LazySingleton.class);


seems off that the log choice is added in this module. should this be moved to some other module?

Sure, let me put this in some other module.

abhishekagarwal87 · 2023-12-16T09:49:48Z

server/src/main/java/org/apache/druid/rpc/indexing/OverlordClientImpl.java

@@ -96,7 +97,8 @@ public ListenableFuture<Void> runTask(final String taskId, final Object taskObje
    return FutureUtils.transform(
        client.asyncRequest(
            new RequestBuilder(HttpMethod.POST, "/druid/indexer/v1/task")
-                .jsonContent(jsonMapper, taskObject),
+                .jsonContent(jsonMapper, taskObject)
+                .header(AuditManager.X_DRUID_AUTHOR, AuditManager.AUTHOR_DRUID_SYSTEM),


is this needed? can we not rely on authenticated identity instead?

This seemed much easier to implement.

I encountered some issues with the other approach:

The authenticated identity would depend on both auth impl and configuration. I couldn't find a definite way to determine the internal service identity. Basic auth creates the druid_system user but I don't see it being used in internal communication. (On second thought, let me see if I can pull the identity from the escalated client that is being injected)

When there is no auth, all requests get the identity allowAll.

When auth is enabled, a user could still use the internal druid service username/password to invoke an API and bypass the audit (This is technically still possible by setting the author header to this specific value but that is less likely to happen. This is one of the reasons I chose to include a config for auditing system requests too).

Please let me know your thoughts on this.

server/src/main/java/org/apache/druid/server/audit/AuditManagerConfig.java

abhishekagarwal87 · 2023-12-16T09:52:45Z

server/src/main/java/org/apache/druid/server/coordinator/duty/KillCompactionConfig.java

+            "KillCompactionConfig",
            "KillCompactionConfig",


identity and author are KillCompactionConfig?

Thanks for catching this. Since we are revisiting auditing in this PR, I guess it would be better to use druid_system or something as the author instead.

kfaraz · 2023-12-19T07:43:22Z

Thanks for the review, @abhishekagarwal87 !
Merging since the IT failure is not caused by this set of change.

xvrl · 2023-12-19T19:24:03Z

@kfaraz the master build started failing for some middle-manager integration tests after merging your PR, could you take a look?

kfaraz · 2023-12-20T08:05:53Z

Yes, @xvrl , I am trying to debug that issue here #15561. It seems like a flaky test as it does seem to pass sometimes. But the flakiness has become pronounced after the changes in this auditing PR somehow.

kfaraz added 2 commits December 4, 2023 18:22

Add AuditLogger

560ca30

Merge branch 'master' of github.com:apache/druid into add_audit_logger

2415a17

github-actions bot added Area - Web Console Area - Ingestion labels Dec 4, 2023

kfaraz added 6 commits December 4, 2023 18:35

Fix compile and checkstyle

9cb9dc6

Audit more endpoints

74860c7

Fix codeql

f418435

Audit some more endpoints

5adc89a

Fix codeQL check

383eb1d

Fix web checks

011319e

github-advanced-security bot found potential problems Dec 4, 2023

View reviewed changes

kfaraz added 3 commits December 4, 2023 23:08

Fix tests

0d88e64

Remove unused variable

23c2939

Fix test

ce41276

abhishekagarwal87 reviewed Dec 5, 2023

View reviewed changes

kfaraz added 8 commits December 5, 2023 15:12

Remove AuditRecord, use AuditEntry

b466857

Merge branch 'master' of github.com:apache/druid into add_audit_logger

75e8e1a

Add identity field in AuditInfo

6c438d5

Simplify usage of author and comment headers

2f2eebc

Fix tests

956e6a5

Add RequestInfo to AuditEntry

6144091

Fix tests

5e5a2fc

Minor cleanup

71ae175

github-actions bot removed the Area - Web Console label Dec 8, 2023

kfaraz added 6 commits December 8, 2023 20:54

Fix tests and coverage

737b796

Fix checks and tests

aae788c

Set author header when using OverlordClient.runTask

ce7a17f

Use constructors in configs for better null-handling

5853e2a

Add config druid.audit.manager.auditSystemRequests

d0efb3b

Use config auditSystemRequests

ed3ecee

kfaraz added 2 commits December 14, 2023 21:31

More audits, attempt to debug failing IT

f42d9a6

More changes

ed22509

abhishekagarwal87 reviewed Dec 16, 2023

View reviewed changes

Use Escalator to determine system identity

0b73b5f

abhishekagarwal87 approved these changes Dec 18, 2023

View reviewed changes

kfaraz added 3 commits December 19, 2023 09:09

Fix failing test

c87fb13

Add more tests for coverage

33d8a22

Fix audit manager bindings

1cff032

kfaraz merged commit 9f56885 into apache:master Dec 19, 2023
82 of 83 checks passed

kfaraz deleted the add_audit_logger branch December 19, 2023 07:44

LakshSingla added this to the 29.0.0 milestone Jan 29, 2024

LakshSingla mentioned this pull request Feb 13, 2024

[DRAFT] 29.0.0 release notes #15896

Closed

jakubmatyszewski mentioned this pull request Aug 20, 2024

Add docs for log audit manager #16927

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging implementation for AuditManager and audit more endpoints #15480

Add logging implementation for AuditManager and audit more endpoints #15480

kfaraz commented Dec 4, 2023 •

edited

Loading

abhishekagarwal87 Dec 5, 2023

abhishekagarwal87 Dec 5, 2023

kfaraz Dec 5, 2023

kfaraz Dec 15, 2023

abhishekagarwal87 Dec 16, 2023

kfaraz Dec 17, 2023

abhishekagarwal87 Dec 16, 2023

kfaraz Dec 17, 2023

abhishekagarwal87 Dec 18, 2023

abhishekagarwal87 Dec 16, 2023

kfaraz Dec 17, 2023 •

edited

Loading

abhishekagarwal87 Dec 16, 2023

kfaraz Dec 17, 2023 •

edited

Loading

abhishekagarwal87 Dec 16, 2023

kfaraz Dec 17, 2023 •

edited

Loading

kfaraz commented Dec 19, 2023

xvrl commented Dec 19, 2023

kfaraz commented Dec 20, 2023

Add logging implementation for AuditManager and audit more endpoints #15480

Add logging implementation for AuditManager and audit more endpoints #15480

Conversation

kfaraz commented Dec 4, 2023 • edited Loading

Description

Sample audit log

Setup

Changes

How is audit performed

New audited endpoints

Release note

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

kfaraz Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfaraz Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfaraz Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

kfaraz commented Dec 19, 2023

xvrl commented Dec 19, 2023

kfaraz commented Dec 20, 2023

kfaraz commented Dec 4, 2023 •

edited

Loading

kfaraz Dec 17, 2023 •

edited

Loading

kfaraz Dec 17, 2023 •

edited

Loading

kfaraz Dec 17, 2023 •

edited

Loading