Exclude domain from name length check

[ozairasim]

Fixes: distribution/distribution#2668 by only including the path (repository name) in the total name length check

As recommended by @thaJeztah in distribution/distribution#2668 (comment), there could be constraints on the total reference length, hence it should be mentioned in the new release

[thaJeztah]

As this const is exported, we should probably alias the old one, and mark it deprecated;

	// NameTotalLengthMax is the maximum total number of characters in a repository name.
	//
	// Deprecated: use [RepositoryNameTotalLengthMax] instead.
	NameTotalLengthMax = RepositoryNameTotalLengthMax

[ozairasim]

Good points, thank you!

[thaJeztah]

Perhaps validate match[2], so that we can keep the return more atomic (return repository{ ...}), instead of constructing the repository and discarding it in case there's a failure.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.71%. Comparing base (8507c7f) to head (aaca75e).

Additional details and impacted files

@@           Coverage Diff           @@
##             main       #9   +/-   ##
=======================================
  Coverage   83.71%   83.71%           
=======================================
  Files           5        5           
  Lines         393      393           
=======================================
  Hits          329      329           
  Misses         54       54           
  Partials       10       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thaJeztah]

This example may be incorrect here, because the first component is only considered to be a domain if it either contains a period (.), a colon (:) or it's literally localhost

reference/reference.go

Lines 8 to 10 in 8507c7f

	// domain := host [':' port-number]
	// host := domain-name | IPv4address | \[ IPv6address \] ; rfc3986 appendix-A
	// domain-name := domain-component ['.' domain-component]*

If we intend to have a domain here, it should probably contain a ., and use one of the designated RFC 6761https://www.rfc-editor.org/rfc/rfc6761.html) domains for this (e.g., example.com or example.org, or <something>.test

[thaJeztah]

Same here

[thaJeztah]

Hm.. why did this one change? Not sure if that's expected 🤔

[ozairasim]

It's already returning ErrReferenceInvalidFormat on main, without the new changes. Should I rather make the test pass by changing the behaviour instead?

[thaJeztah]

Not sure I got that, you mean it's currently failing on main? I just give this test a run, and it seems to pass, or is it broken in some other way?

go test -v -run TestWithNameFailure
=== RUN   TestWithNameFailure
=== PAUSE TestWithNameFailure
=== CONT  TestWithNameFailure
=== RUN   TestWithNameFailure/#00
=== PAUSE TestWithNameFailure/#00
=== RUN   TestWithNameFailure/:justtag
=== PAUSE TestWithNameFailure/:justtag
=== RUN   TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== PAUSE TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== RUN   TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== PAUSE TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== RUN   TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag
=== PAUSE TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag
=== RUN   TestWithNameFailure/aa/asdf$$^/aa
=== PAUSE TestWithNameFailure/aa/asdf$$^/aa
=== CONT  TestWithNameFailure/#00
=== CONT  TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
=== CONT  TestWithNameFailure/:justtag
=== CONT  TestWithNameFailure/aa/asdf$$^/aa
=== CONT  TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag
=== CONT  TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
--- PASS: TestWithNameFailure (0.00s)
    --- PASS: TestWithNameFailure/#00 (0.00s)
    --- PASS: TestWithNameFailure/:justtag (0.00s)
    --- PASS: TestWithNameFailure/aa/asdf$$^/aa (0.00s)
    --- PASS: TestWithNameFailure/validname@invaliddigest:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (0.00s)
    --- PASS: TestWithNameFailure/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a/a:tag (0.00s)
    --- PASS: TestWithNameFailure/@sha256:ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff (0.00s)
PASS
ok  	github.com/distribution/reference	0.689s

[ozairasim]

Sorry for the confusion about making the test pass. The test would pass, but it would return ErrReferenceInvalidFormat instead of ErrNameEmpty which is the expectation. The test will not fail as we are only asserting on main that there is an error and not asserting which error it is.

If you change the assertion from:

if err == nil {
    t.Errorf("no error parsing name. expected: %s", tc.err)
}

to:

if !errors.Is(err, tc.err) {
    t.Errorf("unexpected error parsing name. expected: %s, got: %s", tc.err, err)
}

to check the actual error that is returned is equal to the expectation in the test, the test will fail. So the test is passing because we are not asserting correctly what is expected in the test input

[thaJeztah]

Wondering if there's still constraints (or if there should be) on maximum length of individual path components ("namespace", "repository-name"). At a quick glance, HTTP itself doesn't seem to define constraints; https://www.rfc-editor.org/rfc/rfc3986#section-3.3. Maybe it's fine to have the "total" alone. (it might be fun though to have a "valid" repository, but not able to use it because there's no space left for the total length 🤔

[ozairasim]

it might be fun though to have a "valid" repository, but not able to use it because there's no space left for the total length

Can you please provide an example for this? I didn't completely understand it 😞

[thaJeztah]

Let me also cc @dmcgowan @stevvooe @milosgajdos for more eyes (changes in these areas area always "fun")

[stevvooe]

LGTM (but might have to defer to others)

If I remember correctly, this code path was originally used to qualify the repository name, which we kept short to support a variety of storage backends. The domain was never included in that calculation. As this package gained more scoped, it seems this was an oversight.

I think excluding the domain name from the length check is safe, but we may want to ensure there are sane limits on the domain name. For example, domain components typically have a 63 character limit but getting that right would require more research.

[milosgajdos]

but we may want to ensure there are sane limits on the domain name

I agree with this sentiment, but getting this reasonably right might be a mission. My personal preference would be for this to be defined in distribution-spec if we must.

[milosgajdos]

PR looks good, I'm happy to merge this but I'd like you to open an issue in https://github.com/opencontainers/distribution-spec and reference this PR @ozairasim

It'd be great to have a cross-reference b/w the spec and this PR. it'd be fantastic if the issue listed, other than the description of the problem, what is the behaviour like in the major Registry providers e.g. ECR, GCR, GitLab, etc so the maintainers can gauge the importance and severity of the issue.

Thanks <3

[ozairasim]

Thanks a lot for the review everyone!

And thank you for the suggestion @milosgajdos to open an issue on the spec, I have created the issue to define some limits on the spec as well. Let me know if I can add something else in the issue!

[Jamstah]

Linking spec issue: opencontainers/distribution-spec#530