Add support for IP SANs when using ACME provisioner #576

hslatman · 2021-11-09T20:56:06Z

IP SANS can now be specified when using the CLI to request a certificate from an ACME provisioner.

When no SAN is provided using the --san flag, while the subject provided can be parsed as an IP, the IP will be added as a SAN (IP Address). The IP will ~~NOT~~ also be set as the certificate Subject.

An example invocation looks like this:

$ step ca certificate --provisioner my-acme-provisioner 127.0.0.1 127.0.0.1.crt 127.0.0.1.key
✔ Provisioner: my-acme-provisioner (ACME)
Using Standalone Mode HTTP challenge to validate 127.0.0.1 .. done!
Waiting for Order to be 'ready' for finalization .. done!
Finalizing Order .. done!
✔ Certificate: 127.0.0.1.crt
✔ Private Key: 127.0.0.1.key

The certificate contains an IP Address SAN for 127.0.0.1. The Subject will ~~be empty~~ contain 127.0.0.1 too.

A more extensive example looks like this:

$ step ca certificate --provisioner my-acme-provisioner --san 192.168.32.32 --san 127.0.0.1 --san catest.local ipverification.local ipverification.local.crt ipverification.local.key
✔ Provisioner: my-acme-provisioner (ACME)
Using Standalone Mode HTTP challenge to validate catest.local .. done!
Using Standalone Mode HTTP challenge to validate 192.168.32.32 .. done!
Using Standalone Mode HTTP challenge to validate 127.0.0.1 .. done!
Using Standalone Mode HTTP challenge to validate ipverification.local .. done!
Waiting for Order to be 'ready' for finalization .. done!
Finalizing Order .. done!
✔ Certificate: ipverification.local.crt
✔ Private Key: ipverification.local.key

The resulting certificate contains SANs for the DNS names catest.local and ipverification.local and for the IP Addresses 127.0.0.1 and 192.168.32.32. The Subject is ipverification.local.

The CLI currently only supports HTTP challenges. DNS and TLS-ALPN (tracked in #529) are not (yet) supported.

The logic for (not) adding certain identifiers and receiving challenges became a little bit more unwieldy. I think it's good if I add some tests and/or refactor a bit. Another option may be to wait a little and make the implementation more strict when adding additional options, like ACME EAB support, to the CLI.

This PR currently introduces a slight change in behaviour w.r.t. the subject as one of the ACME identifiers. It will now be included for DNS names, also when Let's Encrypt is not in use. I think it won't break things in "normal" environments. In addition to that I haven't tested against a different ACME CA than step-ca yet; may be a good thing to do before completing this.

Fixes #575.

Fixes smallstep#575

dopey · 2021-11-10T21:33:56Z

In general, I like the PR. Everything looks good to me. But I'm curious, why not allow IP SAN as the subject? We allow it for step ca certificate 127.0.0.1 foo.crt foo.key.

hslatman · 2021-11-10T23:20:08Z

The main reason I treated this case differently is that usage of Common Name is deprecated and putting IP addresses in there seems to be discouraged, at least for public CAs. So far I haven't found a conclusive answer in related RFCs, though, let alone for internal CAs. By only applying the logic for IP addresses, it would still be done for DNS names as the subject, keeping that logic intact (at least for now).

For consistency it may be possible to change how the subject argument is used when requesting a certificate. It could be added to the appropriate slice of identifiers (i.e. DNS, IPs) in the CSR and not be added to the Common Name, for example. CertMagic seems to do the same here: no mention of a subject in the CSR; Only SANs are set. The CA could still force the CN based on the first (DNS) SAN using the forceCN option.

I think the logic becomes a bit more complex if the IP is allowed as the subject. When implementing the fix I observed that the CSR could deviate from the identifiers in the ACME Order, if not corrected appropriately with an IP as Subject. I've also seen two challenges having to be performed for a single identifier 127.0.0.1, once as an IP and once as a DNS. They're not hard blockers, but need to be fixed if a Subject must have the option to be an IP.

maraino

Looks good, but I left a couple of comments

maraino · 2021-11-11T20:16:32Z

utils/cautils/acmeutils.go

+			}
+		}
+		subjectIsNotAnIP := net.ParseIP(af.subject) == nil
+		if !hasSubject && subjectIsNotAnIP {


I think we should also add && len(ips) == 0, if not I think you cannot create a certificate with

CN=localhost IP:[127.0.0.1] DNS: []

And that only if ACME allows you to create certs without a DNS name.

maraino · 2021-11-11T20:19:51Z

utils/cautils/acmeutils.go

+		if subjectIsNotAnIP {
+			_csr.Subject = pkix.Name{


Is this in the spec? Why subject cannot be an IP?

@maraino: It is not; it would've been a lot simpler if it were 😅

So far I haven't found a normative reference that explicitly forbids putting IPs in the Subject Common Name. I did find and read some general (best) practices and conclusions (that others reached after reading the non-conclusive RFCs), though. These were also partially based on practices for the Web PKI (i.e. CAB Forum). The general consensus seems to be that the Common Name should actually not be used anymore, as it's deprecated by Subject Alternative Names and many clients operate on just those. Next to that, putting an IP in the Subject seems to be discouraged in general. It is used when having to support legacy software, though.

Technically I don't see a reason to put anything in the Subject Common Name at all. It has has been deprecated, and clients should've been updated to not use the Common Name when performing their checks. Changing that logic could break some current deployments, so the decision to not fill it shouldn't be taken too lightly, IMO, so it's good that you and @dopey are critical about this. I also understand that there are legacy use cases that need to be supported, but I think those should be exceptions and not the default.

Apart from the above, I did in fact try to set an IP in the Subject before pushing my changes. IIRC I observed that step-ca would perform two challenges for a single IP identifier if it was also in the Subject (I think for both an IP and as a DNS) or would report a mismatch between the CSR and the ACME Order object, depending on my approach. I'm pretty sure these can be fixed if it is decided to support IPs as the Subject, but it might involve a bit more logic and/or changes in step-ca also. I'd probably want to refactor some of this logic to make it more easily testable in that case, then.

Only partially related, but it may influence the decision: yesterday I had a chat with @tashian about the CLI being able to renew certificates when using the ACME protocol. Currently renewals work, but only after generating a new account key and new authorizations are performed. The CLI is effectively stateless in this regard. It may be good if the CLI ACME implementation would be changed to reuse an existing account key, especially if we also want to support ACME EAB. Now that's a larger effort, but we may be able to integrate CertMagic or acmez instead, to not have to (re)do all of this ourselves. Both do not seem to provide a way to add anything as the Subject Common Name, though.

I have to check what the CAB Forum exactly says, but my understanding is that it should not be used to authenticate a server, but I think it's ok to have the CN as an information field for a server.

I'm ok with integrating an acme client library, ours just does the job, but for example, it does more requests than necessary as it doesn't reuse the nonces. But at the same time, I don't think we need all features that lego or certbot have.

We basically have to integrate it well with our step-ca, but at the same time, you should be able to do basic stuff with let's encrypt or any other standard ACME server.

I have to check what the CAB Forum exactly says, but my understanding is that it should not be used to authenticate a server, but I think it's ok to have the CN as an information field for a server.

That makes sense, indeed. We can treat it as an opaque string if provided as an argument or set in a CSR. The default could become to not set it; adding a flag to set it seems appropriate to me. The ACME authorizations shouldn't take the subject into account, which might need a change in step-ca. I'll update this PR with the changes soon.

I'm ok with integrating an acme client library, ours just does the job, but for example, it does more requests than necessary as it doesn't reuse the nonces. But at the same time, I don't think we need all features that lego or certbot have.

We basically have to integrate it well with our step-ca, but at the same time, you should be able to do basic stuff with let's encrypt or any other standard ACME server.

I'll have a look at what the impact is for changing the ACME client implementation, but won't start implementing one till we decide to do so.

If we decide to use a library it should be part of a separate PR.

We now allow IPs to be in the subject. The CLI also ensures that when a Subject Common Name can be parsed as an IP it is included in the Order, so that authorization can be performed by the CA.

To support IPs in the subject using `step-cli`, this PR ensures that Subject Common Names that can be parsed as an IP are also checked to have been authorized before. The PR for `step-cli` is here: github.com/smallstep/cli/pull/576.

hslatman · 2021-12-13T15:26:12Z

@maraino: IPs in the subject are now allowed. If a subject can be parsed as an IP, it will be in the list of Order Identifiers and in the IP address slice of the CSR.

Together with the change in step-ca (github.com/smallstep/certificates/pull/773) this results in an authorization being performed against the IP in the subject. Without the change in step-ca, the IP in the subject would be in the final certificate, but would not've been authorized before that.

While step-ca slightly alters the CSR in this case, I think this is the right thing to do.

dopey · 2021-12-17T06:29:47Z

utils/cautils/acmeutils.go

+		// or IP addresses slice and the corresponding type of
+		// identifier is added to the slice of identifers.
+		if !hasSubject {
+			ip := net.ParseIP(af.subject)


Similar example of coding convention difference. In my opinion we should either write this as a one liner, or make an argument for why breaking it up is always better and start using the new convention everywhere. I'm gonna sound pedantic and whiny, but it makes the code harder to read when we're using two different conventions to handle the same situations throughout.

dopey

lgtm

Add support for IP SANS when using ACME provisioner

def58f9

Fixes smallstep#575

github-actions bot added the needs triage Waiting for discussion / prioritization by team label Nov 9, 2021

hslatman changed the title ~~Add support for IP SANS when using ACME provisioner~~ Add support for IP SANs when using ACME provisioner Nov 9, 2021

maraino self-requested a review November 11, 2021 19:41

maraino requested changes Nov 11, 2021

View reviewed changes

dopey removed the needs triage Waiting for discussion / prioritization by team label Nov 18, 2021

dopey assigned hslatman Dec 2, 2021

Merge branch 'master' into hs/acme-ip-sans

a27bf7e

github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 7, 2021

hslatman removed the needs triage Waiting for discussion / prioritization by team label Dec 7, 2021

Allow IPs in the CSR subject

d5ccc27

We now allow IPs to be in the subject. The CLI also ensures that when a Subject Common Name can be parsed as an IP it is included in the Order, so that authorization can be performed by the CA.

github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 13, 2021

Clean up logic

444f1fc

hslatman requested a review from maraino December 13, 2021 15:22

dopey reviewed Dec 17, 2021

View reviewed changes

hslatman added 2 commits December 22, 2021 10:31

Merge branch 'master' into hs/acme-ip-sans

84697f4

Fix code style

7a1d616

dopey self-requested a review December 23, 2021 01:22

dopey approved these changes Dec 23, 2021

View reviewed changes

hslatman merged commit fd49998 into smallstep:master Dec 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for IP SANs when using ACME provisioner #576

Add support for IP SANs when using ACME provisioner #576

hslatman commented Nov 9, 2021 •

edited

Loading

dopey commented Nov 10, 2021

hslatman commented Nov 10, 2021 •

edited

Loading

maraino left a comment

maraino Nov 11, 2021

maraino Nov 11, 2021

hslatman Nov 11, 2021 •

edited

Loading

maraino Nov 11, 2021

maraino Nov 11, 2021

hslatman Nov 12, 2021

maraino Nov 12, 2021

hslatman commented Dec 13, 2021 •

edited

Loading

dopey Dec 17, 2021

dopey left a comment

Add support for IP SANs when using ACME provisioner #576

Add support for IP SANs when using ACME provisioner #576

Conversation

hslatman commented Nov 9, 2021 • edited Loading

dopey commented Nov 10, 2021

hslatman commented Nov 10, 2021 • edited Loading

maraino left a comment

Choose a reason for hiding this comment

maraino Nov 11, 2021

Choose a reason for hiding this comment

maraino Nov 11, 2021

Choose a reason for hiding this comment

hslatman Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

maraino Nov 11, 2021

Choose a reason for hiding this comment

maraino Nov 11, 2021

Choose a reason for hiding this comment

hslatman Nov 12, 2021

Choose a reason for hiding this comment

maraino Nov 12, 2021

Choose a reason for hiding this comment

hslatman commented Dec 13, 2021 • edited Loading

dopey Dec 17, 2021

Choose a reason for hiding this comment

dopey left a comment

Choose a reason for hiding this comment

hslatman commented Nov 9, 2021 •

edited

Loading

hslatman commented Nov 10, 2021 •

edited

Loading

hslatman Nov 11, 2021 •

edited

Loading

hslatman commented Dec 13, 2021 •

edited

Loading