Add SM2 key management by InfoHunter · Pull Request #12536 · openssl/openssl

InfoHunter · 2020-07-26T15:27:59Z

Add support for SM2 key management in provider. This is mainly based on #11248 (comment)

Checklist

documentation is added or updated
tests are added or updated

InfoHunter · 2020-07-26T15:28:29Z

Note: it's only a skeleton at current stage and I will add real content later ;-)

include/openssl/ec.h

InfoHunter · 2020-07-27T12:46:25Z

I added the SM2 key generation by specifying a new EC flag into the EC_KEY object and thus the ec_generate_key function can behave correctly for an SM2 private key. Any ideas? @openssl/committers

levitte · 2020-07-27T13:04:16Z

I added the SM2 key generation by specifying a new EC flag into the EC_KEY object and thus the ec_generate_key function can behave correctly for an SM2 private key. Any ideas? @openssl/committers

I had the same idea. Considering how little they differ otherwise, I think this is the sane way to do it.

InfoHunter · 2020-08-01T16:54:06Z

WIP has just been removed. Please have a review on this @openssl/committers , thanks!

mattcaswell

How does this work without the equivalent exchange/signature implementations? Or does it fallback to legacy somehow???

Are our current tests actually hitting these codepaths? From code inspection it looks to me like ec_generate_key will crash in the SM2 codepath - which makes we wonder whether we ever actually get there....or am I wrong about the crash?

crypto/ec/ec_key.c

providers/implementations/keymgmt/ec_kmgmt.c

InfoHunter · 2020-08-07T16:11:35Z

How does this work without the equivalent exchange/signature implementations? Or does it fallback to legacy somehow???

Actually this PR should base on PR #11248 , so it will fail from my understandings.

InfoHunter · 2020-08-07T16:12:21Z

Are our current tests actually hitting these codepaths?

Anything I need to do to include this in to our tests?

providers/implementations/keymgmt/ec_kmgmt.c

levitte · 2020-08-07T21:52:10Z

For testing, I would remove these lines and see what happens:

openssl/crypto/evp/pmeth_gn.c

Lines 23 to 25 in 378b163

    
           #if !defined(FIPS_MODULE) && !defined(OPENSSL_NO_EC) 
        
           # define TMP_SM2_HACK 
        
           #endif

InfoHunter · 2020-08-08T14:16:07Z

After I tried to remove the macro @levitte suggested, it seems no changes on the testing side. But I investigate a bit on other test case failure and fixed the test_ecdsa.t to match the new changes introduced in this PR.

InfoHunter · 2020-08-08T14:20:10Z

I noticed one test failure on EVP_PKEY_CTX_set1_id. From my understanding, this should not fail on introducing new SM2 keymgmt code since the ID is located in SM2 pkey ctx. Could you please have a look on this if you have spare time @levitte ?

levitte · 2020-08-09T15:07:12Z

I noticed one test failure on EVP_PKEY_CTX_set1_id. From my understanding, this should not fail on introducing new SM2 keymgmt code since the ID is located in SM2 pkey ctx. Could you please have a look on this if you have spare time @levitte ?

Well, it works with the legacy SM2 implementation, because this:

openssl/crypto/sm2/sm2_pmeth.c

Lines 204 to 225 in 04cb5ec

    
           case EVP_PKEY_CTRL_SET1_ID: 
        
               if (p1 > 0) { 
        
                   tmp_id = OPENSSL_malloc(p1); 
        
                   if (tmp_id == NULL) { 
        
                       SM2err(SM2_F_PKEY_SM2_CTRL, ERR_R_MALLOC_FAILURE); 
        
                       return 0; 
        
                   } 
        
                   memcpy(tmp_id, p2, p1); 
        
                   OPENSSL_free(smctx->id); 
        
                   smctx->id = tmp_id; 
        
               } else { 
        
                   /* set null-ID */ 
        
                   OPENSSL_free(smctx->id); 
        
                   smctx->id = NULL; 
        
               } 
        
               smctx->id_len = (size_t)p1; 
        
               smctx->id_set = 1; 
        
               return 1; 
        
           case EVP_PKEY_CTRL_GET1_ID: 
        
               memcpy(p2, smctx->id, smctx->id_len); 
        
               return 1;

and this:

openssl/crypto/sm2/sm2_pmeth.c

Lines 267 to 280 in 04cb5ec

    
           } else if (strcmp(type, "distid") == 0) { 
        
               return pkey_sm2_ctrl(ctx, EVP_PKEY_CTRL_SET1_ID, 
        
                                    (int)strlen(value), (void *)value); 
        
           } else if (strcmp(type, "hexdistid") == 0) { 
        
               hex_id = OPENSSL_hexstr2buf((const char *)value, &hex_len); 
        
               if (hex_id == NULL) { 
        
                   SM2err(SM2_F_PKEY_SM2_CTRL_STR, ERR_R_PASSED_INVALID_ARGUMENT); 
        
                   return 0; 
        
               } 
        
               ret = pkey_sm2_ctrl(ctx, EVP_PKEY_CTRL_SET1_ID, (int)hex_len, 
        
                                   (void *)hex_id); 
        
               OPENSSL_free(hex_id); 
        
               return ret; 
        
           }

What you need to do is to convert that macro to something that handles OSSL_PARAM, and you need to be able to handle that in the SM2 signature implementation. You can have a look at what we already do for EC for inspiration (the reason I point at UKM stuff is the use of octet_string, which is what I recommend for the ID):

openssl/crypto/ec/ec_ctrl.c

Lines 351 to 422 in 04cb5ec

    
           int EVP_PKEY_CTX_set0_ecdh_kdf_ukm(EVP_PKEY_CTX *ctx, unsigned char *ukm, int len) 
        
           { 
        
               int ret; 
        
               OSSL_PARAM params[2], *p = params; 
        
               ret = evp_pkey_ctx_getset_ecdh_param_checks(ctx); 
        
               if (ret != 1) 
        
                   return ret; 
        
               /* TODO(3.0): Remove this eventually when no more legacy */ 
        
               if (ctx->op.kex.exchprovctx == NULL) 
        
                   return EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_EC, 
        
                                            EVP_PKEY_OP_DERIVE, 
        
                                            EVP_PKEY_CTRL_EC_KDF_UKM, len, (void *)(ukm)); 
        
               *p++ = OSSL_PARAM_construct_octet_string(OSSL_EXCHANGE_PARAM_KDF_UKM, 
        
                                                       /* 
        
                                                        * Cast away the const. This is read 
        
                                                        * only so should be safe 
        
                                                        */ 
        
                                                       (void *)ukm, 
        
                                                       (size_t)len); 
        
               *p++ = OSSL_PARAM_construct_end(); 
        
               ret = evp_pkey_ctx_set_params_strict(ctx, params); 
        
               if (ret == -2) { 
        
                   ERR_raise(ERR_LIB_EVP, EVP_R_COMMAND_NOT_SUPPORTED); 
        
                   /* Uses the same return values as EVP_PKEY_CTX_ctrl */ 
        
                   return -2; 
        
               } 
        
               if (ret == 1) 
        
                   OPENSSL_free(ukm); 
        
               return ret; 
        
           } 
        
           int EVP_PKEY_CTX_get0_ecdh_kdf_ukm(EVP_PKEY_CTX *ctx, unsigned char **pukm) 
        
           { 
        
               size_t ukmlen; 
        
               int ret; 
        
               OSSL_PARAM params[3], *p = params; 
        
               ret = evp_pkey_ctx_getset_ecdh_param_checks(ctx); 
        
               if (ret != 1) 
        
                   return ret; 
        
               /* TODO(3.0): Remove this eventually when no more legacy */ 
        
               if (ctx->op.kex.exchprovctx == NULL) 
        
                   return EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_EC, 
        
                                            EVP_PKEY_OP_DERIVE, 
        
                                            EVP_PKEY_CTRL_GET_EC_KDF_UKM, 0, 
        
                                            (void *)(pukm)); 
        
               *p++ = OSSL_PARAM_construct_octet_ptr(OSSL_EXCHANGE_PARAM_KDF_UKM, 
        
                                                     (void **)pukm, 0); 
        
               *p++ = OSSL_PARAM_construct_size_t(OSSL_EXCHANGE_PARAM_KDF_UKM_LEN, 
        
                                                  &ukmlen); 
        
               *p++ = OSSL_PARAM_construct_end(); 
        
               ret = evp_pkey_ctx_get_params_strict(ctx, params); 
        
               if (ret == -2) { 
        
                   ERR_raise(ERR_LIB_EVP, EVP_R_COMMAND_NOT_SUPPORTED); 
        
                   /* Uses the same return values as EVP_PKEY_CTX_ctrl */ 
        
                   return -2; 
        
               } else if (ret != 1) { 
        
                   return -1; 
        
               } 
        
               if (ukmlen > INT_MAX) 
        
                   return -1; 
        
               return (int)ukmlen; 
        
           }

Other places to touch are:

(to "translate" EVP_PKEY_CTRL_SET1_ID and EVP_PKEY_CTRL_GET1_ID to the new functions)

openssl/crypto/evp/pmeth_lib.c

Line 828 in 04cb5ec

static int legacy_ctrl_to_param(EVP_PKEY_CTX *ctx, int keytype, int optype,

(to "translate" 'distid' to whatever OSSL_PARAM name you come up with)

openssl/crypto/evp/pmeth_lib.c

Line 1016 in 04cb5ec

static int legacy_ctrl_str_to_param(EVP_PKEY_CTX *ctx, const char *name,

levitte · 2020-08-09T15:07:47Z

(.. and now backing away to do more vacation ..)

InfoHunter · 2020-08-12T17:05:33Z

I pulled the commits from #11248 into this PR thus we have the signature implementation. So can we close #11248 and review the whole change together in this PR? (This would be more convenient)

mattcaswell · 2020-08-12T17:31:09Z

Makes sense to me

providers/common/der/build.info

crypto/ec/ec_ctrl.c

crypto/sm2/sm2_aid.c

providers/implementations/keymgmt/ec_kmgmt.c

test/evp_extra_test.c

include/openssl/core_names.h

providers/implementations/signature/sm2sig.c

apps/speed.c

openssl-machine · 2020-09-19T13:00:21Z

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.

InfoHunter · 2020-09-21T04:34:58Z

Needs a rebase.

Done rebasing.

mattcaswell · 2020-09-21T08:28:34Z

Was the rebase trivial? If so then I don't see a need to reset the 24hr timer.

InfoHunter · 2020-09-22T02:29:33Z

Was the rebase trivial? If so then I don't see a need to reset the 24hr timer.

Yes, only 2 build.info files

levitte · 2020-09-22T05:50:04Z

To my judgement, there's no need to delay a merge further.

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12536)

mattcaswell · 2020-09-22T07:42:54Z

Repo is frozen due to the release today - but this is master only so I pushed.

InfoHunter commented Jul 27, 2020

View reviewed changes

include/openssl/ec.h Outdated Show resolved Hide resolved

InfoHunter force-pushed the sm2-keymgmt branch from c5f32b4 to 07fe8ca Compare August 1, 2020 14:57

InfoHunter changed the title ~~WIP: Add SM2 key management~~ Add SM2 key management Aug 1, 2020

InfoHunter requested a review from levitte August 1, 2020 16:53

mattcaswell requested changes Aug 3, 2020

View reviewed changes

crypto/ec/ec_key.c Outdated Show resolved Hide resolved

crypto/ec/ec_key.c Outdated Show resolved Hide resolved

providers/implementations/keymgmt/ec_kmgmt.c Outdated Show resolved Hide resolved

providers/implementations/keymgmt/ec_kmgmt.c Outdated Show resolved Hide resolved

levitte mentioned this pull request Aug 5, 2020

OSSL_STORE: Move 'file:' scheme loader to provider #12587

Closed

levitte reviewed Aug 7, 2020

View reviewed changes

providers/implementations/keymgmt/ec_kmgmt.c Outdated Show resolved Hide resolved

levitte reviewed Aug 15, 2020

View reviewed changes

providers/common/der/build.info Outdated Show resolved Hide resolved

InfoHunter mentioned this pull request Aug 17, 2020

Add SM2 signature algorithm to default provider #11248

Closed

2 tasks

levitte requested changes Aug 17, 2020

View reviewed changes

InfoHunter force-pushed the sm2-keymgmt branch from 27cbedc to 948695b Compare August 17, 2020 05:55

levitte reviewed Aug 17, 2020

View reviewed changes

include/openssl/core_names.h Outdated Show resolved Hide resolved

levitte reviewed Aug 17, 2020

View reviewed changes

providers/implementations/signature/sm2sig.c Outdated Show resolved Hide resolved

levitte reviewed Aug 17, 2020

View reviewed changes

providers/implementations/signature/sm2sig.c Outdated Show resolved Hide resolved

InfoHunter force-pushed the sm2-keymgmt branch from 948695b to 5aa4ac7 Compare August 18, 2020 11:32

levitte mentioned this pull request Aug 18, 2020

[WIP, Parked] EVP: Adapt EVP_PKEY_set_alias_type() for provider-native EVP_PKEYs #12675

Closed

levitte reviewed Aug 19, 2020

View reviewed changes

apps/speed.c Outdated Show resolved Hide resolved

Paul Yang and others added 13 commits September 21, 2020 12:30

Add SM2 key management

c8a1612

Add SM2 signature algorithm to default provider

4a82699

fixup! Add SM2 signature algorithm to default provider

8af9b35

fixup! fixup! Add SM2 signature algorithm to default provider

f3087af

fixup! Add SM2 signature algorithm to default provider

5474b69

fixup! Add SM2 signature algorithm to default provider

ec367f9

fixup! fixup! Add SM2 signature algorithm to default provider

c1f7a49

fixup! fixup! Add SM2 signature algorithm to default provider

c0d5db7

Address review comments

6dba08d

support PARAM_SECURITY_BITS for SM2

390e29c

refactor get params functions

c2154f3

fixup! refactor get params functions

dcd7c5b

Add auto-gen SM2 der files into .gitignore

1057c55

InfoHunter force-pushed the sm2-keymgmt branch from 2332f35 to 1057c55 Compare September 21, 2020 04:34

levitte mentioned this pull request Sep 21, 2020

EVP: Enforce that EVP_PKEY_set_alias_type() only works with legacy keys #12920

Closed

levitte approved these changes Sep 22, 2020

View reviewed changes

openssl-machine pushed a commit that referenced this pull request Sep 22, 2020

Add SM2 key management

7ee511d

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12536)

openssl-machine pushed a commit that referenced this pull request Sep 22, 2020

Add SM2 signature algorithm to default provider

d0b79f8

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12536)

openssl-machine pushed a commit that referenced this pull request Sep 22, 2020

Address review comments

b3d267c

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12536)

openssl-machine pushed a commit that referenced this pull request Sep 22, 2020

support PARAM_SECURITY_BITS for SM2

1d03db9

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12536)

openssl-machine pushed a commit that referenced this pull request Sep 22, 2020

refactor get params functions

e9aa4a1

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12536)

openssl-machine pushed a commit that referenced this pull request Sep 22, 2020

Add auto-gen SM2 der files into .gitignore

8e3a64f

Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12536)

mattcaswell closed this Sep 22, 2020

romen mentioned this pull request Oct 7, 2020

Move SM2 digest signature implementation to provider. #10357

Closed

Uh oh!

Comments

Conversation

InfoHunter commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

InfoHunter commented Jul 26, 2020

Uh oh!

Uh oh!

InfoHunter commented Jul 27, 2020

Uh oh!

levitte commented Jul 27, 2020

Uh oh!

InfoHunter commented Aug 1, 2020

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

InfoHunter commented Aug 7, 2020

Uh oh!

InfoHunter commented Aug 7, 2020

Uh oh!

Uh oh!

levitte commented Aug 7, 2020

Uh oh!

InfoHunter commented Aug 8, 2020

Uh oh!

InfoHunter commented Aug 8, 2020

Uh oh!

levitte commented Aug 9, 2020

Uh oh!

levitte commented Aug 9, 2020

Uh oh!

InfoHunter commented Aug 12, 2020

Uh oh!

mattcaswell commented Aug 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openssl-machine commented Sep 19, 2020

Uh oh!

InfoHunter commented Sep 21, 2020

Uh oh!

mattcaswell commented Sep 21, 2020

Uh oh!

InfoHunter commented Sep 22, 2020

Uh oh!

levitte commented Sep 22, 2020

Uh oh!

mattcaswell commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

InfoHunter commented Jul 26, 2020 •

edited

Loading