containers: Suppress error message when trying to fetch signatures #811
Conversation
pkg/container/container.go
Outdated
| log.Printf("error getting manifest: %v", err) | ||
| } else { | ||
| log.Printf("no signature found for %s", package_url) | ||
| } |
There was a problem hiding this comment.
nit: can we move this block to it's own function? This whole thing is already quite hairy so it would be a little nicer to keep any changes done here small. This whole thing also merits some refactoring but we can do that in another PR.
There was a problem hiding this comment.
I'd be more inclined to move the block with ExtractIdentityFromCertificate to a separate function, rather than this one, but VerifyFromIdentity does take a large number of parameters.
There was a problem hiding this comment.
I did kinda both. The function is split out, even with a large number of parameters the code reads better now IMO, plus I changed the handling of the errors after calling the new functions extractAndValidateSignature to keep the happy path on a single indentation level which would hopefully address the comment Ozz had.
btw I agree that this whole module is ripe for refactoring. I filed issue #817 to get rid of the unsafe type casts at least, especially since they are coming from a potentially untrusted source.
pkg/container/container.go
Outdated
| log.Printf("error getting manifest: %v", err) | ||
| } else { | ||
| log.Printf("no signature found for %s", package_url) | ||
| } |
There was a problem hiding this comment.
I'd be more inclined to move the block with ExtractIdentityFromCertificate to a separate function, rather than this one, but VerifyFromIdentity does take a large number of parameters.
…plit extractAndValidateSignature into a separate function
When fetching container signatures, we attempt to do that for all
container versions and their digests we receive. In case the container
is not signed, we would throw a scary-looking message. The error itself
is handled well in the code by returning an empty signature object, we should
just make the message in the logs more friendly than:
```
{"Timestamp":1693470050051776000,"message":"2023/08/31 10:20:50 error
getting manifest: error getting image: GET
https://ghcr.io/v2/jakubtestorg/testrepo/manifests/sha256-729d8897773756c0c84f785aa2b44f05174fdf19de12a4497ecec042ecd31934.sig:
MANIFEST_UNKNOWN: manifest unknown"}
```
Also splits out the extraction and validation of a signature into
its own function.
jhrozek
force-pushed
the
artifact_debug
branch
from
e8a2f3e
to
026e7a9
Compare
| zerolog.Ctx(ctx).Info(). | ||
| Str("packageUrl", package_url). | ||
| Msg("no manifest found") |
When fetching container signatures, we attempt to do that for all
container versions and their digests we receive. In case the container
is not signed, we would throw a scary-looking message. The error itself
is handled well in the code by returning an empty signature object, we should
just make the message in the logs more friendly than: