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.

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

Re-enable ability to use pre-compiled FIPS #156

Closed
wants to merge 1 commit into from

Conversation

howardjohn
Copy link

This check was added in #130, and broke us.

we currently use precompiled FIPS builds, since we don't want our developers to need to have the ancient/obscure toolchains, etc required to build FIPS boringssl.\

#130 looks useful, but as far as I understand this only allows using FIPS boringcrypto + latest boringssl, not FIPS boringcrypto+boringssl at the same SHA. The former may be something we want to move towards in the future, but it seems nice to have both options.

I would not be surprised if I am misunderstanding #130, but there isn't much context associated with it.

@nmittler
Copy link
Contributor

@nox @inikulin mind taking a look?

@inikulin
Copy link
Collaborator

@howardjohn the change makes sense for me. Could you rebase it on top of recent changes? I think it's also worth emitting a warning compile time message (via println!("cargo:warning=...");) for this configuration warning users that they must ensure that provided binary is actually compiled with fips configuration

@howardjohn
Copy link
Author

@howardjohn the change makes sense for me. Could you rebase it on top of recent changes? I think it's also worth emitting a warning compile time message (via println!("cargo:warning=...");) for this configuration warning users that they must ensure that provided binary is actually compiled with fips configuration

Yep, added both. Thanks

@nox
Copy link
Collaborator

nox commented Oct 11, 2023

@howardjohn Could you tell us exactly how you trigger that error? Are you using feature fips or fips-link-precompiled? Also, which variables are you defining? Are you using BORING_BSSL_PATH?

boring/boring-sys/build.rs

Lines 599 to 607 in 4749c52

let features_with_patches_enabled = cfg!(any(feature = "rpk", feature = "pq-experimental"));
let patches_required = features_with_patches_enabled && !no_patches_enabled;
let build_from_sources_required = cfg!(feature = "fips-link-precompiled") || patches_required;
let is_precompiled_native_lib = env::var("BORING_BSSL_PATH").is_ok();
if is_precompiled_native_lib && build_from_sources_required {
panic!("precompiled BoringSSL was provided, so FIPS configuration or optional patches can't be applied");
}
}

is_precompiled_native_lib is true because you are using BORING_BSSL_PATH, but why is build_from_sources_required true?

If it is because you are enabling feature fips-link-precompiled, then just don't, as it is only relevant for the BORING_SSL_PRECOMPILED_BCM_O builds as introduced by #130, as features fips and fips-link-precompiled were separated in #160.

If it is because you are enabling features pq-experimental or rpk, causing features_with_patches_enabled to be true, then also enable the feature no-patches and you should be set.

@howardjohn
Copy link
Author

Hmm, it seems like pulling from latest boring fixes this without this PR now.

Our setup:

BORING_BSSL_PATH = { value = "vendor/boringssl-fips/linux_x86_64", force = true, relative = true }
BORING_BSSL_INCLUDE_PATH = { value = "vendor/boringssl-fips/include/", force = true, relative = true }

with features=fips

@nox
Copy link
Collaborator

nox commented Oct 13, 2023

Cool, closing this then. Could you check if #170 works for you with

BORING_BSSL_FIPS_PATH = { value = "vendor/boringssl-fips/linux_x86_64", force = true, relative = true }
BORING_BSSL_FIPS_INCLUDE_PATH = { value = "vendor/boringssl-fips/include/", force = true, relative = true }

@nox nox closed this Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants