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

[WIP] Add new provider encoders implementations for more output standards #13095

Closed
wants to merge 4 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Oct 8, 2020

The current encoder implementations only supported PKCS#8 output.
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):

  • RAW: this corresponds to what the type specific i2d_TYPEPrivateKey(),
    i2d_TYPEPublicKey(), PEM_write_bio_TYPEPrivateKey() and
    PEM_write_bio_TYPEPublicKey() produce.
  • RAWKEY: this corresponds to what the function old_priv_encode() in
    EVP_PKEY_ASN1_METHOD produces, and thus the "old" / "traditional"
    output of i2d_PrivateKey() and PEM_write_bio_PrivateKey_traditional().
  • 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 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().

In the OSSL_ALGORITHM tables, these diverse OSSL_DISPATCH arrays can
be re-used with different "structure={name}" properties for maximum
expressiveness if that's desirable. There is an example where the
OSSL_DISPATCH arrays ossl_rsa_to_RAW_der_encoder_functions[] and
ossl_rsa_to_RAW_pem_encoder_functions[] are used twice, once with the
property "structure=raw" and once with the property "structure=pkcs1".

For our internal uses, "structure=raw", "structure=rawkey",
"structure=pkcs8" and "structure=SubjectPublicKeyInfo" will be 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.

@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Oct 8, 2020
@levitte levitte added this to the 3.0.0 beta1 milestone Oct 8, 2020
@levitte levitte added this to In progress in 3.0 New Core + FIPS via automation Oct 8, 2020
@levitte levitte changed the title [WIP, Pending on #13094]Add new provider encoders implementations for more output standards [WIP, Pending on #13094] Add new provider encoders implementations for more output standards Oct 8, 2020
@levitte
Copy link
Member Author

levitte commented Oct 8, 2020

The remaining work is to adjust the OSSL_ENCODER library code... it doesn't deal with the multitude of encoder implementations as I had imagine.

@levitte
Copy link
Member Author

levitte commented Oct 8, 2020

I forgot... encoders for PVK and MSBLOB are still missing, those will be added too.

@levitte levitte mentioned this pull request Oct 8, 2020
@kaduk
Copy link
Contributor

kaduk commented Oct 8, 2020

Having both "RAW" and "RAWKEY" seems like it would be confusing.
Using the name "RAW" also seems to presume that there is a single privileged format for a given key type (and that that privileged format is the same one that the old APIs use), which is not something I'm fully confident about without doing some research.

@levitte
Copy link
Member Author

levitte commented Oct 9, 2020

Having both "RAW" and "RAWKEY" seems like it would be confusing.

You may be right.

The difference I thought I saw was that DH doesn't have old_priv_encode support in crypto/dh/dh_ameth.c, but that there are i2d_DHPrivateKey and i2d_DHPublicKey functions... that was the single reason I had two different names, and entirely for our internal uses (for example for i2d_PrivateKey to work as previously in OSSL_ENCODER terms).

However, now that you made me look again, it turns out that I was wrong, there are no i2d_DHPrivateKey and i2d_DHPublicKey functions, so that takes away all my reasons for RAWKEY.

Using the name "RAW" also seems to presume that there is a single privileged format for a given key type (and that that privileged format is the same one that the old APIs use), which is not something I'm fully confident about without doing some research.

The single privileged format going forward is PKCS8 for private keys and SubjectPublicKeyInfo for public keys. That's a move that OpenSSL has pushed for (albeit slowly) for years. You'll note that X25519, X448, ED25519 and ED448 don't have any i2d_ functionality... that's by design, they can only be encoded through the generic functions i2d_PrivateKey, i2d_PUBKEY, PEM_write_bio_PrivateKey and PEM_write_bio_PUBKEY.

"RAW" (and "RAWKEY") is meant to be an internal name, made for the needs of libcrypto code where it needs to be key type agnostic. For the sake of the users that wants to know how to replace any i2d_TYPEPrivateKey or i2d_TYPEPublicKey call, we need to have more expressive "structure={name}" names, but a "structure=raw" can also work. For more expressive names, help is welcome. I did add a "structure=pkcs1" for RSA keys, because obviously, but haven't spent time trying to come up with good names for DSA and EC.

So in summary, I'll take away RAWKEY as soon as I've verified that it is, indeed, not at all necessary.

@kaduk
Copy link
Contributor

kaduk commented Oct 9, 2020

The single privileged format going forward is PKCS8 for private keys and SubjectPublicKeyInfo for public keys. That's a move that OpenSSL has pushed for (albeit slowly) for years.

Sorry, I was a bit subtle. I would feel uncomfortable asserting this just as the belief of openssl, and would prefer if there was a clear universal (e.g., standards-body) sense of the privileged format.

@levitte
Copy link
Member Author

levitte commented Oct 9, 2020

Sorry, I was a bit subtle. I would feel uncomfortable asserting this just as the belief of openssl, and would prefer if there was a clear universal (e.g., standards-body) sense of the privileged format.

I agree that the message from OpenSSL has been much too subtle in that regard. It's related to the push for using EVP (and therefore EVP_PKEY) and what i2d and PEM_write functions that take an EVP_PKEY do. The most visible that I can find for the moment is in the manual for PEM_write_PrivateKey:

The PrivateKey functions read or write a private key in PEM format
using an EVP_PKEY structure. The write routines use PKCS#8 private key
format and are equivalent to PEM_write_bio_PKCS8PrivateKey().The read
functions transparently handle traditional and PKCS#8 format encrypted
and unencrypted keys.

Interestingly, nothing is said about PUBKEY, which also handle an EVP_PKEY. They uniformly output a SubjectPublicKeyInfo structure.

@levitte
Copy link
Member Author

levitte commented Oct 9, 2020

The discussion about RAW and RAWKEY had me thinking that the structures I've laid out need a redesign...

The current encoder implementations only supported PKCS#8 output.
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):

-   RAW: this corresponds to what the type specific i2d_TYPEPrivateKey(),
    i2d_TYPEPublicKey(), PEM_write_bio_TYPEPrivateKey() and
    PEM_write_bio_TYPEPublicKey() produce.
-   RAWKEY: this corresponds to what the function old_priv_encode() in
    EVP_PKEY_ASN1_METHOD produces, and thus the "old" / "traditional"
    output of i2d_PrivateKey() and PEM_write_bio_PrivateKey_traditional().
-   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 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().

In the OSSL_ALGORITHM tables, these diverse OSSL_DISPATCH arrays can
be re-used with different "structure={name}" properties for maximum
expressiveness if that's desirable.  There is an example where the
OSSL_DISPATCH arrays ossl_rsa_to_RAW_der_encoder_functions[] and
ossl_rsa_to_RAW_pem_encoder_functions[] are used twice, once with the
property "structure=raw" and once with the property "structure=pkcs1".

For our internal uses, "structure=raw", "structure=rawkey",
"structure=pkcs8" and "structure=SubjectPublicKeyInfo" will be 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.
…ndards

Rethink the set of offered encoder implementations.  The original commit
message needs a rewrite.
@levitte levitte changed the title [WIP, Pending on #13094] Add new provider encoders implementations for more output standards [WIP] Add new provider encoders implementations for more output standards Oct 12, 2020
@slontis
Copy link
Member

slontis commented Oct 14, 2020

Are there decoder related changes also?

We have code like this in the ec app.. (Does the decoder need a selector?)

if (informat == FORMAT_ASN1) {
        if (pubin)
            eckey = d2i_EC_PUBKEY_bio(in, NULL);
        else
            eckey = d2i_ECPrivateKey_bio(in, NULL);
    } else if (informat == FORMAT_ENGINE) {
        EVP_PKEY *pkey;
        if (pubin)
            pkey = load_pubkey(infile, informat, 1, passin, e, "public key");
        else
            pkey = load_key(infile, informat, 1, passin, e, "private key");
        if (pkey != NULL) {
            eckey = EVP_PKEY_get1_EC_KEY(pkey);
            EVP_PKEY_free(pkey);
        }
    } else {
        if (pubin)
            eckey = PEM_read_bio_EC_PUBKEY(in, NULL, NULL, NULL);
        else
            eckey = PEM_read_bio_ECPrivateKey(in, NULL, NULL, passin);
    }

@levitte
Copy link
Member Author

levitte commented Oct 15, 2020

Are there decoder related changes also?

Getting there...

@levitte
Copy link
Member Author

levitte commented Oct 16, 2020

So, for this, @kaduk raised a point, that "RAW" was perhaps not the best name for something that's essentially libcrypto internal to be able to support i2d_PrivateKey()...

So regarding how to pick what structure should be used, I have moved away from using properties, for the same reasons that the output type isn't picked with properties (bottom line is that our current fetching model is hard when we want to fetch multiple implementations in one go, so much so that trying that changes the model quite radically, or leads to very ugly hacks). Instead, I'm adding another parameter ("output structure") that can be set in the OSSL_ENCODER_CTX, and using that more or less the same way as the output type.

Now, it comes down to tell OSSL_ENCODER_CTX_new_by_EVP_PKEY() that it should use a key type specific structure, when that's what is desired. "raw" was the structure name I came up with first, and after @kaduk's remark, I switch to something more expressive, "type-specific"... however, that seems just as wrong as "raw". The other thought is that NULL could be valid input, and essentially mean "whatever makes sense to the implementation" or "whatever is the default specifically for this type". So for an RSA key with the output type "DER" or "PEM", NULL as output structure would result in a PKCS#1 structure, while "pkcs8" would result in a PKCS#8 structure. (for output types like "MSBLOB" or "PVK", the output structure is irrelevant, there is only one)

Without having seen the code, does what I say above make some kind of sense?
(and yes, either way, this means that the OSSL_ENCODER API is changing a little bit, specifically OSSL_ENCODER_CTX_new_by_EVP_PKEY() gets another argument)

@slontis
Copy link
Member

slontis commented Oct 16, 2020

What you are describing now is that raw = default. So NULL is fine for this if it is documented.

@levitte
Copy link
Member Author

levitte commented Oct 16, 2020

What you are describing now is that raw = default. So NULL is fine for this if it is documented.

Yup, that's about it. Then it's just up to the existing libcrypto to do the right thing for what's expected, either use NULL or a choice of "pkcs8" and "SubjectPublicKeyInfo" (that's a mouthful, would anyone object to "spki"?)

@mattcaswell
Copy link
Member

NULL as output structure would result in a PKCS#1 structure, while "pkcs8" would result in a PKCS#8 structure.

Wouldn't it be better the other way around, i.e NULL means PKCS8?

@levitte
Copy link
Member Author

levitte commented Oct 16, 2020

Wouldn't it be better the other way around, i.e NULL means PKCS8?

The heart of the problem is that functions like i2d_PrivateKey() need to be able to say "I want whatever is specific for the EVP_PKEY I got" in a uniform way, i.e. it shouldn't have to have special knowledge about the diverse key types. This is needed to be able to continuing to output what it does with an EVP_PKEY_ASN1_METHOD that implements old_priv_encode. So while NULL could certainly mean PKCS#8, we must then invent a uniform structure name for the key types that have encoders for it. "type-specific" is one that I'm currently playing with.

However, NULL might not be a good choice for "whatever is specific for the key type"... I'm thinking of it in OSSL_DECODER terms, and there should be a way to tell the decoders "whatever, you figure it out from the input", and NULL would be a good value for that, and there should be a way to tell OSSL_DECODER that a key type specific structure is expected. NULL can't be used simultaneously for both meanings. So I think I'll settle on "type-specific" for internal usage (that would help when adapting d2i_PrivateKey().

@mattcaswell
Copy link
Member

So while NULL could certainly mean PKCS#8, we must then invent a uniform structure name for the key types that have encoders for it. "type-specific" is one that I'm currently playing with.

In general I think we should be encouraging PKCS#8 over the type specific structures. So if you want a type specific structure you should have to ask explicitly for it. Using NULL wouldn't be great for that....it's too easy to use NULL because "I don't know what the value should be, and NULL seems to work".

@levitte
Copy link
Member Author

levitte commented Oct 16, 2020

... and what do we do when the next standard to supersede PKCS#8 comes along? What should NULL mean then? Would users expect an automagic switch at that time? If that's the case, doesn't NULL really mean "whatever" (which is more relevant for decoders than encoders in a DER/PEM context)

@levitte
Copy link
Member Author

levitte commented Oct 17, 2020

Killing this in favor of #13167

@levitte levitte closed this Oct 17, 2020
3.0 New Core + FIPS automation moved this from In progress to Done Oct 17, 2020
@levitte levitte deleted the more-encoders branch October 17, 2020 11:03
@paulnelsontx paulnelsontx added this to review pending in 3.0.0 estimator Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: review pending This pull request needs review by a committer branch: master Merge to master branch
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants