feat(tee-worker): migrate TEE worker to Phala Cloud CVM#395
Merged
Conversation
…E migration Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a9688c7ccf3d
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: f00994d13664
5 plans covering: TEE worker test gap (Wave 0), dstack SDK + CVM compose (Wave 1), staging infra + CI/CD (Wave 2), documentation updates (Wave 2), and CVM deployment + verification (Wave 3). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ve to apps/, instrumentation - Add 35-01: move tee-worker to apps/, replace duplicated crypto/provider code with @cipherbox/crypto, @cipherbox/core, @cipherbox/sdk-core, rewrite Dockerfile for pnpm workspace builds - Replan 35-02: reduced test scope (TEE-specific logic only, no shared package test duplication) - Expand 35-03: add prom-client metrics, structured JSON logging, dstack SDK install - Renumber 35-03→04→05→06 (old 02→03→04→05) with updated dependencies - Add STRUCTURE.md update task to 35-05 - Fix all paths from tee-worker/ to apps/tee-worker/ - Shift waves from 0-3 to 1-4 - Fix plan checker findings: Dockerfile rewrite, Counter syntax, redundant Task 5, double path typo, stale cd path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: cbb49e3c765b
- Git mv tee-worker to apps/tee-worker for monorepo consistency - Rewrite Dockerfile for pnpm workspace builds (npm ci -> pnpm deploy) - Update deploy-staging.yml build context to repo root - Remove stale tee-worker/** from ci.yml path filters - Delete package-lock.json (pnpm uses root pnpm-lock.yaml)
- Add @cipherbox/crypto, @cipherbox/core, @cipherbox/sdk-core as workspace:* deps - Remove eciesjs, @noble/ed25519, ipns, @libp2p/crypto, multiformats - Keep express, @noble/secp256k1, @noble/hashes (TEE-specific)
- Delegate to createIpnsRecord and marshalIpnsRecord from @cipherbox/core - Remove direct imports of ipns, @libp2p/crypto, @noble/ed25519 - Preserve signIpnsRecord function signature and 48h TEE lifetime
- Import unwrapKey/wrapKey from @cipherbox/crypto instead of eciesjs - Correct parameter order for both functions (data first, key second) - Preserve private key zeroing and epoch fallback logic
…PsaProvider - Add ProviderOptions type with optional fetchFn and timeoutMs - KuboProvider and PsaProvider accept optional ProviderOptions as 3rd param - Default behavior unchanged (globalThis.fetch, 10s/15s timeouts) - Export ProviderOptions from sdk-core
- Use KuboProvider/PsaProvider from @cipherbox/sdk-core for all provider ops - Use unwrapKey from @cipherbox/crypto instead of direct eciesjs - Inject ssrfSafeFetch via ProviderOptions for TEE SSRF safety - Preserve gateway fallback for PSA/CipherBox content fetch - Preserve credential zeroing and batch result tracking
…crypto - Import unwrapKey from @cipherbox/crypto instead of eciesjs decrypt - Correct parameter order (ciphertext, privateKey) - Preserve SSRF validation and protocol probing logic
- Use FetchFn type alias instead of typeof globalThis.fetch for ProviderOptions - Add @types/express-serve-static-core to tee-worker devDeps (pnpm strict hoisting) - tsc --noEmit passes with zero errors - No stale imports or root-level tee-worker references remain
- Created 35-01-SUMMARY.md with execution results - Updated STATE.md with position, decisions, metrics - Updated ROADMAP.md with plan progress
- Add vitest devDependency to apps/tee-worker - Add test and test:watch scripts to package.json - Create vitest.config.ts with node environment
- Deterministic derivation across calls for same epoch - Epoch isolation (different epochs produce different keys) - Key format validation (65-byte uncompressed pubkey, 32-byte privkey) - Public key caching via getPublicKey - Production guard (simulator mode blocked in production)
- Single-epoch decrypt with correct key - Current-epoch-first priority in fallback - Previous-epoch fallback during grace period - Both-epochs-fail error handling - Null previousEpoch skips fallback - Re-encrypt round-trip (old epoch -> new epoch) - Full epoch migration round-trip test - Uses real crypto (no ECIES mocking)
- Auth: valid token, missing/wrong/malformed headers, unconfigured secret - Republish: single entry success, batch independence, epoch upgrade - Republish: input validation (missing/non-array entries), base64 output - Republish: sequence number increment verification - Uses real ECIES crypto, mocks only ipns-signer (tested in @cipherbox/core)
- SUMMARY.md with 36 new tests across 4 test files - STATE.md updated with plan progress - ROADMAP.md updated with plan 02 completion
- Add @phala/dstack-sdk@^0.5.7 as production dependency - Defensive CVM key derivation handling both SDK return types (key vs asUint8Array) - Remove custom type declarations (SDK ships own types)
- Update docker-compose.yml: tappd.sock -> dstack.sock - Create docker-compose.phala.yml for Phala Cloud CVM deployment - CVM identity preservation warning in comments
- Install prom-client as production dependency - HTTP request duration histogram (cipherbox_tee_http_request_duration_seconds) - Republish entries counter (cipherbox_tee_republish_entries_total) - Migration CIDs counter (cipherbox_tee_migration_cids_total) - GET /metrics endpoint (public, no auth) for Prometheus scraping - Metrics middleware skips self-instrumentation
- Minimal structured JSON logger (no external dependencies) - Newline-delimited JSON output to stdout/stderr - Replace all console.log/console.error across all src/ files - Service field for log aggregator filtering - No sensitive data in log entries (no keys, tokens, encrypted content)
- SUMMARY.md with task commits and self-check - STATE.md updated with position (plan 3/6), metrics, decisions - ROADMAP.md updated with plan progress (3/6)
- Remove tee-worker service block (now runs on Phala Cloud CVM) - Add comment documenting external TEE endpoint and deploy command - All other services (api, postgres, redis, ipfs, someguy, caddy, alloy) unchanged
- Add deploy-tee-phala job that deploys TEE worker to Phala Cloud CVM - Update deploy-vps needs to wait for CVM deployment before VPS deploy - Change TEE_WORKER_URL from http://tee-worker:3001 to PHALA_TEE_WORKER_URL env var - Document required GitHub secrets (PHALA_CLOUD_API_KEY) and vars (PHALA_TEE_WORKER_URL)
…ackages - Update TEE worker section to apps/tee-worker/ with Phala Cloud CVM deployment model - Document shared workspace dependencies (@cipherbox/crypto, @cipherbox/core, @cipherbox/sdk-core) - Add @phala/dstack-sdk ^0.5.7 for hardware-backed key derivation - Add prom-client for Prometheus metrics - Add phala CLI to deployment tools section - Update staging platform requirements for external CVM - Update deploy-staging workflow description - Update analysis date to 2026-03-29
- Update environment matrix to show Phala Cloud CVM (testnet) for staging - Update TEE environment matrix staging row - Replace staging TEE section with comprehensive Phala Cloud CVM documentation - Document CVM name, image, endpoint, key derivation, and socket - Add TEE_WORKER_URL as external HTTPS endpoint in staging configuration - Add CVM Identity Preservation critical warning - Add Migration Note (Phase 35) documenting simulator-to-CVM transition
…eport Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: e01d74f2c021
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates the TEE worker into the monorepo apps/tee-worker/ for Phala Cloud CVM deployment, replacing vendored crypto with shared workspace packages, adding observability (metrics + structured logs), updating staging deployment flow, and adding unit tests for TEE-specific orchestration.
Changes:
- Move
tee-worker/→apps/tee-worker/and swap to@cipherbox/*workspace packages. - Add ProviderOptions (
fetchFn,timeoutMs) to sdk-core pinning providers to support SSRF-safe fetch injection. - Add Phala CVM deployment compose, Prometheus metrics + JSON logger, and Vitest coverage for TEE worker.
Reviewed changes
Copilot reviewed 56 out of 63 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tee-worker/src/types/dstack-sdk.d.ts | Removed legacy type stub |
| tee-worker/src/services/migration-worker.ts | Removed old migration worker |
| tee-worker/src/services/ipns-signer.ts | Removed vendored IPNS signing |
| tee-worker/Dockerfile | Removed old standalone Dockerfile |
| packages/sdk-core/src/pinning/types.ts | Add FetchFn/ProviderOptions |
| packages/sdk-core/src/pinning/psa-provider.ts | Inject fetch/timeout options |
| packages/sdk-core/src/pinning/kubo-provider.ts | Inject fetch/timeout options |
| packages/sdk-core/src/pinning/index.ts | Export ProviderOptions |
| packages/sdk-core/src/index.ts | Re-export ProviderOptions |
| docker/docker-compose.staging.yml | Remove local tee-worker service |
| apps/tee-worker/vitest.config.ts | Add Vitest config |
| apps/tee-worker/tsconfig.json | Add TS config for app |
| apps/tee-worker/src/services/tee-keys.ts | Defensive dstack key handling + cache |
| apps/tee-worker/src/services/ssrf-validation.ts | DNS pinning SSRF fetch |
| apps/tee-worker/src/services/migration-worker.ts | Migration via sdk-core providers |
| apps/tee-worker/src/services/logger.ts | Add structured JSON logger |
| apps/tee-worker/src/services/key-manager.ts | Switch to wrapKey/unwrapKey |
| apps/tee-worker/src/services/ipns-signer.ts | Delegate IPNS signing to core |
| apps/tee-worker/src/routes/republish.ts | Batch limits + metrics + logging |
| apps/tee-worker/src/routes/public-key.ts | Epoch bounds + logger |
| apps/tee-worker/src/routes/migrate.ts | CID validation + metrics + logging |
| apps/tee-worker/src/routes/metrics.ts | Add Prometheus scrape route |
| apps/tee-worker/src/routes/health.ts | Use TEE_CURRENT_EPOCH |
| apps/tee-worker/src/routes/connection-test.ts | Switch to unwrapKey + logger |
| apps/tee-worker/src/middleware/metrics.ts | Add HTTP/operation Prom metrics |
| apps/tee-worker/src/middleware/auth.ts | Add bearer secret auth middleware |
| apps/tee-worker/src/index.ts | Wire metrics + auth + routes |
| apps/tee-worker/src/tests/tee-keys.test.ts | Tests: epoch key derivation |
| apps/tee-worker/src/tests/ssrf-validation.test.ts | Tests: DNS pinning behavior |
| apps/tee-worker/src/tests/republish.test.ts | Tests: republish orchestration |
| apps/tee-worker/src/tests/key-manager.test.ts | Tests: epoch fallback/re-encrypt |
| apps/tee-worker/src/tests/auth.test.ts | Tests: auth middleware |
| apps/tee-worker/package.json | Add workspace deps + tests |
| apps/tee-worker/docker-compose.yml | Mount dstack.sock locally |
| apps/tee-worker/docker-compose.phala.yml | Phala CVM deployment compose |
| apps/tee-worker/Dockerfile | Workspace-aware Docker build |
| apps/tee-worker/.env.example | Add env example |
| .planning/phases/35-phala-testnet-tee-migration/35-VERIFICATION.md | Add phase verification report |
| .planning/phases/35-phala-testnet-tee-migration/35-VALIDATION.md | Add validation strategy |
| .planning/phases/35-phala-testnet-tee-migration/35-RESEARCH.md | Add research notes |
| .planning/phases/35-phala-testnet-tee-migration/35-06-SUMMARY.md | Add deployment summary |
| .planning/phases/35-phala-testnet-tee-migration/35-06-PLAN.md | Add execution plan |
| .planning/phases/35-phala-testnet-tee-migration/35-05-SUMMARY.md | Add docs update summary |
| .planning/phases/35-phala-testnet-tee-migration/35-05-PLAN.md | Add docs update plan |
| .planning/phases/35-phala-testnet-tee-migration/35-04-SUMMARY.md | Add infra summary |
| .planning/phases/35-phala-testnet-tee-migration/35-04-PLAN.md | Add infra plan |
| .planning/phases/35-phala-testnet-tee-migration/35-03-SUMMARY.md | Add CVM integration summary |
| .planning/phases/35-phala-testnet-tee-migration/35-03-PLAN.md | Add CVM integration plan |
| .planning/phases/35-phala-testnet-tee-migration/35-02-SUMMARY.md | Add tests summary |
| .planning/phases/35-phala-testnet-tee-migration/35-02-PLAN.md | Add tests plan |
| .planning/phases/35-phala-testnet-tee-migration/35-01-SUMMARY.md | Add move/integration summary |
| .planning/config.json | Fix JSON formatting |
| .planning/codebase/STRUCTURE.md | Update structure for apps/tee-worker |
| .planning/codebase/STACK.md | Update stack docs for Phala CVM |
| .planning/STATE.md | Update planning state |
| .planning/ROADMAP.md | Mark Phase 35 complete |
| .planning/ENVIRONMENTS.md | Document staging Phala CVM |
| .github/workflows/deploy-staging.yml | Add Phala deploy job + update build context |
| .github/workflows/ci.yml | Remove old tee-worker path filter |
Files not reviewed (1)
- tee-worker/package-lock.json: Language not supported
Comments suppressed due to low confidence (3)
apps/tee-worker/src/services/ssrf-validation.ts:119
- ssrfSafeFetch() replaces the URL hostname with the resolved IP for HTTPS requests. With Node’s fetch/undici this will cause TLS certificate validation / SNI to use the IP (not the original hostname), which will typically fail for normal HTTPS endpoints. To keep DNS pinning without breaking TLS, use a custom dispatcher/Agent (e.g., undici Agent with connect.lookup pinned to the resolved IP and servername set to the original hostname), or avoid hostname replacement and instead provide a pinned lookup at connect time.
apps/tee-worker/src/routes/migrate.ts:101 - isValidCidFormat() is not actually validating CIDv1 correctly: it allows characters like '+' and '=' (not valid in multibase base32/base58btc), and it does not properly accept base16 (multibase 'f') CIDs. This can reject valid CIDs and accept invalid ones. Consider using a real CID parser (e.g., multiformats CID decoding) or tightening the regex per multibase alphabet (base32: ^b[a-z2-7]+$, base58btc: ^z[1-9A-HJ-NP-Za-km-z]+$, base16: ^f[0-9a-f]+$) with an additional decode check if possible.
apps/tee-worker/src/routes/migrate.ts:34 - The /migrate route accepts a request-provided currentEpoch (or env fallback) but does not validate it (integer/range) before passing it into getKeypair(). Since MIN_EPOCH/MAX_EPOCH are defined for epoch bounds elsewhere, apply the same validation here to avoid unexpected key derivation paths and potential resource abuse.
…lled regex Replace custom isValidCidFormat regex with standard CID.parse() from the multiformats library (already used in @cipherbox/crypto). Handles all CID versions and multibase encodings correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t-tee-migration # Conflicts: # .planning/ROADMAP.md # .planning/STATE.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8d3707774abf
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
==========================================
+ Coverage 61.15% 71.15% +9.99%
==========================================
Files 132 111 -21
Lines 9750 6791 -2959
Branches 976 980 +4
==========================================
- Hits 5963 4832 -1131
+ Misses 3573 1745 -1828
Partials 214 214
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 63 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- tee-worker/package-lock.json: Language not supported
Comments suppressed due to low confidence (4)
apps/tee-worker/src/services/ssrf-validation.ts:92
ssrfSafeFetchpins DNS by swapping the hostname for an IP and then setting aHostheader “for TLS SNI”, but SNI is derived from the TLS servername (URL hostname / connection options), not from the HTTPHostheader. In CVM mode this is likely to break HTTPS withERR_TLS_CERT_ALTNAME_INVALIDfor normal endpoints (certs usually don’t include the resolved IP). Consider pinning DNS at the connector/agent level (custom lookup/dispatcher) so the URL hostname remains the original host for SNI/cert validation while the connection is forced to the validated IP.
apps/tee-worker/src/services/ssrf-validation.ts:111- Replacing
pinnedUrl.hostnamewith the resolved IP will change TLS verification to the IP address. Even if you keepHost: <original>in headers, Node’s TLS handshake will still use the IP as the server name unless explicitly overridden, so this can fail for most HTTPS endpoints. If you keep this approach, the fetch call likely needs an explicit agent/dispatcher withservernameset toparsed.hostname(or a custom DNS lookup that returns the pinned IP without changing the URL).
apps/tee-worker/src/routes/migrate.ts:35 currentEpochfrom the request body is used directly (or falls back to env) without validating integer-ness and bounds. Since epoch values are used for key derivation inside the TEE, it should be validated similarly to/public-key(e.g.,MIN_EPOCH..MAX_EPOCH) to avoid unexpected behavior/DoS via extremely large/negative epochs.
apps/tee-worker/src/routes/republish.ts:59- The route adds batch-size validation, but the per-entry epoch fields (
currentEpoch/previousEpoch) are still not validated (integer + bounds) before being used for key derivation/decryption. Consider validating epochs alongside the other request checks (similar to/public-keyusingMIN_EPOCH..MAX_EPOCH) and returning a per-entry validation error when out of range.
…tched) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: a568232bfe07
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/sdk-core/src/pinning/index.ts (1)
1-9: Consider re-exportingFetchFnhere too.Line 8 surfaces
ProviderOptions, but consumers still need a deep import orProviderOptions['fetchFn']indirection to name the injected function type directly.Small export surface tweak
export type { PinningProvider, PinResult, PinStatus, PinningMode, ExternalProviderConfig, ConnectionTestResult, + FetchFn, ProviderOptions, } from './types';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/src/pinning/index.ts` around lines 1 - 9, The public index currently re-exports types from './types' but omits the injected function type; add FetchFn to the export list so consumers can import FetchFn directly instead of using ProviderOptions['fetchFn'] or deep-importing; update the export block that lists PinningProvider, PinResult, PinStatus, PinningMode, ExternalProviderConfig, ConnectionTestResult, ProviderOptions to also include FetchFn.apps/tee-worker/src/routes/migrate.ts (1)
33-34: Consider validating parsed epoch value.If
TEE_CURRENT_EPOCHis set to an invalid number (e.g.,"abc"),parseIntreturnsNaN, which could cause unexpected behavior downstream. Consider adding validation.🔧 Proposed validation
- const epoch = currentEpoch ?? parseInt(process.env.TEE_CURRENT_EPOCH || '1', 10); + const parsedEnvEpoch = parseInt(process.env.TEE_CURRENT_EPOCH || '1', 10); + const epoch = currentEpoch ?? (Number.isNaN(parsedEnvEpoch) ? 1 : parsedEnvEpoch);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tee-worker/src/routes/migrate.ts` around lines 33 - 34, The epoch assignment using currentEpoch ?? parseInt(process.env.TEE_CURRENT_EPOCH || '1', 10) can yield NaN if TEE_CURRENT_EPOCH is invalid; update the logic around the epoch variable (where currentEpoch is used) to parse and validate the env value explicitly: if currentEpoch is undefined, attempt to parse process.env.TEE_CURRENT_EPOCH, check Number.isFinite/Number.isNaN and either use a safe default (e.g., 1) or throw a clear error, and ensure the final epoch value is a finite integer before proceeding so downstream code using epoch receives a valid number..planning/STATE.md (1)
87-89: Minor table formatting inconsistency.The Phase 35 rows have 4 cells while the table header defines 5 columns. This matches the format used in other recent entries (lines 82-86), so it's a pre-existing pattern. Per project conventions, STATE.md metrics are informational and minor formatting inconsistencies are acceptable.
Optional fix for table consistency
-| Phase 35 P02 | 5min | 4 tasks | 6 files | -| Phase 35 P03 | 8min | 4 tasks | 14 files | -| Phase 35 P05 | 5min | 3 tasks | 3 files | +| 35 | 02 | 5min | 4 | 6 | +| 35 | 03 | 8min | 4 | 14 | +| 35 | 05 | 5min | 3 | 3 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/STATE.md around lines 87 - 89, The table rows for "Phase 35 P02", "Phase 35 P03", and "Phase 35 P05" in STATE.md have only 4 pipe-separated cells while the table header defines 5 columns; update each of those Phase 35 rows to include a fifth cell (e.g., an empty placeholder like "") so they match the header column count and the other entries' format, ensuring the row text for "Phase 35 P02", "Phase 35 P03", and "Phase 35 P05" uses the same pipe-separated column structure as the surrounding table..planning/phases/35-phala-testnet-tee-migration/35-06-PLAN.md (1)
46-48: Use repo-relative execution-context paths instead of machine-specific absolute paths.The hardcoded
/Users/michael/...paths reduce reproducibility and leak local workstation details. Prefer repository-relative references.♻️ Proposed documentation fix
-@/Users/michael/Code/cipher-box-worktree-general/.claude/get-shit-done/workflows/execute-plan.md -@/Users/michael/Code/cipher-box-worktree-general/.claude/get-shit-done/templates/summary.md +@.claude/get-shit-done/workflows/execute-plan.md +@.claude/get-shit-done/templates/summary.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/phases/35-phala-testnet-tee-migration/35-06-PLAN.md around lines 46 - 48, The two execution-context entries in the execution_context block currently use machine-specific absolute paths ("@/Users/michael/Code/cipher-box-worktree-general/.claude/get-shit-done/workflows/execute-plan.md" and "@/Users/michael/Code/cipher-box-worktree-general/.claude/get-shit-done/templates/summary.md"); change them to repository-relative paths (e.g., "@/.claude/get-shit-done/workflows/execute-plan.md" or "./.claude/..." or simply ".claude/get-shit-done/..." as appropriate) so the file referenced by execution_context uses repo-relative references instead of leaking local workstation paths.apps/tee-worker/src/__tests__/republish.test.ts (1)
252-266: Consider adding a test for empty entries array.The input validation tests cover missing and non-array entries. An edge case not explicitly tested is an empty array (
entries: []), which might have different behavior (200 with empty results vs 400).🧪 Optional: Add empty entries array test
it('handles empty entries array', async () => { const app = await createTestApp(); const res = await postJson(app, '/republish', { entries: [] }); // Verify expected behavior for empty batch expect(res.status).toBe(200); expect(res.body.results).toHaveLength(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tee-worker/src/__tests__/republish.test.ts` around lines 252 - 266, Add a test that covers the empty entries array edge case for the /republish endpoint by calling createTestApp() and postJson(app, '/republish', { entries: [] }) and asserting the expected behavior (e.g., expect HTTP 200 and res.body.results toHaveLength(0) or whatever the handler should return for empty batches); place the new it(...) alongside the existing tests in republish.test.ts so it uses the same helpers (createTestApp and postJson) and a descriptive title like 'handles empty entries array'.apps/tee-worker/src/services/migration-worker.ts (1)
40-43: Consider input validation for hex string.The
decryptEciesfunction assumesencryptedHexis valid hex. If the caller passes malformed input,Buffer.from(encryptedHex, 'hex')will silently produce a shorter/incorrect buffer rather than throwing.♻️ Optional: Add hex validation
async function decryptEcies(encryptedHex: string, teePrivateKey: Uint8Array): Promise<Uint8Array> { + if (!/^[0-9a-fA-F]*$/.test(encryptedHex) || encryptedHex.length % 2 !== 0) { + throw new Error('Invalid hex string for encrypted config'); + } const ciphertext = new Uint8Array(Buffer.from(encryptedHex, 'hex')); return await unwrapKey(ciphertext, teePrivateKey); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tee-worker/src/services/migration-worker.ts` around lines 40 - 43, The decryptEcies function currently trusts encryptedHex and can produce an incorrect buffer silently; before calling unwrapKey, validate that encryptedHex is a non-empty valid hex string (e.g. matches /^[0-9a-fA-F]+$/ and has even length) and throw a descriptive error if it fails; then convert to Uint8Array and call unwrapKey(ciphertext, teePrivateKey) as before so malformed input is rejected early and not passed into unwrapKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/tee-worker/src/services/migration-worker.ts`:
- Around line 105-112: The SSRF validation incorrectly checks
sourceConfig.endpoint and destConfig.endpoint against the literal 'cipherbox',
causing validateEndpointUrl to run always; change both conditions to check
sourceConfig.protocol !== 'cipherbox' and destConfig.protocol !== 'cipherbox' so
validateEndpointUrl(sourceConfig.endpoint) and
validateEndpointUrl(destConfig.endpoint) are skipped when protocol ===
'cipherbox' (keep the validateEndpointUrl calls and DNS rebinding comment
as-is).
In `@apps/tee-worker/src/services/ssrf-validation.ts`:
- Around line 108-119: Don't rewrite the URL hostname to the IP (remove the
pinnedUrl.hostname = address and Host header hack around mergedHeaders/parsed)
because TLS SNI/cert validation will break; instead keep the original url and
create an undici Agent with a custom connect.lookup that returns the resolved
address (use the existing address variable), then call fetch(url, { ...init,
redirect: 'error', dispatcher: agent, headers: ... }) so SNI uses the original
hostname while the TCP connect uses the pinned IP; remove the Host header
override and the pinnedUrl usage in the fetch call.
---
Nitpick comments:
In @.planning/phases/35-phala-testnet-tee-migration/35-06-PLAN.md:
- Around line 46-48: The two execution-context entries in the execution_context
block currently use machine-specific absolute paths
("@/Users/michael/Code/cipher-box-worktree-general/.claude/get-shit-done/workflows/execute-plan.md"
and
"@/Users/michael/Code/cipher-box-worktree-general/.claude/get-shit-done/templates/summary.md");
change them to repository-relative paths (e.g.,
"@/.claude/get-shit-done/workflows/execute-plan.md" or "./.claude/..." or simply
".claude/get-shit-done/..." as appropriate) so the file referenced by
execution_context uses repo-relative references instead of leaking local
workstation paths.
In @.planning/STATE.md:
- Around line 87-89: The table rows for "Phase 35 P02", "Phase 35 P03", and
"Phase 35 P05" in STATE.md have only 4 pipe-separated cells while the table
header defines 5 columns; update each of those Phase 35 rows to include a fifth
cell (e.g., an empty placeholder like "") so they match the header column count
and the other entries' format, ensuring the row text for "Phase 35 P02", "Phase
35 P03", and "Phase 35 P05" uses the same pipe-separated column structure as the
surrounding table.
In `@apps/tee-worker/src/__tests__/republish.test.ts`:
- Around line 252-266: Add a test that covers the empty entries array edge case
for the /republish endpoint by calling createTestApp() and postJson(app,
'/republish', { entries: [] }) and asserting the expected behavior (e.g., expect
HTTP 200 and res.body.results toHaveLength(0) or whatever the handler should
return for empty batches); place the new it(...) alongside the existing tests in
republish.test.ts so it uses the same helpers (createTestApp and postJson) and a
descriptive title like 'handles empty entries array'.
In `@apps/tee-worker/src/routes/migrate.ts`:
- Around line 33-34: The epoch assignment using currentEpoch ??
parseInt(process.env.TEE_CURRENT_EPOCH || '1', 10) can yield NaN if
TEE_CURRENT_EPOCH is invalid; update the logic around the epoch variable (where
currentEpoch is used) to parse and validate the env value explicitly: if
currentEpoch is undefined, attempt to parse process.env.TEE_CURRENT_EPOCH, check
Number.isFinite/Number.isNaN and either use a safe default (e.g., 1) or throw a
clear error, and ensure the final epoch value is a finite integer before
proceeding so downstream code using epoch receives a valid number.
In `@apps/tee-worker/src/services/migration-worker.ts`:
- Around line 40-43: The decryptEcies function currently trusts encryptedHex and
can produce an incorrect buffer silently; before calling unwrapKey, validate
that encryptedHex is a non-empty valid hex string (e.g. matches /^[0-9a-fA-F]+$/
and has even length) and throw a descriptive error if it fails; then convert to
Uint8Array and call unwrapKey(ciphertext, teePrivateKey) as before so malformed
input is rejected early and not passed into unwrapKey.
In `@packages/sdk-core/src/pinning/index.ts`:
- Around line 1-9: The public index currently re-exports types from './types'
but omits the injected function type; add FetchFn to the export list so
consumers can import FetchFn directly instead of using
ProviderOptions['fetchFn'] or deep-importing; update the export block that lists
PinningProvider, PinResult, PinStatus, PinningMode, ExternalProviderConfig,
ConnectionTestResult, ProviderOptions to also include FetchFn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80f4e6e8-d7d6-4ac6-9a98-5997712170e2
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yamltee-worker/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (61)
.github/workflows/ci.yml.github/workflows/deploy-staging.yml.planning/ENVIRONMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/codebase/STACK.md.planning/codebase/STRUCTURE.md.planning/config.json.planning/phases/35-phala-testnet-tee-migration/35-01-PLAN.md.planning/phases/35-phala-testnet-tee-migration/35-01-SUMMARY.md.planning/phases/35-phala-testnet-tee-migration/35-02-PLAN.md.planning/phases/35-phala-testnet-tee-migration/35-02-SUMMARY.md.planning/phases/35-phala-testnet-tee-migration/35-03-PLAN.md.planning/phases/35-phala-testnet-tee-migration/35-03-SUMMARY.md.planning/phases/35-phala-testnet-tee-migration/35-04-PLAN.md.planning/phases/35-phala-testnet-tee-migration/35-04-SUMMARY.md.planning/phases/35-phala-testnet-tee-migration/35-05-PLAN.md.planning/phases/35-phala-testnet-tee-migration/35-05-SUMMARY.md.planning/phases/35-phala-testnet-tee-migration/35-06-PLAN.md.planning/phases/35-phala-testnet-tee-migration/35-06-SUMMARY.md.planning/phases/35-phala-testnet-tee-migration/35-RESEARCH.md.planning/phases/35-phala-testnet-tee-migration/35-VALIDATION.md.planning/phases/35-phala-testnet-tee-migration/35-VERIFICATION.md.planning/security/REVIEW-phase-35.mdapps/tee-worker/.env.exampleapps/tee-worker/Dockerfileapps/tee-worker/docker-compose.phala.ymlapps/tee-worker/docker-compose.ymlapps/tee-worker/package.jsonapps/tee-worker/src/__tests__/auth.test.tsapps/tee-worker/src/__tests__/key-manager.test.tsapps/tee-worker/src/__tests__/republish.test.tsapps/tee-worker/src/__tests__/ssrf-validation.test.tsapps/tee-worker/src/__tests__/tee-keys.test.tsapps/tee-worker/src/index.tsapps/tee-worker/src/middleware/auth.tsapps/tee-worker/src/middleware/metrics.tsapps/tee-worker/src/routes/connection-test.tsapps/tee-worker/src/routes/health.tsapps/tee-worker/src/routes/metrics.tsapps/tee-worker/src/routes/migrate.tsapps/tee-worker/src/routes/public-key.tsapps/tee-worker/src/routes/republish.tsapps/tee-worker/src/services/ipns-signer.tsapps/tee-worker/src/services/key-manager.tsapps/tee-worker/src/services/logger.tsapps/tee-worker/src/services/migration-worker.tsapps/tee-worker/src/services/ssrf-validation.tsapps/tee-worker/src/services/tee-keys.tsapps/tee-worker/tsconfig.jsonapps/tee-worker/vitest.config.tsdocker/docker-compose.staging.ymlpackages/sdk-core/src/index.tspackages/sdk-core/src/pinning/index.tspackages/sdk-core/src/pinning/kubo-provider.tspackages/sdk-core/src/pinning/psa-provider.tspackages/sdk-core/src/pinning/types.tstee-worker/Dockerfiletee-worker/src/services/ipns-signer.tstee-worker/src/services/migration-worker.tstee-worker/src/types/dstack-sdk.d.ts
💤 Files with no reviewable changes (5)
- .github/workflows/ci.yml
- tee-worker/Dockerfile
- tee-worker/src/types/dstack-sdk.d.ts
- tee-worker/src/services/ipns-signer.ts
- tee-worker/src/services/migration-worker.ts
…TLS-safe DNS pinning - migration-worker: check `protocol !== 'cipherbox'` not `endpoint !== 'cipherbox'` (endpoint is always a URL, never the literal string 'cipherbox') - ssrf-validation: use undici Agent with custom lookup instead of hostname rewrite — preserves original hostname for TLS SNI/certificate validation while pinning the TCP connection to the validated IP Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4c69a3e46df5
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/tee-worker/src/services/migration-worker.ts (1)
143-161: Minor inefficiency: data fetched but unused for PSA destinations.When the destination is PSA (
destPsa),pinByCid(line 157) pins by CID reference without using the fetcheddata. The gateway fetch at line 148 is unnecessary for this path. This is a minor optimization opportunity, not a bug—the PSA service fetches content itself from the IPFS network.♻️ Optional optimization to skip unnecessary fetch
// 4. Fetch encrypted blob from source - let data: Uint8Array; - if (sourceKubo) { - data = await sourceKubo.get(cid); - } else { - // PSA and CipherBox: use IPFS gateway to fetch content - data = await fetchFromGateway(cid); - } - // 5. Pin to destination let destCid: string; if (destKubo) { + // Need to fetch data for Kubo destination (re-adds content) + let data: Uint8Array; + if (sourceKubo) { + data = await sourceKubo.get(cid); + } else { + data = await fetchFromGateway(cid); + } const result = await destKubo.pin(data); destCid = result.cid; } else if (destPsa) { + // PSA pins by CID reference - no need to fetch content const result = await destPsa.pinByCid(cid, `migration-${Date.now()}`); destCid = result.cid;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tee-worker/src/services/migration-worker.ts` around lines 143 - 161, The code always fetches the block into `data` via `sourceKubo.get` or `fetchFromGateway` but when `destPsa` is used the migration calls `destPsa.pinByCid(cid, ...)` and never uses `data`; avoid that unnecessary fetch by only retrieving `data` when it will be used (i.e. when `sourceKubo` is present or when `destKubo` will be used). Update the logic around `sourceKubo`, `fetchFromGateway`, `destKubo`, `destPsa`, and the subsequent `pin`/`pinByCid` branches so `data` is fetched only for the `destKubo` path (or other codepaths that actually call `destKubo.pin`), keeping `pinByCid` for `destPsa` unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/tee-worker/src/services/migration-worker.ts`:
- Around line 91-100: Wrap the decryption calls so the TEE private key returned
by getKeypair(currentEpoch) is always zeroed even if decryptEcies throws: obtain
keypair and assign teePrivateKey, then perform
decryptEcies(sourceConfigEncrypted, teePrivateKey) and
decryptEcies(destConfigEncrypted, teePrivateKey) inside a try block and move
teePrivateKey.fill(0) into a finally block; ensure any use of
sourceConfigBytes/destConfigBytes remains available after the try/finally and do
not return before the finally executes so the key is reliably wiped.
---
Nitpick comments:
In `@apps/tee-worker/src/services/migration-worker.ts`:
- Around line 143-161: The code always fetches the block into `data` via
`sourceKubo.get` or `fetchFromGateway` but when `destPsa` is used the migration
calls `destPsa.pinByCid(cid, ...)` and never uses `data`; avoid that unnecessary
fetch by only retrieving `data` when it will be used (i.e. when `sourceKubo` is
present or when `destKubo` will be used). Update the logic around `sourceKubo`,
`fetchFromGateway`, `destKubo`, `destPsa`, and the subsequent `pin`/`pinByCid`
branches so `data` is fetched only for the `destKubo` path (or other codepaths
that actually call `destKubo.pin`), keeping `pinByCid` for `destPsa` unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28dcceab-4002-4e43-9d94-d45f75cf0f8b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/tee-worker/package.jsonapps/tee-worker/src/__tests__/ssrf-validation.test.tsapps/tee-worker/src/services/migration-worker.tsapps/tee-worker/src/services/ssrf-validation.ts
✅ Files skipped from review due to trivial changes (1)
- apps/tee-worker/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/tee-worker/src/services/ssrf-validation.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 64 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- tee-worker/package-lock.json: Language not supported
Comments suppressed due to low confidence (4)
apps/tee-worker/src/routes/connection-test.ts:57
keypair.privateKey.fill(0)only runs after a successfulunwrapKey()call. If decryption/parsing throws before that line, the epoch private key remains in memory until GC. Use a try/finally around the decrypt step (or around the whole handler scope) to ensurekeypair.privateKeyis always zeroed, even on failure.
apps/tee-worker/src/services/ssrf-validation.ts:123ssrfSafeFetchcreates a newundici.Agentfor every request and never closes it. In batch paths (migration/republish probes) this can add significant overhead and may leave open sockets/resources until GC. Consider reusing/caching agents per (hostname,address,family) for a short TTL, or explicitly closing the agent after the request completes.
apps/tee-worker/src/routes/migrate.ts:35currentEpochis taken directly from the request body (or env) without validating it’s a finite integer within the supported epoch range. IfcurrentEpochis a string/float/NaN or an extreme value,getKeypair()will derive an unexpected key (e.g.,epoch-NaN) and can create noisy/abusive workloads. Consider parsing/coercing to an integer and enforcingMIN_EPOCH..MAX_EPOCH(same check as/public-key).
apps/tee-worker/src/routes/republish.ts:60- This adds a batch-size limit, but the handler still processes
entry.currentEpoch/entry.previousEpochfrom the request without validating type/range. Invalid values (string/NaN/negative/huge) can cause derivation of unintended epoch keys and potential DoS. Suggest validating epochs per entry (finite integers,MIN_EPOCH..MAX_EPOCH, andpreviousEpoch < currentEpochwhen provided) before decrypting.
…CH env var Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: ef105fc4443c
…tion worker Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1eb225052a26
…aths Wrap entire migrateBatch body in try/finally so teePrivateKey, sourceConfigBytes, destConfigBytes, and auth token bytes are zeroed even if decryptEcies or validateEndpointUrl throws. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 0aba4147d727
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/tee-worker/src/services/migration-worker.ts (1)
190-192: Consider preserving error context for debugging.The catch block silently adds the CID to the failed list without preserving the error message. This makes debugging migration failures harder when multiple CIDs fail for different reasons.
♻️ Suggested improvement to capture failure reasons
const succeeded: string[] = []; - const failed: string[] = []; + const failed: Array<{ cid: string; reason: string }> = []; for (const cid of cids) { try { // ... existing logic ... - } catch { - failed.push(cid); + } catch (err) { + failed.push({ cid, reason: err instanceof Error ? err.message : 'Unknown error' }); } }This would require updating
MigrationBatchResult.failedtype and consumers accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/tee-worker/src/services/migration-worker.ts` around lines 190 - 192, The catch block in the migration loop currently swallows errors and only pushes cid into MigrationBatchResult.failed; change it to preserve error context by capturing the thrown error (e.g., const err = e instanceof Error ? e : new Error(String(e))) and push a richer failure record (e.g., { cid, error: err.message, stack: err.stack }) instead of just cid; update the MigrationBatchResult.failed type and any consumers of MigrationBatchResult (parsing/aggregation/logging) to handle the new failure object shape so callers can inspect error messages and stacks for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/tee-worker/src/services/migration-worker.ts`:
- Around line 171-174: The current string comparison `destCid !== cid` in
migration-worker.ts will falsely fail when Kubo returns CIDv1 for an input
CIDv0; import CID from 'multiformats/cid' and perform a semantic comparison by
parsing both `cid` and `destCid`, converting each to CIDv1 (e.g., via
CID.parse(...).toV1()) and then comparing their string forms (or use CID.equals
if preferred) so equivalent content CIDs in different versions do not trigger a
CID mismatch.
---
Nitpick comments:
In `@apps/tee-worker/src/services/migration-worker.ts`:
- Around line 190-192: The catch block in the migration loop currently swallows
errors and only pushes cid into MigrationBatchResult.failed; change it to
preserve error context by capturing the thrown error (e.g., const err = e
instanceof Error ? e : new Error(String(e))) and push a richer failure record
(e.g., { cid, error: err.message, stack: err.stack }) instead of just cid;
update the MigrationBatchResult.failed type and any consumers of
MigrationBatchResult (parsing/aggregation/logging) to handle the new failure
object shape so callers can inspect error messages and stacks for debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dda14343-3ae8-432b-94b9-7e7f4508492e
📒 Files selected for processing (3)
apps/tee-worker/src/routes/migrate.tsapps/tee-worker/src/services/migration-worker.tspackages/sdk-core/src/pinning/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/sdk-core/src/pinning/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/tee-worker/src/routes/migrate.ts
…check KuboProvider.pin() returns CIDv1 (cid-version=1) but input CIDs may be CIDv0. String comparison would falsely fail. Normalize both to CIDv1 via CID.parse().toV1().equals() for correct content-addressed comparison. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 673614175e17
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
tee-worker/toapps/tee-worker/and integrate shared monorepo packages (@cipherbox/crypto,@cipherbox/core,@cipherbox/sdk-core) — eliminates vendored crypto dependencies@phala/dstack-sdkwith defensive CVM key derivation, Phala CVM docker-compose, Prometheus metrics (cipherbox_tee_*prefix), and structured JSON loggingdeploy-tee-phalajob for automated CVM deployment to Phala CloudKey changes
tee-worker/→apps/tee-worker/, vendored deps removed@cipherbox/cryptowrapKey/unwrapKey,@cipherbox/coreIPNS signing@cipherbox/sdk-coreKuboProvider/PsaProvider withfetchFninjectionProviderOptionstype withfetchFnandtimeoutMsdeploy-tee-phalajob in deploy-staging.ymlCVM deployment details
https://011f138783487e4c43ea104cfcbacf817ac4f31b-3001.dstack-pha-prod5.phala.networkTest plan
npx tsc --noEmit— clean buildvitest run— 58 tests pass, 8 todos (SSRF integration tests)mode: "cvm"🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chores