feat(auth/gcp): add credentials-service REST client - #1149
Conversation
6d59920 to
e49269e
Compare
034ee34 to
c300cef
Compare
73b5382 to
c2c93fc
Compare
There was a problem hiding this comment.
The routing split, the sealed-outcome modelling of the two very different response shapes, and the resource-name allow-list are all solid, and I couldn't defeat the URL guard with any of the eleven injection candidates I tried (scheme, host, port, query, fragment, percent-encoded and Unicode traversal all bounce off it). Build, go vet, staticcheck, golangci-lint, -race and the full suite are all green. Two things I'd like to see addressed before this lands, plus a handful of smaller ones inline.
The two that matter
-
A redirect off the credentials endpoint hands over the ADC token (
auth/gcp/client.go:121).oauth2.NewClientleavesCheckRedirectnil andoauth2.Transportre-signs every hop, so Go's own cross-host redirect header-stripping never applies. I reproduced it: with a genuinely cross-host redirect, a plainhttp.Clientcorrectly drops the header, but this client deliversAuthorization: Bearer <ADC cloud-platform token>to the redirect target — and then accepts that target's response body as the end-user credential, with a nil error. Acredentials:retrievecall has no reason to redirect, and it's a one-line guard. -
The
X-Goog-Api-Keymirror isn't actually tested (auth/gcp/client_test.go:406). EverywantAPIKeycase passes"X-Goog-Api-Key"as the header name, soh.Get(name)andh.Get("X-Goog-Api-Key")read the same header. I checked by deleting theWithHeaderswrapper: the suite stays green. Since that mirror is the parity behaviour the change is explicitly there to provide, it's worth a case with a genuinely different name.
Smaller things, inline: the API-key branch uses the whole returned header string as the header name; the connector's error.message skips the truncation doPost applies everywhere else; %s vs %q on the error body; the resource-validation test passing for the wrong reason; the oversize check hiding the HTTP status; and a couple of doc gaps around ctx lifetime and Config.HTTPClient.
A question rather than a finding: Config.PollTimeout's doc is explicit that it caps the retry loop and not an individual request, and points callers at ctx — which is fair and idiomatic. Still, RetrieveCredential(context.Background(), …) against a peer that accepts the POST and never answers blocks indefinitely, and the ADC-built client inherits Timeout: 0. Was leaving that entirely to the caller a deliberate call, or would you be open to context.WithDeadline(ctx, deadline) at the top of the loop so the documented bound covers the requests too? (That would also remove the extra POST the loop currently issues after sleeping to exactly the deadline.)
Two nits not worth inlining: if cfg.PollTimeout != 0 at auth/gcp/client.go:113 lets a negative duration through, which silently means "one attempt, then ErrPollTimeout" — > 0 would be safer. And a note for whoever reviews the follow-ups: everything here propagates up the stack, so the ctx-lifetime point below is worth settling at this layer.
Several candidate findings were dropped when reproduction refuted them — including a reported time.After timer leak (not a leak on Go 1.23+), and CRLF injection via the returned header name or token value (net/http rejects both at the wire).
f1d648c to
e6974fd
Compare
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Everything from the last round is in, and — more to the point — the fixes are
held down. I checked each one by deleting it and re-running the package: the
redirect guard, the token-source detachment, the X-Goog-Api-Key mirror, the
resource-name guard, the header-name rejection, the %q escaping, the
status-before-size ordering, the continueUri tag and the negative
PollTimeout all fail a test when removed. I also went back at the URL guard
with the slash cases the charset allows — //evil.example.com/x, a leading
slash, a trailing slash, a doubled inner slash — and every one of them still
lands on the configured host with only URL.Path deformed. The typed APIError
came out better than what I suggested, and it's stricter than the
agentregistry shape it copies, which still formats its body with %s.
Approving. A few small things, none of them blocking. The one I'd fix before
this merges is the ctx sentence on NewClient, inline.
One error still escapes the cap this PR applies everywhere else.
client.go:283
formats the returned header name without truncateForError. A service that
answers with a 900 KB header name produces a 900 078-byte Go error. Same shape
as the connector-message case from last round, and the same one-word fix. %q
is already there, so this is size only — the token doesn't reach the error, I
checked.
Three of the guards have no test holding them. Deleting truncateForError
from
client.go:318
or from
connector.go:46
leaves the package green, because the assertions only check that the body
contains "nope" and "boom" — TestTruncateForError pins the helper but
nothing pins its application. Removing the case <-ctx.Done() arm from
client.go:216
is also survivable: cancellation still surfaces through the next request, so the
outcome holds and only the promptness is lost, up to a whole maxBackoff. Given
two of these three are last round's fixes, an assertion on the length would stop
them quietly regressing.
NewClient(ctx, nil) is documented but never called. The nil-Config
branch at
client.go:123
backs a promise on the exported doc, and every call site in the test file passes
a non-nil &Config{}. fakeADC(t) already makes that testable in one case.
On the question I left open last time, about PollTimeout not covering the
requests themselves: it's still the case that
RetrieveCredential(context.Background(), …) against a peer that accepts the
POST and never answers blocks indefinitely, and that the loop issues one more
POST after sleeping to the deadline — two calls for a single-attempt budget. I'm
no longer asking for context.WithDeadline. doPost uses
http.NewRequestWithContext, and
client.go:105
already points callers at ctx, which is the ordinary Go contract.
One thing I got wrong last round is worth recording, though. I claimed a caller
who wants a per-request timeout has to supply Config.HTTPClient and thereby
lose ADC. That isn't true — oauth2.NewClient carries the Timeout through
from an oauth2.HTTPClient in the context, so passing one in the ctx given to
NewClient gets a bounded client and keeps ADC. I measured it. That's a better
answer than the TokenSource seam I floated, and it deserves a line on
Config.PollTimeout next to the existing advice, since nobody would guess it.
One thing to carry into #1174 rather than fix here.
credentialPayload
captures {token, header} and nothing else. That costs nothing at this layer,
which caches nothing, but a store that caches these has no expiry to evict on.
Do you know whether the services return a lifetime field? If they do, dropping
it here is the constraint the caching layer will run into, and auth.Credential
being Apply(http.Header) error means there's currently nowhere to put it.
Two smaller notes, neither worth changing on its own. The endpoint overrides at
client.go:133
aren't parsed, so an http:// value would send the ADC token in the clear — the
defaults are https and this is developer config, and a scheme check would have
to exempt loopback or it would break newTestClient and
TestNewClientRefusesRedirects, so a sentence on the field is probably the right
weight. And the two services disagree on an unrecognised 200: Agent Identity
fails fast at
agentidentity.go:48
while the connector treats it as pending at
connector.go:70
and polls to the timeout. Both are defensible, neither is pinned.
9ae62f4 to
82b3b4f
Compare
Thanks — all of the smaller items are in as of 82b3b4f. I used your deletion check on each one, since three of them were specifically about guards that nothing held down. The The uncapped header name ( The three unpinned guards. Each now fails when deleted:
The 1 KiB cap is now a package-level
Endpoint overrides — a sentence on the fields, at the weight you suggested: used as given, not parsed, so an The unrecognised 200 — both behaviours are now pinned as table cases, with a comment on why they differ: Agent Identity's result is a closed oneof, so an unknown arm can only be a mismatch worth failing on, whereas a connector operation that simply isn't done yet is normal and keeps being polled. Not changed here: granted-scope reading and retry classification, both of which stay out of this PR. On the lifetime questionYes — both services return one, so the constraint you anticipated is real, and #1174 is where it lands.
That last part settles the caching policy rather than leaving it to taste: #1174 caches only when an expiry is present, because an absent one means "unknown lifetime", and caching that would risk serving a stale credential indefinitely. On "there's currently nowhere to put it" — agreed, and that's precisely why #1174 changes One more field worth knowing about while we're in here: the same success payload carries Finally: the push dismissed your approval (this PR targets |
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).
- 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).
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.
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).
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.
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.
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.
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.
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.
- 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.
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.
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.
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.
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".
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.
82b3b4f to
4fda669
Compare
Problem
ADK-go's auth subsystem needs to fetch end-user credentials from Google Cloud's
Agent Identity and IAM Connector credential services. There are no generated Go
client libraries for these (preview) services, and adk-go has no transport for
them at all — while adk-python already talks to them.
Summary
auth/gcp, a hand-rolled REST client overnet/http(no new moduledependencies) for the Agent Identity and IAM Connector credential services.
Client.RetrieveCredential(ctx, Request)takes a resource name, routes it tothe right service (IAM Connector when the resource matches
projects/*/locations/*/connectors/*, Agent Identity otherwise — same splitas adk-python), retrieves an end-user credential, and maps the
{header, token}result to anauth.Credential: anAuthorization: Bearerheader becomes a bearer credential; any other header becomes a header-based
API key keyed by the full header string and mirrored into
X-Goog-Api-Key(matching adk-python's
_construct_auth_credential).pendingstate (bounded byConfig.PollTimeout, default 10s), and surfacesinteractive consent as
*auth.ConsentRequiredError.NewClient(ctx, *Config); a nil*Configor any zerofield uses defaults), consistent with the rest of adk-go. Calls are
authenticated with Application Default Credentials (cloud-platform scope)
unless
Config.HTTPClientis supplied; endpoints are overridable viaConfig.AgentIdentityEndpoint/Config.ConnectorEndpoint(used by tests).*APIError{StatusCode, Body}— thesame shape
agentregistryexports — so callers can tell a fatal 403 from atransient 503 with
errors.Asinstead of matching on the message.oauth2.Transportre-signs every hopbelow the layer where
net/httpstrips credentials on a cross-host redirect,so following one would hand the cloud-platform token to the redirect target
and let it dictate the returned credential.
ctxboundscredential discovery, not the client's life, so a client built inside a
request-scoped context keeps refreshing its token afterwards.
io.LimitReader(classifying thestatus first, so an oversized error page still names it); validates the
resource name against the GCP resource-name charset (and rejects
..) beforeinterpolating it into the request URL; rejects a returned header that is not a
usable HTTP field name; truncates oversized error bodies on a UTF-8 rune
boundary and escapes them into errors.
auth.CredentialProviderthat resolves the actinguser from the invocation context is a later step (feat(auth/gcp): add GCP credential provider #1173).