From 0bdb62aa7f48579d408a5fc2c0dd19297d5b6e3a Mon Sep 17 00:00:00 2001 From: Cam Hutchison Date: Wed, 6 Mar 2024 06:56:37 +1100 Subject: [PATCH] build: Remove docker builder from building buildboxes (#38952) Do not specify the docker builder via `--builder` when using `docker buildx`, as the method of determine the builder name is flawed and is having local developer impact. The original reason for adding it was to fix the CI build of the FIPS buildbox as building the FIPS buildbox requires using the local docker image cache which is only available with the `docker` builder. Instead unify the centos7 and centos7-fips Dockerfiles and use `--target` to select which bit to build up to for the `buildbox-centos7` and `buildbox-centos7-fips` targets. As as multi-stage build, the `docker-container` driver now does see the base buildbox and builds a correct buildbox when not pushing to the registry (as occurs at the start of tag builds). This is the recommended method in the Docker documentation, with a more complex alternative being "bake files". They are more appropriate when the separate image builds are rather separate. In our case, the two are closely related and work well with the simpler multi-stage build approach. This approach should work for any of the docker builders used and allows developers to select whichever builder works for them, while on CI we will continue to use the `docker` default builder. Remove the `PULL_ON_CI` logic as that is not needed because of the use of `--cache-from` and `-cache-to` used for all buildboxes. Use a `make` trick in `build.assets/arch.mk` to only run `go env GOARCH` once max, or not at all if not needed, and use `GOTOOLCHAIN=local` so we don't update the go toolchain unnecessarily. The GitHub Actions runner has an older version of Go than we specify in our `go.mod` so every time `make` ran on a fresh runner, it would download go1.22.0 even though it was not needed. --- .../workflows/build-ci-buildbox-images.yaml | 7 --- build.assets/Dockerfile-centos7 | 43 ++++++++++++++ build.assets/Dockerfile-centos7-fips | 37 ------------ build.assets/Makefile | 58 +++++++------------ build.assets/arch.mk | 11 +++- 5 files changed, 71 insertions(+), 85 deletions(-) delete mode 100644 build.assets/Dockerfile-centos7-fips diff --git a/.github/workflows/build-ci-buildbox-images.yaml b/.github/workflows/build-ci-buildbox-images.yaml index 00fd83ca241..ef66fa7c2ad 100644 --- a/.github/workflows/build-ci-buildbox-images.yaml +++ b/.github/workflows/build-ci-buildbox-images.yaml @@ -7,7 +7,6 @@ on: - build.assets/Dockerfile - build.assets/Dockerfile-arm - build.assets/Dockerfile-centos7 - - build.assets/Dockerfile-centos7-fips - build.assets/Dockerfile-node - build.assets/Makefile - build.assets/images.mk @@ -121,12 +120,6 @@ jobs: - name: Set up Docker Buildx uses: docker/setup-buildx-action@0d103c3126aa41d772a8362f6aa67afac040f80c # v3.1.0 with: - # The image buildbox-centos7-fips builds depends on the image built by its buildbox-centos7 - # dependency, so we need to use the `docker` driver to ensure that buildbox-centos7 is - # available to us locally as a build-arg. - # - # Sticking with the default driver (`docker-container`) would result in us using the - # image from the remote registry, which would be stale. driver: docker - name: Login to registry diff --git a/build.assets/Dockerfile-centos7 b/build.assets/Dockerfile-centos7 index 73036403c69..1b4551d49f3 100644 --- a/build.assets/Dockerfile-centos7 +++ b/build.assets/Dockerfile-centos7 @@ -179,6 +179,8 @@ RUN git clone --depth=1 https://github.com/gravitational/PCSC.git -b ${LIBPCSCLI make install ## BUILDBOX ################################################################### +# Build the `buildbox` target to build the non-fips buildbox +# ============================================================================ FROM base AS buildbox @@ -324,3 +326,44 @@ ENV BORING_BSSL_FIPS_CPLUS_INCLUDE_PATH=/opt/llvm/include/c++/v1/ VOLUME ["/go/src/github.com/gravitational/teleport"] EXPOSE 6600 2379 2380 + + +## BUILDBOX-FIPS ############################################################## +# Build the `buildbox-fips` target to create a buildbox that creates a FIPS +# Teleport binary. It enables FIPS in the Go toolchain and in OpenSSL, and +# builds a test binary to ensure binaries are FIPS-compliant. + +FROM buildbox as buildbox-fips + +# Set environment variables used for enabling FIPS mode +# `GOEXPERIMENT=boringcrypto` -- enable FIPS mode (BoringCrypto) for Go +# https://github.com/golang/go/blob/master/src/crypto/internal/boring/README.md +# `OPENSSL_FIPS=1` -- enable FIPS mode for OpenSSL +# https://www.openssl.org/docs/man3.0/man7/fips_module.html +ENV GOEXPERIMENT=boringcrypto \ + OPENSSL_FIPS=1 + +# Enable OpenSSL FIPS mode by default +# https://www.openssl.org/docs/man3.0/man7/fips_module.html +COPY fips-files/openssh.cnf /usr/local/ssl/openssl.cnf + +USER ci + +# Validate that environment variables were set +RUN echo "Ensure environment variables are set" && \ + [ -n "$GOEXPERIMENT" ] && \ + [ -n "$OPENSSL_FIPS" ] + +# Validate that Go binaries have BoringCrypto enabled +COPY --chown=ci fips-files/boringtest.nogo /tmp/boringtest.go + +RUN echo "Ensure Go is using BoringCrypto" && \ + go run /tmp/boringtest.go + +RUN echo "Ensure OpenSSL is using FIPS module" && \ + ! openssl md5 /tmp/boringtest.go > /dev/null 2>&1 + +RUN rm /tmp/boringtest.go + +VOLUME ["/go/src/github.com/gravitational/teleport"] +EXPOSE 6600 2379 2380 diff --git a/build.assets/Dockerfile-centos7-fips b/build.assets/Dockerfile-centos7-fips deleted file mode 100644 index 17e0fb9d104..00000000000 --- a/build.assets/Dockerfile-centos7-fips +++ /dev/null @@ -1,37 +0,0 @@ -# syntax=docker/dockerfile:1 - -ARG BUILDBOX_CENTOS7 -FROM ${BUILDBOX_CENTOS7} - -# Set environment variables used for enabling FIPS mode -# `GOEXPERIMENT=boringcrypto` -- enable FIPS mode (BoringCrypto) for Go -# https://github.com/golang/go/blob/master/src/crypto/internal/boring/README.md -# `OPENSSL_FIPS=1` -- enable FIPS mode for OpenSSL -# https://www.openssl.org/docs/man3.0/man7/fips_module.html -ENV GOEXPERIMENT=boringcrypto \ - OPENSSL_FIPS=1 - -# Enable OpenSSL FIPS mode by default -# https://www.openssl.org/docs/man3.0/man7/fips_module.html -COPY fips-files/openssh.cnf /usr/local/ssl/openssl.cnf - -USER ci - -# Validate that environment variables were set -RUN echo "Ensure environment variables are set" && \ - [ -n "$GOEXPERIMENT" ] && \ - [ -n "$OPENSSL_FIPS" ] - -# Validate that Go binaries have BoringCrypto enabled -COPY --chown=ci fips-files/boringtest.nogo /tmp/boringtest.go - -RUN echo "Ensure Go is using BoringCrypto" && \ - go run /tmp/boringtest.go - -RUN echo "Ensure OpenSSL is using FIPS module" && \ - ! openssl md5 /tmp/boringtest.go > /dev/null 2>&1 - -RUN rm /tmp/boringtest.go - -VOLUME ["/go/src/github.com/gravitational/teleport"] -EXPOSE 6600 2379 2380 diff --git a/build.assets/Makefile b/build.assets/Makefile index e3b95363ede..6dd62e82ea4 100644 --- a/build.assets/Makefile +++ b/build.assets/Makefile @@ -70,28 +70,12 @@ endif # $(ARCH) is the target architecture we want to build for. REQUIRE_HOST_ARCH = $(if $(filter-out $(ARCH),$(RUNTIME_ARCH)),$(error Cannot cross-compile $@ $(ARCH) on $(RUNTIME_ARCH))) -# PULL_ON_CI is used by the buildbox targets to pull the latest buildbox before building -# a new one so that less work is done if the buildbox is up-to-date already. This is only -# needed on CI as they start with a fresh state each time. -PULL_ON_CI = $(if $(filter $(CI),true),docker inspect --type=image $(1) >/dev/null 2>&1 || docker pull $(1) || true) - # This determines which make target we call in this repo's top level Makefile when # make release in this Makefile is called. Currently this supports its default value # (release) and release-unix-preserving-webassets. See the release-arm target for # more details. RELEASE_TARGET ?= release -# DOCKER_BUILDER_NAME extracts the name of the buildx builder that uses the "docker" driver. -# -# The awk command looks for the first line that has "docker" in the second (DRIVER) column. -# -F '[ *]+': there may be an asterisk between the first and second column if that record is the default, -# so just subsume the asterisk into the field separator. -DOCKER_BUILDER_NAME ?= $(shell docker buildx ls 2>/dev/null | awk -F '[ *]+' '$$2 == "docker" {print $$1}') - -.PHONY:print-docker-builder-name -print-docker-builder-name: - @echo $(DOCKER_BUILDER_NAME) - # # Build 'teleport' release inside a docker container # @@ -134,9 +118,7 @@ build-binaries-fips: buildbox-centos7-fips webassets # .PHONY:buildbox buildbox: - $(call PULL_ON_CI,$(BUILDBOX)) docker buildx build \ - --builder $(DOCKER_BUILDER_NAME) \ --build-arg UID=$(UID) \ --build-arg GID=$(GID) \ --build-arg BUILDARCH=$(RUNTIME_ARCH) \ @@ -175,9 +157,8 @@ buildbox-fips: buildbox-centos7-fips .PHONY:buildbox-centos7 buildbox-centos7: $(REQUIRE_HOST_ARCH) - $(call PULL_ON_CI,$(BUILDBOX_CENTOS7)) docker buildx build \ - --builder $(DOCKER_BUILDER_NAME) \ + --target buildbox \ --build-arg UID=$(UID) \ --build-arg GID=$(GID) \ --build-arg BUILDBOX_CENTOS7_ASSETS=$(BUILDBOX_CENTOS7_ASSETS) \ @@ -195,25 +176,31 @@ buildbox-centos7: --tag $(BUILDBOX_CENTOS7) -f Dockerfile-centos7 . # -# Builds a Docker buildbox for CentOS 7 FIPS builds -# Uses regular CentOS 7 buildbox as base, so limited arguments are needed. -# -# Because this target uses the image built by the buildbox-centos7 target, it is -# expected to be run with a buildx builder that uses the "docker" driver. If another -# driver is used, the --build-arg BUILDBOX_CENTOS7=$(BUILDBOX_CENTOS7)-$(RUNTIME_ARCH) -# will not know about the local image that was just built, and will pull the image from -# the Docker repository instead, which will be out of date. +# Builds a Docker buildbox for CentOS 7 FIPS builds. This uses the same +# Dockerfile as `buildbox-centos7` but with the `buildbox-fips` target which +# sets the appropriate environment variables and config files to build +# FIPS-compliant binaries. It also does a test build to ensure the built +# binaries are FIPS-compliant. # .PHONY:buildbox-centos7-fips -buildbox-centos7-fips: buildbox-centos7 - $(call PULL_ON_CI,$(BUILDBOX_CENTOS7_FIPS)) +buildbox-centos7-fips: docker buildx build \ - --builder $(DOCKER_BUILDER_NAME) \ - --build-arg BUILDBOX_CENTOS7=$(BUILDBOX_CENTOS7) \ + --target buildbox-fips \ + --build-arg UID=$(UID) \ + --build-arg GID=$(GID) \ + --build-arg BUILDBOX_CENTOS7_ASSETS=$(BUILDBOX_CENTOS7_ASSETS) \ + --build-arg BUILDARCH=$(HOST_ARCH) \ + --build-arg TARGETARCH=$(RUNTIME_ARCH) \ + --build-arg GOLANG_VERSION=$(GOLANG_VERSION) \ + --build-arg RUST_VERSION=$(RUST_VERSION) \ + --build-arg WASM_PACK_VERSION=$(WASM_PACK_VERSION) \ + --build-arg DEVTOOLSET=$(DEVTOOLSET) \ + --build-arg LIBBPF_VERSION=$(LIBBPF_VERSION) \ + --build-arg LIBPCSCLITE_VERSION=$(LIBPCSCLITE_VERSION) \ --cache-to type=inline \ --cache-from $(BUILDBOX_CENTOS7_FIPS) \ $(if $(PUSH),--push,--load) \ - --tag $(BUILDBOX_CENTOS7_FIPS) -f Dockerfile-centos7-fips . + --tag $(BUILDBOX_CENTOS7_FIPS) -f Dockerfile-centos7 . # # Builds a Docker buildbox for ARMv7/ARM64 builds @@ -222,9 +209,7 @@ buildbox-centos7-fips: buildbox-centos7 # .PHONY:buildbox-arm buildbox-arm: - $(call PULL_ON_CI,$(BUILDBOX_ARM)) docker buildx build \ - --builder $(DOCKER_BUILDER_NAME) \ --build-arg UID=$(UID) \ --build-arg GID=$(GID) \ --build-arg BUILDARCH=$(RUNTIME_ARCH) \ @@ -246,9 +231,7 @@ endif # .PHONY:buildbox-node buildbox-node: - $(call PULL_ON_CI,$(BUILDBOX_NODE)) docker buildx build \ - --builder $(DOCKER_BUILDER_NAME) \ --build-arg BUILDARCH=$(RUNTIME_ARCH) \ --build-arg UID=$(UID) \ --build-arg GID=$(GID) \ @@ -635,7 +618,6 @@ print-buildbox-version: .PHONY:build-centos7-assets build-centos7-assets: docker buildx build \ - --builder $(DOCKER_BUILDER_NAME) \ --build-arg BUILDARCH=$(HOST_ARCH) \ --build-arg DEVTOOLSET=$(DEVTOOLSET) \ --build-arg TARGETARCH=$(RUNTIME_ARCH) \ diff --git a/build.assets/arch.mk b/build.assets/arch.mk index 3c19b87ce8f..485f109dde2 100644 --- a/build.assets/arch.mk +++ b/build.assets/arch.mk @@ -1,9 +1,14 @@ # These variables are extracted from build.assets/Makefile so they can be imported # by other Makefiles -ifeq ($(origin ARCH), undefined) # avoid use of "ARCH ?= $(...)" as the lazy loading will repeatedly re-run the same shell command - ARCH := $(shell go env GOARCH) -endif + +# Eval $(ARCH) one time max, or not at all if $(ARCH) is not used. +# Calling `go` can emit an error if not installed, which we do not +# want to do if $(ARCH) is never used. +# https://make.mad-scientist.net/deferred-simple-variable-expansion/ +ARCH = $(eval ARCH := $$(shell GOTOOLCHAIN=local go env GOARCH))$(ARCH) + HOST_ARCH := $(shell uname -m) + RUNTIME_ARCH_x86_64 := amd64 # uname returns different value on Linux (aarch64) and macOS (arm64). RUNTIME_ARCH_arm64 := arm64