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.

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

Jump to bottom

cryptomb: add BN_bn2lebinpad definition for fips build #33756

Merged
merged 4 commits into from
Apr 30, 2024
Merged

cryptomb: add BN_bn2lebinpad definition for fips build #33756

merged 4 commits into from
Apr 30, 2024

Conversation

soulxu
Copy link
Member

@soulxu soulxu commented Apr 24, 2024 •
edited
Loading

Commit Message: cryptomb: add BN_bn2lebinpad definition for fips build
Additional Description:
The fips verison boringSSL is old. so it doesn't have the definition of BN_bn2lebinpad, so patch ipp-crypto to give a definition.

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
Fixes #33585

@soulxu
cryptomb: fix the fips build
ed26217 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@repokitteh-read-only RepoKitteh - Read Only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #33756 was opened by soulxu.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 24, 2024
@repokitteh-read-only RepoKitteh - Read Only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #33756 was opened by soulxu.

see: more, trace.

@soulxu
force the ci compile_time_options job build with contrib extension
5c67fea 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
soulxu
soulxu commented Apr 24, 2024
View reviewed changes
ci/do_ci.sh Outdated Show resolved Hide resolved
@soulxu
Copy link
Member Author

soulxu commented Apr 24, 2024

/assign @phlax

@soulxu
Copy link
Member Author

soulxu commented Apr 24, 2024 •
edited
Loading

Add describe what happened here:

In the very beginning, when we upgrade the boringSSL, we found the non-fips build won't work. Due to duplicated definition of BN_bn2lebinpad
https://github.com/envoyproxy/envoy/pull/30001/files, then we add a patch to remove that definition from the ipp-crypto. This break the fips build, but we don't know, since the CI doesn't test that.

At same time a change to the ipp-crypto was submitted about removing definition of BN_bn2lebinpad intel/cryptography-primitives#63, and ipp-crypto team accepted that. It will be included in new release.

Then later we found this PR #30001 will break the fips build, then another fix coming up #30554, that PR removes the BN_bn2lebinpad patch only for fips build.

And then, we upgraded the ipp-crypto which version removed the definition of BN_bn2lebinpad #32838, and this PR removed the patch and misunderstood that the new version ipp-crypto contained all the temp fixs. That is why fips build break again, also due to our CI doesn't test that, so we don't know that until someone report the issue.

@soulxu soulxu changed the title cryptomb: fix the fips build cryptomb: add BN_bn2lebinpad definition for fips build Apr 24, 2024
@soulxu soulxu mentioned this pull request Apr 24, 2024
Closed
@soulxu
Copy link
Member Author

soulxu commented Apr 24, 2024

Ok, it seems the compile_time_options passed with contrib extension https://github.com/envoyproxy/envoy/pull/33756/checks?check_run_id=24186063786

@soulxu
Revert "force the ci compile_time_options job build with contrib exte…
6b73895 
…nsion"

This reverts commit 5c67fea.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
fix format
63fb743 
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu soulxu marked this pull request as ready for review April 24, 2024 12:39
@soulxu
Copy link
Member Author

soulxu commented Apr 24, 2024

This PR is ready for review, the test change already reverted

phlax
phlax approved these changes Apr 24, 2024
View reviewed changes
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @soulxu

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Apr 24, 2024
@htuch htuch removed their assignment Apr 26, 2024
@soulxu
Copy link
Member Author

soulxu commented Apr 30, 2024

/retest

@soulxu soulxu merged commit 35d00a5 into envoyproxy:main Apr 30, 2024
52 checks passed
soulxu added a commit to soulxu/envoy that referenced this pull request Apr 30, 2024
@soulxu
cryptomb: add BN_bn2lebinpad definition for fips build (envoyproxy#…
e06cc96 
…33756)

The fips verison boringSSL is old. so it doesn't have the definition of BN_bn2lebinpad, so patch ipp-crypto to give a definition.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
This was referenced Apr 30, 2024
Closed
Merged
phlax pushed a commit that referenced this pull request May 2, 2024
@soulxu @phlax
cryptomb: add BN_bn2lebinpad definition for fips build (#33756)
f82322e 
The fips verison boringSSL is old. so it doesn't have the definition of BN_bn2lebinpad, so patch ipp-crypto to give a definition.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phlax phlax phlax approved these changes

Assignees

@phlax phlax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Envoy FIPS v1.30 build failing with errors in new cryptomb contrib extension
3 participants
@soulxu @phlax @htuch