Fixes #11241 problems of using openssl x509 -req self sign certificate by Neo-ZK · Pull Request #11242 · openssl/openssl

Neo-ZK · 2020-03-04T05:55:58Z

the problem is illustrated in issue #11241

…certificate

InfoHunter · 2020-03-04T06:11:25Z

apps/x509.c

                }
-
+#ifndef OPENSSL_NO_SM2
+                if (EVP_PKEY_id(Upkey) == EVP_PKEY_SM2) {


This seems not necessary anymore in 3.0.0. I recall @levitte resolved this to automatically set the SM2 tag when loading a key.

fixed in last commit:-)

A side note: the PR @levitte created during the Chinese lunar new year is #10942 ;-)

InfoHunter · 2020-03-04T06:20:24Z

doc/man1/openssl-x509.pod.in


+Convert a SM2 certificate request into a self signed SM2 certificate:
+
+ openssl x509 -req -in careq.pem -extfile openssl.cnf -signkey key.pem \


You need to add the newly added 'sm2-id' and 'sm2-hex-id' in 'SYNOPSIS' and leave descriptions of these options in 'Signing Options' I think.

InfoHunter · 2020-03-04T07:34:46Z

test/recipes/80-test_x509sm2_sign.t

+                "-noout", "-sm3",
+                "-sm2-id", "1234567812345678",
+                "-sigopt", "sm2_id:1234567812345678"])));
+


There is no newline at the end of this file

fixed, and add a new test case about hex_id

Besides, it seems you need to add a test case that verifies the newly generated certificate to see if it's issued correctly

mattcaswell · 2020-03-04T10:29:35Z

The Travis "red cross" seems relevant.

Also please can you supply a CLA:
https://www.openssl.org/policies/cla.html

InfoHunter · 2020-03-04T12:46:10Z

@zk3326312 You need to make doc-nits after making changes to docs

InfoHunter · 2020-03-04T12:47:17Z

I think the options newly added to x509 command should be sm2-id instead of sm2_id

levitte

The special -sm2-id and -sm2-hex-id options should be dropped. Stick with -sigopt.

levitte · 2020-03-04T13:33:58Z

apps/x509.c

+         */
+        return do_X509_sign(x, pkey, digest, sigopts);
+    }
+#endif


Instead if this special case, it should be enough to replace X509_sign(x, pkey, digest) with do_X509_sign(x, pkey, digest, sigopts) below

levitte · 2020-03-04T13:39:08Z

apps/x509.c

+     "Specify an ID string to verify an SM2 certificate request"},
+    {"sm2-hex-id", OPT_SM2HEXID, 's',
+     "Specify a hex ID string to verify an SM2 certificate request"},
+#endif


There is -sigopt already, no need to make the SM2 id special

Actually using sm-id would make code easier reading, if only use sigopt, then have to resolve sm2-id from string "sm2id:...", it seems that openssl does't use this way

The SM2 EVP_PKEY_METHOD recognises sm2_id. You are using it in the test recipe, so you know this.

levitte · 2020-03-04T13:45:19Z

test/recipes/80-test_x509sm2_sign.t

+                "-signkey", srctop_file("test", "certs", "sm2-root.key"),
+                "-noout", "-sm3",
+                "-sm2-id", "1234567812345678",
+                "-sigopt", "sm2_id:1234567812345678"])));


sm2 ID given twice? Why?

The reason is:
only using -sm2-id would not transfer sm2-id when calling sign(x, Upkey, fkey, days, clrext, digest, extconf, extsect, preserve_dates, sigopts), that would cause sign fail

sm2 ID given twice? Why?

That is because the -sm2-id is used for verifying the CSR and the sm2_id (notice the slight difference between stick and underscore...) in the sigopt is used for signing the CSR into a real certificate.

Because the CSR is generated by the user and the final certificate is signed by the issuer, so the ID in the CSR (which also contains an SM2 signature) could be different compared to the final signing one.

So I think this explains the following question as well:

The special -sm2-id and -sm2-hex-id options should be dropped

Another choice is to add something like vfyopt to the x509 command...

Another choice is to add something like vfyopt to the x509 command...

That would probably be a good thing

So the conclusion is that we should implement a vfyopt in this pr? :-)

What about other openssl commands? Are there options like vfyopt?

There seems to be no similar option

levitte · 2020-03-04T13:45:28Z

test/recipes/80-test_x509sm2_sign.t

+                "-signkey", srctop_file("test", "certs", "sm2-root.key"),
+                "-noout", "-sm3",
+                "-sm2-hex-id", "31:32:33:34:35:36:37:38:31:32:33:34:35:36:37:38",
+                "-sigopt", "sm2_hex_id:31:32:33:34:35:36:37:38:31:32:33:34:35:36:37:38"])));


