Don't delete existing challenges to support wildcard certificates#11
Don't delete existing challenges to support wildcard certificates#11ruben-haegeman wants to merge 1 commit intospfguru:masterfrom
Conversation
ruben-haegeman
force-pushed
the
feature/wildcard-support
branch
from
21e77b1 to
8692114
Compare
ruben-haegeman
force-pushed
the
feature/wildcard-support
branch
from
8692114 to
50dbe71
Compare
|
@fogs |
fogs
left a comment
fogs
left a comment
There was a problem hiding this comment.
Hi Ruben,
Thanks for submitting your PR. I have added some comments to it, please have a look!
Keep up the good work,
fogs
| # The domain name (CN or subject alternative name) being | ||
| # validated. | ||
| # - TOKEN_FILENAME | ||
| # The name of the file is irrelevant for the DNS challenge, yet still provided |
There was a problem hiding this comment.
I understand that spaces at the end of the line are crap, sorry for my poor code quality!
But can we please have a separate commit when removing those?
| # Remove old txt token | ||
| gcloud dns record-sets transaction start --zone $zonename | ||
| gcloud dns record-sets transaction remove --name $existingName --type TXT --ttl $existingTtl --zone $zonename -- "$existingRrdata" | ||
| gcloud dns record-sets transaction execute --zone $zonename --format='value(id)' |
There was a problem hiding this comment.
Is it necessary to perform a transaction execute here and a transaction start in line 59? Or can we also handle that all in one transaction?
| if [ "$existingName" == "_acme-challenge.$DOMAIN." ]; then | ||
| gcloud dns record-sets transaction remove --name $existingName --type TXT --ttl $existingTtl --zone $zonename -- "$existingRrdata" | ||
| # Remove old txt token | ||
| gcloud dns record-sets transaction start --zone $zonename |
There was a problem hiding this comment.
Why was this transaction start pulled into the IF statement?
| # nsresult comes with the TXT RR in double quotes - remove those | ||
| nsresult=${nsresult//$'"'/''} | ||
| until [[ "$nsresult" = "$TOKEN_VALUE" ]]; do | ||
| until [[ "$nsresult" == *"$TOKEN_VALUE"* ]]; do |
There was a problem hiding this comment.
What is the difference between the old version and the new? Won't * expand in bash?
|
|
||
| gcloud dns record-sets transaction remove --name $existingName --type TXT --ttl $existingTtl --zone $zonename -- "$existingRrdata" | ||
| gcloud dns record-sets transaction execute --zone $zonename | ||
| existingRecord=`gcloud dns record-sets list --name "_acme-challenge.$DOMAIN." --type TXT --zone $zonename --format='value(name,rrdatas,ttl)'` |
There was a problem hiding this comment.
Why rrdatas instead of the previously used rrdatas[0]?
| # Remove all record | ||
| gcloud dns record-sets transaction start --zone $zonename | ||
| stringify=${existingRrdata[@]} | ||
| stringify=${stringify// /"\" \""} |
There was a problem hiding this comment.
This is adding another tool requirement to the script - is there no way we can do that in bash? Also, please name variables according to what is in there instead of naming them after the command you got the data from.
2 participants
While generating a wildcard certificate 2 challenges will be deployed for the same domain.
One challenge is deployed to verify
domain.tldand one is deployed for*.domain.tld. Both however will be deployed to the domein_acme_challenge.domain.tldThis fix makes it possible for multiple challenges to exist at the same time for the same domain.
This PR closes issue #8
PS: I'm not fluent in bash so feedback is always welcome.