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

Include embedded BoringSSL into libsoter.a #681

Merged
merged 5 commits into from
Jul 23, 2020

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Jul 20, 2020

Currently, when building Themis with embedded BoringSSL (the one from git submodule), the shared library is linked against the BoringSSL static library and it is embedded into the dylib. That way resulting Themis dylib does not depend on the system OpenSSL. Instead it uses the BoringSSL which we build and embed into it.

However, right now Themis static library does not include embedded BoringSSL in it. The user is expected to find libcrypto.a and libdecrepit.a in BoringSSL build directory, distribute them along with libthemis.a and libsoter.a, and include all four libraries when building the application.

Why is this an issue?

This is not particularly convenient since BoringSSL libraries are buried in the BoringSSL build directory, not available in the usual Themis build directory. It is quite possible that developers will forget about them entirely. They are also not included into the packages we build.

Furthermore, Soter is built with and expects that particular version of BoringSSL to be linked into the application. While BoringSSL generally does a good job at maintaining ABI compatibility, the user may accidentally link some OpenSSL from their system instead of BoringSSL that was built with Themis. This may break in a subtle way.

How we resolve it

In order to avoid all those issues, let's embed BoringSSL into Soter static library, like we do with the shared library. That way the users will have to link only against libthemis.a and libsoter.a.

Note that this is actual only for the case when we are building Themis with embedded BoringSSL. That is, the BoringSSL we provide in submodule. We should not embed system OpenSSL, or any BoringSSL built and provided by the user. In that case the users are expected to take care of the cryptography provider library themselves.

On portability

Unfortunately, one does not simply merge static libraries.

The usual ar tool cannot do that: if you mention *.o files and *.a files it will simply include *.a files into the archive as is and it will confuse the linker which unpacks only one layer of static libraries.

The other traditional tool which is intended for this use case is libtool. On macOS is has somewhat sane interface and is available on the default system installation. But on Linux systems the GNU libtool is typically used. It has... well... a little bit wacky interface and it is normally used with Autotools.

The problem with Autotools is that Themis build system is not expected to bow to their idiosyncrasies. Furthermore, libtool is not available in the default installation on all distros we support.

I do not really want to add a new dependency—especially dependency on Autotools—just for that one use case.

So instead of that, here's an utility script which uses ar to merge multiple static libraries. It has issues of its own, but at least it's portable and should work on any UNIX-like system out of the box.

On symbol conflicts

As a final note, embedding BoringSSL into the static library has some consequences. Because of the way static linkage and static libraries work, this means that libsoter.a provides BoringSSL symbols which may conflict with and/or shadow other BoringSSL or OpenSSL symbols.

This changeset does not do anything about it. Like before, this is the issue that the users have to solve themselves. Though, we can still do something about it later if we manage to use BoringSSL symbol prefixes.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Changelog is updated

Currently, when building Themis with embedded BoringSSL (the one from
git submodule), the shared library is linked against the BoringSSL
static library and it is embedded into the dylib. That way resulting
Themis dylib does not depend on the system OpenSSL. Instead it uses
the BoringSSL which we build and embed into it.

However, right now Themis static library does not include embedded
BoringSSL in it. The user is expected to find "libcrypto.a" and
"libdecrepit.a" in BoringSSL build directory, distribute them along with
"libthemis.a" and "libsoter.a", and include all four libraries when
building the application.

Why is this an issue?
---------------------

This is not particularly convenient since BoringSSL libraries are buried
in the BoringSSL build directory, not available in the usual Themis
build directory. It is quite possible that developers will forget about
them entirely. They are also not included into the packages we build.

Furthermore, Soter is built with and expects that particular version of
BoringSSL to be linked into the application. While BoringSSL generally
does a good job at maintaining ABI compatibility, the user may
accidentally link some OpenSSL from their system instead of BoringSSL
that was built with Themis. This may break in a subtle way.

How we resolve it
-----------------

In order to avoid all those issues, let's embed BoringSSL into Soter
static library, like we do with the shared library. That way the users
will have to link only against "libthemis.a" and "libsoter.a".

Note that this is actual only for the case when we are building Themis
with embedded BoringSSL. That is, the BoringSSL we provide in submodule.
We should not embed system OpenSSL, or any BoringSSL built and provided
by the user. In that case the users are expected to take care of the
cryptography provider library themselves.

On portability
--------------

Unfortunately, one does not simply merge static libraries.

The usual "ar" tool cannot do that: if you mention *.o files and *.a
files it will simply include *.a files into the archive as is and it
will confuse the linker which unpacks only one layer of static
libraries.

The other traditional tool which is intended for this use case is
"libtool". On macOS is has somewhat sane interface and is available on
the default system installation. But on Linux systems the GNU libtool
is typically used. It has... well... a little bit wacky interface and it
is normally used with Autotools.

The problem with Autotools is that Themis build system is not expected
to bow to their idiosyncrasies. Furthermore, on all distros we support
libtool is not available in the default installation.

I do not really want to add a new dependency -- **especially**
dependency on Autotools -- just for that one use case.

So instead of that, here's an utility script which uses "ar" to merge
multiple static libraries. It has issues of its own, but at least it's
portable and should work on any UNIX-like system out of the box.

On symbol conficts
------------------

As a final note, embedding BoringSSL into the static library has some
consequences. Because of the way static linkage and static libraries
work, this means that "libsoter.a" provides BoringSSL symbols which may
conflict with and/or shadow other BoringSSL or OpenSSL symbols.

This changeset does not do anything about it. Like before, this is the
issue that the users have to solve themselves. Though, we can still do
something about it later if we manage to use BoringSSL symbol prefixes.
@ilammy ilammy added core Themis Core written in C, its packages C-BoringSSL Crypto provider: BoringSSL installation Installation of Themis core and wrapper packages labels Jul 20, 2020
Comment on lines +37 to +39
if [[ $# -lt 1 ]]
then
echo >&2 "Invalid command-line: missing target library name"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the script is useless without at least two arguments: a target and minimum one source library.

Suggested change
if [[ $# -lt 1 ]]
then
echo >&2 "Invalid command-line: missing target library name"
if [[ $# -lt 2 ]]
then
echo >&2 "Invalid command-line: target library and at least one source library required"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like the script is useless without at least two arguments: a target and minimum one source library.

Not so useless, it is intentionally made a no op if there are no libraries to merge into the target library. This makes its behavior similar to sum function. It is useful because the case when there are no embedded libraries is no longer a special case.

The makefile filters out *.a files from dependencies of libsoter.a and passes them to this script. If there are no *.a files in dependencies (e.g., when system OpenSSL is used) then merge-static-libs.sh is invoked with only build/libsoter.a and does nothing.

If this case is treated as an error, the Makefile will need to have a conditonal to avoid using merge-static-libs.sh which is unnecessary complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.

scripts/merge-static-libs.sh Outdated Show resolved Hide resolved
scripts/merge-static-libs.sh Outdated Show resolved Hide resolved
ilammy and others added 4 commits July 21, 2020 12:03
Instead of the usual "#!/bin/bash" shebang, use "#!/usr/bin/env bash"
so that the script is executed with whatever Bash is in PATH. This
makes the script a bit more flexible.
@shad has found a nice hack to convert a path into an absolute one.
Use it to avoid copying libraries around for "ar" which can extract
files only into the current directory.

Co-authored-by: Dmytro Shapovalov <[email protected]>
libthemis-src crate needs full source code of Themis as in performs
builds. This includes all build dependencies, such as newly added script
for merging static libraries. Add a symlink to the script directory to
include it into libthemis-src distribution.
Since now we include BoringSSL into "libsoter.a" if needed, we don't
have to mention in Soter dependencies for static linkage anymore.
This is particularly important for WasmThemis.
@ilammy
Copy link
Collaborator Author

ilammy commented Jul 21, 2020

I've included @shadinua suggestions and added a couple more fixes for RustThemis and WasmThemis builds. Now the CI should become green.

@ilammy ilammy added W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages labels Jul 21, 2020
@ilammy ilammy mentioned this pull request Jul 21, 2020
5 tasks
# Actually merge the contents of the library into the target one.
find "$tempdir" -type f -name '*.o' -print0 | xargs -0 "$AR" rs "$target"
# Clean up and go on.
rm -rf "$tempdir"/*
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any possible corner cases when this line will remove the whole user dir ~/*?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It shouldn't unless $tempdir is messed with from outside the script. If it's empty, something before this point will fail (e.g., extraction) and the script will exit before running rm -rf /*.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should add a log message here? Something like "Preparing to remove $tempdir", and then "$tempdir is successfully removed".

even if we are 99.9% sure that this line won't destroy the universe, in a very bad case, people will see the log message to understand what went wrong.

@@ -0,0 +1 @@
../../../../../../scripts
Copy link
Contributor

Choose a reason for hiding this comment

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

😁

@ilammy ilammy merged commit 02229c1 into cossacklabs:master Jul 23, 2020
@ilammy ilammy deleted the embed-boringssl branch July 23, 2020 12:28
@ilammy ilammy mentioned this pull request Jul 27, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-BoringSSL Crypto provider: BoringSSL core Themis Core written in C, its packages installation Installation of Themis core and wrapper packages W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants