Fix psa_generate_key(): return PSA_ERROR_INVALID_ARGUMENT for public key

[mprse]

Description

Fixes #4551

Status

READY

Requires Backporting

Yes
development_2.x
PR #5038

Migrations

NO

Additional comments

Verified that with this fix the following code returns PSA_ERROR_INVALID_ARGUMENT (-135):

int main( int argc, char** argv )
{
    (void)argc;
    (void)argv;
    psa_status_t status;
    mbedtls_svc_key_id_t key_handle;
    psa_key_lifetime_t lifetime = PSA_KEY_LIFETIME_PERSISTENT;

    status = psa_crypto_init();
    if (status != PSA_SUCCESS) {
            printf("Init failed (status = %d)\n", status);
            return 1;
    }

    psa_key_attributes_t key_pair_attributes = PSA_KEY_ATTRIBUTES_INIT;
    psa_set_key_id(&key_pair_attributes, mbedtls_svc_key_id_make( 1, 1 ));
    psa_set_key_lifetime(&key_pair_attributes, lifetime);
    psa_set_key_usage_flags(&key_pair_attributes, PSA_KEY_USAGE_SIGN_HASH | PSA_KEY_USAGE_VERIFY_HASH |
                                                  PSA_KEY_USAGE_ENCRYPT | PSA_KEY_USAGE_DECRYPT |
                                                  PSA_KEY_USAGE_EXPORT );
    psa_set_key_algorithm(&key_pair_attributes, PSA_ALG_CTR);
    psa_set_key_type(&key_pair_attributes, PSA_KEY_TYPE_ECC_PUBLIC_KEY(0));
    psa_set_key_bits(&key_pair_attributes, 128U);

    status = psa_generate_key(&key_pair_attributes, &key_handle);
    if (status != PSA_SUCCESS) {
            printf("Key generation failed (status = %d)\n", status);
            return 1;
    }
}

Result:
Key generation failed (status = -135)

[AndrzejKurek]

There are some tests that will expect this new code as a result. Please see the travis logs, search for "failed".

[gilles-peskine-arm]

The change is correct, but I think the Python code (which admittedly was already not very well-structured before this PR) could be clearer.

[gilles-peskine-arm]

In NotSupported.test_cases_for_key_type_not_supported, the test for is-a-public-key is a bit different (kt.name.endswith('_PUBLIC_KEY')). I'm working on a follow-up that adds a method kt.is_public(). It would be better to have a single place that implements the test “is this key type for a public key?”, otherwise they risk diverging. Can you arrange to pass the information “is public key” down to this function?

Or maybe it would be better to have a completely separate function for the invalid-argument-for-public key case? I find this function rather messy. That's unavoidable to some extent because we want to produce output that has a lot of tweakable parts. Here, I suspect that keeping test_case_for_key_type_not_supported as it originally was, and making a separate function for the new case invalid-argument-for-public key, would make the code easier to maintain.

[mprse]

I would like to confirm if I understand correctly your proposition:

• restore original version of test_case_for_key_type_not_supported()
• add test_case_for_key_type_invalid_argument()

In this solution we would also require to modify NotSupported class (change it to NotSupported_InvalidArgumentor addInvalidArgument class". What option you suggest?

[gilles-peskine-arm]

Yes, that's what I meant.

If we want to make this really clean, taking a high-level view, the job of NotSupported is to enumerate combinations of (key type, algorithm, operation) and generate test cases for when one of them is not supported. If a combination is inherently impossible, rather than not supported by the current configuration of the library, then it's not NotSupported's job to handle this. #4444 will introduce the generation of negative test cases for impossible combinations, but only for operations on existing keys, not for key generation (and not for key import either, but there are no impossible combinations for import). This leaves a gap of impossible combinations for key generation, which are just the cases where the key type is a public key. So, to be clean, there should be a separate CreationFail class (distinct from NotSupported and OpFail).

However, this is an evolving test script, we shouldn't expend too much energy on it. Since NotSupported already does much of the job of CreationFail, we can keep it doing that, with a comment explaining that it's doing a little extra job on the side.

Despite my saing we shouldn't expend too much energy on this, I do still prefer to restore the original test_case_for_key_type_not_supported and make a separate function for the invalid-generate case. I see renaming NotSupported as optional: not-supported is still its main job.

[gilles-peskine-arm]

I've filed #5060 as a follow-up. It's not a top priority, but it is something we'll need eventually and it's a well-defined task that both adds a feature to generate_psa_tests.py and, I think, will make it a bit better structured. So @mprse if you don't have a more urgent task, I suggest you pick it up next.

[mprse]

@gilles-peskine-arm Can we merge this one? It would be easier to work on #5060.