mbedtls
mbedtls copied to clipboard
Mbed-TLS
FFDH 1, 2A, 2B: FFDH add support for import/export key, key agreement, key generation + tests
Description
- Add implementation and tests to enable support for import/export of FFDH keys.
- Add support for FFDH with known groups to psa_key_agreement_raw_internal,
- Add support for FFDH key generation.
Resolves https://github.com/Mbed-TLS/mbedtls/issues/5911 Resolves https://github.com/Mbed-TLS/mbedtls/issues/3261 Resolves https://github.com/Mbed-TLS/mbedtls/issues/5912
Status
READY
Requires Backporting
NO
TO BE RESOLVED
- Test Vector loo long for FFDH 1024 byte keys
I created script for generating test vectors for FFDH key agreement , added tests for psa_raw_key_agreement and everything works except tests for 1024 bytes keys. In this case I'm getting parsing error (looks like some kind of length limitation):
PSA raw key agreement: FFDH 8192 bits ............................. Expected string (with "") for parameter and got: "ede0361026e81a9ad960f674de49449f12ee33c2dda7028c6b7fad7f8f8a7edc495621a6d13e47847873a954adfe7bb6a2ed7c9bc21f3b57458d9116ff4ed06cfca40e2002a70bca91a9a9e0475dd74be7d58453d3cc155ee0b0be20197e14674a7a6f8d903e211cbdbdad1e3383d0d1ae6b4d56837671589d8f151acb34bb4d1cdda55a0f9d1f70e80c61553fd0152bc871e930054efe763fdcd1f8fd1702afa61b3471e7a504612c58ab05ed581b34e2a884c5dd8d2aa919855351719e2cb290d00f0b161c104415f5579731072c1382508421c8d674113b2fe25a0e979455c8f145285ed3d32b744153d3ffab7625a3173440f026ecc62d9dd1bbdff6136f5d9d5245ff307eabfa91f6a10e7cf62a889975c0afd2f707eb8a43c2499c05029ca613edae2741f8e56b186a6390fbb0962323ed6c492620c1c8a24f9a89f15c00bd7263423e714db0fe0381556a15a8e4d1b7383d52fd524425e0200f9d410833330253306b1c23c15c08310bfc12b48131c120db8444d34dd951c5fd6df44e0eecbe92ad5f13641600db68d1d2c7d8ff460058c09d89d4febf2fcaacb40c900e19e4dc868a24ec61361c452541a0fb13da53d61b59806e0598985031e161a2e887420e4c6ce217587c72cd3a7b3085d2383112e1066277ed63e82ec16ac6dc7ce0ade255f30275b9798d4476f31d8d237c4d79b13da9dc6ceed7fe626e4da6eb6cfd234b8fdec4fd4520898b13a77aa034361c0d63edef55595e3e638b48c1c00e8c683c8cffd9fac2a33f73e04aff1f4624669057c7faf51f996e3d64bea3097b4810f99c8f078887be2440f67b249467eb26a03210b4d2baeaa8dc9746a14a6cfb45297e121eef8540eb438270403105c11ef4fed87127545b81e37ee1f942605a5a46253752351dee91d0a171031defa9dd20cbb942e3940fa43542f6fbcb0980f6ef2b36297527f7c0d47e36ea203
- Issue in
psa_key_agreement_ffdh(): please see code review comment.
Rebased on top of development to check if this help with CI (it was green some time ago).
- Rebased anf resolved conflicts
- Refactored PR (reduced number of commits)
- Tested again
CI is green I think this PR still needs 2 reviewers as @Zaya-dyno is not working on the project.
I created script for generating test vectors for FFDH key agreement , added tests for psa_raw_key_agreement and everything works except tests for 1024 bytes keys. In this case I'm getting parsing error (looks like some kind of length limitation):
As a coincidence, I was just reading https://github.com/Mbed-TLS/mbedtls/issues/1866 which might be the culprit here.
I also have a general question: I know that this is not listed in the 3 issues that this PR is going to solve (https://github.com/Mbed-TLS/mbedtls/issues/5911, https://github.com/Mbed-TLS/mbedtls/issues/3261, https://github.com/Mbed-TLS/mbedtls/issues/5912), but since we are adding support for a new feature that goes through PSA, isn't it worth to add also a new component in all.sh for testing the accelerated version of it?
I understand that this PR already introduces a good amount of changes, so perhaps it's something we can postpone to another issue/PR...
since we are adding support for a new feature that goes through PSA, isn't it worth to add also a new component in all.sh for testing the accelerated version of it?
We don't yet have accelerator support for key agreement.
We don't yet have accelerator support for key agreement.
Are you sure about this? Either we do, or this component is not doing at all what I think it's doing. (I think it has ECDH provided only by libtestdriver1 and is still doing ECDH-based key exchanges in TLS successfully, which would indicate support for raw key agreement.) Also, we have a file named tests/src/drivers/test_driver_key_agreement.c (extended by this PR).
Regarding the original question: this PR is large enough as it is, so if we want to test acceleration (key generation and key agreement) that should be a follow-up PR.
We don't yet have accelerator support for key agreement.
Are you sure about this?
Ah, no, you're right, we do now. But indeed, this PR is already big enough, let's not expand the scope to driver support.
I hope I addressed or responded to all comments. Sorry if I missed something. Many comments were to the old version of the code. Let's see if CI is happy now.
Unfortunately the issue with key length was still present while exporting public key. Hopefully now CI accepts this PR.
I'm not sure if some action is needed here looking on CI results.
Restarted to check if win32-mingw(84/115 Test #84: platform-suite .............................***Failed) fail will happen again as Open CI results look fine (TF OpenCI: PR-6010-head TLS Testing — All tests passed).
Generally speaking, if a test that's unrelated to the PR passed on one CI and fails on the other, we can ignore it. (If the test is possibly related to the PR however we should investigate, as it could be a bug in the PR that only happens when some random byte happens to be 0, or things like that.) Here I think it's OK, but I see you've restarted the internal CI. Hopefully it will come back all green this time.
Internal CI: PR-6010-head TLS Testing — All tests **passed**`
All review comments addressed and CI can be now considered as green. Ready for review.
The failure of test_suite_platform on mingw is a known issue. You can ignore any such failure in a PR that doesn't affect the platform support functions/tests. The test case has just been removed.
Ah, no, you're right, we do now. But indeed, this PR is already big enough, let's not expand the scope to driver support.
Should we create a follow-up issue about it, then? On one hand, I'm not sure FFDH accelerators really matter these days. OTOH, since it's supposed to be supported, it should be tested. (Or we should document clearly in a place that's easy to discover that we don't support it. I find it very painful when working with drivers to never know what's supposed to be supported without having to dig in the code - or in practice, ask Ronald or Gilles.)
Even if driver support for FFDH doesn't matter, I suspect that not having driver support will be more hassle than adding it because we'd have to add exceptions to test code.
In current version there is support for key_agreement only, so still needs to be added for import, export, generate key.
One review is still needed. Are you planning to review this @gilles-peskine-arm or it was only guest review and I should ask @valeriosetti ?
I've only done a partial review and I don't intend to do a full review (or at least not soon), so I'd appreciate it if @valeriosetti can do a full review.