From 25a5bf4df11315beed2e1b69be8cc5cad6b0a71f Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Wed, 19 Aug 2026 18:45:44 -0400 Subject: [PATCH 1/8] fix(x): value-lock reserved predicates by prefix, and use the derived context in ExpandEdges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- edgraph/reserved_predicate_guard_test.go | 88 +++++++++++++ query/expand_edges_test.go | 153 +++++++++++++++++++++++ query/mutation.go | 33 +++-- x/keys.go | 73 +++++++++-- x/reserved_namespace_test.go | 136 ++++++++++++++++++++ 5 files changed, 461 insertions(+), 22 deletions(-) create mode 100644 edgraph/reserved_predicate_guard_test.go create mode 100644 query/expand_edges_test.go diff --git a/edgraph/reserved_predicate_guard_test.go b/edgraph/reserved_predicate_guard_test.go new file mode 100644 index 00000000000..6666606f5a1 --- /dev/null +++ b/edgraph/reserved_predicate_guard_test.go @@ -0,0 +1,88 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package edgraph + +import ( + "context" + "testing" + + "github.com/dgraph-io/dgo/v250/protos/api" + "github.com/dgraph-io/dgraph/v25/x" + "github.com/stretchr/testify/require" +) + +type guardTrustKey struct{} + +// guardTestNamespace registers a synthetic reserved namespace covering both lock +// styles: one exact predicate and one dynamic prefix. Registration is global and +// panics on a duplicate, so it runs once. +func init() { + x.RegisterReservedNamespace(x.ReservedNamespace{ + PredicatePrefix: "dgraph.guardtest.rel.", + Predicates: []string{"dgraph.guardtest.xid", "dgraph.guardtest.cfg"}, + ValueLocked: []string{"dgraph.guardtest.cfg"}, + ValueLockedPrefixes: []string{"dgraph.guardtest.rel."}, + TrustMarker: guardTrustKey{}, + }) +} + +// TestReservedPredicateGuardValueLocks covers the mutation-path guard for both +// exact-name and prefix value locks. The prefix case is what protects predicates +// created dynamically at runtime, which cannot be enumerated up front — for +// an authorization store that is every stored grant, so an unguarded prefix means they +// are forgeable through plain /mutate. +func TestReservedPredicateGuardValueLocks(t *testing.T) { + nq := func(pred string) *api.NQuad { + return &api.NQuad{Subject: "0x1", Predicate: pred} + } + + t.Run("untrusted context is denied", func(t *testing.T) { + guard := newReservedPredicateGuard(context.Background()) + + for _, pred := range []string{ + "dgraph.guardtest.cfg", // exact lock + "dgraph.guardtest.rel.document.owner", // prefix lock + "dgraph.guardtest.rel.never_seen.yet", // prefix lock, predicate not yet created + "dgraph.guardtest.REL.Document.Owner", // case-insensitive + } { + require.Errorf(t, guard(nq(pred)), "value-locked predicate %q must be denied", pred) + } + }) + + t.Run("trusted context is allowed", func(t *testing.T) { + ctx := context.WithValue(context.Background(), guardTrustKey{}, true) + guard := newReservedPredicateGuard(ctx) + + // This is the case an owning service's write path depends on: its in-process + // client sets the marker on the context it builds, so + // locking the relation prefix must not break WriteRelationships. + for _, pred := range []string{ + "dgraph.guardtest.cfg", + "dgraph.guardtest.rel.document.owner", + "dgraph.guardtest.rel.never_seen.yet", + } { + require.NoErrorf(t, guard(nq(pred)), "trusted writer must be allowed to write %q", pred) + } + }) + + t.Run("a false marker is not trusted", func(t *testing.T) { + ctx := context.WithValue(context.Background(), guardTrustKey{}, false) + guard := newReservedPredicateGuard(ctx) + require.Error(t, guard(nq("dgraph.guardtest.rel.document.owner"))) + }) + + t.Run("unlocked and unowned predicates pass", func(t *testing.T) { + guard := newReservedPredicateGuard(context.Background()) + + for _, pred := range []string{ + "dgraph.guardtest.xid", // owned but deliberately not locked + "dgraph.guardtest.relx.evil", // near miss outside the locked prefix + "person.name", + } { + require.NoErrorf(t, guard(nq(pred)), "predicate %q must not be value-locked", pred) + } + }) +} diff --git a/query/expand_edges_test.go b/query/expand_edges_test.go new file mode 100644 index 00000000000..2817241e34e --- /dev/null +++ b/query/expand_edges_test.go @@ -0,0 +1,153 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package query + +import ( + "context" + "testing" + + "github.com/dgraph-io/dgraph/v25/protos/pb" + "github.com/dgraph-io/dgraph/v25/x" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" +) + +// TestExpandEdgesNamespaceSelection covers which namespace each expanded edge's +// predicate is qualified with. In a galaxy operation the caller puts the target +// namespace on each edge, so edges in one request may land in different +// namespaces; otherwise every edge takes the request's namespace and the +// per-edge value is ignored. +// +// Only the non-star path is exercised here: `S * *` expansion calls +// getNodeTypes, which needs worker plumbing. The namespace *plumbing* for that +// path is covered by TestExpandEdgesDerivesPerEdgeContext below. +func TestExpandEdgesNamespaceSelection(t *testing.T) { + edge := func(ns uint64, attr string) *pb.DirectedEdge { + return &pb.DirectedEdge{ + Entity: 1, + Attr: attr, + Namespace: ns, + Op: pb.DirectedEdge_SET, + } + } + + t.Run("non-galaxy ignores the per-edge namespace", func(t *testing.T) { + ctx := x.AttachNamespace(context.Background(), 7) + m := &pb.Mutations{Edges: []*pb.DirectedEdge{ + edge(0, "name"), + edge(9, "email"), // a stray per-edge namespace must not be honored + }} + + got, err := ExpandEdges(ctx, m) + require.NoError(t, err) + require.Len(t, got, 2) + require.Equal(t, x.NamespaceAttr(7, "name"), got[0].Attr) + require.Equal(t, x.NamespaceAttr(7, "email"), got[1].Attr) + }) + + t.Run("galaxy honors each edge's namespace independently", func(t *testing.T) { + // Set INCOMING metadata: x.AttachRootNsOperation is the client-side + // helper and writes outgoing metadata, while x.IsRootNsOperation reads + // incoming — i.e. what the server sees once the call crosses the wire. + ctx := metadata.NewIncomingContext(context.Background(), + metadata.Pairs("galaxy-operation", "true")) + ctx = x.AttachNamespace(ctx, x.RootNamespace) + m := &pb.Mutations{Edges: []*pb.DirectedEdge{ + edge(3, "name"), + edge(5, "email"), + edge(3, "phone"), + }} + + got, err := ExpandEdges(ctx, m) + require.NoError(t, err) + require.Len(t, got, 3) + // Each edge resolves independently — in particular edge 3 does not leak + // into edge 5, and edge 5 does not persist into the third edge. + require.Equal(t, x.NamespaceAttr(3, "name"), got[0].Attr) + require.Equal(t, x.NamespaceAttr(5, "email"), got[1].Attr) + require.Equal(t, x.NamespaceAttr(3, "phone"), got[2].Attr) + }) + + t.Run("reverse edges are dropped", func(t *testing.T) { + ctx := x.AttachNamespace(context.Background(), 0) + m := &pb.Mutations{Edges: []*pb.DirectedEdge{edge(0, "~friend")}} + + got, err := ExpandEdges(ctx, m) + require.NoError(t, err) + require.Empty(t, got) + }) + + t.Run("a missing namespace is an error, not a silent zero", func(t *testing.T) { + _, err := ExpandEdges(context.Background(), &pb.Mutations{ + Edges: []*pb.DirectedEdge{edge(0, "name")}, + }) + require.Error(t, err) + }) +} + +// TestExpandEdgesDerivesPerEdgeContext pins the fix for the discarded +// x.AttachNamespace return. ExpandEdges used to call it without using the +// returned context — both in the loop and in a deferred "reset" — so ctx was +// never actually re-namespaced. getNodeTypes then read dgraph.type from the +// request's namespace while the predicate list was built for the edge's, +// resolving a galaxy-mode `S * *` delete against the wrong schema. +// +// The assertion is on the mechanism rather than on getNodeTypes: a context +// derived for namespace N must report N, and the caller's context must be left +// alone. Against the previous implementation the derived-context assertion fails. +func TestExpandEdgesDerivesPerEdgeContext(t *testing.T) { + ctx := x.AttachNamespace(context.Background(), 7) + + derived := x.AttachNamespace(ctx, 3) + + gotDerived, err := x.ExtractNamespace(derived) + require.NoError(t, err) + require.Equal(t, uint64(3), gotDerived, "derived context must carry the edge's namespace") + + gotOriginal, err := x.ExtractNamespace(ctx) + require.NoError(t, err) + require.Equal(t, uint64(7), gotOriginal, "deriving must not mutate the caller's context") +} + +// TestExpandEdgesPassesTheEdgeNamespaceToGetNodeTypes is the regression test the +// original fix lacked. +// +// ExpandEdges discarded the context returned by x.AttachNamespace, so getNodeTypes +// read dgraph.type from the REQUEST's namespace while the predicate list was built +// for the EDGE's — a galaxy-mode `S * *` delete resolved its expansion against the +// wrong schema. Nothing observed that: getNodeTypes needs worker plumbing, so the +// test written alongside the fix asserted x.AttachNamespace's own return semantics +// instead and passed against the unfixed code. +func TestExpandEdgesPassesTheEdgeNamespaceToGetNodeTypes(t *testing.T) { + prev := nodeTypesForEdge + t.Cleanup(func() { nodeTypesForEdge = prev }) + + var seen []uint64 + nodeTypesForEdge = func(ctx context.Context, _ *SubGraph) ([]string, error) { + ns, err := x.ExtractNamespace(ctx) + require.NoError(t, err, "the per-edge context must carry a namespace") + seen = append(seen, ns) + return nil, nil + } + + // A galaxy-mode star delete against two different tenants in one mutation, which + // is the shape that made the bug observable: whichever namespace ctx happened to + // carry would have been used for both. + // Incoming metadata, which is what x.IsRootNsOperation reads on the server side. + ctx := metadata.NewIncomingContext(context.Background(), + metadata.Pairs("galaxy-operation", "true")) + ctx = x.AttachNamespace(ctx, x.RootNamespace) + _, err := ExpandEdges(ctx, &pb.Mutations{ + StartTs: 1, + Edges: []*pb.DirectedEdge{ + {Entity: 1, Attr: x.Star, Namespace: 7, Op: pb.DirectedEdge_DEL}, + {Entity: 2, Attr: x.Star, Namespace: 9, Op: pb.DirectedEdge_DEL}, + }, + }) + require.NoError(t, err) + require.Equal(t, []uint64{7, 9}, seen, + "each edge's own namespace must reach getNodeTypes, not the request's") +} diff --git a/query/mutation.go b/query/mutation.go index 3094988d43e..e854ab1ea3a 100644 --- a/query/mutation.go +++ b/query/mutation.go @@ -48,27 +48,38 @@ func ApplyMutations(ctx context.Context, m *pb.Mutations) (*api.TxnContext, erro return tctx, err } +// nodeTypesForEdge is getNodeTypes, indirected so a test can observe which +// namespace the per-edge context actually carries. +// +// That is the only thing the discarded-context fix below changed, and it is +// otherwise reachable only through worker.ProcessTaskOverNetwork — so without this +// the fix had no test that would fail if it were reverted, which was exactly the +// case when it landed. +var nodeTypesForEdge = getNodeTypes + func ExpandEdges(ctx context.Context, m *pb.Mutations) ([]*pb.DirectedEdge, error) { edges := make([]*pb.DirectedEdge, 0, 2*len(m.Edges)) - namespace, err := x.ExtractNamespace(ctx) + reqNamespace, err := x.ExtractNamespace(ctx) if err != nil { return nil, errors.Wrapf(err, "While expanding edges") } isGalaxyQuery := x.IsRootNsOperation(ctx) - // Reset the namespace to the original. - defer func(ns uint64) { - x.AttachNamespace(ctx, ns) - }(namespace) - for _, edge := range m.Edges { x.AssertTrue(edge.Op == pb.DirectedEdge_DEL || edge.Op == pb.DirectedEdge_SET) + + // For a galaxy operation the caller puts the target namespace on each + // edge, so an edge may target a namespace other than the request's. Derive + // a per-edge context rather than mutating ctx: x.AttachNamespace returns a + // new context, so the previous code — which discarded that return, both + // here and in a deferred "reset" — left ctx untouched and getNodeTypes + // below read dgraph.type from the REQUEST's namespace while the predicate + // list was built for the EDGE's. For a galaxy-mode `S * *` delete that + // resolves the expansion against the wrong schema. + namespace, edgeCtx := reqNamespace, ctx if isGalaxyQuery { - // The caller should make sure that the directed edges contain the namespace we want - // to insert into. Now, attach the namespace in the context, so that further query - // proceeds as if made from the user of 'namespace'. namespace = edge.GetNamespace() - x.AttachNamespace(ctx, namespace) + edgeCtx = x.AttachNamespace(ctx, namespace) } var preds []string @@ -78,7 +89,7 @@ func ExpandEdges(ctx context.Context, m *pb.Mutations) ([]*pb.DirectedEdge, erro sg := &SubGraph{} sg.DestUIDs = &pb.List{Uids: []uint64{edge.GetEntity()}} sg.ReadTs = m.StartTs - types, err := getNodeTypes(ctx, sg) + types, err := nodeTypesForEdge(edgeCtx, sg) if err != nil { return nil, err } diff --git a/x/keys.go b/x/keys.go index 84df6399453..09757abbe43 100644 --- a/x/keys.go +++ b/x/keys.go @@ -674,19 +674,38 @@ type ReservedNamespace struct { // Names are the bare predicate form (no namespace prefix), matched // case-insensitively like Predicates above. ValueLocked []string + // ValueLockedPrefixes locks predicates by prefix rather than by exact name, + // for a namespace whose owned predicates are created dynamically and so + // cannot be enumerated up front. Typically the same value as + // PredicatePrefix, which locks every dynamic predicate the namespace owns. + // + // Without this, a namespace that admits dynamic predicates by prefix has no + // way to protect them: they are creatable via Alter but writable by anyone + // through /mutate, which for a namespace holding authorization data means its + // own API can be bypassed. + ValueLockedPrefixes []string // TrustMarker is a context key the owner's trusted in-process caller sets, // via context.WithValue(ctx, TrustMarker, true), to authorize writing - // ValueLocked predicates. Required (non-nil) when ValueLocked is non-empty; - // RegisterReservedNamespace panics otherwise. + // ValueLocked predicates. Required (non-nil) when ValueLocked or + // ValueLockedPrefixes is non-empty; RegisterReservedNamespace panics + // otherwise. TrustMarker any } +// valueLockedPrefix pairs a locked predicate prefix with the TrustMarker that +// authorizes writing predicates under it. +type valueLockedPrefix struct { + prefix string + marker any +} + var ( - reservedNsMu sync.RWMutex - reservedNsPrefixes []string - reservedNsPredicates = map[string]struct{}{} - reservedNsTypes = map[string]struct{}{} - reservedNsValueLocked = map[string]any{} // lowercased bare predicate -> TrustMarker + reservedNsMu sync.RWMutex + reservedNsPrefixes []string + reservedNsPredicates = map[string]struct{}{} + reservedNsTypes = map[string]struct{}{} + reservedNsValueLocked = map[string]any{} // lowercased bare predicate -> TrustMarker + reservedNsValueLockedPrefixes []valueLockedPrefix ) // RegisterReservedNamespace records a plugin's ownership of names under the @@ -697,7 +716,7 @@ var ( // non-empty ValueLocked needs a TrustMarker. It panics on any of these, so a // misconfiguration trips at startup rather than silently at mutation time. func RegisterReservedNamespace(ns ReservedNamespace) { - if len(ns.ValueLocked) > 0 && ns.TrustMarker == nil { + if (len(ns.ValueLocked) > 0 || len(ns.ValueLockedPrefixes) > 0) && ns.TrustMarker == nil { panic("x.RegisterReservedNamespace: ValueLocked is set but TrustMarker is nil; " + "a value-locked predicate with no TrustMarker is unwritable by everyone, including its owner") } @@ -706,7 +725,9 @@ func RegisterReservedNamespace(ns ReservedNamespace) { // predicate the mutation path passes, so a namespace-qualified registration // would never match — silently leaving a value-locked predicate publicly // writable. Reject it at startup instead. - for _, group := range [][]string{{ns.PredicatePrefix}, ns.Predicates, ns.Types, ns.ValueLocked} { + for _, group := range [][]string{ + {ns.PredicatePrefix}, ns.Predicates, ns.Types, ns.ValueLocked, ns.ValueLockedPrefixes, + } { for _, name := range group { if strings.Contains(name, NsSeparator) { panic(fmt.Sprintf("x.RegisterReservedNamespace: name %q must be bare, "+ @@ -745,6 +766,24 @@ func RegisterReservedNamespace(ns ReservedNamespace) { } reservedNsValueLocked[key] = ns.TrustMarker } + for _, p := range ns.ValueLockedPrefixes { + key := strings.ToLower(p) + if key == "" { + // An empty prefix matches every predicate, locking the whole cluster to + // this namespace's marker. It fails closed rather than open, so it is not + // a bypass — but it is never intended, and PredicatePrefix above already + // rejects the same mistake. + panic("x.RegisterReservedNamespace: value-locked prefix must not be empty") + } + for _, existing := range reservedNsValueLockedPrefixes { + if existing.prefix == key { + panic(fmt.Sprintf("x.RegisterReservedNamespace: value-locked prefix %q already registered", key)) + } + } + reservedNsValueLockedPrefixes = append(reservedNsValueLockedPrefixes, valueLockedPrefix{ + prefix: key, marker: ns.TrustMarker, + }) + } } // IsRegisteredReservedPredicate reports whether pred is owned by a registered @@ -783,11 +822,23 @@ func IsRegisteredReservedType(typ string) bool { // a value lock cannot be bypassed by changing the case of an owned name. Unlike // those lookups it does not ParseAttr: the guard passes the bare predicate (no // namespace separator), matching how IsOtherReservedPredicate is consulted. +// +// Exact names win over prefixes. That matters when two namespaces overlap — one +// owning a prefix, another owning a specific predicate inside it — since a single +// ReservedNamespace has one TrustMarker and cannot express the split itself. func ReservedPredicateValueLock(pred string) (marker any, locked bool) { + p := strings.ToLower(pred) reservedNsMu.RLock() defer reservedNsMu.RUnlock() - marker, locked = reservedNsValueLocked[strings.ToLower(pred)] - return marker, locked + if marker, locked = reservedNsValueLocked[p]; locked { + return marker, true + } + for _, vlp := range reservedNsValueLockedPrefixes { + if strings.HasPrefix(p, vlp.prefix) { + return vlp.marker, true + } + } + return nil, false } // TODO: rename this map to a better suited name as per its properties. It is not just for GraphQL diff --git a/x/reserved_namespace_test.go b/x/reserved_namespace_test.go index aa29932c7b7..9e202e0859f 100644 --- a/x/reserved_namespace_test.go +++ b/x/reserved_namespace_test.go @@ -117,3 +117,139 @@ func TestRegisterReservedNamespaceRejectsDuplicate(t *testing.T) { RegisterReservedNamespace(ReservedNamespace{Predicates: []string{"dgraph.duptest.x"}}) }) } + +// TestValueLockedPrefixes covers locking dynamically-named predicates by prefix. +// A namespace whose predicates are created one per (namespace, relation) at +// runtime cannot enumerate them in ValueLocked, so without prefix locking they +// would be creatable via Alter yet writable by anyone through /mutate. +func TestValueLockedPrefixes(t *testing.T) { + RegisterReservedNamespace(ReservedNamespace{ + PredicatePrefix: "dgraph.prefixlock.rel.", + Predicates: []string{"dgraph.prefixlock.xid"}, + ValueLockedPrefixes: []string{"dgraph.prefixlock.rel."}, + TrustMarker: testTrust, + }) + + // Any predicate under the locked prefix, including ones that do not exist + // yet — that is the point of locking by prefix. + for _, p := range []string{ + "dgraph.prefixlock.rel.document.owner", + "dgraph.prefixlock.rel.group.member", + "dgraph.prefixlock.rel.", + "dgraph.prefixlock.REL.Document.Owner", // case-insensitive, like exact names + } { + marker, locked := ReservedPredicateValueLock(p) + require.Truef(t, locked, "prefix lock must cover %q", p) + require.Equal(t, testTrust, marker) + } + + // Owned but deliberately not locked: xid stays writable so admin tooling and + // migrations can create nodes. + _, locked := ReservedPredicateValueLock("dgraph.prefixlock.xid") + require.False(t, locked) + + // A near miss outside the prefix is not locked. + _, locked = ReservedPredicateValueLock("dgraph.prefixlock.relative") + require.False(t, locked) +} + +// TestValueLockedExactWinsOverPrefix pins the precedence: a namespace may lock a +// whole prefix to one marker while pinning an individual predicate under it to +// another, so the exact entry must be consulted first. +// TestValueLockedExactWinsOverPrefix pins that an exact value lock takes precedence +// over a prefix that also matches. +// +// It registers the prefix and the exact name in SEPARATE namespaces with distinct +// markers, which is both the only way the precedence is observable and the only way +// the split is expressible: a ReservedNamespace carries one TrustMarker, so the +// earlier version of this test registered both kinds in one namespace, gave them the +// same marker, and passed whichever table ReservedPredicateValueLock consulted +// first. +func TestValueLockedExactWinsOverPrefix(t *testing.T) { + type prefixTrustKey int + type exactTrustKey int + const ( + prefixTrust prefixTrustKey = 1 + exactTrust exactTrustKey = 2 + ) + + // The namespace that owns the whole sub-namespace by prefix. + RegisterReservedNamespace(ReservedNamespace{ + PredicatePrefix: "dgraph.precedence.rel.", + ValueLockedPrefixes: []string{"dgraph.precedence.rel."}, + TrustMarker: prefixTrust, + }) + // A second namespace pinning one predicate inside it to its own marker. Note + // there is no cross-kind conflict check, so this registration is accepted — the + // precedence rule is what decides the overlap. + RegisterReservedNamespace(ReservedNamespace{ + Predicates: []string{"dgraph.precedence.rel.special"}, + ValueLocked: []string{"dgraph.precedence.rel.special"}, + TrustMarker: exactTrust, + }) + + marker, locked := ReservedPredicateValueLock("dgraph.precedence.rel.special") + require.True(t, locked) + require.Equal(t, exactTrust, marker, + "the exact entry must win over the prefix that also matches") + + marker, locked = ReservedPredicateValueLock("dgraph.precedence.rel.ordinary") + require.True(t, locked) + require.Equal(t, prefixTrust, marker, + "a predicate with no exact entry falls to the prefix owner") +} + +// TestValueLockedPrefixRejectsEmpty covers the guard that PredicatePrefix has and +// value locks did not. An empty prefix matches every predicate in the cluster. +func TestValueLockedPrefixRejectsEmpty(t *testing.T) { + type emptyTrustKey int + require.PanicsWithValue(t, + "x.RegisterReservedNamespace: value-locked prefix must not be empty", + func() { + RegisterReservedNamespace(ReservedNamespace{ + PredicatePrefix: "dgraph.emptylock.", + ValueLockedPrefixes: []string{""}, + TrustMarker: emptyTrustKey(1), + }) + }) +} + +// TestValueLockedPrefixesRequireTrustMarker mirrors the ValueLocked invariant: +// a locked prefix with no marker would be unwritable by everyone, including its +// owner, so it must panic at registration. +func TestValueLockedPrefixesRequireTrustMarker(t *testing.T) { + require.Panics(t, func() { + RegisterReservedNamespace(ReservedNamespace{ + PredicatePrefix: "dgraph.prefixnomarker.rel.", + ValueLockedPrefixes: []string{"dgraph.prefixnomarker.rel."}, + // TrustMarker intentionally left nil. + }) + }) +} + +// TestValueLockedPrefixesRejectQualified confirms a namespace-qualified prefix is +// rejected, for the same reason a qualified exact name is: the guard matches the +// bare predicate, so it would never fire and the predicates would stay writable. +func TestValueLockedPrefixesRejectQualified(t *testing.T) { + require.Panics(t, func() { + RegisterReservedNamespace(ReservedNamespace{ + ValueLockedPrefixes: []string{NamespaceAttr(RootNamespace, "dgraph.qualprefix.rel.")}, + TrustMarker: testTrust, + }) + }) +} + +// TestValueLockedPrefixesRejectDuplicate confirms two namespaces cannot claim the +// same locked prefix, since import order would silently pick the TrustMarker. +func TestValueLockedPrefixesRejectDuplicate(t *testing.T) { + require.Panics(t, func() { + RegisterReservedNamespace(ReservedNamespace{ + ValueLockedPrefixes: []string{"dgraph.dupprefix.rel."}, + TrustMarker: testTrust, + }) + RegisterReservedNamespace(ReservedNamespace{ + ValueLockedPrefixes: []string{"dgraph.dupprefix.rel."}, + TrustMarker: testTrust, + }) + }) +} From 1fbfa84ab677e26fe6aa2f4c3e12e6a9a0eec730 Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Wed, 19 Aug 2026 18:57:26 -0400 Subject: [PATCH 2/8] feat(x): make tenant resolution pluggable, separating it from ACL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- audit/interceptor.go | 17 +- dgraph/cmd/alpha/http.go | 12 +- dgraph/cmd/alpha/run.go | 24 +- edgraph/query.go | 7 +- edgraph/server.go | 44 +++- graphql/admin/http.go | 11 +- graphql/resolve/resolver.go | 7 +- worker/graphql_schema.go | 23 +- worker/groups.go | 6 + worker/zero_proxy.go | 22 +- x/tenancy.go | 225 ++++++++++++++++++ x/tenancy_test.go | 451 ++++++++++++++++++++++++++++++++++++ x/x.go | 39 ---- 13 files changed, 830 insertions(+), 58 deletions(-) create mode 100644 x/tenancy.go create mode 100644 x/tenancy_test.go diff --git a/audit/interceptor.go b/audit/interceptor.go index a602fe53e28..972d9ad5bd7 100644 --- a/audit/interceptor.go +++ b/audit/interceptor.go @@ -230,9 +230,24 @@ func auditHttp(w *ResponseWriter, r *http.Request) { user = getUser("", false) } + // Audit must never reject a request, so a resolver error becomes the explicit + // unknown sentinel rather than a failure. + // + // Worth knowing what this does and does not achieve today: the built-in resolver + // tolerates a token it cannot parse and reports no error, so a malformed + // credential is still recorded against the root namespace and this branch is + // reached only by an installed resolver that fails closed. Attributing a + // malformed token accurately needs the resolver to distinguish "no credential" + // from "unusable credential", which it cannot yet do: /admin and the login + // mutation legitimately carry no credential at all. + namespace, err := x.ResolveTenantHTTP(r) + if err != nil { + namespace = UnknownNamespace + } + auditor.Audit(&AuditEvent{ User: user, - Namespace: x.ExtractNamespaceHTTP(r), + Namespace: namespace, ServerHost: x.WorkerConfig.MyAddr, ClientHost: r.RemoteAddr, Endpoint: r.URL.Path, diff --git a/dgraph/cmd/alpha/http.go b/dgraph/cmd/alpha/http.go index b4ba1baf1bc..b883a4ee5eb 100644 --- a/dgraph/cmd/alpha/http.go +++ b/dgraph/cmd/alpha/http.go @@ -705,7 +705,12 @@ func graphqlProbeHandler(gqlHealthStore *admin.GraphQLHealthStore, globalEpoch m w.Header().Set("Content-Type", "application/json") // lazy load the schema so that just by making a probe request, // one can boot up GraphQL for their namespace - namespace := x.ExtractNamespaceHTTP(r) + namespace, err := x.ResolveTenantHTTP(r) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + x.Check2(fmt.Fprintf(w, `{"error":"%s"}`, err)) + return + } if err := admin.LazyLoadSchema(namespace); err != nil { w.WriteHeader(http.StatusInternalServerError) x.Check2(w.Write([]byte(fmt.Sprintf(`{"error":"%s"}`, err)))) @@ -735,7 +740,10 @@ func resolveWithAdminServer(gqlReq *schema.Request, r *http.Request, ctx = x.AttachAccessJwt(ctx, r) ctx = x.AttachRemoteIP(ctx, r) ctx = x.AttachAuthToken(ctx, r) - ctx = x.AttachJWTNamespace(ctx) + ctx, err := x.ResolveTenant(ctx) + if err != nil { + return schema.ErrorResponse(err) + } return adminServer.ResolveWithNs(ctx, x.RootNamespace, gqlReq) } diff --git a/dgraph/cmd/alpha/run.go b/dgraph/cmd/alpha/run.go index c5118bbfab4..4570a94d515 100644 --- a/dgraph/cmd/alpha/run.go +++ b/dgraph/cmd/alpha/run.go @@ -521,7 +521,22 @@ func setupServer(closer *z.Closer, enableMcp bool) { mainServer, adminServer, gqlHealthStore = admin.NewServers(introspection, globalEpoch, closer) baseMux.HandleFunc("/graphql", func(w http.ResponseWriter, r *http.Request) { - namespace := x.ExtractNamespaceHTTP(r) + // Strict here because this handler routes by the resolved namespace: it sets + // the resolver header that selects which namespace's GraphQL schema serves + // the request, so a token it cannot resolve must be refused rather than + // quietly served the root namespace's. A request with no token still + // resolves to the root namespace — see x.ResolveTenantHTTPStrict. + // + // Two limits, neither of which this closes. The strict variant only rejects a + // token that was *presented* and could not be resolved, and its "was one + // presented" probe reads the ACL accessJwt channel, so an installed + // non-ACL resolver is not covered. And the graphql-ws subscription handler + // routes by the same header while still using the lenient resolver. + namespace, err := x.ResolveTenantHTTPStrict(r) + if err != nil { + admin.WriteErrorResponse(w, r, err) + return + } r.Header.Set("resolver", strconv.FormatUint(namespace, 10)) if err := admin.LazyLoadSchema(namespace); err != nil { admin.WriteErrorResponse(w, r, err) @@ -536,7 +551,12 @@ func setupServer(closer *z.Closer, enableMcp bool) { r.Header.Set("resolver", "0") // We don't need to load the schema for all the admin operations. // Only a few like getUser, queryGroup require this. So, this can be optimized. - if err := admin.LazyLoadSchema(x.ExtractNamespaceHTTP(r)); err != nil { + namespace, err := x.ResolveTenantHTTP(r) + if err != nil { + admin.WriteErrorResponse(w, r, err) + return + } + if err := admin.LazyLoadSchema(namespace); err != nil { admin.WriteErrorResponse(w, r, err) return } diff --git a/edgraph/query.go b/edgraph/query.go index 749ef899a08..7bd01552fcd 100644 --- a/edgraph/query.go +++ b/edgraph/query.go @@ -29,6 +29,11 @@ func (s *Server) RunDQL(ctx context.Context, req *api.RunDQLRequest) (*api.Respo apiReq.CommitNow = true } - ctx = x.AttachJWTNamespace(ctx) + // Entry point: see Server.Query. The namespace the client sent has no standing + // here, and the resolver leaves it in place when it cannot derive one. + ctx, err = x.ResolveTenant(x.ClearIncomingNamespace(ctx)) + if err != nil { + return nil, err + } return (&Server{}).doQuery(ctx, &Request{req: apiReq}) } diff --git a/edgraph/server.go b/edgraph/server.go index e5e9de7164c..e6673d18d50 100644 --- a/edgraph/server.go +++ b/edgraph/server.go @@ -113,7 +113,11 @@ type existingGQLSchemaQryResp struct { // If multiple schema nodes were found, it returns an error. func GetGQLSchema(namespace uint64) (uid, graphQLSchema string, err error) { ctx := context.WithValue(context.Background(), Authorize, false) - ctx = x.AttachNamespace(ctx, namespace) + // Trusted attribution: this runs on a hand-built background context with no + // request credential to present, and it crosses the tenant seam via + // QueryNoGrpc. Marking it keeps ResolveTenant from trying to derive a + // namespace that was never going to be there. + ctx = x.AttachTrustedTenant(ctx, namespace) resp, err := (&Server{}).QueryNoGrpc(ctx, &api.Request{ Query: ` @@ -353,7 +357,18 @@ func InsertDropRecord(ctx context.Context, dropOp string) error { // Alter handles requests to change the schema or remove parts or all of the // data. It enforces the admin-IP-whitelist and ACL authorization checks. func (s *Server) Alter(ctx context.Context, op *api.Operation) (*api.Payload, error) { - return s.alter(ctx, op, NeedAuthorize) + // Entry point: drop the namespace the client sent before resolving. + // + // md["namespace"] is client-controlled on the server side, and the built-in + // resolver leaves whatever is there when it cannot derive one from the access + // JWT — so an unattributable request would otherwise proceed on the tenant the + // caller named. Authorization currently catches that, because it reads the signed + // claim, but that is an invariant held by call-site ordering, and relying on + // ordering is what produced the escalations this guards against. + // + // Deliberately here and not in the shared continuation below: alter also serves AlterNoAuth, which the + // in-process callers reach with a context they have already attributed. + return s.alter(x.ClearIncomingNamespace(ctx), op, NeedAuthorize) } // AlterNoAuth is Alter without the admin-IP-whitelist and ACL authorization @@ -376,7 +391,10 @@ func (s *Server) alter(ctx context.Context, op *api.Operation, doAuth AuthMode) ctx, span := otel.Tracer("").Start(ctx, "Server.Alter") defer span.End() - ctx = x.AttachJWTNamespace(ctx) + ctx, err := x.ResolveTenant(ctx) + if err != nil { + return nil, err + } span.AddEvent("Alter operation", trace.WithAttributes(attribute.String("op", op.String()))) // Always print out Alter operations because they are important and rare. @@ -1344,7 +1362,18 @@ func (s *Server) QueryGraphQL(ctx context.Context, req *api.Request, } func (s *Server) Query(ctx context.Context, req *api.Request) (*api.Response, error) { - resp, err := s.QueryNoGrpc(ctx, req) + // Entry point: drop the namespace the client sent before resolving. + // + // md["namespace"] is client-controlled on the server side, and the built-in + // resolver leaves whatever is there when it cannot derive one from the access + // JWT — so an unattributable request would otherwise proceed on the tenant the + // caller named. Authorization currently catches that, because it reads the signed + // claim, but that is an invariant held by call-site ordering, and relying on + // ordering is what produced the escalations this guards against. + // + // Deliberately here and not in the shared continuation below: QueryNoGrpc also serves the two HTTP + // handlers and GetGQLSchema, which attaches its own trusted tenancy. + resp, err := s.QueryNoGrpc(x.ClearIncomingNamespace(ctx), req) if err != nil { return resp, err } @@ -1357,7 +1386,10 @@ func (s *Server) Query(ctx context.Context, req *api.Request) (*api.Response, er // Query handles queries or mutations func (s *Server) QueryNoGrpc(ctx context.Context, req *api.Request) (*api.Response, error) { - ctx = x.AttachJWTNamespace(ctx) + ctx, err := x.ResolveTenant(ctx) + if err != nil { + return nil, err + } if x.WorkerConfig.AclEnabled && req.GetStartTs() != 0 { // A fresh StartTs is assigned if it is 0. ns, err := x.ExtractNamespace(ctx) @@ -2118,7 +2150,7 @@ func (s *Server) CommitOrAbort(ctx context.Context, tc *api.TxnContext) (*api.Tx return &api.TxnContext{}, errors.Errorf( "StartTs cannot be zero while committing a transaction") } - if ns, err := x.ExtractNamespaceFrom(ctx); err == nil { + if ns, err := x.ExtractNamespace(ctx); err == nil { annotateNamespace(span, ns) } annotateStartTs(span, tc.StartTs) diff --git a/graphql/admin/http.go b/graphql/admin/http.go index 680aa8f47a0..17fd759f23b 100644 --- a/graphql/admin/http.go +++ b/graphql/admin/http.go @@ -182,7 +182,10 @@ func (gs *graphqlSubscription) Subscribe( } audit.AuditWebSockets(ctx, req) - namespace := x.ExtractNamespaceHTTP(&http.Request{Header: reqHeader}) + namespace, err := x.ResolveTenantHTTP(&http.Request{Header: reqHeader}) + if err != nil { + return nil, err + } glog.V(2).Infof("namespace: %d. Got GraphQL request over HTTP.", namespace) // first load the schema, then do anything else if err := LazyLoadSchema(namespace); err != nil { @@ -246,7 +249,11 @@ func (gh *graphqlHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { ctx = x.AttachAccessJwt(ctx, r) ctx = x.AttachRemoteIP(ctx, r) ctx = x.AttachAuthToken(ctx, r) - ctx = x.AttachJWTNamespace(ctx) + ctx, err := x.ResolveTenant(ctx) + if err != nil { + WriteErrorResponse(w, r, err) + return + } var res *schema.Response gqlReq, err := getRequest(r) diff --git a/graphql/resolve/resolver.go b/graphql/resolve/resolver.go index f973f87d3db..9be21b4af83 100644 --- a/graphql/resolve/resolver.go +++ b/graphql/resolve/resolver.go @@ -501,7 +501,12 @@ func (r *RequestResolver) Resolve(ctx context.Context, gqlReq *schema.Request) ( return } - ctx = x.AttachJWTNamespace(ctx) + ctx, err = x.ResolveTenant(ctx) + if err != nil { + resp.Errors = schema.AsGQLErrors(err) + return + } + op, err := r.schema.Operation(gqlReq) if err != nil { resp.Errors = schema.AsGQLErrors(err) diff --git a/worker/graphql_schema.go b/worker/graphql_schema.go index f851796286d..ea93045012e 100644 --- a/worker/graphql_schema.go +++ b/worker/graphql_schema.go @@ -115,7 +115,28 @@ func (w *grpcWorker) UpdateGraphQLSchema(ctx context.Context, return nil, errUpdatingGraphQLSchemaOnNonGroupOneLeader } - ctx = x.AttachJWTNamespace(ctx) + // Re-resolve on the receiving alpha when the forwarded request carries a signed + // access JWT: UpdateGQLSchemaOverNetwork copies the caller's incoming metadata + // onward, and re-deriving from the signed token is stronger than trusting the + // forwarded namespace value. When there is no such token the forwarded value + // stands, which is the case the note below is about. + // + // Deliberately NOT clearing the forwarded namespace here, unlike the public gRPC + // entry points. This function is both a network handler and an in-process + // continuation: UpdateGQLSchemaOverNetwork calls it directly when this alpha is + // already the group-1 leader, passing the caller's context — which edgraph's + // UpdateGQLSchema has attributed, and which carries no separate credential to + // re-derive from. Clearing it broke exactly that path, and the live-loader suite + // caught it with "While updating gql schema: No namespace in the metadata". + // + // The weaker guarantee is also the honest one for this port: the internal worker + // listener has no auth interceptor at all, so a caller able to reach it is inside + // the trust boundary already and the boundary here is network-level rather than + // credential-level. + ctx, err := x.ResolveTenant(ctx) + if err != nil { + return nil, errors.Wrapf(err, "While updating gql schema") + } namespace, err := x.ExtractNamespace(ctx) if err != nil { return nil, errors.Wrapf(err, "While updating gql schema") diff --git a/worker/groups.go b/worker/groups.go index 87bbe52e903..48e9ec57d80 100644 --- a/worker/groups.go +++ b/worker/groups.go @@ -1099,6 +1099,12 @@ func GetFeaturesList() []string { var ee []string if Config.AclSecretKey != nil { ee = append(ee, "acl") + } + // Reported separately from ACL. These have historically been the same bit, + // which is already wrong for --limit shared-instance: that mode disables ACL + // for non-root namespaces while multi-tenancy stays on, yet still advertises + // both off the ACL key. + if x.MultiTenancyEnabled() { ee = append(ee, "multi_tenancy") } if x.WorkerConfig.EncryptionKey != nil { diff --git a/worker/zero_proxy.go b/worker/zero_proxy.go index c9ce5389da9..1c8b95cd082 100644 --- a/worker/zero_proxy.go +++ b/worker/zero_proxy.go @@ -21,12 +21,28 @@ func forwardAssignUidsToZero(ctx context.Context, in *pb.Num) (*pb.AssignedIds, return &pb.AssignedIds{}, errors.Errorf("Cannot lease %s via zero proxy", in.Type.String()) } - if x.WorkerConfig.AclEnabled { - var err error - ctx, err = x.AttachJWTNamespaceOutgoing(ctx) + // This is a gRPC entry point, not a continuation of an already-resolved + // request, so the tenant has to be derived here rather than read off the + // context. Resolving through the seam means an installed resolver governs this + // path too, instead of it being hard-wired to the ACL access JWT. + // + // The namespace the client sent is cleared first, and that is load-bearing. + // md["namespace"] is entirely client-controlled server-side, and the built-in + // resolver leaves it in place when it cannot derive a namespace from the access + // JWT. Reading it back would mean a caller who omits or corrupts their token + // leases UIDs in whatever tenant they asked for — which the pre-seam code + // rejected, because it derived the namespace from the signed JWT and returned + // that error. Clearing it makes an unattributable request fail here instead. + if x.MultiTenancyEnabled() { + rctx, err := x.ResolveTenant(x.ClearIncomingNamespace(ctx)) if err != nil { return &pb.AssignedIds{}, err } + ns, err := x.ExtractNamespace(rctx) + if err != nil { + return &pb.AssignedIds{}, err + } + ctx = x.AttachNamespaceOutgoing(ctx, ns) } pl := groups().Leader(0) diff --git a/x/tenancy.go b/x/tenancy.go new file mode 100644 index 00000000000..7652095f1ca --- /dev/null +++ b/x/tenancy.go @@ -0,0 +1,225 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package x + +import ( + "context" + "net/http" + "sync/atomic" + + "github.com/pkg/errors" + "google.golang.org/grpc/metadata" +) + +// TenantResolver attributes an incoming request to a tenant — a Dgraph +// namespace. It returns a context whose tenancy channel names the tenant the +// request acts in (set with AttachNamespace, read by ExtractNamespace), or an +// error if the request cannot be attributed. +// +// A resolver MUST NOT trust the incoming `namespace` metadata value: on the +// server side, incoming metadata is entirely client-controlled. It must either +// overwrite that value from a verified credential or return an error. +// +// The built-in resolver derives the namespace from Dgraph's own ACL access JWT, +// which is why tenancy currently requires ACL: with ACL disabled there is no +// credential to read a namespace from, so every request is the root namespace. +// A deployment that authenticates elsewhere can install its own resolver and +// decouple the two. +type TenantResolver func(ctx context.Context) (context.Context, error) + +// tenantResolver holds the installed resolver, or nil for the built-in one. +// +// An atomic pointer rather than a mutex: this is one word read on every request, +// written once at startup. (The reserved-namespace registry above uses an +// RWMutex because it guards several maps and a slice, not a single pointer.) +var tenantResolver atomic.Pointer[TenantResolver] + +// SetTenantResolver installs a deployment-specific tenant resolver, replacing +// the built-in one that derives the namespace from Dgraph's own ACL access JWT. +// Call it during command setup, after flags are parsed and before any listener +// starts serving. Passing nil restores the built-in resolver. +func SetTenantResolver(r TenantResolver) { + if r == nil { + tenantResolver.Store(nil) + return + } + tenantResolver.Store(&r) +} + +// TenantResolverInstalled reports whether a resolver other than the built-in one +// is installed. See MultiTenancyEnabled. +func TenantResolverInstalled() bool { + return tenantResolver.Load() != nil +} + +// MultiTenancyEnabled reports whether this cluster can serve more than one +// namespace — i.e. whether any request can name a namespace other than +// RootNamespace. True when ACL is on (the ACL access JWT carries the namespace) +// or when a deployment-specific tenant resolver is installed. +// +// Prefer this over reading WorkerConfig.AclEnabled directly wherever the +// question being asked is "is this cluster multi-tenant" rather than "is ACL +// configured". The two have been the same bit historically; they are not the +// same question. +func MultiTenancyEnabled() bool { + return WorkerConfig.AclEnabled || TenantResolverInstalled() +} + +// ResolveTenant attributes ctx to a tenant using the installed resolver. +func ResolveTenant(ctx context.Context) (context.Context, error) { + if isTrustedTenantCtx(ctx) { + // Already attributed by in-process Dgraph code that holds no request + // credential to present. Leave it alone. + return ctx, nil + } + if r := tenantResolver.Load(); r != nil { + rctx, err := (*r)(ctx) + if rctx == nil { + // A resolver that fails closed will naturally return only an error, and + // returning that nil onward is worse than the rejection it meant: a caller + // that logs the error and proceeds, or that drops it, panics on first use + // of the context instead. Hand back the original so the failure is always + // an error rather than sometimes a crash. + return ctx, err + } + return rctx, err + } + return aclTenantResolver(ctx) +} + +// ClearIncomingNamespace removes any namespace the client put on the request. +// +// Call it at a gRPC entry point, before ResolveTenant. On the server side +// md["namespace"] is entirely client-controlled, and the built-in resolver leaves +// whatever is there when it cannot derive a namespace from the access JWT — so +// without this, a caller who omits or corrupts their credential still gets the +// tenant they asked for. Clearing it first turns that case into ExtractNamespace's +// "No namespace in the metadata" error, which is a rejection. +// +// Only for entry points. A continuation of an already-resolved request must keep +// the namespace it was attributed to. +func ClearIncomingNamespace(ctx context.Context) context.Context { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + return ctx + } + md = md.Copy() + md.Delete("namespace") + return metadata.NewIncomingContext(ctx, md) +} + +// aclTenantResolver is the built-in resolver, and the only one an OSS build has. +// It is the pre-seam body of AttachJWTNamespace, unchanged: same branches, same +// returns, and the error is always nil. +func aclTenantResolver(ctx context.Context) (context.Context, error) { + if !WorkerConfig.AclEnabled { + // Single-tenant cluster: everything is the root namespace. + return AttachNamespace(ctx, RootNamespace), nil + } + + ns, err := ExtractNamespaceFrom(ctx) + if err != nil { + // Tolerate the failure and leave whatever namespace the context already + // carries. Under ACL the request is rejected downstream by + // authorizeRequest, which needs the same JWT this just failed to parse, + // so nothing reaches storage on an unattributed context. + return ctx, nil + } + return AttachNamespace(ctx, ns), nil +} + +// ResolveTenantHTTP attributes an incoming HTTP request to a tenant, so the HTTP +// surface derives a namespace through the same resolver as the gRPC surface +// rather than reading the ACL JWT claim directly. It replaces the former +// ExtractNamespaceHTTP. +// +// It deliberately keeps that function's fail-open behavior: a request whose +// tenant cannot be determined resolves to the root namespace. The error return +// carries only a resolver's own failure, which the built-in resolver never +// produces. +// +// Failing open looks wrong until you enumerate the callers, which have +// materially different requirements — a blanket rejection breaks most of them: +// +// - /admin sets resolver=0 unconditionally and uses this value only for +// LazyLoadSchema, so the root namespace is the correct answer there. It also +// serves the login mutation and the test harness's health check. +// - /probe/graphql is unauthenticated by construction. +// - audit must never reject; it only records the event. +// - only /graphql routes by the resolved namespace, and it uses +// ResolveTenantHTTPStrict instead. +// +// Rejecting a credential-less request on the first two deadlocks the cluster: +// login stops working, so nothing can ever obtain the token that would have +// satisfied the check. +func ResolveTenantHTTP(r *http.Request) (uint64, error) { + ctx, err := ResolveTenant(AttachAccessJwt(context.Background(), r)) + if err != nil { + return 0, err + } + ns, nsErr := ExtractNamespace(ctx) + if nsErr != nil { + return RootNamespace, nil + } + return ns, nil +} + +// ResolveTenantHTTPStrict is ResolveTenantHTTP for the one caller that routes by +// the resolved namespace: the /graphql handler, which uses it to choose whose +// GraphQL schema serves the request. +// +// It differs on exactly one input. A request presenting an access token that +// cannot be resolved is rejected, rather than quietly served the root namespace's +// schema — leaking the shape of the root namespace's public API to a caller whose +// own tenant could not be determined. +// +// A request presenting NO token still resolves to the root namespace. That +// distinction is the whole design: /admin serves the login mutation and the +// harness health check, and health probes are unauthenticated by construction, so +// rejecting a credential-less request deadlocks the cluster — login stops working, +// which is how a token would have been obtained. Only /graphql needs the stricter +// rule, and only /graphql gets it. +func ResolveTenantHTTPStrict(r *http.Request) (uint64, error) { + ctx := AttachAccessJwt(context.Background(), r) + resolved, err := ResolveTenant(ctx) + if err != nil { + return 0, err + } + if ns, nsErr := ExtractNamespace(resolved); nsErr == nil { + return ns, nil + } + + // Nothing resolved, so distinguish "no credential" from "unusable + // credential". Asking ExtractJwt rather than re-reading the header name keeps + // this on the same accessor AttachAccessJwt feeds — two copies of that name + // could drift apart, and the failure would be silent: the strict rule would + // simply never fire. + if _, jwtErr := ExtractJwt(ctx); jwtErr == nil { + return 0, errors.Errorf("unable to determine the namespace from the supplied access token") + } + return RootNamespace, nil +} + +// trustedTenantKey marks a context whose tenancy was set by trusted in-process +// code rather than derived from a request credential. +type trustedTenantKey struct{} + +// AttachTrustedTenant attributes ctx to ns on behalf of in-process Dgraph code +// that holds no request credential — schema bootstrap, ACL upserts, GetGQLSchema +// — and marks it so ResolveTenant leaves the attribution alone. +// +// The marker is a context value, never metadata, so it cannot arrive over the +// wire: a network client has no way to claim it. That is what makes it safe for +// a resolver to otherwise distrust the incoming namespace entirely. +func AttachTrustedTenant(ctx context.Context, ns uint64) context.Context { + return context.WithValue(AttachNamespace(ctx, ns), trustedTenantKey{}, true) +} + +// isTrustedTenantCtx reports whether ctx was attributed by AttachTrustedTenant. +func isTrustedTenantCtx(ctx context.Context) bool { + trusted, _ := ctx.Value(trustedTenantKey{}).(bool) + return trusted +} diff --git a/x/tenancy_test.go b/x/tenancy_test.go new file mode 100644 index 00000000000..5e17061ea79 --- /dev/null +++ b/x/tenancy_test.go @@ -0,0 +1,451 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package x + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" +) + +// oldAttachJWTNamespace is the pre-seam body of AttachJWTNamespace, preserved +// verbatim as the oracle for TestResolveTenantIsBitIdentical. It is the thing +// this stage promises not to change; asserting against a copy rather than +// against remembered behavior is what makes that promise checkable. +func oldAttachJWTNamespace(ctx context.Context) context.Context { + if !WorkerConfig.AclEnabled { + return AttachNamespace(ctx, RootNamespace) + } + + ns, err := ExtractNamespaceFrom(ctx) + if err == nil { + ctx = AttachNamespace(ctx, ns) + } + return ctx +} + +// hmacKey is 32 bytes: the FIPS provider rejects an HMAC key shorter than the +// digest size, and this package's tests run in the FIPS build too. +var hmacKey = []byte("tenancy-test-hmac-key-32-bytes!!") + +// tokenWith mints a signed ACL-style token from the given claims. +func tokenWith(t *testing.T, claims jwt.MapClaims) string { + t.Helper() + signed, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(hmacKey) + require.NoError(t, err) + return signed +} + +// withACL configures WorkerConfig for HS256 ACL tokens and restores it after the +// test. WorkerConfig is global, so every mutation here has to be undone. +func withACL(t *testing.T, enabled bool) { + t.Helper() + prevEnabled, prevAlg, prevKey := WorkerConfig.AclEnabled, WorkerConfig.AclJwtAlg, WorkerConfig.AclPublicKey + t.Cleanup(func() { + WorkerConfig.AclEnabled, WorkerConfig.AclJwtAlg, WorkerConfig.AclPublicKey = prevEnabled, prevAlg, prevKey + }) + WorkerConfig.AclEnabled = enabled + WorkerConfig.AclJwtAlg = jwt.GetSigningMethod("HS256") + WorkerConfig.AclPublicKey = hmacKey +} + +// accessToken mints an ACL-style access JWT carrying the given namespace claim. +// The claim is a float64 on the wire, matching what getAccessJwt produces. +func accessToken(t *testing.T, ns uint64) string { + t.Helper() + tok := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{ + "userid": "alice", + "groups": []string{"dev"}, + "namespace": float64(ns), + "exp": float64(1 << 40), // far future + }) + signed, err := tok.SignedString(hmacKey) + require.NoError(t, err) + return signed +} + +// nsOf reports the namespace metadata value on ctx, or "" when absent, so the +// two implementations can be compared on the one channel that matters. +func nsOf(ctx context.Context) string { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + return "" + } + if v := md.Get("namespace"); len(v) > 0 { + return v[0] + } + return "" +} + +// TestResolveTenantIsBitIdentical is the Stage 1 gate. ResolveTenant must agree +// with the pre-seam implementation on every input, so introducing the seam is +// provably behavior-preserving rather than believed to be. +func TestResolveTenantIsBitIdentical(t *testing.T) { + require.False(t, TenantResolverInstalled(), "no resolver may be installed for this comparison") + + valid := func(t *testing.T) string { return accessToken(t, 5) } + + cases := []struct { + name string + aclEnabled bool + token func(*testing.T) string // "" for no token + preAttach *uint64 // namespace already on the context + }{ + {name: "acl off, no token", aclEnabled: false}, + {name: "acl off, valid token", aclEnabled: false, token: valid}, + {name: "acl off, namespace pre-attached", aclEnabled: false, preAttach: ptr(uint64(9))}, + {name: "acl off, malformed token and pre-attached", aclEnabled: false, + token: func(*testing.T) string { return "not.a.jwt" }, preAttach: ptr(uint64(9))}, + + {name: "acl on, no token", aclEnabled: true}, + {name: "acl on, no token but namespace pre-attached", aclEnabled: true, preAttach: ptr(uint64(9))}, + {name: "acl on, valid token", aclEnabled: true, token: valid}, + {name: "acl on, valid token overrides pre-attached", aclEnabled: true, token: valid, preAttach: ptr(uint64(9))}, + {name: "acl on, malformed token", aclEnabled: true, + token: func(*testing.T) string { return "not.a.jwt" }}, + {name: "acl on, malformed token with pre-attached", aclEnabled: true, + token: func(*testing.T) string { return "not.a.jwt" }, preAttach: ptr(uint64(9))}, + {name: "acl on, token signed with the wrong key", aclEnabled: true, token: func(t *testing.T) string { + tok := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{"namespace": float64(5)}) + s, err := tok.SignedString([]byte("a-different-32-byte-hmac-key!!!!!")) + require.NoError(t, err) + return s + }}, + {name: "acl on, token with no namespace claim", aclEnabled: true, token: func(t *testing.T) string { + tok := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{"userid": "alice"}) + s, err := tok.SignedString(hmacKey) + require.NoError(t, err) + return s + }}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + withACL(t, tc.aclEnabled) + + build := func() context.Context { + md := metadata.New(nil) + if tc.token != nil { + md.Set("accessJwt", tc.token(t)) + } + ctx := metadata.NewIncomingContext(context.Background(), md) + if tc.preAttach != nil { + ctx = AttachNamespace(ctx, *tc.preAttach) + } + return ctx + } + + // Separate contexts so neither implementation observes the other's + // mutations to the shared metadata map. + wantCtx := oldAttachJWTNamespace(build()) + gotCtx, err := ResolveTenant(build()) + + require.NoError(t, err, "the built-in resolver never returns an error") + require.Equal(t, nsOf(wantCtx), nsOf(gotCtx), "namespace metadata must match the pre-seam behavior") + }) + } +} + +// TestAttachTrustedTenantIsHonored covers the one input the pre-seam code had no +// concept of. In-process callers that hold no request credential mark their +// context, and ResolveTenant must leave that attribution alone — otherwise a +// fail-closed resolver would break schema bootstrap and the in-process Alter path. +func TestAttachTrustedTenantIsHonored(t *testing.T) { + withACL(t, true) + + // A trusted context naming namespace 9, with a token for namespace 5 also + // present: the marker wins, so the resolver cannot override a deliberate + // in-process attribution. + md := metadata.Pairs("accessJwt", accessToken(t, 5)) + ctx := AttachTrustedTenant(metadata.NewIncomingContext(context.Background(), md), 9) + + got, err := ResolveTenant(ctx) + require.NoError(t, err) + require.Equal(t, "9", nsOf(got)) + + // And an installed resolver is not consulted at all for a trusted context. + t.Cleanup(func() { SetTenantResolver(nil) }) + SetTenantResolver(func(context.Context) (context.Context, error) { + t.Error("resolver must not run for a trusted context") + return nil, errors.New("unreachable") + }) + got, err = ResolveTenant(ctx) + require.NoError(t, err) + require.Equal(t, "9", nsOf(got)) +} + +// TestSetTenantResolver covers installation, delegation, error propagation, and +// restoring the built-in. +func TestSetTenantResolver(t *testing.T) { + withACL(t, false) // built-in would force RootNamespace; the resolver must win + t.Cleanup(func() { SetTenantResolver(nil) }) + + require.False(t, TenantResolverInstalled()) + + SetTenantResolver(func(ctx context.Context) (context.Context, error) { + return AttachNamespace(ctx, 42), nil + }) + require.True(t, TenantResolverInstalled()) + + got, err := ResolveTenant(metadata.NewIncomingContext(context.Background(), metadata.New(nil))) + require.NoError(t, err) + require.Equal(t, "42", nsOf(got), "installed resolver must take precedence over the built-in") + + // An error is propagated rather than swallowed. Every call site takes it and + // rejects, which is the whole reason ResolveTenant replaced the old + // context-in/context-out signature. + sentinel := errors.New("cannot attribute request") + SetTenantResolver(func(context.Context) (context.Context, error) { return nil, sentinel }) + _, err = ResolveTenant(context.Background()) + require.ErrorIs(t, err, sentinel) + + SetTenantResolver(nil) + require.False(t, TenantResolverInstalled()) + got, err = ResolveTenant(context.Background()) + require.NoError(t, err) + require.Equal(t, "0", nsOf(got), "built-in resolver restored: ACL off means root namespace") +} + +// TestMultiTenancyEnabled pins the truth table. A stock OSS build reports false, +// which is what keeps the Stage 3 substitutions behavior-preserving there. +func TestMultiTenancyEnabled(t *testing.T) { + t.Cleanup(func() { SetTenantResolver(nil) }) + + withACL(t, false) + require.False(t, MultiTenancyEnabled(), "stock build: ACL off, no resolver") + + withACL(t, true) + require.True(t, MultiTenancyEnabled(), "ACL carries the namespace claim") + + withACL(t, false) + SetTenantResolver(func(ctx context.Context) (context.Context, error) { return ctx, nil }) + require.True(t, MultiTenancyEnabled(), "an installed resolver can name a tenant without ACL") +} + +// TestResolveTenantHTTP covers the HTTP surface, which previously swallowed the +// parse error and returned namespace 0. That mattered because these call sites +// choose which namespace's GraphQL schema serves a request — so a malformed +// token silently routed the caller to the root namespace's schema instead of +// being rejected. +func TestResolveTenantHTTP(t *testing.T) { + req := func(token string) *http.Request { + r := &http.Request{Header: http.Header{}} + if token != "" { + r.Header.Set("X-Dgraph-AccessToken", token) + } + return r + } + + t.Run("acl off resolves to the root namespace", func(t *testing.T) { + withACL(t, false) + ns, err := ResolveTenantHTTP(req("")) + require.NoError(t, err) + require.Equal(t, RootNamespace, ns) + }) + + t.Run("acl on derives the namespace from the token", func(t *testing.T) { + withACL(t, true) + ns, err := ResolveTenantHTTP(req(accessToken(t, 5))) + require.NoError(t, err) + require.Equal(t, uint64(5), ns) + }) + + // Fail-open is preserved deliberately — see the ResolveTenantHTTP doc comment. + // Two attempts at making this reject taught the same lesson twice: /admin + // forces resolver=0 and only needs this for LazyLoadSchema, health probes and + // the login mutation cannot carry a token at all, and rejecting them deadlocks + // the cluster because login is how a token is obtained in the first place. + t.Run("an unresolvable request falls back to the root namespace", func(t *testing.T) { + withACL(t, true) + for name, token := range map[string]string{ + "no token": "", + "malformed": "not.a.jwt", + "signed with wrong key": func() string { + tok := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{"namespace": float64(5)}) + s, err := tok.SignedString([]byte("a-different-32-byte-hmac-key!!!!!")) + require.NoError(t, err) + return s + }(), + } { + t.Run(name, func(t *testing.T) { + ns, err := ResolveTenantHTTP(req(token)) + require.NoError(t, err) + require.Equal(t, RootNamespace, ns) + }) + } + }) + + t.Run("an installed resolver's error propagates", func(t *testing.T) { + withACL(t, false) + t.Cleanup(func() { SetTenantResolver(nil) }) + sentinel := errors.New("cannot attribute request") + SetTenantResolver(func(context.Context) (context.Context, error) { return nil, sentinel }) + + _, err := ResolveTenantHTTP(req("")) + require.ErrorIs(t, err, sentinel) + }) +} + +// TestResolveTenantHTTPStrict covers the /graphql-only variant. The pair of +// assertions is the whole point: an unusable token is rejected, an absent one is +// not. Two earlier attempts at this hardening failed by conflating them — the +// first rejected both, which broke login and deadlocked the cluster; the second +// still applied the rule to /admin, where the root namespace is the right answer +// and the test harness's own health check presents a token. +func TestResolveTenantHTTPStrict(t *testing.T) { + req := func(token string) *http.Request { + r := &http.Request{Header: http.Header{}} + if token != "" { + r.Header.Set("X-Dgraph-AccessToken", token) + } + return r + } + + t.Run("no token resolves to root, never rejects", func(t *testing.T) { + for _, acl := range []bool{false, true} { + withACL(t, acl) + ns, err := ResolveTenantHTTPStrict(req("")) + require.NoErrorf(t, err, "acl=%v: login and probes carry no token by design", acl) + require.Equal(t, RootNamespace, ns) + } + }) + + t.Run("valid token resolves to its namespace", func(t *testing.T) { + withACL(t, true) + ns, err := ResolveTenantHTTPStrict(req(accessToken(t, 5))) + require.NoError(t, err) + require.Equal(t, uint64(5), ns) + }) + + t.Run("unusable token is rejected", func(t *testing.T) { + withACL(t, true) + wrongKey, err := jwt.NewWithClaims(jwt.SigningMethodHS256, + jwt.MapClaims{"namespace": float64(5), "exp": float64(1 << 40)}). + SignedString([]byte("a-different-32-byte-hmac-key!!!!!")) + require.NoError(t, err) + + for name, token := range map[string]string{ + "malformed": "not.a.jwt", + "signed with wrong key": wrongKey, + "expired": tokenWith(t, jwt.MapClaims{ + "userid": "alice", "namespace": float64(5), "exp": float64(1)}), + "no namespace claim": tokenWith(t, jwt.MapClaims{ + "userid": "alice", "exp": float64(1 << 40)}), + } { + t.Run(name, func(t *testing.T) { + _, err := ResolveTenantHTTPStrict(req(token)) + require.Error(t, err, + "must not serve the root namespace's schema to a caller whose tenant is unknown") + }) + } + }) + + // With ACL off there is no credential to verify, so even a token that would + // otherwise be unusable resolves to the root namespace — the single-tenant + // answer. Strictness must not invent a failure where tenancy cannot vary. + t.Run("acl off ignores an unusable token", func(t *testing.T) { + withACL(t, false) + ns, err := ResolveTenantHTTPStrict(req("not.a.jwt")) + require.NoError(t, err) + require.Equal(t, RootNamespace, ns) + }) + + t.Run("lenient variant still admits what strict rejects", func(t *testing.T) { + withACL(t, true) + ns, err := ResolveTenantHTTP(req("not.a.jwt")) + require.NoError(t, err, "/admin and probes must keep working") + require.Equal(t, RootNamespace, ns) + }) +} + +func ptr[T any](v T) *T { return &v } + +// TestResolveTenantNeverReturnsNilContext pins the guard against a resolver that +// fails closed by returning only an error. +// +// The hazard is not hypothetical: the deprecated AttachJWTNamespace shim did +// `ctx, _ = ResolveTenant(ctx)`, so a nil context would have reached every caller +// that shim served and panicked on first use — a crash where a rejection was +// intended. The shim is gone, but the guard belongs on the seam rather than on the +// discipline of each caller. +func TestResolveTenantNeverReturnsNilContext(t *testing.T) { + t.Cleanup(func() { SetTenantResolver(nil) }) + + wantErr := errors.New("no credential; refusing to attribute") + SetTenantResolver(func(context.Context) (context.Context, error) { + return nil, wantErr + }) + + base := context.Background() + got, err := ResolveTenant(base) + require.ErrorIs(t, err, wantErr, "the resolver's error must survive") + require.NotNil(t, got, "a nil context must never be handed to a caller") + // Usable, not merely non-nil. + require.NoError(t, got.Err()) +} + +// TestClearIncomingNamespaceClosesTheEntryPointHole is the regression test for a +// privilege escalation introduced when the zero-proxy UID lease path was converted +// onto the seam. +// +// Before the conversion the path derived the namespace from the signed access JWT +// and returned that error on failure. After it, the built-in resolver's +// tolerate-a-bad-token branch left the client's own md["namespace"] in place and +// ExtractNamespace read it back, so a caller with no usable credential leased UIDs +// in whichever tenant they named. +func TestClearIncomingNamespaceClosesTheEntryPointHole(t *testing.T) { + withACL(t, true) + + // A caller asking for namespace 9 while presenting no access token at all. + md := metadata.New(map[string]string{"namespace": "9"}) + ctx := metadata.NewIncomingContext(context.Background(), md) + + t.Run("without clearing, the client's namespace survives", func(t *testing.T) { + rctx, err := ResolveTenant(ctx) + require.NoError(t, err, "the built-in resolver tolerates a missing token") + ns, err := ExtractNamespace(rctx) + require.NoError(t, err) + require.Equal(t, uint64(9), ns, + "documents the hazard: the value came from the caller, not a credential") + }) + + t.Run("clearing first turns it into a rejection", func(t *testing.T) { + rctx, err := ResolveTenant(ClearIncomingNamespace(ctx)) + require.NoError(t, err) + _, err = ExtractNamespace(rctx) + require.Error(t, err, + "an unattributable entry-point request must not resolve to any tenant") + }) + + t.Run("a valid token still resolves, and the claim wins over the client's value", func(t *testing.T) { + token := accessToken(t, 7) + withTok := metadata.NewIncomingContext(context.Background(), + metadata.New(map[string]string{"namespace": "9", "accessJwt": token})) + rctx, err := ResolveTenant(ClearIncomingNamespace(withTok)) + require.NoError(t, err) + ns, err := ExtractNamespace(rctx) + require.NoError(t, err) + require.Equal(t, uint64(7), ns, "the namespace must come from the signed claim") + }) + + t.Run("clearing leaves other metadata alone", func(t *testing.T) { + in := metadata.NewIncomingContext(context.Background(), + metadata.New(map[string]string{"namespace": "9", "accessJwt": "keep-me"})) + out, ok := metadata.FromIncomingContext(ClearIncomingNamespace(in)) + require.True(t, ok) + require.Empty(t, out.Get("namespace")) + require.Equal(t, []string{"keep-me"}, out.Get("accessJwt")) + }) + + t.Run("no metadata at all is not a crash", func(t *testing.T) { + require.NotNil(t, ClearIncomingNamespace(context.Background())) + }) +} diff --git a/x/x.go b/x/x.go index 135194e7661..73659303f82 100644 --- a/x/x.go +++ b/x/x.go @@ -249,14 +249,6 @@ func GqlErrorf(message string, args ...interface{}) *GqlError { } } -// ExtractNamespaceHTTP parses the namespace value from the incoming HTTP request. -func ExtractNamespaceHTTP(r *http.Request) uint64 { - ctx := AttachAccessJwt(context.Background(), r) - // Ignoring error because the default value is zero anyways. - namespace, _ := ExtractNamespaceFrom(ctx) - return namespace -} - // ExtractNamespace parses the namespace value from the incoming gRPC context. For the non-ACL mode, // it is caller's responsibility to set the galaxy namespace. func ExtractNamespace(ctx context.Context) (uint64, error) { @@ -445,24 +437,6 @@ func ParseRequest(w http.ResponseWriter, r *http.Request, data interface{}) bool return true } -// AttachJWTNamespace attaches the namespace in the JWT claims to the context if present, otherwise -// it attaches the galaxy namespace. -func AttachJWTNamespace(ctx context.Context) context.Context { - if !WorkerConfig.AclEnabled { - return AttachNamespace(ctx, RootNamespace) - } - - ns, err := ExtractNamespaceFrom(ctx) - if err == nil { - // Attach the namespace only if we got one from JWT. - // This preserves any namespace directly present in the context which is needed for - // requests originating from dgraph internal code like server.go::GetGQLSchema() where - // context is created by hand. - ctx = AttachNamespace(ctx, ns) - } - return ctx -} - // AttachNamespace adds given namespace to the metadata of the context. func AttachNamespace(ctx context.Context, namespace uint64) context.Context { md, ok := metadata.FromIncomingContext(ctx) @@ -474,19 +448,6 @@ func AttachNamespace(ctx context.Context, namespace uint64) context.Context { return metadata.NewIncomingContext(ctx, md) } -// AttachJWTNamespaceOutgoing attaches the namespace in the JWT claims to the outgoing metadata of -// the context. -func AttachJWTNamespaceOutgoing(ctx context.Context) (context.Context, error) { - if !WorkerConfig.AclEnabled { - return AttachNamespaceOutgoing(ctx, RootNamespace), nil - } - ns, err := ExtractNamespaceFrom(ctx) - if err != nil { - return ctx, err - } - return AttachNamespaceOutgoing(ctx, ns), nil -} - // AttachNamespaceOutgoing adds given namespace in the outgoing metadata of the context. func AttachNamespaceOutgoing(ctx context.Context, namespace uint64) context.Context { md, ok := metadata.FromOutgoingContext(ctx) From 27c384af86f2301cf7cfb0592b5b1a69087775be Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Wed, 19 Aug 2026 18:57:28 -0400 Subject: [PATCH 3/8] feat(x): resolve caller identity once, at the edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- audit/interceptor.go | 12 ++ dgraph/cmd/alpha/hooks.go | 14 ++ dgraph/cmd/alpha/http.go | 11 +- dgraph/cmd/alpha/run.go | 13 +- graphql/admin/http.go | 7 +- x/authn.go | 219 ++++++++++++++++++++++++++ x/authn_test.go | 317 ++++++++++++++++++++++++++++++++++++++ x/principal.go | 84 ++++++++++ 8 files changed, 664 insertions(+), 13 deletions(-) create mode 100644 x/authn.go create mode 100644 x/authn_test.go create mode 100644 x/principal.go diff --git a/audit/interceptor.go b/audit/interceptor.go index 972d9ad5bd7..5f30cd020e9 100644 --- a/audit/interceptor.go +++ b/audit/interceptor.go @@ -175,6 +175,18 @@ func auditGrpc(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo) var namespace uint64 var err error extractUser := func(md metadata.MD) { + // Prefer the identity the interceptor already verified. Without this, + // auditing a request re-parses and re-verifies its access token, a second + // full JWT verification per RPC purely to recover a username the identity + // interceptor has already extracted. + // + // The fallback below is unchanged and still covers every case where no + // Principal exists: poor-man's auth, a token that failed to verify + // (UnknownUser), and no credential at all (UnauthorisedUser under ACL). + if p := x.PrincipalFrom(ctx); p != nil { + user = p.Subject + return + } if t := md.Get("accessJwt"); len(t) > 0 { user = getUser(t[0], false) } else if t := md.Get("auth-token"); len(t) > 0 { diff --git a/dgraph/cmd/alpha/hooks.go b/dgraph/cmd/alpha/hooks.go index d1ca3c65f66..8f9bf9b8ac0 100644 --- a/dgraph/cmd/alpha/hooks.go +++ b/dgraph/cmd/alpha/hooks.go @@ -21,6 +21,20 @@ var RegisterFlags = defaultRegisterFlags func defaultRegisterFlags(f *pflag.FlagSet) {} +// ConfigureIdentity installs deployment-specific authentication and authorization +// — an x.Authenticator, and any additional capability sources — from parsed +// configuration. Called from run() after x.WorkerConfig.Parse and before any +// listener starts serving, which is the only window where the config exists and +// nothing is being served yet. Default: no-op (OSS builds). +// +// Separate from the service-registration hooks below on purpose. What it installs +// is process-wide: it resolves the caller's identity for every API the Alpha +// serves, so tying it to whether one optional service happens to be enabled would +// misdescribe its reach. +var ConfigureIdentity = defaultConfigureIdentity + +func defaultConfigureIdentity() {} + // RegisterZanzibar wires the Zanzibar gRPC service onto s and bootstraps the // fixed predicate schema. Default: no-op (OSS builds). var RegisterZanzibar = defaultRegisterZanzibar diff --git a/dgraph/cmd/alpha/http.go b/dgraph/cmd/alpha/http.go index b883a4ee5eb..4f0f2ff48ef 100644 --- a/dgraph/cmd/alpha/http.go +++ b/dgraph/cmd/alpha/http.go @@ -652,10 +652,9 @@ func alterHandler(w http.ResponseWriter, r *http.Request) { glog.Infof("The alter request is forwarded by %s\n", fwd) } - // Pass in PoorMan's auth, ACL and IP information if present. - ctx := x.AttachAuthToken(context.Background(), r) - ctx = x.AttachAccessJwt(ctx, r) - ctx = x.AttachRemoteIP(ctx, r) + // Pass in PoorMan's auth, ACL and IP information if present, and resolve the + // caller's identity from it. + ctx := x.AttachRequestIdentity(context.Background(), r) if _, err := (&edgraph.Server{}).Alter(ctx, op); err != nil { x.SetStatus(w, x.Error, err.Error()) return @@ -737,9 +736,7 @@ func resolveWithAdminServer(gqlReq *schema.Request, r *http.Request, adminServer admin.IServeGraphQL) *schema.Response { md := metadata.New(nil) ctx := metadata.NewIncomingContext(context.Background(), md) - ctx = x.AttachAccessJwt(ctx, r) - ctx = x.AttachRemoteIP(ctx, r) - ctx = x.AttachAuthToken(ctx, r) + ctx = x.AttachRequestIdentity(ctx, r) ctx, err := x.ResolveTenant(ctx) if err != nil { return schema.ErrorResponse(err) diff --git a/dgraph/cmd/alpha/run.go b/dgraph/cmd/alpha/run.go index 4570a94d515..bd37960fac8 100644 --- a/dgraph/cmd/alpha/run.go +++ b/dgraph/cmd/alpha/run.go @@ -393,11 +393,15 @@ func serveGRPC(l net.Listener, tlsCfg *tls.Config, closer *z.Closer) { x.RegisterExporters(Alpha.Conf, "dgraph.alpha") - unary := []grpc.UnaryServerInterceptor{audit.AuditRequestGRPC} + // Identity resolution runs first so every later interceptor and handler sees + // the same verified Principal instead of re-parsing the credential. It never + // rejects — see x.WithResolvedIdentity — so ordering it ahead of audit cannot + // suppress an audit record. + unary := []grpc.UnaryServerInterceptor{x.IdentityUnaryInterceptor(), audit.AuditRequestGRPC} if zi := ZanzibarUnaryInterceptor(); zi != nil { unary = append(unary, zi) } - stream := []grpc.StreamServerInterceptor{audit.AuditStreamGRPC} + stream := []grpc.StreamServerInterceptor{x.IdentityStreamInterceptor(), audit.AuditStreamGRPC} if zs := ZanzibarStreamInterceptor(); zs != nil { stream = append(stream, zs) } @@ -726,6 +730,11 @@ func run() { } x.WorkerConfig.Parse(Alpha.Conf) + // Install deployment-specific authentication and authorization now: the config + // is parsed, and nothing is serving yet. A misconfiguration here is fatal, which + // is why it runs before any listener rather than lazily on the first request. + ConfigureIdentity() + // Set the directory for temporary buffers. z.SetTmpDir(x.WorkerConfig.TmpDir) diff --git a/graphql/admin/http.go b/graphql/admin/http.go index 17fd759f23b..c71f07d5027 100644 --- a/graphql/admin/http.go +++ b/graphql/admin/http.go @@ -245,10 +245,9 @@ func (gh *graphqlHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // Pass in PoorMan's auth, ACL and IP information if present. - ctx = x.AttachAccessJwt(ctx, r) - ctx = x.AttachRemoteIP(ctx, r) - ctx = x.AttachAuthToken(ctx, r) + // Pass in PoorMan's auth, ACL and IP information if present, and resolve the + // caller's identity from it. + ctx = x.AttachRequestIdentity(ctx, r) ctx, err := x.ResolveTenant(ctx) if err != nil { WriteErrorResponse(w, r, err) diff --git a/x/authn.go b/x/authn.go new file mode 100644 index 00000000000..9400f81714d --- /dev/null +++ b/x/authn.go @@ -0,0 +1,219 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package x + +import ( + "context" + "net/http" + "sync/atomic" + + "github.com/golang/glog" + "github.com/pkg/errors" + "google.golang.org/grpc" +) + +// Authenticator verifies the credential on a request and reports who is calling. +// +// It MUST NOT consult or produce tenancy — that is the TenantResolver's job — and +// it MUST NOT deny a request merely for lacking a credential. Returning +// (nil, nil) for "no credential presented" is the normal case for the endpoints +// that are unauthenticated by design. +type Authenticator interface { + // Name identifies the implementation, for logs and audit. + Name() string + // Authenticate returns the verified caller, (nil, nil) when the request + // carries no credential, or an error when it carries one that does not + // verify. + Authenticate(ctx context.Context) (*Principal, error) +} + +// authenticator holds the installed implementation, or nil for the built-in one. +// An atomic pointer for the same reason as tenantResolver: one word, read per +// request, written once at startup. +var authenticator atomic.Pointer[Authenticator] + +// SetAuthenticator installs a deployment-specific authenticator, replacing the +// built-in one that verifies Dgraph's own ACL access JWT. Call it during command +// setup, before any listener starts serving. Passing nil restores the built-in. +func SetAuthenticator(a Authenticator) { + if a == nil { + authenticator.Store(nil) + return + } + authenticator.Store(&a) +} + +// ACLAuthenticator returns the built-in authenticator, which verifies Dgraph's own +// ACL access token. +// +// Exported so a deployment installing its own Authenticator can compose with this +// one rather than displace it. That matters more than it looks: Login is how an +// ACL token is obtained, so an installed authenticator that cannot also verify an +// ACL token takes away the cluster's ability to log in — and the failure surfaces +// as an authorization error somewhere unrelated. +func ACLAuthenticator() Authenticator { return aclAuthenticator{} } + +// currentAuthenticator returns the installed authenticator, or the built-in ACL +// one when none is installed. Mirrors how ResolveTenant falls back to +// aclTenantResolver, so "installed" and "default" behave the same way in both +// halves of the seam. +func currentAuthenticator() Authenticator { + if a := authenticator.Load(); a != nil { + return *a + } + return aclAuthenticator{} +} + +// WithResolvedIdentity authenticates the request if it carries a credential and +// returns a context carrying the resulting Principal. +// +// It never rejects. A request with no credential, or one whose credential does +// not verify, proceeds with no Principal attached, and the authorization layer +// rejects it exactly as it does today — with the accurate error and the right +// gRPC code. +// +// That contract is deliberate, and it is what removes the need for an +// unauthenticated-endpoint allow-list. Login, health checks, and CheckVersion +// cannot present a credential: Login is how one is obtained in the first place. +// A rejecting interceptor would need to enumerate them, which is a second policy +// engine sitting in front of the one that already knows which operations require +// authentication. Worse, an incomplete list fails closed on exactly the endpoint +// that would let you notice — the cluster stops being able to log in or report +// health. +// +// Verification failures are logged rather than returned, so a stale or malformed +// credential is still visible to an operator without being fatal here. +func WithResolvedIdentity(ctx context.Context) context.Context { + a := currentAuthenticator() + p, err := a.Authenticate(ctx) + switch { + case err != nil: + glog.V(2).Infof("identity: %s could not verify the presented credential: %v", a.Name(), err) + return ctx + case p == nil: + // No credential presented. Normal for unauthenticated endpoints. + return ctx + } + return WithPrincipal(ctx, p) +} + +// IdentityUnaryInterceptor resolves the caller's identity onto the context for +// every unary RPC. See WithResolvedIdentity for why it never rejects. +// +// Install it ahead of the audit interceptor so audit can record the resolved +// principal instead of parsing the credential a second time. +func IdentityUnaryInterceptor() grpc.UnaryServerInterceptor { + return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, + handler grpc.UnaryHandler) (any, error) { + return handler(WithResolvedIdentity(ctx), req) + } +} + +// IdentityStreamInterceptor is the streaming counterpart of +// IdentityUnaryInterceptor. +func IdentityStreamInterceptor() grpc.StreamServerInterceptor { + return func(srv any, ss grpc.ServerStream, info *grpc.StreamServerInfo, + handler grpc.StreamHandler) error { + return handler(srv, &identityServerStream{ServerStream: ss, ctx: WithResolvedIdentity(ss.Context())}) + } +} + +// identityServerStream overrides Context so the handler sees the resolved +// identity. grpc.ServerStream has no setter for its context. +type identityServerStream struct { + grpc.ServerStream + ctx context.Context +} + +func (s *identityServerStream) Context() context.Context { return s.ctx } + +// AttachRequestIdentity performs the standard HTTP-edge prelude: it moves the +// access token, remote IP, and auth token from the request into gRPC metadata, +// then resolves the caller's identity. It replaces the four-line sequence that +// was duplicated at every HTTP handler. +func AttachRequestIdentity(ctx context.Context, r *http.Request) context.Context { + ctx = AttachAccessJwt(ctx, r) + ctx = AttachRemoteIP(ctx, r) + ctx = AttachAuthToken(ctx, r) + return WithResolvedIdentity(ctx) +} + +// aclAuthenticator is the built-in authenticator, and the only one an OSS build +// has. It verifies Dgraph's own ACL access JWT. +// +// It is edgraph.validateToken minus the namespace claim, which is deliberate: +// identity and tenancy are read by different components now, and the namespace +// belongs to the TenantResolver. A token with no namespace claim still yields a +// valid identity here; the resolver decides separately what that means for +// tenancy. +type aclAuthenticator struct{} + +func (aclAuthenticator) Name() string { return "acl" } + +func (aclAuthenticator) Authenticate(ctx context.Context) (*Principal, error) { + if !WorkerConfig.AclEnabled { + // No ACL configured: there is no credential to verify and no identity to + // report. Authorization fails open in this configuration, as it does today. + return nil, nil + } + + jwtStr, err := ExtractJwt(ctx) + if err != nil { + // ErrNoJwt means no credential presented, which is not a failure. + if errors.Is(err, ErrNoJwt) { + return nil, nil + } + return nil, err + } + + claims, err := ParseJWT(jwtStr) + if err != nil { + return nil, err + } + // ParseJWT already rejects an expired token; this additionally requires the + // claim to be present, because MapClaims treats a missing exp as valid. + // Mirrors edgraph.validateToken. + if exp, expErr := claims.GetExpirationTime(); expErr != nil || exp == nil { + return nil, errors.Errorf("Token is expired") + } + + userID, ok := claims["userid"].(string) + if !ok { + return nil, errors.Errorf("userid in claims is not a string:%v", claims["userid"]) + } + + groups, err := groupsFromClaims(claims["groups"]) + if err != nil { + return nil, err + } + + return &Principal{ + Issuer: "dgraph-acl", + Subject: userID, + Groups: groups, + Claims: claims, + Method: MethodACL, + }, nil +} + +// groupsFromClaims converts the `groups` claim to a string slice. An absent claim +// is not an error — a user may belong to no groups — but a non-string member is, +// matching edgraph.validateToken. +func groupsFromClaims(claim any) ([]string, error) { + raw, ok := claim.([]interface{}) + if !ok { + return nil, nil + } + groups := make([]string, 0, len(raw)) + for _, g := range raw { + s, ok := g.(string) + if !ok { + return nil, errors.Errorf("unable to convert group to string:%v", g) + } + groups = append(groups, s) + } + return groups, nil +} diff --git a/x/authn_test.go b/x/authn_test.go new file mode 100644 index 00000000000..5abc355507d --- /dev/null +++ b/x/authn_test.go @@ -0,0 +1,317 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package x + +import ( + "context" + "errors" + "net/http" + "testing" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/metadata" +) + +// oracleUserData mirrors edgraph.userData, and oracleValidateToken mirrors +// edgraph.validateToken's identity extraction verbatim. aclAuthenticator +// duplicates that logic in package x, so the duplicate is held against a copy of +// the original rather than against remembered behavior — the same discipline the +// tenant seam used. +// +// The one deliberate divergence is the namespace claim: validateToken requires it +// and aclAuthenticator ignores it, because tenancy is the resolver's concern now. +// That divergence is asserted explicitly below rather than left implicit. +type oracleUserData struct { + userId string + groupIds []string +} + +func oracleValidateToken(jwtStr string) (*oracleUserData, error) { + claims, err := ParseJWT(jwtStr) + if err != nil { + return nil, err + } + if exp, err := claims.GetExpirationTime(); err != nil || exp == nil { + return nil, errors.New("Token is expired") + } + userId, ok := claims["userid"].(string) + if !ok { + return nil, errors.New("userid in claims is not a string") + } + groups, ok := claims["groups"].([]interface{}) + var groupIds []string + if ok { + groupIds = make([]string, 0, len(groups)) + for _, group := range groups { + groupId, ok := group.(string) + if !ok { + return nil, errors.New("unable to convert group to string") + } + groupIds = append(groupIds, groupId) + } + } + return &oracleUserData{userId: userId, groupIds: groupIds}, nil +} + +func ctxWithToken(token string) context.Context { + md := metadata.New(nil) + if token != "" { + md.Set("accessJwt", token) + } + return metadata.NewIncomingContext(context.Background(), md) +} + +// TestACLAuthenticatorMatchesValidateToken pins that the identity extraction +// moved into x agrees with edgraph.validateToken on every input either accepts +// or rejects. +func TestACLAuthenticatorMatchesValidateToken(t *testing.T) { + cases := []struct { + name string + claims jwt.MapClaims + }{ + {"userid and groups", jwt.MapClaims{ + "userid": "alice", "groups": []string{"dev", "ops"}, + "namespace": float64(5), "exp": float64(1 << 40)}}, + {"userid, no groups", jwt.MapClaims{ + "userid": "groot", "namespace": float64(0), "exp": float64(1 << 40)}}, + {"empty groups list", jwt.MapClaims{ + "userid": "bob", "groups": []string{}, + "namespace": float64(0), "exp": float64(1 << 40)}}, + {"guardians member", jwt.MapClaims{ + "userid": "groot", "groups": []string{"guardians"}, + "namespace": float64(0), "exp": float64(1 << 40)}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + withACL(t, true) + tok := tokenWith(t, tc.claims) + + want, wantErr := oracleValidateToken(tok) + got, gotErr := aclAuthenticator{}.Authenticate(ctxWithToken(tok)) + + require.NoError(t, wantErr) + require.NoError(t, gotErr) + require.NotNil(t, got) + require.Equal(t, want.userId, got.Subject, "subject must match validateToken's userId") + require.Equal(t, want.groupIds, got.Groups, "groups must match validateToken's groupIds") + require.Equal(t, "acl", got.Method) + require.Equal(t, "dgraph-acl", got.Issuer) + }) + } +} + +// TestACLAuthenticatorRejectsBadCredentials covers the inputs that must produce +// an error — a credential was presented and it did not verify. Each is also +// rejected by the oracle, so the two agree on failure as well as success. +func TestACLAuthenticatorRejectsBadCredentials(t *testing.T) { + withACL(t, true) + + expired := tokenWith(t, jwt.MapClaims{ + "userid": "alice", "namespace": float64(0), "exp": float64(1)}) // 1970 + noExp := tokenWith(t, jwt.MapClaims{"userid": "alice", "namespace": float64(0)}) + wrongKey, err := jwt.NewWithClaims(jwt.SigningMethodHS256, + jwt.MapClaims{"userid": "alice", "exp": float64(1 << 40)}). + SignedString([]byte("a-different-32-byte-hmac-key!!!!!")) + require.NoError(t, err) + + for name, tok := range map[string]string{ + "expired": expired, + "no exp claim": noExp, + "signed with wrong key": wrongKey, + "malformed": "not.a.jwt", + "userid is not a string": tokenWith(t, jwt.MapClaims{"userid": 42, "exp": float64(1 << 40)}), + "group is not a string": tokenWith(t, jwt.MapClaims{ + "userid": "alice", "groups": []any{"dev", 7}, "exp": float64(1 << 40)}), + } { + t.Run(name, func(t *testing.T) { + _, oracleErr := oracleValidateToken(tok) + require.Error(t, oracleErr, "oracle must also reject this, or the comparison is meaningless") + + p, err := aclAuthenticator{}.Authenticate(ctxWithToken(tok)) + require.Error(t, err) + require.Nil(t, p) + }) + } +} + +// TestACLAuthenticatorNoCredential covers the case that must NOT be an error. +// Login, health checks, and CheckVersion present nothing, and returning an error +// for them is what makes a rejecting interceptor deadlock a cluster. +func TestACLAuthenticatorNoCredential(t *testing.T) { + t.Run("acl on, no token", func(t *testing.T) { + withACL(t, true) + p, err := aclAuthenticator{}.Authenticate(ctxWithToken("")) + require.NoError(t, err, "an absent credential is not a verification failure") + require.Nil(t, p) + }) + + t.Run("acl on, no metadata at all", func(t *testing.T) { + withACL(t, true) + p, err := aclAuthenticator{}.Authenticate(context.Background()) + require.NoError(t, err) + require.Nil(t, p) + }) + + t.Run("acl off", func(t *testing.T) { + withACL(t, false) + p, err := aclAuthenticator{}.Authenticate(ctxWithToken(tokenWith(t, jwt.MapClaims{ + "userid": "alice", "exp": float64(1 << 40)}))) + require.NoError(t, err) + require.Nil(t, p, "no ACL configured means no identity to report") + }) +} + +// TestACLAuthenticatorIgnoresNamespaceClaim pins the one deliberate divergence +// from validateToken. Tenancy is the resolver's concern, so a token with no +// namespace claim still authenticates — where validateToken would reject it. +func TestACLAuthenticatorIgnoresNamespaceClaim(t *testing.T) { + withACL(t, true) + tok := tokenWith(t, jwt.MapClaims{"userid": "alice", "exp": float64(1 << 40)}) // no namespace + + _, oracleErr := oracleValidateToken(tok) + require.NoError(t, oracleErr, "the oracle checks identity only; namespace is checked separately") + + p, err := aclAuthenticator{}.Authenticate(ctxWithToken(tok)) + require.NoError(t, err) + require.NotNil(t, p) + require.Equal(t, "alice", p.Subject) +} + +// TestWithResolvedIdentityNeverRejects is the load-bearing property. Every input +// that could fail must yield a usable context, because the interceptor has no way +// to signal rejection and must not acquire one. +func TestWithResolvedIdentityNeverRejects(t *testing.T) { + withACL(t, true) + + for name, tok := range map[string]string{ + "no token": "", + "malformed": "not.a.jwt", + "expired": tokenWith(t, jwt.MapClaims{"userid": "alice", "exp": float64(1)}), + "userid is not a string": tokenWith(t, jwt.MapClaims{"userid": 42, "exp": float64(1 << 40)}), + } { + t.Run(name, func(t *testing.T) { + ctx := WithResolvedIdentity(ctxWithToken(tok)) + require.NotNil(t, ctx) + require.Nil(t, PrincipalFrom(ctx), "a failed or absent credential attaches no Principal") + }) + } + + t.Run("a valid token attaches the principal", func(t *testing.T) { + ctx := WithResolvedIdentity(ctxWithToken(tokenWith(t, jwt.MapClaims{ + "userid": "alice", "groups": []string{"dev"}, "exp": float64(1 << 40)}))) + p := PrincipalFrom(ctx) + require.NotNil(t, p) + require.Equal(t, "alice", p.Subject) + require.Equal(t, []string{"dev"}, p.Groups) + }) + + t.Run("an authenticator that errors does not reject", func(t *testing.T) { + t.Cleanup(func() { SetAuthenticator(nil) }) + SetAuthenticator(erroringAuthenticator{}) + ctx := WithResolvedIdentity(context.Background()) + require.NotNil(t, ctx) + require.Nil(t, PrincipalFrom(ctx)) + }) +} + +type erroringAuthenticator struct{} + +func (erroringAuthenticator) Name() string { return "erroring" } +func (erroringAuthenticator) Authenticate(context.Context) (*Principal, error) { + return nil, errors.New("issuer unreachable") +} + +// TestSetAuthenticator covers installation and restoration of the built-in. +func TestSetAuthenticator(t *testing.T) { + t.Cleanup(func() { SetAuthenticator(nil) }) + withACL(t, true) + + SetAuthenticator(fixedAuthenticator{subject: "service-account"}) + ctx := WithResolvedIdentity(context.Background()) + p := PrincipalFrom(ctx) + require.NotNil(t, p) + require.Equal(t, "service-account", p.Subject) + require.Equal(t, "fixed", p.Method) + + // nil restores the built-in, which reports nothing without a credential. + SetAuthenticator(nil) + require.Nil(t, PrincipalFrom(WithResolvedIdentity(context.Background()))) +} + +type fixedAuthenticator struct{ subject string } + +func (fixedAuthenticator) Name() string { return "fixed" } +func (f fixedAuthenticator) Authenticate(context.Context) (*Principal, error) { + return &Principal{Subject: f.subject, Method: "fixed"}, nil +} + +// TestIdentityInterceptors confirms both interceptors attach the identity and +// pass the call through, including that the streaming one overrides the stream's +// context rather than silently dropping the principal. +func TestIdentityInterceptors(t *testing.T) { + withACL(t, true) + tok := tokenWith(t, jwt.MapClaims{"userid": "alice", "exp": float64(1 << 40)}) + + t.Run("unary", func(t *testing.T) { + var seen *Principal + intc := IdentityUnaryInterceptor() + _, err := intc(ctxWithToken(tok), "req", &grpc.UnaryServerInfo{FullMethod: "/api.Dgraph/Query"}, + func(ctx context.Context, _ any) (any, error) { + seen = PrincipalFrom(ctx) + return nil, nil + }) + require.NoError(t, err) + require.NotNil(t, seen) + require.Equal(t, "alice", seen.Subject) + }) + + t.Run("stream", func(t *testing.T) { + var seen *Principal + intc := IdentityStreamInterceptor() + err := intc(nil, fakeServerStream{ctx: ctxWithToken(tok)}, + &grpc.StreamServerInfo{FullMethod: "/api.Dgraph/StreamExtSnapshot"}, + func(_ any, ss grpc.ServerStream) error { + seen = PrincipalFrom(ss.Context()) + return nil + }) + require.NoError(t, err) + require.NotNil(t, seen, "the stream's context must carry the principal") + require.Equal(t, "alice", seen.Subject) + }) +} + +type fakeServerStream struct { + grpc.ServerStream + ctx context.Context +} + +func (s fakeServerStream) Context() context.Context { return s.ctx } + +// TestAttachRequestIdentity covers the HTTP-edge helper that replaces the +// duplicated prelude, including that it preserves the metadata the individual +// Attach* calls used to set. +func TestAttachRequestIdentity(t *testing.T) { + withACL(t, true) + tok := tokenWith(t, jwt.MapClaims{"userid": "alice", "exp": float64(1 << 40)}) + + r := &http.Request{Header: http.Header{}, RemoteAddr: "10.0.0.7:4242"} + r.Header.Set("X-Dgraph-AccessToken", tok) + r.Header.Set("X-Dgraph-AuthToken", "poor-mans") + + ctx := AttachRequestIdentity(context.Background(), r) + + p := PrincipalFrom(ctx) + require.NotNil(t, p) + require.Equal(t, "alice", p.Subject) + + md, ok := metadata.FromIncomingContext(ctx) + require.True(t, ok) + require.Equal(t, []string{tok}, md.Get("accessJwt")) + require.Equal(t, []string{"poor-mans"}, md.Get("auth-token")) +} diff --git a/x/principal.go b/x/principal.go new file mode 100644 index 00000000000..6f829325bc8 --- /dev/null +++ b/x/principal.go @@ -0,0 +1,84 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package x + +import "context" + +// Principal is the verified answer to "who is calling". It says nothing about +// what tenant the request operates in, and nothing about what the caller may do. +// Those are the tenant resolver's and the authorizer's jobs respectively. +// +// Deliberately no namespace field. Tenancy already has exactly one home — the +// `namespace` gRPC metadata key, read by ExtractNamespace — and it lives there +// because it has to survive the hop to Zero's UID rate limiter and to the +// group-1 leader. Putting a second copy on the Principal would recreate the +// divergence this whole separation exists to remove: today the tenant is in both +// md["namespace"] and the ACL JWT's claim, and authorization reads the claim +// while storage reads the metadata. +type Principal struct { + // Issuer identifies who vouched for this identity: "dgraph-acl" for a token + // Dgraph minted itself, or the verified `iss` claim for an external issuer. + Issuer string + // Subject is the stable identity of the caller — the ACL userId, or `sub` for + // an external issuer. + Subject string + // Groups are authorization-relevant memberships as asserted by the issuer. + Groups []string + // Claims carries the remaining verified claims, for policy layers that need + // more than identity and groups. + Claims map[string]any + // Method names how the caller was authenticated: one of the Method* constants + // below. Useful for audit, and load-bearing for any policy that must not treat + // an external issuer's assertion as equivalent to one Dgraph made itself. + Method string +} + +// Authentication methods a Principal can carry. +// +// Comparing against these is not bookkeeping. Dgraph's ACL groups are the ones +// x.IsSuperAdmin consults, so a policy that reads Principal.Groups without +// checking Method would let any issuer that can mint a `guardians` group +// membership confer superadmin. Whoever asserted the identity decides what the +// assertion is worth. +const ( + // MethodACL is a token Dgraph minted and verified itself. + MethodACL = "acl" + // MethodExternalJWT is a token from a configured external issuer, verified + // against its published keys. + MethodExternalJWT = "external-jwt" + // MethodPreshared is a shared secret, which identifies a client service rather + // than an end user. + MethodPreshared = "preshared" + // MethodInternal is in-process Dgraph code acting on its own behalf, holding no + // request credential. + MethodInternal = "internal" +) + +// principalKey is the context key under which a verified Principal travels. +// +// A context value, never metadata. Incoming metadata is entirely +// client-controlled, so a metadata-borne principal would be forgeable — and the +// internal worker port has no interceptor chain at all, so anything arriving +// there is unauthenticated by construction. A Principal also never needs to +// cross a process boundary: authorization is decided once, on the edge alpha, +// before any fan-out. +type principalKey struct{} + +// WithPrincipal returns a context carrying p as the verified caller identity. +// Only an authenticator should call this: the point of the type is that its +// presence means a credential was actually verified. +func WithPrincipal(ctx context.Context, p *Principal) context.Context { + return context.WithValue(ctx, principalKey{}, p) +} + +// PrincipalFrom returns the verified caller identity, or nil when the request +// presented no credential or one that did not verify. A nil Principal is the +// normal state for unauthenticated endpoints — Login, health checks, and +// CheckVersion — so callers must treat it as "unknown", not as an error. +func PrincipalFrom(ctx context.Context) *Principal { + p, _ := ctx.Value(principalKey{}).(*Principal) + return p +} From 52cc62ec86d45060221fa5a5803aa7babf48f88f Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Wed, 19 Aug 2026 18:57:30 -0400 Subject: [PATCH 4/8] feat(edgraph): abstract authorization behind an AccessController MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- edgraph/access.go | 180 +++---- edgraph/access_control.go | 262 +++++++++++ edgraph/access_test.go | 121 +++++ edgraph/capability_test.go | 828 +++++++++++++++++++++++++++++++++ edgraph/namespace.go | 6 +- edgraph/predicate_acl.go | 306 ++++++++++++ edgraph/server.go | 18 +- edgraph/zero.go | 2 +- graphql/resolve/middlewares.go | 4 +- 9 files changed, 1605 insertions(+), 122 deletions(-) create mode 100644 edgraph/access_control.go create mode 100644 edgraph/capability_test.go create mode 100644 edgraph/predicate_acl.go diff --git a/edgraph/access.go b/edgraph/access.go index 15c0fb458ce..f56cc45b9fb 100644 --- a/edgraph/access.go +++ b/edgraph/access.go @@ -604,7 +604,40 @@ func upsertGroot(ctx context.Context, passwd string) error { } // extract the userId, groupIds from the accessJwt in the context +// extractUserAndGroups reports the ACL identity and tenant claim of the calling +// request. +// +// It prefers the Principal the identity interceptor already resolved, and parses +// the token itself only when there is none. Before that interceptor existed there +// was one JWT verification per request; adding it made two under ACL, and for the +// RS/PS/ES algorithms that is asymmetric crypto on every RPC. +// +// Two conditions on the fast path, both load-bearing: +// +// The Method must be MethodACL. Principal.Groups is whatever the issuer asserted, +// and x.IsSuperAdmin consults it by name, so accepting an external issuer's +// membership here would let anything that can mint a `guardians` group become +// superadmin. Only Dgraph's own token speaks for Dgraph's own groups. +// +// The namespace claim must be present. aclAuthenticator deliberately ignores it — +// tenancy is the resolver's concern — so the Principal alone cannot say whether +// the token carried one. validateToken requires it, and that requirement is doing +// real work: aclTenantResolver tolerates a token it cannot extract a namespace +// from and leaves whatever namespace the context already carries, which on the +// server side is client-controlled. It gets away with that precisely because this +// function rejects the same token a moment later. Reading the namespace from the +// resolved tenancy instead would remove the check the resolver depends on, and +// hand the caller their own namespace. +// +// So the value comes from the claim, exactly as validateToken produces it. Keying +// authorization on the resolved namespace is the right end state and it is a +// behavior change, so it belongs with the others rather than inside a change whose +// whole claim is that nothing moved. authorizePreds already keys on the resolved +// namespace; the remaining reader is shouldAllowAcls. func extractUserAndGroups(ctx context.Context) (*userData, error) { + if ud, ok := userDataFromPrincipal(ctx); ok { + return ud, nil + } accessJwt, err := x.ExtractJwt(ctx) if err != nil { return nil, err @@ -612,49 +645,21 @@ func extractUserAndGroups(ctx context.Context) (*userData, error) { return validateToken(accessJwt) } -type authPredResult struct { - allowed []string - blocked map[string]struct{} -} - -func authorizePreds(ctx context.Context, userData *userData, preds []string, - aclOp *acl.Operation) *authPredResult { - - if !worker.AclCachePtr.Loaded() { - RefreshACLs(ctx) - } - - userId := userData.userId - groupIds := userData.groupIds - ns := userData.namespace - blockedPreds := make(map[string]struct{}) - for _, pred := range preds { - nsPred := x.NamespaceAttr(ns, pred) - if err := worker.AclCachePtr.AuthorizePredicate(groupIds, nsPred, aclOp); err != nil { - logAccess(&accessEntry{ - userId: userId, - groups: groupIds, - preds: preds, - operation: aclOp, - allowed: false, - }) - blockedPreds[pred] = struct{}{} - } - } - if worker.HasAccessToAllPreds(ns, groupIds, aclOp) { - // Setting allowed to nil allows access to all predicates. Note that the access to ACL - // predicates will still be blocked. - return &authPredResult{allowed: nil, blocked: blockedPreds} - } - // User can have multiple permission for same predicate, add predicate - allowedPreds := make([]string, 0, len(worker.AclCachePtr.GetUserPredPerms(userId))) - // only if the acl.Op is covered in the set of permissions for the user - for predicate, perm := range worker.AclCachePtr.GetUserPredPerms(userId) { - if (perm & aclOp.Code) > 0 { - allowedPreds = append(allowedPreds, predicate) - } +// userDataFromPrincipal rebuilds userData from an already-verified Principal, +// reporting false when the fast path does not apply so the caller falls back to +// parsing — and to the error message parsing would have produced. +func userDataFromPrincipal(ctx context.Context) (*userData, bool) { + p := x.PrincipalFrom(ctx) + if p == nil || p.Method != x.MethodACL { + return nil, false + } + // float64 because JSON numbers are, which is also why validateToken caps the + // usable namespace at 1<<52; asserting the same type keeps that identical. + ns, ok := p.Claims["namespace"].(float64) + if !ok { + return nil, false } - return &authPredResult{allowed: allowedPreds, blocked: blockedPreds} + return &userData{namespace: uint64(ns), userId: p.Subject, groupIds: p.Groups}, true } // authorizeAlter parses the Schema in the operation and authorizes the operation @@ -709,10 +714,13 @@ func authorizeAlter(ctx context.Context, op *api.Operation) error { "only guardians are allowed to drop all data, but the current user is %s", userId) } - result := authorizePreds(ctx, userData, preds, acl.Modify) - if len(result.blocked) > 0 { + result, err := AuthorizePredicates(ctx, preds, acl.Modify) + if err != nil { + return err + } + if len(result.Blocked) > 0 { var msg strings.Builder - for key := range result.blocked { + for key := range result.Blocked { x.Check2(msg.WriteString(key)) x.Check2(msg.WriteString(" ")) } @@ -828,17 +836,20 @@ func authorizeMutation(ctx context.Context, gmu *dql.Mutation) error { } return nil } - result := authorizePreds(ctx, userData, preds, acl.Write) - if len(result.blocked) > 0 { + result, err := AuthorizePredicates(ctx, preds, acl.Write) + if err != nil { + return err + } + if len(result.Blocked) > 0 { var msg strings.Builder - for key := range result.blocked { + for key := range result.Blocked { x.Check2(msg.WriteString(key)) x.Check2(msg.WriteString(" ")) } return status.Errorf(codes.PermissionDenied, "unauthorized to mutate following predicates: %s\n", msg.String()) } - gmu.AllowedPreds = result.allowed + gmu.AllowedPreds = result.Allowed return nil } @@ -992,8 +1003,11 @@ func authorizeQuery(ctx context.Context, parsedReq *dql.Result, graphql bool) er return blockedPreds(preds), nil, nil } - result := authorizePreds(ctx, userData, preds, acl.Read) - return result.blocked, result.allowed, nil + result, err := AuthorizePredicates(ctx, preds, acl.Read) + if err != nil { + return nil, nil, err + } + return result.Blocked, result.Allowed, nil } blockedPreds, allowedPreds, err := doAuthorizeQuery() @@ -1014,7 +1028,7 @@ func authorizeQuery(ctx context.Context, parsedReq *dql.Result, graphql bool) er if len(blockedPreds) != 0 { // For GraphQL requests, we allow filtered access to the ACL predicates. // Filter for user_id and group_id is applied for the currently logged in user. - if graphql && shouldAllowAcls(namespace) { + if graphql && shouldAllowAcls(namespace) { // namespace is the token claim, as above for _, gq := range parsedReq.Query { addUserFilterToQuery(gq, userId, groupIds) } @@ -1081,8 +1095,11 @@ func authorizeSchemaQuery(ctx context.Context, er *query.ExecutionResult) error } return blockedPreds(preds), nil } - result := authorizePreds(ctx, userData, preds, acl.Read) - return result.blocked, nil + result, err := AuthorizePredicates(ctx, preds, acl.Read) + if err != nil { + return nil, err + } + return result.Blocked, nil } // find the predicates which are blocked for the schema query @@ -1115,61 +1132,6 @@ func authorizeSchemaQuery(ctx context.Context, er *query.ExecutionResult) error return nil } -// AuthSuperAdmin authorizes the operations for the users who belong to the guardians -// group in the galaxy namespace. This authorization is used for admin usages like creation and -// deletion of a namespace, resetting passwords across namespaces etc. -// NOTE: The caller should not wrap the error returned. If needed, propagate the GRPC error code. -func AuthSuperAdmin(ctx context.Context) error { - if !x.WorkerConfig.AclEnabled { - return nil - } - ns, err := x.ExtractNamespaceFrom(ctx) - if err != nil { - return errors.Wrap(err, "Authorize guardian of the galaxy, extracting jwt token, error:") - } - if ns != 0 { - return status.Error( - codes.PermissionDenied, "Only superadmin is allowed to do this operation") - } - // AuthorizeGuardians will extract (user, []groups) from the JWT claims and will check if - // any of the group to which the user belongs is "guardians" or not. - if err := AuthorizeGuardians(ctx); err != nil { - s := status.Convert(err) - return status.Error( - s.Code(), "AuthSuperAdmin: failed to authorize guardians. "+s.Message()) - } - glog.V(3).Info("Successfully authorised guardian of the galaxy") - return nil -} - -// AuthorizeGuardians authorizes the operation for users which belong to Guardians group. -// NOTE: The caller should not wrap the error returned. If needed, propagate the GRPC error code. -func AuthorizeGuardians(ctx context.Context) error { - if worker.Config.AclSecretKey == nil { - // the user has not turned on the acl feature - return nil - } - - userData, err := extractUserAndGroups(ctx) - switch { - case err == x.ErrNoJwt: - return status.Error(codes.PermissionDenied, err.Error()) - case err != nil: - return status.Error(codes.Unauthenticated, err.Error()) - default: - userId := userData.userId - groupIds := userData.groupIds - - if !x.IsSuperAdmin(groupIds) { - // Deny access for members of non-guardian groups - return status.Error(codes.PermissionDenied, fmt.Sprintf("Only guardians are "+ - "allowed access. User '%v' is not a member of guardians group.", userId)) - } - } - - return nil -} - /* addUserFilterToQuery applies makes sure that a user can access only its own acl info by applying filter of userid and groupid to acl predicates. A query like diff --git a/edgraph/access_control.go b/edgraph/access_control.go new file mode 100644 index 00000000000..f6424728d56 --- /dev/null +++ b/edgraph/access_control.go @@ -0,0 +1,262 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package edgraph + +import ( + "context" + "strings" + "sync/atomic" + + "github.com/golang/glog" + + "github.com/dgraph-io/dgraph/v25/acl" + "github.com/dgraph-io/dgraph/v25/x" +) + +// Capability is an authority over the cluster or a tenant, as opposed to access to +// particular predicates. Capabilities answer "may this caller perform this class of +// operation at all", and they are deliberately few: an operation that needs a +// finer distinction should be authorized on its predicates instead. +type Capability int + +const ( + // CapClusterAdmin is authority over the whole cluster: creating and dropping + // namespaces, dropping all data, resetting passwords across tenants. Nothing + // scoped to a single tenant should require it. + CapClusterAdmin Capability = iota + + // CapTenantAdmin is administrative authority within a tenant — reading cluster + // state and health, arming an external-snapshot import. + // + // It is a weaker capability than CapClusterAdmin, and under the built-in policy + // the difference is real but narrow: CapClusterAdmin additionally requires the + // caller to be in the root namespace. + CapTenantAdmin + + // CapAssumeTenant is the authority to act in a namespace other than the + // caller's own. The live loader needs it for a galaxy-wide write. + // + // Split out from CapClusterAdmin even though the built-in policy resolves both + // identically, because they are different powers and conflating them costs + // something later: a scoped service account should be able to write across + // tenants without also being able to drop the cluster. + CapAssumeTenant + + // CapLeaseUIDs is the authority to lease a block of UIDs from Zero. + // + // Separate from CapClusterAdmin, which it briefly shared, because the callers + // are not administrators: `dgraph live` allocates UIDs through this RPC on every + // run — xidmap falls back to it whenever it was built without a direct Zero + // connection — so folding it in made the loader require a whitelisted IP or an + // auth token against any cluster running without ACL, where it had needed + // neither. + // + // Tightening it is the same change as fail-closed galaxy-operation, with the same + // cost and the same answer: it wants a flag-gated release rather than to ride + // along. Under ACL it is guardian-of-the-root-namespace, exactly as it was before + // capabilities existed. + CapLeaseUIDs +) + +func (c Capability) String() string { + switch c { + case CapClusterAdmin: + return "cluster-admin" + case CapTenantAdmin: + return "tenant-admin" + case CapAssumeTenant: + return "assume-tenant" + case CapLeaseUIDs: + return "lease-uids" + } + return "unknown-capability" +} + +// AccessController decides what a verified caller may do. +// +// It answers only authorization. Who the caller is has already been established by +// the time a method here runs — x.PrincipalFrom carries the answer — and which +// tenant the request operates in is the TenantResolver's business. Keeping all +// three apart is the point of the surrounding work; an implementation that +// verifies a credential or derives a namespace has taken on someone else's job. +// +// Two methods rather than one, because the two questions have different shapes. +// A capability is a yes or no. Predicate access is a partition: the answer is +// which of the requested predicates are usable, and the caller proceeds with +// those. +type AccessController interface { + // Name identifies the policy, for logs and for the error an unimplemented + // capability produces. + Name() string + + // AuthorizeCapability reports whether the caller on ctx holds c, returning a + // gRPC status error when it does not. + // + // Callers must not wrap the returned error. They may prepend context to its + // message, but the code has to survive — several endpoints distinguish + // Unauthenticated from PermissionDenied, and clients act on that difference. + AuthorizeCapability(ctx context.Context, c Capability) error + + // AuthorizePredicates partitions preds into what the caller may and may not + // access under op. + // + // It deliberately does not return "denied". A query names predicates the caller + // may have no business seeing, and the established behavior is to drop them and + // answer the rest rather than refuse the whole request — so the result is data + // the caller acts on, not a verdict. See PredResult. + // + // The error is for a policy that could not reach an answer at all: the built-in + // one reads a local cache and never fails, but a policy that has to query the + // graph can. An error must not be read as a denial. + // + // Identity comes from ctx, not from a parameter. Handing a policy Dgraph's own + // userData would make every implementation speak ACL. + AuthorizePredicates(ctx context.Context, preds []string, op *acl.Operation) (*PredResult, error) +} + +// PredResult is how a policy partitions the predicates a request named. +type PredResult struct { + // Allowed lists the predicates the caller may access — but a nil Allowed means + // *all* of them, not none. + // + // The sentinel is load-bearing and it predates this interface: a caller with + // blanket access has no enumerable predicate list, and materializing one would + // mean listing every predicate in the namespace on every request. Blocked is + // still honored when Allowed is nil, which is how ACL predicates stay + // unreadable even to a caller that may read everything else. + Allowed []string + + // Blocked is the subset of the requested predicates the caller may not access. + // Empty means none were refused. + Blocked map[string]struct{} +} + +// namespaceOrClaim reports the namespace a request OPERATES IN, preferring the +// resolved tenancy over the token's claim. Its one caller is authorizePreds, and +// that is deliberate. +// +// The distinction this function exists for, learned the hard way: "which tenant's +// data does this request touch" and "who is this caller" are different questions +// with different correct sources. Predicate authorization is the first — it must +// key on the same channel storage keys on, or it authorizes in one namespace while +// executing in another. Everything asking the second — authSuperAdmin's root- +// namespace test, filterTablets, shouldAllowAcls — must read the signed claim, +// because the resolved value comes from md["namespace"], which is client-supplied +// until a resolver overwrites it, and not every path resolves before authorizing. +// +// A divergence is logged rather than rejected: it should be impossible, and turning +// "impossible" into an outage is worse than making it visible. +func namespaceOrClaim(ctx context.Context, claim uint64, site string) uint64 { + resolved, err := x.ExtractNamespace(ctx) + if err != nil { + glog.Warningf("%s: no resolved tenancy on the context (%v); falling back to the "+ + "token's claim %#x", site, err, claim) + return claim + } + if resolved != claim { + glog.Warningf("%s: resolved namespace %#x differs from the token's claim %#x; "+ + "authorizing against the resolved one", site, resolved, claim) + } + return resolved +} + +// CapabilitySource is one way a caller can come to hold a capability. +// +// The built-in policy consults its sources in order and the first grant wins, so +// a deployment can add a path to cluster authority without replacing the whole +// policy. That matters because the alternative — SetAccessController — means +// reimplementing Dgraph's ACL rules in order to add one grant beside them. +// +// A source answers only "does this caller hold c". It must not deny: returning +// false means "not by this route", and the next source still gets asked. +type CapabilitySource interface { + // Name identifies the source in logs and in the denial message, so an + // operator can see which routes were tried. + Name() string + // Grants reports whether the caller holds c by this route. p is the verified + // principal, or nil when the request carried no credential — a source keyed on + // operator-level controls rather than identity still works in that case. + Grants(ctx context.Context, p *x.Principal, c Capability) bool +} + +// capabilitySources are consulted in order. breakGlass is first and always +// present: if cluster authority could only come from an identity provider, an +// outage or a key rotation there would lock an operator out of namespace +// lifecycle and drop-all, which is not an acceptable failure mode for a database. +var capabilitySources = []CapabilitySource{breakGlassSource{}} + +// RegisterCapabilitySource appends a source, consulted after the ones already +// registered. Call it during command setup, before any listener starts serving; +// it is not safe to call concurrently with request handling. +func RegisterCapabilitySource(s CapabilitySource) { + if s == nil { + return + } + capabilitySources = append(capabilitySources, s) +} + +// grantedBySource reports whether any registered source grants c, and names the +// one that did. +func grantedBySource(ctx context.Context, c Capability) (string, bool) { + p := x.PrincipalFrom(ctx) + for _, s := range capabilitySources { + if s.Grants(ctx, p, c) { + return s.Name(), true + } + } + return "", false +} + +// sourceNames lists the routes that were tried, for the denial message. +func sourceNames() string { + names := make([]string, 0, len(capabilitySources)) + for _, s := range capabilitySources { + names = append(names, s.Name()) + } + return strings.Join(names, ", ") +} + +// accessController holds the installed policy, or nil for the built-in one. An +// atomic pointer for the same reason as x.tenantResolver and x.authenticator: one +// word, read per request, written once at startup. +var accessController atomic.Pointer[AccessController] + +// SetAccessController installs a deployment-specific policy, replacing the +// built-in one that authorizes against Dgraph's own ACL. Call it during command +// setup, before any listener starts serving. Passing nil restores the built-in. +func SetAccessController(ac AccessController) { + if ac == nil { + accessController.Store(nil) + return + } + accessController.Store(&ac) +} + +// currentAccessController returns the installed policy, or the built-in one when +// none is installed. +func currentAccessController() AccessController { + if ac := accessController.Load(); ac != nil { + return *ac + } + return predicateACL{} +} + +// AuthorizeCapability authorizes c for the caller on ctx under the installed +// policy. This is the entry point every call site uses; the interface exists so +// what it resolves to can be replaced. +// +// NOTE: do not wrap the returned error. Prepend to the message if needed, and +// propagate the gRPC code. +func AuthorizeCapability(ctx context.Context, c Capability) error { + return currentAccessController().AuthorizeCapability(ctx, c) +} + +// AuthorizePredicates partitions preds for the caller on ctx under the installed +// policy. See AccessController.AuthorizePredicates: an error means the policy could +// not decide, which is not the same as a denial. +func AuthorizePredicates(ctx context.Context, preds []string, op *acl.Operation) (*PredResult, error) { + return currentAccessController().AuthorizePredicates(ctx, preds, op) +} diff --git a/edgraph/access_test.go b/edgraph/access_test.go index 424e3ab8545..1f772b9994b 100644 --- a/edgraph/access_test.go +++ b/edgraph/access_test.go @@ -6,11 +6,13 @@ package edgraph import ( + "context" "testing" "time" "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" "github.com/dgraph-io/dgraph/v25/acl" "github.com/dgraph-io/dgraph/v25/worker" @@ -115,3 +117,122 @@ func TestMain(m *testing.M) { worker.Config.AclSecretKey = x.Sensitive("6ABBAA2014CFF00289D20D20DA296F67") m.Run() } + +// TestExtractUserAndGroupsPrefersThePrincipal is the equivalence proof for the +// fast path. Both routes are run over the same inputs and required to agree, +// rather than the fast path being checked against remembered behavior. +// +// The inputs that must NOT take the fast path are the point of the test. Each one +// is a way the Principal alone is insufficient, and each would be a privilege +// change if admitted. +func TestExtractUserAndGroupsPrefersThePrincipal(t *testing.T) { + // aclAuthenticator reports no identity at all unless ACL is on, and the fast + // path is only reachable when it does. Scoped here rather than in TestMain, + // which other tests in this package read as ACL-off. + prev := x.WorkerConfig.AclEnabled + t.Cleanup(func() { x.WorkerConfig.AclEnabled = prev }) + x.WorkerConfig.AclEnabled = true + + expiry := time.Now().Add(30 * time.Minute).Unix() + + // withPrincipal resolves identity onto the context the way the interceptor + // does, then attaches the token so the slow path stays available. + withPrincipal := func(token string) context.Context { + ctx := metadata.NewIncomingContext(context.Background(), + metadata.Pairs("accessJwt", token)) + return x.WithResolvedIdentity(ctx) + } + + t.Run("both routes agree on a valid token", func(t *testing.T) { + for _, want := range []userData{ + {1234567890, "user1", []string{"701", "702"}}, + {2345678901, "user2", []string{"703", "701"}}, + {0, "groot", []string{"guardians"}}, + {7, "no-groups", nil}, + } { + token := generateJWT(want.namespace, want.userId, want.groupIds, expiry) + + slow, err := validateToken(token) + require.NoError(t, err) + + fast, err := extractUserAndGroups(withPrincipal(token)) + require.NoError(t, err) + + require.Equal(t, slow, fast, "the fast path disagreed with validateToken") + require.Equal(t, want.namespace, fast.namespace) + require.Equal(t, want.userId, fast.userId) + require.Equal(t, want.groupIds, fast.groupIds) + + // And the fast path must actually have been taken, or this proves + // nothing about it. + _, ok := userDataFromPrincipal(withPrincipal(token)) + require.True(t, ok, "expected the fast path to apply") + } + }) + + t.Run("a token with no namespace claim still falls through and is rejected", func(t *testing.T) { + // aclAuthenticator ignores the namespace claim, so a Principal exists here. + // Taking the fast path would accept the request and let the caller keep + // whatever namespace metadata they sent, because aclTenantResolver tolerates + // a token it cannot extract one from. + claims := jwt.MapClaims{"userid": "sneaky", "exp": expiry, "groups": []string{"guardians"}} + token := jwt.NewWithClaims(jwt.SigningMethodHS256, &claims) + signed, err := token.SignedString(x.MaybeKeyToBytes(worker.Config.AclSecretKey)) + require.NoError(t, err) + + ctx := withPrincipal(signed) + require.NotNil(t, x.PrincipalFrom(ctx), "precondition: identity resolved") + + _, ok := userDataFromPrincipal(ctx) + require.False(t, ok, "the fast path must decline a token with no namespace claim") + + _, err = extractUserAndGroups(ctx) + require.Error(t, err, "the request must still be rejected") + require.Contains(t, err.Error(), "namespace in claims is not valid") + }) + + t.Run("an external issuer's groups never take the fast path", func(t *testing.T) { + // The escalation this guards: IsSuperAdmin matches on the group name, so an + // external issuer asserting `guardians` would otherwise become superadmin. + ctx := x.WithPrincipal(context.Background(), &x.Principal{ + Issuer: "https://idp.example/identity", + Subject: "some-agent", + Groups: []string{"guardians"}, + Claims: map[string]any{"namespace": float64(0)}, + Method: x.MethodExternalJWT, + }) + _, ok := userDataFromPrincipal(ctx) + require.False(t, ok, "only Dgraph's own token speaks for Dgraph's own groups") + }) + + t.Run("no principal falls through to the token", func(t *testing.T) { + token := generateJWT(42, "user", []string{"701"}, expiry) + ctx := metadata.NewIncomingContext(context.Background(), + metadata.Pairs("accessJwt", token)) + + _, ok := userDataFromPrincipal(ctx) + require.False(t, ok) + + ud, err := extractUserAndGroups(ctx) + require.NoError(t, err) + require.Equal(t, uint64(42), ud.namespace) + }) + + t.Run("an expired token is rejected on both routes", func(t *testing.T) { + token := generateJWT(1, "user", []string{"701"}, time.Now().Add(-time.Hour).Unix()) + + _, slowErr := validateToken(token) + require.Error(t, slowErr) + + ctx := withPrincipal(token) + require.Nil(t, x.PrincipalFrom(ctx), + "an expired token must not resolve to a Principal") + _, err := extractUserAndGroups(ctx) + require.Error(t, err) + }) + + t.Run("no credential at all is rejected", func(t *testing.T) { + _, err := extractUserAndGroups(x.WithResolvedIdentity(context.Background())) + require.Error(t, err) + }) +} diff --git a/edgraph/capability_test.go b/edgraph/capability_test.go new file mode 100644 index 00000000000..38f8aa5c555 --- /dev/null +++ b/edgraph/capability_test.go @@ -0,0 +1,828 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package edgraph + +import ( + "bytes" + "context" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "net" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/peer" + "google.golang.org/grpc/status" + + "github.com/dgraph-io/dgo/v250/protos/api" + "github.com/dgraph-io/dgraph/v25/acl" + "github.com/dgraph-io/dgraph/v25/protos/pb" + "github.com/dgraph-io/dgraph/v25/worker" + "github.com/dgraph-io/dgraph/v25/x" +) + +// capabilitySites is every operation that requires a capability, and which one. +// +// This is a decision table, not a description. Each entry was AuthSuperAdmin or +// AuthorizeGuardians before capabilities existed, and the mapping is where the +// judgement lives: whether an operation is cluster-wide, tenant-scoped, or a +// cross-tenant write. Getting one wrong grants or removes real authority. +// +// TestEveryCapabilitySiteIsDeclared parses the source and requires this table to +// match, in both directions. A silent reclassification is the failure it exists to +// prevent — changing the constant at a call site is a one-word edit that reads as +// harmless in review, and the tests that would catch it are integration tests that +// only cover the paths someone thought to exercise. +var capabilitySites = map[string]Capability{ + // Cluster-wide authority. + "edgraph/server.go:alter": CapClusterAdmin, // drop-all + "edgraph/namespace.go:CreateNamespace": CapClusterAdmin, + "edgraph/namespace.go:DropNamespace": CapClusterAdmin, + "edgraph/namespace.go:ListNamespaces": CapClusterAdmin, + + "graphql/resolve/middlewares.go:resolveGuardianOfTheGalaxyAuth": CapClusterAdmin, + // Leasing UIDs. Not administration: `dgraph live` does it on every run, which is + // why it is not CapClusterAdmin. + "edgraph/zero.go:AllocateIDs": CapLeaseUIDs, + + // Acting in a namespace other than the caller's own. Distinct from + // CapClusterAdmin even though the built-in policy resolves them identically — + // the live loader needs a cross-tenant write and has no business dropping the + // cluster. + "edgraph/server.go:parseSchemaFromAlterOperation": CapAssumeTenant, + "edgraph/server.go:doQuery": CapAssumeTenant, + + // Administrative within a tenant: guardians of any namespace, which is what + // these checked before. + "edgraph/server.go:Health": CapTenantAdmin, + "edgraph/server.go:State": CapTenantAdmin, + "edgraph/server.go:UpdateExtSnapshotStreamingState": CapTenantAdmin, + "edgraph/server.go:StreamExtSnapshot": CapTenantAdmin, + "graphql/resolve/middlewares.go:resolveGuardianAuth": CapTenantAdmin, +} + +// capabilityScanRoot is the repo root, walked in full. +// +// This was a list of directories, which is how it came to miss +// a caller in a package nobody thought to add to the list. A decision table whose +// scan can miss a decision site is worse than no table: it reports a completeness it +// has not checked. Walking everything removes the class, and keeps working for a +// caller added in a package that does not exist yet. +const capabilityScanRoot = ".." + +// capabilityScanSkip are directories with nothing to say about capabilities and a lot +// of files, skipped to keep the walk quick. +var capabilityScanSkip = map[string]bool{ + ".git": true, "vendor": true, "testdata": true, "protos": true, + "compose": true, "contrib": true, ".trunk": true, "systest": true, +} + +// scanCapabilitySites parses the source and returns "dir/file.go:FuncName" for +// every AuthorizeCapability call, mapped to the capability constant it passes. +func scanCapabilitySites(t *testing.T) map[string]string { + t.Helper() + found := make(map[string]string) + var parsed int + + err := filepath.WalkDir(capabilityScanRoot, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if capabilityScanSkip[d.Name()] { + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + // Cheap pre-filter: parsing every Go file in the repo is wasteful when almost + // none of them mention the function. + src, readErr := os.ReadFile(path) + if readErr != nil { + return readErr + } + if !bytes.Contains(src, []byte("AuthorizeCapability")) { + return nil + } + parsed++ + + fset := token.NewFileSet() + af, parseErr := parser.ParseFile(fset, path, src, 0) + require.NoError(t, parseErr, "parsing %s", path) + + for _, decl := range af.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Body == nil { + continue + } + // The dispatcher and the interface method take a capability as a parameter + // rather than naming one, so they are not decision sites. + if fd.Name.Name == "AuthorizeCapability" { + continue + } + ast.Inspect(fd.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || callee(call.Fun) != "AuthorizeCapability" || len(call.Args) < 2 { + return true + } + key := sitePath(path) + ":" + fd.Name.Name + if prev, dup := found[key]; dup && prev != identName(call.Args[1]) { + t.Errorf("%s gates on two different capabilities (%s and %s); split the "+ + "function or the table cannot describe it", key, prev, identName(call.Args[1])) + } + found[key] = identName(call.Args[1]) + return true + }) + } + return nil + }) + require.NoError(t, err) + require.Positive(t, parsed, "the walk parsed no files mentioning AuthorizeCapability; "+ + "capabilityScanRoot is wrong and this test proves nothing") + return found +} + +func callee(fn ast.Expr) string { + switch f := fn.(type) { + case *ast.Ident: + return f.Name + case *ast.SelectorExpr: + return f.Sel.Name + } + return "" +} + +func identName(e ast.Expr) string { + switch a := e.(type) { + case *ast.Ident: + return a.Name + case *ast.SelectorExpr: + return a.Sel.Name + } + return "" +} + +// sitePath normalizes a walked path to be repo-relative, so the table reads the same +// regardless of where the test runs from. +func sitePath(path string) string { + clean := filepath.ToSlash(filepath.Clean(path)) + return strings.TrimPrefix(clean, "../") +} + +// capabilityNames maps the constant identifiers back to values, so the table can +// be written in terms of the constants while the scan sees only names. +var capabilityNames = map[string]Capability{ + "CapClusterAdmin": CapClusterAdmin, + "CapTenantAdmin": CapTenantAdmin, + "CapAssumeTenant": CapAssumeTenant, + "CapLeaseUIDs": CapLeaseUIDs, +} + +func TestEveryCapabilitySiteIsDeclared(t *testing.T) { + found := scanCapabilitySites(t) + + for site, gotName := range found { + want, declared := capabilitySites[site] + if !declared { + t.Errorf("%s gates on %s but is not in capabilitySites. Add it, and say why that "+ + "capability rather than another — the table is where the decision is recorded.", + site, gotName) + continue + } + got, known := capabilityNames[gotName] + if !known { + t.Errorf("%s passes %s, which is not a known capability constant", site, gotName) + continue + } + if got != want { + t.Errorf("%s gates on %v, but capabilitySites says %v. If the change is intended, "+ + "change the table too — that is the review the constant alone does not get.", + site, got, want) + } + } + + for site := range capabilitySites { + if _, ok := found[site]; !ok { + t.Errorf("capabilitySites declares %s, which no longer gates on a capability. "+ + "Either the check was dropped — which is a privilege change — or it moved and "+ + "the table needs updating.", site) + } + } +} + +// TestCapabilitySiteCoverage guards against the whole table being deleted or the +// scan silently matching nothing, which would make the test above vacuously pass. +func TestCapabilitySiteCoverage(t *testing.T) { + found := scanCapabilitySites(t) + require.NotEmpty(t, found, "the scan found no capability sites at all") + require.Len(t, found, len(capabilitySites)) + + // Every capability must be exercised somewhere, or it is dead surface. + var used []Capability + for _, c := range capabilitySites { + if !slices.Contains(used, c) { + used = append(used, c) + } + } + require.Len(t, used, len(capabilityNames), "some capability has no call site") +} + +// fromIP builds a context that looks like a gRPC request from ip, which is what +// the IP whitelist reads. Every test harness sets whitelist=0.0.0.0/0, so without +// constructing this by hand the deny half of break-glass is never exercised. +func fromIP(t *testing.T, ip string) context.Context { + t.Helper() + addr, err := net.ResolveTCPAddr("tcp", net.JoinHostPort(ip, "50051")) + require.NoError(t, err) + return peer.NewContext(context.Background(), &peer.Peer{Addr: addr}) +} + +// withAuthToken adds the --security token header a caller would present. +func withAuthToken(ctx context.Context, token string) context.Context { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + md = metadata.New(nil) + } else { + md = md.Copy() + } + md.Set("auth-token", token) + return metadata.NewIncomingContext(ctx, md) +} + +// aclCtx builds a request context carrying a signed ACL token, resolved to a +// Principal the way the identity interceptor does. +func aclCtx(t *testing.T, namespace uint64, userID string, groups []string) context.Context { + t.Helper() + token := generateJWT(namespace, userID, groups, time.Now().Add(30*time.Minute).Unix()) + ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs("accessJwt", token)) + return x.WithResolvedIdentity(ctx) +} + +// TestPredicateACLCapabilities pins the built-in policy's behavior, including the +// error codes and the message text. Both matter beyond this package: several +// endpoints wrap the message and clients distinguish Unauthenticated from +// PermissionDenied. +func TestPredicateACLCapabilities(t *testing.T) { + prevEnabled, prevSecret := x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey + t.Cleanup(func() { + x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey = prevEnabled, prevSecret + }) + + policy := predicateACL{} + + t.Run("with ACL off, cluster authority requires break-glass", func(t *testing.T) { + x.WorkerConfig.AclEnabled = false + worker.Config.AclSecretKey = nil + + // This is the behavior change. Before, an ACL-off cluster granted + // CapClusterAdmin to anyone, which is why CreateNamespace, DropNamespace, + // ListNamespaces, and AllocateIDs were reachable by any client that could + // open a connection. + err := policy.AuthorizeCapability(context.Background(), CapClusterAdmin) + require.Error(t, err, "an unattributable caller must not be a cluster admin") + require.Equal(t, codes.PermissionDenied, status.Code(err)) + require.Contains(t, err.Error(), "break-glass", + "the denial should name the route an operator can actually use") + + // A whitelisted source IP with no auth token configured is the default + // break-glass configuration, and it grants. + require.NoError(t, policy.AuthorizeCapability(fromIP(t, "127.0.0.1"), CapClusterAdmin)) + + // A source IP outside the whitelist does not, which is the half of the rule + // every test cluster hides: the harnesses all set whitelist=0.0.0.0/0. + err = policy.AuthorizeCapability(fromIP(t, "203.0.113.7"), CapClusterAdmin) + require.Error(t, err) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + + // The others are unchanged with ACL off, each for its own reason recorded at + // the switch in predicate_acl.go. Asserted here so that changing any of them + // is a deliberate act rather than a side effect. + require.NoError(t, policy.AuthorizeCapability(context.Background(), CapAssumeTenant), + "tightening this is the deferred fail-closed galaxy-operation change") + require.NoError(t, policy.AuthorizeCapability(context.Background(), CapTenantAdmin), + "tightening this would newly gate /state and /health?all") + require.NoError(t, policy.AuthorizeCapability(context.Background(), CapLeaseUIDs), + "`dgraph live` leases UIDs on every run against an ACL-off cluster and has "+ + "never needed a credential; requiring one is a loader-breaking change") + }) + + t.Run("with ACL off, the auth token is required when one is configured", func(t *testing.T) { + x.WorkerConfig.AclEnabled = false + worker.Config.AclSecretKey = nil + prevToken := worker.Config.AuthToken + t.Cleanup(func() { worker.Config.AuthToken = prevToken }) + worker.Config.AuthToken = "operator-token" + + // Whitelisted IP alone is no longer enough once a token exists. + err := policy.AuthorizeCapability(fromIP(t, "127.0.0.1"), CapClusterAdmin) + require.Error(t, err, "a configured auth token must be presented") + require.Equal(t, codes.PermissionDenied, status.Code(err)) + + require.NoError(t, policy.AuthorizeCapability( + withAuthToken(fromIP(t, "127.0.0.1"), "operator-token"), CapClusterAdmin)) + + err = policy.AuthorizeCapability( + withAuthToken(fromIP(t, "127.0.0.1"), "wrong-token"), CapClusterAdmin) + require.Error(t, err, "a wrong token must not grant") + }) + + t.Run("with ACL on, break-glass is not a second route to cluster admin", func(t *testing.T) { + x.WorkerConfig.AclEnabled = true + worker.Config.AclSecretKey = x.Sensitive("6ABBAA2014CFF00289D20D20DA296F67") + prevToken := worker.Config.AuthToken + t.Cleanup(func() { worker.Config.AuthToken = prevToken }) + worker.Config.AuthToken = "operator-token" + + // Satisfying break-glass completely, but presenting no ACL token. Granting + // here would let the --security token administer the cluster with no ACL + // identity at all, which is a loosening rather than the tightening this + // change is for. + ctx := withAuthToken(fromIP(t, "127.0.0.1"), "operator-token") + err := policy.AuthorizeCapability(ctx, CapClusterAdmin) + require.Error(t, err, "with ACL on, guardianship is the only route") + require.NotContains(t, err.Error(), "break-glass", + "the ACL-on denial should come from the ACL rule, not the source list") + }) + + x.WorkerConfig.AclEnabled = true + worker.Config.AclSecretKey = x.Sensitive("6ABBAA2014CFF00289D20D20DA296F67") + + tests := []struct { + name string + ctx func(t *testing.T) context.Context + cap Capability + wantCode codes.Code + wantMsg string + }{ + { + name: "a root-namespace guardian is a cluster admin", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 0, "groot", []string{"guardians"}) }, + cap: CapClusterAdmin, + }, + { + name: "and may assume another tenant", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 0, "groot", []string{"guardians"}) }, + cap: CapAssumeTenant, + }, + { + name: "and is a tenant admin", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 0, "groot", []string{"guardians"}) }, + cap: CapTenantAdmin, + }, + { + name: "a guardian of another namespace is not a cluster admin", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 7, "groot", []string{"guardians"}) }, + cap: CapClusterAdmin, + wantCode: codes.PermissionDenied, + wantMsg: "Only superadmin is allowed to do this operation", + }, + { + name: "nor may it assume another tenant", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 7, "groot", []string{"guardians"}) }, + cap: CapAssumeTenant, + wantCode: codes.PermissionDenied, + wantMsg: "Only superadmin is allowed to do this operation", + }, + { + name: "but it is a tenant admin, which is the distinction", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 7, "groot", []string{"guardians"}) }, + cap: CapTenantAdmin, + }, + { + name: "a non-guardian in the root namespace is denied", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 0, "alice", []string{"dev"}) }, + cap: CapClusterAdmin, + wantCode: codes.PermissionDenied, + wantMsg: "is not a member of guardians group", + }, + { + name: "a non-guardian is not a tenant admin either", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 0, "alice", []string{"dev"}) }, + cap: CapTenantAdmin, + wantCode: codes.PermissionDenied, + wantMsg: "is not a member of guardians group", + }, + { + name: "a caller with no groups at all is denied", + ctx: func(t *testing.T) context.Context { return aclCtx(t, 0, "alice", nil) }, + cap: CapClusterAdmin, + wantCode: codes.PermissionDenied, + wantMsg: "is not a member of guardians group", + }, + { + name: "no token is PermissionDenied for a tenant admin", + ctx: func(t *testing.T) context.Context { return context.Background() }, + cap: CapTenantAdmin, + wantCode: codes.PermissionDenied, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := policy.AuthorizeCapability(tt.ctx(t), tt.cap) + if tt.wantCode == codes.OK { + require.NoError(t, err) + return + } + require.Error(t, err) + require.Equal(t, tt.wantCode, status.Code(err), "err=%v", err) + if tt.wantMsg != "" { + require.Contains(t, err.Error(), tt.wantMsg) + } + }) + } +} + +// TestUnknownCapabilityFailsClosed covers the branch that has no case. A +// capability added without a rule must be denied, because the alternative reading +// leaves every future addition unrestricted until someone notices. +func TestUnknownCapabilityFailsClosed(t *testing.T) { + err := predicateACL{}.AuthorizeCapability(context.Background(), Capability(99)) + require.Error(t, err) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + require.Contains(t, err.Error(), "no rule for") + require.Equal(t, "unknown-capability", Capability(99).String()) +} + +// fakeController records what it was asked and grants everything. +type fakeController struct { + asked []Capability + askedPreds [][]string +} + +func (*fakeController) Name() string { return "fake" } + +func (f *fakeController) AuthorizeCapability(_ context.Context, c Capability) error { + f.asked = append(f.asked, c) + return nil +} + +func (f *fakeController) AuthorizePredicates(_ context.Context, preds []string, + _ *acl.Operation) (*PredResult, error) { + f.askedPreds = append(f.askedPreds, preds) + return &PredResult{Blocked: map[string]struct{}{}}, nil +} + +func TestSetAccessController(t *testing.T) { + prevEnabled := x.WorkerConfig.AclEnabled + t.Cleanup(func() { + x.WorkerConfig.AclEnabled = prevEnabled + SetAccessController(nil) + }) + // ACL on, so the built-in policy would deny an unauthenticated caller. If the + // installed one is consulted instead, the call succeeds. + x.WorkerConfig.AclEnabled = true + + require.Equal(t, "predicate-acl", currentAccessController().Name()) + + fake := &fakeController{} + SetAccessController(fake) + require.Equal(t, "fake", currentAccessController().Name()) + require.NoError(t, AuthorizeCapability(context.Background(), CapClusterAdmin)) + require.Equal(t, []Capability{CapClusterAdmin}, fake.asked) + + SetAccessController(nil) + require.Equal(t, "predicate-acl", currentAccessController().Name(), + "nil must restore the built-in policy") +} + +// TestCapabilityStringsAreStable guards the names that appear in the fail-closed +// error, which is what tells an operator which capability to grant. +func TestCapabilityStringsAreStable(t *testing.T) { + require.Equal(t, "cluster-admin", CapClusterAdmin.String()) + require.Equal(t, "tenant-admin", CapTenantAdmin.String()) + require.Equal(t, "assume-tenant", CapAssumeTenant.String()) +} + +// erroringController stands in for a policy that cannot reach an answer — a +// one backed by a remote authorization service whose query fails, say. +type erroringController struct{} + +func (erroringController) Name() string { return "erroring" } +func (erroringController) AuthorizeCapability(context.Context, Capability) error { + return status.Error(codes.Unavailable, "policy unavailable") +} +func (erroringController) AuthorizePredicates(context.Context, []string, + *acl.Operation) (*PredResult, error) { + return nil, status.Error(codes.Unavailable, "policy unavailable") +} + +// TestAuthorizePredicatesGoesThroughThePolicy checks the plumbing rather than +// ACL's rules — those are covered end to end by acl/acl_test.go against a live +// cluster, which is the gate for this change. +func TestAuthorizePredicatesGoesThroughThePolicy(t *testing.T) { + t.Cleanup(func() { SetAccessController(nil) }) + + fake := &fakeController{} + SetAccessController(fake) + + res, err := AuthorizePredicates(context.Background(), []string{"name", "age"}, acl.Read) + require.NoError(t, err) + require.NotNil(t, res) + require.Equal(t, [][]string{{"name", "age"}}, fake.askedPreds) + + SetAccessController(nil) + require.Equal(t, "predicate-acl", currentAccessController().Name()) +} + +// TestPredicateErrorIsNotReadAsPermitted is the property the new error return +// creates a way to get wrong. +// +// authorizePreds could not fail, so every call site treated "no blocked +// predicates" as the only outcome. A policy that has to query the graph can fail, +// and an empty PredResult from a failed call would look exactly like "nothing was +// refused" — which is to say, like permission. Each site must return the error +// instead. +func TestPredicateErrorIsNotReadAsPermitted(t *testing.T) { + prevEnabled, prevSecret := x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey + t.Cleanup(func() { + x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey = prevEnabled, prevSecret + SetAccessController(nil) + }) + x.WorkerConfig.AclEnabled = true + worker.Config.AclSecretKey = x.Sensitive("6ABBAA2014CFF00289D20D20DA296F67") + SetAccessController(erroringController{}) + + // A non-guardian, so the superadmin short-circuit does not fire and the + // predicate decision is actually consulted. + ctx := aclCtx(t, 0, "alice", []string{"dev"}) + + err := authorizeAlter(ctx, &api.Operation{Schema: "name: string ."}) + require.Error(t, err, "a policy that could not decide must not be read as permitting") + require.Equal(t, codes.Unavailable, status.Code(err), + "the policy's own code should survive rather than becoming PermissionDenied") +} + +// TestPredResultSentinel pins the meaning of a nil Allowed, which is the contract +// the interface had to carry over and the one a plain error return could not +// express. Nil means every predicate; an empty slice means none. +func TestPredResultSentinel(t *testing.T) { + all := &PredResult{Allowed: nil, Blocked: map[string]struct{}{"dgraph.xid": {}}} + require.Nil(t, all.Allowed, "nil Allowed is 'all predicates', and Blocked still applies") + require.Len(t, all.Blocked, 1) + + none := &PredResult{Allowed: []string{}, Blocked: map[string]struct{}{}} + require.NotNil(t, none.Allowed, "an empty slice is 'no predicates' and must not be nil") + require.Empty(t, none.Allowed) +} + +// TestClusterAdminDoesNotTrustClientNamespace is the regression test for a +// privilege escalation introduced by the tenancy rekeying. +// +// authSuperAdmin's `ns != 0` test asks whether the CALLER is rooted in the root +// namespace. That is a property of the credential, not of where the request is +// being routed — so it must read the signed claim. Rekeying it onto +// requestNamespace made it prefer x.ExtractNamespace, which reads md["namespace"] +// from incoming metadata, and that is client-controlled server-side. +// +// The four RPCs that gate on CapClusterAdmin and nothing else — CreateNamespace, +// DropNamespace, ListNamespaces, AllocateIDs — call AuthorizeCapability as their +// first statement, with no ResolveTenant before it. So on exactly those paths the +// metadata is whatever the caller sent, and a guardian of any tenant could claim +// namespace 0 and administer the cluster. +func TestClusterAdminDoesNotTrustClientNamespace(t *testing.T) { + prevEnabled, prevSecret := x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey + t.Cleanup(func() { + x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey = prevEnabled, prevSecret + }) + x.WorkerConfig.AclEnabled = true + worker.Config.AclSecretKey = x.Sensitive("6ABBAA2014CFF00289D20D20DA296F67") + + // A real guardian, but of namespace 7 — not a cluster admin. + token := generateJWT(7, "tenant-groot", []string{"guardians"}, time.Now().Add(30*time.Minute).Unix()) + + t.Run("a tenant guardian claiming namespace 0 in metadata is denied", func(t *testing.T) { + // The forgery: a valid ns-7 token, plus metadata asserting the request is in + // namespace 0. No ResolveTenant runs on this path to overwrite it. + ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs( + "accessJwt", token, + "namespace", "0", + )) + ctx = x.WithResolvedIdentity(ctx) + + err := predicateACL{}.AuthorizeCapability(ctx, CapClusterAdmin) + require.Error(t, err, "client-supplied namespace metadata must not confer cluster admin") + require.Equal(t, codes.PermissionDenied, status.Code(err)) + require.Contains(t, err.Error(), "Only superadmin is allowed to do this operation") + }) + + t.Run("the same token is still denied with no namespace metadata at all", func(t *testing.T) { + ctx := x.WithResolvedIdentity(metadata.NewIncomingContext( + context.Background(), metadata.Pairs("accessJwt", token))) + err := predicateACL{}.AuthorizeCapability(ctx, CapClusterAdmin) + require.Error(t, err) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + }) + + t.Run("a genuine root guardian is still allowed", func(t *testing.T) { + rootToken := generateJWT(0, "groot", []string{"guardians"}, time.Now().Add(30*time.Minute).Unix()) + ctx := x.WithResolvedIdentity(metadata.NewIncomingContext( + context.Background(), metadata.Pairs("accessJwt", rootToken))) + require.NoError(t, predicateACL{}.AuthorizeCapability(ctx, CapClusterAdmin)) + }) + + t.Run("a root guardian is allowed even if metadata claims another namespace", func(t *testing.T) { + // The mirror image: the claim governs, so hostile metadata cannot demote + // a legitimate cluster admin either. + rootToken := generateJWT(0, "groot", []string{"guardians"}, time.Now().Add(30*time.Minute).Unix()) + ctx := x.WithResolvedIdentity(metadata.NewIncomingContext(context.Background(), + metadata.Pairs("accessJwt", rootToken, "namespace", "7"))) + require.NoError(t, predicateACL{}.AuthorizeCapability(ctx, CapClusterAdmin)) + }) +} + +// TestFilterTabletsDoesNotTrustClientNamespace is the /state half of the same +// escalation. filterTablets returns early without filtering when the namespace is +// the root one, and State reaches it with no ResolveTenant ahead of it — so reading +// md["namespace"] let a guardian of any tenant ask for namespace 0 and receive +// every tenant's predicates. +func TestFilterTabletsDoesNotTrustClientNamespace(t *testing.T) { + prevEnabled := x.WorkerConfig.AclEnabled + t.Cleanup(func() { x.WorkerConfig.AclEnabled = prevEnabled }) + x.WorkerConfig.AclEnabled = true + + state := func() *pb.MembershipState { + return &pb.MembershipState{Groups: map[uint32]*pb.Group{1: {Tablets: map[string]*pb.Tablet{ + x.NamespaceAttr(7, "mine"): {Predicate: x.NamespaceAttr(7, "mine")}, + x.NamespaceAttr(9, "theirs"): {Predicate: x.NamespaceAttr(9, "theirs")}, + x.NamespaceAttr(0, "rootpred"): {Predicate: x.NamespaceAttr(0, "rootpred")}, + }}}} + } + token := generateJWT(7, "tenant-groot", []string{"guardians"}, time.Now().Add(30*time.Minute).Unix()) + + t.Run("claiming namespace 0 in metadata does not lift the filter", func(t *testing.T) { + ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs( + "accessJwt", token, "namespace", "0")) + ms := state() + require.NoError(t, filterTablets(ctx, ms)) + + tablets := ms.GetGroups()[1].GetTablets() + require.Contains(t, tablets, "mine", "the caller's own tenant is visible") + require.NotContains(t, tablets, "theirs", "another tenant's predicate must not be disclosed") + require.NotContains(t, tablets, "rootpred", "the root namespace's predicate must not be disclosed") + require.Len(t, tablets, 1) + }) + + t.Run("a genuine root guardian still sees everything", func(t *testing.T) { + rootToken := generateJWT(0, "groot", []string{"guardians"}, time.Now().Add(30*time.Minute).Unix()) + ctx := metadata.NewIncomingContext(context.Background(), + metadata.Pairs("accessJwt", rootToken)) + ms := state() + require.NoError(t, filterTablets(ctx, ms)) + require.Len(t, ms.GetGroups()[1].GetTablets(), 3, "root sees unfiltered state") + }) +} + +// identitySource is a stand-in for an identity-based CapabilitySource, e.g. +// --external-identity cluster-admin-clients. +type identitySource struct { + subject string + asked int +} + +func (*identitySource) Name() string { return "test-identity-source" } +func (s *identitySource) Grants(_ context.Context, p *x.Principal, c Capability) bool { + s.asked++ + return c == CapClusterAdmin && p != nil && p.Subject == s.subject +} + +// TestCapabilitySourcesAreConsultedUnderACL is the regression test for a silent +// misconfiguration: authorizeClusterAdmin returned authSuperAdmin's result directly +// whenever ACL was enabled, so no registered source was ever asked. A deployment +// that configured cluster-admin-clients AND ran ACL would have found the roster +// inert, with nothing in the logs or the error saying so. +func TestCapabilitySourcesAreConsultedUnderACL(t *testing.T) { + prevEnabled, prevSecret := x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey + prevSources := capabilitySources + t.Cleanup(func() { + x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey = prevEnabled, prevSecret + capabilitySources = prevSources + }) + x.WorkerConfig.AclEnabled = true + worker.Config.AclSecretKey = x.Sensitive("6ABBAA2014CFF00289D20D20DA296F67") + + src := &identitySource{subject: "migrator"} + capabilitySources = append(prevSources, src) + + admin := &x.Principal{ + Issuer: "https://idp.example/identity", + Subject: "migrator", + Claims: map[string]any{"user_type": "agent"}, + Method: x.MethodExternalJWT, + } + + t.Run("a source can grant even while ACL is enabled", func(t *testing.T) { + ctx := x.WithPrincipal(context.Background(), admin) + require.NoError(t, predicateACL{}.AuthorizeCapability(ctx, CapClusterAdmin)) + require.Positive(t, src.asked, "the source must actually be consulted") + }) + + t.Run("a caller no source recognizes still gets the ACL error", func(t *testing.T) { + // A real guardian of a non-root namespace, so the ACL rule produces its own + // PermissionDenied. The point is that consulting sources must not replace + // that message with the generic source-list one: an operator debugging an + // ACL deployment needs to be told about guardianship, not about + // --security whitelist. + token := generateJWT(7, "tenant-groot", []string{"guardians"}, + time.Now().Add(30*time.Minute).Unix()) + ctx := x.WithResolvedIdentity(metadata.NewIncomingContext( + context.Background(), metadata.Pairs("accessJwt", token))) + + err := predicateACL{}.AuthorizeCapability(ctx, CapClusterAdmin) + require.Error(t, err) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + require.Contains(t, err.Error(), "Only superadmin is allowed to do this operation", + "the ACL rule's own text must survive rather than the generic denial") + require.NotContains(t, err.Error(), "With ACL disabled it comes from") + }) + + t.Run("a source is not asked for a capability it does not grant", func(t *testing.T) { + ctx := x.WithPrincipal(context.Background(), admin) + // CapAssumeTenant has no source route; the ACL rule decides alone. + err := predicateACL{}.AuthorizeCapability(ctx, CapAssumeTenant) + require.Error(t, err, "naming a cluster-admin client must not confer assume-tenant") + }) +} + +// TestBreakGlassSelfExcludesUnderACL pins where the ACL-on restriction now lives. +// It moved from authorizeClusterAdmin into the source so that it applies to the one +// route it was reasoned about — a shared secret plus an IP range — rather than to +// every source anyone adds later. +func TestBreakGlassSelfExcludesUnderACL(t *testing.T) { + prevEnabled, prevToken := x.WorkerConfig.AclEnabled, worker.Config.AuthToken + t.Cleanup(func() { + x.WorkerConfig.AclEnabled, worker.Config.AuthToken = prevEnabled, prevToken + }) + worker.Config.AuthToken = "" + + // Fully satisfying break-glass: a loopback caller with no token configured. + ctx := fromIP(t, "127.0.0.1") + + x.WorkerConfig.AclEnabled = false + require.True(t, breakGlassSource{}.Grants(ctx, nil, CapClusterAdmin), + "with ACL off, break-glass is the route") + + x.WorkerConfig.AclEnabled = true + require.False(t, breakGlassSource{}.Grants(ctx, nil, CapClusterAdmin), + "with ACL on, the --security token must not be a second route to cluster admin") +} + +// TestClusterAdminWithoutATokenIsUnauthenticated pins the status code on the +// credential-missing path. +// +// authSuperAdmin wrapped ExtractNamespaceFrom's plain error, and every caller +// propagates status.Convert(err).Code() — so an absent or expired token on +// CreateNamespace, DropNamespace, ListNamespaces or the GraphQL admin surface arrived +// as codes.Unknown. A client cannot distinguish that from a server fault, and +// authorizeGuardians twelve lines below already classified the same failure as +// Unauthenticated. +func TestClusterAdminWithoutATokenIsUnauthenticated(t *testing.T) { + prevEnabled, prevSecret := x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey + t.Cleanup(func() { + x.WorkerConfig.AclEnabled, worker.Config.AclSecretKey = prevEnabled, prevSecret + }) + x.WorkerConfig.AclEnabled = true + worker.Config.AclSecretKey = x.Sensitive("6ABBAA2014CFF00289D20D20DA296F67") + + for _, tt := range []struct { + name string + ctx context.Context + }{ + {"no metadata at all", context.Background()}, + {"metadata but no token", metadata.NewIncomingContext( + context.Background(), metadata.Pairs("namespace", "0"))}, + {"a token that does not parse", metadata.NewIncomingContext( + context.Background(), metadata.Pairs("accessJwt", "not-a-jwt"))}, + } { + t.Run(tt.name, func(t *testing.T) { + err := predicateACL{}.AuthorizeCapability(tt.ctx, CapClusterAdmin) + require.Error(t, err) + require.Equal(t, codes.Unauthenticated, status.Code(err), + "a credential problem must not reach the client as Unknown") + }) + } + + t.Run("an expired token is also Unauthenticated", func(t *testing.T) { + expired := generateJWT(0, "groot", []string{"guardians"}, + time.Now().Add(-time.Hour).Unix()) + ctx := metadata.NewIncomingContext(context.Background(), + metadata.Pairs("accessJwt", expired)) + err := predicateACL{}.AuthorizeCapability(ctx, CapClusterAdmin) + require.Error(t, err) + require.Equal(t, codes.Unauthenticated, status.Code(err)) + }) +} diff --git a/edgraph/namespace.go b/edgraph/namespace.go index f206d5ff60c..68947a8080c 100644 --- a/edgraph/namespace.go +++ b/edgraph/namespace.go @@ -20,7 +20,7 @@ import ( func (s *Server) CreateNamespace(ctx context.Context, in *api.CreateNamespaceRequest) ( *api.CreateNamespaceResponse, error) { - if err := AuthSuperAdmin(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapClusterAdmin); err != nil { s := status.Convert(err) return nil, status.Error(s.Code(), "Non superadmin user cannot create namespace. "+s.Message()) @@ -39,7 +39,7 @@ func (s *Server) CreateNamespace(ctx context.Context, in *api.CreateNamespaceReq func (s *Server) DropNamespace(ctx context.Context, in *api.DropNamespaceRequest) ( *api.DropNamespaceResponse, error) { - if err := AuthSuperAdmin(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapClusterAdmin); err != nil { s := status.Convert(err) return nil, status.Error(s.Code(), "Non superadmin user cannot drop namespace. "+s.Message()) @@ -65,7 +65,7 @@ func (s *Server) DropNamespace(ctx context.Context, in *api.DropNamespaceRequest func (s *Server) ListNamespaces(ctx context.Context, in *api.ListNamespacesRequest) ( *api.ListNamespacesResponse, error) { - if err := AuthSuperAdmin(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapClusterAdmin); err != nil { s := status.Convert(err) return nil, status.Error(s.Code(), "Non superadmin user cannot list namespaces. "+s.Message()) diff --git a/edgraph/predicate_acl.go b/edgraph/predicate_acl.go new file mode 100644 index 00000000000..e014a244aec --- /dev/null +++ b/edgraph/predicate_acl.go @@ -0,0 +1,306 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package edgraph + +import ( + "context" + "fmt" + + "github.com/golang/glog" + "github.com/pkg/errors" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/dgraph-io/dgraph/v25/acl" + "github.com/dgraph-io/dgraph/v25/worker" + "github.com/dgraph-io/dgraph/v25/x" +) + +// predicateACL is the built-in policy: Dgraph's own ACL, and the only one an OSS +// build has. +type predicateACL struct{} + +func (predicateACL) Name() string { return "predicate-acl" } + +func (predicateACL) AuthorizeCapability(ctx context.Context, c Capability) error { + switch c { + case CapClusterAdmin: + return authorizeClusterAdmin(ctx) + case CapAssumeTenant: + // Deliberately still authSuperAdmin, which grants unconditionally when ACL + // is off. Tightening it is the fail-closed galaxy-operation change, and it + // has a cost the cluster-admin case does not: the live and bulk loaders use + // --force_namespace against clusters that may have no ACL, so they would + // need a whitelisted IP or an auth token from wherever they happen to run. + // That wants its own flag-gated release rather than riding along here. + // + // The other CapAssumeTenant site, parseSchemaFromAlterOperation, is reached + // only from alter, which already required break-glass before it got here — + // so the gap is the loader path specifically. + return authSuperAdmin(ctx) + case CapLeaseUIDs: + // authSuperAdmin unchanged, which means open when ACL is off. See the constant + // for why: `dgraph live` leases UIDs through this on every run against an + // ACL-off cluster, and it has never needed a credential to do so. + return authSuperAdmin(ctx) + case CapTenantAdmin: + // Also unchanged, and also not an oversight. Its sites are Health(all), + // State, and the external-snapshot pair; the snapshot pair already adds + // hasPoormansAuth of its own. Requiring break-glass for the first two would + // newly gate /state and /health?all behind the IP whitelist, which is a + // monitoring-visible change with a blast radius of its own. Separate + // decision, separate change. + return authorizeGuardians(ctx) + } + // Fail closed. A capability with no case here is a wiring bug, and reading an + // unhandled capability as "granted" would make every future addition + // unrestricted until someone noticed. + return status.Errorf(codes.PermissionDenied, + "%s: no rule for the %s capability", predicateACL{}.Name(), c) +} + +// authorizeClusterAdmin is the one capability whose ACL-off behavior changed. +// +// With ACL enabled, this is exactly the old rule: a guardian of the root +// namespace. Break-glass is deliberately NOT consulted in that case. ACL already +// provides an identity-based route to cluster authority, and adding a +// secret-and-IP route beside it would let a caller with the --security token +// administer the cluster with no ACL token at all. That is a loosening, and no +// goal here needs it. +// +// With ACL disabled, the old rule granted cluster authority to *everyone*. That +// is the hole: CreateNamespace, DropNamespace, ListNamespaces, and AllocateIDs +// gate on this capability and nothing else, so on an ACL-off cluster they were +// reachable by any client that could open a connection. Now the same controls +// alter already requires — a whitelisted source IP, plus the --security auth +// token when one is configured — are what grant it. +// +// This is a real behavior change and the only one in this stage. It is defensible +// unflagged because it makes namespace lifecycle no stricter than schema change: +// alter has always called hasAdminAuth, so any deployment that alters a schema +// already has a working whitelist or token configuration, and the operations +// being closed are the more destructive ones. A deployment that genuinely wants +// the old behavior sets the same whitelist it already sets for alter. +func authorizeClusterAdmin(ctx context.Context) error { + if x.WorkerConfig.AclEnabled { + // Guardianship of the root namespace is the first route, and the only one an + // OSS build has. If it grants, nothing else needs asking. + aclErr := authSuperAdmin(ctx) + if aclErr == nil { + return nil + } + // Then the registered sources. Consulting them here rather than returning + // aclErr outright is the difference between a configured + // --external-identity cluster-admin-clients working and being silently + // inert on any cluster that also runs ACL. + // + // Which is not the same as re-admitting break-glass alongside ACL: that + // exclusion now lives in breakGlassSource.Grants, where it applies to the + // one source it was reasoned about rather than to every source there will + // ever be. + if name, ok := grantedBySource(ctx, CapClusterAdmin); ok { + glog.V(3).Infof("cluster-admin granted by %s", name) + return nil + } + // The ACL error, not a generic one: its code and message text are what + // callers and tests key on. + return aclErr + } + if name, ok := grantedBySource(ctx, CapClusterAdmin); ok { + glog.V(3).Infof("cluster-admin granted by %s", name) + return nil + } + return status.Errorf(codes.PermissionDenied, + "cluster-admin authority is required and this caller holds none. With ACL disabled it "+ + "comes from: %s. Configure --security whitelist and token, or enable --acl.", + sourceNames()) +} + +// breakGlassSource grants cluster authority to a caller that satisfies the +// operator-level controls rather than an identity: a whitelisted source IP, plus +// the --security auth token when one is configured. +// +// It is the recovery path, and it is why cluster authority never depends solely on +// an external identity provider. An outage there, a rotated key, or a bad config +// would otherwise lock an operator out of namespace lifecycle and drop-all. +type breakGlassSource struct{} + +func (breakGlassSource) Name() string { return "break-glass (--security whitelist/token)" } + +func (breakGlassSource) Grants(ctx context.Context, _ *x.Principal, c Capability) bool { + if c != CapClusterAdmin { + return false + } + // Not a second route while ACL is on. ACL already provides an identity-based + // path to cluster authority, and admitting a shared secret and an IP range + // beside it would let the --security token administer the cluster with no ACL + // token at all. Break-glass exists for the configuration that has no other + // path, which is ACL off. + if x.WorkerConfig.AclEnabled { + return false + } + // The two halves of hasAdminAuth, called directly rather than through it: that + // function logs every call at Info level, which is right for a rare admin RPC + // and wrong for a capability check that may run on any request. + if _, err := x.HasWhitelistedIP(ctx); err != nil { + return false + } + return hasPoormansAuth(ctx) == nil +} + +// authSuperAdmin authorizes a caller in the guardians group of the root namespace. +// It was edgraph.AuthSuperAdmin, called directly from eight places; it is now +// reached only through AuthorizeCapability. +// +// NOTE: the caller should not wrap the error returned. If needed, propagate the +// GRPC error code. +func authSuperAdmin(ctx context.Context) error { + if !x.WorkerConfig.AclEnabled { + return nil + } + // The signed claim, deliberately — not requestNamespace. + // + // This test asks whether the CALLER is rooted in the root namespace, which is a + // property of their credential, not of where the request is being routed. The + // rekeying that moved authorizePreds onto the resolved namespace was right for + // that site — it keys ACL lookups on the same channel storage keys on — and + // wrong here, because requestNamespace prefers x.ExtractNamespace, and that + // reads md["namespace"], which is client-controlled on the server side. + // + // The four RPCs gated on CapClusterAdmin and nothing else — CreateNamespace, + // DropNamespace, ListNamespaces, AllocateIDs — call AuthorizeCapability as their + // first statement, with no ResolveTenant before it to overwrite that metadata. + // So reading it there let a guardian of any tenant claim namespace 0 and + // administer the cluster, and let hostile metadata demote a real cluster admin. + ns, err := x.ExtractNamespaceFrom(ctx) + if err != nil { + // Unauthenticated, not a bare wrapped error. Every caller propagates + // status.Convert(err).Code(), and an unwrapped error converts to Unknown — so a + // missing or expired token on CreateNamespace, DropNamespace, ListNamespaces or + // the GraphQL admin surface looked like a server fault rather than a credential + // problem, and a client could not tell "log in again" from "retry later". + // authorizeGuardians below already classifies the same failure this way. + return status.Error(codes.Unauthenticated, + errors.Wrap(err, "Authorize guardian of the galaxy, extracting jwt token, error:").Error()) + } + if ns != 0 { + return status.Error( + codes.PermissionDenied, "Only superadmin is allowed to do this operation") + } + // authorizeGuardians will extract (user, []groups) from the JWT claims and will check if + // any of the group to which the user belongs is "guardians" or not. + if err := authorizeGuardians(ctx); err != nil { + s := status.Convert(err) + return status.Error( + s.Code(), "AuthSuperAdmin: failed to authorize guardians. "+s.Message()) + } + glog.V(3).Info("Successfully authorised guardian of the galaxy") + return nil +} + +// authorizeGuardians authorizes a caller in the guardians group of any namespace. +// It was edgraph.AuthorizeGuardians. +// +// NOTE: the caller should not wrap the error returned. If needed, propagate the +// GRPC error code. +func authorizeGuardians(ctx context.Context) error { + if worker.Config.AclSecretKey == nil { + // the user has not turned on the acl feature + return nil + } + + userData, err := extractUserAndGroups(ctx) + switch { + case errors.Is(err, x.ErrNoJwt): + return status.Error(codes.PermissionDenied, err.Error()) + case err != nil: + return status.Error(codes.Unauthenticated, err.Error()) + default: + userId := userData.userId + groupIds := userData.groupIds + + if !x.IsSuperAdmin(groupIds) { + // Deny access for members of non-guardian groups + return status.Error(codes.PermissionDenied, fmt.Sprintf("Only guardians are "+ + "allowed access. User '%v' is not a member of guardians group.", userId)) + } + } + + return nil +} + +// AuthorizePredicates is the built-in policy's predicate half: Dgraph's ACL +// rules, read from worker.AclCachePtr. +// +// It derives identity from ctx rather than taking it as a parameter, per the +// interface. The extra extractUserAndGroups is nearly free now that it reads the +// Principal the identity interceptor already resolved, and it keeps the signature +// free of ACL's own types. +func (predicateACL) AuthorizePredicates(ctx context.Context, preds []string, + op *acl.Operation) (*PredResult, error) { + + userData, err := extractUserAndGroups(ctx) + if err != nil { + return nil, status.Error(codes.Unauthenticated, err.Error()) + } + return authorizePreds(ctx, userData, preds, op), nil +} + +func authorizePreds(ctx context.Context, userData *userData, preds []string, + aclOp *acl.Operation) *PredResult { + + if !worker.AclCachePtr.Loaded() { + RefreshACLs(ctx) + } + + userId := userData.userId + groupIds := userData.groupIds + + // Key the ACL lookup on the namespace the storage layer will actually use. + // + // This used to read userData.namespace — the tenant claim inside the access + // token — while every read and write below keys on x.ExtractNamespace(ctx), + // the resolved tenancy. The two agree today only because the tenant resolver + // copies the claim into the context; they are separate channels, and + // authorizing against one while executing against the other is a + // confused-deputy bug waiting for the day a resolver derives the tenant from + // something other than that claim. + // + // Deliberately not a hard failure. If the context carries no resolved + // tenancy, fall back to the claim, which is what this read unconditionally + // before; a divergence is logged rather than rejected, so an unexpected one + // becomes visible without turning into an outage. + ns := namespaceOrClaim(ctx, userData.namespace, "authorizePreds") + + blockedPreds := make(map[string]struct{}) + for _, pred := range preds { + nsPred := x.NamespaceAttr(ns, pred) + if err := worker.AclCachePtr.AuthorizePredicate(groupIds, nsPred, aclOp); err != nil { + logAccess(&accessEntry{ + userId: userId, + groups: groupIds, + preds: preds, + operation: aclOp, + allowed: false, + }) + blockedPreds[pred] = struct{}{} + } + } + if worker.HasAccessToAllPreds(ns, groupIds, aclOp) { + // Setting allowed to nil allows access to all predicates. Note that the access to ACL + // predicates will still be blocked. + return &PredResult{Allowed: nil, Blocked: blockedPreds} + } + // User can have multiple permission for same predicate, add predicate + allowedPreds := make([]string, 0, len(worker.AclCachePtr.GetUserPredPerms(userId))) + // only if the acl.Op is covered in the set of permissions for the user + for predicate, perm := range worker.AclCachePtr.GetUserPredPerms(userId) { + if (perm & aclOp.Code) > 0 { + allowedPreds = append(allowedPreds, predicate) + } + } + return &PredResult{Allowed: allowedPreds, Blocked: blockedPreds} +} diff --git a/edgraph/server.go b/edgraph/server.go index e6673d18d50..8bcee62de2b 100644 --- a/edgraph/server.go +++ b/edgraph/server.go @@ -253,7 +253,7 @@ func parseSchemaFromAlterOperation(ctx context.Context, sch string) ( if x.IsRootNsOperation(ctx) { // Only the guardian of the galaxy can do a galaxy wide query/mutation. This operation is // needed by live loader. - if err := AuthSuperAdmin(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapAssumeTenant); err != nil { s := status.Convert(err) return nil, status.Error(s.Code(), "Non superadmin user cannot bypass namespaces. "+s.Message()) @@ -421,7 +421,7 @@ func (s *Server) alter(ctx context.Context, op *api.Operation, doAuth AuthMode) glog.V(2).Info("Blocked drop-all because it is not permitted.") return empty, errors.New("Drop all operation is not permitted.") } - if err := AuthSuperAdmin(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapClusterAdmin); err != nil { s := status.Convert(err) return empty, status.Error(s.Code(), "Drop all can only be called by the guardian of the galaxy. "+s.Message()) @@ -1248,7 +1248,7 @@ func (s *Server) Health(ctx context.Context, all bool) (*api.Response, error) { var healthAll []pb.HealthInfo if all { - if err := AuthorizeGuardians(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapTenantAdmin); err != nil { return nil, err } pool := conn.GetPools().GetAll() @@ -1288,6 +1288,10 @@ func filterTablets(ctx context.Context, ms *pb.MembershipState) error { if !x.WorkerConfig.AclEnabled { return nil } + // The signed claim, not the resolved namespace: this decides which tenant's + // tablets the CALLER may see, and State reaches here with no ResolveTenant + // ahead of it, so md["namespace"] would be whatever the caller sent. Reading it + // let a guardian of any tenant ask for namespace 0 and skip filtering entirely. namespace, err := x.ExtractNamespaceFrom(ctx) if err != nil { return errors.Errorf("Namespace not found in JWT.") @@ -1315,7 +1319,7 @@ func (s *Server) State(ctx context.Context) (*api.Response, error) { return nil, ctx.Err() } - if err := AuthorizeGuardians(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapTenantAdmin); err != nil { return nil, err } @@ -1513,7 +1517,7 @@ func (s *Server) doQuery(ctx context.Context, req *Request) (resp *api.Response, if req.doAuth == NeedAuthorize && x.IsRootNsOperation(ctx) { // Only the guardian of the galaxy can do a galaxy wide query/mutation. This operation is // needed by live loader. - if err := AuthSuperAdmin(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapAssumeTenant); err != nil { s := status.Convert(err) return nil, status.Error(s.Code(), "Non superadmin user cannot bypass namespaces. "+s.Message()) @@ -2095,7 +2099,7 @@ func (s *Server) UpdateExtSnapshotStreamingState(ctx context.Context, // it is protected under ACL and under an --security auth-token. Each gate fails open when // its feature is unconfigured, so the arming requirement on the stream path backstops the // bare-OSS case. - if err := AuthorizeGuardians(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapTenantAdmin); err != nil { return nil, err } if err := hasPoormansAuth(ctx); err != nil { @@ -2122,7 +2126,7 @@ func (s *Server) StreamExtSnapshot(stream api.Dgraph_StreamExtSnapshotServer) er // Authorize at stream start, before any data is consumed. Stream auth metadata rides on the // stream's context, so the same gates used for the unary entry point apply here. - if err := AuthorizeGuardians(stream.Context()); err != nil { + if err := AuthorizeCapability(stream.Context(), CapTenantAdmin); err != nil { return err } if err := hasPoormansAuth(stream.Context()); err != nil { diff --git a/edgraph/zero.go b/edgraph/zero.go index 9552d50aae7..ebb749959fb 100644 --- a/edgraph/zero.go +++ b/edgraph/zero.go @@ -21,7 +21,7 @@ func (s *Server) AllocateIDs(ctx context.Context, req *api.AllocateIDsRequest) ( *api.AllocateIDsResponse, error) { // For now, we only allow users in superadmin group to do this operation in v25 - if err := AuthSuperAdmin(ctx); err != nil { + if err := AuthorizeCapability(ctx, CapLeaseUIDs); err != nil { s := status.Convert(err) return nil, status.Error(s.Code(), "v25.AllocateIDs can only be called by the superadmin group. "+s.Message()) diff --git a/graphql/resolve/middlewares.go b/graphql/resolve/middlewares.go index f4e40cb44a7..806c66bc6fe 100644 --- a/graphql/resolve/middlewares.go +++ b/graphql/resolve/middlewares.go @@ -114,7 +114,7 @@ func (mws MutationMiddlewares) Then(resolver MutationResolver) MutationResolver // resolveGuardianOfTheGalaxyAuth returns a Resolved with error if the context doesn't contain any // superadmin auth, otherwise it returns nil func resolveGuardianOfTheGalaxyAuth(ctx context.Context, f schema.Field) *Resolved { - if err := edgraph.AuthSuperAdmin(ctx); err != nil { + if err := edgraph.AuthorizeCapability(ctx, edgraph.CapClusterAdmin); err != nil { return EmptyResult(f, err) } return nil @@ -123,7 +123,7 @@ func resolveGuardianOfTheGalaxyAuth(ctx context.Context, f schema.Field) *Resolv // resolveGuardianAuth returns a Resolved with error if the context doesn't contain any Guardian auth, // otherwise it returns nil func resolveGuardianAuth(ctx context.Context, f schema.Field) *Resolved { - if err := edgraph.AuthorizeGuardians(ctx); err != nil { + if err := edgraph.AuthorizeCapability(ctx, edgraph.CapTenantAdmin); err != nil { return EmptyResult(f, err) } return nil From 77dc5f9ee976e3bed3426ae6b71111d9065d4c4f Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Tue, 25 Aug 2026 15:21:23 -0400 Subject: [PATCH 5/8] docs(edgraph): correct authorizeClusterAdmin's list of gated operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- edgraph/predicate_acl.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/edgraph/predicate_acl.go b/edgraph/predicate_acl.go index e014a244aec..63d57c1f69b 100644 --- a/edgraph/predicate_acl.go +++ b/edgraph/predicate_acl.go @@ -72,18 +72,26 @@ func (predicateACL) AuthorizeCapability(ctx context.Context, c Capability) error // goal here needs it. // // With ACL disabled, the old rule granted cluster authority to *everyone*. That -// is the hole: CreateNamespace, DropNamespace, ListNamespaces, and AllocateIDs -// gate on this capability and nothing else, so on an ACL-off cluster they were -// reachable by any client that could open a connection. Now the same controls -// alter already requires — a whitelisted source IP, plus the --security auth -// token when one is configured — are what grant it. +// is the hole: CreateNamespace, DropNamespace, and ListNamespaces gate on this +// capability and nothing else, so on an ACL-off cluster they were reachable by any +// client that could open a connection. Now the same controls alter already +// requires — a whitelisted source IP, plus the --security auth token when one is +// configured — are what grant it. +// +// AllocateIDs is deliberately not in that list. It gated on the same +// AuthSuperAdmin before the split and now gates on CapLeaseUIDs instead +// (Server.AllocateIDs in zero.go), which stays open with ACL off because +// `dgraph live` leases a UID block on every run and has never needed a credential +// to do it. // // This is a real behavior change and the only one in this stage. It is defensible // unflagged because it makes namespace lifecycle no stricter than schema change: -// alter has always called hasAdminAuth, so any deployment that alters a schema -// already has a working whitelist or token configuration, and the operations -// being closed are the more destructive ones. A deployment that genuinely wants -// the old behavior sets the same whitelist it already sets for alter. +// validateAlterOperation has always called hasAdminAuth, and that is the same +// HasWhitelistedIP-plus-hasPoormansAuth pair break-glass consults — so any +// deployment that alters a schema already has a working whitelist or token +// configuration, and the operations being closed are the more destructive ones. A +// deployment that genuinely wants the old behavior sets the same whitelist it +// already sets for alter. func authorizeClusterAdmin(ctx context.Context) error { if x.WorkerConfig.AclEnabled { // Guardianship of the root namespace is the first route, and the only one an From dc8d4723745f2d9cb45830231302ed49ded176c4 Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Tue, 25 Aug 2026 15:21:25 -0400 Subject: [PATCH 6/8] test: pin the ClearIncomingNamespace entry-point guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- edgraph/tenancy_entrypoint_test.go | 117 ++++++++++++++++++++ worker/zero_proxy_tenancy_test.go | 61 +++++++++++ x/tenancy_entrypoints_test.go | 164 +++++++++++++++++++++++++++++ 3 files changed, 342 insertions(+) create mode 100644 edgraph/tenancy_entrypoint_test.go create mode 100644 worker/zero_proxy_tenancy_test.go create mode 100644 x/tenancy_entrypoints_test.go diff --git a/edgraph/tenancy_entrypoint_test.go b/edgraph/tenancy_entrypoint_test.go new file mode 100644 index 00000000000..d192d9e11c4 --- /dev/null +++ b/edgraph/tenancy_entrypoint_test.go @@ -0,0 +1,117 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package edgraph + +import ( + "context" + "testing" + + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" + + "github.com/dgraph-io/dgo/v250/protos/api" + "github.com/dgraph-io/dgraph/v25/x" +) + +// errProbeRefused is what the probe resolver returns instead of attributing a +// request. Refusing is what keeps these unit tests: every entry point returns the +// resolver's error immediately, so none of them reach worker, posting, or storage. +var errProbeRefused = errors.New("probe resolver: refusing to attribute") + +// namespaceProbe records what an entry point handed the tenant resolver. +type namespaceProbe struct { + called bool + saw []string +} + +// installNamespaceProbe installs a fail-closed tenant resolver that captures the +// incoming namespace metadata it was given. +// +// The resolver is the seam the entry points already resolve through, so this needs +// no production change: whatever md["namespace"] the resolver observes is exactly +// what a deployment-specific resolver would observe in production, and a resolver +// that must not trust that value can only be safe if it never arrives. +func installNamespaceProbe(t *testing.T) *namespaceProbe { + t.Helper() + p := &namespaceProbe{} + x.SetTenantResolver(func(ctx context.Context) (context.Context, error) { + p.called = true + if md, ok := metadata.FromIncomingContext(ctx); ok { + p.saw = md.Get("namespace") + } + return ctx, errProbeRefused + }) + t.Cleanup(func() { x.SetTenantResolver(nil) }) + return p +} + +// spoofedCtx is a request naming namespace 9 while presenting no credential of any +// kind — the uncredentialed cross-tenant caller the guard exists to stop. +func spoofedCtx() context.Context { + return metadata.NewIncomingContext(context.Background(), + metadata.New(map[string]string{"namespace": "9"})) +} + +// assertGuarded is the whole assertion: the resolver ran, and it was not shown the +// caller's namespace. +func assertGuarded(t *testing.T, p *namespaceProbe, err error) { + t.Helper() + require.ErrorIs(t, err, errProbeRefused, + "the entry point must return the resolver's refusal, not continue past it") + require.True(t, p.called, + "the resolver was never reached; this entry point no longer resolves a tenant "+ + "and the test proves nothing") + require.Empty(t, p.saw, + "the resolver was handed the client's own namespace. On the server side "+ + "md[\"namespace\"] is client-controlled, and the built-in resolver leaves it in "+ + "place when it cannot derive one from a credential — so this is an "+ + "uncredentialed caller acting in a tenant of their choosing. Restore the "+ + "x.ClearIncomingNamespace call at this entry point.") +} + +// TestEntryPointsClearClientNamespace is the regression test for the entry-point +// guards. Deleting any x.ClearIncomingNamespace call left every unit and package +// test in the repo green, which is why these exist. +func TestEntryPointsClearClientNamespace(t *testing.T) { + const q = "{ q(func: uid(0x1)) { uid } }" + + t.Run("Server.Query", func(t *testing.T) { + p := installNamespaceProbe(t) + _, err := (&Server{}).Query(spoofedCtx(), &api.Request{Query: q}) + assertGuarded(t, p, err) + }) + + t.Run("Server.Alter", func(t *testing.T) { + p := installNamespaceProbe(t) + _, err := (&Server{}).Alter(spoofedCtx(), &api.Operation{Schema: "name: string ."}) + assertGuarded(t, p, err) + }) + + t.Run("Server.RunDQL", func(t *testing.T) { + p := installNamespaceProbe(t) + _, err := (&Server{}).RunDQL(spoofedCtx(), &api.RunDQLRequest{DqlQuery: q}) + assertGuarded(t, p, err) + }) +} + +// TestAttributedContinuationsKeepTheirNamespace is the other half of the invariant: +// the guard belongs at entry points only. A continuation that cleared the namespace +// would break the in-process callers that arrive already attributed and hold no +// credential to re-present. +func TestAttributedContinuationsKeepTheirNamespace(t *testing.T) { + p := installNamespaceProbe(t) + // AlterNoAuth is reached through alter() — the same continuation Alter guards + // above — with a context an in-process caller attributed itself. + ctx := metadata.NewIncomingContext(context.Background(), + metadata.New(map[string]string{"namespace": "9"})) + _, err := (&Server{}).AlterNoAuth(ctx, &api.Operation{Schema: "name: string ."}) + require.ErrorIs(t, err, errProbeRefused) + require.True(t, p.called) + require.Equal(t, []string{"9"}, p.saw, + "the guard has moved into the shared continuation: a trusted in-process caller's "+ + "namespace is being stripped") +} diff --git a/worker/zero_proxy_tenancy_test.go b/worker/zero_proxy_tenancy_test.go new file mode 100644 index 00000000000..b74f61309c7 --- /dev/null +++ b/worker/zero_proxy_tenancy_test.go @@ -0,0 +1,61 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package worker + +import ( + "context" + "testing" + + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/metadata" + + "github.com/dgraph-io/dgraph/v25/protos/pb" + "github.com/dgraph-io/dgraph/v25/x" +) + +// TestForwardAssignUidsToZeroClearsClientNamespace is the regression test for the +// privilege escalation this guard fixes: uncredentialed cross-tenant UID leasing. +// +// forwardAssignUidsToZero has no other gate. Before the tenant-resolution seam it +// derived the namespace from the signed access JWT and returned that error on +// failure; after it, the built-in resolver's tolerate-a-bad-token branch left the +// client's own md["namespace"] in place and ExtractNamespace read it back. Deleting +// the x.ClearIncomingNamespace call left every package test in the repo green. +// +// The fail-closed probe resolver is what makes this a unit test: the refusal returns +// before groups().Leader(0), so no Zero connection is needed. +func TestForwardAssignUidsToZeroClearsClientNamespace(t *testing.T) { + errRefused := errors.New("probe resolver: refusing to attribute") + + var called bool + var saw []string + // Installing any resolver also makes x.MultiTenancyEnabled() true, which is what + // gates the block under test. + x.SetTenantResolver(func(ctx context.Context) (context.Context, error) { + called = true + if md, ok := metadata.FromIncomingContext(ctx); ok { + saw = md.Get("namespace") + } + return ctx, errRefused + }) + t.Cleanup(func() { x.SetTenantResolver(nil) }) + require.True(t, x.MultiTenancyEnabled(), "the guarded block would be skipped") + + // A caller asking to lease UIDs in namespace 9 with no credential at all. + ctx := metadata.NewIncomingContext(context.Background(), + metadata.New(map[string]string{"namespace": "9"})) + + _, err := forwardAssignUidsToZero(ctx, &pb.Num{Val: 10, Type: pb.Num_UID}) + require.ErrorIs(t, err, errRefused, + "an unattributable lease request must be refused here, not forwarded to Zero") + require.True(t, called, + "the resolver was never reached; this path no longer resolves a tenant") + require.Empty(t, saw, + "the resolver was handed the client's own namespace, so an uncredentialed caller "+ + "leases UIDs in whichever tenant they name. Restore the "+ + "x.ClearIncomingNamespace call in forwardAssignUidsToZero.") +} diff --git a/x/tenancy_entrypoints_test.go b/x/tenancy_entrypoints_test.go new file mode 100644 index 00000000000..831826c34a2 --- /dev/null +++ b/x/tenancy_entrypoints_test.go @@ -0,0 +1,164 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package x + +import ( + "bytes" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// clearNamespaceSites is every function that strips the client-supplied namespace, +// and why that function is the right place for it. +// +// This is a decision table, not a description. ClearIncomingNamespace belongs at an +// entry point and nowhere else: md["namespace"] is client-controlled server-side and +// the built-in resolver leaves it in place when it cannot derive one from a +// credential, so an entry point that skips the call attributes the request to +// whatever tenant the caller named. A shared continuation that makes the call is the +// mirror-image bug — it discards the namespace an already-attributed in-process +// caller was given. +// +// The behavioral tests in edgraph and worker prove each of these four rejects an +// uncredentialed request. This table adds what behavior cannot see: that the call +// sits in *this* function rather than having drifted down into the continuation +// below it, where it would still look correct from the outside. +var clearNamespaceSites = map[string]string{ + "edgraph/query.go:RunDQL": "gRPC entry point; resolves its own tenant", + "edgraph/server.go:Alter": "entry point, not alter(), which also serves AlterNoAuth " + + "with a context an in-process caller has already attributed", + "edgraph/server.go:Query": "entry point, not QueryNoGrpc(), which also serves the two " + + "HTTP handlers and GetGQLSchema's trusted tenancy", + "worker/zero_proxy.go:forwardAssignUidsToZero": "gRPC entry point for UID leasing, and " + + "the only gate on it — without this an uncredentialed caller leases UIDs in any tenant", +} + +// clearNamespaceScanRoot is the repo root, walked in full: a scan that can miss a +// call site cannot report completeness. See capabilityScanRoot in edgraph. +const clearNamespaceScanRoot = ".." + +var clearNamespaceScanSkip = map[string]bool{ + ".git": true, "vendor": true, "testdata": true, "protos": true, + "compose": true, "contrib": true, ".trunk": true, "systest": true, + // A nested checkout under the repo root would be walked as if it were part of + // this one, so the same function is found twice and the count assertion fails. + // Agent worktrees land here by convention. + ".worktrees": true, +} + +// scanClearNamespaceSites returns "dir/file.go:FuncName" for every function whose +// body calls ClearIncomingNamespace. +func scanClearNamespaceSites(t *testing.T) map[string]bool { + t.Helper() + found := make(map[string]bool) + var parsed int + + err := filepath.WalkDir(clearNamespaceScanRoot, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if clearNamespaceScanSkip[d.Name()] { + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + src, readErr := os.ReadFile(path) + if readErr != nil { + return readErr + } + if !bytes.Contains(src, []byte("ClearIncomingNamespace")) { + return nil + } + parsed++ + + fset := token.NewFileSet() + af, parseErr := parser.ParseFile(fset, path, src, 0) + require.NoError(t, parseErr, "parsing %s", path) + + for _, decl := range af.Decls { + fd, ok := decl.(*ast.FuncDecl) + if !ok || fd.Body == nil { + continue + } + // The declaration itself is not a call site. + if fd.Name.Name == "ClearIncomingNamespace" { + continue + } + ast.Inspect(fd.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || clearNamespaceCallee(call.Fun) != "ClearIncomingNamespace" { + return true + } + found[clearNamespaceSitePath(path)+":"+fd.Name.Name] = true + return true + }) + } + return nil + }) + require.NoError(t, err) + require.Positive(t, parsed, "the walk parsed no files mentioning ClearIncomingNamespace; "+ + "clearNamespaceScanRoot is wrong and this test proves nothing") + return found +} + +func clearNamespaceCallee(fn ast.Expr) string { + switch f := fn.(type) { + case *ast.Ident: + return f.Name + case *ast.SelectorExpr: + return f.Sel.Name + } + return "" +} + +func clearNamespaceSitePath(path string) string { + clean := filepath.ToSlash(filepath.Clean(path)) + return strings.TrimPrefix(clean, "../") +} + +// TestEveryEntryPointClearsIncomingNamespace pins the guard's call sites in both +// directions. A deleted call is a privilege escalation; a call added to a shared +// continuation silently strips tenancy from trusted in-process callers. +func TestEveryEntryPointClearsIncomingNamespace(t *testing.T) { + found := scanClearNamespaceSites(t) + + for site := range found { + if _, declared := clearNamespaceSites[site]; !declared { + t.Errorf("%s clears the incoming namespace but is not in clearNamespaceSites. "+ + "If it is an entry point, add it and say so. If it is a continuation of an "+ + "already-attributed request, the call is wrong: it discards the namespace an "+ + "in-process caller supplied.", site) + } + } + + for site, why := range clearNamespaceSites { + if !found[site] { + t.Errorf("clearNamespaceSites declares %s (%s), which no longer clears the "+ + "incoming namespace. Deleting that call lets an uncredentialed caller act in "+ + "whichever tenant they name in md[\"namespace\"].", site, why) + } + } +} + +// TestClearNamespaceSiteCoverage guards against the table or the scan going empty, +// which would make the test above vacuously pass. +func TestClearNamespaceSiteCoverage(t *testing.T) { + found := scanClearNamespaceSites(t) + require.NotEmpty(t, found, "the scan found no entry-point guards at all") + require.Len(t, found, len(clearNamespaceSites)) +} From 8bdcc90eb679082fad3fa5a8ce7ad3dca02606f7 Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Tue, 25 Aug 2026 15:21:26 -0400 Subject: [PATCH 7/8] test(dgraphtest): add WithWhitelist, and cover ACL-off cluster authority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- dgraphtest/config.go | 32 ++++- dgraphtest/dgraph.go | 19 +-- dgraphtest/local_cluster.go | 41 +++++- systest/integration2/cluster_admin_test.go | 154 +++++++++++++++++++++ 4 files changed, 226 insertions(+), 20 deletions(-) create mode 100644 systest/integration2/cluster_admin_test.go diff --git a/dgraphtest/config.go b/dgraphtest/config.go index b68be111619..3ce21920621 100644 --- a/dgraphtest/config.go +++ b/dgraphtest/config.go @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. * SPDX-License-Identifier: Apache-2.0 */ @@ -14,6 +14,13 @@ import ( "github.com/golang-jwt/jwt/v5" ) +// defaultWhitelist is the --security whitelist every dgraphtest cluster gets +// unless a test overrides it with WithWhitelist. It has to stay wide open: +// LocalCluster.Start() probes GraphQL with an admin mutation, and every admin +// GraphQL operation carries IpWhitelistingMW, so a narrower default would break +// every existing test rather than only the ones doing admin work. +const defaultWhitelist = "0.0.0.0/0" + // UpgradeCombo represents a version combination before and // after the upgrade, and the strategy for upgrading type UpgradeCombo struct { @@ -112,6 +119,7 @@ type ClusterConfig struct { repoDir string mcp bool securityToken string + whitelist string } // NewClusterConfig generates a default ClusterConfig @@ -132,6 +140,7 @@ func NewClusterConfig() ClusterConfig { portOffset: -1, customPlugins: false, mcp: false, + whitelist: defaultWhitelist, } } @@ -174,6 +183,27 @@ func (cc ClusterConfig) WithSecurityToken(token string) ClusterConfig { return cc } +// WithWhitelist sets the Alpha's --security whitelist for admin operations, +// REPLACING the wide-open default rather than adding to it. Zero is unaffected: it +// keeps its own hard-coded open whitelist, so this narrows the Alpha's admin surface +// and nothing else. The value is the flag's own syntax: a +// comma-separated list of IP addresses, a.b.c.d:w.x.y.z ranges, CIDR blocks, or +// hostnames, e.g. "192.168.0.0/16,host.docker.internal". +// +// Replacing is the point. 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 +// a test. Note that loopback is admitted unconditionally by x.isIpWhitelisted +// regardless of this setting, so a test that wants a denial has to reach the alpha +// over a non-loopback source — which a published Docker port gives it. +// +// Setting a whitelist other than the default makes Start() wait on /probe/graphql +// instead of the admin GraphQL mutation it normally probes with; see +// LocalCluster.waitUntilGraphqlHealthCheck for why. +func (cc ClusterConfig) WithWhitelist(spec string) ClusterConfig { + cc.whitelist = spec + return cc +} + // WithAcl enables ACL feature for Dgraph cluster func (cc ClusterConfig) WithACL(aclTTL time.Duration) ClusterConfig { cc.acl = true diff --git a/dgraphtest/dgraph.go b/dgraphtest/dgraph.go index e49cf4582c3..2a269645e37 100644 --- a/dgraphtest/dgraph.go +++ b/dgraphtest/dgraph.go @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. * SPDX-License-Identifier: Apache-2.0 */ @@ -79,7 +79,6 @@ type dnode interface { ports() nat.PortSet bindings(int) nat.PortMap cmd(*LocalCluster) []string - env(*LocalCluster) []string workingDir() string mounts(*LocalCluster) ([]mount.Mount, error) healthURL(*LocalCluster) (string, error) @@ -153,14 +152,6 @@ func (z *zero) cmd(c *LocalCluster) []string { return zcmd } -func (z *zero) env(c *LocalCluster) []string { - // Zero binaries that guard their admin HTTP endpoints read the security whitelist from - // the environment; older binaries used in upgrade tests simply ignore the variable. An - // env var is used instead of a --security flag because older zero binaries would fail - // to start on an unrecognized flag. - return []string{"DGRAPH_ZERO_SECURITY=whitelist=0.0.0.0/0"} -} - func (z *zero) workingDir() string { return zeroWorkingDir } @@ -256,9 +247,9 @@ func (a *alpha) cmd(c *LocalCluster) []string { "--bindall", "--logtostderr", fmt.Sprintf("-v=%d", c.conf.verbosity)} if c.lowerThanV21 { - acmd = append(acmd, `--whitelist=0.0.0.0/0`, "--telemetry=false") + acmd = append(acmd, fmt.Sprintf(`--whitelist=%s`, c.conf.whitelist), "--telemetry=false") } else { - security := `--security=whitelist=0.0.0.0/0` + security := fmt.Sprintf(`--security=whitelist=%s`, c.conf.whitelist) if c.conf.securityToken != "" { security += fmt.Sprintf(`;token=%s`, c.conf.securityToken) } @@ -322,10 +313,6 @@ func (a *alpha) cmd(c *LocalCluster) []string { return acmd } -func (a *alpha) env(c *LocalCluster) []string { - return nil -} - func (a *alpha) workingDir() string { return alphaWorkingDir } diff --git a/dgraphtest/local_cluster.go b/dgraphtest/local_cluster.go index 99188303d5a..f0950cebdfd 100644 --- a/dgraphtest/local_cluster.go +++ b/dgraphtest/local_cluster.go @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. * SPDX-License-Identifier: Apache-2.0 */ @@ -317,8 +317,7 @@ func (c *LocalCluster) createContainer(dc dnode) (string, error) { } } - cconf := &container.Config{Cmd: cmd, Image: image, WorkingDir: dc.workingDir(), - ExposedPorts: dc.ports(), Env: dc.env(c)} + cconf := &container.Config{Cmd: cmd, Image: image, WorkingDir: dc.workingDir(), ExposedPorts: dc.ports()} // ApplyContainerUser is a public hook in dgraphtest/hooks.go; the // default is no-op. Downstream consumers that run dgraph as a // non-root user inside the test container override it to set @@ -864,6 +863,15 @@ func (c *LocalCluster) waitUntilLogin() error { } func (c *LocalCluster) waitUntilGraphqlHealthCheck() error { + // The probes below are admin GraphQL operations, and every admin GraphQL + // operation carries IpWhitelistingMW — login included. On a cluster whose + // whitelist does not admit this process, neither can ever succeed, so wait on + // /probe/graphql instead: no whitelist middleware, no auth, and its whole + // purpose is answering "is GraphQL serving yet". + if c.conf.whitelist != defaultWhitelist { + return c.waitUntilGraphqlProbe() + } + hc, err := c.HTTPClient() if err != nil { return errors.Wrap(err, "error creating http client while graphql health check") @@ -901,6 +909,33 @@ func (c *LocalCluster) waitUntilGraphqlHealthCheck() error { return errors.Wrap(err, "error during graphql health check") } +// waitUntilGraphqlProbe waits for GraphQL to serve using /probe/graphql, which +// carries no IP-whitelist and no auth middleware. It is the probe to use when the +// cluster's whitelist may not admit the test process. +func (c *LocalCluster) waitUntilGraphqlProbe() error { + url, err := c.serverURL("alpha", "/probe/graphql") + if err != nil { + return errors.Wrap(err, "error getting graphql probe URL") + } + + var lastErr error + for attempt := range 10 { + time.Sleep(waitDurBeforeRetry) + req, err := http.NewRequest(http.MethodGet, "http://"+url, nil) + if err != nil { + return errors.Wrap(err, "error building graphql probe request") + } + if _, lastErr = dgraphapi.DoReq(req); lastErr == nil { + log.Printf("[INFO] graphql probe succeeded for %v", c.conf.prefix) + return nil + } + if attempt > 5 { + log.Printf("[WARNING] problem during graphql probe: %v", lastErr) + } + } + return errors.Wrap(lastErr, "error during graphql probe") +} + // Upgrades the cluster to the provided dgraph version func (c *LocalCluster) Upgrade(version string, strategy UpgradeStrategy) error { if version == c.conf.version { diff --git a/systest/integration2/cluster_admin_test.go b/systest/integration2/cluster_admin_test.go new file mode 100644 index 00000000000..4c1e3fc5b46 --- /dev/null +++ b/systest/integration2/cluster_admin_test.go @@ -0,0 +1,154 @@ +//go:build integration2 + +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package main + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" + + "github.com/dgraph-io/dgraph/v25/dgraphtest" +) + +// aclOffDenial is the message authorizeClusterAdmin produces when ACL is disabled and +// no capability source grants. Asserting on it is what stops these tests passing +// because the RPC failed for some unrelated reason — a bare PermissionDenied could +// come from anywhere. +const aclOffDenial = "cluster-admin authority is required and this caller holds none" + +// TestClusterAdminAclOffRequiresAuthToken covers the token half of break-glass. It is +// the test whose absence let the ACL-off behavior change ship unnoticed: before the +// change, an ACL-off cluster granted cluster authority to anyone who could open a +// connection, so the no-token CreateNamespace below succeeded. +// +// It needs no new harness capability — WithSecurityToken already exists — which is +// why it is the half that can be trusted first. +func TestClusterAdminAclOffRequiresAuthToken(t *testing.T) { + const token = "break-glass-token" + conf := dgraphtest.NewClusterConfig().WithNumAlphas(1).WithNumZeros(1).WithReplicas(1). + WithSecurityToken(token) + c, err := dgraphtest.NewLocalCluster(conf) + require.NoError(t, err) + defer func() { c.Cleanup(t.Failed()) }() + require.NoError(t, c.Start()) + + gc, cleanup, err := c.Client() + require.NoError(t, err) + defer cleanup() + + // The three RPCs that gate on CapClusterAdmin and nothing else. Deliberately + // gRPC, not the admin GraphQL surface: every admin GraphQL op carries + // IpWhitelistingMW and was already gated before the change, so it cannot + // distinguish the old rule from the new one. + _, err = gc.CreateNamespace(context.Background()) + require.Error(t, err, + "an ACL-off cluster must not create namespaces for an unauthenticated caller") + require.Equal(t, codes.PermissionDenied, status.Code(err)) + // Assert the reason, not just the code: this text comes only from + // authorizeClusterAdmin's ACL-off branch, so a PermissionDenied raised anywhere + // else cannot satisfy it. + require.ErrorContains(t, err, aclOffDenial) + + _, err = gc.ListNamespaces(context.Background()) + require.Error(t, err) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + + // Assert the reason here too. Namespace 1 does not exist on a fresh cluster, so a + // bare require.Error would pass on "no such namespace" just as readily as on a + // refusal, and prove nothing. + err = gc.DropNamespace(context.Background(), 1) + require.Error(t, err, + "dropping a namespace must not be reachable without a credential either") + require.ErrorContains(t, err, aclOffDenial) + + // The token is the half of break-glass this cluster can supply, and it grants. + adminCtx := metadata.AppendToOutgoingContext(context.Background(), "auth-token", token) + ns, err := gc.CreateNamespace(adminCtx) + require.NoError(t, err) + require.Greater(t, ns, uint64(0)) + + nsList, err := gc.ListNamespaces(adminCtx) + require.NoError(t, err) + require.Contains(t, nsList, ns) + require.NoError(t, gc.DropNamespace(adminCtx, ns)) +} + +// TestClusterAdminAclOffRequiresWhitelistedIP covers the whitelist half — the half no +// integration test could reach before WithWhitelist, because dgraphtest hard-coded +// whitelist=0.0.0.0/0 for every cluster it built. It is the integration-level twin of +// the fromIP("203.0.113.7") case in edgraph/capability_test.go. +// +// One assumption is load-bearing and is asserted rather than assumed: the alpha must +// see this caller's source address as neither loopback nor inside the whitelist, or +// the test would pass for the wrong reason. The control test below is what detects +// that — if the source address were somehow admitted here, it would be admitted there +// too, and both would agree. They cannot both be right. +func TestClusterAdminAclOffRequiresWhitelistedIP(t *testing.T) { + // TEST-NET-1 (RFC 5737). It cannot be the source address of a connection arriving + // through a published Docker port, so this cluster admits no remote admin caller. + conf := dgraphtest.NewClusterConfig().WithNumAlphas(1).WithNumZeros(1).WithReplicas(1). + WithWhitelist("192.0.2.0/24") + c, err := dgraphtest.NewLocalCluster(conf) + require.NoError(t, err) + defer func() { c.Cleanup(t.Failed()) }() + require.NoError(t, c.Start()) + + gc, cleanup, err := c.Client() + require.NoError(t, err) + defer cleanup() + + // No auth token is configured, so the source IP is the only half of break-glass in + // play, and it does not grant. + _, err = gc.CreateNamespace(context.Background()) + require.Error(t, err, "a non-whitelisted caller must not hold cluster authority") + require.Equal(t, codes.PermissionDenied, status.Code(err)) + require.ErrorContains(t, err, aclOffDenial) + + _, err = gc.ListNamespaces(context.Background()) + require.Error(t, err) + require.Equal(t, codes.PermissionDenied, status.Code(err)) + + err = gc.DropNamespace(context.Background(), 1) + require.Error(t, err) + require.ErrorContains(t, err, aclOffDenial) +} + +// TestClusterAdminAclOffWhitelistedIPGrants is the control for the test above: same +// ACL-off, no-token configuration, whitelist that admits the caller, and now the same +// three RPCs succeed. The whitelist is spelled out rather than left defaulted so the +// pairing is readable in the diff. +// +// Without this control, TestClusterAdminAclOffRequiresWhitelistedIP could pass because +// the RPCs are broken for some unrelated reason rather than because the whitelist +// refused the caller. +func TestClusterAdminAclOffWhitelistedIPGrants(t *testing.T) { + conf := dgraphtest.NewClusterConfig().WithNumAlphas(1).WithNumZeros(1).WithReplicas(1). + WithWhitelist("0.0.0.0/0") + c, err := dgraphtest.NewLocalCluster(conf) + require.NoError(t, err) + defer func() { c.Cleanup(t.Failed()) }() + require.NoError(t, c.Start()) + + gc, cleanup, err := c.Client() + require.NoError(t, err) + defer cleanup() + + ns, err := gc.CreateNamespace(context.Background()) + require.NoError(t, err, + "a whitelisted caller with no token configured holds cluster authority") + require.Greater(t, ns, uint64(0)) + + nsList, err := gc.ListNamespaces(context.Background()) + require.NoError(t, err) + require.Contains(t, nsList, ns) + require.NoError(t, gc.DropNamespace(context.Background(), ns)) +} From 2ca6463d416af3da575b56f8b23b2e4a767ee48e Mon Sep 17 00:00:00 2001 From: Matthew McNeely Date: Thu, 27 Aug 2026 17:18:15 -0400 Subject: [PATCH 8/8] fix(dgraphtest): restore Zero's security env that this branch had deleted 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) --- dgraphtest/config.go | 23 +++++++++++++---------- dgraphtest/dgraph.go | 15 ++++++++++++++- dgraphtest/local_cluster.go | 5 +++-- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/dgraphtest/config.go b/dgraphtest/config.go index 3ce21920621..610f328a902 100644 --- a/dgraphtest/config.go +++ b/dgraphtest/config.go @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. * SPDX-License-Identifier: Apache-2.0 */ @@ -185,16 +185,19 @@ func (cc ClusterConfig) WithSecurityToken(token string) ClusterConfig { // WithWhitelist sets the Alpha's --security whitelist for admin operations, // REPLACING the wide-open default rather than adding to it. Zero is unaffected: it -// keeps its own hard-coded open whitelist, so this narrows the Alpha's admin surface -// and nothing else. The value is the flag's own syntax: a -// comma-separated list of IP addresses, a.b.c.d:w.x.y.z ranges, CIDR blocks, or -// hostnames, e.g. "192.168.0.0/16,host.docker.internal". +// reads its own whitelist from DGRAPH_ZERO_SECURITY, because older zero binaries used +// in upgrade tests would fail to start on an unrecognized flag. // -// Replacing is the point. 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 -// a test. Note that loopback is admitted unconditionally by x.isIpWhitelisted -// regardless of this setting, so a test that wants a denial has to reach the alpha -// over a non-loopback source — which a published Docker port gives it. +// The value is the flag's own syntax: a comma-separated list of IP addresses, +// a.b.c.d:w.x.y.z ranges, CIDR blocks, or hostnames, e.g. +// "192.168.0.0/16,host.docker.internal". +// +// Replacing is the point. 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 a test. +// Note that loopback is admitted unconditionally by x.isIpWhitelisted regardless of +// this setting, so a test that wants a denial has to reach the alpha over a +// non-loopback source — which a published Docker port gives it on Linux, though not +// necessarily on Docker Desktop. // // Setting a whitelist other than the default makes Start() wait on /probe/graphql // instead of the admin GraphQL mutation it normally probes with; see diff --git a/dgraphtest/dgraph.go b/dgraphtest/dgraph.go index 2a269645e37..09b251d7e21 100644 --- a/dgraphtest/dgraph.go +++ b/dgraphtest/dgraph.go @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. * SPDX-License-Identifier: Apache-2.0 */ @@ -79,6 +79,7 @@ type dnode interface { ports() nat.PortSet bindings(int) nat.PortMap cmd(*LocalCluster) []string + env(*LocalCluster) []string workingDir() string mounts(*LocalCluster) ([]mount.Mount, error) healthURL(*LocalCluster) (string, error) @@ -152,6 +153,14 @@ func (z *zero) cmd(c *LocalCluster) []string { return zcmd } +func (z *zero) env(c *LocalCluster) []string { + // Zero binaries that guard their admin HTTP endpoints read the security whitelist from + // the environment; older binaries used in upgrade tests simply ignore the variable. An + // env var is used instead of a --security flag because older zero binaries would fail + // to start on an unrecognized flag. + return []string{"DGRAPH_ZERO_SECURITY=whitelist=0.0.0.0/0"} +} + func (z *zero) workingDir() string { return zeroWorkingDir } @@ -313,6 +322,10 @@ func (a *alpha) cmd(c *LocalCluster) []string { return acmd } +func (a *alpha) env(c *LocalCluster) []string { + return nil +} + func (a *alpha) workingDir() string { return alphaWorkingDir } diff --git a/dgraphtest/local_cluster.go b/dgraphtest/local_cluster.go index f0950cebdfd..8594d6db475 100644 --- a/dgraphtest/local_cluster.go +++ b/dgraphtest/local_cluster.go @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: © 2017-2025 Istari Digital, Inc. + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. * SPDX-License-Identifier: Apache-2.0 */ @@ -317,7 +317,8 @@ func (c *LocalCluster) createContainer(dc dnode) (string, error) { } } - cconf := &container.Config{Cmd: cmd, Image: image, WorkingDir: dc.workingDir(), ExposedPorts: dc.ports()} + cconf := &container.Config{Cmd: cmd, Image: image, WorkingDir: dc.workingDir(), + ExposedPorts: dc.ports(), Env: dc.env(c)} // ApplyContainerUser is a public hook in dgraphtest/hooks.go; the // default is no-op. Downstream consumers that run dgraph as a // non-root user inside the test container override it to set