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

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

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

tlhackque
Copy link
Contributor

@tlhackque tlhackque commented Mar 17, 2024

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
Copy link
Contributor Author

tlhackque commented Mar 17, 2024

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 tlhackque changed the title Fixes for http-01 stray tokens, dns-01 CNAMEs Fixes for http-01 stray tokens, dns-01 CNAMEs, contact e-mail format and updates Mar 17, 2024
@tlhackque
Copy link
Contributor Author

tlhackque commented Mar 18, 2024

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 tlhackque changed the title Fixes for http-01 stray tokens, dns-01 CNAMEs, contact e-mail format and updates Fixes for http-01 stray tokens, dns-01 CNAMEs, contact e-mail format and updates; account security operations Mar 18, 2024
@tlhackque
Copy link
Contributor Author

tlhackque commented Mar 18, 2024

@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
Copy link
Contributor Author

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 tlhackque changed the title Fixes for http-01 stray tokens, dns-01 CNAMEs, contact e-mail format and updates; account security operations Fixes for http-01 stray tokens, dns-01 CNAMEs, contact e-mail format and updates; account security operations; misc Mar 19, 2024
@tlhackque
Copy link
Contributor Author

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

Also rebased to latest master.

@timkimber
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

tlhackque commented Mar 22, 2024

@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
Copy link
Contributor Author

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

@tlhackque
Copy link
Contributor Author

@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 ""

FTP_PORT not used by ftp.

No code for sftp, davfs, ftpes, or ftps.

Needs tests, but at least this won't fall thru to attempting to delete
from local file system.
This is useful for debugging; it leaves any tokens in the DNS &
records its environment.

It's only meaningful for debuggers (and some problem reports
for which ask for them.
Also fixes bug that caused previous registrations to be invalid.
RFC operations for account security:

--new-account-key replaces the account key with a new one.
  Can modify the type or size as well. (update .cfg first)
  Does not affect certificate validity or pending operations.

--DEACTIVATE-account permanently deactivates the account on
  the server.  Per RFC, can not be revived.  Should not revoke
  existing certificates. (Server's choice.)
Idea from srvrco#267

Fixes typos in template domain.cfg
It's OK for the target of a CNAME not to include the source domain.

It's handy for debug and for system management.

But some people prefer a hash.  We can handle that.
Skip everything having to do with domains & certificates
when doing

--account-id, --new-account-key, --DEACTIVATE-account

This avoids the need to specify a domain name, creating
directories, trying to check the remote - and other unnecessary
(and sometimes harmful) work.

Most of the diffs in this commit are white space.
Handle e-mail update with buggy 409 responses from registration.

Improve contact parsing by replacing call to json_get, which
doesn't seem to handle string array values well.  (It's also
easier to parse the values at the same time.)

No reason to save register response JSON in TEMP_DIR,
so don't.  Appears to be stale debugging code.

Exit after deactivating account.
FIXES srvrco#818 (I hope).

in srvrco#818, @mslavkov reported that date fails in the BG.UTF-8 locale, but
that LC_ALL=C resolved the issue.


Since we already export LANG=C, that would seem to indicate that LC_TIME
is overriding it.  LC_ALL is the safer (stronger) choice.
SERVER_TYPE implies a port number (and possibly s_client options).

Previously, these were hard-coded, requiring a code change for any
new/unique services.

Now, /etc/services is used, so every assigned name is available, and
new services "just work".

The old alias names (and renames) are supported.  And the old
hardcoded defaults will be used if /etc/services is not available.

SERVICES_FILE can be defined to local taste - e.g. on windows,
C:\Windows\System32\drivers\etc\services is equivalent.
Also make constant arrays in find_service_port() read-only
Prompt for confirmation of account deactivation.

If a domain is specified, allow its getssl.cfg to specify
the account key & type.

Don't create an account key for rotation or deactivate if
none exists.
curl isn't changing directory to the specified directory.

Make it explicit in the DELE command.
Replaced with --ssl-reqd.

Note that --ftp-ssl-reqd is an old alias for --ssl-reqd.
--ftp-ssl-reqd is equivalent, but could eventually go away.

-ssl-reqd has been supported since curl version 7.20.0 - in 2010
 (though a related CVE was fixed in 7.79.0 in 2021...)

So this change shouldn't inconvenience any getssl users.
Apparently centos6 is stuck on curl version 7.19, just before
--ssl-reqd turned up in 7.20.  Wow!

Check curl version and select --ssl-reqd for version 7.20+.
Adds -starttls for all protocols currently documented by
openssl s_client (their master branch).

Also allows REMOTE_EXTRA in config files to override built-in
usage.

Reordered extra_cmds to match openssl documentation so it's
easier to see when openssl adds new protocols.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants