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

OSSL_DESERIALIZER fixups #12544

Closed

Conversation

@levitte
Copy link
Member

@levitte levitte commented Jul 27, 2020

This fixes both big and small issues with the OSSL_DESERIALIZER library code:

  1. The passphrase handling was modelled after OSSL_SERIALIZER, which is all about encryption and that the caller must specify the cipher to use... but with deserialization being about decryption and that the necessary info comes with the serialized input, this needed to be redone.
  2. Deserializers for all our asymmetric keys are now added, and can handle public keys too. test/serdes_test.c has been modified to test them all.
  3. Refactored the constructor setting functions, and most of all renamed "finalizer" to "constructor" (thanks @t8m, sorry that I lost that in #12410).
  4. Made the deserialization use purely BIO_tell() and BIO_seek() for file rewind... BIO_reset() is hard to use generically. (fixes #12541)

Additionally, there were a few small fixes in other parts of libcrypto.

@paulidale
Copy link
Contributor

@paulidale paulidale commented Jul 29, 2020

Travis is relevant.

Copy link
Contributor

@paulidale paulidale left a comment

Looks pretty good.
Mostly nits.

doc/man3/EVP_PKEY_set1_RSA.pod Outdated Show resolved Hide resolved
doc/man3/EVP_PKEY_set1_RSA.pod Outdated Show resolved Hide resolved
doc/man3/EVP_PKEY_set1_RSA.pod Outdated Show resolved Hide resolved
doc/man3/EVP_PKEY_set1_RSA.pod Outdated Show resolved Hide resolved
doc/man3/OSSL_DESERIALIZER_CTX_new_by_EVP_PKEY.pod Outdated Show resolved Hide resolved
doc/man3/OSSL_DESERIALIZER_from_bio.pod Outdated
OSSL_DESERIALIZER_CTX_set_cleanup(), respectively.

OSSL_DESERIALIZER_export() is a fallback function for constructors that
can't use the data they get directly for diverse reasons. It takes the same

This comment has been minimized.

@paulidale

paulidale Jul 29, 2020
Contributor

s/can't/cannot/

This comment has been minimized.

@levitte

levitte Jul 30, 2020
Author Member

I don't see why... perhaps my English is thwarted by impressions I've had along the way of learning, I dunno, but "cannot" has been used around me to give off a stronger "not"... make it more "imperative" if you will, having the use approach "must not", which is absolutely not what I try to express here.

Oy, subtleties of natural languages...

This comment has been minimized.

@paulidale

paulidale Jul 30, 2020
Contributor

To me can't reads informally with the same meaning. Almost sloppy in documentation.

Another option would be can not.

This comment has been minimized.

@levitte

levitte Jul 30, 2020
Author Member

I may have a different view of whether documentation needs formal writing. I would see such a requirement as a bit snobbish...

This comment has been minimized.

@paulidale

paulidale Jul 30, 2020
Contributor

The ratio of cannot to can't in the docs is over 8:1. I think that's a clear preference.

I'll note that documentation is not written informally. E.g. the insistence on gender neutral pronouns, preferred methods of spelling.

This comment has been minimized.

@levitte

levitte Jul 31, 2020
Author Member

Er, we have not actually decided whether our manuals should be written in formal (contractions allowed) or informal style (no contractions). Different people have silently done it differently (I tend to write with contractions, others... I have frankly not really cared to take note).

That we have chosen to spell certain words correctly, or to use gender neutral language, sure, those are decision points. How that automatically means formal writing all the way, I do not see.

doc/man3/OSSL_DESERIALIZER_from_bio.pod Outdated Show resolved Hide resolved
doc/man3/OSSL_DESERIALIZER_from_bio.pod Outdated Show resolved Hide resolved
doc/man3/OSSL_DESERIALIZER_from_bio.pod Outdated Show resolved Hide resolved
test/serdes_test.c Show resolved Hide resolved
crypto/evp/p_lib.c Outdated Show resolved Hide resolved
@paulidale
Copy link
Contributor

@paulidale paulidale commented Jul 30, 2020

Travis is still relevant 😿

@levitte
Copy link
Member Author

@levitte levitte commented Jul 30, 2020

Travis is still relevant 😿

Travis is annoying at times... but it's right.
I hope I've fixed it.

@paulidale
Copy link
Contributor

@paulidale paulidale commented Jul 30, 2020

I also created a merge conflict :)

levitte added 18 commits Jul 27, 2020
For now, that's what we see being used.  It's possible that we will
have to figure out a way to specific if these should be implicit or
explicit on a case by case basis.
…S_30

This is needed so RSA keys created from different code paths have a
chance to compare as equal.
The OSSL_DESERIALIZER API makes the incorrect assumption that the
caller must cipher and other pass phrase related parameters to the
individual desserializer implementations, when the reality is that
they only need a passphrase callback, and will be able to figure out
the rest themselves from the input they get.

We simplify it further by never passing any explicit passphrase to the
provider implementation, and simply have them call the passphrase
callback unconditionally when they need, leaving it to libcrypto code
to juggle explicit passphrases, cached passphrases and actual
passphrase callback calls.
To be able to implement this, there was a need for the standard
EVP_PKEY_set1_, EVP_PKEY_get0_ and EVP_PKEY_get1_ functions for
ED25519, ED448, X25519 and X448, as well as the corresponding
EVP_PKEY_assign_ macros.  There was also a need to extend the list of
hard coded names that EVP_PKEY_is_a() recognise.

Along with this, OSSL_FUNC_keymgmt_load() are implemented for all
those key types.

The deserializers for these key types are all implemented generically,
in providers/implementations/serializers/deserializer_der2key.c.
… lengths

We use this in test/serdes_test.c, to compare serializations into PEM,
which aren't necessarily terminated with a NUL byte when they were
written to a BIO_s_mem().
It's not the best idea to set a whole bunch of parameters in one call,
that leads to functions that are hard to update.  Better to re-model
this into several function made to set one parameter each.

This also renames "finalizer" to "constructor", which was suggested
earlier but got lost at the time.
…IO_seek()

Depending on the BIO used, using BIO_reset() may lead to "interesting"
results.  For example, a BIO_f_buffer() on top of another BIO that
handles BIO_reset() as a BIO_seek(bio, 0), the deserialization process
may find itself with a file that's rewound more than expected.

Therefore, OSSL_DESERIALIZER_from_{bio,fp}'s behaviour is changed to
rely purely on BIO_tell() / BIO_seek(), and since BIO_s_mem() is used
internally, it's changed to handle BIO_tell() and BIO_seek() better.

This does currently mean that OSSL_DESERIALIZER can't be easily used
with streams that don't support BIO_tell() / BIO_seek().

Fixes #12541
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
…S_30

This is needed so RSA keys created from different code paths have a
chance to compare as equal.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
The OSSL_DESERIALIZER API makes the incorrect assumption that the
caller must cipher and other pass phrase related parameters to the
individual desserializer implementations, when the reality is that
they only need a passphrase callback, and will be able to figure out
the rest themselves from the input they get.

We simplify it further by never passing any explicit passphrase to the
provider implementation, and simply have them call the passphrase
callback unconditionally when they need, leaving it to libcrypto code
to juggle explicit passphrases, cached passphrases and actual
passphrase callback calls.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
To be able to implement this, there was a need for the standard
EVP_PKEY_set1_, EVP_PKEY_get0_ and EVP_PKEY_get1_ functions for
ED25519, ED448, X25519 and X448, as well as the corresponding
EVP_PKEY_assign_ macros.  There was also a need to extend the list of
hard coded names that EVP_PKEY_is_a() recognise.

Along with this, OSSL_FUNC_keymgmt_load() are implemented for all
those key types.

The deserializers for these key types are all implemented generically,
in providers/implementations/serializers/deserializer_der2key.c.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
… lengths

We use this in test/serdes_test.c, to compare serializations into PEM,
which aren't necessarily terminated with a NUL byte when they were
written to a BIO_s_mem().

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
It's not the best idea to set a whole bunch of parameters in one call,
that leads to functions that are hard to update.  Better to re-model
this into several function made to set one parameter each.

This also renames "finalizer" to "constructor", which was suggested
earlier but got lost at the time.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
…IO_seek()

Depending on the BIO used, using BIO_reset() may lead to "interesting"
results.  For example, a BIO_f_buffer() on top of another BIO that
handles BIO_reset() as a BIO_seek(bio, 0), the deserialization process
may find itself with a file that's rewound more than expected.

Therefore, OSSL_DESERIALIZER_from_{bio,fp}'s behaviour is changed to
rely purely on BIO_tell() / BIO_seek(), and since BIO_s_mem() is used
internally, it's changed to handle BIO_tell() and BIO_seek() better.

This does currently mean that OSSL_DESERIALIZER can't be easily used
with streams that don't support BIO_tell() / BIO_seek().

Fixes #12541

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
openssl-machine pushed a commit that referenced this pull request Aug 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #12544)
@levitte levitte deleted the levitte:provider-deserializers-fixups-20200727 branch Aug 4, 2020
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
For now, that's what we see being used.  It's possible that we will
have to figure out a way to specific if these should be implicit or
explicit on a case by case basis.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
…S_30

This is needed so RSA keys created from different code paths have a
chance to compare as equal.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
The OSSL_DESERIALIZER API makes the incorrect assumption that the
caller must cipher and other pass phrase related parameters to the
individual desserializer implementations, when the reality is that
they only need a passphrase callback, and will be able to figure out
the rest themselves from the input they get.

We simplify it further by never passing any explicit passphrase to the
provider implementation, and simply have them call the passphrase
callback unconditionally when they need, leaving it to libcrypto code
to juggle explicit passphrases, cached passphrases and actual
passphrase callback calls.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
To be able to implement this, there was a need for the standard
EVP_PKEY_set1_, EVP_PKEY_get0_ and EVP_PKEY_get1_ functions for
ED25519, ED448, X25519 and X448, as well as the corresponding
EVP_PKEY_assign_ macros.  There was also a need to extend the list of
hard coded names that EVP_PKEY_is_a() recognise.

Along with this, OSSL_FUNC_keymgmt_load() are implemented for all
those key types.

The deserializers for these key types are all implemented generically,
in providers/implementations/serializers/deserializer_der2key.c.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
… lengths

We use this in test/serdes_test.c, to compare serializations into PEM,
which aren't necessarily terminated with a NUL byte when they were
written to a BIO_s_mem().

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
It's not the best idea to set a whole bunch of parameters in one call,
that leads to functions that are hard to update.  Better to re-model
this into several function made to set one parameter each.

This also renames "finalizer" to "constructor", which was suggested
earlier but got lost at the time.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
…IO_seek()

Depending on the BIO used, using BIO_reset() may lead to "interesting"
results.  For example, a BIO_f_buffer() on top of another BIO that
handles BIO_reset() as a BIO_seek(bio, 0), the deserialization process
may find itself with a file that's rewound more than expected.

Therefore, OSSL_DESERIALIZER_from_{bio,fp}'s behaviour is changed to
rely purely on BIO_tell() / BIO_seek(), and since BIO_s_mem() is used
internally, it's changed to handle BIO_tell() and BIO_seek() better.

This does currently mean that OSSL_DESERIALIZER can't be easily used
with streams that don't support BIO_tell() / BIO_seek().

Fixes openssl#12541

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from openssl#12544)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants