Keep attestation verification enabled in dev deployments - #191
chris-ricketts wants to merge 1 commit into
Conversation
arkana-ai-bot
left a comment
There was a problem hiding this comment.
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.Hourdefaul 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=false → InsecureVerifySkipped=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-deletedclockSyncPollIntervalintoconfig.goonprodClockSyncInterval. Good — the comment travels with the constant.runtime/config_test.go(setConfigTestEnv): Explicitly blankingENCLAVE_MIGRATION_COOLDOWN,ENCLAVE_VERIFY_CLOCK_SOURCE, andENCLAVE_INSECURE_VERIFY_SKIPPEDviat.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
left a comment
There was a problem hiding this comment.
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=truenow 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 whenENCLAVE_DEV=true) explicitly opts into skipping COSE verification — for QEMU-based tests only. Baked into the EIF, not SSM-overridable.ENCLAVE_VERIFY_CLOCK_SOURCE(defaulttrue) can be set independently in either prod or dev.ENCLAVE_MIGRATION_COOLDOWNdefaults 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.
Why
Dev deployments need shorter retention periods, timeouts and intervals while still verifying attestation signatures. Previously,
ENCLAVE_DEV=truealso 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=trueselects these development settings:Attestation signature verification remains enabled by default in both modes. Disabling it requires both
ENCLAVE_DEV=trueandENCLAVE_INSECURE_VERIFY_SKIPPED=true. The skip flag is only read in dev mode; production ignores it entirely, including malformed values.ENCLAVE_VERIFY_CLOCK_SOURCEandENCLAVE_MIGRATION_COOLDOWNremain independently configurable in either mode. Their defaults are nowtrueand24hrespectively 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
LoadConfig, removingsetSecurityConfig.Config.Dev; consumers use the resolved settings.Config.ClockSyncInterval, centralize its defaults and removeclockPollInterval.LoadConfigtests for defaults, overrides, invalid values and dev-only flag handling.IsDevparsing coverage and expand SSM override protection tests across both modes.KMSLockeddirectly instead of switching the entire dev configuration.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.