Double-check issuer in OIDC <-> X.509 match

[znewman01]

The below describes a hypothetical issue. Right now, we do everything correctly. But I'd like to see us be a bit more defensive here.

---

We populate the 1.3.6.1.4.1.57264.1.1 extension in issued certificates (per the certificate specification) based on the iss claims in the OIDC token.

For GitHub, we do it like so:

fulcio/pkg/identity/github/principal.go

Line 90 in 7d5114e

issuer: token.Issuer,

fulcio/pkg/identity/github/principal.go

Line 114 in 7d5114e

Issuer: w.issuer,

AFAICT, there's nothing stopping GitHub from issuing an OIDC token with any iss claim they'd like, which would let them impersonate, say, Google. This means that if any accepted OIDC issuer is compromised, all of them should be considered compromised. @haydentherapper has identified a separate mechanism that addresses this; see below.

---

@haydentherapper's investigation (lightly edited for formatting):

When a token, the issuer claim is extracted from it:

fulcio/pkg/identity/issuerpool.go

Line 28 in 3987a47

url, err := extractIssuerURL(token)

and an Issuer struct is returned that matches:

fulcio/pkg/identity/base/issuer.go

Line 47 in 841a58b

if url == e.issuerURL {

that issuer value. The pool of Issuer structs is built from the configuration that you've linked:

fulcio/pkg/server/issuer_pool.go

Line 29 in 841a58b

func NewIssuerPool(cfg *config.FulcioConfig) identity.IssuerPool {

(Note the concept of issuer pools is new, so there may be some duplicated logic)

Authorize is then called for the issuer:

fulcio/pkg/identity/github/issuer.go

Line 33 in 841a58b

idtoken, err := identity.Authorize(ctx, token)

, which also extracts the issuer claim from the token, fetches a token verifier, and verifies it:

fulcio/pkg/identity/authorize.go

Lines 29 to 38 in 841a58b

	issuer, err := extractIssuerURL(token)
	if err != nil {
	return nil, err
	}
	verifier, ok := config.FromContext(ctx).GetVerifier(issuer)
	if !ok {
	return nil, fmt.Errorf("unsupported issuer: %s", issuer)
	}
	return verifier.Verify(ctx, token)

The mapping from issuer to token verifier is built from the linked mapping too.

If GitHub issues a token whose issuer claim is not the GitHub URL, then Fulcio will find a verifier that verifies a different token or finds no verifier at all.

Dex is the only special case - The issuer in the token and configuration is "oauth2.sigstore.dev", but we also provide the claim that will be used to populate the certificate—IssuerClaim:

fulcio/pkg/identity/email/principal.go

Line 54 in 841a58b

issuer, err := oauthflow.IssuerFromIDToken(token, cfg.IssuerClaim)

Like you said, we have to trust Dex for that case.

---

My request: we should allow configuring explicit allowed issuer value(s) for each issuer that Fulcio supports, and defensively check that they match.

[haydentherapper]

Given that there's a 1-1 mapping between issuer value and verifier, is this needed? If I understand the request, it would be to add a configuration field to check that the issuer value in the token matches what's in the configuration, but this is effectively what we've got already, it's just that the key of the mapping is that configuration.

[znewman01]

First, I could be totally convinced that we should just close this, so maybe that's the right thing to do.

I guess I'm more concerned about the second half: we dispatch to the appropriate verifier based on the OIDC token, but then the issuer that winds up in the ultimate cert isn't double-checked against the config at all. So if this line was somehow wrong for the GitHub issuer, nothing would happen.

Think about the Dex config:

            "https://oauth2.sigstore.dev/auth": {
              "IssuerURL": "https://oauth2.sigstore.dev/auth",
              "ClientID": "sigstore",
              "Type": "email",
              "IssuerClaim": "$.federated_claims.connector_id"
            },

Nowhere in there does it say which issuers that verifier is allowed to create certs for. It tells you how to get that value (IssuerClaim) but not what issuer values are allowed.

I was just thinking that we could have a new "AllowedIssuers" (name needs editing) config value and add a double-check that the Issuers from the X.509 cert is in that set. By default it could just be the same as the IssuerURL (so it matters mostly for Dex).

Again, I don't think anything is wrong right now, but can't hurt to be defensive.