Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver-only ECC: TLS: avoid use of mbedtls_ecp_write_key() (with USE_PSA) #7406

Closed
mpg opened this issue Apr 5, 2023 · 3 comments · Fixed by #7791
Closed

driver-only ECC: TLS: avoid use of mbedtls_ecp_write_key() (with USE_PSA) #7406

mpg opened this issue Apr 5, 2023 · 3 comments · Fixed by #7791
Assignees
Labels
component-tls enhancement size-s Estimated task size: small (~2d)

Comments

@mpg
Copy link
Contributor

mpg commented Apr 5, 2023

Context: see #6839. This is a step towards sub-goal ECP.TLS.
Depends on: #7074

Currently TLS uses mbedtls_ecp_write_key() in one place: server-side in the 1.2 static ECDH key exchanges, where we have a private key as a pk_context (provisioned locally) and want to turn it into a proper PSA key.

After #7074, we'll already have the key in a PSA key slot inside the pk_context so there will be little do to compared to the present situation. Things become much more similar to the case where the pk_context is of type MBEDLTS_PK_OPAQUE, and probably a lot more code can be share with this case (basically the only thing that will change is in what fields things are stored).

This task is to remove the call to mbedtls_ecp_write_key() and all other ECP functions from the MBEDTLS_USE_PSA_CRYPTO version of ssl_get_ecdh_params_from_cert() and simplify the function as much as possible.

@mpg mpg added enhancement component-tls size-s Estimated task size: small (~2d) labels Apr 5, 2023
@mpg mpg changed the title driver-only ECP: TLS: avoid use of mbedtls_ecp_write_key() driver-only ECP: TLS: avoid use of mbedtls_ecp_write_key() (with USE_PSA) Jun 5, 2023
@mpg mpg changed the title driver-only ECP: TLS: avoid use of mbedtls_ecp_write_key() (with USE_PSA) driver-only ECC: TLS: avoid use of mbedtls_ecp_write_key() (with USE_PSA) Jun 15, 2023
@valeriosetti
Copy link
Contributor

As for #7405, it seems to me that also in this case what is being asked by this issue is already implemented in code:

status = psa_export_key(pk->priv_id, buf, sizeof(buf), &key_len);

Wdyt?

@valeriosetti valeriosetti self-assigned this Jun 15, 2023
@mpg
Copy link
Contributor Author

mpg commented Jun 16, 2023

I think the part about not using mbedtls_ecp_write_key() in configurations of interest is done (except it's when MBEDTLS_PK_USE_PSA_EC_DATA while the issue description says when USE_PSA, but that's OK, we can use ECP functions when ECP_C is present).

I'm not so sure about the part "and simplify the function as much as possible": right now in the non-opaque case we are exporting the key then re-importing it, which seems inefficient. I think when USE_PSA_EC_DATA is on, we should be able to merge the opaque and non-opaque case, thus also saving a bit of code size.

Something like this:

    switch (mbedtls_pk_get_type(pk)) {
        case MBEDTLS_PK_OPAQUE:
#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
        case MBEDTLS_PK_ECKEY:
        case MBEDTLS_PK_ECKEY_DH:
        case MBEDTLS_PK_ECDSA:
#endif
            // [just copy the key ID and get metadata from attributes]
            ret = 0;
            break;
#if !defined(MBEDTLS_PK_USE_PSA_EC_DATA)
        case MBEDTLS_PK_ECKEY:
        case MBEDTLS_PK_ECKEY_DH:
        case MBEDTLS_PK_ECDSA:
           // [do it the hard way using ecp_write_key and psa_import]
            ret = 0;
            break;
#endif /* MBEDTLS_PK_USE_PSA_EC_DATA */
        default:
            ret = MBEDTLS_ERR_SSL_PK_TYPE_MISMATCH;
    }

(Not sure if that works, but you get the idea: if we can eliminate some code in the USE_PSA_EC_DATA case, we should.)

@valeriosetti
Copy link
Contributor

right now in the non-opaque case we are exporting the key then re-importing it, which seems inefficient.

Ok, I missed this part, sorry. I agree with your point so I'll try to optimize that code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-tls enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants