Skip to content

Keep attestation verification enabled in dev deployments - #191

Open
chris-ricketts wants to merge 1 commit into
masterfrom
fix/explicit-skip-verify-flag
Open

chris-ricketts wants to merge 1 commit into
masterfrom
fix/explicit-skip-verify-flag

Conversation

@chris-ricketts

@chris-ricketts chris-ricketts commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Why

Dev deployments need shorter retention periods, timeouts and intervals while still verifying attestation signatures. Previously, ENCLAVE_DEV=true also disabled verification.

Skipping attestation signature verification should be reserved for QEMU-based tests and require an explicit flag in addition to dev mode.

How

ENCLAVE_DEV=true selects these development settings:

Setting Dev Production
Migration intent log retention 10 minutes 10 years
Genesis retention 5 minutes 10 years
Migration intent write timeout 2 minutes 10 minutes
Clock sync interval 5 seconds 5 minutes
KMS policy Unlocked Locked

Attestation signature verification remains enabled by default in both modes. Disabling it requires both ENCLAVE_DEV=true and ENCLAVE_INSECURE_VERIFY_SKIPPED=true. The skip flag is only read in dev mode; production ignores it entirely, including malformed values.

ENCLAVE_VERIFY_CLOCK_SOURCE and ENCLAVE_MIGRATION_COOLDOWN remain independently configurable in either mode. Their defaults are now true and 24h respectively in both modes.

The QEMU test configuration explicitly disables attestation and clock-source verification and sets migration cooldown to 2s. The new skip flag is protected from SSM environment overrides.

The README and configuration comments document these defaults, the explicit QEMU settings and the SSM restrictions.

Refactors

  1. Move security defaults and dev adjustments into LoadConfig, removing setSecurityConfig.
  2. Remove Config.Dev; consumers use the resolved settings.
  3. Add Config.ClockSyncInterval, centralize its defaults and remove clockPollInterval.
  4. Replace separate production/dev migration cooldown constants with one shared default.
  5. Consolidate test environment setup and use table-driven LoadConfig tests for defaults, overrides, invalid values and dev-only flag handling.
  6. Consolidate IsDev parsing coverage and expand SSM override protection tests across both modes.
  7. Make KMS isolation tests change KMSLocked directly instead of switching the entire dev configuration.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@arkana-ai-bot arkana-ai-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — Arkana

Reviewed at head dc4b916.

Overall

The intent is correct and the security model is an improvement: separating the QEMU-only attestation bypass (ENCLAVE_INSECURE_VERIFY_SKIPPED) from the broader dev-mode shortcuts means a real dev deployment can have fast timings without silently waiving COSE verification. The flag gating, the SSM protection, and the Nix test configuration all look consistent. One genuine defect and a few smaller observations below.


Defect — typo in constant name

runtime/config.go, new constant block:

defaulMigrationCooldown = 24 * time.Hour

defaul is missing the trailing t. The constant is exported only within the package so this won't break anything at compile time, but it will make searches for default miss it and is exactly the kind of name that accretes a second "corrected" alias over time. Worth fixing before merge.


Security model — verified correct

runtime/config.go (LoadConfig):
InsecureVerifySkipped is initialised false unconditionally, then insecureVerifySkipped() is called only inside if IsDev(). A production binary never reads the env var; ENCLAVE_INSECURE_VERIFY_SKIPPED is also in nonOverridableEnv so the SSM overlay cannot inject it. The three layers of protection (initialise-to-safe, dev-gate, overlay block) are all present.

runtime/environment.go (nonOverridableEnv):
Count goes from 7 to 8. The README's prose ("Eight names are refused") and the list match. ✓


Behavioural change that tests now cover correctly

ENCLAVE_VERIFY_CLOCK_SOURCE default flips from !dev to true in both modes. The QEMU suite compensates by setting ENCLAVE_VERIFY_CLOCK_SOURCE=false explicitly (nix/tests/default.nix, line +183). The new TestLoadConfigVerifyClockSource table confirms the unset → true default holds in both prod and dev, and that an explicit override wins either way.


TestVerifySuccessorAttestation (runtime/migrate_test.go, line 862)

Old: newTestConfig(…, true) → implicitly InsecureVerifySkipped=true, KMSLocked=false.
New: newTestConfig(…, false) + explicit KMSLocked=falseInsecureVerifySkipped=false.

The test exercises "rejects a claim with another lock posture," so only KMSLocked matters for the assertion. InsecureVerifySkipped on the successor config has no effect on what the predecessor's NSM does when verifying, so the change is safe. That said, the old implicit coupling (dev → InsecureVerifySkipped) was exactly the thing this PR removes — the explicit mutation is cleaner.


newTestConfig no longer mirrors LoadConfig exactly for InsecureVerifySkipped

After the PR, newTestConfig(…, true) leaves InsecureVerifySkipped=false (zero value). LoadConfig with ENCLAVE_DEV=true and ENCLAVE_INSECURE_VERIFY_SKIPPED=true sets it true. Any test that goes through newTestConfig and needs InsecureVerifySkipped=true must now set it explicitly — a comment on newTestConfig noting this intent would help future editors avoid recreating the old implicit coupling.


Missing positivity assertion for MigrationCooldown in dev

The old TestSecurityEnvelopeDurationsArePositive asserted MigrationCooldown > 0 for both postures. That test is removed and the replacement TestLoadConfigSecurityDefaults asserts MigrationCooldown == 24h (positive by inspection). TestLoadConfigMigrationCooldown explicitly tests the 0s override path. Nothing is wrong here, but the explicit require.Positive was a useful guard for future constant changes — worth keeping as a one-liner.


Nits

  • runtime/clocksync_linux.go: The comment about the Evervault cadence moved from the now-deleted clockSyncPollInterval into config.go on prodClockSyncInterval. Good — the comment travels with the constant.
  • runtime/config_test.go (setConfigTestEnv): Explicitly blanking ENCLAVE_MIGRATION_COOLDOWN, ENCLAVE_VERIFY_CLOCK_SOURCE, and ENCLAVE_INSECURE_VERIFY_SKIPPED via t.Setenv(…, "") gives the table-driven tests proper isolation. ✓

Summary

One change needed before merge: fix the defaulMigrationCooldown typo (runtime/config.go). Everything else looks correct. The security hardening is sound, the test coverage of the new flag parsing matches the old coverage of VerifyClockSource, and the Nix test matrix sets all three bypass knobs explicitly.

@arkana-ai-bot arkana-ai-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Keep attestation verification enabled in dev deployments

Good security improvement — this fixes a footgun where ENCLAVE_DEV=true silently disabled COSE attestation signature verification in all dev deployments, not just QEMU test harnesses.

What it changes

Splits the old monolithic ENCLAVE_DEV=true behaviour into orthogonal flags:

  • ENCLAVE_DEV=true now only controls: shorter Object Lock retentions, intent write timeout, clock-sync interval, and KMS unlock policy. Attestation verification remains enabled by default in dev.
  • ENCLAVE_INSECURE_VERIFY_SKIPPED=true (only honoured when ENCLAVE_DEV=true) explicitly opts into skipping COSE verification — for QEMU-based tests only. Baked into the EIF, not SSM-overridable.
  • ENCLAVE_VERIFY_CLOCK_SOURCE (default true) can be set independently in either prod or dev.
  • ENCLAVE_MIGRATION_COOLDOWN defaults to 24h in both modes (was 2s in dev before).

The test EIF now sets all flags explicitly (ENCLAVE_DEV=true, ENCLAVE_INSECURE_VERIFY_SKIPPED=true, ENCLAVE_VERIFY_CLOCK_SOURCE=false, ENCLAVE_MIGRATION_COOLDOWN=2s), which is clearer and correct.

Security assessment

The previous design was a trap: operators who set ENCLAVE_DEV=true to get shorter retentions or an unlocked KMS policy during staging inadvertently disabled attestation verification on their staging deployment. The new design requires explicit opt-in to skip verification. ENCLAVE_INSECURE_VERIFY_SKIPPED being EIF-baked means it cannot be enabled remotely after deployment — good.

Verdict

Looks ready to merge. The security improvement is meaningful, the test scaffolding is correct, and the documentation is thorough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants