refactor: add subcommands and separate functionality from artifacts a… #231

asraa · 2022-08-23T20:03:18Z

Signed-off-by: Asra Ali asraa@google.com

This PR does the following:

Add sub-commands verify-artifact and verify-image to the CLI Create subcommands for artifact and image verification #205
Separates verifiers function call and the artifact image digest calculation
Adds an explicit version command to fix Add a -version flag for displaying version information #229. Note that this is a flag generated at build time.
Help message is auto-generated. See Enhacement: Help message should exemplify arguments for run #173
Renames --provenance to --provenance-path. See Rename --provenance to ---provenance-path #93
Update README Update README command #186 and documentation of options Document options #230

Fixes #205
Fixes #229
Fixes #173
Fixes #93

TODO:

Update usage of verifier at HEAD in example-package

…nd images Signed-off-by: Asra Ali <asraa@google.com> remove Signed-off-by: Asra Ali <asraa@google.com> fix Signed-off-by: Asra Ali <asraa@google.com> update build cmd Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2022-08-23T20:10:49Z

@laurentsimon qq: what is the pupose of testing outBuilderID from the main CLI? it was never returned or output before

laurentsimon · 2022-08-23T20:28:57Z

@laurentsimon qq: what is the pupose of testing outBuilderID from the main CLI? it was never returned or output before

I think to display it (someday), the same way that we display information about repo / commit hash. More work is needed to decide what format we need to display it, though.

Wdut?

asraa · 2022-08-23T20:40:50Z

More work is needed to decide what format we need to display it, though.

Gotcha: I can make the command return the outBuilderID as string for now to satisfy the tests (e.g. cmd.Exec(ctx, args) returns (string, error) Later we can formalize on that output.

laurentsimon · 2022-08-23T21:27:19Z

More work is needed to decide what format we need to display it, though.

Gotcha: I can make the command return the outBuilderID as string for now to satisfy the tests (e.g. cmd.Exec(ctx, args) returns (string, error) Later we can formalize on that output.

SG

Signed-off-by: Asra Ali <asraa@google.com>

laurentsimon · 2022-08-23T21:54:26Z

README.md

+| Option | Description |
+| --- | ----------- |
+| `source` | Expects a source, for e.g. `github.com/org/repo`. This option verifies that `ConfigSource` and `Materials` of the SLSA provenance contain the expected `source`. |
+| `branch` | Expects a `branch` like `main` or `dev`. Verifies this against the `github_ref` in the SLSA provenance when the `github_ref_type` is a `branch`, or the `base_ref` or `target_commitish` when a `tag`.|


Let's add that is not supported for all triggers. It depends on GH payload. If trigger is on an annotated tag, it's not available. If a tag is not annotated, but the tag is made not from the HEAD of a branch (say, an older commit from main), it's not supported either.

Also, not supported on manual workflow_dispatch (-workflow-input is needed instead and depends on each project).

laurentsimon · 2022-08-23T21:57:49Z

README.md

+| Option | Description |
+| --- | ----------- |
+| `source` | Expects a source, for e.g. `github.com/org/repo`. This option verifies that `ConfigSource` and `Materials` of the SLSA provenance contain the expected `source`. |
+| `branch` | Expects a `branch` like `main` or `dev`. Verifies this against the `github_ref` in the SLSA provenance when the `github_ref_type` is a `branch`, or the `base_ref` or `target_commitish` when a `tag`.|


I wonder if we need this level of details here. I'm worried it will go out of sync with the implementation.
How about:

Keep the documentation simpler in this README. Maybe we can have a table that explains which options are supported for which builder, and that the branch option is brittle.

If we want more detailed doc, maybe have a README under verifiers/internal/<builder>/README.md. There is still a risk of getting out of sync, of course....

Anything we could do better that what I'm proposing?

Simplified the README.md. I mostly want to make sure that there's documentation for what the options should look like, with some examples in this PR

Eveneually we should have some documentation somewhere of how verification works for specific builders.

Signed-off-by: Asra Ali <asraa@google.com>

laurentsimon

Looks really nice!
Since we're at it, I've added a bunch of comments about argument naming. Let me know what you think.

.github/config-release.yml

README.md

laurentsimon · 2022-08-25T01:19:53Z

README.md

+      --source string            expected source repository that should have produced the binary, e.g. github.com/some/repo
+      --tag string               [optional] expected tag the binary was compiled from
+      --versioned-tag string     [optional] expected version the binary was compiled from. Uses semantic version to match the tag
+      --workflow-input map[]     [optional] a workflow input provided by a user at trigger time in the format 'key=value'. (Only for 'workflow_dispatch' events). (default map[])


this would (maybe ?) be renamed to build-inputs if we follow the build / source / provenance requirements's naming convention. I'm not 100% sure build-input is better than workflow-input... maybe build-workflow-input...

I think build-inputs is an accurate term for workflow dispatch inputs, but i wonder if that would get conflated with other types of inputs.

For GHA builders: the only inputs to workflows is through workflow dispatch inputs. But what about inputs to the trusted builders themselves?

fully agreed. That's why I hinted at -build-workflow-inputs., to make it more explicit that it's for (GitHub) workflows. Not sure this is enough. Is it possible to name the option after the type of builders that support it? -build-gha-inputs. I'm making the name up...

Maybe -build-input would be enough if the content is formatted, like -build-input workflow_dispatch:bla=bla... ?

I think formatting inputs is brittle and if we are considering that it is likely not good. If they are builder specific, I think some options are already interpreted slightly different -- so build-input for GHA could mean workflow inputs.

OTOH I don't know how to distinguish between types of build inputs for a single builder. If someone wants to check on inputs to the trusted builders, can they access the provenance, since that is more of a "recipe"?

For now, I think it's easier to organize as build-XYZ or source-XYZ of the current options. At least it organizes the options.

SGTM.

OTOH I don't know how to distinguish between types of build inputs for a single builder. If someone wants to check on inputs to the trusted builders, can they access the provenance, since that is more of a "recipe"?

fair enough. Let's start with that then. I am not even sure GCB supports "input", since it's just a webhook.

@ianlewis any objection the direction we're taking here?

README.md

cli/slsa-verifier/verify/verify_image.go

Signed-off-by: Asra Ali <asraa@google.com>

laurentsimon

LGTM, thanks

How do we handle merging this PR? Do we merge or wait for the GCB release to be cut?

cli/slsa-verifier/verify/options.go

asraa · 2022-09-01T17:28:38Z

How do we handle merging this PR? Do we merge or wait for the GCB release to be cut?

Given that GCB is breaking too, I think it would be good to merge all in one. That way GCB verification ONLY works with these new CLI arguments, and GCB folks won't have to deal with a breaking API change.

laurentsimon · 2022-09-01T17:35:11Z

alright. Let's make it happen then.

cli/slsa-verifier/verify/options.go

Signed-off-by: Asra Ali <asraa@google.com>

asraa · 2022-09-06T20:34:30Z

@laurentsimon merged your changes on compute digest FN and GCB provenance. Good to merge with slsa-framework/example-package#100?

laurentsimon · 2022-09-06T21:10:26Z

LGTM!

laurentsimon · 2022-09-06T21:10:40Z

What's FN?

asraa · 2022-09-06T22:10:18Z

What's FN?

FN is the compute digest FN you had earlier in the GCB support -- I think we will remove with a better container interface, but was needed to swap in for testing

It's not exposed as a CLI option, but exposed as part of the library right now

One part that is missing in the output if invocation is wrong is that there is no indication that user has to pass path to a file (artifact/image). The remaining potential errors from slsa-framework#173 are handled via slsa-framework#231. Found while looking at slsa-framework#174. Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>

One part that is missing in the output if invocation is wrong is that there is no indication that user has to pass path to a file (artifact/image). The remaining potential errors from #173 are handled via #231. Found while looking at #174. Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com> Signed-off-by: Mihai Maruseac <mihaimaruseac@google.com>

refactor: add subcommands and separate functionality from artifacts a…

17a4ce0

…nd images Signed-off-by: Asra Ali <asraa@google.com> remove Signed-off-by: Asra Ali <asraa@google.com> fix Signed-off-by: Asra Ali <asraa@google.com> update build cmd Signed-off-by: Asra Ali <asraa@google.com>

asraa force-pushed the add-subcommands branch from 938c63d to 17a4ce0 Compare August 23, 2022 20:07

Merge branch 'main' into add-subcommands

39ac40d

Add README docs and fix tests

e5c7b3e

Signed-off-by: Asra Ali <asraa@google.com>

laurentsimon reviewed Aug 23, 2022

View reviewed changes

asraa added 2 commits August 24, 2022 16:35

Merge remote-tracking branch 'upstream/main' into add-subcommands

62e8e1f

simplyify readme

1f42a80

Signed-off-by: Asra Ali <asraa@google.com>

laurentsimon reviewed Aug 25, 2022

View reviewed changes

asraa added 4 commits August 25, 2022 10:51

all but input renaming

bd58f66

Signed-off-by: Asra Ali <asraa@google.com>

Merge remote-tracking branch 'upstream/main' into add-subcommands

775b8f3

update naming options

bbefa40

Signed-off-by: Asra Ali <asraa@google.com>

Merge remote-tracking branch 'upstream/main' into add-subcommands

16eee94

laurentsimon approved these changes Sep 1, 2022

View reviewed changes

cli/slsa-verifier/verify/options.go Outdated Show resolved Hide resolved

laurentsimon reviewed Sep 1, 2022

View reviewed changes

cli/slsa-verifier/verify/options.go Outdated Show resolved Hide resolved

asraa added 2 commits September 1, 2022 12:39

reorder options in code

3281096

Signed-off-by: Asra Ali <asraa@google.com>

update description for build-workflow-input

e49d0bc

Signed-off-by: Asra Ali <asraa@google.com>

asraa mentioned this pull request Sep 1, 2022

Handle updates in verifier CLI arguments and subcommands slsa-framework/example-package#97

Closed

Merge remote-tracking branch 'upstream/main' into add-subcommands

420c434

asraa merged commit ff0ced4 into slsa-framework:main Sep 6, 2022

This was referenced Dec 28, 2022

feat: Enhance help message #418

Merged

Support multiple artifact #91

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add subcommands and separate functionality from artifacts a… #231

refactor: add subcommands and separate functionality from artifacts a… #231

asraa commented Aug 23, 2022 •

edited

Loading

asraa commented Aug 23, 2022

laurentsimon commented Aug 23, 2022

asraa commented Aug 23, 2022

laurentsimon commented Aug 23, 2022

laurentsimon Aug 23, 2022

laurentsimon Aug 23, 2022

laurentsimon Aug 23, 2022

asraa Aug 24, 2022

laurentsimon left a comment

laurentsimon Aug 25, 2022

asraa Aug 25, 2022

laurentsimon Aug 25, 2022 •

edited

Loading

asraa Sep 1, 2022

asraa Sep 1, 2022

laurentsimon Sep 1, 2022

laurentsimon left a comment •

edited

Loading

asraa commented Sep 1, 2022

laurentsimon commented Sep 1, 2022

asraa commented Sep 6, 2022

laurentsimon commented Sep 6, 2022

laurentsimon commented Sep 6, 2022

asraa commented Sep 6, 2022 •

edited

Loading

refactor: add subcommands and separate functionality from artifacts a… #231

refactor: add subcommands and separate functionality from artifacts a… #231

Conversation

asraa commented Aug 23, 2022 • edited Loading

asraa commented Aug 23, 2022

laurentsimon commented Aug 23, 2022

asraa commented Aug 23, 2022

laurentsimon commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon left a comment • edited Loading

Choose a reason for hiding this comment

asraa commented Sep 1, 2022

laurentsimon commented Sep 1, 2022

asraa commented Sep 6, 2022

laurentsimon commented Sep 6, 2022

laurentsimon commented Sep 6, 2022

asraa commented Sep 6, 2022 • edited Loading

asraa commented Aug 23, 2022 •

edited

Loading

laurentsimon Aug 25, 2022 •

edited

Loading

laurentsimon left a comment •

edited

Loading

asraa commented Sep 6, 2022 •

edited

Loading