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

Initial attestations support #2935

Merged
merged 7 commits into from
Aug 23, 2022
Merged

Initial attestations support #2935

merged 7 commits into from
Aug 23, 2022

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Jun 27, 2022

This PR introduces support for in-toto attestations, as specified in part 1 of #2773 (comment).

This is dependent on #2929, and includes a commit from there to better handle garbage collection of attestation refs.

In it's current state, this is a first-pass - and some things are expected to change before merge:

  • Media types for exported content
    • Additionally, these need to be added to the image pusher code to prevent encountered unknown type ... children may not be fetched messages.
  • OS/Platform values for attestations - currently selected none/none
  • Exact Gateway API protobuf format
  • Attestation struct format
  • Hopefully a few more tests

@jedevc jedevc marked this pull request as draft June 27, 2022 15:06
@jedevc jedevc force-pushed the attestations branch 2 times, most recently from d11f29d to 2c364d7 Compare June 27, 2022 15:20
Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

Great work! Just a few initial thoughts... :)

exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Show resolved Hide resolved
@jedevc
Copy link
Member Author

jedevc commented Jun 27, 2022

Not quite sure how this model would interact with tooling that generates a lot of attestations. One possible way might be to allow grouping attestations together into layers, though I'm not quite sure the mechanics of the API that would use.

@jedevc
Copy link
Member Author

jedevc commented Jul 7, 2022

Another complexity that's come up that we might want to investigate - we might want to have the capacity to have scanners able to analyze individual layers - so for that we might want to expose the ability to create attestations about single layers. Exactly how a scanner might create these attestations feels like something that's up to each scanner, and the interface that gets defined for that, but the storage format is something to maybe work out sooner. We could potentially allow attaching attestations to arbitrary Refs and then translate them to layer digests as part of the exporter?

@errordeveloper
Copy link
Contributor

errordeveloper commented Jul 11, 2022

Another complexity that's come up that we might want to investigate - we might want to have the capacity to have scanners able to analyze individual layers - so for that we might want to expose the ability to create attestations about single layers. Exactly how a scanner might create these attestations feels like something that's up to each scanner, and the interface that gets defined for that, but the storage format is something to maybe work out sooner. We could potentially allow attaching attestations to arbitrary Refs and then translate them to layer digests as part of the exporter?

One use-case would be to produce attestations about what buildkit itself does, i.e. for a RUN statement you would want to have a trace of what binaries were exec'ed, what files openend etc. You would probably want to included checksum of each file as well as stdout and stderr. For COPY/ADD you do want file chechsums too as well as list of any files that had been overwritten. These use-cases would probably translate to buildkit features, perhaps these could helpeful in shaping some aspects of the implementation.

@jedevc jedevc marked this pull request as ready for review July 18, 2022 16:46
@jedevc
Copy link
Member Author

jedevc commented Jul 18, 2022

Based on discussions, a couple notes:

  • Buildkit will have to have some knowledge of attestations somewhere - we can't entirely treat them like a completely opaque blob. It might still be worth distinguishing between the concept of a "generic buildkit attestation" which is then translated to an in-toto attestation at export, like the current code does?
  • Layer-based attestations are probably not going to be a thing, since they may not be particularly useful, since they don't necessarily map explicitly to the frontend's input (like a dockerfile). How files in an image map to an instruction, or the layer-based structure of an image is expressed in SBOMs is likely an issue to be solved at the frontend level, and expressed within the structure of the attestation if necessary.
  • Attestation bundling is already sort of supported atm, by splitting out each platform's attestations into a separate manifest. In the future, we could allow splitting out attestations further into multiple attestation manifests per platform if necessary if we want to split out bundles even more - but we can likely address this later as we see how the unbundled stuff scales.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

One thing I might have forgotten in some descriptions is that attestations shouldn't be only for image-based exporter. They should also work with local/tar exporter by creating additional files. So currently some code being under containerimage pkg might not be fully correct.

To create the additional files there needs to be some logic for naming them. It is similar to the predicate type, but maybe not exactly the same value? Also I think in these cases it may be more common that in-toto wrapping is optional. Eg. lets say I export local binary with provenance and SBOM attestation that both are exported as a separate file. Provenance is only defined via in-toto so doesn't make much sense to split it out but for SBOM I might expect to have a file with just the SBOM data. Maybe it is fine if all our defaults are in in-toto wrapping for now.

If it complicates things we can pick this part up in a follow up but a thing to consider when choosing the code location and proto types.

frontend/gateway/pb/gateway.proto Outdated Show resolved Hide resolved
frontend/gateway/pb/gateway.proto Show resolved Hide resolved
frontend/gateway/forwarder/forward.go Outdated Show resolved Hide resolved
frontend/result.go Outdated Show resolved Hide resolved
exporter/containerimage/attestations.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
client/client_test.go Outdated Show resolved Hide resolved
package exptypes

const (
MediaTypeDockerSchema2AttestationType = "application/vnd.in-toto+json"
Copy link
Member Author

Choose a reason for hiding this comment

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

Might want to sync with the containerd people on what to call this - since we'll need to get this to be recognized, or we'll see the the "reference for unknown type: application/vnd.in-toto+json" warning from https://github.com/jedevc/buildkit/blob/wip-attestations-sbom/vendor/github.com/containerd/containerd/remotes/handlers.go#L83

In the meantime, not quite sure where these media type definitions belong...

exporter/containerimage/exptypes/attestations.go Outdated Show resolved Hide resolved
@jedevc jedevc force-pushed the attestations branch 2 times, most recently from 2471252 to c651b95 Compare July 20, 2022 15:57
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
@jedevc
Copy link
Member Author

jedevc commented Jul 26, 2022

I'm happy with the state of this now 😄 A couple of follow-ups will likely be necessary: adding support for other exporters (e.g. tar/local), clean-up of conversions between different Result types (which makes the attestation plumbing ugly), and probably other things as we add SBOMs.

@jedevc
Copy link
Member Author

jedevc commented Aug 4, 2022

As discussed with @tonistiigi, to keep the scope of this PR low, a list of the planned follow-ups:

message Subject {
message SelfSubject {
}
message RawSubject {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if allowing RawSubject isn't a security concern actually. If we let the frontend to claim that attestation is for a random digest there is no way BuildKit can validate this is any way.

It is possible we should only allow attesting data when we can prove how it was built. Or at least we should make it so that it is clear that this subject is not allowed to point to any container objects so validators know that this is not to be trusted.

Something to pick up again on follow-ups.

Copy link
Member Author

@jedevc jedevc Aug 10, 2022

Choose a reason for hiding this comment

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

I think it depends on the security model that users are running buildkit under - but agreed.

We can revisit this, I think we could work round the need for a RawSubject by allowing attesting to individual files (composed of a Ref + Path), with something like a FileSubject which is the main other use I can see.

message InToto {
string predicateType = 1;
Ref predicateRef = 2;
string predicatePath = 3;
Copy link
Member

Choose a reason for hiding this comment

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

Follow-up: as discussed for SBOMs we should have a way that this path can point to a list of attestations. So SBOM generator can generate multiple SBOMs with a single invocation that are listed in a file and BuildKit will create an independent attestation record for each file.

frontend/gateway/client/result.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
exporter/containerimage/writer.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
solver/pb/caps.go Outdated Show resolved Hide resolved
Introduce attestations message structures to protobuf. Each result can
contain multiple attestations, keyed by the platform they apply to.
Each attestation has a predicate type, a ref+path to the predicate
content, and a selection of subjects, which can either be freeform
name + digest fields, or the resulting image manifest for the target.

Signed-off-by: Justin Chadwell <me@jedevc.com>
This patch connects the raw protobuf attestations through to the gateway
clients and servers, ensuring that the results are properly passed
through correctly, and that the appropriate refs are correctly cleaned
up.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Connect the attestations output from the frontend outputs into the
exporters, allowing the containerimage-based exporters to correctly
create in-toto attestations according to OCI reference-types Proposal F.

Attestations are grouped per-platform, and placed into manifests that
reference their targets - each layer in these manifests contain a custom
in-toto media type with the raw attestation content.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
The image info registry helpers provide utilities for reading content
from providers such as registries which can be easily used to inspect
image content. This is an alternative to requiring the containerd
puller, or exporting as an oci tarball.

By moving the helpers into testutil, we allow other testing components
to utilize the helpers, instead of just the dockerfile tests.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
This will eventually clients to query the server to confirm if
attestations are supported. However, for now, we can keep it disabled
until the gateway api stabilizes.

Signed-off-by: Justin Chadwell <me@jedevc.com>
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.

None yet

4 participants