Change bundle verification test to not depend on signing

[steiza]

For #81

Summary

Today, https://github.com/sigstore/sigstore-conformance/blob/main/test/test_bundle.py requires a client to implement signing in order to test bundle verification. It would be nice to be able to test verification without requiring the client to implement signing.

Release Note

NONE

Documentation

N/A

[woodruffw]

Sorry for the delay here! My plan is to merge #85, then this 🙂

[di]

@woodruffw bump on this now that #85 has been merged!

[woodruffw]

Selftest is failing here, possibly because this needs a rebase. @steiza mind merging/rebasing here? 🙂

[woodruffw]

Hmm, not sure why these are still failing. I'll be able to take a look later, but also CCing @tetsuo-cpp for opinions.

[tetsuo-cpp]

Hmm, not sure why these are still failing. I'll be able to take a look later, but also CCing @tetsuo-cpp for opinions.

That's odd! I just re-ran CI against main and it's also failing so it's definitely not related to this PR. Not sure what's changed in the past few weeks though since it passed when we cut the last sigstore-conformance release...

[steiza]

I think I've identified what's going on, although I'm not sure what the right next step is to correct it.

I'm using the 2.0.0rc1 of sigstore-python:

steiza/sigstore-conformance$ sigstore --version
sigstore 2.0.0rc1

I ran just the bundle verification (with signing skipped - so we don't need a valid identity-token) to get the full command that was failing:

steiza/sigstore-conformance$ pytest test/test_bundle.py --entrypoint /Users/steiza/code/steiza/sigstore-conformance/sigstore-python-conformance --identity-token asdf --skip-signing
...
E               subprocess.CalledProcessError: Command '['/Users/steiza/code/steiza/sigstore-conformance/sigstore-python-conformance', 'verify-bundle', '--bundle', PosixPath('a.txt.sigstore'), '--certificate-identity', 'https://github.com/sigstore-conformance/extremely-dangerous-public-oidc-beacon/.github/workflows/extremely-dangerous-oidc-beacon.yml@refs/heads/main', '--certificate-oidc-issuer', 'https://token.actions.githubusercontent.com', PosixPath('a.txt')]' returned non-zero exit status 1.

I ran just that command locally:

steiza/sigstore-conformance$ /Users/steiza/code/steiza/sigstore-conformance/sigstore-python-conformance verify-bundle --bundle test/assets/a.txt.sigstore --certificate-identity 'https://github.com/sigstore-conformance/extremely-dangerous-public-oidc-beacon/.github/workflows/extremely-dangerous-oidc-beacon.yml@refs/heads/main' --certificate-oidc-issuer 'https://token.actions.githubusercontent.com' --offline test/assets/a.txt
An issue occurred while parsing the verification materials.

The provided verification materials are malformed and may have been
modified maliciously.

Additional context:

expected checkpoint in inclusion proof
...

It looks like that functionality landed in https://github.com/sigstore/sigstore-python/pull/634/files, which was between the sigstore-python 1.1.2 and 2.0.0rc1 releases.

... but again, I'm not sure what the right next step is. Where should the checkpoint information be coming from? Or maybe sigstore-python expecting something that really should be optional?

[di]

Moved this to #88 since it's unrelated to this PR.

[steiza]

There's quite a bit going on here, but I think I understand one of the failing bundle tests:

______________________ test_verify_rejects_invalid_digest ______________________
...
>       with pytest.raises(CalledProcessError):
E       Failed: DID NOT RAISE <class 'subprocess.CalledProcessError'>

/home/runner/work/sigstore-conformance/sigstore-conformance/test/test_bundle.py:130: Failed

This might have started passing the test (by failing verification) because of the lack of inclusion proof checkpoint. It seems to be a re-manifestation of #84 (comment):

steiza/sigstore-conformance$ sigstore verify github --bundle test/assets/a.txt.invalid_digest.sigstore --cert-identity https://github.com/sigstore-conformance/extremely-dangerous-public-oidc-beacon/.github/workflows/extremely-dangerous-oidc-beacon.yml@refs/heads/main --offline test/assets/a.txt
OK: test/assets/a.txt

[steiza]

I rebased to bring in changes from #89, but it looks like I have some more debugging to do.

[woodruffw]

Looks like it might be an isolation problem:

E           usage: sigstore [-h] [-V] [-v] [--staging] [--rekor-url URL]
E                           [--rekor-root-pubkey FILE]
E                           COMMAND ...
E           sigstore: error: Refusing to overwrite outputs without --overwrite: a.txt.sigstore

[woodruffw]

This one is interesting:

client = <test.client.SigstoreClient object at 0x7f8108875310>
make_materials_by_type = <function make_materials_by_type.<locals>._make_materials_by_type at 0x7f810[88](https://github.com/sigstore/sigstore-conformance/actions/runs/5693019578/job/15432496745?pr=82#step:5:91)43b00>

    def test_verify_rejects_invalid_digest(
        client: SigstoreClient, make_materials_by_type: _MakeMaterialsByType
    ) -> None:
        """
        Check that the client rejects a bundle with a modified digest.
        """
    
        materials: BundleMaterials
        input_path, materials = make_materials_by_type("a.txt", BundleMaterials)
        materials.bundle = Path("a.txt.invalid_digest.sigstore")
    
>       with pytest.raises(ClientFail):
E       Failed: DID NOT RAISE <class 'test.client.ClientFail'>

I need to check now, but I'm pretty sure sigstore-python ignores the digest in the bundle entirely (we always reconstruct it from the user's input). If the client spec says that we have to check it though, then this is probably our bug and I'll issue a fix (I'll also check on that now) 🙂

