ociclient: `Content-Length` "SHOULD" be present, but isn't guaranteed

[tianon]

This is really related to what we discussed/implemented in #29 -- many of the methods in ociclient pass down a "knownDigest" value when they're making additional requests in response to a descriptor, especially so that those requests results can still have a Descriptor even if the server doesn't implement the "SHOULD" of providing the Docker-Content-Digest header:

oci/ociregistry/ociclient/client.go

Lines 168 to 175 in 2c00c10

	digest := digest.Digest(resp.Header.Get("Docker-Content-Digest"))
	if digest != "" {
	if !ociref.IsValidDigest(string(digest)) {
	return ociregistry.Descriptor{}, fmt.Errorf("bad digest %q found in response", digest)
	}
	} else {
	digest = knownDigest
	}

However, as I've noted in #29 (comment), having Content-Length is also technically not a given: https://www.rfc-editor.org/rfc/rfc9110.html#name-content-length

We've seen cases of this in practice tripping this error block (although to be fair, they're not cases where the server is supposed to be omitting Content-Length and servers definitely should include it, but it's valid for them not to):

oci/ociregistry/ociclient/client.go

Lines 162 to 164 in 2c00c10

	if resp.ContentLength < 0 {
	return ociregistry.Descriptor{}, fmt.Errorf("unknown content length")
	}

With respect to the OCI descriptor data type, that's digest and size, and the other relevant value is mediaType, which the code also already handles with a (sub-optimal) fallback:

oci/ociregistry/ociclient/client.go

Lines 141 to 144 in 2c00c10

	contentType := resp.Header.Get("Content-Type")
	if contentType == "" {
	contentType = "application/octet-stream"
	}

Arguably, if a request is made in response to an explicit descriptor (pulling the child of a manifest, for example), the values from the original descriptor should always be preferred as "more trusted" than the values from the response headers anyways (it's also arguable that in the case of different digest algorithms in digest, they should both be verified, but that's a whole separate concern).

For now, what I'm proposing to improve this situation is upgrading the knownDigest digest.Digest values we pass down/around to be full knownDescriptor ociregistry.Descriptor objects instead. I'm not sure how this might exhibit all the way up in ociregistry.Interface, because I think for this to work it probably has to? Maybe some way to shove a value into the context? (isn't that what context is for? apologies if not, it's not a strong suit for me, but I feel bad throwing out this issue without making an attempt at proposing solutions 🙇 ❤️)

Regardless, thanks as always for creating and maintaining such a great library. 😄 ❤️

[tianon]

oof, this gets even worse with transport compression - Go's net/http helpfully adds Accept-Encoding: gzip for us, and then auto-decompresses the response body for us transparently when we get it, but the Content-Length is the compressed size, not the uncompressed size.

I think this might be the only OCI client that's affected by this though, because the BlobReader interface that infers (most of) a full OCI descriptor from the HTTP headers needs Content-Length for size, but it isn't that at all. 😭

[tianon]

Ah, the way net/http implemented this, a registry providing the implicitly requested gzip compression ends up looking exactly like this issue because Content-Length gets removed:

	// Uncompressed reports whether the response was sent compressed but
	// was decompressed by the http package. When true, reading from
	// Body yields the uncompressed content instead of the compressed
	// content actually set from the server, ContentLength is set to -1,
	// and the "Content-Length" and "Content-Encoding" fields are deleted
	// from the responseHeader. To get the original response from
	// the server, set Transport.DisableCompression to true.
	Uncompressed bool

[rogpeppe]

Thanks again for continuing to look at these issues - it's very helpful. I shall try to have to think about this when I get back to my keyboard. It's been a while since I was deep into this code...