srvrco / getssl

obtain free SSL certificates from letsencrypt ACME server Suitable for automating the process on remote servers.
GNU General Public License v3.0
2.07k stars 372 forks source link

DNS-01 validation for non-wildcard names #843

Open adaugherity opened 4 months ago

adaugherity commented 4 months ago

Describe the bug DNS validation via Sectigo's ACME service currently only works with wildcard hostnames. I'm not sure if it is a JSON-parsing bug or if Sectigo's response payload is different than expected, but this test is failing because wildcard is not an empty string (-z) but in fact false.

To Reproduce We have obtained a key from Sectigo to issue certificates for certain subdomains we control. With these settings:

VALIDATE_VIA_DNS="true"
DNS_ADD_COMMAND=/usr/bin/true
DNS_DEL_COMMAND=/usr/bin/true

they will issue certificates for hostnames inside said subdomains and return as valid, so DNS record creation isn't needed. (I don't currently have scripting/API access to our DNS anyway.)

However with getssl this currently only works for wildcard certs. It fails for non-wildcard hostnames (including e.g. a cert for both *.mydomain.example.com and mydomain.example.com) -- in that case, the wildcard name is validated but the non-wildcard one is not, and then getssl attempts to create TXT records, which obviously doesn't work.

Running in debug mode I see it loop through all the subjectAltName values, each of which succeeds with valid status, but then getssl doesn't think the non-wildcard names are valid, due to the false vs empty string issue. It took tracing with bash -x to reveal that the -z "$wildcard" test was failing.

Sanitized debug output:

Requesting authorizations link for https://acme.sectigo.com/v2/InCommonRSAOV/authz/XXXXX

url https://acme.sectigo.com/v2/InCommonRSAOV/authz/XXXXX

using KID=https://acme.sectigo.com/v2/InCommonRSAOV/account/YYYYY

payload =

responseHeaders HTTP/1.1 200 OK
Server: nginx
Date: Tue, 19 Mar 2024 19:59:45 GMT
Content-Type: application/json
Content-Length: 298
Connection: keep-alive
Replay-Nonce: P9[...]
Cache-Control: max-age=0, no-cache, no-store
Access-Control-Allow-Origin: *
Link: <https://acme.sectigo.com/v2/InCommonRSAOV>;rel="index"
Retry-After: 300
Strict-Transport-Security: max-age=15724800; includeSubDomains

response {"identifier":{"type":"dns","value":"mydomain.example.com"},"status":"valid","expires":"2024-04-18T19:59:35Z","challenges":[{"type":"sectigo-dns-01","url":"https://acme.sectigo.com/v2/InCommonRSAOV/chall/xxxx","status":"valid","validated":"2024-03-19T19:59:35Z"}],"wildcard":false}

code 200

response status = valid

wildcard=false
Verifying mydomain.example.com

authlink response =

uri json was blank

keyauthorization json was blank.uxh[...]

auth_key ME[...]

adding DNS RR via command: /usr/bin/true mydomain.example.com ME[...]

Expected behavior Save validation results when the payload contains "wildcard":false.

With this simple diff, getssl properly acquires certificates for non-wildcard hostnames.

diff --git a/getssl b/getssl
index 3eab631..d200e91 100755
--- a/getssl
+++ b/getssl
@@ -1234,6 +1234,9 @@ create_order() {
       authdomain=$(json_get "$response" "identifier" "value")
       wildcard=$(json_get "$response" "wildcard")
       debug wildcard="$wildcard"
+      if [[ "$wildcard" == "false" ]]; then
+        wildcard=""
+      fi
       # find array position (This is O(n2) but doubt that we'll see performance issues)
       dn=0
       for d in "${alldomains[@]}"; do

I didn't make a PR yet since I'm not sure what the "proper" fix is. Should the wildcard value always be a boolean, and the empty/non-empty test is in error, or should we accept both "false" and "" as non-wildcard and consider everything else as wildcard?

Operating system (please complete the following information):

Additional context With the above diff, the debug output differs following the wildcard=false line:

response status = valid

wildcard=false

Saving authorization response for mydomain.example.com for domain alldomains[2]

Response = {"identifier":{"type":"dns","value":"mydomain.example.com"},"status":"valid","expires":"2024-04-18T20:11:22Z","challenges":[{"type":"sectigo-dns-01","url":"https://acme.sectigo.com/v2/InCommonRSAOV/chall/xxxx","status":"valid","validated":"2024-03-19T20:11:22Z"}],"wildcard":false}
[...]
Verifying mydomain.example.com
mydomain.example.com is already validated
pbhenson commented 3 months ago

Heh, I just ran into the same issue. I've been using Incommon ACME for a year or so for both wildcard and non-wildcard certs, then it mysteriously broke within the past few weeks. I tracked it down to the same code you did, but fixed it a little differently:

1233a1234,1236
>       if [ -z "$wildcard" ] ; then
>         wildcard=false
>       fi
1240c1243
<         if [[ ( "$lower_d" == "$authdomain" && -z "$wildcard" ) || ( "$lower_d" == "*.${authdomain}" && -n "$wildcard" ) ]]; then
---
>       if [[ ( "$lower_d" == "$authdomain" && "$wildcard" == "false" ) || ( "$lower_d" == "*.${authdomain}" && "$wildcard" == "true" ) ]]; then

IMHO, per spec, JSON booleans are either "true" or "false", so checking those values seems more "correct".

I assume in the past Incommon just didn't include the wildcard attribute if the request wasn't for a wildcard, and now they do with the value "false". Per the ACME spec for wildcard certs:

https://datatracker.ietf.org/doc/html/rfc8555

"The returned authorization MUST include the optional "wildcard" field, with a value of true."

I guess either leaving it out or setting it to false for non-wildcard certs is acceptable from the server side.

jsnider2 commented 3 months ago

I ran into this as well. RFC 8555 section 7.1.4 clearly reads that it can be either true or absent. present and false is not allowed. getssl is mostly correct in that if it is present it should be able to assume it is "true," but it is a bit fragile to just check for empty or not.

wildcard (optional, boolean): This field MUST be present and true for authorizations created as a result of a newOrder request containing a DNS identifier with a value that was a wildcard domain name. For other authorizations, it MUST be absent. Wildcard domain names are described in Section 7.1.3.

I fixed it in my own fork by checking for the value "true" which as we see above is the only allowed value. I put that in as a PR #844.

I also have a ticket open with Sectigo to see if they will fix this. I don't have high hopes, but if by some miracle they actually do something I'll share it here.

adaugherity commented 3 months ago

There's actually an erratum clarifying that either absent or "false" is acceptable, so upon further reading I don't believe Sectigo is being non-compliant. Note that the example in that section contains "wildcard": false!

Edit: this erratum was reported in 2020, but just verified on 2024-03-22. Coincidence? Maybe, but awfully timely!

I agree that interpreting "true" as wildcard and everything else as not makes the most sense.