packages: build Go binaries for FIPS and non-FIPS

[bcressey]

Issue number:
Related: #1667

Description of changes:
For every Go package that ran afoul of the new check-fips script, I added "bin" and "fips-bin" subpackages with the corresponding binaries. Apart from the packaging metadata, the core change is very simple: build each binary twice, first with go build and then with gofips build.

For cases where build arguments would lead to excessive duplication or very long lines, I moved them into arrays so the copy/paste wasn't quite as offensive.

I also touched a handful of Go packages that didn't need to be built twice, to drop arguments like -buildmode=pie that come from GOFLAGS.

Testing done:
Ran a variety of variants - aws-dev, aws-k8s-1.24, aws-k8s-1.28-nvidia, aws-ecs-1, aws-ecs-2 - built with and without the FIPS flag set. The nodes were able to join clusters and run pods.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

[bcressey]

Each top-level package now depends on a capability like this to ensure that its binaries are installed - since the package wouldn't be very useful without those!

[bcressey]

Each binary package provides the %{name}(binaries) capability that the top-level package needs.

It requires the top-level package (so that dnf install bottlerocket-runc-bin also installs bottlerocket-runc) as well as the corresponding image feature capability from the metadata package. This will either be "fips" or "no-fips" depending on the variant.

Finally, it conflicts with the opposite image feature or the other binary package. The conflict helps the package manager eliminate the "wrong" package from consideration, and also prevents bottlerocket-runc-bin and bottlerocket-runc-fips-bin from being installed together.

[bcressey]

This spec needed one of the largest changes which led to this new macro (discussed in bottlerocket-os/bottlerocket-sdk#162).

Basically the old CGO_ENABLED=0 trick to get a static binary doesn't work for FIPS builds that need AWS-LC, and this macro sets up an environment that leaves CGO enabled and uses our musl-targeted GCC as the external linker, since musl supports a usable form static linking and glibc does not.

[bcressey]

%set_cross_go_flags brings the standard GOFLAGS and GOLDFLAGS into the environment. Afterwards, we need to unset the CC variable or else it'll fail to build the host codegen programs.

The export %{kube_cc} line below (unchanged) is what sets the cross-compiler in the way that the Makefile expects.

[yeazelm]

This looks right to me. The one thing that has me scratching my head is that I noticed the very repetitive and predictable changes made to all the spec files. It is really noticeable in a PR such as this one where we have basically touched all go builds at once. I haven't thought of a simpler way to accomplish this so I'll approve :) But I do wonder if we could code gen or template some of these things more deeply to make edits to something like this easier to do across all the packages at once.

[yeazelm]

I'm not finding a great place to ask this, so this line is as good as any. Do we have a general idea on what should have FIPS and what shouldn't? I think we are finding them all in this PR but I'm unsure how we document what one needs to consider when the next Go package needs to be added to the repo. Is it just "always do these things for all Go packages?" If so, I think I'm fine with that but it might be nice to consider a way for us to avoid having so much repetitive logic for non-FIPS and FIPS builds and to update our guidance for packaging Go-based packages so we don't collectively forget what thought went into this PR later down the line.

[bcressey]

I'm not finding a great place to ask this, so this line is as good as any. Do we have a general idea on what should have FIPS and what shouldn't?

The check-fips script handles this pretty well. It becomes a build error to include a Go binary that imports crypto/tls without also building a FIPS-enabled version of that binary. At that point the packager has to address it. Some packages, like oci-add-hooks, are fine and don't need any special treatment.

I didn't want to gate the use of a new SDK on a PR like this, so the check is currently off by default. Once this merges I can flip that around in the next SDK release.

[larvacea]

The lede cites check-fips as the motivation. Is the criterion "if the binary uses crypto/tls, it needs a FIPS-equivalent using goboringcrypto"?

In the check-fips script, the test applied to the FIPS binary is:

      if ! uses_go_boring_crypto "${f}"; then

Is this sufficient, or should the script test:

      if uses_go_crypto_tls "${f}" then
        echo "${f} is built with crypto/tls" >&2
        (( miscompiled+=1 ))
      elif ! uses_go_boring_crypto "${f}"; then
        echo "${f} is not built with FIPS crypto (but ${b} is built with crypto/tls)" >&2
        (( miscompiled+=1 ))
      fi

I may be overcomplicating this, but I have encountered the "we linked library good-fred, but we also linked library fred-evil-twin, and the TLA customers DO NOT WANT" problem, so I worry.

[yeazelm]

I didn't want to gate the use of a new SDK on a PR like this, so the check is currently off by default. Once this merges I can flip that around in the next SDK release.

This is great, I think this is what I was hoping to see to make sure we don't have drift from the good state after this PR as we make changes to the Go packages.

[bcressey]

I may be overcomplicating this, but I have encountered the "we linked library good-fred, but we also linked library fred-evil-twin, and the TLA customers DO NOT WANT" problem, so I worry.

The Go upstream uses (such as boring.go and notboring.go) are gated behind conditionals like:

//go:build boringcrypto
//go:build !boringcrypto

That is pretty fundamental to how GOEXPERIMENT=boringcrypto is implemented so I am not very worried about including both - the build would fail because of multiple definitions for the same function, for example.

[bcressey]

But I do wonder if we could code gen or template some of these things more deeply to make edits to something like this easier to do across all the packages at once.

I considered having a macro along the lines of %debug_package since the metadata is essentially mechanical, as you say.

I didn't like the idea much because it felt too magical or potentially confusing. It works for debug info because that's a transparent background process that usually doesn't intrude on the actual process of building and installing the software - you don't have to refer to the paths in %install. The packages are defined, the file lists are generated automatically, and the %files section for the package just refers to the list.

For the FIPS / non-FIPS distinction, we could automate the definitions and the file section, but we'd be left with somewhat magical incantations like:

%global fips_package 1

Name: blah

%build
go build ....
gofips build ....

%install
install -d %{buildroot}%{_cross_bindir}
install -p -m 0755 go-bin %{buildroot}%{_cross_bindir}

install -d %{buildroot}%{_cross_fips_bindir}
install -p -m 0755 go-fips-bin %{buildroot}%{_cross_fips_bindir}

%files
<everything except the binaries>

It just felt too weird to populate the buildroot with files - especially the key binaries, like runc for the runc package - and then not list them in %files. Worse, it's actually a packaging error to list them there: they'd be included twice, and potentially both versions would end up on the system at runtime.

And if we don't automate %files for the subpackages, then we end up with sections referring to subpackages that aren't visible in the spec file.

In my mind, that only left a macro to generate the subpackage definitions, like %generate_fips_and_non_fips_subpackages. I suppose that could be OK but I tend to prefer the explicit definitions. They are annoying to add the first time but after that it's just something I skip past unless I'm trying to understand or adjust requirements, at which point it's nice to just have them in plain sight.

[yeazelm]

I suppose that could be OK but I tend to prefer the explicit definitions.

I agree with your all your comments but this one rings true the most to me. The verbosity is annoying at first but it does seem like the spec files have just the right amount of context vs opaque magic that might be hard to understand. I went through several of these general approaches in my head when writing my comment but failed to enumerate them so well so thanks for doing that, it really helps solidify that we arrived at the ideal approach!

[arnaldo2792]

No fips version for this package?

[bcressey]

No fips version for this package?

It doesn't import crypto/tls, so it doesn't need it. oci-add-hooks and ecs-gpu-init were the same way.

[bcressey]

⬆️ force push rebases on develop, fixing conflicts and dropping changes that were merged in separate commits (7bbd73f, 30aeecd, 35c2432).

[webern]

What does this part prevent?

or %{name}-bin

[bcressey]

What does this part prevent?

or %{name}-bin

It's meant to prevent the bin and fips-bin packages from both being installed, since only one set of binaries would end up in use.

It's a bit redundant but hypothetically nothing stops someone from adding Provides: %{_cross_os}image-feature(fips) to a random package in their variant that doesn't have that feature set. So even if that happened, this would force only one of the two packages to be installed.