-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PIV: Add AES keys for MGM management keys #578
base: main
Are you sure you want to change the base?
Conversation
BlockCipher + BlockDecrypt + BlockEncrypt + KeyInit + private::Seal | ||
{ | ||
/// The KeySized used for this algorithm | ||
const KEY_SIZE: u8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tony-iqlusion Is it possible to get this out of cipher
's generics? I didn't have a huge amount of success there, as it seems to be that yubikey.rs needs this in a const context, but the generics are not const. I understand why key size for rust crypto traits might not be possible to make const, but I want to know if I didn't miss anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KeyInit
has a supertrait bound on KeySizeUser
: https://docs.rs/crypto-common/latest/crypto_common/trait.KeySizeUser.html
That has an associated typenum::Unsigned
you can get the USIZE
constant for, something like:
<Cipher as KeySizeUser>::KeySize::USIZE
It might be possible to get away with all that in a const context if you're only accessing the associated constant, but it might need an MSRV bump. I believe they expanded generic support for const fn
recently. Or I could be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which would you prefer a version bump or the hard-codes
(I am very agnostic here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are forced to do an MSRV bump due to upstream dependencies moving to hybrid-array
; this was done in #582. So we will end up doing a version bump in the next release.
return Err(Error::AuthenticationError); | ||
} | ||
|
||
// send a response to the cards challenge and a challenge of our own. | ||
let response = mgm_key.decrypt(challenge.data()[4..12].try_into()?); | ||
let card_challenge = mgm_key.card_challenge(&card_response.data()[4..])?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bit tricky and the documentation of the APDU's is not clear.
The docs would suggest that what is to be sent through AES would be 8 bytes; however, that wouldn't match the block size and I don't think would work.
Peeking at the ykman
C code, it appears that in there they use the block length of the cipher being used 8 bytes for 3des due to being 56bit and 16 bytes due to AES's block being 128 bits.
As such, I modified this to use sizes from the challenge instead.
If this is unclear, I can add some documentation to this effect, or we can talk to Yubico about a minor issue in their documentation. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a pointer to the ykman C code here (a github permalink, perhaps) would be nice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you're looking for is this bit:
https://github.com/Yubico/yubikey-manager/blob/b6d1bfdb6aa580beb26de61f6393be879a2e5f23/yubikit/piv.py#L160-L163
So 8 bytes for three DES, and 16 bytes for AES.
I've otherwise found yubico very responsive on PRs on their documentation,
58d3e8c
to
254f06f
Compare
This commit adds support for setting and getting the AES management keys, these are available in firmwars 5.4 and later, and are now the default in firmwares 5.7. The key is handled via being generic on a limit number of allowed alogrithms, using implementations of those from rust-crypto crates. Right now support in PIV MGM keys is for: * TripleDes (`0x03`) - The key type originally used * AES128 (`0x08`) - The new key type using a 128 bit key * AES192 (`0x0A`) - The new key type using a 192 bit key, this also doubles as the algorithm for firmwares 5.7 and later, where the default key is the same as the original TripleDes key. * AES256 (`0x0C`) - The new key type using a 256 bit key Suitable type aliases are provided for each of these key types. The rationale here for exposing the key as a generic type parameter is to largely use the original logic, but avoid scattered enums and provide the end user with some degree of control over the key types at compile time (it should, for instance be relatively easy make 3Des keys uncompileable). See: https://docs.yubico.com/yesdk/users-manual/application-piv/apdu/auth-mgmt.html
`Yubikey` hosts methods to do authentication with the MGM key in a one shot method, and via broken out methods (`get_auth_challenge` and `verify_auth_response`). These methods are a little hard to make work with AES or 3DES keys and currently have no integration tests. Rather than having duplicate logic (and subsequently duplicating error tests), these methods are being removed.
254f06f
to
434d224
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool! I made a couple nit-level comments below.
One worry that occurs to me: doing everything at the type level makes this crate's code very clean, which is nice. But it also makes the crate much harder to use, because it seems like it'll basically require every user of this crate to do type-based dispatch on the management key type.
I'm going to experiment with integrating this into an existing application that wants to be able to work with new YubiKeys, and I'll come back and report my experience. But I wonder if there's a way to internalize some of the management-key-type logic (e.g., handling dispatch in YubiKey
) to avoid requiring every consumer of this crate to reinvent the wheel.
(But again I haven't played with it, so this is only a vague worry for now. I'll have more concrete feedback soon.)
/// Management Key (MGM). | ||
/// | ||
/// This key is used to authenticate to the management applet running on | ||
/// a YubiKey in order to perform administrative functions. | ||
/// | ||
/// The only supported algorithm for MGM keys is 3DES. | ||
/// The only supported algorithm for MGM keys are 3DES and AES. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The only supported algorithm for MGM keys are 3DES and AES. | |
/// The supported algorithms for MGM keys are 3DES and AES. |
} | ||
|
||
/// Create an MGM key from the given byte array. | ||
/// Create an MGM key from the given key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Create an MGM key from the given key. | |
/// Create an [`MgmKey`] from the given [`Key`]. |
return Err(Error::AuthenticationError); | ||
} | ||
|
||
// send a response to the cards challenge and a challenge of our own. | ||
let response = mgm_key.decrypt(challenge.data()[4..12].try_into()?); | ||
let card_challenge = mgm_key.card_challenge(&card_response.data()[4..])?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a pointer to the ykman C code here (a github permalink, perhaps) would be nice here.
@@ -633,53 +625,6 @@ impl YubiKey { | |||
txn.save_object(object_id, indata) | |||
} | |||
|
|||
/// Get an auth challenge. | |||
#[cfg(feature = "untested")] | |||
pub fn get_auth_challenge(&mut self) -> Result<[u8; 8]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this PR is anyway a breaking API change, so maybe it doesn't matter, but: is there a reason not to preserve the API w.r.t. get_auth_challenge
and verify_auth_response
? I suppose they'd have to take a generic argument...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, very sorry! I missed the commit message regarding this removal.
btw: PR up: GregBowyer#1 |
PIV: formatting and lint improvements
I went back and forth on this, I could make the rationale from the commit message clearer:
For $DAYJOB use cases we found it to be good to force the key to be a specific type in our calling code but being generic on it might be more irritating. I think it might be worth doing a followup to expose a wrapping enum for people who want to avoid the types directly. |
Yep, this makes sense to me 👍 By the way, I'm seeing some issues with 3des on pre-5.7.1 keys (specifically, |
Eep nothing I have seen, but I will see if I have an old firmware key around for a test; I may have easily got something wrong there |
This could also be accomplished by e.g. a My personal preference would probably be to use an internal enum and abstracting over the difference rather than exposing it to the end user. |
Found it! 3des should use EDIT: GregBowyer#2 |
Oh sorry that one is on me I messed that up and fat fingered it |
EnumOn an enum would this basically work? pub enum MgmKey {
AES(MgmKey<aes::xxx>),
DES(Mgmkey<tdes::xxx),
} (If so, I could try to make that a thing) Feature flagOn being a feature flag, I doubt that will work. You would curse anyone trying to program a firmware 5.4 key to have to turn on the feature flag to then … turn it off when they put an AES key in place? I think for transition of older firmware we probably want to allow the key to be able to understand that it is 3Des configured. |
Maybe there could be an on-by-default flag for 3des to allow someone to disable it at compile time if they really want to. Thinking about this more, I think we probably want both a "simple" mode, where the crate makes a good decision for you, and a manual mode that gives fine-grained control. For simple mode, I'd suggest two things:
I think all of this could be done just by adding a new type that implements something close to the |
The other place we could check is the mgmt API and see what the default key is. I am inclined to check the version, but it is an option (Similar to how the unit test currently works). I will find some time before Friday to make a simple enum based object (as above) that implements the trait but hides the key specifics for users who generally dont care, with a go at your above suggestions. |
Ah, yes! This makes sense. Speaking as captain obvious :) one nice thing about doing it with version number is that the version number is available without querying the YubiKey once you've connected.
I was thinking about this last night as I updated my application to support multiple key types (which, by the way, ended up being slightly manual---not terrible in my case, but I could see how it would get out of hand depending on the application). One other obvious option is to just give up on unifying the simple and advanced APIs: if you know what key type you want, you use the advanced API, if you just want something automatic you use the simple API. I'm not sure if this helps anyone---I don't think the simple API is so bad that you need to strip it down for the advanced case. The one big difference is that the simple API requires a connection to the YubiKey (to figure out what key is already in place, to check version number, etc.), whereas the advanced API doesn't. I had a half-formed idea for a dyn API that's not exactly pretty, and that I was treating as the "plan to throw one away" prototype. There are probably a couple other cases to handle (derived vs protected, for example), but even providing just this API would already have made my use-case pretty easy. (One thing that isn't obvious to me is if there's a way to make the upgrade path for folks coming from 0.8.0 completely trivial. I think no, but since it's version 0.9.x presumably that's not the highest possible priority...) struct DynMgmKey<'a> {
yk: &'mut YubiKey,
key: MgmKeyEnum,
}
#[derive(Clone, Default)]
enum MgmKeyEnum {
#[default]
Unknown,
TripleDes(MgmKey3Des),
Aes128(MgmKeyAes128),
Aes192(MgmKeyAes192),
Aes256(MgmKeyAes256),
}
impl<'a> DynMgmKey<'a> {
pub fn new<'b>(yk: &'b mut YubiKey) -> Self<'b> {
Self { yk, ..Default::default() }
}
pub fn new_with<'b>(yk: &'b mut YubiKey, key: MgmKeyEnum) -> Self<'b> {
Self { yk, key }
}
pub fn into_key(self) -> MgmKeyEnum {
self.key
}
pub fn authenticate_with_protected(&mut self) -> Result<(), Error> {
// try each auth type with protected key, updating `self.key` with the successful
// key type and erroring if all auth types fail.
// Adding a lower-level `get_protected_raw` fn that returns the raw data might be
// nice here to avoid needlessly querying the YubiKey multiple times.
}
pub fn authenticate_default(&mut self) -> Result<(), Error> {
// as above, but use the default key
}
pub fn generate(&mut self) -> Result<(), Error> {
// assume already authenticated?
// check version number and choose MgmKeyAes192 or MgmKey3Des
}
pub fn set_protected(&mut self) -> Result<(), Error> {
// assume already authenticated?
match self.key {
// match and call yk.set_protected(...)
}
}
} |
Thats pretty much what I was thinking (although I was going to see if I could make MgmKeyEnum) also implement the trait as a second attempt after doing the enum |
.try_into() | ||
.expect("value fits in u8"), | ||
); | ||
data.push(0x80); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data.push(0x80); | |
data.push(TAG_AUTH_WITNESS); |
to be added near the head:
/// Tag for sending back the response to the card challenge
pub(crate) const TAG_AUTH_WITNESS: u8 = 0x80;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I will add
data.push(0x80); | ||
data.push(challenge_len as u8); | ||
data.extend_from_slice(&card_challenge); | ||
data.push(0x81); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data.push(0x81); | |
data.push(TAG_AUTH_CHALLENGE); |
to be added near the head:
/// Tag for the host challenge
pub(crate) const TAG_AUTH_CHALLENGE: u8 = 0x81;
This looks great! Thanks a lot for putting that together! |
let key = Key::<C>::from_slice(&DEFAULT_MGM_KEY).clone(); | ||
Self { | ||
key, | ||
_cipher: std::marker::PhantomData, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I wanted to test your PR locally, but noticed that here you have Default implementation for all MgmKeys. However, the DEFAULT_MGM_KEY
is 24 bytes, so the Key::<C>::from_slice
will panic if someone tries to create MgmKeyAes128::default()
or MgmKeyAes256::default()
. Maybe it makes sense to implement Default only for 3DES and AES-192?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth making default work regardless, although this might be an indication the API is not quite right yet as per the previous conversations.
Sorry I have not had the time to explore other API surfaces, this is on my list |
@@ -165,11 +292,11 @@ impl MgmKey { | |||
e | |||
})?; | |||
|
|||
if item.len() != DES_LEN_3DES { | |||
if item.len() != C::KEY_SIZE as usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: 3DES and AES-192 have the same key size, so this API will let the user read a protected 3DES key as an AES-192 key, and vice versa. The Yubikey is aware of the difference, because Ins::SetMgmKey
takes new_key.algorithm_id()
as an argument.
As I noted in #330 (comment), we need to figure out how to distinguish a 3DES management key from an AES-192 management key. I'm going to hunt around in the ykman
source to see how they do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, we can (for firmware 5.3 and up, where it is available) just use the GET METADATA extension.
This follows the conversation in #330 using a trait-based approach.
I bumped into this while building tools to program new yubikeys where they default to AES192 as the default algorithm.
As such, I figured it was time to add the support in.
Thoughts?