Skip to content

fix(ns8): normalise the registry path case on the fetch, not just the token #152

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.

Sign up for GitHub

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

Closed
shockalotti wants to merge 1 commit into from
Closed

fix(ns8): normalise the registry path case on the fetch, not just the token #152

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 46 additions & 12 deletions packaging/ns8/assert-multiarc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,29 @@
--expect-app-tag T assert every label names exactly this app tag, so the
module release and the app release are the same version

There is deliberately NO option to override the app repository. The app image a
module runs is whatever its org.nethserver.images label names, so the label is
the only authoritative source and it is also what a fork needs: a module built
under someone else's namespace names their app image, and deriving from the
label handles that with no configuration. An override here would be worse than
useless, because it would let a pair check be asserted against an app repository
the module does not actually reference, hiding a mislabelled module.
There is deliberately no option to override the app repository, and the reason
is narrower than "an override is always wrong".

The app image a module runs is whatever its org.nethserver.images label names,
so the label is the only authoritative source. Deriving from it also needs no
configuration for a fork: a module built under another namespace names their app
image, and reading the label handles that on its own.

So an override used as a SUBSTITUTE for the label is a footgun. It would assert
the pair against an app repository the module does not actually reference, which
hides a mislabelled module instead of catching it. That is the same hole
--expect-app-tag exists to close, one level up.

An override used as an ASSERTION instead would be strictly stronger, not weaker,

@pullfrog pullfrog Bot Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This module docstring is argparse's description, so it is printed verbatim by --help — which is now 55 lines, roughly 23 of them an essay about --expect-app-repo, a flag that does not exist, and argparse's default formatter collapses the paragraphs into one run-on block. The substitute-vs-assertion distinction is worth keeping and worth the comment density this file already has; consider shortening this section to the rule itself (two or three sentences) and leaving the rest as an inline comment near --expect-app-tag in main(), where it is readable but does not hit every --help.

and is a real future need. Today --expect-app-tag catches a wrong app TAG, but a
module labelled `ghcr.io/someoneelse/calnode:0.11.1` still passes as long as that
image exists and is multi-arch. A `--expect-app-repo` would fail unless the label
names exactly the repository AND tag this release publishes, which is what a fork
publishing under its own namespace wants and cannot express from the label alone.

We do not need it until someone maintains such a fork. Until then, adding it as a
substitute would be the bug; adding it as an assertion would be the fix. The
distinction to preserve is which of the two it is, not whether to have it.

Exit status is 0 only if every tag passes every applicable check.

Expand Down Expand Up @@ -72,12 +88,28 @@ class Fail(Exception):
"""A check failed in a way worth reporting verbatim."""


