PK: inlined & simplify sign functions

[bjwtaylor]

Description

PK: inlined & simplify sign functions depends #499 depends Mbed-TLS/mbedtls#10439 resolves #515

We may need to add a task if it doesn't already exist to change MBEDTLS_ECDSA_DETERMINISTIC to PSA_ALG_DETERMINISTIC_ECDSA as itwas preventing some tests involved in this PR from running. I have changed these ones, however there maybe more that need investigation.
From the original issue the following test cases have been added where not present.
Non-wrapping context:

• Positive: RSA (v1.5), ECC (see "common" below)
• Negative: trying to sign with a public key (seems missing?)
  • This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L515
• Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH) -> according to lcov this is missing!
  • This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L519
    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
  • This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L647
• Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
  • This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L599
    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.
  • This is covered as part of https://github.com/bjwtaylor/TF-PSA-Crypto/blob/589b7c46d873bdee3bd4d282a14e9792dcbd3137/tests/suites/test_suite_pk.data#L533 and other similar cases
• Negative: the output buffer is too small (seems missing?)
  • This was added here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f5322b7cf59730e53ee00db5c196a937bf635c80/tests/suites/test_suite_pk.data#L555

PR checklist

• changelog not required because: No changes
• framework PR not required
• mbedtls development PR not required because: No changes
• mbedtls 3.6 PR not required because: No changes
• tests not required because: No changes

[mpg]

Test vectors are provided here:

TF-PSA-Crypto/tests/suites/test_suite_pk.data

Line 538 in 1a4690b

ECDSA restartable sign/verify: ECDSA, max_ops=250

