Repository navigation
Conversation
…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.
Follows the rename. For google#6461.
Follows the rename. 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.
main now runs a root LlmAgent through the node runtime, which builds its own invocation context. Thread caller_principal through _run_node_async into _node_runner_utils.run_node_async and set it on the context after construction, the way abort_signal is, so a principal set by a serving layer reaches the confirmation gate on that path too. Without this the gate saw None (in-process caller) for every A2A-served root agent.
…de path Fails without the previous commit: the principal arrived as None on the node runtime path, which is where every root LlmAgent runs now.
|
@xuanyang15 Merged main (through f44d512, as 96c13a2) and resolved the two conflicts: Two things worth a look beyond the merge:
Locally on this head: |
|
Once this picks up main, workflow On I ran it with this PR merged onto Would you rather cover ToolNode here, or keep it as a follow-up and narrow the guide's "Only the tool confirmation flow reads the principal today"? Happy to add the repro as a test either way. |
Review found a second consumer of human-in-the-loop answers, the workflow tool node, that the gate in the agent confirmation flow did not cover. The decision moves next to CallerPrincipal so both consumers refuse the same callers.
No behaviour change on this path; the gate keeps rewriting refused confirmations to confirmed=False and now shares the decision with the tool node.
…pal (3/5) _ToolNode._apply_confirmation_resume (ce132b9) stored ctx.resume_inputs on ctx.tool_confirmation with no principal check, and apply_confirmation_gate is a no-op for tools whose check_require_confirmation is False yet read tool_confirmation themselves, ExecuteBashTool included. With strict mode on, an unauthenticated caller could resume a paused tool node with confirmed=True and the tool ran. The answer is now rewritten to confirmed=False before it is stored, so every reader sees the refusal.
…callers (4/5) Covers the require_confirmation path and the request_confirmation path on the tool node in strict mode, the default-mode warning, and an authenticated caller as control. All six fail on the tree without the tool node change.
The guide said only the tool confirmation flow reads the principal; it now lists the LlmAgent confirmation flow and the workflow tool node resume, and notes that tools reading tool_confirmation directly see the refusal.
|
@Santoshkumarpuppala you are right, and thank you for running it. Covered here rather than in a follow-up: the decision now lives in caller_may_confirm next to CallerPrincipal (0fc5ab2), the agent flow delegates to it (51775f4), and _ToolNode._apply_confirmation_resume applies it before the answer is stored, so tools that read tool_confirmation themselves see confirmed=False (e95b1ad). Tests in 4ac01e9 are your repro on both tool node paths (require_confirmation and request_confirmation), strict and default mode, with an authenticated caller as control; all fail on the tree without e95b1ad. Guide updated in 1d39ebb to name both paths. Branch is merged with main at 86a47f6. If you want to push your repro as a test on top, go ahead and I will keep it. Team, please add @Santoshkumarpuppala as co-contributor |
3 participants
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
ExecuteBashTooland every other confirmation-gated tool run ontool_confirmation.confirmedalone. Nothing upstream establishes who set it, so a remote A2A peer can approve the tool call it just caused.The earlier attempt (
cb1aceb, then9e9eaa6, reverted by9a32eba) keyed the guard off the presence ofa2a_metadataonRunConfig.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 theadk_request_confirmationcall 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 asgoogle.adk.agents.CallerPrincipal) is frozen,extra="forbid", and carries two facts: did a serving layer authenticate the caller, and as whom.InvocationContext.caller_principalhas three meaningful states:NoneRunner.run_async)authenticated=FalseSTRICT_CALLER_PRINCIPALauthenticated=Truebuild_caller_principal()in the A2A request converter reads the samecall_contextthat_get_user_idalready reads, and keeps the bit_get_user_iddiscards._get_user_idkeeps its return contract. A caller is authenticated exactly when_get_user_idtakes its call-context branch rather than theA2A_USER_fallback, plus one extra condition: an a2aUserreportingis_authenticatedFalse 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
RequestContextimmediately after the pluggablerequest_converterreturns, 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=Falserather than dropping it.confirmed=Falseis a state the framework already has a contract for:ExecuteBashToolreturns{"error": "This tool call is rejected."}for it, andtest_request_confirmation_processor_tool_not_confirmedalready 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_eventsdescribes 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, registeredEXPERIMENTALwithdefault_on=False, and read throughis_feature_enabled. That gives operators the three things the registry already provides for every staged behavior change:ADK_ENABLE_STRICT_CALLER_PRINCIPAL=1in the environment,override_feature_enabled(FeatureName.STRICT_CALLER_PRINCIPAL, True)where environment variables are not practical, and a one-linedefault_onchange when the default flips.DYNAMIC_INSTRUCTION_ROUTINGandSNAKE_CASE_SKILL_NAMEare the existing precedents for an off-by-default entry.I considered and rejected two alternatives. A field on
RunConfigis out becauseRunConfigis caller-influenceable (/runpopulatescustom_metadatafrom 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.STABLEis 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
AgentRunRequesttouched here is the A2A one ina2a/converters/request_converter.py. Nothing undersrc/google/adk/cli/references it, so the/runHTTP body cannot setcaller_principal.Runner.runwrapper is untouched. A direct in-process call has no serving layer to vouch for it._new_invocation_context_for_liveis untouched. Live sessions getNoneand behave as today./runstill needs real API-server auth. This is not a substitute for that and I am not claiming it is.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 onbuild_caller_principal, including the parametrized agreement-with-_get_user_idinvariant.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 carryingconfirmed=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.