sigstore / sigstore-conformance

Conformance testing for Sigstore clients
https://sigstore.dev
7 stars 10 forks source link

Redo staging TUF root to not contain deprecated keys #140

Open steiza opened 5 months ago

steiza commented 5 months ago

Description

As clients add support for --staging, they will need to support the staging TUF root, which today includes a key of type PKCS1_RSA_PKCS1V5 which is deprecated in protobuf-specs v0.3.x.

As clients adopt protobuf-specs v0.3.x, instead of having them add support for a deprecated key type, we could re-make the staging root to not use deprecated keys. We probably do want at least 2 ctlog keys, one of which is outside of its valid time range, to ensure clients are properly verifying keys that were previously valid.

haydentherapper commented 5 months ago

SGTM. I would recommend we simply duplicate the key (example below), and instruct clients to ignore any keys whose keyDetails they do not understand so they can choose to drop support for the deprecated values. I'm not sure how clients are handling this currently, if they fail or simply ignore the keys - @woodruffw, what's python do for example?

If we choose to wipe the log, we'll modify the set of trusted keys later to remove the duplicate, or wait until all clients have been updated to support the new keyDetails values.

Extra key:

    {
      "baseUrl": "https://ctfe.sigstage.dev/test",
      "hashAlgorithm": "SHA2_256",
      "publicKey": {
        "rawBytes": "MIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEA27A2MPQXm0I0v7/Ly5BIauDjRZF5Jor9vU+QheoE2UIIsZHcyYq3slHzSSHy2lLj1ZD2d91CtJ492ZXqnBmsr4TwZ9jQ05tW2mGIRI8u2DqN8LpuNYZGz/f9SZrjhQQmUttqWmtu3UoLfKz6NbNXUnoo+NhZFcFRLXJ8VporVhuiAmL7zqT53cXR3yQfFPCUDeGnRksnlhVIAJc3AHZZSHQJ8DEXMhh35TVv2nYhTI3rID7GwjXXw4ocz7RGDD37ky6p39Tl5NB71gT1eSqhZhGHEYHIPXraEBd5+3w9qIuLWlp5Ej/K6Mu4ELioXKCUimCbwy+Cs8UhHFlqcyg4AysOHJwIadXIa8LsY51jnVSGrGOEBZevopmQPNPtyfFY3dmXSS+6Z3RD2Gd6oDnNGJzpSyEk410Ag5uvNDfYzJLCWX9tU8lIxNwdFYmIwpd89HijyRyoGnoJ3entd63cvKfuuix5r+GHyKp1Xm1L5j5AWM6P+z0xigwkiXnt+adexAl1J9wdDxv/pUFEESRF4DG8DFGVtbdH6aR1A5/vD4krO4tC1QYUSeyL5Mvsw8WRqIFHcXtgybtxylljvNcGMV1KXQC8UFDmpGZVDSHx6v3e/BHMrZ7gjoCCfVMZ/cFcQi0W2AIHPYEMH/C95J2r4XbHMRdYXpovpOoT5Ca78gsCAwEAAQ==",
        "keyDetails": "PKIX_RSA_PKCS1V15_4096_SHA256",
        "validFor": {
          "start": "2021-03-14T00:00:00.000Z",
          "end": "2022-07-31T00:00:00.000Z"
        }
      },
      "logId": {
        "keyId": "G3wUKk6ZK6ffHh/FdCRUE2wVekyzHEEIpSG4savnv0w="
      }
    },
haydentherapper commented 5 months ago

Key generated with https://go.dev/play/p/O-xYbvvgCaY

woodruffw commented 5 months ago

I'm not sure how clients are handling this currently, if they fail or simply ignore the keys - @woodruffw, what's python do for example?

I believe sigstore-python currently ignores keyDetails entirely, and blindly loads keys (the only supported types being ECDSA and RSA at the moment, both with SHA-256).

Here's where we naively load keys, whether PEM or DER:

https://github.com/sigstore/sigstore-python/blob/dc4aa82feaa18fbba93b05dcf71383751c56ba33/sigstore/_internal/trustroot.py#L113-L129

and verify checks the key's type, hardcoding the hash as SHA-256:

https://github.com/sigstore/sigstore-python/blob/dc4aa82feaa18fbba93b05dcf71383751c56ba33/sigstore/_internal/trustroot.py#L138-L170

(This is entirely non-ideal 😅 -- I think it predates TUF and the trusted root significantly. I'll look into refactoring it to make sure that we both handle keyDetails and also select the hash based on it.)

haydentherapper commented 5 months ago

FWIW, in Golang, we'll likely do the same, because the standard libraries do a great job of abstracting away details about the key algorithm that we don't need any hints. And where the standard libraries don't handle encodings like PKIX vs PKCS, we made sigstore/sigstore as another abstraction.

So I guess we need to let clients know to fail gracefully regardless if they want to look at keyDetails or blindly load in keys - either approach seems fine!

kommendorkapten commented 5 months ago

Sigstore JavaScript respects the key type as the underlying crypto package needs the information when parsing DER encoded data.