fix msq compaction to apply security policies by clintropolis · Pull Request #18741 · apache/druid

clintropolis · 2025-11-13T20:25:50Z

Description

Fixes an issue with MSQ compaction tasks can fail if a policy enforcer is enabled, since compaction generates MSQ tasks directly instead of going through the broker, it would miss out on decorating the DataSource as a restricted datasource with the policy set.

To fix, MSQCompactionRunner now uses an escalator to create an auth result and uses the policy from that to decorate the query it generates.

To test, I hijacked CompactionSupervisorTest and switched it to use MSQ engine and set it up with an auth config and policy enforcer (with a newly added AllowAllWithPolicyAuthResource which is essentially allow-all but with NoRestrictionPolicy permitted)

kfaraz

Left some minor suggestions.

kfaraz · 2025-11-14T13:52:43Z

+    }
+  }
+
+  @JsonTypeName(AUTH_TYPE)


Why is a json type name needed for the module?

it is not, this was an accidental copy and paste when i was setting it on the other classes

kfaraz · 2025-11-14T13:53:04Z

+
+public class AllowAllWithPolicyAuthResource implements EmbeddedResource
+{
+  private static final String AUTH_TYPE = "actually-allow-all";


Nit: any specific reason for the prefix "actually"? 🙂 Should we use some other name like allow-all-with-policy?

i named it this on discovering that allowAll does not actually allow everything if you have a policy enforcer configured because it does not set policies. I considered just fixing allowAll to be able to set the NoRestrictionPolicy, on the other hand there is probably limited scenario where you would have both allowAll and also a policy enforcer configured, and this has a somewhat ugly side-effect of setting a policy always (and so making a restricted datasource on the query) even if you don't have policy enforcement or policies of any sort, which seems a bit odd for out of the box behavior, so I did this instead.

Part of the purpose of the enforcer mechanism is to allow people to verify that allowAll is not being used by accident. So, adding a no-restriction policy to allowAll wouldn't be a "fix", it would partially defeat the purpose of the enforcer. It's fine to have an option for tests though that does this.

i changed things so that allowAll itself can have an optional policy defined, so deleted this stuff since is no longer necessary

kfaraz · 2025-11-14T13:53:12Z

+import java.util.List;
+import java.util.Map;
+
+public class AllowAllWithPolicyAuthResource implements EmbeddedResource


Please add a short javadoc.

kfaraz · 2025-11-14T13:54:24Z

+      final MapBinder<String, Authenticator> authenticatorMapBinder = PolyBind.optionBinder(
+          binder,
+          Key.get(Authenticator.class)
+      );
+      authenticatorMapBinder.addBinding(AUTH_TYPE)


Nit: can be chained.

Suggested change

final MapBinder<String, Authenticator> authenticatorMapBinder = PolyBind.optionBinder(

binder,

Key.get(Authenticator.class)

);

authenticatorMapBinder.addBinding(AUTH_TYPE)

PolyBind.optionBinder(binder, Key.get(Authenticator.class))

.addBinding(AUTH_TYPE)

guilty, i just copied these from other auth modules 🙃

kfaraz · 2025-11-14T14:00:02Z

    return msqControllerTasks;
  }

+  private Query<?> maybeApplyPolicy(DataSchema dataSchema, Query<?> query)


Nit: Please add a short javadoc too.

Suggested change

private Query<?> maybeApplyPolicy(DataSchema dataSchema, Query<?> query)

private Query<?> maybeApplyPolicyOnDatasource(DataSchema dataSchema, Query<?> query)

i didn't think this was needed since Policy apply to datasources so it seemed redundant, but can rename if you feel like it isn't obvious

this is changed now to just be getInputDataSource and is used by the 'build' query methods directly to create the DataSource instead of rewriting the query later

kfaraz · 2025-11-14T14:06:43Z

+           .addCommonProperty("druid.auth.authenticatorChain", "[\"test\"]")
+           .addCommonProperty("druid.auth.authenticator.test.type", "actually-allow-all")
+           .addCommonProperty("druid.auth.authorizers", "[\"test\"]")
+           .addCommonProperty("druid.auth.authorizer.test.type", "actually-allow-all")


Instead of writing a new one, can't we use the allowAll authenticaator/authorizer policy itself alongwith enforcer.allowedPolicies config?

see #18741 (comment), allowAll doesn't work if you have an enforcer defined because it does not set any policy on the auth result (and doing so would have some undesirable side-effects)

kfaraz · 2025-11-14T14:08:01Z

  {
    final UpdateResponse updateResponse = cluster.callApi().onLeaderOverlord(
-        o -> o.updateClusterCompactionConfig(new ClusterCompactionConfig(1.0, 100, null, true, null))
+        o -> o.updateClusterCompactionConfig(new ClusterCompactionConfig(1.0, 100, null, true, CompactionEngine.MSQ))


Maybe keep both the test cases, native and MSQ.

… only auth stuff since no longer needed

PR apache#18741 flipped an AND to an OR, potentially causing policies from non-table-read authorizations to apply to a resource. This patch fixes the logic and also introduces a defensive check that only a single policy is in play per resource.

PR #18741 flipped an AND to an OR, potentially causing policies from non-table-read authorizations to apply to a resource. This patch fixes the logic and also introduces a defensive check that only a single policy is in play per resource.

clintropolis added the Bug label Nov 13, 2025

github-actions Bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Nov 13, 2025

fix msq compaction to apply security policies

675c565

clintropolis force-pushed the fix-msq-compaction-policy-stuff branch from bc82725 to 675c565 Compare November 13, 2025 20:27

clintropolis added the jacoco:skip label Nov 13, 2025

clintropolis added 2 commits November 13, 2025 13:30

trick style bot

80699b6

dependencies

5eac83f

github-actions Bot added the Area - Dependencies label Nov 13, 2025

clintropolis added 2 commits November 13, 2025 14:31

bots bots bots

a6388d2

fix test

0637446

kfaraz reviewed Nov 14, 2025

View reviewed changes

allowAll now can allowAll by optionally setting a policy, delete test…

6dd22ab

… only auth stuff since no longer needed

github-actions Bot added the Area - Ingestion label Nov 14, 2025

clintropolis added 4 commits November 14, 2025 13:35

oops, cleanup

4ba10db

unused dependency

a686f0d

nicer

729c7cf

better name

0666516

kfaraz approved these changes Nov 17, 2025

View reviewed changes

gianm merged commit f5dde64 into apache:master Nov 18, 2025
103 of 104 checks passed

clintropolis deleted the fix-msq-compaction-policy-stuff branch November 18, 2025 00:24

gianm mentioned this pull request Nov 18, 2025

Fix logic error in policy application. #18753

Merged

kgyrtkirk added this to the 36.0.0 milestone Jan 19, 2026

	private Query<?> maybeApplyPolicy(DataSchema dataSchema, Query<?> query)
	private Query<?> maybeApplyPolicyOnDatasource(DataSchema dataSchema, Query<?> query)

Conversation

clintropolis commented Nov 13, 2025

Description

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gianm Nov 14, 2025 •

edited

Loading

clintropolis Nov 14, 2025 •

edited

Loading