gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

PK: inline & simplify sign_func #515

New issue
Open
opened 2025-10-13 05:47:46 -05:00 by mpg · 1 comment
mpg commented 2025-10-13 05:47:46 -05:00 (Migrated from github.com)

This is an internal refactoring that should be completely transparent to users. Before the refactoring with want to double-check that we have sufficient testing, and add tests if necessary.

Things that should be tested:

  • Non-wrapping context:
    • Positive: RSA (v1.5), ECC (see "common" below)
    • Negative: trying to sign with a public key (seems missing?)
    • Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH) -> according to lcov this is missing!
  • Wrapping context (created with pk_wrap_psa()):
    • Positive: RSA (v1.5 and v2.1 (anysalt or not)), ECC (plain ECDSA or deterministic) (see "common" below)
    • Negative: the key's usage bits don't allow signing
    • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
  • Common:
    • Positive: check the result with pk_verify() or verify_ext(). When we know deterministic ECDSA will be used, check the signature output against test vectors.
    • Negative: the output buffer is too small (seems missing?)

Once testing is in good shape, we want to:

  • Move the body of all sign_func functions to the body of pk_sign_restartable().
  • Merge as much as possible of the various branches.
  • Avoid reliance on the PK type as much as possible.
  • Do any other cleanup or simplification we can think of.

See Manuel's prototype: #518

Note: we should not change observable behaviour, except that on error cases we can return a different error code, as long as the documentation was not promising a specific error code for this particular case (and the new code makes sense of course).

Scope

This issue is about the public function mbedtls_pk_sign() and the corresponding sign_func function pointers. The fact the mbedtls_pk_sign() happens to be implemented as the non-restartable case of mbedtls_pk_sign_restartable() is an implementation detail. The testing part of this issue is all about ensuring mbedtls_pk_sign() is tested appropriately.

Other issues in this series:

  • #522 is about mbedtls_pk_get_bitlen()
  • #521 is about mbedtls_pk_check_pair()
  • #519 is about mbedtls_pk_verify()
  • #520 is about mbedtls_pk_sign_restartable() and mbedtls_pk_verify_restartable()
This is an internal refactoring that should be completely transparent to users. Before the refactoring with want to double-check that we have sufficient testing, and add tests if necessary. Things that should be tested: - Non-wrapping context: - Positive: RSA (v1.5), ECC (see "common" below) - Negative: trying to sign with a public key (seems missing?) - Negative: trying to sign with a key that doesn't allow signing (`ECKEY_DH`) -> according to lcov this is missing! - Wrapping context (created with `pk_wrap_psa()`): - Positive: RSA (v1.5 and v2.1 (anysalt or not)), ECC (plain ECDSA or deterministic) (see "common" below) - Negative: the key's usage bits don't allow signing - Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH) - Common: - Positive: check the result with `pk_verify()` or `verify_ext()`. When we know deterministic ECDSA will be used, check the signature output against test vectors. - Negative: the output buffer is too small (seems missing?) Once testing is in good shape, we want to: - Move the body of all `sign_func` functions to the body of `pk_sign_restartable()`. - Merge as much as possible of the various branches. - Avoid reliance on the PK type as much as possible. - Do any other cleanup or simplification we can think of. See Manuel's prototype: #518 Note: we should not change observable behaviour, except that on error cases we can return a different error code, as long as the documentation was not promising a specific error code for this particular case (and the new code makes sense of course). ## Scope This issue is about the public function `mbedtls_pk_sign()` and the corresponding `sign_func` function pointers. The fact the `mbedtls_pk_sign()` happens to be implemented as the non-restartable case of `mbedtls_pk_sign_restartable()` is an implementation detail. The testing part of this issue is all about ensuring `mbedtls_pk_sign()` is tested appropriately. Other issues in this series: - #522 is about `mbedtls_pk_get_bitlen()` - #521 is about `mbedtls_pk_check_pair()` - #519 is about `mbedtls_pk_verify()` - #520 is about `mbedtls_pk_sign_restartable()` and `mbedtls_pk_verify_restartable()`
valeriosetti commented 2025-12-05 08:49:02 -06:00 (Migrated from github.com)

Based on the discussions we had during the meeting here I'm writing down the test matrix that I created by expanding requirements above. @mpg @bjwtaylor feel free to edit this message if you think that something is not correct or missing.

PK generating functions

  • non-wrapping with keypair
    • mbedtls_pk_copy_from_psa
    • mbedtls_pk_parse_key
  • non-wrapping with only public key
    • mbedtls_pk_copy_public_from_psa
    • mbedtls_pk_parse_public_key
  • wrapping
    • mbedtls_pk_wrap_psa

Test plan requirement:

  • Non-wrapping context:
    • Positive: RSA (v1.5), ECC (see "common" below)
    • Negative: trying to sign with a public key (seems missing?)
    • Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH) -> according to lcov this is missing!
  • Wrapping context (created with pk_wrap_psa()):
    • Positive: RSA (v1.5 and v2.1 (anysalt or not)), ECC (plain ECDSA or deterministic) (see "common" below)
    • Negative: the key's usage bits don't allow signing
    • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
  • Common:
    • Positive: check the result with pk_verify() or verify_ext(). When we know deterministic ECDSA will be used, check the signature output against test vectors.
    • Negative: the output buffer is too small (seems missing?)

Matrix expanded:

  • non-wrapping -> mbedtls_pk_parse_key

    • EC
      • positive: sign with pk_sign() and verify with pk_verify()
      • positive: sign with deterministic ECDSA and compare against test vectors
      • negative: sign with outbuf buffer to small
      • negative: sign with a key that doesn't allow signing (ECKEY_DH)
    • RSA
      • positive: sign with pk_sign() and verify with pk_verify()
      • positive: sign with pk_sign_ext() (for PSS) and verify with pk_verify_ext()
      • negative: sign with outbuf buffer to small

  • non-wrapping -> mbedtls_pk_copy_from_psa

    • EC
      • positive: sign with pk_sign() and verify with pk_verify()
      • positive: sign with deterministic ECDSA and compare against test vectors
      • negative: sign with outbuf buffer to small
      • negative: sign with a key that doesn't allow signing (ECKEY_DH)
    • RSA
      • positive: sign with pk_sign() and verify with pk_verify()
      • positive: sign with pk_sign_ext() (for PSS) and verify with pk_verify_ext()
      • negative: sign with outbuf buffer to small

  • non-wrapping -> mbedtls_pk_parse_public_key

    • EC
      • negative: sign with only public key
    • RSA
      • negative: sign with only public key

  • non-wrapping -> mbedtls_pk_copy_public_from_psa

    • EC
      • negative: sign with only public key
    • RSA
      • negative: sign with only public key

  • wrapping -> mbedtls_pk_wrap_psa

    • EC
      • positive: sign with pk_sign() and verify with pk_verify()
      • positive: sign with deterministic ECDSA and compare against test vectors
      • negative: sign with outbuf buffer to small
      • negative: sign when the key's usage bits don't allow signing
      • negative: the key's algorithm is not a signature algorithm (ex: ECDH)
    • RSA
      • positive: sign with pk_sign() and verify with pk_verify()
      • positive: sign with pk_sign_ext() and verify with pk_verify_ext() for PSS
      • negative: sign with outbuf buffer to small
      • negative: sign when the key's usage bits don't allow signing
      • negative: the key's algorithm is not a signature algorithm (ex: RSA encrypt)
