security: add authorization checks to MetricsHandler#16186
Open
adilburaksen wants to merge 1 commit into
Open
Conversation
The /v3/metrics query and search endpoints selected metrics by caller-supplied namespace/app/program tags but never enforced access, so any authenticated user could read or enumerate another namespaces operational metrics. Inject ContextAccessEnforcer and enforce StandardPermission.GET on the namespace derived from the request tags before executing the query/search (and for each query in the batch path), mirroring the visibility checks the sibling LogHttpHandler performs in the same module. A cross-namespace (namespace-less) query now requires GET on the system namespace.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Studio metrics endpoints in
MetricsHandler(cdap-watchdog) select metrics bycaller-supplied
tagvalues (namespace,app,program, …) but never enforceauthorization, so any authenticated user can read or enumerate the operational metrics
of any namespace/app/program:
POST /v3/metrics/query→MetricsQueryHelper.executeBatchQueries/executeTagQueryPOST /v3/metrics/search→searchTags/searchMetricThe CDAP router authenticates but does not perform per-resource authorization — that is
the handler's responsibility. The sibling observability handler in the same module,
LogHttpHandler, already enforcesStandardPermission.GETon the target program beforereturning logs (
ensureVisibilityOnProgram);MetricsHandlerwas added without theequivalent check. Metrics are namespace-scoped —
DefaultMetricStoreaggregates on thenamespace tag — so this is a real cross-namespace authorization gap, not just a modeling
concern.
Fix
Inject
ContextAccessEnforcerand, before executing any query/search, derive the targetnamespace from the request tags and enforce
StandardPermission.GET:GETon thatNamespaceId;GETon the system namespace;ContextAccessEnforceris already bound in the metrics-query injector(
MetricsTwillRunnable/MetricsServiceMain/StandaloneMainall installAuthorizationEnforcementModule), so no wiring change is needed. This mirrors theexisting
security: add authorization checks to …handler hardening.Testing
I verified the gap and the enforcement points by source inspection (no
AccessEnforcerreference existed in
MetricsHandler, unlike its siblingLogHttpHandler). I don't havea local Apple/CI build environment to run the watchdog suite; happy to add a unit test
(e.g. asserting an unauthorized principal is rejected before the query executes) in the
style the reviewers prefer.