Cloudflare DNS broken - is this really the fix?

[moto8801]

I've been using acme.sh with Cloudflare for a while now with no trouble. Today it stopped working. Line 62 in dns_cf evaluated false and therefore returned an error. Line 62 checks that the GET txt records JSON response contains success:true as below:
if ! printf "%s" "$response" | grep \"success\":true >/dev/null; then

This line has been there for 4 years.

Below is the response in my debug output:
[Tue Apr 28 15:46:26 UTC 2020] response='{ "result": [], "success": true, "errors": [], "messages": [], "result_info": { "page": 1, "per_page": 20, "count": 0, "total_count": 0, "total_pages": 1 } }'

As of today it seems, line 62 does not parse the response correctly. I had to modify it to include a space before true:
if ! printf "%s" "$response" | grep \"success\":\ true >/dev/null; then

It then started working. Am I going crazy?

[auto-comment]

If this is a bug report, please upgrade to the latest code and try again:
如果有 bug, 请先更新到最新版试试:
acme.sh --upgrade
please also provide the log with --debug 2.
同时请提供调试输出 --debug 2
see: https://github.com/acmesh-official/acme.sh/wiki/How-to-debug-acme.sh
Without --debug 2 log, your issue will NEVER get replied.
没有调试输出, 你的 issue 不会得到任何解答.

[tatablack]

I was about to open the exact same issue! 😅
I had been using an older acme.sh version; today I decided to update it and start using Cloudflare's new tokens instead of the global API key, and ran into the same problem - fixed in the same way (and I was also puzzled by seeing that the code hadn't been changed in four years).

For context, I used the latest master as of 2 hours ago, on a Synology NAS.

[moto8801]

I've noticed while doing the PR that DNS records are not being deleted correctly as the parsing of the JSON from the GET call is not extracting the ID of the record correctly.

[sebile]

maybe there is a better way to parse the json response from the cloudflare api then using grep, even if the fix proposed by mtis88 in #2890 should be working

[tatablack]

There are also a couple of checks performed using the _contains function (which uses grep behind the scenes - search for _contains "$response" '"success":true' in the same file.

I'm guessing they may fail as well, but I haven't tested it.

[moto8801]

To be clear on what the problem is here, Cloudflare are now pretty printing their JSON responses for some reason, breaking the checking of properties using grep. I think the best fix here is to un pretty print the response somehow, rather than changing each individual grep. Especially if Cloudflare change it back. I'm not sure how to do this with only the shell. Any ideas?

[moto8801]

That's interesting because my PR #2891 (which I've closed) minifies all of the JSON responses and everything was working for me, both adding and deleting records.

[joecot]

@mtis88 Sorry, been messing with this for too long. I did a clean install of acme.sh and added response=$(echo $response | jj -u) to _cf_rest in dns_cf.sh, and I can confirm that all the requests are working.

While I'm a little uncomfortable with your approach of just removing all spaces from the response, it doesn't seem like there's anything in the JSON responses that would be fundamentally altered by doing so, and it doesn't require more dependencies. If we're already using grep to parse JSON, I guess wallpapering over the bad wallpaper would be fine for now. Maybe add the pull request back based off of development instead of master?

[moto8801]

@joecot I closed the PR for that exact reason - it removes spaces anywhere in the string. But as you say, it actually works in this case so I am actually using this solution myself for the time being, I just thought for wider use there would be a safer solution.

I was looking for some --debug 2 output in other issues and I found some in #2789. The responses in there are compacted vs the prettified one's I'm seeing today. I wonder why CF have done this?

[joecot]

@mtis88 I just looked through my logs of responses that I minified using jj. I'm not seeing any meaningful spaces within the responses. I see it in fields like original registrar name ("namecheap, inc. (id: 1068)") and in the description of the cloudflare tier used ("Free Website"). Nothing meaningful.

The only place I would expect meaningful spaces in the JSON are within the errors or messages properties, which the cloudflare integration does not appear to touch. So maybe we should debug2 the response before and after stripping spaces, but otherwise it doesn't appear like it will matter.

[moto8801]

@joecot I've created a branch from dev with the change. Happy to submit the PR if we think it's acceptable.

[joecot]

@mtis88 given the existing use of grep and such to parse JSON in this integration, I think your change to strip spaces is more than acceptable. A more robust JSON parsing would be better, but I'm assuming decisions were made to skip that to avoid more dependencies.

But I would suggest making it:

  if [ "$?" != "0" ]; then
    _err "error $ep"
    return 1
  fi
  _debug2 response "$response"
  response=$(echo $response | tr -d [:space:])
  _debug2 stripped_response "$response"
  return 0

Have it alter the response after the error checking, and debug2 output the response before and after you strip whitespace, so if there are error messages they're more readable.

[Aterfax]

Also having this error in OpenWRT latest build with the ACME scripts - edited the line to the first post and it fixed the issue.

[johannrichard]

Is there a reason why we can’t just check for zero or more whitespace in the grep expressions (there are other instances with the same problem, e.g. when removing the record)?

if ! printf "%s" "$response" | grep '\"success\":\s*true' >/dev/null; then

(It works on a couple of systems of mine, not sure that’s portable though. But you get the idea)

PS: maybe someone over at CloudFlare was just debugging the API and added pretty-printing for that: 😄

https://www.cloudflarestatus.com/incidents/jljrz1789325

[Neilpang]

@johannrichard
Thanks for the info.

I just don't trust grep. it may not support regex on some platforms.
So, I just trim out all the space keys.

it's all fixed now.

Thanks.

[xdmitry]

@Neilpang could you please also update docker image neilpang/acme.sh with this fix?

[Neilpang]

@xdmitry the scrip in docker container will upgrade itself tomorrow.

[xdmitry]

@Neilpang I'm still getting error message when trying to renew certificate with docker container. I'm using it as below

docker run -v /etc/acme:/acme.sh --rm neilpang/acme.sh --issue -d domain1 --challenge-alias domain2 --dns dns_cf --accountemail email

[BTSmolders]

@Neilpang Could you please fix the build pipeline for Docker (either in Docker Hub or Travis-CI). The latest image on Docker Hub is 3 months old and I also would like to use this fix.

[Neilpang]

@xdmitry @BTSmolders

it's updated. please try again.

Thanks.

[dkerr64]

@Neilpang can you please tag a new release (2.8.6?) so that this gets propagated ASAP. My regular cron job failed overnight as it tried to renew a certificate that used Cloudflare DNS TXT verification. Replacing my dns_cf.sh file with this fix solved the problem. This feels like a widespread sev 1 issue for which fix should be send out ASAP.
Thanks.

[xdmitry]

@Neilpang it works now, thank you very much!

[Neilpang]

@dkerr64

Here you go:
https://github.com/acmesh-official/acme.sh/releases/tag/2.8.6