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 exit status should be improved #948

Closed
Itxaka opened this issue Oct 25, 2021 · 10 comments
Closed

CLI exit status should be improved #948

Itxaka opened this issue Oct 25, 2021 · 10 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@Itxaka
Copy link
Contributor

Itxaka commented Oct 25, 2021

Description
Currently if you run cosign verify against a non existing image, against a not signed image, against a signed image with a different key, the exit status is the same (1)

This makes difficult to automate using cosign to verify stuff, especially in projects where the signing is starting or in which all artifacts may not be signed and those are out of your control.

cosign should probably try to exit with a different status code if the verify failed, or if the image is not verified.

Examples:

Image without signature:

error: fetching signatures: remote image: GET https://index.docker.io/v2/library/ubuntu/manifests/sha256-626ffe58f6e7566e00254b638eb7e0f3b11d4da9675088f4781a50ae288f3322.sig: MANIFEST_UNKNOWN: manifest unknown; unknown tag=sha256-626ffe58f6e7566e00254b638eb7e0f3b11d4da9675088f4781a50ae288f3322.sig
$ echo $?                                 
1

Non existing tag

error: GET https://index.docker.io/v2/library/ubuntu/manifests/latestf: MANIFEST_UNKNOWN: manifest unknown; unknown tag=latestf
$ echo $?                                 
1

Signature not matching

error: no matching signatures:
failed to verify signature
$ echo $?                                 
1

Cheers!

@Itxaka Itxaka added the enhancement New feature or request label Oct 25, 2021
@dlorenc
Copy link
Member

dlorenc commented Dec 19, 2021

+1 on this! We could switch to a different exit code.

@znewman01 znewman01 added the good first issue Good for newcomers label Sep 2, 2022
@ChrisJBurns
Copy link
Contributor

ChrisJBurns commented Dec 20, 2022

@dlorenc @Itxaka Am happy to work on this, although there are some reserved exit codes that have their own definition across Windows and Linux. I was hoping to piggy back off existing codes that may have a common definition across operating systems (in the same way that HTTP Status Codes do), but the fact that each operating system has its own defined set, we may have to define our own and (most likely) override the ones that may exist for the underlying operating systems.

3,4,5 for the three cases above? If so, do we think this will have any knock-on effects on the system to which the cosign binary runs on? If not, am happy to put up a PR on this with:
3 - Image without signature
4 - Non existing tag
5 - Signature not matching

@Itxaka
Copy link
Contributor Author

Itxaka commented Dec 21, 2022

IMO any numbers not reserved will do, as long as there is a differentiation on the errors that are generated so automating things can work with those exit codes. for example, I can deal with error 4 (non existing tag) as that migth be a user error, but If Im checking images in an automated fashion and I get error 5 (signature not matching) that should raise an alert as the image might be compromised. Same with 4 (image without signature), as I can make an automated job that checks for that and signs the images if they dont have a signature.

This is real scenario BTW, we check for signatures on our github.com/rancher/elemental-toolkit packages and fi signatures are missing our job will sign it. Currently we have no way of knowing if the error is missing signature or other :)

@znewman01
Copy link
Contributor

IMO any numbers not reserved will do, as long as there is a differentiation on the errors that are generated so automating things can work with those exit codes

Strong +1 to this. @ChrisJBurns I'll assign this issue to you for now; if you come up with a convention, please document it (maybe README.md? I think long run we'll want to have this in the equivalent of a man page)

@Itxaka
Copy link
Contributor Author

Itxaka commented Dec 21, 2022

FYI for elemental-cli we also had to implement exit codes under a cobra cli and ended up with a constants file which had all the exit codes with an explanation comment and we have a job to autogenerate the docs with some go code

That means that we never forget to update the docs with the exit codes when new ones are added, and the docs always have a small explanation of what the exit code means, very handy :)

You could probably hook the exit-code docs autogeneration here https://github.com/sigstore/cosign/blob/main/cmd/help/main.go#L34 as part of the gendocs command

@ChrisJBurns
Copy link
Contributor

All, I have implemented a very draft-ish version of this in the following commit. It's not perfect, still needs a cleanup, but I've managed to get a custom exit code thrown back (12) for unmatching signature, in addition to doc generation via the example from elemental-cli (@Itxaka hope you don't mind, I quite literally copied and pasted the GenerateDocs func as it worked perfectly just to get something).

The only thing left now is to probably conform to a pattern of how we do errors. Currently, I can see there is a mix and max of how we do this. One of the ways is that there is a VerificationError object and a NewVerificationError method in the errors.go file. But moving forward, we should probably conform around a CosignError object with a Message and Code variable, with a more generic RaiseError function that allows for the passing of a specific Error type struct (e.g. ErrNoMatchingSignatures = &CosignError{....}.

Welcome your thoughts @znewman01 @Itxaka @dlorenc. If we move towards the above approach, there's going to be a few changes around the code to ensure the rest of the errors are thrown the same way.

@znewman01
Copy link
Contributor

Awesome, thanks @ChrisJBurns! Something like that SGTM.

A couple main points of feedback:

  • I'd like to separate the CLI errors (which include exit status etc.) from general Cosign errors. So basically I'd suggest moving a lot of pkg/error to cmd/cosign/error.
  • Cosign pkg/error errors should get wrapped as they get handled in cmd/*

Otherwise I'm pretty happy with a cleaned-up version of what you proposed

@ChrisJBurns
Copy link
Contributor

@znewman01 Feel free to take a look (#2673). Managed to keep code to minimum to reduce side affects on other functionality. Should be backwards compatible also and only affect code that explicitly has mapped error to exit codes, otherwise the standard exit code 1 is returned.

Should be somewhat extensible also, so for other errors and exit codes, it should be relatively easy to add the functionality for them. The PR contains not only the throwing of a custom exit code for "no matching signatures", but also for all supporting code to make this work. The next PR's that address the other 2 errors in the issue description, should be a matter of just adding the correct error types and exit codes and returning them in the places the error happens.

@ChrisJBurns
Copy link
Contributor

@Itxaka @znewman01

I think this can be closed as completed with fixes implemented in:

If you run cosign through the following examples you'll get the correct exit codes as defined in https://github.com/sigstore/cosign/blob/main/doc/cosign_exit_codes.md.

$ cosign version
...
GitVersion:    2.0.0
GitCommit:     d6b9001f8e6ed745fb845849d623274c897d55f2
GitTreeState:  "clean"
BuildDate:     2023-02-23T19:26:35Z
GoVersion:     go1.20.1
Compiler:      gc
Platform:      darwin/amd64

Image with no signature

$ SRC_IMAGE=busybox
$ SRC_DIGEST=$(crane digest busybox)
$ IMAGE_URI=ttl.sh/$(uuidgen | head -c 8 | tr 'A-Z' 'a-z')
$ crane cp $SRC_IMAGE@$SRC_DIGEST $IMAGE_URI:1h
$ IMAGE_URI_DIGEST=$IMAGE_URI@$SRC_DIGEST
$ cosign verify --key cosign.pub $IMAGE_URI_DIGEST
Error: no signatures found for image
main.go:69: error during command execution: no signatures found for image
$ echo $?
10

Image with non existing tag

$ cosign verify --key cosign.pub ttl.sh/37708c4:1h
Error: image tag not found: GET https://ttl.sh/v2/37708c4/manifests/1h: MANIFEST_UNKNOWN: manifest unknown; map[Tag:1h]
main.go:69: error during command execution: image tag not found: GET https://ttl.sh/v2/37708c4/manifests/1h: MANIFEST_UNKNOWN: manifest unknown; map[Tag:1h]
$ echo $?
11

Image with no matching signature

$ cosign verify --key blue.pub $IMAGE_URI_DIGEST
Error: no matching signatures:
error verifying bundle: comparing public key PEMs, expected -----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEI2cpYDnNla7+kidLeulgtt3ZwAyC
m4PWQZO3mZA3Rxu6VRwtYpGTCflxteDrwgmQcrhHSyqyKxwQloLdPtHeaQ==
-----END PUBLIC KEY-----
, got -----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEi0+0MRDdb1d5A9dSEVJ/84fVjSVQ
IFYZl6hON0gEaFhzMogfyLT4lUVVoPc9LhwsQAeIWpamm6oInU5+FEGEfw==
-----END PUBLIC KEY-----
$ echo $?
12

@Itxaka
Copy link
Contributor Author

Itxaka commented Apr 10, 2023

Looking really good to me, thank you folks for implementing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants