zonemaster / zonemaster-ldns

A Perl interface module for Zonemaster to NLnet Labs' ldns library

Other

8 stars 12 forks source link

Use libidn2 #133

Closed ghost closed 2 years ago

ghost commented 2 years ago

Purpose

Replace libidn with libidn2 following https://libidn.gitlab.io/libidn2/manual/libidn2.html#Converting-from-libidn.

Context

131

Changes

update the Makefile.PL to look for libidn2 instead of libidn
use libidn2 header in include/LDNS.h
use new API in src/LDNS.xs

How to test this PR

The tests should pass.

Discussion

Old code was using IDNA_ALLOW_UNASSIGNED in idna_to_ascii_8z. Even if this flag exists in IDN2 (IDN2_ALLOW_UNASSIGNED) it seems it has been kept for compatibility reason and is unused. I haven't dig any further, but do we need to keep this flag, or should we update some logic in the code to keep the expected behavior we had with the old code?

ghost commented 2 years ago

Since #27 Travis always runs the tests with the network. Should I fix the test to be aligned with the live data? Or should we make the test run without network by default (so that on installation they are run in a close environment)?

ghost commented 2 years ago

I went with updating the tests. I realize that the nic.se zone has evolved recently. Maybe we should make a pass over the tests to reduce potential gaps with the current configuration.

mattias-p commented 2 years ago

Did you consider using https://metacpan.org/pod/Net::LibIDN2 instead? That way maybe we could simply drop some of our own XS code.

ghost commented 2 years ago

Did you consider using https://metacpan.org/pod/Net::LibIDN2 instead?

No I haven't. Thanks for the pointer.

matsduf commented 2 years ago

I think it would be a good idea to use Net::LibIDN2 instead. There is some magic that the IDN libraries do, such as down casing and making NFC. With Net::LibIDN2 we would have control of that. We could have a test case that verifies that labels that look like A-labels also are valid A-labels.

Would we loose anything by using Net::LibIDN2 instead?

matsduf commented 2 years ago

Should we have any discussion about this PR? I thought that this would be replaced by #144.

ghost commented 2 years ago

Since I don't think I'll be able to address #144 for v2022.1, I think it would be nice to integrate this change in v2022.1.

@matsduf, is the suggested flag correct? Do you see a change in behavior by using libidn2?

ghost commented 2 years ago

I've seen I forgot to update the Dockerfile and some references to "libidn". This is fixed. Please re-review.