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

Fulcio: Switch to new-style claim extensions #425

Closed
woodruffw opened this issue Jan 11, 2023 · 8 comments · Fixed by #1004
Closed

Fulcio: Switch to new-style claim extensions #425

woodruffw opened this issue Jan 11, 2023 · 8 comments · Fixed by #1004
Assignees
Labels
component:api Public APIs enhancement New feature or request
Milestone

Comments

@woodruffw
Copy link
Member

Just filing this for tracking purposes: sigstore/fulcio#945 will change Fulcio's certificate extensions to make them more generic, avoiding unnecessary references to implementation details for e.g. GitHub.

These changes will follow Fulcio's deprecation policy, so no action is immediately required on our part.

@woodruffw woodruffw added enhancement New feature or request component:api Public APIs labels Jan 11, 2023
@woodruffw
Copy link
Member Author

Looks like these changes have been merged upstream, so we should check to see whether the production or staging instances have deployed them. If so, we should follow suit.

@woodruffw
Copy link
Member Author

This changes have been fully deployed, so we should begin supporting them.

@woodruffw woodruffw added this to the 2.0 milestone Apr 26, 2023
@woodruffw woodruffw assigned woodruffw and unassigned jleightcap Jul 18, 2023
@jku
Copy link
Member

jku commented Sep 7, 2023

I did some quick tests in jku@5041820:

  • backwards compat decision needs to be made: likely we should support verifying already existing certificates that contain github OIDs only, like the test data currently in tree? My WIP patch does not do this but there's at least three ways about it:
    • Add more policies to policy module (e.g. OIDCIssuer is a AnyOf policy over both OIDCIssuerV1 and OIDCIssuerV2) -- this is quite a lot of new classes
    • make _SingleX509ExtPolicy support "alternative oids": I like this option, it's likely the least amount of work
    • keep complexity out of policy module completely (all policies are just _SingleX509ExtPolicy with a specific OID). Instead put the complexity in "verify github" code
  • GitHubWorkflowRepository no longer exists: the CLI can paper over the difference between that and the current Source Repository URI, but a single policy can't cover both
  • GitHubWorkflowName no longer exists: it probably should never have been used anyway since it has no real claim value -- it's not unique at all and SAN already contains the workflow path. We may want to expose Build Signer Digest as policy instead (?) and the CLI could use "sha" argument as default value for that
  • I'm not sure what is the value of Build Signer URI -- should the cli use the cert identity argument there too?

@woodruffw
Copy link
Member Author

FWIW, I also have an initial stab at this up at #715: it keeps the old extension handling in place while adding new APIs for the new extensions. My thinking was that backwards compatibility could be maintained by using the AnyOf building block to compose the old and new extensions.

(More generally, using these new extensions is blocked by the fact that they contain DER encodings for their values, which pyca/cryptography doesn't support for arbitrary third-party extensions yet. That's being tracked here: pyca/cryptography#9283.)

@jku
Copy link
Member

jku commented Sep 8, 2023

FWIW, I also have an initial stab at this up at #715: it keeps the old extension handling in place while adding new APIs for the new extensions. My thinking was that backwards compatibility could be maintained by using the AnyOf building block to compose the old and new extensions.

Yeah this works but does lead to a lot of classes (e.g. GitHubWorkflowTrigger(_SingleX509ExtPolicy), _BuildTrigger(_SingleX509ExtPolicy), BuildTrigger(Anyof)). Supporting "deprecations" in _SingleX509ExtPolicy makes that more manageable since you could just have BuildTrigger(_SingleX509ExtPolicy) -- this is with the assumption that API stability of the policy module is not that important, but supporting old certificates is very important (which is my intuition). But either solution definitely works.

(More generally, using these new extensions is blocked by the fact that they contain DER encodings for their values, which pyca/cryptography doesn't support for arbitrary third-party extensions yet.

ooh, good to know

@jku
Copy link
Member

jku commented Sep 8, 2023

After reading up on the situation in cryptography:

  • it looks like this should not block a sigstore-python release as solving this will likely take some time?
  • is this going to be a scheduling issue: what's the plan if fulcio does start thinking about a 2.0 release and the cryptography DER API is not close to being implemented:
    • ask fulcio to extend support for the deprecated OIDs or
    • do DER parsing crimes in sigstore-python (with pyasn1 or something)

@woodruffw
Copy link
Member Author

this is with the assumption that API stability of the policy module is not that important, but supporting old certificates is very important (which is my intuition)

Yeah, this is a fair point. IMO it'd be okay to break the API here between major versions if we think the resulting code will be smaller (especially since the migration path will be smooth).

it looks like this should not block a sigstore-python release as solving this will likely take some time?

Yes, agreed -- I have this as a 2.0 milestone ATM but I don't think it should block at 2.0 release of sigstore-python (since the legacy extensions continue to work just fine). We can move it to 3.0, I think.

is this going to be a scheduling issue: what's the plan if fulcio does start thinking about a 2.0 release and the cryptography DER API is not close to being implemented:

DER crimes with pyasn1 is an option, but not one I find appealing 😅

Short of a full declarative ASN.1 API in Python becoming available, I can think of two other options:

  • Convince PyCA to include upstream support for the Sigstore X.509v3 extensions (no idea whether they'll consider this; I'll ask)
  • Create another PyPI package that uses PyCA Cryptography's ASN.1 APIs to parse these extensions (which should be very simple, since they're mostly no more complicated than OCTET STRING wrappers).

Regardless I think we have options available to us, so we won't need to ask Fulcio to prolong its support for the deprecated extensions 🙂

@woodruffw woodruffw removed this from the 2.0 milestone Sep 11, 2023
@woodruffw
Copy link
Member Author

Dropped this from 2.0, per #766.

@woodruffw woodruffw added this to the 3.0 milestone Sep 11, 2023
@woodruffw woodruffw changed the title Changes to Fulcio's certificate extensions Fulcio: Switch to new-style claim extensions Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:api Public APIs enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants