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

[docs] OID information: provide more detail about the structure of each X.509 extension? #900

Closed
woodruffw opened this issue Nov 29, 2022 · 4 comments · Fixed by #1073
Closed
Labels
enhancement New feature or request

Comments

@woodruffw
Copy link
Member

I was perusing the oid-info doc for the nth time, and I noticed that we don't currently explicitly specify how each OID-identified X.509v3 extension is laid out.

For example:

1.3.6.1.4.1.57264.1.2 | GitHub Workflow Trigger
This contains the event_name claim from the GitHub OIDC Identity token that contains the name of the event that triggered the workflow run. (docs)

In particular, it might make sense to clarify that each of our extensions is (currently) laid out "inline", rather than being a nested ASN.1 structure containing the string or other data.

@woodruffw woodruffw added the enhancement New feature or request label Nov 29, 2022
@haydentherapper
Copy link
Contributor

+1! Though I also like the idea of using ASN.1 structures for future OIDs.

@haydentherapper
Copy link
Contributor

Thought a bit more about this - this is going to be driven by how easy this is to do in golang, I haven't looked yet. If implicit/inline is the default, we'll probably just keep that, I don't want to complicate parsing. Given the ASN1 structure is a single string, explicit tagging isn't needed.

@haydentherapper
Copy link
Contributor

The above comment is a little incorrect! As I'm sure is a common phrase, I misunderstood ASN.1.

Implicit/explicit is only relevant when the structure contains tagged fields, for example [5] IMPLICIT UTF8String or [5] EXPLICIT UTF8String.

An extension sequence looks like the following (from RFC5280):

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
        }

Consider this structure from a cert: https://lapo.it/asn1js/#MCkGCisGAQQBg78wAQEEG2h0dHBzOi8vYWNjb3VudHMuZ29vZ2xlLmNvbQ. This contains the extension ID and extension value. The extension value is an OCTET STRING, which contains the issuer. When parsing the cert, Go makes it very easy to extract the extension value from the extension, so no one complained when setting the value to be just a string. The problem is, this isn't RFC5280 compliant, because a raw string is not an ASN.1 value. @loosebazooka noticed this when working with the Java libraries, the extension value was expected to be proper ASN.1

Proper ASN.1 would be wrapping the string in its type, for example 0C 02 68 69, which is a UTF8String "hi". We could go further and define a SEQUENCE that would contain the issuer, but this isn't really necessary given it's just one value.

https://lapo.it/asn1js/#MCsGCisGAQQBg78wAQEEHQwbaHR0cHM6Ly9hY2NvdW50cy5nb29nbGUuY29t would be the fix, wrapping the issuer in a UTF8String. I propose that we duplicate the issuer extension alongside the other work to standardize CI extensions, and mark the old issuer as deprecated.

@woodruffw
Copy link
Member Author

woodruffw commented Mar 11, 2023

I propose that we duplicate the issuer extension alongside the other work to standardize CI extensions, and mark the old issuer as deprecated.

That makes sense to me!

FWIW, I still think we could save ourselves some future migration pain by switching to a single X.509 extension for all embedded claims, e.g. something like:

ClaimsExtension :: SEQUENCE {
  version INTEGER,
  claims Claims,
}

Claims ::= SET OF Claim

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

...where ClaimExtension would then become the DER-encoded extnValue body. This also "solves" the UTF8String problem by using ANY DEFINED BY id, so would then save a few bytes on each string by not needing to wrap it as DER.

(This avoids the problem in #981 by punting on whether any such extension should be marked as critical.)

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

Successfully merging a pull request may close this issue.

2 participants