oscerd commented on code in PR #737:
URL: https://github.com/apache/camel-karaf/pull/737#discussion_r3870712111
##########
features/src/main/feature/camel-features.xml:
##########
@@ -315,9 +315,27 @@
<condition>shell</condition>
<bundle>mvn:org.apache.camel.karaf/camel-karaf-shell/${project.version}</bundle>
</conditional>
- <!-- allow camel to access its own mbeans for karaf commands and other
needs -->
+ <!--
+ JMX role mapping for the org.apache.camel MBean domain.
+
+ Karaf resolves ACL PIDs most specific first and only falls through
to the
+ next one when no rule in the current PID matches the operation, so
this
+ file needs to list only the operations that should differ from the
stock
+ jmx.acl defaults (get*/list*/is* = viewer, everything else =
admin).
+
+ Context and route lifecycle is mapped to manager, matching how
Karaf maps
+ bundle lifecycle in its own jmx.acl.org.apache.karaf.bundle.
Everything not
+ listed here - message injection (sendBody*, requestBody*),
endpoint and
+ route mutation (createEndpoint, removeEndpoints,
addOrUpdateRoutes*) and
+ the dump* operations, which resolve property placeholders - falls
through
+ to the jmx.acl default of admin.
+ -->
<config name="jmx.acl.org.apache.camel">
- * = *
+ start* = manager
+ stop* = manager
+ suspend* = manager
+ resume* = manager
+ reset* = manager
Review Comment:
`browse*` is in as suggested, and `reset* = manager` was already there. I
kept the committed suggestion and refined `dump*` on top of it in `d0338f290`,
because as a wildcard it grants more than the read-only access you were asking
to restore.
Reading the MBeans is what changed my mind about the shape:
- `dumpRoutesAsXml` has four overloads and `dumpRoutesAsYaml` five, plus the
`ManagedRouteMBean` twins. The **first** boolean is `resolvePlaceholders` in
every family, and every no-arg form passes `false` — from the bytecode,
`dumpRoutesAsXml()` calls `dumpRoutesAsXml(false, true)`. So the no-arg dumps
are safe and the boolean overloads are the ones that can print resolved
credentials.
- `dump*` also matches
`ManagedBacklogTracerMBean.dumpAllTracedMessagesAsXml()` /
`dumpTracedMessagesAsXml(String)`, which return **traced message bodies**, and
`ManagedDumpRoutesStrategyMBean.dumpRoutes(String)`, which **writes files**.
My first instinct was to keep your wildcard and pin the dangerous arguments
(`dumpRoutesAsXml(boolean)[true] = admin`). Karaf does support that —
`ACLConfigurationParser.getExactArgSignature`, with regex matching on the
values — but it **fails open**: an overload added by a future Camel matches no
argument rule, falls back to `dump*`, and is granted to `viewer`. Repackaging
each new Camel release is precisely what this repository does, so that is a
routine event, not a hypothetical, and it would reopen the credential path
silently on a version bump.
Listing the safe operations fails the other way: an unrecognised operation
stays at Karaf's `admin` default until someone looks at it.
Verified rather than reasoned about — I ran the mapping through Karaf
4.4.8's own `ACLConfigurationParser`, driven the way
`KarafMBeanServerGuard.getRequiredRoles` drives it (domain PID first, fall
through to the stock root `jmx.acl` on `NO_MATCH`). 24 expectations, all
holding:
```
dumpRoutesAsXml() -> [viewer]
dumpRoutesStatsAsXml(false,true) -> [viewer]
dumpRoutesAsXml(true) -> [admin]
dumpRoutesAsYaml(true,true,true) -> [admin]
dumpAllTracedMessagesAsXml() -> [admin]
dumpRoutes(String) [writes files] -> [admin]
dumpRoutesAsXml(b,b,b,b) [future] -> [admin] <- the fail-open case
getCamelId / listRoutes -> [viewer]
sendStringBody(String,String) -> [admin]
```
**One thing I would rather you ruled on than have me decide quietly.**
`browse* = viewer` grants `browseMessageBody(Integer)` and
`browseAllMessagesAsXml(Boolean)` — live **message bodies** — to the monitoring
role. That is the same argument I just made about credentials, applied to
payloads rather than configuration. I have left it as you suggested because it
is genuinely the read-only inspection surface and it is your call, not mine;
but if you would rather `viewer` saw inflight *counts* and not message
*content*, the split would be `browse()` on the inflight/await managers at
viewer and `ManagedBrowsableEndpointMBean.browse*` left at admin. Happy either
way — say which and I will make it match.
_Claude Code on behalf of Andrea Cosentino_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
