Bug 2071998: pkg/verify: Expose underlying signature errors #1358
Conversation
Sometimes we fail to verify because we find signatures we don't trust. Sometimes we fail to find signatures at all, either because the backing stores have no signatures, or because we fail to contact the backing stores. With this commit, we pass all that useful debugging context on to the caller. Not in the string, because we don't want to overwhelm all callers, but they can get it via Unwrap [1] so they can show the details where they have space to do so. [1]: https://pkg.go.dev/errors#Unwrap
wking
changed the title
wking
force-pushed
the
expose-underlying-signature-errors
branch
from
1ca5837 to
1b9753d
Compare
openshift-ci
bot
added
the
approved
wking
changed the title
openshift-ci
bot
added
bugzilla/severity-high
|
@wking: This pull request references Bugzilla bug 2071998, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions 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/test-infra repository. |
|
@wking: 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/test-infra repository. I understand the commands that are listed here. |
| err error | ||
| } | ||
|
|
||
| func (e *wrapError) Error() string { |
There was a problem hiding this comment.
wrapError is from Go, because I wanted fmt.Sprintf("...%w")'s Unwrap support, but without formatting the whole, possibly long aggregate error into the message. We need to use Error and Unwrap to match the error interface.
| } | ||
| return fmt.Errorf("unable to locate a valid signature for one or more sources") | ||
| err := &wrapError{ | ||
| msg: fmt.Sprintf("unable to verify %s against keyrings: %s", releaseDigest, strings.Join(remainingKeyRings, ", ")), |
There was a problem hiding this comment.
Looks like we are logging the keys too. I am wondering if it is fine from security point of view>
There was a problem hiding this comment.
we're logging the keyring name, which is already public in our GitHub source. The only secret keys here are internal to Red Hat's build-time release signing. By the time we get out to clusters verifying signatures, it's all public names and public keys.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LalatenduMohanty, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@wking: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with Bugzilla bug 2071998 has not been moved to the MODIFIED state. DetailsIn response to this:
Instructions 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/test-infra repository. |
Picking up openshift/library-go@1b9753d298 (Bug 2071998: pkg/verify: Expose underlying signature errors, 2022-04-21, openshift/library-go#1358). Generated with: $ go get -u github.com/openshift/library-go go: downloading github.com/openshift/library-go v0.0.0-20220421210707-c1f281b7efaa go: downloading github.com/prometheus/client_golang v1.11.1 go get: upgraded github.com/openshift/library-go v0.0.0-20220407182450-db47826e7275 => v0.0.0-20220421210707-c1f281b7efaa go get: upgraded github.com/prometheus/client_golang v1.11.0 => v1.11.1 $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.17.3 linux/amd64 [1]: openshift/library-go#1358
Extending 1b9753d (pkg/verify: Expose underlying signature errors, 2022-04-21, openshift#1358) with timestamps, so it's easy to see what's slow in situations like [1] where some portion of signature verification is surprisingly slow, and it's currently not clear what aspect is causing the slowdown. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2071998#c2
Extending 1b9753d (pkg/verify: Expose underlying signature errors, 2022-04-21, openshift#1358) with information about when store retrieval is exhausted, so it's easy to see what's slow in situations like [1] where some portion of signature verification is surprisingly slow, and it's currently not clear what aspect is causing the slowdown. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=2071998#c2
Picking up openshift/library-go@1b9753d298 (Bug 2071998: pkg/verify: Expose underlying signature errors, 2022-04-21, openshift/library-go#1358) and openshift/library-go#1371. Generated with: $ go get -u github.com/openshift/library-go $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.17.3 linux/amd64
3 participants
Sometimes we fail to verify because we find signatures we don't trust. Sometimes we fail to find signatures at all, either because the backing stores have no signatures, or because we fail to contact the backing stores. With this commit, we pass all that useful debugging context on to the caller. Not in the string, because we don't want to overwhelm all callers, but they can get it via
Unwrapso they can show the details where they have space to do so.