Feature/openssl ecc keypair support #1686

dol · 2015-12-18T12:06:13Z

The current ECC (eliptic curver crypthography) support of the OpenSSL extensions has no possibility to generate a EC public key pair.

The PR is a modified version of the patch provided in https://bugs.php.net/bug.php?id=61204 .

Renamed 'ec_group_name' parameter to 'curve_name' to be compliant with the export parameter naming of openssl_pkey_get_details().
Added some missing memory free statements
Added more test cases

The PR also fixes some tabs/spaces formatting issues.

bukka · 2016-01-04T16:56:38Z

ext/openssl/openssl.c

could you possibly add extra indent tab when continuing condition - it's a bit more readable then... ;)

I searched for all the '/^\s+&&/' occurrences within openssl.c and the indentation is always one tab. I leave it like this to be compliant with the file. @bukka Can you refer to a style guide that defines this case. I was not able to find one. Thank you.

yeah leave it. it was just a suggestion to make it a bit more readable - not really defined anywhere. you are right that it's better to make it consistent with the rest of the file...

bukka · 2016-01-04T17:15:45Z

First of all thanks for this contribution!

I cherry-picked the first commit with white space issues (48ae925) so pls merge master into it and it should hopefully reduce the diff (not sure why it's not picked up automatically... :) )

I had a quick look and added few comments. I didn't have time to verify and test it. I think that this would be a nice addition to 7.1 so we still have some time to do that later... ;) But looks good overall!

Thanks

bukka · 2016-01-04T17:17:25Z

ext/openssl/openssl.c

no aliases should be probably here... :)

dol · 2016-01-05T11:13:56Z

@bukka Thank you for your time to review the PR and add very helpful comments and thoughts. As I mentioned in my code comment I found a inconsistency with the group parameters. In retrospective taking the patch from https://bugs.php.net/bug.php?id=61204 and fixing a few things was not enough. ;-)

I'll update the PR that fixes this additional found issues:

Implement the changes @bukka suggested
Return false for openssl_pkey_new() the curve name and the corresponding group parameters are missing or unknown
If one of ['ec']['x'], ['ec']['y'] or ['ec']['d'] parameters missing when using openssl_pkey_new() softfail and generate a new EC key pair. That's already the case, but there is not test case for this. I'll add them to the ECC test case.
Perform proper checks in openssl_pkey_new() and do cleanup if something fails.

I appreciate the cherry-pick of the white space cleanup.

dol · 2016-01-06T23:35:28Z

Fixed the mentioned issues, rebased against current php-src master and squashed commits.

The PR is not complete yet. I found a incorrect handling of the private and public key parameters when creating a key from 'd', 'x' and 'y'.
I'll release a fix and the corresponding test cases soon.

dol · 2016-01-07T00:38:04Z

From my side the RP is finished. This version of the PR fixes the following problem:
If a EC key was created by specifying the 'd', 'x', but 'y' was missing, a new key pair was generated due to the fact that the public key was missing. Event if 'x' and 'y' are missing, the point of the public key can be constructed (calculated) from the private key information 'd'.
This issue is fixed and covered by multiple test cases.

bukka · 2016-01-08T15:04:13Z

Thanks. I will take a look a look once I have time and will test it. As I said there is quite a bit of time before 7.1 and need to sort out few other things before that.

Thanks again for your work!

dol · 2016-01-08T15:59:11Z

Good to know. I'll provide an other patch to openssl_csr_new() that will help to define the content that goes into the x509 extension 'subject alternative name'.

bwoebi · 2016-01-13T22:32:52Z

In general this looks like a self-contained addition, potentially eligible for 7.0.next too. So @bukka you may as well review and merge it now.

bukka · 2016-01-20T17:24:14Z

I'm more in favour of 7.1 for this. It adds a new function and new options for pkey. It is also touching the current code in some places. We should also review it carefully to make sure that everything is fine. Anyway I will take a look once I have more time and finish all other stuff that I plan to do.

dol · 2016-06-13T20:11:50Z

@bukka I just realized PHP 7.1 alpha1 was release some days ago. I updated the PR with the latest master.
Do I need a RFC to be passed to merge this enhancement? I've only contributed minor patches and fixes to the core.
Is this even a feature? Reading the https://wiki.php.net/rfc and additional source not helped to get a clear view. I found @bukka RFC about OpenSSL AEAD support which is kind of a similar extension.
Thank you in advance for help.

bukka · 2016-06-14T09:25:01Z

That's cool. I actually planned to look at this next weekend. Will have to merge few other things before that.

I think that we don't need an RFC if no one objects against the future. The AEAD had RFC because we didn't fully agree on API but this is much more straight forward. However it should be definitely introduced on internals. So if you could send an email summarising what exactly is added (new function, extra params for pkey_new...), that would be great. If no one objects for a week or so and all is cool, I will try to merge it before beta.

bukka · 2016-06-15T18:23:39Z

ext/openssl/tests/029.phpt

+$detailsFromScratch9 = array(
+	"ec" => array(
+		"curve_name" => "prime192v1",
+		"d" => gmp_export("3138550867681922400546388175470823984762234518836963313664"),


Just a quick thing that I noticed in your email. Please don't use gmp in openssl tests as it creates dependency on gmp ext which would require extra skip if. Please use hex value in hex2bin.

@bukka

New features: - openssl_get_curve_names => list ECC curve names - generate a ECC public key pair - generate an CSR with an ECC key - export x,y,d params of ECC public/private key Thanks to @bukka for the review and feedback

bukka · 2016-06-16T18:52:08Z

ext/openssl/openssl.c

+#ifdef HAVE_EVP_PKEY_EC
+		} else if ((data = zend_hash_str_find(Z_ARRVAL_P(args), "ec", sizeof("ec") - 1)) != NULL &&
+			Z_TYPE_P(data) == IS_ARRAY) {
+			pkey = EVP_PKEY_new();


C89 compat issue (should be done after definitions)

bukka · 2016-06-16T19:01:14Z

ext/openssl/openssl.c

+		&& Z_TYPE_P(item) == IS_STRING) {
+		req->curve_name = OBJ_sn2nid(Z_STRVAL_P(item));
+		if (req->curve_name == NID_undef) {
+			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unknown elliptic curve (short) name %s", Z_STRVAL_P(item));


remove TSRMLS_CC from here and everywhere else ;)

bukka · 2016-06-19T17:32:48Z

I just merged #1950 . If you find a little bit of time, it would be great if you could add php_openssl_store_errors(); after each case that stores errors to the OpenSSL queue (of course relating just to your additions...)

bukka · 2016-06-19T17:33:07Z

@dil ^

php-pulls · 2016-06-26T16:01:29Z

Comment on behalf of bukka at php.net:

Superseded by #1959

dol · 2016-06-26T22:00:23Z

@bukka You are a hero. I just wanted to implement your suggestions today. Haven't had the time to look into the improvement due to family time.

jpauli added the Feature label Dec 24, 2015

bukka reviewed Jan 4, 2016
View reviewed changes

ext/openssl/openssl.c Outdated

Copy link

Member

bukka Jan 4, 2016

Choose a reason for hiding this comment

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

no aliases should be probably here... :)

kelunik mentioned this pull request Mar 30, 2016

ECC Support kelunik/acme#14

Open

bukka reviewed Jun 15, 2016
View reviewed changes

bukka reviewed Jun 16, 2016
View reviewed changes

php-pulls closed this Jun 26, 2016

Feature/openssl ecc keypair support #1686

Feature/openssl ecc keypair support #1686

Uh oh!

Conversation

dol commented Dec 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka commented Jan 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dol commented Jan 5, 2016

Uh oh!

dol commented Jan 6, 2016

Uh oh!

dol commented Jan 7, 2016

Uh oh!

bukka commented Jan 8, 2016

Uh oh!

dol commented Jan 8, 2016

Uh oh!

bwoebi commented Jan 13, 2016

Uh oh!

bukka commented Jan 20, 2016

Uh oh!

dol commented Jun 13, 2016

Uh oh!

bukka commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka commented Jun 19, 2016

Uh oh!

bukka commented Jun 19, 2016

Uh oh!

php-pulls commented Jun 26, 2016

Uh oh!

dol commented Jun 26, 2016

Uh oh!

Uh oh!