Skip to content

build: migrate BoringSSL FIPS to genrule_repository. #5404

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.

Sign up for GitHub

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

Merged
htuch merged 1 commit into from
Dec 27, 2018
Merged

build: migrate BoringSSL FIPS to genrule_repository. #5404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bazel/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,12 @@ config_setting(
)

# Alias pointing to the selected version of BoringSSL:
# - BoringSSL FIPS from @envoy_deps//:boringssl_fips,
# - BoringSSL FIPS from @boringssl_fips//:ssl,
# - non-FIPS BoringSSL from @boringssl//:ssl.
alias(
name = "boringssl",
actual = select({
"//bazel:boringssl_fips": "//external:boringssl_fips",
"//bazel:boringssl_fips": "@boringssl_fips//:ssl",
"//conditions:default": "@boringssl//:ssl",
}),
)
33 changes: 33 additions & 0 deletions bazel/external/boringssl_fips.BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
load(":genrule_cmd.bzl", "genrule_cmd")

cc_library(
name = "crypto",
srcs = [
"crypto/libcrypto.a",
],
hdrs = glob(["boringssl/include/openssl/*.h"]),
defines = ["BORINGSSL_FIPS"],
includes = ["boringssl/include"],
visibility = ["//visibility:public"],
)

cc_library(
name = "ssl",
srcs = [
"ssl/libssl.a",
],
hdrs = glob(["boringssl/include/openssl/*.h"]),
includes = ["boringssl/include"],
visibility = ["//visibility:public"],
deps = [":crypto"],
)

genrule(
name = "build",
srcs = glob(["boringssl/**"]),
outs = [
"crypto/libcrypto.a",
"ssl/libssl.a",
],
cmd = genrule_cmd("@envoy//bazel/external:boringssl_fips.genrule_cmd"),
)
95 changes: 95 additions & 0 deletions bazel/external/boringssl_fips.genrule_cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
#!/bin/bash

set -e

# BoringSSL build as described in the Security Policy for BoringCrypto module (2018-10-25):
# https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp3318.pdf

# This works only on Linux-x86_64.
if [[ `uname` != "Linux" || `uname -m` != "x86_64" ]]; then
echo "ERROR: BoringSSL FIPS is currently supported only on Linux-x86_64."
exit 1
fi

# Bazel magic.
ROOT=$$(dirname $(rootpath boringssl/BUILDING.md))/..
pushd $$ROOT

# Build tools requirements:
# - Clang compiler version 6.0.1 (http://releases.llvm.org/download.html)
# - Go programming language version 1.10.3 (https://golang.org/dl/)
# - Ninja build system version 1.8.2 (https://github.com/ninja-build/ninja/releases)

# Override $$PATH for build tools, to avoid picking up anything else.
export PATH="$$(dirname `which cmake`):/usr/bin:/bin"

# Clang 6.0.1
Copy link
Member

@htuch htuch Dec 27, 2018

Choose a reason for hiding this comment

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

FWIW, I think it would be ideal if we could use a consistent toolchain across the entire build. We've had issues in the past with string libraries and compiler/glibc couplings that would be simplified here. Can't we just mandate that the user actually builds under clang-6.0.1 if they want a real FIPS build?

Copy link
Contributor Author

@PiotrSikora PiotrSikora Dec 27, 2018

Choose a reason for hiding this comment

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

AFAIK, the issue that you're referring to is/was with linking against versions of libstdc++ that provide different ABIs, not using different compilers.

There are no issues with linking libraries produced using different toolchains (in fact, we're doing this already, since all prebuilt libraries are always built using gcc... same goes for the system libraries), and enforcing older toolchain on rest of the build is going to result in arguably "worse" binary, for no good reason, IMHO.

Yes, we could mandate that users build using clang-6.0.1, go-1.10.3 and ninja-1.8.2, and fail the build otherwise, but then we're only making things harder for the users.

Copy link
Member

@lizan lizan Dec 27, 2018

Choose a reason for hiding this comment

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

I don't see a strong reason to mandate users build using clang-6.0.1, go-1.10.3 and ninja-1.8.2, we might be able to skip download if existing toolchains matches the version we expected. It will make also make CI harder if we want FIPS build with different build image.

Btw, is it possible to use Bazel managed Go binary in a genrule?

VERSION=6.0.1
SHA256=7ea204ecd78c39154d72dfc0d4a79f7cce1b2264da2551bb2eef10e266d54d91
PLATFORM="x86_64-linux-gnu-ubuntu-16.04"

curl -sLO https://releases.llvm.org/"$$VERSION"/clang+llvm-"$$VERSION"-"$$PLATFORM".tar.xz \
&& echo "$$SHA256" clang+llvm-"$$VERSION"-"$$PLATFORM".tar.xz | sha256sum --check
tar xf clang+llvm-"$$VERSION"-"$$PLATFORM".tar.xz

export HOME="$$PWD"
printf "set(CMAKE_C_COMPILER \"clang\")\nset(CMAKE_CXX_COMPILER \"clang++\")\n" > $${HOME}/toolchain
export PATH="$$PWD/clang+llvm-$$VERSION-$$PLATFORM/bin:$$PATH"

if [[ `clang --version | head -1 | awk '{print $$3}'` != "$$VERSION" ]]; then
echo "ERROR: Clang version doesn't match."
exit 1
fi

# Go 1.10.3
VERSION=1.10.3
SHA256=fa1b0e45d3b647c252f51f5e1204aba049cde4af177ef9f2181f43004f901035
Copy link
Member

@htuch htuch Dec 24, 2018

Choose a reason for hiding this comment

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

Shouldn't these be first class dependencies in Bazel rather than fetched in the script? This would allow more uniform treatment and centralization of dependencies in the repository locations file.. right now it's bit weird, since the boringssl FIPS library is there, but not a bunch of its implicit dependencies..

Copy link
Contributor Author

@PiotrSikora PiotrSikora Dec 24, 2018

Choose a reason for hiding this comment

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

I was thinking about promoting those while moving the recipe to genrule_repository (having everything in the script was a bit more acceptable when it was a recipe), but I left it as-is to make the migration faster, since the recipe breaks build for people using CentOS (all recipes are always built, regardless of whether they are used or not, whereas external repositories are built only when needed).

Promoting clang/go/ninja to Bazel dependencies is an incremental improvement that isn't as urgent and can be done later, IMHO.

Having said that, those are specific versions of build tools required by the Security Policy for BoringSSL FIPS, so I'm not sure how much value there is in making them "visible" to Bazel and other dependencies.

Copy link
Member

@htuch htuch Dec 25, 2018

Choose a reason for hiding this comment

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

The advantage of Bazel visibility is that it simplifies OSS compliance and security analysis by end users. Folks want to know what they are building and where it comes from. Can you file an issue and add a TODO? I agree this can be a future PR.

Copy link
Contributor Author

@PiotrSikora PiotrSikora Dec 27, 2018

Choose a reason for hiding this comment

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

Those are build tools, and AFAIK, we don't track those anywhere (neither for Envoy itself, nor for other dependencies), so I'm not sure why BoringSSL FIPS should be an exception, and how it would help with the OSS compliance or security analysis.

Furthermore, there are build tools (i.e. cmake, perl) used for BoringSSL builds, which we don't download (since Security Policy doesn't require specific version of those tools), so at best, we'd have incomplete list of build-time dependencies, which is IMHO more misleading than helpful.

I can file an issue if you really want, but I don't see a reason to block this PR on it.

Copy link
Member

@htuch htuch Dec 27, 2018

Choose a reason for hiding this comment

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

I think we need an issue. While these are build tools, some of them imply build-time link dependencies or generated code, e.g. the toolchain. We don't track this for Envoy proper, since we expect the outer build system to be tracking the environment's clang. Now we're adding an opaque toolchain to the mix. I think for go/cmake/perl, it's probably fine as is.

Copy link
Contributor Author

@PiotrSikora PiotrSikora Dec 27, 2018

Choose a reason for hiding this comment

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

There: #5428.

PLATFORM="linux-amd64"

curl -sLO https://dl.google.com/go/go"$$VERSION"."$$PLATFORM".tar.gz \
&& echo "$$SHA256" go"$$VERSION"."$$PLATFORM".tar.gz | sha256sum --check
tar xf go"$$VERSION"."$$PLATFORM".tar.gz

export GOROOT="$$PWD/go"
export PATH="$$GOROOT/bin:$$PATH"

if [[ `go version | awk '{print $$3}'` != "go$$VERSION" ]]; then
echo "ERROR: Go version doesn't match."
exit 1
fi

# Ninja 1.8.2
VERSION=1.8.2
SHA256=d2fea9ff33b3ef353161ed906f260d565ca55b8ca0568fa07b1d2cab90a84a07
Copy link
Member

@htuch htuch Dec 24, 2018

Choose a reason for hiding this comment

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

Thoughts on how this will interact with #5049?

Copy link
Contributor Author

@PiotrSikora PiotrSikora Dec 24, 2018

Choose a reason for hiding this comment

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

This is build-time dependency (and a build system, at that), so I don't think there is any point in listing it as Envoy dependency.

PLATFORM="linux"

curl -sLO https://github.com/ninja-build/ninja/releases/download/v"$$VERSION"/ninja-"$$PLATFORM".zip \
&& echo "$$SHA256" ninja-"$$PLATFORM".zip | sha256sum --check
unzip ninja-"$$PLATFORM".zip

export PATH="$$PWD:$$PATH"

if [[ `ninja --version` != "$$VERSION" ]]; then
echo "ERROR: Ninja version doesn't match."
exit 1
fi

# Clean after previous build.
rm -rf boringssl/build

# Build BoringSSL.
cd boringssl
mkdir build && cd build && cmake -GNinja -DCMAKE_TOOLCHAIN_FILE=$${HOME}/toolchain -DFIPS=1 -DCMAKE_BUILD_TYPE=Release ..
ninja
ninja run_tests

# Verify correctness of the FIPS build.
if [[ `tool/bssl isfips` != "1" ]]; then
echo "ERROR: BoringSSL tool didn't report FIPS build."
exit 1
fi

# Move compiled libraries to the expected destinations.
popd
mv $$ROOT/boringssl/build/crypto/libcrypto.a $(execpath crypto/libcrypto.a)
mv $$ROOT/boringssl/build/ssl/libssl.a $(execpath ssl/libssl.a)
30 changes: 20 additions & 10 deletions bazel/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ load("@bazel_tools//tools/cpp:lib_cc_configure.bzl", "get_env_var")

# dict of {build recipe name: longform extension name,}
PPC_SKIP_TARGETS = {"luajit": "envoy.filters.http.lua"}
NO_BORINGSSL_FIPS = "boringssl_fips"

# go version for rules_go
GO_VERSION = "1.10.4"
Expand Down Expand Up @@ -261,21 +260,24 @@ def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
actual = path + ":" + t,
)

# Binding for //external:ssl pointing to the selected version of BoringSSL.
native.bind(
name = "ssl",
actual = "@envoy//bazel:boringssl",
)

# Treat Envoy's overall build config as an external repo, so projects that
# build Envoy as a subcomponent can easily override the config.
if "envoy_build_config" not in native.existing_rules().keys():
_default_envoy_build_config(name = "envoy_build_config")

# Binding to an alias pointing to the selected version of BoringSSL:
# - BoringSSL FIPS from @boringssl_fips//:ssl,
# - non-FIPS BoringSSL from @boringssl//:ssl.
_boringssl()
_boringssl_fips()
native.bind(
name = "ssl",
actual = "@envoy//bazel:boringssl",
)

# The long repo names (`com_github_fmtlib_fmt` instead of `fmtlib`) are
# semi-standard in the Bazel community, intended to avoid both duplicate
# dependencies and name conflicts.
_boringssl()
_com_google_absl()
_com_github_bombela_backward()
_com_github_circonus_labs_libcircllhist()
Expand Down Expand Up @@ -307,6 +309,16 @@ def envoy_dependencies(path = "@envoy_deps//", skip_targets = []):
def _boringssl():
_repository_impl("boringssl")

def _boringssl_fips():
location = REPOSITORY_LOCATIONS["boringssl_fips"]
genrule_repository(
name = "boringssl_fips",
urls = location["urls"],
sha256 = location["sha256"],
genrule_cmd_file = "@envoy//bazel/external:boringssl_fips.genrule_cmd",
build_file = "@envoy//bazel/external:boringssl_fips.BUILD",
)

def _com_github_bombela_backward():
_repository_impl(
name = "com_github_bombela_backward",
Expand Down Expand Up @@ -583,8 +595,6 @@ def _apply_dep_blacklist(ctxt, recipes):
skip_list = []
if _is_linux_ppc(ctxt):
skip_list += PPC_SKIP_TARGETS.keys()
if not _is_linux_x86_64(ctxt):
skip_list.append(NO_BORINGSSL_FIPS)
for t in recipes:
if t not in skip_list:
newlist.append(t)
Expand Down
5 changes: 5 additions & 0 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ REPOSITORY_LOCATIONS = dict(
# chromium-71.0.3578.80
urls = ["https://github.com/google/boringssl/archive/77e47de9e16ec8865d1bc6d614dd918141f094d2.tar.gz"],
),
boringssl_fips = dict(
sha256 = "b12ad676ee533824f698741bd127f6fbc82c46344398a6d78d25e62c6c418c73",
# fips-20180730
urls = ["https://commondatastorage.googleapis.com/chromium-boringssl-docs/fips/boringssl-66005f41fbc3529ffe8d007708756720529da20d.tar.xz"],
),
com_google_absl = dict(
sha256 = "e35082e88b9da04f4d68094c05ba112502a5063712f3021adfa465306d238c76",
strip_prefix = "abseil-cpp-cc8dcd307b76a575d2e3e0958a4fe4c7193c2f68",
Expand Down
1 change: 0 additions & 1 deletion bazel/target_recipes.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
TARGET_RECIPES = {
"ares": "cares",
"benchmark": "benchmark",
"boringssl_fips": "boringssl_fips",
"event": "libevent",
"tcmalloc_and_profiler": "gperftools",
"luajit": "luajit",
Expand Down
96 changes: 0 additions & 96 deletions ci/build_container/build_recipes/boringssl_fips.sh
View file
Open in desktop

This file was deleted.

20 changes: 0 additions & 20 deletions ci/prebuilt/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,6 @@ cc_library(
includes = ["thirdparty_build/include"],
)

cc_library(
name = "boringcrypto_fips",
srcs = [
"thirdparty_build/lib/libcrypto.a",
],
hdrs = glob(["thirdparty_build/include/openssl/**/*.h"]),
defines = ["BORINGSSL_FIPS"],
includes = ["thirdparty_build/include"],
)

cc_library(
name = "boringssl_fips",
srcs = [
"thirdparty_build/lib/libssl.a",
],
hdrs = glob(["thirdparty_build/include/openssl/**/*.h"]),
includes = ["thirdparty_build/include"],
deps = [":boringcrypto_fips"],
)

cc_library(
name = "event",
srcs = select({
Expand Down