def normalize_repo(repo):
"""Registry repository paths are case-sensitive and must be lowercase.

ghcr's TOKEN endpoint wants a lowercase scope and answers HTTP 400
otherwise. Its MANIFEST endpoint is stricter: an uppercase path is simply a
repository that does not exist, so it answers HTTP 404 while the lowercase
path of the very same image resolves.

That combination is what made this invisible. The owner arrives from GitHub
as `github.repository_owner` verbatim, which is `Calnode` for this repo, so
Comment on lines +99 to +100

@pullfrog pullfrog Bot Oct 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The normaliser is correct and I confirmed it turns the red-main case green, but the mechanism in this docstring does not reproduce against ghcr today: an uppercase owner segment fetches fine (Calnode/ns8-calnode → HTTP 200, identical digest), and an uppercase scope is accepted too. What actually 404s is ghcr.io/Calnode/ns8-calnode — because both workflows pass a fully-qualified ref that the script prepends https://ghcr.io/v2/ to, and ghcr strips that redundant ghcr.io/ segment case-sensitively, which pushes the real owner into the case-sensitive repository-name position. Worth correcting, because the next person reasoning from this paragraph will conclude the owner case is what matters, when lowercasing Calnode in a bare Calnode/ns8-calnode changes nothing.

Technical details 
# The docstring's causal claim is not reproducible; the real trigger is the doubled host

## Measured against ghcr.io (anonymous pull token for calnode/ns8-calnode, manifests/:edge)

| repo path            | token scope | manifest                    |
|----------------------|-------------|-----------------------------|
| calnode/ns8-calnode  | 200         | 200                         |
| Calnode/ns8-calnode  | 200         | 200, same digest            |
| CALNODE/ns8-calnode  | 200         | 200, same digest            |
| calnode/NS8-CALNODE  | -           | 404                         |
| ghcr.io/calnode/ns8-calnode | 200  | 200, same digest            |
| ghcr.io/Calnode/ns8-calnode | 400  | 404                         |
| ghcr.io/foo/bar      | -           | 404                         |

`calnode/ns8-calnode`, `CALNODE/ns8-calnode` and `ghcr.io/calnode/ns8-calnode` all
return digest sha256:06a54f2c...713 — so ghcr matches the OWNER segment
case-insensitively, silently strips a redundant leading `ghcr.io/` segment, and is
case-sensitive on the remainder. `ghcr.io/Calnode/ns8-calnode` 404s because after the
stripped host, `Calnode` sits in the repository-name position.

## Affected sites

- packaging/ns8/assert-multiarc.py:99-103 — the paragraph claims the manifest endpoint
  answers 404 for "an uppercase path". Only true for the repository-name segment, and
  the case that actually broke CI (`Calnode`, the owner) is fine until the redundant
  host moves it into that position.
- packaging/ns8/assert-multiarc.py:120,131 — `get()`/`get_blob()` interpolate the caller's
  `repo` verbatim into `{REGISTRY}/v2/{repo}/...`, so a fully-qualified arg yields
  `https://ghcr.io/v2/ghcr.io/...`.
- .github/workflows/ns8-module.yml:264 and .github/workflows/release-verify.yml:114 —
  both set `MODULE_REPO: ghcr.io/${{ github.repository_owner }}/ns8-calnode`, i.e. they
  pass a fully-qualified ref into a positional arg documented (assert-multiarc.py:258) as
  `calnode/ns8-calnode`. Neither validates the arg's shape.
- packaging/ns8/assert-multiarc.py:114 — the token scope built from that arg is
  `repository:ghcr.io/calnode/ns8-calnode:pull`, which ghcr accepts today but which no
  spec authorises. Lowercasing fixes the case, not the malformed scope.

## Required outcome

- The docstring must state the rule that is actually true and still worth obeying: a
  repository NAME must be lowercase (OCI distribution-spec `<name>` regex
  `[a-z0-9]+((\.|_|__|-+)[a-z0-9]+)*`, tags are the part that may be uppercase), and
  `docker`/`podman`/`skopeo` all reject an uppercase name client-side. ghcr's leniency
  in the owner segment is undocumented and can change, so normalising is load-bearing,
  not defensive. That argument survives whatever ghcr does tomorrow; "it 404s" may not.
- The paragraph should stop attributing the CI failure to the owner's case alone.

## Suggested approach

Keep the three `normalize_repo()` call sites — the fix works and matches the precedent
already in packaging/ns8/push-manifest.sh:36 (`repobase="${repobase,,}"`). Two optional
improvements, both small:

1. Parse the positional arg once at the entry point rather than lowercasing at three URL
   builders: strip a leading host with the same logic `split_ref()` already uses for
   `org.nethserver.images` labels (assert-multiarc.py:141), reject a host that is not
   ghcr.io, then lowercase. That yields a well-formed `repository:calnode/ns8-calnode:pull`
   scope and `https://ghcr.io/v2/calnode/ns8-calnode/manifests/edge`, so the script stops
   depending on ghcr tolerating a doubled path segment. It also retires the loose wording
   "the single point where a registry path becomes a URL" — there are three call sites
   today (harmless, the function is idempotent, but a future fourth URL builder is not
   covered by that sentence).
2. `normalize_repo` is pure and CI only runs `python3 -m py_compile` on this file
   (ns8-module.yml:36). A handful of offline unit tests over the URL and scope strings
   would have caught the token-vs-fetch half of this bug without a network call, which is
   the failure mode this file has now produced twice.

## Open questions for the human

- Was the HTTP 400 on an uppercase scope, and the 404 on the uppercase manifest, actually
  observed, or inferred? My measurements cannot reproduce either for the owner segment
  today. If it was observed, the authenticated-vs-anonymous distinction may be the missing
  variable and is worth a sentence in the docstring; if it was inferred, the paragraph
  should not claim it as measured.

ns8-module.yml passes `ghcr.io/Calnode/ns8-calnode`. Lowercasing inside
token() alone fixed the scope request and left the fetch 404ing, so the
release looked fixed while every push to main and dev stayed red.

Normalise at the single point where a registry path becomes a URL, so every
caller is covered and neither endpoint can be missed again.
"""
return repo.lower()


def token(repo):
# ghcr's token endpoint requires the repository path LOWERCASE and answers
# HTTP 400 otherwise. The owner arrives from GitHub as `github.repository_owner`
# verbatim, which is `Calnode` for this repo, so normalise here rather than
# trusting every caller to remember: a case slip here fails a correct release.
repo = repo.lower()
repo = normalize_repo(repo)
if repo not in _tokens:
url = f"{REGISTRY}/token?scope=repository:{repo}:pull&service=ghcr.io"
with urllib.request.urlopen(url, timeout=30) as f:
Expand All @@ -87,6 +119,7 @@ def token(repo):

def get(repo, reference):
"""GET a manifest, returning (body, mediaType)."""
repo = normalize_repo(repo)
req = urllib.request.Request(
f"{REGISTRY}/v2/{repo}/manifests/{reference}",
headers={"Authorization": f"Bearer {token(repo)}", "Accept": ACCEPT},
Expand All @@ -96,6 +129,7 @@ def get(repo, reference):


def get_blob(repo, digest):
repo = normalize_repo(repo)
req = urllib.request.Request(
f"{REGISTRY}/v2/{repo}/blobs/{digest}",
headers={"Authorization": f"Bearer {token(repo)}"},
Expand Down
Loading