Initializing client regression between v0.5.2 and v0.6.0

[jonjohnsonjr]

I am unsure exactly what caused this (and the issue may be in sigstore, but that doesn't seem to have changed), but calling this function:

https://github.com/sigstore/sigstore/blob/8fac32e450fa2fb263313255dc779799095f0ce5/pkg/tuf/client.go#L335-L349

Gives this error:

initializing tuf: unable to initialize client, local cache may be corrupt: tuf: error unmarshalling key: invalid PEM value:

Whereas reverting to v0.5.2 suceeds.

[asraa]

Are you using an updated sigstore client (or cleared the local cache?) this is because before GA sigstore was using an invalid format root that wasn't compatible with newer versions of go-tuf.

[jonjohnsonjr]

Are you using an updated sigstore client (or cleared the local cache?)

Great questions! I ran into this on GitHub actions where I assume there is no pre-existing cache, but that might be wrong.

before GA sigstore was using an invalid format root that wasn't compatible with newer versions of go-tuf.

I'm not entirely sure what this means. Do you have a link to more info?

[asraa]

If you have a link to the GHA it'd be easier to debug. Here's some links about that issue, and I'm not totally sure unless I see if it's that from your GHA..

Original issue that sigstore was using PEM encoded public keys in the root: https://github.com/sigstore/root-signing/issues/329 The fix in go-tuf: https://github.com/sigstore/root-signing/pull/375 Sigstore migrating to the spec-compliant format: https://github.com/sigstore/root-signing/pull/380

I swear there were posted issues of clients hitting this, but maybe it was only in Slack.

[hectorj2f]

@asraa I can see a similar issue in policy-controller https://github.com/sigstore/policy-controller/actions/runs/5677821039/job/15386817220?pr=903#step:5:81.

[asraa]

Oh! Weird, if this is popping up recently in daily runs then I have no idea. @haydentherapper for any context if something has changed.

[asraa]

@haydentherapper probably this needs to be bumped to a more recent version, and deps updated... https://github.com/sigstore/sigstore/blob/main/pkg/tuf/repository/root.json

[haydentherapper]

Is https://github.com/sigstore/cosign/pull/3128 related?

[haydentherapper]

We hadn't updated to 0.6.0 yet cause I wasn't sure where the KDF params change would break things.

[haydentherapper]

We have this failure here too - https://github.com/sigstore/sigstore/pull/1286

[trishankatdatadog]

We hadn't updated to 0.6.0 yet cause I wasn't sure where the KDF params change would break things.

Cc @rdimitrov

[trishankatdatadog]

Maybe we should add a Sigstore maintainer also to go-tuf to try to prevent this sort of issues going forward...

[jonjohnsonjr]

If you have a link to the GHA it'd be easier to debug.

Others have linked to more failures, but here's mine: https://github.com/chainguard-dev/terraform-provider-cosign/actions/runs/5673744402/job/15375885559

Thanks for the links, I'll take a look.

[rdimitrov]

As far as I understand the changes to the encrypted package (KDF update/what we notify as a potential breaking change) are not what’s causing this since the sigstore use case of NewFromEnv does not encrypt/decrypt anything through it, keys included. update: I was too fast to say that without a deeper look, so scratch that. Phone debugging is not helpful 🙈

In any case the sigstore unit tests seem to reproduce this issue so I’d suggest someone to debug running them locally by gradually testing which change in go-tuf might have caused this from 0.5.2 to 0.6.0.

PS. Unfortunately I’m in Denmark up until the end of next week so I cannot debug properly from my phone.

PS2. A few helping notes for someone that is willing to give it a try - the following would help replace go-tuf with a local clone for testing in sigstore’s go.mod- replace github.com/theupdateframework/go-tuf => <path-to-local-copy>. Gradually introducing the changes can be done through git cherry picking or interactive rebasing.

[rdimitrov]

I returned from PTO and debugged it with sigstore's unit tests today.

It turns out that the issue is caused by fix: Update the ecdsa key type to the latest spec (1.0.32). Reverting the commit 2adcfe74e69d474d298a991d9643bed13da055d7 does fix the failed tests in https://github.com/sigstore/sigstore.

I did not have more time, but I'll try to figure out the root cause next week. Feel free to chime in if you already have an idea.

cc: @asraa @kommendorkapten @hectorj2f @haydentherapper @jonjohnsonjr

[asraa]

If that's the case then I expect it's because we're parsing the old ecdsa key type like I mentioned above. I still think the fix is probably to update the sigstore/sigstore root metadata: https://github.com/sigstore/sigstore/blob/main/pkg/tuf/repository/root.json

I think the reason that commit shows up is now is because the key KeyTypeECDSA_SHA2_P256 changed underlying (which is a breaking change), meanwhile the deprecated key format changed to the key type KeyTypeECDSA_SHA2_P256_OLD_FMT. In reality, this line below should have also changed to KeyTypeECDSA_SHA2_P256_OLD_FMT to add the correct deprectaed ecdsa verifier to the store.

https://github.com/theupdateframework/go-tuf/blob/4e4f7f3c2772bf637d0ff9c524ddda5e6dd8dc79/pkg/deprecated/set_ecdsa/set_ecdsa.go#L22

So this fix is (1) to update the root metadata so we no longer are parsing the deprecated ecdsa format or (2) update the line above so that the deprecated ecdsa format is actually added to the store with the correct key value.

[haydentherapper]

@asraa, thanks, that makes perfect sense. I'd rather avoid continuing to deal with hex-encoded ecdsa keys, so I'm good to bump root.json to version 5 or the latest. Though I think we should do (2) also unless we're also entirely removing the deprecated ecdsa format.

[rdimitrov]

Isn't it better if we load the correct verifier here with NewDeprecatedEcdsaVerifier - https://github.com/theupdateframework/go-tuf/blob/4e4f7f3c2772bf637d0ff9c524ddda5e6dd8dc79/pkg/keys/ecdsa.go#L23

[asraa]

Isn't it better if we load the correct verifier here with NewDeprecatedEcdsaVerifier -

The deprecated package is separate and needs explicit inclusion (which it is in the sigstore ecosystem) - I think it would be also good if someone corrected the key here: https://github.com/theupdateframework/go-tuf/blob/4e4f7f3c2772bf637d0ff9c524ddda5e6dd8dc79/pkg/deprecated/set_ecdsa/set_ecdsa.go#L22 to match what @rdimitrov is saying above.

[rdimitrov]

I'll recap everything that needs to be changed. If we agree that's all, I'll open a fix so we can cut a patch release and resolve this quickly 👍

• Change key type to data.KeyTypeECDSA_SHA2_P256_OLD_FMT in https://github.com/theupdateframework/go-tuf/blob/4e4f7f3c2772bf637d0ff9c524ddda5e6dd8dc79/pkg/deprecated/set_ecdsa/set_ecdsa.go#L22
• Change the correct verifier NewDeprecatedEcdsaVerifier - https://github.com/theupdateframework/go-tuf/blob/4e4f7f3c2772bf637d0ff9c524ddda5e6dd8dc79/pkg/keys/ecdsa.go#L23

[asraa]

The first is correct!

The second does not need to be changed - this is the intended old ECDSA verifier (spec compliant). The deprecated package imported the old go-tuf not-spec-compliant ecdsa verifier.

[rdimitrov]

@asraa - Thanks! 💯 I'll make sure to open a PR later and hopefully cut a patch release in a day or so so we fix this 👍

[haydentherapper]

https://github.com/sigstore/sigstore/pull/1312 will also fix this for Sigstore.

For set_ecdsa, I think we need to store a mapping for both data.KeyTypeECDSA_SHA2_P256_OLD_FMT and data.KeyTypeECDSA_SHA2_P256 in case a client is verifying using both the old non-compliant format and a newly generated root.

[asraa]

For set_ecdsa, I think we need to store a mapping for both data.KeyTypeECDSA_SHA2_P256_OLD_FMT and data.KeyTypeECDSA_SHA2_P256 in case a client is verifying using both the old non-compliant format and a newly generated root.

We currently don't generate a root with data.KeyTypeECDSA_SHA2_P256 - this is a different key value - we generate with and old and new ecdsa verifier, but it's always the OLD_FMT key.

I would also suggest migrating to the new key type for the next root signing.

[haydentherapper]

That makes sense. I was thinking about cases where we must always support the outdated format and the new format, like in rekor (unless we bump the TUF type version).

[asraa]

I was thinking about cases where we must always support the outdated format and the new format

Yeah, i'm not totally sure it's possible, since it's meant to be a compile time setting :/ But you could internally access the verifiers, I guess.

[rdimitrov]

The fix is now part of v0.6.1 (just released) 🎉

Thank you all for your help! 💯