Again, twice? Why?

InfoHunter · 2020-03-04T16:18:10Z

Furthermore, I think the initial commit's title is too long and there is not message body in it - but we can resolve this when the PR gets merged by anyone who is doing the squash job.

mattcaswell · 2020-03-04T18:05:55Z

Close/reopen to kick CLA bot.

levitte · 2020-03-05T06:44:26Z

FYI, I'm having a closer look at this. I'm still convinced that the use of -sigopts must be enough to set the SM2 id during signature, but something fails when I try that, so I'm trying to figure out what.

levitte · 2020-03-05T06:54:38Z

apps/x509.c

+            X509_REQ_set0_sm2_id(req, v);
+        }
+#endif
        i = X509_REQ_verify(req, pkey);


So it seems that what's really missing is a do_X509_REQ_verify that takes a sigopts stack...

Yes, it seems that problem is how to set an sm2-id to a X509_REQ with sig-opt

But that loses the ability of setting different IDs for CSR and final signing separately.

@william-zk, it seems to go deeper, there's no X509_REQ_verify_ctx() either, so support seems to be lacking quite deeply. I'm preparing a PR that adds such support.

@InfoHunter, sorry, you lost me there... are you essentially asking for vfyopts? That would certainly be possible...

@levitte , so what you meant was adding 'vfyopts' and then use its content in something like do_X509_REQ_verify ?

levitte · 2020-03-05T11:51:07Z

I see where this PR is coming from... we have the same kind of SM2 hackery all over the place already. I have some ideas on how to make it... a little less hacky. PR coming up!

InfoHunter · 2020-03-10T02:12:06Z

I see where this PR is coming from... we have the same kind of SM2 hackery all over the place already. I have some ideas on how to make it... a little less hacky. PR coming up!

Any update on this?

levitte · 2020-03-10T10:33:26Z

Any update on this?

So far, an issue where I've written down my analysis of the actual problem, #11293. I'll follow up with a PR shortly.

levitte · 2020-03-16T03:11:48Z

@william-zk, thank you for this PR. I can understand if it felt like a waste, but considering how it helped highlight problematic code, I see it as highly valuable, even if we didn't use it directly.

Neo-ZK · 2020-03-16T03:15:29Z

@william-zk, thank you for this PR. I can understand if it felt like a waste, but considering how it helped highlight problematic code, I see it as highly valuable, even if we didn't use it directly.

oh, that's all right:-), I'd very happy to see that there is a more effective way to solve the problem. Later I'd help to find if there is any problem in openssl x509 ca...，if problem exist, I would make a issue and help to fix it

Fixes issue 11241, problems of using openssl x509 -req self sign sm2 …

0daa8fa

…certificate

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Mar 4, 2020

InfoHunter reviewed Mar 4, 2020

View reviewed changes

add test case

55791ad

Neo-ZK force-pushed the dev_fixup_x509_req_sm2 branch from bb3e706 to 55791ad Compare March 4, 2020 07:08

InfoHunter reviewed Mar 4, 2020

View reviewed changes

fix up problems of comments

92f911d

levitte requested changes Mar 4, 2020

View reviewed changes

mattcaswell closed this Mar 4, 2020

mattcaswell reopened this Mar 4, 2020

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Mar 4, 2020

Neo-ZK force-pushed the dev_fixup_x509_req_sm2 branch 2 times, most recently from 6582ffd to 5e700d8 Compare March 5, 2020 02:45

fix up pod problems, add new test case

c830177

Neo-ZK force-pushed the dev_fixup_x509_req_sm2 branch from 5e700d8 to c830177 Compare March 5, 2020 03:26

levitte reviewed Mar 5, 2020

View reviewed changes

levitte mentioned this pull request Mar 10, 2020

The special treatment of sm2_id shouldn't be necessary #11293

Closed

Neo-ZK closed this Mar 16, 2020


		Convert a SM2 certificate request into a self signed SM2 certificate:

		openssl x509 -req -in careq.pem -extfile openssl.cnf -signkey key.pem \

Uh oh!

Comments

Conversation

Neo-ZK commented Mar 4, 2020

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!

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!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Mar 4, 2020

Uh oh!

InfoHunter commented Mar 4, 2020

Uh oh!

InfoHunter commented Mar 4, 2020

Uh oh!

levitte left a comment

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!

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InfoHunter commented Mar 4, 2020

Uh oh!

mattcaswell commented Mar 4, 2020

Uh oh!

levitte commented Mar 5, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Mar 5, 2020

Uh oh!

InfoHunter commented Mar 10, 2020

Uh oh!

InfoHunter Mar 4, 2020 •

edited

Loading