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

Fixes for http-01 stray tokens, dns-01 CNAMEs, contact e-mail format and updates; account security operations; misc #841

Open tlhackque opened 4 months ago

tlhackque commented 4 months ago

Fixes #693 Fixes #827 Fixes #839 Fixes #840 Fixes #267 Fixes #801 Fixes #818

Tests for the http-01 fixes should be added. See e#839 and #693 for the issues; I don't have servers for the missing protocols. The dns-01 fixes break some of the current tests - it's the tests that need fixing.

This restricts CNAMEs to what getssl itself is capable of handline. The previous code, as described in #840, was incompatible with the APIs used by the dns_add and dns_del dns_scripts. The restrictions don't seem onerous and avoid cryptic DNS entries.

Short form: CNAMEs in the validating domain must be from _acme-challenge.(something in the domain, or null) to _acme-challenge.(the same full domain name).(the domain updatable by the DNS scripts). See #840 for an example - which belongs in the documentation. [changed: see edit below]

This commit also includes a developer's script - most people can ignore it.

Additional commits in later comments.

Finally, note that tlhackque/certtools has useful tools for monitoring certificate expiration, and dealing with any stray dns-01 tokens.

Edit: A later commit allows CNAMEs to omit the client domain name in the target, but they still must begin with _acme-challenge.

tlhackque commented 4 months ago

Fixes #827 Automagically update the contact e-mail when ACCOUNT_EMAIL is changed in getssl.cfg.

Note that prior registrations included a space after mailto: when registering (e.g. mailto: @example.net). This made them invalid. Such registrations will be fixed the next time getssl runs.

Note that a simple way to update the contact e-mail without renewing/issuing certificates is to use getssl --account-id <any configured domain>

Footnote: The contact item is an array - it's possible to specify a list of e-mails. getssl doesn't support that. Maybe it should.

Edit: ... and now it does. Yet another commit.

tlhackque commented 4 months ago

Added the missing admin/security functions to rotate account key (key compromise, or policy) and deactivate account.

Could use tests - verified with LE.

These functions (and --account-id) really shouldn't require a domain. Fixing that requires more logic change than I have time for at the moment.

tlhackque commented 4 months ago

@timkimber : The test "failures" with this PR are due to invalid CNAMEs in the test environment, which getssl now detects.

These CNAMEs must have the format:

_acme-challenge.from-domain. CNAME _acme-challenge[.from-domain].to-domain.

where [from-domain] is optional.

The reason is that the dns_scripts unconditionally pre-pend _acme-challenge. to the host name of the TXT RR that they add. The from-domain name is what the validator looks for. The to-domain is where the txt records are added.

The current CNAMEs in the test environment have targets that could not be updated by the dns_scripts. Changing all the dns_scripts would be a fair bit of work, error-prone, and hard to test. Plus we don't know about other scripts using the API that haven't been contributed. Thus the restriction. (Which is what almost all articles on the ACME CNAME suggest.)

The test failures are getssl reporting that the CNAME target is not in this form.

The reasonable choices are:

  1. Change the current tests to use a valid (for getssl) CNAME.
  2. Leave the current tests as-is, but make these "failures" pass (this verifies the trivial diagnostic)
  3. (2) plus add new tests with valid CNAMEs.

You'll have to change the CNAMEs in the test environment for (1) or (3). I don't recommend (2) - it uses a lot of test time to validate a trivial diagnostic in getssl.

For a (currently) working example:

dig @8.8.8.8 _acme-challenge.andreasvonhuene.com txt _acme-challenge.www.andreasvonhuene.com txt

I put some dummy TXT records at the target that will last until the next time I run cleanup. Normally the targets of ACME CNAMEs only exist during validation.

The getssl in this PR has successfully added real validations, LE successfully followed the CNAMEs, and issued certificates.

It should be OK to merge.

Edit: updated required CNAME format to match later commit

tlhackque commented 4 months ago

Implemented a version of #267 ( @zazaz-de )'s token substitution in challenge ACLs.

Not clear why the proposal wanted to substitute $DOMAIN; $SAN seems more useful. But if you place an ACL in the main config file, maybe. I added both.

These seem harmless if not used.

Something similar might be useful for the certificate/key/chain files (in which case updating copy_file_to_location would be the better choice. But that would be more involved. Not letting the perfect be the enemy of the good. 7 years is long enough for the proposal to wait for some attention.

Also fixed some typos in the template domain config.

tlhackque commented 4 months ago

Fixed #801 - added support for specifying local IP address in nsupdate challenge responses.

Also rebased to latest master.

timkimber commented 4 months ago

@tlhackque thanks so much for all your recent changes, I'll start looking through and merging them as soon as I can (hopefully this week)

tlhackque commented 4 months ago

I relaxed the (new) restriction on CNAME targets to allow for hashed names. The target must start with _acme-challenge., but no longer requires that the source domain name follow.

One might use hashed names if worried that a stolen zone file on the updateable server could be used to find clients. That's pretty unlikely since one would have to catch it during an update. Of course, the hash is present in the (static) CNAME on the client. Nonetheless, while it's a lot easier to track down issues on the updatable server if the domain name is included, it was a well-intentioned, but unnecessary restriction.

In corner cases, a hash target also can prevent the target domain name from being too long. E.g., _acme-challenge.veryverylong.domain => _acme-challenge.veryverylong.domain.alsoverylong.maybelonger.target.domain. With a hash of the source domain, the insertion is a fixed length and becomes _acme-challenge.veryverylong.domain => _acme-challenge.<hash>.alsoverylong.maybelonger.target.domain.

Tested both ways (on real domains), including wildcard cert. Everyone is still happy.

Also, the next release of acme_token_check (in my certtools repo) has learned how to find, generate (and remove) CNAME redirects in addition to TXT tokens. Including hashed targets. If the source server supports DNS UPDATE, it can even install the CNAMEs for you. Otherwise, you can paste into your zone file/GUI.

acme_token_check would make your life easier when generating tests, so have a look. Release probably later today.

tlhackque commented 3 months ago

@timkimber I did release acme_token_check, with support for ACME verification redirection (CNAMEs).

Also, I made changes (mostly whitespace) to ensure that the account management commands don't require or play with any domain. Besides doing busy work, there can be side effects.

e.g. before, if you tried getssl --account-id, it would fail/display help for lack of a domain.

So you'd try getssl --acount-id foo because no domain applies. getssl happily created a domain directory for "foo".

Things go down hill.

Now, what happens is what you expect. Just what's necessary to register your account and execute the command.

It would be cleaner to reorganize the code, but with all the global variables & shared state, that seemed too risky.

If you suppress the whitespace changes (diff -b), the commit is actually quite small...

Hopefully you can merge before I find something else...

tlhackque commented 3 months ago

Should now fix #818 - and any cousin bugs. LC_ALL overrides all I18n variables (except LANGUAGE, which doesn't effect getssl).

tlhackque commented 3 months ago

@timkimber: Some of the test failures were not due to CNAME issues. Those had to do with curl version changes, and some FTP issues in the new token removals that use FTP variants. These were masked by curl and CNAME test issues. Fixed now.

DNS failures remain that I'm out of cycles to address. Not clear why the bad CNAME shows up randomly.

The only way to eliminate the bad CNAME restriction would be to change the dns_scripts API - and all the scripts. I'm very reluctant to do that, even though it's not conceptually difficult.

How are the CNAMEs setup for the tests? Below are 3 references in the tests where even the comments have invalid targets. I can't see how they ever worked. Maybe with dns_*_manual - and ignoring the suggested record?

u1-test-get_auth_dns-dig.bats:108:@test "Check get_auth_dns using dig CNAME (public dns)" {
u1-test-get_auth_dns-dig.bats:109:    # Test that get_auth_dns() handles scenario where CNAME query returns just a CNAME record
u1-test-get_auth_dns-dig.bats:114:    # www.duckdns.org.        600     IN      CNAME   DuckDNSAppELB-570522007.us-west-2.elb.amazonaws.com.

u2-test-get_auth_dns-drill.bats:119:@test "Check get_auth_dns using drill CNAME (public dns)" {
u2-test-get_auth_dns-drill.bats:125:    # Test that get_auth_dns() handles scenario where CNAME query returns just a CNAME record
u2-test-get_auth_dns-drill.bats:130:    # www.duckdns.org.        600     IN      CNAME   DuckDNSAppELB-570522007.us-west-2.elb.amaz

u7-test-get_auth_dns-nslookup.bats:116:@test "Check get_auth_dns using nslookup CNAME (public dns)" {
u7-test-get_auth_dns-nslookup.bats:117:    # Test that get_auth_dns() handles scenario where CNAME query returns just a CNAME record
u7-test-get_auth_dns-nslookup.bats:122:    # www.duckdns.org.        600     IN      CNAME   DuckDNSAppELB-570522007.us-west-2.elb.a

E.g. ubuntu18's nslookup:

#   Mar 25 19:02:05 get_auth_dns:1476 Cannot find nameserver record for _acme-challenge.ubuntu18.getssl.test, using parent domain ubuntu18.getssl.test
#   Mar 25 19:02:05 get_auth_dns:1476 nslookup  -debug -type=soa -type=ns ubuntu18.getssl.test
#   Mar 25 19:02:05 get_auth_dns:1476 Warning: Couldn't find primary DNS server - please set PUBLIC_DNS_SERVER or AUTH_DNS_SERVER in config
#   Mar 25 19:02:05 get_auth_dns:1476 This means getssl cannot check the DNS entry has been updated

This is the bad CNAME (acmedns):

#   getssl: UBUNTU-ACMEDNS-GETSSL.FREEDDNS.ORG: _acme-challenge.ubuntu-acmedns-getssl.freeddns.org uses a CNAME to 7268181b-7075-4dce-be51-9c20c205cf6e.auth.acme-dns.io, which does not start with '_acme-challenge.', which is required by getssl

Unbuntu14 fails to get installed token

#   Mar 25 18:54:32 add_dns_rr:1496 adding DNS RR via command: /getssl/dns_scripts/dns_add_challtestsrv a.ubuntu14.getssl.test FL1JUwuKTJqMdH_0NYRknqUsuKqCHkV8e8REqnZiI7U
#   Mar 25 18:54:32 check_challenge_completion_dns:1500 checking DNS at 10.30.50.3
#   Mar 25 18:54:32 check_challenge_completion_dns:1500 dig TXT _acme-challenge.a.ubuntu14.getssl.test @10.30.50.3
#   Mar 25 18:54:32 check_challenge_completion_dns:1500 check_result="FL1JUwuKTJqMdH_0NYRknqUsuKqCHkV8e8REqnZiI7U"
#   Mar 25 18:54:32 check_challenge_completion_dns:1500 expecting  "FL1JUwuKTJqMdH_0NYRknqUsuKqCHkV8e8REqnZiI7U"
#   Mar 25 18:54:32 check_challenge_completion_dns:1500 10.30.50.3 gave ... "FL1JUwuKTJqMdH_0NYRknqUsuKqCHkV8e8REqnZiI7U"
#   Mar 25 18:54:32 check_challenge_completion:1503 sending request to ACME server saying we're ready for challenge
#   Mar 25 18:54:32 send_signed_request:546 url https://pebble:14000/chalZ/c7b0hPsfrwTs1zIDldYhSkeuIRzbOg1-4MLQWAVkHX8
#   TDhurlRBlqS84q3hkJknSPD3s_LXqSUrJ8jkU1nlv-c
#   Mar 25 18:54:32 send_signed_request:546 using KID=https://pebble:14000/my-account/9
#   Mar 25 18:54:32 send_signed_request:546 payload = {}
#   getssl: ERROR curl "https://pebble:14000/chalZ/c7b0hPsfrwTs1zIDldYhSkeuIRzbOg1-4MLQWAVkHX8
#   TDhurlRBlqS84q3hkJknSPD3s_LXqSUrJ8jkU1nlv-c" failed with 3 and returned ""