-
Notifications
You must be signed in to change notification settings - Fork 2
[SECURESIGN-2287] add artifact verification #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR introduces Sigstore-based verification by replacing the artifact verification endpoint with a GET, extending API models/specs with comprehensive verification parameters, adding a new internal verify package that implements policy-driven Sigstore and TUF flows, and wiring the service to invoke this new logic with updated dependencies. Sequence diagram for new Sigstore-based artifact verification flowsequenceDiagram
participant Client
participant API
participant ArtifactService
participant VerifyService
Client->>API: GET /api/v1/artifacts/verify (with verification params)
API->>ArtifactService: VerifyArtifact(request)
ArtifactService->>VerifyService: VerifyArtifact(ctx, verifyOpts)
VerifyService->>VerifyService: Build bundle, apply verification policy
VerifyService-->>ArtifactService: Verification result
ArtifactService-->>API: Verification response
API-->>Client: Verification result
ER diagram for updated VerifyArtifactRequest and VerifyArtifactResponse modelserDiagram
VERIFYARTIFACTREQUEST {
string artifact
string artifactDigest
string artifactDigestAlgorithm
string expectedOIDIssuer
string expectedOIDIssuerRegex
string expectedSAN
string expectedSANRegex
boolean requireTimestamp
boolean requireCTLog
boolean requireTLog
string minBundleVersion
string trustedPublicKey
string trustedRootJSONPath
string tufRootURL
string tufTrustedRoot
object annotations
boolean offline
string output
}
VERIFYARTIFACTRESPONSE {
boolean verified
string message
}
VERIFYARTIFACTREQUEST ||--o{ VERIFYARTIFACTRESPONSE : verifies
Class diagram for updated VerifyArtifactRequest modelclassDiagram
class VerifyArtifactRequest {
+string Artifact
+string ArtifactDigest
+string ArtifactDigestAlgorithm
+string ExpectedOIDIssuer
+string ExpectedOIDIssuerRegex
+string ExpectedSAN
+string ExpectedSANRegex
+bool RequireTimestamp
+bool RequireCTLog
+bool RequireTLog
+string MinBundleVersion
+string TrustedPublicKey
+string TrustedRootJSONPath
+string TufRootURL
+string TufTrustedRoot
+map<string, string> Annotations
+bool Offline
+VerifyArtifactRequestOutput Output
}
class VerifyArtifactRequestOutput {
<<enum>>
json
text
}
VerifyArtifactRequest --> VerifyArtifactRequestOutput
Class diagram for new VerifyOptions struct in verify packageclassDiagram
class VerifyOptions {
+string OCIImage
+string ArtifactDigest
+string ArtifactDigestAlgorithm
+string ExpectedOIDIssuer
+string ExpectedOIDIssuerRegex
+string ExpectedSAN
+string ExpectedSANRegex
+bool RequireTimestamp
+bool RequireCTLog
+bool RequireTLog
+string MinBundleVersion
+string TrustedPublicKey
+string TrustedRootJSONPath
+string TUFRootURL
+string TUFTrustedRoot
}
VerifyOptions : +NewVerifyOptions()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The API change to use GET with a request body for verification is non-standard—consider reverting to POST or moving parameters into query strings to align with HTTP conventions.
- In artifactService.VerifyArtifact you’re dereferencing pointer fields like req.ExpectedOIDIssuer without nil checks—add validation or defaults to prevent runtime panics on missing inputs.
- The internal verify package is very large and handles multiple concerns; splitting it into smaller modules (e.g. bundle extraction, TUF logic, policy enforcement) would improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The API change to use GET with a request body for verification is non-standard—consider reverting to POST or moving parameters into query strings to align with HTTP conventions.
- In artifactService.VerifyArtifact you’re dereferencing pointer fields like req.ExpectedOIDIssuer without nil checks—add validation or defaults to prevent runtime panics on missing inputs.
- The internal verify package is very large and handles multiple concerns; splitting it into smaller modules (e.g. bundle extraction, TUF logic, policy enforcement) would improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `internal/api/handlers.go:55` </location>
<code_context>
}
-func (h *Handler) PostApiV1ArtifactsVerify(w http.ResponseWriter, r *http.Request) {
+func (h *Handler) GetApiV1ArtifactsVerify(w http.ResponseWriter, r *http.Request) {
var req models.VerifyArtifactRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
</code_context>
<issue_to_address>
**issue:** Decoding request body for GET endpoint may not be standard practice.
Some clients and servers may not support GET requests with a body. Use query parameters for input, or change to POST if a request body is necessary.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
8c6782d to
252b0e2
Compare
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
README.md
Outdated
| # RHTAS Console | ||
|
|
||
| The RHTAS Console is a Go-based RESTful API server, providing functionality for signing and verifying software artifacts using Cosign, interacting with Sigstore's Rekor transparency log, and managing trust configurations with TUF and Fulcio. This repository serves as the backend for the RHTAS Console application, with plans to potentially add a frontend in the future. | ||
| The RHTAS Console is a Go-based RESTful API server, providing functionality for verifying software artifacts, interacting with Sigstore's Rekor transparency log, and managing trust configurations with TUF and Fulcio. This repository serves as the backend for the RHTAS Console application, with plans to potentially add a frontend in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with plans to potentially add a frontend in the future
I think we can already link to the frontend here 😄 https://github.com/securesign/rhtas-console-ui
internal/services/artifact.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func (s *artifactService) VerifyArtifact(ctx context.Context, req models.VerifyArtifactRequest) (models.VerifyArtifactResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also be listing and verifying all attestations? Or is that out of scope for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kahboom in the latest commits, I've added support for attestation verification.
Do we need another endpoint to list all attestations/signatures ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that it would be good to have a separate endpoint but I would like to spend a little bit more time investigating this, if you don't mind. Could we leave that for a separate PR and I'll create a Jira issue if we decide to go that direction? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! we can do like this.
So to resume, this PR objective is to verify both signatures and attestations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New security issues found
09aa6bc to
892a3e9
Compare
internal/services/verify/verify.go
Outdated
| } | ||
| } | ||
|
|
||
| func VerifyArtifact(ctx context.Context, verifyOpts VerifyOptions) (details string, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx is passed in here but not used
internal/models/models.go
Outdated
| // Annotations Optional key-value annotations to verify in the signature | ||
| Annotations *map[string]string `json:"annotations,omitempty"` | ||
| // PredicateType The type of the predicate for the attestation. | ||
| PredicateType *string `json:"PredicateType,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| PredicateType *string `json:"PredicateType,omitempty"` | |
| PredicateType *string `json:"predicateType,omitempty"` |
internal/services/verify/verify.go
Outdated
| apiVersion := jsonData["apiVersion"].(string) | ||
| kind := jsonData["kind"].(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be worth check this assertion is ok using ok
Summary by Sourcery
Enable comprehensive artifact verification, extending the API schema and models, and wiring in a new Sigstore-based verification implementation with support for OIDC, CT/TLog, TUF trust, and optional timestamp checks.
New Features:
Enhancements:
Build: