feat(edgraph): add an AccessController and separate identity from tenancy by matthewmcneely · Pull Request #9813 · dgraph-io/dgraph

matthewmcneely · 2026-08-20T12:32:32Z

Description

Dgraph derives three different answers from one JWT parse: who is calling, which tenant the
request operates in, and what the caller may do. x.AttachJWTNamespace is where they meet —

if !WorkerConfig.AclEnabled { return AttachNamespace(ctx, RootNamespace) }
ns, err := ExtractNamespaceFrom(ctx)          // verifies the ACL-signed JWT
if err == nil { ctx = AttachNamespace(ctx, ns) }   // on error: leaves the client's value
return ctx

— and three things follow from that coupling.

Tenancy requires ACL. With ACL off every request is clamped to namespace 0, so there is no
supported "multi-tenant, external login" configuration. --limit shared-instance already documents
expecting an access JWT "constructed outside dgraph", so this generalizes a shipped mode rather
than inventing one.

Tenancy lives in two places. The namespace is in both md["namespace"] (Go context metadata) and the JWT claim.
Authorization reads the claim (authorizePreds), storage reads the metadata (x.ExtractNamespace,
~25 sites). They agree only because one is copied from the other.

Resolution is trusting. On a parse failure the function leaves whatever namespace the context
already carried, which on the server side is entirely client-controlled. Authorization catches it
downstream today, so this is an invariant held by call-site ordering.

This PR gives each concern its own seam, with the built-in implementations being today's bodies
moved verbatim. There are also two independent bug fixes up front that were in the way.

A performance consequence worth stating on its own. A /query under ACL currently verifies its
access JWT three to four times: AttachJWTNamespace, authorizeQuery → validateToken, audit's
getUser, and again per-forwarding-hop where worker re-parses on a second machine to recover a
number the sender already knew. After this PR it verifies once, at the edge, and the resolved
namespace is forwarded. For RS/PS/ES that is asymmetric crypto removed from every RPC.

Commits. One PR, four commits, each building and testing standalone — read in order, they are
much easier to review than the squashed diff.

Commit	What	Size
`fix(x)`	Two independent fixes: value-lock reserved predicates by prefix, and use the derived context in `ExpandEdges`	5 files, +461
`feat(x)`	`TenantResolver` seam + the ~13 call sites, `AttachTrustedTenant`, `ClearIncomingNamespace`	13 files, +830
`feat(x)`	`Principal` + `Authenticator` + the gRPC/HTTP interceptors + a `ConfigureIdentity` boot hook	8 files, +664
`feat(edgraph)`	`AccessController`: `AuthorizeCapability` / `AuthorizePredicates`, `CapabilitySource`, and the 14 reclassified call sites	9 files, +1605

Two things to look at specifically.

Principal deliberately has no namespace field. Tenancy has exactly one home — md["namespace"],
because it must survive the hop to Zero's UID rate limiter and to the group-1 leader — and a second
copy on the Principal would recreate the divergence the split exists to remove. The Principal
travels as a context.Value and never as metadata: incoming metadata is client-controlled, and the
internal worker port has no interceptor chain at all, so a metadata-borne principal would be
attacker-supplied on any cluster whose internal port is reachable.

AuthorizePredicates returns a set rather than an error because authorizeQuery's real contract
is to silently drop unauthorized predicates and rewrite the query, not to reject it — and a nil
Allowed distinctly means "all" rather than "none". A plain error collapses both.

Behavior with no resolver, authenticator, or capability source installed is unchanged, which is
every build in this repo. x/tenancy_test.go pins that by keeping a copy of the old
AttachJWTNamespace body as an oracle and requiring both to agree across ACL on/off, JWT
absent/malformed/valid, and namespace pre-attached or not. x/authn_test.go does the same for
identity extraction against edgraph.validateToken.

Deliberate changes that are not behavior-preserving, all in the last commit:

authSuperAdmin returns Unauthenticated when the namespace claim cannot be parsed, instead of
continuing as namespace 0. That was an escalation with ACL off and a demotion with it on.
AssignUids moves to a new CapLeaseUIDs rather than cluster admin. dgraph live against an
ACL-off cluster reaches it through xidmap, where AuthSuperAdmin returned nil unconditionally;
requiring cluster admin to lease a UID block would break the loader under any policy that
actually authorizes.
breakGlassSource grants capabilities to a caller that already satisfies hasAdminAuth (the
--security IP whitelist plus token). Not new authority — those endpoints already accept it — but
it is a path to cluster admin that does not require the ACL secret to be live. It deliberately does
not grant when ACL is on, so an ACL cluster's authorization is untouched.
/health reports multi_tenancy separately from acl. They were derived from the same
AclSecretKey check, which is already wrong for shared-instance.

Removed from x: AttachJWTNamespace, AttachJWTNamespaceOutgoing, and ExtractNamespaceHTTP,
each replaced by a variant that returns an error. Flagging it since they are exported, though x is
not a documented API surface and there are no remaining in-tree callers.

Testing

41 new test functions, +2094 test LOC. Three worth calling out, because each was written after
checking that the obvious version of it passes with the fix reverted:

edgraph/capability_test.go walks the repo with go/ast and pins every call site to its
capability, so a reclassification or a new call fails a test rather than passing review unnoticed.
ExpandEdges now calls ProcessTaskOverNetwork through a package-level var, purely so the test
can assert each edge's namespace actually arrives there. Without that seam the fix had no failing
test, which was the case as originally written.
edgraph/reserved_predicate_guard_test.go drives the mutation-path guard directly with a synthetic
namespace carrying both an exact and a prefix value lock, since a prefix lock that the guard does
not consult means those predicates are writable by anyone through plain /mutate.

Per-commit, all four green: x, edgraph, query, audit, worker, graphql/resolve,
graphql/admin, dgraph/cmd/alpha. go vet ./... output is line-for-line identical to
upstream/main (61 pre-existing copylocks findings, same set, only line numbers shifted).

Checklist

The PR title follows the
Conventional Commits syntax, leading
with fix:, feat:, chore:, ci:, etc.
Code compiles correctly and linting (via trunk) passes locally
Tests added for new functionality, or regression tests for bug fixes added as applicable

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

… context in ExpandEdges Two independent fixes, both standalone. **Value locks by prefix.** ReservedNamespace can already admit dynamically-named predicates by prefix, but a value lock could only name them exactly — so a namespace whose predicates are created at runtime had no way to protect them. They were creatable via Alter and writable by anyone through /mutate, because the guard had no entry to match. ValueLockedPrefixes closes that: the registry keeps the exact-name map and a prefix list, and ReservedPredicateValueLock consults the exact names first so a specific predicate can be pinned to a different owner than the prefix it sits under. An empty prefix is rejected, mirroring the guard PredicatePrefix already has: it would match every predicate in the cluster and lock them all to one marker. Duplicate prefixes panic at registration for the same reason exact duplicates do — import order would otherwise decide which owner wins. **ExpandEdges.** Both x.AttachNamespace calls discarded the returned context, including one in a deferred "reset". metadata.FromIncomingContext returns a copy, so neither did anything, and the comment's promise that "further query proceeds as if made from the user of 'namespace'" was false. In galaxy mode a `S * *` delete built its predicate list for the edge's namespace while getNodeTypes read dgraph.type from the request's, resolving the expansion against the wrong schema. Latent today because the only in-tree galaxy producer is the live loader, which sends only SET. getNodeTypes reaches worker.ProcessTaskOverNetwork, so ExpandEdges now calls it through a package-level var and the test asserts each edge's namespace arrives there. Without that seam the fix had no test that failed when it was reverted, which was the case as originally written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

AttachJWTNamespace was the single place Dgraph derived a request's namespace, and it did so by parsing the ACL-signed access JWT. Three consequences followed from that one coupling. Tenancy required ACL: with ACL off every request was clamped to namespace 0, so there was no supported "multi-tenant, external login" configuration — even though --limit shared-instance already documents expecting an access JWT "constructed outside dgraph". Tenancy lived in two places. The namespace was in both md["namespace"] and the JWT claim, and authorization read the claim while storage read the metadata. They agreed only because one was copied from the other. And resolution was trusting: on a parse failure the function left whatever namespace the context already carried, which on the server side is entirely client-controlled. x/tenancy.go makes the decision pluggable — TenantResolver plus SetTenantResolver, mirroring the RegisterReservedNamespace pattern already in x — and converts the call sites to take the resolver's error instead of continuing on a context whose namespace could not be derived. The built-in resolver is the old body verbatim, so an unconfigured build is bit-identical; x/tenancy_test.go pins that by keeping a copy of the old implementation and requiring both to agree across ACL on/off, JWT absent/malformed/valid, and namespace pre-attached or not. AttachTrustedTenant is how in-process code that holds no request credential attributes its own context: GetGQLSchema, UpdateGQLSchema and the schema-bootstrap path all build a context by hand and would otherwise be refused. The marker is a context value, never metadata, so it cannot arrive over the wire. ClearIncomingNamespace is the counterpart for entry points. md["namespace"] has no standing on an inbound request, and the built-in resolver leaves it in place when it cannot derive a namespace — so the zero-proxy UID lease path, converted here, would otherwise honour whichever tenant an uncredentialed caller named. It is applied at entry points only; a continuation of an already-resolved request must keep what it was attributed. CommitOrAbort's trace annotation moves to the resolved namespace. It is a span attribute that ignores its own error, so it is free, and the namespace a reader wants on the span is the one the transaction actually ran in. Also reports multi_tenancy separately from acl in the feature list. They were derived from the same AclSecretKey check, which is already wrong for shared-instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The counterpart to the tenant seam. Dgraph read three things out of one JWT parse — who is calling, which tenant they are in, and what they may do — and the only way to learn an identity was extractUserAndGroups against the ACL public key. Any abstraction over authorization would therefore have hard-coded Dgraph's own ACL as its identity source. x.Principal is the verified answer to "who is calling": issuer, subject, groups, the remaining claims, and how it was authenticated. It deliberately has no namespace field. Tenancy has exactly one home — md["namespace"], because it must survive the hop to Zero's UID rate limiter and the group-1 leader — and a second copy on the Principal would recreate the divergence the split exists to remove. It travels as a context value, never metadata. Incoming metadata is client-controlled, so a metadata-borne principal would be forgeable, and the internal worker port has no interceptor chain at all — anything arriving there is unauthenticated by construction. It never needs to cross a process boundary either: authorization is decided once, at the edge. x.Authenticator makes verification pluggable, defaulting to the built-in ACL one, which is validateToken's identity extraction minus the namespace claim. ACLAuthenticator is exported so a deployment installing its own can compose with it rather than displace it: Login is how an ACL token is obtained, so an authenticator that cannot also verify one takes away the cluster's ability to log in, and the failure surfaces as an authorization error somewhere unrelated. The interceptors never reject. That contract is what removes the need for an unauthenticated-endpoint allow-list: Login, health checks and CheckVersion cannot present a credential. A rejecting interceptor would need to enumerate them — a second policy engine in front of the one that already knows which operations require authentication — and an incomplete list fails closed on exactly the endpoints that would let an operator notice. Rejection stays where it already returns the right codes. ConfigureIdentity is the boot hook for installing all of this, called after the config is parsed and before any listener serves. It is separate from the service-registration hooks because what it installs is process-wide. Ordering the interceptors ahead of audit is safe because they never reject, and it lets audit read the resolved principal instead of verifying the token a second time purely to recover a username. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The last of the three concerns. Identity and tenancy now have seams; authorization was still 14 direct calls to two ACL-specific helpers, spread across edgraph and graphql/resolve, each deciding policy inline. **Two methods, because one cannot express what the ACL path already does.** AuthorizeCapability answers "may this caller do this class of thing" — CapClusterAdmin, CapTenantAdmin, CapAssumeTenant, CapLeaseUIDs. AuthorizePredicates answers "which of these predicates may they touch", and it must return a set rather than an error: authorizeQuery's real contract is to silently drop unauthorized predicates and rewrite the query, not to reject it, and a nil Allowed distinctly means "all" rather than "none". A plain error return would collapse both. **The capabilities are what the call sites meant, not what they said.** All 14 called AuthSuperAdmin or AuthorizeGuardians, so the distinctions were invisible: - CapAssumeTenant for the two galaxy-operation sites. Behaviour-identical under the built-in policy, which requires cluster admin for it, but it lets a deployment grant cross-tenant write without granting namespace lifecycle. - CapLeaseUIDs for AssignUids. Not a new grant — it is a fix. The path is reachable with ACL off, where AuthSuperAdmin returned nil unconditionally, so `dgraph live` worked; under any policy that actually authorizes, requiring cluster admin to lease a UID block would break the loader. The capability is what a bulk writer needs and nothing more. edgraph/capability_test.go pins the mapping with a go/ast walk over the repo, so a site that changes capability, or a new call that adds one, fails a test rather than passing review unnoticed. It also asserts the two graphql/resolve sites are still there — they are the only callers outside edgraph, and so the evidence that the abstraction is reachable from a second package rather than an internal rename. **CapabilitySource is how a capability is granted without an ACL user.** An ordered list, first grant wins, with the built-in policy last. breakGlassSource is registered unconditionally and grants everything to a caller that already satisfies hasAdminAuth — the IP whitelist plus --security token. That is not new authority: those endpoints already accept it. What it adds is a path to cluster admin that does not depend on the ACL secret being live, which matters because with the default HS256 the ACL verify key IS the signing key, so "ACL on, just to mint admins" keeps the largest credential in the deployment active for an unrelated job. It deliberately does not grant when ACL is on, so an ACL cluster's authorization is unchanged by this commit. **The two claim-reads are deliberate, and are now documented as such.** authSuperAdmin's ns != 0 test and filterTablets both read the namespace from the signed claim rather than the resolved one, which looks like exactly the two-channel divergence the tenant seam exists to remove. It is not: both are asking "who is this caller", not "which tenant does this request touch". State reaches filterTablets with no ResolveTenant ahead of it, so the resolved channel there is only ever what the caller sent — reading it would let a guardian of any tenant ask for namespace 0 and skip filtering entirely. Same shape in authSuperAdmin, where a caller who can influence resolution could otherwise promote themselves. AuthorizePredicates is the one that keys on the resolved namespace, because that question really is about the data the request touches. What does change is the failure mode: authSuperAdmin returns Unauthenticated when the claim cannot be parsed, rather than continuing as namespace 0 — an escalation with ACL off, and a demotion with it on. **extractUserAndGroups prefers the resolved principal**, so a gRPC request under ACL verifies its JWT once at the edge instead of once in the interceptor and again here. For RS/PS/ES that is asymmetric crypto per RPC. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The comment named AllocateIDs among the operations that gate on CapClusterAdmin. It does not, and the distinction is the reason CapLeaseUIDs exists: `dgraph live` leases a UID block through AllocateIDs on every run, including against ACL-off clusters where it has never presented a credential, so routing it through the tightened capability would have broken the loader. Server.AllocateIDs passes CapLeaseUIDs, which is still authSuperAdmin and still open with ACL off. Left as written, the comment described the pre-split state in the present tense, right next to the paragraph explaining which operations the ACL-off change closes — so a reader would reasonably take it as the contract and conclude the loader path was tightened too. Also names validateAlterOperation as the hasAdminAuth caller the "no stricter than schema change" argument rests on, and that it consults the same HasWhitelistedIP-plus-hasPoormansAuth pair as break-glass. That parity was asserted; it is checkable, and pointing at the function makes it so. Comment-only; no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The guards this PR adds are unpinned by its own tests. Removing all four x.ClearIncomingNamespace calls leaves ./x ./edgraph ./worker ./query and ./graphql/... green — verified by doing it — because the only coverage, x/tenancy_test.go, exercises the function directly rather than any call site. That matters most at forwardAssignUidsToZero, which has no other gate. The tenant seam is what makes md["namespace"] reachable there at all: the built-in resolver tolerates a credential it cannot parse and leaves whatever namespace the context already carried, which on the server side is client-supplied. So omitting the guard would introduce uncredentialed cross-tenant UID leasing, and nothing in the suite would have said so. Behavioral, all four entry points. Each drives a request that names namespace 9 while presenting no credential, through a fail-closed resolver installed with the public x.SetTenantResolver seam, and asserts the resolver was never shown the caller's namespace. No production change and no cluster is needed: ResolveTenant is the first thing that can fail in each path, so the refusal returns before worker/posting or groups().Leader(0) is reached. Structural, because behavior cannot see where the call sits. Moving the guard from Server.Query down into QueryNoGrpc keeps every behavioral test passing — from outside the entry point the namespace still never reaches the resolver — while breaking the two HTTP handlers and GetGQLSchema, which arrive already attributed and hold no credential to re-present. That mutation is why the go/ast layer exists; it is the one case the behavioral layer is blind to. Same shape as edgraph/capability_test.go, and it pins both directions: a missing call is an escalation, an added one in a shared continuation strips tenancy from trusted in-process callers. Verified against five separate mutations. All four deletions fail both layers; the move fails the structural layer alone. Test-only; no production file changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The ACL-off cluster-admin tightening shipped with no integration coverage, and not by oversight: dgraphtest hard-codes --security=whitelist=0.0.0.0/0 for every cluster it builds and none of the ClusterConfig options override it, so no test in this repo could construct a cluster where an IP-whitelist denial was even possible. The unit test has to fabricate a peer context with fromIP("203.0.113.7") for exactly that reason. This is the harness gap, closed. WithWhitelist replaces the default rather than adding to it. The default is 0.0.0.0/0, so an additive option could never express a cluster that denies anyone, which is the only configuration worth testing. The pre-v21 --whitelist branch is threaded too, so the option is not silently inert under WithVersion("v20..."). Start() needed a second probe to make it usable. Its GraphQL readiness check is an admin mutation, and every admin GraphQL operation carries IpWhitelistingMW — login included — so on a cluster whose whitelist excludes the test process it can never succeed and the cluster never comes up. waitUntilGraphqlProbe waits on /probe/graphql instead: no whitelist middleware, no auth, and answering "is GraphQL serving yet" is its whole purpose. This is load-bearing, not tidying — without it all three tests below die in c.Start() on "unauthorized ip address". Three tests, covering both halves of break-glass and a control. Each denial asserts the message authorizeClusterAdmin produces on its ACL-off branch, not merely PermissionDenied, because a bare PermissionDenied could come from anywhere — and the reason is not otherwise observable, since breakGlassSource calls HasWhitelistedIP and hasPoormansAuth directly rather than through hasAdminAuth precisely to avoid logging on every capability check. The whitelist pair is self-checking: the restrictive cluster is denied and the open one is granted, and the whitelist spec is the only difference between them. Opposite outcomes from one changed variable is what rules out the caller's source address being quietly admitted, which would otherwise make the restrictive test pass vacuously. Existing tests are unaffected: the emitted flags are byte-identical when the default applies, and the only ClusterConfig literal in the repo is inside NewClusterConfig, so all 71 callers get the default and none can reach an empty whitelist. Verified by running against an image and bind-mount binary rebuilt from this branch — the ones already on disk predated the change and would have proved nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…eted An earlier commit on this branch reverted upstream changes to three dgraphtest files. The damage was not in the intended change but in how it was applied: the files were overwritten wholesale from another tree rather than patched, so every upstream edit to them since that tree diverged was silently undone. What went missing, all from #9788: - zero.env(), returning DGRAPH_ZERO_SECURITY=whitelist=0.0.0.0/0 - alpha.env(), and env(*LocalCluster) on the dnode interface - the Env: dc.env(c) wiring in createContainer - SPDX copyright years in two files, regressed 2026 -> 2025 Zero guards /moveTablet and /removeNode with adminAuthHandler(strict=true), so they are protected whether or not --security is configured: with neither a token nor a whitelist, only a loopback caller is admitted. Deleting the env var left the harness unable to reach those endpoints, which is what made TestUniqueMultipleGroups fail on an assertion about tablet placement while the real cause was authorization. Measured rather than inferred: DGRAPH_ZERO_SECURITY appears 3 times in the log of the run that passed and 0 times in the run that failed. The three files are now byte-identical to upstream/main apart from the two lines this branch means to change, which is the whole of the intended diff: - acmd = append(acmd, `--whitelist=0.0.0.0/0`, "--telemetry=false") - security := `--security=whitelist=0.0.0.0/0` Also corrects WithWhitelist's doc comment, which claimed Zero keeps a hard-coded open whitelist. Zero reads its own from the environment, and the comment now says so and says why an env var is used instead of a flag: an older zero binary in an upgrade test would fail to start on an unrecognized one. Verified by auditing every deleted line on the branch rather than by building: the only deletions in dgraphtest/ are the two above, config.go and local_cluster.go delete nothing, and x/x.go deletes exactly the three helpers this work replaces with IsIpWhitelisted and the loopback case intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

matthewmcneely requested a review from a team as a code owner August 20, 2026 12:32

matthewmcneely requested review from shiva-istari and shivaji-kharse August 20, 2026 14:03

matthewmcneely and others added 7 commits August 25, 2026 15:21

matthewmcneely force-pushed the matthewmcneely/accesscontroller-abstraction branch from 178ddf5 to 8bdcc90 Compare August 25, 2026 19:23

This comment has been minimized.

Sign in to view

matthewmcneely mentioned this pull request Aug 27, 2026

fix(dgraphtest): give Zero the admin whitelist the Alpha already gets #9818

Closed

3 tasks

Navigation Menu

Uh oh!

feat(edgraph): add an AccessController and separate identity from tenancy - #9813

feat(edgraph): add an AccessController and separate identity from tenancy#9813
matthewmcneely wants to merge 8 commits into
mainfrom
matthewmcneely/accesscontroller-abstraction

matthewmcneely commented Aug 20, 2026 •

edited

Loading

Uh oh!

This comment has been minimized.

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

matthewmcneely commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

matthewmcneely commented Aug 20, 2026 •

edited

Loading