Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PSA interruptible key agreement APIs #9490

Merged

Conversation

waleed-elmelegy-arm
Copy link
Contributor

@waleed-elmelegy-arm waleed-elmelegy-arm commented Aug 20, 2024

Description

Fix #9107
Add PSA interruptible key agreement APIs

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.

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

Help make review efficient:

  • Multiple simple commits
    • please structure your PR into a series of small commits, each of which does one thing
  • Avoid force-push
    • please do not force-push to update your PR - just add new commit(s)
  • See our Guidelines for Contributors for more details about the review process.

@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the add-iop-key-agrmnt-api branch 3 times, most recently from 958f30f to 8036a1f Compare August 22, 2024 17:06
@waleed-elmelegy-arm waleed-elmelegy-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-m Estimated task size: medium (~1w) component-psa PSA keystore/dispatch layer (storage, drivers, …) and removed DO-NOT-MERGE labels Aug 22, 2024
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a first pass of looking at the general shape and a few places in the code. I have not done a proper review yet.

I think we should resolve #7029 first.

@@ -7732,6 +7732,128 @@ psa_status_t psa_key_agreement(mbedtls_svc_key_id_t private_key,
return status;
}

uint32_t psa_key_agreement_iop_get_num_ops(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many users don't care about interruptible operations and don't want to pay for the code size. This is a preexisting issue with signature. For new interruptible code we should do it right from the start, it's easier than fixing it later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As agreed, we will do this later, further discussion on this in #7029

tf-psa-crypto/core/psa_crypto.c Outdated Show resolved Hide resolved

if (!PSA_KEY_TYPE_IS_ECC_KEY_PAIR(private_key_attributes.type) ||
!PSA_ALG_IS_ECDH(alg)) {
return PSA_ERROR_INVALID_ARGUMENT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the mechanism doesn't support restartable operations (e.g. FFDH), this function must succeed, and complete() must return the result immediately.

That also applies when MBEDTLS_PSA_BUILTIN_ALG_ECDH is disabled. The structure of compile-time conditionals in this function doesn't look right to me.

Also applies to the other functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Paul's suggestion, I agree with limiting the scope of this quarter's work for key agreement and other functions to ECC keys. We do need to file issues to finish the API later (it can be done in parallel with the X.509/TLS work), and we do need to be careful that the API design supports it (the PSA API does, but we also need the compile-time selection API and where relevant the pk wrappers).

tf-psa-crypto/core/psa_crypto.c Outdated Show resolved Hide resolved
tf-psa-crypto/core/psa_crypto_ecp.c Outdated Show resolved Hide resolved
tf-psa-crypto/core/psa_crypto_ecp.c Outdated Show resolved Hide resolved
tf-psa-crypto/tests/suites/test_suite_psa_crypto.function Outdated Show resolved Hide resolved
tf-psa-crypto/tests/suites/test_suite_psa_crypto.function Outdated Show resolved Hide resolved

static uint32_t interruptible_key_agreement_get_min_num_ops(size_t key_bits)
{
uint32_t min_values[3][5] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about curve families other than SECP_R1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on the curve size, and we only support Short Weierstrass curves for interruptible operations.

@@ -7357,6 +7357,102 @@ PSA key agreement: FFDH RFC7919 6144 key + HKDF-SHA256: read 1+255
depends_on:PSA_WANT_ALG_FFDH:PSA_WANT_ALG_HKDF:PSA_WANT_ALG_SHA_256:PSA_WANT_KEY_TYPE_DH_KEY_PAIR_BASIC:PSA_WANT_KEY_TYPE_DH_KEY_PAIR_IMPORT:PSA_WANT_DH_RFC7919_6144
key_agreement_output:PSA_ALG_KEY_AGREEMENT(PSA_ALG_FFDH, PSA_ALG_HKDF(PSA_ALG_SHA_256)):PSA_KEY_TYPE_DH_KEY_PAIR(PSA_DH_FAMILY_RFC7919):"bbaec0a6c20e67aa77bd9db1f682b20227d3e17944ccf9ea639e437202309c29dc876a8d209e81e59e1d7584284089c4ffb3356e28acca6c94164752e7e331cee7fccdb3d08604a5faaf91c02cab4ea6ad2926e28d1dee9fadd437b2b8a5116c689869c0972529e4c362aaa8427c95f42d8a60c1f38f9f672c837a097bcd1a8c068c11a33ce36517915dae1ba47e2646aef079e6c84b9656991ef0f6ceb9f7f95c97e7232cc5c41c0335aed99169133702cb8d95ef1e9eb5af583f3469a77277243fe61f16dd5b4f9f4972e3d30050f289f891daf8146ff87cf2845c419dfe2ca0525c5e2e8fc6566d7118fadaf0103b24319061f862e2584e5fba1063d55365b78379820d335ee924ac0871ceb3a2a339fba250011371b53426bab5f48e9704b7a9e77d14d5f6cafcfbdb45463e6935be31bc87eafd9b6d228a5b76c2baa6364f450a4ac557dd07ed4b1a13f5603e2b3bb270e831f0f2950f52c52d866fdaeb748a4cbb6f20b332795fffb8cf77a34ef75d8105973f1fdada6a3b050a28c12268104a8f1cce9a86ebce1749a97e9e5f00608229799aa5b7a356fca7b8bb5c7829cb18a136836bb37f5165deb89b33f0b69c473236025bc649d382d008fbc7c8c84390b9d86b173e45fa1e162e0eabd7914f2ec4c26d5350be064fc0d68bf16446188dd4a76ac1267a63b764070b48342a884891eeddbba95257348764c646aef160523af105a719aedb041a28b81516dbe89e80592f687eb341aff447a4165ac145889ae3e8a14c948c82b581b35d8f7d1c4f5e0f838773a472ad0025b1ca0b1c8bfe58c42079194b9aa9c5a1139472e7f917655a3ae297c9a8e3bfa6e108242a5ac01b92a9e94d7b51fbe2732d68f1ec5c12607add5e9bddbe5a4837e9fa16a66b5d83456df4f9febb14158dc5ea467b7cc288fe58f28cade38fa3d4c8864c3cb93bda6d39ad28f7dab8b8c0be34f675d268d82ba6a2e22ba49a5e7de5d08edae35ec17d1419288719a4f82dfb7aad6f7b68c4216c69b83af7438771622e48381841d1fcb6081d41b1b84eae37912b34dc8df1794bb47ad87f94d9c841aa98":"31b48495f611fd0205994fc523bfbc6b72949417f28392d30c1c98878bde0ca467ab6d6fe58522df9749154f95c9683f9590c295cd2b62ff9c59f2a71aaa3f7cb72761740cdcac8994c3623e8c07e2991dac60c2ccba818623013467cfca64f9a3b58523d4a4982571365db08aa9de048303c2a48d1c02c9aafc2ecd6eaae1c5bce8314503d0711d755b59134cbfc773250690121f58fc5171ea34fe88e753d5ab3da23e0557aa326b408c2f55aad2b6f40504509c2203f353bcb17e7b2c61fdcba04c3f8c136ef5d14c38ded6ff0455f59f3052b52b2d45f76a2c3b4b09af388a57ebd9d33393853b83b8033b6973cf662907e62380b66b4ce04b82ab8fcd35f40083a330587e27daa0f84c21fc5d04af03104785f85cb880ae61024cf6cfd1dc14149fdff6653968458fb5761cf2cbf8263e915099eb209d1d149bd7a5b4e48b108f07a1f7c17aa4cbf7b3aa25075956f93f127d46b6392834e7781e46f0e2d1ba14ce2f2d91f9db106bf94c7110ace1bf6105cd9351031e0ec7b52a599ae41256581c1379be5882c352c750709c1b8d37cd8d1442ae5547db0f5a1371eca211f028428572a0fcc4c0852ec1f9be4de14a32536087f520cdeaf54c52b203bb6ff0008b2099fb0e1dff4547563a71db416c5b97ef8e7677d8edd15a2ae75dc64b817117fe5e0478cfa1a18e15cb44cfcc990c5f01127b3906187c18562c876631a046a70015e84b6c553be23168e572cedb5912a6505ff8bb65722cc0e9556e967600711b8d8a8e414811c9809aa3e15f680fdbb2b2297e414824fda530b501b278c35f3f0f0ac61da3262de7b8aa44e31544c593c8521f8ce4921b8d7df7d7382c97718efd03650caa5620bc0e6fb9753dfe26c78b0b6a3231391b9324ee6b7c81b45e7e90e5573ab6cb263b114d78eaba7eb2bc668dd57b6eef126abcdf8355656beac58ddbaeb0551a4083fd5a2bd0e405d35737b7c3c6f0f0190403c13b57e3ef7b6b76206725758523ef98e4053fb8e05147a74577b61b0935dc5eb699945d3290e78bcc9015c9c3210ffed7d6e96c6c8202e46ad37155d07f3e8c2d9a":"10":"5d324ec021d57640dee474c442f3a25390de6ff13175f70fad977003bd78fcdfeda87d2a5cc8447b9729990b11e7949c6ebb37a2d3c2fa69a85d79d216a6a489c8c5186576c112ca94c1bce156b819fb010a4168e8c91e777b87dceb0de4f1828c45297e3b513f4ff57bfb874a7c0d3cd709332922394bcddbc0bf959668810ce1ec8dbff662ea620b9ee7186cdde9845185ea87ded242fbffb7f526d875b6b1dbd09a4008b4d2c1034621a75efd6140c7d6fc883d79f7c3b7f7ae21b74e62a9c26f682c9dd48cacdc7f0c4ec5eb32a5c505aa5949d4008ece502bca5612f84ae73164acd2d3399cc9aee5cf615de62dd31c63a407f5c988b5c61a124ce08c"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing: tests with algorithms that don't have an interruptible implementation, and tests of the interruptible functions when interruptible functionality is disabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to move those tests to #9108, but are we ok moving the relevant functionality there too?

Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than agreeing with Gilles comments (especially on the tests), looks generally good to me, however need clarity on what we are leaving until #9108

As regards #7029 I think this will have to wait until later as a potential code size improvement, given the amount of work it potentially entails (although it would still be good to hammer that issue out)

tf-psa-crypto/core/psa_crypto_ecp.c Outdated Show resolved Hide resolved
tf-psa-crypto/core/psa_crypto_ecp.c Outdated Show resolved Hide resolved
tf-psa-crypto/include/psa/crypto_struct.h Show resolved Hide resolved
@yanesca yanesca added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Sep 3, 2024
@waleed-elmelegy-arm waleed-elmelegy-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 4, 2024
@paul-elliott-arm paul-elliott-arm self-requested a review September 6, 2024 14:35
Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this for now - extra testing coming in #9109

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the whole code.

Due to the lack of non-nominal tests, I haven't reviewed test coverage. Furthermore, while I've reviewed for local correctness, in the absence of negative testing, I haven't reviewed for potentially missing validation or error detection. I'll need to do another pass on the code once we have a good view of the potential for errors.

ChangeLog.d/add-psa-iop-key-agreement.txt Outdated Show resolved Hide resolved
tf-psa-crypto/core/psa_crypto.c Outdated Show resolved Hide resolved
tf-psa-crypto/core/psa_crypto.c Outdated Show resolved Hide resolved
tf-psa-crypto/core/psa_crypto.c Show resolved Hide resolved
goto exit;
}

operation->ctx.mbedtls_ctx.attributes = attributes;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributes is owned by the caller of the library. After this function returns, the pointer is no longer valid. The use of attributes in complete() can be a use-after-free. We need to make a copy of the structure, we can't keep a pointer.

Please fix and add a non-regression test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referenced this comment to be added in negative testing ticket.

tf-psa-crypto/tests/suites/test_suite_psa_crypto.function Outdated Show resolved Hide resolved
tf-psa-crypto/tests/suites/test_suite_psa_crypto.function Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work priority-high High priority - will be reviewed soon and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Sep 13, 2024
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
- rename psa_driver_wrapper_key_agreement_xxx to
  psa_driver_wrapper_key_agreement_iop_xxx.
- reorganise the paraemters of psa_driver_wrapper_key_agreement_setup

Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
…nd normal APIs

Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
@waleed-elmelegy-arm
Copy link
Contributor Author

Had to force push to rebase due to conflicts with development

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initializers are now correct, but the ones for ECDH are still more complex than needed (and also not compliant to C++11).

tf-psa-crypto/drivers/builtin/include/mbedtls/ecdh.h Outdated Show resolved Hide resolved
tf-psa-crypto/drivers/builtin/include/mbedtls/ecdh.h Outdated Show resolved Hide resolved
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! LGTM.

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 7, 2024
Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paul-elliott-arm paul-elliott-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 8, 2024
@yanesca yanesca added this pull request to the merge queue Nov 11, 2024
Merged via the queue into Mbed-TLS:development with commit 241b901 Nov 11, 2024
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-psa PSA keystore/dispatch layer (storage, drivers, …) priority-high High priority - will be reviewed soon size-m Estimated task size: medium (~1w)
Development

Successfully merging this pull request may close these issues.

Implement PSA Interruptible ECC Key Agreement.
5 participants