From ab28a0a449c1bae7bdaca5022ec261bdddf0ec63 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Thu, 20 Aug 2026 13:14:28 -0400 Subject: [PATCH] Relay the caller's Authorization to the agent when fetching its card MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /chat/{ns}/{name}/agent-card sent no Authorization header to the agent, so an agent hosted by `authbridge exec` could never have its card fetched: the inbound pipeline answered the unauthenticated request with 401 and WWW-Authenticate: Bearer. `agents card` and `agents chat` both failed, and --with-authorization did not help — that flag attaches the token to the A2A message, which is sent after the card lookup that was failing. Confirmed against a live agent by the pipeline's own policy header: the violation was auth.malformed_header before, meaning no header arrived, and auth.token_expired after, meaning the relayed token arrived and was parsed. A stub agent driven through the real command logs the full bearer token. The header is forwarded verbatim rather than read from the config file. This handler proxies one request and the caller has already chosen which identity to present; reading a token from disk would attach a credential to a request that deliberately carried none, and would make the response depend on state the caller cannot see. The destination is the instance record's loopback inbound address, never anything the request controls, which is what makes relaying a bearer token acceptable here. The 401 hint was also wrong for this case, naming the one remedy that cannot work. A 401 the server relayed from an agent now says the credentials were accepted and that signing in will not help, pointing at the pipeline policy and the token's audience instead. An unrecognized body still gets the original sign-in hint, so nothing regresses. `agents chat` help claimed the card lookup always carries the context's token; it does not when --server is given. The weather-service example comments out its spiffe and mtls blocks, since mTLS requires the SPIFFE block and a local run has no workload API socket. Assisted by Claude. Signed-off-by: Ed Snible --- cmd/agents_chat.go | 8 +- cmd/root.go | 30 ++++ cmd/root_test.go | 53 +++++++ .../authbridge-local-weather-service.yaml | 9 +- internal/serve/agentcard.go | 35 +++- internal/serve/agentcard_test.go | 149 +++++++++++++++++- 6 files changed, 269 insertions(+), 15 deletions(-) diff --git a/cmd/agents_chat.go b/cmd/agents_chat.go index 5d8ff77..af574b9 100644 --- a/cmd/agents_chat.go +++ b/cmd/agents_chat.go @@ -40,9 +40,11 @@ perfectly reachable; --address http://: is the way past it. ` + "`a2a send`" + `: the message text is sent as a single user text part, the response is streamed event by event as it arrives, and --with-authorization attaches the effective context's bearer token as an Authorization header on each -request. Note that the card lookup always carries the context's token, since it -goes to the platform API, while the message carries one only with ---with-authorization. +request. Note that the card lookup carries the context's token, since it goes to +the platform API — but an explicit --server sends none — while the message carries +one only with --with-authorization. A local cortex relays that token on to the +agent when it fetches the card, which is what lets it describe an agent hosted +behind an authbridge inbound pipeline. With --verbose both the card lookup and the message are reported on stderr.`, Args: cobra.ExactArgs(1), diff --git a/cmd/root.go b/cmd/root.go index 74b0885..7bf85d3 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -12,6 +12,7 @@ import ( "fmt" "net/http" "os" + "strings" "github.com/spf13/cobra" @@ -244,6 +245,14 @@ func errorHint(err error) string { switch statusErr.StatusCode { case http.StatusUnauthorized: + // A 401 the server relayed from something it called on our behalf is a + // different problem with different advice: our credentials were accepted, so + // signing in again fixes nothing. Reported separately rather than folded in, + // because the generic hint actively misleads here — it names the one remedy + // that cannot work. + if upstream := relayedUnauthorizedHint(statusErr.Body); upstream != "" { + return upstream + } // Deliberately not suggested for 403: that is an authenticated // identity lacking permission, where signing in again changes // nothing and the advice would send the user in a circle. @@ -253,6 +262,27 @@ func errorHint(err error) string { } } +// relayedUnauthorizedHint returns advice for a 401 whose body shows the server was +// reporting an upstream's refusal rather than its own, or "" when it was not. +// +// The distinction is worth drawing because the two are indistinguishable by status +// alone, and the remedies are opposites: for our own rejection, sign in again; for a +// relayed one, our credentials were fine and the agent is what refused them. +// +// Matched on the body text the agent-card endpoint produces (see +// internal/serve/agentcard.go), which is also the shape the Python backend's +// equivalent returns. A body this does not recognize yields "" and the caller falls +// back to the generic hint, so an unrecognized 401 is no worse than before. +func relayedUnauthorizedHint(body string) string { + if !strings.Contains(body, "failed to fetch agent card from") { + return "" + } + return "Hint: your credentials were accepted; the agent itself refused them, so " + + "`rossoctl login` will not help. An agent hosted by `authbridge exec` sits behind " + + "its inbound pipeline — check that pipeline's policy, and that the token carries " + + "the audience and scopes it requires (`rossoctl auth status`)." +} + func init() { rootCmd.PersistentFlags().BoolVarP(&verbose, "verbose", "v", false, "enable verbose output") rootCmd.PersistentFlags().StringVar(&server, "server", "", "Rossoctl API server URI (overrides the current context; default: current context's server)") diff --git a/cmd/root_test.go b/cmd/root_test.go index 01888dd..1ea4cd5 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -320,3 +320,56 @@ func TestErrorHintQuietForOtherErrors(t *testing.T) { }) } } + +// TestErrorHintDistinguishesRelayedUnauthorized verifies a 401 the server relayed +// from an agent it called on our behalf does not get the sign-in hint. +// +// The two 401s are indistinguishable by status but have opposite remedies. This body +// is what `agents card` produces for an agent behind an `authbridge exec` inbound +// pipeline: our credentials were accepted by the server, the agent refused them, and +// `rossoctl login` cannot help — so naming it would send the user to the one remedy +// that is guaranteed not to work. +func TestErrorHintDistinguishesRelayedUnauthorized(t *testing.T) { + err := &apiclient.StatusError{ + Endpoint: "http://localhost:9097/api/v1/chat/team1/weather-praxis/agent-card", + StatusCode: http.StatusUnauthorized, + Body: `{"detail":"failed to fetch agent card from 127.0.0.1:38531: agent returned 401"}`, + } + + hint := errorHint(err) + if hint == "" { + t.Fatal("a relayed 401 should still produce a hint") + } + // It must not *instruct* a sign-in. Naming the command to rule it out is fine — + // and is why this checks for the instruction rather than for the command name, + // which appears in "`rossoctl login` will not help". + if strings.Contains(hint, "Run `rossoctl login`") { + t.Errorf("hint %q must not instruct a sign-in: the server accepted the credentials", hint) + } + if !strings.Contains(hint, "will not help") { + t.Errorf("hint %q should say signing in will not help, so the reader stops reaching for it", hint) + } + // It has to say whose refusal this was, or the reader is left where they started. + if !strings.Contains(hint, "agent") { + t.Errorf("hint %q should say the agent refused the credentials", hint) + } +} + +// TestErrorHintFallsBackForUnrecognizedBody verifies a 401 whose body is not a +// recognized relay still gets the ordinary sign-in hint. +// +// The relay detection is a body-text match, so it must fail open: an unrecognized +// 401 should be no worse off than before the distinction existed. +func TestErrorHintFallsBackForUnrecognizedBody(t *testing.T) { + for _, body := range []string{ + `{"detail":"Token signing key not found"}`, + `{"detail":"failed to connect to agent at 127.0.0.1:38531: connection refused"}`, + "", + } { + err := &apiclient.StatusError{StatusCode: http.StatusUnauthorized, Body: body} + hint := errorHint(err) + if !strings.Contains(hint, "rossoctl login") { + t.Errorf("for body %q, hint = %q; want the sign-in hint", body, hint) + } + } +} diff --git a/examples/authbridge-local-weather-service.yaml b/examples/authbridge-local-weather-service.yaml index ac2f519..aa804ac 100644 --- a/examples/authbridge-local-weather-service.yaml +++ b/examples/authbridge-local-weather-service.yaml @@ -36,8 +36,6 @@ listener: # :8000 — that is authbridge's address, in front of it. reverse_proxy_backend: http://127.0.0.1:8001 mode: proxy-sidecar -mtls: - mode: permissive pipeline: inbound: plugins: @@ -61,5 +59,8 @@ pipeline: keycloak_realm: rossoctl keycloak_url: http://keycloak.localtest.me:8080/ name: token-exchange -spiffe: - socket: unix:///spiffe-workload-api/spire-agent.sock +# spiffe: +# socket: unix:///spiffe-workload-api/spire-agent.sock +# Commented out, because mTLS requires the SPIFFE block +# mtls: +# mode: permissive diff --git a/internal/serve/agentcard.go b/internal/serve/agentcard.go index 2c7c07a..e6ad05c 100644 --- a/internal/serve/agentcard.go +++ b/internal/serve/agentcard.go @@ -27,8 +27,9 @@ const agentCardPath = "/.well-known/agent-card.json" // must not hang this server's own client indefinitely. const agentCardTimeout = 10 * time.Second -// cardFetcher fetches the card document at the given URL. A variable so a test can -// answer without a live agent, following lister and getter above; production +// cardFetcher fetches the card document at the given URL, sending authorization +// verbatim as the Authorization header when it is non-empty. A variable so a test +// can answer without a live agent, following lister and getter above; production // always uses the real HTTP client. var cardFetcher = fetchCardDocument @@ -84,8 +85,25 @@ func agentCardRoute(opts) http.HandlerFunc { return } + // The caller's Authorization header is relayed to the agent. An agent hosted + // by `authbridge exec` sits behind the inbound pipeline, which answers an + // unauthenticated request with 401 and WWW-Authenticate: Bearer — so without + // this, a card could never be fetched for exactly the agents this server + // exists to describe, and the failure surfaced as a 401 from this endpoint + // that read as "your credentials were rejected" when they had never been sent. + // + // Relayed verbatim rather than read from the config file: this handler is a + // proxy for one request, and the caller has already chosen which identity to + // present. Reading a token from disk would make the response depend on state + // the caller cannot see, and would attach a credential to a request that + // deliberately carried none. + // + // Only ever sent to inst.InboundAddr, which is a loopback address this host + // recorded for a process it started. That matters: relaying a bearer token is + // safe here because the destination is not caller-controlled — it comes from + // the instance record, not from the request. cardURL := "http://" + inst.InboundAddr + agentCardPath - body, status, err := cardFetcher(r.Context(), cardURL) + body, status, err := cardFetcher(r.Context(), cardURL, r.Header.Get("Authorization")) if err != nil { // 503, matching the backend's httpx.RequestError branch: the agent could // not be reached, which is the agent's state and not a fault in this @@ -125,10 +143,16 @@ func agentCardRoute(opts) http.HandlerFunc { // fetchCardDocument GETs the card document, returning the body and status. The // body is read even for an error status so a caller can report what was served. // +// authorization, when non-empty, is sent as the Authorization header verbatim — +// scheme included, since the caller's own header is being forwarded rather than a +// token being formatted. An empty value sends no header at all, which is what an +// unauthenticated caller asked for and what an agent with no inbound policy +// expects. +// // The read is capped: this server is asking a process it does not control, and an // agent streaming an endless body should fail rather than consume this server's // memory. A card is a few kilobytes, so a megabyte is generous. -func fetchCardDocument(ctx context.Context, cardURL string) ([]byte, int, error) { +func fetchCardDocument(ctx context.Context, cardURL, authorization string) ([]byte, int, error) { ctx, cancel := context.WithTimeout(ctx, agentCardTimeout) defer cancel() @@ -137,6 +161,9 @@ func fetchCardDocument(ctx context.Context, cardURL string) ([]byte, int, error) return nil, 0, err } req.Header.Set("Accept", "application/json") + if authorization != "" { + req.Header.Set("Authorization", authorization) + } resp, err := http.DefaultClient.Do(req) if err != nil { diff --git a/internal/serve/agentcard_test.go b/internal/serve/agentcard_test.go index 4fa4525..5f64515 100644 --- a/internal/serve/agentcard_test.go +++ b/internal/serve/agentcard_test.go @@ -13,15 +13,24 @@ import ( // stubCardFetcher replaces the card fetch with a canned answer, so these tests do // not need a live agent listening on the fixture's inbound address. func stubCardFetcher(t *testing.T, body string, status int, err error) *string { + t.Helper() + requested, _ := stubCardFetcherRecording(t, body, status, err) + return requested +} + +// stubCardFetcherRecording is stubCardFetcher, additionally reporting the +// Authorization value the handler passed on. Separate so the existing callers stay +// as they were: only the relay tests care about the header. +func stubCardFetcherRecording(t *testing.T, body string, status int, err error) (requestedURL, authorization *string) { t.Helper() saved := cardFetcher - var requested string - cardFetcher = func(_ context.Context, cardURL string) ([]byte, int, error) { - requested = cardURL + var gotURL, gotAuth string + cardFetcher = func(_ context.Context, cardURL, auth string) ([]byte, int, error) { + gotURL, gotAuth = cardURL, auth return []byte(body), status, err } t.Cleanup(func() { cardFetcher = saved }) - return &requested + return &gotURL, &gotAuth } // a2aCard is a card as a v0.3 A2A server serves one: url at the top level, @@ -346,3 +355,135 @@ func TestAgentCardWithoutInboundAddressIsUnavailable(t *testing.T) { t.Errorf("detail = %q, should name the instance", detail) } } + +// getCardWithAuth requests the endpoint with an Authorization header, returning the +// status. Separate from getCard because these tests care about what was relayed +// rather than about the decoded card. +func getCardWithAuth(t *testing.T, ts *httptest.Server, path, authorization string) int { + t.Helper() + req, err := http.NewRequest(http.MethodGet, ts.URL+path, nil) + if err != nil { + t.Fatalf("building request: %v", err) + } + if authorization != "" { + req.Header.Set("Authorization", authorization) + } + res, err := ts.Client().Do(req) + if err != nil { + t.Fatalf("GET %s: %v", path, err) + } + defer func() { _ = res.Body.Close() }() + return res.StatusCode +} + +// TestAgentCardRelaysAuthorization verifies the caller's Authorization header is +// forwarded to the agent. +// +// Without this the card of an agent hosted by `authbridge exec` can never be +// fetched: the inbound pipeline answers an unauthenticated request with 401 and +// WWW-Authenticate: Bearer, and the failure surfaced here as a 401 that read as +// "your credentials were rejected" when none had been sent. +func TestAgentCardRelaysAuthorization(t *testing.T) { + stubGetter(t, mixedInstances()) + _, auth := stubCardFetcherRecording(t, a2aCard, http.StatusOK, nil) + ts := newTestServer(t, "/api/v1") + + const token = "Bearer test-token-value" + if status := getCardWithAuth(t, ts, "/api/v1/chat/recorded1/swift-falcon-0001/agent-card", token); status != http.StatusOK { + t.Fatalf("status = %d, want 200", status) + } + + // Verbatim, scheme included: the caller's own header is being forwarded, not a + // token being reformatted. + if *auth != token { + t.Errorf("relayed Authorization = %q, want %q", *auth, token) + } +} + +// TestAgentCardRelaysAuthorizationVerbatim verifies a non-Bearer scheme is passed on +// unchanged, rather than being parsed or reformatted. +// +// The handler does not interpret the credential — an agent's inbound policy decides +// what it accepts, and rewriting the header here would break a scheme this code does +// not know about. +func TestAgentCardRelaysAuthorizationVerbatim(t *testing.T) { + for _, header := range []string{ + "Bearer abc.def.ghi", + "Basic dXNlcjpwYXNz", + "DPoP some-other-credential", + } { + t.Run(header, func(t *testing.T) { + stubGetter(t, mixedInstances()) + _, auth := stubCardFetcherRecording(t, a2aCard, http.StatusOK, nil) + ts := newTestServer(t, "/api/v1") + + getCardWithAuth(t, ts, "/api/v1/chat/recorded1/swift-falcon-0001/agent-card", header) + if *auth != header { + t.Errorf("relayed %q, want %q", *auth, header) + } + }) + } +} + +// TestAgentCardSendsNoAuthorizationWhenNoneGiven verifies an unauthenticated request +// stays unauthenticated. +// +// The handler must not supply a credential of its own — reading a token from the +// config file would attach one to a request that deliberately carried none, and would +// make the response depend on state the caller cannot see. +func TestAgentCardSendsNoAuthorizationWhenNoneGiven(t *testing.T) { + stubGetter(t, mixedInstances()) + _, auth := stubCardFetcherRecording(t, a2aCard, http.StatusOK, nil) + ts := newTestServer(t, "/api/v1") + + getCard(t, ts, "/api/v1/chat/recorded1/swift-falcon-0001/agent-card") + + if *auth != "" { + t.Errorf("relayed Authorization = %q, want none for an unauthenticated caller", *auth) + } +} + +// TestFetchCardDocumentSetsAuthorization verifies the real fetcher — not the stub — +// sends the header, and sends none when the value is empty. +// +// Worth testing against a live server: every test above replaces cardFetcher, so +// nothing else covers the function that actually builds the outbound request. +func TestFetchCardDocumentSetsAuthorization(t *testing.T) { + for _, tc := range []struct { + name string + authorization string + wantHeader string + wantPresent bool + }{ + {"with a token", "Bearer xyz", "Bearer xyz", true}, + {"empty sends no header", "", "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + var got string + var present bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.Header.Get("Authorization") + _, present = r.Header["Authorization"] + if r.Header.Get("Accept") != "application/json" { + t.Errorf("Accept = %q, want application/json", r.Header.Get("Accept")) + } + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + _, status, err := fetchCardDocument(context.Background(), srv.URL, tc.authorization) + if err != nil { + t.Fatalf("fetchCardDocument: %v", err) + } + if status != http.StatusOK { + t.Errorf("status = %d, want 200", status) + } + if got != tc.wantHeader { + t.Errorf("Authorization = %q, want %q", got, tc.wantHeader) + } + if present != tc.wantPresent { + t.Errorf("Authorization present = %v, want %v", present, tc.wantPresent) + } + }) + } +}