Use default Verifier for the public key contained in a certificate (closes #74)

[ret2libc]

Summary

Closes #74

Release Note

• use default verifier based on public key type, according to the algorithm registry in sigstore/sigstore (https://github.com/sigstore/sigstore/blob/main/pkg/signature/algorithm_registry.go)

Documentation

• ed25519 public keys will use by default the ed25519ph algorithm

[kommendorkapten]

Thanks @ret2libc, could you run go mod tidy then push again?

[ret2libc]

Thanks @ret2libc, could you run go mod tidy then push again?

@kommendorkapten fixed!

[kommendorkapten]

Thanks!

[ret2libc]

@kommendorkapten can you approve the the workflows?

[kommendorkapten]

Can you also update the title to refer to the issue: #74 (I believe that's the correct one).

[kommendorkapten]

The conformance test is failing, and the version was updated in main a few days ago, would you mind rebasing your changes on main so we can rule out that as the cause for the error?

[ret2libc]

The conformance test is failing, and the version was updated in main a few days ago, would you mind rebasing your changes on main so we can rule out that as the cause for the error?

I think it was already updated, but I re-did anyway and it's still failing :(

What's weird is that this change shouldn't really affect most common scenarios as nothing changes for ecdsa-p256. Moreover, I tested sigstore-conformance locally and I don't see any error here.

[ret2libc]

I think I understand now. All errors are in test_verify_cpython_release_bundles and that test is disabled locally. I'll dig into that.

[kommendorkapten]

Hm, I ran this (before the rebase)

kommendorkapten@m1m14-msft:~/git/sigstore-go % git merge-base --fork-point main
4c441c3cff4cbfb3e96d9a7703670d8ddec1628c

And that commit was before the merge in main, but anyway. It's still failing :(

The test is successful for me locally too, but there are some extra work that has to be done locally to run the tests that fails (verify cpython) that requires a clone of https://github.com/woodruffw/cpython-release-tracker IIRC. I can spend some time after lunch to see if I can get conformance test running properly locally. You can some references here https://github.com/sigstore/sigstore-conformance/blob/main/action.yml#L37

[ret2libc]

Hm, I ran this (before the rebase)

kommendorkapten@m1m14-msft:~/git/sigstore-go % git merge-base --fork-point main
4c441c3cff4cbfb3e96d9a7703670d8ddec1628c

And that commit was before the merge in main, but anyway. It's still failing :(

The test is successful for me locally too, but there are some extra work that has to be done locally to run the tests that fails (verify cpython) that requires a clone of https://github.com/woodruffw/cpython-release-tracker IIRC. I can spend some time after lunch to see if I can get conformance test running properly locally. You can some references here https://github.com/sigstore/sigstore-conformance/blob/main/action.yml#L37

Yes indeed! I just found out about it and I was able to reproduce locally. There are some non-p256 keys there :) Thanks

[ret2libc]

The problem now is that this is essentially a breaking change: by using a given hash for a given key type (instead of using sha256 everywhere), we cannot verify anymore blobs signed by older cosign with a key like ecdsa-384.

ECDSA-P384 now expects a SHA384, but until now we just used SHA256. Thus the errors.

AFAIS there are a bunch of tests in sigstore-conformance that uses ECDSA-P384 (no other non-standard keys). This was discussed previously and I was under the impression we were fine with the breakage. It would require re-signing all blobs with the new cosign (sigstore/cosign#4050).

Just to re-iterate, the options are:

1. fix the sigstore-conformance tests so that ECDSA-P384 uses SHA384. We communicate this properly to users as a breaking change. This happens only for the BYOK case, because by default cosign uses ECDSA-P256 and we are not aware of other sigstore clients that allow BYOK.
2. have a compatibility layer in sigstore-go. We return a SignerVerifier that first performs verification with the new default algorithm, if that fails it fallbacks to use the older algorithm. This can also be controlled with a policy. At some point later we disable compatibility.

cc @haydentherapper @woodruffw

[kommendorkapten]

Thanks for the summary @ret2libc

My vote would be to fix conformance test so that P-384 uses SHA-384 to avoid confusion, and look at P-384/SHA-256 as a legacy construct.

If needed, we could bump the conformance version to indicate that his is a potentially breaking change, to let other clients update in their pace.

[ret2libc]

Apparently the assumption that ECDSA-384 was not too much used might be wrong. Parts of the conformance tests use python executables and sigstore bundles (e.g. https://www.python.org/ftp/python/3.10.1/) and those .sigstore files have a ECDSA-P384 public key inside, but they use SHA256.

Not supporting ECDSA-P384 + SHA256 means people won't be able to verify those python binaries anymore or they need to be re-generated (I'm still not sure how those are generated). This affects all python releases after ~3.10.

For reference:

• https://peps.python.org/pep-0761/
• https://www.python.org/downloads/metadata/sigstore/

[haydentherapper]

@ret2libc thanks for digging into this. Can we go with the previous proposal to support p384/521 paired with sha256 as a deprecated key type?

If we can support this on the verification path, but use the correct pairing on the sign path, this won’t be breaking. The tricky part is how to do verification - we can try multiple key types, albeit that’s a bit sketchy.

The bundle can contain a key hint, could we also add which algorithm is used? Then we can update all cpython bundles.

Worst case, we can just support this old mapping and deprecate it with major releases across all clients later on.

[haydentherapper]

Another thought - maybe we can treat keys vs certs differently. I doubt there are many self provided keys that also requested certificates.

[kommendorkapten]

I was thinking about this a bit, and I think we can do this in a compatible and non-sketchy/brittle way:

That seems reasonable I think. However, as we are standardizing on a set of supported algorithms in the bundle spec, can we at least make sure that this fallback scheme is only valid for bundles up v5? That would make me feel a bit better.

[woodruffw]

That seems reasonable I think. However, as we are standardizing on a set of supported algorithms in the bundle spec, can we at least make sure that this fallback scheme is only valid for bundles up v5? That would make me feel a bit better.

Same 🙂 -- I suppose that would prevent infallible automatic upgrades of older bundles to newer versions, but IMO that's not a huge issue.

[ret2libc]

If we can support this on the verification path, but use the correct pairing on the sign path, this won’t be breaking. The tricky part is how to do verification - we can try multiple key types, albeit that’s a bit sketchy.

I was thinking about this a bit, and I think we can do this in a compatible and non-sketchy/brittle way:

1. Perform the "normal" pairing verification flow: P256+SHA256, P384+SHA384, etc.
2. If the key/cert conveys a P384 key but the SHA384 image doesn't verify, then additionally compute the SHA256 image and attempt verification with that.

This should be doable.

That seems reasonable I think. However, as we are standardizing on a set of supported algorithms in the bundle spec, can we at least make sure that this fallback scheme is only valid for bundles up v5? That would make me feel a bit better.

I'll investigate what's needed for that!

[haydentherapper]

LGTM to all of the above.

Can we review if there's any service changes we need to make or revert with this? I just checked Rekor, and it looks like since we aren't loading in any default hashes, we'll continue to use sha256, so clients can continue to upload key-signed artifacts to Rekor. (I was thinking about this in the context of the Cosign change - currently you can upload an artifact to Rekor whose digest is sha512, but I think the artifact must be signed using sha256)

[ret2libc]

Ok this seems to work. I've implemented fallbacks for P384/P521, but right now it's always enabled. If we want to do this compatibility check only for some artifacts, we need a way to propogate the bundle version somehow (not sure there is one already).

Moreover, shall we update the client spec to mention the fallback mechanisms?

[ret2libc]

In order to enable/disable the fallback logic based on the bundle version, I had to expose a Version method in SignedEntity. This way the logic is not policy-controlled, but just controlled based on the entity being verified.

[haydentherapper]

Overall this LGTM but I'd want another maintainer to chime in as well

[haydentherapper]

This would mean this fallback verifier is only valid for hashedrekord and not DSSE, yes? We should document if so

[ret2libc]

to be honest there's no runtime check or anything in place to enforce that this is done only for hashedrekord. It will just use pre-hashed every time there is a ed25519 key no matter what. Shall we restrict the behaviour to just hashedrekord?

[haydentherapper]

Can we pass the type in and set whether it’s the PH variant or not based on the type? There could also be DSSE bundles with the older mapping that we should support.

[kommendorkapten]

For the future, this should be exposed as a policy option I think. But that's for the future when we have v0.4 bundles 🙂

[ret2libc]

I initially exposed it as an option, but then every sigstore-go user (well, maybe there's only cosign, I'm not sure) would have to implement the bundle version check+policy enable/disable by themselves. In this way, instead, v0.4 bundles will never use the fallback logic, while v0.3 will. This is required if clients still want to verify older signatures, I think.

[kommendorkapten]

That's fair. And to prevent downgrade attacks, the Bundle.MinVersion check can be exposed as a verification option WithMinBundleVersion to safeguard that no older bundles can be verified if a user want to ensure only non deprecated algorithms are used.

[kommendorkapten]

Nice job, thank you!

[steiza]

TIL that Go booleans default to false

[haydentherapper]

Looks great! Just one thought on supporting dsse

[haydentherapper]

tagged https://github.com/sigstore/sigstore/releases/tag/v1.9.2

[haydentherapper]

Can we pass the type in and set whether it’s the PH variant or not based on the type? There could also be DSSE bundles with the older mapping that we should support.