PIV: Add AES keys for MGM management keys

[GregBowyer]

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?

[kwantam]

btw: cargo clippy --tests --all-features reveals some issues. ~@GregBowyer I'll have a PR against your repo shortly.~

PR up: https://github.com/GregBowyer/yubikey.rs/pull/1

[GregBowyer]

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.)

I went back and forth on this, I could make the rationale from the commit message clearer:

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).

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.

[kwantam]

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, YubiKey::authenticate is failing with the default key after a piv reset). Still tracking it down, probably something I'm doing wrong. Any chance this is something you've seen?

[GregBowyer]

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, YubiKey::authenticate is failing with the default key after a piv reset). Still tracking it down, probably something I'm doing wrong. Any chance this is something you've seen?

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

[tony-iqlusion]

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).

This could also be accomplished by e.g. a tdes feature and gating the relevant support for 3DES management keys on it.

My personal preference would probably be to use an internal enum and abstracting over the difference rather than exposing it to the end user.

[kwantam]

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

Found it! 3des should use des::TdesEde3, but this PR changes it to use des::TdesEee3. Easy fix incoming.

EDIT: https://github.com/GregBowyer/yubikey.rs/pull/2

[GregBowyer]

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

Found it! 3des should use des::TdesEde3, but this PR changes it to use des::TdesEee3. Easy fix incoming.

Oh sorry that one is on me I messed that up and fat fingered it

[GregBowyer]

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).

This could also be accomplished by e.g. a tdes feature and gating the relevant support for 3DES management keys on it.

My personal preference would probably be to use an internal enum and abstracting over the difference rather than exposing it to the end user.

Enum

On 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 flag

On 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.

[kwantam]

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:

• when creating a new key, check firmware version. If less than 5.7.1 use 3DES, otherwise use AES192 (maybe 5.4.2+ supports AES192, in which case that could be the line instead).
• when retrieving a protected key or authenticating with a user-supplied key, automatically figure out which cipher to use. Probably not by version number, but by actually trying the authentication. The reason is, different versions of ykman behave differently. With ykman v5.0.1, ykman piv access change-management-key -p uses 3DES on a v5.7.1 YubiKey, whereas the same command with ykman v5.5.1 uses AES192. So just looking at the version number isn't enough.

I think all of this could be done just by adding a new type that implements something close to the MgmKeyAlgorithm trait from this PR. Not exactly, because neither KEY_SIZE nor ALGORITHM_ID could be consts for the dynamic-cipher type. But probably that's OK...

[GregBowyer]

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:

* when creating a new key, check firmware version. If less than 5.7.1 use 3DES, otherwise use AES192 (maybe 5.4.2+ supports AES192, in which case that could be the line instead).

* when retrieving a protected key or authenticating with a user-supplied key, automatically figure out which cipher to use. Probably not by version number, but by actually trying the authentication. The reason is, different versions of `ykman` behave differently. With ykman v5.0.1, `ykman piv access change-management-key -p` uses 3DES on a v5.7.1 YubiKey, whereas the same command with ykman v5.5.1 uses AES192. So just looking at the version number isn't enough.

I think all of this could be done just by adding a new type that implements something close to the MgmKeyAlgorithm trait from this PR. Not exactly, because neither KEY_SIZE nor ALGORITHM_ID could be consts for the dynamic-cipher type. But probably that's OK...

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.

[kwantam]

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).

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 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.

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(...)
        }
    }
}

[GregBowyer]

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).

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 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.

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

[baloo]

This looks great! Thanks a lot for putting that together!

[GregBowyer]

Sorry I have not had the time to explore other API surfaces, this is on my list