Fix GCS bucket-wide permissions vulnerability using Signed URLs#276
Fix GCS bucket-wide permissions vulnerability using Signed URLs#276jeffcarp wants to merge 1 commit into
Conversation
Address a security vulnerability where Kinetic worker pods received
bucket-wide storage.objectAdmin permissions on the shared jobs bucket,
potentially allowing cross-job data leakage.
We resolve this by implementing a secure GSA split and client-side
Signed URL generation:
1. GSA Split & Least Privilege (program.py):
- Split the single GSA into 4 specialized accounts:
- GKE Nodes GSA: Zero GCS access (only logging/monitoring/AR).
- Worker GSA: Workload Identity bound, restricted via GCS IAM
Conditions to only read from `default/data-cache/*` and
`default/data-markers/*` (protecting FUSE/Data API).
- Signer GSA: Has `objectAdmin` on the jobs bucket. Impersonatable
by the deployer via `roles/iam.serviceAccountTokenCreator`.
- Build GSA: Cloud Build execution (unchanged).
2. Client-Side Signed URLs (storage.py, execution.py):
- Client impersonates the Signer GSA at runtime to generate v4
Signed URLs (7-day expiry) for transient job artifacts (payload,
context, result, requirements, debug sentinels).
- Wrapped in a graceful fallback: if Signer GSA is missing or
impersonation fails, we fall back to legacy `gs://` URIs, ensuring
backward compatibility with un-upgraded clusters.
3. Backend & Runner Updates (gke_client.py, pathways_client.py, remote_runner.py):
- GKE and Pathways backends pass Signed URLs to the container.
- Remote runner supports downloading/uploading via HTTP(S) using
these Signed URLs, requiring zero GCS credentials inside the pod.
- Sentinel operations (debug-ready, leader-ready) updated to use
HTTP PUT/GET against Signed URLs when available.
All changes verified via comprehensive unit tests in program_test.py,
storage_test.py, and remote_runner_test.py.
TAG=agy
CONV=745a5171-9f28-459e-abd5-bb328498b864
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
=======================================
Coverage ? 91.62%
=======================================
Files ? 82
Lines ? 12668
Branches ? 0
=======================================
Hits ? 11607
Misses ? 1061
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements GCS Signed URLs for secure job isolation, restricting GKE workload pods from having broad GCS access by splitting service accounts into Node, Worker, and Signer SAs. The runner is updated to download and upload artifacts using these signed URLs. Feedback on the changes highlights three key improvements: replacing the fragile urllib.request.urlretrieve with a more robust urlopen implementation to handle HTTP errors and timeouts, reducing the signed URL expiration to 12 hours to avoid SignatureDoesNotMatch errors caused by GCP key rotation, and dynamically prefixing the deployer email with user: or serviceAccount: to support automated CI/CD environments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if gcs_path.startswith("http://") or gcs_path.startswith("https://"): | ||
| logging.info("Downloading from Signed URL: %s", gcs_path) | ||
| urllib.request.urlretrieve(gcs_path, local_path) | ||
| return |
There was a problem hiding this comment.
Using urllib.request.urlretrieve is fragile because it does not raise exceptions for non-2xx HTTP status codes (such as 403 Forbidden or 404 Not Found). Instead, it will write the error response body (e.g., GCS XML error pages) directly to the local file path, which will cause confusing failures (like BadZipFile or UnpicklingError) later in the execution. Additionally, urlretrieve does not support a timeout parameter, which can cause the runner to hang indefinitely on network issues. Replacing it with urllib.request.urlopen and shutil.copyfileobj resolves both issues by raising an HTTPError on failure and allowing a proper timeout.
| if gcs_path.startswith("http://") or gcs_path.startswith("https://"): | |
| logging.info("Downloading from Signed URL: %s", gcs_path) | |
| urllib.request.urlretrieve(gcs_path, local_path) | |
| return | |
| if gcs_path.startswith("http://") or gcs_path.startswith("https://"): | |
| logging.info("Downloading from Signed URL: %s", gcs_path) | |
| req = urllib.request.Request(gcs_path, method="GET") | |
| with urllib.request.urlopen(req, timeout=60) as response: | |
| with open(local_path, "wb") as f: | |
| shutil.copyfileobj(response, f) | |
| return |
References
- Poke Holes in the Implementation: Actively search for and point out failing edge cases, race conditions, or unhandled exceptions in the implementation. (link)
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it. (link)
| bucket = client.bucket(bucket_name) | ||
|
|
||
| # 4. Generate signed URLs | ||
| expiration = datetime.timedelta(days=7) |
There was a problem hiding this comment.
When generating GCS Signed URLs using impersonated credentials (which relies on the IAM Credentials signBlob API under the hood), Google rotates the signing keys approximately every 12 hours. Consequently, any signed URL generated with an expiration greater than 12 hours (such as days=7) will stop working after 12 hours, leading to SignatureDoesNotMatch errors at runtime. Since these are transient job artifacts, reducing the expiration to hours=12 ensures correctness and aligns with GCP's platform limitations.
| expiration = datetime.timedelta(days=7) | |
| expiration = datetime.timedelta(hours=12) |
References
- Poke Holes in the Implementation: Actively search for and point out failing edge cases, race conditions, or unhandled exceptions in the implementation. (link)
| "deployer-impersonate-signer", | ||
| service_account_id=signer_sa.name, | ||
| role="roles/iam.serviceAccountTokenCreator", | ||
| member=pulumi.Output.format("user:{0}", deployer_email), |
There was a problem hiding this comment.
Hardcoding the user: prefix for the deployer email assumes that the deployer is always a user account. However, in automated environments, CI/CD pipelines, or Pulumi Cloud runs, the deployer is often a service account. Hardcoding user: will cause GKE/GCP IAM to reject the policy with an invalid member error. We should dynamically prefix with serviceAccount: or user: depending on the email domain.
member=pulumi.Output.from_input(deployer_email).apply(
lambda email: f"serviceAccount:{email}" if email.endswith(".gserviceaccount.com") else f"user:{email}"
),References
- Demand Robustness: Do not accept fragile code. If the proposed code is not robust enough or lacks proper error handling, explicitly tell the author why the current approach is brittle and what must be done to reinforce it. (link)
2 participants
By default Kinetic runners have GCS-wide access to the shared jobs bucket, even to the payloads of other user's jobs. This may result in exposing env vars or other secrets. This PR adds Signed GCS URLs to ensure access is only allowed by the uploading user.
This does require each user to run
kinetic upagain on their cluster to upgrade the configuration. The code gracefully falls back to the old read/write method for clusters that haven't upgraded yet.We resolve this by implementing a secure GSA split and client-side Signed URL generation:
GSA Split & Least Privilege (program.py):
default/data-cache/*anddefault/data-markers/*(protecting FUSE/Data API). - Signer GSA: HasobjectAdminon the jobs bucket. Impersonatable by the deployer viaroles/iam.serviceAccountTokenCreator. - Build GSA: Cloud Build execution (unchanged).Client-Side Signed URLs (storage.py, execution.py):
gs://URIs, ensuring backward compatibility with un-upgraded clusters.Backend & Runner Updates (gke_client.py, pathways_client.py, remote_runner.py):
All changes verified via comprehensive unit tests in program_test.py, storage_test.py, and remote_runner_test.py.
TAG=agy
CONV=745a5171-9f28-459e-abd5-bb328498b864
Description
Contributor Agreement
Please check all boxes below before submitting your PR for review: