Reject untrusted pool spec.apiUrl (SSRF / secret exfiltration) - #1
Closed
staging-devin-ai-integration[bot] wants to merge 17 commits into
Closed
staging-devin-ai-integration[bot] wants to merge 17 commits into
staging-devin-ai-integration[bot] wants to merge 17 commits into
Conversation
…ocs) Co-Authored-By: Nikhil Jha <nikhil@cognition.ai>
…er election The scaffold's contract predated `devin worker start`'s direct-serve mode (DEVIN_REMOTE_SESSION_TOKEN skips the queue API entirely), so worker pods now hold only a per-session connect token — claim/renew/release all happen centrally in the operator. Connect tokens now outlive the max session lifetime (30d), so token secrets are written once at claim time. - api: full /opbeta/outposts client (list/claim/release + SSE watch with cursor resume), wire types matched to the router's actual shapes - controller: level-based reconcile edge-triggered by per-pool SSE watchers, pure planner for the session<->pod lifecycle, finalizer releasing claims on pool deletion, acceptor-ID ConfigMap, status + metrics - snapshot: FilesystemSnapshot = retained per-session PVC at the worker data dir; GkeSnapshot = podsnapshot.gke.io/v1 CRDs (manual trigger per suspend, snapshots grouped by session label); pods survive until snapshots are Ready - elector: Lease-based leader election, so replicaCount > 1 is now allowed (failover must stay inside the ~5min claim TTL) - tests: in-memory mock of the queue API with faithful CAS/lazy-expiry/ keyset-cursor/SSE semantics, plus planner/pod-merge/config unit tests Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
First CLI release carrying the worker direct-serve mode the operator targets. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
kube pulls in ring while reqwest 0.13 pulls in aws-lc-rs, so rustls cannot pick a default on its own and panics at first TLS use. Also move the CRD into the chart's crds/ directory: as a template it was applied too late for a fresh `helm install` with defaultPool.enabled to validate the OutpostPool resource. Helm never upgrades crds/, so CRD changes now need a manual kubectl apply on operator upgrades. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
PodSnapshotManualTrigger reports the created snapshot as status.snapshotCreated.name (observed on GKE 1.36), not status.snapshotName; polling the wrong field left suspends stuck in-progress until the give-up timeout. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
Observed on a real cluster: post-checkpoint (`postCheckpoint: stop`) the kubelet restarts the stopped gVisor container under OnFailure, so pods churn while suspended, and every new pod incarnation fired a fresh manual trigger — whose (even unfinished) snapshot pruned the good one via maxSnapshotCountPerGroup: 1. - treat an existing Ready snapshot for the session as done before considering any trigger - only fire triggers at running, non-terminating pods; if the worker is already gone, suspend without a snapshot instead of holding the claim - failed triggers (Triggered=False, e.g. "context deadline exceeded") are spent: delete + retry while the pod lives, abandon otherwise Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
Suspended sessions can be tombstoned by the sweeper without the operator ever observing session_status=terminated, leaking their snapshot artifacts until pool deletion. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
Releasing a suspended session leaves its phase=pending row in the queue until the sweeper tombstones it; treating that row as claimable made the operator flap suspend->release->claim, recreating and deleting the worker every pass. Claim only rows whose session_status is pending. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
The previous fix excluded every non-pending session_status from claiming, but the brain enqueues the queue row at session start, so an unclaimed fresh row's session is already *running*. Only terminal states (terminated/suspended leftovers awaiting the sweeper) are excluded now. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
GKE restore is best-effort with a silent cold-start fallback, and the node's pod-snapshot agent (a DaemonSet) races workload pods onto freshly scaled-up nodes — a resume that triggers a scale-up usually starts before the agent and silently loses its memory state. There is no platform barrier; Google's own agent-sandbox client handles this the same way: pin the exact snapshot and verify the restore. - prepare() now pins the session's latest Ready snapshot onto the pod via podsnapshot.gke.io/ps-name (and can return provider-owned pod annotations generally) - new SnapshotProvider::verify_restore judges the pod's PodRestored condition against the pin - the reconciler recycles a cold-started pod once (marker on the token secret, which lives exactly one claim cycle); by the retry the node's agent is up, so this also holds under many simultaneous resumes Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
The already-durable check treated the snapshot the pod was restored from as satisfying the next suspend, skipping the new checkpoint and losing everything since the resume. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
Observed live: an ek-family checkpoint restored onto a different node crash-looped with "OCI runtime restore failed: incompatible FeatureSet" (pin your workers to one machine family via a custom ComputeClass to avoid this — see the GKE docs' non-E2 guidance). The operator now converts such crash loops into one unpinned retry (deliberate cold start) instead of endlessly giving the session back, while the agent-race recycle keeps its pin. Deleting snapshots also no longer counts them as Ready while their finalizers run. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Staging-Devin AI <166158716+staging-devin-ai-integration[bot]@users.noreply.github.com>
Contributor
Author
|
Prompt hidden (unlisted session) |
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
⚙️ Control Options:
|
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.
Summary
spec.apiUrlwas taken verbatim from theOutpostPooland paired with the value of whateverSecretthe pool named inspec.tokenSecretRef, which the operator reads with its cluster-wide-read ServiceAccount. Anyone who could create anOutpostPooltherefore made the operator a confused deputy: pointapiUrlat their own host, reference any Secret in the namespace, and the first reconcile ships that Secret's plaintext value to them in theAuthorization: Bearerheader of the list/watch/claim/release calls (plus arbitrary SSRF from inside the cluster network).The operator now only talks to URLs it trusts:
apply()andcleanup()callresolve_api_urlbeforepool_token(), so a pool naming an untrusted URL never even triggers a Secret read (and no request is made to that host, not even a release during deletion); the pool goesDegradedwithApiUrlNotAllowedon its Ready condition. Exact-match against operator config is deliberate — it also kills the usual bypass classes (scheme downgrade, suffix hosts likeapi.devin.ai.attacker.tld, userinfo@, metadata IPs), which are covered in the newresolve_api_url_accepts_only_trusted_urlstest.Default behaviour is unchanged for pools that omit
apiUrlor set it to the operator's default. A pool that legitimately needs another endpoint requires a cluster admin to add it to the chart's newoperator.apiUrlAllowlist(rendered intoDEVIN_API_URL_ALLOWLIST), which is exactly the admin approval the vulnerability was missing.Note: the finding's other suggestions (constraining which Secrets a pool may reference, narrowing the operator's secret-read RBAC) are not in this PR — with the URL pinned to operator config, the token can only ever go to a trusted Devin endpoint, so the exfiltration path is closed; happy to follow up on those as defense in depth.
I could not compile or run the test suite locally: this session's network policy only allows the git manager, so the pinned toolchain and the crates proxy (
pkg.cognition.build) are unreachable.cargo fmtwas run; CI covers clippy/check/test and the CRD-up-to-date diff (the regeneratedapiUrldescription was hand-edited to match the doc comment sincecrdgenalso needs to build).Link to Devin session: https://staging.itsdev.in/sessions/988d59cf133b405aa98b86a2a854daad
Devin Review