Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Combine the individual OIDC extensions into a single extension #1132

Open
woodruffw opened this issue Apr 23, 2023 · 4 comments
Open
Labels
enhancement New feature or request

Comments

@woodruffw
Copy link
Member

This is a breakout of the unrelated discussion/proposal in #981, where this was originally brought up incidentally.

Problem statement

At the moment, Fulcio-issued certificates contain a bunch of X.509 extensions, roughly one per claim in the corresponding OIDC credential/identity. For example, the extension with OID 1.3.6.1.4.1.57264.1.14 is expected to hold a value corresponding to the "Source Repository Ref," which will be the git ref for things like GitHub Actions.

This works perfectly fine, and corresponds roughly to how other PKI ecosystems use X.509 extensions. At the same time, it has a few deficiencies:

Strict compliance

X.509 extensions are defined by RFC 5280 as:

   Extension  ::=  SEQUENCE  {
        extnID      OBJECT IDENTIFIER,
        critical    BOOLEAN DEFAULT FALSE,
        extnValue   OCTET STRING
                    -- contains the DER encoding of an ASN.1 value
                    -- corresponding to the extension type identified
                    -- by extnID
        }

Critically, extnValue is required to be a nested DER encoding, which most of the OIDC claims are not (instead, they embed the raw value of the claim as bytes).

The three solutions here are to:

  1. Define the problem away by defining our own ASN.1 structure (see proposal below), in which OIDC claim values can be raw rather than wrapped in DER;
  2. Go through a migration period, where existing OIDC claim extensions are duplicated with correctly encoded ones;
  3. Both of the above.

Space efficiency

Encoding each OIDC claim as its own X.509 extension is not space efficient, leading to larger certificate encodings (and thus larger transparency log entries) than strictly necessary.

In particular:

  1. Separate extensions means a linear overhead of N SEQUENCE payloads;
  2. Correctly encoded extensions require a linear overhead of N nested DER payloads;
  3. Each migration away from old claim encodings requires a linear overhead of claim values with new OIDs.

Domain separation

At the moment, the OIDC claim extensions are the only custom claims added by Fulcio. They can be considered "authenticated," in the sense that they are not only bound to the certificate itself but also derived directly from the original identity token.

In the future, Fulcio may choose to introduce additional extensions, ones that wouldn't be considered "authenticated" in the same way. Introducing those into the same "namespace" (of extensions) as the OIDC claims could potentially result in user and developer confusion about the provenance and trustworthiness of their extension values.

See also #1131 for some motivation for this part, although I think the notion of "domain separation" in this context is independently valuable.

Proposed solution

My proposed solution is to replace N X.509 extensions for N OIDC claims with a single X.509 extension.

Two proposed structures for this extension:

Proposal 1:

Claims ::= SEQUENCE OF Claim

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

Proposal 2:

Claims ::= SEQUENCE OF Claim

Identifiers ::= SEQUENCE OF OBJECT IDENTIFIER

Claim ::= SEQUENCE {
  ids Identifiers,
  value OCTET STRING, -- defined by ids
}

In both of these proposals, the top-level Claims definition would then be wrapped in an ordinary X.509 extension, with a new Sigstore-controlled OID.

Proposal (2) differs primarily from Proposal (1) in that it offers a few key space and forward-compatibility optimizations:

  1. Each Claim has a list of potential identifiers, meaning that future changes to claim OIDs can be accommodated without duplicating value by just adding the new OID to the ids sequence. This both saves space (no value duplication) and makes forward compatibility simpler (no additional extension to search for; no potential dual-state between the "legacy" value and the "new" value).
  2. value is explicitly an OCTET STRING, meaning that it doesn't obligate any nested DER encoding (and doesn't contract RFC 5280, being a nested value within an extension).

Alternatives considered

Doing nothing. IMO the current state of affairs is not a significant problem; the primary advantages to this proposal would be conceptual purity, strict compliance with RFC 5280, and lower long-term certificate sizes and diminished OID migration friction.

Problems identified

None, currently.

This would require a deprecation/migration period, but this is no different from the deprecation requirements for the current "legacy" OIDs/extensions for GitHub-specific OIDC claims.


CC @znewman01 and @haydentherapper for consideration 🙂

@woodruffw woodruffw added the enhancement New feature or request label Apr 23, 2023
@woodruffw
Copy link
Member Author

Re: #981 (comment) in particular: I agree that this is a significant change for V2 of Fulcio, but IMO it's no more of a breaking change than the other OID changes, and can have the exact same migration pathway as proposed there: the "legacy" extensions can be included as well for a while, and then eventually rolled off.

@woodruffw
Copy link
Member Author

(Also, as a design note: I chose SEQUENCE instead of SET in the definitions above because of some ambiguity in the DER specifications about how SET is actually encoded and whether it really guarantees uniqueness. Uniqueness would be an application-level expectation for both Claims and Identifiers.)

@haydentherapper
Copy link
Contributor

Adding a bit more context to my comment in the other thread - This proposed change is solid technically, and if we were to start from scratch, this would be a good option. I don't think it buys us much to do it now though:

  • This doesn't save any work for clients, as they'll need to accept both certificate formats for verification. You could say that at some point we drop support for separate extensions, but that breaks verification for all existing signed artifacts out there, and it means we may not see adoption of new client versions that do drop support.
  • The space savings are O(bytes), I don't expect the addition of a lot more claims.
  • ASN.1 is complex, and I'd prefer to not maintain structures if we don't have to.
  • We already fixed the incorrect encoding for the new extensions we added, where each is now a DER string (could be another type too). This leaves 6 duplicated extensions (5 of which were going to be duplicated because they were GitHub specific).

@woodruffw
Copy link
Member Author

  • We already fixed the incorrect encoding for the new extensions we added, where each is now a DER string (could be another type too). This leaves 6 duplicated extensions (5 of which were going to be duplicated because they were GitHub specific).

Oh, this was a mistake on my part: I missed the part in oid-info.md where the new extensions were DER-encoded. Sorry for that!

I also was under the impression that the new extensions hadn't actually deployed yet, and were being gated on a 2.0 for Fulcio. I just checked and saw that they have been deployed, however, so my size argument is even less compelling 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants