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

New v2 API for more customizable orders #23

Merged
merged 4 commits into from
Mar 8, 2024
Merged

New v2 API for more customizable orders #23

merged 4 commits into from
Mar 8, 2024

Conversation

mholt
Copy link
Owner

@mholt mholt commented Feb 22, 2024

This is a new high-level API in the acmez package to allow the user more control over ACME orders.

In v1, there are three high-level methods on the acmez.Client:

  • ObtainCertificate()
  • ObtainCertificateForCSR()
  • ObtainCertificateForCSRSource()

And their list of arguments was growing quite long, considering we wanted to add support for customizing the NotAfter field of Orders, as well as draft support for ARI which lets you specify a certificate you are replacing/renewing in the Order itself.

To avoid further breaking changes to method signatures brought about by new ACME extensions, I decided to move all the high-level parameters of an Order into a new type, OrderParameters, which encapsulates the user's ACME account, the CSR source, the identifier list, the optional NotAfter, and the optional ID of a cert being renewed (for ARI).

I also wanted to clean up the exported methods so there aren't so many. In v2 there are just:

  • ObtainCertificate()
  • ObtainCertificateForSANs()

The first is the more flexible call, but requires that you make an OrderParameters struct first. (There are also helper methods for this for the most common use cases.)

The second is more like the original ObtainCertificate(), in that it is quite opinionated: it takes the list of SANs as a string slice, the private key, and the account, and will generate the CSR for you along the way. I figure this is the 90% use case, if not more, for this package, so it saves you 2 extra steps (generating the CSR, then generating the order params). But maybe those two steps are easy enough now that they also have helper functions in v2, ObtainCertificateForSANs is not even necessary?

I also changed the CSRSource interface slightly; now it takes the identifiers as an argument. I originally also added the private key as an argument, so it could theoretically have everything it needed to generate the CSR right there, but then it felt weird adding the private key to ObtainCertificate()'s signature... maybe because the StaticCSR(), which is most commonly used I think, doesn't actually generate a CSR, so we're passing in the private key for nothing. There's no harm in that, just felt weird. I might still restore that.

@hslatman Would you be so kind as to take a look when you have a few minutes? I want to make sure this works for your use case with the more cutting-edge identifier and attestation types... thank you in advance 😊

@mholt mholt mentioned this pull request Feb 22, 2024
@hslatman
Copy link
Contributor

@mholt haven't integrated it in code yet, but it looks like it'll be compatible in terms of behavior.

Using Subjects for the ACME identifiers could be a bit surprising, given that they're not necessarily the same as the Subject Common Name. It does seem to be an intentional choice, though, so I'm curious why it's not simply Identifiers?

@mholt
Copy link
Owner Author

mholt commented Feb 23, 2024

Thanks for taking a look!

Using Subjects for the ACME identifiers could be a bit surprising, given that they're not necessarily the same as the Subject Common Name. It does seem to be an intentional choice, though, so I'm curious why it's not simply Identifiers?

We are ignoring the Common Name because it is deprecated. Do you mean: why am I calling the struct field Subjects even though it is []acme.Identifier type? No real reason in particular except that I feel like most certificate users are familiar with the concept of SANs which are known as subjects instead of identifiers, which is more of an ACME thing IMO. But I could definitely call the field Identifiers instead. And maybe that is truer to what it is anyway.

@mholt
Copy link
Owner Author

mholt commented Mar 7, 2024

I decided to keep the CSRSource interface as-is (not passing in the private key). I don't think the private key needs to depend on the challenge performed, does it? Even if it does, the CSRSource that you specify at the beginning can be a type that is involved with the challenge process, or regardless, can have access to the private key if needed, rather than having to pass the private key into acmez (a third-party library for our users).

I renamed Subjects to Identifiers in the OrderParameters.

I've implemented the Replaces field for ARI draft-03 support.

Personally I think I'm content with this API and will merge it in soon as v2.

@hslatman
Copy link
Contributor

hslatman commented Mar 8, 2024

@mholt I'll give it another look, and hopefully a quick try in actual code.

Great that they're now Identifiers. My main concern was indeed that they're identifiers in ACME. Besides that, with SANs standing for Subject Alternative Names, they're more like "alternative names" for the Subject, than an actual Subject (although there's not that much of a difference in practice, of course).

@mholt
Copy link
Owner Author

mholt commented Mar 8, 2024

@hslatman Cool, yeah that makes sense. I haven't thought of it that way before.

I'll go ahead and merge this for now, maybe tag a beta release, and we can still make changes until the stable. Thank you!

@mholt mholt merged commit c0dcf7b into master Mar 8, 2024
@mholt mholt deleted the new-api branch March 8, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants