Skip to content

Refactor image signature in acceptance tests#2948

Merged
st3penta merged 1 commit intoconforma:mainfrom
st3penta:refactor-acceptance-signature
Dec 15, 2025
Merged

Refactor image signature in acceptance tests#2948
st3penta merged 1 commit intoconforma:mainfrom
st3penta:refactor-acceptance-signature

Conversation

@st3penta
Copy link
Contributor

@st3penta st3penta commented Sep 17, 2025

User description

This commit refactors the function 'CreateAndPushImageSignature' so that it now creates a cosign signature that also contains the bundle with transparency log data.

The signature is created using the cosign.TLogUpload function, and stubbing the rekor endpoints that get called during the tlog entry creation process.
The result is a signature that can be successfully verified using the 'cosign verify' command.

The acceptance tests using this new signature will be closer to the real-world scenario, but they need some refactoring that will be done in a later PR.

Assisted by: Claude Code

Ref: https://issues.redhat.com/browse/EC-1210


PR Type

Enhancement


Description

  • Refactor image signature and attestation creation to include transparency log uploads

  • Implement cosign.TLogUpload flow with bundle information in signatures and attestations

  • Replace manual Rekor entry creation with automatic stubs during cosign operations

  • Update Rekor entry types: hashedrekord for signatures, intoto v0.0.2 for attestations

  • Remove explicit Rekor entry creation steps from acceptance test scenarios


Diagram Walkthrough

flowchart LR
  A["Image/Attestation Creation"] --> B["cosign.TLogUpload"]
  B --> C["Rekor Stub Entry Creation"]
  C --> D["Bundle Information"]
  D --> E["Signature/Attestation Layer"]
  E --> F["Registry Push"]
Loading

File Walkthrough

Relevant files
Enhancement
3 files
image.go
Add transparency log upload to signature and attestation creation
+208/-34
kubernetes.go
Remove explicit Rekor entry creation calls                             
+0/-11   
rekor.go
Refactor Rekor entry creation for signatures and attestations
+277/-91
Dependencies
4 files
go.mod
Add hashicorp HTTP client dependencies                                     
+2/-0     
go.sum
Update dependency checksums                                                           
+2/-0     
go.sum
Update Kubernetes dependency version                                         
+0/-2     
go.sum
Update Kubernetes dependency version                                         
+0/-2     
Tests
3 files
task_validate_image.feature
Add stub rekord to background setup                                           
+1/-0     
validate_image.feature
Remove manual Rekor entry creation steps from scenarios   
+3/-89   
vsa.feature
Remove manual Rekor entry creation steps from scenarios   
+0/-8     

@codecov
Copy link

codecov bot commented Sep 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
acceptance 54.75% <ø> (-1.08%) ⬇️
generative 18.99% <ø> (ø)
integration 27.92% <ø> (ø)
unit 67.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.
see 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@st3penta st3penta force-pushed the refactor-acceptance-signature branch 4 times, most recently from d8b4d7b to 906711b Compare September 18, 2025 09:42
// statement as required by the tests
// statement as required by the tests. This implementation now includes transparency
// log upload to generate bundle information like Tekton Chains does for attestations.
func createAndPushAttestationWithPatches(ctx context.Context, imageName, keyName string, patches *godog.Table) (context.Context, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function was also refactored to align it to the signing process of the image

Comment on lines +143 to 144
// do it. This implementation includes transparency log upload to generate bundle information.
func CreateAndPushImageSignature(ctx context.Context, imageName string, keyName string) (context.Context, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function is the main focus of the refactoring in this PR

return ctx, err
}

err = rekor.RekorEntryForImageSignature(ctx, imageRef)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rekor entry creation is now handled directly during signature creation

return ctx, err
}

err = rekor.RekorEntryForAttestation(ctx, imageRef)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rekor entry creation is now handled directly during signature creation

@st3penta st3penta force-pushed the refactor-acceptance-signature branch 2 times, most recently from 7c7c7e7 to 315e53f Compare September 18, 2025 09:56
Comment on lines -924 to 939

Scenario: rekor entries required
Scenario: signatures with embedded bundles verify without external rekor queries
Given a key pair named "known"
Given an image named "acceptance/rekor-by-default"
Given a valid image signature of "acceptance/rekor-by-default" image signed by the "known" key
Given a valid attestation of "acceptance/rekor-by-default" signed by the "known" key
Given rekor entries are cleared
Given a git repository named "rekor-by-default" with
| main.rego | examples/happy_day.rego |
Given policy configuration named "ec-policy" with specification
"""
{"sources": [{"policy": ["git::https://${GITHOST}/git/rekor-by-default.git"]}]}
"""
When ec command is run with "validate image --image ${REGISTRY}/acceptance/rekor-by-default --rekor-url ${REKOR} --policy acceptance/ec-policy --public-key ${known_PUBLIC_KEY} --output json"
Then the exit status should be 1
Then the exit status should be 0
Then the output should match the snapshot
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original scenario was no longer relevant, given the refactoring to the signing flows.

this updated version tests that if a signature has bundle information in it, the verification doesn't need to query rekor to pass

@st3penta st3penta marked this pull request as ready for review September 18, 2025 10:02

// creates the signature image with the correct media type and config and appends
// the signature layer to it
singnatureImage := mutate.MediaType(empty.Image, types.OCIManifestSchema1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more nature singing.. ? 😜

@simonbaird
Copy link
Member

Seems like a lot of effort figure it out, nice work. Lgtm.

My one thought: For Konflux right now, iiuc, Chains is configured to not use Rekor when signing things. Is there a code path in our acceptance tests where we sign things with no transparency log? If not, do you think we should have one? Either way it's not a blocker for this PR, I'm just wondering about it.

@st3penta
Copy link
Contributor Author

Is there a code path in our acceptance tests where we sign things with no transparency log?

No, with this PR all the signatures generated in the acceptance tests will have a bundle attached, with rekor entry data in it.
I think that, while having the rekor bundle verification is a more complete test, it could be worth having at least one test case that covers the verification of the signature without rekor (or in other words, the call to ec validate with the --ignore-rekor flag set).
Should i file a story for that?

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 15, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Shadowed identifier: A local variable named time shadows the imported time package, harming readability and
increasing the risk of confusion/errors.

Referred Code
// Use current Unix timestamp for integrated time
time := time.Now().Unix()

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing bounds check: The code indexes dsseEnvelope.Signatures[0] without verifying Signatures is non-empty,
which can panic on malformed/empty DSSE envelopes.

Referred Code
// Parse the DSSE envelope that was created by attestation.SignStatement
var dsseEnvelope struct {
	Payload     string `json:"payload"`
	PayloadType string `json:"payloadType"`
	Signatures  []struct {
		Keyid string `json:"keyid"`
		Sig   string `json:"sig"`
	} `json:"signatures"`
}

err = json.Unmarshal(attestationData, &dsseEnvelope)
if err != nil {
	return nil, nil, fmt.Errorf("failed to parse DSSE envelope: %w", err)
}

// Convert public key to base64
publicKeyBase64 := strfmt.Base64(publicKey)

// Create signature structure for the envelope
sigBase64 := strfmt.Base64(dsseEnvelope.Signatures[0].Sig)
signatures := []*models.IntotoV002SchemaContentEnvelopeSignaturesItems0{


 ... (clipped 5 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Data in error: The error path includes raw input data in the returned message, potentially exposing
sensitive content if propagated to user-visible output.

Referred Code
if signature.Sig == "" {
	return "", fmt.Errorf("data missing 'sig' key: %s", data)
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unescaped JSONPath interpolation: The JSONPath expression is built via string interpolation using signature.Sig and hash
without escaping, which could break matching or enable JSONPath injection depending on
WireMock's JSONPath handling.

Referred Code
// JsonPathFromSignature returns the JSON Path expression to be used in the wiremock stub
// for a signature query. The expression matches the value of the signature's content.
func JsonPathFromSignature(data []byte) (string, error) {
	signature := cosign.Signatures{}
	if err := json.Unmarshal(data, &signature); err != nil {
		return "", fmt.Errorf("unmarshalling signature: %w", err)
	}

	if signature.Sig == "" {
		return "", fmt.Errorf("data missing 'sig' key: %s", data)
	}

	return fmt.Sprintf("$..[?(@.content=='%s')]", signature.Sig), nil
}

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@st3penta
Copy link
Contributor Author

i think this was closed by mistake (closed instead of merged), since i can't find or recall any information about its closure.

i will rebase and merge it

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Refactor duplicates complex Rekor stubbing

Consolidate duplicated WireMock stubbing logic from
StubRekorEntryCreationForSignature and StubRekorEntryCreationForAttestation into
a single helper function to improve maintainability.

Examples:

acceptance/rekor/rekor.go [365-428]
func StubRekorEntryCreationForSignature(ctx context.Context, data []byte, signature []byte, signatureJSON []byte, publicKey []byte) error {
	state := testenv.FetchState[rekorState](ctx)

	logEntry, entryUUID, err := computeLogEntryForSignature(ctx, publicKey, data, signature)
	if err != nil {
		return err
	}

	// Compute the signed entry timestamp using the Rekor private key
	signedTimestamp, err := ComputeEntryTimestamp(state.KeyPair.PrivateBytes, state.KeyPair.Password(), *logEntry)

 ... (clipped 54 lines)
acceptance/rekor/rekor.go [432-496]
func StubRekorEntryCreationForAttestation(ctx context.Context, attestationData []byte, publicKey []byte) error {
	state := testenv.FetchState[rekorState](ctx)

	logEntry, entryUUID, err := computeLogEntryForAttestation(ctx, publicKey, attestationData)
	if err != nil {
		return err
	}

	// Compute the signed entry timestamp using the Rekor private key
	signedTimestamp, err := ComputeEntryTimestamp(state.KeyPair.PrivateBytes, state.KeyPair.Password(), *logEntry)

 ... (clipped 55 lines)

Solution Walkthrough:

Before:

func StubRekorEntryCreationForSignature(ctx, data, signature, signatureJSON, publicKey) error {
  // ...
  logEntry, entryUUID, err := computeLogEntryForSignature(...)
  // ...
  signedTimestamp, err := ComputeEntryTimestamp(...)
  // ...
  response := map[string]*models.LogEntryAnon{...}
  responseBody, err := json.Marshal(response)
  // ...
  wiremock.StubFor(ctx, wiremock.Post("/api/v1/log/entries")...) // Stub creation
  // ...
  jsonPathQueryValue, err := JsonPathFromSignature(...)
  retrievalResponse := []map[string]*models.LogEntryAnon{response}
  retrievalResponseBody, err := json.Marshal(retrievalResponse)
  // ...
  wiremock.StubFor(ctx, wiremock.Post("/api/v1/log/entries/retrieve")...) // Stub retrieval
}

func StubRekorEntryCreationForAttestation(ctx, attestationData, publicKey) error {
  // ...
  logEntry, entryUUID, err := computeLogEntryForAttestation(...)
  // ...
  // IDENTICAL LOGIC for timestamp, response creation, and stubbing
  // ...
}

After:

func stubRekorEntryCreation(ctx, logEntry, entryUUID, jsonPathQueryValue) error {
  // ...
  signedTimestamp, err := ComputeEntryTimestamp(...)
  // ...
  response := map[string]*models.LogEntryAnon{...}
  responseBody, err := json.Marshal(response)
  // ...
  wiremock.StubFor(ctx, wiremock.Post("/api/v1/log/entries")...) // Stub creation
  // ...
  retrievalResponse := []map[string]*models.LogEntryAnon{response}
  retrievalResponseBody, err := json.Marshal(retrievalResponse)
  // ...
  wiremock.StubFor(ctx, wiremock.Post("/api/v1/log/entries/retrieve")...) // Stub retrieval
}

func StubRekorEntryCreationForSignature(ctx, data, signature, signatureJSON, publicKey) error {
  logEntry, entryUUID, err := computeLogEntryForSignature(...)
  jsonPathQueryValue, err := JsonPathFromSignature(...)
  return stubRekorEntryCreation(ctx, logEntry, entryUUID, jsonPathQueryValue)
}

func StubRekorEntryCreationForAttestation(ctx, attestationData, publicKey) error {
  logEntry, entryUUID, err := computeLogEntryForAttestation(...)
  jsonPathQueryValue := // ... compute path for attestation
  return stubRekorEntryCreation(ctx, logEntry, entryUUID, jsonPathQueryValue)
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies significant and complex duplicated logic in the new StubRekorEntryCreationForSignature and StubRekorEntryCreationForAttestation functions, and abstracting this into a helper would substantially improve code quality and maintainability.

Medium
  • Update

@st3penta st3penta force-pushed the refactor-acceptance-signature branch from 315e53f to 235c812 Compare December 15, 2025 13:14
This commit refactors the functions 'CreateAndPushImageSignature' and
'createAndPushAttestationWithPatches' so that they now replicate the
transparency log entry creation on the rekor stub.

The signature and the attestation are now created using the
cosign.TLogUpload function, and stubbing the rekor endpoints that get
called during the tlog entry creation process.
The result is a signature that can be successfully verified using the
'cosign verify' command, and an attestation that has a corresponding
entry in rekor.

This refactor also removed the need to explicitly create rekor entries
in the acceptance tests, since this is now part of the cosign flow.

The acceptance tests using this new rekor flow now reflect more
accuratly the real-world scenario.

Assisted by: Claude Code

Ref: https://issues.redhat.com/browse/EC-1210
@st3penta st3penta force-pushed the refactor-acceptance-signature branch from 235c812 to 5387e73 Compare December 15, 2025 13:16
@st3penta st3penta merged commit 6498df4 into conforma:main Dec 15, 2025
11 checks passed
@st3penta st3penta deleted the refactor-acceptance-signature branch December 15, 2025 13:36
simonbaird added a commit to simonbaird/conforma-cli that referenced this pull request Dec 16, 2025
After PR conforma#2948 was merged, we don't need these steps any more. IIUC
the rekor entries get created automatically in a more realistic way.

The PR for EC-1210 was accidentally left unmerged for a long time,
which explains why the new VSA feature testing still had the
obsolete step.

Related to...
Ref: https://issues.redhat.com/browse/EC-1210
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants