SLSA attestations for the Build resource (aka app image attestations) #1449

chenbh · 2023-12-13T22:45:50Z

Implementation of #1374, the feature is marked experimental until the RFC has been merged.

This PR adds SLSA attestations to the Build resource:

EXPERIMENTAL_GENERATE_SLSA_ATTESTATION="true" can be set on the controller to enable these behaviours
A new status .status.latestAttestationImage will stamp out the immutable digest of the attestation
The attestation is also pushed to a mutable image tag based off of the app image's digest (digest my-app-image@sha:1234 will have the attestation stored at tag my-app-image:sha256-1234). This is consistent with how cosign stores their attestations and can be verified with cosign verify-attestation ...
Recommend viewing signed images with cosign verify-attestation --type=slsaprovenance1 ..., alternatively unsigned or signed attestations can be viewed with
```
crane export <ATTESTATION_IMAGE> - | jq -r '.payload' | base64 -D | jq
```
Any secrets attached to the Build's service account, or the kpack controller's service account with the annotation kpack.io/slsa: "" will be used as private keys for signing
Signing RSA, ECDSA, ED25519, and Cosign keys are supported, the way to use them is specified in the RFC

Things still needed:

E2E tests
Docs about how to use/configure it
Docs about SLSA level provided by kpack

codecov-commenter · 2023-12-13T22:49:10Z

Codecov Report

Attention: 200 lines in your changes are missing coverage. Please review.

Comparison is base (155c8a7) 67.17% compared to head (2046f40) 67.19%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
pkg/reconciler/build/build.go	50.00%	39 Missing and 12 partials ⚠️
pkg/slsa/sign.go	57.14%	24 Missing and 12 partials ⚠️
pkg/slsa/attest.go	83.67%	17 Missing and 7 partials ⚠️
pkg/slsa/image.go	51.06%	16 Missing and 7 partials ⚠️
pkg/slsa/pkcs8_signer.go	67.30%	14 Missing and 3 partials ⚠️
pkg/blob/fetch.go	60.52%	10 Missing and 5 partials ⚠️
pkg/cosign/image_signer.go	50.00%	6 Missing and 4 partials ⚠️
pkg/secret/signing_secret.go	83.92%	6 Missing and 3 partials ⚠️
pkg/registry/fetch.go	73.07%	4 Missing and 3 partials ⚠️
pkg/config/lifecycle_provider.go	50.00%	2 Missing and 1 partial ⚠️
... and 2 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1449      +/-   ##
==========================================
+ Coverage   67.17%   67.19%   +0.02%     
==========================================
  Files         134      140       +6     
  Lines        8276     8827     +551     
==========================================
+ Hits         5559     5931     +372     
- Misses       2264     2389     +125     
- Partials      453      507      +54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chenbh · 2024-01-05T17:53:59Z

@matthewmcnew @samj1912 @AidanDelaney can I get some non-Broadcom 👀 on this or the RFC?

config/controller.yaml

AidanDelaney

I hate to provide a partial review, but I have limited time today. Will complete the review tomorrow. Most of my comments are trivial and can be ignored as style changes. I have one question on whether the start/stop time is necessary in an attestation.

pkg/blob/fetch.go

AidanDelaney · 2024-01-08T13:19:02Z

pkg/cosign/image_signer.go

-		if _, found := os.LookupEnv(cosignutil.CosignDockerMediaTypesEnv); found {
-			err = os.Unsetenv(cosignutil.CosignDockerMediaTypesEnv)
+		if _, found := os.LookupEnv(CosignDockerMediaTypesEnv); found {
+			err = os.Unsetenv(CosignDockerMediaTypesEnv)


Is this to prevent an end-user from injecting an incorrect media type through an environmental variable?

I believe this is to avoid pollution from having multiple signing secrets. Since each signing secret specifies the media type to be generated, if the current secret doesn't specify a format, we should use cosign's default type instead of whatever the previous secret used.

The way we do cosign is also a bit interesting because the builder signing component (SignBuilder) is done in the controller container, but the app image signing (Sign) is done in the build pod.

I do want to unify them sometime in the future (currently playing around with the idea of a new pod/deployment in the kpack namespace that handles both signing and attestation), but for now the 2 methods of this ImageSigner object is used in completely different codepaths

AidanDelaney · 2024-01-08T13:21:21Z

pkg/cosign/image_signer_test.go


 				report = createReportToml(t, expectedImageName, imageDigest)

-				os.Unsetenv(cosignutil.CosignRepositoryEnv)
-				os.Unsetenv(cosignutil.CosignDockerMediaTypesEnv)
+				os.Unsetenv(CosignRepositoryEnv)


Do we have any guarantees about the order of the tests? AFAIK unsetting these env vars here have an impact on tests that run subsequently. Do we want to change the environment for subsequent tests?

This is more of a preliminary cleaning of the test environment, any test that requires the env var will set it explicitly. Technically it should be enough to unset it at the end of the tests, but I guess cleaning before the tests protects against accidentally inheriting env vars from the shell when running the tests.

AidanDelaney · 2024-01-08T13:35:33Z

pkg/secret/signing_secret.go

+	return extractAttestationKeyFromSecrets(cosignSecrets, privKeySecrets)
+}
+
+func filterCosignSecrets(serviceAccountSecrets []*corev1.Secret, annotation string) []*corev1.Secret {


Is this filterCosignSecrets or gatherCosignSecrets? I'd expect a filter to apply a predicate to some collection. Here it looks like we're gathering the secrets and packing them into a struct, should they exist.

I'm indifferent to either name. I would say that this function does apply a predicate: it throws away any items that doesn't have certain annotations. It's return value is also a list of the same type as the input list, it's even in the same order.

pkg/slsa/attest.go

before we were only generating it for the git source type. this eventually gets turned into a label on the image as specified in https://github.com/buildpacks/spec/blob/main/platform.md#iobuildpacksprojectmetadata-json Signed-off-by: Bohan Chen <[email protected]>

todo: - extract source from app image labels - record builder image Signed-off-by: Bohan Chen <[email protected]>

The source metadata is read in from the project-metadata label on the built image. This is done for 2 reasons: 1. It resolves floating pointers to concrete shas, this can be the case with a git source pointing to a branch, or a registry source pointing to a tag. We also can't figure out a blob's checksum without actually pulling it 2. It guarantees that this is the version pulled down by the `prepare` step and avoid weird TOCTOU edge cases where the underlying bits changed between the creation of the Build and the execution of the container By exporting the Project, Source, Metadata, and Version structs, I hope to create a code relation between the different packages, so that if we every change one in the future, the compiler will yell at us instead of us having to remember the relationship. Signed-off-by: Bohan Chen <[email protected]>

supported key types are rsa, ecdsa, ed25519, and cosign (which is basically a wrapper around the other 3). there are some smoke tests with hard coded signatures, but i feel the main tests should be the interop with cosign cli Signed-off-by: Bohan Chen <[email protected]>

since we already have a package dedicated to grabbing secrets out of k8s, lets expand it further so that it knows how to extract signing keys out of it. Signed-off-by: Bohan Chen <[email protected]>

tomkennedy513

this is awesome

natalieparellano

Amazing work. I only reviewed the docs files

docs/slsa.md

- split out signing and pushing into different functions mainly to make the error messages clearer - move attestation sign and write functions to obj so that we can mock out the attester interface later - also change the slsa buildid to include `slsa` as a subpath to make it clearer what its used for Signed-off-by: Bohan Chen <[email protected]>

- the controller is now aware of its own service account name, this is to fetch all the secrets associated with it (currently for attestation only, but can be expanded to cosign later) - the feature flag for enabling slsa is marked experimental since the RFC hasn't been merged in yet Signed-off-by: Bohan Chen <[email protected]>

rename slsa.go as the name wasn't really descriptive of what its doing bugs: - client-go strips the TypeMeta from the object so we have to pass it in manually. - granting rbac for the controller to get namespaces - and using version instead of identifier so we don't get the git sha when generating the build type link - ecdsa signing requires digest not full msg - stick to consistent tense for err msgs - and make the start/stop time parsing best effort - the slsa spec says its optional so we shouldn't fail the whole attestation if we can't find it Signed-off-by: Bohan Chen <[email protected]>

since tests are run in parallel, cluster-scoped resources needed to be created with a different name for each of the suites Signed-off-by: Bohan Chen <[email protected]>

Signed-off-by: Bohan Chen <[email protected]>

chenbh force-pushed the implement-slsa branch 2 times, most recently from 09bc5a4 to 755cdc1 Compare December 15, 2023 20:57

chenbh marked this pull request as ready for review December 15, 2023 20:57

chenbh requested a review from a team as a code owner December 15, 2023 20:57

chenbh changed the title ~~Implement SLSA attestations~~ SLSA attestations for the Build resource (aka app image attestations) Dec 15, 2023

chenbh force-pushed the implement-slsa branch from ee57012 to 84c91fa Compare January 5, 2024 17:00

tomkennedy513 reviewed Jan 5, 2024

View reviewed changes

config/controller.yaml Show resolved Hide resolved

AidanDelaney reviewed Jan 8, 2024

View reviewed changes

chenbh force-pushed the implement-slsa branch from 7401311 to 592cada Compare January 10, 2024 20:13

chenbh mentioned this pull request Jan 10, 2024

Generate project-metadata.toml for all source types #1437

Closed

chenbh force-pushed the implement-slsa branch 3 times, most recently from 264b3bc to 46fd2ec Compare January 10, 2024 20:36

chenbh added 5 commits January 12, 2024 09:58

add support for basic slsa statements

67adab8

todo: - extract source from app image labels - record builder image Signed-off-by: Bohan Chen <[email protected]>

move slsa and cosign logic into secrets pkg

6966b3f

since we already have a package dedicated to grabbing secrets out of k8s, lets expand it further so that it knows how to extract signing keys out of it. Signed-off-by: Bohan Chen <[email protected]>

chenbh force-pushed the implement-slsa branch from 0cbef2f to 6323fde Compare January 12, 2024 14:58

tomkennedy513 approved these changes Jan 12, 2024

View reviewed changes

natalieparellano reviewed Jan 17, 2024

View reviewed changes

docs/slsa.md Outdated Show resolved Hide resolved

docs/slsa.md Outdated Show resolved Hide resolved

chenbh added 6 commits January 17, 2024 15:29

add slsa tests and refactor how tests are run

d94c937

since tests are run in parallel, cluster-scoped resources needed to be created with a different name for each of the suites Signed-off-by: Bohan Chen <[email protected]>

document slsa

18ef43d

Signed-off-by: Bohan Chen <[email protected]>

enable slsa attestations in gha e2e

2046f40

Signed-off-by: Bohan Chen <[email protected]>

chenbh force-pushed the implement-slsa branch from 6323fde to 2046f40 Compare January 17, 2024 20:32

chenbh merged commit 3344d70 into main Jan 17, 2024
3 checks passed

chenbh deleted the implement-slsa branch January 17, 2024 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SLSA attestations for the Build resource (aka app image attestations) #1449

SLSA attestations for the Build resource (aka app image attestations) #1449

chenbh commented Dec 13, 2023 •

edited

Loading

codecov-commenter commented Dec 13, 2023 •

edited

Loading

chenbh commented Jan 5, 2024

AidanDelaney left a comment

AidanDelaney Jan 8, 2024

chenbh Jan 8, 2024

chenbh Jan 8, 2024

AidanDelaney Jan 8, 2024

chenbh Jan 8, 2024

AidanDelaney Jan 8, 2024

chenbh Jan 8, 2024

tomkennedy513 left a comment

natalieparellano left a comment

SLSA attestations for the Build resource (aka app image attestations) #1449

SLSA attestations for the Build resource (aka app image attestations) #1449

Conversation

chenbh commented Dec 13, 2023 • edited Loading

codecov-commenter commented Dec 13, 2023 • edited Loading

Codecov Report

chenbh commented Jan 5, 2024

AidanDelaney left a comment

Choose a reason for hiding this comment

AidanDelaney Jan 8, 2024

Choose a reason for hiding this comment

chenbh Jan 8, 2024

Choose a reason for hiding this comment

chenbh Jan 8, 2024

Choose a reason for hiding this comment

AidanDelaney Jan 8, 2024

Choose a reason for hiding this comment

chenbh Jan 8, 2024

Choose a reason for hiding this comment

AidanDelaney Jan 8, 2024

Choose a reason for hiding this comment

chenbh Jan 8, 2024

Choose a reason for hiding this comment

tomkennedy513 left a comment

Choose a reason for hiding this comment

natalieparellano left a comment

Choose a reason for hiding this comment

chenbh commented Dec 13, 2023 •

edited

Loading

codecov-commenter commented Dec 13, 2023 •

edited

Loading