Inline check_pair functions #580
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!580
Loading…
Reference in a new issue
No description provided.
Delete branch "inline-check_pair"
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
Inline check_pair functions resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/521
PR checklist
52B code size reduction, thanks!
@ -1070,2 +1049,2 @@TEST_EQUAL(mbedtls_pk_check_pair(&pub, &prv), MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE);}TEST_EQUAL(mbedtls_pk_wrap_psa(&prv, wrapped_key_id), 0);TEST_EQUAL(mbedtls_pk_check_pair(&pub, &prv), expected_ret);So, this was because the previous implementation was able to check the function pointer to check_pair in the structure to see if it was supported. With the new architecture this isn't available, so it progresses further before failing with a different error. You make a good point though, so I've added additional logic to return the same error as before.
Yes, this is a good point. I've removed the type check.
I've replaced these calls with the new mbedtls_pk_get_key_type function.
@ -1070,2 +1049,2 @@TEST_EQUAL(mbedtls_pk_check_pair(&pub, &prv), MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE);}TEST_EQUAL(mbedtls_pk_wrap_psa(&prv, wrapped_key_id), 0);TEST_EQUAL(mbedtls_pk_check_pair(&pub, &prv), expected_ret);My expectation is that with the new implementation it should progress further to success, not to a different error.
I don't care a lot about adding support for wrapped RSA keys, I only suggested it because I though it would be easy. So I'm OK if we keep erroring out on wrapped RSA keys.
OTOH, I do care quite a lot about understanding how our code works, and so far I have no idea why wrapped RSA is giving this error. I wouldn't like us to merge this PR without understanding that.
To re-iterate, deciding that we don't add support for RSA keys after all is OK with me, but making that decision without fully understanding why the obvious implementation doesn't work would not make me happy.
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)Why do we need to do that? We should already have the serialized public key of the
prvcontext inprv->pub_rawand be able to using without messing with export or anything. Why are you not using the implementation suggested by Gilles?The plan was that Gilles's suggestion (turned form pseudo-code to actual code, see following comments) would be the entire implementation of the function, ie right after the initial NULL checks we'd directly have
return <the expression in Gilles's> comment(or something equivalent).Again, I don't mind deviating from the original plan, but if we do, I want to understand why.
@ -1070,2 +1049,2 @@TEST_EQUAL(mbedtls_pk_check_pair(&pub, &prv), MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE);}TEST_EQUAL(mbedtls_pk_wrap_psa(&prv, wrapped_key_id), 0);TEST_EQUAL(mbedtls_pk_check_pair(&pub, &prv), expected_ret);So, this comment was prior to the decision to enable wrapped RSA keys. I think these should now work, though I'm still testing and developing the latest version.
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)Not sure, I'm implementing it as per above now though.
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)Ok, think I might have come across the reason. So for the current plan to work we assume that the the supplied context already has the exported public key supplied, so the caller has to call something like mbedtls_pk_parse_keyfile() and then mbedtls_pk_set_pubkey_from_prv(). However in the original implementation I was assuming that this was not the case and allowing for use case where mbedtls_pk_set_pubkey_from_prv() had not been called. In this case the pubkey context is blank and therefore we can't check it. Admittedly it would be better to call mbedtls_pk_set_pubkey_from_prv(), rather than export and calloc. So this is a fair point. @gilles-peskine-arm whilst Manuel is on holiday, am I safe to assume the public key has been exported into the context? Or do I need to export it prior to attempting the check pair functionality?
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)I haven't followed the evolution of pk last quarter, but I believe that was part the goal: to make the handling of different cases uniform, including keeping the public key in the context in all cases. (Why would we waste
MBEDTLS_PK_MAX_PUBKEY_RAW_LENbytes in the pk context if it's not to use them?)@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)So, I've probably been following it closer than you have. Though unfortunately this one only came up when Manuel mentioned it. So the issue is that all the the check_pair functions currently don't assume the public key is exported prior to being called. So if you call them after the context is populated with the private key using a function like mbedtls_pk_parse_keyfile() they will still succeed, as they will just export the public key from the private one. In the implementation suggested we are assuming the key has been exported and therefore don't bother to export it again. I think this is sensible, as you say why would we waste the bytes? Though the issue is this is a change to the API of check_pair and might break things, if other parts of the system/existing users are assuming you don't need to populate the public key for this to work. A basic run of the test suite seems to succeed with this change, so we might be ok. It's difficult to know for sure though. What do you think, is this change ok?
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)The
check_pairfunction assumes a valid pk context (if one is in unpopulated it will reliably return an error). It's not something that gets called internally while constructing a context. Is there a way for a valid, populated pk context containing a key pair not to have thepub_rawfield filled? AFAIK there isn't: all ways of populating a pk context with a private key callmbedtls_pk_set_pubkey_from_prv.You can search the code of
pk*.cto see if pk callscheck_pairinternally. It doesn't. Any other code can't populate the PK object itself, it has to rely on one of the public functions inpk.h. And AFAICT they do. The documentation of thepub_rawfield doesn't mention any restriction.So yes, please change
check_pairto the simple implementation where it assumes that thepub_rawfield is populated. If there's a case where it doesn't work and we have adequate test coverage (which I haven't checked), that would have to be an edge case that we haven't thought of testing, and the fix would be to populatepub_rawin that edge case.@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)OK, sounds good. I'll update it assuming that it is populated and return an error if it's not.
Apologies for the force push, however it was necessary to add a missed DCO.
Updated as per Manuel's test plan, Test report as follows:
<html> <head> </head>@gilles-peskine-arm, so I tried removing the export from check pair as discussed and it appears to be a "feature" required by the sample programs here:
github.com/bjwtaylor/mbedtls@069cfbd43c/programs/ssl/ssl_server2.c (L1170). Maybe one to discuss at stand up, for now I have added an improved version of the export. Maybe it might be better to change the functionality of the sample programs though the problem is how many people will have copied that implementation and then this would be a breaking change for them?@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)To sum up & confirm what has been said: we are moving towards more uniformity in PK. Previously
pub_rawwas not always populated by public functions that populate a context. Since some time last quarter, it always is (if it isn't, that's something we should fix).The current implementations of
check_pair()date back from a time before this was guaranteed. So they can't rely on that assumption and must useexport(). But the new implementation fortunately comes at a time where a lot more things are uniformly guaranteed, and can rely on that.Nope,
mbedtls_pk_parse_keyfile()should already be callingmbedtls_pk_set_pubkey_from_prv(), it it's not, we should fix it. (Anyway,set_pubkey_from_prv()is not a public function, users can't call it, it's out job to call it at the right time(s).)There are currently 5 ways to populate a PK context:
mbedtls_pk_wrap_psa(),mbedtls_copy{,_public}_from_psa()andmbedtls_pk_parse{,_public}_key(plus the obvious "file" variants of the two parse functions). The theory/goal is that all those 5 ways should result in a PK context where all fields (pub_raw,psa_type,bits) are set and valid, so that the rest of the code can use them without worrying what kind of context this is or how it was populated (the only exception beingpriv_idwhich is currently only set for private keys aka keypairs).I think we are already there, but if I'm wrong and we're not, the proper fix is to get there, not work around the fact that we're not there yet.
@bjwtaylor
ssl_server2is a test program, not a sample program. It may be taking liberties with the API. If you follow the API, all the ways to construct a pk object should fill in the public key field, or if they don't we should fix that, maybe not in this PR but as part of the ongoing pk cleanup.What's failing here is specifically the tests of
MBEDTLS_SSL_ASYNC_PRIVATE. That test code might plausibly do bad things with pk contexts that used to be ok, but no longer are. To summarize this feature, it's a way for a TLS server to have its private key in some external server (predating PSA secure elements). So when it's in use, it's plausible that there would be a pk context that normally contains a private key, but actually doesn't. It's weird that this would cause a problem incheck_pairthough, and by looking quickly at the source code I can't see an obvious problem.Would you mind looking at the failure in a debugger and seeing where precisely
ssl_server2is doing something bad? (I.e. look deeper than just where it notices something bad.) I wrote that feature so I might be able to help, but it's been years so I don't remember much other than the general design.IMO in this PR, because otherwise we need to work around it and it makes this PR more complicated than it should be anyway. (Also, I just remembered why I think it's already done: the implementation of
mbedtls_pk_write_pubkey_psa()relies on the fact thatpub_rawis populated, and that function is properly tested with all 5 public ways of populating a PK context, so really we should be fine already.)I was going to ask the same: how exactly is the context created? I strongly suspect it's not with one of the 5 public APIs mentioned above, but rather by directly calling
mbedtls_pk_setup()(now an internal function) or perhaps even directly messing with the private fields of the PK context structure). We should either change the code to use only public functions if that can easily be done, or otherwise if the code is already using private stuff, then it may as well call one more internal function, namelymbedtls_pk_set_pubkey_from_prv(), so that the context it populates in unsanctioned ways still satisfies the invariants of context populated in one of the official ways.That's the thing: it's neither accessing
pk_ctx(which doesn't even exist anymore) nor callingmbedtls_pk_setup(not called at all in mbedtls). I also can't find anything like a shallow copy. There may be places where more than one piece of the code have a pointer to the same context, and this could cause one piece to callmbedtls_pk_freewhen another piece still needs the context, but (1) I don't see how it could cause a problem now that didn't cause a problem before, and (2) AFAICT that potential weirdness only happens with SNI and even non-SNI test cases are failing.@mpg I like this suggestion. I'll investigate further, as to how the program functions and how it ends up with a partially populated context. Though I suspect to fix the underlying issue maybe a more work than we want to do and is arguably outside of the scope of this issue. If this program is just for test, not intended as an example and is already using the API in unsanctioned ways. Then adding one more unsanctioned API for now and logging another issue might be the best way forward if @gilles-peskine-arm you're happy with that?
So, on further investigation it looks like it's using mbedtls_pk_parse_keyfile() here
github.com/bjwtaylor/mbedtls@069cfbd43c/programs/ssl/ssl_server2.c (L2615)to populate the privkey. Which I know doesn't populate the pubkey and I guess is now not public? I think I copied this code and was using the same function in the test, thinking this was public :( So I was planning to add a mbedtls_pk_set_pubkey_from_prv() here and will do the same in the test progs. Let me know if anyone has objections though.Even if
mbedtls_pk_parse_keyfileis not in the public API, I would expect it to be just load file +mbedtls_pk_parse_key. And in fact, that is exactly what the function does. So how do we end up with a malformed pk object aftermbedtls_pk_parse_keyfile?That's a good question, I know that call ends up with a malformed PK object. Though I've not dug into why, I shall investigate. Interestingly mbedtls_pk_parse_keyfile is in pk.h, so presumably this means it's public? So adding an additional mbedtls_pk_set_pubkey_from_prv(), is not as easy as I though as I'd need to hack the include path, so it can access pk_internal.h. Which I don't like, let me have another look if the issue is with the mbedtls_pk_parse_keyfile() it might be better to fix it.
Yes,
mbedtls_pk_parse_keyfileis in fact public (we talked about making it private, but evidently we didn't end up doing it).It's definitely a problem if
mbedtls_pk_parse_keyfileconstructs a malformed key. But I doubt that this is the problem, because a lot of our test code uses that function.I suspect that the code of
ssl_server2is doing something unusual aftermbedtls_pk_parse_keyfile, and based on which test cases fail, it's evidently in code guarded byMBEDTLS_SSL_ASYNC_PRIVATE. But so far I can't see what.I agree, it's weird that only the async cases fail. I'll look into it and report back.
So it looks like there could be a corner case where the public key is not exported if you are using pem encoded keys in a file. I've added a fix, which seems to resolve it locally. Let's see what the CI thinks though.
So, it looks like the fix worked overnight. Let me know what people think to it? I think at best it's a temporary fix, long term we may want to privatise this function and maybe refactor it to avoid duplicate calls. Though I think if so this is out of scope of this issue. I've also added some tidy up.
Looking pretty good! A few minor points, but I think the overall shape is good now.
Ok, I'm surprised the changes here require this. If this is for
memcmp(), I'd rather just includestring.h.@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)In the long run, we want to get rid of
pk_info. I know this is pre-existing, but do we really need to keep this now?Actually, I tried removing this line (keeping only the two lines below based on
psa_type) and all the tests seem to still pass locally. So, I think we should just stop checkingpk_infohere.The documentation says
#PSA_ERROR_INVALID_ARGUMENT if a context is invalid.NowMBEDTLS_ERR_PK_BAD_INPUT_DATAis indeed an alias forPSA_ERROR_INVALID_ARGUMENTbut I would find it a bit more readable to directly writereturn PSA_ERROR_INVALID_ARGUMENThere - that way it's obvious we do what the documentation says, without relying on non-local knowledge of error code aliasing.Minor, pre-existing: the documentation says "another error code if the keys do not match" (right after PSA_ERROR_INVALID_ARGUMENT if a context is invalid), so technically we should return something other than
PSA_ERROR_INVALID_ARGUMENTakaMBEDTLS_ERR_PK_BAD_INPUT_DATAhere.The problem here, I'm not sure what would be more fitting.
MBEDTLS_ERR_PK_TYPE_MISMATCHis perhaps the least bad match, but it's not ideal either.Or perhaps we just change the documentation to match the existing behaviour: s/Another/Some/ non-zero value if the keys do not match?
Minor: I would check the types before checking the contents. Intuitively makes more sense. Also is more consistent with existing behaviour.
@ -964,2 +964,3 @@(ret = mbedtls_pk_rsa_set_key(pk, pem.buf, pem.buflen)) != 0) {(ret = mbedtls_pk_rsa_set_key(pk, pem.buf, pem.buflen)) != 0 ||(ret = mbedtls_pk_set_pubkey_from_prv(pk)) != 0) {mbedtls_pk_free(pk);Nice catch, thanks!
Ok, interesting, so
parse_key()has different code paths for PEM and DER, and was improperly populating the context in some code paths, and this was not caught by existing tests because the only tests that currently rely onpub_rawbeing properly populated are using DER not PEM. Our testing strategy was not good enough and we were lucky thatssl_server2caught it for us.I think we probably want to revisit a previous suggestion by Gilles about testing: create a helper function that checks invariants of a populated PK context (like
psa_typeis not NONE,pub_raw_lenis not 0, etc. perhaps even that the contents ofpub_rawmatch what we get from exporting frompriv_id, etc.) and then add calls to this helper to all existing parsing test functions (or more generally, tests function that are about a method of populating a PK context).This is obviously out of scope here. I'll file a separate issue and might have a go at it myself.
@ -712,15 +711,10 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t sig_type,/**Nope, that's an internal detail, users are not supposed to know what this means and should not be exposed to this. I think the way we want to express this is to add "Must be populated" to both
pubandprvarguments. (Also, we already say above#PSA_ERROR_INVALID_ARGUMENT if a context is invalid.)@ -520,3 +520,3 @@depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:PSA_WANT_ALG_RSA_PKCS1V15_SIGN:MBEDTLS_PEM_PARSE_Cmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_BAD_INPUT_DATAmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_TYPE_MISMATCHMinor: we wouldn't need to change the expected return code here if we kept the existing behaviour of checking the type before we check the content (see my other comment on the implementation).
Nit: I see what you mean but brainpool curves also are over a prime field and have prime order. I think "NIST vs Brainpool" would be better.
@ -1046,3 +1032,3 @@mbedtls_pk_init(&pub);mbedtls_pk_init(&prv);mbedtls_pk_init(&alt);mbedtls_pk_init(&empty_ctx);Minor, pre-existing: I think we're no longer using this
altcontext and could remove it.That change does not look correct to me:
TEST_ASSERT()(or these days,TEST_EQUAL(), but anyway)expected_retis not zero, we want to keep testing with wrapped and check that we get the same (non-zero) return value.I think the code was fine as it was, and this change is probably a leftover from some intermediate state or debugging?
Thanks! For the sake of completeness, can we add the following too?
@ -964,2 +964,3 @@(ret = mbedtls_pk_rsa_set_key(pk, pem.buf, pem.buflen)) != 0) {(ret = mbedtls_pk_rsa_set_key(pk, pem.buf, pem.buflen)) != 0 ||(ret = mbedtls_pk_set_pubkey_from_prv(pk)) != 0) {mbedtls_pk_free(pk);I completely agree, this is probably the tip of the iceberg. There are probably other corner cases that we are not catching in the test. The new tests I added will catch this one as I think I copied the test programs implementation of reading pem files. Though we should probably add more, it's also easier to debug these things if they are caught in the standard tests rather than having to rely on ssl-opt. Also agree it is out of scope here though.
@ -964,2 +964,3 @@(ret = mbedtls_pk_rsa_set_key(pk, pem.buf, pem.buflen)) != 0) {(ret = mbedtls_pk_rsa_set_key(pk, pem.buf, pem.buflen)) != 0 ||(ret = mbedtls_pk_set_pubkey_from_prv(pk)) != 0) {mbedtls_pk_free(pk);https://github.com/Mbed-TLS/mbedtls/issues/10573
@ -712,15 +711,10 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t sig_type,/**OK, I've removed the note. If we don't want to expose the detail the content is largely explained by the note above.
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)OK, I've updated it. Maybe one that's outside the scope of this function, however we now have mbedtls_* functions in PK that are in psa and use a mixture of MBEDTLS* and PSA* error codes. Presumably we should eventually move everything to PSA*? Or do we have another plan?
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)I don't have a strong opinion either way, so I've updated it to MBEDTLS_ERR_PK_TYPE_MISMATCH. As this is the smalles change. Let me know if you think the doc change would be better though.
@ -520,3 +520,3 @@depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:PSA_WANT_ALG_RSA_PKCS1V15_SIGN:MBEDTLS_PEM_PARSE_Cmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_BAD_INPUT_DATAmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_TYPE_MISMATCHI think this is related to the previous comment on the MBEDTLS_ERR_PK_BAD_INPUT_DATA -> MBEDTLS_ERR_PK_TYPE_MISMATCH. As above I've changed this and have also now updated the data file.
@ -1046,3 +1032,3 @@mbedtls_pk_init(&pub);mbedtls_pk_init(&prv);mbedtls_pk_init(&alt);mbedtls_pk_init(&empty_ctx);Should now be removed.
Should now be updated.
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)I've put the change in, also seems to pass locally for me. Though lets see if the CI agrees.
@ -520,3 +520,3 @@depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:PSA_WANT_ALG_RSA_PKCS1V15_SIGN:MBEDTLS_PEM_PARSE_Cmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_BAD_INPUT_DATAmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_TYPE_MISMATCHSo, currently it's using a NIST though it's a prime field one. We could change it to a binary field, however I don't believe we have any in the data_files directory. I could create one, though I would then need another PR on the framework. Let's discuss in the catchup if this is worth it?
@ -1046,3 +1032,3 @@mbedtls_pk_init(&pub);mbedtls_pk_init(&prv);mbedtls_pk_init(&alt);mbedtls_pk_init(&empty_ctx);So, I've basically compacted this function to do multiple tests in one. This has probably reduced test size, though it may of also unacceptably decreased readablity. The idea of this line is that expected ret is used twice in the tests to check the output of multiple check_pair tests. So in some cases we want to check that the call to check_pair on that line and sometimes here
github.com/bjwtaylor/TF-PSA-Crypto@1b4b6a491d/tests/suites/test_suite_pk.function (L1066). If we want to check the second instance then this one will return ) and the test will continue. If we want to check this one, then it will return an error and we want to exit at this point as we are probably using test data that is crafted just to use this case to fail. Let me know what you think, maybe we leave it as is. Or maybe we need two expected_return variables. Or maybe we need to unravel this into a number of simpler functions for readability?@ -520,3 +520,3 @@depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:PSA_WANT_ALG_RSA_PKCS1V15_SIGN:MBEDTLS_PEM_PARSE_Cmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_BAD_INPUT_DATAmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_TYPE_MISMATCHWe don't support binary fields at all. I'm perfectly happy with the test data as it is, I just think "NIST vs Brainpool" would be a better description for it than "prime vs brainpool", considering "prime" is true of all the curves in this test (and actually all the curves we support).
Github won't let me line comment on this, but I'm surprised there's a commit here updating the framework pointer: why is that needed for this PR?
@ -520,3 +520,3 @@depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:PSA_WANT_ALG_RSA_PKCS1V15_SIGN:MBEDTLS_PEM_PARSE_Cmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_BAD_INPUT_DATAmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_TYPE_MISMATCHUnderstood, now corrected.
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)Using a mixture is OK for the time being. But here since the documentation mentions the PSA one, I thought using the same name in the implementation would make correctness more obvious.
@ -520,3 +520,3 @@depends_on:PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY:PSA_WANT_ALG_RSA_PKCS1V15_SIGN:MBEDTLS_PEM_PARSE_Cmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_BAD_INPUT_DATAmbedtls_pk_check_pair:"../framework/data_files/server1.pubkey":"../framework/data_files/server2.key":MBEDTLS_ERR_PK_TYPE_MISMATCHAh sorry for not including a link( github wouldn't let me as the other comment was still pending), but this was related to this comment actually.
@mpg, I suspect the framework pointer update was needed at some point to resolve an issue as the pointer was maybe out of date? Either that or I was trying multiple things to rebase and left this in even though it had no impact. It isn't currently required though, so I can remove it. Though I'd then need to do a force push. As we will probably need to upgrade it at some point anyway and it doesn't cause any issue it it worth it?
@ -1046,3 +1032,3 @@mbedtls_pk_init(&pub);mbedtls_pk_init(&prv);mbedtls_pk_init(&alt);mbedtls_pk_init(&empty_ctx);Ok I see. Yes I think that's a bit too clever for readability. We should either have a comment about it, or perhaps use two parameters
expected_ret1andexpected_ret2.But taking a step back, I don't really understand why we'd expect different results on lines 1053 and 1066. Either the key materials match or they don't, and that should have nothing to do with whether the private context is wrapping or not.
Am I missing something?
@ -1340,22 +1340,29 @@ int mbedtls_pk_sign_ext(mbedtls_pk_sigalg_t pk_type,int mbedtls_pk_check_pair(const mbedtls_pk_context *pub,const mbedtls_pk_context *prv)I've now swapped these over.
Minor: please remove the comment "unsupported functions: check_pair, debug" a few lines above. We're removing
check_pairhere, and debug was removed in another PR but without updating the comment, so let's clean it up here while at it.Also, does this function still need
pk2or was it only used forcheck_pair?Well if it was to resolve an issue, you should know which one. As to rebasing, this PR was not rebased recently, so that seems weird. I can understand that in complex situations sometimes we try a lot of things and don't always remember everything, but here I don't feel like anything particularly complex was going on, just run of the mill rework to address review comments. So it seems weird to end up with unexplained commits in this context.
Generally speaking, let's avoid force pushes unless required. But that's also a reason to avoid updating submodule pointers unless actually required: they tend to cause conflicts that then require force-pushes to resolve.
So, in the future, please do not update submodule pointers unless there is a specific reason - and then be prepared to explain that reason :)
@ -1046,3 +1032,3 @@mbedtls_pk_init(&pub);mbedtls_pk_init(&prv);mbedtls_pk_init(&alt);mbedtls_pk_init(&empty_ctx);I don't think I'm missing anything. I applied the following patch and tests are still passing locally:
This is left over from debug, now removed.
@ -1046,3 +1032,3 @@mbedtls_pk_init(&pub);mbedtls_pk_init(&prv);mbedtls_pk_init(&alt);mbedtls_pk_init(&empty_ctx);Agreed, now removed. Lets see if the CI agrees.
I've removed the comment and the unused context.
I only have 1 question, but a part from this it looks good to me
@ -1346,1 +1346,4 @@pub->pub_raw_len == 0 ||prv->pub_raw_len == 0) {return PSA_ERROR_INVALID_ARGUMENT;}don't we want to also add a check for
pub->pub_raw_len == 0here?I know the function is going to fail anyway because below there is
prv->pub_raw_len != pub->pub_raw_len, but it would return a different error code. IMOPSA_ERROR_INVALID_ARGUMENTwould be more appropriate. Wdyt?@ -1346,1 +1346,4 @@pub->pub_raw_len == 0 ||prv->pub_raw_len == 0) {return PSA_ERROR_INVALID_ARGUMENT;}It shouldn't matter because in theory there is no situation where
pk_info != NULLbutpub_raw_len == 0. However in practice this might still happen (even though we'd consider it a bug).In the long run, we'd like to remove
pk_info, and since we want to keep checking that both contexts are valid (ie, populated), I think it makes sense to add a check forpub->pub_raw_len, which will remain when we removepk_infoin the future.LGTM, thanks!
@mpg I was about to add this PR to the merge queue when a question suddenly popped up: is the fact that after this PR we allow also opaque RSA keys to have
check_pairsomething that's worth a changelog?Perhaps the
check_pairfunctionality is mostly used "internally" from the TLS side so this is not much of interest for final users. Moreover I don't want to delay this PR for no reason, so I'll let you decide whether it is OK or not to add the changelog or add to the merge queue