[feat] Deliver agent credentials through Daytona Secrets#5242
[feat] Deliver agent credentials through Daytona Secrets#5242mmabrouk wants to merge 46 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| }; | ||
| return { | ||
| prepare, | ||
| async prepareForCreate() { |
There was a problem hiding this comment.
Review focus: a cached active lease is valid only for a successful reconnect. Any fresh create first retires and cleans that lease, then reserves/provisions a new attempt. This closes the reviewed failure path where reconnect failed, activation targeted an already-active lease, and the newly created sandbox could leak.
| AgentSecretLeaseDBE.owner_kind == query.owner.kind.value, | ||
| AgentSecretLeaseDBE.owner_id == query.owner.id, | ||
| ) | ||
| if query.windowing.next: |
There was a problem hiding this comment.
Review focus: retry time is only an eligibility filter. Page traversal uses immutable (created_at, id), so recordRetry cannot move a row across the cursor during a janitor sweep. The migration and DBE indexes use the same tuple.
| Pass an existing --env-file (e.g. --env-file .env.$LICENSE.$STAGE.local) or create the file." | ||
| fi | ||
|
|
||
| # Never place runner control material in the shared service env file: every service |
There was a problem hiding this comment.
Review focus: the shared Compose env file is injected into many services, so runner control material is rejected there. The dedicated second env file is interpolation-only; explicit Compose mappings give the control token to API+runner and the epoch HMAC key to runner only. This boundary was also verified inside the live containers.
| return {"kind": self.kind, "name": self.name} | ||
|
|
||
|
|
||
| class ResolvedCredential(BaseModel): |
There was a problem hiding this comment.
Review focus: every resolved secret is classified by binding and usage. opaque_http credentials must go through Daytona Secrets on remote runs; local_use remains the explicit non-isolated exception. The value is excluded from repr, redaction is seeded before dispatch, and no delivery-policy field is exposed publicly.
|
This is a second implementation version. Here is what changed after the independent security review:
Suggested review order: typed SDK contract, runner plan/provider lifecycle, API lease state machine and DAO, then deployment wiring. I added inline comments at the four highest-risk boundaries. One manual release gate remains intentionally open: a credit-consuming Daytona sandbox run needs explicit credit approval. The draft includes exact QA steps for that run. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdks/python/agenta/sdk/redaction/context.py (1)
19-26: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't cache the fallback redactor in the ContextVar
Redactoris mutable (_known,_bounded,_sorted_values), and the SDK call sites here don’t enterredaction_context(). On the first miss,get_active_redactor()stores a no-op instance in the current context, so laterwith_known_secrets()calls can keep accumulating state across logical runs in reused tasks. Return a fresh fallback without setting it, or make sure every request/job path is wrapped inredaction_context().sdks/python/agenta/sdk/agents/connections/resolver.py (1)
131-143: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSilent credential drop when provider has no resolvable env var.
If
self._api_keyis set butself._env_varis unset andproviderisn't in_PROVIDER_ENV_VARS,envstays empty and the new call reportscredential_mode="runtime_provided"— misrepresenting an explicitly-supplied credential as "let the harness handle its own login." The key is silently dropped with no error.🐛 Proposed fix
provider = self._provider or model.provider or "" env: Dict[str, str] = {} if self._api_key: env_var = self._env_var or _PROVIDER_ENV_VARS.get(provider.lower()) - if env_var: - env[env_var] = self._api_key + if not env_var: + raise ValueError( + f"cannot resolve an env var for provider {provider!r}; " + "pass env_var explicitly to StaticConnectionResolver" + ) + env[env_var] = self._api_key endpoint = Endpoint(base_url=self._base_url) if self._base_url else None
🧹 Nitpick comments (10)
services/runner/package.json (1)
25-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
@daytona/sdkinstead of the deprecated package
@daytonaio/sdk@0.196.0exists, but it’s marked as moved to@daytona/sdkwith the same API. Switch this dependency to the new package name.api/oss/tests/pytest/unit/agent_secret_leases/test_service.py (1)
253-287: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a negative test for the "one child CAS update at a time" invariant.
This test only exercises the happy path (a single
resource_updatesentry per in-phase transition). Given the test's own name emphasizes updating "one child... at a time," and the PR objectives call out claim-consistency/fencing as a specific concern, a companion negative case (submitting tworesource_updatesin a singlebeginProvisioning/beginCleanupcall and asserting it's rejected) would confirm the invariant is actually enforced rather than just exercised by omission.services/runner/tests/unit/session-pool.test.ts (1)
464-517: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a test for
transientCredentialsHash
This block only asserts.sandboxCredentialsHash. Add a case that changestoolCallback.authorizationand assertstransientCredentialsHashchanges independently ofsandboxCredentialsHash.services/runner/tests/unit/daytona-secret-plan.test.ts (1)
2-2: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConfirm
exactHttpsHostisn't a second, divergent SSRF guard.This suite imports
exactHttpsHostdirectly fromdaytona-secret-plan.ts, separate from the dedicatedssrf-guard.ts/ssrf-guard.test.tsin this same PR. Worth confirming these share one implementation (or at least an equivalent blocklist) rather than two independently-maintained SSRF checks that can drift — e.g. this list omits the cloud-metadata address (169.254.169.254) and RFC1918 private ranges thatssrf-guardpresumably covers.Also applies to: 10-10
services/runner/src/engines/sandbox_agent/daemon.ts (1)
121-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent field name in the same doc block.
Line 125 was updated to reference
plan.modelEnvironment, but line 124 (two lines above) still says "the resolvedsecretsare the sole authority." Mixed terminology in a security-critical "clear-then-apply" doc block can mislead future maintainers about which field is actually authoritative.✏️ Suggested wording fix
* Clear-then-apply (Security rule 5): on a MANAGED run (`credentialMode === "env"`) the - * resolved `secrets` are the sole authority, so the daemon must NOT inherit the sidecar's own + * resolved `plan.modelEnvironment` is the sole authority, so the daemon must NOT inherit the sidecar's own * provider keys (the caller applies only `plan.modelEnvironment`). When true, no `KNOWN_PROVIDER_ENV_VARS`api/oss/src/dbs/postgres/agent_secret_leases/dbes.py (1)
102-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winName the
sandbox_labeluniqueness constraint explicitlysandbox_labelis still usingunique=True; switch it to a namedUniqueConstraintso the schema object stays stable and matches the rest of this file.services/runner/src/engines/sandbox_agent/daytona-secret-plan.ts (1)
125-131: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider canonicalizing key order before hashing for a truly stable epoch marker.
credentialEpochHmacrelies onJSON.stringifyover objects (consumer,binding) whose key order is only stable because construction is currently consistent; it isn't explicitly canonicalized. Since this hash drives rotation/cleanup decisions ("Deterministic Daytona Secret creation" per PR objectives), an incidental change in upstream key ordering would spuriously change the digest even for semantically identical plans. Building explicit ordered tuples (or sorting object keys) before stringifying would make the guarantee explicit rather than incidental.api/oss/src/core/agent_secret_leases/dtos.py (1)
55-60: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTighten
ResourceBinding.namenormalization beyond "no control chars/whitespace".The check
any(ord(char) < 33 for char in value)only excludes control characters and the space character; it still admits characters such as=,;,$, backslashes, etc. Given binding names are used to build environment-variable/secret keys (e.g.OPENAI_API_KEY) and feedprovider_secret_namegeneration, a stricter allow-list (e.g.[A-Za-z0-9_.\-]+, or a kind-specific pattern perBindingKind) would prevent malformed identifiers from propagating into Daytona secret/env names.♻️ Proposed fix
+import re + `@field_validator`("name") `@classmethod` def validate_name(cls, value: str) -> str: - if value != value.strip() or any(ord(char) < 33 for char in value): + if not re.fullmatch(r"[A-Za-z0-9_.\-]+", value): raise ValueError("binding name must be normalized") return valueapi/oss/src/apis/fastapi/agent_secret_leases/models.py (1)
106-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate consumer-key validation logic across API and core layers.
ConsumerResponse.validate_keymirrorsResourceConsumer.validate_keyinapi/oss/src/core/agent_secret_leases/dtos.pyalmost exactly. Consider extracting the MODEL/HTTP_MCP key-invariant check into a shared helper (e.g. intypes.py) that both models call, to avoid the two copies drifting apart over time.api/oss/src/apis/fastapi/agent_secret_leases/utils.py (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the runner-control header constant.
CONTROL_HEADERhere and_RUNNER_CONTROL_HEADERinapi/oss/src/middlewares/auth.pyare two independent copies of the same literal ("X-Agenta-Runner-Control-Token"). A future rename of one without the other silently breaks the control-token auth path (middleware bypass and/or this dependency stop agreeing on the header name), with nothing to catch the drift.Extract this into a single shared constant (e.g. alongside
RunnerConfiginapi/oss/src/utils/env.py, or a small shared constants module) and import it from both places.♻️ Proposed fix
-CONTROL_HEADER = "X-Agenta-Runner-Control-Token" +from oss.src.utils.env import RUNNER_CONTROL_HEADER as CONTROL_HEADER(and update
api/oss/src/middlewares/auth.pyto import the same constant instead of defining_RUNNER_CONTROL_HEADERlocally)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f51de113-36a4-4f2f-b4f9-5679792236d7
⛔ Files ignored due to path filters (1)
services/runner/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (96)
api/entrypoints/routers.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000011_add_agent_secret_leases.pyapi/oss/src/apis/fastapi/agent_secret_leases/__init__.pyapi/oss/src/apis/fastapi/agent_secret_leases/models.pyapi/oss/src/apis/fastapi/agent_secret_leases/router.pyapi/oss/src/apis/fastapi/agent_secret_leases/utils.pyapi/oss/src/core/agent_secret_leases/__init__.pyapi/oss/src/core/agent_secret_leases/dtos.pyapi/oss/src/core/agent_secret_leases/interfaces.pyapi/oss/src/core/agent_secret_leases/service.pyapi/oss/src/core/agent_secret_leases/types.pyapi/oss/src/dbs/postgres/agent_secret_leases/__init__.pyapi/oss/src/dbs/postgres/agent_secret_leases/dao.pyapi/oss/src/dbs/postgres/agent_secret_leases/dbes.pyapi/oss/src/dbs/postgres/agent_secret_leases/mappings.pyapi/oss/src/middlewares/auth.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/unit/agent_secret_leases/__init__.pyapi/oss/tests/pytest/unit/agent_secret_leases/fixtures/secret_lease_wire.jsonapi/oss/tests/pytest/unit/agent_secret_leases/test_auth_cursor.pyapi/oss/tests/pytest/unit/agent_secret_leases/test_models.pyapi/oss/tests/pytest/unit/agent_secret_leases/test_schema.pyapi/oss/tests/pytest/unit/agent_secret_leases/test_service.pyhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.local.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.local.ymlhosting/docker-compose/oss/docker-compose.gh.ssl.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/docker-compose/run.shhosting/docker-compose/runner-secrets.env.examplehosting/kubernetes/helm/templates/api-deployment.yamlhosting/kubernetes/helm/templates/runner-deployment.yamlhosting/kubernetes/helm/templates/secrets.yamlhosting/kubernetes/helm/values.schema.jsonhosting/kubernetes/helm/values.yamlhosting/railway/oss/scripts/configure.shsdks/python/agenta/sdk/agents/adapters/sandbox_agent.pysdks/python/agenta/sdk/agents/connections/__init__.pysdks/python/agenta/sdk/agents/connections/endpoints.pysdks/python/agenta/sdk/agents/connections/errors.pysdks/python/agenta/sdk/agents/connections/models.pysdks/python/agenta/sdk/agents/connections/resolver.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/handler.pysdks/python/agenta/sdk/agents/interfaces.pysdks/python/agenta/sdk/agents/mcp/__init__.pysdks/python/agenta/sdk/agents/mcp/models.pysdks/python/agenta/sdk/agents/mcp/resolver.pysdks/python/agenta/sdk/agents/platform/connections.pysdks/python/agenta/sdk/agents/utils/wire.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/agenta/sdk/redaction/context.pysdks/python/agenta/sdk/redaction/seed.pysdks/python/oss/tests/pytest/unit/agents/connections/test_dtos_model_ref.pysdks/python/oss/tests/pytest/unit/agents/connections/test_models.pysdks/python/oss/tests/pytest/unit/agents/connections/test_resolver.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.claude.jsonsdks/python/oss/tests/pytest/unit/agents/golden/run_request.pi_core.jsonsdks/python/oss/tests/pytest/unit/agents/mcp/test_resolver.pysdks/python/oss/tests/pytest/unit/agents/platform/test_connections_http.pysdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pysdks/python/oss/tests/pytest/unit/agents/test_wire_models.pyservices/runner/package.jsonservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/daemon.tsservices/runner/src/engines/sandbox_agent/daytona-secret-janitor.tsservices/runner/src/engines/sandbox_agent/daytona-secret-plan.tsservices/runner/src/engines/sandbox_agent/daytona-secret-provider.tsservices/runner/src/engines/sandbox_agent/daytona-secrets.tsservices/runner/src/engines/sandbox_agent/errors.tsservices/runner/src/engines/sandbox_agent/mcp.tsservices/runner/src/engines/sandbox_agent/pause.tsservices/runner/src/engines/sandbox_agent/provider.tsservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/src/engines/sandbox_agent/secret-lease-control.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/protocol.tsservices/runner/src/server.tsservices/runner/tests/unit/daytona-secret-janitor.test.tsservices/runner/tests/unit/daytona-secret-plan.test.tsservices/runner/tests/unit/daytona-secret-provider.test.tsservices/runner/tests/unit/daytona-secrets.test.tsservices/runner/tests/unit/mcp-servers.test.tsservices/runner/tests/unit/sandbox-agent-daemon.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsservices/runner/tests/unit/secret-lease-control.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-mcp-layering.test.tsservices/runner/tests/unit/session-pool.test.tsservices/runner/tests/unit/ssrf-guard.test.tsservices/runner/tests/unit/wire-contract.test.ts
💤 Files with no reviewable changes (1)
- sdks/python/agenta/sdk/agents/adapters/sandbox_agent.py
| _ALLOWED_SOURCE_STATES = { | ||
| LeaseTransition.BEGIN_PROVISIONING: { | ||
| LeaseState.RESERVED, | ||
| LeaseState.PROVISIONING, | ||
| }, | ||
| LeaseTransition.RECORD_SANDBOX: {LeaseState.PROVISIONING}, | ||
| LeaseTransition.ACTIVATE: {LeaseState.PROVISIONING}, | ||
| LeaseTransition.REQUEST_CLEANUP: { | ||
| LeaseState.RESERVED, | ||
| LeaseState.PROVISIONING, | ||
| LeaseState.ACTIVE, | ||
| LeaseState.CLEANING, | ||
| }, | ||
| LeaseTransition.BEGIN_CLEANUP: { | ||
| LeaseState.CLEANUP_PENDING, | ||
| LeaseState.CLEANING, | ||
| }, | ||
| LeaseTransition.RECORD_RETRY: {LeaseState.CLEANING}, | ||
| LeaseTransition.MARK_DELETED: {LeaseState.CLEANING}, | ||
| LeaseTransition.QUARANTINE: { | ||
| LeaseState.RESERVED, | ||
| LeaseState.PROVISIONING, | ||
| LeaseState.ACTIVE, | ||
| LeaseState.CLEANUP_PENDING, | ||
| LeaseState.CLEANING, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat -n api/oss/src/dbs/postgres/agent_secret_leases/dao.py | sed -n '1,400p'
rg -n 'require_claim|janitor' api/oss/src/dbs/postgres/agent_secret_leases/dao.py api/oss/src/apis/fastapi/agent_secret_leases/models.pyRepository: Agenta-AI/agenta
Length of output: 19361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== service.py (key sections) ==\n'
cat -n api/oss/src/core/agent_secret_leases/service.py | sed -n '1,260p'
printf '\n== router.py (mutate endpoint) ==\n'
cat -n api/oss/src/apis/fastapi/agent_secret_leases/router.py | sed -n '1,260p'
printf '\n== models.py (request/route inputs) ==\n'
cat -n api/oss/src/apis/fastapi/agent_secret_leases/models.py | sed -n '1,260p'
printf '\n== additional janitor/claim references in core agent_secret_leases ==\n'
rg -n 'janitor|require_claim|BEGIN_CLEANUP|RECORD_RETRY|MARK_DELETED|QUARANTINE|validate_mutation' \
api/oss/src/core/agent_secret_leases \
api/oss/src/apis/fastapi/agent_secret_leases \
api/oss/src/dbs/postgres/agent_secret_leasesRepository: Agenta-AI/agenta
Length of output: 31363
Restrict cleanup transitions to janitor callers. BEGIN_CLEANUP, RECORD_RETRY, and MARK_DELETED are still reachable from the normal tenant PATCH path: the router passes janitor=False, and both validate_mutation and dao.mutate only enforce claim ownership when require_claim=True. A tenant can therefore advance a lease into cleanup/deleted states and mark resources deleted in the DB without a janitor claim, breaking the fenced-claim flow and risking orphaned Daytona secrets/sandboxes.
| now = datetime.now(timezone.utc) | ||
| async with self.engine.session() as session: | ||
| stmt = ( | ||
| select(AgentSecretLeaseDBE) | ||
| .options(selectinload(AgentSecretLeaseDBE.resources)) | ||
| .where( | ||
| AgentSecretLeaseDBE.id == lease_id, | ||
| AgentSecretLeaseDBE.version == mutation.expected_version, | ||
| ) | ||
| .with_for_update() | ||
| ) | ||
| if scope is not None: | ||
| stmt = self._scope(stmt, scope) | ||
| lease = (await session.execute(stmt)).scalar_one_or_none() | ||
| if lease is None: | ||
| return None | ||
| if require_claim and ( | ||
| mutation.claim is None | ||
| or lease.claim_id != mutation.claim.id | ||
| or lease.claim_generation != mutation.claim.generation | ||
| or lease.claim_expires_at is None | ||
| or lease.claim_expires_at <= now | ||
| ): | ||
| return None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
TOCTOU: now is captured before the row lock is acquired, weakening claim fencing.
now is computed before SELECT ... FOR UPDATE executes. If that statement blocks on lock contention, the claim-expiry check (lease.claim_expires_at <= now) uses a stale, earlier timestamp — an actually-expired claim (expired between the stale now and the real current time) can still pass fencing once the lock is finally acquired. This directly weakens the janitor claim-fencing guarantee this PR is meant to harden.
🔒️ Proposed fix
async def mutate(
self,
*,
lease_id: UUID,
scope: Optional[TenantScope],
mutation: LeaseMutation,
require_claim: bool,
) -> Optional[AgentSecretLease]:
- now = datetime.now(timezone.utc)
async with self.engine.session() as session:
stmt = (
select(AgentSecretLeaseDBE)
.options(selectinload(AgentSecretLeaseDBE.resources))
.where(
AgentSecretLeaseDBE.id == lease_id,
AgentSecretLeaseDBE.version == mutation.expected_version,
)
.with_for_update()
)
if scope is not None:
stmt = self._scope(stmt, scope)
lease = (await session.execute(stmt)).scalar_one_or_none()
if lease is None:
return None
+ now = datetime.now(timezone.utc)
if require_claim and (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| now = datetime.now(timezone.utc) | |
| async with self.engine.session() as session: | |
| stmt = ( | |
| select(AgentSecretLeaseDBE) | |
| .options(selectinload(AgentSecretLeaseDBE.resources)) | |
| .where( | |
| AgentSecretLeaseDBE.id == lease_id, | |
| AgentSecretLeaseDBE.version == mutation.expected_version, | |
| ) | |
| .with_for_update() | |
| ) | |
| if scope is not None: | |
| stmt = self._scope(stmt, scope) | |
| lease = (await session.execute(stmt)).scalar_one_or_none() | |
| if lease is None: | |
| return None | |
| if require_claim and ( | |
| mutation.claim is None | |
| or lease.claim_id != mutation.claim.id | |
| or lease.claim_generation != mutation.claim.generation | |
| or lease.claim_expires_at is None | |
| or lease.claim_expires_at <= now | |
| ): | |
| return None | |
| async with self.engine.session() as session: | |
| stmt = ( | |
| select(AgentSecretLeaseDBE) | |
| .options(selectinload(AgentSecretLeaseDBE.resources)) | |
| .where( | |
| AgentSecretLeaseDBE.id == lease_id, | |
| AgentSecretLeaseDBE.version == mutation.expected_version, | |
| ) | |
| .with_for_update() | |
| ) | |
| if scope is not None: | |
| stmt = self._scope(stmt, scope) | |
| lease = (await session.execute(stmt)).scalar_one_or_none() | |
| if lease is None: | |
| return None | |
| now = datetime.now(timezone.utc) | |
| if require_claim and ( | |
| mutation.claim is None | |
| or lease.claim_id != mutation.claim.id | |
| or lease.claim_generation != mutation.claim.generation | |
| or lease.claim_expires_at is None | |
| or lease.claim_expires_at <= now | |
| ): | |
| return None |
| if any(item.usage == "opaque_http" for item in self.credentials): | ||
| base_url = self.endpoint.base_url if self.endpoint else None | ||
| parsed = urlparse(base_url or "") | ||
| if parsed.scheme.lower() != "https" or not parsed.hostname: | ||
| raise ValueError( | ||
| "opaque_http model credentials require an effective HTTPS endpoint" | ||
| ) | ||
| return self |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the existing private-host / SSRF check used for MCP HTTP URLs, to confirm
# whether it can be shared with connections/models.py instead of duplicating logic.
rg -n "unsafe url|private|internal" sdks/python/agenta/sdk/agents/mcp/resolver.py -A3 -B3Repository: Agenta-AI/agenta
Length of output: 490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the connection model validator and the MCP SSRF guard helper.
files=(
"sdks/python/agenta/sdk/agents/connections/models.py"
"sdks/python/agenta/sdk/agents/mcp/resolver.py"
)
for f in "${files[@]}"; do
echo "===== $f ====="
wc -l "$f"
done
echo "===== models.py outline ====="
ast-grep outline sdks/python/agenta/sdk/agents/connections/models.py --view expanded
echo "===== resolver.py outline ====="
ast-grep outline sdks/python/agenta/sdk/agents/mcp/resolver.py --view expandedRepository: Agenta-AI/agenta
Length of output: 2683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant slices around the opaque_http validation and the URL-allowance helper.
sed -n '210,250p' sdks/python/agenta/sdk/agents/connections/models.py
echo "-----"
sed -n '1,140p' sdks/python/agenta/sdk/agents/mcp/resolver.pyRepository: Agenta-AI/agenta
Length of output: 5320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for reusable URL safety helpers and tests around private/internal host rejection.
rg -n "assert_endpoint_url_allowed|is_private|unsafe url|127\\.0\\.0\\.1|localhost|private.*host|internal.*host" sdks/python/agenta -SRepository: Agenta-AI/agenta
Length of output: 1898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the helper implementation and its tests so the review comment can point at the
# existing SSRF guard accurately.
rg -n "def assert_endpoint_url_allowed|assert_endpoint_url_allowed\(" sdks/python/agenta -S
echo "-----"
rg -n "test_unsafe_http_mcp_url_is_a_configuration_error|ssrf-guard" sdks/python/agenta -SRepository: Agenta-AI/agenta
Length of output: 566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect where ResolvedConnection.endpoint/base_url comes from and whether custom deployments
# can carry arbitrary URLs.
rg -n "ResolvedConnection|base_url|deployment=\"custom\"|custom\".*base_url|endpoint:" sdks/python/agenta/sdk -SRepository: Agenta-AI/agenta
Length of output: 9292
Reject private/internal hosts for opaque_http endpoints too. HTTPS plus a hostname still allows localhost/private destinations; reuse assert_endpoint_url_allowed (or the same host-safety check) here so secret-bearing credentials can’t be sent to an unsafe custom base_url.
| # ``model`` stays the plain string handed to the harness. ``model_ref`` carries author | ||
| # intent for resolution; only the resolved connection crosses the runner boundary. | ||
| model: Optional[str] = None | ||
| model_ref: Optional[ModelRef] = None | ||
| # ``resolved_connection`` carries the least-privilege output of a ``ConnectionResolver`` | ||
| # (threaded down from ``SessionConfig``). It is the authoritative source of the non-secret | ||
| # provider/model descriptor on the wire when present; unset leaves the wire unchanged (the | ||
| # golden contract). Its ``env`` is the secret channel and never reaches the wire here (it | ||
| # rides ``secrets``). See :meth:`wire_resolved_connection`. | ||
| # ``resolved_connection`` carries the route and typed credential bindings produced by the | ||
| # resolver. It serializes as one consumer-owned ``modelConnection`` object. | ||
| resolved_connection: Optional[ResolvedConnection] = None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Verify self.model still reaches the wire payload without colliding with resolved_connection.model.
self.model is now documented as the plain string handed to the harness, distinct from resolved_connection.model (the resolver's own model field). But wire_model_connection() never reads self.model — it only emits "model" (sourced from self.resolved_connection.model) when resolved_connection is set, and emits nothing at all otherwise.
This raises two open questions that this snippet alone can't resolve:
- Is
self.modelserialized through some other path whenresolved_connectionisNone(harness-login / no-resolved-connection runs), or is it silently dropped from the wire? - When
resolved_connectionIS set, does mergingwire_model_connection()'s"model"key with any generic field dump ofself.modelrisk one silently overwriting the other under the same"model"wire key?
Given this directly affects which model the harness receives, please confirm against the wire-assembly code and the run_request.*.json golden fixtures.
#!/bin/bash
rg -n "def to_wire|wire_model_connection|self\.model\b" sdks/python/agenta/sdk/agents/dtos.py
rg -n '"model"' sdks/python/oss/tests/pytest/unit/agents/golden/run_request.claude.json sdks/python/oss/tests/pytest/unit/agents/golden/run_request.pi_core.jsonAlso applies to: 771-778
| export function exactHttpsHost(rawUrl: string): string { | ||
| let url: URL; | ||
| try { | ||
| url = new URL(rawUrl); | ||
| } catch { | ||
| return fail("credential endpoint is not a valid URL"); | ||
| } | ||
| if (url.protocol !== "https:") fail("credential endpoint must use HTTPS"); | ||
| if (url.username || url.password || url.hash) fail("credential endpoint contains prohibited URL components"); | ||
| if (url.port && url.port !== "443") fail("explicit non-default ports are not supported"); | ||
| const host = url.hostname.toLowerCase().replace(/\.$/, ""); | ||
| if (!host || host === "localhost" || host.endsWith(".localhost") || host.includes("*")) { | ||
| fail("credential endpoint host is prohibited"); | ||
| } | ||
| const ipVersion = isIP(host); | ||
| if ((ipVersion === 4 && privateIpv4(host)) || ipVersion === 6) { | ||
| fail("credential endpoint must not be a private, reserved, or IPv6 literal"); | ||
| } | ||
| return host; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant source =="
git ls-files 'services/runner/src/engines/sandbox_agent/daytona-secret-plan.ts'
nl -ba services/runner/src/engines/sandbox_agent/daytona-secret-plan.ts | sed -n '1,140p'
echo
echo "== search for exactHttpsHost usage =="
rg -n "exactHttpsHost|allowedHost|credential endpoint" services/runner/src/engines/sandbox_agent -S
echo
echo "== runtime behavior probe =="
node - <<'JS'
const { isIP } = require('node:net');
for (const raw of [
'https://[::1]/x',
'https://[fc00::1]/x',
'https://127.0.0.1/x',
'https://example.com/x',
]) {
const u = new URL(raw);
const host = u.hostname.toLowerCase().replace(/\.$/, '');
const bare = host.startsWith('[') && host.endsWith(']') ? host.slice(1, -1) : host;
console.log({ raw, hostname: u.hostname, host, bare, isIP_host: isIP(host), isIP_bare: isIP(bare) });
}
JSRepository: Agenta-AI/agenta
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== source =="
sed -n '1,140p' services/runner/src/engines/sandbox_agent/daytona-secret-plan.ts | cat -n
echo
echo "== usages =="
rg -n "exactHttpsHost|allowedHost|credential endpoint" services/runner/src/engines/sandbox_agent -S
echo
echo "== runtime probe =="
node - <<'JS'
const { isIP } = require('node:net');
for (const raw of [
'https://[::1]/x',
'https://[fc00::1]/x',
'https://127.0.0.1/x',
'https://example.com/x',
]) {
const u = new URL(raw);
const host = u.hostname.toLowerCase().replace(/\.$/, '');
const bare = host.startsWith('[') && host.endsWith(']') ? host.slice(1, -1) : host;
console.log(JSON.stringify({
raw,
hostname: u.hostname,
host,
bare,
isIP_host: isIP(host),
isIP_bare: isIP(bare),
}));
}
JSRepository: Agenta-AI/agenta
Length of output: 12040
Strip brackets before the IP check. url.hostname keeps IPv6 literals in bracket form ([::1]), so net.isIP(host) returns 0 and bracketed IPv6 hosts slip through as approved allowedHost values.
| traceparent = headers.get("traceparent") | ||
| if not traceparent: | ||
| authorization = headers.get("Authorization") | ||
| if not traceparent and not authorization: |
There was a problem hiding this comment.
🤖 The AI agent says: Hotfix review note: this preserves invoke authorization when no traceparent is present, which unblocks the current playground path without adding a wire field or treating the gateway callback token as tenant authorization. This is intentionally narrow. The follow-up architecture should separate platform/control-plane authorization from telemetry context in a typed SDK field so authorization does not depend on tracing being enabled.
|
🤖 The AI agent says: Hotfix and live QA update The SDK now preserves invoke authorization even when an invocation has no Live verification on the playground stack:
So the hotfix is working, but the current environment still needs a Daytona API key/role with Secrets CRUD permission before opaque Daytona playground runs can complete. Follow-up required: move platform/control-plane authorization out of |
| try { | ||
| await sandbox.start(); | ||
| } catch (error) { | ||
| if (!isStateChangeInProgress(error)) throw error; |
There was a problem hiding this comment.
🤖 The AI agent says: Review note: this handles the exact warm-eviction race observed in the playground. Daytona may report Sandbox state change in progress when start() overlaps an in-flight stop/delete. We retry only that explicit transient message, refresh the provider state between attempts, preserve terminal not-found handling, and share the existing 10-second reconnect deadline so retries remain bounded.
|
🤖 The AI agent says: Added the Daytona warm-session transition-race fix requested during live QA. Observed behavior: a config or credential change evicted the warm session while the next request attempted to reconnect. Daytona returned Changed in
Verification:
I also corrected the local sidecar mount for the already-built in-sandbox tool MCP shim. That was deployment state only, not a repository change. |
|
🤖 The AI agent says: 🔸 Decision needed: approve the #5242 recut before any more implementation work. I recommend two stacked PRs:
The repository research found a concrete second consumer: Composio trigger subscriptions currently create the provider trigger before local persistence and can lose cleanup state after provider deletion failures. Gateway connections have a similar lifecycle gap. This supports a reusable external-resource kernel, but neither product domain should be replaced by it. Please decide:
Detailed design and API links are now in the PR description. I recommend reviewing the architecture and migration direction before reviewing the current lease implementation. |
mmabrouk
force-pushed
the
feat/daytona-secret-delivery
branch
from
5b21986 to
2ababd2
Compare
1 participant
Context
Daytona Secrets can keep supported model and HTTP MCP credentials out of the sandbox environment. The current implementation also adds crash-safe cleanup, warm-session recovery, credential rotation, retries, worker claims, an API router, and a database migration.
Those are two separate architecture decisions. This draft is now the review point for recutting them into two stacked PRs. The current database-backed implementation should not merge unchanged.
Proposed stack
PR A: deliver opaque credentials through Daytona Secrets
PR A contains the functional security mechanism:
PR A contains no API router, migration, shared auth-middleware change, durable janitor, claims, retry cursor, control token, or lease HTTP client.
A hard runner crash may leave an orphaned Daytona resource. The feature therefore remains disabled for production until PR B merges.
PR B: reconcile managed external resources durably
PR B adds a reviewed managed_resources control plane:
Product domains remain authoritative. Gateway connections remain connections, and trigger subscriptions remain subscriptions. Managed-resource rows only record how Agenta realizes those objects in external systems.
The Daytona credential plan and provider adapter from PR A must remain unchanged when PR B is added.
Why make the durability layer reusable?
Agenta already has another concrete instance of the same failure class. Composio trigger creation happens before local subscription persistence, so a database failure can orphan the provider trigger. Provider deletion failures may also be followed by local deletion, which loses the durable cleanup record.
A later trigger-controller PR can adopt the same kernel. Future MCP authorization stays in gateway_connections, while any provider-created registration, token revocation, or gateway allocation can use managed-resource reconciliation behind that product contract.
This PR does not migrate connections or triggers. They demonstrate that the control-plane primitive has a real second consumer.
Existing Agenta patterns
The proposal reuses:
No generic durable external-resource reconciler exists in Agenta today.
Migration direction
Migration 011 has not shipped and has no production data. Replace the Daytona-specific agent_secret_leases schema before merge. Do not ship that schema and immediately add a production rename migration.
Preview databases that already applied migration 011 can downgrade to 010 and apply the replacement migration.
Detailed plan
Current verification
The existing implementation has extensive SDK, runner, API, migration, and live control-plane coverage. That work remains useful source material for the recut. This planning revision changes documentation only.
Review focus
Please review the domain boundary before reviewing implementation details:
Do not approve the migration or API subsystem until those questions are settled.