Add mbedtls_pk_get_key_type() #532
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!532
Loading…
Reference in a new issue
No description provided.
Delete branch "pk-get-key-type-function"
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?
Introduce a new function,
mbedtls_pk_get_key_type(), to get the PSA key type of a PK context.Note: Currently this usesmbedtls_pk_type_tandmbedtls_pk_get_type()internally, but I intend to refactor it to avoid using this soon.Fixes #509
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.
Rebased and conflicts resolved, this should be ready for review!
Just a general comment as this is the first time I'm looking at this PR since the approach changed: the fact that you've had to add calls to
mbedtls_pk_set_psa_type_from_private()in so many places inpkparse.cmakes me a bit nervous: how do we know we haven't forgotten one, or that we won't forget it when adding a new code path in the future (which we probably will some time in 2026 for ML-DSA)?Hopefully the tests should catch this, but that just move the problem to a different place: how do we know we're testing all cases, and that we will keep doing so in the future?
I wonder if we could structure the code differently to mitigate these risks. I'm thinking of two different approaches:
pk_rsa.candpk_ecc.cthat would be the single point (for each key type) wherepriv_idgets populated while parsing a key, and setpsa_typethere? (That is, the way you did it for public keys.)_internaland make the public function a wrapper that just calls the internal function thenmbedtls_pk_set_psa_type_from_private().Wdyt?
I'm happy with either of those two approaches, but the approach from the top seems to be the more robust.
If we were to do the first (from the bottom) approach and then add ML-DSA, we'd need to add a new call to
mbedtls_pk_set_psa_type_from_private()for the new key type, which we might miss.I think the 'right' thing to do here would be to see if
mbedtls_pk_parse_key()can be restructured so that it's easier to do post-parsing actions (and to make it easier to follow the control flow, as it's quite difficult to read at the moment). However, this would take too much time and I'm sure there would be some complications to it.Actually I would be in favor of an approach "from the bottom". Unless I missed something, in the end
mbedtls_pk_[ecc|rsa]_set_keyis always called for setting the key frommbedtls_pk_parse_key. Can't we set thepsa_typethere?This would be more uniform with what is done in case of public keys.
With this solution you can also remove change in
copy_from_psabecause that one callsmbedtls_pk_[ecc|rsa]_set_keyas well.mbedtls_pk_[ecc|rsa]_set_keyare the only point where they key pair is set, so settingpsa_typeshould guarantee that we're not missing any corner case.Agree
If we keep this solution (i.e. we don't opt for the "from bottom" approach), no
psa_reset_key_attributesbefore returning?@ -2164,3 +2164,243 @@ exit:PSA_DONE();Generating keys at every iteration might slow down test case. I think that
pk_psa_setup()can be helpful here (and also inpk_get_key_type_copy_from_psa_keypair,pk_get_key_type_copy_from_psa_publicandpk_get_key_type_copy_public_from_psa). It uses predefined keys so you don't need to generate them every time. This also relaxes requirements for the test function as key generation is no more required (i.e. no need forPSA_WANT_KEY_TYPE_[ECC|RSA]_KEY_PAIR_GENERATE).@ -403,6 +408,21 @@ int mbedtls_pk_can_do_psa(const mbedtls_pk_context *pk, psa_algorithm_t alg,int mbedtls_pk_get_psa_attributes(const mbedtls_pk_context *pk,psa_key_usage_t usage,Should we mention that the context must have been at least initialized first?
In the last 2 commits I have fixed a couple of very small size calculation issues that I noticed when fixing CI failures.
This is a little cheeky of me as they are technically unrelated to this PR, so I will happily remove those commits if requested.
Just reviewing the last two commits. Will review the rest as my next step.
@ -750,1 +746,3 @@* - 0 to 1 bytes of leading 0 due to the sign bit.* is at most 3 bytes. The length of the encoding is overestimated as follows:** - Take int(bits / 8) as the number of full bytes taken by the value.Nit: I'm more used to ceil(x/8) = (x + 7) / 8. Of course, both are equivalent, unless x is near the end of the type's range: that is, your formula may have a problem for
x == 0while mine may have a problem whenx > UINT_MAX - 7. Here we already assumebits <= 2^19 - 9(see above) and in general I think we already assumeUINT_MAX >= 2^32 -1so we should be good. OTOH,x == 0seems a bit unlikely but not explicitly ruled out by existing comments. Wdyt?I think the previous formula was correct, for reasons that are not obvious from the comment:
bits / 8is correct;bits / 8underestimates by 1, but OTOH we know for sure we won't need a leading 0 (because the top bit of the top byte is unset), so the+5uat the end overestimates by 1, which exactly compensates for the underestimation in the division by 8.If you agree with this reasoning, I think we can restore the previous formula, but update the comment because we really shouldn't leave such things to the reader.
For reference, in general when we notice and fix an issue unrelated to the original goal of the PR, we should ask ourselves:
This is especially relevant when the original goal of the PR is something that's development-only and/or doesn't require a ChangeLog.
Here, after reviewing the changes, I think the only issues were in the comments, so the answer to (2) would be no, and regarding (1) I'm not sure but I'd say yes (especially the 2nd one which looks really suspicious and had me a bit worried for a moment).
I think last time I asked Gilles, he said calling
reset_key_attributes()is good practice but it's not an actual bug if we don't call it.Intuitively, I think I had a preference for the approach "from the bottom", for the same reasons as Valerio. But you have a point that it means when we add ML-DSA we have to remember to set
psa_typein the equivalent "bottom" function - or equivalently, we have to remember adding a test that callspk_get_key_type()on a parsed ML-DSA key.Well, if I may answer my own question: what if we take the existing test functions for parsing and add a mandatory parameter for "expected key type" (or just a check if the function is already specialised for one type)? I mean, presumably (hopefully!) we already have tests covering all cases that can happen during parsing, so we can just take advantage of that. Also, when we add support for parsing ML-DSA keys, surely we'll want to add test cases using
pk_parse_key()and if this has a parameterexpected_key_type(that it checks against callingpk_get_type()on the parsed key) then we can't forget to provide it.Wdyt?
Alternative approach:
psa_typeis only set for public keys, but for private keyspk_get_type()always retrieves it frompriv_id.Dream: we store all keys (public or private) in
priv_id, then we don't need a separatepsa_typefield. I'm explicitly not suggesting we make that change in this PR though :) But I think once we've progressed enough on the cleanups planned for this quarter, that's something I'd like to consider (should be easier to do by then).@ -750,1 +746,3 @@* - 0 to 1 bytes of leading 0 due to the sign bit.* is at most 3 bytes. The length of the encoding is overestimated as follows:** - Take int(bits / 8) as the number of full bytes taken by the value.Yes, that sounds correct to me, I'll update it.
That would work well, I think! Should I change the name of the new field to something like
pk->public_psa_typein that case?Yes, I think that would be a better name if we go with this approach!
I confirm. Since 3.6.0, there are no cases where
psa_reset_key_attributes()does anything more thanmemset(0).I don't think we need even that. I would suggest, rather, that every parsing function should call a test function
pk_exercise_key(), and that function should make some consistency checks which include validating thepsa_typefield against the content (and complains if it sees a type or a content format that it doesn't know about).I think we should always have
psa_type. It's a straightfoward way to know what the context contains. It costs very little memory and it avoids having to make a client-server call each time we want the type when PSA is client-server. Even when PSA is local, it avoids having to allocate an attribute structure for a call topsa_get_key_attributes.psa_typeshould be 0 only for a blank context.I like that approach. Then actually every test function that's about a key creation method (not just parsing, also wrapping or copying from psa) can call this "consistency checks" function, which also serves as executable documentation for the invariants of a populated
pk_contextstructure.However, this might be a bit more work and I'd like to avoid delaying this PR, as there are other tasks that depend on it. So here's my suggestion:
psa_type, using the approach "from the bottom" favoured by myself and Valerio, but don't expand the testing beyond what's already in this PR (ie, we're confident this is correct now, but might not be fully confident this will remain correct when we add new key types).test_suite_pk.functionand use it in all existing test functions that are about parsing, or other methods of key creation (ie, future-proof our testing).@ -750,1 +746,3 @@* - 0 to 1 bytes of leading 0 due to the sign bit.* is at most 3 bytes. The length of the encoding is overestimated as follows:** - Take int(bits / 8) as the number of full bytes taken by the value.Nullified by other changes (the old formula was correct).
In the end, we did not, so resolving this.
I believe I have addressed everything here, it should be ready for another round of review.
One thing I am unsure of the approach on is this: The function
mbedtls_pk_ecc_set_pubkey()is used in 2 situations:pk->psa_typetoPSA_KEY_TYPE_ECC_PUBLIC_KEY(family)pk->psa_typeas it is, since it is set already bymbedtls_pk_ecc_set_key()toPSA_KEY_TYPE_ECC_KEY_PAIR(family)I have solved this by adding a boolean parameter
is_keypairtombedtls_pk_ecc_set_pubkey()(allowed as it is internal-only) but reviewers may disagree as to whether this is a suitably neat solution.A part from really 2 minor suggestions, I'm not super happy about the
is_keypairparameter inmbedtls_pk_ecc_set_pubkeyso I proposed an alternative solution (through a patch). Let me know what you think about it?@ -163,6 +165,16 @@ int mbedtls_pk_ecc_set_pubkey(mbedtls_pk_context *pk, const unsigned char *pub,return MBEDTLS_ERR_PK_INVALID_PUBKEY;I'm not a huge fan of the addition of the
is_keypairparameter. It caused changes in drivers (ex: pkparse) and test code and IMO it's not strictly necessary. Similar scenarios are already handled in other functions, for example:mbedtls_pk_ecc_set_groupherembedtls_pk_rsa_set_keyhereI know that in this case
mbedtls_pk_ecc_set_keyset the key-pair key type whilembedtls_pk_ecc_set_pubkeytries to set the public key type and that the 2 are not the same, but wouldn't this patch pk_ecc.c.patch do the same without extrais_keypairparameter?@ -134,6 +134,11 @@ typedef struct mbedtls_pk_context {/* Public key information. */Perhaps it's just me not reading it properly, but the way it's written now seem to suggest that there are other possibilities a part from public keys to get a PK context which is not backed by a PSA key, but this is not correct.
I would have rephrased it as:
This is really minor though and perhaps I overthought about it, so if you think this is not important feel free to ignore it :)
While reading about this fix, I checked also the line below and it says
7 bytes for the public exponent, but then above it'spublicExponent INTEGER, -- 32-bit. The two don't match IMO. Wdyt?@ -163,6 +165,16 @@ int mbedtls_pk_ecc_set_pubkey(mbedtls_pk_context *pk, const unsigned char *pub,return MBEDTLS_ERR_PK_INVALID_PUBKEY;On a second thought another thing that seems suspect to me is: why doesn't RSA keys need this fix?
@ -163,6 +165,16 @@ int mbedtls_pk_ecc_set_pubkey(mbedtls_pk_context *pk, const unsigned char *pub,return MBEDTLS_ERR_PK_INVALID_PUBKEY;I think it's because in RSA the keys are encoded differently: the private key encoding (n, e, d, p, q, dp, dq, qp) is naturally a superset of the public key encoding (n, e). While with ECC the private key can be encoded just on its own (d) and optionally the public part (Q) can be encoded, or it will be recomputed from the private key (Q = d * G). So for private keys we just parse d, and only then set Q as a 2nd step.
Not sure if that makes sense?
I believe this is correct because the 32-bit Integer is encoded in ASN.1 format, so we have:
@ -134,6 +134,11 @@ typedef struct mbedtls_pk_context {/* Public key information. */I've reworded it with a comma for extra clarity 🙂
Oh good point! Thanks!
It's almost OK to me. I only have 1 question about a dependency in some tests. Once this is resolved I will approve.
@ -1497,0 +1525,4 @@PK get type from parsed key, RSA, DERdepends_on:PSA_WANT_KEY_TYPE_RSA_KEY_PAIR_BASICpk_get_key_type_parse_key:"../framework/data_files/server1.key.der":PSA_KEY_TYPE_RSA_KEY_PAIRInitially I thought this was a duplicate of the test on line 1510, but then I realized that this uses files in DER format whereas the other is in PEM format. So now the question is instead: don't we need
MBEDTLS_PEM_PARSE_Cdependency on tests using PEM format? And also: why CI is not failing for this?Thanks for your patience in addressing all my comments. LGTM now!
LGTM, thanks!