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

Need to actually support arm64 builds in a way that is compatible with CI #357

Closed
bleggett opened this issue Jan 26, 2023 · 9 comments
Closed

Comments

@bleggett
Copy link
Contributor

bleggett commented Jan 26, 2023

  • We aren't validating fips-less builds in CI
  • We can't build arm64 (or any other target Rust might otherwise support just fine) with FIPS
  • FIPS is usually not relevant for contributors/local dev
  • Contributors can't disable FIPS without local hacks.
  • This leads to inevitable build entropy for supported build pathways not gated by CI, see Debian cmake package missing standard targets/templates (enablesfips-less builds of ztunnel) tools#2344
  • If fips is always going to be optional/have very specific and unique environmental requirements (which will always be the case - even upstream boring Rust lib has FIPS as an opt-in flag, the inverse of what we have) - we should make sure our CI can build with and without it.

While we must build with and without fips for every CI build, the decision to make the fips feature a default feature complicates the ability to easily perform automated builds with and without fips - because cargo doesn't support negating features individually, they either must be optional, or they must be defaults - and the only way to disable defaults is to disable all default features and selectively re-enable them, which sucks (both for forward compat and usability, see current solution), and makes doing CI validation of fips on/off impossible without locally editing files, see current best attempt #139

Suggestion:

  1. mimic upstream boring Rust lib and make fips a non-default feature
  2. Change CI to build 2x ->
  • BORING_BSSL_PATH="vendor/boringssl-fips/linux_x86_64" BORING_BSSL_INCLUDE_PATH="vendor/boringssl-fips/linux_x86_64/include/" cargo build --features fips
  • cargo build

This would remove the current obstacles to simple automated builds of both fips and non-fips variants, and is more obvious/normative of an approach.

EDIT: Changing this to we should be able to build on more than one arch in a way that is actually automatable and doesn't create a lot of work for ourselves - that's the core problem.

@howardjohn
Copy link
Member

non-fips was added exclusively to workaround a short term limitation (#35) (note the issue was closed, should probably be opened as "support fips+arm64"). Do you have a use case for non-fips long term? on x86?

@bleggett
Copy link
Contributor Author

bleggett commented Jan 27, 2023

As someone who has worked at a place where FIPS compliance mattered -

  1. Regardless of arch, realistically we will always need to support building without FIPS for one reason or another - FIPS cert is slow, it requires specific libraries, there are multiple revisions of it, people may want to build against libraries that have fixes/algos FIPS hasn't certified yet, it will simply never support all current platforms in a timely fashion, non-US customers who do not have FIPS compliance requirements may want to use a more current set of libraries (see also ECC + NIST curves), etc etc etc.

  2. boring itself does not force fips compliance as a default Cargo feature for the same reasons, I see no real reason why we should, if they do not (in fact, in all my experience with fips compliant libraries, fips being an optional mode is very much the expected norm)

  3. And critically, given how Cargo+boring works, we cannot force fips as a default and also support automated non-FIPS builds in a clean fashion, which creates friction for contributors on different architectures - if our build is clean and we pick smart defaults, building on a Rust-supported architecture shouldn't be an obstacle to anyone, if they do not care about FIPS.

non-fips was added exclusively to workaround a short term limitation

Yes - my contention is that the "right way" to fix this in the long term is to play nice with Cargo in the way I described, rather than fighting it.

The simplest and least fragile fix for this limitation is to continue to support fips and non-fips builds, make fips a non-default feature (as boring does), and build both via automation as a CI check. Our current approach is un-automatable and non-obvious, and we lose nothing by making fips builds optional as long as we are CI gating both versions. We gain the ability to automate both variants, which to me is easily the most important thing.

@howardjohn
Copy link
Member

We have already decided to only ship a FIPS build for ztunnel from the Istio project. Making the default thing with cargo run build what we are actually going to ship is much higher priority than the UX of dealing with non-FIPS. If someone wants to go out of their way to deal with non-FIPS for a vendor, I don't mind making that work - but not at the expense of the happy path for most developers/users.

So I am fine with keeping support and with checking in CI, but not making it non-default.

@bleggett
Copy link
Contributor Author

Aight, if the default-ness is non-negotiable (and we don't intend to publish non-fips builds ATM), I'll leave this open and see what I can do within those parameters.

@bleggett
Copy link
Contributor Author

bleggett commented Jan 30, 2023

  • We unfortunately (but reasonably) cannot dynamically munge env vars in our own build.rs to select the right vendored bssl-fips build for a dependency of ours (boring rust bindings).
  • This means that an argsless cargo build with FIPS enabled by default is only ever possible with a hardcoded single os/arch combo, or with manual config edits.
  • Vendoring binary deps is generally not a resilient or tidy solution in the long term anyway.

This leaves us with 2 options for a clean, automatable build

  1. Let boring-sys (upstream dep) auto-build boringssl-fips in its own build.rs as it currently tries to do.
  2. Document that people must build their own boringssl-fips and point boring-sys at it.

Because of FIPS cert lag, both of these options necessarily create fragile environmental dependencies on very specific clang builds which will frequently be unavailable in build environments, unless we provide our own.

Option # 1 (let boring-sys handle+build its own deps) seems best.

To enable this, best approach seems to be to create a ztunnel-specific istio-build-tools variant that looks more or less like https://github.com/istio/ztunnel/blob/master/docker/remote-env/Dockerfile - then make shell should give a consistent build env with correct clang for every build-tools variant we support.

Generally speaking, that approach is blocked by upstream boring cloudflare/boring#97 - once that feature is in upstream, we can install boring-sys as a Gitref +patchdep in our Cargo.toml:

[patch.crates-io]
boring-sys = { git = "https://github.com/cloudflare/boring", tag = "v2.1.0"  }

and it should cleanly build inside a custom build-tools with the requisite Clang.

This would unblock both FIPS and non-FIPS CI gates, as well as currently-required arm64 hacks.

@howardjohn
Copy link
Member

I would much prefer we have a prebuilt boringssl checked in. This avoids the need to have obscure dependencies (until a few weeks ago this was clang-7 which is virtually impossible to install on modern OS's) and ensures cargo run just works for the common case (x86 linux). I disagree this is a bad practice; it is copied wholesale from Golang's boringssl support.

I would much prefer changes are made to the upstream library to make the build more flexible to meet our needs

@bleggett
Copy link
Contributor Author

bleggett commented Jan 30, 2023

I would much prefer we have a prebuilt boringssl checked in. This avoids the need to have obscure dependencies (until a few weeks ago this was clang-7 which is virtually impossible to install on modern OS's) and ensures cargo run just works for the common case (x86 linux).

That approach avoids the need to have obscure dependencies (a problem we can, currently do, and realistically will always have to mitigate with containerized build envs) and introduces the inability to easily call cargo build/cargo run outside of linux/x86. Swaps a problem that's easy for us to fix with a problem that isn't, for what amounts to little tangible benefit.

Our "default case" is not linux/x86 - our default case is supporting at least every OS/arch combo istio already supports with published artifacts and a functional build process - that means linux/x86 and linux/arm64 at a minimum - ideally it should mean "whatever Linux arch your rust toolchain supports as a target" but FIPS, as always, will perpetually complicate that, especially if we make it so that people who don't need it or want it can't turn it off.

The only way upstream changes can enable us to build what we need to build (without us adding options/overrides/env vars/args to plain cargo invocations) is if they either

  1. have an array of arch/OS specific vendored binary env vars
  2. their build.rs is enhanced to select the correct vendored build subpath at build time, e.g <os>_<arch>/lib, rather than assuming that there is only one preselected vendored build (this is what they do today)

First is messy and unreasonable to ask upstream to support IMO, second is less so, and could be an option.

I disagree this is a bad practice; it is copied wholesale from Golang's boringssl support.

It's a cumbersome practice - especially since we are not Golang - and requires every downstream boringssl consumer to make, publish, and update binary builds of boringssl for every architecture they support building against as a vendoring chore. Sticking with fixed build envs and requiring specificclang variants is also cumbersome, but has the benefit of not requiring downstream consumers to explicitly support every target arch their codebase can build on with handrolled binaries on an ongoing basis, and is simplifiable with containerized build envs.

@bleggett bleggett changed the title Need to actually test fips-less builds in CI Need to actually support arm64 (or anything else that may come along) builds in a way that is compatible with CI Feb 6, 2023
@bleggett bleggett changed the title Need to actually support arm64 (or anything else that may come along) builds in a way that is compatible with CI Need to actually support arm64 builds in a way that is compatible with CI Feb 6, 2023
@bleggett
Copy link
Contributor Author

bleggett commented Feb 15, 2023

Closing this - I think there is still followup work to be done on our end, and potentially in upstream boring, to make vendoring/CI/builds simpler, and to possibly get away from vendoring entirely.

I also think a non-FIPS build is probably something we will have to support eventually.

But we have arm64 builds happening as part of CI now, and that was the blocker.

@bleggett
Copy link
Contributor Author

Raised #399 to track the only outstanding followups.

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

No branches or pull requests

2 participants