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

Add new provider encoders implementations for more output standards, take 2 #13167

Closed
wants to merge 35 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Oct 17, 2020

The current encoder implementations only supported PKCS#8 output for private keys and SubjectPublicKeyInfo for public keys. However, if we are to deprecate key type specific i2d_ and PEM_write_ functions in favor of using OSSL_ENCODER, the users need an easy enough migration path to do what they've done so far.

The encoders are now enhanced to cover most of those cases, and are classified like this (visible in the OSSL_DISPATCH array names):

  • type-specific: this corresponds to what the type specific i2d_TYPEPrivateKey(), i2d_TYPEPublicKey(), i2d_TYPEparams() (or as the case may be i2d_TYPEParameters()), PEM_write_bio_TYPEPrivateKey(), PEM_write_bio_TYPEPublicKey() and PEM_write_bio_TYPEParameters() produce.
  • PKCS8: this corresponds to what the function priv_encode() in EVP_PKEY_ASN1_METHOD produces, and thus the fallback (when there's no old_priv_encode() defined) output of i2d_PrivateKey(), as well as the normal output output of PEM_write_bio_PrivateKey().
  • SubjectPublicKeyInfo: this corresponds to the what the function pub_encode() in EVP_PKEY_ASN1_METHOD produces, and thus the output of i2d_PUBKEY() and PEM_write_bio_PUBKEY().

Corresponding entries are added to the appropriate OSSL_ALGORITHM tables.

To make it possible for the user as well is libcrypto internals to specify precisely what kind of structure is desired, the OSSL_ENCODER API has been enhanced to handle an output structure parameter alongside the output type. OSSL_ENCODER_CTX_new_by_EVP_PKEY() has been modified to reflect this.

It is permitted to use NULL for the output structure for the cases where that's not relevant (which is the case for MSBLOB and PVK output types), or the caller doesn't care.

For our internal uses, the output structure types "type-specific", "pkcs8" and "SubjectPublicKeyInfo" are used uniformly. That allows us to map deprecated functionality to the corresponding OSSL_ENCODER calls without having to have special knowledge of the more standardised structure names for each key type in libcrypto. Additional structure names are only added for the convenience of the users that may wish to use OSSL_ENCODER directly and use structure names that suit them better.


NOTES:

This PR replaces #13095, and depends on #13166 and #13140
This PR does not implement MSBLOB and PVK encoders, that's left to be done in another PR.
This PR does not implement a separate DER->PEM encoder, that's left to be done in another PR.

@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Oct 17, 2020
@levitte levitte added this to the 3.0.0 beta1 milestone Oct 17, 2020
@levitte levitte added this to In progress in 3.0 New Core + FIPS via automation Oct 17, 2020
@levitte
Copy link
Member Author

levitte commented Oct 17, 2020

A note on #13140:

Without that PR, the new test test/encoder_legacy_test.c exhibits the following errors:

    # INFO:  @ ../levitte-deprecate-harder/test/encoder_legacy_test.c:347
    # Test OSSL_ENCODER against PEM_write_bio_{TYPE}params for DH
    # ERROR: (string) 'str_provided == str_legacy' failed @ ../levitte-deprecate-harder/test/encoder_legacy_test.c:208
    # --- str_provided
    # +++ str_legacy
    #    0:- '-----BEGIN DH PARAMETERS-----.MIICCgKCAQEA1i8yhKZX0RtLEAptBKtRVE'
    #    0:+ '-----BEGIN DH PARAMETERS-----.MIICDgKCAQEA1i8yhKZX0RtLEAptBKtRVE'
    #                                           ^                             
    #   64:  'enKKecXPemJUIg0iRuIExno7Tmm8o8.F2a2gWEYOLK0O6tAnOiCJMo0X9uLqTIk8'
    #  128:  'W5VfaSZx64V7540kX6OSLQqfxwD/d/F.3hEoa++Lf56Gu1CMCYDLDhZPZtUkU0an'
    #  192:  'TSNunUumt7N2a6b2pmpqdkBmIZD/2sac.FPiydetRv+ixKN+0Tqe0Bwr+XbVs7jO'
    #  256:  'HxFC7JXvMFvVGNEVzVA7oAFNbSq/FJ7ty.OJAso9IJv0oIzbuPi0SNWScuSZ35Ia'
    #  320:  'oBEvpPkLJ/iRANqi8U2CpQ8jsl+OWriTOm.gU35JaiO8s6syq0QEkJUwkKwRo+cv'
    #  384:  '7m1PwKCAQEAjEv8wXqVLqGdACXH51FBnniQ.cMNo2uh+ixvRzvmi2kTEU4tvf8QP'
    #  448:  'VfCp1VuBFXU+V6PEXrzfDbUnxp5tDSFfsP3j.sLZPLUiFtzt1YoBypecj9DrvS4H'
    #  512:  'xXeKbZq3WnvuKUhx9ByY3686TeCmea91xEdkF.vSJJxDqCyT0pbPGsU66Z8Prhhv'
    #  576:  'xHGdOb6dNr5HS5PcFWrx023Ax3DcSCBhGy9il3.0AlqqGqI1t1PSLKUcnTMt4W0Z'
    #  640:  'vYSWRuua13ry80u3eMGM+c52ed5xChrkWwaqPpP.hWMLPfK0sWy2dIjUdHvFrGSa'
    #  704:- 'x0feZ2ieqoifC0mxR2RFsa9p/oyI5keMtF1pcQ==.-----END DH PARAMETERS-'
    #  704:+ 'x0feZ2ieqoifC0mxR2RFsa9p/oyI5keMtF1pcQIC.ANs=.-----END DH PARAME'
    #                                               ^^ ^^^^^^^^^^^^^^^^^^^^^^^
    #  768:- '----.'
    #  768:+ 'TERS-----.'
    #         ^^^^^
    # 
    # INFO:  @ ../levitte-deprecate-harder/test/encoder_legacy_test.c:390
    # Test OSSL_ENCODER against i2d_{TYPE}params for DH
    # ERROR: (memory) 'der_provided == der_legacy' failed @ ../levitte-deprecate-harder/test/encoder_legacy_test.c:291
    # --- der_provided
    # +++ der_legacy
    # 0000:-3082020a02820101 00d62f3284a657d1 1b4b100a6d04ab51 5447a728a79c5cf7
    # 0000:+3082020e02820101 00d62f3284a657d1 1b4b100a6d04ab51 5447a728a79c5cf7
    #             ^^                                                           
    # 0020: a6254220d2246e20 4c67a3b4e69bca3c 1766b681611838b2 b43bab409ce88224
    # 0040: ca345fdb8ba93224 f16e557da499c7ae 15ef9e34917e8e48 b42a7f1c03fddfc5
    # 0060: de11286bef8b7f9e 86bb508c0980cb0e 164f66d5245346a7 4d236e9d4ba6b7b3
    # 0080: 766ba6f6a66a6a76 40662190ffdac69c 14f8b275eb51bfe8 b128dfb44ea7b407
    # 00a0: 0afe5db56cee3387 c450bb257bcc16f5 46344573540ee800 535b4aafc527bb72
    # 00c0: 38902ca3d209bf4a 08cdbb8f8b448d59 272e499df921aa01 12fa4f90b27f8910
    # 00e0: 0daa2f14d82a50f2 3b25f8e5ab8933a6 814df925a88ef2ce accaad10124254c2
    # 0100: 42b0468f9cbfb9b5 3f02820101008c4b fcc17a952ea19d00 25c7e751419e7890
    # 0120: 70c368dae87e8b1b d1cef9a2da44c453 8b6f7fc40f55f0a9 d55b8115753e57a3
    # 0140: c45ebcdf0db527c6 9e6d0d215fb0fde3 b0b64f2d4885b73b 75628072a5e723f4
    # 0160: 3aef4b81f15de29b 66add69efb8a521c 7d072637ebce9378 299e6bdd7111d905
    # 0180: bd2249c43a82c93d 296cf1ac53ae99f0 fae186fc4719d39b e9d36be474b93dc1
    # 01a0: 56af1d36dc0c770d c4820611b2f62977 d0096aa86a88d6dd 4f48b2947274ccb7
    # 01c0: 85b466f612591bae 6b5debcbcd2edde3 0633e739d9e779c4 286b916c1aa8fa4f
    # 01e0: 85630b3df2b4b16c b67488d4747bc5ac 649ac747de67689e aa889f0b49b14764
    # 0200:-45b1af69fe8c88e6 478cb45d6971
    # 0200:+45b1af69fe8c88e6 478cb45d69710202 00db
    # 
    not ok 1 - iteration 1

The explanation is that the DH parameter generation in provider/implementations/keymgmt/dh_kmgmt.c doesn't set the length (and neither do the EVP_PKEY_METHOD generators), but when creating a downgraded copy of the key, the DH_set_pqg() call sets the length in the legacy key parameters, and thereby make the output differ

@levitte levitte mentioned this pull request Oct 17, 2020
@levitte levitte changed the title [Pending on #13140, #13166] Add new provider encoders implementations for more output standards, take 2 [Pending on #13140] Add new provider encoders implementations for more output standards, take 2 Oct 19, 2020
crypto/asn1/i2d_evp.c Outdated Show resolved Hide resolved
doc/man7/provider-encoder.pod Outdated Show resolved Hide resolved
crypto/encode_decode/encoder_lib.c Outdated Show resolved Hide resolved
@kaduk
Copy link
Contributor

kaduk commented Oct 19, 2020

Since I had commented on #13095 I felt obligated to take at least a quick look. The overall approach seems reasonable (i.e., better than 13095), though I didn't look in terribly close detail. I agree with Tomáš that OSSL_ENCODER_CTX_prune_encoders() should probably be an implementation detail -- furthermore, is there any case where ending up with n>1 encoders left is a success case?

I will leave with one potentially controversial thought: the "type-specific" output structure seems to be organized solely by the legacy of historic openssl behavior. Perhaps "legacy" would be appropriate to include as part of the name, since we want to try to normalize on pkcs8 usage going forward?

@levitte
Copy link
Member Author

levitte commented Oct 23, 2020

furthermore, is there any case where ending up with n>1 encoders left is a success case?

In the end? Yes, the idea is that it should be possible to get a chain, i.e. key_to_DER -> DER_to_PEM. We currently don't have that in our encoders, but I do plan to refactor that, in a separate PR.

@levitte
Copy link
Member Author

levitte commented Oct 23, 2020

I will leave with one potentially controversial thought: the "type-specific" output structure seems to be organized solely by the legacy of historic openssl behavior. Perhaps "legacy" would be appropriate to include as part of the name, since we want to try to normalize on pkcs8 usage going forward?

The "old", "historic" and "legacy" terms have been argued against, with the argument that standards like PKCS#1 aren't legacy, so I'm trying hard to not ruffle those feathers yet again. "type-specific" is the best I could come up with as a general mnemonic that doesn't imply lesser validity.

As for enforcing PKCS#8, you'll note that PEM_write_bio_PrivateKey() does that unconditionally.

For OSSL_ENCODER and OSSL_DECODER, this is much more in the hands of the caller, i.e. they are in control no matter what. We can encourage them to use PKCS#8 (or whatever we fancy in the future), but we can't force anything. Let's not forget that since this depends on the providers, the user may use whatever provider they want with whatever structures that one supports. All we can control is how we use OSSL_ENCODER and OSSL_DECODER internally and in our apps.

@kaduk
Copy link
Contributor

kaduk commented Oct 23, 2020

furthermore, is there any case where ending up with n>1 encoders left is a success case?

In the end? Yes, the idea is that it should be possible to get a chain, i.e. key_to_DER -> DER_to_PEM. We currently don't have that in our encoders, but I do plan to refactor that, in a separate PR.

Ah, thanks. The "chain" part didn't make it through my thick skull, and I was naively assume some kind of parallel outputs, which obviously doesn't work well with the API surface in question.

@t8m
Copy link
Member

t8m commented Oct 26, 2020

The CI failure is relevant

@levitte
Copy link
Member Author

levitte commented Oct 26, 2020

The CI failure is relevant

Some of it yeah... some of it is because #13140 needs to go in first

@levitte levitte force-pushed the more-encoders-2 branch 4 times, most recently from 297d25d to 4e20ebe Compare October 27, 2020 14:15
@levitte levitte changed the title [Pending on #13140] Add new provider encoders implementations for more output standards, take 2 Add new provider encoders implementations for more output standards, take 2 Oct 27, 2020
@levitte levitte changed the title Add new provider encoders implementations for more output standards, take 2 [WIP] Add new provider encoders implementations for more output standards, take 2 Oct 28, 2020
@levitte
Copy link
Member Author

levitte commented Oct 28, 2020

Back to WIP to work on removing OSSL_ENCODER_CTX_prune_encoders()

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Nov 9, 2020
@slontis slontis 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 Nov 9, 2020
@h-vetinari
Copy link
Contributor

Do the following things need to be tracked for 3.0.0 or even beta1?

  • This PR does not implement MSBLOB and PVK encoders, that's left to be done in another PR.
  • This PR does not implement a separate DER->PEM encoder, that's left to be done in another PR.
  • #define OSSL_STRUCTURE_NAME_PKCS1 "pkcs1" etc.

@romen romen added the triaged: OTC evaluated This issue/pr was triaged by OTC label Nov 10, 2020
@openssl-machine
Copy link
Collaborator

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.

@paulidale paulidale 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 Nov 10, 2020
@paulidale
Copy link
Contributor

These items should be tracked for beta 1.

openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
OSSL_FUNC_encoder_does_selection() is a dispatchable encoder implementation
function that should return 1 if the given |selection| is supported by an
encoder implementation and 0 if not.  This can be used by libcrypto
functionality to figure out if an encoder implementation should be
considered or not.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
OSSL_ENCODER_CTX_new_by_EVP_PKEY() takes one more argument to express
the desired outermost structure for the output.

This also adds OSSL_ENCODER_CTX_prune_encoders(), which is used to
reduce the stack of encoders found according to criteria formed from
the combination of desired selection, output type and output
structure.

squash! ENCODER: Add output structure support for EVP_PKEY encoding

Replace the paragraph talking about OSSL_ENCODER_CTX_prune_encoders() with:

The encoding processor encoder_process() is enhanced with better
analysis of the stack of encoder implementations.  To avoid having to
keep an on the side array of information, it uses recursion.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
The base functionality to implement the keypair encoders doesn't
change much, but this results in a more massive amount of
OSSL_DISPATCH and OSSL_ALGORITHM arrays, to support a fine grained
selection of implementation based on what parts of the keypair
structure (combinations of key parameters, public key and private key)
should be output, the output type ("TEXT", "DER" or "PEM") and the
outermost output structure ("pkcs8", "SubjectPublicKeyInfo", key
type specific structures, ...).

We add support for the generic structure name "type-specific", to
allow selecting that without knowing the exact name of that structure.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
This also modifies i2d_PublicKey() and i2d_KeyParams() to support
provided keys.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
openssl-machine pushed a commit that referenced this pull request Nov 11, 2020
The FIPS provider module doesn't have any encoders, the base provider
is needed for that.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #13167)
@levitte
Copy link
Member Author

levitte commented Nov 11, 2020

Merged

45da4a0 CORE: Add support for specifying the outermost object structure
8a98a50 ENCODER: Add support for specifying the outermost output structure
cd861ab ENCODER: Add support for OSSL_FUNC_encoder_does_selection()
b9a2afd ENCODER: Add output structure support for EVP_PKEY encoding
0b9f90f ENCODER: Add tracing
c319b62 PROV: Re-implement all the keypair encoders
4227e50 Adapt libcrypto functionality to specify the desired output structure
973a52c test/endecode_test.c: Update to specify output structures
f49d486 test/evp_libctx_test.c: use OSSL_ENCODER instead of i2d_PublicKey()
122e81f test/recipes/30-test_evp_libctx.t: use fips-and-base.cnf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch triaged: OTC evaluated This issue/pr was triaged by OTC
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

9 participants