Skip to content

Comments

OSSL_DESERIALIZER: New API for provider based deserializers#12410

Closed
levitte wants to merge 34 commits intoopenssl:masterfrom
levitte:provider-deserializers
Closed

OSSL_DESERIALIZER: New API for provider based deserializers#12410
levitte wants to merge 34 commits intoopenssl:masterfrom
levitte:provider-deserializers

Conversation

@levitte
Copy link
Member

@levitte levitte commented Jul 9, 2020

Deserializing may be a complex thing. Input needs to be recognised and then dealt with, possibly in multiple steps (PEM -> DER, DER -> RSA).

The deserializers should be possible to pick very precisely, and should also be searchable and combinable in chains.

To allow precise picking, we base the "algorithm name" on the product that is desired ("RSA" for an RSA key), and a gettable algorithm parameter "input" specifying the expected input by name (for example, "DER" if DER is expected). This input name can then be used to find deserializers that produces the desired input (in this example, serializers named "DER"). That's how they get chained together.

On the provider side, the deserializing function feeds back the deserialized data to the caller through an OSSL_CALLBACK function. The possible parameters are:

  • "data-type" - the value is a UTF8 string that describes the content type. This is optional, but may be passed by some deserializers, for example a PEM to DER deserializer.
  • "data" - the value is an octet string that is the contents of the deserialized object, to be dealt by the caller as they see fit. In a deserializer chain, libcrypto code will simply pass this on to the next deserializer in the chain.
  • "reference" - the value is an octet string that can be used as an object reference with a select number of other functions in the same provider (further explained below)

In case that's needed, the deserializer also has an export function, which takes an object reference, and exports that object's parameters via a callback, exactly like the KEYMGMT export function. This can be used to transfer an object to a different provider, should the need arise.

What's that object reference?

An object reference is a reference to the actual desired object. The exact contents of the reference itself depends entirely on the provider. It could be a UUID for all we care...

With this, the KEYMGMT set of functions has gained a new one, OSSL_FUNC_keymgmt_load(), which takes a references and returns the corresponding keydata. The combined EVP_KEYMGMT and keydata can then be wrapped in an EVP_PKEY and used.

In OpenSSL providers, that's actually simply an indirect pointer to the object, and allows OSSL_FUNC_keymgmt_load() to be called from inside a deserializer callback call, and to "take over" the object held by the deserializer. The result is that we avoid having to do some tricky memory management to align DESERIALIZER code and KEYMGMT code, all we need is [ahem] an easy pointer trick.

@levitte
Copy link
Member Author

levitte commented Jul 9, 2020

This becomes a fundament for the OSSL_STORE work, which has stalled gravely in #9389

@slontis
Copy link
Member

slontis commented Jul 10, 2020

Will review on Monday..

@levitte
Copy link
Member Author

levitte commented Jul 10, 2020

Will review on Monday..

Cool, that'll give me time to work out the kinks I missed

@levitte
Copy link
Member Author

levitte commented Jul 13, 2020

I've had some thoughts on the public API, which is currently a bit too incomplete, so this is temporarly WIP.

Comments are still welcome

@levitte levitte changed the title OSSL_DESERIALIZER: New API for provider based deserializers [WIP] OSSL_DESERIALIZER: New API for provider based deserializers Jul 13, 2020
@levitte levitte force-pushed the provider-deserializers branch from ccfb7e6 to 4048c4d Compare July 15, 2020 00:39
@levitte levitte changed the title [WIP] OSSL_DESERIALIZER: New API for provider based deserializers OSSL_DESERIALIZER: New API for provider based deserializers Jul 15, 2020
@levitte
Copy link
Member Author

levitte commented Jul 15, 2020

Ok, now I got something that I'm satisfied with, and that makes this fairly easy to extend.

Unfortunately, it turns out that the SYNOPSIS parser in util/find-doc-nits is lacking... I have an ongoing fix, which has exposed quite a lot of SYNOPSISes that are a bit lacking, so that'll be a separate PR.

@slontis
Copy link
Member

slontis commented Jul 15, 2020

Unfortunately, it turns out that the SYNOPSIS parser in util/find-doc-nits is lacking... I have an ongoing fix, which has exposed quite a lot of SYNOPSISes that are a bit lacking, so that'll be a separate PR.

@levitte - maybe you could delegate that to someone- so that you can do some more important tasks?
Happy to help..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulidale Is there a place where these should be documented?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well these are a mouthful - wouldnt it be better to keep it consistent with other methods?
i.e.
OSSL_DESERIALIZER_FINAL *final;
OSSL_DESERIALIZER_CLEANUP *cleanup;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not related the way you seem to assume. But sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm backing off from that change for now. If you read the manual on the subject, the text is quite fluent when written in terms of a finalizer... I'm not quite sure how to rewrite that using final with the same kind of flow.

Perhaps we need another term entirely instead of finalize? Like I said, this is nowhere near the meaning of "final" as seen in EVP.

Copy link
Member

@t8m t8m Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor? As in object construction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... "constructor" is less a mouthful than "finalizer"??? 😉

But, other than that, it is a viable option. But boy, those function names are getting long...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant it as being more descriptive not less mouthful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the up_ref call failed.. Should the err: path be doing OSSL_DESERIALIZER_free ??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do note that the err path does that on deser_inst->deser, which is assigned further down

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just use deser instead of deser_inst->deser in quite a few places.

Copy link
Member

@slontis slontis Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code could be a good place to start adding error stacks to :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like it should be

    if (deser_inst->deserctx != NULL)
        deser->freectx(deser_inst->deserctx);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have already asserted that deser is not null.
The value of deser_inst->deser is irrelevant to the freectx.

The value is newed via

deser_inst->deserctx = deser_inst->deser->newctx(provctx)

So that is what should determine if the freectx is called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just like to say that this function is not easy to understand. I will come back to it when I have browsed further, and have figured out where it is used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... although, those names are already a mouthful, I tried to keep it short. Ideas are certainly welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use "DESER" like you do for the variables? (And "_instance" as a prefix can probably just be removed.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So prefix everything with OSSL_DESER instead of OSSL_DESERIALIZE... for consistency, we should probably change OSSL_SERIALIZE to OSSL_SER, then... it doesn't strike me as the most intuitive prefixes, but it is a viable option if we can all agree that the less readable trumps the mouthful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I have some refactoring thoughts for the serializer API as well, so a name change would simply become part of that)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, I cannot pronounce serializer deserializer without breaking my tongue 😄
Have you considered exporter/importer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richsalz, I didn't mean to disqualify your suggestion. I find the function names I came up with excruciatingly long to type, but the naming was stuck in my mind, so I do appreciate suggestions to get unstuck.

Serialize / parse sounds like an odd pair, though. But, makes me think of pack / unpack as an option (although I would probably confuse the two, so maybe not the best choice).

@t8m, export / import could actually be viable! OSSL_EXPORT and OSSL_IMPORT, I kinda like the ring of that. However, it might be confusing when we thing of the export / import functionality in KEYMGMT... Still, that might be a possibility.
(also, since the KEYMGMT export / import functionality convert internal data to / from OSSL_PARAM, it is essentially a serialization / deserialization... That might help)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to apologize, glad to help unstick. Just make sure it also didn't dislodge any perl knowledge :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah! No chance

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the shorter name too. I don't think most users would have a problem with OSSL_IMPORT/EXPORT. The KEYMGMT import/export is mostly hidden in the provider boundary.

Kind of similar to the user facing EVP_*_get_params calls versus the get_params calls providers use.

swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
This makes the following functions available for libcrypto code:

evp_keymgmt_util_try_import()  - callback function
evp_keymgmt_util_assign_pkey() - assigns keymgmt and keydata to an EVP_PKEY
evp_keymgmt_util_make_pkey()   - creates an EVP_PKEY from keymgmt and keydata

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
This function is used to create a keydata for a key that libcrypto
only has a reference to.

This introduces provider references, the contents which only the
provider know how to interpret.  Outside of the provider, this is just
an array of bytes.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
This adds a method OSSL_DESERIALIZER, a deserializer context and basic
support to use a set of serializers to get a desired type of data, as
well as deserializer chains.

The idea is that the caller can call OSSL_DESERIALIZER_CTX_add_serializer()
to set up the set of desired results, and to add possible chains, call
OSSL_DESERIALIZER_CTX_add_extra().  All these deserializers are pushed
on an internal stack.

The actual deserialization is then performed using functions like
OSSL_DESERIALIZER_from_bio().  When performing deserialization, the
inernal stack is walked backwards, keeping track of the deserialized
data and its type along the way, until the data kan be processed into
the desired type of data.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
EVP_PKEY is the fundamental type for provider side code, so we
implement specific support for it, in form of a special context
constructor.

This constructor looks up and collects all available KEYMGMT
implementations, and then uses those names to collect deserializer
implementations, as described in the previous commit.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
This test revolves around a central function that will first serialize
an EVP_PKEY, then deserialize the result into a new EVP_PKEY and
compare the two.

The following tests are currently implemented:

1.  EVP_PKEY (RSA) -> DER, then DER -> EVP_PKEY (RSA).
2.  EVP_PKEY (RSA) -> PEM, then PEM -> EVP_PKEY (RSA).
    This one exercises deserializer chains, as we know that there is a
    PEM -> DER and a DER -> EVP_PKEY (RSA) deserializer, but no direct
    PEM -> EVP_PKEY (RSA) deserializer.

Additionally, a small fix in test_fail_string_common(), as strcmp()
could run past a buffer if one of the strings isn't terminated with
a null byte within the given length.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Serialization will only encrypt, so there's no point telling
OSSL_SERIALIZER_CTX_set_passphrase_cb() that's going to happen.

We fix the declaration of OSSL_DESERIALIZER_CTX_set_passphrase_cb()
the same way.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
This implements these functions:

OSSL_DESERIALIZER_CTX_set_cipher()
OSSL_DESERIALIZER_CTX_set_passphrase()
OSSL_DESERIALIZER_CTX_set_passphrase_ui()
OSSL_DESERIALIZER_CTX_set_passphrase_cb()

To be able to deal with multiple deserializers trying to work on the
same byte array and wanting to decrypt it while doing so, the
deserializer caches the passphrase.  This cache is cleared at the end
of OSSL_DESERIALIZER_from_bio().

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
This adds variants of already existing tests, but where the object
is encrypted / decrypted along the way as well.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
…ncryption

This adds legacy PEM variants of already existing tests.

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Shane Lontis <[email protected]>
(Merged from openssl#12410)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants