Skip to content

Comments

Refactor SM2#10942

Closed
levitte wants to merge 20 commits intoopenssl:masterfrom
levitte:refactor-sm2
Closed

Refactor SM2#10942
levitte wants to merge 20 commits intoopenssl:masterfrom
levitte:refactor-sm2

Conversation

@levitte
Copy link
Member

@levitte levitte commented Jan 24, 2020

Up until now, applications had to set the type of SM2 EVP_PKEYs explicitly for SM2 processing to take place, with a call like this:

    if (EVP_PKEY_id(pkey) == EVP_PKEY_EC
        && (eckey = EVP_PKEY_get0_EC_KEY(pkey)) != NULL
        && (group = EC_KEY_get0_group(eckey)) != NULL
        && EC_GROUP_get_curve_name(group) == NID_sm2)
        EVP_PKEY_set_alias_type(EVP_PKEY_SM2);

The goal of this refactoring is to eliminate the need for such extra application side processing, and instead having that happen automatically and an EC_KEY is assigned to an EVP_PKEY.

Furthermore, the functions X509_REQ_verify() and X509_verify() were doing some heavy lifting with SM2 keys, via a function that was almost a straight copy of ASN1_item_verify(). This is replaced with a new function ASN1_item_verify_ctx() that takes an EVP_MD_CTX pointer instead of a EVP_PKEY one, and through which all sorts of parameters can be passed, such as the SM2 identity.
Doing this also prepares for a provider side key adaptation.

@levitte levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Jan 24, 2020
@levitte
Copy link
Member Author

levitte commented Jan 24, 2020

I'm preparing yet another in the series of fixes for #10797 issues, which depends on this cleanup of X509_verify() and X509_REQ_verify(). With this, it will be very difficult to go forward with that.

Comment on lines -511 to +512
Copy link
Member

Choose a reason for hiding this comment

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

This should probably backported to 1.1.1 as well!

Copy link
Member Author

Choose a reason for hiding this comment

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

Just this part, right?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... There was some construction to establish link between a signature and hash algorithm somewhere, and it was used for GOST algorithms.

@levitte levitte force-pushed the refactor-sm2 branch 3 times, most recently from 3b46bcc to ffa0c7a Compare January 28, 2020 17:50
@mattcaswell
Copy link
Member

I'd really like it if @InfoHunter commented on this PR

…rdingly

This means that when loaded or created, EC EVP_PKEYs with the SM2
curve will be regarded as EVP_PKEY_SM2 type keys by default.
Applications are no longer forced to check and fix this.

It's still possible, for those who want this, to set the key type to
EVP_PKEY_EC and thereby run the normal EC computations with the SM2
curve.  This has to be done explicitly.
This makes it possible to generate SM2 parameters and keys like this:

    EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_SM2);
    EVP_PKEY *pkey = EVP_PKEY_new();

    EVP_PKEY_keygen_init(pctx);
    EVP_PKEY_keygen(pctx, pkey);
…ining

The solution to incorporate the SM2 identity processing was an off
the side hack that more or less duplicated the ASN1_item_verify()
code with just a few lines being different.  We replace this with
a new function ASN1_item_verify_ctx(), which takes an EVP_MD_CTX
pointer instead of an EVP_PKEY pointer, just like its sibling
ASN1_item_sign_ctx().

This allows us to refactor X509_verify() and X509_REQ_verify() to
simply create a local EVP_MD_CTX and an attached EVP_PKEY_CTX,
which gets to hold the SM2 identity, if there is one, and then let
ASN1_item_verify_ctx() to its job.

This will also make it easier to adapt ASN1_item_verify_ctx() for
provider based keys.
Since the SM2 curve now implies SM2 computation by default,
test/ecdsatest.c is faulty for not passing an SM2 id.  We could handle
it by creating the appropriate EVP_PKEY_CTX and giving it the SM2 id,
but we do want a test of switching a key with the SM2 curve to become
a "normal" EC key with EVP_PKEY_set_alias_type(), so that's what we do
here.

test/evp_test.c, on the other hand, doesn't need to explicitly set the
EVP_PKEY_SM2 alias type, as that now happens automatically.
There's no longer any need to make an EVP_PKEY type change for SM2
keys, so we trim away that code.
New narrative for ecdsatest:

We test all the curves once for each EC key type we have, i.e. one
round trip with EVP_PKEY_EC and one with EVP_PKEY_SM2.  This shows
that we can use "normal" EC computations on keys with the SM2 curve
(which have the type EVP_PKEY_SM2 by default) and SM2 computations
with any other curve (which have the type EVP_PKEY_EC by default)
@beldmit
Copy link
Member

beldmit commented Jan 30, 2020

No, it was not, I wasn't sure if it is a problem of the PR or master.

@levitte
Copy link
Member Author

levitte commented Jan 30, 2020

I've fixed all the things I could catch on my laptop, let's hope the CIs get happier

Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

Nice work!

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 30, 2020
@mattcaswell mattcaswell added approval: review pending This pull request needs review by a committer and removed approval: done This pull request has the required number of approvals labels Jan 31, 2020
@mattcaswell
Copy link
Member

Looks good apart from the documentation issue - which I do think needs resolving.

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 31, 2020
openssl-machine pushed a commit that referenced this pull request Feb 2, 2020
…rdingly

This means that when loaded or created, EC EVP_PKEYs with the SM2
curve will be regarded as EVP_PKEY_SM2 type keys by default.
Applications are no longer forced to check and fix this.

It's still possible, for those who want this, to set the key type to
EVP_PKEY_EC and thereby run the normal EC computations with the SM2
curve.  This has to be done explicitly.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from #10942)
openssl-machine pushed a commit that referenced this pull request Feb 2, 2020
This makes it possible to generate SM2 parameters and keys like this:

    EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_SM2);
    EVP_PKEY *pkey = EVP_PKEY_new();

    EVP_PKEY_keygen_init(pctx);
    EVP_PKEY_keygen(pctx, pkey);

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from #10942)
openssl-machine pushed a commit that referenced this pull request Feb 2, 2020
…ining

The solution to incorporate the SM2 identity processing was an off
the side hack that more or less duplicated the ASN1_item_verify()
code with just a few lines being different.  We replace this with
a new function ASN1_item_verify_ctx(), which takes an EVP_MD_CTX
pointer instead of an EVP_PKEY pointer, just like its sibling
ASN1_item_sign_ctx().

This allows us to refactor X509_verify() and X509_REQ_verify() to
simply create a local EVP_MD_CTX and an attached EVP_PKEY_CTX,
which gets to hold the SM2 identity, if there is one, and then let
ASN1_item_verify_ctx() to its job.

This will also make it easier to adapt ASN1_item_verify_ctx() for
provider based keys.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from #10942)
openssl-machine pushed a commit that referenced this pull request Feb 2, 2020
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from #10942)
openssl-machine pushed a commit that referenced this pull request Feb 2, 2020
With test/ecdsatest.c, we test all the curves once for each EC key
type we have, i.e. one round trip with EVP_PKEY_EC and one with
EVP_PKEY_SM2.  This shows that we can use "normal" EC computations on
keys with the SM2 curve (which have the type EVP_PKEY_SM2 by default)
and SM2 computations with any other curve (which have the type
EVP_PKEY_EC by default)

test/evp_test.c, on the other hand, doesn't need to explicitly set the
EVP_PKEY_SM2 alias type, as that now happens automatically.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from #10942)
openssl-machine pushed a commit that referenced this pull request Feb 2, 2020
There's no longer any need to make an EVP_PKEY type change for SM2
keys, so we trim away that code.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from #10942)
openssl-machine pushed a commit that referenced this pull request Feb 2, 2020
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from #10942)
@levitte
Copy link
Member Author

levitte commented Feb 2, 2020

Merged

f4e4382 EVP_PKEY_assign_EC_KEY(): detect SM2 curve and set EVP_PKEY type accordingly
0943d5d Add SM2 specific parameter and key generation
bbaddbc X509: Refactor X509_verify() and X509_REQ_verify() for better streamlining
ef077ba Make SM3 a mandatory hash function for SM2.
3995de2 Adapt tests for SM2 changes.
bac1030 Adapt some 'openssl' commands for SM2 changes.
7f293d9 CHANGES: Add note about the refactoring of SM2 EVP_PKEYs

@levitte levitte closed this Feb 2, 2020
* as an error, so it's better to accept the control, check the
* value and return a corresponding value.
*/
if (p1 != NID_sm2) {
Copy link
Member

Choose a reason for hiding this comment

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

I am sorry for the very late response - Just got back from the Chinese lunar new year vacation.

I would like to make a note here explaining that why we just use NID_sm2 and ignore other curves. Though the current NID_sm2 is the only recommended curve but SM2 algorithms (signature/encryption) can actually take other curves as well - if I recall correctly there exist test vectors for SM2 using other curves in some SM2 specification documents.

For OpenSSL I don't think we need to provide the ability to set other curves other than NID_sm2 because the NID_sm2 curve is the only one being used in practice. And when we talk about SM2 in different standard organization (ISO/IETF/...), there is also one curve NID_sm2.

But I still think it's better to leave a comment here (and the paramgen function below as well) that OpenSSL implements only the NID_sm2 curve and provides no other capability of setting other customized curves so that OpenSSL's implementation can only be valid with the SM2 test vectors that use NID_sm2.

@levitte ,what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't limit it like that. What I did was to change the default behaviour, but you can still change the EVP_PKEY type with EVP_PKEY_set_alias_type

It's possible to switch back and forth between the types B<EVP_PKEY_EC>
and B<EVP_PKEY_SM2> with a call to EVP_PKEY_set_alias_type() on keys
assigned with this macro if it's desirable to do a normal EC
computations with the SM2 curve instead of the special SM2
Copy link
Member

Choose a reason for hiding this comment

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

I find this paragraph is misleading that it sounds we provide a way to encourage users to use the the SM2 curve to do a normal EC computation, which I think it should be discouraged to do so. Or should we make it mandatory to use SM2 curve with SM2 special computation only?

mattcaswell added a commit to mattcaswell/openssl that referenced this pull request Feb 4, 2020
PR openssl#10942 introduced the new function ASN1_item_verify_ctx(), but did
not document it with the promise that documentation would follow soon.
We temporarily add this function to missingcrypto.txt until it has been
done.
openssl-machine pushed a commit that referenced this pull request Feb 7, 2020
PR #10942 introduced the new function ASN1_item_verify_ctx(), but did
not document it with the promise that documentation would follow soon.
We temporarily add this function to missingcrypto.txt until it has been
done.

Reviewed-by: Paul Dale <[email protected]>
(Merged from #10980)
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.

5 participants