Ok so this breaks storage tests, but the validation succeeds which makes me think the v2 dependency tracking is trashy as it always has been 🫠

Thanks I'll take a look at the failing tests

I've re-run the failed job and it seems we're good now, but I'm now wondering if there is some race condition lurking somewhere in the tests 🤨

Why? Just because you can, doesn't mean you should. Client support for h2c is limited. If anyone wants to expose a registry over h2c, they can stick a reverse proxy in front. I don't see a compelling reason to natively support h2c in distribution.

Thanks for the feedback @corhere, I should have linked the issue in this PR too. See the use case in #4241

Not sure how one would put a reverse proxy when using a managed service or any kind of hosted load balancer.
I was also following the suggestion in #4246 (comment)

If it makes sense I can put this via an opt-in configuration too as in #4246

Fair, that's a legitimate use case. I'll have to think about whether it makes more sense as opt-in or opt-out

Personally, I'd keep it opt-in for now, instead of enforcing it on users in v3 release.

If there is a stronger demand for this we can make it a default in the next release and change it to opt-out.

Personally, I'd keep it opt-in for now, instead of enforcing it on users in v3 release.

Done in 8eb4f48

I've re-run the failed job and it seems we're good now, but I'm now wondering if there is some race condition lurking somewhere in the tests 🤨

Fixed the storage tests in #4251

Do you know of clients that support this?

Would enabling it by default break any scenarios you can think of?

As we are doing a major release, turning feature like this on by default makes sense to me as long as its safe and useable.

Do you know of clients that support this?

I tested it in the Docker Go SDK and it works. Haven't tried about other clients. Having it work in the Go SDK means that the engine/daemon supports it right?

@Jamstah The "client" in the author's case is the GCP Cloud Run application gateway.

I don't think we should enable h2c support by default, ever. All mechanisms for client/server negotiation of h2c have been deprecated, so h2c is only useable by a client with prior knowledge that the server supports h2c. I think it's safe to say that no sane registry client should be making h2c requests to a distribution service over the internet, so h2c support in the distribution server is only useful in a deployment scenario such as the GCP Cloud Run one where an HTTP/2-compatible application gateway is terminating TLS without downgrading the reverse-proxied connection to HTTP/1.1. In practice, "prior knowledge" for the gateway takes the form of explicit configuration to forward requests to the backend as h2c, so an administrator would have to explicitly opt into using h2c in their environment regardless. Therefore enabling h2c support in distribution by default would only be a trivial convenience for administrators who know about h2c and want to use it in their environment, while being of no benefit to anyone else.

If h2c support is enabled, it might be possible to smuggle crafted HTTP/2 requests past an HTTP/1.1-only application gateway that the gateway would have otherwise blocked. Having h2c support enabled by default would therefore open everyone else up to HTTP request smuggling attacks they would not otherwise be impacted by. Furthermore, having Go's HTTP/2 server implementation enabled increases the attack surface for remotely-exploitable vulnerabilities. It would be an acceptable risk iff the distribution service could receive legitimate HTTP/2 requests. Only the administrator of the distribution service can judge for themselves whether the risk is acceptable for their environment. Therefore h2c disabled by default is the only correct choice, in my view.

Thanks for the explanation. Sounds like we're on the right track then, and can merge after adding to the docs.

CI failure seems unrelated (fixed in #4256).

Added another commit for the docs in 61b4e27.

I updated the http2 part to clarify it's only used when tls is configured and also used the proper casing for HTTP where applicable.

Please let me know what you think

Now that #4256 has been merged, mind rebasing @erezrokah

Looks like we still need to remove

Looks like we still need to remove

#4257

Looks like labeler@v5 changed the structure of the config file significantly hence https://github.com/distribution/distribution/actions/runs/7543288691/job/20533972457?pr=4248#step:2:9 (see https://github.com/actions/labeler/tree/main?tab=readme-ov-file#breaking-changes-in-v5)

Looks like labeler@v5 changed the structure of the config file significantly hence

It's hard not feel persistently depressed when debugging GHA issues....it really is

It's hard not feel persistently depressed when debugging GHA issues....it really is

Hopefully #4258 fixes it

Can you squash @erezrokah

Can you squash @erezrokah

I rebased the PR. Do you want me to squash all commits into a single one or you'll do it once you merge the PR?

Squash down to one commit before merge, please. Squash-merge is disabled on this repository.

Squash down to one commit before merge, please. Squash-merge is disabled on this repository.

Done, thanks

Thanks everyone for the thoughtful reviews 🚀