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

Certificate request subject commonName domain is not taken into account #304

Open
bruncsak opened this issue Mar 11, 2020 · 15 comments
Open

Comments

@bruncsak
Copy link

RFC8555 tells:

   The CSR MUST indicate the exact same
   set of requested identifiers as the initial newOrder request.
   Identifiers of type "dns" MUST appear either in the commonName
   portion of the requested subject name or in an extensionRequest
   attribute [RFC2985] requesting a subjectAltName extension, or both.

When the subject of the CSR has CN=abc.dom.org and the subjectAltName has only DNS:xyz.dom.org, by the RFC an order must be created with both abc.dom.org and xyz.dom.org as identifiers. After making the order ready, the submitted CSR is not accepted with the following error:

{
   "type": "urn:ietf:params:acme:error:unauthorized",
   "detail": "Order includes different number of DNSnames identifiers than CSR specifies",
   "status": 403
}

For reference see:
https://community.letsencrypt.org/t/the-way-domain-name-in-the-subject-of-the-certificate-request-treated/116107

@felixfontein
Copy link
Contributor

The RFC does not say that domains from commonName must be accepted by the CA. In fact, commonName is deprecated (see for example https://security.stackexchange.com/questions/69156/using-commonname-for-dns-vs-subjaltname). This has also been discussed both here and in the Let's Encrypt community forum.

@bruncsak
Copy link
Author

Hi @felixfontein,
Thanks for the info, it is really useful! So, at the end, is there any plan to make pebble conform to RFC8555 in that respect, or will it stay nonconforming as today?

@felixfontein
Copy link
Contributor

@bruncsak as I see it, Pebble does conform to RFC8555 in that respect (as do Boulder and BuyPass).

@bruncsak
Copy link
Author

Sorry, I do not want to enter into debate on that, but the behavior is different between pebble and letsencrypt/buypass. See the earlier mentioned reference: https://community.letsencrypt.org/t/the-way-domain-name-in-the-subject-of-the-certificate-request-treated/116107
For the order, containing the same set of identifiers letsencrypt's and buypass's staging server issues the certificate, while pebble is not (for the same CSR). So how do you interpret the section I quoted from the RFC? My interpretation is the following:
I create an order with two identifiers: abc.dom.org and xyz.dom.org. When the order state is ready, I am permitted to submit a CSR to get a certificate where (I translate the text):

   The CSR MUST indicate the exact same
   set of requested identifiers as the initial newOrder request (abc.dom.org + xyz.dom.org).
   Identifiers of type "dns" MUST appear either in the commonName
   portion of the requested subject name (abc.dom.org) or in an extensionRequest
   attribute [RFC2985] requesting a subjectAltName extension (xyz.dom.org), or both (does not apply here).

@jsha
Copy link
Contributor

jsha commented Mar 11, 2020

Hi @bruncsak,

Thanks for reporting! And @felixfontein thanks for digging out the link to the previous Pebble issue. The Pebble behavior is indeed intentional. In the early days of Let's Encrypt we chose to accept CSRs that only specify names in the CN without specifying them in the SAN. That was probably a mistake, but a minor one, and correcting it now would introduce compatibility problems with little benefit.

Our general goal with Pebble is to be a bit stricter about things, not to match all behavior of other implementations like Boulder and Buypass. So we're planning to keep the current behavior. If you'd like to maximize compatibility between the three implementations, I think putting all names in the SAN should work.

@bruncsak
Copy link
Author

bruncsak commented Mar 12, 2020

Thanks for the reply and clarification @jsha!

Is there a RFC8555 divergences document for pebble, similarly, as boulder has the following: https://github.com/letsencrypt/boulder/blob/master/docs/acme-divergences.md ?

If you'd like to maximize compatibility between the three implementations, I think putting all names in the SAN should work.

As I am writing code of an ACME client, I would like to be generic enough to handle all possible cases. There is an option where the CSR is not generated by the client, but received as input, so it is not possible to change the content of the CSR. Most likely I will add specific code to better match the pebble's behavior.

When boulder receives a CSR where the subject is empty, it adds one domain as commonName.
Unlike buypass, it happily issues the certificate with empty subject. This certificate looks really weird looking at for example with firefox certificate viewer: you have to click on a blank line to see the details of the certificate. Really funny!

@bruncsak
Copy link
Author

So, I implemented a workaround in my client: bruncsak/ght-acme.sh@0f27ce7

@felixfontein
Copy link
Contributor

@bruncsak I still don't understand why you think that RFC8555 mandates that commonName is supported. The part you quote from the RFC only says that every DNS identifier specified in newOrder must appear in commonName, SAN or both. It does not say that the CA has to accept CSRs which use commonName at all.

@jsha what's your interpretation of the RFC?

@bruncsak
Copy link
Author

Oh, I see what you mean. All ACME servers are compatible with RFC8555, but different additional restriction on the CSR are not excluded which leads to incompatible behavior. So my code is correct and needed, just the comments are misleading.

@bruncsak
Copy link
Author

So here is the patch to my client, fixing the comments: bruncsak/ght-acme.sh@3d46cf6

@felixfontein
Copy link
Contributor

@bruncsak looks much better IMO :)

@jsha
Copy link
Contributor

jsha commented Mar 16, 2020

The above looks good, thanks for hashing it out! It sounds like maybe a useful addition to Pebble would be for it to have a more explicit error in this case. For instance, "The CSR's Subject commonName contains a domain name that is not in the subjectAlternativeNames of the CSR."

What do you think?

@bruncsak
Copy link
Author

Today pebble accepts domain name in the Subject commonName, but it totally disregards its value, like it would not be there. Your proposed change would be more than cosmetics; before it is normal condition (certificate issued) then it becomes error condition, refusing the certificate request.

@felixfontein
Copy link
Contributor

All improved error messages are cosmetic, but still very helpful. In this case I would definitely prefer having "The CSR's Subject commonName contains a domain name that is not in the subjectAlternativeNames of the CSR." instead of the current "Order includes different number of DNSnames identifiers than CSR specifies". This saves client developers quite some time to find out what the problem actually is.

@bruncsak
Copy link
Author

bruncsak commented Mar 21, 2020

Yes, that could be good to have a specific error message for that case.
What about the case where one identifier is in the order, and this identifier is in the CSR's subjectAlternativeNames, and there is a different identifier in the Subject commonName .
Would the CSR still be accepted as before?

trevordixon added a commit to trevordixon/crypto that referenced this issue Jun 21, 2020
Allows autocert to work with Pebble (see letsencrypt/pebble#304).
trevordixon added a commit to trevordixon/crypto that referenced this issue Mar 22, 2021
Allows autocert to work with Pebble (see letsencrypt/pebble#304).
trevordixon added a commit to trevordixon/crypto that referenced this issue Mar 22, 2021
Allows autocert to work with Pebble (see letsencrypt/pebble#304).
trevordixon added a commit to trevordixon/crypto that referenced this issue Mar 22, 2021
Allows autocert to work with Pebble (see letsencrypt/pebble#304).
trevordixon added a commit to trevordixon/crypto that referenced this issue Mar 22, 2021
Allows autocert to work with Pebble (see letsencrypt/pebble#304).
gopherbot pushed a commit to golang/crypto that referenced this issue Mar 22, 2021
More compliant with the spec and allows autocert to work
with Pebble (see letsencrypt/pebble#304).

Fixes golang/go#39746.

Change-Id: I0f41d5b41800d57eb53055cad248e50573c6070f
GitHub-Last-Rev: 777115c
GitHub-Pull-Request: #143
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/294389
Reviewed-by: Filippo Valsorda <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Trust: Dmitri Shuralyov <[email protected]>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
More compliant with the spec and allows autocert to work
with Pebble (see letsencrypt/pebble#304).

Fixes golang/go#39746.

Change-Id: I0f41d5b41800d57eb53055cad248e50573c6070f
GitHub-Last-Rev: 777115c545a5266609fad6888d24d586ed4c2916
GitHub-Pull-Request: golang/crypto#143
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/294389
Reviewed-by: Filippo Valsorda <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Trust: Dmitri Shuralyov <[email protected]>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
More compliant with the spec and allows autocert to work
with Pebble (see letsencrypt/pebble#304).

Fixes golang/go#39746.

Change-Id: I0f41d5b41800d57eb53055cad248e50573c6070f
GitHub-Last-Rev: 777115c
GitHub-Pull-Request: golang#143
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/294389
Reviewed-by: Filippo Valsorda <[email protected]>
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Trust: Dmitri Shuralyov <[email protected]>
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

No branches or pull requests

3 participants