Conversation
|
Hi @kpramesh2212, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
|
@kpramesh2212 please apply DCO and merge main. See Line 412 in 491b0af |
phlax
left a comment
There was a problem hiding this comment.
thanks for working on this @kpramesh2212
bazel/external/aws_lc.BUILD
Outdated
| ], | ||
| cmd = "$(location {}) $(location crypto/libcrypto.a) $(location ssl/libssl.a)".format("@envoy//bazel/external:aws_lc.genrule_cmd"), | ||
| tools = ["@envoy//bazel/external:aws_lc.genrule_cmd"], | ||
| tags = ["no-sandbox"], |
There was a problem hiding this comment.
i think this is wrong/unnecessary
There was a problem hiding this comment.
@phlax I am working on this will update the PR once I fix it.
There was a problem hiding this comment.
@phlax I have removed that tag and changed the AWS_LC cmd to get it compiling
| continue; | ||
| } | ||
|
|
||
| #ifdef OPENSSL_IS_BORINGSSL |
There was a problem hiding this comment.
I do not think you need this ifdef, since compiler will have no issue convert non const type to const. But not the other way around. I would just ad a comment that aws+lc needs non const input to X509_NAME_dup
source/common/tls/context_impl.cc
Outdated
| switch (policy.value()) { | ||
| using ProtoPolicy = envoy::extensions::transport_sockets::tls::v3::TlsParameters; | ||
| case ProtoPolicy::FIPS_202205: | ||
| #ifdef OPENSSL_IS_BORINGSSL |
There was a problem hiding this comment.
Does this change also fix a build error, or is it a runtime error? If it is a runtime error, can you move it into a separate PR to keep unrelated changes separate, please?
There was a problem hiding this comment.
@yanavlasov Yes without that if block the build is failing
There was a problem hiding this comment.
The preferred way to fix these types of build errors is to define something in aws_lc_compat.h for whatever is missing/different in aws-lc.
- Upgrade AWS-LC from 1.66.2 to 1.70.0 - Replace runtime tool downloads with Bazel external repos for Clang, Go, Ninja, and CMake (per-arch: x86_64, aarch64, ppc64le) - Fix sandbox: build in writable temp dir with GOCACHE instead of execroot - Add X509_NAME_dup const_cast for AWS-LC/OpenSSL API compatibility - Reject FIPS_202205 policy when not using BoringSSL - Add Gazelle resolve directives for golang.org/x/text and x/tools Signed-off-by: Ramesh KP <ramesh_kp.kurichi_ponnuswamy@genesys.com>
022f67a to
71f464b
Compare
bazel/external/aws_lc.BUILD
Outdated
| name = "cmake_bin", | ||
| srcs = [ | ||
| "@aws_lc_cmake_linux_ppc64le//:all", | ||
| "@aws_lc_cmake_linux_ppc64le//:bootstrap", |
There was a problem hiding this comment.
this should be agnostic to arch - aws can be used eg on arm also if a fips build is required
also wondering if all this duplication is necessary
There was a problem hiding this comment.
if this works for builds other than ppce, i think this repo is just misnamed - its not ppce-specific
iiuc - it should probably be called fips_cmake_src or similar to be consistent with how we have existing fips repos
|
I have build envoy in both x86 and arm machines with AWS_LC and build is completing fine |
|
Assigning to @phlax who understands this better and is already reviewing :) |
|
|
||
| bssl::UniquePtr<X509_NAME> name_dup(X509_NAME_dup(name)); | ||
| // AWS-LC/OpenSSL require non-const input to X509_NAME_dup; BoringSSL accepts const. | ||
| bssl::UniquePtr<X509_NAME> name_dup(X509_NAME_dup(const_cast<X509_NAME*>(name))); |
There was a problem hiding this comment.
Don't const-cast; just remove the the const on line 554
source/common/tls/context_impl.cc
Outdated
| switch (policy.value()) { | ||
| using ProtoPolicy = envoy::extensions::transport_sockets::tls::v3::TlsParameters; | ||
| case ProtoPolicy::FIPS_202205: | ||
| #ifdef OPENSSL_IS_BORINGSSL |
There was a problem hiding this comment.
The preferred way to fix these types of build errors is to define something in aws_lc_compat.h for whatever is missing/different in aws-lc.
| continue; | ||
| } | ||
|
|
||
| #ifdef OPENSSL_IS_BORINGSSL |
There was a problem hiding this comment.
remove const_cast same as above
|
@phlax @ggreenway I have fixed all the review comments could you please take a look into this PR please |
|
Hi! Just following up on this PR. I've incorporated all the requested changes from the previous review. Whenever you get a chance, I'd appreciate a re-review. Thanks a lot for your help! |
phlax
left a comment
There was a problem hiding this comment.
the main issue for us reviewing this is that its not supported, and therefore not tested
i think we can probably accomodate this but you will need to reduce the duplication and deps to do so
wrt duplication - also wondering if this cant reuse more existing code - but lets address above first and then consider that possibility
| ./bootstrap --parallel=$$(nproc) | ||
| make -j$$(nproc) | ||
| cp bin/cmake $@ | ||
| """, |
There was a problem hiding this comment.
| """, | |
| """, | |
| target_compatible_with = select({ | |
| "@platforms//cpu:x86_64": ["@platforms//:incompatible"], | |
| "@platforms//cpu:aarch64": ["@platforms//:incompatible"], | |
| "//conditions:default": [], | |
| }), |
as these to platforms have already a prebuilt - i think this should be marked as incompatible with them
bazel/external/aws_lc.BUILD
Outdated
| ] + select({ | ||
| "@platforms//cpu:x86_64": [ | ||
| "@aws_lc_clang_x86_64//:bin/clang", | ||
| "@aws_lc_clang_x86_64//:bin/clang++", |
There was a problem hiding this comment.
this is not correct - for x86 it shoudl use the toolchains_llvm platform - same for aarch64 below
| OS=`uname` | ||
| ARCH=`uname -m` | ||
| OS=$(uname) | ||
| ARCH=$(uname -m) |
There was a problem hiding this comment.
this is definitely an improvement - i think this file still doesnt pass shellcheck tho (annoyingly its not currently tested by ci)
could you run
$ shellcheck -x bazel/external/aws_lc.genrule_cmdand at least ensure that no new issues are added
bazel/external/aws_lc.genrule_cmd
Outdated
| echo "PATH: $PATH" | ||
| echo "PLATFORM: $PLATFORM" | ||
| echo "ERROR: CMake version doesn't match. Expected: ${VERSION}, Got: $(cmake --version | head -n1)" | ||
| VERSION=4.2.3 |
There was a problem hiding this comment.
fwiw we have removed these version checks from the boringssl fips build as we have adopted a slightly different policy for managing fips (see bazel/SSL.md)
bazel/repositories.bzl
Outdated
| build_file_content = GO_BUILD_CONTENT, | ||
| ) | ||
| external_http_archive( | ||
| name = "aws_lc_go_aarch64", |
There was a problem hiding this comment.
are these go binaries that aws_lc produces - if not they dont want the aws_lc prefix - also wondering why you need to add go binaries for x86/arm when we already have them
bazel/repository_locations.bzl
Outdated
| urls = ["https://dl.google.com/go/go{version}.linux-arm64.tar.gz"], | ||
| ), | ||
|
|
||
| aws_lc_clang_x86_64 = dict( |
There was a problem hiding this comment.
as commented above we dont want these deps - its duplicating the toolchain
bazel/repository_locations.bzl
Outdated
| urls = ["https://github.com/llvm/llvm-project/releases/download/llvmorg-{version}/clang+llvm-{version}-powerpc64le-linux-ubuntu-18.04.tar.xz"], | ||
| ), | ||
|
|
||
| aws_lc_go_x86_64 = dict( |
There was a problem hiding this comment.
same with these deps - they are unnecessary
bazel/repository_locations.bzl
Outdated
| strip_prefix = "go", | ||
| urls = ["https://dl.google.com/go/go{version}.linux-arm64.tar.gz"], | ||
| ), | ||
| aws_lc_go_ppc64le = dict( |
There was a problem hiding this comment.
and these next two also should nto be prefixed with aws_lc
04922dd to
dfd6fc3
Compare
|
@phlax I have fixed all the comments |
|
@phlax @ggreenway @yanavlasov I have fixed all the comments could you please take a look into this PR once you get a chance |
|
Thanks @kpramesh2212 for these changes! We have been wanting this functionality on our end for a while now. Is there anything I can do to help move this PR along? It seems like FIPS-140-2 is not going to be compliant in September this year so this change is urgently needed. |
phlax
left a comment
There was a problem hiding this comment.
lgtm, thanks for iterating @kpramesh2212
will also need signoff from @ggreenway
Signed-off-by: Ramesh KP <ramesh_kp.kurichi_ponnuswamy@genesys.com>
dfd6fc3 to
7fbfa4e
Compare
|
Sorry the CI was failing I had to fix it could you please take on look at this change please https://github.com/envoyproxy/envoy/compare/dfd6fc3288ec93e2ef0dd31465c26dd991f700d0..7fbfa4e4a1d63d96d718033cbc1f29422d244d80 |
Use of generative AI
I used generative AI to help fix the AWS-LC FIPS build. I am not a C++ developer, so this PR should be reviewed carefully before merging. I was able to compile Envoy successfully with the changes below, but I may not have fully understood all implications. Please review thoroughly.
Additional Description
This PR fixes several build failures encountered when building Envoy with
--config=aws-lc-fips, particularly on aarch64 and in container/CI environments. These fixes address the build infrastructure issues that prevent the AWS-LC FIPS build from completing.Note for native aarch64 builds: The build also requires
BAZEL_USE_HOST_SYSROOT=True(set as an environment variable before building). This is an existing Envoy option documented inbazel/repo.bzland is not changed in this PR.Risk Level
Low – changes are limited to build configuration and external dependency setup, not runtime code.
Testing
Production build (successful):
Debug build (successful):
Environment: Native aarch64 Linux build with
BAZEL_USE_HOST_SYSROOT=Trueexported.Docs Changes
None. Consider documenting
BAZEL_USE_HOST_SYSROOT=Truefor native aarch64 AWS-LC FIPS builds in a follow-up.Release Notes
Platform Specific Features
Fixes #43904
BUILD machine
EXPLANATION OF CHANGES (for review comments or PR description)
Go dependency resolution (bazel/dependency_imports.bzl)
import golang.org/x/text and golang.org/x/tools, but Gazelle didn't resolve
them, causing "missing strict dependencies" errors.
imports to @org_golang_x_text and @org_golang_x_tools. Also added
org_golang_x_tools go_repository if it wasn't present.
Tar ownership (bazel/external/aws_lc.genrule_cmd)
and CMake tarballs. These archives contain ownership metadata (uid 11827,
gid 9000). In containers or sandboxes, those uid/gid don't exist, so tar
fails with "Cannot change ownership: Invalid argument".
use the current user's uid/gid. Safe for build toolchains and works in
restricted environments.
AWS-LC genrule sandbox (bazel/external/aws_lc.BUILD)
configure. Bazel's sandbox makes the execroot read-only for some actions,
causing "Read-only file system" when CMake runs configure_file.
outside the sandbox. Only this genrule is affected; other build actions
remain sandboxed.
Host sysroot (environment variable - not a code change)
expected libc layout, causing linker errors like "cannot find
/lib/aarch64-linux-gnu/libc.so.6".
the host's libc. Document this for native aarch64 builds.
SUMMARY TABLE
================================================================================