[RFC] Should Fulcio put the critical bit on its OIDC extensions?

[woodruffw]

As discussed in the Sigstore Slack: the certificates currently issued by Fulcio don't include critical bits on any OIDC extensions, even when those extensions are essential to the conceptual policy or identity being verified.

So, the question: should we include the critical bit on these extensions?

Pros:

• The critical bit is the intended way in X.509 to signal that an extension must be accounted for during verification, which maps closely to how we expect these OIDC extensions to be used (i.e., clients use at least some subset of these extensions to match a policy.)
• Using the critical bit turns client design problems into compliance problems: it provides Sigstore with normative authority ("you're not following the spec!") when clients don't do verification correctly, rather than getting bogged down in design questions.

Cons:

• There is a decent amount of impedance mismatch between X.509's notion of "criticality" and how identity verification works/is expected to work in Sigstore: depending on the policy, some OIDC extensions don't need to be checked, despite being marked as "critical" under this change.
• Not a con as such, but the critical bit is widely ignored by X.509 clients. There's no formal guarantee that any given client handles it at all (much less correctly), so its inclusion by Fulcio would be purely descriptive and offers no direct security benefits.

In this situation, I believe that the pros (weakly) outweigh the cons -- Sigstore can specify in documentation that the critical bit on its extensions really means that some but the empty subset of extensions must be verified.

Longer term, I wonder if this impedance mismatch might be a good reason to re-think how Fulcio embeds OIDC claims as extensions -- we're already planning to change the current extensions to use ASN.1 types per #900 and #974, so perhaps it makes sense to change from N extensions for N claims to a single (critical) extension that uses an ASN.1 SEQUENCE structure to lay out all claims internally?

cc @znewman01 @haydentherapper @SantiagoTorres

[znewman01]

Longer term, I wonder if this impedance mismatch might be a good reason to re-think how Fulcio embeds OIDC claims as extensions -- we're already planning to change the current extensions to use ASN.1 types per #900 and #974, so perhaps it makes sense to change from N extensions for N claims to a single (critical) extension that uses an ASN.1 SEQUENCE structure to lay out all claims internally?

I like this idea quite a bit, TBH, and would be open to that being the only action we take from this issue, even if it'll take a bit longer.

But I'm not opposed to calling these other extensions critical for now (though I worry this might break clients, even the ones that are parsing these extensions.)

[haydentherapper]

One concern is how ASN1 parsers handle structs with unknown fields. I’m thinking about the case where we add new claims to the certificate. I’d like to avoid having to make a new major release of fulcio to support this.

Changing the critical bit would be a breaking change, so this would be tackled as part of the next major release of fulcio at the earliest.

[woodruffw]

One concern is how ASN1 parsers handle structs with unknown fields. I’m thinking about the case where we add new claims to the certificate. I’d like to avoid having to make a new major release of fulcio to support this.

Making sure I understand: is the concern here that, with a single extension instead of multiple, we'd be causing problems for some ASN.1 parsers by adding/removing fields that would otherwise be fine as standalone extensions?

If so, I think a reasonable definition would be this (inspired by X.509's own Attributes/Attribute):

Claims ::= SET OF Claim

Claim ::= SEQUENCE {
  id OBJECT IDENTIFIER,
  value ANY DEFINED BY id,
}

where Claims would then be the value of the singular (critical) extension, and each Claim.id would be one of the OIDs already specified by Fulcio. This should then be fully forwards compatible (since the Claims set could have elements added or removed from it without changing the structure).

Edit: 100% agreed this would be a breaking change; just wanted to make sure I understood!

[znewman01]

One concern is how ASN1 parsers handle structs with unknown fields. I’m thinking about the case where we add new claims to the certificate. I’d like to avoid having to make a new major release of fulcio to support this.

I thought that in general parsers should gracefully ignore unknown extensions? And that's the point of the "critical" bit—to mark an extension as not-to-be-ignored. So IIUC adding new extensions is fine even if they have new structs; marking critical is the thing to be careful about.

Changing the critical bit would be a breaking change, so this would be tackled as part of the next major release of fulcio at the earliest.

Agreed it's breaking and we need to do it safely, but I'm a little concerned about "wait for a major version and then breaking changes are okay" as a strategy. While technically it complies with our versioning guarantees, I worry about difficulties in adoption. We don't really control all the clients.

I think something like what @woodruffw is proposing we could achieve something like:

1. Add new claim format in addition to old one (not critical). Old clients will ignore this.
2. Teach new clients about the new claim format, tell them to prefer it.
3. Communicate, wait a long time for old clients to go end-of-life.
4. Drop old claim format and mark new one critical (at major version bump).

[woodruffw]

I like that approach! That would give us a very graceful transition period on sigstore-python.

[haydentherapper]

where Claims would then be the value of the singular (critical) extension, and each Claim.id would be one of the OIDs already specified by Fulcio. This should then be fully forwards compatible (since the Claims set could have elements added or removed from it without changing the structure).

I was thinking that you'd unmarshal to a struct with each claim specified, but what you suggested makes sense. It is a little strange to me though that the extension would be marked as critical but a client could ignore some of the OIDs within the extension if they aren't known.

I would prefer we keep the current approach of an extension per value (each extension being critical), rather than encode all in a single extension. These seem pretty similar to me, so unless there's a motivation to switch, I'd prefer to keep what we've been doing.

Add new claim format in addition to old one (not critical). Old clients will ignore this.
Teach new clients about the new claim format, tell them to prefer it.
Communicate, wait a long time for old clients to go end-of-life.
Drop old claim format and mark new one critical (at major version bump).

Yep, that's totally reasonable for doing breaking changes.

[woodruffw]

It is a little strange to me though that the extension would be marked as critical but a client could ignore some of the OIDs within the extension if they aren't known.

Yeah, that's a good point. It's not clear to me which is more idiomatic:

1. A single critical extension with internal members that are subject to interpretation, meaning some might be skipped;
2. N different critical extensions, of which some subset must always be inspected and interpreted (while others may be skipped)

[haydentherapper]

Actually, it might be an issue if every extension is critical if a client doesn’t care about some values, say, the custom OIDs set for CI. If it’s marked as critical, a client should know how to deal with the extension. This might be why most extensions for web PKI certs are marked as non critical, not sure though.

[haydentherapper]

Also if any new extensions are added, then a client wouldn’t yet know of them, so I don’t think you can mark them as critical.

[haydentherapper]

Closing due to the above

[woodruffw]

Sorry for the necropost here!

FWIW, I think there's still a part of this issue that's worth considering (the part not related to the critical bit): #981 (comment) would be a refactoring of the claims layout to make it more flexible/extensible in the future, while also giving us the flexibility to add "non-authenticated" extensions (i.e. ones derived from the user's request, rather than from the OIDC identity).

Should I open a separate issue for that, or should we reuse this one?

[znewman01]

Yeah, I'd like to see a separate issue for that. But it's a good idea :)

[woodruffw]

Sounds good! I'll break it out.

[haydentherapper]

Happy to discuss it more in a separate issue, but we won't want to break clients this significantly with a V2 unless there's good motivation to do so.