From e027e55e86292fc0ec37be3517edb918406723cb Mon Sep 17 00:00:00 2001 From: HosniBelfeki Date: Tue, 1 Sep 2026 23:13:41 +0100 Subject: [PATCH 1/3] node: fail closed when a required-labels spec names no label X-Sam-Required-Labels (and the identical required_labels parameter on the call_remote_tool MCP tool) is a fail-closed control: the request is only served by a provider that attests one of the named labels. An empty requirement set is the documented way to say "no requirement", and it switches the control off completely -- VerifyPeerLabels returns before it opens a stream, and rankProviders skips the label filter. parseRequiredLabels skipped empty comma-separated entries and then returned whatever it had accumulated, so a specification that carried content but named no label produced an empty set and no error: X-Sam-Required-Labels: ,, -> nil, nil -> no requirement at all The caller asked to be constrained and was silently served by an arbitrary provider instead. Reproduced end to end at the facade boundary: the request was forwarded and answered 200. Only a blank specification now means "no requirement"; a non-blank one that yields no label is rejected. Empty entries alongside real ones stay tolerated, so a trailing comma remains harmless, and both call sites already surface the error (400 invalid_request on the facade, a failed tool call over MCP). The sibling parsers were checked and are deliberately left alone: parseLabelsFlag in cmd/sam-node declares a node's own labels, where parsing to nothing grants less rather than more, and it is re-validated by api.ValidateLabels at startup. Tests, next to the change in internal/node/openai_facade_test.go: - ContentfulSpecNamingNoLabelFailsClosed pins the six separator-only shapes as errors (all six returned a nil error before this change) - BlankSpecMeansNoRequirement keeps "" and whitespace-only unconstrained - EmptyEntriesBesideRealOnesStayTolerated guards against over-correcting the trailing-comma case - Facade_Completions_ContentfulRequiredLabelsNamingNoLabelIsRejected asserts 400 and that no provider is reached (it saw 200 before) No new module: stdlib strings/fmt only, both already imported. --- internal/node/labels_gate.go | 13 ++++- internal/node/openai_facade_test.go | 86 +++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/internal/node/labels_gate.go b/internal/node/labels_gate.go index 6adcbe92..3c19efd0 100644 --- a/internal/node/labels_gate.go +++ b/internal/node/labels_gate.go @@ -32,8 +32,16 @@ import ( // parseRequiredLabels splits the X-Sam-Required-Labels header value // (comma-separated "key=value" pairs) into a label map; any malformed entry // rejects the whole request. +// +// Only a blank specification means "no requirement". A specification that +// carries content but names no label — ",," or a lone separator — is rejected +// instead of being read as unconstrained: an empty requirement set switches +// the label gate off entirely (see VerifyPeerLabels and rankProviders), so +// silently deriving one from a caller's non-blank input would turn a +// fail-closed control into a fail-open one. Empty entries *alongside* real +// ones stay tolerated, so a trailing comma is still harmless. func parseRequiredLabels(h string) (map[string]string, error) { - if h == "" { + if strings.TrimSpace(h) == "" { return nil, nil } var out map[string]string @@ -61,6 +69,9 @@ func parseRequiredLabels(h string) (map[string]string, error) { } out[k] = v } + if len(out) == 0 { + return nil, fmt.Errorf("invalid required labels %q: expected at least one key=value pair", h) + } return out, nil } diff --git a/internal/node/openai_facade_test.go b/internal/node/openai_facade_test.go index 4fc1b3b6..748b2047 100644 --- a/internal/node/openai_facade_test.go +++ b/internal/node/openai_facade_test.go @@ -478,6 +478,92 @@ func TestParseRequiredLabels(t *testing.T) { } } +// An empty requirement set turns the label gate off (VerifyPeerLabels returns +// immediately, and rankProviders skips the label filter), so the only input +// allowed to produce one is a genuinely blank specification. +func TestParseRequiredLabels_BlankSpecMeansNoRequirement(t *testing.T) { + for _, in := range []string{"", " ", " ", " "} { + got, err := parseRequiredLabels(in) + if err != nil || got != nil { + t.Errorf("parseRequiredLabels(%q): got %v, %v; want nil, nil", in, got, err) + } + } +} + +// Regression: a specification that carries content but names no label used to +// parse to zero requirements with no error, silently disabling a fail-closed +// control for a caller who asked to be constrained. It must be an error. +func TestParseRequiredLabels_ContentfulSpecNamingNoLabelFailsClosed(t *testing.T) { + for _, in := range []string{",", ",,", ",,,", " , ", " , ", " ,, "} { + got, err := parseRequiredLabels(in) + if err == nil { + t.Errorf("parseRequiredLabels(%q): got %v, nil; want an error, because zero requirements means an unconstrained request", in, got) + } + if got != nil { + t.Errorf("parseRequiredLabels(%q): labels must be nil on error, got %v", in, got) + } + } +} + +// The stricter rule must not reach empty entries that merely sit next to real +// ones: a trailing comma is a normal way to write a list and stays harmless. +func TestParseRequiredLabels_EmptyEntriesBesideRealOnesStayTolerated(t *testing.T) { + tests := []struct { + in string + want map[string]string + }{ + {"region=eu,", map[string]string{"region": "eu"}}, + {",region=eu", map[string]string{"region": "eu"}}, + {" region=eu , , team=platform ,,", map[string]string{"region": "eu", "team": "platform"}}, + } + for _, tt := range tests { + got, err := parseRequiredLabels(tt.in) + if err != nil { + t.Errorf("parseRequiredLabels(%q): unexpected error %v", tt.in, err) + continue + } + if len(got) != len(tt.want) { + t.Errorf("parseRequiredLabels(%q): got %v, want %v", tt.in, got, tt.want) + continue + } + for k, v := range tt.want { + if got[k] != v { + t.Errorf("parseRequiredLabels(%q): got[%q] = %q, want %q", tt.in, k, got[k], v) + } + } + } +} + +// The boundary behaviour the parser fix exists for: a caller who asked to be +// constrained is refused, not quietly served by an arbitrary provider. +func TestFacade_Completions_ContentfulRequiredLabelsNamingNoLabelIsRejected(t *testing.T) { + f := newTestFacade() + f.discover = func(_ context.Context) ([]*api.DiscoveredProvider, error) { + return []*api.DiscoveredProvider{{PeerId: "peerUS", SrvName: "srvUS"}}, nil + } + f.remoteModels = func(_ context.Context, _, _ string) ([]string, error) { + return []string{"m1"}, nil + } + forwarded := false + f.forward = http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + forwarded = true + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", + strings.NewReader(`{"model":"m1","messages":[{"role":"user","content":"hi"}]}`)) + req.Header.Set(api.HeaderSamRequiredLabels, ",,") + rec := httptest.NewRecorder() + f.handleCompletions(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("status: got %d, want %d (body %s)", rec.Code, http.StatusBadRequest, rec.Body.String()) + } + if forwarded { + t.Error("request must not reach a provider: the caller asked for a label constraint that named no label") + } +} + func TestRankProviders_LabelMatchIsExactAndCaseSensitive(t *testing.T) { f := newTestFacade() provider := modelProvider{peerID: "peerEU", service: "srv", labels: map[string]string{"region": "eu-de"}} From ecec9d2cee7e67c4f6f8ccc913369e56cec464fb Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Wed, 2 Sep 2026 14:15:06 +0000 Subject: [PATCH 2/3] node: pin the fail-open labels shape at the a2a egress boundary PR #347 added a third parseRequiredLabels call site after the fail-closed fix was written. The shared parser covers it, but the a2a egress gate now pins ",," as a 400 alongside the malformed-entry case. --- internal/node/a2a_service_test.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/internal/node/a2a_service_test.go b/internal/node/a2a_service_test.go index 5c234b1d..d13e3db9 100644 --- a/internal/node/a2a_service_test.go +++ b/internal/node/a2a_service_test.go @@ -79,15 +79,18 @@ func TestA2AEgressHookNonA2APassthrough(t *testing.T) { } func TestA2AEgressHookMalformedLabels(t *testing.T) { - rec := httptest.NewRecorder() - req := httptest.NewRequest("POST", "/sam/12D3KooWpeer/a2a/agent/", nil) - req.Header.Set(api.HeaderSamRequiredLabels, "not-a-label") - _, ok := applyEgressMiddleware(nil, rec, req) - if ok { - t.Fatal("malformed labels must be refused") - } - if rec.Code != http.StatusBadRequest { - t.Fatalf("status = %d, want 400", rec.Code) + // ",," is the fail-open shape: it must not parse to "no requirement". + for _, header := range []string{"not-a-label", ",,"} { + rec := httptest.NewRecorder() + req := httptest.NewRequest("POST", "/sam/12D3KooWpeer/a2a/agent/", nil) + req.Header.Set(api.HeaderSamRequiredLabels, header) + _, ok := applyEgressMiddleware(nil, rec, req) + if ok { + t.Fatalf("labels header %q must be refused", header) + } + if rec.Code != http.StatusBadRequest { + t.Fatalf("labels header %q: status = %d, want 400", header, rec.Code) + } } } From 8e6c047655a82376c22878a00c521829c24ad295 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Wed, 2 Sep 2026 14:24:02 +0000 Subject: [PATCH 3/3] identity: one generous datalog budget for every biscuit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestVerifyBiscuit_Concurrent flaked in CI: 5,000 concurrent VerifyBiscuit calls each carried a 500ms WithMaxDuration — a wall-clock bound — and on a 2-core -race runner a goroutine can be starved past that budget purely by scheduling (#348 has the measurements: raising the constant only improves the odds, no finite value is immune under enough contention). None of these tests asserts timing, so the budget only needs to never bind: all thirteen call sites (500ms at four, 5s at nine) now share a single deliberately generous testTimeout constant. --- internal/identity/biscuit_test.go | 32 ++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/internal/identity/biscuit_test.go b/internal/identity/biscuit_test.go index 3b5f6190..a0e4d5e4 100644 --- a/internal/identity/biscuit_test.go +++ b/internal/identity/biscuit_test.go @@ -31,6 +31,12 @@ import ( "github.com/libp2p/go-libp2p/core/peer" ) +// testTimeout bounds datalog evaluation in these tests. It is deliberately +// generous: WithMaxDuration is a wall-clock bound, and under -race on an +// oversubscribed CI runner a goroutine can be starved long past a sub-second +// budget purely by scheduling. No test here asserts anything about timing. +const testTimeout = time.Minute + func TestVerifyBiscuit_Expiration(t *testing.T) { pub, priv, err := ed25519.GenerateKey(rand.Reader) if err != nil { @@ -77,7 +83,7 @@ func TestVerifyBiscuit_Expiration(t *testing.T) { t.Fatalf("MintBiscuitToken failed: %v", err) } - _, err = VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) + _, err = VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, testTimeout) if tt.expectError && err == nil { t.Errorf("Expected error due to expiration, got nil") } @@ -123,7 +129,7 @@ func TestMintBiscuitToken_ClaimsTranslation(t *testing.T) { t.Fatalf("Failed to unmarshal biscuit: %v", err) } - authorizer, err := b.Authorizer(pub, biscuit.WithWorldOptions(datalog.WithMaxDuration(500*time.Millisecond))) + authorizer, err := b.Authorizer(pub, biscuit.WithWorldOptions(datalog.WithMaxDuration(testTimeout))) if err != nil { t.Fatalf("Failed to get authorizer: %v", err) } @@ -216,7 +222,7 @@ func TestVerifyBiscuit_Concurrent(t *testing.T) { go func() { defer wg.Done() for j := 0; j < 100; j++ { - _, err := VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, 500*time.Millisecond) + _, err := VerifyBiscuit(biscuitData, dummyPeer, []ed25519.PublicKey{pub}, testTimeout) if err != nil { t.Errorf("Concurrent verification failed: %v", err) return @@ -260,7 +266,7 @@ func TestMintBiscuitToken(t *testing.T) { if err != nil { t.Fatal(err) } - authorizer, err := b.Authorizer(pub, biscuit.WithWorldOptions(datalog.WithMaxDuration(500*time.Millisecond))) + authorizer, err := b.Authorizer(pub, biscuit.WithWorldOptions(datalog.WithMaxDuration(testTimeout))) if err != nil { t.Fatal(err) } @@ -360,7 +366,7 @@ func TestMintBiscuitToken_VariousClaimsTypes(t *testing.T) { t.Fatalf("Unmarshal biscuit failed: %v", err) } - authorizer, err := b.Authorizer(pub, AuthorizerOptions(5*time.Second)...) + authorizer, err := b.Authorizer(pub, AuthorizerOptions(testTimeout)...) if err != nil { t.Fatalf("Authorizer failed: %v", err) } @@ -439,7 +445,7 @@ func TestMintBiscuitToken_WithPolicyRoles(t *testing.T) { t.Fatalf("Unmarshal biscuit failed: %v", err) } - authorizer, err := b.Authorizer(pub, AuthorizerOptions(5*time.Second)...) + authorizer, err := b.Authorizer(pub, AuthorizerOptions(testTimeout)...) if err != nil { t.Fatalf("Authorizer failed: %v", err) } @@ -468,7 +474,7 @@ func TestMintBiscuitToken_WithPolicyRoles(t *testing.T) { t.Fatalf("Unmarshal biscuit failed: %v", err) } - authorizer, err := b.Authorizer(pub, AuthorizerOptions(5*time.Second)...) + authorizer, err := b.Authorizer(pub, AuthorizerOptions(testTimeout)...) if err != nil { t.Fatalf("Authorizer failed: %v", err) } @@ -504,7 +510,7 @@ func TestMintBiscuitToken_LabelFacts(t *testing.T) { // Every declared label is present as its own fact. for k, v := range labels { - authorizer, err := b.Authorizer(pub, AuthorizerOptions(5*time.Second)...) + authorizer, err := b.Authorizer(pub, AuthorizerOptions(testTimeout)...) if err != nil { t.Fatalf("Authorizer failed: %v", err) } @@ -526,7 +532,7 @@ func TestMintBiscuitToken_LabelFacts(t *testing.T) { if err != nil { t.Fatalf("Unmarshal biscuit failed: %v", err) } - authorizer, err := b2.Authorizer(pub, AuthorizerOptions(5*time.Second)...) + authorizer, err := b2.Authorizer(pub, AuthorizerOptions(testTimeout)...) if err != nil { t.Fatalf("Authorizer failed: %v", err) } @@ -566,7 +572,7 @@ func TestVerifyAndExtractPeerID_MultipleTrustedKeys(t *testing.T) { // trustedPublicKeys has pub1 first, pub2 second (pub2 is the signer) trustedKeys := []ed25519.PublicKey{pub1, pub2} - extractedPeer, err := VerifyAndExtractPeerID(trustedKeys, biscuitData, 5*time.Second) + extractedPeer, err := VerifyAndExtractPeerID(trustedKeys, biscuitData, testTimeout) if err != nil { t.Fatalf("VerifyAndExtractPeerID failed with multiple trusted keys: %v", err) } @@ -603,15 +609,15 @@ func TestExtractPeerIDExpiry(t *testing.T) { t.Fatal(err) } - if _, err := VerifyAndExtractPeerID(trustedKeys, fresh, 5*time.Second); err != nil { + if _, err := VerifyAndExtractPeerID(trustedKeys, fresh, testTimeout); err != nil { t.Errorf("unexpired token rejected: %v", err) } - if _, err := VerifyAndExtractPeerID(trustedKeys, expired, 5*time.Second); err == nil { + if _, err := VerifyAndExtractPeerID(trustedKeys, expired, testTimeout); err == nil { t.Error("expired token accepted by the expiry-enforcing variant") } // The refresh flow depends on the exempt variant staying permissive. - if got, err := VerifyExpiredAndExtractPeerID(trustedKeys, expired, 5*time.Second); err != nil { + if got, err := VerifyExpiredAndExtractPeerID(trustedKeys, expired, testTimeout); err != nil { t.Errorf("expired token rejected by the refresh variant: %v", err) } else if got != dummyPeer { t.Errorf("got peer %s, want %s", got, dummyPeer)