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

Export attestations for local exporter #3197

Merged
merged 5 commits into from
Nov 23, 2022

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Oct 19, 2022

Implements the local exporter part of #3184.

The attestation extraction logic is moved to a separate utils package, so that other exporters can use it. Additionally, the bundling logic is slightly reworked, to add an explicit Unbundle step before Extracting. This ensures that the Extract method can perform a one-to-one translation from result.Attestations to intoto.Statements, which allows for deriving intermediate properties such as the Path.

The attestation files are exported similar to another platform ref, with file names equivalent to the original attestation files had (though this should be pretty easy to change if we'd prefer a separate output location specified by the user).

I'm not quite sure what to put for the subject field for intoto (unless there's a canonical way of calculating the hash of a directory that I'm not aware of?), so for now I've left those empty.

@jedevc jedevc changed the title Attestations exporter local Export attestations for local exporter Oct 19, 2022
@tonistiigi
Copy link
Member

You can use this pkg https://github.com/moby/buildkit/compare/master...tonistiigi:buildkit:staticfs?expand=1 to add new static files(attestation data) to the transfer context and then merge it with the existing FS implementation.

@jedevc jedevc force-pushed the attestations-exporter-local branch from c505156 to adf0de4 Compare October 21, 2022 16:10
@jedevc jedevc force-pushed the attestations-exporter-local branch 2 times, most recently from 12e699d to ac7b819 Compare October 21, 2022 16:25
solver/result/attestation.go Outdated Show resolved Hide resolved
exporter/attestation/extract.go Outdated Show resolved Hide resolved
exporter/attestation/extract.go Outdated Show resolved Hide resolved
exporter/local/export.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
@jedevc jedevc force-pushed the attestations-exporter-local branch 2 times, most recently from 65a0647 to 792dfa5 Compare October 26, 2022 13:20
@jedevc jedevc force-pushed the attestations-exporter-local branch from 792dfa5 to e647335 Compare November 2, 2022 14:50
@jedevc
Copy link
Member Author

jedevc commented Nov 2, 2022

🐛 StaticFS/MergeFS have a bug with order of traversal, where x.txt will be sorted directly after x/ because of the ASCII ordering - we need to apply a fix similar to in contenthash.

Edit: Fixed in 3e1153f.

@jedevc
Copy link
Member Author

jedevc commented Nov 16, 2022

Rebased onto master after #3240 merged.

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.

Building with provenance attestation, I get a cryptic "context canceled" error. The issue seems to be that

Kind: gatewaypb.AttestationKindInToto,
doesn't set a Path. In addition to fixing this, if this key is required it should be validated or at least the error should indicate what is going on.

exporter/local/export.go Show resolved Hide resolved
@jedevc
Copy link
Member Author

jedevc commented Nov 17, 2022

Fixed the context cancelled issue. Also found a bug in trying to generate the subject hash for a file that is a symlink - I've refactored to only hash regular files, ignoring directories/symlinks/everything else.

@jedevc jedevc force-pushed the attestations-exporter-local branch 2 times, most recently from 21b59c2 to 389f839 Compare November 22, 2022 15:40
exporter/local/export.go Outdated Show resolved Hide resolved
return errors.Wrap(err, "failed to marshal attestation")
}

if attestations[i].Path == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Better to have this validation somewhere else, like AddAttestation. If somebody sends incorrect options it shouldn't go unnoticed to them if they forget to test the local exporter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will resolve this one in a follow-up - agree that it should be somewhere else, but AddAttestation doesn't currently return an error, so might be a bit more invasive than the above comment.

@jedevc jedevc force-pushed the attestations-exporter-local branch 2 times, most recently from b5ccb68 to 9150344 Compare November 22, 2022 17:21
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

As discussed we should not set platform in output dir if only one is defined to be aligned with current local output logic. Prefix platform can be enforced for single platform using BUILDKIT_MULTI_PLATFORM attr:

PrefixPlatform: exportMap,

tonistiigi and others added 5 commits November 23, 2022 16:31
These utilities can be used to add new static files
to the fs that is transferred between daemon and client.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
The character "." (and others) appear before "/" in the ASCII table so
when sorting, these will appear directly after the directory, before the
contents.

When the results of the sort are then used for calls to Walk(), the
traversal order will be incorrect, causing fsutil's filesync
functionality to reject the server's transfer.

To fix this, we replace "/" in the pathname with the null character, to
ensure that it is sorted before the others.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Previously, the test for generating a nested structures was determined
by the number of refs, which for attestations, can be more than one even
though there is a single platform.

The code is reworked to ensure that if a single platform is specified,
we still get a flat structure, even if attestations are present.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants