yourivw / LEClient

An easy-to-use PHP ACME v2 client library, designed to be used with LetsEncrypt.
MIT License
204 stars 98 forks source link

Bug on Google DNS check #109

Closed svdigital-development closed 3 years ago

svdigital-development commented 3 years ago

Hi @yourivw I just found a possible bug or issue on Google DNS check (DNS-01 challenge).

The problem verifies on method LEFunctions::checkDNSChallenge()

and more specifically on: https://github.com/yourivw/LEClient/blob/master/src/LEFunctions.php#L238

if($answer->data === ('"' . $DNSDigest . '"')) return true;

Here an example of a real response (domain name is obfuscated by me) Google DNS gave me 10 minutes ago (real domain, real DNS challenge):

{"AD":false,"Answer":[{"TTL":21599,"data":"z3fUSnQ81oLTqUXqeSS_iK7byaC0Y9YJe4g4f-P-jk0","name":"_acme-challenge.testdomain.com.","type":16},{"TTL":21599,"data":"zb_jQEVsOmCg7OT25VvSURj6PKhvpPbj5EY8aw5Gydk","name":"_acme-challenge.testdomain.com.","type":16}],"CD":false,"Comment":"Response from 178.33.142.161.","Question":[{"name":"_acme-challenge.testdomain.com.","type":16}],"RA":true,"RD":true,"Status":0,"TC":false}

The content of $answer->data is not '"' . $DNSDigest . '"', but actually is: $DNSDigest itself.

With this fix the challenge is verified and certificate emitted:

if($answer->data === ('"' . $DNSDigest . '"')) return true;

if($answer->data === $DNSDigest) return true;
svdigital-development commented 3 years ago

(I just updated my comment above because a keyboard shortcut submitted it too early)

milesizzo commented 3 years ago

This one bit me today too! Thanks for reporting and fixing.

bilogic commented 3 years ago

@svdigital-development @milesizzo This repo seems abandoned, how are you guys overcoming this issue?

milesizzo commented 3 years ago

Hi @bilogic, I'm not sure it's abandoned but I overcame the issue by disabling "local check"; it's not necessary (it just checks the DNS entry or the HTTP endpoint succeeds before checking with upstream):

$order->verifyPendingOrderAuthorization($identifier, $type, false);

See LEOrder.php, line 377:

public function verifyPendingOrderAuthorization($identifier, $type, $localcheck = true)
bilogic commented 3 years ago

@milesizzo erm, there is a bug in Google DNS check as detailed by @svdigital-development and fixed by @qem.

milesizzo commented 3 years ago

Yeah and I overcame it by not using the local check, that's the code path that hits the Google DNS. As I said, you don't need to do it, it's just a failsafe. If you want to use the fix, change your composer version to dev-master.

bilogic commented 3 years ago

@milesizzo ah ok, I understand you now. thanks.

viharm commented 3 years ago

I had the same problem today

bilogic commented 3 years ago

@viharm remove the double quotes like in the OP

viharm commented 3 years ago

@viharm remove the double quotes like in the OP

Sure, will do. Will this be included in the next fix/release?

bilogic commented 3 years ago

@viharm we have all been trying to reach the owner

yourivw commented 3 years ago

@svdigital-development @milesizzo @bilogic @viharm My apologies, I don't have a lot of time anymore to work in this project. I am also not using it a lot anymore myself. Nevertheless, the issue should now be resolved with the new release.

bilogic commented 3 years ago

@yourivw we can help you maintain if you are comfortable thank you!