[SECURESIGN-3261] Extend artifact verification API with ViewModel data #40
Conversation
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
Reviewer's GuideRefactors the artifact verification flow to return a rich structured VerifyArtifactResponse with signatures, attestations, artifact metadata, and summary counts, introduces view-model helpers for signatures/attestations, and extends the OpenAPI/models accordingly while removing legacy flat verification and metadata-signature listing logic. Sequence diagram for the new artifact verification flowsequenceDiagram
actor "Client"
participant "ArtifactService" as ArtifactService
participant "VerifyService" as VerifyService
participant "Registry" as Registry
"Client"->>"ArtifactService": "POST /api/v1/artifacts/verify"
"ArtifactService"->>"VerifyService": "VerifyArtifact(VerifyOptions)"
"VerifyService"->>"Registry": "signingLayersFromOCIImage(OCIImage)"
"Registry"-->>"VerifyService": "Signing layers[] or error"
alt "Error getting signing layers"
"VerifyService"-->>"ArtifactService": "Error"
"ArtifactService"-->>"Client": "HTTP error with empty VerifyArtifactResponse"
else "Signing layers found"
loop "For each signing layer"
"VerifyService"->>"VerifyService": "VerifyAndGetSignatureView(verifyOpts, layer)"
"VerifyService"->>"VerifyService": "bundleFromSigningLayer(layer, hasTlog, hasTimestamp)"
"VerifyService"->>"VerifyService": "extractSignatureViewFromLayer(layer, bundle)"
"VerifyService"->>"VerifyService": "VerifyLayer(verifyOpts with ExpectedSAN, bundle)"
alt "Verification fails"
"VerifyService"-->>"ArtifactService": "Error (invalid signature layer)"
"ArtifactService"-->>"Client": "HTTP error with empty VerifyArtifactResponse"
else "Verification succeeds"
"VerifyService"->>"VerifyService": "Append SignatureView to VerifyArtifactResponse.Signatures"
"VerifyService"->>"VerifyService": "Increment SignatureCount and RekorEntryCount in Summary"
end
end
"VerifyService"->>"Registry": "attestationLayersFromOCIImage(OCIImage)"
"Registry"-->>"VerifyService": "Attestation layers[] or error"
alt "Error getting attestation layers"
"VerifyService"-->>"ArtifactService": "Error"
"ArtifactService"-->>"Client": "HTTP error with empty VerifyArtifactResponse"
else "Attestation layers found"
loop "For each attestation layer"
"VerifyService"->>"VerifyService": "VerifyAndGetAttestationView(verifyOpts, layer)"
"VerifyService"->>"VerifyService": "bundleFromAttestationLayer(OCIImage, layer, hasTlog, hasTimestamp)"
"VerifyService"->>"VerifyService": "extractAttestationViewFromLayer(layer, bundle)"
"VerifyService"->>"VerifyService": "getSANFromCert(certificate annotation)"
"VerifyService"->>"VerifyService": "VerifyLayer(verifyOpts with ExpectedSAN, bundle)"
alt "Verification fails"
"VerifyService"-->>"ArtifactService": "Error (invalid attestation layer)"
"ArtifactService"-->>"Client": "HTTP error with empty VerifyArtifactResponse"
else "Verification succeeds"
"VerifyService"->>"VerifyService": "Populate predicateType and rawStatementJson from VerificationResult"
"VerifyService"->>"VerifyService": "Append AttestationView to VerifyArtifactResponse.Attestations"
"VerifyService"->>"VerifyService": "Increment AttestationCount and RekorEntryCount in Summary"
end
end
end
"VerifyService"->>"VerifyService": "GetImageMetadata(OCIImage, username, password)"
"VerifyService"->>"Registry": "remote.Get / remote.Image / ConfigFile"
"Registry"-->>"VerifyService": "Descriptor, Image, Config or error"
alt "Metadata error"
"VerifyService"-->>"ArtifactService": "Error"
"ArtifactService"-->>"Client": "HTTP error with empty VerifyArtifactResponse"
else "Metadata fetched"
"VerifyService"->>"VerifyService": "Build ImageMetadataResponse (artifact, metadata, digest)"
"VerifyService"-->>"ArtifactService": "VerifyArtifactResponse {signatures, attestations, summary, artifact}"
"ArtifactService"-->>"Client": "200 OK with structured VerifyArtifactResponse"
end
end
Updated class diagram for VerifyArtifactResponse and related view modelsclassDiagram
class VerifyArtifactResponse {
+ImageMetadataResponse artifact
+AttestationView[] attestations
+SignatureView[] signatures
+ArtifactSummaryView summary
}
class ArtifactSummaryView {
+int signatureCount
+int attestationCount
+int rekorEntryCount
}
class ImageMetadataResponse {
+string digest
+string image
+Metadata metadata
}
class Metadata {
+string mediaType
+int64 size
+time.Time created
+map~string,string~ labels
}
class SignatureView {
+int id
+string digest
+ParsedCertificate signingCertificate
+ParsedCertificate[] certificateChain
+map~string,interfaceAny~ tlogEntry
+string rawBundleJson
+time.Time timestamp
+string signatureStatus
}
class AttestationView {
+int id
+string digest
+map~string,interfaceAny~ tlogEntry
+string rawBundleJson
+string rawStatementJson
+string predicateType
+time.Time timestamp
+string attestationStatus
}
class ParsedCertificate {
+CertificateRole role
+string subject
+string issuer
+time.Time notBefore
+time.Time notAfter
+string[] sans
+string serialNumber
+bool isCa
+string pem
}
class CertificateRole {
<<enumeration>>
Leaf
Intermediate
Root
}
VerifyArtifactResponse --> ImageMetadataResponse : "artifact"
VerifyArtifactResponse --> SignatureView : "signatures *"
VerifyArtifactResponse --> AttestationView : "attestations *"
VerifyArtifactResponse --> ArtifactSummaryView : "summary"
ImageMetadataResponse --> Metadata : "metadata"
SignatureView --> ParsedCertificate : "signingCertificate"
SignatureView --> ParsedCertificate : "certificateChain *"
ParsedCertificate --> CertificateRole : "role"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- In extractSignatureViewFromLayer/VerifyAndGetSignatureView you assume at least one parsed certificate and at least one SAN (e.g. cert slice index 0 and SigningCertificate.Sans[0]), which can panic on malformed or SAN-less certificates; consider returning a clear error when no certs/SANs are present instead of indexing blindly.
- identifyCertRole can return the string "unknown" for models.CertificateRole, but the OpenAPI enum for CertificateRole only allows leaf/intermediate/root; either extend the enum or change the function to return one of the defined values to keep API and implementation consistent.
- The helper functions isNotFound/isAuthError/isConnectionError now exist both in internal/services/verify and internal/services/artifact with similar logic; consider centralizing them to avoid duplication and potential drift in behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In extractSignatureViewFromLayer/VerifyAndGetSignatureView you assume at least one parsed certificate and at least one SAN (e.g. cert slice index 0 and SigningCertificate.Sans[0]), which can panic on malformed or SAN-less certificates; consider returning a clear error when no certs/SANs are present instead of indexing blindly.
- identifyCertRole can return the string "unknown" for models.CertificateRole, but the OpenAPI enum for CertificateRole only allows leaf/intermediate/root; either extend the enum or change the function to return one of the defined values to keep API and implementation consistent.
- The helper functions isNotFound/isAuthError/isConnectionError now exist both in internal/services/verify and internal/services/artifact with similar logic; consider centralizing them to avoid duplication and potential drift in behavior.
## Individual Comments
### Comment 1
<location> `internal/services/verify/verify.go:314` </location>
<code_context>
- return "", fmt.Errorf("failed to marshal verifier result: %w", err)
+ return invalidSignatureView, fmt.Errorf("failed to extract SignatureView from layer %w", err)
}
+ verifyOpts.ExpectedSAN = signatureView.SigningCertificate.Sans[0]
- if err := json.Unmarshal(resultBytes, &resultMap); err != nil {
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing the first SAN without checking length risks a panic when no SANs are present.
This assumes `SigningCertificate.Sans` has at least one entry and will panic if it’s empty. Please add a length check and, if there are no SANs, either fall back to `ExpectedSANRegex` or return a clear error/mark the signature as invalid instead of indexing `Sans[0]`.
</issue_to_address>
### Comment 2
<location> `internal/services/verify/verify.go:484-488` </location>
<code_context>
+ signingCertStr = cert
+ }
+
+ cert, err := parsePEMCertificates(signingCertStr)
if err != nil {
- return nil, fmt.Errorf("failed to marshal map to JSON: %w", err)
+ return models.SignatureView{}, fmt.Errorf("failed parsing signing certificate: %w", err)
+ }
+ c := cert[0]
+ sanList := mergeSANs(c.Cert)
+ sn := c.Cert.SerialNumber.String()
</code_context>
<issue_to_address>
**issue (bug_risk):** `parsePEMCertificates` can return an empty slice, which will cause an index-out-of-range panic on `cert[0]`.
If the `dev.sigstore.cosign/certificate` annotation is missing or malformed, `signingCertStr` may be empty or contain no valid PEM blocks. In that case `parsePEMCertificates` returns an empty slice and `cert[0]` panics.
Please guard this by checking `len(cert) > 0` before indexing and return a clear error (and `invalidSignatureView`) when no certificate is parsed, consistent with how the attestation path handles missing certificates.
</issue_to_address>
### Comment 3
<location> `internal/services/verify/verify.go:334-343` </location>
<code_context>
+ attestationView, err = extractAttestationViewFromLayer(layer, b)
</code_context>
<issue_to_address>
**suggestion:** Error handling in `VerifyAndGetAttestationView` is inconsistent for the statement marshaling error case.
For consistency, this path should also return `invalidAttestationView` when `json.MarshalIndent(verificationResult.Statement, ...)` fails, rather than a zero-valued `models.AttestationView{}`. That way, any caller can reliably assume that a non-nil error always pairs with `invalidAttestationView`, making the helper less error-prone to reuse.
Suggested implementation:
```golang
statementBytes, err := json.MarshalIndent(verificationResult.Statement, "", " ")
if err != nil {
return invalidAttestationView, fmt.Errorf("failed to marshal statement: %w", err)
}
```
I assumed the marshaling code inside VerifyAndGetAttestationView currently looks like:
- It calls json.MarshalIndent(verificationResult.Statement, ..., ...) into a variable (e.g., statementBytes).
- On error, it returns models.AttestationView{} and the error.
If the exact variable names or error message differ, please adjust the SEARCH block to match your current code and keep the same change in behavior: always return invalidAttestationView instead of a zero-valued models.AttestationView{} when json.MarshalIndent fails. This ensures all error paths in VerifyAndGetAttestationView consistently return invalidAttestationView alongside a non-nil error.
</issue_to_address>
### Comment 4
<location> `internal/services/verify/verify.go:971-974` </location>
<code_context>
-// attestationLayerFromOCIImage returns the attestation layer from the OCI image reference
-func attestationLayerFromOCIImage(imageRef string, predicateType string) (*v1.Descriptor, error) {
+// attestationLayersFromOCIImage returns the attestation layer from signing layer
+func attestationLayersFromOCIImage(imageRef string) ([]*v1.Descriptor, error) {
ref, err := name.ParseReference(imageRef)
if err != nil {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** All attestation manifest layers are returned without filtering, which may cause verification to fail on non-DSSE layers.
Previously, `attestationLayerFromOCIImage` filtered by media type and predicate type so only DSSE attestations were processed. Now `attestationLayersFromOCIImage` returns all layers, and `VerifyAndGetAttestationView` assumes each is a valid DSSE bundle. If the manifest includes non-`application/vnd.dsse.envelope.v1+json` layers or mixed predicate types, `bundleFromAttestationLayer` and downstream logic may fail and abort verification for the whole artifact. Please reintroduce media-type filtering here (and, if still applicable, predicate-type filtering based on `verifyOpts.PredicateType`) so only DSSE attestation layers are returned.
Suggested implementation:
```golang
// attestationLayersFromOCIImage returns only DSSE attestation layers (and optionally
// filters by predicate type) from the signing image manifest. This mirrors the
// previous attestationLayerFromOCIImage behavior so that downstream verification
// only sees DSSE bundles with the expected predicate type.
func attestationLayersFromOCIImage(imageRef string, predicateType string) ([]*v1.Descriptor, error) {
ref, err := name.ParseReference(imageRef)
if err != nil {
return nil, fmt.Errorf("parsing image reference %q: %w", imageRef, err)
}
// Resolve the top-level descriptor for the image reference.
desc, err := remote.Get(ref)
if err != nil {
return nil, fmt.Errorf("getting remote descriptor for %q: %w", imageRef, err)
}
// We expect an index that contains an attestation manifest.
idx, err := desc.ImageIndex()
if err != nil {
return nil, fmt.Errorf("getting image index for %q: %w", imageRef, err)
}
indexManifest, err := idx.IndexManifest()
if err != nil {
return nil, fmt.Errorf("getting index manifest for %q: %w", imageRef, err)
}
var result []*v1.Descriptor
for _, m := range indexManifest.Manifests {
img, err := idx.Image(m.Digest)
if err != nil {
// Skip this child on error instead of aborting the whole verification.
continue
}
manifest, err := img.Manifest()
if err != nil {
continue
}
for i := range manifest.Layers {
layer := manifest.Layers[i]
// 1) Filter by DSSE envelope media type so we don't try to treat
// arbitrary layers as DSSE bundles.
if string(layer.MediaType) != "application/vnd.dsse.envelope.v1+json" {
continue
}
// 2) If a specific predicate type is requested, further filter based
// on the standard cosign predicateType annotation when present.
if predicateType != "" && layer.Annotations != nil {
if pt, ok := layer.Annotations["dev.sigstore.cosign/predicateType"]; ok && pt != predicateType {
continue
}
}
// Append a pointer to this descriptor.
l := layer
result = append(result, &l)
}
}
return result, nil
}
```
To fully implement the requested behavior and integrate this change with the rest of the file, you should also:
1) Update all call sites of attestationLayersFromOCIImage to pass the predicate type:
- Wherever you currently have:
attLayers, err := attestationLayersFromOCIImage(imageRef)
change it to:
attLayers, err := attestationLayersFromOCIImage(imageRef, verifyOpts.PredicateType)
(or pass the appropriate predicateType string if your verify options are named differently).
2) Ensure the necessary imports are present at the top of internal/services/verify/verify.go:
- "fmt"
- "github.com/google/go-containerregistry/pkg/name"
- "github.com/google/go-containerregistry/pkg/v1"
- "github.com/google/go-containerregistry/pkg/v1/remote"
If your project already imports these (likely, given the existing OCI logic), no further changes are needed.
3) If your codebase already defines constants for:
- the DSSE media type (e.g., application/vnd.dsse.envelope.v1+json), or
- the predicateType annotation key (e.g., dev.sigstore.cosign/predicateType),
replace the string literals in the new function with those existing constants to stay consistent with the rest of the codebase.
</issue_to_address>
### Comment 5
<location> `internal/services/verify/verify.go:1219-1228` </location>
<code_context>
+}
+
+// identifyCertRole classifies a certificate as leaf, intermediate, root, or unknown.
+func identifyCertRole(cert *x509.Certificate) models.CertificateRole {
+ if cert == nil {
+ return "unknown"
+ }
+
+ // Leaf certificate: not a CA
+ if !cert.IsCA {
+ return "leaf"
+ }
+
+ // Check for self-signed: subject == issuer
+ isSelfSigned := bytes.Equal(cert.RawSubject, cert.RawIssuer)
+
+ // Verify signature with its own public key
+ if isSelfSigned {
+ if err := cert.CheckSignatureFrom(cert); err != nil {
+ isSelfSigned = false
+ }
+ }
+
+ if isSelfSigned {
+ return "root"
+ }
+
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The `identifyCertRole` default return value does not align with the defined `CertificateRole` constants and OpenAPI enum.
Because `models.CertificateRole` is a string alias, the compiler won’t prevent returning values outside the allowed set. The `nil`-cert branch returns "unknown", which isn’t one of the defined constants or the OpenAPI enum values (`leaf`, `intermediate`, `root`). This violates the documented contract and could confuse or break clients if it’s ever hit. I’d recommend either preventing `nil` from reaching `identifyCertRole`, or mapping the default to a documented value (or empty string) and handling that explicitly on the client side.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fghanmi
force-pushed
the
SECURESIGN-3261
branch
from
63af7e2 to
b997230
Compare
fghanmi
force-pushed
the
SECURESIGN-3261
branch
from
99c8137 to
af7d0eb
Compare
JasonPowr
approved these changes
Nov 26, 2025
kahboom
approved these changes
Nov 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
PR Type
Enhancement
Description
Restructure artifact verification API response with detailed ViewModel data
detailsobject with structuredSignatureViewandAttestationViewarraysArtifactSummaryViewwith signature, attestation, and Rekor entry countsExtend certificate handling with parsed certificate chain information
ParsedCertificatemodel with role, subject, issuer, SANs, and validity datesCertificateRoleenum (leaf, intermediate, root) for certificate classificationRefactor verification logic to process multiple signatures and attestations
Simplify
ImageMetadataResponseby removing embedded signatures and attestationsDiagram Walkthrough
File Walkthrough
models.go
Add ViewModel data structures for verification responseinternal/models/models.go
CertificateRoletype and constants (leaf, intermediate, root)ParsedCertificatestruct with certificate details (role,subject, issuer, SANs, validity dates, PEM)
SignatureViewstruct replacing oldSignaturetype with parsedcertificate chain and metadata
AttestationViewstruct with attestation-specific fields (predicatetype, raw statement JSON, status)
ArtifactSummaryViewstruct with signature, attestation, and Rekorentry counts
VerifyArtifactResponseto include artifact metadata,signatures array, attestations array, and summary
AttestationsandSignaturesfields fromImageMetadataResponseverify.go
Refactor verification to build structured ViewModel responsesinternal/services/verify/verify.go
VerifyArtifactto returnVerifyArtifactResponseinstead ofJSON string
separate view extraction
VerifyLayerfunction to handle bundle verification logicVerifyAndGetSignatureViewandVerifyAndGetAttestationViewfunctions for detailed view extraction
GetImageMetadatafunction to fetch OCI image metadataparsePEMCertificates,identifyCertRole,mergeSANs)isConnectionError)
layers
extractSignatureViewFromLayerandextractAttestationViewFromLayerfor view construction
artifact.go
Simplify artifact service by removing signature/attestation logicinternal/services/artifact.go
GetImageMetadatafunction
VerifyArtifactto return structured response instead of rawJSON details
getImageSignaturesAndChainsandgetImageAttestationsrhtas-console.yaml
Update OpenAPI schema with new ViewModel structuresinternal/api/openapi/rhtas-console.yaml
VerifyArtifactResponseproperties withsignatures,attestations,summary, andartifactarrays/objectsCertificateRoleenum schema (leaf, intermediate, root)ParsedCertificateschema with certificate details and roleinformation
SignatureViewschema with signing certificate, certificate chain,and metadata
AttestationViewschema with predicate type, raw statement JSON,and status
ArtifactSummaryViewschema with signature, attestation, and Rekorentry counts
SignatureandSignaturesschemassignaturesandattestationsfields fromImageMetadataResponse