rmbolger / Posh-ACME

PowerShell module and ACME client to create certificates from Let's Encrypt (or other ACME CA)
https://poshac.me/docs/latest/
MIT License
747 stars 184 forks source link

Certs with multiple SAN values using challenge aliases or multiple plugins may fail validation #127

Closed whbingham closed 5 years ago

whbingham commented 5 years ago

I have found that when using the Posh-ACME routines for a new or renewal ACME certificate with multiple SANs on one certificate that the DNS Challenge will fail sometimes. After troubleshooting, the failure has to do with the fact that the response from the ACME request for each SAN does not necessarily return in the same order (sequence) that the requests were sent. This means that the list of DNSAliases, when they are different for each SAN, do not align with the order of the ACME response. As a result the DNS Challenge record is published to the wrong DNS record and the certificate will fail the challenge. This would also be an issue if there were multiple DNS Plugins for the different SANs since they would be in the wrong order as well.

Therefore, I have made a modification in the New-PAOrder script to reorder the ACME responses for the order.identifiers and the order.authorizations to match the original order request that aligns with the DNSAliases and DNS Plugins used in the order request. I always try to provide a solution instead of just pointing out a problem. With that, I am attaching a revision to the New-PAOrder script which has solved this problem for me. Whether this is the proper way to correct the issue or not, I will let you decide, but you are welcome to use this as you see fit. Thanks for your work on this module.

New-PAOrder-Revised.txt @whbingham

rmbolger commented 5 years ago

This is an excellent find. Thanks! I definitely haven't tested the challenge aliases or multiple plugins thoroughly enough. I'm guessing this bug is more likely the more SANs you have in a cert as well and I usually only test SAN stuff with like 2-3 names.

Thanks for the fix suggestion as well. For a relatively small fix to a small project like this, attaching the file is just fine. But if you wanted to get fancy in the future, the typical workflow looks like this:

I might toy around with alternative ways to fix this bug. But this may end up being the simplest way to do it.

rmbolger commented 5 years ago

So in the process of applying this fix, I realized the ACME spec is even more ambiguous on this topic than I thought. Not only will the identifiers returned in the order object not necessarily match the sequence of identifiers you sent in the original payload. They won't necessarily match the sequence of authorizations either. As in, identifier[0] may not match up with authorization[0]. So with the fix, I had no choice but to query the details for each authorization URL in order to match them to their associated identifier.

I also managed to find some other long standing bugs related to using challenge aliases and multiple plugins in Submit-ChallengeValidation. So thanks again for the find!

whbingham commented 5 years ago

Ryan,

I began programming with FORTRAN using punch cards back in the ‘70s on a AMDOL mainframe. Having that background means my brain think more like top down programming than object oriented. I am not a professional programmer and I do not profess to understand all the nuances regarding proper programming etiquette and especially github. I am glad that I was able to identify the source of the problem and it is great that you were able to correct the issue quickly for everyone. I originally was using the module testing my certificate automation setup with only two SANs using DNSAliases in a split-brain setup and everything worked fine, but when I entered six SANs, it failed. That is why I started troubleshooting the issue and found the wrong challenges being written to the wrong DNS records. I have now downloaded the updated module that you issued last night and all works as anticipated with my six SANs and no failures. Thanks for your prompt attention to this matter.