deploy-from-self if skopeo < 1.22.2#5863
Draft
QiWang19 wants to merge 3 commits intoopenshift:mainfrom
Draft
Conversation
For skopeo < 1.22.2, create temp insecureAcceptAnything policy.json in rpm-ostree to avoid multi-arch release image signature errors. Signed-off-by: Qi Wang <qiwan@redhat.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Member
Author
|
/test all |
WalkthroughThe changes add a capability check function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: QiWang19 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/daemon/rpm-ostree.go (1)
305-317: Consider reducing fallback policy blast radius.This fallback sets wildcard
insecureAcceptAnythingfor alldockerandcontainers-storagerefs in rpm-ostreed. If not strictly required, scoping to the specific generated transport key would reduce temporary exposure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/daemon/rpm-ostree.go` around lines 305 - 317, The fallback policy currently applies signature.NewPRInsecureAcceptAnything() to the wildcard "" scope for both "docker" and imagePolicyTransportContainerStorage, which opens all refs; instead, narrow the blast radius by assigning the insecure AcceptAnything requirement only to the specific transport key(s) used for rpm-ostree in this routine (i.e., replace the "" map key with the generated transport reference string used elsewhere in the function — the exact transport key variable/name used for the rpm-ostree image refs — and do not add a global wildcard entry), keeping the rest of signature.Policy unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/daemon/update.go`:
- Around line 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).
---
Nitpick comments:
In `@pkg/daemon/rpm-ostree.go`:
- Around line 305-317: The fallback policy currently applies
signature.NewPRInsecureAcceptAnything() to the wildcard "" scope for both
"docker" and imagePolicyTransportContainerStorage, which opens all refs;
instead, narrow the blast radius by assigning the insecure AcceptAnything
requirement only to the specific transport key(s) used for rpm-ostree in this
routine (i.e., replace the "" map key with the generated transport reference
string used elsewhere in the function — the exact transport key variable/name
used for the rpm-ostree image refs — and do not add a global wildcard entry),
keeping the rest of signature.Policy unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fdc191e4-cfdd-4baa-81af-158a041031fa
📒 Files selected for processing (2)
pkg/daemon/rpm-ostree.gopkg/daemon/update.go
Comment on lines
+2951
to
+2987
| 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 | ||
| } |
There was a problem hiding this comment.
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).
Contributor
|
@QiWang19: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This reverts commit 90019b0.
uses deploy-from-self when skopeo is actually older than 1.22.2 that has issue with multi-arch sigstore verification Signed-off-by: Qi Wang <qiwan@redhat.com>
QiWang19
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/daemon/daemon.go (1)
1241-1242: Make fallback logs explicit about which capability failed.The current message merges two failure modes. Logging both predicates improves triage when firstboot falls back to container mode.
🔎 Suggested refactor
- if !newEnough || !skopeoSupportsMultiArchSigstore() { - logSystem("rpm-ostree or skopeo is not new enough for new-format image; forcing an update via container and queuing immediate reboot") + skopeoSupports := skopeoSupportsMultiArchSigstore() + if !newEnough || !skopeoSupports { + logSystem("forcing update via container and queuing immediate reboot (rpm-ostree-container-feature=%t, skopeo-multiarch-sigstore=%t)", newEnough, skopeoSupports)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/daemon/daemon.go` around lines 1241 - 1242, The fallback log message in daemon.go currently conflates two checks; update the logSystem call in the branch guarded by newEnough and skopeoSupportsMultiArchSigstore() to explicitly state which capability failed by referencing the boolean newEnough and the predicate skopeoSupportsMultiArchSigstore(), e.g., include both conditions or their values in the message so the log indicates whether rpm-ostree is too old, skopeo lacks multi-arch sigstore support, or both; modify the logSystem invocation accordingly in the same conditional block where it currently emits the combined message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/daemon/daemon.go`:
- Around line 1239-1240: Update the comment that begins "If Skopeo used by
rpm-ostree..." to correct the typo and reverse the meaning: state that older
skopeo versions do NOT support multi-arch Sigstore verification (replace
"supporting" with "do not support" and fix "verificaiton" to "verification"), so
the comment accurately explains why we fall back to running a privileged
container. Keep the rest of the context and links unchanged.
---
Nitpick comments:
In `@pkg/daemon/daemon.go`:
- Around line 1241-1242: The fallback log message in daemon.go currently
conflates two checks; update the logSystem call in the branch guarded by
newEnough and skopeoSupportsMultiArchSigstore() to explicitly state which
capability failed by referencing the boolean newEnough and the predicate
skopeoSupportsMultiArchSigstore(), e.g., include both conditions or their values
in the message so the log indicates whether rpm-ostree is too old, skopeo lacks
multi-arch sigstore support, or both; modify the logSystem invocation
accordingly in the same conditional block where it currently emits the combined
message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fae61a2d-3ff6-4259-9789-a07166a83738
📒 Files selected for processing (2)
pkg/daemon/daemon.gopkg/daemon/update.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/daemon/update.go
Comment on lines
+1239
to
+1240
| // If Skopeo used by rpm-ostree is an older version supporting multi-arch Sigstore verificaiton, run as a privileged container which has updated skopeo. | ||
| // See https://redhat.atlassian.net/browse/OCPBUGS-83826 and https://redhat.atlassian.net/browse/OCPBUGS-81187 |
There was a problem hiding this comment.
Fix fallback comment wording (reversed meaning + typo).
The comment currently says older skopeo versions are “supporting” multi-arch Sigstore verification and has a typo (“verificaiton”). It should say older versions do not support it.
✏️ Proposed comment fix
- // If Skopeo used by rpm-ostree is an older version supporting multi-arch Sigstore verificaiton, run as a privileged container which has updated skopeo.
+ // If skopeo used by rpm-ostree is an older version not supporting multi-arch Sigstore verification,
+ // run as a privileged container which has updated skopeo.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // If Skopeo used by rpm-ostree is an older version supporting multi-arch Sigstore verificaiton, run as a privileged container which has updated skopeo. | |
| // See https://redhat.atlassian.net/browse/OCPBUGS-83826 and https://redhat.atlassian.net/browse/OCPBUGS-81187 | |
| // If skopeo used by rpm-ostree is an older version not supporting multi-arch Sigstore verification, | |
| // run as a privileged container which has updated skopeo. | |
| // See https://redhat.atlassian.net/browse/OCPBUGS-83826 and https://redhat.atlassian.net/browse/OCPBUGS-81187 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/daemon/daemon.go` around lines 1239 - 1240, Update the comment that
begins "If Skopeo used by rpm-ostree..." to correct the typo and reverse the
meaning: state that older skopeo versions do NOT support multi-arch Sigstore
verification (replace "supporting" with "do not support" and fix "verificaiton"
to "verification"), so the comment accurately explains why we fall back to
running a privileged container. Keep the rest of the context and links
unchanged.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
uses deploy-from-self when skopeo is actually older than 1.22.2 that has issue
with multi-arch sigstore verification
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit