From 6ba36bbbc1cd636311a530a9d7d4638ee356cdec Mon Sep 17 00:00:00 2001 From: wolo Date: Thu, 9 Jul 2026 07:13:05 +0000 Subject: [PATCH 01/15] feat(auth/gcp): add credentials-service REST client Add a hand-rolled REST client for the Google Cloud Agent Identity and IAM Connector credential services. Given a resource name it routes to the right service, retrieves an end-user credential, polls while the service reports a non-interactive pending state, and maps the {header, token} result to an auth.Credential (bearer or header API key), or to auth.ErrConsentRequired when interactive consent is required. No generated Go clients exist for these preview services (verified: no such module on pkg.go.dev) and the surface is a single RPC, so this is hand-rolled over net/http, authenticating calls with Application Default Credentials (cloud-platform). No new module dependencies. This is the transport layer only; the CredentialProvider that resolves the acting user from the invocation context is a later step (it needs a shared agent.FromContext helper). --- auth/gcp/agentidentity.go | 74 ++++++++++ auth/gcp/client.go | 227 ++++++++++++++++++++++++++++ auth/gcp/client_test.go | 304 ++++++++++++++++++++++++++++++++++++++ auth/gcp/connector.go | 82 ++++++++++ auth/gcp/doc.go | 30 ++++ 5 files changed, 717 insertions(+) create mode 100644 auth/gcp/agentidentity.go create mode 100644 auth/gcp/client.go create mode 100644 auth/gcp/client_test.go create mode 100644 auth/gcp/connector.go create mode 100644 auth/gcp/doc.go diff --git a/auth/gcp/agentidentity.go b/auth/gcp/agentidentity.go new file mode 100644 index 000000000..5a8d689ad --- /dev/null +++ b/auth/gcp/agentidentity.go @@ -0,0 +1,74 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gcp + +import ( + "context" + "fmt" +) + +// agentIdentityRequest is the JSON body for RetrieveCredentials (the auth +// provider is bound to the URL path, not the body). +type agentIdentityRequest struct { + UserID string `json:"userId,omitempty"` + Scopes []string `json:"scopes,omitempty"` + ContinueURI string `json:"continueUri,omitempty"` +} + +// agentIdentityResponse mirrors the RetrieveCredentialsResponse "result" oneof. +type agentIdentityResponse struct { + Success *struct { + Token string `json:"token"` + Header string `json:"header"` + } `json:"success"` + Pending *struct{} `json:"pending"` + UriConsentRequired *consentDetail `json:"uriConsentRequired"` + ConsentRejected *struct{} `json:"consentRejected"` +} + +// consentDetail is the shared uri-consent payload across both services. +type consentDetail struct { + AuthorizationURI string `json:"authorizationUri"` + ConsentNonce string `json:"consentNonce"` +} + +// retrieveAgentIdentity calls the Agent Identity service, whose response is +// returned synchronously (no long-running-operation wrapper). +func (c *Client) retrieveAgentIdentity(ctx context.Context, req Request) (retrieveResult, error) { + url := fmt.Sprintf("%s/v1/%s/credentials:retrieve", c.agentIdentityURL, req.Resource) + body := agentIdentityRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} + + var out agentIdentityResponse + if err := c.doPost(ctx, url, body, &out); err != nil { + return retrieveResult{}, err + } + + switch { + case out.Success != nil: + return retrieveResult{status: statusOK, token: out.Success.Token, header: out.Success.Header}, nil + case out.UriConsentRequired != nil: + return retrieveResult{ + status: statusConsentRequired, + consentURI: out.UriConsentRequired.AuthorizationURI, + consentNonce: out.UriConsentRequired.ConsentNonce, + }, nil + case out.ConsentRejected != nil: + return retrieveResult{status: statusRejected}, nil + case out.Pending != nil: + return retrieveResult{status: statusPending}, nil + default: + return retrieveResult{}, fmt.Errorf("gcp: agent identity returned an empty result for %q", req.Resource) + } +} diff --git a/auth/gcp/client.go b/auth/gcp/client.go new file mode 100644 index 000000000..949bc1ce2 --- /dev/null +++ b/auth/gcp/client.go @@ -0,0 +1,227 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gcp + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "regexp" + "strings" + "time" + + "golang.org/x/oauth2" + "golang.org/x/oauth2/google" + + "google.golang.org/adk/v2/auth" +) + +const ( + cloudPlatformScope = "https://www.googleapis.com/auth/cloud-platform" + defaultAgentIdentityURL = "https://agentidentitycredentials.googleapis.com" + defaultConnectorURL = "https://iamconnectorcredentials.googleapis.com" + + defaultPollTimeout = 10 * time.Second + defaultInitialBackoff = 500 * time.Millisecond + maxBackoff = 8 * time.Second +) + +// connectorResourceRE matches an IAM Connector resource name; anything else is +// routed to the Agent Identity service (same split as adk-python). +var connectorResourceRE = regexp.MustCompile(`^projects/[^/]+/locations/[^/]+/connectors/[^/]+$`) + +// Client retrieves end-user credentials from the Agent Identity / IAM Connector +// credential services and maps them to [auth.Credential]. +type Client struct { + httpClient *http.Client + agentIdentityURL string + connectorURL string + pollTimeout time.Duration + initialBackoff time.Duration +} + +// Option configures a [Client]. +type Option func(*Client) + +// WithHTTPClient sets the HTTP client used to call the credential services. +// When unset, [NewClient] builds one from Application Default Credentials. +func WithHTTPClient(h *http.Client) Option { return func(c *Client) { c.httpClient = h } } + +// WithAgentIdentityEndpoint overrides the Agent Identity base URL (scheme+host). +func WithAgentIdentityEndpoint(url string) Option { + return func(c *Client) { c.agentIdentityURL = url } +} + +// WithConnectorEndpoint overrides the IAM Connector base URL (scheme+host). +func WithConnectorEndpoint(url string) Option { + return func(c *Client) { c.connectorURL = url } +} + +// WithPollTimeout bounds the total time spent polling a pending retrieval. +func WithPollTimeout(d time.Duration) Option { return func(c *Client) { c.pollTimeout = d } } + +// NewClient builds a Client. Unless [WithHTTPClient] is supplied, it discovers +// Application Default Credentials (cloud-platform scope) to authenticate calls +// to the credential services. +func NewClient(ctx context.Context, opts ...Option) (*Client, error) { + c := &Client{ + agentIdentityURL: defaultAgentIdentityURL, + connectorURL: defaultConnectorURL, + pollTimeout: defaultPollTimeout, + initialBackoff: defaultInitialBackoff, + } + for _, opt := range opts { + opt(c) + } + if c.httpClient == nil { + creds, err := google.FindDefaultCredentials(ctx, cloudPlatformScope) + if err != nil { + return nil, fmt.Errorf("gcp: find default credentials: %w", err) + } + c.httpClient = oauth2.NewClient(ctx, creds.TokenSource) + } + return c, nil +} + +// Request identifies the resource and acting user for a credential retrieval. +type Request struct { + // Resource is a full resource name. A name matching + // projects/*/locations/*/connectors/* is routed to the IAM Connector + // service; anything else (e.g. .../authProviders/*) to Agent Identity. + Resource string + // UserID is the acting end user's identity. Required. + UserID string + // Scopes are the OAuth scopes requested for the credential. + Scopes []string + // ContinueURI is the developer-hosted URI used to finalize managed-OAuth + // (3-legged) flows. Unused by non-interactive flows. + ContinueURI string +} + +// RetrieveCredential retrieves a credential for req, polling while the service +// reports a non-interactive pending state (up to the configured poll timeout). +// If interactive consent is required it returns an [auth.ConsentRequiredError]. +func (c *Client) RetrieveCredential(ctx context.Context, req Request) (*auth.Credential, error) { + if req.Resource == "" { + return nil, fmt.Errorf("gcp: RetrieveCredential requires a Resource") + } + if req.UserID == "" { + return nil, fmt.Errorf("gcp: RetrieveCredential requires a UserID") + } + + retrieve := c.retrieveAgentIdentity + if connectorResourceRE.MatchString(req.Resource) { + retrieve = c.retrieveConnector + } + + deadline := time.Now().Add(c.pollTimeout) + backoff := c.initialBackoff + for { + res, err := retrieve(ctx, req) + if err != nil { + return nil, err + } + switch res.status { + case statusOK: + return mapCredential(res.header, res.token) + case statusConsentRequired: + return nil, &auth.ConsentRequiredError{AuthURI: res.consentURI, Nonce: res.consentNonce} + case statusRejected: + return nil, fmt.Errorf("gcp: user consent rejected for %q", req.Resource) + case statusPending: + remaining := time.Until(deadline) + if remaining <= 0 { + return nil, fmt.Errorf("gcp: timed out waiting for credentials for %q", req.Resource) + } + wait := min(backoff, remaining) + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-time.After(wait): + } + backoff = min(backoff*2, maxBackoff) + } + } +} + +// retrieveStatus is the normalized outcome of a single retrieval call. +type retrieveStatus int + +const ( + statusOK retrieveStatus = iota + statusPending + statusConsentRequired + statusRejected +) + +type retrieveResult struct { + status retrieveStatus + token string + header string + consentURI string + consentNonce string +} + +// mapCredential maps the service's {header, token} tuple to an [auth.Credential]: +// an "Authorization: Bearer" header becomes a bearer credential; any other header +// name becomes a header-based API key. +func mapCredential(header, token string) (*auth.Credential, error) { + if header == "" || token == "" { + return nil, fmt.Errorf("gcp: credentials service returned an empty header or token") + } + name, hint, _ := strings.Cut(header, ":") + name = strings.TrimSpace(name) + if strings.EqualFold(name, "authorization") && + strings.HasPrefix(strings.ToLower(strings.TrimSpace(hint)), "bearer") { + return &auth.Credential{HTTP: &auth.HTTPCredential{Scheme: "bearer", Token: token}}, nil + } + // Non-bearer header: place the token in the named header. (adk-python also + // mirrors it into X-GOOG-API-KEY; deferred until a case needs it.) + return &auth.Credential{APIKey: &auth.APIKeyCredential{Name: name, Value: token}}, nil +} + +// doPost sends body as JSON to url and decodes a JSON response into out. +func (c *Client) doPost(ctx context.Context, url string, body, out any) error { + buf, err := json.Marshal(body) + if err != nil { + return fmt.Errorf("gcp: marshal request: %w", err) + } + httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(buf)) + if err != nil { + return fmt.Errorf("gcp: build request: %w", err) + } + httpReq.Header.Set("Content-Type", "application/json") + + resp, err := c.httpClient.Do(httpReq) + if err != nil { + return fmt.Errorf("gcp: call credentials service: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + data, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return fmt.Errorf("gcp: read response: %w", err) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return fmt.Errorf("gcp: credentials service returned status %d: %s", resp.StatusCode, strings.TrimSpace(string(data))) + } + if err := json.Unmarshal(data, out); err != nil { + return fmt.Errorf("gcp: decode response: %w", err) + } + return nil +} diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go new file mode 100644 index 000000000..2ec5e609a --- /dev/null +++ b/auth/gcp/client_test.go @@ -0,0 +1,304 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gcp + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "google.golang.org/adk/v2/auth" +) + +const ( + authProviderResource = "projects/p/locations/l/authProviders/ap" + connectorResource = "projects/p/locations/l/connectors/co" +) + +// newTestClient points both service endpoints at srv and uses a tiny backoff so +// polling tests are fast. +func newTestClient(t *testing.T, srv *httptest.Server) *Client { + t.Helper() + c, err := NewClient(context.Background(), + WithHTTPClient(srv.Client()), + WithAgentIdentityEndpoint(srv.URL), + WithConnectorEndpoint(srv.URL), + WithPollTimeout(2*time.Second), + ) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + c.initialBackoff = time.Millisecond + return c +} + +// sequenceServer replies with bodies in order, repeating the last one. +func sequenceServer(bodies ...string) (*httptest.Server, *int32) { + var n int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + i := int(atomic.AddInt32(&n, 1)) - 1 + if i >= len(bodies) { + i = len(bodies) - 1 + } + _, _ = io.WriteString(w, bodies[i]) + })) + return srv, &n +} + +func TestRetrieveAgentIdentityBearer(t *testing.T) { + srv, _ := sequenceServer(`{"success":{"token":"tok","header":"Authorization: Bearer"}}`) + defer srv.Close() + + cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: authProviderResource, UserID: "u"}) + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + if cred.HTTP == nil || cred.HTTP.Scheme != "bearer" || cred.HTTP.Token != "tok" { + t.Fatalf("credential = %+v, want bearer token %q", cred, "tok") + } +} + +func TestRetrieveAgentIdentityCustomHeader(t *testing.T) { + srv, _ := sequenceServer(`{"success":{"token":"KEY","header":"X-Goog-Api-Key"}}`) + defer srv.Close() + + cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: authProviderResource, UserID: "u"}) + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + if cred.APIKey == nil || cred.APIKey.Name != "X-Goog-Api-Key" || cred.APIKey.Value != "KEY" { + t.Fatalf("credential = %+v, want API key header", cred) + } +} + +func TestRetrieveAgentIdentityConsentRequired(t *testing.T) { + srv, _ := sequenceServer(`{"uriConsentRequired":{"authorizationUri":"https://consent","consentNonce":"n"}}`) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: authProviderResource, UserID: "u"}) + var consent *auth.ConsentRequiredError + if !errors.As(err, &consent) { + t.Fatalf("error = %v, want *auth.ConsentRequiredError", err) + } + if consent.AuthURI != "https://consent" || consent.Nonce != "n" { + t.Errorf("consent = %+v, want auth_uri/nonce set", consent) + } +} + +func TestRetrieveAgentIdentityConsentRejected(t *testing.T) { + srv, _ := sequenceServer(`{"consentRejected":{}}`) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: authProviderResource, UserID: "u"}) + if err == nil { + t.Fatal("RetrieveCredential() = nil error, want rejection error") + } + var consent *auth.ConsentRequiredError + if errors.As(err, &consent) { + t.Fatalf("error = %v, want a plain error (not ConsentRequiredError) for rejection", err) + } +} + +func TestRetrieveAgentIdentityPollsPending(t *testing.T) { + srv, calls := sequenceServer( + `{"pending":{}}`, + `{"success":{"token":"tok","header":"Authorization: Bearer"}}`, + ) + defer srv.Close() + + cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: authProviderResource, UserID: "u"}) + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + if cred.HTTP == nil || cred.HTTP.Token != "tok" { + t.Fatalf("credential = %+v, want bearer token", cred) + } + if got := atomic.LoadInt32(calls); got != 2 { + t.Errorf("service calls = %d, want 2 (pending then success)", got) + } +} + +func TestRetrieveConnectorBearer(t *testing.T) { + srv, _ := sequenceServer(`{"done":true,"response":{"@type":"x","token":"tok","header":"Authorization: Bearer"}}`) + defer srv.Close() + + cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: connectorResource, UserID: "u"}) + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + if cred.HTTP == nil || cred.HTTP.Token != "tok" { + t.Fatalf("credential = %+v, want bearer token", cred) + } +} + +func TestRetrieveConnectorPollsConsentPending(t *testing.T) { + srv, calls := sequenceServer( + `{"metadata":{"@type":"x","consentPending":{}}}`, + `{"done":true,"response":{"token":"tok","header":"Authorization: Bearer"}}`, + ) + defer srv.Close() + + cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: connectorResource, UserID: "u"}) + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + if cred.HTTP == nil || cred.HTTP.Token != "tok" { + t.Fatalf("credential = %+v, want bearer token", cred) + } + if got := atomic.LoadInt32(calls); got != 2 { + t.Errorf("service calls = %d, want 2 (pending then success)", got) + } +} + +func TestRetrieveConnectorConsentRequired(t *testing.T) { + srv, _ := sequenceServer(`{"metadata":{"uriConsentRequired":{"authorizationUri":"https://c","consentNonce":"n"}}}`) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: connectorResource, UserID: "u"}) + var consent *auth.ConsentRequiredError + if !errors.As(err, &consent) { + t.Fatalf("error = %v, want *auth.ConsentRequiredError", err) + } +} + +func TestRetrieveConnectorOperationError(t *testing.T) { + srv, _ := sequenceServer(`{"error":{"message":"boom"}}`) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: connectorResource, UserID: "u"}) + if err == nil || !strings.Contains(err.Error(), "boom") { + t.Fatalf("error = %v, want it to contain %q", err, "boom") + } +} + +func TestRetrieveRoutesByResource(t *testing.T) { + tests := []struct { + name string + resource string + wantPrefix string + }{ + {"connector", connectorResource, "/v1alpha/"}, + {"auth provider", authProviderResource, "/v1/"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var gotPath, gotMethod, gotUserID string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath, gotMethod = r.URL.Path, r.Method + var body struct { + UserID string `json:"userId"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotUserID = body.UserID + _, _ = io.WriteString(w, `{"done":true,"response":{"token":"t","header":"Authorization: Bearer"},"success":{"token":"t","header":"Authorization: Bearer"}}`) + })) + defer srv.Close() + + if _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: tc.resource, UserID: "user-1"}); err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + if gotMethod != http.MethodPost { + t.Errorf("method = %q, want POST", gotMethod) + } + if !strings.HasPrefix(gotPath, tc.wantPrefix) || !strings.Contains(gotPath, tc.resource) || !strings.HasSuffix(gotPath, "/credentials:retrieve") { + t.Errorf("path = %q, want prefix %q containing %q and suffix :retrieve", gotPath, tc.wantPrefix, tc.resource) + } + if gotUserID != "user-1" { + t.Errorf("body userId = %q, want %q", gotUserID, "user-1") + } + }) + } +} + +func TestRetrieveHTTPError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "nope", http.StatusInternalServerError) + })) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + Request{Resource: authProviderResource, UserID: "u"}) + if err == nil || !strings.Contains(err.Error(), "500") { + t.Fatalf("error = %v, want it to mention status 500", err) + } +} + +func TestRetrieveValidatesRequest(t *testing.T) { + c := &Client{httpClient: http.DefaultClient} + if _, err := c.RetrieveCredential(context.Background(), Request{UserID: "u"}); err == nil { + t.Error("missing Resource: got nil error, want error") + } + if _, err := c.RetrieveCredential(context.Background(), Request{Resource: authProviderResource}); err == nil { + t.Error("missing UserID: got nil error, want error") + } +} + +func TestMapCredential(t *testing.T) { + tests := []struct { + name string + header string + token string + wantBearer string // non-empty => expect bearer token + wantAPIKey [2]string + wantErr bool + }{ + {name: "authorization bearer", header: "Authorization: Bearer", token: "t", wantBearer: "t"}, + {name: "authorization bearer lowercase", header: "authorization: bearer", token: "t", wantBearer: "t"}, + {name: "custom header", header: "X-Goog-Api-Key", token: "k", wantAPIKey: [2]string{"X-Goog-Api-Key", "k"}}, + {name: "empty header", header: "", token: "t", wantErr: true}, + {name: "empty token", header: "Authorization: Bearer", token: "", wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cred, err := mapCredential(tc.header, tc.token) + if tc.wantErr { + if err == nil { + t.Fatal("mapCredential() = nil error, want error") + } + return + } + if err != nil { + t.Fatalf("mapCredential() error = %v", err) + } + switch { + case tc.wantBearer != "": + if cred.HTTP == nil || cred.HTTP.Token != tc.wantBearer { + t.Errorf("credential = %+v, want bearer %q", cred, tc.wantBearer) + } + default: + if cred.APIKey == nil || cred.APIKey.Name != tc.wantAPIKey[0] || cred.APIKey.Value != tc.wantAPIKey[1] { + t.Errorf("credential = %+v, want API key %v", cred, tc.wantAPIKey) + } + } + }) + } +} diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go new file mode 100644 index 000000000..77cf190bc --- /dev/null +++ b/auth/gcp/connector.go @@ -0,0 +1,82 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gcp + +import ( + "context" + "fmt" +) + +// connectorRequest is the JSON body for RetrieveCredentials (the connector is +// bound to the URL path, not the body). +type connectorRequest struct { + UserID string `json:"userId,omitempty"` + Scopes []string `json:"scopes,omitempty"` + ContinueURI string `json:"continueUri,omitempty"` + ForceRefresh bool `json:"forceRefresh,omitempty"` +} + +// connectorOperation is the google.longrunning.Operation wrapper the IAM +// Connector service returns. The service does not implement true LROs, so the +// terminal result is read inline from response/metadata. The Any-typed +// response/metadata carry an extra "@type" field that is ignored here. +type connectorOperation struct { + Done bool `json:"done"` + Response *struct { + Token string `json:"token"` + Header string `json:"header"` + } `json:"response"` + Metadata *struct { + ConsentPending *struct{} `json:"consentPending"` + UriConsentRequired *consentDetail `json:"uriConsentRequired"` + ConsentRejected *struct{} `json:"consentRejected"` + } `json:"metadata"` + Error *struct { + Message string `json:"message"` + } `json:"error"` +} + +// retrieveConnector calls the IAM Connector service and normalizes its +// Operation-wrapped response. +func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveResult, error) { + url := fmt.Sprintf("%s/v1alpha/%s/credentials:retrieve", c.connectorURL, req.Resource) + body := connectorRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} + + var op connectorOperation + if err := c.doPost(ctx, url, body, &op); err != nil { + return retrieveResult{}, err + } + + if op.Error != nil { + return retrieveResult{}, fmt.Errorf("gcp: connector operation failed: %s", op.Error.Message) + } + if op.Done && op.Response != nil { + return retrieveResult{status: statusOK, token: op.Response.Token, header: op.Response.Header}, nil + } + if md := op.Metadata; md != nil { + switch { + case md.UriConsentRequired != nil: + return retrieveResult{ + status: statusConsentRequired, + consentURI: md.UriConsentRequired.AuthorizationURI, + consentNonce: md.UriConsentRequired.ConsentNonce, + }, nil + case md.ConsentRejected != nil: + return retrieveResult{status: statusRejected}, nil + } + } + // No terminal result and no consent requirement: keep polling. + return retrieveResult{status: statusPending}, nil +} diff --git a/auth/gcp/doc.go b/auth/gcp/doc.go new file mode 100644 index 000000000..11aec2d70 --- /dev/null +++ b/auth/gcp/doc.go @@ -0,0 +1,30 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package gcp is a hand-rolled REST client for the Google Cloud Agent Identity +// and IAM Connector credential services. Given a resource name it retrieves an +// end-user credential and maps it to an [auth.Credential], polling while the +// service reports a non-interactive "pending" state and surfacing interactive +// consent as an [auth.ConsentRequiredError]. +// +// No generated Go client libraries exist for these (preview) services and their +// surface is a single RPC, so the client is hand-rolled over net/http to keep +// dependencies light. Calls to the credential services are authenticated with +// Application Default Credentials (cloud-platform scope) unless a custom +// *http.Client is supplied. +// +// This package holds only the transport-level client. The [auth.CredentialProvider] +// that resolves the acting user from the invocation context is a separate, +// higher layer. +package gcp From b0b3b5131f17cddc2ff6dff70928a41cfdf764d8 Mon Sep 17 00:00:00 2001 From: wolo Date: Mon, 13 Jul 2026 12:10:44 +0000 Subject: [PATCH 02/15] fix(auth/gcp): apply review feedback - Add ErrConsentRejected / ErrPollTimeout sentinels (errors.Is-able) and wrap the rejection/timeout returns with them. - Add a context-cancellation-during-poll test (no hang; surfaces context.Canceled). - Move test helpers below the tests; use t.Context() in tests. - Fix the URIConsentRequired initialism. - Note the X-GOOG-API-KEY mirror as a follow-up TODO (needs an additive AdditionalHeaders field on auth.APIKeyCredential; non-breaking). --- auth/gcp/agentidentity.go | 8 +-- auth/gcp/client.go | 19 +++++-- auth/gcp/client_test.go | 110 ++++++++++++++++++++++---------------- auth/gcp/connector.go | 8 +-- 4 files changed, 87 insertions(+), 58 deletions(-) diff --git a/auth/gcp/agentidentity.go b/auth/gcp/agentidentity.go index 5a8d689ad..e5e6232a0 100644 --- a/auth/gcp/agentidentity.go +++ b/auth/gcp/agentidentity.go @@ -34,7 +34,7 @@ type agentIdentityResponse struct { Header string `json:"header"` } `json:"success"` Pending *struct{} `json:"pending"` - UriConsentRequired *consentDetail `json:"uriConsentRequired"` + URIConsentRequired *consentDetail `json:"uriConsentRequired"` ConsentRejected *struct{} `json:"consentRejected"` } @@ -58,11 +58,11 @@ func (c *Client) retrieveAgentIdentity(ctx context.Context, req Request) (retrie switch { case out.Success != nil: return retrieveResult{status: statusOK, token: out.Success.Token, header: out.Success.Header}, nil - case out.UriConsentRequired != nil: + case out.URIConsentRequired != nil: return retrieveResult{ status: statusConsentRequired, - consentURI: out.UriConsentRequired.AuthorizationURI, - consentNonce: out.UriConsentRequired.ConsentNonce, + consentURI: out.URIConsentRequired.AuthorizationURI, + consentNonce: out.URIConsentRequired.ConsentNonce, }, nil case out.ConsentRejected != nil: return retrieveResult{status: statusRejected}, nil diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 949bc1ce2..72228e9e3 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -18,6 +18,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -45,6 +46,15 @@ const ( // routed to the Agent Identity service (same split as adk-python). var connectorResourceRE = regexp.MustCompile(`^projects/[^/]+/locations/[^/]+/connectors/[^/]+$`) +// Sentinel errors from [Client.RetrieveCredential]; callers test with errors.Is. +var ( + // ErrConsentRejected means the end user rejected the consent request. + ErrConsentRejected = errors.New("gcp: user consent rejected") + // ErrPollTimeout means polling exceeded the poll timeout while the credential + // was still pending. + ErrPollTimeout = errors.New("gcp: timed out waiting for credentials") +) + // Client retrieves end-user credentials from the Agent Identity / IAM Connector // credential services and maps them to [auth.Credential]. type Client struct { @@ -142,11 +152,11 @@ func (c *Client) RetrieveCredential(ctx context.Context, req Request) (*auth.Cre case statusConsentRequired: return nil, &auth.ConsentRequiredError{AuthURI: res.consentURI, Nonce: res.consentNonce} case statusRejected: - return nil, fmt.Errorf("gcp: user consent rejected for %q", req.Resource) + return nil, fmt.Errorf("%w for %q", ErrConsentRejected, req.Resource) case statusPending: remaining := time.Until(deadline) if remaining <= 0 { - return nil, fmt.Errorf("gcp: timed out waiting for credentials for %q", req.Resource) + return nil, fmt.Errorf("%w for %q", ErrPollTimeout, req.Resource) } wait := min(backoff, remaining) select { @@ -190,8 +200,9 @@ func mapCredential(header, token string) (*auth.Credential, error) { strings.HasPrefix(strings.ToLower(strings.TrimSpace(hint)), "bearer") { return &auth.Credential{HTTP: &auth.HTTPCredential{Scheme: "bearer", Token: token}}, nil } - // Non-bearer header: place the token in the named header. (adk-python also - // mirrors it into X-GOOG-API-KEY; deferred until a case needs it.) + // Non-bearer header -> header-based API key. + // TODO: for full adk-python parity also mirror the token into X-GOOG-API-KEY; + // needs an AdditionalHeaders field on auth.APIKeyCredential (additive, non-breaking). return &auth.Credential{APIKey: &auth.APIKeyCredential{Name: name, Value: token}}, nil } diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 2ec5e609a..9a7b5e632 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -34,41 +34,11 @@ const ( connectorResource = "projects/p/locations/l/connectors/co" ) -// newTestClient points both service endpoints at srv and uses a tiny backoff so -// polling tests are fast. -func newTestClient(t *testing.T, srv *httptest.Server) *Client { - t.Helper() - c, err := NewClient(context.Background(), - WithHTTPClient(srv.Client()), - WithAgentIdentityEndpoint(srv.URL), - WithConnectorEndpoint(srv.URL), - WithPollTimeout(2*time.Second), - ) - if err != nil { - t.Fatalf("NewClient() error = %v", err) - } - c.initialBackoff = time.Millisecond - return c -} - -// sequenceServer replies with bodies in order, repeating the last one. -func sequenceServer(bodies ...string) (*httptest.Server, *int32) { - var n int32 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - i := int(atomic.AddInt32(&n, 1)) - 1 - if i >= len(bodies) { - i = len(bodies) - 1 - } - _, _ = io.WriteString(w, bodies[i]) - })) - return srv, &n -} - func TestRetrieveAgentIdentityBearer(t *testing.T) { srv, _ := sequenceServer(`{"success":{"token":"tok","header":"Authorization: Bearer"}}`) defer srv.Close() - cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) @@ -82,7 +52,7 @@ func TestRetrieveAgentIdentityCustomHeader(t *testing.T) { srv, _ := sequenceServer(`{"success":{"token":"KEY","header":"X-Goog-Api-Key"}}`) defer srv.Close() - cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) @@ -96,7 +66,7 @@ func TestRetrieveAgentIdentityConsentRequired(t *testing.T) { srv, _ := sequenceServer(`{"uriConsentRequired":{"authorizationUri":"https://consent","consentNonce":"n"}}`) defer srv.Close() - _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) var consent *auth.ConsentRequiredError if !errors.As(err, &consent) { @@ -111,14 +81,14 @@ func TestRetrieveAgentIdentityConsentRejected(t *testing.T) { srv, _ := sequenceServer(`{"consentRejected":{}}`) defer srv.Close() - _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) - if err == nil { - t.Fatal("RetrieveCredential() = nil error, want rejection error") + if !errors.Is(err, ErrConsentRejected) { + t.Fatalf("error = %v, want ErrConsentRejected", err) } var consent *auth.ConsentRequiredError if errors.As(err, &consent) { - t.Fatalf("error = %v, want a plain error (not ConsentRequiredError) for rejection", err) + t.Fatalf("error = %v, want a plain rejection (not ConsentRequiredError)", err) } } @@ -129,7 +99,7 @@ func TestRetrieveAgentIdentityPollsPending(t *testing.T) { ) defer srv.Close() - cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) @@ -146,7 +116,7 @@ func TestRetrieveConnectorBearer(t *testing.T) { srv, _ := sequenceServer(`{"done":true,"response":{"@type":"x","token":"tok","header":"Authorization: Bearer"}}`) defer srv.Close() - cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: connectorResource, UserID: "u"}) if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) @@ -163,7 +133,7 @@ func TestRetrieveConnectorPollsConsentPending(t *testing.T) { ) defer srv.Close() - cred, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: connectorResource, UserID: "u"}) if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) @@ -180,7 +150,7 @@ func TestRetrieveConnectorConsentRequired(t *testing.T) { srv, _ := sequenceServer(`{"metadata":{"uriConsentRequired":{"authorizationUri":"https://c","consentNonce":"n"}}}`) defer srv.Close() - _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: connectorResource, UserID: "u"}) var consent *auth.ConsentRequiredError if !errors.As(err, &consent) { @@ -192,7 +162,7 @@ func TestRetrieveConnectorOperationError(t *testing.T) { srv, _ := sequenceServer(`{"error":{"message":"boom"}}`) defer srv.Close() - _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: connectorResource, UserID: "u"}) if err == nil || !strings.Contains(err.Error(), "boom") { t.Fatalf("error = %v, want it to contain %q", err, "boom") @@ -222,7 +192,7 @@ func TestRetrieveRoutesByResource(t *testing.T) { })) defer srv.Close() - if _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + if _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: tc.resource, UserID: "user-1"}); err != nil { t.Fatalf("RetrieveCredential() error = %v", err) } @@ -245,7 +215,7 @@ func TestRetrieveHTTPError(t *testing.T) { })) defer srv.Close() - _, err := newTestClient(t, srv).RetrieveCredential(context.Background(), + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) if err == nil || !strings.Contains(err.Error(), "500") { t.Fatalf("error = %v, want it to mention status 500", err) @@ -254,10 +224,10 @@ func TestRetrieveHTTPError(t *testing.T) { func TestRetrieveValidatesRequest(t *testing.T) { c := &Client{httpClient: http.DefaultClient} - if _, err := c.RetrieveCredential(context.Background(), Request{UserID: "u"}); err == nil { + if _, err := c.RetrieveCredential(t.Context(), Request{UserID: "u"}); err == nil { t.Error("missing Resource: got nil error, want error") } - if _, err := c.RetrieveCredential(context.Background(), Request{Resource: authProviderResource}); err == nil { + if _, err := c.RetrieveCredential(t.Context(), Request{Resource: authProviderResource}); err == nil { t.Error("missing UserID: got nil error, want error") } } @@ -302,3 +272,51 @@ func TestMapCredential(t *testing.T) { }) } } + +// TestRetrieveContextCanceledWhilePending verifies that canceling the context +// aborts a pending poll promptly (no hang) and surfaces context.Canceled. +func TestRetrieveContextCanceledWhilePending(t *testing.T) { + srv, _ := sequenceServer(`{"pending":{}}`) // never resolves + defer srv.Close() + + c := newTestClient(t, srv) + c.initialBackoff = 50 * time.Millisecond // park in the poll wait, then cancel + + ctx, cancel := context.WithCancel(t.Context()) + time.AfterFunc(10*time.Millisecond, cancel) + + _, err := c.RetrieveCredential(ctx, Request{Resource: authProviderResource, UserID: "u"}) + if !errors.Is(err, context.Canceled) { + t.Fatalf("RetrieveCredential() error = %v, want context.Canceled", err) + } +} + +// newTestClient points both service endpoints at srv and uses a tiny backoff so +// polling tests are fast. +func newTestClient(t *testing.T, srv *httptest.Server) *Client { + t.Helper() + c, err := NewClient(t.Context(), + WithHTTPClient(srv.Client()), + WithAgentIdentityEndpoint(srv.URL), + WithConnectorEndpoint(srv.URL), + WithPollTimeout(2*time.Second), + ) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + c.initialBackoff = time.Millisecond + return c +} + +// sequenceServer replies with bodies in order, repeating the last one. +func sequenceServer(bodies ...string) (*httptest.Server, *int32) { + var n int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + i := int(atomic.AddInt32(&n, 1)) - 1 + if i >= len(bodies) { + i = len(bodies) - 1 + } + _, _ = io.WriteString(w, bodies[i]) + })) + return srv, &n +} diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index 77cf190bc..795f83abc 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -40,7 +40,7 @@ type connectorOperation struct { } `json:"response"` Metadata *struct { ConsentPending *struct{} `json:"consentPending"` - UriConsentRequired *consentDetail `json:"uriConsentRequired"` + URIConsentRequired *consentDetail `json:"uriConsentRequired"` ConsentRejected *struct{} `json:"consentRejected"` } `json:"metadata"` Error *struct { @@ -67,11 +67,11 @@ func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveRe } if md := op.Metadata; md != nil { switch { - case md.UriConsentRequired != nil: + case md.URIConsentRequired != nil: return retrieveResult{ status: statusConsentRequired, - consentURI: md.UriConsentRequired.AuthorizationURI, - consentNonce: md.UriConsentRequired.ConsentNonce, + consentURI: md.URIConsentRequired.AuthorizationURI, + consentNonce: md.URIConsentRequired.ConsentNonce, }, nil case md.ConsentRejected != nil: return retrieveResult{status: statusRejected}, nil From d13f4a080d312c752c389ca6c9a3e90719f2c5ab Mon Sep 17 00:00:00 2001 From: wolo Date: Thu, 16 Jul 2026 11:57:08 +0000 Subject: [PATCH 03/15] fix(auth/gcp): port to auth.Credential interface and harden connector The auth core package redesigned Credential from a struct into an interface (BearerCredential/APIKeyCredential/OAuth2Credential), so the client no longer compiled against its base. Port the mapping accordingly, and apply the remaining review feedback on the connector. - mapCredential returns auth.Credential: auth.BearerCredential for an "Authorization: Bearer" header, auth.APIKeyCredential otherwise. RetrieveCredential's return type changes from *auth.Credential to auth.Credential. - Connector: a done operation carrying no credential now returns an error instead of being treated as pending and polled to the timeout; drop the unused consentPending metadata field. - Test ErrPollTimeout and the connector done-without-credential path. --- auth/gcp/client.go | 12 +++--- auth/gcp/client_test.go | 85 +++++++++++++++++++++++++++++++---------- auth/gcp/connector.go | 8 +++- 3 files changed, 76 insertions(+), 29 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 72228e9e3..92ef89b68 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -126,7 +126,7 @@ type Request struct { // RetrieveCredential retrieves a credential for req, polling while the service // reports a non-interactive pending state (up to the configured poll timeout). // If interactive consent is required it returns an [auth.ConsentRequiredError]. -func (c *Client) RetrieveCredential(ctx context.Context, req Request) (*auth.Credential, error) { +func (c *Client) RetrieveCredential(ctx context.Context, req Request) (auth.Credential, error) { if req.Resource == "" { return nil, fmt.Errorf("gcp: RetrieveCredential requires a Resource") } @@ -190,7 +190,7 @@ type retrieveResult struct { // mapCredential maps the service's {header, token} tuple to an [auth.Credential]: // an "Authorization: Bearer" header becomes a bearer credential; any other header // name becomes a header-based API key. -func mapCredential(header, token string) (*auth.Credential, error) { +func mapCredential(header, token string) (auth.Credential, error) { if header == "" || token == "" { return nil, fmt.Errorf("gcp: credentials service returned an empty header or token") } @@ -198,12 +198,12 @@ func mapCredential(header, token string) (*auth.Credential, error) { name = strings.TrimSpace(name) if strings.EqualFold(name, "authorization") && strings.HasPrefix(strings.ToLower(strings.TrimSpace(hint)), "bearer") { - return &auth.Credential{HTTP: &auth.HTTPCredential{Scheme: "bearer", Token: token}}, nil + return auth.BearerCredential{Token: token}, nil } // Non-bearer header -> header-based API key. - // TODO: for full adk-python parity also mirror the token into X-GOOG-API-KEY; - // needs an AdditionalHeaders field on auth.APIKeyCredential (additive, non-breaking). - return &auth.Credential{APIKey: &auth.APIKeyCredential{Name: name, Value: token}}, nil + // TODO: for full adk-python parity also mirror the token into X-GOOG-API-KEY + // (via auth.WithHeaders) as a follow-up. + return auth.APIKeyCredential{Name: name, Value: token}, nil } // doPost sends body as JSON to url and decodes a JSON response into out. diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 9a7b5e632..cdaafbcd6 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -43,9 +43,7 @@ func TestRetrieveAgentIdentityBearer(t *testing.T) { if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) } - if cred.HTTP == nil || cred.HTTP.Scheme != "bearer" || cred.HTTP.Token != "tok" { - t.Fatalf("credential = %+v, want bearer token %q", cred, "tok") - } + wantBearer(t, cred, "tok") } func TestRetrieveAgentIdentityCustomHeader(t *testing.T) { @@ -57,9 +55,7 @@ func TestRetrieveAgentIdentityCustomHeader(t *testing.T) { if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) } - if cred.APIKey == nil || cred.APIKey.Name != "X-Goog-Api-Key" || cred.APIKey.Value != "KEY" { - t.Fatalf("credential = %+v, want API key header", cred) - } + wantAPIKey(t, cred, "X-Goog-Api-Key", "KEY") } func TestRetrieveAgentIdentityConsentRequired(t *testing.T) { @@ -104,9 +100,7 @@ func TestRetrieveAgentIdentityPollsPending(t *testing.T) { if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) } - if cred.HTTP == nil || cred.HTTP.Token != "tok" { - t.Fatalf("credential = %+v, want bearer token", cred) - } + wantBearer(t, cred, "tok") if got := atomic.LoadInt32(calls); got != 2 { t.Errorf("service calls = %d, want 2 (pending then success)", got) } @@ -121,9 +115,7 @@ func TestRetrieveConnectorBearer(t *testing.T) { if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) } - if cred.HTTP == nil || cred.HTTP.Token != "tok" { - t.Fatalf("credential = %+v, want bearer token", cred) - } + wantBearer(t, cred, "tok") } func TestRetrieveConnectorPollsConsentPending(t *testing.T) { @@ -138,9 +130,7 @@ func TestRetrieveConnectorPollsConsentPending(t *testing.T) { if err != nil { t.Fatalf("RetrieveCredential() error = %v", err) } - if cred.HTTP == nil || cred.HTTP.Token != "tok" { - t.Fatalf("credential = %+v, want bearer token", cred) - } + wantBearer(t, cred, "tok") if got := atomic.LoadInt32(calls); got != 2 { t.Errorf("service calls = %d, want 2 (pending then success)", got) } @@ -261,13 +251,9 @@ func TestMapCredential(t *testing.T) { } switch { case tc.wantBearer != "": - if cred.HTTP == nil || cred.HTTP.Token != tc.wantBearer { - t.Errorf("credential = %+v, want bearer %q", cred, tc.wantBearer) - } + wantBearer(t, cred, tc.wantBearer) default: - if cred.APIKey == nil || cred.APIKey.Name != tc.wantAPIKey[0] || cred.APIKey.Value != tc.wantAPIKey[1] { - t.Errorf("credential = %+v, want API key %v", cred, tc.wantAPIKey) - } + wantAPIKey(t, cred, tc.wantAPIKey[0], tc.wantAPIKey[1]) } }) } @@ -291,6 +277,39 @@ func TestRetrieveContextCanceledWhilePending(t *testing.T) { } } +// TestRetrievePollTimeout verifies that a service stuck in the non-interactive +// pending state past the poll timeout surfaces ErrPollTimeout (no hang). +func TestRetrievePollTimeout(t *testing.T) { + srv, _ := sequenceServer(`{"pending":{}}`) // never resolves + defer srv.Close() + + c := newTestClient(t, srv) + c.pollTimeout = 30 * time.Millisecond + + _, err := c.RetrieveCredential(t.Context(), + Request{Resource: authProviderResource, UserID: "u"}) + if !errors.Is(err, ErrPollTimeout) { + t.Fatalf("RetrieveCredential() error = %v, want ErrPollTimeout", err) + } +} + +// TestRetrieveConnectorDoneWithoutResponse verifies that a terminal (done) +// connector operation carrying no credential fails fast with an error, rather +// than being treated as pending and polled until the timeout. +func TestRetrieveConnectorDoneWithoutResponse(t *testing.T) { + srv, _ := sequenceServer(`{"done":true}`) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + Request{Resource: connectorResource, UserID: "u"}) + if err == nil || !strings.Contains(err.Error(), "no credential") { + t.Fatalf("error = %v, want it to mention %q", err, "no credential") + } + if errors.Is(err, ErrPollTimeout) { + t.Fatalf("error = %v, want a done-without-credential error, not a poll timeout", err) + } +} + // newTestClient points both service endpoints at srv and uses a tiny backoff so // polling tests are fast. func newTestClient(t *testing.T, srv *httptest.Server) *Client { @@ -320,3 +339,27 @@ func sequenceServer(bodies ...string) (*httptest.Server, *int32) { })) return srv, &n } + +// wantBearer fails t unless cred is an auth.BearerCredential carrying token. +func wantBearer(t *testing.T, cred auth.Credential, token string) { + t.Helper() + b, ok := cred.(auth.BearerCredential) + if !ok { + t.Fatalf("credential = %#v, want auth.BearerCredential", cred) + } + if b.Token != token { + t.Fatalf("bearer token = %q, want %q", b.Token, token) + } +} + +// wantAPIKey fails t unless cred is an auth.APIKeyCredential with name and value. +func wantAPIKey(t *testing.T, cred auth.Credential, name, value string) { + t.Helper() + k, ok := cred.(auth.APIKeyCredential) + if !ok { + t.Fatalf("credential = %#v, want auth.APIKeyCredential", cred) + } + if k.Name != name || k.Value != value { + t.Fatalf("api key = {name:%q value:%q}, want {name:%q value:%q}", k.Name, k.Value, name, value) + } +} diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index 795f83abc..ba8d048b6 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -39,7 +39,6 @@ type connectorOperation struct { Header string `json:"header"` } `json:"response"` Metadata *struct { - ConsentPending *struct{} `json:"consentPending"` URIConsentRequired *consentDetail `json:"uriConsentRequired"` ConsentRejected *struct{} `json:"consentRejected"` } `json:"metadata"` @@ -62,7 +61,12 @@ func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveRe if op.Error != nil { return retrieveResult{}, fmt.Errorf("gcp: connector operation failed: %s", op.Error.Message) } - if op.Done && op.Response != nil { + if op.Done { + // A terminal operation must carry a credential; treat an empty result as + // an error rather than polling to the timeout. + if op.Response == nil { + return retrieveResult{}, fmt.Errorf("gcp: connector operation done but returned no credential for %q", req.Resource) + } return retrieveResult{status: statusOK, token: op.Response.Token, header: op.Response.Header}, nil } if md := op.Metadata; md != nil { From d049dc34b302b06fd38688cfc61cc571eb7a4ee9 Mon Sep 17 00:00:00 2001 From: wolo Date: Fri, 17 Jul 2026 10:11:35 +0000 Subject: [PATCH 04/15] fix(auth/gcp): mirror X-Goog-Api-Key and model connector consent states Custom (non-bearer) headers now also set X-Goog-Api-Key alongside the service's own header, matching adk-python's credential mapping. Model the IAM Connector metadata consent_pending status explicitly (per the v1alpha RetrieveCredentialsMetadata status oneof) instead of relying on the unknown-status fall-through, and add a test for the connector consent_rejected path (a real proto field that adk-python's connector provider omits). --- auth/gcp/client.go | 9 +++++---- auth/gcp/client_test.go | 27 +++++++++++++++++++++------ auth/gcp/connector.go | 7 ++++++- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 92ef89b68..76e3091c1 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -200,10 +200,11 @@ func mapCredential(header, token string) (auth.Credential, error) { strings.HasPrefix(strings.ToLower(strings.TrimSpace(hint)), "bearer") { return auth.BearerCredential{Token: token}, nil } - // Non-bearer header -> header-based API key. - // TODO: for full adk-python parity also mirror the token into X-GOOG-API-KEY - // (via auth.WithHeaders) as a follow-up. - return auth.APIKeyCredential{Name: name, Value: token}, nil + // Non-bearer header -> header-based API key. adk-python also mirrors the + // token into X-Goog-Api-Key for custom headers (alongside the service's own + // header), so match that behavior. + key := auth.APIKeyCredential{Name: name, Value: token} + return auth.WithHeaders(key, map[string]string{"X-Goog-Api-Key": token}), nil } // doPost sends body as JSON to url and decodes a JSON response into out. diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index cdaafbcd6..fca155243 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -148,6 +148,17 @@ func TestRetrieveConnectorConsentRequired(t *testing.T) { } } +func TestRetrieveConnectorConsentRejected(t *testing.T) { + srv, _ := sequenceServer(`{"metadata":{"consentRejected":{}}}`) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + Request{Resource: connectorResource, UserID: "u"}) + if !errors.Is(err, ErrConsentRejected) { + t.Fatalf("error = %v, want ErrConsentRejected", err) + } +} + func TestRetrieveConnectorOperationError(t *testing.T) { srv, _ := sequenceServer(`{"error":{"message":"boom"}}`) defer srv.Close() @@ -352,14 +363,18 @@ func wantBearer(t *testing.T, cred auth.Credential, token string) { } } -// wantAPIKey fails t unless cred is an auth.APIKeyCredential with name and value. +// wantAPIKey fails t unless applying cred sets the named header and the +// X-Goog-Api-Key mirror (adk-python parity) to value. func wantAPIKey(t *testing.T, cred auth.Credential, name, value string) { t.Helper() - k, ok := cred.(auth.APIKeyCredential) - if !ok { - t.Fatalf("credential = %#v, want auth.APIKeyCredential", cred) + h := http.Header{} + if err := cred.Apply(h); err != nil { + t.Fatalf("cred.Apply() error = %v", err) + } + if got := h.Get(name); got != value { + t.Errorf("header %q = %q, want %q", name, got, value) } - if k.Name != name || k.Value != value { - t.Fatalf("api key = {name:%q value:%q}, want {name:%q value:%q}", k.Name, k.Value, name, value) + if got := h.Get("X-Goog-Api-Key"); got != value { + t.Errorf("X-Goog-Api-Key = %q, want %q (adk-python parity)", got, value) } } diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index ba8d048b6..4f0fed89c 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -39,6 +39,7 @@ type connectorOperation struct { Header string `json:"header"` } `json:"response"` Metadata *struct { + ConsentPending *struct{} `json:"consentPending"` URIConsentRequired *consentDetail `json:"uriConsentRequired"` ConsentRejected *struct{} `json:"consentRejected"` } `json:"metadata"` @@ -79,8 +80,12 @@ func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveRe }, nil case md.ConsentRejected != nil: return retrieveResult{status: statusRejected}, nil + case md.ConsentPending != nil: + return retrieveResult{status: statusPending}, nil } } - // No terminal result and no consent requirement: keep polling. + // The metadata status oneof is consent_pending, uri_consent_required, or + // consent_rejected; treat an absent/unknown status as pending and keep + // polling (consent_pending means "no action required, just retry"). return retrieveResult{status: statusPending}, nil } From 79bb81da0329859622880a40aa0209c18199fbaa Mon Sep 17 00:00:00 2001 From: wolo Date: Fri, 17 Jul 2026 10:23:57 +0000 Subject: [PATCH 05/15] refactor(auth/gcp): contain result oneof behind result() methods The wire responses emulated the services' result oneof with nullable-pointer structs and mapped them to a retrieveResult inline in each retrieve* method. Move that mapping into result() methods on agentIdentityResponse and connectorOperation, so transport (doPost) is separated from interpretation (now a pure, unit-testable step), and extract the duplicated {token, header} success shape into a shared credentialPayload type. No behavior change. --- auth/gcp/agentidentity.go | 50 ++++++++++++++++++++------------------- auth/gcp/client.go | 8 +++++++ auth/gcp/connector.go | 47 ++++++++++++++++++------------------ 3 files changed, 58 insertions(+), 47 deletions(-) diff --git a/auth/gcp/agentidentity.go b/auth/gcp/agentidentity.go index e5e6232a0..be20367e0 100644 --- a/auth/gcp/agentidentity.go +++ b/auth/gcp/agentidentity.go @@ -29,13 +29,10 @@ type agentIdentityRequest struct { // agentIdentityResponse mirrors the RetrieveCredentialsResponse "result" oneof. type agentIdentityResponse struct { - Success *struct { - Token string `json:"token"` - Header string `json:"header"` - } `json:"success"` - Pending *struct{} `json:"pending"` - URIConsentRequired *consentDetail `json:"uriConsentRequired"` - ConsentRejected *struct{} `json:"consentRejected"` + Success *credentialPayload `json:"success"` + Pending *struct{} `json:"pending"` + URIConsentRequired *consentDetail `json:"uriConsentRequired"` + ConsentRejected *struct{} `json:"consentRejected"` } // consentDetail is the shared uri-consent payload across both services. @@ -44,6 +41,27 @@ type consentDetail struct { ConsentNonce string `json:"consentNonce"` } +// result collapses the response's "result" oneof into a retrieveResult, erroring +// if the service returned no recognized arm. +func (r agentIdentityResponse) result(resource string) (retrieveResult, error) { + switch { + case r.Success != nil: + return retrieveResult{status: statusOK, token: r.Success.Token, header: r.Success.Header}, nil + case r.URIConsentRequired != nil: + return retrieveResult{ + status: statusConsentRequired, + consentURI: r.URIConsentRequired.AuthorizationURI, + consentNonce: r.URIConsentRequired.ConsentNonce, + }, nil + case r.ConsentRejected != nil: + return retrieveResult{status: statusRejected}, nil + case r.Pending != nil: + return retrieveResult{status: statusPending}, nil + default: + return retrieveResult{}, fmt.Errorf("gcp: agent identity returned an empty result for %q", resource) + } +} + // retrieveAgentIdentity calls the Agent Identity service, whose response is // returned synchronously (no long-running-operation wrapper). func (c *Client) retrieveAgentIdentity(ctx context.Context, req Request) (retrieveResult, error) { @@ -54,21 +72,5 @@ func (c *Client) retrieveAgentIdentity(ctx context.Context, req Request) (retrie if err := c.doPost(ctx, url, body, &out); err != nil { return retrieveResult{}, err } - - switch { - case out.Success != nil: - return retrieveResult{status: statusOK, token: out.Success.Token, header: out.Success.Header}, nil - case out.URIConsentRequired != nil: - return retrieveResult{ - status: statusConsentRequired, - consentURI: out.URIConsentRequired.AuthorizationURI, - consentNonce: out.URIConsentRequired.ConsentNonce, - }, nil - case out.ConsentRejected != nil: - return retrieveResult{status: statusRejected}, nil - case out.Pending != nil: - return retrieveResult{status: statusPending}, nil - default: - return retrieveResult{}, fmt.Errorf("gcp: agent identity returned an empty result for %q", req.Resource) - } + return out.result(req.Resource) } diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 76e3091c1..03048a388 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -187,6 +187,14 @@ type retrieveResult struct { consentNonce string } +// credentialPayload is the {header, token} success shape returned by both +// services (nested under "success" for Agent Identity, under the operation +// "response" for the IAM Connector). +type credentialPayload struct { + Token string `json:"token"` + Header string `json:"header"` +} + // mapCredential maps the service's {header, token} tuple to an [auth.Credential]: // an "Authorization: Bearer" header becomes a bearer credential; any other header // name becomes a header-based API key. diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index 4f0fed89c..370658c7a 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -33,11 +33,8 @@ type connectorRequest struct { // terminal result is read inline from response/metadata. The Any-typed // response/metadata carry an extra "@type" field that is ignored here. type connectorOperation struct { - Done bool `json:"done"` - Response *struct { - Token string `json:"token"` - Header string `json:"header"` - } `json:"response"` + Done bool `json:"done"` + Response *credentialPayload `json:"response"` Metadata *struct { ConsentPending *struct{} `json:"consentPending"` URIConsentRequired *consentDetail `json:"uriConsentRequired"` @@ -48,29 +45,20 @@ type connectorOperation struct { } `json:"error"` } -// retrieveConnector calls the IAM Connector service and normalizes its -// Operation-wrapped response. -func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveResult, error) { - url := fmt.Sprintf("%s/v1alpha/%s/credentials:retrieve", c.connectorURL, req.Resource) - body := connectorRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} - - var op connectorOperation - if err := c.doPost(ctx, url, body, &op); err != nil { - return retrieveResult{}, err +// result collapses the Operation-wrapped response into a retrieveResult. +func (o connectorOperation) result(resource string) (retrieveResult, error) { + if o.Error != nil { + return retrieveResult{}, fmt.Errorf("gcp: connector operation failed: %s", o.Error.Message) } - - if op.Error != nil { - return retrieveResult{}, fmt.Errorf("gcp: connector operation failed: %s", op.Error.Message) - } - if op.Done { + if o.Done { // A terminal operation must carry a credential; treat an empty result as // an error rather than polling to the timeout. - if op.Response == nil { - return retrieveResult{}, fmt.Errorf("gcp: connector operation done but returned no credential for %q", req.Resource) + if o.Response == nil { + return retrieveResult{}, fmt.Errorf("gcp: connector operation done but returned no credential for %q", resource) } - return retrieveResult{status: statusOK, token: op.Response.Token, header: op.Response.Header}, nil + return retrieveResult{status: statusOK, token: o.Response.Token, header: o.Response.Header}, nil } - if md := op.Metadata; md != nil { + if md := o.Metadata; md != nil { switch { case md.URIConsentRequired != nil: return retrieveResult{ @@ -89,3 +77,16 @@ func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveRe // polling (consent_pending means "no action required, just retry"). return retrieveResult{status: statusPending}, nil } + +// retrieveConnector calls the IAM Connector service and normalizes its +// Operation-wrapped response. +func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveResult, error) { + url := fmt.Sprintf("%s/v1alpha/%s/credentials:retrieve", c.connectorURL, req.Resource) + body := connectorRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} + + var op connectorOperation + if err := c.doPost(ctx, url, body, &op); err != nil { + return retrieveResult{}, err + } + return op.result(req.Resource) +} From 4bfccedda3e832ac397d255c1d95a98e8e4bbebd Mon Sep 17 00:00:00 2001 From: wolo Date: Fri, 17 Jul 2026 10:33:44 +0000 Subject: [PATCH 06/15] refactor(auth/gcp): model the retrieval outcome as a sealed interface Replace the retrieveResult struct + retrieveStatus enum (a fat struct whose valid fields depended on the status) with a sealed outcome sum type (credOutcome / pendingOutcome / consentOutcome / rejectedOutcome), so each arm carries only its own fields and RetrieveCredential type-switches on it. The per-service result() methods now return outcome. No behavior change. --- auth/gcp/agentidentity.go | 24 ++++++++---------- auth/gcp/client.go | 52 ++++++++++++++++++++++----------------- auth/gcp/connector.go | 26 +++++++++----------- 3 files changed, 51 insertions(+), 51 deletions(-) diff --git a/auth/gcp/agentidentity.go b/auth/gcp/agentidentity.go index be20367e0..5ec60907e 100644 --- a/auth/gcp/agentidentity.go +++ b/auth/gcp/agentidentity.go @@ -41,36 +41,32 @@ type consentDetail struct { ConsentNonce string `json:"consentNonce"` } -// result collapses the response's "result" oneof into a retrieveResult, erroring -// if the service returned no recognized arm. -func (r agentIdentityResponse) result(resource string) (retrieveResult, error) { +// result collapses the response's "result" oneof into an outcome, erroring if +// the service returned no recognized arm. +func (r agentIdentityResponse) result(resource string) (outcome, error) { switch { case r.Success != nil: - return retrieveResult{status: statusOK, token: r.Success.Token, header: r.Success.Header}, nil + return credOutcome{header: r.Success.Header, token: r.Success.Token}, nil case r.URIConsentRequired != nil: - return retrieveResult{ - status: statusConsentRequired, - consentURI: r.URIConsentRequired.AuthorizationURI, - consentNonce: r.URIConsentRequired.ConsentNonce, - }, nil + return consentOutcome{authURI: r.URIConsentRequired.AuthorizationURI, nonce: r.URIConsentRequired.ConsentNonce}, nil case r.ConsentRejected != nil: - return retrieveResult{status: statusRejected}, nil + return rejectedOutcome{}, nil case r.Pending != nil: - return retrieveResult{status: statusPending}, nil + return pendingOutcome{}, nil default: - return retrieveResult{}, fmt.Errorf("gcp: agent identity returned an empty result for %q", resource) + return nil, fmt.Errorf("gcp: agent identity returned an empty result for %q", resource) } } // retrieveAgentIdentity calls the Agent Identity service, whose response is // returned synchronously (no long-running-operation wrapper). -func (c *Client) retrieveAgentIdentity(ctx context.Context, req Request) (retrieveResult, error) { +func (c *Client) retrieveAgentIdentity(ctx context.Context, req Request) (outcome, error) { url := fmt.Sprintf("%s/v1/%s/credentials:retrieve", c.agentIdentityURL, req.Resource) body := agentIdentityRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} var out agentIdentityResponse if err := c.doPost(ctx, url, body, &out); err != nil { - return retrieveResult{}, err + return nil, err } return out.result(req.Resource) } diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 03048a388..d65ab28f6 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -146,14 +146,14 @@ func (c *Client) RetrieveCredential(ctx context.Context, req Request) (auth.Cred if err != nil { return nil, err } - switch res.status { - case statusOK: - return mapCredential(res.header, res.token) - case statusConsentRequired: - return nil, &auth.ConsentRequiredError{AuthURI: res.consentURI, Nonce: res.consentNonce} - case statusRejected: + switch o := res.(type) { + case credOutcome: + return mapCredential(o.header, o.token) + case consentOutcome: + return nil, &auth.ConsentRequiredError{AuthURI: o.authURI, Nonce: o.nonce} + case rejectedOutcome: return nil, fmt.Errorf("%w for %q", ErrConsentRejected, req.Resource) - case statusPending: + case pendingOutcome: remaining := time.Until(deadline) if remaining <= 0 { return nil, fmt.Errorf("%w for %q", ErrPollTimeout, req.Resource) @@ -165,27 +165,35 @@ func (c *Client) RetrieveCredential(ctx context.Context, req Request) (auth.Cred case <-time.After(wait): } backoff = min(backoff*2, maxBackoff) + default: + return nil, fmt.Errorf("gcp: unexpected retrieval outcome %T", res) } } } -// retrieveStatus is the normalized outcome of a single retrieval call. -type retrieveStatus int - -const ( - statusOK retrieveStatus = iota - statusPending - statusConsentRequired - statusRejected +// outcome is the normalized result of one retrieval attempt from either service: +// exactly one concrete type is returned, so RetrieveCredential type-switches on +// it (a closed sum type over the services' result oneof). +type outcome interface{ isOutcome() } + +type ( + // credOutcome carries a successfully retrieved {header, token} credential. + credOutcome struct{ header, token string } + // pendingOutcome means retrieval is still pending; poll again. + pendingOutcome struct{} + // consentOutcome means interactive consent is required at authURI. + consentOutcome struct { + authURI string + nonce string + } + // rejectedOutcome means the end user rejected consent. + rejectedOutcome struct{} ) -type retrieveResult struct { - status retrieveStatus - token string - header string - consentURI string - consentNonce string -} +func (credOutcome) isOutcome() {} +func (pendingOutcome) isOutcome() {} +func (consentOutcome) isOutcome() {} +func (rejectedOutcome) isOutcome() {} // credentialPayload is the {header, token} success shape returned by both // services (nested under "success" for Agent Identity, under the operation diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index 370658c7a..d3232d231 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -45,48 +45,44 @@ type connectorOperation struct { } `json:"error"` } -// result collapses the Operation-wrapped response into a retrieveResult. -func (o connectorOperation) result(resource string) (retrieveResult, error) { +// result collapses the Operation-wrapped response into an outcome. +func (o connectorOperation) result(resource string) (outcome, error) { if o.Error != nil { - return retrieveResult{}, fmt.Errorf("gcp: connector operation failed: %s", o.Error.Message) + return nil, fmt.Errorf("gcp: connector operation failed: %s", o.Error.Message) } if o.Done { // A terminal operation must carry a credential; treat an empty result as // an error rather than polling to the timeout. if o.Response == nil { - return retrieveResult{}, fmt.Errorf("gcp: connector operation done but returned no credential for %q", resource) + return nil, fmt.Errorf("gcp: connector operation done but returned no credential for %q", resource) } - return retrieveResult{status: statusOK, token: o.Response.Token, header: o.Response.Header}, nil + return credOutcome{header: o.Response.Header, token: o.Response.Token}, nil } if md := o.Metadata; md != nil { switch { case md.URIConsentRequired != nil: - return retrieveResult{ - status: statusConsentRequired, - consentURI: md.URIConsentRequired.AuthorizationURI, - consentNonce: md.URIConsentRequired.ConsentNonce, - }, nil + return consentOutcome{authURI: md.URIConsentRequired.AuthorizationURI, nonce: md.URIConsentRequired.ConsentNonce}, nil case md.ConsentRejected != nil: - return retrieveResult{status: statusRejected}, nil + return rejectedOutcome{}, nil case md.ConsentPending != nil: - return retrieveResult{status: statusPending}, nil + return pendingOutcome{}, nil } } // The metadata status oneof is consent_pending, uri_consent_required, or // consent_rejected; treat an absent/unknown status as pending and keep // polling (consent_pending means "no action required, just retry"). - return retrieveResult{status: statusPending}, nil + return pendingOutcome{}, nil } // retrieveConnector calls the IAM Connector service and normalizes its // Operation-wrapped response. -func (c *Client) retrieveConnector(ctx context.Context, req Request) (retrieveResult, error) { +func (c *Client) retrieveConnector(ctx context.Context, req Request) (outcome, error) { url := fmt.Sprintf("%s/v1alpha/%s/credentials:retrieve", c.connectorURL, req.Resource) body := connectorRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} var op connectorOperation if err := c.doPost(ctx, url, body, &op); err != nil { - return retrieveResult{}, err + return nil, err } return op.result(req.Resource) } From c11f2db93b2a04fa2f9ee19555bc7c3285da155b Mon Sep 17 00:00:00 2001 From: wolo Date: Fri, 17 Jul 2026 11:07:53 +0000 Subject: [PATCH 07/15] test(auth/gcp): consolidate retrieval scenarios into table-driven tests Fold the eleven near-identical TestRetrieveAgentIdentity*/TestRetrieveConnector* scenario tests (plus the done-without-credential case) into a single table-driven TestRetrieveCredential, and make TestRetrieveValidatesRequest table-driven too. The routing, HTTP-error, mapCredential, cancellation, and poll-timeout tests stay separate (distinct setup/timing). No coverage change. --- auth/gcp/client_test.go | 294 +++++++++++++++++++--------------------- 1 file changed, 143 insertions(+), 151 deletions(-) diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index fca155243..87035eca7 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -34,139 +34,140 @@ const ( connectorResource = "projects/p/locations/l/connectors/co" ) -func TestRetrieveAgentIdentityBearer(t *testing.T) { - srv, _ := sequenceServer(`{"success":{"token":"tok","header":"Authorization: Bearer"}}`) - defer srv.Close() - - cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: authProviderResource, UserID: "u"}) - if err != nil { - t.Fatalf("RetrieveCredential() error = %v", err) - } - wantBearer(t, cred, "tok") -} - -func TestRetrieveAgentIdentityCustomHeader(t *testing.T) { - srv, _ := sequenceServer(`{"success":{"token":"KEY","header":"X-Goog-Api-Key"}}`) - defer srv.Close() - - cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: authProviderResource, UserID: "u"}) - if err != nil { - t.Fatalf("RetrieveCredential() error = %v", err) - } - wantAPIKey(t, cred, "X-Goog-Api-Key", "KEY") -} - -func TestRetrieveAgentIdentityConsentRequired(t *testing.T) { - srv, _ := sequenceServer(`{"uriConsentRequired":{"authorizationUri":"https://consent","consentNonce":"n"}}`) - defer srv.Close() - - _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: authProviderResource, UserID: "u"}) - var consent *auth.ConsentRequiredError - if !errors.As(err, &consent) { - t.Fatalf("error = %v, want *auth.ConsentRequiredError", err) - } - if consent.AuthURI != "https://consent" || consent.Nonce != "n" { - t.Errorf("consent = %+v, want auth_uri/nonce set", consent) - } -} - -func TestRetrieveAgentIdentityConsentRejected(t *testing.T) { - srv, _ := sequenceServer(`{"consentRejected":{}}`) - defer srv.Close() - - _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: authProviderResource, UserID: "u"}) - if !errors.Is(err, ErrConsentRejected) { - t.Fatalf("error = %v, want ErrConsentRejected", err) - } - var consent *auth.ConsentRequiredError - if errors.As(err, &consent) { - t.Fatalf("error = %v, want a plain rejection (not ConsentRequiredError)", err) - } -} - -func TestRetrieveAgentIdentityPollsPending(t *testing.T) { - srv, calls := sequenceServer( - `{"pending":{}}`, - `{"success":{"token":"tok","header":"Authorization: Bearer"}}`, - ) - defer srv.Close() - - cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: authProviderResource, UserID: "u"}) - if err != nil { - t.Fatalf("RetrieveCredential() error = %v", err) - } - wantBearer(t, cred, "tok") - if got := atomic.LoadInt32(calls); got != 2 { - t.Errorf("service calls = %d, want 2 (pending then success)", got) - } -} - -func TestRetrieveConnectorBearer(t *testing.T) { - srv, _ := sequenceServer(`{"done":true,"response":{"@type":"x","token":"tok","header":"Authorization: Bearer"}}`) - defer srv.Close() - - cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: connectorResource, UserID: "u"}) - if err != nil { - t.Fatalf("RetrieveCredential() error = %v", err) - } - wantBearer(t, cred, "tok") -} - -func TestRetrieveConnectorPollsConsentPending(t *testing.T) { - srv, calls := sequenceServer( - `{"metadata":{"@type":"x","consentPending":{}}}`, - `{"done":true,"response":{"token":"tok","header":"Authorization: Bearer"}}`, - ) - defer srv.Close() - - cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: connectorResource, UserID: "u"}) - if err != nil { - t.Fatalf("RetrieveCredential() error = %v", err) - } - wantBearer(t, cred, "tok") - if got := atomic.LoadInt32(calls); got != 2 { - t.Errorf("service calls = %d, want 2 (pending then success)", got) - } -} - -func TestRetrieveConnectorConsentRequired(t *testing.T) { - srv, _ := sequenceServer(`{"metadata":{"uriConsentRequired":{"authorizationUri":"https://c","consentNonce":"n"}}}`) - defer srv.Close() - - _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: connectorResource, UserID: "u"}) - var consent *auth.ConsentRequiredError - if !errors.As(err, &consent) { - t.Fatalf("error = %v, want *auth.ConsentRequiredError", err) +// TestRetrieveCredential drives RetrieveCredential end to end for both services +// via a fake server that replays the given response bodies in order. Each case +// sets exactly one expectation (a credential, a consent error, an errors.Is +// target, or an error substring). +func TestRetrieveCredential(t *testing.T) { + tests := []struct { + name string + resource string + bodies []string + wantCalls int // >0 => assert the number of service calls + wantBearer string // expect a bearer credential carrying this token + wantAPIKey [2]string // expect an API-key credential {name, value} + wantConsent [2]string // expect *auth.ConsentRequiredError {authURI, nonce} + wantErrIs error // expect errors.Is(err, target) + wantErrText string // expect err to contain this substring + }{ + // Agent Identity: synchronous "result" oneof. + { + name: "agent identity bearer", + resource: authProviderResource, + bodies: []string{`{"success":{"token":"tok","header":"Authorization: Bearer"}}`}, + wantBearer: "tok", + }, + { + name: "agent identity custom header", + resource: authProviderResource, + bodies: []string{`{"success":{"token":"KEY","header":"X-Goog-Api-Key"}}`}, + wantAPIKey: [2]string{"X-Goog-Api-Key", "KEY"}, + }, + { + name: "agent identity consent required", + resource: authProviderResource, + bodies: []string{`{"uriConsentRequired":{"authorizationUri":"https://consent","consentNonce":"n"}}`}, + wantConsent: [2]string{"https://consent", "n"}, + }, + { + name: "agent identity consent rejected", + resource: authProviderResource, + bodies: []string{`{"consentRejected":{}}`}, + wantErrIs: ErrConsentRejected, + }, + { + name: "agent identity polls pending then succeeds", + resource: authProviderResource, + bodies: []string{`{"pending":{}}`, `{"success":{"token":"tok","header":"Authorization: Bearer"}}`}, + wantBearer: "tok", + wantCalls: 2, + }, + // IAM Connector: google.longrunning.Operation wrapper. + { + name: "connector bearer", + resource: connectorResource, + bodies: []string{`{"done":true,"response":{"@type":"x","token":"tok","header":"Authorization: Bearer"}}`}, + wantBearer: "tok", + }, + { + name: "connector polls consent pending then succeeds", + resource: connectorResource, + bodies: []string{`{"metadata":{"@type":"x","consentPending":{}}}`, `{"done":true,"response":{"token":"tok","header":"Authorization: Bearer"}}`}, + wantBearer: "tok", + wantCalls: 2, + }, + { + name: "connector consent required", + resource: connectorResource, + bodies: []string{`{"metadata":{"uriConsentRequired":{"authorizationUri":"https://c","consentNonce":"n"}}}`}, + wantConsent: [2]string{"https://c", "n"}, + }, + { + name: "connector consent rejected", + resource: connectorResource, + bodies: []string{`{"metadata":{"consentRejected":{}}}`}, + wantErrIs: ErrConsentRejected, + }, + { + name: "connector operation error", + resource: connectorResource, + bodies: []string{`{"error":{"message":"boom"}}`}, + wantErrText: "boom", + }, + { + // A terminal (done) operation carrying no credential must fail fast, + // not be treated as pending and polled to the timeout. + name: "connector done without credential", + resource: connectorResource, + bodies: []string{`{"done":true}`}, + wantErrText: "no credential", + }, } -} - -func TestRetrieveConnectorConsentRejected(t *testing.T) { - srv, _ := sequenceServer(`{"metadata":{"consentRejected":{}}}`) - defer srv.Close() + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + srv, calls := sequenceServer(tc.bodies...) + defer srv.Close() - _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: connectorResource, UserID: "u"}) - if !errors.Is(err, ErrConsentRejected) { - t.Fatalf("error = %v, want ErrConsentRejected", err) - } -} + cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + Request{Resource: tc.resource, UserID: "u"}) -func TestRetrieveConnectorOperationError(t *testing.T) { - srv, _ := sequenceServer(`{"error":{"message":"boom"}}`) - defer srv.Close() + switch { + case tc.wantBearer != "": + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + wantBearer(t, cred, tc.wantBearer) + case tc.wantAPIKey[0] != "": + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + wantAPIKey(t, cred, tc.wantAPIKey[0], tc.wantAPIKey[1]) + case tc.wantConsent[0] != "": + var consent *auth.ConsentRequiredError + if !errors.As(err, &consent) { + t.Fatalf("error = %v, want *auth.ConsentRequiredError", err) + } + if consent.AuthURI != tc.wantConsent[0] || consent.Nonce != tc.wantConsent[1] { + t.Errorf("consent = %+v, want {authURI:%q nonce:%q}", consent, tc.wantConsent[0], tc.wantConsent[1]) + } + case tc.wantErrIs != nil: + if !errors.Is(err, tc.wantErrIs) { + t.Fatalf("error = %v, want errors.Is %v", err, tc.wantErrIs) + } + case tc.wantErrText != "": + if err == nil || !strings.Contains(err.Error(), tc.wantErrText) { + t.Fatalf("error = %v, want it to contain %q", err, tc.wantErrText) + } + default: + t.Fatalf("test case %q sets no expectation", tc.name) + } - _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: connectorResource, UserID: "u"}) - if err == nil || !strings.Contains(err.Error(), "boom") { - t.Fatalf("error = %v, want it to contain %q", err, "boom") + if tc.wantCalls != 0 { + if got := int(atomic.LoadInt32(calls)); got != tc.wantCalls { + t.Errorf("service calls = %d, want %d", got, tc.wantCalls) + } + } + }) } } @@ -224,12 +225,20 @@ func TestRetrieveHTTPError(t *testing.T) { } func TestRetrieveValidatesRequest(t *testing.T) { - c := &Client{httpClient: http.DefaultClient} - if _, err := c.RetrieveCredential(t.Context(), Request{UserID: "u"}); err == nil { - t.Error("missing Resource: got nil error, want error") + tests := []struct { + name string + req Request + }{ + {name: "missing resource", req: Request{UserID: "u"}}, + {name: "missing user id", req: Request{Resource: authProviderResource}}, } - if _, err := c.RetrieveCredential(t.Context(), Request{Resource: authProviderResource}); err == nil { - t.Error("missing UserID: got nil error, want error") + c := &Client{httpClient: http.DefaultClient} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if _, err := c.RetrieveCredential(t.Context(), tc.req); err == nil { + t.Errorf("RetrieveCredential(%+v) = nil error, want error", tc.req) + } + }) } } @@ -304,23 +313,6 @@ func TestRetrievePollTimeout(t *testing.T) { } } -// TestRetrieveConnectorDoneWithoutResponse verifies that a terminal (done) -// connector operation carrying no credential fails fast with an error, rather -// than being treated as pending and polled until the timeout. -func TestRetrieveConnectorDoneWithoutResponse(t *testing.T) { - srv, _ := sequenceServer(`{"done":true}`) - defer srv.Close() - - _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: connectorResource, UserID: "u"}) - if err == nil || !strings.Contains(err.Error(), "no credential") { - t.Fatalf("error = %v, want it to mention %q", err, "no credential") - } - if errors.Is(err, ErrPollTimeout) { - t.Fatalf("error = %v, want a done-without-credential error, not a poll timeout", err) - } -} - // newTestClient points both service endpoints at srv and uses a tiny backoff so // polling tests are fast. func newTestClient(t *testing.T, srv *httptest.Server) *Client { From 7c105e458662c3b21375d89cef0a2cb949d1274f Mon Sep 17 00:00:00 2001 From: wolo Date: Fri, 17 Jul 2026 11:13:07 +0000 Subject: [PATCH 08/15] docs(auth/gcp): tighten a few verbose comments Trim four slightly wordy comments (outcome, credentialPayload, the connector pending fall-through, and the TestRetrieveCredential doc) down to the essential "why" after the outcome/table-driven refactors. No code changes; the remaining comments were verified accurate and already terse. --- auth/gcp/client.go | 10 ++++------ auth/gcp/client_test.go | 5 ++--- auth/gcp/connector.go | 5 ++--- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index d65ab28f6..3a947cf03 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -171,9 +171,8 @@ func (c *Client) RetrieveCredential(ctx context.Context, req Request) (auth.Cred } } -// outcome is the normalized result of one retrieval attempt from either service: -// exactly one concrete type is returned, so RetrieveCredential type-switches on -// it (a closed sum type over the services' result oneof). +// outcome is the normalized result of one retrieval attempt — a closed sum type +// (one arm per state) that RetrieveCredential type-switches on. type outcome interface{ isOutcome() } type ( @@ -195,9 +194,8 @@ func (pendingOutcome) isOutcome() {} func (consentOutcome) isOutcome() {} func (rejectedOutcome) isOutcome() {} -// credentialPayload is the {header, token} success shape returned by both -// services (nested under "success" for Agent Identity, under the operation -// "response" for the IAM Connector). +// credentialPayload is the {header, token} success shape shared by both services +// (under "success" for Agent Identity, "response" for the IAM Connector operation). type credentialPayload struct { Token string `json:"token"` Header string `json:"header"` diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 87035eca7..bc4fd07d9 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -35,9 +35,8 @@ const ( ) // TestRetrieveCredential drives RetrieveCredential end to end for both services -// via a fake server that replays the given response bodies in order. Each case -// sets exactly one expectation (a credential, a consent error, an errors.Is -// target, or an error substring). +// via a fake server that replays response bodies in order; each case asserts one +// expected outcome. func TestRetrieveCredential(t *testing.T) { tests := []struct { name string diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index d3232d231..1d1e8e17f 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -68,9 +68,8 @@ func (o connectorOperation) result(resource string) (outcome, error) { return pendingOutcome{}, nil } } - // The metadata status oneof is consent_pending, uri_consent_required, or - // consent_rejected; treat an absent/unknown status as pending and keep - // polling (consent_pending means "no action required, just retry"). + // Absent/unknown status → pending: consent_pending means "just retry", and a + // non-terminal operation should keep being polled. return pendingOutcome{}, nil } From 3cf9dbf4d1880af02e336bfc9e17966b86ccabef Mon Sep 17 00:00:00 2001 From: wolo Date: Fri, 17 Jul 2026 13:00:28 +0000 Subject: [PATCH 09/15] refactor(auth/gcp): use a Config struct instead of functional options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit adk-go overwhelmingly constructs objects with config structs (runner, agent, llmagent, agenttool, skilltoolset, mcptoolset, agentregistry, gemini, ...); only apigee and telemetry use functional options. Replace the Option/With* API with a Config struct and NewClient(ctx, *Config) — a nil cfg or any zero field uses defaults — matching the gemini/agenttool nil-config precedent and the go-expert-review / adk-go-review "config structs" convention. No behavior change. --- auth/gcp/client.go | 52 ++++++++++++++++++++++------------------- auth/gcp/client_test.go | 12 +++++----- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 3a947cf03..b2db34f3b 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -65,38 +65,42 @@ type Client struct { initialBackoff time.Duration } -// Option configures a [Client]. -type Option func(*Client) - -// WithHTTPClient sets the HTTP client used to call the credential services. -// When unset, [NewClient] builds one from Application Default Credentials. -func WithHTTPClient(h *http.Client) Option { return func(c *Client) { c.httpClient = h } } - -// WithAgentIdentityEndpoint overrides the Agent Identity base URL (scheme+host). -func WithAgentIdentityEndpoint(url string) Option { - return func(c *Client) { c.agentIdentityURL = url } -} - -// WithConnectorEndpoint overrides the IAM Connector base URL (scheme+host). -func WithConnectorEndpoint(url string) Option { - return func(c *Client) { c.connectorURL = url } +// Config configures a [Client]. A nil *Config, or any zero-valued field, uses +// the corresponding default. +type Config struct { + // HTTPClient calls the credential services. If nil, [NewClient] builds one + // from Application Default Credentials (cloud-platform scope). + HTTPClient *http.Client + // AgentIdentityEndpoint overrides the Agent Identity base URL (scheme+host). + AgentIdentityEndpoint string + // ConnectorEndpoint overrides the IAM Connector base URL (scheme+host). + ConnectorEndpoint string + // PollTimeout bounds the total time spent polling a pending retrieval. + PollTimeout time.Duration } -// WithPollTimeout bounds the total time spent polling a pending retrieval. -func WithPollTimeout(d time.Duration) Option { return func(c *Client) { c.pollTimeout = d } } - -// NewClient builds a Client. Unless [WithHTTPClient] is supplied, it discovers -// Application Default Credentials (cloud-platform scope) to authenticate calls -// to the credential services. -func NewClient(ctx context.Context, opts ...Option) (*Client, error) { +// NewClient builds a Client from cfg; a nil cfg (or any zero field) uses +// defaults. Unless cfg.HTTPClient is set, it discovers Application Default +// Credentials (cloud-platform scope) to authenticate calls to the services. +func NewClient(ctx context.Context, cfg *Config) (*Client, error) { + if cfg == nil { + cfg = &Config{} + } c := &Client{ + httpClient: cfg.HTTPClient, agentIdentityURL: defaultAgentIdentityURL, connectorURL: defaultConnectorURL, pollTimeout: defaultPollTimeout, initialBackoff: defaultInitialBackoff, } - for _, opt := range opts { - opt(c) + if cfg.AgentIdentityEndpoint != "" { + c.agentIdentityURL = cfg.AgentIdentityEndpoint + } + if cfg.ConnectorEndpoint != "" { + c.connectorURL = cfg.ConnectorEndpoint + } + if cfg.PollTimeout != 0 { + c.pollTimeout = cfg.PollTimeout } if c.httpClient == nil { creds, err := google.FindDefaultCredentials(ctx, cloudPlatformScope) diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index bc4fd07d9..0ab850421 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -316,12 +316,12 @@ func TestRetrievePollTimeout(t *testing.T) { // polling tests are fast. func newTestClient(t *testing.T, srv *httptest.Server) *Client { t.Helper() - c, err := NewClient(t.Context(), - WithHTTPClient(srv.Client()), - WithAgentIdentityEndpoint(srv.URL), - WithConnectorEndpoint(srv.URL), - WithPollTimeout(2*time.Second), - ) + c, err := NewClient(t.Context(), &Config{ + HTTPClient: srv.Client(), + AgentIdentityEndpoint: srv.URL, + ConnectorEndpoint: srv.URL, + PollTimeout: 2 * time.Second, + }) if err != nil { t.Fatalf("NewClient() error = %v", err) } From 282ab39a0377878ff816bd4e364bfe7ffeffda24 Mon Sep 17 00:00:00 2001 From: wolo Date: Sun, 19 Jul 2026 10:27:26 +0000 Subject: [PATCH 10/15] fix(auth/gcp): harden HTTP handling and drop a dead field - Reject an oversized response body instead of feeding json.Unmarshal silently truncated bytes; cap error-body text so a large gateway page doesn't bloat the returned error. - Trim trailing slashes on endpoint overrides so a configured "host/" can't produce a "//v1/..." path. - Send Accept: application/json and include the operation error code in the connector failure message (which could be empty before). - Remove the unused connectorRequest.ForceRefresh field; use errors.New for the constant validation errors. - Add a NewClient test covering defaults and trailing-slash trimming. --- auth/gcp/agentidentity.go | 10 +---- auth/gcp/client.go | 77 ++++++++++++++++++++++++++++++--------- auth/gcp/client_test.go | 41 +++++++++++++++++++++ auth/gcp/connector.go | 17 +++------ 4 files changed, 108 insertions(+), 37 deletions(-) diff --git a/auth/gcp/agentidentity.go b/auth/gcp/agentidentity.go index 5ec60907e..b91382cdc 100644 --- a/auth/gcp/agentidentity.go +++ b/auth/gcp/agentidentity.go @@ -19,14 +19,6 @@ import ( "fmt" ) -// agentIdentityRequest is the JSON body for RetrieveCredentials (the auth -// provider is bound to the URL path, not the body). -type agentIdentityRequest struct { - UserID string `json:"userId,omitempty"` - Scopes []string `json:"scopes,omitempty"` - ContinueURI string `json:"continueUri,omitempty"` -} - // agentIdentityResponse mirrors the RetrieveCredentialsResponse "result" oneof. type agentIdentityResponse struct { Success *credentialPayload `json:"success"` @@ -62,7 +54,7 @@ func (r agentIdentityResponse) result(resource string) (outcome, error) { // returned synchronously (no long-running-operation wrapper). func (c *Client) retrieveAgentIdentity(ctx context.Context, req Request) (outcome, error) { url := fmt.Sprintf("%s/v1/%s/credentials:retrieve", c.agentIdentityURL, req.Resource) - body := agentIdentityRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} + body := retrieveRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} var out agentIdentityResponse if err := c.doPost(ctx, url, body, &out); err != nil { diff --git a/auth/gcp/client.go b/auth/gcp/client.go index b2db34f3b..c450cee82 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -25,6 +25,7 @@ import ( "regexp" "strings" "time" + "unicode/utf8" "golang.org/x/oauth2" "golang.org/x/oauth2/google" @@ -37,7 +38,9 @@ const ( defaultAgentIdentityURL = "https://agentidentitycredentials.googleapis.com" defaultConnectorURL = "https://iamconnectorcredentials.googleapis.com" - defaultPollTimeout = 10 * time.Second + defaultPollTimeout = 10 * time.Second + // The credentials service documents an exponential polling backoff + // (0.5, 1, 2, 4, 8s); these constants track it. defaultInitialBackoff = 500 * time.Millisecond maxBackoff = 8 * time.Second ) @@ -46,6 +49,12 @@ const ( // routed to the Agent Identity service (same split as adk-python). var connectorResourceRE = regexp.MustCompile(`^projects/[^/]+/locations/[^/]+/connectors/[^/]+$`) +// resourceNameRE bounds a resource name to the characters GCP resource names +// use, so it can't inject extra path segments, a query, or a fragment into the +// request URL it is interpolated into. A separate ".." check blocks path +// traversal (dots are allowed so domain-style ids still pass). +var resourceNameRE = regexp.MustCompile(`^[A-Za-z0-9._~/-]+$`) + // Sentinel errors from [Client.RetrieveCredential]; callers test with errors.Is. var ( // ErrConsentRejected means the end user rejected the consent request. @@ -75,7 +84,9 @@ type Config struct { AgentIdentityEndpoint string // ConnectorEndpoint overrides the IAM Connector base URL (scheme+host). ConnectorEndpoint string - // PollTimeout bounds the total time spent polling a pending retrieval. + // PollTimeout bounds the wall-clock time spent retrying a pending retrieval. + // It caps the retry loop, not an individual request; bound a single stalled + // request via ctx (or an HTTPClient with its own Timeout). PollTimeout time.Duration } @@ -94,10 +105,10 @@ func NewClient(ctx context.Context, cfg *Config) (*Client, error) { initialBackoff: defaultInitialBackoff, } if cfg.AgentIdentityEndpoint != "" { - c.agentIdentityURL = cfg.AgentIdentityEndpoint + c.agentIdentityURL = strings.TrimRight(cfg.AgentIdentityEndpoint, "/") } if cfg.ConnectorEndpoint != "" { - c.connectorURL = cfg.ConnectorEndpoint + c.connectorURL = strings.TrimRight(cfg.ConnectorEndpoint, "/") } if cfg.PollTimeout != 0 { c.pollTimeout = cfg.PollTimeout @@ -132,10 +143,13 @@ type Request struct { // If interactive consent is required it returns an [auth.ConsentRequiredError]. func (c *Client) RetrieveCredential(ctx context.Context, req Request) (auth.Credential, error) { if req.Resource == "" { - return nil, fmt.Errorf("gcp: RetrieveCredential requires a Resource") + return nil, errors.New("gcp: RetrieveCredential requires a Resource") } if req.UserID == "" { - return nil, fmt.Errorf("gcp: RetrieveCredential requires a UserID") + return nil, errors.New("gcp: RetrieveCredential requires a UserID") + } + if !resourceNameRE.MatchString(req.Resource) || strings.Contains(req.Resource, "..") { + return nil, fmt.Errorf("gcp: RetrieveCredential resource %q has invalid characters", req.Resource) } retrieve := c.retrieveAgentIdentity @@ -205,23 +219,29 @@ type credentialPayload struct { Header string `json:"header"` } +// retrieveRequest is the JSON body for both services' credentials:retrieve RPC +// (the auth provider / connector is bound to the URL path, not the body). +type retrieveRequest struct { + UserID string `json:"userId,omitempty"` + Scopes []string `json:"scopes,omitempty"` + ContinueURI string `json:"continueUri,omitempty"` +} + // mapCredential maps the service's {header, token} tuple to an [auth.Credential]: // an "Authorization: Bearer" header becomes a bearer credential; any other header // name becomes a header-based API key. func mapCredential(header, token string) (auth.Credential, error) { if header == "" || token == "" { - return nil, fmt.Errorf("gcp: credentials service returned an empty header or token") + return nil, errors.New("gcp: credentials service returned an empty header or token") } - name, hint, _ := strings.Cut(header, ":") - name = strings.TrimSpace(name) - if strings.EqualFold(name, "authorization") && - strings.HasPrefix(strings.ToLower(strings.TrimSpace(hint)), "bearer") { + name, scheme, _ := strings.Cut(header, ":") + if strings.EqualFold(strings.TrimSpace(name), "authorization") && + strings.HasPrefix(strings.ToLower(strings.TrimSpace(scheme)), "bearer") { return auth.BearerCredential{Token: token}, nil } - // Non-bearer header -> header-based API key. adk-python also mirrors the - // token into X-Goog-Api-Key for custom headers (alongside the service's own - // header), so match that behavior. - key := auth.APIKeyCredential{Name: name, Value: token} + // Non-bearer header -> header-based API key. Matches adk-python: key by the + // full returned header, and mirror the token into X-Goog-Api-Key too. + key := auth.APIKeyCredential{Name: header, Value: token} return auth.WithHeaders(key, map[string]string{"X-Goog-Api-Key": token}), nil } @@ -236,6 +256,7 @@ func (c *Client) doPost(ctx context.Context, url string, body, out any) error { return fmt.Errorf("gcp: build request: %w", err) } httpReq.Header.Set("Content-Type", "application/json") + httpReq.Header.Set("Accept", "application/json") resp, err := c.httpClient.Do(httpReq) if err != nil { @@ -243,15 +264,37 @@ func (c *Client) doPost(ctx context.Context, url string, body, out any) error { } defer func() { _ = resp.Body.Close() }() - data, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + // Read one byte past the cap so an oversized body is caught explicitly rather + // than fed to json.Unmarshal as silently truncated (and thus garbled) JSON. + const maxBody = 1 << 20 + data, err := io.ReadAll(io.LimitReader(resp.Body, maxBody+1)) if err != nil { return fmt.Errorf("gcp: read response: %w", err) } + if len(data) > maxBody { + return fmt.Errorf("gcp: credentials service response exceeded %d bytes", maxBody) + } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return fmt.Errorf("gcp: credentials service returned status %d: %s", resp.StatusCode, strings.TrimSpace(string(data))) + return fmt.Errorf("gcp: credentials service returned status %d: %s", resp.StatusCode, truncateForError(strings.TrimSpace(string(data)))) } if err := json.Unmarshal(data, out); err != nil { return fmt.Errorf("gcp: decode response: %w", err) } return nil } + +// truncateForError caps an error body so a large (e.g. HTML gateway) response +// doesn't bloat the returned error. +func truncateForError(s string) string { + const max = 1024 + if len(s) <= max { + return s + } + // Back up to a rune boundary so a multi-byte rune straddling the cap isn't + // sliced into a mangled partial rune. + cut := max + for cut > 0 && !utf8.RuneStart(s[cut]) { + cut-- + } + return s[:cut] + "..." +} diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 0ab850421..375754368 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -230,6 +230,9 @@ func TestRetrieveValidatesRequest(t *testing.T) { }{ {name: "missing resource", req: Request{UserID: "u"}}, {name: "missing user id", req: Request{Resource: authProviderResource}}, + {name: "resource path traversal", req: Request{Resource: "projects/p/../q/authProviders/a", UserID: "u"}}, + {name: "resource query injection", req: Request{Resource: "projects/p/authProviders/a?x=1", UserID: "u"}}, + {name: "resource with space", req: Request{Resource: "projects/p/authProviders/a b", UserID: "u"}}, } c := &Client{httpClient: http.DefaultClient} for _, tc := range tests { @@ -241,6 +244,44 @@ func TestRetrieveValidatesRequest(t *testing.T) { } } +func TestNewClient(t *testing.T) { + t.Run("defaults", func(t *testing.T) { + // Supply HTTPClient so the constructor skips the ADC lookup (offline test). + c, err := NewClient(t.Context(), &Config{HTTPClient: http.DefaultClient}) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + if c.agentIdentityURL != defaultAgentIdentityURL { + t.Errorf("agentIdentityURL = %q, want %q", c.agentIdentityURL, defaultAgentIdentityURL) + } + if c.connectorURL != defaultConnectorURL { + t.Errorf("connectorURL = %q, want %q", c.connectorURL, defaultConnectorURL) + } + if c.pollTimeout != defaultPollTimeout { + t.Errorf("pollTimeout = %v, want %v", c.pollTimeout, defaultPollTimeout) + } + if c.initialBackoff != defaultInitialBackoff { + t.Errorf("initialBackoff = %v, want %v", c.initialBackoff, defaultInitialBackoff) + } + }) + t.Run("trims endpoint trailing slash", func(t *testing.T) { + c, err := NewClient(t.Context(), &Config{ + HTTPClient: http.DefaultClient, + AgentIdentityEndpoint: "https://ai.example.com/", + ConnectorEndpoint: "https://conn.example.com/", + }) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + if c.agentIdentityURL != "https://ai.example.com" { + t.Errorf("agentIdentityURL = %q, want trailing slash trimmed", c.agentIdentityURL) + } + if c.connectorURL != "https://conn.example.com" { + t.Errorf("connectorURL = %q, want trailing slash trimmed", c.connectorURL) + } + }) +} + func TestMapCredential(t *testing.T) { tests := []struct { name string diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index 1d1e8e17f..969d7764a 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -19,15 +19,6 @@ import ( "fmt" ) -// connectorRequest is the JSON body for RetrieveCredentials (the connector is -// bound to the URL path, not the body). -type connectorRequest struct { - UserID string `json:"userId,omitempty"` - Scopes []string `json:"scopes,omitempty"` - ContinueURI string `json:"continueUri,omitempty"` - ForceRefresh bool `json:"forceRefresh,omitempty"` -} - // connectorOperation is the google.longrunning.Operation wrapper the IAM // Connector service returns. The service does not implement true LROs, so the // terminal result is read inline from response/metadata. The Any-typed @@ -41,6 +32,7 @@ type connectorOperation struct { ConsentRejected *struct{} `json:"consentRejected"` } `json:"metadata"` Error *struct { + Code int `json:"code"` Message string `json:"message"` } `json:"error"` } @@ -48,7 +40,10 @@ type connectorOperation struct { // result collapses the Operation-wrapped response into an outcome. func (o connectorOperation) result(resource string) (outcome, error) { if o.Error != nil { - return nil, fmt.Errorf("gcp: connector operation failed: %s", o.Error.Message) + if o.Error.Message != "" { + return nil, fmt.Errorf("gcp: connector operation failed (code %d): %s", o.Error.Code, o.Error.Message) + } + return nil, fmt.Errorf("gcp: connector operation failed (code %d)", o.Error.Code) } if o.Done { // A terminal operation must carry a credential; treat an empty result as @@ -77,7 +72,7 @@ func (o connectorOperation) result(resource string) (outcome, error) { // Operation-wrapped response. func (c *Client) retrieveConnector(ctx context.Context, req Request) (outcome, error) { url := fmt.Sprintf("%s/v1alpha/%s/credentials:retrieve", c.connectorURL, req.Resource) - body := connectorRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} + body := retrieveRequest{UserID: req.UserID, Scopes: req.Scopes, ContinueURI: req.ContinueURI} var op connectorOperation if err := c.doPost(ctx, url, body, &op); err != nil { From cebc430ba9fbe5d4b0293f6cc9527ae9859eac7e Mon Sep 17 00:00:00 2001 From: wolo Date: Fri, 7 Aug 2026 07:54:00 +0000 Subject: [PATCH 11/15] fix(auth/gcp): refuse redirects and close the review gaps The ADC-backed client followed redirects, and oauth2.Transport re-signs every hop below the layer where net/http strips credentials on a cross-host redirect. A 307 off the credentials endpoint therefore handed the cloud-platform token to whatever host the redirect named, and RetrieveCredential accepted that host's body as the end-user credential with a nil error. Three of the tests that looked like they pinned this package's guarantees passed with the guarantee deleted. - Refuse redirects on the ADC-built client; a credentials:retrieve call has no reason to redirect, and the existing non-2xx check now surfaces the 3xx. - Reject a returned header that is not a usable HTTP field name, so the failure lands at the cause instead of aborting a later request inside net/http. - Truncate and quote the connector's error.message, which bypassed both the 1 MiB cap and the escaping doPost applies everywhere else. - Quote the response body in the status error so a service-controlled body cannot forge lines in an operator's log. - Keep the HTTP status in the oversize-body error; it was the one actionable field and the size check runs first. - Bound truncateForError's backward scan: the body need not be UTF-8, and an unbounded scan over continuation bytes discarded every diagnostic byte. - Treat a negative PollTimeout as unset rather than "one attempt, then timeout". - Document that ctx is retained for every later token refresh, and that a supplied HTTPClient replaces ADC entirely. Tests: point the request-validation test at a live server and assert the service is never called, so deleting the resource checks now fails; mirror the token to a header that is not X-Goog-Api-Key, so deleting the mirror now fails; drive the real ADC branch, so deleting the redirect guard now fails; assert scopes and continueUri on the wire; cover the non-UTF-8 truncation path. --- auth/gcp/client.go | 62 +++++++++++++-- auth/gcp/client_test.go | 172 ++++++++++++++++++++++++++++++++++++++-- auth/gcp/connector.go | 4 +- 3 files changed, 222 insertions(+), 16 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index c450cee82..4ad9af3f0 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -78,7 +78,9 @@ type Client struct { // the corresponding default. type Config struct { // HTTPClient calls the credential services. If nil, [NewClient] builds one - // from Application Default Credentials (cloud-platform scope). + // from Application Default Credentials (cloud-platform scope). If set, it is + // used verbatim and ADC is not applied, so it must carry its own credentials + // and should refuse redirects for the reason [NewClient] describes. HTTPClient *http.Client // AgentIdentityEndpoint overrides the Agent Identity base URL (scheme+host). AgentIdentityEndpoint string @@ -93,6 +95,15 @@ type Config struct { // NewClient builds a Client from cfg; a nil cfg (or any zero field) uses // defaults. Unless cfg.HTTPClient is set, it discovers Application Default // Credentials (cloud-platform scope) to authenticate calls to the services. +// +// ctx is retained by the token source backing the returned client and is used +// for every later refresh, not just for discovery: a request-scoped ctx yields +// a Client that works until the first token expiry and then fails every +// retrieval with "context canceled". Pass a context that outlives the Client. +// +// The ADC-backed client refuses redirects. A credentials:retrieve call has no +// reason to redirect, and following one would re-sign the request and hand the +// cloud-platform token to the redirect target. func NewClient(ctx context.Context, cfg *Config) (*Client, error) { if cfg == nil { cfg = &Config{} @@ -110,7 +121,7 @@ func NewClient(ctx context.Context, cfg *Config) (*Client, error) { if cfg.ConnectorEndpoint != "" { c.connectorURL = strings.TrimRight(cfg.ConnectorEndpoint, "/") } - if cfg.PollTimeout != 0 { + if cfg.PollTimeout > 0 { c.pollTimeout = cfg.PollTimeout } if c.httpClient == nil { @@ -118,7 +129,14 @@ func NewClient(ctx context.Context, cfg *Config) (*Client, error) { if err != nil { return nil, fmt.Errorf("gcp: find default credentials: %w", err) } - c.httpClient = oauth2.NewClient(ctx, creds.TokenSource) + hc := oauth2.NewClient(ctx, creds.TokenSource) + // oauth2.Transport re-signs every hop, below the layer where net/http + // strips credentials on a cross-host redirect, so a redirect would leak + // the token to whatever host it names. + hc.CheckRedirect = func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + } + c.httpClient = hc } return c, nil } @@ -241,6 +259,11 @@ func mapCredential(header, token string) (auth.Credential, error) { } // Non-bearer header -> header-based API key. Matches adk-python: key by the // full returned header, and mirror the token into X-Goog-Api-Key too. + // Rejecting an unusable name here keeps the failure at the cause: net/http + // would otherwise accept the credential and abort the eventual request. + if !validHeaderFieldName(header) { + return nil, fmt.Errorf("gcp: credentials service returned %q, which is not a usable HTTP header name", header) + } key := auth.APIKeyCredential{Name: header, Value: token} return auth.WithHeaders(key, map[string]string{"X-Goog-Api-Key": token}), nil } @@ -272,10 +295,12 @@ func (c *Client) doPost(ctx context.Context, url string, body, out any) error { return fmt.Errorf("gcp: read response: %w", err) } if len(data) > maxBody { - return fmt.Errorf("gcp: credentials service response exceeded %d bytes", maxBody) + return fmt.Errorf("gcp: credentials service returned status %d with a response exceeding %d bytes", resp.StatusCode, maxBody) } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return fmt.Errorf("gcp: credentials service returned status %d: %s", resp.StatusCode, truncateForError(strings.TrimSpace(string(data)))) + // %q, not %s: the body is service-controlled and can carry control bytes + // that would otherwise forge lines in an operator's log. + return fmt.Errorf("gcp: credentials service returned status %d: %q", resp.StatusCode, truncateForError(strings.TrimSpace(string(data)))) } if err := json.Unmarshal(data, out); err != nil { return fmt.Errorf("gcp: decode response: %w", err) @@ -283,6 +308,24 @@ func (c *Client) doPost(ctx context.Context, url string, body, out any) error { return nil } +// validHeaderFieldName reports whether s is an RFC 9110 field name (a token). +// Hand-rolled because the module depends on golang.org/x/net only indirectly. +func validHeaderFieldName(s string) bool { + if s == "" { + return false + } + for i := 0; i < len(s); i++ { + c := s[i] + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + case strings.ContainsRune("!#$%&'*+-.^_`|~", rune(c)): + default: + return false + } + } + return true +} + // truncateForError caps an error body so a large (e.g. HTML gateway) response // doesn't bloat the returned error. func truncateForError(s string) string { @@ -291,10 +334,15 @@ func truncateForError(s string) string { return s } // Back up to a rune boundary so a multi-byte rune straddling the cap isn't - // sliced into a mangled partial rune. + // sliced into a mangled partial rune. Bounded: the body need not be UTF-8 at + // all, and an unbounded scan over continuation bytes would walk to 0 and + // discard every byte of diagnostic context. cut := max - for cut > 0 && !utf8.RuneStart(s[cut]) { + for i := 0; i < utf8.UTFMax-1 && cut > 0 && !utf8.RuneStart(s[cut]); i++ { cut-- } + if !utf8.RuneStart(s[cut]) { + cut = max + } return s[:cut] + "..." } diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 375754368..0fb21ef1e 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -21,6 +21,9 @@ import ( "io" "net/http" "net/http/httptest" + "os" + "path/filepath" + "slices" "strings" "sync/atomic" "testing" @@ -181,20 +184,27 @@ func TestRetrieveRoutesByResource(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - var gotPath, gotMethod, gotUserID string + var gotPath, gotMethod, gotUserID, gotContinueURI string + var gotScopes []string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { gotPath, gotMethod = r.URL.Path, r.Method var body struct { - UserID string `json:"userId"` + UserID string `json:"userId"` + Scopes []string `json:"scopes"` + ContinueURI string `json:"continueUri"` } _ = json.NewDecoder(r.Body).Decode(&body) - gotUserID = body.UserID + gotUserID, gotScopes, gotContinueURI = body.UserID, body.Scopes, body.ContinueURI _, _ = io.WriteString(w, `{"done":true,"response":{"token":"t","header":"Authorization: Bearer"},"success":{"token":"t","header":"Authorization: Bearer"}}`) })) defer srv.Close() - if _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), - Request{Resource: tc.resource, UserID: "user-1"}); err != nil { + if _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{ + Resource: tc.resource, + UserID: "user-1", + Scopes: []string{"scope-a", "scope-b"}, + ContinueURI: "https://example.test/continue", + }); err != nil { t.Fatalf("RetrieveCredential() error = %v", err) } if gotMethod != http.MethodPost { @@ -206,6 +216,14 @@ func TestRetrieveRoutesByResource(t *testing.T) { if gotUserID != "user-1" { t.Errorf("body userId = %q, want %q", gotUserID, "user-1") } + // ContinueURI is what makes the 3-legged flow work, so a wrong tag + // here would be silent and expensive. + if !slices.Equal(gotScopes, []string{"scope-a", "scope-b"}) { + t.Errorf("body scopes = %q, want [scope-a scope-b]", gotScopes) + } + if gotContinueURI != "https://example.test/continue" { + t.Errorf("body continueUri = %q, want %q", gotContinueURI, "https://example.test/continue") + } }) } } @@ -234,11 +252,26 @@ func TestRetrieveValidatesRequest(t *testing.T) { {name: "resource query injection", req: Request{Resource: "projects/p/authProviders/a?x=1", UserID: "u"}}, {name: "resource with space", req: Request{Resource: "projects/p/authProviders/a b", UserID: "u"}}, } - c := &Client{httpClient: http.DefaultClient} + // Point at a live server: a client with no endpoint fails at transport for + // every input, which cannot tell a rejected request from an unreachable one. + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + hits.Add(1) + })) + defer srv.Close() + c := newTestClient(t, srv) for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - if _, err := c.RetrieveCredential(t.Context(), tc.req); err == nil { - t.Errorf("RetrieveCredential(%+v) = nil error, want error", tc.req) + hits.Store(0) + _, err := c.RetrieveCredential(t.Context(), tc.req) + if err == nil { + t.Fatalf("RetrieveCredential(%+v) = nil error, want error", tc.req) + } + if !strings.Contains(err.Error(), "requires a") && !strings.Contains(err.Error(), "invalid characters") { + t.Errorf("error = %v, want a request-validation error", err) + } + if got := hits.Load(); got != 0 { + t.Errorf("credentials service called %d time(s); a rejected request must not reach the wire", got) } }) } @@ -294,8 +327,13 @@ func TestMapCredential(t *testing.T) { {name: "authorization bearer", header: "Authorization: Bearer", token: "t", wantBearer: "t"}, {name: "authorization bearer lowercase", header: "authorization: bearer", token: "t", wantBearer: "t"}, {name: "custom header", header: "X-Goog-Api-Key", token: "k", wantAPIKey: [2]string{"X-Goog-Api-Key", "k"}}, + // A name that is NOT X-Goog-Api-Key: with the mirror deleted, the two + // assertions in wantAPIKey would otherwise read the same header and pass. + {name: "third-party header is mirrored", header: "X-Acme-Token", token: "k", wantAPIKey: [2]string{"X-Acme-Token", "k"}}, {name: "empty header", header: "", token: "t", wantErr: true}, {name: "empty token", header: "Authorization: Bearer", token: "", wantErr: true}, + {name: "header carrying a scheme is not a usable field name", header: "X-Api-Key: Token", token: "k", wantErr: true}, + {name: "bare authorization is not a usable field name", header: "Authorization", token: "k", wantAPIKey: [2]string{"Authorization", "k"}}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -410,3 +448,121 @@ func wantAPIKey(t *testing.T, cred auth.Credential, name, value string) { t.Errorf("X-Goog-Api-Key = %q, want %q (adk-python parity)", got, value) } } + +// TestNewClientRefusesRedirects pins the ADC client's redirect guard: oauth2's +// transport re-signs every hop below net/http's cross-host stripping, so a +// followed redirect would hand the cloud-platform token to the target and let +// it dictate the returned credential. Drives the real ADC branch of NewClient, +// so deleting the guard fails here. +func TestNewClientRefusesRedirects(t *testing.T) { + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"access_token":"ADC-TOKEN","token_type":"Bearer","expires_in":3600}`) + })) + defer tokenSrv.Close() + + var targetSawAuth string + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + targetSawAuth = r.Header.Get("Authorization") + _, _ = io.WriteString(w, `{"success":{"token":"attacker","header":"Authorization: Bearer"}}`) + })) + defer target.Close() + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL+r.URL.Path, http.StatusTemporaryRedirect) + })) + defer redirector.Close() + + adc := filepath.Join(t.TempDir(), "adc.json") + if err := os.WriteFile(adc, []byte(`{"type":"authorized_user","client_id":"c","client_secret":"s","refresh_token":"r","token_uri":"`+tokenSrv.URL+`"}`), 0o600); err != nil { + t.Fatalf("write fake ADC: %v", err) + } + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", adc) + + c, err := NewClient(t.Context(), &Config{ + AgentIdentityEndpoint: redirector.URL, + ConnectorEndpoint: redirector.URL, + }) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + cred, err := c.RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) + if err == nil { + t.Fatalf("RetrieveCredential() = %#v, nil error; want the 3xx surfaced as an error", cred) + } + if targetSawAuth != "" { + t.Errorf("redirect target received Authorization %q; the token must not leave the configured host", targetSawAuth) + } +} + +// TestDoPostOversizeKeepsStatus: the size check runs first, so it must carry the +// status or the most actionable field is lost. +func TestDoPostOversizeKeepsStatus(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadGateway) + _, _ = io.WriteString(w, strings.Repeat("x", (1<<20)+10)) + })) + defer srv.Close() + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + Request{Resource: authProviderResource, UserID: "u"}) + if err == nil { + t.Fatal("RetrieveCredential() = nil error, want error") + } + if !strings.Contains(err.Error(), "502") { + t.Errorf("error = %v, want it to name status 502", err) + } +} + +// TestDoPostEscapesErrorBody: a service-controlled body must not be able to +// forge log lines through the returned error. +func TestDoPostEscapesErrorBody(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = io.WriteString(w, "unavailable\r\nINFO auth: credential granted user=victim") + })) + defer srv.Close() + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + Request{Resource: authProviderResource, UserID: "u"}) + if err == nil { + t.Fatal("RetrieveCredential() = nil error, want error") + } + if strings.Contains(err.Error(), "\r\n") { + t.Errorf("error carries raw control bytes: %q", err.Error()) + } + if !strings.Contains(err.Error(), `\r\n`) { + t.Errorf("error = %q, want the body escaped", err.Error()) + } +} + +func TestTruncateForError(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {name: "short is unchanged", in: "nope", want: "nope"}, + {name: "long is cut", in: strings.Repeat("a", 2000), want: strings.Repeat("a", 1024) + "..."}, + // A body need not be UTF-8; an unbounded backup would walk to 0 here and + // throw away every byte of diagnostic context. + {name: "non utf8 keeps context", in: strings.Repeat("\x80", 2000), want: strings.Repeat("\x80", 1024) + "..."}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := truncateForError(tc.in); got != tc.want { + t.Errorf("truncateForError() length = %d, want %d", len(got), len(tc.want)) + } + }) + } +} + +func TestNewClientRejectsNegativePollTimeout(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + defer srv.Close() + c, err := NewClient(t.Context(), &Config{HTTPClient: srv.Client(), PollTimeout: -time.Second}) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + if c.pollTimeout != defaultPollTimeout { + t.Errorf("pollTimeout = %v, want the default %v (a negative value must not mean 'never retry')", + c.pollTimeout, defaultPollTimeout) + } +} diff --git a/auth/gcp/connector.go b/auth/gcp/connector.go index 969d7764a..65dc2f640 100644 --- a/auth/gcp/connector.go +++ b/auth/gcp/connector.go @@ -41,7 +41,9 @@ type connectorOperation struct { func (o connectorOperation) result(resource string) (outcome, error) { if o.Error != nil { if o.Error.Message != "" { - return nil, fmt.Errorf("gcp: connector operation failed (code %d): %s", o.Error.Code, o.Error.Message) + // Same treatment doPost gives a response body: the message is + // service-controlled and otherwise bypasses both the cap and escaping. + return nil, fmt.Errorf("gcp: connector operation failed (code %d): %q", o.Error.Code, truncateForError(o.Error.Message)) } return nil, fmt.Errorf("gcp: connector operation failed (code %d)", o.Error.Code) } From e640f7bc95d9a069b3dc50eef9db1d109eba4aa0 Mon Sep 17 00:00:00 2001 From: wolo Date: Tue, 11 Aug 2026 14:03:15 +0000 Subject: [PATCH 12/15] fix(auth/gcp): detach the ADC token source from the construction context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token source built by FindDefaultCredentials captures the context it is given and reuses it for every later refresh, so a Client constructed inside a request-scoped context stops working once that context ends — the credential provider in the follow-up change builds its client exactly that way, from a bounded context that is cancelled on return. Discovery itself does not need the caller's cancellation: its only network probe (GCE metadata detection) ignores the context and bounds itself. So pass a detached context and keep the caller's values. --- auth/gcp/client.go | 12 ++++++---- auth/gcp/client_test.go | 53 ++++++++++++++++++++++++++++++++--------- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 4ad9af3f0..29cd26352 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -96,10 +96,9 @@ type Config struct { // defaults. Unless cfg.HTTPClient is set, it discovers Application Default // Credentials (cloud-platform scope) to authenticate calls to the services. // -// ctx is retained by the token source backing the returned client and is used -// for every later refresh, not just for discovery: a request-scoped ctx yields -// a Client that works until the first token expiry and then fails every -// retrieval with "context canceled". Pass a context that outlives the Client. +// ctx bounds credential discovery only: the token source backing the returned +// client is detached from ctx's cancellation, so a Client built inside a +// request-scoped context keeps refreshing its token after that request ends. // // The ADC-backed client refuses redirects. A credentials:retrieve call has no // reason to redirect, and following one would re-sign the request and hand the @@ -125,7 +124,10 @@ func NewClient(ctx context.Context, cfg *Config) (*Client, error) { c.pollTimeout = cfg.PollTimeout } if c.httpClient == nil { - creds, err := google.FindDefaultCredentials(ctx, cloudPlatformScope) + // The token source captures this context and reuses it for every later + // refresh, so it must outlive the call; discovery itself needs no + // cancellation (its only network probe bounds itself). + creds, err := google.FindDefaultCredentials(context.WithoutCancel(ctx), cloudPlatformScope) if err != nil { return nil, fmt.Errorf("gcp: find default credentials: %w", err) } diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 0fb21ef1e..f727fa01e 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -455,11 +455,7 @@ func wantAPIKey(t *testing.T, cred auth.Credential, name, value string) { // it dictate the returned credential. Drives the real ADC branch of NewClient, // so deleting the guard fails here. func TestNewClientRefusesRedirects(t *testing.T) { - tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, _ = io.WriteString(w, `{"access_token":"ADC-TOKEN","token_type":"Bearer","expires_in":3600}`) - })) - defer tokenSrv.Close() + fakeADC(t) var targetSawAuth string target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -472,12 +468,6 @@ func TestNewClientRefusesRedirects(t *testing.T) { })) defer redirector.Close() - adc := filepath.Join(t.TempDir(), "adc.json") - if err := os.WriteFile(adc, []byte(`{"type":"authorized_user","client_id":"c","client_secret":"s","refresh_token":"r","token_uri":"`+tokenSrv.URL+`"}`), 0o600); err != nil { - t.Fatalf("write fake ADC: %v", err) - } - t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", adc) - c, err := NewClient(t.Context(), &Config{ AgentIdentityEndpoint: redirector.URL, ConnectorEndpoint: redirector.URL, @@ -494,6 +484,47 @@ func TestNewClientRefusesRedirects(t *testing.T) { } } +// TestNewClientOutlivesConstructionCtx pins the token source's detachment from +// the construction context. Callers build the client inside a bounded, +// request-scoped context (the auth/gcp credential provider does exactly that), +// and every token minted after that context ends must still authenticate. +func TestNewClientOutlivesConstructionCtx(t *testing.T) { + fakeADC(t) + srv, _ := sequenceServer(`{"success":{"token":"tok","header":"Authorization: Bearer"}}`) + defer srv.Close() + + ctx, cancel := context.WithCancel(t.Context()) + c, err := NewClient(ctx, &Config{AgentIdentityEndpoint: srv.URL, ConnectorEndpoint: srv.URL}) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + cancel() + + cred, err := c.RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) + if err != nil { + t.Fatalf("RetrieveCredential() error = %v", err) + } + wantBearer(t, cred, "tok") +} + +// fakeADC points Application Default Credentials at a local token server so the +// ADC branch of NewClient runs offline. The token expires immediately, so every +// call mints a fresh one and the token source's own context stays observable. +func fakeADC(t *testing.T) { + t.Helper() + tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"access_token":"ADC-TOKEN","token_type":"Bearer","expires_in":1}`) + })) + t.Cleanup(tokenSrv.Close) + + adc := filepath.Join(t.TempDir(), "adc.json") + if err := os.WriteFile(adc, []byte(`{"type":"authorized_user","client_id":"c","client_secret":"s","refresh_token":"r","token_uri":"`+tokenSrv.URL+`"}`), 0o600); err != nil { + t.Fatalf("write fake ADC: %v", err) + } + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", adc) +} + // TestDoPostOversizeKeepsStatus: the size check runs first, so it must carry the // status or the most actionable field is lost. func TestDoPostOversizeKeepsStatus(t *testing.T) { From 1d56ef4fd7db56b1d618d63ba60d0a673ab37845 Mon Sep 17 00:00:00 2001 From: wolo Date: Tue, 11 Aug 2026 14:04:38 +0000 Subject: [PATCH 13/15] feat(auth/gcp): return a typed APIError for a non-2xx status A non-2xx status is the most common failure a credentials client sees, and a formatted string forces the caller to match on the message to tell a fatal 403 from a retryable 503. Return the same shape agentregistry already exports for its REST client, so callers use errors.As instead. Classifying the status before the body-size check also keeps the status on an error page too large to read: only a 2xx over the cap is now a bare size error. --- auth/gcp/client.go | 28 ++++++++++++++++++++++------ auth/gcp/client_test.go | 38 ++++++++++++++++++++++++++++++-------- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 29cd26352..3da4ce000 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -64,6 +64,22 @@ var ( ErrPollTimeout = errors.New("gcp: timed out waiting for credentials") ) +// APIError is returned when a credential service responds with a non-2xx +// status. Callers match it with errors.As to tell a fatal status (say 403) from +// a transient one (503) without matching on the message. +type APIError struct { + // StatusCode is the HTTP status code of the response. + StatusCode int + // Body is the response body, truncated, useful for diagnosing the failure. + Body string +} + +func (e *APIError) Error() string { + // %q, not %s: the body is service-controlled and can carry control bytes + // that would otherwise forge lines in an operator's log. + return fmt.Sprintf("gcp: credentials service returned status %d: %q", e.StatusCode, e.Body) +} + // Client retrieves end-user credentials from the Agent Identity / IAM Connector // credential services and maps them to [auth.Credential]. type Client struct { @@ -296,13 +312,13 @@ func (c *Client) doPost(ctx context.Context, url string, body, out any) error { if err != nil { return fmt.Errorf("gcp: read response: %w", err) } - if len(data) > maxBody { - return fmt.Errorf("gcp: credentials service returned status %d with a response exceeding %d bytes", resp.StatusCode, maxBody) - } + // Classify the status before the size check, so an oversized error page still + // reports the status — the most actionable field — instead of only its size. if resp.StatusCode < 200 || resp.StatusCode >= 300 { - // %q, not %s: the body is service-controlled and can carry control bytes - // that would otherwise forge lines in an operator's log. - return fmt.Errorf("gcp: credentials service returned status %d: %q", resp.StatusCode, truncateForError(strings.TrimSpace(string(data)))) + return &APIError{StatusCode: resp.StatusCode, Body: truncateForError(strings.TrimSpace(string(data)))} + } + if len(data) > maxBody { + return fmt.Errorf("gcp: credentials service response exceeded %d bytes", maxBody) } if err := json.Unmarshal(data, out); err != nil { return fmt.Errorf("gcp: decode response: %w", err) diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index f727fa01e..79843ce20 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -236,8 +236,15 @@ func TestRetrieveHTTPError(t *testing.T) { _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) - if err == nil || !strings.Contains(err.Error(), "500") { - t.Fatalf("error = %v, want it to mention status 500", err) + var apiErr *APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error = %v, want *APIError", err) + } + if apiErr.StatusCode != http.StatusInternalServerError { + t.Errorf("StatusCode = %d, want %d", apiErr.StatusCode, http.StatusInternalServerError) + } + if !strings.Contains(apiErr.Body, "nope") { + t.Errorf("Body = %q, want it to carry the response body", apiErr.Body) } } @@ -525,8 +532,8 @@ func fakeADC(t *testing.T) { t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", adc) } -// TestDoPostOversizeKeepsStatus: the size check runs first, so it must carry the -// status or the most actionable field is lost. +// TestDoPostOversizeKeepsStatus: an error page big enough to trip the body cap +// must still report its status, the most actionable field. func TestDoPostOversizeKeepsStatus(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusBadGateway) @@ -535,11 +542,26 @@ func TestDoPostOversizeKeepsStatus(t *testing.T) { defer srv.Close() _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), Request{Resource: authProviderResource, UserID: "u"}) - if err == nil { - t.Fatal("RetrieveCredential() = nil error, want error") + var apiErr *APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error = %v, want *APIError", err) + } + if apiErr.StatusCode != http.StatusBadGateway { + t.Errorf("StatusCode = %d, want %d", apiErr.StatusCode, http.StatusBadGateway) } - if !strings.Contains(err.Error(), "502") { - t.Errorf("error = %v, want it to name status 502", err) +} + +// A 2xx body over the cap must be rejected, not handed to json.Unmarshal +// truncated (and thus garbled). +func TestDoPostRejectsOversizeSuccess(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.WriteString(w, `{"success":{"token":"t","header":"Authorization: Bearer"}}`+strings.Repeat(" ", 1<<20)) + })) + defer srv.Close() + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + Request{Resource: authProviderResource, UserID: "u"}) + if err == nil || !strings.Contains(err.Error(), "exceeded") { + t.Fatalf("error = %v, want the oversize response rejected", err) } } From 573e47a76559ec8875de964a13c76a2468a6399a Mon Sep 17 00:00:00 2001 From: wolo Date: Wed, 12 Aug 2026 20:35:14 +0000 Subject: [PATCH 14/15] test(auth/gcp): fix three misleading spots in the client tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A case named for rejecting a header asserted that it is accepted, the note explaining why continueUri matters sat above the scopes assertion, and truncateForError's failure message reported only lengths — so a body cut in the wrong place but at the right size printed "1027, want 1027". --- auth/gcp/client_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 79843ce20..1f0cf6340 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -216,11 +216,11 @@ func TestRetrieveRoutesByResource(t *testing.T) { if gotUserID != "user-1" { t.Errorf("body userId = %q, want %q", gotUserID, "user-1") } - // ContinueURI is what makes the 3-legged flow work, so a wrong tag - // here would be silent and expensive. if !slices.Equal(gotScopes, []string{"scope-a", "scope-b"}) { t.Errorf("body scopes = %q, want [scope-a scope-b]", gotScopes) } + // ContinueURI is what makes the 3-legged flow work, so a wrong tag + // here would be silent and expensive. if gotContinueURI != "https://example.test/continue" { t.Errorf("body continueUri = %q, want %q", gotContinueURI, "https://example.test/continue") } @@ -340,7 +340,7 @@ func TestMapCredential(t *testing.T) { {name: "empty header", header: "", token: "t", wantErr: true}, {name: "empty token", header: "Authorization: Bearer", token: "", wantErr: true}, {name: "header carrying a scheme is not a usable field name", header: "X-Api-Key: Token", token: "k", wantErr: true}, - {name: "bare authorization is not a usable field name", header: "Authorization", token: "k", wantAPIKey: [2]string{"Authorization", "k"}}, + {name: "bare authorization maps to an api key", header: "Authorization", token: "k", wantAPIKey: [2]string{"Authorization", "k"}}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -600,8 +600,11 @@ func TestTruncateForError(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { + // Report the tail as well as the length: a body that is cut at the + // wrong place can still come out the right size. if got := truncateForError(tc.in); got != tc.want { - t.Errorf("truncateForError() length = %d, want %d", len(got), len(tc.want)) + t.Errorf("truncateForError() = %d bytes ending %q, want %d bytes ending %q", + len(got), got[max(0, len(got)-8):], len(tc.want), tc.want[max(0, len(tc.want)-8):]) } }) } From 4fda669fddb1bcc13ed955739cc3735ee4d3cd09 Mon Sep 17 00:00:00 2001 From: wolo Date: Tue, 18 Aug 2026 20:16:39 +0000 Subject: [PATCH 15/15] fix(auth/gcp): correct the NewClient ctx doc and cap the last error Follow-up to the approval nits. The NewClient godoc claimed ctx bounds credential discovery, which is the one thing it does not do: FindDefaultCredentials is called with WithoutCancel, and x/oauth2's discovery path never consults a caller's context anyway. It now says ctx is used for discovery only and its cancellation is not honored, matching the inline comment it disagreed with. The rejected-header-name error was the last site formatting service-controlled text without the cap every other error site applies; a 900 KB header name produced a 900 KB error. Also pin the guards that no test held: deleting truncateForError from doPost, from the connector operation message or from the header-name error, or dropping the ctx arm of the poll wait, each now fails a test. The poll-wait case cost only promptness, so that assertion is on elapsed time. - Document that an http.Client in the ctx under oauth2.HTTPClient bounds requests while keeping ADC, and that the endpoint overrides are unparsed. - Cover the documented NewClient(ctx, nil) path, and pin the two services' deliberate disagreement on an unrecognised 200. --- auth/gcp/client.go | 23 +++++++--- auth/gcp/client_test.go | 93 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 107 insertions(+), 9 deletions(-) diff --git a/auth/gcp/client.go b/auth/gcp/client.go index 3da4ce000..1851bb6f4 100644 --- a/auth/gcp/client.go +++ b/auth/gcp/client.go @@ -99,12 +99,19 @@ type Config struct { // and should refuse redirects for the reason [NewClient] describes. HTTPClient *http.Client // AgentIdentityEndpoint overrides the Agent Identity base URL (scheme+host). + // It is used as given, not parsed: an http:// value would send the ADC token + // in the clear, so keep it https outside tests. AgentIdentityEndpoint string - // ConnectorEndpoint overrides the IAM Connector base URL (scheme+host). + // ConnectorEndpoint overrides the IAM Connector base URL (scheme+host), with + // the same caveat as AgentIdentityEndpoint. ConnectorEndpoint string // PollTimeout bounds the wall-clock time spent retrying a pending retrieval. // It caps the retry loop, not an individual request; bound a single stalled // request via ctx (or an HTTPClient with its own Timeout). + // + // To bound requests without giving up ADC, put an [http.Client] carrying a + // Timeout in the context passed to [NewClient] under [oauth2.HTTPClient]: + // its Timeout is carried through to the ADC-backed client. PollTimeout time.Duration } @@ -112,9 +119,10 @@ type Config struct { // defaults. Unless cfg.HTTPClient is set, it discovers Application Default // Credentials (cloud-platform scope) to authenticate calls to the services. // -// ctx bounds credential discovery only: the token source backing the returned -// client is detached from ctx's cancellation, so a Client built inside a -// request-scoped context keeps refreshing its token after that request ends. +// ctx is used for credential discovery only, and its cancellation is not +// honored: the token source backing the returned client is detached from ctx, +// so a Client built inside a request-scoped context keeps refreshing its token +// after that request ends. // // The ADC-backed client refuses redirects. A credentials:retrieve call has no // reason to redirect, and following one would re-sign the request and hand the @@ -280,7 +288,7 @@ func mapCredential(header, token string) (auth.Credential, error) { // Rejecting an unusable name here keeps the failure at the cause: net/http // would otherwise accept the credential and abort the eventual request. if !validHeaderFieldName(header) { - return nil, fmt.Errorf("gcp: credentials service returned %q, which is not a usable HTTP header name", header) + return nil, fmt.Errorf("gcp: credentials service returned %q, which is not a usable HTTP header name", truncateForError(header)) } key := auth.APIKeyCredential{Name: header, Value: token} return auth.WithHeaders(key, map[string]string{"X-Goog-Api-Key": token}), nil @@ -344,10 +352,13 @@ func validHeaderFieldName(s string) bool { return true } +// maxErrorBody caps service-controlled text carried into an error. +const maxErrorBody = 1024 + // truncateForError caps an error body so a large (e.g. HTML gateway) response // doesn't bloat the returned error. func truncateForError(s string) string { - const max = 1024 + const max = maxErrorBody if len(s) <= max { return s } diff --git a/auth/gcp/client_test.go b/auth/gcp/client_test.go index 1f0cf6340..119c40a10 100644 --- a/auth/gcp/client_test.go +++ b/auth/gcp/client_test.go @@ -51,6 +51,7 @@ func TestRetrieveCredential(t *testing.T) { wantConsent [2]string // expect *auth.ConsentRequiredError {authURI, nonce} wantErrIs error // expect errors.Is(err, target) wantErrText string // expect err to contain this substring + pollTimeout time.Duration }{ // Agent Identity: synchronous "result" oneof. { @@ -124,13 +125,36 @@ func TestRetrieveCredential(t *testing.T) { bodies: []string{`{"done":true}`}, wantErrText: "no credential", }, + // The two services deliberately disagree on an unrecognised 200: Agent + // Identity's result is a closed oneof, so an unknown arm can only be a + // mismatch worth failing on... + { + name: "agent identity unrecognized result fails fast", + resource: authProviderResource, + bodies: []string{`{}`}, + wantErrText: "empty result", + wantCalls: 1, + }, + // ...whereas a connector operation that is merely not done yet is normal, + // so an unrecognised one keeps being polled until the timeout. + { + name: "connector unrecognized operation polls to timeout", + resource: connectorResource, + bodies: []string{`{}`}, + wantErrIs: ErrPollTimeout, + pollTimeout: 30 * time.Millisecond, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { srv, calls := sequenceServer(tc.bodies...) defer srv.Close() - cred, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + c := newTestClient(t, srv) + if tc.pollTimeout > 0 { + c.pollTimeout = tc.pollTimeout + } + cred, err := c.RetrieveCredential(t.Context(), Request{Resource: tc.resource, UserID: "u"}) switch { @@ -304,6 +328,23 @@ func TestNewClient(t *testing.T) { t.Errorf("initialBackoff = %v, want %v", c.initialBackoff, defaultInitialBackoff) } }) + t.Run("nil config uses defaults", func(t *testing.T) { + // The nil-Config path the exported doc promises; it takes the ADC branch. + fakeADC(t) + c, err := NewClient(t.Context(), nil) + if err != nil { + t.Fatalf("NewClient() error = %v", err) + } + if c.httpClient == nil { + t.Error("httpClient = nil, want an ADC-backed client") + } + if c.agentIdentityURL != defaultAgentIdentityURL || c.connectorURL != defaultConnectorURL { + t.Errorf("endpoints = %q / %q, want the defaults", c.agentIdentityURL, c.connectorURL) + } + if c.pollTimeout != defaultPollTimeout { + t.Errorf("pollTimeout = %v, want %v", c.pollTimeout, defaultPollTimeout) + } + }) t.Run("trims endpoint trailing slash", func(t *testing.T) { c, err := NewClient(t.Context(), &Config{ HTTPClient: http.DefaultClient, @@ -366,20 +407,45 @@ func TestMapCredential(t *testing.T) { // TestRetrieveContextCanceledWhilePending verifies that canceling the context // aborts a pending poll promptly (no hang) and surfaces context.Canceled. +// The rejected header name is service-controlled and reaches the error by a +// third path, separate from a response body and an operation message. +func TestMapCredentialCapsHeaderNameInError(t *testing.T) { + _, err := mapCredential(strings.Repeat("x", 900_000)+": Token", "SECRET-TOKEN") + if err == nil { + t.Fatal("mapCredential() = nil error, want error") + } + if len(err.Error()) > 2*maxErrorBody { + t.Errorf("error is %d bytes, want the header name capped to %d", len(err.Error()), maxErrorBody) + } + if strings.Contains(err.Error(), "SECRET-TOKEN") { + t.Error("error carries the token") + } +} + func TestRetrieveContextCanceledWhilePending(t *testing.T) { srv, _ := sequenceServer(`{"pending":{}}`) // never resolves defer srv.Close() c := newTestClient(t, srv) - c.initialBackoff = 50 * time.Millisecond // park in the poll wait, then cancel + // A backoff far longer than the window asserted below. Without the ctx arm of + // the poll wait, cancellation is only noticed on the next request, so the + // outcome still holds and only the promptness — the point here — is lost. + c.pollTimeout = time.Minute + c.initialBackoff = 30 * time.Second ctx, cancel := context.WithCancel(t.Context()) - time.AfterFunc(10*time.Millisecond, cancel) + time.AfterFunc(20*time.Millisecond, cancel) + start := time.Now() _, err := c.RetrieveCredential(ctx, Request{Resource: authProviderResource, UserID: "u"}) + elapsed := time.Since(start) + if !errors.Is(err, context.Canceled) { t.Fatalf("RetrieveCredential() error = %v, want context.Canceled", err) } + if elapsed > 5*time.Second { + t.Errorf("returned after %v, want promptly after cancellation (backoff was %v)", elapsed, c.initialBackoff) + } } // TestRetrievePollTimeout verifies that a service stuck in the non-interactive @@ -549,6 +615,27 @@ func TestDoPostOversizeKeepsStatus(t *testing.T) { if apiErr.StatusCode != http.StatusBadGateway { t.Errorf("StatusCode = %d, want %d", apiErr.StatusCode, http.StatusBadGateway) } + // Pin the truncation, not just the helper: without it the whole 1 MiB page + // rides along in the error. + if len(apiErr.Body) > maxErrorBody+len("...") { + t.Errorf("Body = %d bytes, want it capped to %d", len(apiErr.Body), maxErrorBody) + } +} + +// A service-controlled operation message must be capped like any response body; +// it reaches the error by a different path than doPost's body. +func TestRetrieveConnectorErrorMessageIsCapped(t *testing.T) { + srv, _ := sequenceServer(`{"error":{"code":7,"message":"` + strings.Repeat("x", 900_000) + `"}}`) + defer srv.Close() + + _, err := newTestClient(t, srv).RetrieveCredential(t.Context(), + Request{Resource: connectorResource, UserID: "u"}) + if err == nil { + t.Fatal("RetrieveCredential() = nil error, want error") + } + if len(err.Error()) > 2*maxErrorBody { + t.Errorf("error is %d bytes, want the message capped to %d", len(err.Error()), maxErrorBody) + } } // A 2xx body over the cap must be rejected, not handed to json.Unmarshal