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) + } + }) + } +}