Skip to content

feat: gate human-in-the-loop tool confirmation on the caller principal - #7134

Open
cybrdude wants to merge 22 commits into
google:mainfrom
cybrdude:caller-principal-gate
Open

cybrdude wants to merge 22 commits into
google:mainfrom
cybrdude:caller-principal-gate

Conversation

@cybrdude

Copy link
Copy Markdown
Contributor

Follow-up to #6461, taking the direction you gave there: the principal belongs in the framework, warn plus opt-in strict now, consider flipping at the next major.

What this changes

ExecuteBashTool and every other confirmation-gated tool run on tool_confirmation.confirmed alone. Nothing upstream establishes who set it, so a remote A2A peer can approve the tool call it just caused.

The earlier attempt (cb1aceb, then 9e9eaa6, reverted by 9a32eba) keyed the guard off the presence of a2a_metadata on RunConfig.custom_metadata. That is a transport marker, and a transport is a proxy for identity. It failed in both directions: it refused authenticated peers because they arrived over A2A, and it admitted anyone reaching a path that does not stamp the marker. It also refused by returning early, which left the adk_request_confirmation call pending with nothing to resolve it and hung every HITL tool. That is what got it reverted.

This asks the authentication question instead.

Shape

CallerPrincipal (new, agents/_caller_principal.py, exported as google.adk.agents.CallerPrincipal) is frozen, extra="forbid", and carries two facts: did a serving layer authenticate the caller, and as whom.

InvocationContext.caller_principal has three meaningful states:

state meaning resultt
None no remote trust boundary was crossed (in-process Runner.run_async) allow
authenticated=False a serving edge handled the request and could not say who sent it warn by default, refuse under STRICT_CALLER_PRINCIPAL
authenticated=True the edge verified the caller allow

build_caller_principal() in the A2A request converter reads the same call_context that _get_user_id already reads, and keeps the bit _get_user_id discards. _get_user_id keeps its return contract. A caller is authenticated exactly when _get_user_id takes its call-context branch rather than the A2A_USER_ fallback, plus one extra condition: an a2a User reporting is_authenticated False is not vouched for even if it carries a name. Stricter by one condition, never looser. A parametrized test asserts the two cannot drift apart, since drift is the failure mode that produced this bug in the first place.

Both executors re-derive the principal from the RequestContext immediately after the pluggable request_converter returns, so a custom converter can neither drop it nor assert one the server never established.

explicit refusal

This is the part that matters given the last revert. A refusal rewrites the confirmation to confirmed=False rather than dropping it. confirmed=False is a state the framework already has a contract for: ExecuteBashTool returns {"error": "This tool call is rejected."} for it, and test_request_confirmation_processor_tool_not_confirmed already pins that path. So the pending call resolves with a reason the caller can see and the turn ends. Nothing hangs.

The gate runs after the consumed-confirmation dedup, so the multi-turn re-entry case that _build_consumed_dynamic_confirmation_events describes stays a no-op. There is a test pinning that specifically.

Default behavior and the strict switch

Warn and proceed by default. Strict refusal is FeatureName.STRICT_CALLER_PRINCIPAL, registered EXPERIMENTAL with default_on=False, and read through is_feature_enabled. That gives operators the three things the registry already provides for every staged behavior change: ADK_ENABLE_STRICT_CALLER_PRINCIPAL=1 in the environment, override_feature_enabled(FeatureName.STRICT_CALLER_PRINCIPAL, True) where environment variables are not practical, and a one-line default_on change when the default flips. DYNAMIC_INSTRUCTION_ROUTING and SNAKE_CASE_SKILL_NAME are the existing precedents for an off-by-default entry.

I considered and rejected two alternatives. A field on RunConfig is out because RunConfig is caller-influenceable (/run populates custom_metadata from the request body), and a security switch a caller can reach is not a switch. A bespoke environment variable, which is what the first draft of this branch used, is out because it would be a second mechanism for something the registry already does, without the programmatic override or the test scoping.

Flipping the default is deliberately not in this PR. FeatureStage.STABLE is documented as "no breaking changes without MAJOR version bump," and changing what an existing unauthenticated A2A deployment does with an approval is exactly that kind of change. The registry entry carries a comment saying the flip is the whole of the planned next-major change.

scope

  • The AgentRunRequest touched here is the A2A one in a2a/converters/request_converter.py. Nothing under src/google/adk/cli/ references it, so the /run HTTP body cannot set caller_principal.
  • The synchronous Runner.run wrapper is untouched. A direct in-process call has no serving layer to vouch for it.
  • _new_invocation_context_for_live is untouched. Live sessions get None and behave as today.
  • /run still needs real API-server auth. This is not a substitute for that and I am not claiming it is.
  • Every hunk is additive against main: 13 files, +745 / -0. The 22 commits are granular by design, including the two that apply what the pre-commit run reported, and squash cleanly.

Tests

test_request_converter.py: seven cases on build_caller_principal, including the parametrized agreement-with-_get_user_id invariant.

test_confirmation.py: absent principal, authenticated, unauthenticated under the default, unauthenticated under strict, strict against an in-process caller, and strict against multi-turn re-entry. The strict test asserts that execution is reached carrying confirmed=False, rather than asserting no events. A stall would also produce no events, so that assertion would not distinguish the fix from the bug it replaces.

