Fix and re-enable test/extended/images/signatures.go #25880
Conversation
mtrmac
force-pushed
the
verify-signature-over-the-wall
branch
from
c934c04 to
720b931
Compare
|
/assign |
mtrmac
force-pushed
the
verify-signature-over-the-wall
branch
from
720b931 to
8f511a8
Compare
test/extended/images/signatures.go
Outdated
| // Newer versions of Skopeo default to X-R-S-S if present, this test (as of 2020-02) uses skopeo-0.1.40-11.el7_8.x86_64, which defaults to sigstore. | ||
| "--registries.d", "/this/does/not/exist", | ||
|
|
||
| "copy", "--sign-by", "[email protected]", | ||
| "--dest-creds=" + user + ":" + token, | ||
| // TODO: test with this turned to true as well | ||
| "--dest-tls-verify=false", | ||
| "docker://docker.io/library/memcached:latest", |
There was a problem hiding this comment.
Is this external registry reference OK to include, or is there a recommended set of other testing images to use nowadays?
There was a problem hiding this comment.
Given docker rate-limiting and the requirement to run these tests offline, I'd encourage to pick something from these https://github.com/openshift/origin/blob/master/test/extended/util/image/zz_generated.txt
There was a problem hiding this comment.
It's better to use test/extended/util/image.ShellImage() unless you need a specific image.
There was a problem hiding this comment.
The only possible concern with an internal image is that the registry has a limited number of signature slots (per image, not per ISTag). Updated, let’s see.
There was a problem hiding this comment.
That breaks, apparently because ShellImage is docker://image-registry.openshift-image-registry.svc:5000/openshift/tools:latest (and that is accessible using the users’ token), but the deduplicated image (same image ID) has dockerImageReference:registry.build01.ci.openshift.org/ci-op-$private/stable@sha256:7932f3a766ddd6a0ee541fef4f40e51e440c9b7bfcf0f138023f6b2efd8cec74 , and that reference to the same underlying image) seems not to be readable the same same token.
We don’t need any specific image but we need an unique image that is not deduplicated to an unaccessible repo — or extra privileges to read the Image.dockerImageReference repo.
There was a problem hiding this comment.
The test user should have a system-image:auditor role, and thus access to all in-cluster images.
If I understand the failure ( https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/25880/pull-ci-openshift-origin-master-e2e-aws-image-registry/1359960263960301568 ) correctly (which I’m not at all sure about), ShellImage() is an ImageStreamTag referring to registry.build01.ci.… (which is external to the cluster, is that right?), with referencePolicy.type: Source. So, users of ShellImage are expected to contact an external registry directly, using some appropriate credentials.
But https://github.com/openshift/oc/blob/9f54c1d4f68c8530ac9466c655a4e55eb04a1459/pkg/cli/admin/verifyimagesignature/verify-signature.go#L264 blindly passes the current OpenShift token to any registry, even if remote. That looks rather incorrect?!
mtrmac
force-pushed
the
verify-signature-over-the-wall
branch
from
8f511a8 to
dabf4b7
Compare
|
/retest |
mtrmac
force-pushed
the
verify-signature-over-the-wall
branch
2 times, most recently
from
f3b2b92 to
270a954
Compare
|
/hold The current version passes, by signing the just-built |
openshift-ci-robot
added
the
do-not-merge/hold
|
/retest |
|
@mtrmac it looks like the current approach is viable and works, any objections to merging this as is or you want time to resolve the issue you've mentioned in #25880 (comment) ? |
|
I’m fine with re-enabling the test as is, and dealing with that bug (if it is a bug) separately; I wanted to draw attention to the failure to confirm that I’m interpreting it correctly. |
openshift-ci-robot
removed
the
do-not-merge/hold
openshift-ci-robot
added
lgtm
|
/retest Please review the full test history for this PR and help us cut down flakes. |
8 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
- Use docker:// instead of atomic:, as recommended back in openshift#21782 (comment) openshift#21782 (comment) Signed-off-by: Miloslav Trmač <[email protected]>
Don't read the image to be signed from docker.io. Instead, sign the just-built signer image (which is unique, so there always should be enough free signature slots left). Send the required credentials to the source registry. Also use the injected service CA instead of disabling TLS. I don't know whether it is supposed to work like that (per https://github.com/openshift/openshift-docs/blob/enterprise-4.1/release_notes/ocp-4-1-release-notes.adoc#service-ca-bundle-changes the path is deprecated) but the same path is already assumed to exist by the preceding (oc login). Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
verify-signature-over-the-wall
branch
from
8f197c8 to
922ee64
Compare
|
Updated just strings/commits, no logic:
|
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
docker://instead ofatomic:, as recommended back in remove oapi #21782 (comment) remove oapi #21782 (comment)