[woodruffw]

Hmm, the client spec itself doesn't mention the bundle's contained digest at all (more generally, it's pretty sparse on the bundle as an input format, although I believe that's going to change with sigstore/sig-clients#8).

On the protobuf-specs side, this is what we have:

// MessageSignature stores the computed signature over a message.
message MessageSignature {
        // Message digest can be used to identify the artifact.
        HashOutput message_digest = 1 [(google.api.field_behavior) = REQUIRED];
        // The raw bytes as returned from the signature algorithm.
        // The signature algorithm (and so the format of the signature bytes)
        // are determined by the contents of the 'verification_material',
        // either a key-pair or a certificate. If using a certificate, the
        // certificate contains the required information on the signature
        // algorithm.
        // When using a key pair, the algorithm MUST be part of the public
        // key, which MUST be communicated out-of-band.
        bytes signature = 2 [(google.api.field_behavior) = REQUIRED];
}

So the message_digest is marked required, but we only have a weak "can" on its use. But perhaps we should strengthen that? CC @znewman01 @haydentherapper

[haydentherapper]

may seems accurate, I don't think it needs to be a requirement for the client to use this field. Do you recall recall why we made it required?

[woodruffw]

Do you recall recall why we made it required?

Nope, no recollection. I recall arguing against including the digest at all originally, since (IMO) it's a probable source of implementation errors (since it's a piece of dual state that always needs to be checked against the actual input that the user if verifying). IIRC the justification for including it was that some users may wish to "verify" bundles with just their intrinsic digest, although I'm still a little blurry on what doing so would accomplish (since it's effectively verifying a signature over anything, not a particular expected input.)

[woodruffw]

As a result, we're in a bit of a funny state here: arguably users shouldn't ever need to check message_digest (and doing so possibly indicates a logic error during verification), but we also have it marked as required. Consequently, signers may end up putting all kinds of incorrect stuff in their field, and most verifiers would probably never notice. This is arguably not a big deal (since it doesn't affect verification in any way), but perhaps we should check it anyways even if we don't actually use it? I'm not sure.

[haydentherapper]

I'd be fine with either dropping the message or changing to optional.

[woodruffw]

I'd be fine with either dropping the message or changing to optional.

Yeah, I think changing it to optional makes sense for now. IMO it really shouldn't be in the bundle at all (especially since we don't have a concrete use case for it, per sigstore/protobuf-specs#30 (comment)), but removing it would be a relatively disruptive protobuf change unlike our other (compatible) changes made between 0.1 and 0.2.

I'll make a PR for that in a second.

[woodruffw]

Opened sigstore/protobuf-specs#114 for that tweak.

[woodruffw]

LGTM, thanks @steiza! Especially for bearing with the test pain here 🙂