RBAC Support - #6729

High-level pass (draft — I'll do the detailed review when marked ready).

The RFC #6422 structure lands cleanly. Things I like from a first read:

• Single source of truth for the access-control model. ROUTE_MIN_ROLE
  in rbac.rs is the entire policy, one line per (method, path), and
  min_role_for re-exports it to OpenAPI so the reference and the runtime
  cannot drift. This is exactly the right shape — reviewing a permissions
  change is now a one-diff-in-one-file operation.
• Deny-by-default with a test that enforces it. A registered /v0 route
  missing from the table is refused (fail closed), and
  every_registered_v0_route_is_classified fails the build when someone
  adds a new endpoint without classifying it. That is the correct default
  for auth, and the correct place to catch omissions.
• Systematic role-matrix test. every_route_admits_exactly_its_minimum_role
  is the check I would have asked for — every role × every classified route,
  both directions. Reverting a Role::* in the table or breaking satisfies
  can't slip through.
• Invariants at the schema. oidc_trust_owner_is_platform
  (tenant_id IS NULL ⇔ role = 'owner') and the partial UNIQUE on
  (name) for owner trusts make the "owner is platform-wide, others are
  tenant-scoped" split unforgeable at the DB rather than in Rust. The
  API-key scopes text[] → role text migration with the write ∈ scopes
  backfill preserves existing access without a compat shim.
• Role is a single total order (Read < Write < Admin < Owner via
  derived Ord), and satisfies is one comparison. No permission bitset,
  no matrix — the doc comment even calls out the ordering invariant. Good.

Open questions I'll want answered on the ready-for-review pass (no action
now, just noting so I don't forget):

1. Concurrent role revocation. If an admin demotes user X to read mid-
   request, the current request continues at the old role because the
   principal was resolved at auth-validate time. Is that intentional (bounded
   staleness, no per-request DB hit) or a gap to close?
2. Audit sink. audit() writes to tracing at info/debug — that is a
   reasonable start, but production auditability usually wants a structured
   sink that survives log rotation and can be replayed. Is the plan to
   promote this to a durable event log later, or is tracing the durable
   answer?
3. Owner trust caps. V35 says role is "capped at the creator's role at
   write time." Is that enforced in oidc_trust insert, or is a
   write-role user able to POST a trust with role: admin? Worth an
   explicit test.
4. Owner acting across tenants. The audit line already flags
   home_tenant != acting_tenant — is there a rate limit or explicit
   opt-in step, or does an owner silently switch tenants by header on
   every request?

All CI concerns (checks, migrations forward/backward) I'll cover on the
ready-for-review pass; nothing structural to change from what I can see.
Nice work.

Full pass on the ready-for-review tip (ae5664df).

The four questions from my draft review are addressed:

1. Role revocation vs in-flight requests — the role is resolved once per request in auth_validator/api_key_auth, so a revocation caught between validate and handler cannot revert an in-flight request. That is the accepted RBAC latency across most systems; worth an explicit note somewhere (rustdoc on AuthenticatedPrincipal?) so a future reader does not expect strict per-op enforcement.
2. Structured audit sink — audit now routes through tracing at debug, which is fine as an internal sink; a structured downstream target can subscribe to the audit: span key later without touching the enforcement path.
3. Role capped at creator's role — enforced at write time in three places (api_key.rs::post_api_key, api_oidc_trust.rs::post_oidc_trust, tenant.rs::check_grantable_role), all raising RoleExceedsCreator. Owner path is gated separately by requiring the caller be an owner (via ROUTE_MIN_ROLE) and by the CHECK constraint on the trust table. Good.
4. Owner cross-tenant switching — owner selects via Feldera-Tenant, resolved through resolve_owner_acting_tenant (name/UUID, never creates a tenant). A separate rate limit or opt-in header still seems worth doing eventually — an owner token being replayed to switch tenants at high rate would be indistinguishable from legitimate use here — but it is a defense-in-depth item, not a blocker for merge.

Full-pass verdict: LGTM. Notable strengths:

• ROUTE_MIN_ROLE + every_registered_v0_route_is_classified + no_stale_route_table_entries + every_v0_operation_documents_its_min_role is the right shape: policy, code, docs, and tests all triangulated. A new endpoint that skips classification fails the build, not production.
• Merging V35 into a single migration is the right call — the two migrations reference each other and running one without the other would leave the schema in a state RBAC does not tolerate. Existing installs still play forward cleanly (single V35 append, backfill preserves write).
• claim_matches was rewritten to an ordered-literal scan with a dedicated non-overlap test (literals_do_not_overlap) — that closes the earlier worry about pattern semantics.
• Model-test of OIDC trust + membership operations is exactly the coverage the draft was missing; the note on why it lives outside the StorageAction enum (stack budget) is well-earned.
• oidc_trust_owner_is_platform CHECK and the partial unique index on owner-trust names are both in the migration, so an application bug cannot produce an ill-shaped row.
• No AI-attribution trailers in any commit; rustdoc-fix commit is genuinely rustdoc-only (removes broken intra-doc links, wraps a bare URL, no behavior change).

Two minor observations (non-blocking, feel free to ignore):

• docs/roles.md line ~48 says an owner trust "outranks any tenant-scoped trust the same token also matches". That is a correct description of the code (match_oidc_trust returns both and the auth path prefers the owner match), but the operational implication — a workload that today runs at write in one tenant will silently upgrade to owner across the platform the moment someone registers a matching owner trust — is subtle. Worth restating in the security-implications box on that page in a follow-up, or possibly logging a warn! when an owner-trust match preempts a tenant-scoped match for the same token, so operators can see it happen.
• create_tenant/add_tenant_user fall back to provider = "manual" when no AuthConfiguration is attached. The comment says the owner routes are unreachable without auth, which is true today via ROUTE_MIN_ROLE. If the auth-none tenant bootstrap path is ever wired up, a "manual" provider will not match any real iss, so no login will resolve into a tenant created that way. Consider expect("AuthConfiguration present on any /v0 route") instead of a silent fallback, so a future regression breaks noisily.

Fixes the actix match_pattern() misclassification — turns out it was there all along. The bug: actix resolves the matched pattern by path only, so GET .../connectors/{connector_name}/completion_token was reporting POST .../connectors/{connector_name}/{action} as its template and getting denied as unclassified. Correct diagnosis and the right fix.

The new classify(method, path) walks ROUTE_MIN_ROLE bucketed by (method, segment_count) and picks the candidate with the most literal-segment matches. Method is part of the key, so GET .../start no longer sees the POST .../{action} entry. Literal-beats-placeholder tiebreak matches every router convention, and the every_table_entry_resolves_from_a_concrete_path meta-test is exactly the guard you want: the table itself is round-trip-checked, so any future entry whose shape is shadowed will fail the suite rather than silently 403 at runtime.

authorize now takes (method, routed, path, principal) and returns the matched pattern on success so the audit line still logs the template, not the concrete path. routed=false short-circuits so a 404 is still actix's job.

The integration test registers the placeholder route before the literal one (the actual failure ordering) and asserts both the token endpoint and the /{action} endpoint keep their own rules. Good.

Nit, not blocking: candidates_by_shape() allocates a Vec per shape at init and classify walks it linearly. For the current table size that's fine; if the table grows into the hundreds it's worth revisiting. The #[allow(clippy::type_complexity)] is the honest tell that the map value type is dense — a small struct Candidate { pattern, role } would read better than the tuple.

Carrying my APPROVE.

I will submit reviews progressively as I review

This review includes the api/

These are comments for the db module

APPROVE-carry.

Single new commit 25e19fc4 on top of the prior tip, applying Mihai's review feedback:

• run_auth_token_command now picks cmd /C on Windows and sh -c elsewhere. Legit: fda.exe ships on Windows but had no shell there. The three POSIX-syntax tests (printf, true, echo ... >&2; exit 3) are correctly gated #[cfg(unix)].
• The --auth-token-command examples switch from # shell comments (which clap paragraph-joined into one line) to backticked bullets — reads correctly now.
• Rename doc drops the misleading "asserts" clause and re-frames --displace-existing around atomicity, and a comment on the displaced response field records what it means. Good.

No functional change to RBAC or the tenant surface. Prior APPROVE stands.

APPROVE-carry.

Four new commits since 25e19fc apply Mihai's + lalithsuresh's review feedback across the RBAC surface:

• a18e3f14 [rbac surface]: ROUTE_MIN_ROLE drops the Option<Role> arm (no route sits at "any authenticated"), so lookup/middleware/OpenAPI shed a case they never took; new invariant test pins that a state-changing route never admits read. Admin/owner audit moved to info (operators need those). Migration header rewritten to say what it does. displaced and the scope Option gain names that match their meaning.
• 7e80ff7a [owner is config-only]: owner is no longer mintable at runtime. New --owner-trusts / FELDERA_OWNER_TRUSTS accepts [{issuer,subject,audience?}]; the DB row for owner trust is gone, so oidc_trust_relationship.tenant_id becomes NOT NULL, the CHECK is role IN ('read','write','admin'), and the platform query parameter + Option<TenantId> scope disappear from the whole storage layer. Owner naming no tenant now acts in the default tenant (the automation-first-tenant path). GET /v0/config/owners reports configured owners; the admin page swaps its owner-trust panel for a config view.
• edd7f6dc [owner from login provider]: is_configured_owner || owner_trusts.admits on the browser login path too, closing the "write a trust for your own IdP and it does nothing" trap.
• 39acde4a [rbac demo]: exercises the new surface — workload token reaches tenant routes with no Feldera-Tenant header, GET /v0/config/owners, role: owner on create_oidc_trust refused as 400 (invalid variant) rather than 403.

Spot-checked:

• OwnerTrusts newtype + FromStr refuses empty issuer/subject at startup (fail-loud); wildcard patterns still go through claim_matches; no audience = accept any (documented, and there is a test for the empty audiences case).
• oidc_trust_auth: SSRF/DoS gate consults owner_trusts.names_issuer(iss) in addition to the DB, so a config-only owner issuer doesn't get shut out of key fetches; issuer match is exact (test covers the .evil.test suffix).
• bearer_auth (login path): only a provider-verified email may match an owners entry (email_verified == true), so an unverified email can't confer owner; owner_trusts.admits also consulted with iss/sub/aud from the login token.
• resolve_owner_acting_tenant strictly resolves by UUID or name — never creates a tenant, so a typo becomes 404 rather than crossing into the wrong tenant.
• Migration is idempotent: IF NOT EXISTS, pg_constraint lookup for the old auto-named UNIQUE, feldera.auth_issuer GUC fallback, rank-by-pipeline-count tiebreaker, <name> (<id>) for the losers.
• 4 new commits × 4 well-composed commit messages, no AI-attribution trailers, Signed-off-by on all. CI checks currently empty (probably queued or run recently); prior tip was green.

Nothing to block on. Mihai's inline nits on the earlier tips have all landed; lalithsuresh's docs suggestions on authentication/index.mdx from 07-27 are on a different set of files and don't fall in this delta.

Great work on this! the RBAC model and OIDC workload-identity trusts address several pain points for us. We're running Feldera with issuerTenant: true and EntraID federated through authentik as our IdP.

A few things we're excited about:

• OIDC trusts will let us replace static API keys in CI/CD with short-lived tokens
• The role hierarchy gives us much better control than the current flat Read/Write scopes
• Default-to-Read for new API keys is a good call

We'd also love to see per-pipeline permissions. We have pipelines with different sensitivity levels sharing a tenant, and currently any user with Write access can modify any pipeline. Ideally:

• User A can query a pipeline but not modify it
• User B has full read/write on that pipeline
• User C has no access to it at all
• API keys scoped to specific pipelines

We understand this is a bigger lift than tenant-level RBAC, but it would be valuable for teams running shared tenants with mixed workloads. Happy to discuss our use case in more detail if helpful.

Copy change reads well and matches actual behavior (login re-adds; provider revoke disables). The rbac_up.sh fix using ${arr[@]+"${arr[@]}"} is the right idiom for bash 3.2 under set -u. LGTM.

APPROVE.

The Feldera-Tenant header fix (86ac9f8) is a real one and worth the churn. Three auth paths silently substituted a caller's only match when it named a tenant it wasn't trusted in — the branch that reads the selector was skipped whenever the selector was redundant, which is precisely when the check matters. The rewrite makes the rule uniform across all three: a header is always resolved and checked against the caller's authorized set, its absence keeps the previous "one match is unambiguous" semantics, and the derived-tenant path (issuer/sub fallback) now refuses a header that names anything other than what it derived. The .filter(|s| !s.is_empty()) on the header is quiet good hygiene — an empty header behaves like no header, not like a request to enter the empty-string tenant.

The test module in be1cdb3 is a big net gain — the previous single-manager 18-row script really could not have exercised most of what RBAC does, and the commit message honestly explains why (a token whose iss matches the login provider is verified as a login and never reaches the trust path). Three distinct issuers plus a rogue signer make wrong-key and wrong-issuer distinguishable, the module ordering by filename with serial execution is called out because pytest guarantees exactly that ordering, and the claim-pattern subject prefixing is a nice touch — the previous unfalsifiable-strict-case failure mode is a subtle bug that would only surface in review as "the strict test never runs and always reports the matcher as too permissive."

test_tenant_the_token_does_not_name_is_refused in test_3_negative.py directly pins the behavior the auth.rs commit fixes, which is the ideal shape.

The auto-fmt commit a7fce0d is noise (line-break rewraps in auth.rs and test_5_owner_revocation.py), zero semantic content.

Signed-off-by on both human commits, no AI trailers, mergeable clean.

LGTM

Re-approve at 5225e97 (five new commits since a7fce0d).

• ddc1066e (route matrix): the previous matrix could pass with a mistyped probe path because "not denied" is also what an unrouted path returns, and authentication precedes routing so the unauthenticated sweep is silent about whether a probe reached a route. Split into three tiers keyed to where auth/authz sit relative to routing: 401 has no routing evidence, 403 is evidence for authorized routes with denial-capable roles, and 200 via a parameterless route (GET /pipelines) covers URL composition for the read-only band where no role is denied and 404 would satisfy every assertion. Correcting the earlier comment about denial preceding routing is right — that mismatch was what let the doubled /v0 slip through last time. Nice recovery.

• a15706d7: fmt.

• 83701cb5: 5-line --allow-internal addition to rbac_up.sh so the demo registered trust can point at localhost. Matches the "restrict where a registered OIDC trust may point" hardening from 79688122.

• fac36bea (Monaco unmount race): real bug. MonacoEditorRunes.onMount awaits the loader (and the json contrib when jsonSchemas is set); a consumer that unmounts inside that window leaves the bound container null, and editor.create walks up from null and throws. Nothing awaits the onMount promise so the throw surfaces as an unhandled rejection after the test has already reported green — that's the "vitest all-green then exits non-zero" mode. Fix records teardown in onDestroy and checks it before touching the DOM. The new JSONDialog.svelte.spec.ts closes the dialog while the loader is in flight and confirms three rejections without the guard. Correct diagnosis, minimal fix.

• 5225e977: diagnostics. Previous CI failure on test_02_pipeline_survives_enabling_auth was a bare 401-vs-200 with no manager logs because the workflow's always() step dumped docker logs after the manager fixture removed its container. Moving the capture to a pytest report hook runs it while the fixture is still alive and attaches logs to the failing test rather than the job; raising RUST_LOG=auth,oidc=debug ensures the 401's actual refusal reason is in the captured log. Good instrumentation.

All five have Signed-off-by, no AI trailers. Single-purpose commits with prose that reads like the author already argued with themselves before writing.

APPROVE.

Two new commits since prior APPROVE @ 5225e977 (07-31 18:57Z).

db8fa85d (Gerd) adds a container-side TLS probe to platform_rbac/manager.py: on failure, pytest_runtest_makereport runs docker exec inside the manager container to print id, the SSL_CERT_FILE path, ls -l of the bundle, and two curls against the primary IdP's discovery URL — one with --cacert, one without. The point is that InvalidCertificate(UnknownIssuer) looks identical whether the bundle never reached the container or reached it and doesn't chain, and curl --cacert is the one output that separates them. Local runs get an empty string (no _container), so this is CI-only diagnostic weight — worth the ~30 lines. Probe URL comes from primary_idp fixture through the report hook, which is the right shape.

12c8e843 (Gerd) is the actual fix that motivated db8fa85d: --private-ca-cert-path didn't reach either OIDC fetch path. Two independent bugs behind that:

1. The federated JWKS path (oidc_http_client) was already on reqwest but only trusted the platform's rustls roots — the deployment's own CA had nowhere to go. Now CommonConfig::configured_root_certificates() reads the configured CA bundles once at ServerState::new (so authentication never touches the filesystem), and every fetch site threads &[Certificate] through: fetch_jwks_uri_from_discovery, fetch_issuer_jwks, generic_oidc_auth_config, JwkCache::get, fetch_jwk_keys, fetch_jwk_oidc_keys. oidc_http_client calls add_root_certificate for each — reqwest's rustls path merges rather than replacing, so a public IdP still verifies via the platform roots.

2. The login provider's JWKS fetch was on awc::Client::new(), which — as the commit message explains — silently loses SSL_CERT_FILE and the deployment's roots whenever pipeline-manager is co-built with a crate asking for rustls-0_23-webpki-roots (adapters does), because awc picks one root source at compile time and webpki-roots wins the feature vote. Building pipeline-manager alone kept native roots, so the two builds disagreed — exactly the kind of "works in dev, breaks in the release image" bug that eats afternoons. Fix: fetch_jwk_oidc_keys now goes through oidc_http_client(OidcDestination::OperatorConfigured, extra_roots), sharing the federated path's client. awc and its JsonPayloadError branches drop entirely from this file. Correct move — one client, one root policy, one place to reason about.

AuthError sheds JwkPayload and JwkContentType (awc-shaped) and JwkFetch swaps to reqwest::Error. A malformed key set now surfaces as JwkShape, a failed request as JwkFetch — clean rename. The existing invalid_url test passes &[] for extra roots, correct.

The RBAC suite now names the CA through --private-ca-cert-path (moved into _flags()) instead of SSL_CERT_FILE, which is what a real deployment with a private CA would do and which is what makes the suite regress if this fix is reverted. _ca_bundle() and its SYSTEM_CA_BUNDLES fallback are gone — they existed only because SSL_CERT_FILE replaces rather than adds, which is exactly the semantics --private-ca-cert-path now avoids.

Both commits Signed-off-by, no AI trailers. HTTPS, RBAC and OIDC trust job is green — this fix is exactly what makes it green. The failing Platform Integration Tests job on the tip is unrelated to the auth changes (I can't fetch the log from here to confirm, but the diff has no plausible interaction with the OSS Docker integration matrix; treating as flake unless it recurs). Full APPROVE.

Two new commits since prior APPROVE at 12c8e84 (07-31 21:55).

a1222ba (gz) supersedes the previous "Trust the configured CA when fetching OIDC keys" commit with the same title. The Rust code (api/main.rs, auth.rs, config.rs, oidc/fetch.rs) is byte-identical to the version I already approved. The only substantive delta is in python/tests/platform_rbac/manager.py: the _ca_bundle() helper and its SYSTEM_CA_BUNDLES constant are gone (they merged the suite's CA with the system store into a bundle passed via SSL_CERT_FILE), the manager is now pointed at the private CA directly via --private-ca-cert-path={self.ca_cert}, and the SSL_CERT_FILE env entry is dropped from start(). container_tls_view is retargeted to what the container sees of the CA it was pointed at (path + ls -l + curl --cacert) rather than what it sees of a bundle stitched into SSL_CERT_FILE. This is the right cleanup — it matches how a real private-CA deployment configures the manager and keeps the regression signal you built in earlier: revert the fix and discovery stops verifying, the suite fails.

22f9d9f (gz) is a CI-only reliability fix. .github/workflows/ci.yml's single cancel-if-tests-integration-platform-failed sentinel watches the caller-side invoke-tests-integration-platform job, and that stands in for the whole called workflow — it reports failure only after every job inside finishes. So a fast-failing HTTPS/RBAC job would leave oss-platform-tests holding a GKE runner for hours. Adds three per-job sentinels inside test-integration-platform.yml (cancel-if-manager-no-network-failed, cancel-if-manager-https-rbac-failed, cancel-if-oss-platform-tests-failed) that each needs: exactly one target job, if: failure(), and POST a cancel against github.run_id — which inside a called workflow names the caller's run, so the cancel drops the whole run rather than just this file. The scheme is now documented next to ci.yml's existing sentinel comment so anyone adding a job in a called workflow knows to add one here too. Sensible, minimal, well-commented.

Both Signed-off-by, no AI trailers.

Fresh APPROVE (upgrading prior APPROVE @ 12c8e84).