Extend Credential structure with Password Safe field

[szszszsz]

Extend Credential structure with Password Safe field

Add GetCredential command
Allow additional fields in Register command
New tags for the PWS fields
New OTP kind to signalize OTP being unused in the given Credential, without changing the data structure

Remarks:

• This change is backwards-compatible with the previous user data, and do not need data migration.
• Data created with firmware using this application may block working of the older version (e.g. if user moved to test firmware, and want to get back to stable firmware). Reset procedure should be able to recover all features though.

Fixes #60

[robin-nitrokey]

Looks good.

What do you think about moving the credential data into an enum because they should be mutually exclusive? Something like this:

pub struct Credential<'l> {
    pub label: &'l [u8],
    pub touch_required: bool,
    pub encryption_key_type: EncryptionKeyType,
    pub counter: Option<u32>,
    pub data: CredentialData<'l>,
}

pub enum CredentialData<'l> {
    OneTimePassword {
        kind: oath::Kind,
        algorithm: oath::Algorithm,
        digits: u8,
        secret: &'l [u8],
    }.
    StaticPassword {
        login: &'l [u8],
        password: &'l [u8],
        metadata: &'l [u8],
    },
}

Data created with firmware using this application may block working of the older version

We should talk about if we want to try to avoid this. For example, we could add a version number and just ignore data that we don’t understand. Not sure if it is worth the effort.

[szszszsz]

Thank you for the review!

1. I was considering dividing this into different types as well, but it does not make much sense. Keeping PWS and OTP slots separate will be cumbersome from the user POV, while having them both seems quite handy - see KeepassXC design. What is missing here is an update/modify command, which would allow to add and remove PWS and OTP data to the Credential structure. See, that with this PR it will be allowed to create an "empty" Credential as well.
2. Another thing is extending the data structure vs reforming it. Since the Secrets App is nearly feature-complete, it does little sense to me to provoke data migrations at this point, as this code will be never touched again, except for any potential logic errors. Extending the structure looks like a natural choice from this perspective (it was the whole point in using CBOR in the first place anyway).

[szszszsz]

We should talk about if we want to try to avoid this. For example, we could add a version number and just ignore data that we don’t understand. Not sure if it is worth the effort.

I do not see a point in tracking this (edit: as long as we do not have hard migrations, but just extensions) since downgrades in stables are forbidden. This particular note is rather for the users of the test firmware, which could find some of their newly created credentials not working, if "downgraded" to the last stable.
In this case incompatibility comes from the newly introduced OTP slot type, marking being unused, which would probably make deserialization errors on use.

[robin-nitrokey]

Just to be clear, I’m only talking about the data structures in the code, not the serialization format. It could still make sense to move the fields into structs that are flattened during the serialization, e. g.:

pub struct Credential<'l> {
    pub label: &'l [u8],
    pub touch_required: bool,
    pub encryption_key_type: EncryptionKeyType,
    pub counter: Option<u32>,
    pub otp: Option<OtpCredentialData<'l>>,
    pub static: Option<StaticCredentialData<'l>>,
}

pub struct OtpCredentialData<'l> {
    pub kind: oath::Kind,
    pub algorithm: oath::Algorithm,
    pub digits: u8,
    pub secret: &'l [u8],
}

pub struct StaticCredentialData<'l> {
    pub login: &'l [u8],
    pub password: &'l [u8],
    pub metadata: &'l [u8],
}

My concern is that code with implicit invariants, e. g. if kind: Option<Kind> is Some(_), then algorithm: Option<Algorithm> must also be Some(_), is often hard to read and maintain compared to more explicit code like combining these two variables into a tuple or struct with a single Option. In this case it probably does not make a big difference though.

This particular note is rather for the users of the test firmware, which could find some of their newly created credentials not working, if "downgraded" to the last stable.

That’s totally fine. My understanding was that adding a credential with the test firmware might block the user from accessing other credentials using the stable firmware. While this would also be okay, I’d rather try to avoid such situations if possible.

[szszszsz]

Ah, I missed lifetimes and confused it with the serialized data type indeed. In that case I agree, it would be nice to keep the clean types planes in the commands and function handling them.

Since this looks like a bigger refactorization, can we do that in a separate PR?
Edit: registered as #66

[robin-nitrokey]

Sure, can also be done in a separate PR.

[robin-nitrokey]

I think we also need a way to query the credential metadata (without the secrets) that does not require a button press. For the nitrokey-app2, we want to first determine the type of the credential (static password? OTP, if yes, which algorithm?), and then provide the appropriate actions to the user. Currently this would mean a GetCredential call that has to be confirmed by button press or PIN entry. Maybe there could be a separate GetCredentialMetadata command that does not require confirmation and only returns which kind of secrets are available for a credential?

[szszszsz]

I think we also need a way to query the credential metadata (without the secrets) that does not require a button press. For the nitrokey-app2, we want to first determine the type of the credential (static password? OTP, if yes, which algorithm?), and then provide the appropriate actions to the user.

I believe the list command already returns type of the Credential. See the associated pynitrokey's PR. It does not require button press (outdated Secrets App?).

Note: this feedback should not be a PR rejection, but feature-request in a separate ticket (please write user scenario there, which will be used for the design update).

[robin-nitrokey]

Ah, I missed that because it is only returned as bytes in pynitrokey. But as far as I see, it only contains the OTP type and no information about the static password, right? I can create a separate issue for that if you prefer.

[szszszsz]

But as far as I see, it only contains the OTP type and no information about the static password, right?

It might be possible it is listing only one type right now, even if there are multiple types specified. To check.

I can create a separate issue for that if you prefer.

Please do