enclave support#5
Conversation
|
@aruokhai merge with master to make CI green |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔒 Arkana Code Review — introspector enclave framework
Verdict: Request Changes
This PR adds AWS Nitro Enclave scaffolding to protect the introspector signing oracle's private key. The architecture (KMS-backed secrets, attestation via nitriding, reproducible Nix builds) is sound. However, there are several configuration bugs that will prevent the app from starting inside the enclave.
🚨 PROTOCOL-CRITICAL — Requires human review
The introspector is a signing oracle that signs forfeit transactions, commitment transactions, and VTXO tree operations (SubmitFinalization, SubmitTx). The enclave protects the signing key. Misconfiguration here means either:
- The key leaks (enclave doesn't boot, fallback to unprotected mode)
- The service doesn't start (signing oracle offline → rounds stall)
A human must sign off on this before merge.
🔴 Critical Issues
1. Missing INTROSPECTOR_NO_TLS=true — app will fail to start in enclave
enclave/enclave.yaml:44 — The env block only sets INTROSPECTOR_PORT. But the introspector app generates self-signed TLS certs by default (internal/interface/grpc/service.go:57-64). Inside the enclave, nitriding already handles TLS termination. The app needs INTROSPECTOR_NO_TLS=true to run in plaintext mode behind nitriding, otherwise it will either:
- Fail to generate certs (no writable TLS datadir inside the minimal rootfs)
- Double-encrypt traffic (TLS inside TLS), breaking the nitriding reverse proxy
env:
INTROSPECTOR_PORT: "7074"
INTROSPECTOR_NO_TLS: "true" # ← add this2. Missing INTROSPECTOR_ARKD_URL — app crashes on startup
internal/config/config.go:78-80 — LoadConfig() returns a fatal error if ArkdURL is empty. The enclave config doesn't set this env var anywhere. The app will crash immediately on boot.
env:
INTROSPECTOR_PORT: "7074"
INTROSPECTOR_ARKD_URL: "..." # ← required, app won't start without it3. Missing INTROSPECTOR_DATADIR — default path won't exist in enclave
internal/config/config.go:27 — Default datadir is ~/.introspector (via arklib.AppDataDir). The enclave rootfs (flake.nix:119) only has /app/data as a writable directory. Without setting INTROSPECTOR_DATADIR=/app/data, the app will fail when trying to write to a non-existent home directory.
🟡 Medium Issues
4. account: "" in enclave.yaml will cause deploy failures
enclave/enclave.yaml:7 — AWS account ID is blank. If the enclave CLI doesn't validate this early, deploys will fail with cryptic AWS errors. Either add a placeholder with a comment, or add validation.
5. Port mismatch may confuse operators
enclave/enclave.yaml:44 sets INTROSPECTOR_PORT: "7074" but the app's default is 7073 (internal/config/config.go:28). This is fine functionally (the env var overrides the default), but the PR description references port 7074 as the "application port" without explaining why it differs from the codebase default. Add a comment in the YAML explaining the runtime supervisor expects port 7074.
6. CI workflow silently swallows manifest publish failures
.github/workflows/build-eif.yml:65 — continue-on-error: true on the "Publish build manifest" step means failed manifest publishes are invisible. For a signing oracle, you want to know if the attestation manifest (PCR values) failed to publish. At minimum, add an if: failure() notification step, or remove continue-on-error.
7. eif-latest release has a TOCTOU race
.github/workflows/build-eif.yml:53-62 — The workflow deletes the eif-latest release then recreates it. If two builds run concurrently (e.g., rapid pushes), one can delete the other's release mid-upload. Consider using a unique tag per build and updating a latest pointer atomically.
🟢 Looks Good
- Signing key via KMS + attestation: The
secretsblock correctly mapssigning_key→INTROSPECTOR_SECRET_KEY, fetched via enclave attestation. Key never touches disk. - Reproducible builds via Nix: Pinned nixpkgs, pinned app commit SHA, vendor hashes — good for PCR reproducibility.
- Nix flake structure: Clean separation of runtime supervisor and user app. The
--impurerequirement forBUILD_CONFIG_PATHis documented. - Flake lock pins: All inputs are locked with specific revs and narHashes.
Summary
Fix the three critical env var issues (NO_TLS, ARKD_URL, DATADIR) — without them, the introspector will not start inside the enclave. The enclave framework itself is well-structured, but the app-specific configuration is incomplete.
There was a problem hiding this comment.
🔒 Protocol-Critical Review — Enclave Framework
Requesting changes + mandatory human review. This PR manages the INTROSPECTOR_SECRET_KEY signing key lifecycle via AWS Nitro Enclave attestation. The introspector signing key is used by bancod for transaction signing — bugs here lose real money.
⛔ CRITICAL — Must fix before merge
1. enclave/enclave.yaml:12 — is_kms_key_locked: false with no production gate
is_kms_key_locked: falseWith this setting, AWS root can kms:PutKeyPolicy on the locked key and decrypt the signing key outside the enclave. The comment explains the tradeoff, but there's no mechanism to prevent this config from reaching production. Consider:
- Add a CI check or deploy-time assertion that production deployments require
is_kms_key_locked: true - At minimum, add a prominent
# ⚠️ MUST be true for productioncomment
2. enclave/enclave.yaml:10 — account: "" (empty)
account: "" # AWS account ID (required)The field is marked "required" but is empty. If the enclave CLI doesn't validate this early, it'll fail late with a confusing error (or worse, silently default). Either populate it or add a CI validation step.
⚠️ HIGH — Should fix
3. .github/workflows/build-eif.yml:57-61 — Mutable eif-latest release is a supply-chain risk
gh release delete "$TAG" --yes 2>/dev/null || true
gh release create "$TAG" ...Every push to master replaces the EIF at eif-latest. Since enclave.yaml:42 sets release_tag: "eif-latest", anyone running enclave tofu --remote gets whatever was last built. There's no signature verification or pinning mechanism in this workflow. Consider:
- Sign the EIF artifact or publish a checksum signed by a known key
- Use immutable version-tagged releases for production (keep
eif-latestfor dev only)
4. .github/workflows/build-eif.yml:93-108 — TOCTOU race + silent failures
continue-on-error: trueThe Publish build manifest step checks if deployment.json exists in the latest release, then deletes and recreates it. Two concurrent builds race on this. The continue-on-error: true swallows all failures — including legitimate ones like auth failures or malformed JSON. At minimum, don't suppress the exit code; let the check-then-act race be visible.
5. enclave/flake.nix:24-26 — configPath default diverges from enclave repo
if p != "" then p else "../.enclave/build-config.json";The enclave repo uses "./enclave/build-config.json". The enclave-skeleton repo uses "/src/enclave/build-config.json". This PR uses "../.enclave/build-config.json". Three repos, three different defaults. This will cause confusion. Should this match the enclave repo's convention since both are for the same project?
📋 MEDIUM — Consider fixing
6. enclave/flake.nix:82-88 — Fragile binary rename
for f in $out/bin/*; do
if [ "$(basename "$f")" != "${appCfg.binary_name}" ]; then
mv "$f" "$out/bin/${appCfg.binary_name}"
fi
doneIf nix_sub_packages produces multiple binaries (currently just cmd, but future changes could add more), the loop renames the last one and leaves others behind — or clobbers the correct binary. Pre-existing pattern from skeleton, but worth hardening given this is production infra.
7. enclave/flake.nix:37 vs enclave/flake.nix:61 — Go version inconsistency
The runtime build uses eifBuildGoModule (which overrides Go to go_1_26), and upstream-app also uses eifBuildGoModule. Good — they match. But the vendor-hash-check (line 160) also uses eifBuildGoModule, which means it builds with Go 1.26 even though its only purpose is to discover the vendor hash. Not a bug, just unnecessary coupling.
8. No tests
This PR adds 477 lines of infra code with zero tests. The build-eif.yml workflow serves as a partial integration test (it'll fail if the Nix build breaks), but there's no validation that:
enclave.yamlvalues are internally consistent- The pinned
nix_rev(67616e2) produces a working binary - The runtime version (
v0.0.72) is compatible with this app version
✅ What looks good
- Reproducible builds via Nix — pinned nixpkgs
nixos-25.11, pinnednix_rev, pinned runtimev0.0.72. Correct approach. nix_revverified — commit67616e2exists and isuse ark-lib to verify tapscript signatures (#73), a recent master commit. 3 commits behind HEAD but those are test-only (fuzz).- Nitriding/viproxy vendored into runtime — cleaner than the skeleton approach with 4 separate binaries. Entrypoint is directly
/app/runtime. - Secret management pattern — KMS-encrypted signing key decrypted only inside the enclave via PCR0 attestation. This is the correct architecture.
- Cross-compilation support —
eachSystemincludes ARM and macOS for dev builds; EIF always targets x86_64-linux.
🔍 Cross-repo impact
- bancod is the primary consumer (
pkg/arkade,pkg/client,api-spec). This PR does NOT change any of those packages — it only adds theenclave/directory. No downstream breakage. - banco (TypeScript) consumes introspector via HTTP. No API changes here. No breakage.
- enclave repo has a very similar but divergent
flake.nix. These should be kept in sync or one should be canonical.
louisinger
left a comment
louisinger
left a comment
There was a problem hiding this comment.
we need to document this flow in a new enclave/README file describing each file, link to potential documentation etc...
…add new environment variables to enclave configuration
There was a problem hiding this comment.
🔒 Arkana Follow-up Review — New Commits (24b8874..955652a)
Good progress. The major structural changes (removing host-side scaffolding, adding immutable releases, fixing env vars) are solid. Three of my original critical issues are resolved. One new critical issue found.
✅ Fixed since last review
INTROSPECTOR_NO_TLS: "true"added —enclave/enclave.yaml:46. Correct. This also makesINTROSPECTOR_DATADIRnon-critical (datadir is only used for TLS cert storage, which is now skipped).INTROSPECTOR_ARKD_URLadded —enclave/enclave.yaml:47. Partially fixed (see critical #1 below).INTRPSOECTOR_SIGNING_KEYtypo fixed →INTROSPECTOR_SECRET_KEY—enclave/enclave.yaml:50. Good catch.continue-on-error: trueremoved from CI. Build failures are now visible.- Immutable per-commit releases added —
.github/workflows/build-eif.yml:62-78. Production can now pin toeif-${COMMIT_SHA::12}. - Host-side scaffolding removed (gvproxy/, systemd/, scripts/, user_data/). The enclave CLI handles all of this now. Cleaner.
🔴 CRITICAL — Must fix
1. enclave/enclave.yaml:47 — {{arkd_url}} is not a valid template variable
INTROSPECTOR_ARKD_URL: "{{arkd_url}}"The enclave CLI (cli/build.go:144-146 in introspector-enclave) only expands three template variables: {{region}}, {{prefix}}, {{version}}. The string {{arkd_url}} will be baked into the EIF as the literal string "{{arkd_url}}". At runtime, LoadConfig() (internal/config/config.go:78) will see a non-empty ArkdURL, pass validation, and then the app will try to dial {{arkd_url}} as a gRPC endpoint — instant crash.
Fix options:
- (a) Add
arkd_urlas a supported template var in the enclave CLI, or - (b) Construct the URL from existing vars:
"https://{{prefix}}-arkd.{{region}}.your-domain.com", or - (c) Pass
INTROSPECTOR_ARKD_URLas a runtime secret (not baked into the EIF) so it can vary per deployment without rebuilding
Option (c) is best for a signing oracle — you don't want to rebuild the EIF (and change PCR0) just because the arkd endpoint changes.
🟡 Still open from previous reviews
2. enclave/enclave.yaml:7 — account: "" still empty
Flagged twice now. Either populate it with a placeholder + comment, or add validation in CI/the enclave CLI so enclave build fails early with a clear error.
3. enclave/enclave.yaml:40 — release_tag: "eif-latest" contradicts the workflow's own guidance
The workflow comment at .github/workflows/build-eif.yml:80 says:
Prod must NOT pin here.
But enclave.yaml sets release_tag: "eif-latest", which is what enclave tofu --remote uses. Anyone running enclave tofu --remote against this config gets the mutable release. Should be an immutable tag for production configs, or at minimum add a comment that this must be changed for prod.
4. No enclave/README.md — @louisinger requested this in a previous review. Still missing.
✅ New code looks good
- Nix flake (
enclave/flake.nix): Well-structured. Reproducible build with pinned nixpkgs, pinned runtime, cross-compilation support.enclaveEnvcorrectly propagates all app env vars.secretsCfgJsoncorrectly serializes the secrets config for runtime discovery. - Workflow rewrite (
.github/workflows/build-eif.yml): Much cleaner. Immutable releases with idempotentif ! gh release viewguard. Timestamped build manifests. Thelatestrelease conditional (grep -q deployment.json) correctly avoids overwriting deploy manifests. - Scaffolding removal: Removing gvproxy, systemd services, user_data, and scripts in favor of the enclave CLI managing these is the right call. Less surface area, fewer config drift opportunities.
Summary
One critical issue: {{arkd_url}} won't be expanded and will crash the app at runtime. Fix that, add the README, and this is close to mergeable.
There was a problem hiding this comment.
🔒 Arkana Review — New Commit (955652a → efa6bee)
The workflow rewrite from auto-build-on-push to manual versioned releases is the correct direction. One new issue introduced, two prior issues still open.
✅ What this commit does right
- Manual
workflow_dispatchwith version input — eliminates accidental EIF publishes on every push. Operators explicitly choose when to cut a release. Good. - Strict version validation — regex
^v[0-9]+\.[0-9]+\.[0-9]+$, duplicate release check, duplicate tag check, all withset -euo pipefail. Solid. --target "$COMMIT_SHA"ongh release create— pins the git tag to the exact commit that was checked out, not HEAD at tag-creation time. Correct.deployment.jsonbundled in the release — consumers can fetch PCR values and commit hash from the same release. Cleaner than the previous separatebuild-*/latestrelease dance.- Artifact retention bumped to 30 days — reasonable for a signing oracle.
🔴 CRITICAL — enclave.yaml:42 — release_tag: "eif-latest" is now broken
The old workflow created a mutable eif-latest release on every build. The new workflow only creates versioned releases (e.g., v0.0.29). No code path creates eif-latest anymore.
But enclave.yaml still says:
release_tag: "eif-latest"enclave tofu --remote uses this tag to download the EIF from GitHub Releases. It will try to fetch from .../releases/download/eif-latest/image.eif — which will never exist under the new workflow. Every remote deployment will fail.
Fix: Change to a real version tag, or add a comment that this must be set before running enclave tofu --remote:
release_tag: "v0.0.1" # Must match a Release EIF workflow run🟡 Still open from previous reviews (unfixed)
1. enclave.yaml:47 — {{arkd_url}} is not a valid template variable (flagged in reviews #1 and #2)
The enclave CLI only expands {{region}}, {{prefix}}, {{version}}. The literal string {{arkd_url}} will be baked into the EIF and crash the app at runtime when it tries to dial it as a gRPC endpoint. This is a runtime crash for the signing oracle.
2. enclave.yaml:7 — account: "" still empty (flagged in reviews #1 and #2)
3. No enclave/README.md — requested by @louisinger in a previous comment.
🟡 Minor — release-eif.yml:52 — enclave CLI not pinned
run: go install github.com/ArkLabsHQ/introspector-enclave/cli/cmd/enclave@latest@latest means different builds could silently use different CLI versions, producing different EIF images (and different PCR0 values) from identical source. For a signing oracle where PCR0 is the trust anchor, this is risky. Pin to a specific version:
run: go install github.com/ArkLabsHQ/introspector-enclave/cli/cmd/[email protected](This was also true of the old workflow, but now that releases are explicit and versioned, reproducibility matters more.)
Summary
The release model change is a clear improvement — explicit, versioned, immutable. But release_tag: "eif-latest" is now inconsistent with the workflow and will break remote deployments. The {{arkd_url}} runtime crash from review #2 is still unfixed.
Fix the critical items and this is ready.
|
@louisinger This env seems to be problematic INTROSPECTOR_ARKD_URL, because we need to have all environment variables baked into the EIF. I reckon that it will be different for mutiny and mainnet. |
I need more context to understand. What is EIF and why it's a problem if mutiny/mainnet env is different ? do we need/want them to be public in the repository ? |
EIF is similar to a Docker image, but for an enclave. The yaml file is used for building the enclave and deployment. For now, we bake the env variables into the EIF file. I reckon that I can include a runtime configuration functionality, so deployment can configure such environment variables. |
There was a problem hiding this comment.
🔒 Arkana Review — New Commit (efa6bee → 99269a3)
New: enclave/README.md ✅
Good addition — addresses @louisinger's request. Clear instructions for CLI install, commit pinning, local builds, and tagged releases.
One inconsistency in the README itself:
-
README.md:8— Documents@latestinstall but then recommends pinning at line 16. The primary install command should use a pinned version since this is a signing oracle. Suggest swapping the order: pinned first,@latestas a "for development only" note. -
README.md:67— States "Production deploys should pinapp.release_tag… never to a moving tag." Good advice, butenclave.yaml:38still hasrelease_tag: "eif-latest", contradicting the README's own guidance.
🔴 Critical issues from previous reviews — STILL UNFIXED
1. enclave.yaml:45 — {{arkd_url}} is not a valid template variable (flagged in reviews #1, #2, #3)
The enclave CLI only expands {{region}}, {{prefix}}, {{version}}. The literal string {{arkd_url}} will be baked into the EIF. The introspector will attempt to dial {{arkd_url}} as a gRPC endpoint and crash. This is a runtime crash for the signing oracle.
2. enclave.yaml:38 — release_tag: "eif-latest" is inconsistent with the workflow (flagged in review #3)
The Release EIF workflow creates versioned releases (v0.1.0), not eif-latest. enclave tofu --remote will try to fetch from a release that doesn't exist. Every remote deployment will fail.
3. enclave.yaml:7 — account: "" still empty (flagged in reviews #1, #2, #3)
4. release-eif.yml:49 — enclave CLI not pinned (flagged in review #3)
@latest in CI means different builds can silently use different CLI versions, producing different EIF images with different PCR0 values. For a signing oracle where PCR0 is the trust anchor, pin to a specific version.
These are the same issues from my last three reviews. The README is a welcome addition but the critical config bugs need fixing before this can merge.
There was a problem hiding this comment.
🔒 Arkana Review — New Commit (efa6bee → 99269a3)
New: enclave/README.md ✅
Good addition — addresses @louisinger's request. Clear instructions for CLI install, commit pinning, local builds, and tagged releases.
One inconsistency in the README itself:
-
README.md:8— Documents@latestinstall but then recommends pinning at line 16. The primary install command should use a pinned version since this is a signing oracle. Suggest swapping the order: pinned first,@latestas a "for development only" note. -
README.md:67— States "Production deploys should pinapp.release_tag… never to a moving tag." Good advice, butenclave.yaml:38still hasrelease_tag: "eif-latest", contradicting the README's own guidance.
🔴 Critical issues from previous reviews — STILL UNFIXED
1. enclave.yaml:45 — {{arkd_url}} is not a valid template variable (flagged in reviews #1, #2, #3)
The enclave CLI only expands {{region}}, {{prefix}}, {{version}}. The literal string {{arkd_url}} will be baked into the EIF. The introspector will attempt to dial {{arkd_url}} as a gRPC endpoint and crash. This is a runtime crash for the signing oracle.
2. enclave.yaml:38 — release_tag: "eif-latest" is inconsistent with the workflow (flagged in review #3)
The Release EIF workflow creates versioned releases (v0.1.0), not eif-latest. enclave tofu --remote will try to fetch from a release that doesn't exist. Every remote deployment will fail.
3. enclave.yaml:7 — account: "" still empty (flagged in reviews #1, #2, #3)
4. release-eif.yml:49 — enclave CLI not pinned (flagged in review #3)
@latest in CI means different builds can silently use different CLI versions, producing different EIF images with different PCR0 values. For a signing oracle where PCR0 is the trust anchor, pin to a specific version.
These are the same issues from my last three reviews. The README is a welcome addition but the critical config bugs need fixing before this can merge.
🔒 Arkana Review — New Commit (99269a3 → 529499c)
|
| # | File:Line | Issue | First flagged |
|---|---|---|---|
| 1 | enclave.yaml:45 |
{{arkd_url}} is not a valid template var → literal string baked into EIF → runtime crash |
Review #1 |
| 2 | enclave.yaml:38 |
release_tag: "eif-latest" doesn't match workflow releases (vX.Y.Z) → 404 on deploy |
Review #3 |
| 3 | enclave.yaml:7 |
account: "" empty — required field |
Review #1 |
| 4 | release-eif.yml:49 |
enclave CLI @latest in CI → non-deterministic PCR0 |
Review #3 |
Issues 1 and 2 are deployment blockers.
- Introduced a Dockerfile to build and run a mock version of the arkd gRPC service. - Created go.mod and go.sum files to manage dependencies for the mock service. - Implemented main.go to define the mock service with a GetInfo method. - Added run.sh script for end-to-end local enclave testing, including setup for mock services and integration tests. - Created seed.yaml for initial configuration of keys and aliases.
- Updated `docker-compose.yml` to replace `local-kms` and `kms-proxy` with `aws-mocks` service for improved AWS service mocking. - Removed `heartbeat.py` as it is no longer needed for the test setup. - Modified `integration-test.sh` to build the `grpc-client` dynamically and updated paths for PCR file. - Deleted `run.sh` as it was overly complex and integrated into the Docker setup. - Removed `seed.yaml` as it is no longer required for the test configuration.
2 participants
Summary
What's included
Configuration (
enclave/enclave.yaml)Declarative enclave deployment config specifying:
m6i.xlarge)dcec46c)7074) and secret references (signing key via KMS)Networking (
enclave/gvproxy/)192.168.127.2)Enclave entrypoint (
enclave/start.sh)Orchestrates the enclave boot sequence:
192.168.127.1)Host-side init (
enclave/scripts/enclave_init.sh)Starts the Nitro Enclave via
nitro-cli run-enclavewith configurable CPU, memory, and CID. Polls until the enclave exits, designed to run under systemd withRestart=always.Systemd services (
enclave/systemd/)enclave-watchdog.serviceenclave-imds-proxy.servicegvproxy.serviceEC2 user data (
enclave/user_data/user_data)Cloud-init script that provisions a fresh EC2 instance:
/etc/environment