feat: support distribution v3.x via x5c JWT header#20
Conversation
distribution/distribution v3.0.0 replaced libtrust with go-jose and keys its trustedKeys map by RFC 7638 JWK thumbprint, so tokens signed with the old libtrust-style kid header (which v2.8.1 used) are no longer matched. Switch to emitting the signing certificate in the JWS x5c protected header. Both v2.8.1 (parseAndVerifyCertChain) and v3.0.0 (verifyCertChain) verify x5c against the rootcertbundle, so the same token works on both — no libtrust dependency needed. - pkg/server/auth.go: drop libtrust kid derivation; embed the signing cert as base64 DER in the x5c header. - go.mod/go.sum: remove github.com/docker/libtrust (archived). - scripts/simple-tests.sh: parametrize REGISTRY_VERSIONS so CI exercises both v2.8.1 and v3.0.0 (defaults to both). - pkg/server/server_test.go: cover the x5c regression and the previously-untested Authenticate / Authorize / DecodeScope* paths (raises pkg/server coverage from 0% to ~27%). - README/README_zh: note v2/v3 dual compatibility and v3's config path. - CLAUDE.md: project guidance with the x5c rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 43 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR removes the ChangesLibtrust Removal and x5c Header Migration
Multi-Version Registry Test Harness
Documentation and Compatibility Notes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
scripts/simple-tests.sh ran `skopeo copy ... docker-archive:/tmp/registry-src.tar` inside do_tests, so the second iteration (v3.0.0) failed with "docker-archive doesn't support modifying existing images" because the v2.8.1 pass had already written the file. Move the source-image fetch into its own prepare_source_image called once from main(), and guard with -f so reruns are idempotent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/server/options/basic.go (1)
127-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
AuthConfigLabelSelectoris assigned from the wrong field.
Line 130currently assignso.AuthConfigNamespace; this drops the configured selector and breaks secret selection behavior.Proposed fix
func (o *BasicOptions) ApplyToServer(server *server.Server) error { server.BasicConfig.AuthConfigFile = o.AuthConfigFile server.BasicConfig.AuthConfigNamespace = o.AuthConfigNamespace - server.BasicConfig.AuthConfigLabelSelector = o.AuthConfigNamespace + server.BasicConfig.AuthConfigLabelSelector = o.AuthConfigLabelSelector server.BasicConfig.AuthTokenDuration = o.AuthTokenDuration server.BasicConfig.AuthThirdpartyServer = o.AuthThirdpartyServerAs per coding guidelines: "Fix copy-paste bug in
BasicOptions.ApplyToServer(pkg/server/options/basic.go:130): assigno.AuthConfigLabelSelector(noto.AuthConfigNamespace) intoserver.BasicConfig.AuthConfigLabelSelector."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/options/basic.go` around lines 127 - 131, In BasicOptions.ApplyToServer, fix the copy-paste bug where server.BasicConfig.AuthConfigLabelSelector is set from o.AuthConfigNamespace; change the assignment to use o.AuthConfigLabelSelector instead so the label selector configured on BasicOptions is propagated to server.BasicConfig.AuthConfigLabelSelector (locate the assignment in the ApplyToServer method and replace the wrong RHS).pkg/server/auth.go (1)
226-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the cert matches the RSA private key before emitting
x5c
generateSigneronly checks the private key is RSA, but it emitsx5cfroma.cert.Rawwithout ensuringa.cert.PublicKeymatchesrsaPrivKey.PublicKey(and without guardinga.certbeing nil). If--auth-public-cert-fileand--auth-private-key-filedrift, startup succeeds and every issued token will fail verification downstream.Suggested guard
func (a *AuthProcessor) generateSigner(privateKeyFile string) error { data, err := ioutil.ReadFile(privateKeyFile) if err != nil { return err } privKey, err := keyutil.ParsePrivateKeyPEM(data) if err != nil { return err } rsaPrivKey, ok := privKey.(*rsa.PrivateKey) if !ok { return fmt.Errorf("the private key is not in RSA format") } + if a.cert == nil { + return fmt.Errorf("public certificate not loaded") + } + certPubKey, ok := a.cert.PublicKey.(*rsa.PublicKey) + if !ok { + return fmt.Errorf("the certificate public key is not in RSA format") + } + if certPubKey.N.Cmp(rsaPrivKey.PublicKey.N) != 0 || certPubKey.E != rsaPrivKey.PublicKey.E { + return fmt.Errorf("the certificate does not match the private key") + } op := (&jose.SignerOptions{}).WithHeader( jose.HeaderKey("x5c"), []string{base64.StdEncoding.EncodeToString(a.cert.Raw)}, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/auth.go` around lines 226 - 249, generateSigner currently assumes a.cert is set and that its public key matches the parsed RSA private key before adding the x5c header; add checks in generateSigner to ensure a.cert is not nil and that a.cert.PublicKey corresponds to rsaPrivKey.PublicKey (compare modulus/exponent for *rsa.PublicKey) and return a clear error if they differ (so tokens aren't signed with a mismatched cert), before constructing the jose.SignerOptions and creating a.signer.
🧹 Nitpick comments (3)
pkg/server/options/basic.go (1)
43-43: ⚡ Quick winFix stale constructor comment text.
This comment references
NewConsoleOptions/ConsoleOptions, but the function isNewBasicOptionsand returns*BasicOptions.Proposed fix
-// // NewConsoleOptions creates a ConsoleOptions object with default parameters. +// NewBasicOptions creates a BasicOptions object with default parameters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/options/basic.go` at line 43, Update the stale constructor comment to reference the correct symbols: replace mentions of NewConsoleOptions/ConsoleOptions with NewBasicOptions/*BasicOptions* so the doc comment accurately describes that NewBasicOptions creates a BasicOptions object with default parameters; ensure the comment text matches the function name NewBasicOptions and the returned type BasicOptions.pkg/server/server_test.go (1)
213-268: ⚡ Quick winCover the named-user→
_anonymousfallback in one handler-level test.The current suite proves
_anonymousrules work when requested directly, but it does not exercise the production path where a named user gets no matching scope andpkg/server/handlers.gofalls through to_anonymous. That leaves the documented auth contract unpinned.As per coding guidelines,
Lookup order for users/authorizations is: Secret first, then Static, then third-party. Implement fallthrough to _anonymous user when a user has no matching authorization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/server_test.go` around lines 213 - 268, Add a new assertion in TestLoadFromFile_YAMLAndAuthorize to cover the handler-level fallthrough to the _anonymous rules: call p.Authorize("alice", AccessScope{{Type: RepositoryAccessType, Name: "public/x"}}) and assert it returns a single rule with PullAction (same expectations as the existing anonymous test), referencing the TestLoadFromFile_YAMLAndAuthorize test and the Authorize method/AnonymousUser/_anonymous rule to ensure a named user with no matching rule falls back to anonymous.scripts/simple-tests.sh (1)
120-127: ⚡ Quick winGuarantee registry cleanup on failures.
With
set -e, any failure insideinstall_registry,start_registry, ordo_testsexitsmainbeforestop_registryruns. That leaves the background registry bound to:5000, which makes local reruns flaky and hides the real failing version.♻️ Suggested cleanup guard
function main() { + trap 'stop_registry' EXIT prepare_certs prepare_skopeo prepare_auth prepare_registry_auth for version in ${REGISTRY_VERSIONS}; do echo "=== testing against distribution ${version} ===" install_registry "${version}" start_registry do_tests "${version}" stop_registry done }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/simple-tests.sh` around lines 120 - 127, The loop can leave the registry running if a command fails because set -e exits before stop_registry; fix by adding a cleanup trap and a flag around each iteration: set REGISTRY_STARTED=0, call install_registry, call start_registry and set REGISTRY_STARTED=1, install a trap on EXIT that checks REGISTRY_STARTED and calls stop_registry if set, run do_tests, then explicitly call stop_registry, clear REGISTRY_STARTED and remove the trap; reference the existing functions install_registry, start_registry, do_tests, stop_registry to implement this guard so the registry is always cleaned up on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/server_test.go`:
- Line 271: Replace all uses of httptest.NewRequest in the test to call
httptest.NewRequestWithContext(context.Background(), ...) instead and add an
import for the context package; specifically update the calls creating req (the
instances at lines noted: the occurrences that construct the request for
"/auth/token" etc.) to use NewRequestWithContext(context.Background(), "GET",
"http://x/auth/token?service=svc&scope=repository:foo/bar:pull,push", nil)
pattern, and add "context" to the import block so the tests compile and use a
background context.
---
Outside diff comments:
In `@pkg/server/auth.go`:
- Around line 226-249: generateSigner currently assumes a.cert is set and that
its public key matches the parsed RSA private key before adding the x5c header;
add checks in generateSigner to ensure a.cert is not nil and that
a.cert.PublicKey corresponds to rsaPrivKey.PublicKey (compare modulus/exponent
for *rsa.PublicKey) and return a clear error if they differ (so tokens aren't
signed with a mismatched cert), before constructing the jose.SignerOptions and
creating a.signer.
In `@pkg/server/options/basic.go`:
- Around line 127-131: In BasicOptions.ApplyToServer, fix the copy-paste bug
where server.BasicConfig.AuthConfigLabelSelector is set from
o.AuthConfigNamespace; change the assignment to use o.AuthConfigLabelSelector
instead so the label selector configured on BasicOptions is propagated to
server.BasicConfig.AuthConfigLabelSelector (locate the assignment in the
ApplyToServer method and replace the wrong RHS).
---
Nitpick comments:
In `@pkg/server/options/basic.go`:
- Line 43: Update the stale constructor comment to reference the correct
symbols: replace mentions of NewConsoleOptions/ConsoleOptions with
NewBasicOptions/*BasicOptions* so the doc comment accurately describes that
NewBasicOptions creates a BasicOptions object with default parameters; ensure
the comment text matches the function name NewBasicOptions and the returned type
BasicOptions.
In `@pkg/server/server_test.go`:
- Around line 213-268: Add a new assertion in TestLoadFromFile_YAMLAndAuthorize
to cover the handler-level fallthrough to the _anonymous rules: call
p.Authorize("alice", AccessScope{{Type: RepositoryAccessType, Name:
"public/x"}}) and assert it returns a single rule with PullAction (same
expectations as the existing anonymous test), referencing the
TestLoadFromFile_YAMLAndAuthorize test and the Authorize
method/AnonymousUser/_anonymous rule to ensure a named user with no matching
rule falls back to anonymous.
In `@scripts/simple-tests.sh`:
- Around line 120-127: The loop can leave the registry running if a command
fails because set -e exits before stop_registry; fix by adding a cleanup trap
and a flag around each iteration: set REGISTRY_STARTED=0, call install_registry,
call start_registry and set REGISTRY_STARTED=1, install a trap on EXIT that
checks REGISTRY_STARTED and calls stop_registry if set, run do_tests, then
explicitly call stop_registry, clear REGISTRY_STARTED and remove the trap;
reference the existing functions install_registry, start_registry, do_tests,
stop_registry to implement this guard so the registry is always cleaned up on
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a084e42-e013-401d-968b-be24ca1c0d0c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
CLAUDE.mdREADME.mdREADME_zh.mdgo.modpkg/server/auth.gopkg/server/options/basic.gopkg/server/server_test.goscripts/simple-tests.sh
💤 Files with no reviewable changes (1)
- go.mod
| } | ||
|
|
||
| func TestDecodeScope_FromQuery(t *testing.T) { | ||
| req := httptest.NewRequest("GET", "http://x/auth/token?service=svc&scope=repository:foo/bar:pull,push", nil) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=go '\bhttptest\.NewRequest\s*\(' pkg/server/server_test.goRepository: alauda/registry-auth
Length of output: 594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check whether golangci-lint enables the noctx rule
rg -n "noctx" -S . || true
# 2) See whether the file already uses NewRequestWithContext anywhere
rg -n --type=go "httptest\.NewRequestWithContext" pkg/server/server_test.go || true
# 3) Confirm context imports in the file
rg -n --type=go "import \\(|context\\." pkg/server/server_test.go || trueRepository: alauda/registry-auth
Length of output: 75
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate golangci config
ls -la .golangci.* 2>/dev/null || true
fd -t f -g ".golangci.y*ml" -g "golangci.y*ml" -g "*lint*" . || true
# Print any explicit noctx configuration
rg -n "noctx" -S . || true
# Show the import block in the target test file (small slice)
sed -n '1,80p' pkg/server/server_test.goRepository: alauda/registry-auth
Length of output: 2727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find golangci-lint config files (including hidden ones)
echo "== config files =="
ls -la .golangci* 2>/dev/null || true
fd -t f --hidden -g '*golangci*.y*ml' --color never . || true
fd -t f --hidden -g '*golangci*.toml' --color never . || true
# 2) Search for noctx rule enablement in config
echo "== noctx mentions =="
rg -n "noctx" --hidden -S . || true
# 3) Check current usage in the target test file
echo "== NewRequestWithContext usage =="
rg -n --type=go "httptest\.NewRequestWithContext" pkg/server/server_test.go || true
echo "== context import/usage =="
rg -n --type=go "(^|\s)\"context\"|context\." pkg/server/server_test.go || trueRepository: alauda/registry-auth
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root listing (look for config dirs/files) =="
ls -la
echo "== golangci-lint mentions =="
rg -n --hidden -S "golangci-lint|golangci" . || true
echo "== noctx mentions =="
rg -n --hidden -S "noctx" . || true
echo "== any NewRequestWithContext usage in repo =="
rg -n --hidden --type=go "httptest\.NewRequestWithContext" . || trueRepository: alauda/registry-auth
Length of output: 1508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search for golangci-lint usage =="
rg -n --hidden -S "golangci-lint" . || true
echo "== search for noctx enabled flags =="
rg -n --hidden -S "(--enable[= ]noctx|--disable[= ]noctx|noctx)" . || true
echo "== inspect call sites in pkg/server/server_test.go =="
nl -ba pkg/server/server_test.go | sed -n '250,410p'Repository: alauda/registry-auth
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile (lint section) =="
rg -n "golangci|lint|noctx" Makefile .github -S || true
echo
sed -n '1,220p' Makefile
echo
echo "== workflows (if any) =="
fd -t f --hidden .github -e yml -e yaml -e json 2>/dev/null || true
rg -n "golangci|noctx" .github -S --hidden || true
echo
echo "== pkg/server/server_test.go around call sites =="
sed -n '240,410p' pkg/server/server_test.goRepository: alauda/registry-auth
Length of output: 6557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search .github for golangci-lint/noctx =="
rg -n --hidden -S "golangci-lint|noctx" .github scripts . || true
echo
echo "== search for 'context' usage in pkg/server/server_test.go =="
rg -n --type=go "(^|\s)\"context\"|context\." pkg/server/server_test.go || true
echo
echo "== show exact import block for pkg/server/server_test.go =="
sed -n '1,40p' pkg/server/server_test.go
echo
echo "== show exact call sites with a bit more surrounding context =="
# show around each line reported earlier
for n in 271 288 299 327 363 371 380; do
echo "---- around $n ----"
sed -n "$((n-3)),$((n+3))p" pkg/server/server_test.go
doneRepository: alauda/registry-auth
Length of output: 3178
🏁 Script executed:
rg -nP --type=go '\bhttptest\.NewRequest\s*\(' pkg/server/server_test.goRepository: alauda/registry-auth
Length of output: 594
Replace httptest.NewRequest with httptest.NewRequestWithContext (add context import).
pkg/server/server_test.go uses httptest.NewRequest(...) at lines 271, 288, 299, 327, 363, 371, and 380. Convert each to httptest.NewRequestWithContext(context.Background(), ...) and add context to the imports (it’s not currently imported).
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 271-271: net/http/httptest.NewRequest must not be called. use net/http/httptest.NewRequestWithContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/server/server_test.go` at line 271, Replace all uses of
httptest.NewRequest in the test to call
httptest.NewRequestWithContext(context.Background(), ...) instead and add an
import for the context package; specifically update the calls creating req (the
instances at lines noted: the occurrences that construct the request for
"/auth/token" etc.) to use NewRequestWithContext(context.Background(), "GET",
"http://x/auth/token?service=svc&scope=repository:foo/bar:pull,push", nil)
pattern, and add "context" to the import block so the tests compile and use a
background context.
ClaimAccess fields had no `json:` tags, so the JWT's `access` array was marshalled with the Go field names — `Type`/`Name`/`Actions`. Distribution v2.8.1 happened to accept that (Go's encoding/json does a case-insensitive fallback), but distribution v3 uses go-jose v3's JSON fork which dropped that fallback. v3 parsed the access entries as empty and returned "insufficient scope" on blob HEAD/GET, even though registry-auth had signed a token covering pull,push. Tag the fields with the lowercase keys mandated by the Docker token spec (type/name/actions) and extend TestSign_EmitsX5cHeaderAndVerifies to assert the raw payload uses those keys, so this can't regress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
distribution/distributionv3.0.0 replaced libtrust with go-jose and keys itstrustedKeysmap by RFC 7638 JWK thumbprint, so tokens signed with the libtrust-stylekidheader that registry-auth used are rejected by v3.x5cprotected header (signing cert in base64 DER). Both v2.8.1 (parseAndVerifyCertChain) and v3.0.0 (verifyCertChain) verifyx5cagainst therootcertbundle, so the same registry-auth-issued token works on both versions — and the archivedgithub.com/docker/libtrustdependency goes away.base/manifest/tags/blob/blob-upload/catalog) and config schema (version: 0.1) match v2 exactly, so no path / scope / registry-config changes were needed.Changes
pkg/server/auth.go— emitx5cinstead of libtrustkid; store*x509.CertificateonAuthProcessor; removelibtrustand unusedcryptoimports.go.mod/go.sum— dropgithub.com/docker/libtrust.pkg/server/server_test.go— new file. The headlineTestSign_EmitsX5cHeaderAndVerifieslocks down the x5c header contents and chain-verification path; additional tests coverAuthenticate(anonymous / plaintext / bcrypt / wrong-password),Authorize(literal / regex / anonymous fallback),DecodeScope,DecodeScopeFromUrl(base / catalog / manifests / blobs / tags / delete / unknown), andIsScopeActionMatch. Coverage onpkg/serverrises from 0% to ~27%.scripts/simple-tests.sh— parametrized viaREGISTRY_VERSIONS(default"v2.8.1 v3.0.0"); each version runs the full skopeo push/pull round-trip in its own pass with storage wiped between runs.README.md/README_zh.md— note v2/v3 dual compatibility and v3's/etc/distribution/config.ymldefault config path.CLAUDE.md— new project guidance; the JWT signing section explains thex5crationale.Test plan
make fmt vet test— all 12 unit tests pass.registrybinary against the sametoken.crtrootcertbundle, hit registry-auth's/v2/proxy with Basic admin and got HTTP 200 (token signed with x5c was accepted by the registry's verify path), without auth got 401, andcurl -H "Authorization: Bearer <token>"directly against the registry also returned 200.bash scripts/simple-tests.shin an environment where/usr/binis writable to exercise the full skopeo push/pull round-trip on both versions.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
Tests
Chores