Page MenuHomePhabricator

CN + SNI list on config file doesn't match issued certificate on some scenarios
Closed, ResolvedPublic

Description

today, @Andrew accidentally hit an interesting corner case on acme-chief. He committed the following config as part of https://gerrit.wikimedia.org/r/c/operations/puppet/+/496785:

ldap-labtest:
    CN: 'ldap-labtest.eqiad.wikimedia.org'
    SNI:
    - 'labtestservices2001.wikimedia.org'
    challenge: dns-01
    authorized_regexes:
    - '^labtestservices2001\.wikimedia\.org'

as you can notice, the CN is not included in the SAN/SNI list, but Let's Encrypt always includes the CN in the SNI list. On the first attempt. acme-chief was able to issue the certificate as expected. But this triggers the following behaviour on certificate status re-evaluation:

Mar 15 15:00:01 acmechief1001 acme-chief-backend[14291]: Certificate ldap-labtest type ec-prime256v1 has SANs {'labtestservices2001.wikimedia.org', 'ldap-labtest.eqiad.wikimedia.org'} but is configured for {'labtestservices2001.wikimedia.org'}, moving back to re-issue
Mar 15 15:00:01 acmechief1001 acme-chief-backend[14291]: Certificate ldap-labtest type rsa-2048 has SANs {'labtestservices2001.wikimedia.org', 'ldap-labtest.eqiad.wikimedia.org'} but is configured for {'labtestservices2001.wikimedia.org'}, moving back to re-issue

of course this triggers an infinite loop, hammering Let's Encrypt rate limits and every request related to that configured certificate is rejected by Let's Encrypt driving acme-chief crazy

Event Timeline

Vgutierrez triaged this task as Medium priority.Mar 15 2019, 6:23 PM
Vgutierrez created this task.

we should probably reject configs that don't include CN in SAN

that's the easiest/saner way of solving this issue :)

Change 497242 had a related patch set uploaded (by Vgutierrez; owner: Vgutierrez):
[operations/software/acme-chief@master] acme-chief: Ensure that the CN is part of the SNI list for certs config

https://gerrit.wikimedia.org/r/497242

@Krenair I think that the proposed patch is even simpler. It ensures that the CN is always part of the SNI list

Change 497242 merged by Vgutierrez:
[operations/software/acme-chief@master] acme-chief: Ensure that the CN is part of the SNI list for certs config

https://gerrit.wikimedia.org/r/497242

Change 497439 had a related patch set uploaded (by Alex Monk; owner: Vgutierrez):
[operations/software/acme-chief@debian] acme-chief: Ensure that the CN is part of the SNI list for certs config

https://gerrit.wikimedia.org/r/497439

Change 497444 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/software/acme-chief@master] Release 0.12

https://gerrit.wikimedia.org/r/497444

Change 497444 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/software/acme-chief@master] Release 0.13

https://gerrit.wikimedia.org/r/497444

Change 497439 merged by jenkins-bot:
[operations/software/acme-chief@debian] acme-chief: Ensure that the CN is part of the SNI list for certs config

https://gerrit.wikimedia.org/r/497439

Change 497444 merged by Vgutierrez:
[operations/software/acme-chief@master] Release 0.13

https://gerrit.wikimedia.org/r/497444

Change 497527 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/software/acme-chief@debian] Release 0.13

https://gerrit.wikimedia.org/r/497527

Change 497527 merged by jenkins-bot:
[operations/software/acme-chief@debian] Release 0.13

https://gerrit.wikimedia.org/r/497527

Change 497532 had a related patch set uploaded (by Alex Monk; owner: Alex Monk):
[operations/software/acme-chief@debian] debian: Add release 0.13 to changelog

https://gerrit.wikimedia.org/r/497532

Change 497532 merged by jenkins-bot:
[operations/software/acme-chief@debian] debian: Add release 0.13 to changelog

https://gerrit.wikimedia.org/r/497532