fix: mr review

Author: spydaNVIDIA

Cargo.lock

@@ -149,19 +149,23 @@ fn lockdown_ikm_key(version: u32) -> CredentialKey {
 // fetch_kdf_secret fetches the IKM for the KDF from the dedicated site-wide
 // lockdown credential, decoupled from the BMC root so the two can be rotated
 // independently.
+//
+// Returns the IKM version it resolved alongside the secret so the caller can
+// durably record the exact version a card is locked under, rather than
+// re-reading the (mutable) site-wide target later. Today the version is
+// `CURRENT_LOCKDOWN_IKM_VERSION`; the rotation engine will own advancing it.
 async fn fetch_kdf_secret(
     credential_reader: &dyn CredentialReader,
-) -> Result<String, eyre::Report> {
-    let ikm_key = lockdown_ikm_key(CURRENT_LOCKDOWN_IKM_VERSION);
+) -> Result<(u32, String), eyre::Report> {
+    let version = CURRENT_LOCKDOWN_IKM_VERSION;
+    let ikm_key = lockdown_ikm_key(version);
     let credentials = credential_reader
         .get_credentials(&ikm_key)
         .await?
-        .ok_or_else(|| {
-            eyre::eyre!("lockdown IKM v{CURRENT_LOCKDOWN_IKM_VERSION} not found; site not seeded")
-        })?;
+        .ok_or_else(|| eyre::eyre!("lockdown IKM v{version} not found; site not seeded"))?;
     let Credentials::UsernamePassword { password, .. } = credentials;
 
-    Ok(password)
+    Ok((version, password))
 }
 
 // ensure_lockdown_ikm_seeded idempotently seeds the dedicated site-wide
@@ -227,16 +231,32 @@ pub async fn ensure_lockdown_ikm_seeded(
     }
 }
 
+// SupernicLockdownKey is a derived lockdown key together with the site-wide
+// lockdown IKM version it was derived from. The version travels with the key so
+// the lock flow can durably record the exact version the card is locked under.
+pub struct SupernicLockdownKey {
+    // The 16-character hex lockdown key sent to the device.
+    pub key: String,
+    // The site-wide lockdown IKM version `key` was derived from.
+    pub ikm_version: u32,
+}
+
 // build_supernic_lockdown_key builds a single lockdown key using
 // the latest KdfContextVersion. Use this for locking a card.
+//
+// Returns the derived key together with the IKM version it used (see
+// `SupernicLockdownKey`). The unlock flow can ignore the version; the lock flow
+// persists it so the recorded convergence version matches what actually locked
+// the card.
 pub async fn build_supernic_lockdown_key(
     db_reader: &PgPool,
     dpa_interface_id: DpaInterfaceId,
     credential_reader: &dyn CredentialReader,
-) -> Result<String, eyre::Report> {
+) -> Result<SupernicLockdownKey, eyre::Report> {
     let ctx = build_kdf_context(db_reader, dpa_interface_id).await?;
-    let secret = fetch_kdf_secret(credential_reader).await?;
-    build_lockdown_key(secret.as_bytes(), &ctx, KdfContextVersion::V1)
+    let (ikm_version, secret) = fetch_kdf_secret(credential_reader).await?;
+    let key = build_lockdown_key(secret.as_bytes(), &ctx, KdfContextVersion::V1)?;
+    Ok(SupernicLockdownKey { key, ikm_version })
 }
 
 #[cfg(test)]
@@ -441,7 +461,8 @@ mod tests {
             .await
             .unwrap();
 
-        let secret = fetch_kdf_secret(&store).await.unwrap();
+        let (version, secret) = fetch_kdf_secret(&store).await.unwrap();
+        assert_eq!(version, CURRENT_LOCKDOWN_IKM_VERSION);
         assert_eq!(secret, "ikm-pass");
     }
 

crates/api-core/src/dpa/lockdown.rs

@@ -293,6 +293,13 @@ impl From<DatabaseError> for CarbideError {
             DatabaseError::InvalidArgument(e) => InvalidArgument(e),
             DatabaseError::InvalidConfiguration(e) => InvalidConfiguration(e),
             DatabaseError::MissingArgument(e) => MissingArgument(e),
+            // A corrupted/absent site-wide rotation invariant is an internal
+            // state error, not a client-correctable one.
+            DatabaseError::MissingSitewideRotationTarget(credential_type) => Internal {
+                message: format!(
+                    "no site-wide rotation target for credential type: {credential_type:?}"
+                ),
+            },
             DatabaseError::NetworkParseError(e) => NetworkParseError(e),
             DatabaseError::NetworkSegmentDelete(e) => NetworkSegmentDelete(e),
             DatabaseError::NetworkSegmentDuplicateMacAddress(e) => {

crates/api-core/src/errors.rs

@@ -257,7 +257,7 @@ async fn build_unlock_command(
     machine_id: MachineId,
     pci_name: &str,
 ) -> CarbideResult<DpaCommand<'static>> {
-    let key = crate::dpa::lockdown::build_supernic_lockdown_key(
+    let lockdown = crate::dpa::lockdown::build_supernic_lockdown_key(
         &api.database_connection,
         sn.id,
         &*api.credential_manager,
@@ -271,8 +271,10 @@ async fn build_unlock_command(
 
     tracing::info!(%machine_id, %pci_name, "Unlocking DPA");
 
+    // The unlock flow does not record convergence, so the derived IKM version is
+    // not persisted here.
     Ok(DpaCommand {
-        op: OpCode::Unlock { key },
+        op: OpCode::Unlock { key: lockdown.key },
     })
 }
 
@@ -414,7 +416,7 @@ async fn build_lock_command(
     machine_id: MachineId,
     pci_name: &str,
 ) -> CarbideResult<DpaCommand<'static>> {
-    let key = crate::dpa::lockdown::build_supernic_lockdown_key(
+    let lockdown = crate::dpa::lockdown::build_supernic_lockdown_key(
         &api.database_connection,
         sn.id,
         &*api.credential_manager,
@@ -426,9 +428,37 @@ async fn build_lock_command(
         ))
     })?;
 
-    tracing::info!(%machine_id, %pci_name, "Locking DPA");
+    // Stage the IKM version we are about to lock the card with as the in-flight
+    // rotation marker (`rotating_to_version`) on the card's lockdown_ikm row
+    // *before* issuing the lock command. dpa-manager's `handle_locking` promotes
+    // exactly this value to the convergence version when the card reports Locked
+    // -- never the (possibly advanced) site-wide target re-read at observation
+    // time. Staging first means we only ever issue a lock for a version we have
+    // already recorded our intent to use; if the write fails we surface the error
+    // and do not lock. The writer is idempotent across the per-cycle
+    // re-derivation while Locking.
+    let ikm_version = i32::try_from(lockdown.ikm_version).map_err(|e| CarbideError::Internal {
+        message: format!(
+            "lockdown IKM version {} does not fit in i32 for DPA {pci_name}: {e}",
+            lockdown.ikm_version
+        ),
+    })?;
+    let mut conn = api.database_connection.acquire().await.map_err(|e| {
+        CarbideError::GenericErrorFromReport(eyre!(
+            "failed to acquire connection to stage lockdown IKM rotation for DPA {pci_name}: {e}"
+        ))
+    })?;
+    db::credential_rotation::mark_device_rotating_to_version(
+        &mut conn,
+        sn.mac_address,
+        db::credential_rotation::CredentialRotationType::LockdownIkm,
+        ikm_version,
+    )
+    .await?;
+
+    tracing::info!(%machine_id, %pci_name, ikm_version = lockdown.ikm_version, "Locking DPA");
     Ok(DpaCommand {
-        op: OpCode::Lock { key },
+        op: OpCode::Lock { key: lockdown.key },
     })
 }
 

crates/api-core/src/handlers/dpa.rs

@@ -1316,7 +1316,7 @@ async fn get_device_lockdown_key(
                 }
             })?;
 
-    let lockdown_key = crate::dpa::lockdown::build_supernic_lockdown_key(
+    let lockdown = crate::dpa::lockdown::build_supernic_lockdown_key(
         &api.database_connection,
         dpa_interface.id,
         &*api.credential_manager,
@@ -1328,5 +1328,5 @@ async fn get_device_lockdown_key(
         ),
     })?;
 
-    Ok(lockdown_key)
+    Ok(lockdown.key)
 }

crates/api-core/src/handlers/mlx_admin.rs

@@ -16,7 +16,11 @@ CREATE TABLE sitewide_credential_rotation (
     target_version  integer     NOT NULL,
     started_at      timestamptz NOT NULL DEFAULT now(),
     -- Free-form initiator/reason metadata; must never contain secrets.
-    request_meta    jsonb       NOT NULL DEFAULT '{}'::jsonb
+    request_meta    jsonb       NOT NULL DEFAULT '{}'::jsonb,
+    -- Versions are 0-based (0 = pre-rotation baseline), so the target is
+    -- non-negative by construction.
+    CONSTRAINT sitewide_credential_rotation_target_version_non_negative
+        CHECK (target_version >= 0)
 );
 
 -- One row per (device_mac, credential_type): per-device convergence state.
@@ -38,7 +42,20 @@ CREATE TABLE device_credential_rotation (
     -- Redacted last-error string for observability; never contains secrets.
     rotate_last_error_redacted text,
     rotate_quarantined_until   timestamptz,
-    PRIMARY KEY (device_mac, credential_type)
+    PRIMARY KEY (device_mac, credential_type),
+    -- Version/counter fields are non-negative by construction: versions are
+    -- 0-based and rotate_attempts counts up from 0. These CHECKs are the only
+    -- guard at write time -- manual repairs and the backfill data migration
+    -- bypass the Rust writers -- so they keep the rotation engine from
+    -- reasoning over impossible state. A NULL current_version ("not yet
+    -- established") or rotating_to_version ("no rotation in flight") satisfies
+    -- the CHECK and stays legal.
+    CONSTRAINT device_credential_rotation_current_version_non_negative
+        CHECK (current_version >= 0),
+    CONSTRAINT device_credential_rotation_rotating_to_version_non_negative
+        CHECK (rotating_to_version >= 0),
+    CONSTRAINT device_credential_rotation_rotate_attempts_non_negative
+        CHECK (rotate_attempts >= 0)
 );
 
 -- Hot path: "which devices for this credential type still need rotation"