Codecov Report

Attention: Patch coverage is 27.27273% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 40.57%. Comparing base (2ef6022) to head (9d95704).
Report is 88 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3670      +/-   ##
==========================================
+ Coverage   40.10%   40.57%   +0.47%     
==========================================
  Files         155      157       +2     
  Lines       10044    10164     +120     
==========================================
+ Hits         4028     4124      +96     
- Misses       5530     5540      +10     
- Partials      486      500      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm not very familiar with the OCI part of the codebase, so this might be incorrect, but isn't the metadata already downloaded by this point?

I'm also not convinced this is needed, as this seems like handling a case where someone has manipulated the contents of the registry object. Exiting out early is fine, but it will fail nonetheless later on.

@haydentherapper yes the MediaType is already known to us when we download the signature manifest. so this check is not really downloading or querying the mediaType from registry. Instead it is fetching it from the downloaded manifest file. So this check does not add any extra Get towards registry.
The check will only insure that we start downloading the layers which are of correct media type. i agree if we don't put the check the verification will staill fail but this will account to download of the layers which can have big memory requirement and unnecessary will consume the runtime memory of the system with the data which is not good.

This check is particular important in case someone is using cosign as a library, as without this check if some one apply the signature tag to a artifact which is not a signature currently cosign will start downloading all the layers, as cosign keeps those in memory for further verification this will increase the memory usage of the process using cosign library and than may result in restart of the process. If you agree i am extend this check to also check the size of the layer if the size is more than what we define as max permissible size of a signature we can block the download of layer if it exceed that size limit. this will make the cosign library usage more secure

@haydentherapper can you pls comment do you see this change useful as it is not introducing any additional overhead but enhancing the security, as i mentioned we can enhance it further by adding check to not download a signature layer if it is beyond a specific size.

I'm not sure if @haydentherapper's first question was answered, or if I didn't quite understand it. Isn't the manifest already downloaded at this point? It looks like the download happens here, a few lines before the verifySignatures call, so I'm not sure how checking the media type in verifySignatures prevents the download?

If you agree i am extend this check to also check the size of the layer if the size is more than what we define as max permissible size of a signature we can block the download of layer if it exceed that size limit.

We already check the size of the layer before reading it into memory - have you found a case where this check is insufficient or incorrect?

@cmurphy thanks for your comments.
The problem we want to fix is to stop download of layers of the manifest when someone put a signature tag on a manifest which is not really a signature manifest. so when we download the signatures two things are done by cosign

1. download the manifest file
2. download the layers present in the manifest file
   The enhancement i am suggesting is that once we download the manifest file and before we start downloading the layers of the manifest we should check the media Type present in the layer, and if the media type of layer is not that of a signature payload we should not download it. so we just do a check of mediaType of layer from the downloaded manifest file before we trigger download of the manifest layers.

The layers are downloaded
when we invoke
sig, err := static.Copy(sig)

I agree we have a env variable COSIGN_MAX_ATTACHMENT_SIZE which has default value of 128 Mb, ideally our signature payload is not that big and it would have been better to have a more specific limit for signatures, but still we can tune this env variable according to our needs.
So as i mentioned if we check the Media Type of layers before we download those layers we would optimize of cosign handling and we would not download stale layers which are actually not signature data.