-
Notifications
You must be signed in to change notification settings - Fork 38
Fix: Pin IBAC user intent against FIFO eviction; add listener.skip_hosts #500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| package extproc | ||
|
|
||
| import ( | ||
| "context" | ||
| "sync/atomic" | ||
| "testing" | ||
| "time" | ||
|
|
||
| extprocv3 "github.com/envoyproxy/go-control-plane/envoy/service/ext_proc/v3" | ||
|
|
||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/listener/skiphost" | ||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/pipeline" | ||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/plugins/plugintesting" | ||
| "github.com/kagenti/kagenti-extensions/authbridge/authlib/session" | ||
| ) | ||
|
|
||
| // markerPlugin records one Invocation per OnRequest call so tests can | ||
| // assert whether the outbound pipeline ran. Mirrors the helper in the | ||
| // forwardproxy skiphost tests; kept local to avoid a public test-only | ||
| // type in plugintesting. | ||
| type markerPlugin struct { | ||
| calls atomic.Int32 | ||
| } | ||
|
|
||
| func (p *markerPlugin) Name() string { return "marker" } | ||
| func (p *markerPlugin) Capabilities() pipeline.PluginCapabilities { return pipeline.PluginCapabilities{} } | ||
| func (p *markerPlugin) OnResponse(context.Context, *pipeline.Context) pipeline.Action { | ||
| return pipeline.Action{Type: pipeline.Continue} | ||
| } | ||
|
|
||
| func (p *markerPlugin) OnRequest(_ context.Context, pctx *pipeline.Context) pipeline.Action { | ||
| p.calls.Add(1) | ||
| pctx.Record(pipeline.Invocation{ | ||
| Plugin: "marker", | ||
| Action: pipeline.ActionObserve, | ||
| Phase: pipeline.InvocationPhaseRequest, | ||
| Reason: "ran", | ||
| }) | ||
| return pipeline.Action{Type: pipeline.Continue} | ||
| } | ||
|
|
||
| func newSkipServer(t *testing.T, store *session.Store, skip *skiphost.Matcher) (*Server, *markerPlugin) { | ||
| t.Helper() | ||
| inbound, err := plugintesting.BuildPipeline([]pipeline.Plugin{}) | ||
| if err != nil { | ||
| t.Fatalf("building inbound pipeline: %v", err) | ||
| } | ||
| mp := &markerPlugin{} | ||
| outbound, err := plugintesting.BuildPipeline([]pipeline.Plugin{mp}) | ||
| if err != nil { | ||
| t.Fatalf("building outbound pipeline: %v", err) | ||
| } | ||
| return &Server{ | ||
| InboundPipeline: pipeline.NewHolder(inbound), | ||
| OutboundPipeline: pipeline.NewHolder(outbound), | ||
| Sessions: store, | ||
| SkipHosts: skip, | ||
| }, mp | ||
| } | ||
|
|
||
| // TestExtProc_SkipHosts_OutboundBypass asserts the headers-only outbound | ||
| // path: a SkipHosts-matched destination produces zero plugin invocations | ||
| // and zero session events, and the response is a plain pass-through. | ||
| // The motivating case is OTel-collector traffic in envoy-sidecar | ||
| // deployments — without this gate, every export from the agent would | ||
| // run the pipeline and append a session event, evicting the inbound | ||
| // A2A user intent from the FIFO buffer. | ||
| func TestExtProc_SkipHosts_OutboundBypass(t *testing.T) { | ||
| store := session.New(5*time.Minute, 100, 0) | ||
| defer store.Close() | ||
|
|
||
| // Match the agent's exgentic-style FQDN pattern: a leading-* glob | ||
| // against the fixed suffix is the operator-friendly way to write | ||
| // this and matches the hostname after net.SplitHostPort strips the | ||
| // :8335 from pctx.Host. | ||
| skip, err := skiphost.New([]string{"otel-collector.*.svc.cluster.local"}) | ||
| if err != nil { | ||
| t.Fatalf("skiphost.New: %v", err) | ||
| } | ||
|
|
||
| srv, mp := newSkipServer(t, store, skip) | ||
|
|
||
| stream := &mockStream{ | ||
| ctx: context.Background(), | ||
| requests: []*extprocv3.ProcessingRequest{ | ||
| outboundRequest(makeHeaders( | ||
| ":authority", "otel-collector.kagenti-system.svc.cluster.local:8335", | ||
| ":path", "/v1/traces", | ||
| )), | ||
| }, | ||
| } | ||
|
|
||
| _ = srv.Process(stream) | ||
|
|
||
| if len(stream.responses) != 1 { | ||
| t.Fatalf("expected 1 response, got %d", len(stream.responses)) | ||
| } | ||
| rh := stream.responses[0].GetRequestHeaders() | ||
| if rh == nil { | ||
| t.Fatal("expected RequestHeaders pass-through response") | ||
| } | ||
| if rh.Response != nil && rh.Response.HeaderMutation != nil && | ||
| len(rh.Response.HeaderMutation.SetHeaders) > 0 { | ||
| t.Error("skipped host must not have header mutations (pipeline did not run)") | ||
| } | ||
| if mp.calls.Load() != 0 { | ||
| t.Errorf("pipeline ran %d times; want 0 — SkipHosts must short-circuit before pipeline.Run", mp.calls.Load()) | ||
| } | ||
| if sessions := store.ListSessions(); len(sessions) != 0 { | ||
| t.Errorf("%d session(s) recorded; want 0 — SkipHosts must skip recording entirely", len(sessions)) | ||
| } | ||
| } | ||
|
|
||
| // TestExtProc_SkipHosts_NonMatchingRunsPipeline is the regression guard: | ||
| // with a SkipHosts list set, hosts that don't match must still run the | ||
| // pipeline and have their Invocation recorded. Without this pairing, | ||
| // the bypass test above could pass trivially with a globally disabled | ||
| // pipeline. | ||
| func TestExtProc_SkipHosts_NonMatchingRunsPipeline(t *testing.T) { | ||
| store := session.New(5*time.Minute, 100, 0) | ||
| defer store.Close() | ||
|
|
||
| skip, err := skiphost.New([]string{"otel-collector*"}) | ||
| if err != nil { | ||
| t.Fatalf("skiphost.New: %v", err) | ||
| } | ||
|
|
||
| srv, mp := newSkipServer(t, store, skip) | ||
|
|
||
| stream := &mockStream{ | ||
| ctx: context.Background(), | ||
| requests: []*extprocv3.ProcessingRequest{ | ||
| outboundRequest(makeHeaders( | ||
| ":authority", "github-tool-mcp:8000", | ||
| ":path", "/mcp", | ||
| )), | ||
| }, | ||
| } | ||
|
|
||
| _ = srv.Process(stream) | ||
|
|
||
| if mp.calls.Load() != 1 { | ||
| t.Errorf("pipeline ran %d times; want 1 — host did not match skip list", mp.calls.Load()) | ||
| } | ||
| if sessions := store.ListSessions(); len(sessions) != 1 { | ||
| t.Errorf("session count = %d; want 1 — Invocation should drive recording for non-skipped hosts", len(sessions)) | ||
| } | ||
| } | ||
|
|
||
| // TestExtProc_SkipHosts_NilMatcherPreservesBehavior asserts the | ||
| // upgrade-safety contract: a Server without SkipHosts (nil Matcher) | ||
| // behaves identically to today's code. Pipeline runs, sessions record. | ||
| func TestExtProc_SkipHosts_NilMatcherPreservesBehavior(t *testing.T) { | ||
| store := session.New(5*time.Minute, 100, 0) | ||
| defer store.Close() | ||
|
|
||
| srv, mp := newSkipServer(t, store, nil) | ||
|
|
||
| stream := &mockStream{ | ||
| ctx: context.Background(), | ||
| requests: []*extprocv3.ProcessingRequest{ | ||
| outboundRequest(makeHeaders( | ||
| ":authority", "any-service", | ||
| ":path", "/", | ||
| )), | ||
| }, | ||
| } | ||
|
|
||
| _ = srv.Process(stream) | ||
|
|
||
| if mp.calls.Load() != 1 { | ||
| t.Errorf("nil SkipHosts: pipeline ran %d times, want 1", mp.calls.Load()) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (security) — The skip match keys on
pctx.Host, which on this path is the agent-supplied:authority(fallbackhostheader) — i.e. the agent proposes the value that decides whether it's exempted from OBO/policy/IBAC. The CONNECT path (forwardproxy/server.go:710) is safe-by-construction (it dials the samer.Hostit matched), but ext_proc trusts Envoy's authority handling.Two asks: (a) document in
config.gothat ext_proc-mode skip trusts:authorityso operators know the match value is only as trustworthy as Envoy's authority rewriting; and (b) emit a counter / structured log on each skip — a skipped host currently leaves no trace in/v1/sessionsor abctl, so a successful self-exemption is invisible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in e8e632d.
(a)
ListenerConfig.SkipHostsgodoc now spells out the trust model per deployment shape — ext_proc keys on Envoy's:authority(agent-controlled), HTTP-forward keys onr.Host(agent-controlled), CONNECT keys onr.Hostwhich IS the dial target so it's safe-by-construction. Explicit "do NOT list a destination here you'd want IBAC / token-exchange to deny on."(b) Each skip now emits a
slog.Infoline carrying host, matched pattern, and method/path on every skip path (extproc handleOutbound + handleOutboundBody, forwardproxy handleRequest + handleConnect). Skipped operator config:MatchPattern(host) (string, bool)returns the raw operator-supplied pattern that matched, so the log entry attributes back to the specificskip_hostsline. No counter yet — wanted to keep the change scoped to "leave a trace" rather than build out a stats surface; happy to follow up if you want the counter too.