Inline get bitlen #575
No reviewers
Labels
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/TF-PSA-Crypto!575
Loading…
Reference in a new issue
No description provided.
Delete branch "inline-get_bitlen"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Add additional tests to for the get bitlen functionality and simplify the current implementation by in lining it. Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/522
Verification report provided here
https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/575#issuecomment-3670594867
PR checklist
Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.
Thanks, I'll add this in
I've committed the change, though locally it seems to fail on keys bigger than 2048. Lets see if the CI agrees, though have I missed something?
Looks like the CI has failed with that variable set. From the code it looks like maybe I need MBEDTLS_TEST_PK_ALLOW_RSA_KEY_PAIR_4096 or MBEDTLS_TEST_STATIC_KEY_SLOTS_SUPPORT_RSA_4096 as this appears to get set here
github.com/Mbed-TLS/mbedtls-framework@6c9076eef1/tests/include/test/psa_crypto_helpers.h (L534)when the buffer is big enough to store 4096. Though I'm not sure this is what these variables were intended for?I've added a commit that changes the dependency to !MBEDTLS_PSA_STATIC_KEY_SLOTS, which I think it what @davidhorstmann-arm was suggesting. It seems to do the trick in the local tests, though let me know what people think and lets see if the CI agrees.
I don't see why changes added by this PR require this include. Why is it necessary?
I'm not a huge fan of this approach. Nowadays:
ctx->priv_idset (EC, RSA, Opaque);ctx->bitsset.I suggest to simplify this block like this:
It should work the same (TBH I didn't test it), but it drops the dependency on
strncmp().??
?
I think you can use
mbedtls_pk_copy_public_from_psa()here and get rid ofpsa_pub_key_from_priv()call and thepub_keyvariableThe call to
psa_destroy_key()here wasn't required before this PR as the key should normally be destroyed just before theexitlabel. Is this really necessary?Seconded. In addition to Valerio's points, eventually we want to remove
pk_infoentirely, so please don't rely on it in new code. (Also, eventually we want to remove most if not all strings from crypto, so don't rely on string comparison in new code.)In addition to the line comments I left, I'm not convinced by the test plan. There are currently 5 ways to populate a PK context:
mbedtls_pk_wrap_psa()mbedtls_pk_copy_from_psa()mbedtls_pk_copy_public_from_psa()mbedtls_pk_parse_key()mbedtls_pk_parse_public_key()We want each of them to be appropriately tested (both keypairs and public keys (except for (1)), both RSA and ECC, each with 2 different key sizes).
I'm satisfied that the first case is well covered by the improvements you made to
pk_psa_utils().With minor adaptations (see line comment) this function can probably also cover (2) and (3). On second thought, I'm not sure that would be right:
pk_psa_utils()is currently focused on wrapping, so adding unrelated stuff just muddies the water. Perhaps having a dedicated testing function would be cleaner. Or re-usepk_copy_from_psa_success()?However I'm more worried about (4) and (5). Superficially, it looks like it should be the job of the
pk_utils()test function, but it callspk_setup()which constructs the context manually. (Also, it only does keypairs, not public keys.)So, unless I missed something, (4) and (5) are not covered at all. I'm not sure what's the simplest way to test them at this point. One way would be to add a
bitlenparameter to the existing parsing functions intest_suite_pkparse, but that would probably mean a lot of lines changed. Or we could just add a dedicated function intest_suite_pkthat takes as input a serialized private or public key (with a parameter indicating which it is), and its expected bitlen, then calls the appropriate parse function, thenget_bitlen()and we're done.Wdyt?
On second thought, shouldn't we just ensure that
ctx->bitsis always set for all keys, as we do forpsa_typein #532? I don't really like struct having fields that are sometimes set sometimes not depending on context and you have to remember when - sure it's documented, but how often do we forget to update the documentation when we change the code?Pre-existing, but this looks wrong: we'll be creating a key pair, so we should depend on
RSA_KEY_PAIRnotRSA_PUBLIC_KEY.I think ideally, here or elsewhere, we want to test
get_bitlen()both on contexts created withcopy_from_psa()and withcopy_public_from_psa()(see my general comment about the test plan). It would probably be convenient to do it here.However this should either be done using new PK contexts, or after the existing "test that freeing the context does not destroy the key" block, that's currently below.
Even if the key is normally destroyed earlier, we should also consider the abnormal path where an assertion fails and we jump to exit early: in that case we still want to destroy the key in order to avoid any leaks. So, I think the previous version was wrong and the new one is more robust.
(Perhaps the comment on line 720 should be moved to the line before to make it clearer it applies to the whole paragraph not just its first line.)
Adding my personal opinion here. Unless changes to already existing test functions is trivial/simple I prefer to create new test functions for specific purposes. Extending what's already there makes test functions hard to read: lot of input parameters, lot of operations carried out internally... IMO this is not easy to review and maintain.
The worst consequence that I can think of in adding new test functions is that we duplicate some test that's already there. This is not bad though: it's true that testing will take slightly longer, but that's better than missing test coverage or make something hard to read.
It's not required, it should now be removed.
Now removed.
Should now be updated.
Now updated as per @mpg's comment.
So, @valeriosetti comment looks sensible. So I've updated the code to reflect this @mpg I'm not sure what you're after in your final comment. However let me know if you're unhappy with the original suggestion and we can change it.
@valeriosetti I've updated this line as per your comment. @mpg I'll include you point on test coverage in a separate commit in response to the test section.
Should now be updated.
So, if I use this config the tests never get run. I've had a dig around and PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_BASIC seems to be the likely candidate. So I've updated to this, lets see what the CI does and let me know what you think to this choice?
@mpg copying you comment here so we can reply in a thread and it's easier to track:
On second thought, shouldn't we just ensure that ctx->bits is always set for all keys, as we do for psa_type in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/532? I don't really like struct having fields that are sometimes set sometimes not depending on context and you have to remember when - sure it's documented, but how often do we forget to update the documentation when we change the code?
Not sure what you're after here? Is it a general architectural question on whether we should be checking somewhere else whether bits != 0 or are you after the above (recently added solution) or something else?
@mpg again adding your comments as a thread to collate them better:
In addition to the line comments I left, I'm not convinced by the test plan. There are currently 5 ways to populate a PK context:
We want each of them to be appropriately tested (both keypairs and public keys (except for (1)), both RSA and ECC, each with 2 different key sizes).
I'm satisfied that the first case is well covered by the improvements you made to pk_psa_utils().
With minor adaptations (see line comment) this function can probably also cover (2) and (3). On second thought, I'm not sure that would be right: pk_psa_utils() is currently focused on wrapping, so adding unrelated stuff just muddies the water. Perhaps having a dedicated testing function would be cleaner. Or re-use pk_copy_from_psa_success()?
However I'm more worried about (4) and (5). Superficially, it looks like it should be the job of the pk_utils() test function, but it calls pk_setup() which constructs the context manually. (Also, it only does keypairs, not public keys.)
So, unless I missed something, (4) and (5) are not covered at all. I'm not sure what's the simplest way to test them at this point. One way would be to add a bitlen parameter to the existing parsing functions in test_suite_pkparse, but that would probably mean a lot of lines changed. Or we could just add a dedicated function in test_suite_pk that takes as input a serialized private or public key (with a parameter indicating which it is), and its expected bitlen, then calls the appropriate parse function, then get_bitlen() and we're done.
Wdyt?
@valeriosetti I think your comment is related to the above, so adding it to the same thread:
One way would be to add a bitlen parameter to the existing parsing functions in test_suite_pkparse, but that would probably mean a lot of lines changed. Or we could just add a dedicated function in test_suite_pk that takes as input a serialized private or public key (with a parameter indicating which it is), and its expected bitlen, then calls the appropriate parse function, then get_bitlen() and we're done.
Adding my personal opinion here. Unless changes to already existing test functions is trivial/simple I prefer to create new test functions for specific purposes. Extending what's already there makes test functions hard to read: lot of input parameters, lot of operations carried out internally... IMO this is not easy to review and maintain.
The worst consequence that I can think of in adding new test functions is that we duplicate some test that's already there. This is not bad though: it's true that testing will take slightly longer, but that's better than missing test coverage or make something hard to read.
Thanks for creating a thread :) Yes, that's a general architectural question: once a
pk_contexthas been populated (by one of the 5 methods mentioned above), isctx->bitsguaranteed to be valid, ie hold the actual bit length of the key?I think as a general principle, it would be nice for the answer to this question to be "yes", rather than "well, depends which function was used to populate your context - if it was X or Y then yes, otherwise you'll have to do Z instead to get the bitlen". That's a general thought about fields in
pk_context.However, for this particular field, since we have a getter function which can be used everywhere (instead of accessing the field), I think it's not that important. So, on second thought, not requesting any change here - especially as this PR is already large enough as it is :)
@mpg gave this the thumb up, so I'll add a function that takes a key(s) and calls the function and getbitlen. Let me know. Hopefully this should complete the required test plan, though let me know if anyone thinks we need anything else.
As discussed, we want to reach a balance between to valid goals: (a) not making tests more difficult to read than they already are, and (b) not stalling PRs looking for perfection.
IMO, here a good solution could be:
mbedtls_pk_copy_from_psa()andmbedtls_pk_copy_public_from_psa()to the existing function that testsmbedtls_pk_wrap_psa()- IMO this is a natural grouping as those are the 3 ways to populate a PK context from a PSA key. This is the approach used in this PR so far, that we can keep and just slightly expand to test bothcopyandcopy_public.parse_keyandparse_public_keywith a parameter indicating which one it is.Note that if this works this is probably a general pattern for testing something with a PK context:
Wdyt @bjwtaylor @valeriosetti ?
Note: in principle, it is desirable for tests that parse key to parse them from a string in the .data file, as opposed to a file in the filesystem, in order to maintain coverage in configurations without filesystem support (
MBEDTLS_FS_IOdisabled).However, in the spirit of balance between quality and speed, I'm happy for the parsing tests in this PR (and others in the same series) to use files as inputs, as I think that's both easier to write and to review (not having to document and check long strings of hex).
I think both approaches have their pro's and con's, so I don't have a strong preference for either. So if @mpg and @valeriosetti you can agree on one or the other I'm happy with either.
Also I've rocket emoji'd the original threads from this, as the discussion has been now moved here.
I'm sure there is some rework still to go, but at present this PR saves 36B of code size on M55 🙂
So, I believe I've now implemented the requested test plan as per the below verification matrix
<html>Let me know what people think.
Oops, I was about to ask for the very same change that @mpg suggested here when I saw it was already discussed. To me it would make sense as it aligns with what is being done in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/577#discussion_r2597951282.
Since we're going to test
mbedtls_pk_get_bitlen()with all the possible ways of generating a PK context nowadays, we should be granted that we're not forgetting anything.I'm deeply sorry to ask for this change since I was the one who proposed the patch for it initially, but the comment I linked above made me realize that this is indeed the right path to follow.
However if we think that this PR is becoming too large, we can address this with a small follow-up PR specifically targeted for this change.
I'm super sorry, I just realized that I had this unsent message that was pending. I don´t know why it wasn't sent.
I think it's mostly directed to @mpg because the goal is to find a "shared design pattern" in PK that should be used in all the function.
Anyway in case @mpg agrees with my point, the following patch should be enough to make
tests_suite_pktesting happy again:But please wait for @mpg feedback before trying to integrate this. Meanwhile I will take a look at tests
The newly introduced test function is pretty compact, but it covers most of the cases. The only missing one is the PK context generated with
mbedtls_pk_wrap_psafor which you linked tests to be here but it seems to me that that function is testingcheck_pairand notget_bitlen. Am I missing something?@ -8,6 +8,22 @@ PK write valid parametersdepends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEYSince we're not going to perform any operation why do we need to ask for ECDSA? Isn't this enough:
The same holds also for the other EC test below
Since we're importing a key pair I think that
would be better. Wdyt?
The same holds also for the other RSA test below
@ -964,6 +965,61 @@ void valid_parameters()}Invert the order:
The reason is that
PSA_INITcan fail and if this happen you are going to free resources withmbedtls_pk_freethat were never initialized.Wait, it seems to me that here you are initializing
pktwice in a row (pk_setup+mbedtls_pk_parse_key). I'm a bit surprised that this actually works since the 2nd call should definitely fail becausepkis already initialized. @mpg what am I missing here?I think this should be
because otherwise it's the same as in line 992
You don't need
pub_key_id(i.e. a PSA public key) to callmbedtls_pk_copy_public_from_psa. You can usepk.priv_iddirectly and it will work the same. I think you can just removepub_key_idfrom this function.@ -967,0 +1005,4 @@TEST_EQUAL(mbedtls_pk_get_psa_attributes(&pk, PSA_KEY_USAGE_SIGN_HASH,&wrapped_key_attr), 0);TEST_EQUAL(mbedtls_pk_import_into_psa(&pk, &wrapped_key_attr, &wrapped_key_id), 0);Wait, I think here you re-initializing
pk_pub(previous initialization is on line 994 withmbedtls_pk_parse_public_key) withoutmbedtls_pk_freein between. This is fine for now becauseinitis simply going to clear the PK structure and we're not leaking open keys in PSA, but if in the future we want to have PSA key IDs for public keys in PK in the same way we're doing with key pairs then this code would become problematic I think.This is a relatively small change, so if we think we need it we can add it to this PR. Not sure if this is what @mpg is also after, though if he agrees this solves the issue we can add this.
@ -964,6 +965,61 @@ void valid_parameters()}Good point, should now be corrected.
@ -964,6 +965,61 @@ void valid_parameters()}As for why it works, that's one for @mpg. I have however removed the setup though, so at least the duplication isn't present any more and we can hopefully close this one?
@ -964,6 +965,61 @@ void valid_parameters()}Yes, good spot this is a copy paste error. Should now be corrected.
@ -967,0 +1005,4 @@TEST_EQUAL(mbedtls_pk_get_psa_attributes(&pk, PSA_KEY_USAGE_SIGN_HASH,&wrapped_key_attr), 0);TEST_EQUAL(mbedtls_pk_import_into_psa(&pk, &wrapped_key_attr, &wrapped_key_id), 0);So I have added the additional free, as I agree this should be required. I guess we also have a general architectural question for @mpg about whether this use of these functions should currently or in the future behave differently?
@ -964,6 +965,61 @@ void valid_parameters()}Thanks for pointing this out, I assumed you would need a separate ID to store the public key. Though I've removed it and everything still works, so I think the change should work.
@ -8,6 +8,22 @@ PK write valid parametersdepends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEYShould now be updated.
@ -8,6 +8,22 @@ PK write valid parametersdepends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEYAgree, should now be updated.
@ -967,0 +1016,4 @@mbedtls_pk_free(&pk);mbedtls_pk_free(&pk_pub);mbedtls_pk_free(&new_pk);PSA_DONE();@valeriosetti, good spot on the wrap. I'm not sure how it got missed, probably in the pre-christmas rush to get something out before everyone went on holiday. I've added the test here now though. I'll close the previous comment and update the verification matrix. Then lets see what the CI thinks on this change.
@ -967,0 +1016,4 @@mbedtls_pk_free(&pk);mbedtls_pk_free(&pk_pub);mbedtls_pk_free(&new_pk);PSA_DONE();Looks like the CI is happy, so hopefully this is good to go.
Mostly look good to me. I only left one possible improvement to the test function.
There is however one unresolved comment that is not addressed and I think it needs @mpg feedback.
@ -967,0 +994,4 @@TEST_ASSERT(mbedtls_pk_get_bitlen(&pk) == bitlen);TEST_ASSERT(mbedtls_pk_parse_public_key(&pk_pub, pub_key->x, pub_key->len) == 0);TEST_ASSERT(mbedtls_pk_get_bitlen(&pk_pub) == bitlen);I think you can simplify this now:
pk_typeandcurve_or_keybitsare overkilling, you just need to know the bit size to use as check below. So why not just using that as input parameter and getting rid of this block?Yes, this patch looks like exactly what I was suggesting, and I still think we should go in this direction. I wouldn't block this PR on it, but the patch looks small enough, so @bjwtaylor if you're still happy to integrate this here, that would also be my preference.
@ -967,0 +994,4 @@TEST_ASSERT(mbedtls_pk_get_bitlen(&pk) == bitlen);TEST_ASSERT(mbedtls_pk_parse_public_key(&pk_pub, pub_key->x, pub_key->len) == 0);TEST_ASSERT(mbedtls_pk_get_bitlen(&pk_pub) == bitlen);So the aim of the current architecture is to move the intelligence of how the keybits is calculated into the function, to reduce maintenance. Yes we could add another parameter to the function, though this would then mean every time we need to add a new call in the data file you would need to add this to it. We'd also need to change each one individually if we needed to refactor anything. So I think the extra complexity is justified, though happy to change it if you feel strongly that it's not worth it. What do you think?
@ -967,0 +994,4 @@TEST_ASSERT(mbedtls_pk_get_bitlen(&pk) == bitlen);TEST_ASSERT(mbedtls_pk_parse_public_key(&pk_pub, pub_key->x, pub_key->len) == 0);TEST_ASSERT(mbedtls_pk_get_bitlen(&pk_pub) == bitlen);Actually my proposal was to remove
pk_typeandcurve_or_keybitsand replace them withbitsizeparameter, so it's not adding a new parameter, but reducing them. In any case, whenever you add a new line to the data file using this function, you need to specify either the bitsize (my solution) orcurve_or_keybits(your solution), because in both cases you need to have a "known good value" for comparison.Anyway I don´t have a strong preference, both solution work just fine IMO, so feel free to ignore this comment ;)
So, some of this was already in. Though I've also now added the bits that weren't.
I'm currently checking the latest version of this PR, but I don't see the changes to
mbedtls_pk_wrap_psathat I made in my patch above. Weren't they necessary to pass tests?Apologies should now be there.
So, I think the bits variable of the patch needs to be of type size_t to prevent a failure in the windows compiler. I've pushed the change let's see what the CI thinks and then we can decide what we think to this change.
Given the prototype of
psa_get_key_bitsis:static size_t psa_get_key_bits(const psa_key_attributes_t *attributes);I think your change is fine. Sorry for the mistake in the patch.
Looks pretty good to me, just two nits in the test function.
I like how tests are structured. In the data file it's easy to see which key types & sizes are tested, and in the test function it's easy to see we're testing all 5 methods of populating a context. This makes the (positive part of the) test matrix both complete and, perhaps more importantly easy to understand and extend in the future (eg when we add ML-DSA support). Thank you for that.
Just one point though: is there any place where we're testing that
mbedtls_pk_get_bitlen(&pk)on a context that was just initialised (pk_init()) but never populated successfully returns 0? I couldn't find it. If there is non, I suggest adding one invalid_parameters()(just callpk_get_bitlenright after the call topk_init()).@ -964,6 +965,61 @@ void valid_parameters()}This function looks like it is common to RSA and ECC, should it really depend on
PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_BASIC? I think the whole function shouldn't, on only the ECC-specific parts within the function should be guarded with#if defined(...). (And the specific test cases that use ECC need the appropriate dependencies, but that's already the case.)All calls to
_init()that have a corresponding_free()call in the exit block need to go before the first possible failure. Otherwise we might jump toexit:and call_free()on uninitialised data, which may result in all manner of unpleasant outcomes. It's not a huge deal since it's test code, but we should still demonstrate good practices.[rant]I've been advocating for years that every structure should come with an
_INITmacro so that we can always dombedtls_pk_context pk = MBEDTLS_PK_INIT;and not have to think. We keep placing the_initcall too late in test code, and if we make this mistake in test code, then it's likely some users are making the same mistake in places where it matters much more than test code. IMO initialisation during declaration is a much more robust pattern, and I think most PSA types allows it, so only the legacy types from PK, X.509 and TLS are missing it. Guess it's just never really the time to do it...[/rant]@mpg in response to your question on the freshly initialised context bitlen=0, if I've understood what you;re after correctly we do that here
github.com/bjwtaylor/TF-PSA-Crypto@acde1fd416/tests/suites/test_suite_pk.function (L914)@ -964,6 +965,61 @@ void valid_parameters()}I've updated this to remove the dependency on PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_BASIC, from what I can tell the dependencies on the individual test cases should be sufficient to cover it. Let me know what you think and we will see if the CI agrees.
@ -964,6 +965,61 @@ void valid_parameters()}I've moved them above the first test, hopefully this resolves the issue. Though let me know if you were after something else.
@ -964,6 +965,61 @@ void valid_parameters()}I strongly suspect those two lines
need to be guarded with something to ensure
mbedtls_ecc_group_to_psa()exists in this build. That might be whydepends.pyfailed (haven't checked yet).@ -964,6 +965,61 @@ void valid_parameters()}Bingo! In
depends.py !PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_BASICwhen compiling this file:This function's declaration is guarded by
#if defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY)so we should use the same guard when using it.At
e44f2ed5ccLGTM except for the CI failure independs.py.I checked and the other CI failure, in the memsan component, looks like a flaky test and unrelated to this PR, so it can be ignored (unless/until it repeats).
Ah, I looked there and I missed it as I my attention was drawn to the line above where we pass NULL. Yep, you've understood what I'm after correctly, I had just missed it :)
At
7d20c6b352LGTM, hoping the CI agrees.