Drop root-server configuration option #276

k0ekk0ek · 2023-04-03T14:12:24Z

Fixes #275.

wcawijngaards

Looks like this would enable use of packaged versions, without needing a recompile. The new option is implemented with code in all the places it needs to.

wcawijngaards · 2023-04-05T07:53:48Z

dbaccess.c

+#ifdef ROOT_SERVER
+	root_server = nsd->options ? nsd->options->root_server : 1;
+#else
+	root_server = nsd->options ? nsd->options->root_server : 0;
+#endif


Having the define to allow root-server by default makes sense.

The define here seems to be only for when nsd.options is NULL. This only happens during the unit test. So, it does not set the default here, but only the value used during unit tests. Perhaps it is cleaner to say so in a comment, or no have the if on nsd.options NULL or not, and instead make changes in the unit tests to set the value. tpkg/cutest/cutest_namedb.c:90 in create_and_read_db. nsd.options = opt;
tpkg/cutest/cutest_iter.c:56 in create_zone, nsd.options = options;
And then also set nsd.options->root_server = 1; if it needs to be enabled for the unit test.

So, in that case, the code turns into ifdef ROOT_SERVER root_server = 1; else root_server = nsd->options->root_server; endif. Or am I missing the intent of the code and ifdef and NULL checks?

Also in tpks/cutest/qtest.c:75 nsd->options->root_server=1; could be useful for the unit test. Or edit the files tpkg/cutest_qroot.tdir/root.conf root2.conf and unsigned.conf to set the option. If test modifications is a good solution here these places may also need modification.

wcawijngaards · 2023-04-05T07:58:25Z

nsd-checkzone.c

@@ -80,7 +80,7 @@ check_zone(struct nsd* nsd, const char* name, const char* fname, FILE *out,
 	}

 	/* read the zone */
-	errors = zonec_read(name, fname, zone);
+	errors = zonec_read(name, fname, zone, nsd->options->root_server);


The value of nsd->options->root_server here is the default. The nsd.conf file has not been read in. For the user with a root zone that tries to nsd-checkzone the root.zone file, that would fail if they used the nsd.conf root-server: yes option. Because the default of no is from the options allocation routine, and the config file is not read at this point. It would need to read the config file to have that option, but this may also not be desirable, because of all the other stuff that can be in a config file, or maybe add a commandline option to set root-server value to nsd-checkzone.

Perhaps a simple nsd->options->root_server = 1; could suffice here. The operator with a wrongly configured root zone can be told the configuration does not work from the nsd-checkconf utility, or when the server starts. The zone file itself, can be just fine, and when checked produce no error by itself.

wcawijngaards · 2023-04-05T07:59:47Z

verify.c

@@ -112,11 +112,11 @@ static inline size_t print_line(struct verifier_stream *stream, int eof)
 		return 0;

 	if (len > LOGLINELEN) {
-		fmt = stream->cut ? ".. %.*s .." : "%.*s ..";
+		fmt = stream->cut ? "verifier: .. %.*s .." : "%.*s ..";


This seems to be from a different issue. Code looks fine, by the way.

anandb-ripencc · 2023-04-18T12:26:21Z

I wrote in #275 that the --enable-root-server option is not necessary. NSD is the only one that has this restriction, but it does not make sense. It has been argued that it will prevent accidents, but I have never seen an accident happen with BIND, Knot DNS or PowerDNS, none of which have such a restriction. Can you please reconsider this, and DROP the entire option altogether? Make things simpler, not more complex, please.

anandb-ripencc · 2023-04-18T12:28:23Z

If you can present an actual case of misconfiguration by an operator, leading to actual problems, I'd be willing to consider leaving this option in. Without such a case, and the mere theoretical danger foreseen by someone, is not enough to maintain this option.

anandb-ripencc · 2023-04-18T12:29:38Z

Name servers like BIND are going down the path of less code, less complexity and leanness, and here we have an example of more code and more complexity. It really feels wrong.

k0ekk0ek · 2023-04-20T09:23:36Z

Hi @anandb-ripencc. The option is to prevent people used to BIND from configuring a hints zone and thereby unintentionally serve a hint version of the root zone. Since this option has been there from the start, it'd make sense you did not see any issues pop up 🙂 For me personally, it's hard to state whether or not the option has actually prevented issues in the past or will help prevent them in the future. But, IMHO this is not the type of option that makes everything super complex or introduce bugs. Of course, your claim may still be valid. @wtoorop, @wcawijngaards any thoughts?

wcawijngaards · 2023-04-20T09:31:18Z

I think that implementing the options that Anand likes, is a good idea, because it is a feature for a root server, and RIPE is a root operator. But in addition, one of the failures is loading a root.hints file in a root zone, something that is prevented by a parse failure due to a lack of a SOA record. It is trying to be idiot proof, and this is not really meant for Anand, but for mistakes by other users, but I have no examples of these mistakes; people porting BIND config files and adding root zones; but then Anand says those software configs do not have the problem, so maybe it is not a problem or no longer perceived as such a problem.

k0ekk0ek · 2023-04-20T09:46:11Z

I didn't think of the "parse error" angle. If the file cannot be loaded to begin with, I think it makes sense to simply remove the option(s) and instead add a test (if it's not already there) to verify a hints file cannot be loaded(?)

wtoorop · 2023-04-20T09:51:22Z

Hi @anandb-ripencc. The option is to prevent people used to BIND from configuring a hints zone and thereby unintentionally serve a hint version of the root zone. Since this option has been there from the start, it'd make sense you did not see any issues pop up slightly_smiling_face For me personally, it's hard to state whether or not the option has actually prevented issues in the past or will help prevent them in the future. But, IMHO this is not the type of option that makes everything super complex or introduce bugs. Of course, your claim may still be valid. @wtoorop, @wcawijngaards any thoughts?

The change doesn't introduce any real or new complexity into the code, and it is the most conservative, most backwards compatible way to deal with the request (The default of the option is yes when --enable-root-server was used). So I'd say the PR is the proper approach.

That said, I'm also just fine with only changing the default value of the --enable-root-server option in configure.ac as the only change to address this. But then we should give a small warning about the backwards compatible change in the release notes I guess.

anandb-ripencc · 2023-04-20T10:35:19Z

I would like to maintain my view that the --enable-root-server option, as well as this new code, are all there for a THEORETICALLY perceived problem, one that I have never seen.

@wcawijngaards has already pointed out that the one possible scenario where users might get it wrong is the case of loading root.hints, but that won't work, as it is not a valid zone file. NSD will not load the zone, and there won't be an operational problem. I've been on the NSD mailing list for many years, and I've never heard of anyone asking why they could not load root.hints. Similarly, I've been on the Knot DNS mailing list for years, and have never come across anyone asking for this.

I am also unhappy about that idea that a new option in nsd.conf, called root_server, will default to either on or off depending on the compile time option. There will then be builds of NSD out there where the default behaviour will differ. One will only be able to tell the difference by looking at the nsd.conf man page. As a champion of consistency, I do not like this at all.

So I would still like to argue for dropping this --enable-root-server option altogether. You can emit a warning in case someone tries to use it.

anandb-ripencc · 2023-04-20T10:36:40Z

@bjovereinder what do you think of this discussion? I know you're neither a developer, nor an end user, but do you see my point?

wtoorop · 2023-04-20T11:40:53Z

For what it's worth, I do see your point @anandb-ripencc and I agree that the option would be confusing. So it looks to me we're all fine with dropping it (Jeroen was already suggesting that, and I also stated that I wouldn't mind).

bjovereinder · 2023-04-20T12:25:44Z

Hi Anand,

@bjovereinder what do you think of this discussion? I know you're neither a developer, nor an end user, but do you see my point?

Thanks. Jeroen was already updating the PR that largely covered your concerns, with attention on backward compatibility.

-- Benno

Fixes NLnetLabs#275.

k0ekk0ek · 2023-05-30T14:48:06Z

Updated according to comments. @wcawijngaards, can you have a quick look to see if you agree with the changes?

wcawijngaards · 2023-05-30T15:05:05Z

It looks okay to me.

Fixes NLnetLabs#240.

k0ekk0ek requested a review from wcawijngaards April 3, 2023 14:12

k0ekk0ek force-pushed the root-server branch from 7eb04e3 to 4aefe19 Compare April 5, 2023 08:05

wcawijngaards approved these changes Apr 5, 2023

View reviewed changes

Drop unnecessary root server checks

2ddbacd

Fixes NLnetLabs#275.

k0ekk0ek force-pushed the root-server branch from 4aefe19 to c57cbfe Compare May 30, 2023 14:44

Prefix messages originating from verifier

589e82e

Fixes NLnetLabs#240.

k0ekk0ek force-pushed the root-server branch from c57cbfe to 589e82e Compare May 30, 2023 15:09

k0ekk0ek changed the title ~~Add root-server configuration option~~ Drop root-server configuration option May 30, 2023

k0ekk0ek merged commit be37560 into NLnetLabs:master May 30, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop root-server configuration option #276

Drop root-server configuration option #276

k0ekk0ek commented Apr 3, 2023

wcawijngaards left a comment

wcawijngaards Apr 5, 2023

wcawijngaards Apr 5, 2023

wcawijngaards Apr 5, 2023

wcawijngaards Apr 5, 2023

wcawijngaards Apr 5, 2023

anandb-ripencc commented Apr 18, 2023 •

edited

anandb-ripencc commented Apr 18, 2023

anandb-ripencc commented Apr 18, 2023

k0ekk0ek commented Apr 20, 2023

wcawijngaards commented Apr 20, 2023

k0ekk0ek commented Apr 20, 2023

wtoorop commented Apr 20, 2023 •

edited

anandb-ripencc commented Apr 20, 2023

anandb-ripencc commented Apr 20, 2023 •

edited

wtoorop commented Apr 20, 2023 •

edited

bjovereinder commented Apr 20, 2023

k0ekk0ek commented May 30, 2023

wcawijngaards commented May 30, 2023

Drop root-server configuration option #276

Drop root-server configuration option #276

Conversation

k0ekk0ek commented Apr 3, 2023

wcawijngaards left a comment

Choose a reason for hiding this comment

wcawijngaards Apr 5, 2023

Choose a reason for hiding this comment

wcawijngaards Apr 5, 2023

Choose a reason for hiding this comment

wcawijngaards Apr 5, 2023

Choose a reason for hiding this comment

wcawijngaards Apr 5, 2023

Choose a reason for hiding this comment

wcawijngaards Apr 5, 2023

Choose a reason for hiding this comment

anandb-ripencc commented Apr 18, 2023 • edited

anandb-ripencc commented Apr 18, 2023

anandb-ripencc commented Apr 18, 2023

k0ekk0ek commented Apr 20, 2023

wcawijngaards commented Apr 20, 2023

k0ekk0ek commented Apr 20, 2023

wtoorop commented Apr 20, 2023 • edited

anandb-ripencc commented Apr 20, 2023

anandb-ripencc commented Apr 20, 2023 • edited

wtoorop commented Apr 20, 2023 • edited

bjovereinder commented Apr 20, 2023

k0ekk0ek commented May 30, 2023

wcawijngaards commented May 30, 2023

anandb-ripencc commented Apr 18, 2023 •

edited

wtoorop commented Apr 20, 2023 •

edited

anandb-ripencc commented Apr 20, 2023 •

edited

wtoorop commented Apr 20, 2023 •

edited