enable CMS sign/verify for provider-implemented PKEYs #17733

baentsch · 2022-02-18T13:25:53Z

Fixes ~~(part of)~~ #17717

Note: More work might be required for further CMS functionality. This PR covers issue 2 reported in #17717 and all that is being used and tested in the OQS project: Generating CMS signed data and verification of that with non-classic/provider-implemented algorithms back-and-forth between an (OQS-enabled)OpenSSL 1.1.1 fork and the OpenSSL3 oqs-provider.

A more complete OpenSSL-only CMS issue, possibly calling for a complete external provider test suite, might be called for to cover all CMS functionality.

beldmit · 2022-02-18T13:37:27Z

Is it possible to integrate your provider tests as part of external tests as it is currently done with GOST engine?

t8m · 2022-02-18T13:44:13Z

crypto/cms/cms_sd.c

+        /* Digest presence check really necessary? */
+        if (alg1 == NULL || alg1->algorithm == NULL)
+            return -1;


Perhaps there can be algorithms that integrate the digest, i.e. there is no separate digest algorithm?

Yes -- that exactly is the case for OQS algorithms (see issue 1 in #17717). However, the whole OpenSSL CMS logic completely depends on a digest being present. When reading into the CMS RFC the standard seems to mandate this, so the comment above is gone in 72d6281 as this check makes sense for code implementing the CMS standard (which might need amendment for quantum-safe crypto but we can't solve this here...). This also means that this PR completely fixes #17717 as issue 1 there is explained by this.

t8m · 2022-02-18T13:44:53Z

crypto/cms/cms_sd.c

+            return -1;
+
+        /* Generic PKEY->name->NID->OID mapping check */
+        const char* typename = EVP_PKEY_get0_type_name(pkey);


The declaration has to be at the beginning of the block. Also space should be before * not after.

Thx. Fixed in 72d6281.

crypto/cms/cms_sd.c

beldmit · 2022-02-18T13:58:56Z

Is it possible to use your provider for external tests (similar to gost-engine)?

baentsch · 2022-02-19T10:04:08Z

Is it possible to use your provider for external tests (similar to gost-engine)?

That sounds like a good idea and I don't see a reason why not -- it's a shared lib after all. Is there documentation how this is done for the gost-engine / what needs to be done to set this up?

levitte · 2022-02-19T18:41:22Z

crypto/cms/cms_sd.c

+        /* Generic PKEY->name->NID->OID mapping check */
+        if (typename == NULL)
+            return -1;
+        return X509_ALGOR_set0(alg2, OBJ_nid2obj(OBJ_sn2nid(typename)), V_ASN1_UNDEF, NULL);


I suggest replacing OBJ_nid2obj(OBJ_sn2nid(typename)) with OBJ_txt2obj(typename, 0)

Done in e2f1215.

beldmit · 2022-02-19T18:42:33Z

@baentsch please take a look at
https://github.com/openssl/openssl/blob/master/test/recipes/95-test_external_gost_engine.t
gost-engine is checked out as a submodule, built and run its internal cipher suite

baentsch · 2022-02-20T06:52:27Z

please take a look at
https://github.com/openssl/openssl/blob/master/test/recipes/95-test_external_gost_engine.t
gost-engine is checked out as a submodule, built and run its internal cipher suite

Thanks for that pointer. Cool: the limitations there are the same as for oqsprovider (no Windows/VMS). Will use as blueprint (aiming to mirror https://github.com/openssl/openssl/blob/master/test/README-external.md#gost-engine-test-suite) and bring as separate PR: OK for you, @beldmit?

beldmit · 2022-02-20T06:55:02Z

Sure. As there currently are no external providers with brand new algorithm, any additional test will be useful and can help stabilising the functionality

crypto/cms/cms_sd.c

crypto/objects/obj_xref.c

mattcaswell · 2022-03-01T09:00:58Z

Looks good now. Should there be some kind of test for this?

t8m · 2022-03-01T09:52:30Z

Looks good now. Should there be some kind of test for this?

At least given the code is now also used for DSA and ECDSA, it will be tested if we have tests for CMS using DSA and/or ECDSA.

And we seem to have some in the test_cms recipe.

mattcaswell · 2022-03-01T09:54:33Z

At least given the code is now also used for DSA and ECDSA, it will be tested if we have tests for CMS using DSA and/or ECDSA.

But the point of the change is to allow other algorithms (especially ones without an associated hash alg) to work. Should there be a test for this scenario?

baentsch · 2022-03-02T08:28:25Z

Yes, please

Done.

openssl-machine · 2022-03-03T11:00:14Z

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

mattcaswell · 2022-03-03T11:17:35Z

this PR has failing CI tests

bot is confused. Added the label.

We need to handle signatures with and without digest algs and we generalize the ossl_cms_ecdsa_dsa_sign() function to other algorithms that are handled in the same way. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17733)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17733)

mattcaswell · 2022-03-03T13:25:30Z

It seems this has been merged so closing. Thanks for the contribution.

We need to handle signatures with and without digest algs and we generalize the ossl_cms_ecdsa_dsa_sign() function to other algorithms that are handled in the same way. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit d15d561)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit 0654421)

We need to handle signatures with and without digest algs and we generalize the ossl_cms_ecdsa_dsa_sign() function to other algorithms that are handled in the same way. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit d15d561)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit 0654421)

We need to handle signatures with and without digest algs and we generalize the ossl_cms_ecdsa_dsa_sign() function to other algorithms that are handled in the same way. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit d15d561)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit 0654421)

We need to handle signatures with and without digest algs and we generalize the ossl_cms_ecdsa_dsa_sign() function to other algorithms that are handled in the same way. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17733) (cherry picked from commit d15d561)

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17733) (cherry picked from commit 0654421)

We need to handle signatures with and without digest algs and we generalize the ossl_cms_ecdsa_dsa_sign() function to other algorithms that are handled in the same way. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733)

enable CMS sign/verify for provider-implemented PKEYs

4eeb579

t8m requested changes Feb 18, 2022

View reviewed changes

t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Feb 18, 2022

addressing PR review feedback

72d6281

baentsch requested a review from t8m February 19, 2022 13:36

baentsch mentioned this pull request Feb 19, 2022

CMS (interop) tests open-quantum-safe/oqs-provider#49

Merged

levitte reviewed Feb 19, 2022

View reviewed changes

improving OBJ_ function use (PR feedback)

e2f1215

baentsch requested a review from levitte February 20, 2022 08:26

t8m approved these changes Feb 25, 2022

View reviewed changes

t8m added the approval: review pending This pull request needs review by a committer label Feb 25, 2022

mattcaswell requested changes Feb 25, 2022

View reviewed changes

crypto/cms/cms_sd.c Show resolved Hide resolved

crypto/cms/cms_sd.c Outdated Show resolved Hide resolved

handle signatures with and without digest algs

35cf0e7

baentsch requested a review from mattcaswell February 26, 2022 22:13

t8m requested changes Feb 28, 2022

View reviewed changes

crypto/cms/cms_sd.c Outdated Show resolved Hide resolved

crypto/cms/cms_sd.c Outdated Show resolved Hide resolved

crypto/cms/cms_sd.c Outdated Show resolved Hide resolved

mattcaswell requested changes Feb 28, 2022

View reviewed changes

crypto/cms/cms_sd.c Outdated Show resolved Hide resolved

crypto/objects/obj_xref.c Outdated Show resolved Hide resolved

corrections for PR feedback

6a61720

t8m approved these changes Feb 28, 2022

View reviewed changes

mattcaswell reviewed Feb 28, 2022

View reviewed changes

crypto/objects/obj_xref.c Show resolved Hide resolved

comments updated as per PR feedback [skip ci]

082012b

Testing OBJ_find_sigid_by_algs

5ad1217

