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

RFC: SLSA attestation support #1374

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

RFC: SLSA attestation support #1374

wants to merge 5 commits into from

Conversation

chenbh
Copy link
Contributor

@chenbh chenbh commented Nov 13, 2023

@chenbh chenbh requested a review from a team as a code owner November 13, 2023 21:59
@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (759bd29) 67.30% compared to head (b20b1ea) 67.37%.
Report is 43 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1374      +/-   ##
==========================================
+ Coverage   67.30%   67.37%   +0.06%     
==========================================
  Files         133      134       +1     
  Lines        8228     8339     +111     
==========================================
+ Hits         5538     5618      +80     
- Misses       2237     2267      +30     
- Partials      453      454       +1     

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

rfcs/0000-slsa-attestation.md Outdated Show resolved Hide resolved
Comment on lines 142 to 180
"annotations": {
"paketo-buildpacks/go-build": "2.1.0",
"paketo-buildpacks/go-mod-vendor": "1.0.25",
"paketo-buildpacks/node-engine": "3.0.1",
"paketo-buildpacks/go-dist": "2.4.2",
"paketo-buildpacks/yarn": "1.2.0",
"paketo-buildpacks/liberty": "3.8.9",
"paketo-buildpacks/bellsoft-liberica": "10.4.2",
"paketo-buildpacks/spring-boot": "5.27.5",
"paketo-buildpacks/google-stackdriver": "8.0.3",
"paketo-buildpacks/ca-certificates": "3.6.6",
"paketo-buildpacks/ca-certificates": "3.6.5",
"paketo-buildpacks/apache-tomee": "1.7.7",
"paketo-buildpacks/apache-tomcat": "7.13.15",
"paketo-buildpacks/azure-application-insights": "5.17.1",
"paketo-buildpacks/procfile": "5.6.7",
"paketo-buildpacks/procfile": "5.6.6",
"paketo-buildpacks/java-memory-assistant": "1.4.8",
"paketo-buildpacks/datadog": "4.3.0",
"paketo-buildpacks/encrypt-at-rest": "4.5.9",
"paketo-buildpacks/maven": "6.15.11",
"paketo-buildpacks/gradle": "7.6.1",
"paketo-buildpacks/sbt": "6.12.9",
"paketo-buildpacks/clojure-tools": "2.8.12",
"paketo-buildpacks/leiningen": "4.6.8",
"paketo-buildpacks/watchexec": "2.8.6",
"paketo-buildpacks/watchexec": "2.8.5",
"paketo-buildpacks/syft": "1.39.0",
"paketo-buildpacks/jattach": "1.4.8",
"paketo-buildpacks/executable-jar": "6.8.2",
"paketo-buildpacks/git": "1.0.7",
"paketo-buildpacks/dist-zip": "5.6.7",
"paketo-buildpacks/environment-variables": "4.5.6",
"paketo-buildpacks/environment-variables": "4.5.5",
"paketo-buildpacks/image-labels": "4.5.5",
"paketo-buildpacks/image-labels": "4.5.4",
"paketo-buildpacks/java": "10.3.1",
"paketo-buildpacks/go": "4.6.0"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While this records the version of buildpacks that was used in the builder, it doesn't record the order. I feel like if you have the builder image's digest it shouldn't matter too much since you're more interested in the integrity of the build rather than reproducing it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we expect consumers of the SLSA provenance to have access to the builder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to interpret your question as "do we expect them to have access to the registry that the builder and app images are pushed to" and not "do we expect them to have permissions to pull the builder image". I consider permissions a human/procedural problem and not an infrastructure concern.

If they don't care about build reproducibility then it shouldn't matter, simply recording the digest is enough to verify the integrity of the build.

If they do want to be able to reproduce the build bit-for-bit, then there's really no way to do it without having access to the registry. Because if you want to completely recreate the builder from yaml, then at some point you would need the buildpackage images (and the stack images) which also requires you to have the access to the registry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like if you have the builder image's digest it shouldn't matter too much since you're more interested in the integrity of the build rather than reproducing it.

How do we define the integrity of the build? I asked because this provides the buildpacks under the assumption that the builder digest and the buildpacks are sufficient. But, can't the output of the build be different with different orders of buildpacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the core of my thinking is that anything with a checksum can be treated as a immutable basic dependency and verifying it is outside the scope of SLSA (or at least kpack's involvement in SLSA). And from my understanding, a different order would result in a different image config which would result in a different digest.

To put it differently, I think kpack should treat the builder image as a dependency from an external system, not an artifact that was generated by kpack itself. This is to further emphasize that SLSA should apply to only the Build resource, not the entire system that kpack provides (ClusterStack, Buildpacks, Builder, Image).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the core of my thinking is that anything with a checksum can be treated as a immutable basic dependency and verifying it is outside the scope of SLSA (or at least kpack's involvement in SLSA)

The reason I hold this axiom is that it would get really ugly if we have to start considering "how can I verify that the checksums in the go.sum of my app is correct?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking about it a bit more, I think I'm going to generalize the annotations section to be "the labels associated with the builder image". This would save us from doing any special parsing of the image (like figuring out buildpack version or order grouping), while surfacing more metadata (stack id, release date, distro, etc) and being more future-proof.

Comment on lines 275 to 277
- [cosign key][cosign-key-format]. This is to ensure interop with `cosign verify-attestation`.
- ECDSA private keys. This is due to it being the recommended SLSA suite.
- RSA private keys. This is due to it being an extreamly popular key type.
Copy link
Contributor Author

@chenbh chenbh Nov 13, 2023

Choose a reason for hiding this comment

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

The only verification tool that works with key-based attestation is cosign, but they don't support non cosign-format keys.

So if there's no (nice) way of verifying ECDSA and RSA key signatures, should we even support it?

Signed-off-by: Bohan Chen <[email protected]>
Signed-off-by: Daniel Chen <[email protected]>
The builder annotation change is motivated by not wanting to keep up to
date with how to parse the builder metadata.

The signing key annotation change is to make it possible to use cosign
secrets for signing or attesting only without opting into both.

Also add ED25519 since the golang crypto/x509 lib supports all 3 in the
PKCS8 implementation

Signed-off-by: Bohan Chen <[email protected]>
There's actually 2 images that are completely generated by kpack (i.e.
not brought in by user). The builder image is stiched together from
existing layers, and the app image that is build via the builder image.

While the app image attestation is likely the most important one, we
should still attest that our builder images are not tempered with

Signed-off-by: Bohan Chen <[email protected]>
@chenbh
Copy link
Contributor Author

chenbh commented Dec 15, 2023

@matthewmcnew I reworked the RFC so that there's 2 attestations schemas generated, the app image (Build resource), and the builder image (Builder/ClusterBuilder).

Comment on lines +512 to +514
- Currently there's no tools on the market that can actually verify generic key-based attestations.
[slsa-verifier][slsa-verifier] uses keyless signing and ony supports GitHub Actions and Google Cloud Build,
`cosign` does key based verification, but only with its own key type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did we land here? How do we expect users of kpack to validate these attestations?

@tomkennedy513
Copy link
Collaborator

@chenbh can we merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants