Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of IDNA/Unicode domains #436

Merged
merged 18 commits into from
May 9, 2022

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Apr 5, 2022

SUMMARY

Improve IDNA/Unicode handling.

Fixes #426.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

various

@felixfontein felixfontein force-pushed the dns-names branch 3 times, most recently from e304680 to f83e2f0 Compare April 5, 2022 05:37
@felixfontein
Copy link
Contributor Author

felixfontein commented Apr 5, 2022

One problem with the current approach is that Python's IDNA encoding apparently is kind of outdated, see https://pypi.org/project/idna/ (project description) and the official docs https://docs.python.org/3/library/codecs.html#module-encodings.idna. It might be better to add an optional dependency on the idna library and use it to do the conversion, and fail if a conversion is needed and the library is not present.

@felixfontein
Copy link
Contributor Author

Note that idna is only required if IDNs are used. If IDNs worked before with an older cryptography / PyOpenSSL version, then it was already using IDNA to do the conversion (and thus IDNA was already installed), so this isn't a breaking change for any existing configuration. (Assuming nobody patched their cryptography to use Python's IDNA2003 implementation instead :) )

@felixfontein felixfontein force-pushed the dns-names branch 2 times, most recently from dacbef9 to 7c8c79a Compare April 18, 2022 10:25
@felixfontein
Copy link
Contributor Author

The Python 3.5 version included in the Ansible 2.9 default test container has a stupid bug which causes the above error. This has been fixed in later versions of Python 3.5 (the default test container coming with devel doesn't have it for example). (This was fixed in python/cpython@4655d57#diff-b3712475a413ec972134c0260c8f1eb1deefb66184f740ef00c37b4487ef873e as part of https://bugs.python.org/issue36742, and the fix first appeared in 3.5.8 rc 1.)

@felixfontein felixfontein changed the title [WIP] Improve handling of IDNA/Unicode domains Improve handling of IDNA/Unicode domains Apr 19, 2022
@felixfontein
Copy link
Contributor Author

ready_for_review

@felixfontein
Copy link
Contributor Author

Confirmed to be working as expected in #426 (comment)

Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Love to see i18n support

@felixfontein felixfontein merged commit 4cf9515 into ansible-collections:main May 9, 2022
@felixfontein felixfontein deleted the dns-names branch May 9, 2022 17:57
@felixfontein
Copy link
Contributor Author

@briantist thanks a lot for reviewing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameter to switch between Punycode and Unicode
2 participants