Skip to content

Require every UI API endpoint to declare its authorization - #9004

Open
GGraziadei wants to merge 2 commits into
apache:masterfrom
GGraziadei:enforce-ui-config-authorization
Open

Require every UI API endpoint to declare its authorization#9004
GGraziadei wants to merge 2 commits into
apache:masterfrom
GGraziadei:enforce-ui-config-authorization

Conversation

@GGraziadei

@GGraziadei GGraziadei commented Aug 23, 2026

Copy link
Copy Markdown
Member

Summary

  • AuthorizedUserFilter no longer skips authorization when a resource method carries no @AuthNimbusOp; such a request is now rejected.
  • Endpoints that intentionally have no operation gate declare it with the new @UnauthenticatedNimbusOp, so the choice is explicit and reviewable. /history/summary uses it, since Nimbus already filters that response for the authenticated remote user.
  • /cluster/configuration is gated on getNimbusConf, matching the ACL the direct Thrift path applies.
  • getNimbusConf masks credentials before serialization, so the Thrift path is covered as well as the UI proxy.
  • Adds the missing @Password markers to credential-bearing keys in Config.java: the cluster ZooKeeper auth payload, and the Thrift, Netty and ZooKeeper TLS store passwords. These were absent, so those values were also reaching daemon logs through the existing masking call sites.
  • Adds ConfigUtils.maskCredentials, used when a config is served over an API: it masks the annotated keys plus string values whose key name denotes a secret. Plugins read their own keys straight out of the config map, so those keys belong to no annotated field and the annotation scan cannot see them. Only string values are considered, so timeouts and class lists whose names merely mention credentials keep their value.

Compatibility

The filter is registered only for org.apache.storm.daemon.ui.resources, which holds a single resource class; all its endpoints declare one of the two annotations. A new endpoint added without either annotation is rejected until it declares one, which is the intent.

Testing

  • mvn -pl storm-client test -DskipITs — 663 tests, 0 failures
  • mvn -pl storm-webapp test -DskipITs — 99 tests, 0 failures
  • mvn -pl storm-server test -DskipITs — 491 tests, 0 failures
  • New tests cover the filter decision for gated, opted-out and undeclared endpoints, the annotation coverage of every API endpoint, and the masking of the served configuration.
  • Verified on a local cluster: with an authorizer that denies, /cluster/configuration returns 403 where it previously returned 200, /history/summary keeps working, and the served configuration masks the credential keys while leaving paths and timeouts untouched.

@GGraziadei GGraziadei self-assigned this Aug 23, 2026
@GGraziadei
GGraziadei force-pushed the enforce-ui-config-authorization branch from aba3c5d to 06a6cf0 Compare August 23, 2026 11:31
@rzo1 rzo1 added this to the 3.1.0 milestone Aug 23, 2026

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest sequencing behind #9003 and rebase once we have that in.

@GGraziadei
GGraziadei marked this pull request as draft August 23, 2026 18:41

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9003 is in - perhaps go for a rebase.

@rzo1
rzo1 marked this pull request as ready for review August 24, 2026 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants