Surface the right `Name()` from our principal.

mattmoor commented 2 months ago

Summary

The cosign logic for interacting with Fulcio treats identity tokens as largely opaque, and most of the logic for how issuers and subjects and whatnot is handled happens server-side. However, for the "proof of possession" cosign has some logic (from sigstore/sigstore) that fumbles with email and sub claims in ways that have (until now) been compatible with Fulcio principals.

The Chainguard provider is the first provider that optionally includes an email claim, but we always want the subject we use to be our opaque identifier string (from sub). This creates a tear in the fulcio/cosign continuum, and so we must surface what cosign is signing as Name() even though that isn't necessarily what we embed in the certificate.

The only correct way to implement Name() today is to match what this function does, and current implementations happen to align, but unfortunately because of how this abstraction is formulated it is challenging to actually change how we confirm the proof of possession to use this directly in place of the principal itself.

Fixes: https://github.com/sigstore/cosign/issues/3777

Release Note

This corrects the Chainguard PoP verification flow for tokens that embed email claims (makes Name() consistent with cosign).

Documentation

N/A

mattmoor commented 2 months ago

whoops, amended with my test file

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 49.58%. Comparing base (cf238ac) to head (587eb8e). Report is 136 commits behind head on main.

Files	Patch %	Lines
pkg/identity/chainguard/principal.go	60.00%	1 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1726 +/- ## ========================================== - Coverage 57.93% 49.58% -8.36% ========================================== Files 50 71 +21 Lines 3119 4185 +1066 ========================================== + Hits 1807 2075 +268 - Misses 1154 1880 +726 - Partials 158 230 +72 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

haydentherapper commented 2 months ago

I'll cut 1.5.1 with this fix. I'm going to need to revert a PR that when in recently first, as a cherry-picked release is a bit of a pain.

sigstore / fulcio