I realise now that the way the issue was worded is ambiguous (things were perfectly clear in my mind, but for people who can't read my mind I should have made some assumptions more explicit). I've added a section about scope that should hopefully clarify things: #515

[mpg]

@bjwtaylor There are some commits there that I authored, can I assume you've reviewed them? (That way, I can be a reviewer for the rest of the PR.)

[mpg]

@bjwtaylor Just had a quick look at the list of commits, and I'm afraid I'm not happy with the order: I'd like the new tests to come first, and only then the refactoring. That way we can be confident the refactoring is not accidentally changing observable behaviour. (Holds for all issues in this series: the order is always (1) add tests if needed, (2) refactor.)

[bjwtaylor]

@mpg, thanks for your feedback. It's safe to assume I've reviewed your commits. In terms of the commit order, this makes perfect sense. Do you want me to put the test changes right at the beginning before your commits, or after them?

[valeriosetti]

Just took an initial look at the PR. I'll probably take another one tomorrow

[valeriosetti]

This comment refers to mbedtls_pk_psa_rsa_sign_ext above. Why didn't you embed it into mbedtls_pk_sign_ext() in the same way as you've done with other signature functions?
Is there perhaps another issue for the sign_ext function? If so then sorry, but I didn't find it.

[bjwtaylor]

So, I believe this is out of scope of the issue #515 as this is purely about inlining the functions involved in the sign_func element of the struct mbedtls_pk_info_t. I'm not sure if this is or should be involved in another issue? Or if there is another plan for this function? Probably a question for @mpg. Though if he's happy to grow the scope we can probably include it in this PR.

[valeriosetti]

I was looking at issue #523 and it turned out that refactoring of _ext functions should happen there indeed, so please ignore my comment.
OTOH this sets this PR and the companion one about verify functions as "prerequisite" for #523. It's not a strong prerequisite, but there is a high chance of huge conflicts.

[bjwtaylor]

Ok, I'll ignore it then. In terms of the conflicts, assuming none of the changes are too controversial I am hoping to get the comments sorted soon and get it merged. For #521 I've barely started it yet, however it is next on the list. I've noticed you've taken on #523, so see how you get on and how bad the conflicts are. However if there are a lot maybe we delay it till after #521? I've had a quick look at it and a lot of the tests are similar, so hopefully it shouldn't take too long. It all depends on if there's anything else in the backlog and how important it is though?

[bjwtaylor]

So, as discussed in standup I think the current plan now is to drop the verify and split this into two. Test and implement. Let me know if there is any impact on what you are doing and maybe some coordination will be required.

[valeriosetti]

I expanded the test coverage table requested by @mpg in #515 and checked against what would be available in test_suite_pk.data the the top of this PR and I got the following:

Non-wrapping context:

• ECC

  • Positive: check the result with pk_verify() or verify_ext()
  • Positive: check deterministic ECDSA against test vectors
  • Negative: trying to sign with a public key
  • Negative: trying to sign with a key that doesn't allow signing (ECKEY_DH)
  • Negative: the output buffer is too small

• RSA 1.5

  • Positive: check the result with pk_verify() or verify_ext().
  • Negative: trying to sign with a public key
  • Negative: the output buffer is too small

Wrapping context

• RSA 1.5

  • Positive: check the result with pk_verify() or verify_ext().
  • Negative: the output buffer is too small
  • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
  • Negative: the key's usage bits don't allow signing

• RSA 2.1

  • Positive: check the result with pk_verify() or verify_ext().
  • Negative: the output buffer is too small
  • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
  • Negative: the key's usage bits don't allow signing

• ECC (ECDSA normal/deterministic)

  • Positive: check the result with pk_verify() or verify_ext().
  • Negative: the output buffer is too small
  • Negative: the key's algorithm is not a signature algorithm (eg RSA decrypt or ECDH)
  • Negative: the key's usage bits don't allow signing

Of course, I might have missed something, so I ask the 2nd reviewer to fix/integrate as necessary.

[valeriosetti]

I forgot to add one general question on the implementation before submitting the previous review. Sorry

[valeriosetti]

This comment is not related to this specific line, but to the entire block that follows. Why do we need to implement all this non-restartable code block in a function that ends with _restartable()?
IMO it would make more sense to have it all under mbedtls_pk_sign() and then letting mbedtls_pk_sign_restartable() to call mbedtls_pk_sign() when the operation is not restartable (for any reason), rather than the other way around as it is now.
It's not blocking, but it would be cleaner to me.

[bjwtaylor]

So this is probably a question for @mpg, as he implemented this code. I suspect there was a reason he did it this way?

[mpg]

Yes, I considered this. The point of keeping this inside the restartable function was to avoid duplicating the first two paragraphs (parameters check and call to pk_hashlen_helper()). However that's probably a weak justification for having a function that does two entirely different things.

I can think of two approaches:

1. Accept duplicating those 6 lines between sign() and sign_restartable(), considering that's a small price to pay for readability. Then sign_restartable() calls sign() in the non-restartable case.
2. Introduce a new function sign_internal(). Then sign() keeps calling sign_restartable() which does the checks/adjustments, then either goes the restartable path or calls sign_internal(). (And in the future, the restartable path would be to call sign_restartable_internal() when we inline that part.)

Wdyt? I agree we should take this opportunity to make the code clean and readable, and that the current status (single function implementing both restartable and non-restartable one after the other) is not what we want.

[bjwtaylor]

So, to potentially add an option 3. We could rename the function and we could then keep restartable/non-restartable as is. I think if we start splitting it out we are going back towards the original position with the function pointers, which is not ideal either. If @mpg and @valeriosetti you have a strong preference for either, I'm happy to go with it though.

[bjwtaylor]

@valeriosetti, I can't reply to the comment on test cases. So I've added a response here:

Positive: check deterministic ECDSA against test vectors, I've discussed this with Gilles and we decided we only need to test 1 curve and the ECDSA, 256 Bits (Prime Field) is covered by existing tests:
here https://github.com/bjwtaylor/TF-PSA-Crypto/blob/5df23a947a4ff43d54a8a20b45a2f56bc7aa7eff/tests/suites/test_suite_pk.data#L521

Negative: trying to sign with a public key: this was covered by https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L515

Negative: the output buffer is too small (ECC), covered by
https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L551 and

https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L555

Negative: the key's usage bits don't allow signing (RSA 1.5)

https://github.com/bjwtaylor/TF-PSA-Crypto/blob/f832f63ac873c918e94347394453aae2f7b8ecdc/tests/suites/test_suite_pk.data#L643

I'll add test cases for if we think they are necessary:

Negative: the output buffer is too small (RSA1.5/RSA2.1)
Negative: the key's usage bits don't allow signing (RSA 2.1)
Negative: the key's usage bits don't allow signing (ECC)