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

Limit size of manifest #1711

Merged
merged 1 commit into from
May 22, 2023
Merged

Limit size of manifest #1711

merged 1 commit into from
May 22, 2023

Conversation

AdamKorcz
Copy link
Contributor

This adds a limit to the size of manifest that can get pulled.

Since the manifest is untrusted, this imposes a security risk to go-containerregistry users; If the size of the manifest is excessive compared to the ressources available on the machine running go-containerregistry, then a large manifest could cause Denial of Service by exhausting the memory of the machine.

Signed-off-by: AdamKorcz <adam@adalogics.com>
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2023

Codecov Report

Merging #1711 (bb4c98e) into main (e61c519) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1711      +/-   ##
==========================================
- Coverage   72.20%   72.16%   -0.05%     
==========================================
  Files         121      121              
  Lines        9720     9720              
==========================================
- Hits         7018     7014       -4     
- Misses       2028     2030       +2     
- Partials      674      676       +2     
Impacted Files Coverage Δ
pkg/v1/remote/fetcher.go 74.50% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

We should probably make this configurable, but 100mb should be way higher than anything would reasonably hit, so I think it's fine to merge as is.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

We should probably make this configurable, but 100mb should be way higher than anything would reasonably hit, so I think it's fine to merge as is.

@jonjohnsonjr jonjohnsonjr merged commit 5fe7f2e into google:main May 22, 2023
18 checks passed
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.

None yet

3 participants