certbot-zimbra
certbot-zimbra copied to clipboard
YetOpen
find_additional_public_hostnames() enhancement for virtual hosts
Change the way certbot-zimbra establishes the list of SAN's (#161). In addition to zimbraPublicServiceHostname also look at entries added to zimbraVirtualHostname to find all SAN's for the certificate.
This would cause a regression for https://github.com/YetOpen/certbot-zimbra/issues/95 https://github.com/YetOpen/certbot-zimbra/commit/039a48f89f01f8b56f046430d520bc81744feadc which was a fix for slowness in find_additional_public_hostnames that was because a separate zmprov was executed for each domain. From that bug report we know we have users with >100 domains so they would be impacted.
Some quick tests:
zmprov -l gd ~5s
zmprov gd ~10s
If there are 100 domains and zmprov is ran twice per domain: 100 * 2 * 5s = 1000s = 16m40s If zmprov_opts is empty (doesn't use ldap, that would be a custom modification by the user) it would take 2000s = 33m20s
Although it would not be as bad as before that bugfix, where find_addidional_public_hostnames would also be ran in the pre-hook (so on every cron renew), that commit changed it so it's only ran when a new cert is requested via --new.
But it would drastically increase the time it would take for requesting a new certificate via --new if -u | --no-public-hostname-detection is not used.
I think it's a good enhancement to the script but it should be made to work like it does now.
You'd only need to modify the gawk script to add the zimbraVirtualHostname call, and sed script to also match it. Adding sort | uniq to the end is also good. I don't know why you'd need tr '\n' ' ' as bash already interprets newlines as array element separators.
I tried this and it works, but I don't have any zimbraVirtualHostname set so I'm not 100% if it's right.
su - zimbra -c "zmprov $zmprov_opts gad" | gawk '{printf "gd %s zimbraPublicServiceHostname\ngd %s zimbraVirtualHostname\n", $0, $0}' \
| su - zimbra -c "zmprov $zmprov_opts -" \
| sed "/prov>/d;/# name/d;/$domain/d;/^$/d;s/^zimbraPublicServiceHostname: \(.*\)/\1/;s/^zimbraVirtualHostname: \(.*\)/\1/" \
| sort -u`
Maybe this could also be made better/more secure using a read loop, as right now the output of this pipeline is not quoted, which could be an ACE if one of the executables in the pipeline were compromised. Using "read" you could assign values to elements of the array and avoid the non-quoted subshell.
If you can modify the PR to leave the pipeline as-is and just change it like I showed above, I would approve it.
Looks good, can you please test it and report. My server doesn't have any zimbraPublicServiceHostname or zimbraVirtualHostname (different from zmhostname) so it doesn't return anything here.
Yes, of course, I did tested the creation and the renewal process on my server. Working as expected.
On a side note I will have to create another pull request to add the option "--preferred-chain" to get the this correctly passed to certbot - the advice in https://github.com/YetOpen/certbot-zimbra/blob/master/README.md regarding this is not working for me.
Did you read the latest readme? You tried with the latest script from master that includes 8101d9be ? Can you paste the error here? I can add the preferred chain option as default to the script if it's still needed.