Add support for NSID option + update internal LDNS to 1.8.3 #151

ghost · 2022-08-23T17:23:24Z

Purpose

Since version 1.8.2 (released earlier this year) LDNS can easily retrieve the NSID from the nameserver. This is a proof of concept to retrieve the NSID in Zonemaster::LDNS.

Context

Addresses zonemaster/zonemaster-engine#178 (comment)
Addresses #143.

Changes

New nsid and get_nsid methods for Zonemaster::LDNS::Packet.
Update internal LDNS to 1.8.3.

How to test this PR

After installing ldns 1.8.2 (or later), the following snippet should return the NSID from the host it connects to (if any):

use Zonemaster::LDNS;
#print(Zonemaster::LDNS::lib_version() . "\n");

my $host = '194.0.9.1';
my $pkt = Zonemaster::LDNS::Packet->new('domain.example');
$pkt->nsid; # set the NSID EDNS option
my $res = Zonemaster::LDNS->new($host)->query_with_pkt($pkt);
print($res->get_nsid() . "\n");

matsduf · 2022-08-23T21:33:05Z

It is an interesting feature for Zonemaster, but shouldn't the first step be to make Zonemaster-LDNS work with v1.8.2 -- or even v1.8.3 with bug fixes?

FreeBSD has support for v1.8.3, but is that true for Debian, Ubuntu and Rocky Linux? Else we have to use built-in LDNS again.

ghost · 2022-08-24T09:13:57Z

I've added some logic to check that the installed version of libldns is at least 1.8.2 to prevent errors.

is that true for Debian, Ubuntu and Rocky Linux?

