win-acme / win-acme

A simple ACME client for Windows (for use with Let's Encrypt et al.)
https://www.win-acme.com/
Apache License 2.0
5.28k stars 816 forks source link

Feedback RFC2136 plugin #2364

Closed JensSpanier closed 1 year ago

JensSpanier commented 1 year ago

Thanks for implementing RFC2136!

I've just tested this new plugin and found a few things.

1. Hostname of DNS server not working

At one step win-acme is asking for DNS server host/ip. If you provide a hostname (like dns.example.com) an error is shown after chosing additional installation steps:

(SocketException): Ein ungültiges Argument wurde angegeben.
 Wrapped in FormatException: An invalid IP address was specified.

grafik

If you provide an IP address here, it works. I know that certbots RFC2136 plugin also only allows IP addresses. Maybe this also the case here?

2. TXT don't get deleted + only works on retry

I tried to create a certificate for tolljens.de,*.tolljens.de. I also retried with another domain and the result was the same. This was the result: grafik

As you can see, the record can't get deleted (there were 3 TXT records in the end) and it also works only after retrying.

The error in the logs says

2023-04-21 10:45:38.473 +02:00 [VRB] Starting post-validation cleanup
2023-04-21 10:45:38.477 +02:00 [ERR] Error deleting DNS record
System.Exception: FormatError
   at PKISharp.WACS.Plugins.ValidationPlugins.Dns.Rfc2136.SendUpdate(DnsUpdateMessage msg)
   at PKISharp.WACS.Plugins.ValidationPlugins.Dns.Rfc2136.DeleteRecord(DnsValidationRecord record)
2023-04-21 10:45:38.477 +02:00 [VRB] Post-validation cleanup was succesful

I also can't see any reason why letsencrypt is sending the unauthorized error. Both records are available on both DNS servers and the precheck is also successful.


If you need a system for testing, I would be happy to provide you the needed credentials for my DNS setup. You can use one of my unused domains or I can setup a domain for you.

Platform:

WouterTinus commented 1 year ago

Thanks for testing the plugin Jens! It'd be very convenient for me to have some testing credentials to debug this feature a bit. Any domain will do, just make sure it's not something that is used for production, as I suppose a misfire of RFC2136 could make the whole zone unusable.

The fact that you were (eventually) able to get a certificate already gives me hope that getting the rest right shouldn't be too hard. If you could the details to win.acme.simple@gmail.com I'll promise to treat them carefully.

JensSpanier commented 1 year ago

I've sent you an email with the information

WouterTinus commented 1 year ago

Thanks, that helps a lot.

I'm not sure if we're looking at server issues or client issues here. Perhaps your server logs will paint a more complete picture.

Anyway you can try the build here: https://ci.appveyor.com/project/WouterTinus/win-acme-s8t9q/builds/46854594/artifacts

(No need to download everything, just the new rfc2136 plugin build will do the trick).

JensSpanier commented 1 year ago

I needed to also download win-acme becaus of the changes in it for GetIps.

21-Apr-2023 22:55:35.846 client @0x7f733405eb28: updating zone 'example.com/IN': adding an RR at '_acme-challenge.test.example.com' TXT "3z_4-qmSD5loLvbzdHcS4h4h6sK6gxHiDCV7pjOj2zQ"
21-Apr-2023 22:56:11.236 client @0x7f73405a92e8l: updating zone 'example.com/IN': adding an RR at '_acme-challenge.test.example.com' TXT "3z_4-qmSD5loLvbzdHcS4h4h6sK6gxHiDCV7pjOj2zQ"

A delete action (triggered manually) looks like this in the logs:

21-Apr-2023 22:59:20.335 client @0x7fe5cc07bcb8: updating zone 'example.com/IN': deleting an RR at _acme-challenge.test.example.com TXT
WouterTinus commented 1 year ago

Looks like a bug in the library to me, I've asked the author to take a look at it.

Creating all records at once is possible, but it would have to be explicitly enabled for this plugin, and even then it will only work if the user enables parallel processing in settings.json

WouterTinus commented 1 year ago

I was able to work around the delete bug and have enabled parallelism (you still have to set DisableMultithreading to false to get this behaviour though)

https://ci.appveyor.com/project/WouterTinus/win-acme-s8t9q/builds/46858981/artifacts

WouterTinus commented 1 year ago

Fix included in 2.2.5