Repository: sigstore/fulcio
Pull request #1726 (2024-07-10 19:42:59)

Surface the right `Name()` from our principal.
#### 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: whoops, amended with my test file
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/sigstore/fulcio/pull/1726?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) 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`)](https://app.codecov.io/gh/sigstore/fulcio/commit/cf238accd68ef13e1e3425a5d87db2f352de173c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) to head [(`587eb8e`)](https://app.codecov.io/gh/sigstore/fulcio/commit/587eb8ede00fb0601676b603ce59414f85d2d3ae?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).
> Report is 136 commits behind head on main.

| [Files](https://app.codecov.io/gh/sigstore/fulcio/pull/1726?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | Patch % | Lines |
|---|---|---|
| [pkg/identity/chainguard/principal.go](https://app.codecov.io/gh/sigstore/fulcio/pull/1726?src=pr&el=tree&filepath=pkg%2Fidentity%2Fchainguard%2Fprincipal.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL2lkZW50aXR5L2NoYWluZ3VhcmQvcHJpbmNpcGFsLmdv) | 60.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/sigstore/fulcio/pull/1726?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) |

<details><summary>Additional details and impacted files</summary>


```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     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/sigstore/fulcio/pull/1726?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).


Patch:
diff --git a/pkg/identity/chainguard/principal.go b/pkg/identity/chainguard/principal.go
index d2940c6..194b899 100644
--- a/pkg/identity/chainguard/principal.go
+++ b/pkg/identity/chainguard/principal.go
@@ -22,11 +22,13 @@ import (
 	"github.com/coreos/go-oidc/v3/oidc"
 	"github.com/sigstore/fulcio/pkg/certificate"
 	"github.com/sigstore/fulcio/pkg/identity"
+	"github.com/sigstore/sigstore/pkg/oauthflow"
 )
 
 type workflowPrincipal struct {
 	issuer  string
 	subject string
+	name    string
 
 	actor            map[string]string
 	servicePrincipal string
@@ -35,7 +37,7 @@ type workflowPrincipal struct {
 var _ identity.Principal = (*workflowPrincipal)(nil)
 
 func (w workflowPrincipal) Name(_ context.Context) string {
-	return w.subject
+	return w.name
 }
 
 func PrincipalFromIDToken(_ context.Context, token *oidc.IDToken) (identity.Principal, error) {
@@ -50,9 +52,19 @@ func PrincipalFromIDToken(_ context.Context, token *oidc.IDToken) (identity.Prin
 		return nil, err
 	}
 
+	// This is the exact function that cosign uses to extract the "subject"
+	// (misnomer) from the token in order to establish "proof of possession".
+	// We MUST use this to implement Name() or tokens that embed an email claim
+	// will fail to sign because of this divergent logic.
+	name, err := oauthflow.SubjectFromToken(token)
+	if err != nil {
+		return nil, err
+	}
+
 	return &workflowPrincipal{
 		issuer:           token.Issuer,
 		subject:          token.Subject,
+		name:             name,
 		actor:            claims.Actor,
 		servicePrincipal: claims.Internal.ServicePrincipal,
 	}, nil


Test patch:
diff --git a/pkg/identity/chainguard/principal_test.go b/pkg/identity/chainguard/principal_test.go
index db995db..cd666aa 100644
--- a/pkg/identity/chainguard/principal_test.go
+++ b/pkg/identity/chainguard/principal_test.go
@@ -60,6 +60,7 @@ func TestJobPrincipalFromIDToken(t *testing.T) {
 			ExpectPrincipal: workflowPrincipal{
 				issuer:  "https://issuer.enforce.dev",
 				subject: id.String(),
+				name:    id.String(),
 				actor: map[string]string{
 					"iss": "https://iss.example.com/",
 					"sub": fmt.Sprintf("catalog-syncer:%s", group.String()),
@@ -85,6 +86,34 @@ func TestJobPrincipalFromIDToken(t *testing.T) {
 			ExpectPrincipal: workflowPrincipal{
 				issuer:  "https://issuer.enforce.dev",
 				subject: group.String(),
+				name:    group.String(),
+				actor: map[string]string{
+					"iss": "https://auth.chainguard.dev/",
+					"sub": "google-oauth2|1234567890",
+					"aud": "fdsaldfkjhasldf",
+				},
+			},
+			WantErr: false,
+		},
+		`Human SSO token (with email)`: {
+			Claims: map[string]interface{}{
+				"iss":            "https://issuer.enforce.dev",
+				"sub":            group.String(),
+				"email":          "jane@doe.dev",
+				"email_verified": true,
+				// Actor claims track the identity that was used to assume the
+				// Chainguard identity.  In this case, it is the Catalog Syncer
+				// service principal.
+				"act": map[string]string{
+					"iss": "https://auth.chainguard.dev/",
+					"sub": "google-oauth2|1234567890",
+					"aud": "fdsaldfkjhasldf",
+				},
+			},
+			ExpectPrincipal: workflowPrincipal{
+				issuer:  "https://issuer.enforce.dev",
+				subject: group.String(),
+				name:    "jane@doe.dev",
 				actor: map[string]string{
 					"iss": "https://auth.chainguard.dev/",
 					"sub": "google-oauth2|1234567890",