Check key usage

[MarcusJGStreets]

Written up Giles' suggestion.
I seem to have more wording on errors that for other functions.

[athoelke]

We need a description of the function that helps users and implementers understand it better - perhaps a full table to map algorithm categories and usage flags to API functions?

The usage flag needs aligning with the Issue discussion.

The failure conditions look right, but should be formatted as the standard set of .. retval:: directives, which will remove any need to specify ordering.

[athoelke]

This API should be PSA_KEY_USAGE_DERIVE_PUBLIC. The operations it is related to currently use PSA_KEY_USAGE_DERIVE as a policy check: that is both key agreement and PAKE.

The buffer comment is wrong in two ways (and maybe uneccessary here):

• In the key-agreement functions, the public key is always supplied as a buffer to the API, and no actual key object has a policy to check.
• But. in an asymmetric PAKE operation that uses asymmetric keys derived from the password, such as SPAKE2+, both roles in the PAKE operation use a key object for set up, and check for PSA_KEY_USAGE_DERIVE.

However, for psa_check_key_usage(), it is necessary to indicate which part of the agreement (private/public) or PAKE role (prover/verifier) the key is being checked for.

See the latest comment on the Issue (#279 (comment)), which summarises the proposed change.

[athoelke]

I am wondering if we should actually include a full table that maps algorithm categories and usage flags to the 'identified function'?

[gilles-peskine-arm]

I agree with Andrew's points, especially #282 (comment) and #282 (comment). I've raised a couple of additional minor points.

[gilles-peskine-arm]

The private key and the public key don't exist as separate objects. A key pair can both sign and verify, unlike a public key which can only verify. There's no need to bring in a “private key” as a separate part of the key pair.

[athoelke]

The shape looks right. Lots of fine-tuning comments...

[athoelke]

I've rebased to tidy up merge history, and applied some copy-editing to align this PR with the specification formatting/style.

There are a couple of open issues:

1. This PR allocates usage flag 0x00010000 for PSA_KEY_USAGE_DERIVE_PUBLIC, and Add support for key wrapping (v2) #224 does so for wrapping keys. The wrapping API exists in at least one implementation with this value.

   Although this value is neighbouring a derivation-related key usage flag (so would be tidy to keep there), the PSA_KEY_USAGE_DERIVE_PUBLIC flag is not present in a key attributes' usage flags, and it might be reasonable to allocate this flag to a different bit (e.g. bit 7).

2. Should psa_check_key_usage() be documented as part of the Key policies chapter (where it is now directly after psa_get_key_usage_flags()), or perhaps in Key management, or perhaps somewhere else?

[gilles-peskine-arm]

Mostly wording concerns.

[gilles-peskine-arm]

It would be good to clarify what the implementation is supposed to check, in cases where the key type alone is not enough to decide whether an operation is possible. For example, some asymmetric operations have a constraint of the form key_size >= hash_length + padding_length, so presumably the implementation should check that the key size is sufficiently large.

Are there any cases where a key could be valid for some cryptographic operation but not others, and this would not be reflected in the key attributes (type, size, policy)?

[athoelke]

I'll use the same wording as we do in the PSA_ERROR_INVALID_ARGUMENT return value - "the key is compatible with the algorithm and usage"

[gilles-peskine-arm]

Right now, the corresponding function in TF-PSA-Crypto (not psa_check_key_usage(), but a function that is meant to be compatible when its key argument is a PSA key) accepts a 512-bit RSA key for PSA_ALG_RSA_OAEP(PSA_ALG_SHA_512) (if applicable policies are ok), even though the key cannot ever work for this algorithm (OAEP requires a key that's larger than the hash plus a few bytes of overhead). It works this way because we only check the key type, the algorithm and the policy.

(Note that, other than being too small for comfort, there's nothing wrong with the key itself. If its policy has PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH), the key can be used for OAEP-MD5, OAEP-SHA1, etc.)

I think this is a bug, because I think psa_check_key_usage(key, alg, usage) is intended to return err if an operation based on the given usage would return err for the given key and algorithm. Attempting to encrypt or decrypt with this small RSA key using this specific hash-parametrized algorithm would always fail, so the usage check should also fail.

However, I don't find the specification fully clear here. I would like something (a remark, an example, …) that explicitly states that knowing the type and policy of the key is not always enough to determine the result.

[athoelke]

I will reword this initial paragraph again, and try to better capture that intention: this API is designed to determine whether the implementation is able to carry out the associated operation with the provided key and algorithm; and if not, why not.

An error from ths API should imply an error would be received from the associated operation.

More advanced compatibility checks are recommended, but should this go as far as to require that a structured or asymmetric key is validated to meet specific constraints of the algorithm (i.e. does not have any not-permitted values)?

[gilles-peskine-arm]

should this go as far as to require that a structured or asymmetric key is validated to meet specific constraints of the algorithm (i.e. does not have any not-permitted values)?

That's part of why there's an ambiguity: no, this function must not be required to do math. Otherwise it would need to call into the crypto backend for sure, it would need to dispatch to a driver, etc.

An error from ths API should imply an error would be received from the associated operation.

Sure. The ambiguity is to what extent the converse is true.

[athoelke]

In addition, this function should perhaps also check that the algorithm is valid. For example a GCM algorithm identifier can be constructed with a 9-byte tag length - and this API should report that this is invalid.

[athoelke]

I've reworked this now, and also included the key-management usage flags. Warrants a re-review

[gilles-peskine-arm]

Mostly LGTM (apart from one occurrence of the old numerical value), but there are still a couple of points of discussion.

[gilles-peskine-arm]

Should we change that to allow either PSA_KEY_USAGE_DERIVE or PSA_KEY_USAGE_DERIVE_PUBLIC?

The situation is actually somewhat similar to PSA_KEY_USAGE_SIGN_MESSAGE vs PSA_KEY_USAGE_SIGN_HASH, in that we had keys originally created with only the SIGN_HASH permission and we wanted to allow them for an operation that is naturally controlled by SIGN_MESSAGE. What we did then was to say that any key created with the SIGN_MESSAGE flag also gains the SIGN_HASH flag (reported if you query the key's usage). So maybe we should do the same here, namely say that any key created with PSA_KEY_USAGE_DERIVE also gains PSA_KEY_USAGE_DERIVE_PUBLIC (if the crypto core complies with spec 1.4), in addition to changing the requirement in psa_pake_setup to require DERIVE_PUBLIC when doing an operation in the verifier role?

[athoelke]

It was the other way around: setting PSA_KEY_USAGE_SIGN_HASH causes PSA_KEY_USAGE_SIGN_MESSAGE to also be set for the key. But the pattern fits in this case as well.

As well as source code compatibility, we probably ought to consider an existing key store in which a SPAKE2+ verifier (public) key only has DERIVE usage, and for which the policy is not updatable. In this case, we would want the updated implementation to accept a SPAKE2+ public key with DERIVE usage. Perhaps the wording should be that keys created with DERIVE usage, implicitly also have DERIVE_PUBLIC usage - and it is undefined whether that happens when the key is stored, or when the usage flags are inspected?

For newly created/derived keys (if we introduce this change), then the ('connected registration' flow in Figure 4 of SPAKE2+ registration) would set DERIVE_PUBLIC on the key imported into the Verifier key store, instead of DERIVE.

If the verifier key is not transferred from the Prover during the registration phase, and both roles start from the shared password ('independent registration' in Figure 4 of SPAKE2+ registration) - then the Verifier will derive the SPAKE2+ key pair from the password, and just provide the key-pair to the PAKE operation for the Verifier role. In this case the verifier could just set DERIVE_PUBLIC usage on the key-pair, which restricts it to only be valid in the Verifier role (and makes the Prover part of the key-pair unusable in that key object).

In both cases, DERIVE usage should be permitted, for source code compatibility for applications written against the current spec - this utilises the 'implicit DERIVE_PUBLIC' rule described above.

[athoelke]

I think that adopting PSA_KEY_USAGE_DERIVE_PUBLIC as a 1st-class policy flag, as described here, is not necessary for this particular PR that introduces the flag to enable the desired psa_check_key_usage() behavior.

I am going to split this idea, and its discussion, into a separate Issue, that can be pursued as a follow-up to this PR.

[gilles-peskine-arm]

Right now, the corresponding function in TF-PSA-Crypto (not psa_check_key_usage(), but a function that is meant to be compatible when its key argument is a PSA key) accepts a 512-bit RSA key for PSA_ALG_RSA_OAEP(PSA_ALG_SHA_512) (if applicable policies are ok), even though the key cannot ever work for this algorithm (OAEP requires a key that's larger than the hash plus a few bytes of overhead). It works this way because we only check the key type, the algorithm and the policy.

(Note that, other than being too small for comfort, there's nothing wrong with the key itself. If its policy has PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH), the key can be used for OAEP-MD5, OAEP-SHA1, etc.)

I think this is a bug, because I think psa_check_key_usage(key, alg, usage) is intended to return err if an operation based on the given usage would return err for the given key and algorithm. Attempting to encrypt or decrypt with this small RSA key using this specific hash-parametrized algorithm would always fail, so the usage check should also fail.

However, I don't find the specification fully clear here. I would like something (a remark, an example, …) that explicitly states that knowing the type and policy of the key is not always enough to determine the result.

I've made the changes

[gilles-peskine-arm]

Just a few typos, and of course #282 (comment) is still a todo (but we can fill it in later).

[gilles-peskine-arm]

A key factor is that this shouldn't require any “math”, since the core must be able to decide without calling a driver (we aren't planning a driver entry point for this API).

[athoelke]

Will maybe add this as an implementation note?

[gilles-peskine-arm]

LGTM