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

Adding release Dockerfiles #601

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Conversation

mheese
Copy link
Contributor

@mheese mheese commented Jun 7, 2023

This PR aims to add a Dockerfile for a release version of the rust keylime agent.

This is a work-in-progress at this point. It is being discussed in the keylime-operator slack channel.

It currently contains a local build script like in the main keylime repository. Also, it contains different versions which have their advantages/disadvantages that we should discuss:

  • Dockerfile.distroless: an absolute bare minimum container which does not even have a shell. It is based on the "distroless" container effort from Google (which for example forms the base images for a couple of bigger projects like Kubernetes)
  • Dockerfile.wolfi: also a bare minimum container which does not even have a shell. It is based on the wolfi container distribution from Chainguard which in turn was inspired by Google's distroless and ko projects. Its focus is a dedicated container distribution which heavily focuses on security, frequent updates and updates faster/more often than distroless
  • Dockerfile.fedora: is based on Fedora 38. This is a "fat" image, but both the build stage as well as the assembly stage are based upon Fedora 38. This is probably closer to what we know works.

Here a comparison of the two versions:

Dockerfile.distroless

  • builder stage based on official Rust Docker images
  • official Rust image (which is chosen here) is based on Debian 11
  • has an additional custom compilation/installation stage for libarchive in the builder stage with minimal options for libarchive (which are enough for our purposes)
  • has an additional compilation/installation stage for tpm2-tss with minimal options which should be enough for our purposes (fapi and fapi policy libs are disabled)
  • the assembly stage uses gcr.io/distroless/cc as a base image which is a minimal no-shell base image including C runtime dependencies (libc, libgcc, etc.) as well as OpenSSL and root CA certificates. This image is intended for use by languages like Rust or D according to its documentation
  • the distroless images are all based on Debian 11 at this point in time, so this makes the build and assembly compatible
  • dependencies and compiled agent are copied from the builder stage

Pros of this approach are IMHO:

  • maximized for security as it truly does not contain what is not needed
  • image size is quite good (still bigger than I expected though)
  • good control over used versions of dependencies and rust version etc.

Cons of this approach are IMHO:

  • based on Debian 11 which uses oldish versions of things - probably not an issue at all though, and it does have all security patches after all
  • OpenSSL version is 1.1 here - again, not sure if this is an issue for us
  • harder to debug (because there is no shell)

image size:

REPOSITORY                                                         TAG       IMAGE ID       CREATED          SIZE
keylime_agent                                                      latest    3ce6c74b8a73   22 minutes ago   57.6MB

Dockerfile.wolfi

  • builder stage based on wolfi-base docker images
  • rust is being installed through wolfi which is the latest version - unfortunately Chainguard's rust image does not work because it does not allow to add dependencies
  • has an additional compilation/installation stage for tpm2-tss with minimal options which should be enough for our purposes (fapi and fapi policy libs are disabled)
  • the assembly stage uses cgr.dev/chainguard/cc-dynamic as a base image which is a minimal no-shell base image including C runtime dependencies (libc, libgcc, etc.). This image is intended for use by languages like Rust even according to Chainguard's rust image documentation
  • both builder and assembly stage images use the same base, so there is no potential incompatibilities between binaries/libraries
  • dependencies and compiled agent are copied from the builder stage

Pros of this approach are IMHO:

  • maximized for security as it truly does not contain what is not needed
  • even better than distroless in regards to security IMHO
  • image size is quite good (still bigger than I expected though)
  • good control over used versions of dependencies and rust version etc.

Cons of this approach are IMHO:

  • Chainguard is a relatively new player in this game (not sure that matters, just potentially for long-term maintainability)
  • harder to debug (because there is no shell)

image size:

REPOSITORY                                                         TAG                        IMAGE ID       CREATED          SIZE
keylime_agent                                                      latest                     367bada26999   6 minutes ago    42.4MB

Dockerfile.fedora

  • builder stage uses Fedora 38 like the existing docker image for testing
  • all build dependencies are installed through Fedora packages
  • runtime dependencies are installed in the assembly stage (as compared to copied)

Pros of this approach are IMHO:

  • probably the closest to what we know works
  • easier to debug (has shell and tools, etc.)

Cons of this approach are IMHO:

  • image size is "fat" for an agent which should be very small
  • still contains a lot of tools and things (including package manager) which aren't necessary and are just an increase in attack surface
  • even the keylime agent has a lot of library dependencies because of its C library dependencies that are for sure not required at all and are solely there to satisfy linker requirements

image size:

REPOSITORY                                                         TAG       IMAGE ID       CREATED             SIZE
keylime_agent                                                      latest    cbf68e330487   17 seconds ago      203MB

@aplanas
Copy link
Contributor

aplanas commented Jun 7, 2023

Very nice. Some time ago I did one for openSUSE:

https://build.opensuse.org/package/show/devel:microos:containers/rust-keylime-image

Maybe we can find some common rules

@mheese
Copy link
Contributor Author

mheese commented Jun 7, 2023

Very nice. Some time ago I did one for openSUSE:

https://build.opensuse.org/package/show/devel:microos:containers/rust-keylime-image

Maybe we can find some common rules

Nice! I like the podman labels and volumes, healthcheck and stopsignal definitions. As I was writing this with Kubernetes in my mind, I did not consider any of these. As they are not mutually exclusive though, they should be included imho. I'll try to incorporate those.

Copy link
Contributor

@ansasaki ansasaki left a comment

Choose a reason for hiding this comment

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

I suggested some changes to reduce the fedora-based image, basically using fedora-minimal as the base image.
With this change, I get:

REPOSITORY                                                  TAG               IMAGE ID      CREATED         SIZE
localhost/keylime_agent                                     latest            9c9c97702d63  17 minutes ago  124 MB

docker/release/Dockerfile.fedora Outdated Show resolved Hide resolved
docker/release/Dockerfile.fedora Outdated Show resolved Hide resolved
docker/release/Dockerfile.fedora Outdated Show resolved Hide resolved
docker/release/Dockerfile.fedora Outdated Show resolved Hide resolved
docker/release/Dockerfile.fedora Outdated Show resolved Hide resolved
docker/release/Dockerfile.fedora Outdated Show resolved Hide resolved
@ansasaki
Copy link
Contributor

ansasaki commented Jun 9, 2023

/packit retest-failed

@stringlytyped
Copy link

This is great stuff! When I put together my Dockerfiles, I had a slightly different use case in mind as my goal was primarily to support our internal KL development efforts at HPE. Distroless release images were at the back of my mind as a stretch goal that I just never got around to. So I am really happy to see this PR!

I've had a look through the files and had some notes/queries:

  • I noticed that swtpm is getting installed as a dependency in Dockerfile.fedora but does not seem to get copied to the release image in the assembly stage. Is this needed to build the agent? (I don't see this dependency in the distroless Dockerfile.)
  • Similarly, I am not sure if tpm2-tools and tpm2-abrmd are necessary.
  • It might be good if the files could copy the default config file over to /etc/keylime. Doing so would not preclude the user from supplying their own config file by mounting a volume, or overwriting the defaults with environment variables.
  • You mention that the Google distroless images are based on the previous Debian release. Might you consider using Chainguard images instead? They use a custom distro which is supposed to get security updates faster than other images. They've published a blog post discussing their differences with Google's distroless effort here. And their Rust image is here, if you are curious.
  • I like that your build script passes the version through as build arg instead of the template approach taken by the main repo.

@mheese
Copy link
Contributor Author

mheese commented Jun 16, 2023

This is great stuff! When I put together my Dockerfiles, I had a slightly different use case in mind as my goal was primarily to support our internal KL development efforts at HPE. Distroless release images were at the back of my mind as a stretch goal that I just never got around to. So I am really happy to see this PR!

thanks! 👍

I've had a look through the files and had some notes/queries:

* I noticed that swtpm is getting installed as a dependency in `Dockerfile.fedora` but does not seem to get copied to the release image in the assembly stage. Is this needed to build the agent? (I don't see this dependency in the distroless `Dockerfile`.)

* Similarly, I am not sure if tpm2-tools and tpm2-abrmd are necessary.

I'm pretty sure they can be removed (actually very certain). The base for the Fedora builder stage was copy&pasted from the existing Dockerfile. I have to admit that I did not carefully review everything that gets installed there. Good catch!

* It might be good if the files could copy the [default config file](https://github.com/keylime/rust-keylime/blob/master/keylime-agent.conf) over to `/etc/keylime`. Doing so would not preclude the user from supplying their own config file by mounting a volume, or overwriting the defaults with environment variables.

Yes, I like that. Good point!

* You mention that the Google distroless images are based on the previous Debian release. Might you consider using Chainguard images instead? They use a custom distro which is supposed to get security updates faster than other images. They've published a blog post discussing their differences with Google's distroless effort [here](https://dev.to/chainguard/container-images-for-the-cloud-native-era-2mk1). And their Rust image is [here](https://edu.chainguard.dev/chainguard/chainguard-images/reference/rust/overview/), if you are curious.

Tbh, I never even heard of Chainguard/Wolfi before. I will definitely check it out. The idea here was to propose several files and discuss it with the community. Also, it's not uncommon for some projects to provide multiple tags with different base images.

What I like about the distroless images is that they are "as open as it gets" from an Open Source perspective. And they are well respected as base images in the Kubernetes community.

From a quick look at the links you provided it is definitely worth looking into Chainguard/Wolfi and also have an option for that. I like the SBOMs and cosign/sigstore signatures for sure. It's very much aligned with what we are doing at Hedgehog ourselves.

* I like that your build script passes the version through as build arg instead of the template approach taken by the main repo.

Yeah, no need to template it just because of that.

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #601 (8df6511) into master (88e033c) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 59.46% <ø> (ø)
upstream-unit-tests 59.81% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@mheese mheese marked this pull request as ready for review June 22, 2023 05:00
@mheese
Copy link
Contributor Author

mheese commented Jun 22, 2023

The images now work like this tested with the Helm charts in: keylime/attestation-operator#13

The Rust Static code check currently fails with a security vulnerability check problem: openssl needs to be upgraded!

Should I do this just in this PR, or create a separate one for that?

@ansasaki
Copy link
Contributor

The images now work like this tested with the Helm charts in: keylime/attestation-operator#13

The Rust Static code check currently fails with a security vulnerability check problem: openssl needs to be upgraded!

Should I do this just in this PR, or create a separate one for that?

There is one open already by dependabot: #605

@mheese mheese mentioned this pull request Jun 27, 2023
21 tasks
Comment on lines +62 to +63
# define the stopsignal to be SIGINT like the systemd service file does
STOPSIGNAL SIGINT
Copy link
Contributor

Choose a reason for hiding this comment

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

With #613, this should not be necessary anymore

Comment on lines +104 to +105
# define the stopsignal to be SIGINT like the systemd service file does
STOPSIGNAL SIGINT
Copy link
Contributor

Choose a reason for hiding this comment

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

With #613, this should not be necessary anymore

Comment on lines +93 to +94
# define the stopsignal to be SIGINT like the systemd service file does
STOPSIGNAL SIGINT
Copy link
Contributor

Choose a reason for hiding this comment

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

With #613, this should not be necessary anymore

@ansasaki
Copy link
Contributor

@mheese Could you please rebase this?

@mheese
Copy link
Contributor Author

mheese commented Jul 11, 2023

@ansasaki rebased and force-pushed 👍

with regards to your comment about the STOPSIGNAL not being necessary anymore because of #613 , I'll make another PR where I'm going to remove this as well as the systemd service line, and I'll cross-reference it

@ansasaki ansasaki merged commit 09cbb7d into keylime:master Jul 12, 2023
@mheese mheese deleted the release-docker branch July 12, 2023 15:11
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.

7 participants