fix: enforce root role authorization when signing metadata

[ompushkara]

No description provided.

[qodo-for-securesign]

PR Summary by Qodo

Enforce root-role authorization for metadata signatures

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Rejects metadata signing keys not authorized for the TUF root role.
• Counts only unique, root-authorized signatures when enforcing root thresholds.
• Adds regression coverage for unauthorized and duplicate signatures.

Diagram

graph TD
A["Signing key"] --> B{"Root authorized?"} -->|Yes| C["Replace signature"] --> D["Sign metadata"] --> E["Count unique keys"] --> F{"Threshold met?"} -->|Yes| H["Save root"]
B -->|No| G["Reject signing"]
F -->|No| G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Cryptographically verify existing signatures

• ➕ Ensures threshold counts include only valid signatures from authorized root keys.
• ➕ Protects against malformed or fabricated pre-existing signature entries.
• ➖ Broadens the change beyond the reported authorization and deduplication defect.
• ➖ Requires careful handling of current-root and cross-signing verification semantics.

Recommendation: The PR’s set-based authorization and uniqueness checks are appropriate for this focused fix and should be retained. Full cryptographic verification could be considered separately if existing metadata signatures may come from untrusted sources.

Files changed (2) +68 / -4

Bug fix (1) +28 / -4

sign.goEnforce root-role authorization and signature uniqueness +28/-4

Enforce root-role authorization and signature uniqueness

• Signing now rejects keys absent from the root role’s authorized key IDs, even when they exist elsewhere in root metadata. Threshold validation counts only unique signatures associated with authorized root keys, preventing duplicates or other-role signatures from satisfying the threshold.

internal/root/sign.go

Tests (1) +40 / -0

sign_test.goCover unauthorized and duplicate root signatures +40/-0

Cover unauthorized and duplicate root signatures

• Adds regression tests proving that keys assigned only to another TUF role cannot sign root metadata. Also verifies that duplicate authorized signatures and signatures from unauthorized keys do not satisfy the root threshold.

internal/root/sign_test.go

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.32%. Comparing base (9111c3e) to head (b817f0b).

Files with missing lines	Patch %	Lines
internal/root/sign.go	92.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   70.18%   70.32%   +0.14%     
==========================================
  Files          32       32              
  Lines        3109     3131      +22     
==========================================
+ Hits         2182     2202      +20     
- Misses        563      564       +1     
- Partials      364      365       +1     

Flag	Coverage Δ
unit	70.32% <92.00%> (+0.14%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[qodo-for-securesign]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. A null root role crashes signing ✓ Resolved 🐞 Bug ☼ Reliability

Description

signRootWithKey treats a successful root-role map lookup as proof that the role pointer is non-nil
and immediately ranges over rootRole.KeyIDs. A parsed root containing "root": null reaches this
path for any requested key and panics rather than returning a validation error.

Code

internal/root/sign.go[R101-103]

+	authorized := false
+	for _, authorizedKeyID := range rootRole.KeyIDs {
+		if authorizedKeyID == keyID {

Relevance

●●● Strong

Deterministic nil dereference on malformed metadata; repository accepts concrete reliability fixes
for malformed-input crashes.

PR-#73
PR-#5

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

loadRoot returns metadata after parsing without validating role pointers, while signRootWithKey
checks only the map lookup boolean before dereferencing the returned pointer. Every requested
filesystem or Vault key passes through this function, making malformed role input an unhandled panic
path.

internal/root/modify.go[29-34]
internal/root/sign.go[52-81]
internal/root/sign.go[95-103]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A root-role entry can be present with a nil pointer, causing the newly added authorization loop to panic when it accesses `KeyIDs`.

## Fix Focus Areas
- internal/root/sign.go[95-103]
- internal/root/sign.go[129-145]

## Recommended Fix
Treat both a missing root-role entry and a nil role pointer as invalid metadata and return a descriptive error before dereferencing it. Apply equivalent nil checks while validating all role thresholds, and add malformed-metadata tests covering a null root role.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Signing can produce unverifiable roots ✓ Resolved 🐞 Bug ≡ Correctness

Description

signRootWithKey now authorizes keyID solely through rootRole.KeyIDs and no longer confirms
that validationMd.Signed.Keys contains its public key. During ordinary signing of externally
supplied or hand-edited metadata with a dangling root-role ID, md.Sign and the threshold count
accept that ID before Sign saves a signature with no corresponding verification key.

Code

internal/root/sign.go[R102-104]

+	for _, authorizedKeyID := range rootRole.KeyIDs {
+		if authorizedKeyID == keyID {
+			authorized = true

Relevance

●●● Strong

Dangling role IDs can create unverifiable signatures; repository accepts validation fixes preventing
malformed metadata and integrity failures.

PR-#5
PR-#49

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Root roles contain key-ID references separately from the public-key map: the normal AddKey path
supplies a parsed public key to Signed.AddKey, while the changed signing path checks only the role
reference. The resulting signature is assigned that ID, and the changed threshold logic also counts
it solely by ID, so neither stage restores the removed key-map validation.

internal/root/sign.go[95-109]
internal/root/sign.go[114-127]
internal/root/sign.go[142-156]
internal/root/modify.go[198-217]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Root signing now accepts a key ID listed in the root role even when that ID has no public-key entry in `Signed.Keys`, allowing metadata to pass threshold checks without a corresponding verification key.

## Fix Focus Areas
- internal/root/sign.go[95-109]
- internal/root/sign.go[142-156]

## Recommended Fix
Keep the root-role membership check, but also require the key ID to exist in `validationMd.Signed.Keys` before signing. When counting authorized signatures, intersect root-role IDs with keys actually present in `md.Signed.Keys`, and add a regression test using a dangling root-role key ID.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Cross-repo context — repo relationships

  Explored: repo: securesign/secure-sign-operator (sha: eeb3b448)

  Explored: repo: securesign/sigstore-e2e (sha: 66ad6a7b)

Review mode: ⚖️ Balanced: This changes root-role authorization and signature-threshold validation in security-sensitive metadata signing, warranting a careful complete review.

Tip of the day

💡 Did you know, you can choose which labels appear on a finding, and whether they show icons or text

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[ompushkara]

Aiming for 1.5.1 release