From 155c4aa2ba0c39bf33eca1277a76ef8f2ad7153d Mon Sep 17 00:00:00 2001 From: Philipp Serr Date: Thu, 3 Sep 2026 19:05:41 +0000 Subject: [PATCH 1/3] feat(piv): resolve mgmt-key algorithm by probing Cards that don't answer GET METADATA (pre-5.3 YubiKey firmware, non-Yubico PIV applets) left us assuming Triple-DES for the 9B management key. That is wrong for any such card provisioned with an AES management key: GENERAL AUTHENTICATE is then issued under the wrong P1 and the card rejects it. Resolve the algorithm instead of guessing: - GET METADATA still wins outright when the card answers it. - Otherwise probe every algorithm's GENERAL AUTHENTICATE P1 with a bare witness request (step 1 only -- no key-derived material reaches the card) and collect the ones the card starts with SW 9000. Narrow that set to the algorithms whose key matches the length in hand; if more than one remains (only 3DES and AES-192 collide, both 24 bytes) prefer 3DES, the historical default. If the card accepted no probe at all, fall back to the key length alone -- no worse than before. A real transport failure during probing propagates as itself; only a non-9000 status word means "not this P1, try the next". New: PivSession::reported_management_key_algorithm (metadata answer with no fallback) and resolve_management_key_algorithm. The 9 GUI PIV admin flows and the CLI's open_piv_authed now call the resolver; the CLI keeps its tailored wrong-length message for the metadata-present case. PivBadKeyLength's message is broadened to cover "matches no algorithm this card accepts". The length->candidates table and the probe-result selection are pure functions (mgmt_algs_for_key_len, pick_mgmt_alg) with unit tests; the probe loop itself needs a live card, consistent with the rest of this module. --- crates/keyroost-transport/src/lib.rs | 2 +- crates/keyroost-transport/src/piv.rs | 201 ++++++++++++++++++++++++++- crates/keyroost/src/main.rs | 18 +-- crates/keyroostctl/src/main.rs | 34 +++-- 4 files changed, 232 insertions(+), 23 deletions(-) diff --git a/crates/keyroost-transport/src/lib.rs b/crates/keyroost-transport/src/lib.rs index 4da9fd2f..85c8373f 100644 --- a/crates/keyroost-transport/src/lib.rs +++ b/crates/keyroost-transport/src/lib.rs @@ -265,7 +265,7 @@ impl fmt::Display for TransportError { TransportError::PivBadKeyLength => { write!( f, - "PIV management key has the wrong length for its algorithm" + "PIV management key length does not match any algorithm this card accepts" ) } TransportError::PivBadPinLength => { diff --git a/crates/keyroost-transport/src/piv.rs b/crates/keyroost-transport/src/piv.rs index 06679331..4ad441aa 100644 --- a/crates/keyroost-transport/src/piv.rs +++ b/crates/keyroost-transport/src/piv.rs @@ -297,6 +297,50 @@ fn move_key_supported(version: Option<&[u8]>) -> bool { } } +/// The management-key algorithms whose key is exactly `key_len` bytes, 3DES +/// first. Empty when no algorithm uses that length. +/// +/// [`PivSession::resolve_management_key_algorithm`] uses this as its fallback +/// when GET METADATA is absent *and* the card accepts no GENERAL AUTHENTICATE +/// probe — i.e. when only the key length is left to go on. Only 24 bytes is +/// ambiguous (3DES and AES-192 share it); 3DES leads because it was the sole +/// pre-metadata option. 16 → AES-128 and 32 → AES-256 are unique; 8 +/// (single-DES) and everything else map to nothing. +fn mgmt_algs_for_key_len(key_len: usize) -> &'static [MgmtAlg] { + match key_len { + 16 => &[MgmtAlg::Aes128], + 24 => &[MgmtAlg::TripleDes, MgmtAlg::Aes192], + 32 => &[MgmtAlg::Aes256], + _ => &[], + } +} + +/// Pick the management-key algorithm to authenticate with from the set the +/// card accepted a GENERAL AUTHENTICATE witness request for (`accepted`), given +/// the length of the key in hand. `None` when nothing fits `key_len`. +/// +/// * `accepted` empty → the probe told us nothing; fall back to the +/// length-only table ([`mgmt_algs_for_key_len`]). +/// * otherwise keep only accepted algorithms whose key is `key_len` bytes. +/// * if that still leaves more than one — which can only be 3DES + AES-192, +/// the sole length collision (both 24 bytes) — prefer 3DES, the historical +/// pre-GET-METADATA default. +fn pick_mgmt_alg(accepted: &[MgmtAlg], key_len: usize) -> Option { + let by_len: Vec = if accepted.is_empty() { + mgmt_algs_for_key_len(key_len).to_vec() + } else { + accepted + .iter() + .copied() + .filter(|a| a.key_len() == key_len) + .collect() + }; + if by_len.contains(&MgmtAlg::TripleDes) { + return Some(MgmtAlg::TripleDes); + } + by_len.first().copied() +} + /// A fresh, random 16-byte CHUID GUID — host-side only, no card I/O. For /// pre-filling (and, via a "refresh" action, regenerating) a "New CHUID" /// GUID input before [`PivSession::new_chuid`] writes whatever the user @@ -512,16 +556,113 @@ impl PivSession { piv::parse_metadata(&data).ok() } + /// The card-management (9B) key's algorithm *as the card reports it* via + /// GET METADATA, or `None` when the card doesn't answer the extension + /// (pre-5.3 YubiKey firmware, and non-Yubico applets that stub it out). + /// Unlike [`Self::management_key_algorithm`] this makes no assumption about + /// what an absent answer means — see [`Self::resolve_management_key_algorithm`]. + pub fn reported_management_key_algorithm(&mut self) -> Option { + self.metadata(piv::KEY_REF_MANAGEMENT) + .and_then(|m| m.algorithm) + .and_then(MgmtAlg::from_id) + } + /// The card-management (9B) key's algorithm, from GET METADATA. Defaults to /// [`MgmtAlg::TripleDes`] when the card doesn't report it (pre-5.3 firmware, /// where 3DES was the only option). + /// + /// Prefer [`Self::resolve_management_key_algorithm`] when a key of known + /// length is in hand: it disambiguates cards without GET METADATA instead + /// of blindly assuming 3DES. pub fn management_key_algorithm(&mut self) -> MgmtAlg { - self.metadata(piv::KEY_REF_MANAGEMENT) - .and_then(|m| m.algorithm) - .and_then(MgmtAlg::from_id) + self.reported_management_key_algorithm() .unwrap_or(MgmtAlg::TripleDes) } + /// Decide which algorithm to run management-key authentication under, given + /// the length of the key the caller holds. + /// + /// 1. If GET METADATA reports an algorithm, that wins (its expected key + /// length is checked against `key_len`). + /// 2. Otherwise, probe **every** algorithm — regardless of `key_len` — with + /// GENERAL AUTHENTICATE step 1 (request witness only; no key-derived + /// material reaches the card) and collect the ones the card accepts with + /// `SW 9000`. Then: + /// * narrow that accepted set to the algorithms whose key is `key_len` + /// bytes; + /// * if one remains, use it; + /// * if several remain and 3DES is among them, use 3DES (the historical + /// pre-metadata default — only 3DES and AES-192 can collide on length); + /// * if the card accepted nothing, fall back to `key_len` alone. + /// + /// The chosen algorithm is only *returned*; the caller still runs + /// [`Self::authenticate_management`] with it to actually authenticate. + /// + /// Returns [`TransportError::PivBadKeyLength`] when nothing the card + /// accepts (or, on an empty probe, no algorithm at all) matches `key_len`. + pub fn resolve_management_key_algorithm( + &mut self, + key_len: usize, + ) -> Result { + if let Some(alg) = self.reported_management_key_algorithm() { + if alg.key_len() != key_len { + return Err(TransportError::PivBadKeyLength); + } + return Ok(alg); + } + + // No GET METADATA. Probe each algorithm's GENERAL AUTHENTICATE P1 with a + // bare witness request and see which the card is willing to start. + const ALL: [MgmtAlg; 4] = [ + MgmtAlg::TripleDes, + MgmtAlg::Aes128, + MgmtAlg::Aes192, + MgmtAlg::Aes256, + ]; + trace::line(self.debug, || { + "piv mgmt-key: GET METADATA unsupported; probing every GENERAL \ + AUTHENTICATE P1 with a witness request" + .to_string() + }); + let mut accepted: Vec = Vec::with_capacity(ALL.len()); + for alg in ALL { + // A transport failure (card pulled, reader gone) is fatal for every + // candidate — propagate it rather than mislabel it "bad key + // length". Only a non-9000 status word means "not this P1". + let (_, sw) = self.transmit_full(&piv::general_auth_request_witness( + alg, + piv::KEY_REF_MANAGEMENT, + ))?; + let ok = sw == piv::SW_OK; + trace::line(self.debug, || { + format!( + "piv mgmt-key: probe {} (P1={:#04x}) -> {}", + alg.label(), + alg.id(), + if ok { + "accepted".to_string() + } else { + format!("rejected (SW {sw:04X})") + } + ) + }); + if ok { + accepted.push(alg); + } + } + + if accepted.is_empty() { + trace::line(self.debug, || { + "piv mgmt-key: card accepted no probe; falling back to key length".to_string() + }); + } + let chosen = pick_mgmt_alg(&accepted, key_len).ok_or(TransportError::PivBadKeyLength)?; + trace::line(self.debug, || { + format!("piv mgmt-key: selected {}", chosen.label()) + }); + Ok(chosen) + } + /// Authenticate to the card-management key via the GENERAL AUTHENTICATE /// witness/challenge round. Required before key generation, certificate /// import, set-management-key, and set-pin-retries. `alg` must match the @@ -1764,6 +1905,60 @@ mod tests { assert!(move_key_supported(None)); } + #[test] + fn mgmt_algs_for_key_len_disambiguates_by_length() { + use MgmtAlg::*; + // Unique lengths pin a single algorithm. + assert_eq!(mgmt_algs_for_key_len(16), &[Aes128]); + assert_eq!(mgmt_algs_for_key_len(32), &[Aes256]); + // 24 bytes fits both 3DES and AES-192 — 3DES first (the historical + // pre-GET-METADATA default), then the P1 probe decides. + assert_eq!(mgmt_algs_for_key_len(24), &[TripleDes, Aes192]); + // Every candidate's own key_len() must round-trip back to a list that + // contains it — the table and MgmtAlg::key_len() must not drift apart. + for alg in [TripleDes, Aes128, Aes192, Aes256] { + assert!(mgmt_algs_for_key_len(alg.key_len()).contains(&alg)); + } + // Nothing plausible-looking (single-DES) or absurd resolves. + assert!(mgmt_algs_for_key_len(8).is_empty()); + assert!(mgmt_algs_for_key_len(0).is_empty()); + assert!(mgmt_algs_for_key_len(20).is_empty()); + } + + #[test] + fn pick_mgmt_alg_narrows_probe_results_by_length() { + use MgmtAlg::*; + + // One probe accepted, right length -> that one. + assert_eq!(pick_mgmt_alg(&[Aes128], 16), Some(Aes128)); + // One probe accepted, wrong length for the key in hand -> nothing. + assert_eq!(pick_mgmt_alg(&[Aes256], 24), None); + + // A lax card accepts every P1. The 24-byte key narrows it to 3DES + + // AES-192, and 3DES wins the tie. + assert_eq!( + pick_mgmt_alg(&[TripleDes, Aes128, Aes192, Aes256], 24), + Some(TripleDes) + ); + // Same lax card, 16-byte key -> unambiguously AES-128. + assert_eq!( + pick_mgmt_alg(&[TripleDes, Aes128, Aes192, Aes256], 16), + Some(Aes128) + ); + // 24-byte key, but only AES-192 was accepted -> no 3DES to prefer. + assert_eq!(pick_mgmt_alg(&[Aes192], 24), Some(Aes192)); + // 24-byte key, both 24-byte algs accepted but in the other order -> + // still 3DES (preference, not position). + assert_eq!(pick_mgmt_alg(&[Aes192, TripleDes], 24), Some(TripleDes)); + + // Empty accepted set = probe learned nothing -> fall back to length + // alone (same as the pre-probe behaviour). + assert_eq!(pick_mgmt_alg(&[], 16), Some(Aes128)); + assert_eq!(pick_mgmt_alg(&[], 24), Some(TripleDes)); + assert_eq!(pick_mgmt_alg(&[], 32), Some(Aes256)); + assert_eq!(pick_mgmt_alg(&[], 20), None); + } + // --- pubkey-cache invalidation semantics --------------------------------- // // The session methods that drive these transitions need a live card, so diff --git a/crates/keyroost/src/main.rs b/crates/keyroost/src/main.rs index d89b1524..ba2c3eff 100644 --- a/crates/keyroost/src/main.rs +++ b/crates/keyroost/src/main.rs @@ -6708,7 +6708,7 @@ impl App { self.spawn_job("Setting PIV retry counts\u{2026}", move || { let result = (|| -> Result { let mut s = keyroost_transport::PivSession::open(&name)?; - let alg = s.management_key_algorithm(); + let alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(alg, &mgmt)?; s.verify_pin(pin.as_bytes())?; s.set_pin_retries(pin_tries, puk_tries)?; @@ -6749,7 +6749,7 @@ impl App { TransportError, > { let mut s = keyroost_transport::PivSession::open(&name)?; - let mgmt_alg = s.management_key_algorithm(); + let mgmt_alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(mgmt_alg, &mgmt)?; let pubkey = s.generate_key(slot, alg, pin_policy, touch_policy)?; Ok((pubkey, s.status()?)) @@ -6837,7 +6837,7 @@ impl App { let der = cert_bytes_to_der(&bytes) .ok_or(TransportError::MalformedResponse("file is not PEM or DER"))?; let mut s = keyroost_transport::PivSession::open(&name)?; - let mgmt_alg = s.management_key_algorithm(); + let mgmt_alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(mgmt_alg, &mgmt)?; s.import_certificate(slot, &der)?; s.status() @@ -6870,7 +6870,7 @@ impl App { self.spawn_job("Deleting certificate\u{2026}", move || { let result = (|| -> Result { let mut s = keyroost_transport::PivSession::open(&name)?; - let mgmt_alg = s.management_key_algorithm(); + let mgmt_alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(mgmt_alg, &mgmt)?; s.clear_certificate(slot)?; s.status() @@ -6934,7 +6934,7 @@ impl App { self.spawn_job("Writing a new CHUID\u{2026}", move || { let result = (|| -> Result { let mut s = keyroost_transport::PivSession::open(&name)?; - let mgmt_alg = s.management_key_algorithm(); + let mgmt_alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(mgmt_alg, &mgmt)?; s.new_chuid(&guid, &expiration)?; s.status() @@ -6967,7 +6967,7 @@ impl App { self.spawn_job("Deleting key\u{2026}", move || { let result = (|| -> Result { let mut s = keyroost_transport::PivSession::open(&name)?; - let mgmt_alg = s.management_key_algorithm(); + let mgmt_alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(mgmt_alg, &mgmt)?; s.delete_key(slot)?; s.status() @@ -7006,7 +7006,7 @@ impl App { self.spawn_job("Moving key\u{2026}", move || { let result = (|| -> Result { let mut s = keyroost_transport::PivSession::open(&name)?; - let mgmt_alg = s.management_key_algorithm(); + let mgmt_alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(mgmt_alg, &mgmt)?; s.move_key(src, dest)?; s.status() @@ -7141,7 +7141,7 @@ impl App { move || { let result = (|| -> Result { let mut s = keyroost_transport::PivSession::open(&name)?; - let mgmt_alg = s.management_key_algorithm(); + let mgmt_alg = s.resolve_management_key_algorithm(mgmt.len())?; s.authenticate_management(mgmt_alg, &mgmt)?; if let Some((alg, key)) = known_key { s.remember_pubkey(slot, alg, key); @@ -7315,7 +7315,7 @@ impl App { self.spawn_job("Changing management key\u{2026}", move || { let result = (|| -> Result { let mut s = keyroost_transport::PivSession::open(&name)?; - let cur_alg = s.management_key_algorithm(); + let cur_alg = s.resolve_management_key_algorithm(old.len())?; s.authenticate_management(cur_alg, &old)?; s.set_management_key(new_alg, &new, false)?; s.status() diff --git a/crates/keyroostctl/src/main.rs b/crates/keyroostctl/src/main.rs index 764906ac..158a4aa5 100644 --- a/crates/keyroostctl/src/main.rs +++ b/crates/keyroostctl/src/main.rs @@ -6613,16 +6613,30 @@ fn open_piv_authed( mgmt_key: &[u8], ) -> Result> { let mut session = open_piv(reader, debug)?; - let alg = session.management_key_algorithm(); - if mgmt_key.len() != alg.key_len() { - return Err(format!( - "management key is {} bytes; this card's {} key needs {}", - mgmt_key.len(), - alg.label(), - alg.key_len() - ) - .into()); - } + // Prefer GET METADATA; when the card stubs it out, fall back to the key + // length (and, for a 24-byte key, a GENERAL AUTHENTICATE P1 probe) rather + // than blindly assuming 3DES. + let alg = match session.reported_management_key_algorithm() { + Some(reported) if mgmt_key.len() != reported.key_len() => { + return Err(format!( + "management key is {} bytes; this card's {} key needs {}", + mgmt_key.len(), + reported.label(), + reported.key_len() + ) + .into()); + } + Some(reported) => reported, + None => session + .resolve_management_key_algorithm(mgmt_key.len()) + .map_err(|_| { + format!( + "management key is {} bytes, which does not match any \ + PIV management-key algorithm this card accepts", + mgmt_key.len() + ) + })?, + }; session.authenticate_management(alg, mgmt_key)?; Ok(session) } From 1dd8bbb459d8c4f95eae6f29fe07fe42377e2bb0 Mon Sep 17 00:00:00 2001 From: framefilter Date: Mon, 7 Sep 2026 13:45:48 -0400 Subject: [PATCH 2/3] piv: keep transport failures distinct from the key-length verdict in the CLI probe path The CLI wrapper around the management-key algorithm probe mapped every error from the resolver to the "does not match any algorithm this card accepts" message. The transport layer takes care to propagate a mid-probe transport failure (card pulled, reader gone) as itself; the wrapper undid that. Only PivBadKeyLength now gets the friendly wording. The comment above it also said the probe ran only for 24-byte keys, which is not what the resolver does. Maintainer-side fixup for #124. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01XpyGDFxYw5DSD97HXBAc2q --- crates/keyroostctl/src/main.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/crates/keyroostctl/src/main.rs b/crates/keyroostctl/src/main.rs index 158a4aa5..0c4de495 100644 --- a/crates/keyroostctl/src/main.rs +++ b/crates/keyroostctl/src/main.rs @@ -6613,8 +6613,8 @@ fn open_piv_authed( mgmt_key: &[u8], ) -> Result> { let mut session = open_piv(reader, debug)?; - // Prefer GET METADATA; when the card stubs it out, fall back to the key - // length (and, for a 24-byte key, a GENERAL AUTHENTICATE P1 probe) rather + // Prefer GET METADATA; when the card stubs it out, probe every GENERAL + // AUTHENTICATE P1 with a witness request and narrow by key length, rather // than blindly assuming 3DES. let alg = match session.reported_management_key_algorithm() { Some(reported) if mgmt_key.len() != reported.key_len() => { @@ -6629,12 +6629,19 @@ fn open_piv_authed( Some(reported) => reported, None => session .resolve_management_key_algorithm(mgmt_key.len()) - .map_err(|_| { - format!( - "management key is {} bytes, which does not match any \ - PIV management-key algorithm this card accepts", - mgmt_key.len() - ) + .map_err(|e| -> Box { + match e { + // Only the length verdict gets the friendly wording; a + // transport failure mid-probe (card pulled, reader gone) + // must surface as itself, not as a key-length complaint. + TransportError::PivBadKeyLength => format!( + "management key is {} bytes, which does not match any \ + PIV management-key algorithm this card accepts", + mgmt_key.len() + ) + .into(), + other => other.into(), + } })?, }; session.authenticate_management(alg, mgmt_key)?; From 8003c3438c5868140ac19ecb392c51c5efe5dd23 Mon Sep 17 00:00:00 2001 From: framefilter Date: Mon, 7 Sep 2026 13:45:48 -0400 Subject: [PATCH 3/3] changelog: fragment for the PIV management-key algorithm probe (re #124) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01XpyGDFxYw5DSD97HXBAc2q --- changelog.d/fixed-124-piv-mgmt-key-algorithm-probe.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog.d/fixed-124-piv-mgmt-key-algorithm-probe.md diff --git a/changelog.d/fixed-124-piv-mgmt-key-algorithm-probe.md b/changelog.d/fixed-124-piv-mgmt-key-algorithm-probe.md new file mode 100644 index 00000000..9ff9c4ce --- /dev/null +++ b/changelog.d/fixed-124-piv-mgmt-key-algorithm-probe.md @@ -0,0 +1,11 @@ +- **PIV management-key authentication no longer assumes 3DES on cards that + lack GET METADATA.** That Yubico extension is the only place a card reports + its 9B key's algorithm, and applets without it (the Cryptnox OpenFIPS201 + variant, which ships with an AES key) were authenticated as 3DES and + failed. When the extension is absent, keyroost now probes each algorithm + with a bare GENERAL AUTHENTICATE witness request (no key material leaves + the host), keeps the ones the card accepts, and narrows by the length of + the key in hand; a 24-byte key that fits both 3DES and AES-192 still + resolves to 3DES, so older YubiKeys behave as before. Applies to the CLI + and the GUI alike, and `--debug` traces each probe's verdict. Contributed + by @episource. ([#124])