Skip to content

Conversation

@valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented Nov 28, 2025

Description

This is a prerequisite for Mbed-TLS/TF-PSA-Crypto#570

PR checklist

@valeriosetti valeriosetti 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-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Nov 28, 2025
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

Just one question re server11.key, but mostly looks good otherwise.

Thanks for doing this the 'right' way as it will really help us out in the future!

@@ -0,0 +1,5 @@
-----BEGIN EC PRIVATE KEY-----
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you've regenerated server11.key, was this intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I had not read the commit messages, apologies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NP ;)
server11.key should be a new file, not replacing an existing one. Looking at the diff of this PR it seems that this should be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The weird thing is that the CI is failing and that's totally unexpected since I added new files that are not used anywhere in the current development branches of mbedtls and tf-psa-crypto. I need to investigate

@gilles-peskine-arm
Copy link
Contributor

I recently made an incompatible change in TF-PSA-Crypto (moving crypto_adjust_config_synonyms) that requires a framework change. Sorry. You'll need to rebase your framework branch.

This is basically identical to "server3.crt", i.e. it contains an EC public
key and it's signed by a RSA one. The difference is that in this case
we're using a secp256r1 EC key, instead of the secp192r1 that was used
in "server3.crt".

Signed-off-by: Valerio Setti <[email protected]>
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-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 very sorry to ask, but would you mind updating the middle commit message? It says that the new key is secp192k1 rather than secp256k1, which is incorrect (and one of the things we're trying to remove so fairly confusing).

I'm happy with it otherwise and the rebase should not change any content so should be quick to re-review.

This is a secp256k1 EC key. The goal is to use it in tests where a key
that does not belong to the "suite-b" list is required.
For example it can be used as counterpart of "server5.key" since this one
is secp256r1 and this curve type belong to "suite-b".

Signed-off-by: Valerio Setti <[email protected]>
This is almost identical to "server5-rsa-signed.crt" in the sense that it
includes an EC public key and it's signed with an RSA one.
The main difference compared to "server5-rsa-signed.crt" is that in this
case we're using a secp256k1 key, instead the companion one uses a
secp256r1. The important thing here is that the "k1" type does not belong
to "suite-b", while "r1" does.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti
Copy link
Contributor Author

I'm very sorry to ask, but would you mind updating the middle commit message?

No problem at all, that sentence was totally misleading indeed. It's fixed now :)

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-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, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci Needs to pass CI tests 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 priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Projects

Development

Successfully merging this pull request may close these issues.

3 participants