build: migrate BoringSSL FIPS to genrule_repository.

[PiotrSikora]

Signed-off-by: Piotr Sikora piotrsikora@google.com

[PiotrSikora]

Per @htuch's comment in #5218.

[htuch]

Thanks for taking this on, looks good, but I'm wondering about how to best treat the internal curls for go/ninja/clang etc.

[htuch]

Thoughts on how this will interact with #5049?

[PiotrSikora]

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.

[htuch]

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..

[PiotrSikora]

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.

[htuch]

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.

[PiotrSikora]

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.

[htuch]

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.

[PiotrSikora]

There: #5428.

[htuch]

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?

[PiotrSikora]

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.

[lizan]

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?

[htuch]

I think folks who want FIPS are advanced users. These types of wrappers just hide the fine detail. Making more things transparent is a feature. OTOH, I'm happy to disagree and commit on this one if you feel strongly here.

…

On Thu, Dec 27, 2018 at 6:22 PM Piotr Sikora ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bazel/external/boringssl_fips.genrule_cmd <#5404 (comment)>: > + 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 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. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#5404 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKaLv38dCmeXC79drN2P_khCE13dafQCks5u9HUugaJpZM4ZfjaF> .

[htuch]

I will merge for now as this is blocking a bunch of folks on some distros and also #5425. I'm guessing we will be evolving our strategy for FIPS builds over time.