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

cli: silence usage on errors #265

Closed
wants to merge 1 commit into from
Closed

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Sep 19, 2022

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

Otherwise, when RunE returns an error, we also print the usage, which is, at that point, correct: e.g.

$ go run ./cli/slsa-verifier verify-image ghcr.io/slsa-framework/example-package.e2e.container.workflow_dispatch.main.default.slsa3@sha256:c8d763cff21aaaee7ae925416ba8627dcb05dd38ecf48683249f4ef5f76f0155 --source-uri github.com/slsa-framework/example-package --print-provenance --source-tag v1.2.3
FAILED: SLSA verification failed: no valid attestations found on OCI registry: expected tag 'refs/tags/v1.2.3', got '': tag used to generate the binary does not match provenance
Usage:
  slsa-verifier verify-image [flags]

Flags:
      --build-workflow-input map[]    [optional] a workflow input provided by a user at trigger time in the format 'key=value'. (Only for 'workflow_dispatch' events on GitHub Actions). (default map[])
      --builder-id string             EXPERIMENTAL: the unique builder ID who created the provenance
  -h, --help                          help for verify-image
      --print-provenance              print the verified provenance to stdout
      --provenance-path string        path to a provenance file
      --source-branch string          [optional] expected branch the binary was compiled from
      --source-tag string             [optional] expected tag the binary was compiled from
      --source-uri string             expected source repository that should have produced the binary, e.g. github.com/some/repo
      --source-versioned-tag string   [optional] expected version the binary was compiled from. Uses semantic version to match the tag

no valid attestations found on OCI registry: expected tag 'refs/tags/v1.2.3', got '': tag used to generate the binary does not match provenance
exit status 1

Signed-off-by: Asra Ali <asraa@google.com>
@@ -34,6 +34,7 @@ For more information on SLSA, visit https://slsa.dev`,
c.AddCommand(verifyImageCmd())
// We print our own errors and usage in the check function.
c.SilenceErrors = true
c.SilenceUsage = true
Copy link
Member

Choose a reason for hiding this comment

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

Does this also silence usage when there is actually a flag parsing error? or just when there is an error returned by RunE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me see: so if a flag is parsed incorrectly with this change it'll currently print

$ go run ./cli/slsa-verifier/ verify-artifact --provenance testdata/go/v1.2.0/binary-linux-amd64-workflow_dispatch.intoto.jsonl --source-uri github.com/slsa-framework/example-package testdata/go/v1.2.0/binary-linux-amd64-workflow_dispatch
unknown flag: --provenance

The problem is, that it can't distinguish between error types from flag parsing or generally from RunE. You'll see the usage string only with --help.

This seems like others must have run into this issue. Looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: There's no way to distinguish a flag parsing error from a RunE error, unless we handle them separately: spf13/cobra#340

I think the better thing to do is to switch from RunE to Run and use our own custom error handler. Will update today

Copy link
Member

Choose a reason for hiding this comment

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

SG, i think this is what I've done in the past for this reason but my memory was fuzzy.

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.

2 participants