Recommend enforcing limits on manifest/blob sizes

[imjasonh]

In https://github.com/opencontainers/image-spec/pull/826 some concerns were raised about the new field being used as a vector for DoS attacks, if registries and clients don't enforce some maximum size on the data they accept.

This concern isn't unique to the data field. I can think of a few places where this could crop up in the existing specs:

• annotations
• digests (sha123:...10GB of garbage...)
• blob uploads/fetches in general

In all cases servers need to be resilient to malicious or badly configured clients, and vice versa.

It might be enough to just recommend that implementers enforce some maximum size when receiving manifests or blobs, and not bother recommending specific validations for every field that could contain arbitrary data.

(Correct me if I'm wrong, this sounds like a concern for distribution-spec, and not the image-spec)

[sudo-bmitch]

My suggestion follows the advice of others on the OCI meetings.

1. The limit should be on the manifest, and not just the data or any other single field within the manifest.
2. The limit should be a minimum that clients and registries are both expected to support, and not a maximum.

Registries can choose to allow larger manifests, and clients can attempt to use a larger manifests, allowing each to innovate. But for portability, tooling that creates manifests should avoid exceeding this limit unless it knows the registry and client both support a larger value.

For picking a value, I'd want to check what max limits may already exist in tooling today, and pick something at our below those existing values.

[imjasonh]

I wasn't thinking that the spec would recommend or even suggest a specific limit value, just that it would recommend to implementers that they SHOULD enforce one for their own benefit, and let them work out what that might be.

What seems like a pathologically large manifest today might seem quaintly small in 10 years.

We could say something like "as of April 2021, we consider X to be a reasonable limit for portability across common implementations" but that seems hard to wordsmith well enough that people don't just interpret it as "never exceed X".

[sudo-bmitch]

I'd suggest that we should specify that lower limit. It doesn't prevent implementations from supporting more than that limit, but it does provide interoperability when each tool could have a different limit. Where possible, we want to avoid a situation where some clients can't run images created by some build tooling because they made different assumptions of this lower limit.

Build tools should attempt to remain below the limit where possible, throwing a warning when the resulting manifest can't be created that's small enough. While client tools and registries should support at least the lower limit.

Future enhancements to the spec that won't be used by older clients and registries (like artifacts) could also increase this lower limit, allowing the spec to grow over time with interoperability.

[imjasonh]

Informal testing indicates:

• gcr.io supports manifests up to 10MB, after which it fails with UNKNOWN: Bad Request.
• quay.io supports manifests up to 1MB, after which it fails with 413 Request Entity Too Large (an nginx error, not a spec error)
• docker.io supports manifests up to 4MB, MANIFEST_INVALID: manifest invalid; http: request body too large

[sudo-bmitch]

distribution/distribution appears to limit to 4MB: https://github.com/distribution/distribution/blob/main/registry/handlers/manifests.go#L31

[imjasonh]

@SteveLasker can you provide data for ACR? I don't have creds handy to test it out myself.

[SteveLasker]

ACR has a manifest size of 4mb, which we only documented as a result of this conversation. However, we've not had any problems to date, as manifests have been small in size, because there's no real pattern for users to put lots of content in a manifest.

Layer sizes are 200gb.

We have had problems with users pushing excessive layer sizes, particularly around ML scenarios. Which is an interesting problem. I could easily see someone trying to build ML tooling, where they may try to push the model as a data element, which is likely excessive to what manifests are intended to support. (https://github.com/opencontainers/distribution-spec/issues/290)

ACR limits are captured here: Service tier features and limits

While I support a max manifest and size limit, I don't believe this should be used to account for the proposed data element without a constraint on the data element. I would consider the manifest size to be similar to a circuit breaker.

• Each circuit has a breaker, set to a specific size
• The main panel has a master limit.

The sum of the individual breakers (10, 15, 20, 50amps) far exceeds the max of the panel (200amps). This proposal is good to enforce a limit on the manifest, but without limits on elements that can exceed reasonable size, we leave ourselves open to inconsistencies and user frustration, rather than providing a spec that promotes standards and consistencies across implementations.

[SteveLasker]

@imjasonh

can you provide data for ACR? I don't have creds handy to test it out myself.

https://azure.microsoft.com/free/

[jonjohnsonjr]

but without limits on elements that can exceed reasonable size, we leave ourselves open to inconsistencies and user frustration

Individual circuit breakers prevent a given loop from getting so hot that your house burns down.

What failure mode do you anticipate for an individual element's size that is not prevented by a reasonable size limit on the manifest? Concretely, what do you think the maximum length of a string should be, and why?

[SteveLasker]

Both individual and the master breaker prevent this, as any individual circuit can support a max (for various reasons). While the master breaker also protects from bad things. But, lets explore:

The manifest concerns are covered in the list captured here. This is what I'd call the mainline (the line that runs from the street to the house)

For annotations, it's a good question. I don't know of many implementations that make great use of the annotations, which is likely why we're not seeing a lot of adoption. I would like to see meta-data services become a standardized thing. The idea would be users could query on meta-data to find the artifacts that match them. To support these scenarios, query string name/value pairs would likely have a "reasonable limit". The indexes we would all build to support a meta-data search would also be super helpful to have a "reasonable size".

Since ACR doesn't index annotations today, I don't have a way to test what already exists. I'd probably open the bid at 256 characters. I could possibly raise that bid to 512 if there were really good reasons. But, I'd also want to think about what APIs would look like for name/value pairs.

For instance, if I do a google search for "oci annotations", the search bar populates with https://www.google.com/search?q=oci+annotations&source=hp&ei=e_wWYe3NCbXN0PEPuZ6JoAM&iflsig=AINFCbYAAAAAYRcKi5tCjJEeotEC6h6sEklNfppYddJg&oq=oci+annotations&gs_lcp=Cgdnd3Mtd2l6EAMyBQgAEIAEMgYIABAWEB46CAgAEOoCEI8BOhEILhCABBCxAxDHARCjAhCTAjoICAAQgAQQsQM6CAguEIAEELEDOgsIABCABBCxAxCDAToOCC4QgAQQsQMQxwEQowI6BQguEIAEOggILhCABBCTAjoOCC4QsQMQgwEQxwEQrwE6CAgAELEDEIMBOgsILhCxAxDHARCjAjoLCC4QgAQQxwEQrwFQog5Yyxpg-bUBaAFwAHgAgAFGiAH-BJIBAjE1mAEAoAEBsAEK&sclient=gws-wiz&ved=0ahUKEwit97jwj6_yAhW1JjQIHTlPAjQQ4dUDCAk&uact=5

I'm not sure I want to know what all the other goo in the URL means.

[jonjohnsonjr]

For annotations, it's a good question. I don't know of many implementations that make great use of the annotations, which is likely why we're not seeing a lot of adoption. I would like to see meta-data services become a standardized thing. The idea would be users could query on meta-data to find the artifacts that match them.

That's a reasonable application. I'd recommend opening a discussion to limit the maximum size of annotations (or suggest a "safe" boundary) either in distribution-spec or image-spec to that end. I'd argue that this is actually a good reason to support the data field PR to ensure that it lives outside of annotations if we expect people to start indexing annotations. Without the data field, stuffing things in annotations is the most obvious alternative.

[sudo-bmitch]

I don't believe this should be used to account for the proposed data element without a constraint on the data element.

@SteveLasker would you create a separate PR for that request?