lookup lowercase domain names when verifying authorizations to preven…

[lspiehler]

…t failure when CSR has mixed-case names

SUMMARY

I received a CSR from a vendor that had uppercase characters in someof the requested domain names, which resulted in the following error when validating challenges with the acme_certificate module: "Found no authorization information for "dns:My.FQDN.com"!". The problem was caused by the authorizations in the returned challenge data having all domains (keys for the validation object) converted to lowercase, but when the keys were being looked up to make sure each is valid, the original case in the CSR was used.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

acme_certificate

ADDITIONAL INFORMATION

Before change

TASK [ansible_acme : Complete challenge] ***************************************
fatal: [localhost]: FAILED! => {"changed": false, "msg": "Found no authorization information for \"dns:My.FQDN.com\"!", "other": {}}

After change, task completes successfully.

[felixfontein]

I think this change is incorrect, as this might only work with one specific CA, but could break behavior with other CAs.

This probably needs a more general solution that allows matching DNS (and while we're at it, probably also IP) identifiers that differ up to case.

[felixfontein]

Out of curiosity, which CA(s) do(es) exhibit this normalization behavior?

[lspiehler]

I'm observing the behavior with Let's Encrypt. It's the only CA I've tested so far. What do you think about also forcing the "authorizations" keys to be lowercase? See my last commit. Thanks!

[lspiehler]

I ran into the issue you described while testing the ACME server I developed for https://PKIaaS.io. I noticed there's another place the authorizations dict is defined for ACMEv2 in the load_authorizations function. After making the keys lowercase there as well, I can confirm this change is working successfully for Let's Encrypt, ZeroSSL, BuyPass, and PKIaaS.io. Let me know if there's anything else you'd like me to test. Thanks!

[lspiehler]

@felixfontein, have you had a chance to review the latest commit? I think I've addressed the concerns you've mentioned. Let me know if there's anything additional I can do. Thanks!

[felixfontein]

@lspiehler the PR is still on my review list, but currently I don't have much time. I should be able to take a closer look later this month, probably in the next week or the week thereafter.

[felixfontein]

I did find some time to review this. There are three issues:

1. Instead of calling .lower() all over the place, add a function normalize_combined_identifier which converts the identifier part to lower-case, and use that one.
2. Your patch is potentially modifying the return values of the module in some places. In line 968 of modules/acme_certificate.py, change split_identifier(k)[1] to v.identifier (you can get rid of k completely); the same is true for lines 758 and 759, where you can use authz.identifier instead of identifier.
3. You don't normalize in line 786 of modules/acme_certificate.py.

[felixfontein]

This line should not have a change.

[lspiehler]

Firstly, thank you for your time and patience. I'm hoping I understood all of the issues you mentioned and your suggestions. I believe I've addressed all three. I've also tested again with the same four CAs I tested previously (Let's Encrypt, ZeroSSL, BuyPass, and PKIaaS.io) using multiple SANs with some lower case only and some mixed case. All tests were successful. Let me know if there's anything else I can do. Thanks!

[felixfontein]

Can you please also add a changelog fragment? Thanks.

[lspiehler]

I committed your suggestion and included a changelog fragment. Thanks for all your help!

[felixfontein]

One last small change :)

[lspiehler]

Suggestion committed!

[felixfontein]

@lspiehler thanks a lot for reporting and fixing this!

[felixfontein]

@lspiehler community.crypto 2.22.2 is out including this fix.