Open Mukuls77 opened 2 months ago
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.
Files | Patch % | Lines |
---|---|---|
pkg/cosign/verify.go | 27.27% | 6 Missing and 2 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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
The layers are downloaded when we invoke sig, err := static.Copy(sig) https://github.com/sigstore/cosign/blob/fa17fab56bc15a7a86b7e785fabdcf0e469ac802/pkg/cosign/verify.go#L611
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.
Summary
Closes #3669 Currently while doing the verification of signatures cosign does not check the MediaType of the layers before downloading those. In the case when some one mistakenly put the signature tag on an artifact which is not a signature than cosign will download all the layers present in the Manifest file of the artifact and than we start verification which will eventually fail as the artifact was not really a signature. The fix provides an additional check to check the Media Type before downloading the layers. this will avoid unnecessary download of stale data and will quickly reject the verification as the artifact is not a signature.
Release Note
Documentation
No change in documentation