Skip to content
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

Include skipped signatures in VerificationResult #48

Open
haydentherapper opened this issue Dec 15, 2023 · 3 comments
Open

Include skipped signatures in VerificationResult #48

haydentherapper opened this issue Dec 15, 2023 · 3 comments
Labels
enhancement New feature or request v1.0 items we want to consider for a v1.0 release

Comments

@haydentherapper
Copy link
Contributor

Description

#47 and #45 introduce skipping log and TSA signatures respectively that the trust bundle cannot verify. This information is not passed back to the verifier, the signatures are just silently skipped over.

We can update VerificationResults to pass this information back, which could be helpful for debugging that the bundle contains the expected trust root material.

@haydentherapper
Copy link
Contributor Author

We might also want to split out parsing errors from verification errors. For example, for timestamp verification, we will skip a timestamp either if it is an invalid RFC3161 structure or if verification fails.

@haydentherapper haydentherapper added the v1.0 items we want to consider for a v1.0 release label Jun 5, 2024
ggrangel added a commit to ggrangel/sigstore-go that referenced this issue Jul 20, 2024
Addresses sigstore#48. Instead of just ignoring the signatures the trust bundle
couldn't verify, we want to pass this information back to the caller.

To avoid API changes, in order to detect the unsinged entries, for each
signature type, we loop through all available signatures and mark the
ones the weren't verified.

Signed-off-by: Gustavo Rangel <[email protected]>
@haydentherapper
Copy link
Contributor Author

At the sigstore-go meeting, we discussed that VerificationResult currently contains metadata that is verified. @phillmv pointed out that this struct could be passed to a policy controller for further evaluation. Adding a field like UnverifiedResults into this struct might mislead verifiers.

@dmitris
Copy link
Contributor

dmitris commented Oct 22, 2024

I had a related experience and asked a question on Slack:

a question about this error handling instance:
https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/tsa.go#L113-L116
		// Ensure timestamp responses are from trusted sources
		timestamp, err := tsaverification.VerifyTimestampResponse(signedTimestamp, bytes.NewReader(dsseSignatureBytes), trustedRootVerificationOptions)
		if err != nil {
			continue
		}
don't we want to expose / return those errors in some form?  For example, I'm now struggling with an ASN.1 parsing error  from [github.com/digitorus/timestamp.ParseResponse](http://github.com/digitorus/timestamp.ParseResponse) (called from [github.com/sigstore/timestamp-authority/pkg/verification.VerifyTimestampResponse](http://github.com/sigstore/timestamp-authority/pkg/verification.VerifyTimestampResponse)):

error parsing response into Timestamp: asn1: structure error: tags don't match (16 vs {class:1 tag:13 length:73 isCompound:false}) {optional:false explicit:false application:false private:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false} response @2

but I had to run the debugger to see this error - otherwise all that is "exposed" to the user is what seems somewhat cryptic error:
failed to verify timestamps: threshold not met for verified signed & log entry integrated timestamps: 0 < 1
maybe there could be at least some verbose or even debug flag that one can enable (or propagate the option) for easier troubleshooting and error overview, before firing up the debugger

[tsa.go](https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/tsa.go)
        timestamp, err := tsaverification.VerifyTimestampResponse(signedTimestamp, bytes.NewReader(dsseSignatureBytes), trustedRootVerificationOptions)
        if err != nil {
            continue
        }

@haydentherapper responded there:

We do want to surface the errors, but within each verification, we want to ignore anything that doesn't properly verify - the challenge is separating what we can't verify because it's malformed vs what we don't trust, so we just skip over any failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v1.0 items we want to consider for a v1.0 release
Projects
None yet
Development

No branches or pull requests

2 participants