Repository navigation
Conversation
c1cfe16 to
2f6a1f9
Compare
2f6a1f9 to
3d5f600
Compare
kvinwang
left a comment
There was a problem hiding this comment.
Review after marking ready. The guest-side change still makes sense. The onboarding half needs rework on top of current next, and the PR needs a rebase.
1. The onboarding commit is superseded by next and would break the path it touches.
Since this branch was cut, #1302/#1406 moved handover to Admin.GetKmsKey. With a source_token, Keys::onboard on next already does what this commit does: a self-issued cert on a single client that carries the cert_validator, with ensure_kms_allowed run after the fetch. The tokenless path that remains exists for sources older than 0.6.0, which serve KMS.GetKmsKey publicly (public_key_handover) and pin their temp CA. Moving that path to self-issued certs would make it refuse exactly the sources it is there for.
The binding gap this PR fixes is still present on that path, though. RaClient::new_mtls sets no validator, so the source attestation is checked on the GetTempCaCert connection while the keys arrive on another one. Suggested shape: drop this commit's onboarding change. On the tokenless path, keep minting from the temp CA for compatibility, attach the same capturing validator to the mTLS client, and run ensure_kms_allowed on the attestation captured from the connection that returned the keys. new_mtls can then go.
2. The verify_key_provider_id claim is too strong (code comment in request_app_keys_from_kms_url, CHANGELOG, PR body).
verify_key_provider_id returns Ok(()) when app_compose.key_provider_id is empty, which is the default, so in general a substituted CA does survive to key use. The posture is unchanged from GetTempCaCert, and that is fine. The binding a relying party checks is the key-provider launch event (RTMR3/PCR14) against the on-chain kmsInfo or a pinned key, with verify_key_provider_id as an extra in-guest check only when the app pins it. Please word it that way.
3. Compatibility.
A guest built from this change fails at the TLS handshake against any KMS ≤ 0.5.11. With 0.6.0 released that is acceptable, but the failure surfaces as a bare handshake error at boot. Consider adding context to that error, e.g. "KMS refused a self-issued RA-TLS certificate; the KMS must be ≥ 0.6.0".
4. Nits.
- Dropping
tmp_ca_key/tmp_ca_certfrom the publicdstack_types::KeyProvider::Kmsbreaks out-of-tree readers that deserialize new.appkeys.jsonfiles with those fields required. That is probably acceptable; worth a line in the CHANGELOG entry next to the existing "readers ignore them" note. - The onboarding
gen_ra_certduplicates the body ofgenerate_self_signed_ra_certexcept for the quote source. A small helper that takes aVersionedAttestationwould let both share the cert construction. This is moot if (1) drops that change.
3d5f600 to
862f4ce
Compare
Signed-off-by: Kevin Wang <wy721@qq.com>
Signed-off-by: Kevin Wang <wy721@qq.com>
Signed-off-by: Kevin Wang <wy721@qq.com>
862f4ce to
f23abfa
Compare
Client-side follow-up to #1106 (shipped in 0.6.0): guests stop fetching the KMS temp CA to mint their RA-TLS client certificates. Also closes a binding gap on the legacy (tokenless) KMS onboarding path.
Problem
Guests.
request_app_keys_from_kms_url,cert-client, anddstack-util get-keyscallGetTempCaCertand mint their client certificate from the returned CA key, only so rustls' pinned-CA check would pass. #1106 removed that pin: the KMS authenticates the attestation inside the certificate and ignores the issuer. The fetch is now ceremony with a cost. A CA private key that authenticates nobody gets persisted into.appkeys.jsonand reused for the CVM's lifetime. It looks like a secret and is not one, which is why it keeps getting reported (#561, #619).Onboarding without
source_token. This is the path kept for sources older than 0.6.0 (publicKMS.GetKmsKey,public_key_handover). It verified the source's attestation on theGetTempCaCertconnection, then fetched the root keys overRaClient::new_mtls, which has nocert_validator. So the connection that actually carried the keys was never attested, and nothing bound it to the one that was.Fix
ra-tls: addgenerate_self_signed_ra_cert. The quote'sreport_databinds the certificate's own SPKI, which is the whole identity.GetMeta, over the same unauthenticated connectionGetTempCaCertused, so the trust posture is unchanged. Relying parties bind the KMS through thekey-providerlaunch event (RTMR3/PCR14), checked against on-chainkmsInfoor a pinned key.verify_key_provider_idalso enforces that binding in-guest, but only when the app pinskey_provider_id. AGetAppKeyfailure now says the KMS must be 0.6.0 or later. The same applies toget-keys.dstack_types::KeyProvider::Kmsdropstmp_ca_key/tmp_ca_cert. Files written by older images still parse. Out-of-tree readers that require those fields will fail on new files (noted in the CHANGELOG).nextis untouched. The tokenless path still mints from the source's temp CA, for compatibility. It fetches that CA on a client without a validator, and then fetches the keys on one client that carries the capturingcert_validator.ensure_kms_allowedruns on the attestation captured from that key-carrying connection before the keys are used. Both paths now share the same client construction.RaClient::new_mtlshad no other users and is removed.GetTempCaCertstays: guest images built before this change and tokenless onboarding still use it.Version requirement
Verification
cargo fmt --all --checkis clean.cargo clippy -p ra-tls -p ra-rpc -p cert-client -p dstack-util -p dstack-kms -p dstack-types --all-features -- -D warnings --allow unused_variablesis clean. With--tests, the only failures are ones already present onnext.cargo test --all-featurespasses for the same crates. This includes thera-rpcratls_client_authsuite, which covers the KMS accepting self-issued and temp-CA-minted certificates and rejecting certificates without an attestation.curl: a self-issued certificate with an attestation passes the handshake and reaches quote verification; one without an attestation is rejected at the handshake;GetMetareturnsca_cert.GetMeta→GetAppKey, acert-clientSignCertround trip, and a tokenless onboard from a 0.5.x source.