From a44f5acdf373d8691a4ca6607eee52fae802b69d Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Fri, 28 Feb 2020 11:49:06 +1100 Subject: [PATCH] letsencrypt: force renewal on certificate change There is a bug, or misfeature, in acme.sh using dns manual mode where it will not renew the certificate when new domains are added to an existing certificate. It appears to generate the TXT record requests correctly, but then when we renew the certificate it thinks it is not time and skips it. This is filed upstream with [1] however we can work around it, and generally be better anyway. For each letsencrypt host, during certificate request we build up the "acme_txt_required" key which is a list of TXT record tuples. Currently we keep the challenge domain in the first entry, which is not useful (all our hosts have the same challenge domain, amce.opendev.org). Modify this to be the certificate key from the host config. To be clear; when a host has letsencrypt_certs: hostname-cert-main: hostname.opendev.org altname.opendev.org hostname-cert-secondary: secondary.opendev.org secondaryalt.opendev.org acme_txt_required when renewing all certs will end up looking like: [ (hostname-cert-main, ), (hostname-cert-main, ), (hostname-cert-secondary, ), (hostname-cert-secondary, >) ] In the certificate creation path, we walk "acme_txt_required" and take the unique 0-value entries; this gives us the list of keys in "letsencrypt_certs" which were actually updated. We can then force renewal for these certs, because we know they changed in some way that requires reissuing them (within renewal time, or new domains). This isn't just a work-around, it is generically better too. Previously if any cert on host required an update, we would try to update them all. This would be a no-op; acme.sh would just skip doing anything; but now we don't even have to call into the renewal if we know nothing has changed. [1] https://github.com/acmesh-official/acme.sh/issues/2763 Change-Id: I1e82c64217d46d7e1acc0111dff4db2f0062c42a --- .../files/driver.sh | 11 ++++++++++ .../letsencrypt-create-certs/tasks/main.yaml | 14 +++++++++++- .../letsencrypt-request-certs/tasks/acme.yaml | 22 +++++++++---------- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/playbooks/roles/letsencrypt-acme-sh-install/files/driver.sh b/playbooks/roles/letsencrypt-acme-sh-install/files/driver.sh index 99b0e15869..d8576f85c9 100644 --- a/playbooks/roles/letsencrypt-acme-sh-install/files/driver.sh +++ b/playbooks/roles/letsencrypt-acme-sh-install/files/driver.sh @@ -45,10 +45,21 @@ if [[ ${1} == "issue" ]]; then elif [[ ${1} == "renew" ]]; then shift; for arg in "$@"; do + # NOTE(ianw) 2020-02-28 : we only set force here because of a + # bug/misfeature in acme.sh dns manual-mode where it does not + # notice that the renewal is required when we update domain + # names in a cert + # (https://github.com/acmesh-official/acme.sh/issues/2763). + # This is safe (i.e. will not explode our quota limits by + # constantly renewing) because Ansible only calls this path + # when TXT records have been installed for this certificate; + # i.e. we will never run this renewal unless it is actually + # required. $ACME_SH ${STAGING} \ --cert-home ${CERT_HOME} \ --no-color \ --yes-I-know-dns-manual-mode-enough-go-ahead-please \ + --force \ --renew \ $arg 2>&1 | tee -a ${LOG_FILE} done diff --git a/playbooks/roles/letsencrypt-create-certs/tasks/main.yaml b/playbooks/roles/letsencrypt-create-certs/tasks/main.yaml index 449e2f09c7..3dc8c91da1 100644 --- a/playbooks/roles/letsencrypt-create-certs/tasks/main.yaml +++ b/playbooks/roles/letsencrypt-create-certs/tasks/main.yaml @@ -7,7 +7,19 @@ msg: "acme_txt_required is not defined; was letsencrypt-request-certs run?" when: acme_txt_required is not defined +# acme_txt_keys is a list of tuples +# +# (key from letsencrypt_certs, required TXT record) +# +# So in words, we walk acme_txt_required and keep a list of the unique +# 0-values of each entry. This is then the keys from +# letsencrypt_certs that actually had updates; these are the only ones +# we need to do a renewal for. +- name: Generate list of changed certificates + set_fact: + acme_txt_changed: '{{ acme_txt_required|map("first")|list|unique }}' + - name: Include ACME renewal include_tasks: acme.yaml loop: "{{ query('dict', letsencrypt_certs) }}" - when: acme_txt_required | length > 0 + when: item.key in acme_txt_changed diff --git a/playbooks/roles/letsencrypt-request-certs/tasks/acme.yaml b/playbooks/roles/letsencrypt-request-certs/tasks/acme.yaml index d52f8020e1..8c79f8ac43 100644 --- a/playbooks/roles/letsencrypt-request-certs/tasks/acme.yaml +++ b/playbooks/roles/letsencrypt-request-certs/tasks/acme.yaml @@ -14,17 +14,15 @@ environment: LETSENCRYPT_STAGING: '{{ "1" if letsencrypt_use_staging else "0" }}' -# NOTE(ianw): The output is challenge-domain:txt-key which we split -# into a tuple here. acme.sh by default puts the hostname into the -# challenge domain it outputs. For simplicity, we don't actually make -# use of the full challenge-domain part; our default CNAME setup -# points "_acme-challenge.host.opendev.org" to just "acme.opendev.org" -# -- thus we put all the keys into "top-level" TXT records directly at -# acme.opendev.org. letsencyrpt doesn't care; it just follows the -# CNAME and enumerates all the TXT records in acme.opendev.org looking -# for one that matches. So even though we don't put it in the dns -# records, having the hostname the TXT record is for is handy for -# debugging, etc, so we pass it through. +# NOTE(ianw): The output of the driver is +# +# challenge-domain:TXT-key +# +# We don't care about the challenge-domain part (we have set all +# _acme-challenge.hostname.o.o records as CNAMES to acme.opendev.org). +# Record the config key along with the TXT record; later we use it to +# check which config keys have been updated and need a refresh. +# - set_fact: - acme_txt_required: '{{ acme_txt_required + [(item.split(":")[0], item.split(":")[1])] }}' + acme_txt_required: '{{ acme_txt_required + [(cert.key, item.split(":")[1])] }}' loop: '{{ acme_output.stdout_lines }}'