The PR grew past its purpose. What it needs to do is refuse to bootstrap an
agent network endpoint onto a cluster that cannot serve it, which is the
private capability check on the picked cluster. Everything that accreted
around it — canonicalising proxy addresses at connect, refusing another
account's cluster or a host another account pinned, withdrawing a claim
lost to a concurrent one, folding casing on migrated settings rows — is
security work in its own right and moves to follow-up PRs, where each can
be reviewed against its own threat rather than as a rider on this one.
This restores main's version of every file outside that purpose and reduces
the validation to: a cluster the account can see must have a live embedded
proxy, and a cluster management holds no row for stays pinnable
(address-first). The e2e test and the fixture seeds are unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sa3DsBDP3VciAi4PPG17L6
A cluster address is claimed two ways: an account-scoped proxy row, and an
agent network gateway pin on the address. Each side checked the other
before writing — IsClusterAddressAvailable before SaveProxy,
HasForeignAccountProxyAtHost before the settings insert — but check and
write are separate autocommit statements, so two concurrent claimants could
each pass their check and both commit, leaving a pin no proxy will ever
serve next to the proxy row that displaces it.
Both sides now re-read after they write. Manager.Connect re-asks
availability once the proxy row is committed and, if the address is no
longer free or the answer is inconclusive, deletes its own row and returns
ErrClusterAddressUnavailable, which the connect path reports as
AlreadyExists exactly as the pre-write check would have. bootstrapLabeled
re-asks ownership once the settings row is committed and withdraws the pin
on the same terms. Because both write before they re-read, of two
concurrent claimants at least one re-reads after the other has committed
and backs off — on sqlite, postgres and mysql alike, since each statement
sees every commit before it. Both may back off, which costs a retry;
neither keeps a claim the other holds.
No lock spans the proxies and settings tables portably, and a claims table
would be more machinery than the property needs, so the re-read is the
whole mechanism. DeleteProxy is session-guarded like DisconnectProxy, so a
stale session withdrawing itself cannot take out a newer session's row.
Reported by CodeRabbit on #7402 (CWE-362).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sa3DsBDP3VciAi4PPG17L6
At proxy connect time, the declared cluster address is validated for
shape and checked for availability (`IsClusterAddressAvailable`), and
from then on the declaration is what routes the cluster's mappings to
the connection. Deployments that embed management through the
integrations seam may need a policy on that claim — deciding which
credential is allowed to declare which address.
This adds an optional `ProxyConnectAuthorizer` hook on
`ProxyServiceServer`, following the pattern of the existing `Set*` seams
(`SetServiceManager`, `SetAgentNetworkSynthesizer`,
`SetAgentNetworkLimitsService`, `SetProxyController`):
- A nil-able interface field plus `SetProxyConnectAuthorizer`, guarded
by the existing mutex.
- One call at the end of `validateProxyConnect`, so both
`GetMappingUpdate` and `SyncMappings` are covered by a single site.
- **Nothing installs it by default** — with the hook unset (always, in
this repo), behavior is byte-for-byte unchanged, which the tests pin.
Design details:
- The authorizer runs **last** — after input validation and the
availability check — and **outside** the account-scoped branch, so
management-wide tokens and token-less connects are also presented to it
rather than bypassing policy.
- The authorizer receives the presented `*types.ProxyAccessToken` (nil
when none), the proxy ID, and the declared address. Everything it needs
is already in the request/context; no proto or schema change.
- A plain error from the authorizer surfaces as `PermissionDenied`,
keeping an authorization rejection distinguishable from the
`AlreadyExists` used for address conflicts in proxy logs. A status error
passes through unchanged so implementations can pick their own code.
