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

fix _get_root() so that it successfully gets the root domain #3542

Merged
merged 2 commits into from
Jun 9, 2021

Conversation

DerVerruckteFuchs
Copy link
Contributor

The _acme-challenge TXT record is not getting created for subdomains like subdomain.example.com that are part of the same zone as example.com. The issue is at lines 207 and 208 in the _get_root() function. Using https://management.1984hosting.com/domains/soacheck/?zone=subdomain.example.com&nameserver=ns0.1984.is. returns a {"serial": null} JSON response, instead of {"serial": $SOME_NUMBER} response that valid zones would get. The if statement on line 208 only checks if serial is contained in the response. This results in _domain getting assigned the value subdomain.example.com and _sub_domain getting assigned _acme-challenge. There is no zone subdomain.example.com (not my real subdomain/domain, obviously) as my subdomain is part of the same zone my root domain is in. This results in the attempt to add a DNS record for the nonexistent zone predictably failing. (As an aside, if subdomain.example.com and example.com are separate zones, there is a possibility dns_1984hosting.sh would not fail to issue/renew certs for either zone in its currently broken state.) Changing the if statement to check if the response contains serial and does NOT contain null results in the while loop continuing until _domain gets assigned the value example.com and _sub_domain gets assigned _acme-challenge.subdomain. The final result is that the _acme-challenge TXT record actually gets set. This fix should also work for subdomains like foo.bar.subdomain.example.com and other URLs with larger/"deeper" subdomain depths where the root domain and subdomain are part of the same zone.

I was able to renew my pfsense cert with my changes.

Earlier explanation of issue from #2851 (comment).

@DerVerruckteFuchs
Copy link
Contributor Author

Did the dev branch already have its own fix for this? There was a merge conflict with my branch that had a slightly different fix that looks like it would take care of the issue.

@DerVerruckteFuchs
Copy link
Contributor Author

It looks like d154118 may have also fixed this issue. The commit message was ambiguous, so I did not know what it was for initially. It seems there were other things that needed fixed that I had not looked at yet.

@Neilpang
Copy link
Member

Neilpang commented Jun 6, 2021

is this commit still necessary?

@DerVerruckteFuchs
Copy link
Contributor Author

is this commit still necessary?

Mostly not, but it could be. The only difference is on line 207/208 or so where the check looks for "null" instead or 'null}'. Unless the JSON output for the SOA check changes, the current check should work fine. This commit may be necessary if the JSON output changes, and null is no longer the last/only value in the set of key/value pairs, so that checking for 'null}' fails. I'm thinking back to the HTML tag checking issue from #3541. I would expect HTML changes to be more frequent than the JSON output, but checking for null became necessary for logins to work again recently.

@Neilpang Neilpang merged commit a438c84 into acmesh-official:dev Jun 9, 2021
@DerVerruckteFuchs DerVerruckteFuchs deleted the _get_root()-fix branch April 25, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants