-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1461324 - Log image changes on verify-image-signature without --save #19976
Bug 1461324 - Log image changes on verify-image-signature without --save #19976
Conversation
@wozniakjan maybe it would be worth updating the image signature docs to warn people about this behavior? |
@bparees it actually says it, roughly in the middle of that page "Using the --save flag on already verified image together with invalid GPG key or invalid expected identity causes the saved verification status to be removed, and the image will become unverified." which is similar to the information the origin/pkg/oc/admin/image/verify-signature.go Lines 45 to 47 in 5e0bfba
@bmcelvee do you think maybe a red exclamation mark would be appropriate here? https://github.com/openshift/openshift-docs/blob/master/admin_guide/image_signatures.adoc |
@wozniakjan that's not the same as removing the (unverified) signatures which is what happens, right? |
@wozniakjan I agree, it really couldn't hurt to call that line out in an "important" box. As is it's easy for a reader to gloss over it and run into trouble. I can open a docs PR now, or wait to see if additional docs updates are required for this PR. |
That may very well be. I read the code first and then tried to see if I can find corresponding information in docs and probably made an assumption a little to early. I don't think the code discriminates between verified/unverified signature, it removes all signatures if origin/pkg/oc/admin/image/verify-signature.go Lines 211 to 218 in 5e0bfba
@bmcelvee PR emphasizing the information in "important" box would be great I think, maybe also explicitly stating |
@wozniakjan is this still WIP? |
@bparees thanks for the reminder, removing the WIP. Wasn't entirely sure whether docs update or more verbose logging, and in the end, both might complement each other well |
yeah i think both are good. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, wozniakjan 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 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. |
https://bugzilla.redhat.com/show_bug.cgi?id=1461324