Closed agrawroh closed 7 months ago
phlax
commented
7 months ago @agrawroh i think this extension should probably be disabled for FIPS - it is for arm, just not for x86
currently the way of doing that is a bit messy - and has had some recent prs to resolve related issues - but i think if it should always be disabled we want to mark the BUILD target with the nofips label
adisuissa
commented
7 months ago cc @giantcroc @soulxu for cryptomb owners input.
soulxu
commented
7 months ago Recently we upgrade the ipp-crypto, then the temp fix for the BN_bn2lebinpad was removed, since the ipp-crypto fix that also. https://github.com/envoyproxy/envoy/pull/32838/files#diff-9d29f5207ccdac885d0d1ebd78a329857698e120557d73f6e1a011235c5051b3
cc @zhxie
let us give some test
soulxu
commented
7 months ago I can fix this build error by the below patch:
diff --git a/bazel/ipp-crypto-fips.patch b/bazel/ipp-crypto-fips.patch
new file mode 100644
index 0000000000..e9ab2620c8
--- /dev/null
+++ b/bazel/ipp-crypto-fips.patch
@@ -0,0 +1,16 @@
+diff --git a/sources/ippcp/crypto_mb/src/common/ifma_cvt52.c b/sources/ippcp/crypto_mb/src/common/ifma_cvt52.c
+index e6db178c..222531ab 100644
+--- a/sources/ippcp/crypto_mb/src/common/ifma_cvt52.c
++++ b/sources/ippcp/crypto_mb/src/common/ifma_cvt52.c
+@@ -19,6 +19,11 @@
+
+ #include <assert.h>
+
++#include <openssl/bn.h>
++static int BN_bn2lebinpad(const BIGNUM *a, unsigned char *to, int tolen) {
++ return BN_bn2le_padded(to, tolen, a);
++}
++
+ #if defined(_MSC_VER) && (_MSC_VER < 1920)
+ // Disable optimization for VS2017 due to AVX512 masking bug
+ #define DISABLE_OPTIMIZATION __pragma(optimize( "", off ))
\ No newline at end of file
diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl
index 9719778081..f578ce0b07 100644
--- a/bazel/repositories.bzl
+++ b/bazel/repositories.bzl
@@ -546,6 +546,8 @@ def _com_github_unicode_org_icu():
def _com_github_intel_ipp_crypto_crypto_mb():
external_http_archive(
name = "com_github_intel_ipp_crypto_crypto_mb",
+ patches = ["@envoy//bazel:ipp-crypto-fips.patch"],
+ patch_args = ["-p1"],
build_file_content = BUILD_ALL_CONTENT,
)This really confused me, we removed the BN_bn2lebinpad declaration previously to make the FIPS build work https://github.com/envoyproxy/envoy/pull/30001/files
But why we need that BN_bn2lebinpad declaration again, we even didn't upgrade the version of boringSSL fips after the fix https://github.com/envoyproxy/envoy/pull/30001
I need to figure that out.
soulxu
commented
7 months ago This really confused me, we removed the
BN_bn2lebinpaddeclaration previously to make the FIPS build work https://github.com/envoyproxy/envoy/pull/30001/filesBut why we need that
BN_bn2lebinpaddeclaration again, we even didn't upgrade the version of boringSSL fips after the fix #30001
I figured out the reason and described here https://github.com/envoyproxy/envoy/pull/33756#issuecomment-2074058990
soulxu
commented
7 months ago And our CI doesn't test FIPs build with contrib extensions today, give a test, if we force compile-time-option build with contrib, then we can reproduce this build issue in the CI https://github.com/envoyproxy/envoy/pull/33757, but yea, we need to discuss how to test that in the CI officially.
Description
Envoy v1.30.0 FIPS build seems to be failing for newly added
cryptombextension [Link] on AMD64.Bazel Logs