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

Serialisation: add a built-in serialisation provider. #12104

Closed
wants to merge 4 commits into from

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Jun 10, 2020

Move the libcrypto serialisation functionality into a place where it can be provided at some point. The serialisation still remains native in the default provider.

Add additional code to the list command to display what kind of serialisation each entry is capable of.

Having the FIPS provider auto load the serialisation provider is a future (but necessary) enhancement.

  • documentation is added or updated
  • tests are added or updated
@paulidale paulidale added this to the 3.0.0 milestone Jun 10, 2020
@paulidale paulidale changed the title Serialisation: add a built-in serialisation provider. WIP: Serialisation: add a built-in serialisation provider. Jun 10, 2020
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 10, 2020

This separates out the serialisation into a separate provider, however it lives in libcrypto and is included in the default provider automatically. The FIPS provider needs to load this when it is loaded and unload it when it is unloaded.

Added a -serializers option to the list app. This includes extra information about the format and type of serialisation supported.

providers/build.info Outdated Show resolved Hide resolved
apps/list.c Outdated Show resolved Hide resolved
@paulidale paulidale force-pushed the paulidale:serialisation branch 2 times, most recently Jun 10, 2020
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 15, 2020

Note: there appears to be a lock order inversion that is trigger by this change.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 15, 2020

The threading issue is reported in #12151 (which is on master).

@paulidale paulidale force-pushed the paulidale:serialisation branch 2 times, most recently Jun 18, 2020
@paulidale paulidale changed the title WIP: Serialisation: add a built-in serialisation provider. Serialisation: add a built-in serialisation provider. Jun 18, 2020
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 18, 2020

Out of WIP.

The threading issue that caused the lock up was a result of a problem with this PR (i.e. the issue existed regardless but was triggered due to a problem here).

The "provider=" properties for the serialisers is fixed as serialization even if it comes from the default provider. I don't see this as being a problem, but if others disagree, it can be fixed easily.

@levitte
Copy link
Member

@levitte levitte commented Jun 18, 2020

The "provider=" properties for the serialisers is fixed as serialization even if it comes from the default provider. I don't see this as being a problem, but if others disagree, it can be fixed easily.

That has me wonder what the meaning is with that particular property. Why have it at all?

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 18, 2020

We've got "provider=XXX" for all other instances. In this case the provider is the serialization provider even if it isn't loaded.

As I mentioned, I'm happy to change this to provider=default.

Regardless, provider=serialization will always be exactly equivalent to provider=default. Examine the output of openssl list --serializations both with and without -provider serialization specified.

doc/man1/openssl-list.pod.in Show resolved Hide resolved
doc/man7/OSSL_PROVIDER-default.pod Outdated Show resolved Hide resolved
@levitte levitte added this to Needs review in 3.0 New Core + FIPS via automation Jun 18, 2020
@levitte
Copy link
Member

@levitte levitte commented Jun 18, 2020

I noticed, with some amusement, the spelling change from "serialise" to "serialize"... I actually thought the 's' spelling was favoured in AU 😉
But ah well, the functionality is spelt with a 'z', so we might as well adopt that everywhere, even though we usually have a slight lean toward current British spelling...

For further amusement: https://english.stackexchange.com/questions/38232/usage-of-z-in-the-word-serialized-in-english

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 18, 2020

English uses 'z' and 's' in -[sz]se pretty much interchangeably, alhtough 's' is generally preferred. American only uses 'z'. My preference is towards 's'.

However, there were plenty of instances using 'z' already in the code, which made standardising on that reasonable. Yes, I used 's' deliberately :)

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 20, 2020

@kaduk
Copy link
Contributor

@kaduk kaduk commented Jun 20, 2020

However, there were plenty of instances using 'z' already in the code, which made standardising on that reasonable. Yes, I used 's' deliberately :)

Reminds me of how yesterday I got a build error in my new code by putting in a check against IV_STATE_UNINITIALIZED when the actual macro uses the 'S' spelling. Perhaps, in the case of macros, we should provide both spellings?

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 21, 2020

That has me wonder what the meaning is with that particular property. Why have it at all?

I'm happy to duplicate the table, I don't really see a need for this property. People want an implementation that conforms to X, the provider should be irrelevant. It was introduced so that the default and FIPS providers could coexist and share the serialisation. I'd vote to remove it -- albeit not as part of this PR.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 21, 2020

I noticed, with some amusement, the spelling change from "serialise" to "serialize"... I actually thought the 's' spelling was favoured in AU 😉

In English, either z or s are allowed in the -i[zs]e word ending. For the most part the s is preferred but both are permissible (with a few exceptions of course, this is English). Australian follows this convention as well but with an ever so slightly stronger influence for the American -ize.

I went through the code and noticed that there were far more serialize (typo checking changed that to the -ise form if that's any indication) than serialise, so I changed things to the former. My opinion is towards the -ise version but not strongly enough to cause a fuss.

That written, I'll happily change all to the -ise form if we want. I think her Majesty's English is what we prefer. Update on Monday.

@levitte
Copy link
Member

@levitte levitte commented Jun 21, 2020

the provider should be irrelevant.

Yes, but if we do specify it, it should be correct.

@levitte
Copy link
Member

@levitte levitte commented Jun 21, 2020

I think her Majesty's English is what we prefer.

Most of us at least 😉

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 21, 2020

Review comments addressed.

Apart from unifying the spelling of all the -ise and -ize words. I'm happy to drop the changes I did in this area so that something with wider coverage can be done in the future.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jun 23, 2020

Ping for review.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 28, 2020

All bar listing the FIPS serialisers fixed.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 28, 2020

doc-nits failure reported by travis sees relevant. The test_verify failure OTOH looks odd. Several failures due to that but I can't see how it is related.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 28, 2020

Looks good except for the travis issues.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 28, 2020

The test_verify failure OTOH looks odd.

This is fixed by #12549

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 28, 2020

Had to miss something. Should be fixed now.

@paulidale paulidale force-pushed the paulidale:serialisation branch Jul 28, 2020
Pauli added 3 commits Jun 10, 2020
Move the libcrypto serialisation functionality into a place where it can
be provided at some point. The serialisation still remains native in the
default provider.

Add additional code to the list command to display what kind of serialisation
each entry is capable of.

Having the FIPS provider auto load the base provider is a future
(but necessary) enhancement.
@paulidale paulidale force-pushed the paulidale:serialisation branch to 49e7e95 Jul 28, 2020
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 28, 2020

I've squashed an rebased the changes -- the build failure was a link fix in the documentation.

I've also added a fixup commit that adds a -deserializers option to the list command and this will require review.

Copy link
Member

@mattcaswell mattcaswell left a comment

I'm slightly confused by the Travis status showing in this PR. It's showing as queued.

However here it is showing as green:
https://travis-ci.com/github/openssl/openssl/builds/177529986

So - approved on that basis.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 29, 2020

Travis showed up twice when I last pushed. Both builds have the same ID (36399).

I've no idea why this happened.

@slontis
Copy link
Contributor

@slontis slontis commented Jul 30, 2020

openssl-machine pushed a commit that referenced this pull request Jul 30, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #12104)
openssl-machine pushed a commit that referenced this pull request Jul 30, 2020
Move the libcrypto serialisation functionality into a place where it can
be provided at some point. The serialisation still remains native in the
default provider.

Add additional code to the list command to display what kind of serialisation
each entry is capable of.

Having the FIPS provider auto load the base provider is a future
(but necessary) enhancement.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #12104)
openssl-machine pushed a commit that referenced this pull request Jul 30, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #12104)
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Jul 30, 2020

Merged to master. Thanks for all the reviews etc.

@paulidale paulidale closed this Jul 30, 2020
3.0 New Core + FIPS automation moved this from Needs review to Done Jul 30, 2020
@paulidale paulidale deleted the paulidale:serialisation branch Jul 30, 2020
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#12104)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Move the libcrypto serialisation functionality into a place where it can
be provided at some point. The serialisation still remains native in the
default provider.

Add additional code to the list command to display what kind of serialisation
each entry is capable of.

Having the FIPS provider auto load the base provider is a future
(but necessary) enhancement.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#12104)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from openssl#12104)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants