Skip to content
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

Improve ext-openssl to generate EC keys with parameters #9991

Closed
wants to merge 1 commit into from

Conversation

Eno-CN
Copy link
Contributor

@Eno-CN Eno-CN commented Nov 22, 2022

Feat: support ext-openssl Explicit EC Parameters

@cmb69
Copy link
Contributor

cmb69 commented Nov 23, 2022

This looks like a change that should target the master branch (not any of the stable branches).

@cmb69 cmb69 requested a review from bukka November 23, 2022 11:02
.vscode/settings.json Outdated Show resolved Hide resolved
@Eno-CN Eno-CN changed the base branch from PHP-8.1 to master November 23, 2022 12:02
@Eno-CN
Copy link
Contributor Author

Eno-CN commented Nov 23, 2022

This looks like a change that should target the master branch (not any of the stable branches).

@cmb69 The change have targeted the master branch. Will it be merged into PHP 8.1 and above?

@Eno-CN Eno-CN requested review from cmb69 and removed request for bukka November 25, 2022 05:02
@Eno-CN Eno-CN changed the title Improve ext-openssl generate EC keys under OpenSSL 3.0 Improve ext-openssl to generate EC keys with more parameters Nov 25, 2022
@Eno-CN
Copy link
Contributor Author

Eno-CN commented Nov 25, 2022

@cmb69 I added compatibility code for OpenSSL<3.0. It may need to re-review, thank you for much.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

This is just an initial review without checking the logic extensively. The main thing that is missing is cover all the new functionality by tests which is a requirement to get it merged. Preferably try to not modify existing tests but create new ones.

ext/openssl/tests/openssl_pkey_new_basic.phpt Outdated Show resolved Hide resolved
ext/openssl/openssl.c Outdated Show resolved Hide resolved
ext/openssl/openssl.c Outdated Show resolved Hide resolved
ext/openssl/openssl.c Outdated Show resolved Hide resolved
ext/openssl/openssl.c Outdated Show resolved Hide resolved
ext/openssl/openssl.c Outdated Show resolved Hide resolved
ext/openssl/openssl.c Outdated Show resolved Hide resolved
@bukka
Copy link
Member

bukka commented Nov 25, 2022

Just to answer

Will it be merged into PHP 8.1 and above?

No because it is a new feature which is not exactly self contained so it can go only to master - it means PHP 8.3.

@cmb69 cmb69 removed their request for review November 25, 2022 14:34
@Eno-CN Eno-CN closed this Nov 25, 2022
@Eno-CN Eno-CN force-pushed the php-8.1-fix-openssl-20221123020501 branch from cd7a2a4 to eb83e02 Compare November 25, 2022 14:41
@Eno-CN Eno-CN reopened this Nov 25, 2022
@Eno-CN Eno-CN requested a review from bukka November 27, 2022 03:09
@Eno-CN Eno-CN changed the title Improve ext-openssl to generate EC keys with more parameters Improve ext-openssl to generate EC keys with parameters Nov 28, 2022
@bukka
Copy link
Member

bukka commented Nov 30, 2022

@Eno-CN Looks better now but I will need to find some time for a proper review and testing which might take me around a month or possibly slightly longer just to give some expectation as I have got some other things on my list before this one which has got still plenty of time before the change freeze. If you could rebase the changes on master so there's no conflict, that would be great!

@Eno-CN Eno-CN force-pushed the php-8.1-fix-openssl-20221123020501 branch 7 times, most recently from ec51180 to 403ba69 Compare June 15, 2023 05:53
@Eno-CN Eno-CN closed this Jun 17, 2023
@Eno-CN Eno-CN reopened this Jun 17, 2023
@Eno-CN Eno-CN closed this Jun 17, 2023
@Eno-CN Eno-CN reopened this Jun 17, 2023
@Eno-CN Eno-CN force-pushed the php-8.1-fix-openssl-20221123020501 branch 5 times, most recently from 3088eba to 6763590 Compare June 21, 2023 13:56
@Eno-CN
Copy link
Contributor Author

Eno-CN commented Jun 21, 2023

Changes

Add Explicit EC Parameters for openssl_pkey_new

By specifying explicit EC parameters, new curves can be created instead of just using the built-in curves that can be generated by "curve name".
Common EC Parameters
The followings are explicit EC parameters that I added :

  • field_type (optional)

    The value should be either prime-field[default value] or characteristic-two-field, which correspond to prime field Fp and binary field F2^m. only prime field supported temporarily

  • p

    For a curve over Fp , p is the prime for the field.
    For a curve over F2^m , prepresents the irreducible polynomial - each bit represents a term in thepolynomial.
    Therefore, there will either be three or five bits set dependent on whether the polynomial is a trinomial or a pentanomial.

  • a

    See the following param b.

  • b

    a and b represents the coefficients of the curve
    For Fp : y^2 mod p = x^3 +ax + b mod p
    For F2^m : y^2 + xy = x^3 + ax^2 + b

  • generator

    The generator is a well defined basepoint G on the curve chosen for cryptographic operations.
    It can be generated by the next params g_x and g_y.
    The encoding conforms with Sec. 2.3.3 of the SECG SEC 1 ("Elliptic Curve Cryptography") standard.

  • g_x and g_y

    Set (g_x,g_y) to define the basepoint G on curve which defined by params p, a and b.
    They are set to generate generator or ignored whether generator is existed.

  • order

    Integers used for point multiplications will be [0,order - 1].
    NOTICE : For SM2, integers used for the point multiplications will be [1,order - 1].

  • seed (optional)

    seed is an optional value that is for information purposes only.
    It represents the random number seed used to generate the coefficient b from a random number.

  • cofactor (optional)

    cofactor is an optional value.
    order multiplied by the cofactor gives the number of points on the curve.

Optimize the code for generating EC public/private keys

Adjusted the code style to be consistent with the overall code style of openssl.c and enhanced the code readability.

The point (x,y) defines the public key.
d defines the private key.
A private key can generate a public key.

Use EVP_PKEY_generate() to generate keypair instead of EVP_PKEY_keygen()

EVP_PKEY_keygen() do exactly the same thing as EVP_PKEY_generate(), after checking that the corresponding EVP_PKEY_keygen_init() was used to initialize ctx.
EVP_PKEY_keygen() is an older function that is kept for backward compatibility.
It is safe to use EVP_PKEY_generate() instead.

Fix sm2 compatibility bugs on openssl_pkey_get_details

#9422 - openssl ext sm2 compatibility

Reference 1
Reference 2

Examples

EC - generate keypair with curve_name

<?php
/* 
 * Custom parameters x, y, and d are not supported with SM2 in OpenSSL 3.x.
 * Directly creating EVP_PKEY_CTX using EVP_PKEY_CTX_new_from_name(NULL, "SM2", NULL) 
 * will result in generating incorrect private keys (which cannot be correctly recognized 
 * by existing external applications based on the SM2 algorithm).
 */
$curve_name = 'SM2';
$pkey = openssl_pkey_new(array(
    'ec'=> array(
        'curve_name' => $curve_name,
    )
));

$details = openssl_pkey_get_details($pkey);
var_dump($details);
$pubkey = $details['key'];
openssl_pkey_export($pkey, $prikey);
echo 'Private Key:', PHP_EOL, $prikey, PHP_EOL;
echo 'Public Key:', PHP_EOL, $pubkey, PHP_EOL;

EC - generate keypair with custom params (OSCCA WAPIP192v1 Elliptic curve)

<?php
$d = hex2bin('8D0AC65AAEA0D6B96254C65817D4A143A9E7A03876F1A37D'); // private key binary
$x = hex2bin('98E07AAD50C31F9189EBE6B8B5C70E5DEE59D7A8BC344CC6'); // public key x binary
$y = hex2bin('6109D3D96E52D0867B9D05D72D07BE5876A3D973E0E96792'); // public key y binary

$p = hex2bin('BDB6F4FE3E8B1D9E0DA8C0D46F4C318CEFE4AFE3B6B8551F');
$a = hex2bin('BB8E5E8FBC115E139FE6A814FE48AAA6F0ADA1AA5DF91985');
$b = hex2bin('1854BEBDC31B21B7AEFC80AB0ECD10D5B1B3308E6DBF11C1');
$g_x = hex2bin('4AD5F7048DE709AD51236DE65E4D4B482C836DC6E4106640');
$g_y = hex2bin('02BB3A02D4AAADACAE24817A4CA3A1B014B5270432DB27D2');
$order = hex2bin('BDB6F4FE3E8B1D9E0DA8C0D40FC962195DFAE76F56564677');

$pkey = openssl_pkey_new(array(
    'ec'=> array(
        'p' => $p,
        'a' => $a,
        'b' => $b,
        'order' => $order,
        'g_x' => $g_x,
        'g_y' => $g_y,
        //'d' => $d, // import the private key to generate keypairs
    )
));

$details = openssl_pkey_get_details($pkey);
var_dump($details);
$pubkey = $details['key'];
openssl_pkey_export($pkey, $prikey);
echo 'Private Key:', PHP_EOL, $prikey, PHP_EOL;
echo 'Public Key:', PHP_EOL, $pubkey, PHP_EOL;

EC - generate keypair with custom params (SM2 curve)

<?php
$p = hex2bin('FFFFFFFEFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000FFFFFFFFFFFFFFFF');
$a = hex2bin('FFFFFFFEFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000FFFFFFFFFFFFFFFC');
$b = hex2bin('28E9FA9E9D9F5E344D5A9E4BCF6509A7F39789F515AB8F92DDBCBD414D940E93');
$g_x = hex2bin('32C4AE2C1F1981195F9904466A39C9948FE30BBFF2660BE1715A4589334C74C7');
$g_y = hex2bin('BC3736A2F4F6779C59BDCEE36B692153D0A9877CC62A474002DF32E52139F0A0');
$order = hex2bin('FFFFFFFEFFFFFFFFFFFFFFFFFFFFFFFF7203DF6B21C6052B53BBF40939D54123');

/* 
 * Custom parameters x, y, and d are not supported with SM2 in OpenSSL 3.x.
 * Directly creating EVP_PKEY_CTX using EVP_PKEY_CTX_new_from_name(NULL, "SM2", NULL) 
 * will result in generating incorrect private keys (which cannot be correctly recognized 
 * by existing external applications based on the SM2 algorithm).
 */
$pkey = openssl_pkey_new(array(
    'ec'=> array(
        'p' => $p,
        'a' => $a,
        'b' => $b,
        'order' => $order,
        'g_x' => $g_x,
        'g_y' => $g_y,
    )
));

/* 
 * It is not entirely the same as generating keys through the SM2 curve naming method.
 * So the generated key will be in PKCS8 format to store algorithm information.
 */
$details = openssl_pkey_get_details($pkey);
var_dump($details);
$pubkey = $details['key'];
openssl_pkey_export($pkey, $prikey);
echo 'Private Key:', PHP_EOL, $prikey, PHP_EOL;
echo 'Public Key:', PHP_EOL, $pubkey, PHP_EOL;

Reference

[1] Examples - Explicit parameters to define curve
[2] Common EC Parameters
[3] SM2 Key Changes
[4] Examples - Custom params not supported with SM2
[5] Creating an ECC keypair using raw key data
[6] EVP_PKEY_keygen DESCRIPTION
[7] EVP_PKEY_CTX_new On Key Types

@Eno-CN
Copy link
Contributor Author

Eno-CN commented Jun 21, 2023

@bukka After testing, it was found that this issue is related to the excessive length of the first comment in the PR. When the length of the first comment is too long, it causes a certain special change in the output format of the phpinfo test script, which leads to erroneous assertions in the test script.

@Eno-CN Eno-CN force-pushed the php-8.1-fix-openssl-20221123020501 branch from 670fc00 to 96a5314 Compare June 21, 2023 19:45
@Eno-CN
Copy link
Contributor Author

Eno-CN commented Jun 22, 2023

@bukka Now all CI tests have passed. If you have time, please review the code changes.

@Eno-CN Eno-CN force-pushed the php-8.1-fix-openssl-20221123020501 branch 2 times, most recently from 5246c93 to bc708e5 Compare June 26, 2023 05:17
@Eno-CN Eno-CN force-pushed the php-8.1-fix-openssl-20221123020501 branch from bc708e5 to ac963e0 Compare June 30, 2023 05:20
Generate EC keys using the macro OPENSSL_EC_EXPLICIT_CURVE compatible with OpenSSL versions below 1.1.0

Fix SM2 compatibility bugs

Separate EC tests

Add SM2 compatibility test
@Eno-CN Eno-CN force-pushed the php-8.1-fix-openssl-20221123020501 branch from e9839f5 to 1174633 Compare June 30, 2023 10:05
Comment on lines +4953 to +4967
int base_id = 0;

if (EVP_PKEY_id(pkey) != EVP_PKEY_KEYMGMT) {
base_id = EVP_PKEY_base_id(pkey);
} else {
const char *type_name = EVP_PKEY_get0_type_name(pkey);
if (type_name) {
int nid = OBJ_txt2nid(type_name);
if (nid != NID_undef) {
base_id = EVP_PKEY_type(nid);
}
}
}

switch (base_id) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems actually as a bug fix so adding note for myself to back port it potentially.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

@Eno-CN I have done another proper review of the code and all looks good. I have also done extensive testing of all the main supported OpenSSL versions (1.0.2, 1.1.1, 3.0, latest 3.1) and all tests are passing for me locally. Pipeline is also good. So everything seems good. Thanks for your work on this!

@bukka bukka closed this in 0dadd66 Jul 6, 2023
@Eno-CN
Copy link
Contributor Author

Eno-CN commented Jul 7, 2023

@bukka Thank you very much for your thorough review and extensive testing! I'm glad to hear that everything is working fine. Your expertise and experience have made a significant contribution to this project. Thank you for your efforts! If there are any further questions or topics you'd like to discuss, please let me know.

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

Successfully merging this pull request may close these issues.

None yet

3 participants