PK: inline & simplify check_pair functions

mpg commented

2025-10-14 08:15:02 -05:00

(Migrated from github.com)

Similar to #515 and #519 but with check_pair_func. Reminder of the steps:

Review existing tests (see below) and possibly complete them.
Refactor
Optional: if the refactoring results in more cases being naturally supported, add corresponding test cases.

Step 1:

Positive: matching key pairs, both normal
Positive: matching key pairs, private is wrapping
Negative: non-matching pairs of the same type (different key materials, same bit size or curve)
Negative: non-matching pairs of the same type (RSA and ECC: different bit/curve size, ECC: same bit size but different curve)
Negative: non-matching pairs of different types (RSA vs ECC)
Negative: non-matching pairs (different key material), private is wrapping

Step 2: the usual.

Step 3: currently wrapping (formerly "opaque") RSA keys are not supported, consider adding support for them as it would probably use the same code path as ECC (just with different sizes).

Similar to #515 and #519 but with `check_pair_func`. Reminder of the steps: 1. Review existing tests (see below) and possibly complete them. 2. Refactor 3. Optional: if the refactoring results in more cases being naturally supported, add corresponding test cases. Step 1: - Positive: matching key pairs, both normal - Positive: matching key pairs, private is wrapping - Negative: non-matching pairs of the same type (different key materials, same bit size or curve) - Negative: non-matching pairs of the same type (RSA and ECC: different bit/curve size, ECC: same bit size but different curve) - Negative: non-matching pairs of different types (RSA vs ECC) - Negative: non-matching pairs (different key material), private is wrapping Step 2: the usual. Step 3: currently wrapping (formerly "opaque") RSA keys are not supported, consider adding support for them as it would probably use the same code path as ECC (just with different sizes).

gilles-peskine-arm commented

2025-10-14 08:41:41 -05:00

(Migrated from github.com)

For check_pair, I think it's easiest to write the implementation from scratch than do do progressive refactoring:

PSA_KEY_TYPE_IS_KEY_PAIR(get_psa_key_type(prv)) &&
get_psa_key_type(pub) == PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR(get_psa_key_type(prv)) &&
get_psa_key_bits(pub) == get_psa_key_bits(prv) &&
prv->pub_raw_len == pub->pub_raw_len &&
!memcmp(prv->pub_raw, pub->pub_raw, pub->pub_raw_len)

That's already how it's done when the private key is wrapped.

For `check_pair`, I think it's easiest to write the implementation from scratch than do do progressive refactoring: ``` PSA_KEY_TYPE_IS_KEY_PAIR(get_psa_key_type(prv)) && get_psa_key_type(pub) == PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR(get_psa_key_type(prv)) && get_psa_key_bits(pub) == get_psa_key_bits(prv) && prv->pub_raw_len == pub->pub_raw_len && !memcmp(prv->pub_raw, pub->pub_raw, pub->pub_raw_len) ``` That's already how it's done when the private key is wrapped.

👍 1

mpg commented

2025-12-16 03:34:50 -06:00

(Migrated from github.com)

I agree with Gille's proposal for a direct rewrite. I'll note that get_psa_key_type(prv) was pseudo-code at the time this issue was created, and can now be written as prv->psa_type.

Also get_psa_key_bits() is actually mbedtls_pk_get_bitlen() of course.

I think considering this function quite simple, we don't need a full test matrix (all 5 ways of creating each input context + each type for each input context, etc) and it's enough to ensure full branch coverage in the proposed implementation.

I'll review the existing tests and make a proposal if they need to be extended.

I agree with Gille's proposal for a direct rewrite. I'll note that `get_psa_key_type(prv)` was pseudo-code at the time this issue was created, and can now be written as `prv->psa_type`. Also `get_psa_key_bits()` is actually `mbedtls_pk_get_bitlen()` of course. I think considering this function quite simple, we don't need a full test matrix (all 5 ways of creating each input context + each type for each input context, etc) and it's enough to ensure full branch coverage in the proposed implementation. I'll review the existing tests and make a proposal if they need to be extended.

mpg commented

2026-01-06 05:41:50 -06:00

(Migrated from github.com)

@gilles-peskine-arm Regarding your proposed implementation

PSA_KEY_TYPE_IS_KEY_PAIR(get_psa_key_type(prv)) &&
get_psa_key_type(pub) == PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR(get_psa_key_type(prv)) &&
get_psa_key_bits(pub) == get_psa_key_bits(prv) &&
prv->pub_raw_len == pub->pub_raw_len &&
!memcmp(prv->pub_raw, pub->pub_raw, pub->pub_raw_len)

I'm really not sure we need to check get_psa_key_bits(pub) == get_psa_key_bits(prv) in addition to the following two lines (compare raw buffers including their lengths).

I think the most important check here is comparing the buffers content (including lengths). I agree we should also check key types in order to enforce proper usage and protect against possible polyglot keys (sequences of bytes that could be parsed either as a valid ECC key or as a valid RSA key - not sure if that's possible or not, but I don't want to have to think about it, so just check the types).

So I'd like to suggest just:

PSA_KEY_TYPE_IS_KEY_PAIR(get_psa_key_type(prv)) &&
get_psa_key_type(pub) == PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR(get_psa_key_type(prv)) &&
prv->pub_raw_len == pub->pub_raw_len &&
!memcmp(prv->pub_raw, pub->pub_raw, pub->pub_raw_len)

My reasoning is that this matches the (non-policy) mandatory inputs to psa_import_public_key(): in addition to buffer length and content, key type is a mandatory input (via the attributes), but key size is determined from the buffer (can be set in the attributes too but that's optional).

Wdyt? Am I missing something or were you just overly cautious here?

(Where I'm coming from is that I have a hard time coming up with a test case where only the bitlen check would fail, but not the buffer length+content comparison.)

@gilles-peskine-arm Regarding your proposed implementation ```c PSA_KEY_TYPE_IS_KEY_PAIR(get_psa_key_type(prv)) && get_psa_key_type(pub) == PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR(get_psa_key_type(prv)) && get_psa_key_bits(pub) == get_psa_key_bits(prv) && prv->pub_raw_len == pub->pub_raw_len && !memcmp(prv->pub_raw, pub->pub_raw, pub->pub_raw_len) ``` I'm really not sure we need to check `get_psa_key_bits(pub) == get_psa_key_bits(prv)` in addition to the following two lines (compare raw buffers including their lengths). I think the most important check here is comparing the buffers content (including lengths). I agree we should also check key types in order to enforce proper usage and protect against possible polyglot keys (sequences of bytes that could be parsed either as a valid ECC key or as a valid RSA key - not sure if that's possible or not, but I don't want to have to think about it, so just check the types). So I'd like to suggest just: ```c PSA_KEY_TYPE_IS_KEY_PAIR(get_psa_key_type(prv)) && get_psa_key_type(pub) == PSA_KEY_TYPE_PUBLIC_KEY_OF_KEY_PAIR(get_psa_key_type(prv)) && prv->pub_raw_len == pub->pub_raw_len && !memcmp(prv->pub_raw, pub->pub_raw, pub->pub_raw_len) ``` My reasoning is that this matches the (non-policy) mandatory inputs to `psa_import_public_key()`: in addition to buffer length and content, key type is a mandatory input (via the attributes), but key size is determined from the buffer (can be set in the attributes too but that's optional). Wdyt? Am I missing something or were you just overly cautious here? (Where I'm coming from is that I have a hard time coming up with a test case where only the bitlen check would fail, but not the buffer length+content comparison.)

mpg commented

2026-01-06 05:55:58 -06:00

(Migrated from github.com)

Test design

Review of the existing

Documentation

Currently it says:

 * \note            This function currently does not work on keys created with
 *                  \c mbedtls_pk_wrap_psa().

I'm not sure that's correct: I think the function already works when prv is a wrapping
ECC key (but not when it's a wrapping RSA key).

Anyway, this limitation is a historical accident and it makes no sense
preserving it. So, let's remove this note from mbedtls_pk_check_pair()'s
documentation, and remove the corresponding point from mbedtls_pk_wrap_psa()'s
documentation too.

The documentation promises specific error codes on specific conditions. We
should try to preserve them when applicable:

#MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE if the keys could not be checked - well
that should no longer happen, after lifting the restriction mentioned above.
#PSA_ERROR_INVALID_ARGUMENT if a context is invalid - OK, we should keep
returning that if a context has not been populated, or if we're passed a
public key as the private key argument.
Another non-zero value if the keys do no match - OK, that leaves us some
freedom. We can try preserving existing return codes when it's free, but we're
free to change them if that's more convenient.

Existing test functions

pk_psa_utils(): check that it fails with "opaque" (wrapping) contexts (RSA,
ECC) -> no longer relevant, remove that.
mbedtls_pk_check_pair(): inputs are two files + expected return code, allows
both positive and some negative testing.
- first call to check_pair() with with both contexts created by parsing
- second call is with prv wrapping and pub still create by parsing (and is
  currently expecting failure for RSA keys, but we're changing that).
pk_copy_from_psa_success(): calls check_pair() with contexts created by
copy_from_psa() (both public and private). Also has a 2 other contexts:
pk_priv_copy_public and pk_pub_copy_public, created with
copy_public_from_psa() but those are not tested with
check_pair.

Existing test cases

pk_psa_utils:: no longer relevant, we're removing check_pair() from here,
see above.
mbedtls_pk_check_pair: test cases:
1. positive: ECC 256r1 vs matching
2. negative: ECC 256r1 vs non-matching (also ECC 256r1)
3. positive: RSA 2048 vs matching
4. negative: RSA 2048 vs non-matching (also RSA 2048)
5. negative: pub ECC 256r1 vs prv RSA 2048
pk_copy_from_psa_success: (positive only):
- ECC 256r1, 521r1, 256k1
- RSA 2048

Proposed test plan

We keep all existing test functions and cases except for the check_pair-specific par of pk_psa_utils() as mentioned above. We also extend the existing test functions and add new test cases for them.

Missing test cases to be added

Positive:

newly supported case: prv is wrapping RSA -> just adjust existing code in
mbedtls_pk_check_pair() so that RSA keys are tested the same as ECC keys
(actually a simplification, no longer need to distinguish ECC vs RSA).
public key was created with copy_public_from_psa -> extend
pk_copy_from_psa_success() to also call check_pair on the existing
contexts pk_priv_copy_public and pk_pub_copy_public in addition to the
existing call.

Negative:

pub or prv hasn't been populated -> add an empty context in
the existing test function mbedtls_pk_check_pair(), add calls with (pub,
empty) and (empty, prv) and always expect failure (in addition to normal
testing that can either succeed or fail depending on the data).
The function expects (pub, prv) -> what if we pass (pub, pub), (prv, pub) or
(prv, prv)? -> as above, we can just add calls with the wrong argument in
the existing test function mbedtls_pk_check_pair() always expecting failure.
(All other items below assume arguments in the correct order and populated.)
pub and prv have the same type (both RSA or both ECC) but different sizes ->
should test with both pub larger than prv and prv larger than prv (we want to
make sure our internal memcmp() never overreads). Can be done by adding new
negative test cases for mbedtls_pk_check_pair: with varying key sizes. Since
the function accepts two file names as arguments, it should be a simple matter
of locating existing files with keys of different sizes, and adjusting the
test case dependencies.
pub and prv are both ECC keys of the same bitsize, but different curves ->
again can be done by adding a new case for mbedtls_pk_check_pair: as above.

Missing test cases that we accept

Negative testing is only done with contexts populater by parsing or wrapping,
contexts populated with copy_from_psa() or copy_public_from_psa() are only
used in positive testing. I think that's acceptable considering how simple the
function is, and that positive testing of copy contexts is enough to establish
that their pub_raw and pub_raw_len are correctly populated.

Wdty @bjwtaylor @valeriosetti ?

## Test design ### Review of the existing **Documentation** Currently it says: ``` * \note This function currently does not work on keys created with * \c mbedtls_pk_wrap_psa(). ``` I'm not sure that's correct: I think the function already works when prv is a wrapping ECC key (but not when it's a wrapping RSA key). Anyway, this limitation is a historical accident and it makes no sense preserving it. So, let's remove this note from `mbedtls_pk_check_pair()`'s documentation, and remove the corresponding point from `mbedtls_pk_wrap_psa()`'s documentation too. The documentation promises specific error codes on specific conditions. We should try to preserve them when applicable: - `#MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE` if the keys could not be checked - well that should no longer happen, after lifting the restriction mentioned above. - `#PSA_ERROR_INVALID_ARGUMENT` if a context is invalid - OK, we should keep returning that if a context has not been populated, or if we're passed a public key as the private key argument. - Another non-zero value if the keys do no match - OK, that leaves us some freedom. We can try preserving existing return codes when it's free, but we're free to change them if that's more convenient. **Existing test functions** - `pk_psa_utils()`: check that it fails with "opaque" (wrapping) contexts (RSA, ECC) -> no longer relevant, remove that. - `mbedtls_pk_check_pair()`: inputs are two files + expected return code, allows both positive and some negative testing. - first call to `check_pair()` with with both contexts created by parsing - second call is with prv wrapping and pub still create by parsing (and is currently expecting failure for RSA keys, but we're changing that). - `pk_copy_from_psa_success()`: calls `check_pair()` with contexts created by `copy_from_psa()` (both public and private). Also has a 2 other contexts: `pk_priv_copy_public` and `pk_pub_copy_public`, created with `copy_public_from_psa()` but those are not tested with `check_pair`. **Existing test cases** - `pk_psa_utils:`: no longer relevant, we're removing `check_pair()` from here, see above. - `mbedtls_pk_check_pair:` test cases: 1. positive: ECC 256r1 vs matching 2. negative: ECC 256r1 vs non-matching (also ECC 256r1) 3. positive: RSA 2048 vs matching 4. negative: RSA 2048 vs non-matching (also RSA 2048) 5. negative: pub ECC 256r1 vs prv RSA 2048 - `pk_copy_from_psa_success:` (positive only): - ECC 256r1, 521r1, 256k1 - RSA 2048 ## Proposed test plan We keep all existing test functions and cases except for the `check_pair`-specific par of `pk_psa_utils()` as mentioned above. We also extend the existing test functions and add new test cases for them. **Missing test cases to be added** _Positive_: - newly supported case: prv is wrapping RSA -> just adjust existing code in `mbedtls_pk_check_pair()` so that RSA keys are tested the same as ECC keys (actually a simplification, no longer need to distinguish ECC vs RSA). - public key was created with `copy_public_from_psa` -> extend `pk_copy_from_psa_success()` to also call `check_pair` on the existing contexts `pk_priv_copy_public` and `pk_pub_copy_public` in addition to the existing call. _Negative_: - pub or prv hasn't been populated -> add an `empty` context in the existing test function `mbedtls_pk_check_pair()`, add calls with (pub, empty) and (empty, prv) and always expect failure (in addition to normal testing that can either succeed or fail depending on the data). - The function expects (pub, prv) -> what if we pass (pub, pub), (prv, pub) or (prv, prv)? -> as above, we can just add calls with the wrong argument in the existing test function `mbedtls_pk_check_pair()` always expecting failure. (All other items below assume arguments in the correct order and populated.) - pub and prv have the same type (both RSA or both ECC) but different sizes -> should test with both pub larger than prv and prv larger than prv (we want to make sure our internal `memcmp()` never overreads). Can be done by adding new negative test cases for `mbedtls_pk_check_pair:` with varying key sizes. Since the function accepts two file names as arguments, it should be a simple matter of locating existing files with keys of different sizes, and adjusting the test case dependencies. - pub and prv are both ECC keys of the same bitsize, but different curves -> again can be done by adding a new case for `mbedtls_pk_check_pair:` as above. **Missing test cases that we accept** Negative testing is only done with contexts populater by parsing or wrapping, contexts populated with `copy_from_psa()` or `copy_public_from_psa()` are only used in positive testing. I think that's acceptable considering how simple the function is, and that positive testing of `copy` contexts is enough to establish that their `pub_raw` and `pub_raw_len` are correctly populated. Wdty @bjwtaylor @valeriosetti ?

👍 1

gilles-peskine-arm commented

2026-01-06 06:15:16 -06:00

(Migrated from github.com)

It's definitely possible for two key pairs of different types to have the same public key representation. A simple example would be two different elliptic curves of the same size. Actually finding one would involve solving a discrete log, though. Generally speaking, finding a key pair from a public key is supposed to be hard!

Maybe it's actually feasible between Curve25519 and Ed25519, because of the mathematical relationship between the two curves?

With the same type but different bit-sizes, I don't think it's currently possible to have the same public key representation. As you note, psa_import_key can deduce the bit-size from the type and public key representation. However, this isn't an absolute guarantee that we make in the API design. We just tried to make import more convenient, and we've succeeded for all currently defined key types, but we can't promise that we'll always succeed.

Conceptually the bit-size is almost part of the key type. I had considered making it part of the key type, but I ended up treating them as separate attributes because I thought it would be too inconvenient sometimes, especially with RSA, key derivation fodder and HMAC, for which there's a wide range of sensible key sizes.

Comparing the bit-sizes is extremely cheap. Not comparing the bit-sizes is probably safe but it's tricky to justify. So I think that omitting the bit-size comparison is a useless risk.

It's definitely possible for two key pairs of different types to have the same public key representation. A simple example would be two different elliptic curves of the same size. Actually finding one would involve solving a discrete log, though. Generally speaking, finding a key pair from a public key is supposed to be hard! Maybe it's actually feasible between Curve25519 and Ed25519, because of the mathematical relationship between the two curves? With the same type but different bit-sizes, I don't think it's _currently_ possible to have the same public key representation. As you note, `psa_import_key` can deduce the bit-size from the type and public key representation. However, this isn't an absolute guarantee that we make in the API design. We just tried to make import more convenient, and we've succeeded for all currently defined key types, but we can't promise that we'll always succeed. Conceptually the bit-size is almost part of the key type. I had considered making it part of the key type, but I ended up treating them as separate attributes because I thought it would be too inconvenient sometimes, especially with RSA, key derivation fodder and HMAC, for which there's a wide range of sensible key sizes. Comparing the bit-sizes is extremely cheap. Not comparing the bit-sizes is probably safe but it's tricky to justify. So I think that omitting the bit-size comparison is a useless risk.

mpg commented

2026-01-07 02:49:34 -06:00

(Migrated from github.com)

Makes sense. Let's keep the bitlength check then.

Minor update to my proposed test plan then: negative test with two RSA keys of the same bitlength (ie, same modulus size) but public exponents of different byte sizes (eg, one with the usual e = 65537, another with e = 3). This is to ensure branch coverage of the case where the bitlengths are identical but the pub_raw_len differ.

It might be the case that all our existing RSA keys have e = 65537, so we may need to generate a test keypair with a different e just for this.

Makes sense. Let's keep the bitlength check then. Minor update to my proposed test plan then: negative test with two RSA keys of the same bitlength (ie, same modulus size) but public exponents of different byte sizes (eg, one with the usual e = 65537, another with e = 3). This is to ensure branch coverage of the case where the bitlengths are identical but the `pub_raw_len` differ. It might be the case that all our existing RSA keys have e = 65537, so we may need to generate a test keypair with a different e just for this.

mpg commented

2026-01-07 03:11:16 -06:00

(Migrated from github.com)

It might be the case that all our existing RSA keys have e = 65537, so we may need to generate a test keypair with a different e just for this.

Well it seems to be the case indeed:

% for f in *.key rsa*.pem; do                                                                           git|heads/main|
if grep 'RSA PRIVATE' $f >/dev/null && ! grep ENCRYPTED $f >/dev/null; then
printf "$f: "; openssl rsa -in $f -text -noout | grep publicExp
fi
done
cli-rsa.key: publicExponent: 65537 (0x10001)
format_rsa.key: publicExponent: 65537 (0x10001)
rsa512.key: publicExponent: 65537 (0x10001)
rsa521.key: publicExponent: 65537 (0x10001)
rsa522.key: publicExponent: 65537 (0x10001)
rsa528.key: publicExponent: 65537 (0x10001)
server1.key: publicExponent: 65537 (0x10001)
server2.key: publicExponent: 65537 (0x10001)
server4.key: publicExponent: 65537 (0x10001)
server8.key: publicExponent: 65537 (0x10001)
server9.key: publicExponent: 65537 (0x10001)
test-ca-alt.key: publicExponent: 65537 (0x10001)
test-ca_unenc.key: publicExponent: 65537 (0x10001)
test-int-ca.key: publicExponent: 65537 (0x10001)
rsa4096_prv.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_1024_clear.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_2048_clear.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_4096_clear.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_768_clear.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_769_clear.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_770_clear.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_776_clear.pem: publicExponent: 65537 (0x10001)
rsa_pkcs1_784_clear.pem: publicExponent: 65537 (0x10001)

However generating a key with a different exponent should be easy:

openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:4294967297 -out rsa_larger_pubexp_private.pem
openssl rsa -in rsa_larger_pubexp_private.pem -pubout -out rsa_larger_pubexp_public.pem

(Just requires a preliminary PR to the framework to add these two files (and corresponding Makefile entries) before we can use them in tests.)

> It might be the case that all our existing RSA keys have e = 65537, so we may need to generate a test keypair with a different e just for this. Well it seems to be the case indeed: ``` % for f in *.key rsa*.pem; do git|heads/main| if grep 'RSA PRIVATE' $f >/dev/null && ! grep ENCRYPTED $f >/dev/null; then printf "$f: "; openssl rsa -in $f -text -noout | grep publicExp fi done cli-rsa.key: publicExponent: 65537 (0x10001) format_rsa.key: publicExponent: 65537 (0x10001) rsa512.key: publicExponent: 65537 (0x10001) rsa521.key: publicExponent: 65537 (0x10001) rsa522.key: publicExponent: 65537 (0x10001) rsa528.key: publicExponent: 65537 (0x10001) server1.key: publicExponent: 65537 (0x10001) server2.key: publicExponent: 65537 (0x10001) server4.key: publicExponent: 65537 (0x10001) server8.key: publicExponent: 65537 (0x10001) server9.key: publicExponent: 65537 (0x10001) test-ca-alt.key: publicExponent: 65537 (0x10001) test-ca_unenc.key: publicExponent: 65537 (0x10001) test-int-ca.key: publicExponent: 65537 (0x10001) rsa4096_prv.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_1024_clear.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_2048_clear.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_4096_clear.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_768_clear.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_769_clear.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_770_clear.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_776_clear.pem: publicExponent: 65537 (0x10001) rsa_pkcs1_784_clear.pem: publicExponent: 65537 (0x10001) ``` However generating a key with a different exponent should be easy: ``` openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:4294967297 -out rsa_larger_pubexp_private.pem openssl rsa -in rsa_larger_pubexp_private.pem -pubout -out rsa_larger_pubexp_public.pem ``` (Just requires a preliminary PR to the framework to add these two files (and corresponding Makefile entries) before we can use them in tests.)

bjwtaylor commented

2026-01-13 02:46:44 -06:00

(Migrated from github.com)

@mpg, just got round to reviewing this. Mostly makes sense, however just one thing to confirm on wrapped keys. In the current implementation, I've done the first part of inlining the functions and we currently only support wrapped ECC keys. We do not currently support wrapped RSA keys. I've not dug into why this is, though from the above it sounds like it's something we want to investigate further and resolve as there is no architectural reason why this should be the case is this correct?

mpg commented

2026-01-13 03:05:40 -06:00

(Migrated from github.com)

Indeed, currently we only support check_pair() for wrapped ECC keys, but not RSA keys. The reason is entirely "we never came around to implementing it for RSA keys", and as you say there is no architectural reason, just historical happenstance.

My reasoning is at this point, where we are unifying the implementation for all key types (wrapped or not, RSA or ECC), maintaining the restriction what wrapped RSA keys are not supported would actually make the code (and documentation and tests) more complicated than removing this historical restriction, so I'm suggesting we just remove the restriction. Clearer code, more features, less to document, just slightly different set of things to test.

Indeed, currently we only support `check_pair()` for wrapped ECC keys, but not RSA keys. The reason is entirely "we never came around to implementing it for RSA keys", and as you say there is no architectural reason, just historical happenstance. My reasoning is at this point, where we are unifying the implementation for all key types (wrapped or not, RSA or ECC), maintaining the restriction what wrapped RSA keys are not supported would actually make the code (and documentation and tests) more complicated than removing this historical restriction, so I'm suggesting we just remove the restriction. Clearer code, more features, less to document, just slightly different set of things to test.

bjwtaylor commented

2026-01-13 03:08:18 -06:00

(Migrated from github.com)

OK, thanks for the confirmation. I'll get on with working out how to remove this restriction.

👍 1

mpg commented

2026-01-13 03:20:28 -06:00

(Migrated from github.com)

Note that the reason I'm suggesting we remove this restriction is I believe there is nothing to work out: just using the simple implementation suggested by Gilles should to the trick. Well, for the implementation part - for the testing part, I've outlined above but basically it's remove the negative test for this case and add a positive one.

Basically the restriction exists because each key type had a different implementation, in particular "wrapped ECC" and "wrapped RSA" had a different one - and we never wrote that for "wrapped RSA". Here, there will be code path to implement them all, and it should "just work" for wrapped RSA as it does for all the others.

Note that the reason I'm suggesting we remove this restriction is I believe there is nothing to work out: just using the simple implementation [suggested by Gilles](https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/521#issuecomment-3401958915) should to the trick. Well, for the implementation part - for the testing part, I've outlined above but basically it's remove the negative test for this case and add a positive one. Basically the restriction exists because each key type had a different implementation, in particular "wrapped ECC" and "wrapped RSA" had a different one - and we never wrote that for "wrapped RSA". Here, there will be code path to implement them all, and it should "just work" for wrapped RSA as it does for all the others.

bjwtaylor commented

2026-01-13 08:06:57 -06:00

(Migrated from github.com)

So, initially I had a implementation with the check on wrapped RSA removed and the system generated a MBEDTLS_ERR_ASN1_BUF_TOO_SMALL error which David pointed out in this thread nhttps://github.com/Mbed-TLS/TF-PSA-Crypto/pull/580#discussion_r2606315448 eeded to be restored to the original error. So in the current implementation we have a check implemented here github.com/bjwtaylor/TF-PSA-Crypto@d0b3565e85/drivers/builtin/src/pk.c (L1358) to return, I agree though I can't think of an architectural reason why this should be needed. So I'll look into removing it.

So, initially I had a implementation with the check on wrapped RSA removed and the system generated a MBEDTLS_ERR_ASN1_BUF_TOO_SMALL error which David pointed out in this thread nhttps://github.com/Mbed-TLS/TF-PSA-Crypto/pull/580#discussion_r2606315448 eeded to be restored to the original error. So in the current implementation we have a check implemented here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/d0b3565e8596903726e1fb28f2853aa75c40aba0/drivers/builtin/src/pk.c#L1358 to return, I agree though I can't think of an architectural reason why this should be needed. So I'll look into removing it.

Rows
Columns

PK: inline & simplify check_pair functions #521

Test design

Review of the existing

Proposed test plan