REGRESSION: new failure when `ctlog` is absent

[mattmoor]

Description

The following policy works fine with the latest released version of things, but I noticed adding unit testing at HEAD that it fails(!)

apiVersion: policy.sigstore.dev/v1beta1
kind: ClusterImagePolicy
metadata:
  name: ko-default-base-image-policy
spec:
  images:
  - glob: cgr.dev/chainguard/static*
  authorities:
  - keyless:
      url: https://fulcio.sigstore.dev

I see the following error:

signature keyless validation failed for authority authority-0 for cgr.dev/chainguard/static@sha256:39ae0654d64cb72003216f6148e581e6d7cf239ac32325867af46666e31739d2: no matching signatures:
            checking expiry on certificate with bundle: certificate expired before signatures were entered in log: 2023-01-02T00:20:01Z is before 2023-01-02T14:37:07-08:00:

However, I was wondering why only certain tests failed, and this was the only one without ctlog. Adding that back (since this publishes to Rekor) lets things pass:

    ctlog:
      url: https://rekor.sigstore.dev

Version

I believe that things are fine in the latest cut release, but I assume that something about the new TrustRoot handling at HEAD has caused the path where ctlog is unspecified to be broken.

🚨 WE SHOULD FIX THIS BEFORE THE NEXT RELEASE 🚨

[hectorj2f]

I believe the issue comes from https://github.com/sigstore/policy-controller/blob/main/pkg/webhook/validator.go#L1292 and returns nil https://github.com/sigstore/policy-controller/blob/d6ef1f37c9c634fdb2693c11f8aa91c19e76e7d8/pkg/webhook/validator.go#L1434. In a first look, I have the feeling we should default to rekor.sigstore.dev similarly to how cosign does. Otherwise we should expose a way to express a skip-tlog-verification here. We also bumped deps to use sigstore/cosign/v2 in main, that might also cause some issues.

[mattmoor]

Currently omitting ctlog is how we skip tlog verification 😞

[vaikas]

I don't think we should be defaulting to rekor.sigstore.dev.

The intended behaviour was that if there's no ctlog specified it will skip it, but I think we're taking the offline verification path since there's no client, but there's a public key for Rekor that's baked into the tuf root. https://github.com/sigstore/policy-controller/commit/e14c9c8be11d17f6992119e7f141786ad19bcfe3

UPDATE HERE It is not taking the offline path, it's skipping the bundle verification, but it's then using the current timestamp on cosign library, and from what I can tell, that should never really work because the certs should be short lived by design, so using a current time seems wrong.

[vaikas]

After looking at this some more, I think this may be a bug in cosign or I'm just holding it wrong. I verified that we're passing the correct flags (well, what I believe to be correct) to the verify from policy-controller. And I can repro this with the cosign from head (see below). Or at least I think this shows the exact behaviour that we're seeing from the policy-controller.

vaikas@villes-mbp cosign % ./cosign verify cgr.dev/chainguard/static@sha256:39ae0654d64cb72003216f6148e581e6d7cf239ac32325867af46666e31739d2 --certificate-identity='https://github.com/chainguard-images/images/.github/workflows/release.yaml@refs/heads/main' --certificate-oidc-issuer='https://token.actions.githubusercontent.com'

Verification for cgr.dev/chainguard/static@sha256:39ae0654d64cb72003216f6148e581e6d7cf239ac32325867af46666e31739d2 --
The following checks were performed on each of these signatures:
  - The cosign claims were validated
  - Existence of the claims in the transparency log was verified offline
  - Any certificates were verified against the Fulcio roots.

[{"critical":{"identity":{"docker-reference":"ghcr.io/chainguard-images/static"},"image":{"docker-manifest-digest":"sha256:39ae0654d64cb72003216f6148e581e6d7cf239ac32325867af46666e31739d2"},"type":"cosign container image signature"},"optional":{"1.3.6.1.4.1.57264.1.1":"https://token.actions.githubusercontent.com","1.3.6.1.4.1.57264.1.2":"schedule","1.3.6.1.4.1.57264.1.3":"e0e47eb5f17844c26a041480b6cd6c8ff612778f","1.3.6.1.4.1.57264.1.4":".github/workflows/release.yaml","1.3.6.1.4.1.57264.1.5":"chainguard-images/images","1.3.6.1.4.1.57264.1.6":"refs/heads/main","Bundle":{"SignedEntryTimestamp":"MEYCIQD0mNj/m4tLjb/o2/7+SUAimv8gIkj2hE5ZE/5wCI8iogIhAND5nqBgET4sj+5lqiTni7dDOxITBeBVgPpb/8keMGgi","Payload":{"body":"eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaGFzaGVkcmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiJmODQ1N2UzMTQyZTVlMGIxYWRjMjhhNWU0ZDI4OGViOWIyYTMzNTBiODgzZWZkYTdhNjZjOTE5MTI0NDIzOGQzIn19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1FUUNJQjhGMWtsNzV2Uk9mc2pnbWRqZWMzbXo2T08vU2pIcHlQQlZmVzB2UkVPT0FpQnl2YnVTL01zWVAyczBzUU1mVms4dFRPOFZ3Tkt0RkkzdkdFbEh0T0I4MEE9PSIsInB1YmxpY0tleSI6eyJjb250ZW50IjoiTFMwdExTMUNSVWRKVGlCRFJWSlVTVVpKUTBGVVJTMHRMUzB0Q2sxSlNVUjJSRU5EUVRCSFowRjNTVUpCWjBsVlIyTklZMDB4Y1ZoSlNqQXlUMGhXTW5sbVZXMHdTVGg1WldkdmQwTm5XVWxMYjFwSmVtb3dSVUYzVFhjS1RucEZWazFDVFVkQk1WVkZRMmhOVFdNeWJHNWpNMUoyWTIxVmRWcEhWakpOVWpSM1NFRlpSRlpSVVVSRmVGWjZZVmRrZW1SSE9YbGFVekZ3WW01U2JBcGpiVEZzV2tkc2FHUkhWWGRJYUdOT1RXcE5kMDFVUVhsTlJFRjRUVVJCZUZkb1kwNU5hazEzVFZSQmVVMUVRWGxOUkVGNFYycEJRVTFHYTNkRmQxbElDa3R2V2tsNmFqQkRRVkZaU1V0dldrbDZhakJFUVZGalJGRm5RVVZVY2tZdmIwUkpNRXBOSzJoT2IzSkRlRUZFV2tZNU1GQkpVSE50YUZreVMwaFpRemNLUTFGVVJUTlJhak15VWpaalJtZE9iVUpTY2pscldYVlFia0Z1WjFobVQxRXpaMlpZUVM4MVlqZG9RbWR6T0VveFpFdFBRMEZ0UVhkblowcGpUVUUwUndwQk1WVmtSSGRGUWk5M1VVVkJkMGxJWjBSQlZFSm5UbFpJVTFWRlJFUkJTMEpuWjNKQ1owVkdRbEZqUkVGNlFXUkNaMDVXU0ZFMFJVWm5VVlZSWlhReUNqZ3ZPVmRNYURoMFZUUXhhMjQwZG1ZeVRWVnphMDlGZDBoM1dVUldVakJxUWtKbmQwWnZRVlV6T1ZCd2VqRlphMFZhWWpWeFRtcHdTMFpYYVhocE5Ga0tXa1E0ZDJGQldVUldVakJTUVZGSUwwSkdOSGRZU1ZwaFlVaFNNR05JVFRaTWVUbHVZVmhTYjJSWFNYVlpNamwwVERKT2IxbFhiSFZhTTFab1kyMVJkQXBoVnpGb1dqSldla3d5YkhSWlYyUnNZM2s0ZFZveWJEQmhTRlpwVEROa2RtTnRkRzFpUnprelkzazVlVnBYZUd4WldFNXNURzVzYUdKWGVFRmpiVlp0Q21ONU9XOWFWMFpyWTNrNWRGbFhiSFZOUkd0SFEybHpSMEZSVVVKbk56aDNRVkZGUlVzeWFEQmtTRUo2VDJrNGRtUkhPWEphVnpSMVdWZE9NR0ZYT1hVS1kzazFibUZZVW05a1Ywb3hZekpXZVZreU9YVmtSMVoxWkVNMWFtSXlNSGRHWjFsTFMzZFpRa0pCUjBSMmVrRkNRV2RSU1dNeVRtOWFWMUl4WWtkVmR3cE9aMWxMUzNkWlFrSkJSMFIyZWtGQ1FYZFJiMXBVUW14T1JHUnNXV3BXYlUxVVl6Uk9SRkpxVFdwYWFFMUVVWGhPUkdkM1dXcGFhbHBFV21wUFIxcHRDazVxUlhsT2VtTTBXbXBCYzBKbmIzSkNaMFZGUVZsUEwwMUJSVVZDUWpSMVdqSnNNR0ZJVm1sTU0yUjJZMjEwYldKSE9UTmplVGw1V2xkNGJGbFlUbXdLVEc1c2FHSlhkM2RLWjFsTFMzZFpRa0pCUjBSMmVrRkNRbEZSV1ZreWFHaGhWelZ1WkZkR2VWcERNWEJpVjBadVdsaE5kbUZYTVdoYU1sWjZUVUl3UndwRGFYTkhRVkZSUW1jM09IZEJVVmxGUkROS2JGcHVUWFpoUjFab1draE5kbUpYUm5CaWFrTkNhV2RaUzB0M1dVSkNRVWhYWlZGSlJVRm5VamhDU0c5QkNtVkJRakpCVGpBNVRVZHlSM2g0UlhsWmVHdGxTRXBzYms1M1MybFRiRFkwTTJwNWRDODBaVXRqYjBGMlMyVTJUMEZCUVVKb1Z5OVJWVkU0UVVGQlVVUUtRVVZqZDFKUlNXZFhOREl6UW14U1dYWmlRVk5ZVGpZelVqTTJhRzUxVlZsa05XMVRTVUpGZGxwblEwTjBMM1JyWlZSUlEwbFJSRVZPV1VGRmVFSkNUd281TlhGaFFreEVVbVZEWjBGSVdIaENXa3g1VG5KbGFGWnRSM2RWVVcxTEwyNXFRVXRDWjJkeGFHdHFUMUJSVVVSQmQwNXdRVVJDYlVGcVJVRnpaV3Q0Q25KWGRFTnhlQ3RyV21rMGRXWkxTekE1YVRkVU9HSm1kVTVWVFVSbVVWSlZNMkp4VDNrekszbFhPRVZLTkZKSWNHUnZja3RZVEVwVVlYQndjRUZxUlVFS2REQmFhVk5PVWpOa01VOU9TR3A1ZEhObWRsUmlkbFJaZGpoMFNYaHNjSFZQVUdkblJIbE9kV2d4Vnk5NE5rMXlUMnBVY0hOeWRXaGlaMkZJVEM5NmVRb3RMUzB0TFVWT1JDQkRSVkpVU1VaSlEwRlVSUzB0TFMwdENnPT0ifX19fQ==","integratedTime":1672618221,"logIndex":10286378,"logID":"c0d23d6ad406973f9559f3ba2d1ca01f84147d8ffc5b8445c224f98b9591801d"}},"Issuer":"https://token.actions.githubusercontent.com","Subject":"https://github.com/chainguard-images/images/.github/workflows/release.yaml@refs/heads/main","githubWorkflowName":".github/workflows/release.yaml","githubWorkflowRef":"refs/heads/main","githubWorkflowRepository":"chainguard-images/images","githubWorkflowSha":"e0e47eb5f17844c26a041480b6cd6c8ff612778f","githubWorkflowTrigger":"schedule","run_attempt":"1","run_id":"3819302484","sha":"e0e47eb5f17844c26a041480b6cd6c8ff612778f"}}]
vaikas@villes-mbp cosign % ./cosign verify cgr.dev/chainguard/static@sha256:39ae0654d64cb72003216f6148e581e6d7cf239ac32325867af46666e31739d2 --certificate-identity='https://github.com/chainguard-images/images/.github/workflows/release.yaml@refs/heads/main' --certificate-oidc-issuer='https://token.actions.githubusercontent.com' --insecure-skip-tlo
g-verify
**Warning** Skipping tlog verification is an insecure practice that lacks of transparency and auditability verification for the signature.
Error: no matching signatures:
checking expiry on certificate with bundle: certificate expired before signatures were entered in log: 2023-01-02T00:20:01Z is before 2023-01-04T11:00:44+02:00
main.go:62: error during command execution: no matching signatures:
checking expiry on certificate with bundle: certificate expired before signatures were entered in log: 2023-01-02T00:20:01Z is before 2023-01-04T11:00:44+02:00

[vaikas]

Ok, so not really sure what the right behaviour here is. In cosign: https://github.com/sigstore/cosign/blob/main/pkg/cosign/verify.go#L708

So, it uses the current time which seems goofy. The error message is misleading (verified on local copy) in a sense that it says 'checking expiry on certificate with bundle' but because we're specifying --insecure-skip-tlog-verify so there's no bundle even fetched. I'll file on issue on cosign to see if it's something there, but in any case, at least the behaviour is the same with policy-controller / cosign when trying to skip the tlog verification.

[vaikas]

https://github.com/sigstore/cosign/issues/2590

[mattmoor]

FYI since this is now merged, we can repro by removing these lines at HEAD: https://github.com/sigstore/policy-controller/blob/6cf51647e9d7aa8512d06a4938228b1dce39a509/pkg/policy/policy_test.go#L57-L60

If this is a cosign issue we should probably get a regression test for this upstreamed as well (once it's fixed)! Thanks for digging into this 🙏 :party-ville:

[hectorj2f]

@vaikas (https://github.com/sigstore/policy-controller/blob/main/pkg/webhook/validator.go#L1307) @mattmoor Based on your comments, I feel we want to solve this problem without adding any temporal new API fields for the moment due to the new coming changes proposed here https://github.com/sigstore/cosign/issues/2466. These new changes can be discussed in a different issue #503.

Our latest release doesn't include these changes https://github.com/sigstore/policy-controller/blob/main/pkg/webhook/validator.go#L1307, so I'd say the safest choice consists on defaulting to rekor.sigstore.dev ctlog if no url or explicit ctlog has been defined. That will leave out any current option to enable skipTlogVerification or offline verification which are not supported today either due to #502.

What do you think about it ?

[mattmoor]

To be clear, I am fine with adding options to enable new verification configurations, but I would prefer to very clearly separate the changes fixing the regression from those new verification capabilities, so we can just be 100% clear that the next release won't break folks and require new fields to fix.

Hopefully that clarifies my comments on the PR? Sorry for the delayed response.

[hectorj2f]

So I propose to fix this issue by defaulting to rekor.sigstore.dev when ctlog.url=nil for a keyless authority. Then we tackle/discuss how to support the offline, ignoreTLog, ignoreSCT mode in a different issue/PR.

[mattmoor]

Just to be completely clear to make sure we're talking about the same thing... 😅

ctlog.url = "" should be different from ctlog = nil, and the regression this issue is covering is the latter.

I bring it up because I want to make sure ctlog = nil doesn't become ctlog.url = "rekor.sigstore.dev".