…ched for

Introduces a small, frozen model that records whether the serving edge
authenticated the caller of an invocation, and who it authenticated them as.

This exists so that trust decisions downstream (notably the human-in-the-loop
tool confirmation gate) can key off an authentication fact rather than off a
transport marker. A transport is a proxy for identity, and a proxy for identity
fails in both directions: it refuses legitimate authenticated peers and it
admits anyone who can reach an ungated path.

No behavior change on its own. Wiring and the gate follow in later commits.

For google#6461.
Adds an optional CallerPrincipal to the invocation context so downstream
code can ask an authentication question instead of a transport question.

None keeps the existing meaning for every in-process caller: no remote trust
boundary was crossed, so there is nothing to vouch for. Nothing reads the
field yet.

For google#6461.
Runner.run_async takes an optional caller_principal and passes it through
both invocation setup paths into _new_invocation_context.

Keyword-only with a default, so every existing caller is unaffected and the
synchronous Runner.run wrapper stays as it is: a direct in-process call has no
serving layer to vouch for it.

For google#6461.
build_caller_principal reads the same call_context._get_user_id already
reads, and keeps the part _get_user_id throws away: whether an authenticator
produced the name, or whether it was synthesized from the caller-supplied
context id.

_get_user_id keeps its return contract. The principal rides on
AgentRunRequest, which both executors splat into Runner.run_async.

For google#6461.
A2aAgentExecutorConfig.request_converter is replaceable, so a custom
converter could otherwise drop the principal and silently reopen the hole.
Re-deriving it in the executor from the RequestContext makes it structural
rather than convention-dependent, in both directions: a custom converter can
neither remove the principal nor assert one the server never established.

For google#6461.
Same backstop as the legacy executor. Both implementations splat the
AgentRunRequest into Runner.run_async, so stamping here is what actually
reaches the invocation.

For google#6461.
The human-in-the-loop gate now asks whether the serving layer authenticated
the caller, instead of asking nothing at all.

No principal allows: no remote trust boundary was crossed. Authenticated
allows. Present-but-unauthenticated is the only refusal, and it is explicit --
the confirmation is rewritten to confirmed=False so the pending tool call
resolves as rejected, rather than dropped, which is what made the earlier
guard stall every HITL tool and get reverted.

Warn by default, strict behind ADK_STRICT_CALLER_PRINCIPAL, so upgrading
cannot break a deployment that runs A2A without an authenticator.

For google#6461.
… path

Six tests: absent principal, authenticated, unauthenticated under the default
warn behavior, unauthenticated under strict, strict against an in-process
caller, and the multi-turn re-entry case that the previous guard broke.

The strict test asserts the refusal is explicit -- execution is reached with
confirmed=False -- rather than asserting no events, which is what a stall
would also produce.

For google#6461.
The first cut keyed off is_authenticated alone and passed user_name straight
through. Two problems: user_name is typed Optional[str] on CallerPrincipal, so
any non-str value raises at construction time, and the principal could
disagree with the user id derived from the same request.

Now a caller is authenticated exactly when _get_user_id takes its
call-context branch -- a real, non-empty user name -- with one extra check
that an a2a User reporting is_authenticated False is not vouched for even if
it carries a name. Stricter by one condition, never looser, and the two
functions cannot drift apart.

For google#6461.
…ser_id

Seven cases, including a parametrized one asserting the principal is
authenticated exactly when _get_user_id takes its call-context branch. That
is the invariant that keeps the two from drifting apart again.

For google#6461.
… policy

New files under src/google/adk/ are private by default (check-new-py-prefix);
public symbols are exported through the package __init__. Renames
caller_principal.py to _caller_principal.py. The public export and the
import-site updates follow in the next commits.

Also trims one 83-column docstring line to fit the 80-column limit.

For google#6461.
The module is private by policy; this is the public surface for it, next to
InvocationContext and RunConfig, which are the two things it is used with.

For google#6461.
Follows the rename. Import order matches isort: an underscore-prefixed
module sorts before base_agent.

For google#6461.
Puts the strict switch where ADK already keeps staged behavior changes.
Operators opt in with ADK_ENABLE_STRICT_CALLER_PRINCIPAL=1 or
override_feature_enabled(FeatureName.STRICT_CALLER_PRINCIPAL, True); the
next-major flip is a one-line default_on change here.

For google#6461.
Replaces the bespoke ADK_STRICT_CALLER_PRINCIPAL env var with
is_feature_enabled(FeatureName.STRICT_CALLER_PRINCIPAL), which gives
operators the env var (ADK_ENABLE_STRICT_CALLER_PRINCIPAL), the programmatic
override, and the registry default through one mechanism the codebase
already uses for staged behavior changes.

For google#6461.
Strict mode is now enabled the way every other ADK feature is, through
ADK_ENABLE_STRICT_CALLER_PRINCIPAL. Also flattens the event-building helper
so no line exceeds 80 columns.

For google#6461.
Required for a new source unit by check-new-py-prefix. Follows the
adk-unit-guide template: get started, how it works, the three principal
states, strict mode, configuration options, limitations.

For google#6461.
Exactly what the pre-commit run reported.
Exactly what the pre-commit run reported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants