PK: remove `pk_can_do()` #523

New issue

Open

opened 2025-10-15 03:49:55 -05:00 by mpg · 2 comments

mpg commented

2025-10-15 03:49:55 -05:00

(Migrated from github.com)

The function mbedtls_pk_can_do() should be removed. Before that we need to remove the last places it's used:

Externally in mbedtls: covered by https://github.com/Mbed-TLS/mbedtls/issues/10453
Internally in mbedtls_pk_can_do_ext(): well fortunately we want to remove that function as well, and it's not used anywhere so that's an easy one.
Internally in mbedtls_pk_sign_ext() and mbedtls_pk_verify_ext(): see discussion below.

Regarding the last point, it is entirely possible that the check is redundant these days and can just be removed, because ultimately psa_sign_hash() (or psa_verify_hash()) will return an error if the key is not capable of doing the operation anyway. So, the first thing I'd try is to simply remove the check and see if the tests still pass. However before that we want to double-check we have sufficient negative testing for sign_ext() and verify_ext(). (Possibly we'll need to ensure PSA_ERROR_NOT_PERMITTED is correctly translated to MBEDTLS_ERR_PK_TYPE_MISMATCH which might involve switching from PSA_PK_RSA_TO_MBEDTLS_ERR() to PSA_PK_TO_MBEDTLS_ERR().)

If this approach turns out not to work for some reason, we can always fall back to using mbedtls_pk_can_do_psa(), but I don't think that should be necessary.

Note: even though the issue title is about pk_can_do() (because the definition of done is this function is gone) the bulk of the work is refactoring sign_ext() and verify_ext(), and as such we need to ensure we have proper testing (including negative) in place before we touch the code. Also, we cannot change observable behaviour unless it was undocumented.

Depends on: https://github.com/Mbed-TLS/mbedtls/issues/10453

Potential conflicts: this is very likely going to conflict with #515 and #519 since all of them are touching the same library and test functions.

The function `mbedtls_pk_can_do()` should be removed. Before that we need to remove the last places it's used: - Externally in mbedtls: covered by https://github.com/Mbed-TLS/mbedtls/issues/10453 - Internally in `mbedtls_pk_can_do_ext()`: well fortunately we want to remove that function as well, and it's not used anywhere so that's an easy one. - Internally in `mbedtls_pk_sign_ext()` and `mbedtls_pk_verify_ext()`: see discussion below. Regarding the last point, it is entirely possible that the check is redundant these days and can just be removed, because ultimately `psa_sign_hash()` (or `psa_verify_hash()`) will return an error if the key is not capable of doing the operation anyway. So, the first thing I'd try is to simply remove the check and see if the tests still pass. However before that we want to double-check we have sufficient negative testing for `sign_ext()` and `verify_ext()`. (Possibly we'll need to ensure `PSA_ERROR_NOT_PERMITTED` is correctly translated to `MBEDTLS_ERR_PK_TYPE_MISMATCH` which might involve switching from `PSA_PK_RSA_TO_MBEDTLS_ERR()` to `PSA_PK_TO_MBEDTLS_ERR()`.) If this approach turns out not to work for some reason, we can always fall back to using `mbedtls_pk_can_do_psa()`, but I don't think that should be necessary. Note: even though the issue title is about `pk_can_do()` (because the definition of done is this function is gone) the bulk of the work is refactoring `sign_ext()` and `verify_ext()`, and as such we need to ensure we have proper testing (including negative) in place before we touch the code. Also, we cannot change observable behaviour unless it was undocumented. **Depends on:** https://github.com/Mbed-TLS/mbedtls/issues/10453 **Potential conflicts:** this is very likely going to conflict with #515 and #519 since all of them are touching the same library and test functions.

gilles-peskine-arm commented

2025-10-15 10:01:00 -05:00

(Migrated from github.com)

See https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/203 and https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/204 for an analysis and partial prototype of the removal of mbedtls_pk_type_t, mbedtls_pk_get_type and mbedtls_pk_can_do.

See https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/203 and https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/204 for an analysis and partial prototype of the removal of `mbedtls_pk_type_t`, `mbedtls_pk_get_type` and `mbedtls_pk_can_do`.

valeriosetti commented

2025-12-19 02:07:14 -06:00

(Migrated from github.com)

This is the test plan that I have implemented in #600

RSA PKCS v1.5 + SHA256
- copy_from_psa
  - positive: PKCS v1.5
  - negative[1]: PSS
  - negative[1]: PSS + SHA384
  - negative: PKCS v1.5 + no MD
- wrap_psa
  - positive: PKCS v1.5
  - negative: PSS
RSA PSS (ANY_SALT) + SHA256
- copy_from_psa
  - positive: PKCS v1.5
  - negative[1]: PSS
  - negative[1]: PSS + SHA384
  - negative: PSS + no MD
- wrap_psa
  - positive: PSS
  - negative[1]: PKCS v1.5
  - negative: PSS + no MD
  - negative: PSS + SHA384
RSA PSS (ANY_SALT) + SHA256
- wrap_psa
  - positive: PSS + SHA384
secp256r1 + ECDSA(SHA256)
- copy_from_psa:
  - positive: ECDSA
  - negative: PSS
- wrap_psa:
  - positive: ECDSA
  - negative: PSS

[1]: these are tests which should ideally be negative, but they just happen to be positive for some reason. Explanations are provided on the data file, so please check there for further details.

This is the test plan that I have implemented in #600 - RSA PKCS v1.5 + SHA256 - copy_from_psa - positive: PKCS v1.5 - negative[1]: PSS - negative[1]: PSS + SHA384 - negative: PKCS v1.5 + no MD - wrap_psa - positive: PKCS v1.5 - negative: PSS - RSA PSS (ANY_SALT) + SHA256 - copy_from_psa - positive: PKCS v1.5 - negative[1]: PSS - negative[1]: PSS + SHA384 - negative: PSS + no MD - wrap_psa - positive: PSS - negative[1]: PKCS v1.5 - negative: PSS + no MD - negative: PSS + SHA384 - RSA PSS (ANY_SALT) + SHA256 - wrap_psa - positive: PSS + SHA384 - secp256r1 + ECDSA(SHA256) - copy_from_psa: - positive: ECDSA - negative: PSS - wrap_psa: - positive: ECDSA - negative: PSS [1]: these are tests which should ideally be negative, but they just happen to be positive for some reason. Explanations are provided on the data file, so please check there for further details.

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto#523

No description provided.

Rows
Columns

PK: remove pk_can_do() #523

PK: remove `pk_can_do()` #523