From 255c87cc97709b1cf57bd39f1525e623dc274e9d Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sun, 26 Jul 2026 11:21:46 +0000 Subject: [PATCH] agentnetwork: tighten comment blocks to <=250 chars Condense the comments added in this change so each block stays within 250 characters (and well under 140 per line), keeping the essential rationale. No behaviour change. --- .../guardrail_multipolicy_test.go | 37 ++----------- .../internals/modules/agentnetwork/manager.go | 11 ++-- .../modules/agentnetwork/policyselect.go | 31 ++++------- .../agentnetwork/policyselect_model_test.go | 13 ++--- .../modules/agentnetwork/synthesizer.go | 52 ++++++------------- .../modules/agentnetwork/synthesizer_test.go | 10 ++-- .../builtin/llm_guardrail/factory.go | 24 +++------ .../builtin/llm_guardrail/middleware.go | 22 +++----- .../builtin/llm_guardrail/middleware_test.go | 7 ++- 9 files changed, 61 insertions(+), 146 deletions(-) diff --git a/e2e/agentnetwork/guardrail_multipolicy_test.go b/e2e/agentnetwork/guardrail_multipolicy_test.go index f78c93d28..1d664fb58 100644 --- a/e2e/agentnetwork/guardrail_multipolicy_test.go +++ b/e2e/agentnetwork/guardrail_multipolicy_test.go @@ -14,40 +14,9 @@ import ( "github.com/netbirdio/netbird/shared/management/http/api" ) -// TestGuardrailMultiPolicyModelAllowlist is the end-to-end regression guard for -// the multi-policy interactions of the model-allowlist guardrail: what happens -// when several enabled policies govern an account and some carry a guardrail -// while others don't. The earlier implementation merged every policy's allowlist -// into ONE account-wide union enforced flat on every request, which produced two -// defects this test pins: -// -// - false-ALLOW (cross-group leak): a model allowlisted only for another -// group's policy became usable by any caller, because the union ignored -// which policy/group actually authorised the request; and -// - false-DENY: a policy with NO guardrail (intended unrestricted) still had -// its traffic blocked by some other policy's allowlist. -// -// The fix scopes enforcement to the matched policy/group in management -// (SelectPolicyForRequest) with a per-provider fail-closed backstop at the -// proxy. The account here has one client in grpMain and three policies over two -// catch-all upstreams (the mock vLLM answers any model 200, so a 403 can only be -// a policy decision, never a routing miss): -// -// - polMain : grpMain -> pRestricted, guardrail allowlisting modelSelected -// - polOther: grpOther -> pRestricted, guardrail allowlisting modelOther -// - polOpen : grpMain -> pOpen, NO guardrail (unrestricted) -// -// Providers declare their models so routing is deterministic. Over the tunnel, -// as the grpMain client: -// -// - modelSelected on pRestricted is served (200) — allowed by grpMain's policy; -// - modelOther on pRestricted is denied 403 (llm_policy.model_blocked) — it is -// allowlisted only for grpOther and must NOT leak to grpMain; and -// - openModel on pOpen is served (200) — the un-guardrailed policy leaves that -// provider unrestricted and must NOT be blocked by another policy's list. -// -// Under the old account-wide union the middle case returned 200 (the leak) and -// the last case returned 403 (the false-deny); both are inverted here. +// TestGuardrailMultiPolicyModelAllowlist: modelSelected served (200), grpOther's +// modelOther denied for the grpMain client (403 model_blocked, no cross-group +// leak), and openModel on the un-guardrailed policy's provider served (200). func TestGuardrailMultiPolicyModelAllowlist(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Minute) defer cancel() diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 9c1f979d5..77c77ce44 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -92,13 +92,10 @@ type PolicySelectionInput struct { UserID string GroupIDs []string ProviderID string - // Model is the (already-normalised) upstream model identifier the proxy - // extracted from the request. The proxy's request parser strips - // path-routed provider decoration (Bedrock region/version, Vertex - // @version) before emitting it, so a plain case-insensitive compare - // against a guardrail allowlist is sufficient here. Empty when the model - // could not be determined — treated as "not permitted" by any policy that - // restricts models (fail closed), mirroring the proxy guardrail. + // Model is the already-normalised upstream model id the proxy extracted + // (parser strips Bedrock region/version, Vertex @version), so a + // case-insensitive compare suffices. Empty = undetermined → not permitted + // (fail closed). Model string } diff --git a/management/internals/modules/agentnetwork/policyselect.go b/management/internals/modules/agentnetwork/policyselect.go index f2495d6c5..9bb893b36 100644 --- a/management/internals/modules/agentnetwork/policyselect.go +++ b/management/internals/modules/agentnetwork/policyselect.go @@ -37,9 +37,8 @@ const ( //nolint:gosec // account deny code label, not a credential denyCodeAccountBudgetCapExceeded = "llm_account.budget_cap_exceeded" // denyCodeModelBlocked is returned when policies govern the request's - // (provider, caller-groups) but none of them permits the requested model - // under its guardrail allowlist. Matches the proxy guardrail's code so the - // two enforcement layers surface the same label. + // (provider, caller-groups) but none permits the model. Matches the proxy + // guardrail's code so both layers surface the same label. denyCodeModelBlocked = "llm_policy.model_blocked" ) @@ -165,18 +164,9 @@ func (m *managerImpl) SelectPolicyForRequest(ctx context.Context, in PolicySelec } candidates := filterApplicablePolicies(policies, in) - // Model-allowlist gate (per-policy, per-group). Among the policies that - // authorise this (provider, caller-groups), keep only those whose guardrails - // permit the requested model; a policy with no allowlist-enabled guardrail is - // unrestricted and always permits. When policies govern this request but none - // permits the model, deny. This is the authoritative allowlist decision: - // because it is scoped to the matched policies, a model allowlisted for one - // group/provider never leaks to another (no account-wide union), and a policy - // that carries no guardrail is genuinely unrestricted rather than being caught - // by some other policy's allowlist. - // Only consult guardrails when at least one candidate policy references one; - // a policy with no guardrail is unrestricted, so when none of them carries a - // guardrail the gate is a no-op and we skip the store read entirely. + // Model-allowlist gate scoped to the matched policies: keep candidates whose + // guardrails permit the model (none enabled = unrestricted), deny when + // policies apply but none permits it. Skip the load when none has a guardrail. if len(candidates) > 0 && anyPolicyHasGuardrails(candidates) { guardrailsByID, gErr := m.loadGuardrailsByID(ctx, in.AccountID) if gErr != nil { @@ -323,13 +313,10 @@ func filterModelPermittedPolicies(policies []*types.Policy, byID map[string]*typ return out } -// policyPermitsModel reports whether a policy permits the requested model under -// its attached guardrails. A policy with no allowlist-enabled guardrail is -// unrestricted (permits any model, including an empty/undetermined one). A -// policy with one or more allowlist-enabled guardrails permits the model only -// when it appears in the union of those allowlists; an empty/undetermined model -// never matches a non-empty allowlist, so such a policy fails closed — the same -// contract the proxy guardrail enforces for path-routed providers. +// policyPermitsModel reports whether a policy permits the model. No +// allowlist-enabled guardrail = unrestricted (permits any, incl. empty); +// otherwise the model must be in the union of its allowlists, so an +// empty/undetermined model fails closed. func policyPermitsModel(p *types.Policy, byID map[string]*types.Guardrail, model string) bool { if p == nil { return false diff --git a/management/internals/modules/agentnetwork/policyselect_model_test.go b/management/internals/modules/agentnetwork/policyselect_model_test.go index 9097ac150..56c1dac23 100644 --- a/management/internals/modules/agentnetwork/policyselect_model_test.go +++ b/management/internals/modules/agentnetwork/policyselect_model_test.go @@ -120,10 +120,9 @@ func TestSelectPolicy_CaseInsensitiveModelMatch(t *testing.T) { assert.True(t, res.Allow, "case/whitespace variants must match the allowlist entry") } -// TestSelectPolicy_UnguardedPolicyIsUnrestricted is the false-deny fix: when -// two policies authorise the same (provider, group) and one carries no -// guardrail, that un-guardrailed policy makes the request unrestricted — it is -// NOT caught by the other policy's allowlist. +// TestSelectPolicy_UnguardedPolicyIsUnrestricted is the false-deny fix: when two +// policies authorise the same (provider, group) and one has no guardrail, that +// policy makes the request unrestricted — not caught by the other's allowlist. func TestSelectPolicy_UnguardedPolicyIsUnrestricted(t *testing.T) { ctrl := gomock.NewController(t) mgr, mockStore := newSelectorMgr(t, ctrl) @@ -146,10 +145,8 @@ func TestSelectPolicy_UnguardedPolicyIsUnrestricted(t *testing.T) { } // TestSelectPolicy_AllowlistDoesNotLeakAcrossGroups is the false-allow fix: a -// model allowlisted only for grp-b must not be usable by a caller in grp-a, -// even though both groups' policies target the same provider. The selector only -// considers policies applicable to the caller's groups, so grp-b's allowlist -// never enters grp-a's decision. +// model allowlisted only for grp-b must not be usable by a grp-a caller. The +// selector considers only policies applicable to the caller's groups. func TestSelectPolicy_AllowlistDoesNotLeakAcrossGroups(t *testing.T) { ctrl := gomock.NewController(t) mgr, mockStore := newSelectorMgr(t, ctrl) diff --git a/management/internals/modules/agentnetwork/synthesizer.go b/management/internals/modules/agentnetwork/synthesizer.go index 6099d70d4..3349d3ac4 100644 --- a/management/internals/modules/agentnetwork/synthesizer.go +++ b/management/internals/modules/agentnetwork/synthesizer.go @@ -235,12 +235,10 @@ func SynthesizeServices(ctx context.Context, s store.Store, accountID string) ([ mergedGuardrails := mergeGuardrails(enabledPolicies, guardrailsByID) applyAccountCollectionControls(&mergedGuardrails, settings) - // The proxy guardrail is the fail-closed backstop, scoped per provider so it - // never applies one policy's allowlist to another provider's traffic. The - // authoritative per-policy/group model decision lives in management's - // SelectPolicyForRequest; a provider only lands in this map when EVERY policy - // authorising it restricts models, so an un-guardrailed policy leaves its - // providers unrestricted here. + // The proxy guardrail is a per-provider fail-closed backstop; the + // authoritative per-policy/group decision is management's + // SelectPolicyForRequest. A provider lands in this map only when every + // authorising policy restricts models. providerAllowlists := buildProviderAllowlists(enabledPolicies, guardrailsByID) guardrailJSON, err := marshalGuardrailConfig(providerAllowlists, mergedGuardrails.PromptCapture) if err != nil { @@ -852,15 +850,9 @@ func marshalGuardrailConfig(providerAllowlists map[string][]string, capture Merg return out, nil } -// buildProviderAllowlists computes the per-provider model allowlist the proxy -// guardrail enforces as a fail-closed backstop. A provider is included ONLY when -// every enabled policy that authorises it restricts models (has at least one -// allowlist-enabled guardrail); its list is then the union of those policies' -// allowed models. If any authorising policy leaves models unrestricted, the -// provider is omitted entirely — the proxy treats an absent provider as -// unrestricted and defers to management's per-policy/group decision, so a -// mixed set of policies can neither leak a model across providers nor wrongly -// block an un-guardrailed policy's traffic. +// buildProviderAllowlists returns the proxy's per-provider backstop: a provider +// is included only when every authorising policy restricts models (their union); +// if any leaves it unrestricted it is omitted, so management decides per group. func buildProviderAllowlists(policies []*types.Policy, byID map[string]*types.Guardrail) map[string][]string { type providerAcc struct { models map[string]struct{} @@ -905,10 +897,9 @@ func buildProviderAllowlists(policies []*types.Policy, byID map[string]*types.Gu return out } -// policyModelAllowlist reports whether a policy restricts models (has at least -// one allowlist-enabled guardrail) and the union of allowed models across those -// guardrails. Models are returned verbatim; the proxy factory lowercases and -// trims them at decode time, matching the runtime compare. +// policyModelAllowlist reports whether a policy restricts models (has an +// allowlist-enabled guardrail) and the union of allowed models. Models are +// verbatim; the proxy factory lowercases/trims them at decode time. func policyModelAllowlist(p *types.Policy, byID map[string]*types.Guardrail) (bool, []string) { restricted := false var models []string @@ -1070,11 +1061,9 @@ func unionSourceGroups(policies []*types.Policy) []string { return out } -// MergedGuardrails is the intermediate the synthesiser folds policy guardrails -// into. Only prompt capture is merged here — the model allowlist is emitted -// per-provider via buildProviderAllowlists, and token/budget/retention moved off -// guardrails onto Policy.Limits and account Settings — so this carries just the -// prompt-capture decision the capture parsers and guardrail config consume. +// MergedGuardrails is the synthesiser's fold target. Only prompt capture is +// merged here — the model allowlist is emitted per-provider, and +// token/budget/retention moved onto Policy.Limits and account Settings. type MergedGuardrails struct { PromptCapture MergedPromptCapture } @@ -1084,17 +1073,10 @@ type MergedPromptCapture struct { RedactPii bool `json:"redact_pii"` } -// mergeGuardrails computes the prompt-capture portion of the effective -// guardrail spec, given the referencing policies and the account's guardrail -// catalogue. Policy enabled-ness is the caller's responsibility — only enabled -// policies should be passed in. -// -// The model allowlist is NOT merged here: it is enforced per-policy/group in -// management (SelectPolicyForRequest) and shipped to the proxy per-provider via -// buildProviderAllowlists, so a single account-wide union would be both -// incorrect (leaks a model across groups/providers) and redundant. Token, -// budget, and retention likewise live off guardrails now (Policy.Limits and -// account Settings), so prompt capture is all that remains to merge. +// mergeGuardrails folds the referencing policies' guardrails into the +// prompt-capture decision only. The model allowlist is enforced per-policy/group +// in management and shipped per-provider; token/budget/retention live off +// guardrails now. // // Merge rule — prompt capture: enabled if any policy enables it; redact_pii // sticks if any enabling policy turns it on. diff --git a/management/internals/modules/agentnetwork/synthesizer_test.go b/management/internals/modules/agentnetwork/synthesizer_test.go index 04948fdbe..8a18a9b59 100644 --- a/management/internals/modules/agentnetwork/synthesizer_test.go +++ b/management/internals/modules/agentnetwork/synthesizer_test.go @@ -1031,12 +1031,10 @@ func TestSynthesizeServices_GuardrailMerge_AllowlistUnion_LimitsRestrictive(t *t var cfg guardrailConfig require.NoError(t, json.Unmarshal(guardrailJSON, &cfg), "guardrail config must unmarshal cleanly") - // Both policies restrict the same provider, so the proxy-side per-provider - // backstop carries the union of their models. This coarse union is - // deliberate: it can still let grp-a reach grp-b's model at the proxy layer, - // which management's per-policy/group check (SelectPolicyForRequest) is the - // one that rejects. The backstop only guarantees nothing outside the - // provider's union slips through when management is unreachable. + // Both policies restrict the same provider, so the per-provider backstop + // carries the union of their models — a coarse gate that management's + // per-policy/group check narrows; it only blocks models outside the union + // when management is down. assert.ElementsMatch(t, []string{"gpt-5.4-mini", "gpt-5.4-pro"}, cfg.ProviderAllowlists["prov-1"], "per-provider allowlist union must keep both models") } diff --git a/proxy/internal/middleware/builtin/llm_guardrail/factory.go b/proxy/internal/middleware/builtin/llm_guardrail/factory.go index 3409ce0c0..9cec0dc68 100644 --- a/proxy/internal/middleware/builtin/llm_guardrail/factory.go +++ b/proxy/internal/middleware/builtin/llm_guardrail/factory.go @@ -13,17 +13,10 @@ import ( // runtime path consumes the normalised allowlists; raw config is not // retained beyond construction. type Config struct { - // ProviderAllowlists maps a resolved provider id (the value llm_router - // stamps as KeyLLMResolvedProviderID) to that provider's model allowlist. A - // provider present here is restricted to the listed models; a provider - // absent is unrestricted. The synthesiser only lists a provider when EVERY - // policy authorising it enables an allowlist, so a provider reachable by any - // un-guardrailed policy is intentionally absent (unrestricted) here and the - // precise per-policy/group decision is left to management. Keeping the gate - // per-provider — rather than one account-wide union — is what stops a model - // allowlisted for one provider from leaking onto another and stops an - // un-guardrailed policy's traffic from being blocked by an unrelated - // policy's allowlist. + // ProviderAllowlists maps a resolved provider id (KeyLLMResolvedProviderID) to + // its model allowlist. A provider present is restricted to those models; one + // absent is unrestricted. Kept per-provider so one provider's list can't leak + // onto another. ProviderAllowlists map[string][]string `json:"provider_allowlists,omitempty"` PromptCapture PromptCapture `json:"prompt_capture"` } @@ -65,11 +58,10 @@ func isEmptyJSON(raw []byte) bool { return false } -// normaliseConfig lowercases and trims allowlist entries so the runtime -// match is case-insensitive. Empty entries are dropped. A provider whose -// entries all drop out keeps an empty (non-nil) list — an allowlist that -// permits nothing — which is the intended "deny every model" for that -// provider, distinct from the provider being absent (unrestricted). +// normaliseConfig lowercases and trims allowlist entries for case-insensitive +// matching; empty entries drop. A provider whose entries all drop keeps an empty +// (non-nil) list — "deny every model" — distinct from an absent provider +// (unrestricted). func normaliseConfig(cfg Config) Config { if len(cfg.ProviderAllowlists) == 0 { cfg.ProviderAllowlists = nil diff --git a/proxy/internal/middleware/builtin/llm_guardrail/middleware.go b/proxy/internal/middleware/builtin/llm_guardrail/middleware.go index 671906763..1863aff20 100644 --- a/proxy/internal/middleware/builtin/llm_guardrail/middleware.go +++ b/proxy/internal/middleware/builtin/llm_guardrail/middleware.go @@ -111,20 +111,16 @@ func (m *Middleware) Invoke(_ context.Context, in *middleware.Input) (*middlewar // is a no-op. func (m *Middleware) Close() error { return nil } -// evaluateAllowlist returns a deny Output when the allowlist for the request's -// resolved provider rejects the model. A nil return means the request should -// proceed. The allowlist is scoped to the provider llm_router resolved: a -// provider absent from the config is unrestricted, so an un-guardrailed policy's -// traffic is never caught by an unrelated provider's allowlist. +// evaluateAllowlist denies when the resolved provider's allowlist rejects the +// model; nil means proceed. Scoped to the provider llm_router resolved, so an +// unrestricted provider (absent from config) is never caught by another's list. func (m *Middleware) evaluateAllowlist(providerID, model string, modelPresent bool) *middleware.Output { if len(m.cfg.ProviderAllowlists) == 0 { return nil } - // Restrictions exist for this account but the resolved provider is unknown, - // so we cannot tell whether this request is bound for a restricted provider. - // Fail closed rather than wave it through. In practice llm_router always - // stamps the resolved provider before the guardrail runs (or denies first), - // so this is a defensive guard, not the common path. + // Restrictions exist but the resolved provider is unknown, so we can't tell + // if this request targets a restricted provider — fail closed. llm_router + // normally stamps the provider first, so this is a defensive guard. if providerID == "" { return denyModel("", denyCodeModelUnknown, denyMessageModelUnknown, denyReasonModelUnknown) } @@ -135,10 +131,8 @@ func (m *Middleware) evaluateAllowlist(providerID, model string, modelPresent bo return nil } // Fail closed: with an allowlist in effect for this provider, a request whose - // model the upstream parser could not extract (absent or empty) must be - // denied rather than allowed. This is what enforces the allowlist for - // URL/path-routed providers (Bedrock, Vertex, ...) whose model lives outside - // the JSON body. + // model the parser couldn't extract (absent/empty) is denied. This enforces + // the allowlist for path-routed providers (Bedrock, Vertex) with no body model. if !modelPresent || normaliseModel(model) == "" { return denyModel("", denyCodeModelUnknown, denyMessageModelUnknown, denyReasonModelUnknown) } diff --git a/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go b/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go index b27974b50..2c3b413c9 100644 --- a/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go +++ b/proxy/internal/middleware/builtin/llm_guardrail/middleware_test.go @@ -164,10 +164,9 @@ func TestAllowlistEmptyListAllowsMissingModel(t *testing.T) { } func TestUnrestrictedProviderAllowsAnyModel(t *testing.T) { - // testProvider is restricted, but the request resolved to otherProvider, - // which carries no allowlist. Its traffic must not be caught by the other - // provider's restriction — this is the cross-provider-leak / false-deny - // guard. Management owns any per-policy/group decision for otherProvider. + // The request resolved to otherProvider, which has no allowlist, so its + // traffic must not be caught by testProvider's restriction — the + // cross-provider-leak / false-deny guard. mw := New(providerCfg("gpt-4o")) out, err := mw.Invoke(context.Background(), newInputProvider(otherProvider, middleware.KV{Key: middleware.KeyLLMModel, Value: "claude-opus-4"},