Skip to content

Conversation

levitte
Copy link
Member

@levitte levitte commented Oct 3, 2019

This is in response to #10070

The idea is to rework our provider build by place all algorithm implementations in a static library and let each provider main (fipsprov.c, defltprov.c, legacyprov.c) get what they like from it by simply using the symbols supplied by the implementations.

So far, the following is submitted:

  • Changes to Configure and build file templates to allow libraries to be "linked" with other static libraries through build.info SOURCE[] statements. This will allow the static library with implementations to be included into any built-in provider (such as the default provider).

  • Rearrangement of the provider build.info files to create static libraries for the algorithm implementations. These are:

    1. libimplementations.a, containing everything that's FIPS agnostic. That's essentially all the code from providers/common/{ciphers,digests,exchange,kdfs,keymgmt,macs,signature}, /providers/default/{ciphers,digests,kdfs,macs} and providers/legacy/digests.
    2. libnonfips.a, containing any special support that the implementations need when used with non-FIPS providers. This should be a fairly minimal library, as those providers are linked with libcrypto, or built in. With this library, FIPS_MODE is undefined.
    3. libfips.a, containing any special support that the implementations need when used with the FIPS provider. This is a bit more substantial, and will include all the bits from libcrypto that need to be built specially for the FIPS module to make it self contained. With this library, FIPS_MODE is defined.
  • There are two places among the implementations where FIPS_MODE is checked. These are two small code sections taht only set a couple of constants, so I'll move them to source files of their own, which will be used both for libfips.a and libnonfips.a, thereby having them compiled twice, once with FIPS_MODE defined and once with it undefined.

  • Moved around the provider source code as follows:

    1. providers/implementations now contains all implementations. The build.info files in there are made in such a way that "moving" an implementation to the legacy provider is changing the its goal variable.
    2. Code that is common for all implementations, regardless of which provider they end up in, remains within providers/common, and its object files are collected in a separate library, providers/libcommon.a, which is then used by all the providers.
    3. providers/default and providers/legacy are emptied and gone. The provider mains have moved up, i.e. providers/defltprov.c and providers/legacyprov.c. Since the FIPS provider is composed or more than one file, I've left them alone for now.
    4. providers/common/include/internal has been renamed to providers/common/include/prov, so we're now including the provider header files like this: #include "prov/header.h"
    5. provider_algs.h is renamed to implementations.h and is located in providers/implementations/include/prov.
    6. The utility code other than the common cipher and digest coded remains in providers/common, and is used as source for both libfips.a and libnonfips.a.

Summary - In practical terms:

[This will be updated as this PR progresses]

  • All building block code that is used by all our providers is now collected in providers/libcommon.a.
  • All the object files from algorithm implementation code in providers/common/, providers/default/ and providers/legacy/ are now collected in providers/libimplementations.a, except for the two nuggest of FIPS_MODE sensitive code.
  • The two nuggest of FIPS_MODE sensitive code have been placed in their own source, and are built into two object files each, one that is collected in providers/libfips.a (for which FIPS_MODE is defined) and one that is collected in providers/libnonfips.a (for which FIPS_MODE is undefined).
  • All the object files from code in crypto/ that are built a second time specially for the FIPS module (in other words, with FIPS_MODE defined) and got used directly to build provider/fips.so are now collected in providers/libfips.a.
  • The FIPS provider module is built from providers/fips/fipsprov.c and providers/fips/selftest.c and linked with providers/libimplementations.a, providers/libcommon.a and providers/libfips.a
  • The Legacy provider module is built from providers/legacy/legacyprov.c and linked with providers/libimplementations.a, providers/libcommon.a, providers/libnonfips.a and libcrypto.
  • The Default provider is built into libcrypto, so all that it needs is essentially added to libcrypto, including the same object files that were collected in providers/libimplementations.a, providers/libcommon.a and in providers/libnonfips.a, and finally the object file built from providers/default/defltprov.c.
    (this may seem unnecessary, but it avoids having the algorithm implementations getting built twice, once for libcrypto and once for providers/libimplementations.a, which is needed anyway)
    (also, should we ever want to make the default provider a separate module, it's a turnkey change)

To do:

  • Rework providers/fipsprov.c as a templates, so it can use a selection of algorithms, as given by configuration. [needs more thought]

@levitte levitte added the branch: master Merge to master branch label Oct 3, 2019
@levitte levitte force-pushed the reorganize-providers branch from 033bb71 to 3111716 Compare October 3, 2019 23:48
@richsalz
Copy link
Contributor

richsalz commented Oct 4, 2019

Either I am confused, or I just don't see how this works. Using the the AESNI assembly version(s) of AES as an example, can you explain where the code goes? Is it a single source file compiled into two libraries? What about the filter/checksum approach that's already defined?

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

The AES assembler stuff ends up in crypto/aes/build.info's $COMMON, and as you can see for yourself, they end up in providers/libfips.a, i.e. constitute "support" for the implementations in providers

I haven't forgotten about the checksum part. There will be a db with the necessary information. That will also tell exactly what algo implementations may be included in the FIPS module.

@richsalz
Copy link
Contributor

richsalz commented Oct 4, 2019

And the several-dozen crypto implementation files that use FIPS_MODE? Which libraries do they go into? Use crypto/ec/ecp_nistp224.c as an example.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

Same story. Basically, anything that ends up in providers/libfips.a is built with FIPS_MODE defined

@richsalz
Copy link
Contributor

richsalz commented Oct 4, 2019

So FIPS version of ecp_nist224p.c ends up in libfips, but where does the non-FIPS version end up?

As I read the intro comment, it seems a bit misleading since "libfips is more substantial." Maybe, but there are several dozen files with FIPS_MODE ifdef's. They all get compiled twice into two libraries, but it's not clear where.

(To forestall one reply, I think a high-level description is needed, not just reading the details of the build.info files and trying to reverse-engineer what's going on.)

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

I have to ask, is "and will include all the bits from libcrypto that need to be built specially for the FIPS module to make it self contained" unclear? How can I express that better?

@richsalz
Copy link
Contributor

richsalz commented Oct 4, 2019

By saying where all the non-fips parts of libcrypto go? And not implying that this will be small?

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

For the non-fips providers the same source ends up built into libcrypto, same as usual.

@richsalz
Copy link
Contributor

richsalz commented Oct 4, 2019

Or are you depending on a FIPS module linking fips.a nonfips.a in that order will pick up the FIPS implementation?

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

No.

The FIPS module is built on libimplementations.a + libfips.a + fipsprov.c + selftest.c. Nothing else.
The legacy module is built on libimplementations.a + libnonfips.a + legacyprov.c and is linked with libcrypto.
The default provider is built in, so is essentially like the legacy provider.

@t8m
Copy link
Member

t8m commented Oct 4, 2019

Great work, Richard. This should work for the downstream FIPS validation fine. And hopefully it will not complicate the upstream OpenSSL FIPS module validation much.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

From the interaction with @richsalz last night, I realised that the description was lacking a bit. I've added a practical summary, I hope that clarifies things.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

Open question

Should implementations from the legacy provider end up in a separate static library, say providers/liblegacy.a, instead of provider/libimplementations.a? While that takes away the Legacy provider from the idea of "a menu of implementations" (which is one of the objectives from #10070), it also ensures that the legacy implementations are not part of "the menu of implementations" and can thereby not end up in any other provider by mistake, or suddenly have an implementation that is not yet considered legacy.

Making a switch from libimplementations.a to liblegacy.a will be made easy, it will be done in one place and one place only (a suitable variable in the appropriate build.info)

Update: it would be silly to have a liblegacy.a just for the legacy provider, so if we separate out all legacy implementations, they can just as well be added directly to the Legacy provider module, without going through a static library.

Update 2: However, the legacy implementations source should still be located in the same directory structure as the non-legacy ones, to avoid having to move around files as implementations age. This is another objective from #10070

@t8m
Copy link
Member

t8m commented Oct 4, 2019

This makes sense to me as well.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

Yeah, it seems like a sane way forward, doesn't it? Is there any reason to have the Legacy provider use the "menu of implementations" idea, i.e. to make the included algorithms selectable? That would be a reason to collect them in liblegacy.a. If not, directly into the module it is.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

(is it just me, or are we kinda removing all the no- options to disable algorithms in the process? I kind like that...)

@mattcaswell
Copy link
Member

Yeah, it seems like a sane way forward, doesn't it? Is there any reason to have the Legacy provider use the "menu of implementations" idea, i.e. to make the included algorithms selectable? That would be a reason to collect them in liblegacy.a. If not, directly into the module it is.

Yeah, it would be kind of cool to make the decision to move an algorithm from default to legacy a simple change that could be done at Configure time. So while we might make decisions about what algorithms get moved out to legacy or not, it gives downstream the opportunity to be more or less aggressive.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

Yeah, it would be kind of cool to make the decision to move an algorithm from default to legacy a simple change that could be done at Configure time.

Wait what? That's not at all what I'm proposing. What I'm proposing is that in a build.info, we can have something like this:

$DSA_GOAL=../../libimplementation.a

SOURCE[$DSA_GOAL]=dsa.c

and when we decide it's legacy, all that will be needed is to change the variable:

$DSA_GOAL=../../legacy

BUT, what I'm proposing is that it could be configurable what things we consider legacy that will actually end up in providers/legacy.so.

What you seem to propose is that legacy implementations should end up in providers/libimplementations.a and that what ends up in which provider is selectable in configuration, i.e. you're basically answering "no" to my open question. I'm cool with that, but it does carry a higher risk of mistakes.

@mattcaswell
Copy link
Member

Ah, ok. I'm fine with your approach too.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

I think I'll go for the providers/legacy.a idea. Having a static library like that allows us to make smarter build.info files. providers/build.info could have the following:

IF[{- !$disabled{legacy} -}]
  LIBS{noinst}=liblegacy.a
  INCLUDE[liblegacy.a]=.. ../include common/include
  IF[{- $disabled{module} -}]
    SOURCE[../libcrypto]=liblegacy.a
    DEFINE[liblegacy.a]=STATIC_LEGACY
    DEFINE[../libcrypto]=STATIC_LEGACY
  ELSE
    MODULES=legacy
    DEPEND[legacy]=liblegacy.a ../libcrypto
    IF[{- defined $target{shared_defflag} -}]
      SOURCE[legacy]=legacy.ld
      GENERATE[legacy.ld]=../util/providers.num
    ENDIF
  ENDIF
ENDIF

Then, any build.info file that carries instructions for legacy algorithms can simply have this:

$WHATEVER_GOAL=../../liblegacy.a

The rest will work itself out regardless of if the Legacy provider becomes a separate module or built in.

@levitte levitte force-pushed the reorganize-providers branch from 82d2fe7 to d1a6c2b Compare October 4, 2019 09:36
@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

Looking at providers/common/include, there are a few things I'd like to change:

  1. Have providers/implementations/include/prov rather than providers/implementations/include/internal and have us include "prov/blah,h". This goes along the lines of keeping separate namespaces for different inclusions that came about with The Big Header Reorganization.
  2. Not have the ciphers subdirectory for inclusion.

Anyone for? Against?

@levitte levitte force-pushed the reorganize-providers branch 2 times, most recently from 3842634 to ffbb0f1 Compare October 4, 2019 10:28
@richsalz
Copy link
Contributor

richsalz commented Oct 4, 2019

This seems much cleaner. It should have a more detailed write-up somewhere, probably in the design doc, but FWIW I support this.

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

I have done enough with this for today... if CIs agree, it could actually be approved as is, and leave the templating stuff for the next PR

@levitte
Copy link
Member Author

levitte commented Oct 4, 2019

Description updated.

Added functionality to use static libraries as source for other
libraries.  When done this way, the target library will use the object
files from the sourced static libraries, making the sourced libraries
work as "containers" for object files.

We also need to make sure that the Unix Makefile template knows how to
deal with shared libraries and modules that depend on static libraries.
That's new situation we haven't had before.
We put almost everything in these internal static libraries:

libcommon               Block building code that can be used by all
                        our implementations, legacy and non-legacy
                        alike.
libimplementations      All non-legacy algorithm implementations and
                        only them.  All the code that ends up here is
                        agnostic to the definitions of FIPS_MODE.
liblegacy               All legacy implementations.

libnonfips              Support code for the algorithm implementations.
                        Built with FIPS_MODE undefined.  Any code that
                        checks that FIPS_MODE isn't defined must end
                        up in this library.
libfips                 Support code for the algorithm implementations.
                        Built with FIPS_MODE defined.  Any code that
                        checks that FIPS_MODE is defined must end up
                        in this library.

The FIPS provider module is built from providers/fips/*.c and linked
with libimplementations, libcommon and libfips.

The Legacy provider module is built from providers/legacy/*.c and
linked with liblegacy, libcommon and libcrypto.
If module building is disabled, the object files from liblegacy and
libcommon are added to libcrypto and the Legacy provider becomes a
built-in provider.

The Default provider module is built-in, so it ends up being linked
with libimplementations, libcommon and libnonfips.  For libcrypto in
form of static library, the object files from those other libraries
are simply being added to libcrypto.
From providers/common/ to providers/implementations/
From providers/default/ to providers/implementations/
From providers/{common,default,legacy}/ to providers/implementations/
However, providers/common/digests/digest_common.c stays where it is,
because it's support code rather than an implementation.

To better support all kinds of implementations with common code, we
add the library providers/libcommon.a.  Code that ends up in this
library must be FIPS agnostic.

While we're moving things around, though, we move digestscommon.h
from providers/common/include/internal to providers/common/include/prov,
thereby starting on a provider specific include structure, which
follows the line of thoughts of the recent header file reorganization.
We modify the affected '#include "internal/something.h"' to
'#include "prov/something.h"'.
From providers/{common,default}/ to providers/implementations/

Except for common code, which remains in providers/common/ciphers/.
However, we do move providers/common/include/internal/ciphers/*.h
to providers/common/include/prov/, and adjust all source including
any of those header files.
New name is providers/implementations/include/prov/implementations.h
All inclusions are adapted accordingly.
The end up in providers/common/include/prov/.
All inclusions are adjusted accordingly.
providers/default/defltprov.c and providers/legacy/legacyprov.c
are moved up to providers/ and providers/build.info is adjusted
accordingly.
@levitte levitte force-pushed the reorganize-providers branch from cc12247 to 43eea3f Compare October 9, 2019 16:54
@levitte
Copy link
Member Author

levitte commented Oct 9, 2019

All review comments addressed, and rebased

@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Oct 10, 2019
levitte added a commit that referenced this pull request Oct 10, 2019
The build.info grammar's regular expressions were a horrible read.
By assigning certain sub-expressions to variables, we hope to make
it a little more readable.

Also, the handling of build.info attributes is reworked to use a
common function instead of having copies of the same code.

Finally, the attributes are reorganized to specify if they belong with
programs, libraries, modules or scripts.  This will enable more
intricate attribute assignment in changes to come.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
We want to attach attributes on dependencies.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
The dependency resolution is made uniquely to resolve proper library
order when linking a program, a module or a shared library.

resolvedepends() did a little too much at once, so it's now reduced to
only collect dependencies (and is renamed to collectdepends()), while
a new function, expanddepends(), expands a list of dependency to
insure that dependent libraries are present after depending libraries,
and finally there is reducedepends() which removes unnecessary
duplicates, leaving only the last one.

resolvedepends() is now a simple utility routine that calls the three
mentioned above in correct order.

As part of this, we implement weak dependencies through the 'weak'
build.info attribute.  This is meant to cause a specific order between
libraries without requiring that they are all present.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
Added functionality to use static libraries as source for other
libraries.  When done this way, the target library will use the object
files from the sourced static libraries, making the sourced libraries
work as "containers" for object files.

We also need to make sure that the Unix Makefile template knows how to
deal with shared libraries and modules that depend on static libraries.
That's new situation we haven't had before.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
We put almost everything in these internal static libraries:

libcommon               Block building code that can be used by all
                        our implementations, legacy and non-legacy
                        alike.
libimplementations      All non-legacy algorithm implementations and
                        only them.  All the code that ends up here is
                        agnostic to the definitions of FIPS_MODE.
liblegacy               All legacy implementations.

libnonfips              Support code for the algorithm implementations.
                        Built with FIPS_MODE undefined.  Any code that
                        checks that FIPS_MODE isn't defined must end
                        up in this library.
libfips                 Support code for the algorithm implementations.
                        Built with FIPS_MODE defined.  Any code that
                        checks that FIPS_MODE is defined must end up
                        in this library.

The FIPS provider module is built from providers/fips/*.c and linked
with libimplementations, libcommon and libfips.

The Legacy provider module is built from providers/legacy/*.c and
linked with liblegacy, libcommon and libcrypto.
If module building is disabled, the object files from liblegacy and
libcommon are added to libcrypto and the Legacy provider becomes a
built-in provider.

The Default provider module is built-in, so it ends up being linked
with libimplementations, libcommon and libnonfips.  For libcrypto in
form of static library, the object files from those other libraries
are simply being added to libcrypto.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
From providers/common/ to providers/implementations/

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
From providers/default/ to providers/implementations/

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
From providers/{common,default,legacy}/ to providers/implementations/
However, providers/common/digests/digest_common.c stays where it is,
because it's support code rather than an implementation.

To better support all kinds of implementations with common code, we
add the library providers/libcommon.a.  Code that ends up in this
library must be FIPS agnostic.

While we're moving things around, though, we move digestscommon.h
from providers/common/include/internal to providers/common/include/prov,
thereby starting on a provider specific include structure, which
follows the line of thoughts of the recent header file reorganization.
We modify the affected '#include "internal/something.h"' to
'#include "prov/something.h"'.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
From providers/{common,default}/ to providers/implementations/

Except for common code, which remains in providers/common/ciphers/.
However, we do move providers/common/include/internal/ciphers/*.h
to providers/common/include/prov/, and adjust all source including
any of those header files.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
New name is providers/implementations/include/prov/implementations.h
All inclusions are adapted accordingly.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
The end up in providers/common/include/prov/.
All inclusions are adjusted accordingly.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
levitte added a commit that referenced this pull request Oct 10, 2019
providers/default/defltprov.c and providers/legacy/legacyprov.c
are moved up to providers/ and providers/build.info is adjusted
accordingly.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #10088)
@levitte
Copy link
Member Author

levitte commented Oct 10, 2019

Merged.

285dacc Configure: rework build.info grammar and attributes
9eba593 Configure: Implement attributes for DEPEND[xxx]
bdea50c Configurations/common.tmpl: Rework dependency resolution
e805c2d Build files: Make it possible to source libraries into other libraries
dec95d7 Rework how our providers are built
5687e35 Providers: move common exchange,kdfs,keymgmt,macs,signature
e42cf71 Providers: move default kdfs,macs
7c214f1 Providers: move all digests
604e884 Providers: move all ciphers
af3e7e1 Cleanup: move providers/common/include/internal/provider_args.h
ddd2131 Cleanup: move remaining providers/common/include/internal/*.h
600703f Cleanup: move provider mains up

@levitte levitte closed this Oct 10, 2019
@t8m
Copy link
Member

t8m commented Oct 10, 2019

Great work! Thanks Richard!

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 Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants