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
Implement deterministic ECDSA sign (RFC6979) #18809
Conversation
@pauli - you are ok with adding extra fields to the DRBG_HMAC struct.. It gets ugly if I try to separate the data.. Putting into another file also horrible since the struct then needs to be shared via the header.. |
The following code was used to generate test vectors for dsa
|
616caf9
to
4281bfa
Compare
Test vectors added and rebased to fixup commit message |
Put back into draft form - whilst I figure out if this fits in better as a KDF, as suggested by Pauli. |
The output of the algorithm is a value k (nonce), This is a BIGNUM in the range [2....q-1] |
How does this differ from using the DRBG to generate bytes which need converting to a BN? |
d3dd5e4
to
a62294c
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.
Looks good.
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.
With typos addressed.
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
cee578d
to
8d7220c
Compare
Requires reapproval since i added tests.. |
@paulidale requires reapproval |
test/recipes/30-test_evp.t
Outdated
@@ -119,6 +121,8 @@ my @defltfiles = qw( | |||
evppkey_rsa.txt | |||
); | |||
push @defltfiles, qw(evppkey_brainpool.txt) unless $no_ec; | |||
push @defltfiles, qw(evppkey_ecdsa_rfc6979.txt) unless $no_ec || $no_ec2m; |
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.
Could you please split the ec2m testcases into a separate file?
I'll approve then.
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.
Hmm, actually $no_ec2m should not be necessary. The evp_test should handle missing support for a key type gracefully. Could you please drop it?
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 actually crashed with that line removed..
I am a bit suprised this has not happened before now.. Basically the "Key=XXX" lines fails.. so it sets t->skip = 1.. And then it continues to parse every line of the test (And NULL pointer access in then possible).
I have changed it so it skips the rest of the parsing in this case - I could check for NULL, but it seems silly to continue parsing a skipped test.
24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually. |
This pull request is ready to merge |
Merged to master. Thank you. A trivial merge conflict in 30-test_evp.t was fixed on merge. A fixup commit had to be reordered as it did not apply cleanly when reordered to be immediately after the commit it was a fixup for. This was squashed into the following commit instead. There are no changes to the consequent diff of this entire PR. |
This PR is based off the contributions in PR #9223 by Jemmy1228. It has been modified and reworked to: (1) Work with providers (2) Support ECDSA and DSA (3) Add a KDF HMAC_DRBG implementation that shares code with the RAND HMAC_DRBG. A nonce_type is passed around inside the Signing API's, in order to support any future deterministic algorithms. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #18809)
parameter. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #18809)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #18809)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #18809)
@@ -460,6 +463,7 @@ extern "C" { | |||
#define OSSL_SIGNATURE_PARAM_MGF1_PROPERTIES \ | |||
OSSL_PKEY_PARAM_MGF1_PROPERTIES | |||
#define OSSL_SIGNATURE_PARAM_DIGEST_SIZE OSSL_PKEY_PARAM_DIGEST_SIZE | |||
#define OSSL_SIGNATURE_PARAM_NONCE_TYPE "nonce_type" |
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.
in the updated documentation, the new param string is stated to be nonce-type
.
Should this be nonce-type
rather than nonce_type
?
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.
yeah we should change this... before it makes it into a actual release.
This PR is based off the contributions in PR openssl#9223 by Jemmy1228. It has been modified and reworked to: (1) Work with providers (2) Support ECDSA and DSA (3) Add a KDF HMAC_DRBG implementation that shares code with the RAND HMAC_DRBG. A nonce_type is passed around inside the Signing API's, in order to support any future deterministic algorithms. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#18809)
parameter. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#18809)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#18809)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl#18809)
This PR is based off the contributions in PR #9223 by Jemmy1228.
It has been modified and reworked to:
(1) Work with providers
(2) Support ECDSA and DSA
(3) Add a KDF HMAC_DRBG implementation that shares code with the RAND HMAC_DRBG.
A nonce_type is passed around in the API's, just in case there are
future deterministic algorithms.
Added test vectors for ECDSA from @bbbrumley
bbbrumley@921c037
Checklist