Repository navigation
fix: implement cosign signature inheritance for OCI index children - #27
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request implements inheritance of Cosign signatures from parent artifacts to child artifacts. The controller now queries parent references when direct Cosign signatures are absent, collects inherited signatures from parents, and stores them in a new Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Handler
participant Controller as Controller
participant ArtifactMgr as ArtifactManager
participant AccessoryMgr as AccessoryManager
Handler->>Controller: Get Artifact with<br/>WithAccessory:true
Controller->>ArtifactMgr: Load Artifact
ArtifactMgr-->>Controller: Artifact (no direct Cosign)
Controller->>+Controller: populateAccessories()
Controller->>AccessoryMgr: List Direct Accessories
AccessoryMgr-->>Controller: [empty or non-Cosign]
alt No Direct Cosign Found
Controller->>ArtifactMgr: ListReferences(artifactID)
ArtifactMgr-->>Controller: Parent References
loop For Each Parent Reference
Controller->>AccessoryMgr: List Parent Accessories
AccessoryMgr-->>Controller: Parent Accessories
alt Cosign Signature Found
Controller->>Controller: Append to InheritedAccessories
Note over Controller: Short-circuit
end
end
end
Controller-->>-Controller: Return with InheritedAccessories
Controller-->>Handler: Artifact + InheritedAccessories
Handler->>Handler: Include InheritedAccessories<br/>in response when applicable
Handler-->>Handler: Return ListAccessories response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/server/v2.0/handler/artifact.go">
<violation number="1" location="src/server/v2.0/handler/artifact.go:396">
P2: This block re-queries the same artifact (`art.ID`) instead of a parent artifact, so inherited cosign signatures are never checked even when a parent has them. The logic needs to resolve and query the parent artifact IDs before scanning accessories.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/server/v2.0/handler/artifact.go (1)
399-399: Use the typed constant instead of the hardcoded string"signature.cosign".
controller.gousesaccessorymodel.TypeCosignSignaturefromsrc/pkg/accessory/model/base/base.go. Using the raw string here is inconsistent and will silently diverge if the constant's value is ever updated.+ "github.com/goharbor/harbor/src/pkg/accessory/model/base" ... - if acc.GetData().Type == "signature.cosign" { + if acc.GetData().Type == base.TypeCosignSignature {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/v2.0/handler/artifact.go` at line 399, Replace the hardcoded string "signature.cosign" with the typed constant accessorymodel.TypeCosignSignature where acc.GetData().Type is compared (in the artifact handling code that currently uses acc.GetData().Type == "signature.cosign"); update imports to include the accessorymodel package if not already imported so the code references accessorymodel.TypeCosignSignature instead of the raw string to keep consistency with controller.go and avoid divergence if the constant changes.src/controller/artifact/controller.go (1)
803-823: ExtraListReferencesquery for every non-Cosign-signed artifact may significantly increase DB load.
populateAccessoriesis called for every artifact returned byGet,List,GetByReference, andListWithLatest. The new block issues an additionalListReferencesquery for every artifact that lacks a Cosign signature — in most registries, the overwhelming majority. On a large registry (thousands of artifacts, high-frequency list operations), this will multiply DB query volume proportionally.Consider gating the parent-lookup on an additional condition that reliably identifies OCI-index children (e.g., artifact
MediaTypematching an OCI/Docker index child manifest type) before paying theListReferencescost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controller/artifact/controller.go` around lines 803 - 823, The extra ListReferences call inside the populateAccessories block (when !hasCosign) causes a DB query for every non-Cosign artifact; guard that call by first checking the artifact’s media type to ensure it’s an OCI index/manifest-list child before invoking c.artMgr.ListReferences. Update the code in the same function where hasCosign is checked to inspect the artifact media type (e.g., art.GetMediaType() or the field used for manifest media type) and only run c.artMgr.ListReferences(...), accessoryMgr.List(...), and the parent signature merge when the media type matches known index/manifest-list types (application/vnd.oci.image.index.v1+json or application/vnd.docker.distribution.manifest.list.v2+json); otherwise skip the parent lookup and return. Ensure you reference the existing symbols hasCosign, c.artMgr.ListReferences, c.accessoryMgr.List, and art.Accessories when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controller/artifact/controller.go`:
- Around line 815-819: The current populateAccessories implementation appends
parent accessory objects (pAcc) into art.Accessories which corrupts downstream
logic (Walk, copyDeeply, deleteDeeply) because pAcc.GetData().ArtifactID/Digest
refer to the parent; change populateAccessories to stop mutating art.Accessories
with inherited entries and instead append these parent-derived accessories into
a new display-only field (e.g., add InheritedAccessories
[]accessorymodel.Accessory to the Artifact model), leave art.Accessories
containing only direct accessories, and update any assembler/handler display
code (assembleArtifact and API response builder) to present the combined view by
merging art.Accessories and art.InheritedAccessories for UI purposes while
ensuring Walk, copyDeeply, and deleteDeeply continue to operate only on
art.Accessories.
- Around line 807-809: In populateAccessories, the call to ListReferences
currently swallows errors by returning when err != nil || len(parents) == 0;
change this so any non-nil err is logged using the file's existing logging
pattern (same logger used elsewhere in this file, e.g., logger.Errorf or
controller-level logger) and only short-circuit silently when err == nil and
len(parents) == 0; keep the existing handling of an empty parents slice but
ensure persistence failures from ListReferences are logged with the error and
context (function name ListReferences, the artifact ID/variable used) before
returning.
In `@src/server/v2.0/handler/artifact.go`:
- Line 391: The raw substring check using strings.Contains on params.Q is
brittle; instead, after the query is parsed by BuildQuery use the parsed keyword
map produced (e.g., the query/params BuildQuery result or the
parsedKeywords/keywords map used elsewhere in artifact.go) to detect a signature
type and check that the signature keyword equals "cosign" (for example
keywords["signature"] == "cosign") before applying the accessory fallback when
len(accs) == 0; update the conditional that references params.Q to inspect that
parsed keyword map (or use the query object's helper like HasKeyword/GetKeyword)
so the detection is robust to serialization/format changes.
- Around line 391-407: Remove the redundant fetch stored in the variable
"parents" (the call to a.artCtl.GetByReference that refetches the same artifact)
since it does not retrieve OCI-index parents and the real parent-inheritance
logic runs in assembleArtifact/populateAccessories via a.artCtl.Get(ctx, art.ID,
&artifact.Option{WithAccessory: true}); instead, delete the parents variable and
its nil-check, call a.artCtl.Get for accessories as done in the loop, and stop
discarding errors with "_" — capture returned errors from
a.artCtl.GetByReference and a.artCtl.Get (or just the latter if the former is
removed) and log them (use the existing logger available in the handler) so
failures are visible.
---
Nitpick comments:
In `@src/controller/artifact/controller.go`:
- Around line 803-823: The extra ListReferences call inside the
populateAccessories block (when !hasCosign) causes a DB query for every
non-Cosign artifact; guard that call by first checking the artifact’s media type
to ensure it’s an OCI index/manifest-list child before invoking
c.artMgr.ListReferences. Update the code in the same function where hasCosign is
checked to inspect the artifact media type (e.g., art.GetMediaType() or the
field used for manifest media type) and only run c.artMgr.ListReferences(...),
accessoryMgr.List(...), and the parent signature merge when the media type
matches known index/manifest-list types (application/vnd.oci.image.index.v1+json
or application/vnd.docker.distribution.manifest.list.v2+json); otherwise skip
the parent lookup and return. Ensure you reference the existing symbols
hasCosign, c.artMgr.ListReferences, c.accessoryMgr.List, and art.Accessories
when applying the change.
In `@src/server/v2.0/handler/artifact.go`:
- Line 399: Replace the hardcoded string "signature.cosign" with the typed
constant accessorymodel.TypeCosignSignature where acc.GetData().Type is compared
(in the artifact handling code that currently uses acc.GetData().Type ==
"signature.cosign"); update imports to include the accessorymodel package if not
already imported so the code references accessorymodel.TypeCosignSignature
instead of the raw string to keep consistency with controller.go and avoid
divergence if the constant changes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/v2.0/handler/artifact.go (1)
400-407:artWithAccs.AccessoriesinallAccsis effectively redundant here.Control reaches this point only when
len(accs) == 0— meaningaccMgr.List(ctx, query)(scoped to the sameSubjectArtifactID) already returned no accessories.artWithAccs.Accessoriesis populated bypopulateAccessoriesvia the same underlyingaccMgr.Listcall, so it will also be empty. The only field that can produce a result here isartWithAccs.InheritedAccessories. Iterating both obscures the intent and adds noise.♻️ Proposed simplification
- // Combine direct and inherited accessories - allAccs := append(artWithAccs.Accessories, artWithAccs.InheritedAccessories...) - for _, acc := range allAccs { + for _, acc := range artWithAccs.InheritedAccessories { if acc.GetData().Type == accessorymodel.TypeCosignSignature { accs = append(accs, acc) total = 1 break } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/v2.0/handler/artifact.go` around lines 400 - 407, The loop builds allAccs by appending artWithAccs.Accessories and artWithAccs.InheritedAccessories even though artWithAccs.Accessories is always empty here (accMgr.List returned no results); simplify by iterating only artWithAccs.InheritedAccessories, checking acc.GetData().Type == accessorymodel.TypeCosignSignature, appending the match to accs and setting total = 1, and remove the redundant allAccs variable and reference to artWithAccs.Accessories; this keeps the logic clear and uses the existing symbols accs, artWithAccs.InheritedAccessories, accessorymodel.TypeCosignSignature, and the context where accMgr.List was called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/v2.0/handler/artifact.go`:
- Line 394: The current fragile substring check using
strings.Contains(lib.StringValue(params.Q), "signature.cosign") should be
replaced by examining the parsed keyword map that BuildQuery returns: call or
reuse BuildQuery(...) to obtain the query keywords (or the existing parsed map
variable produced by BuildQuery) and test for the presence of the specific key
"signature.cosign" (or its exact keyword name) rather than doing a raw substring
match against params.Q; ensure you handle nil/empty parsed maps safely and keep
the check localized where the current conditional exists in artifact.go so
behavior remains identical but robust to serialization changes.
---
Nitpick comments:
In `@src/server/v2.0/handler/artifact.go`:
- Around line 400-407: The loop builds allAccs by appending
artWithAccs.Accessories and artWithAccs.InheritedAccessories even though
artWithAccs.Accessories is always empty here (accMgr.List returned no results);
simplify by iterating only artWithAccs.InheritedAccessories, checking
acc.GetData().Type == accessorymodel.TypeCosignSignature, appending the match to
accs and setting total = 1, and remove the redundant allAccs variable and
reference to artWithAccs.Accessories; this keeps the logic clear and uses the
existing symbols accs, artWithAccs.InheritedAccessories,
accessorymodel.TypeCosignSignature, and the context where accMgr.List was
called.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/server/v2.0/handler/artifact.go">
<violation number="1" location="src/server/v2.0/handler/artifact.go:375">
P2: Always fetching accessories via `WithAccessory: true` introduces a redundant accessory list query on every `ListAccessories` request (populateAccessories already lists all accessories), even though the handler immediately performs its own paginated `Count`/`List`. Consider only loading accessories when the inheritance fallback is actually needed to avoid this extra DB hit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/v2.0/handler/artifact.go`:
- Around line 391-401: Change the guard to check total == 0 instead of len(accs)
== 0 to avoid triggering inheritance when a paginated page is empty but total
indicates accessories exist; specifically update the conditional that currently
uses accs to use total, iterate art.InheritedAccessories to find
accessorymodel.TypeCosignSignature, append that accessory to accs, set total = 1
(or total = int64(len(accs)) but since you break after the first match total
will always be 1), and update the comment to accurately state that we set total
to 1 because we break after adding the single inherited accessory rather than
implying a dynamic resize.
- Around line 374-375: Change the pagination check in ListAccessories so it
tests the global count (total == 0) instead of the current page length
(len(accs) == 0); in the artifact lookup avoid always passing WithAccessory:
true to a.artCtl.GetByReference (which triggers the expensive parent-index walk)
and instead fetch accessories eagerly only when total == 0 (i.e., when no direct
accessories exist) by deferring the inherit-population path to the fallback
branch (use GetByReference or the existing populate/inherit routine there);
update the misleading comment about "Dynamically update total..." to accurately
describe the early break behavior, and replace the brittle
strings.Contains(lib.StringValue(params.Q), "signature.cosign") check with a
proper query parse/inspection helper (or a clear TODO to implement a robust
query parser) so accessory-type detection is reliable.
---
Duplicate comments:
In `@src/server/v2.0/handler/artifact.go`:
- Line 393: The condition using strings.Contains on params.Q is brittle;
instead, after calling BuildQuery (the parsed query step that produces the
keyword map), inspect the parsed keyword/field map produced by BuildQuery (not
the raw params.Q string) to detect a cosign signature type (e.g., check for a
"signature.cosign" key or a signature/type field in the parsed keywords) and
only run the accs==0 branch when that parsed key indicates cosign; update the
logic around accs and the cosign detection to use the BuildQuery output rather
than lib.StringValue(params.Q).
Aloui-Ikram
force-pushed
the
fix-issue-17-signature-inheritance
branch
from
February 21, 2026 14:58
2c1192d to
eb03f8f
Compare
Aloui-Ikram
changed the title
Vad1mo
pushed a commit
that referenced
this pull request
Mar 10, 2026
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Preview images for this PR are available in
Verify a preview image: Verify SBOM attestation: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dockerfile/dev.trivy-adapter.dockerfile`:
- Around line 11-17: The builder stage currently runs CGO_ENABLED=0 go build
without target platform args; add ARG TARGETARCH (and ARG TARGETVARIANT if
needed) at the top of the builder stage and export/set cross-compilation
environment variables before the build. Specifically, update the Dockerfile
builder stage that contains CGO_ENABLED=0 and the go build invocation so it sets
GOOS=linux and GOARCH based on ARG TARGETARCH (and set GOARM when TARGETARCH is
"arm" using TARGETVARIANT) so the CGO_ENABLED=0 go build -o /scanner-trivy
cmd/scanner-trivy/main.go runs as a multi-arch cross-compile.
🪄 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: 1dbcda7e-d298-4fa4-88b7-610cd69475b7
📒 Files selected for processing (3)
devenv/docker-compose.ymldockerfile/dev.trivy-adapter.dockerfilesrc/controller/artifact/controller_test.go
Comment on lines
+11
to
+17
| FROM golang:${GO_VERSION}-alpine AS builder | ||
| ARG HARBOR_SCANNER_TRIVY_VERSION | ||
| RUN apk add --no-cache git | ||
| RUN git clone --branch ${HARBOR_SCANNER_TRIVY_VERSION} --depth 1 \ | ||
| https://github.com/goharbor/harbor-scanner-trivy.git /src | ||
| WORKDIR /src | ||
| RUN CGO_ENABLED=0 go build -o /scanner-trivy cmd/scanner-trivy/main.go |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check multi-arch patterns used by Dockerfiles:"
fd -e dockerfile . dockerfile | xargs rg -n 'TARGETARCH|TARGETOS|--platform=\$BUILDPLATFORM|linux-\$\{?TARGETARCH'
echo
echo "Inspect dev trivy adapter build stage:"
rg -n 'FROM golang|go build|TARGETARCH|TARGETOS|BUILDPLATFORM' dockerfile/dev.trivy-adapter.dockerfileRepository: container-registry/harbor-next
Length of output: 1248
Add explicit multi-arch build args in the Go build stage.
The builder stage at lines 11-17 compiles without explicit target platform configuration, violating the repository's multi-architecture build requirement. Other Dockerfiles in the repo consistently use ARG TARGETARCH and cross-compilation flags; this dev Dockerfile must follow the same pattern.
Proposed fix
- FROM golang:${GO_VERSION}-alpine AS builder
+ FROM --platform=$BUILDPLATFORM golang:${GO_VERSION}-alpine AS builder
+ ARG TARGETOS
+ ARG TARGETARCH
ARG HARBOR_SCANNER_TRIVY_VERSION
RUN apk add --no-cache git
RUN git clone --branch ${HARBOR_SCANNER_TRIVY_VERSION} --depth 1 \
https://github.com/goharbor/harbor-scanner-trivy.git /src
WORKDIR /src
- RUN CGO_ENABLED=0 go build -o /scanner-trivy cmd/scanner-trivy/main.go
+ RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -o /scanner-trivy cmd/scanner-trivy/main.go📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FROM golang:${GO_VERSION}-alpine AS builder | |
| ARG HARBOR_SCANNER_TRIVY_VERSION | |
| RUN apk add --no-cache git | |
| RUN git clone --branch ${HARBOR_SCANNER_TRIVY_VERSION} --depth 1 \ | |
| https://github.com/goharbor/harbor-scanner-trivy.git /src | |
| WORKDIR /src | |
| RUN CGO_ENABLED=0 go build -o /scanner-trivy cmd/scanner-trivy/main.go | |
| FROM --platform=$BUILDPLATFORM golang:${GO_VERSION}-alpine AS builder | |
| ARG TARGETOS | |
| ARG TARGETARCH | |
| ARG HARBOR_SCANNER_TRIVY_VERSION | |
| RUN apk add --no-cache git | |
| RUN git clone --branch ${HARBOR_SCANNER_TRIVY_VERSION} --depth 1 \ | |
| https://github.com/goharbor/harbor-scanner-trivy.git /src | |
| WORKDIR /src | |
| RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -o /scanner-trivy cmd/scanner-trivy/main.go |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dockerfile/dev.trivy-adapter.dockerfile` around lines 11 - 17, The builder
stage currently runs CGO_ENABLED=0 go build without target platform args; add
ARG TARGETARCH (and ARG TARGETVARIANT if needed) at the top of the builder stage
and export/set cross-compilation environment variables before the build.
Specifically, update the Dockerfile builder stage that contains CGO_ENABLED=0
and the go build invocation so it sets GOOS=linux and GOARCH based on ARG
TARGETARCH (and set GOARM when TARGETARCH is "arm" using TARGETVARIANT) so the
CGO_ENABLED=0 go build -o /scanner-trivy cmd/scanner-trivy/main.go runs as a
multi-arch cross-compile.
Signed-off-by: Aloui-Ikram <ikramaloui145@example.com> Signed-off-by: bupd <bupdprasanth@gmail.com>
- Added `InheritedAccessories` to the Artifact model to prevent mixing inherited parent signatures with direct artifact accessories. - Updated `populateAccessories` to store parent signatures in the new field to maintain data integrity. - Fixed `ListAccessories` handler to properly parse multi-type UI queries using `strings.Contains` and serve inherited signatures when requested. Signed-off-by: Aloui-Ikram <ikramaloui145@example.com> Signed-off-by: bupd <bupdprasanth@gmail.com>
The new populateAccessories logic calls artMgr.ListReferences whenever an artifact has no direct cosign signature. Several existing tests (TestList, TestListWithLatest, TestDeleteDeeply, TestWalk) mock accMgr.List to return empty accessories but did not set up a ListReferences expectation, causing the mock to panic. Add the missing mock expectations returning an empty reference slice so the new code path exits cleanly. Signed-off-by: bupd <bupdprasanth@gmail.com>
bupd
force-pushed
the
fix-issue-17-signature-inheritance
branch
from
March 17, 2026 23:17
cdcb732 to
01cb866
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/server/v2.0/handler/artifact.go (1)
391-410:⚠️ Potential issue | 🟠 MajorPagination bug: use
total == 0instead oflen(accs) == 0.The condition
len(accs) == 0checks the current page result, not whether any direct accessories exist globally. This causes incorrect behavior:
- User requests page 3 of accessories
- Total direct accessories: 20 (spread across pages 1-2)
- Page 3 is empty, so
len(accs) == 0is true- Inheritance fallback runs incorrectly
totalgets overwritten to1, breaking the responseThe fix should use the global count instead.
Proposed fix
- // If no direct signature found, check for Cosign signatures inherited from a parent OCI index. - if len(accs) == 0 { + // If no direct accessories exist globally, check for inherited Cosign signatures. + if total == 0 { // Use strings.Contains because the UI sends a list of types (e.g. type={signature.cosign ...}) if strings.Contains(lib.StringValue(params.Q), "signature.cosign") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/v2.0/handler/artifact.go` around lines 391 - 410, The pagination bug is caused by checking the current page result (len(accs) == 0) instead of the global accessory count; change the condition to use the global total (if total == 0) before attempting to load inherited Cosign signatures via a.artCtl.Get(ctx, art.ID, &artifact.Option{WithAccessory: true}); keep the strings.Contains(lib.StringValue(params.Q), "signature.cosign") check, search artWithAccs.Accessories and artWithAccs.InheritedAccessories for accessorymodel.TypeCosignSignature, append any found accessory to accs, and only set total = 1 when you truly discovered an inherited signature (so you don't overwrite a correct nonzero total for paged results).
🧹 Nitpick comments (2)
src/controller/artifact/controller.go (1)
814-825: Inconsistent error handling:accessoryMgr.Listerrors are silently swallowed.On line 808, errors from
ListReferencesare logged before returning, but errors fromc.accessoryMgr.Liston line 816 are silently swallowed withcontinue. For consistency and debuggability, consider logging these errors as well.Proposed fix
for _, p := range parents { parentAccs, err := c.accessoryMgr.List(ctx, q.New(q.KeyWords{"SubjectArtifactID": p.ParentID})) if err != nil { + log.Errorf("failed to list accessories of parent artifact %d: %v", p.ParentID, err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controller/artifact/controller.go` around lines 814 - 825, The loop silently swallows errors from c.accessoryMgr.List; change it to log the error (using the same logger used when handling ListReferences errors) including context like the parent.ParentID and the query, then continue; update the block around accessoryMgr.List in the controller.go method so errors are logged before skipping to the next parent (referencing accessoryMgr.List and the parents slice to locate the code).src/controller/artifact/controller_test.go (1)
323-323: Mock additions forListReferencesare appropriate.These mocks are necessary to support the new
ListReferencescall inpopulateAccessories. Returning empty slices correctly exercises the "no parent references" path.Consider adding a test case that verifies the inheritance path works correctly—e.g., when a parent artifact has a CosignSignature accessory, the child artifact's
InheritedAccessoriesshould be populated.Also applies to: 362-362, 529-529, 706-706
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controller/artifact/controller_test.go` at line 323, Add a test that verifies accessory inheritance by mocking artifact manager calls: have c.artMgr.On("ListReferences", mock.Anything, mock.Anything).Return([]*artifact.Reference{parentRef}, nil) so populateAccessories finds a parent, and mock c.artMgr.On("ListAccessories", mock.Anything, parentRef).Return([]*artifact.Accessory{cosignSigAccessory}, nil) (or the actual accessory type used in tests) so the parent exposes a CosignSignature accessory; call populateAccessories (or the test helper that triggers it) for the child artifact and assert the child's InheritedAccessories contains the CosignSignature entry. Ensure you reuse the existing test scaffolding and mocks (ListReferences, ListAccessories, parentRef, cosignSigAccessory, populateAccessories, and InheritedAccessories) and add cleanup expectations for the mocked calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/v2.0/handler/artifact.go`:
- Around line 391-410: The pagination bug is caused by checking the current page
result (len(accs) == 0) instead of the global accessory count; change the
condition to use the global total (if total == 0) before attempting to load
inherited Cosign signatures via a.artCtl.Get(ctx, art.ID,
&artifact.Option{WithAccessory: true}); keep the
strings.Contains(lib.StringValue(params.Q), "signature.cosign") check, search
artWithAccs.Accessories and artWithAccs.InheritedAccessories for
accessorymodel.TypeCosignSignature, append any found accessory to accs, and only
set total = 1 when you truly discovered an inherited signature (so you don't
overwrite a correct nonzero total for paged results).
---
Nitpick comments:
In `@src/controller/artifact/controller_test.go`:
- Line 323: Add a test that verifies accessory inheritance by mocking artifact
manager calls: have c.artMgr.On("ListReferences", mock.Anything,
mock.Anything).Return([]*artifact.Reference{parentRef}, nil) so
populateAccessories finds a parent, and mock c.artMgr.On("ListAccessories",
mock.Anything, parentRef).Return([]*artifact.Accessory{cosignSigAccessory}, nil)
(or the actual accessory type used in tests) so the parent exposes a
CosignSignature accessory; call populateAccessories (or the test helper that
triggers it) for the child artifact and assert the child's InheritedAccessories
contains the CosignSignature entry. Ensure you reuse the existing test
scaffolding and mocks (ListReferences, ListAccessories, parentRef,
cosignSigAccessory, populateAccessories, and InheritedAccessories) and add
cleanup expectations for the mocked calls.
In `@src/controller/artifact/controller.go`:
- Around line 814-825: The loop silently swallows errors from
c.accessoryMgr.List; change it to log the error (using the same logger used when
handling ListReferences errors) including context like the parent.ParentID and
the query, then continue; update the block around accessoryMgr.List in the
controller.go method so errors are logged before skipping to the next parent
(referencing accessoryMgr.List and the parents slice to locate the code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 875298da-1740-42e9-ad30-515069b4e8cc
📒 Files selected for processing (4)
src/controller/artifact/controller.gosrc/controller/artifact/controller_test.gosrc/controller/artifact/model.gosrc/server/v2.0/handler/artifact.go
bupd
requested changes
Mar 17, 2026
bupd
left a comment
bupd
left a comment
Member
There was a problem hiding this comment.
@Aloui-Ikram this has one issue - the way you handle the inherited is wrong - it should not say signed in standalone artifacts.
simply, inheritance of sign should only apply when the artifact is exclusively reachable as a child of the signed artifact,
it should not apply when the artifact is independently pushed as standalone
bupd
force-pushed
the
fix-issue-17-signature-inheritance
branch
from
March 18, 2026 00:19
f1d55b5 to
01cb866
Compare
Member
|
i am merging this one - after deep diving in - there is a way these things are handled. and the standalone artifact showing is basically the verbose way of harbor showing the artifacts - it has to do with the way i created the multi-arch image - since i pushed the childs first and the index afterwards - this happnd - anyways its good now that we know how this would behave in different scenarios. so yeah. 
|
Merged
bupd
pushed a commit
that referenced
this pull request
May 13, 2026
This PR prepares the next release based on conventional commits. --- ## [2.15.0](v2.14.0...v2.15.0) (2026-05-12) ### Features * Add CI/CD Pipelines, Release Automation, and Developer Tooling ([#45](#45)) ([aed9379](aed9379)) * Add Conditional Immutability Rules Compatible with Retention Policy ([#33](#33)) ([ab409ac](ab409ac)) * Add configurable landing page for unauthenticated users ([#152](#152)) ([67fb2d6](67fb2d6)) * Add LDAP Admin Filter ([#34](#34)) ([7a919e9](7a919e9)) * Add Subscription Menu With Chargebee Integration ([#40](#40)) ([850da22](850da22)) * Audit Log Max Page Size up to 10000 ([#37](#37)) ([21862b1](21862b1)) * **compose:** production compose, images, devenv overhaul with DHI base images, non-root containers, production compose ([bcc27db](bcc27db)) * **db:** Upgrade to pgx/v5 and pgxpool for connection pooling ([#118](#118)) ([612a3c5](612a3c5)) * **docs:** Add documentation generation tasks for helm-docs and SVGBob diagrams ([79abc5e](79abc5e)) * **portal:** Add Copy Pull Command Button on Tags ([#61](#61)) ([73ec73b](73ec73b)) * **portal:** Add copy pull command to tag links ([e3fb9d3](e3fb9d3)) * **portal:** add download button to export audit logs ([b536ad1](b536ad1)) * **portal:** Add Repository-Level Pull Command to Artifact List Tab ([#32](#32)) ([c3b9cf9](c3b9cf9)) * **portal:** always build OpenAPI UI in background ([7c27b9f](7c27b9f)) * Randomise Seconds When Scheduling Jobs ([#35](#35)) ([e88647f](e88647f)) * **ui:** Redirect Pull URLs to Project Repository Page ([#63](#63)) ([3cfd879](3cfd879)) * unauthenticated access to ui ([704dcd4](704dcd4)) ### Bug Fixes * [upstream] Add missing AWS ECR regions ([0d50d99](0d50d99)) * [upstream] Add User-Agent header to all registry requests ([e7d0be5](e7d0be5)) * [upstream] Append Custom CAs to System CA Pool ([d8d52f9](d8d52f9)) * [upstream] Bump Trivy to v0.69.2 Following Supply Chain Incident ([c17e06c](c17e06c)) * [upstream] Check Error First Before Other Checks ([2abc8da](2abc8da)) * [upstream] Format version span indentation in about dialog ([74ce587](74ce587)) * [upstream] Proxy Cache Serve Local on Remote Not Found ([4d1c757](4d1c757)) * [upstream] Remove Payload From Config Audit Log ([b4a0a2c](b4a0a2c)) * [upstream] Swagger Replication Rule Invalid JSON ([6d0f605](6d0f605)) * [upstream] Update parent and child artifact pull times ([0fc0ec7](0fc0ec7)) * [upstream] Update Verify Remote Cert Tooltip for Registry Endpoints ([8f997c2](8f997c2)) * [upstream] Wrong Operation Response Name for UpdateRepository ([e4e3a48](e4e3a48)) * add -trimpath to go build flags to prevent local path leaks ([31351a2](31351a2)) * Add Dockerfile Healthchecks ([#204](#204)) ([1bf2770](1bf2770)) * Address Devenv Review Feedback ([#196](#196)) ([5903d32](5903d32)) * Address PR [#119](#119) review feedback ([d683f71](d683f71)) * Allow Negative Serial Numbers in x509 Certificates ([#36](#36)) ([b3c99cb](b3c99cb)) * Avoid holding pull time lock during async DB flushes ([d19d080](d19d080)) * Classify BuildKit attestations as accessories ([#85](#85)) ([01d2fc3](01d2fc3)) * Clean up unused portal UI components and configuration ([966baee](966baee)) * **db:** Remove redundant sql.DB Close in dbpool.Pool.Close() ([#150](#150)) ([6b0e929](6b0e929)) * **deps:** Bump go-jose/go-jose/v4 to v4.1.4 for CVE-2026-34986 ([dce1151](dce1151)) * **deps:** Bump go.opentelemetry.io/otel/sdk to v1.43.0 for PATH hijack CVE ([45b8d5f](45b8d5f)) * **devenv:** registryctl crashes on startup , missing config file argument ([#28](#28)) ([7e10546](7e10546)) * **dev:** Fix Dev Environment Docker Compose and Trivy Adapter Setup ([#83](#83)) ([6bbe1e9](6bbe1e9)) * Expand Global Search Input ([6c15cf3](6c15cf3)) * **exporter:** Bake Harbor version into exporter image at build time ([22b73ff](22b73ff)) * **exporter:** Remove redundant database URL field from exporter config ([#148](#148)) ([00b151e](00b151e)) * Handle proxy-cache races in UpdatePullTime and correct artForPullTime construction ([1d9cc7e](1d9cc7e)) * Honor unauthenticated project redirects ([#187](#187)) ([362039c](362039c)) * **image:** Use pre-built binaries in registry and trivy-adapter dockerfiles, fix --load/--push output ([c7fd1bb](c7fd1bb)) * implement cosign signature inheritance for OCI index children ([#27](#27)) ([c6f9e37](c6f9e37)) * Improve CA Pool Test Assertion and Use Typed NotFoundError in Purge API ([#55](#55)) ([f499a3c](f499a3c)) * **portal:** [upstream] UI Statistics Display Are Not Aligned ([78dc662](78dc662)) * **portal:** Fix i18n Key Typos and Add Missing zh-TW Translation ([#64](#64)) ([7759d0c](7759d0c)) * **portal:** Fix Proxy Cache Checkbox Visibility, Guard, and i18n Keys ([#54](#54)) ([d2034f0](d2034f0)) * **portal:** stabilize test runner ([3b23e05](3b23e05)) * Proxy Cache Fallback Local - Even When Remote Does Not Exist ([#38](#38)) ([0fe897d](0fe897d)) * **proxy:** [upstream] Preserve URL path prefix during registry auth discovery ([cf4f538](cf4f538)) * **proxy:** [upstream] Serve local artifact on remote not found in proxy cache ([e9877f1](e9877f1)) * Re-add missing in-toto attestation accessory model import ([#149](#149)) ([a508b0a](a508b0a)) * remove unauthorised banner ([704dcd4](704dcd4)) * Replace scannable content type skiplist with allowlist and add scan timeout ([#151](#151)) ([29a0245](29a0245)) * Resolve Lint And Vulnerability Issues ([#210](#210)) ([7e5d534](7e5d534)) * Restore Postgres 18 Volume Mount ([#197](#197)) ([b99f6f3](b99f6f3)) * **security:** [upstream] Reject Bearer Tokens Issued Before Project Creation ([4efbb56](4efbb56)) * **security:** reject bearer tokens issued before project creation ([#31](#31)) ([a7a7ce1](a7a7ce1)) * **session:** [upstream] Use Correct Maxlifetime in SessionRegenerate ([f8b5f82](f8b5f82)) * Set Release-Please Manifest to 2.14.0 for Correct 2.15.0 First Release ([#51](#51)) ([0afa522](0afa522)) * Use fully qualified PostgreSQL image name for Podman compatibility ([c300e3f](c300e3f)) ### Performance Improvements * **ci:** speed up unit test pipeline ([#147](#147)) ([5c8c40b](5c8c40b)) ### Code Refactoring * [upstream] Omit Unnecessary Reassignment ([1f412ed](1f412ed)) * **portal:** portal openapi refactor ([#48](#48)) ([7532146](7532146)) ### Documentation * Add PR Description Template to CLAUDE.md ([#62](#62)) ([ce9bffe](ce9bffe)) * Add Security Policy ([#190](#190)) ([63a5db2](63a5db2)) * Align README security reporting ([#195](#195)) ([12fa891](12fa891)) * Simplify CLAUDE.md ([#159](#159)) ([d19f8d3](d19f8d3)) * Update CONTRIBUTING.md with PR Description Template and Title Format ([#81](#81)) ([53cc0fa](53cc0fa)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Jul 9, 2026
chlins
pushed a commit
to Aloui-Ikram/harbor
that referenced
this pull request
Aug 10, 2026
When a multi-arch OCI index is signed with cosign, its per-arch child artifacts have no accessory of their own, so the UI shows them as unsigned. Populate InheritedAccessories from the parent index's cosign signature (display only, never used in copy/delete/walk) and surface it in ListAccessories when the query filters for signature.cosign. Ported from container-registry/harbor-next#27. Signed-off-by: aloui ikram <ikram@container-registry.com>
chlins
pushed a commit
to goharbor/harbor
that referenced
this pull request
Aug 10, 2026
* fix: implement cosign signature inheritance for OCI index children When a multi-arch OCI index is signed with cosign, its per-arch child artifacts have no accessory of their own, so the UI shows them as unsigned. Populate InheritedAccessories from the parent index's cosign signature (display only, never used in copy/delete/walk) and surface it in ListAccessories when the query filters for signature.cosign. Ported from container-registry/harbor-next#27. Signed-off-by: aloui ikram <ikram@container-registry.com> * refactor: expose inherited cosign signatures as a separate API field Rework the OCI index signature inheritance so that the accessory list stays faithful to registry content. The parent's signature is no longer merged into ListAccessories for a child digest; instead the already computed InheritedAccessories is exposed as a new, explicit inherited_accessories field on the artifact get and list endpoints. The inherited entries keep the parent index as their subject digest, so they remain distinguishable from a direct signature: cosign verify against the child digest still fails, and the referrers API still does not list them for that digest. Responses that do not ask for the field are unchanged. Resolving the field costs a reference lookup per artifact, so it is opt-in behind a new with_inherited_accessory query parameter, and an artifact that no index references short-circuits after a single indexed lookup. with_accessory could not serve as the gate because it defaults to true and is also set on the copy path. The Portal reads the new field and renders a distinct icon with a tooltip, and now fetches the child accessories inline with the artifact instead of issuing one extra request per child. Signed-off-by: aloui ikram <ikram@container-registry.com> * feat: extend signature inheritance to notation The inheritance introduced for cosign applies equally to notation. Like cosign, notation signs a single digest and has no flag to descend into the children of an index, so a notation-signed index leaves its children carrying no signature of their own. The Portal already treats both kinds as signed, so handling only cosign meant a notation-signed index still showed every one of its children as unsigned. Inherit both signature types, both when collecting the parent's accessories and in the check that skips artifacts already signed in their own right. That second place matters on its own: a child with its own notation signature under a cosign-signed index was not recognised as directly signed, and so was given an inherited entry, contradicting the documented meaning of the field. Accessory kinds that are not signatures stay excluded, since an SBOM or an accelerator on an index says nothing about the children of that index. Take the inherited icon colour from the Clarity neutral status token rather than a literal, so that it follows the light and dark themes instead of remaining darker than the text beside it in dark mode. Signed-off-by: aloui ikram <ikram@container-registry.com> --------- Signed-off-by: aloui ikram <ikram@container-registry.com> Co-authored-by: aloui ikram <ikram@container-registry.com> Co-authored-by: Wang Yan <wangyan_0219@hotmail.com>
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
before :
After :
Summary by cubic
Implements cosign signature inheritance from OCI index parents so unsigned children show as signed when the parent is signed. Updates the accessories API to surface inherited signatures when the UI filters for cosign.
Bug Fixes
Refactors
Written for commit eb03f8f. Summary will update on new commits.
Fixes #17
Summary by CodeRabbit