oscerd commented on PR #737: URL: https://github.com/apache/camel-karaf/pull/737#issuecomment-5449764813
@jbonofre before this lands — one line in it contradicts the rationale the same PR documents, and I would rather raise it now than after the merge. The `security-model.adoc` bullet added here states the principle: > the overloads that take a `resolvePlaceholders` flag, **and the backlog tracer/debugger operations that return traced message bodies**, are left at Karaf's `admin` default, so an operation added by a future Camel release is denied rather than granted until it is reviewed The `dump*` side implements that faithfully. There is no `dump*` wildcard, so `dumpTracedMessagesAsXml`, `dumpTracedMessagesAsJSon` and `dumpTracedMessages` on `ManagedBacklogTracerMBean` / `ManagedBacklogDebuggerMBean` all fall through to `admin`, and so does every `(boolean…)` dump overload. That is the right shape, and it fails closed on overloads Camel adds later. Four lines above it, though: ``` browse* = viewer ``` `browse*` reaches the same class of data the bullet says is withheld, just off browsable endpoints rather than the tracer. From `ManagedBrowsableEndpointMBean`: | operation | what a viewer gets | |---|---| | `browseMessageBody(Integer index)` | the message body, no flag to suppress it | | `browseExchange(Integer index)` | the exchange | | `browseAllMessagesAsXml(Boolean includeBody)` | caller passes `includeBody=true` | | `browseMessageAsXml(Integer, Boolean includeBody)` | same | | `browseRangeMessagesAsXml(Integer, Integer, Boolean)` | same | So on any browsable endpoint in the container — `seda`, `mock`, JMS browse, `file` — a principal holding only `viewer` can read live message payloads. That is business data in flight, and it is a wider disclosure than the resolved property placeholders the rest of this commit closes. It is also a wildcard, so it fails *open*: a `browse*` operation added by a future Camel release is granted to `viewer` without review, which is precisely the property the bullet says the mapping avoids. Worth noting it was not in the original issue either — #728 was about the `* = *` blanket, and the read-only set under discussion was `list*` / `get*` / `is*` / non-placeholder `dump*`. Two consistent ways out, both fine by me: 1. **Drop `browse* = viewer`.** It falls through to `admin`, matching the tracer reasoning exactly, and the documented principle holds as written. My preference, on the grounds that the wildcard fails open. 2. **Keep it and amend the bullet** to say that message bodies *are* exposed to `viewer` through `browse*`, so operators reading the security model are not misled about what the shipped mapping grants. Everything else in the PR I checked against the real MBean interfaces: all 15 `dump*` signatures match actual operations, no typos, no dead rules, and `dumpRoutesAsXml()` delegates to `dumpRoutesAsXml(false, true)` so the no-arg forms genuinely do not resolve placeholders. --- _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]
