Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 40 additions & 19 deletions pkg/daemon/rpm-ostree.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,27 +296,48 @@ func (r *RpmOstreeClient) patchPoliciesForContainerStorage(podmanImageInfo *Podm
return err
}

policy, err := signature.NewPolicyFromBytes(policyOriginalContent)
if err != nil {
return err
}
var policy *signature.Policy

// Skopeo (< 1.22.2) fails on multi-arch manifest-lists Sigstore verification , overrides
// policy to avoid "A signature was required, but no signature exists" errors during
// rpm-ostree rebase operations on older boot images.
// See https://issues.redhat.com/browse/OCPBUGS-81187 for details.
if !skopeoSupportsMultiArchSigstore() {
klog.Infof("skopeo does not support multi-arch Sigstore, using fully permissive policy for rpm-ostree")
policy = &signature.Policy{
Default: signature.PolicyRequirements{signature.NewPRInsecureAcceptAnything()},
Transports: map[string]signature.PolicyTransportScopes{
"docker": {
"": signature.PolicyRequirements{signature.NewPRInsecureAcceptAnything()},
},
imagePolicyTransportContainerStorage: {
"": signature.PolicyRequirements{signature.NewPRInsecureAcceptAnything()},
},
},
}
} else {
policy, err = signature.NewPolicyFromBytes(policyOriginalContent)
if err != nil {
return err
}

_, containerStoragePoliciesPresent := policy.Transports[imagePolicyTransportContainerStorage]
if (reflect.DeepEqual(policy.Default[0], signature.PolicyRequirements{signature.NewPRInsecureAcceptAnything()}) && !containerStoragePoliciesPresent) {
// Temporary patching the policies.json file can be skipped, with warranties, and without re-implementing the
// logic that evaluates the policies or importing it under the following circumstances (must match all):
// 1. The default policy should be "insecureAcceptAnything"
// 2. Transport-specific policies for containers-storage shouldn't be in place.
return nil
}
_, containerStoragePoliciesPresent := policy.Transports[imagePolicyTransportContainerStorage]
if (reflect.DeepEqual(policy.Default[0], signature.PolicyRequirements{signature.NewPRInsecureAcceptAnything()}) && !containerStoragePoliciesPresent) {
// Temporary patching the policies.json file can be skipped, with warranties, and without re-implementing the
// logic that evaluates the policies or importing it under the following circumstances (must match all):
// 1. The default policy should be "insecureAcceptAnything"
// 2. Transport-specific policies for containers-storage shouldn't be in place.
return nil
}

// At this point there's no warranty the policy will allow rpm-ostree to fetch the image
// from local storage -> Add a specific rule to allow the image
if !containerStoragePoliciesPresent {
policy.Transports[imagePolicyTransportContainerStorage] = make(map[string]signature.PolicyRequirements)
}
policy.Transports[imagePolicyTransportContainerStorage][url] = signature.PolicyRequirements{
signature.NewPRInsecureAcceptAnything(),
// At this point there's no warranty the policy will allow rpm-ostree to fetch the image
// from local storage -> Add a specific rule to allow the image
if !containerStoragePoliciesPresent {
policy.Transports[imagePolicyTransportContainerStorage] = make(map[string]signature.PolicyRequirements)
}
policy.Transports[imagePolicyTransportContainerStorage][url] = signature.PolicyRequirements{
signature.NewPRInsecureAcceptAnything(),
}
}

// Prepare the json patched content
Expand Down
58 changes: 53 additions & 5 deletions pkg/daemon/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -2909,35 +2909,83 @@ func runCmdSync(cmdName string, args ...string) error {
return nil
}

const (
// minPodmanVersionForSigstore is the minimum podman version that supports Sigstore verification
// https://issues.redhat.com/browse/OCPBUGS-38809
minPodmanVersionForSigstore = "4.4.1"
// minSkopeoVersionForMultiArchSigstore is the minimum skopeo version that supports multi-arch Sigstore verification
// https://issues.redhat.com/browse/OCPBUGS-81187
minSkopeoVersionForMultiArchSigstore = "1.22.2"
)

var (
podmanSigstoreSupported sync.Once
podmanSigstoreSupportedValue bool
)

func podmanSupportsSigstore() bool {
podmanSigstoreSupported.Do(func() {
// https://issues.redhat.com/browse/OCPBUGS-38809 failed for base image 4.11 or older, OCP 4.12 is with podman 4.4.1
// returns false if podman version is less than 4.4.1
cmd := exec.Command("podman", "version", "-f", "{{.APIVersion}}")
out, err := cmd.CombinedOutput()
if err != nil {
klog.Errorf("failed to run podman version: %v", err)
podmanSigstoreSupportedValue = false
return
}
sigstorePodman := "4.4.1"
// Example version format: 5.3.0-rc1
imgPodmanVersion, err := semver.NewVersion(strings.TrimSpace(string(out)))
if err != nil {
klog.Errorf("failed to parse podman version: %v", err)
podmanSigstoreSupportedValue = false
return
}
podmanSigstoreSupportedValue = imgPodmanVersion.Compare(*semver.New(sigstorePodman)) >= 0
podmanSigstoreSupportedValue = imgPodmanVersion.Compare(*semver.New(minPodmanVersionForSigstore)) >= 0
})
return podmanSigstoreSupportedValue
}

var (
skopeoMultiArchSigstoreSupported sync.Once
skopeoMultiArchSigstoreSupportedValue bool
)

func skopeoSupportsMultiArchSigstore() bool {
skopeoMultiArchSigstoreSupported.Do(func() {
// Multi-arch Sigstore verification requires skopeo >= 1.22.2.
// Older boot images will fail with "A signature was required, but no signature exists"
// when rpm-ostree attempts to pull multi-arch images with Sigstore enforcement.
cmd := exec.Command("skopeo", "--version")
out, err := cmd.CombinedOutput()
if err != nil {
klog.Errorf("failed to run skopeo --version: %v", err)
skopeoMultiArchSigstoreSupportedValue = false
return
}
// Example output: "skopeo version 1.21.0-dev commit: d8be59c1ecc5c1860b7bab4f60721d55da2cda9a"
outStr := strings.TrimSpace(string(out))
fields := strings.Fields(outStr)
if len(fields) < 3 {
klog.Errorf("failed to parse skopeo version output: %s", outStr)
skopeoMultiArchSigstoreSupportedValue = false
return
}

versionStr := fields[2]
if dashIdx := strings.Index(versionStr, "-"); dashIdx != -1 {
versionStr = versionStr[:dashIdx]
}

skopeoVersion, err := semver.NewVersion(versionStr)
if err != nil {
klog.Errorf("failed to parse skopeo version %s: %v", versionStr, err)
skopeoMultiArchSigstoreSupportedValue = false
return
}
skopeoMultiArchSigstoreSupportedValue = skopeoVersion.Compare(*semver.New(minSkopeoVersionForMultiArchSigstore)) >= 0
klog.Infof("skopeo version %s, multi-arch Sigstore support: %v", versionStr, skopeoMultiArchSigstoreSupportedValue)
})
return skopeoMultiArchSigstoreSupportedValue
}
Comment on lines +2948 to +2981
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Avoid fail-open downgrade when the skopeo probe fails.

At Line 2958, Line 2967, and Line 2978, probe/parsing errors force false, and that directly triggers the fully permissive policy branch in pkg/daemon/rpm-ostree.go Line 305. Detection failures should not silently disable verification.

🔧 Suggested direction
-var (
-	skopeoMultiArchSigstoreSupported      sync.Once
-	skopeoMultiArchSigstoreSupportedValue bool
-)
+var (
+	skopeoMultiArchSigstoreSupported      sync.Once
+	skopeoMultiArchSigstoreSupportedValue bool
+	skopeoMultiArchSigstoreProbeErr       error
+)

-func skopeoSupportsMultiArchSigstore() bool {
+func skopeoSupportsMultiArchSigstore() (bool, error) {
 	skopeoMultiArchSigstoreSupported.Do(func() {
 		cmd := exec.Command("skopeo", "--version")
 		out, err := cmd.CombinedOutput()
 		if err != nil {
-			klog.Errorf("failed to run skopeo --version: %v", err)
-			skopeoMultiArchSigstoreSupportedValue = false
+			skopeoMultiArchSigstoreProbeErr = fmt.Errorf("failed to run skopeo --version: %w", err)
 			return
 		}
 		...
 		if err != nil {
-			klog.Errorf("failed to parse skopeo version %s: %v", versionStr, err)
-			skopeoMultiArchSigstoreSupportedValue = false
+			skopeoMultiArchSigstoreProbeErr = fmt.Errorf("failed to parse skopeo version %s: %w", versionStr, err)
 			return
 		}
 		skopeoMultiArchSigstoreSupportedValue = skopeoVersion.Compare(*semver.New(minSkopeoVersionForMultiArchSigstore)) >= 0
 	})
-	return skopeoMultiArchSigstoreSupportedValue
+	return skopeoMultiArchSigstoreSupportedValue, skopeoMultiArchSigstoreProbeErr
 }

Then gate permissive fallback only when probe succeeded and version is truly below minimum.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/daemon/update.go` around lines 2951 - 2987, The probe currently treats
any skopeo --version parse error as "false" which enables the permissive
fallback; modify skopeoSupportsMultiArchSigstore by introducing a probe-success
flag (e.g., skopeoMultiArchSigstoreProbeSucceeded) that is only set to true when
the command runs and the version comparison completes successfully (use existing
symbols skopeoMultiArchSigstoreSupported.Do,
skopeoMultiArchSigstoreSupportedValue, minSkopeoVersionForMultiArchSigstore and
semver parsing), and do not flip skopeoMultiArchSigstoreSupportedValue on
parse/exec errors; then update the code path in rpm-ostree.go that currently
gates permissive fallback on skopeoMultiArchSigstoreSupportedValue to require
the probe-success flag as well (i.e., only allow the permissive branch when the
probe succeeded and the version is below the minimum).


// Log a message to the systemd journal as well as our stdout
func logSystem(format string, a ...interface{}) {
message := fmt.Sprintf(format, a...)
Expand Down