-
Notifications
You must be signed in to change notification settings - Fork 42
[framework] Remove support for secp192[k|r]1 curves #242
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
base: main
Are you sure you want to change the base?
Conversation
davidhorstmann-arm
left a comment
There was a problem hiding this 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----- | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
I recently made an incompatible change in TF-PSA-Crypto (moving |
8338ee0 to
ca68004
Compare
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]>
ca68004 to
f7d3257
Compare
There was a problem hiding this 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]>
f7d3257 to
85cbd7a
Compare
No problem at all, that sentence was totally misleading indeed. It's fixed now :) |
davidhorstmann-arm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Description
This is a prerequisite for Mbed-TLS/TF-PSA-Crypto#570
PR checklist