Skip to content

refactor(guest): drop the unread mr-kms launch event - #1007

Merged
kvinwang merged 2 commits into
masterfrom
fix/drop-mr-kms
Aug 6, 2026
Merged

refactor(guest): drop the unread mr-kms launch event#1007
kvinwang merged 2 commits into
masterfrom
fix/drop-mr-kms

Conversation

@kvinwang

@kvinwang kvinwang commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Problem

mr-kms is a write-only measurement. dstack-util extends RTMR3 with the KMS
CVM's mr_aggregated after every GetAppKey, and nothing anywhere reads it
back. Grepping the whole repo — Rust, Solidity, TypeScript, Python, Go, the four
SDKs, test fixtures, event-log snapshots — finds no lookup by name:

$ rg -n 'mr[-_]kms' --glob '!CHANGELOG.md'
dstack/dstack-util/src/system_setup.rs:2069:  warn!("Skipping mr-kms runtime event for ...")
dstack/dstack-util/src/system_setup.rs:2093:  emit_runtime_event("mr-kms", &kms_measurement)
dstack/kms/src/config.rs:66:                  /// extend `mr-kms`, so a remote verifier can still tell
dstack/kms/src/main.rs:127:                   mr-kms. Intended for local development only
docs/tutorials/attestation-verification.md:529

Two emitters and three sentences of prose. dstack/verifier/ never mentions it;
the only path that could surface the name is verification.rs:96, which copies
event.event into an RTMR-mismatch diagnostic and therefore never runs on a
successful verification.

Three things follow from having no reader:

  1. The one piece of documentation is wrong. attestation-verification.md:529
    describes the payload as "KMS public key hash". It is the KMS's
    mr_aggregated. Nobody noticed because nobody consumes it.
  2. Absence is ambiguous. The event is emitted only when the KMS RPC
    certificate carries a decodable attestation, and skipped for
    attest_rpc_cert = false or an unsupported quote type. A missing mr-kms
    could mean an unattested KMS, an unsupported platform, or an older guest —
    indistinguishable. kms/src/config.rs:66 claims "a remote verifier can still
    tell"; with no reader and no way to disambiguate, it cannot.
  3. It made the TLS handshake carry a side effect. Capturing the value across
    the callback needed an Arc<Mutex<Option<[u8; 32]>>> cloned into the
    cert_validator closure, a decode_app_info call, and a bespoke
    is_unsupported_app_info_quote string-matching helper for the platforms where
    that decode is not supported — all inside what is otherwise a certificate
    check.

The intent was KMS build accountability, and that need is real: the KMS root key
survives KMS upgrades by design (onboarding replicates it), so key-provider,
which records the root CA public key, cannot distinguish two KMS builds sharing
one root. But the enforced answer already exists on-chain in
DstackKms.kmsAllowedAggregatedMrs, checked by isKmsAllowed when a KMS boots
and asks for that root key. mr-kms was an unenforced echo of it.

Fix

Remove the event and everything that existed only to produce it.

request_app_keys_from_kms_url loses the mutex, the clone, the decode_app_info
call and the take-and-emit block. cert_validator captures nothing now, so it
becomes a plain function next to the file's other one, AppIdValidator:

.cert_validator(Box::new(validate_kms_rpc_cert))

Dropping decode_app_info from the validator removes no check.
ra-rpc/src/client.rs:157 runs verify_with_ra_pubkey — report-data binding to
the certificate SubjectPublicKeyInfo, quote signature, collateral — before
calling the validator. decode_app_info only parsed identity out of the KMS's
event log to reach mr_aggregated, and with mr-kms gone nothing consumes that
log. The KMS identity the guest actually enforces is its CA public key, pinned by
verify_key_provider_id against app_compose.key_provider_id.

The stale prose in kms/src/config.rs, kms/src/main.rs, kms/kms.toml and the
incorrect docs row go with it.

Alternative considered

Finishing the feature instead: have dstack-verifier surface mr-kms in
VerificationResponse.details, cross-check it against
kmsAllowedAggregatedMrs, and emit it unconditionally so absence stops being
ambiguous. Rejected for now — it is a new verification surface with an on-chain
dependency, and shipping it should be a deliberate design change rather than a
rescue of an event that has had no reader since #135. Removing it first keeps the
launch log honest about what is actually checked; the accountability question can
be reopened on its own terms.

Compatibility

No allowlisted measurement changes. boot-mr-done is emitted at
system_setup.rs:2641, before request_app_keys() at :2717, so mr-kms
always landed after the boot-time snapshot. The on-chain KMS and app policies
read that truncated snapshot (replay_runtime_events(.., Some("boot-mr-done"))),
which never contained it. Registered mrAggregated and deviceId entries are
untouched.

Only the runtime RTMR3 of newly built guest images changes, and no authorization
path reads it. Event-log replay is name-agnostic, so an existing CVM whose log
contains mr-kms still replays to its quoted RTMR3 and verifies unchanged — no
verifier-side change is needed for either generation.

Verification

  • rg -n 'mr[-_]kms' over the worktree returns only CHANGELOG.md history.
  • cargo clippy -p dstack-util -p dstack-kms --all-targets: no new warnings
    (the two remaining ones are pre-existing, in amd_attest.rs and
    onboard_service.rs).
  • cargo test -p dstack-util -p dstack-kms: 102 passed, 0 failed.
  • Confirmed the removed decode is redundant by reading the RA-TLS client path:
    ra-rpc/src/client.rs:145-165 verifies the attestation and only then invokes
    the validator.
  • Confirmed the emission order claim from the source: boot-mr-done at
    system_setup.rs:2641 precedes request_app_keys() at :2717.

Copilot AI lite review requested due to automatic review settings August 5, 2026 08:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR removes the unused mr-kms runtime measurement event emission from the guest launch path, and cleans up related KMS configuration prose and documentation so the codebase reflects what is actually verified/enforced.

Changes:

  • Remove mr-kms emission and the associated RA-TLS certificate parsing/mutex capture from dstack-util guest setup; replace the inline closure with a standalone validate_kms_rpc_cert function.
  • Update KMS warning/config docs (kms/src/main.rs, kms/src/config.rs, kms/kms.toml) to drop references to mr-kms.
  • Remove the mr-kms row from the attestation verification tutorial event list.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
dstack/kms/src/main.rs Updates warning text when attest_rpc_cert = false to remove mr-kms mention.
dstack/kms/src/config.rs Removes stale description about guests extending mr-kms when KMS RPC cert is unattested.
dstack/kms/kms.toml Updates config comments to match the new behavior/documentation (no mr-kms).
dstack/dstack-util/src/system_setup.rs Drops mr-kms emission and related certificate attestation decoding; introduces validate_kms_rpc_cert.
docs/tutorials/attestation-verification.md Removes the mr-kms expected event entry.
Suppressed comments (1)

dstack/dstack-util/src/system_setup.rs:1955

  • Error message capitalization: per the repo style guide, error messages should start lowercase (CLAUDE.md:149-154).
    if usage != "kms:rpc" {
        bail!("Invalid server cert usage: {usage}");
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread dstack/dstack-util/src/system_setup.rs
kvinwang and others added 2 commits August 5, 2026 20:26
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@kvinwang
kvinwang enabled auto-merge August 6, 2026 03:28
@kvinwang
kvinwang merged commit da983e6 into master Aug 6, 2026
15 checks passed
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