Based on the discussions we had during the meeting here I'm writing down the test matrix that I created by expanding requirements above. @mpg @bjwtaylor feel free to edit this message if you think that something is not correct or missing. # PK generating functions - non-wrapping with keypair - mbedtls_pk_copy_from_psa - mbedtls_pk_parse_key - non-wrapping with only public key - mbedtls_pk_copy_public_from_psa - mbedtls_pk_parse_public_key - wrapping - mbedtls_pk_wrap_psa # Test plan requirement: - Non-wrapping context: - Positive: RSA (v1.5), ECC (see "common" below) - Negative: trying to sign with a public key (seems missing?) - Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH) -> according to lcov this is missing! - Wrapping context (created with pk_wrap_psa()): - Positive: RSA (v1.5 and v2.1 (anysalt or not)), ECC (plain ECDSA or deterministic) (see "common" below) - Negative: the key's usage bits don't allow signing - Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH) - Common: - Positive: check the result with pk_verify() or verify_ext(). When we know deterministic ECDSA will be used, check the signature output against test vectors. - Negative: the output buffer is too small (seems missing?) # Matrix expanded: - non-wrapping -> mbedtls_pk_parse_key - EC - [ ] positive: sign with pk_sign() and verify with pk_verify() - [ ] positive: sign with deterministic ECDSA and compare against test vectors - [ ] negative: sign with outbuf buffer to small - [ ] negative: sign with a key that doesn't allow signing (ECKEY_DH) - RSA - [ ] positive: sign with pk_sign() and verify with pk_verify() - [ ] positive: sign with pk_sign_ext() (for PSS) and verify with pk_verify_ext() - [ ] negative: sign with outbuf buffer to small - non-wrapping -> mbedtls_pk_copy_from_psa - EC - [ ] positive: sign with pk_sign() and verify with pk_verify() - [ ] positive: sign with deterministic ECDSA and compare against test vectors - [ ] negative: sign with outbuf buffer to small - [ ] negative: sign with a key that doesn't allow signing (ECKEY_DH) - RSA - [ ] positive: sign with pk_sign() and verify with pk_verify() - [ ] positive: sign with pk_sign_ext() (for PSS) and verify with pk_verify_ext() - [ ] negative: sign with outbuf buffer to small - non-wrapping -> mbedtls_pk_parse_public_key - EC - [ ] negative: sign with only public key - RSA - [ ] negative: sign with only public key - non-wrapping -> mbedtls_pk_copy_public_from_psa - EC - [ ] negative: sign with only public key - RSA - [ ] negative: sign with only public key - wrapping -> mbedtls_pk_wrap_psa - EC - [ ] positive: sign with pk_sign() and verify with pk_verify() - [ ] positive: sign with deterministic ECDSA and compare against test vectors - [ ] negative: sign with outbuf buffer to small - [ ] negative: sign when the key's usage bits don't allow signing - [ ] negative: the key's algorithm is not a signature algorithm (ex: ECDH) - RSA - [ ] positive: sign with pk_sign() and verify with pk_verify() - [ ] positive: sign with pk_sign_ext() and verify with pk_verify_ext() for PSS - [ ] negative: sign with outbuf buffer to small - [ ] negative: sign when the key's usage bits don't allow signing - [ ] negative: the key's algorithm is not a signature algorithm (ex: RSA encrypt)
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
development
tf-psa-crypto-1.1
archive/20261005T153337Z-force/pull/927
archive/20261001T141055Z-force/pull/927
archive/20261005T153337Z-force/pull/931
archive/20261001T083616Z-force/pull/927
archive/20260930T232327Z-force/pull/927
archive/20260930T154618Z-force/pull/927
archive/20260929T231919Z-rescue/rohankl-arm/mldsa-pubkey-899
archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9342-public-key-import-export
archive/20260929T231919Z-rescue/ManoharPaturi/fix-pem-ct-padding
archive/20260929T231919Z-rescue/ManoharPaturi/fix-asn1write-null-guard
archive/20260929T231919Z-rescue/ManoharPaturi/fix-pbes2-params-validation
archive/20260929T231919Z-rescue/valeriosetti/blake2
archive/20260929T231919Z-rescue/jfdev001/v1.1.1_bump-cmake-minimum-version
archive/20260929T231919Z-rescue/jfdev001/bump-cmake-minimum-version
archive/20260929T231919Z-rescue/ronald-cron-arm/sm3-base
archive/20260929T231919Z-rescue/valeriosetti/blake2-hmac
archive/20260929T231919Z-rescue/v1rtu3x/enforce-hash-length-sign-verify
archive/20260929T231919Z-rescue/bjwtaylor/file-backed-keystore-doc
archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9344-set-context
archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9343-key-pair-import-export-depends-on-9342
archive/20260929T231919Z-rescue/romart/development
archive/20260929T231919Z-rescue/sweetlilmre/constant-time-embedded-perf
archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-rsa-pss
archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-ecdsa
archive/20260929T231919Z-rescue/JulioLoayzaM/add-wycheproof-tests
archive/20260929T231919Z-rescue/oubaid-a/dev_psa_driver_wrapper
archive/20260929T231919Z-rescue/gilles-peskine-arm/keystoreless-prototype-1
archive/20260929T231919Z-rescue/valeriosetti/issue9580
archive/20260929T231919Z-rescue/LeonieReichertABB/hpke-functionality
archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-hash-no-buffer-copy-1.1
archive/20260929T231919Z-rescue/vanhauser-thc/development
archive/20260929T231919Z-rescue/tpambor/key-wrap
archive/20260929T231919Z-rescue/ruiliio/psa_xts
archive/20261011T063515Z-force/pull/538
archive/20260929T231919Z-rescue/gilles-peskine-arm/fix-broken-links-1.1
archive/20260929T231919Z-rescue/valeriosetti/issue669
archive/20260929T231919Z-rescue/gilles-peskine-arm/prepare-release-test
archive/20260929T231919Z-rescue/krish2718/wpa3-sae-design-note
archive/20260929T231919Z-rescue/nzzlinh/fix_iar_endianness_1.1
archive/20260929T231919Z-rescue/nzzlinh/fix-iar-endianness
archive/20260929T231919Z-rescue/abtom87/fix-pbkdf2-iter-count
archive/20260929T231919Z-rescue/minosgalanakis/tools/prepare_release_lite
archive/20260929T231919Z-rescue/Harshal5/fix/aes-xts-in-place-cts
archive/20260929T231919Z-rescue/sigvartmh/pr/spake2p-9347-prover-depends-on-9342-9343-9344
archive/20260929T231919Z-rescue/crabel99/async-hardware-support-c35df962
tf-psa-crypto-1.2.0_mergeback
tf-psa-crypto-1.1.1_mergeback
archive/20260929T231919Z-rescue/asmellby/feature/dispatch-library
tf-psa-crypto-release-sync
archive/20260929T231919Z-rescue/ChakshuGupta13/fix/ecdsa-zeroize-stack-key
archive/20260929T231919Z-rescue/gilles-peskine-arm/security-md-option-names
archive/20260929T231919Z-rescue/parmi93/patch-2
archive/20260929T231919Z-rescue/bjwtaylor/remove-mldsa-sign-from-build
archive/20260929T231919Z-rescue/bjwtaylor/tf-m-build-issues
archive/20260929T231919Z-rescue/bjwtaylor/test-context-population
archive/20260929T231919Z-rescue/gilles-peskine-arm/iar-1.1.0
archive/20260929T231919Z-rescue/gilles-peskine-arm/fork-helper-unit-tests-create-crypto
archive/20260929T231919Z-rescue/bjwtaylor/redundant-declarations
archive/20260929T231919Z-rescue/hasheddan/docs/fix-broken-links
main
archive/20260929T231919Z-rescue/bjwtaylor/reintroduce-dh
archive/20260929T231919Z-rescue/bjwtaylor/cipher-update-incorrect-error
archive/20260929T231919Z-rescue/Marandil/fix/mbedtls-4314-parse-explicit-curve
archive/20260929T231919Z-rescue/schneider-electric/dev/plaj-se/parse-rsassa-pss
archive/20260929T231919Z-rescue/minosgalanakis/bugfix/program-fuzz-mscv
archive/20260929T231919Z-rescue/daverodgman/aesgcm
archive/20260929T231919Z-rescue/gilles-peskine-arm/sha3-coverity-20260224
archive/20260929T231919Z-rescue/bensze01/undef
archive/20260929T231919Z-rescue/valeriosetti/issue523
archive/20260929T231919Z-rescue/dannytsen/ppc-dev
archive/20260929T231919Z-rescue/daverodgman/aes12
archive/20260929T231919Z-rescue/gilles-peskine-arm/ascon-prototype
archive/20260929T231919Z-rescue/daverodgman/chacha10
archive/20260929T231919Z-rescue/kusumitg/implement_psa_pake_set_context_for_spake2p
archive/20260929T231919Z-rescue/davidhorstmann-arm/add-code-size-target
archive/20260929T231919Z-rescue/bjwtaylor/simplify-sign_func
archive/20260929T231919Z-rescue/mfil/feature/aes_ni_parallel
archive/20260929T231919Z-rescue/ivv19041994/patch-1
archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-verify-policy-relax
public_2/main
archive/20260929T231919Z-rescue/mpg/pk-sign-proto
release/tf-psa-crypto1.0.0-documentation
archive/20260929T231919Z-rescue/gilles-peskine-arm/0e-plans-july-updates
archive/20260929T231919Z-rescue/davidhorstmann-arm/guard-context-types-in-client-only
archive/20260929T231919Z-rescue/gilles-peskine-arm/psa-driver-interface-spec-remove
archive/20260929T231919Z-rescue/gilles-peskine-arm/ecp-self-test-race-crypto
archive/20260929T231919Z-rescue/KiruthikPrakash/feature/9342_add-support-for-spake2p-public-key-import-export
archive/20260929T231919Z-rescue/ruiliio/chacha20_16
archive/20260929T231919Z-rescue/irwir/winsock2
archive/20260929T231919Z-rescue/aphroteus/development
archive/20260929T231919Z-rescue/amjoul01/amjoul01/psa-mac-out-of-bound-fix
archive/20260929T231919Z-rescue/gilles-peskine-arm/everest-submodule-crypto
archive/20260929T231919Z-rescue/yamt/xtensa-ccount
archive/20260929T231919Z-rescue/gilles-peskine-arm/all.sh-generate-only-before-checks-crypto
archive/20260929T231919Z-rescue/Harshal5/feat/builtin_cmac_driver_use_psa_crypto_interface
archive/20260929T231919Z-rescue/gilles-peskine-arm/test-data_t-empty_is_empty-crypto
archive/20260929T231919Z-rescue/jean-baptisteboric-eaton/oids-pkcs7-authenticated-attributes
dev/bensze01/public-only-commit
archive/20260929T231919Z-rescue/waleed-elmelegy-arm/psa-key-wrap-aes-kw
archive/20260929T231919Z-rescue/tfloch/psa_add_xts_algorithm
archive/20260929T231919Z-rescue/gilles-peskine-arm/msancf-clang16-crypto
archive/20260929T231919Z-rescue/DemiMarie/document-unexpected-tags
archive/20260929T231919Z-rescue/DemiMarie/strict-len
archive/20260929T231919Z-rescue/valeriosetti/issue9255-tfpsacrypto
archive/20260929T231919Z-rescue/daantimmer/use-compiler-frontend
archive/20260929T231919Z-rescue/benmcollins/enable-rsassa-pss-pkparse
dev/paul-elliott-arm/add_iop_key_agreement_negative_tests
add-iop-export-pub-key-get-num-ops
dev/rc/new-2-d635ea1
dev/bensze01/jenkins
v1.2.0
tf-psa-crypto-1.2.0
v1.1.1
tf-psa-crypto-1.1.1
v1.1.0
tf-psa-crypto-1.1.0
v1.0.0
tf-psa-crypto-1.0.0
v1.0.0-beta
tf-psa-crypto-1.0.0-beta
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
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

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

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
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Mbed-TLS/TF-PSA-Crypto#515
No description provided.