baentsch requested a review from mattcaswell March 2, 2022 08:26

t8m approved these changes Mar 2, 2022

View reviewed changes

mattcaswell approved these changes Mar 2, 2022

View reviewed changes

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 Mar 2, 2022

mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 3, 2022

openssl-machine pushed a commit that referenced this pull request Mar 3, 2022

Add testing of OBJ_find_sigid_by_algs()

0654421

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17733)

mattcaswell closed this Mar 3, 2022

baentsch mentioned this pull request Mar 8, 2022

adding external oqsprovider testing #17832

Closed

slontis mentioned this pull request Jul 22, 2022

Can a OpenSSL release that contains PR's that exist in master but not in the 3.0.X branch be created? #18846

Open

baentsch mentioned this pull request Oct 15, 2022

CMS working with external provider? #17717

Closed

t8m pushed a commit to t8m/openssl that referenced this pull request Nov 4, 2022

Add testing of OBJ_find_sigid_by_algs()

c2c1bf9

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit 0654421)

t8m pushed a commit to t8m/openssl that referenced this pull request Nov 4, 2022

Add testing of OBJ_find_sigid_by_algs()

b1e464b

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit 0654421)

t8m pushed a commit to t8m/openssl that referenced this pull request Nov 9, 2022

Add testing of OBJ_find_sigid_by_algs()

5a7d743

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#17733) (cherry picked from commit 0654421)

openssl-machine pushed a commit that referenced this pull request Nov 11, 2022

Add testing of OBJ_find_sigid_by_algs()

0f3de2f

Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17733) (cherry picked from commit 0654421)

kamlesh0hrs mentioned this pull request May 22, 2023

CMS signing fails (Openssl is not able to use oqsprovider) if CMS_NOATTR is specified in CMS_add1_signer open-quantum-safe/oqs-provider#169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable CMS sign/verify for provider-implemented PKEYs #17733

enable CMS sign/verify for provider-implemented PKEYs #17733

baentsch commented Feb 18, 2022 •

edited

beldmit commented Feb 18, 2022

t8m Feb 18, 2022

baentsch Feb 19, 2022

t8m Feb 18, 2022

baentsch Feb 19, 2022

beldmit commented Feb 18, 2022

baentsch commented Feb 19, 2022

levitte Feb 19, 2022

baentsch Feb 20, 2022

beldmit commented Feb 19, 2022

baentsch commented Feb 20, 2022

beldmit commented Feb 20, 2022

mattcaswell commented Mar 1, 2022

t8m commented Mar 1, 2022

mattcaswell commented Mar 1, 2022

baentsch commented Mar 2, 2022

openssl-machine commented Mar 3, 2022

mattcaswell commented Mar 3, 2022

mattcaswell commented Mar 3, 2022

enable CMS sign/verify for provider-implemented PKEYs #17733

enable CMS sign/verify for provider-implemented PKEYs #17733

Conversation

baentsch commented Feb 18, 2022 • edited

beldmit commented Feb 18, 2022

t8m Feb 18, 2022

Choose a reason for hiding this comment

baentsch Feb 19, 2022

Choose a reason for hiding this comment

t8m Feb 18, 2022

Choose a reason for hiding this comment

baentsch Feb 19, 2022

Choose a reason for hiding this comment

beldmit commented Feb 18, 2022

baentsch commented Feb 19, 2022

levitte Feb 19, 2022

Choose a reason for hiding this comment

baentsch Feb 20, 2022

Choose a reason for hiding this comment

beldmit commented Feb 19, 2022

baentsch commented Feb 20, 2022

beldmit commented Feb 20, 2022

mattcaswell commented Mar 1, 2022

t8m commented Mar 1, 2022

mattcaswell commented Mar 1, 2022

baentsch commented Mar 2, 2022

openssl-machine commented Mar 3, 2022

mattcaswell commented Mar 3, 2022

mattcaswell commented Mar 3, 2022

baentsch commented Feb 18, 2022 •

edited