From what I see, there is currently no support (and I don't know when there will be)

matsduf · 2022-08-25T02:56:29Z

From what I see, there is currently no support (and I don't know when there will be)

Is it feasible to let the debian packages of Zonemaster-LDNS include internal LDNS? If the feature is to be used the new version of LDNS must be installed everywhere.

matsduf

The submodule in Zonemaster-LDNS must be updated to point at the tag of LDNS 1.8.3. Else built-in LDNS will not be able to use this feature. There is no reason not to do that.

ghost · 2022-09-28T15:15:03Z

There is no reason not to do that.

I didn't want to update the submodule since there is an open issue about considering such an update #143. I'll do it in a separate PR.

matsduf · 2022-09-28T15:24:16Z

I didn't want to update the submodule since there is an open issue about considering such an update #143. I'll do it in a separate PR.

This PR introduces features from the new version. Why not include that now?

ghost · 2022-09-28T15:50:43Z

I've updated the internal LDNS to latest release, which is 1.8.3.

* internal LDNS is 1.8.3 so it supports NSID * external LDNS is unknown, a check is needed

matsduf

Looks fine.

ghost · 2022-10-12T12:33:07Z

The test file was missing in the MANIFEST.

matsduf

Press return to run 'make distcheck'...
"/usr/local/bin/perl" "-Iinc" "-MExtUtils::Manifest=fullcheck" -e fullcheck
Not in MANIFEST: ldns/.github/FUNDING.yml
Not in MANIFEST: ldns/.github/workflows/testsuite.yml
Not in MANIFEST: ldns/libdns.doxygen.in
Not in MANIFEST: ldns/README-Travis.md

matsduf · 2022-10-17T10:07:27Z

I have created an installation package for CPAN and try to install it on Ubuntu with built-in LDNS. I fails:

config.status: creating ldns/config.h
config.status: executing libtool commands
copying header files
make[1]: Entering directory '/root/.cpanm/work/1666001014.249574/Zonemaster-LDNS-2.2.2/ldns'
./libtool --tag=CC --quiet --mode=compile gcc -I. -I.  -DHAVE_CONFIG_H -DLDNS_TRUST_ANCHOR_FILE="\"/usr/local/etc/unbound/root.key\"" -Wunused-function -Wstrict-prototypes -Wwrite-strings -W -Wall -fPIC  -c ./buffer.c -o buffer.lo
In file included from ./ldns/util.h:19,
                 from ./ldns/ldns.h:95,
                 from ./buffer.c:12:
./ldns/common.h:30:44: error: token "@" is not valid in preprocessor expressions
   30 | #define LDNS_BUILD_CONFIG_USE_DSA          @ldns_build_config_use_dsa@
      |                                            ^
./ldns/keys.h:85:5: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_DSA'
   85 | #if LDNS_BUILD_CONFIG_USE_DSA
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
./ldns/common.h:30:44: error: token "@" is not valid in preprocessor expressions
   30 | #define LDNS_BUILD_CONFIG_USE_DSA          @ldns_build_config_use_dsa@
      |                                            ^
./ldns/keys.h:91:5: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_DSA'
   91 | #if LDNS_BUILD_CONFIG_USE_DSA
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
./ldns/common.h:31:44: error: token "@" is not valid in preprocessor expressions
   31 | #define LDNS_BUILD_CONFIG_USE_ED25519      @ldns_build_config_use_ed25519@
      |                                            ^
./ldns/keys.h:97:5: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_ED25519'
   97 | #if LDNS_BUILD_CONFIG_USE_ED25519
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./ldns/common.h:32:44: error: token "@" is not valid in preprocessor expressions
   32 | #define LDNS_BUILD_CONFIG_USE_ED448        @ldns_build_config_use_ed448@
      |                                            ^
./ldns/keys.h:100:5: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_ED448'
  100 | #if LDNS_BUILD_CONFIG_USE_ED448
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
./ldns/common.h:30:44: error: token "@" is not valid in preprocessor expressions
   30 | #define LDNS_BUILD_CONFIG_USE_DSA          @ldns_build_config_use_dsa@
      |                                            ^
./ldns/keys.h:258:6: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_DSA'
  258 | # if LDNS_BUILD_CONFIG_USE_DSA
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~
./ldns/common.h:30:44: error: token "@" is not valid in preprocessor expressions
   30 | #define LDNS_BUILD_CONFIG_USE_DSA          @ldns_build_config_use_dsa@
      |                                            ^
./ldns/keys.h:323:6: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_DSA'
  323 | # if LDNS_BUILD_CONFIG_USE_DSA
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~
./ldns/common.h:30:44: error: token "@" is not valid in preprocessor expressions
   30 | #define LDNS_BUILD_CONFIG_USE_DSA          @ldns_build_config_use_dsa@
      |                                            ^
./ldns/keys.h:341:6: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_DSA'
  341 | # if LDNS_BUILD_CONFIG_USE_DSA
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~
./ldns/common.h:30:44: error: token "@" is not valid in preprocessor expressions
   30 | #define LDNS_BUILD_CONFIG_USE_DSA          @ldns_build_config_use_dsa@
      |                                            ^
./ldns/keys.h:467:6: note: in expansion of macro 'LDNS_BUILD_CONFIG_USE_DSA'
  467 | # if LDNS_BUILD_CONFIG_USE_DSA
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~
make[1]: *** [Makefile:140: buffer.lo] Error 1
make[1]: Leaving directory '/root/.cpanm/work/1666001014.249574/Zonemaster-LDNS-2.2.2/ldns'
make: *** [Makefile:1433: ldns/.libs/libldns.a] Error 2
FAIL

ghost · 2022-10-17T12:15:49Z

I have created an installation package for CPAN and try to install it on Ubuntu with built-in LDNS. I fails:

I can't reproduce on Ubuntu 22.04. Can you provide more information on the steps you used?

matsduf · 2022-10-17T14:03:03Z

First create a package on one server (FreeBSD) with the relevant branch checked out:

git branch -v | head -1
* (HEAD detached at pnax/nsid) 7535a41 Missing files in MANIFEST from libldns

Then

perl Makefile.PL --no-ed25519
make all
make distcheck
make dist

The distribution file is copied to the target server:

$ lsb_release -a 2>/dev/null | grep Description
Description:	Ubuntu 22.04 LTS

Installing Zonemaster-LDNS (or trying):

$ sudo cpanm -iv Zonemaster-LDNS-2.2.2-PR151-7535a41-2022-10-17-A.tar.gz

Have you created a distrubution file? I could try yours, and you could try mine (enclosed).
Zonemaster-LDNS-2.2.2-PR151-7535a41-2022-10-17-A.tar.gz

ghost · 2022-10-17T15:05:15Z

With the distribution file you provided I could reproduce. I haven't looked more thoroughly into it yet. Here is the distribution file I used: Zonemaster-LDNS-2.2.2-PR151.tar.gz

matsduf · 2022-10-17T15:37:06Z

I compared the contents of the two packages. The following files differ:

./ldns/config.sub
./ldns/configure
./ldns/config.guess
./ldns/ltmain.sh
./ldns/ldns/config.h.in

The rest of the files are identical.

I tested your package on Ubuntu and FreeBSD, and it passed. I tested mine on FreeBSD too, and it failed. I will recreate the package and also create a package from master and develop branches.

matsduf · 2022-10-17T16:06:41Z

When changing between the branch of this PR and the develop branch the submodule path "ldns" must be updated. When testing, "git submodule update" seemed to do that (since the ldns/Changelog file was updated) but something did not work right. Something else must be done. We should support that since we need to be able to switch between branches with different submodule content.

Do you know the correct git commands to wipe the current content and checking out the correct content?

The following works, but there might be something in one command that works

git submodule deinit -f .
git submodule update --init --recursive

matsduf · 2022-10-17T16:37:04Z

With updated instructions for how to reset the submodule it seems to work. See zonemaster/zonemaster#1108 (and please review it). I will retest this PR.

matsduf

It works fine, but I have one request of change:

I suggest that the ldns/Changelog file moved from MANIFEST.SKIP to MANIFEST to make it easy to verify the version of LDNS in the Zonemaster-LDNS package.

ghost · 2022-10-19T07:50:12Z

When testing, "git submodule update" seemed to do that (since the ldns/Changelog file was updated) but something did not work right.

@matsduf can you be more explicit on what fails with git submodule update?

matsduf · 2022-10-19T11:16:45Z

When testing, "git submodule update" seemed to do that (since the ldns/Changelog file was updated) but something did not work right.

@matsduf can you be more explicit on what fails with git submodule update?

If you build Zonemaster-LDNS with one branch where LDNS is 1.7.1 (e.g. current develop) and then switch to another branch where LDNS is 1.8.3 then git submodule update will not do a complete clean-up. Specifically, ldns/configure/ will have the following lines

PACKAGE_VERSION='1.7.1'
PACKAGE_STRING='ldns 1.7.1'

instead of

PACKAGE_VERSION='1.8.3'
PACKAGE_STRING='ldns 1.8.3'

With git submodule deinit -f . ; git submodule update --init --recursive the file (ldns/configure) will be removed, and then recreated when building again.

ghost · 2022-10-20T14:43:28Z

I think I found a better solution than removing the submodule. This comes to using the update of ldns/Changelog as a prerequisite in the Makefile, see #158.

ghost requested review from mattias-p, matsduf, vlevigneron, hannaeko, tgreenx and marc-vanderwal August 23, 2022 17:23

Alexandre Pion added 2 commits September 28, 2022 15:32

Set and get EDNS option NSID for/from a packet

4fdde09

Enable NSID feature with LDNS version >= 1.8.2

c0ba9f3

ghost mentioned this pull request Sep 28, 2022

PoC: Send queries with EDNS option NSID set and retrieve the value if available zonemaster/zonemaster-engine#1133

Closed

2 tasks

matsduf requested changes Sep 28, 2022

View reviewed changes

Update internal ldns to 1.8.3

4e2d6da

ghost changed the title ~~POC: set and get EDNS option NSID for/from a packet~~ Add support for NSID option Sep 28, 2022

ghost requested a review from matsduf September 28, 2022 16:16

Correctly look up for NSID support

4463084

* internal LDNS is 1.8.3 so it supports NSID * external LDNS is unknown, a check is needed

ghost changed the title ~~Add support for NSID option~~ Add support for NSID option + update internal LDNS to 1.8.3 Oct 4, 2022

matsduf previously approved these changes Oct 5, 2022

View reviewed changes

ghost added this to the v2022.2 milestone Oct 12, 2022

Alexandre Pion added 2 commits October 12, 2022 14:32

Unit test for NSID support

56ed543

Don't overwrite already defined EDNS options

a8ccdb7

ghost dismissed matsduf’s stale review via a8ccdb7 October 12, 2022 12:32

ghost requested a review from matsduf October 12, 2022 12:33

matsduf requested changes Oct 14, 2022

View reviewed changes

Missing files in MANIFEST from libldns

7535a41

matsduf approved these changes Oct 18, 2022

View reviewed changes

ghost mentioned this pull request Oct 20, 2022

Automatically reconfigure internal libldns on "ldns/Changelog" updates #158

Merged

ghost merged commit b5930f6 into zonemaster:develop Nov 3, 2022

hannaeko added S-ReleaseTested Status: The PR has been successfully tested in release testing V-Minor Versioning: The change gives an update of minor in version. labels Dec 7, 2022

matsduf mentioned this pull request Dec 13, 2022

Consider updating to LDNS 1.8.1 #143

Closed

ghost deleted the nsid branch May 24, 2023 12:51

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for NSID option + update internal LDNS to 1.8.3 #151

Add support for NSID option + update internal LDNS to 1.8.3 #151

ghost commented Aug 23, 2022 •

edited by ghost

Loading

matsduf commented Aug 23, 2022

ghost commented Aug 24, 2022

matsduf commented Aug 25, 2022 •

edited

Loading

matsduf left a comment

ghost commented Sep 28, 2022

matsduf commented Sep 28, 2022

ghost commented Sep 28, 2022

matsduf left a comment

ghost commented Oct 12, 2022

matsduf left a comment

matsduf commented Oct 17, 2022

ghost commented Oct 17, 2022

matsduf commented Oct 17, 2022

ghost commented Oct 17, 2022

matsduf commented Oct 17, 2022 •

edited

Loading

matsduf commented Oct 17, 2022 •

edited

Loading

matsduf commented Oct 17, 2022

matsduf left a comment

ghost commented Oct 19, 2022

matsduf commented Oct 19, 2022 •

edited

Loading

ghost commented Oct 20, 2022 •

edited by ghost

Loading

Add support for NSID option + update internal LDNS to 1.8.3 #151

Add support for NSID option + update internal LDNS to 1.8.3 #151

Conversation

ghost commented Aug 23, 2022 • edited by ghost Loading

Purpose

Context

Changes

How to test this PR

matsduf commented Aug 23, 2022

ghost commented Aug 24, 2022

matsduf commented Aug 25, 2022 • edited Loading

matsduf left a comment

Choose a reason for hiding this comment

ghost commented Sep 28, 2022

matsduf commented Sep 28, 2022

ghost commented Sep 28, 2022

matsduf left a comment

Choose a reason for hiding this comment

ghost commented Oct 12, 2022

matsduf left a comment

Choose a reason for hiding this comment

matsduf commented Oct 17, 2022

ghost commented Oct 17, 2022

matsduf commented Oct 17, 2022

ghost commented Oct 17, 2022

matsduf commented Oct 17, 2022 • edited Loading

matsduf commented Oct 17, 2022 • edited Loading

matsduf commented Oct 17, 2022

matsduf left a comment

Choose a reason for hiding this comment

ghost commented Oct 19, 2022

matsduf commented Oct 19, 2022 • edited Loading

ghost commented Oct 20, 2022 • edited by ghost Loading

ghost commented Aug 23, 2022 •

edited by ghost

Loading

matsduf commented Aug 25, 2022 •

edited

Loading

matsduf commented Oct 17, 2022 •

edited

Loading

matsduf commented Oct 17, 2022 •

edited

Loading

matsduf commented Oct 19, 2022 •

edited

Loading

ghost commented Oct 20, 2022 •

edited by ghost

Loading