fix(replication): Normalise CORE_URL comparison in isLocalHarbor - #873
Conversation
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
|
This change may need patch-release backports. Comment with one of these commands to open a cherry-pick PR:
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
7d36fd9 to
57ad94b
Compare
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Warning Outdated: this PR is not the top of stack #907, so the stack's preview image is published on #876. What follows is from an earlier revision of this PR. Preview images for this PR are available in
Verify a preview image: Verify the SBOM attestation: |
57ad94b to
3453274
Compare
Core stamps the local replication source URL from its own CORE_URL and jobservice compares the result against its own. A deployment that sets an explicit default port on only one of the two — http://harbor-core versus http://harbor-core:80, which the documented SBOM workaround did — made the raw string compare fail. The secret authorizer was then skipped and the adapter fell back to basic auth with an empty username, so every private project failed replication with 401 while public ones still worked through anonymous pull. Parse both URLs and compare scheme, host and port with :80 and :443 made explicit for http and https. If either side fails to parse, or carries no scheme or host, the exact string compare still decides. Signed-off-by: Prasanth Baskar <prasanth@8gears.com>
3453274 to
d57e422
Compare
Userinfo, query, or fragment now only match through the exact string compare, so a registry URL carrying embedded credentials can no longer be classified local by the normalized path. Signed-off-by: Prasanth Baskar <prasanth@8gears.com>
|
Addressed the review and tightened the code: normalization now applies only to bare scheme://host[:port][/path] URLs (new bareURL guard) — userinfo, query, or fragment must match through the exact string compare, so a URL with embedded credentials can no longer be classified local via the normalized path. Comments trimmed to the essentials, error/shape guards folded together, three test cases added for the userinfo/query paths. |
|
/backport v2.15 |
|
Opened backport PR for |
Signed-off-by: Prasanth Baskar <prasanth@8gears.com> (cherry picked from commit 51dda34) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## Release Notes ## Commercial Features _No changes this release._ ## What's Changed ### Fixes - **api:** Reject query operands the column cannot take (backport [#925](#925)) ([30d06ea](30d06ea)) - **compose:** Preserve the request port in the nginx Host header in [#926](#926) ([310d6e0](310d6e0)) - **db:** Widen execution.revision to bigint (backport [#924](#924)) ([1d5719e](1d5719e)) - **deps:** Bump go dependencies minor versions (backport [#895](#895)) by @bupd in [#899](#899) ([a4b7e1c](a4b7e1c)) - **deps:** Bump vendored builds: trivy 0.74.0, adapter v0.41.0, pin cleanup (backport [#893](#893)) by @bupd in [#898](#898) ([5bcfece](5bcfece)) - **deps:** Upgrade Go to 1.26.8 (backport [#892](#892)) by @bupd in [#897](#897) ([ff01ea1](ff01ea1)) - **quota:** Enable retry backoff on quota usage updates (backport [#716](#716)) ([75fab53](75fab53)) - **registry:** Answer 400 when the endpoint URL cannot be reached in [#923](#923) ([07d4436](07d4436)) - **replication:** Normalise CORE_URL comparison in isLocalHarbor in [#873](#873) ([49cd3cb](49cd3cb)) - **scan:** Keep the last report when the raw report is empty (backport [#863](#863)) ([1433e30](1433e30)) ### Upstream - add size limit for audit log payload (goharbor/harbor[#23755](https://github.com/container-registry/harbor-next/issues/23755)) by @stonezdj in [goharbor/harbor#23755](goharbor/harbor#23755) ([45e97fd](45e97fd)) - fix(i18n): improve Korean translation of FULL_NAME (goharbor/harbor[#22701](https://github.com/container-registry/harbor-next/issues/22701)) by @InGyu-Moon in [goharbor/harbor#22701](goharbor/harbor#22701) ([9ea0d71](9ea0d71)) - Prevent 500 if no project members have been provided (goharbor/harbor[#23642](https://github.com/container-registry/harbor-next/issues/23642)) by @Yanni8 in [goharbor/harbor#23642](goharbor/harbor#23642) ([c518ae7](c518ae7)) --- ## Container Images Multi-arch images (`linux/amd64`, `linux/arm64`) signed with [cosign](https://github.com/sigstore/cosign). | Image | Reference | |-------|-----------| | `harbor-core` | `8gears.container-registry.com/8gcr/harbor-core:v2.15.9` | | `harbor-jobservice` | `8gears.container-registry.com/8gcr/harbor-jobservice:v2.15.9` | | `harbor-registryctl` | `8gears.container-registry.com/8gcr/harbor-registryctl:v2.15.9` | | `harbor-exporter` | `8gears.container-registry.com/8gcr/harbor-exporter:v2.15.9` | | `harbor-portal` | `8gears.container-registry.com/8gcr/harbor-portal:v2.15.9` | | `harbor-registry` | `8gears.container-registry.com/8gcr/harbor-registry:v2.15.9` | | `trivy-adapter` | `8gears.container-registry.com/8gcr/trivy-adapter:v2.15.9` | **Verify an image signature:** ```sh cosign verify \ --certificate-identity "https://github.com/container-registry/harbor-next/.github/workflows/release-please.yml@refs/heads/release-2.15" \ --certificate-oidc-issuer "https://token.actions.githubusercontent.com" \ 8gears.container-registry.com/8gcr/harbor-core:v2.15.9 ``` Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Vadim Bauer <vadim@8gears.com> Co-authored-by: Prasanth Baskar <prasanth@8gears.com>
What
isLocalHarborcompared the replication registry URL toCORE_URLas a raw string:Core stamps the local replication source URL from its own
CORE_URL; jobservice, which runs the replication job, compares that value against its own. When the two differ only by an explicit default port the compare fails, the secret authorizer is skipped, and the adapter falls back to basic auth with an empty username.The effect is asymmetric and easy to misread: public projects keep replicating because anonymous pull is allowed, while every private project fails with
401 unauthorized to access repository: <repo>, action: pull.How it was found
A 2.15.3 → 2.15.8 tenant upgrade on a real deployment. The chart renders
http://harbor-corefor both components, but the SBOM workaround for images below v2.15.5 setscore.extraEnv CORE_URL=http://harbor-core:80— so only core's value carried the port.web-proxy(public) replicated Succeed 4/4 whilebase-os(private) Failed 1/1, both to a remote registry and to itself. Setting jobservice'sCORE_URLto the identical string made the private policy Succeed 1/1 immediately.Reproduced on 2.15.8
Confirmed independently on a Harbor Next 2.15.8 stood up on k3d from this chart, replicating to a plain
registry:2destination — the failure is on the replication source, so no second Harbor is needed. All cases run back-to-back on one cluster.:80/ default:80/:80The asymmetry is the tell: only the private project fails, because the fallback to anonymous basic auth still satisfies a public project's anonymous pull.
jobservice, in the failing case:
The
adapter.go:39warning is a clean oracle for which branch ran:Detected LOCAL Harbor instance (URL=http://harbor-core matches CORE_URL)present, both projects succeed:80— warning absent, private project 401s:80— warning returns asURL=http://harbor-core:80 matches CORE_URL, zero errorsChain confirmed live: core stamps the local registry URL from its own
CORE_URL(getLocalRegistry()insrc/pkg/reg/manager.go:171→config.InternalCoreURL()); jobservice compares that against its ownos.Getenv("CORE_URL"). On mismatchadapter.Newskips the secret authorizer and falls back tobasic.NewAuthorizer(AccessKey, ...), whereAccessKeyis empty forCredentialTypeSecret— anonymous. Public projects still replicate, private ones 401.Fix
Parse both URLs and compare scheme, host and port, with
:80made explicit forhttpand:443forhttps. Trailing slashes are ignored. If either side fails to parse, or has no scheme or host, the exact string compare still decides — a bareharbor-coreparses intoPathwith empty scheme and host, which would otherwise compare equal to an unsetCORE_URL.This is the same equivalence the v2auth middleware already applies when matching a request host against the configured URL (
match()insrc/server/middleware/v2auth/auth.go:141), reached from the other direction: that one strips a default port, this one adds it.isLocalHarborwas the last raw==comparison of this kind left in the tree. Whether the two should share one helper is worth doing, but not inside a bugfix that would then touch the auth middleware — filed as #880 rather than bundled here.Test
TestIsLocalHarbor, 18 cases. The two that encode the reproduced failure:plus both https/
:443directions, trailing slash, uppercase host, non-default ports, different host, different scheme, different path, unparseable and schemeless input, and an emptyCORE_URL.Not changed
The chart already renders one consistent
CORE_URLfor both components:The divergence only appears when an operator overrides core's value through
core.extraEnv. That workaround is obsolete at HEAD —accessoryRefinsrc/pkg/scan/util.gonow handles a portless single-label host, which was the reason for pinning:80in the first place. Confirmed on a 2.15.8 cluster: with the default portless config and no override anywhere, SBOM generation on a private project succeeds and pushes itssbom.harboraccessory (28447 B).Remediation for a tenant already carrying the override — order matters
The override is still load-bearing below v2.15.5. Do not delete it as a standalone values cleanup.
It works around SBOM push sending robot credentials to Docker Hub when
CORE_URLis portless, fixed by #465 ("Keep SBOM accessory push on the local registry whenCORE_URLhas no port") in v2.15.5. Every tenant known to carry the override is unpinned and therefore at the globalharbor_version: v2.15.3— below that fix. The 2.15.8 evidence above says the workaround is dead at 2.15.8; it says nothing about where those tenants are today. Delete it from a v2.15.3 tenant and SBOM push regresses: one silent breakage traded for another.On a build that predates this fix, removal and the move to ≥ v2.15.5 must be one atomic change — delete early and SBOM push regresses on v2.15.3; delete late and private replication stays broken in between.
The two halves of that trap rest on different kinds of evidence, which is worth keeping straight:
:80/ default row in the matrix above was run on 2.15.8, i.e. already past fix(scan): Keep SBOM accessory push on the local registry when CORE_URL has no port #465 and without this fix, and private replication failed 401 while public succeeded.CORE_URL. What is verifiable is the mechanism: atv2.15.3,GenAccessoryArtbuilt the reference asname.ParseReference(fmt.Sprintf("%s/%s@%s", sq.Registry.URL, ...)).ParseReferenceonly treats the first path segment as a registry if it contains a.or a:, so a portless single-label host likeharbor-coreparses as a Docker Hub namespace and the push leaves the cluster. v2.15.5 replaced that withname.NewRegistry(registryURL)+reg.Repo(repository)and added theportless single-label host stays the registrytest case — absent atv2.15.3andv2.15.4, present atv2.15.5andv2.15.8.A build carrying this fix removes the need to coordinate at all. The one-sided mismatch becomes inert, so the upgrade can be taken on its own with the override left in place, and the values cleanup scheduled separately as ordinary hygiene.
That regime does not yet exist for any affected tenant, and merging this PR alone will not create it. This PR targets
main. Every tenant carrying the override is on the 2.15 line and their realistic target is v2.15.8 — also a 2.15 build. So the relaxation above is conditional on arelease-2.15backport, not on this merging.The precedent is #465 itself, the fix this whole argument leans on: it needed both a
mainPR (#465, merged 2026-07-21) and arelease-2.15backport (#581, merged 2026-08-07) to reach v2.15.8. This change currently has only themainhalf, and no backport PR for it exists.So: a
release-2.15backport is required for this fix to reach any currently-affected tenant. Until one lands and those tenants are moved onto a build carrying it, the pre-fix atomic constraint governs their v2.15.8 upgrade unchanged. Not opened here because the repo's pattern is to backport after themainPR merges — see #867, #868, #870, all titled(backport #N).End state is still deleted from both components, not mirrored onto jobservice. Mirroring works — the
:80/:80row above succeeds — but it pins an obsolete workaround into the tenant files and re-breaks the moment someone edits one side.Tenant list and the pre-fix constraint:
herdr-cc/tasks/featgate-status.md§5.