Feedback on implementation.md

[jans23]

This feedback refers to implementation.md.

The purpose of those commands is difficult to understand without a process description. Looking forward to future revisions describing those.

Having a "Terminology" chapter and consistent usage of those terms would make the description better understandable. I suggest to extend the Terminology chapter in the README. Also I find it more clear to write those terms appearing in the text in italic and starting with capital letter.

Do LOGIN and LOGOUT match UNLOCK and RESET described in the README? I'm confident about UNLOCK = LOGIN but I'm not sure what our intent of RESET was? Was it really to LOGOUT or did we mean a different kind of reset?

Is the command PIN_ATTEMPTS really required? It's a subset of STATUS nevertheless. I would prefer a single STATUS only, as long as there are no technical constrains such as performance or limited package length which make STATUS inconvenient to be used frequently.

INITIALIZE_SEED, RESTORE_FROM_SEED: Terminology "seed" and "master key" are used inconsistently. Are they the same?

Also these terms are largely unclear to me: "Nitrokey Webcrypt's master key WC_MASTER_KEY along with the authentication tag. Finally it is encrypted through another secret key - WC_MASTER_ENC_KEY." What are WC_MASTER_ENC_KEY, authentication tag?

"The passphrase is processed through a hash function (e.g. Argon2) with known parameters client side" - This hash function needs to be specified because otherwise different implementations may become incompatible. I believe this is implemented in the JS library. Nevertheless we should specify it here.
How could we change the hash algorithm in a later point in time, assuming that different 3rd party implementations exists in the field? We don't have a technical measure to enforce an algorithm update or at least to detect incompatibilities. Perhaps storing the algorithm's identifier in the device would allow to detect incompatibilities. Not sure this is the best solution but I'm brainstorming...

Inconsistent to README which states "In any case also a touch button press is required to authorize every sign and decrypt operation."

INITIALIZE_SEED: I suggest a ENTROPY parameter which is mixed with HWRNG's entropy during generating the master key.

"Reused FIDO U2F key-wrapping implementation below" - FIDO U2F doesn't know WC_MASTER_ENC_KEY for example, which I see in the pseudocode. Thus, which parts of the algorithm are from FIDO U2F exactly?

Key operations which require authentication such as SIGN may throw an error NOT CONFIRMED or NOT AUTHENTICATED or alike. Would such error code make sense? In doubt, see FIDO2 specification's error codes.

SIGN: "Returns SIGNATURE as a result, as well as the sent hash named INHASH" - I don't understand what INHASH is and I don't see it in the pseudocode either.

DECRYPT: "ECCEKEY ephemeral ECC public key for deriving the shared secret" - I don't understand this. Please elaborate.

STATUS: "number of available Resident Keys slots" - What does available mean? Available such as "existing hardware resources of the device" or as "not occupied yet"?

"On FIDO2 factory reset the Nitrokey Webcrypt's secrets should be reinitialized to random values." This brings me to the question: What is the default state when shipping vanilla devices? Will they be empty or randomly initialized? I believe they will be empty so that user's first task would be to call
INITIALIZE_SEED or RESTORE_FROM_SEED. If this is true, the statement "secrets should be reinitialized to random values" is wrong.

[szszszsz]

Do LOGIN and LOGOUT match UNLOCK and RESET described in the README? I'm confident about UNLOCK = LOGIN but I'm not sure what our intent of RESET was? Was it really to LOGOUT or did we mean a different kind of reset?

I believe UNLOCK indeed corresponds to the LOGIN command. About RESET, this was to introduce FACTORY_RESET for the U2F backwards compatibility. I will make proper corrections to make this less confusing, as well as change the names in the original document.

Is the command PIN_ATTEMPTS really required? It's a subset of STATUS nevertheless. I would prefer a single STATUS only, as long as there are no technical constrains such as performance or limited package length which make STATUS inconvenient to be used frequently.

During the development I have decided to move PIN counter to the STATUS command, making the other command obsolete. It will be removed in the next iteration.

INITIALIZE_SEED, RESTORE_FROM_SEED: Terminology "seed" and "master key" are used inconsistently. Are they the same?

Seed, or Word Seed, is the source of the Webcrypt's secrets (gained through some translation operation, like hash sum), e.g. Master Key, which is in turn used for all key operations. The former could be a human readable representation for the backup purposes. Will elaborate on that in the document.

Also these terms are largely unclear to me: "Nitrokey Webcrypt's master key WC_MASTER_KEY along with the authentication tag. Finally it is encrypted through another secret key - WC_MASTER_ENC_KEY." What are WC_MASTER_ENC_KEY, authentication tag?

These are elements left from the FIDO U2F / FIDO2 key wrapping solution we have implemented in the Nitrokey FIDO2 and upstream. I left them as it is with the intention of showing our starting point from a known solution, and iteratively improve it.
Authentication tag is added to each generated Key Handle for validation - to make sure given key was generated on given device, and for given Origin. This is usable in case, where the Key Handle is stored externally (but not if the keys are passphrase based - that is not generated randomly).
Regarding WC_MASTER_ENC_KEY constant, it was used in the original solution to introduce another random data to the final private key through AES256. Whether this is needed to derive a strong key needs a discussion.

"The passphrase is processed through a hash function (e.g. Argon2) with known parameters client side" - This hash function needs to be specified because otherwise different implementations may become incompatible. I believe this is implemented in the JS library. Nevertheless we should specify it here.
How could we change the hash algorithm in a later point in time, assuming that different 3rd party implementations exists in the field? We don't have a technical measure to enforce an algorithm update or at least to detect incompatibilities. Perhaps storing the algorithm's identifier in the device would allow to detect incompatibilities. Not sure this is the best solution but I'm brainstorming...

I agree - both the hash algorithm and its parameters have to be standardized.
We can encode selected hash algorithm (or security level, meaning here a set of parameters for given calculation complexity) as a flag in the Word Seed during the initialization (so during the restore from backup it would be set correctly).

Inconsistent to README which states "In any case also a touch button press is required to authorize every sign and decrypt operation."

Confirmed. I corrected the overview table for the commands.

INITIALIZE_SEED: I suggest a ENTROPY parameter which is mixed with HWRNG's entropy during generating the master key.

Good idea. Will be added as a required argument, and xored with the HWRNG output.

"Reused FIDO U2F key-wrapping implementation below" - FIDO U2F doesn't know WC_MASTER_ENC_KEY for example, which I see in the pseudocode. Thus, which parts of the algorithm are from FIDO U2F exactly?

I have reused the key derivation implementation, but changed the constants to a separate set for Webcrypt, meaning each constant has now similar one in the both solutions.

Key operations which require authentication such as SIGN may throw an error NOT CONFIRMED or NOT AUTHENTICATED or alike. Would such error code make sense? In doubt, see FIDO2 specification's error codes.

Good catch. To be added.

SIGN: "Returns SIGNATURE as a result, as well as the sent hash named INHASH" - I don't understand what INHASH is and I don't see it in the pseudocode either.

The INHASH parameter is returned to caller to show what was signed, in similar manner as it is done in the FIDO U2F / FIDO2. The name is indeed confusing and will be changed to HASH, the same as input parameter.

DECRYPT: "ECCEKEY ephemeral ECC public key for deriving the shared secret" - I don't understand this. Please elaborate.

Description for that command misses, that it uses ECDH to establish a shared secret. What it means is, that to encrypt the data caller has to create a one-time ECC key-pair, use private key from it and our public key to establish a shared secret, encrypt the data with it and send us the public key from that one-time key pair along with ciphertext for decryption.
To be extended in the document.

STATUS: "number of available Resident Keys slots" - What does available mean? Available such as "existing hardware resources of the device" or as "not occupied yet"?

This should be a number of available Resident Key slots to be populated.
To discuss, whether it would not be misused for the fingerprinting.

"On FIDO2 factory reset the Nitrokey Webcrypt's secrets should be reinitialized to random values." This brings me to the question: What is the default state when shipping vanilla devices? Will they be empty or randomly initialized? I believe they will be empty so that user's first task would be to call
INITIALIZE_SEED or RESTORE_FROM_SEED. If this is true, the statement "secrets should be reinitialized to random values" is wrong.

The Webcrypt's fields are always initialized to make sure there would be never a case, where user has run operations with empty secrets. With that in mind, these should probably not be used as user does not have a Word Seed backup for it, and will lose all the keys with the device malfunction.
Will add proper condition to not allow the Webcrypt's usage in case the device was not initialized, e.g. when vanilla or after a FIDO2 factory reset.

• Update/add Terminology chapter
• Add process / use case description