Skip to content

Comments

ecx: add key generation support.#11371

Closed
paulidale wants to merge 7 commits intoopenssl:masterfrom
paulidale:ecx_keygen
Closed

ecx: add key generation support.#11371
paulidale wants to merge 7 commits intoopenssl:masterfrom
paulidale:ecx_keygen

Conversation

@paulidale
Copy link
Contributor

X25519, X448, Ed25519 and Ed448.

  • documentation is added or updated
  • tests are added or updated

@paulidale paulidale added the branch: master Applies to master branch label Mar 20, 2020
@paulidale paulidale added this to the 3.0.0 milestone Mar 20, 2020
@paulidale
Copy link
Contributor Author

Very early WIP, TLS tests are completely broken.

@slontis
Copy link
Member

slontis commented Mar 20, 2020

In travis it doesnt get past

make update
Due to s390 related code..

@slontis
Copy link
Member

slontis commented Mar 20, 2020

Failure in ./test/evp_test ./test/recipes/30-test_evp_data/evppkey.txt is in fips mode as these are not in the fips module.
You may need to seperate them out into another .txt file and add into the .t file.

@slontis
Copy link
Member

slontis commented Mar 20, 2020

It looks like you broke it good :)

@slontis
Copy link
Member

slontis commented Mar 20, 2020

Pauli just having a cursory look at a failure in ssl_test_old:

I think it generates one of your new keys and then runs..

static int add_key_share(SSL *s, WPACKET pkt, unsigned int curve_id)
{
....
/
Encode the public key. */
encodedlen = EVP_PKEY_get1_tls_encodedpoint(key_share_key,
&encoded_point);

which then calls rv = evp_pkey_asn1_ctrl(pkey, ASN1_PKEY_CTRL_GET1_TLS_ENCPT, 0, ppt);

which probably then calls legacy_asn1_ctrl_to_param which will not handle ASN1_PKEY_CTRL_GET1_TLS_ENCPT and then return -2;

@mattcaswell you may already be aware of this?

@paulidale
Copy link
Contributor Author

I added these in FIPS mode. I suspect there are things that have been missed so far.
Or I've missed thing that I shouldn't have.

@mattcaswell
Copy link
Member

@mattcaswell you may already be aware of this?

Yes - I made the same note here wrt ec keymgmt:

#11328 (comment)

@paulidale
Copy link
Contributor Author

Failing in the TLS tests, but finally passing the EVP ones.

@paulidale paulidale force-pushed the ecx_keygen branch 8 times, most recently from 80a6c9e to 12c5018 Compare April 5, 2020 23:57
@paulidale paulidale marked this pull request as ready for review April 7, 2020 07:05
@paulidale
Copy link
Contributor Author

This seems to be close now, the TLS tests are rubbish but otherwise it seems to work. After building this branch:

$ ./util/opensslwrap.sh genpkey -algorithm ED25519 -out ed.pem
$ ./util/opensslwrap.sh pkeyutl -sign -rawin -in /tmp/f -inkey ed.pem -out sig

Then on a fresh 3.0 (master) build:

$ ./util/opensslwrap.sh pkeyutl -verify -rawin -in /tmp/f -inkey ed.pem -sigfile sig

I get:

Signature Verified Successfully

Where /tmp/f is a trivial test file being verified.

@mattcaswell
Copy link
Member

Can this be rebased? Will the various fixes that @levitte put in help with the TLS failures now?

@mattcaswell
Copy link
Member

Just needs a rebase (to address the merge conflicts) and this fix, and all the tests will start passing:

diff --git a/crypto/evp/pmeth_gn.c b/crypto/evp/pmeth_gn.c
index 67800282de..9a7cdcf591 100644
--- a/crypto/evp/pmeth_gn.c
+++ b/crypto/evp/pmeth_gn.c
@@ -155,8 +155,9 @@ int EVP_PKEY_gen(EVP_PKEY_CTX *ctx, EVP_PKEY **ppkey)
 
         if (keydata == NULL)
             goto not_supported;
-        ret = evp_keymgmt_gen_set_template(ctx->keymgmt,
-                                           ctx->op.keymgmt.genctx, keydata);
+        if (ctx->keymgmt->gen_set_template != NULL)
+            ret = evp_keymgmt_gen_set_template(ctx->keymgmt,
+                                            ctx->op.keymgmt.genctx, keydata);
     }
 
     /*

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Approved on the assumption that #11550 is merged first and the "TLS test fixes" commit is dropped from here following a rebase (and CIs are still ok with it).

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Apr 16, 2020
@paulidale
Copy link
Contributor Author

I've removed the commit. The CIs are expected to fail at this point.

Once #11550 is merged, I'll rebase and confirm the CIs pass.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
Specifically for x25519, x448, ed25519 and ed448.

Reviewed-by: Matt Caswell <[email protected]>
(Merged from #11371)
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #11371)
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #11371)
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
…on-approved algorithms

Reviewed-by: Matt Caswell <[email protected]>
(Merged from #11371)
openssl-machine pushed a commit that referenced this pull request Apr 17, 2020
Also note how to select them.

Reviewed-by: Matt Caswell <[email protected]>
(Merged from #11371)
@paulidale
Copy link
Contributor Author

Travis is happy, merged. Thanks for all the input and assistance.

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

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants