search

zonemaster / zonemaster-engine

The Zonemaster Engine - part of the Zonemaster project
Other
34 stars 33 forks source link

Not-existent name server doesn't cause a failure #278

Closed aabdnn closed 7 years ago

aabdnn commented 7 years ago

I expect this test to fail (because name server abcdef does not exist):

zonemaster-cli 193.in-addr.arpa --ns pri.authdns.ripe.net --ns sns-pb.isc.org --ns tinnie.arin.net --ns sec3.apnic.net --ns abcdef

However, Zonemaster (CLI 1.0.5, engine 1.0.16) happily passes this, with no errors emitted. This is a blocker for us, because it would allow bogus "nserver" attributes in domain objects in the RIPE Database.

matsduf commented 7 years ago

I can confirm the bug in CLI, and I think that I found the place in the code (https://github.com/dotse/zonemaster-cli/issues/39). I created a ticket in the correct repo.

@aabdnn, have you confirmed that the error exists in the Engine too? When I look at the code and method add_fake_delegation in Zonemaster.pm it is stated that all names must have at least one IP address.

aabdnn commented 7 years ago

Hi @matsduf. I think this logic is incorrect. IP addresses should only be required for in-zone name server names. If a name server name is outside the bailiwick of the zone, then only its name should be passed to the engine. The engine should then lookup the addresses of this name server, before starting tests.

Consequently, if the engine cannot resolve a name to addresses, then this is a failure, and the engine should report this failure.

From your description it seems that the CLI and the RPC API resolve name server names to addresses, and then pass these to the engine. In my opinion, this is wrong. Neither the CLI nor the RPC API should do any resolution. They should merely be interfaces to the engine, and it is the engine that should figure all this out and fail if a bogus name server is passed to it.

aabdnn commented 7 years ago

I meant to mention that this same issue occurs when a test is submitted by the RPC API. So the problem isn't just related to the CLI.

matsduf commented 7 years ago

@aabdnn

I meant to mention that this same issue occurs when a test is submitted by the RPC API. So the problem isn't just related to the CLI.

I have confirmed that Zonemaster Backend has the same problem (bug) and I have created an issue for that (https://github.com/dotse/zonemaster-backend/issues/266). If that problem comes from code in Zonemaster Backend or Zonemaster Engine is not clear (to me). As you can see, I have confirmed that Zonemaster CLI has as bug in its own code creating its problem.

I think this logic is incorrect. IP addresses should only be required for in-zone name server names. If a name server name is outside the bailiwick of the zone, then only its name should be passed to the engine. The engine should then lookup the addresses of this name server, before starting tests.

I agree with you that the behavior is incorrect. In-zone names must not be looked up in undelegated tests. The IP address must be provided as glue. I do not want to define a design now, but I do think that things that are shared between CLI and Backend should be exposed as a "service" in Engine. See issues https://github.com/dotse/zonemaster-cli/issues/40 and https://github.com/dotse/zonemaster-backend/issues/267.

aabdnn commented 7 years ago

Hi @matsduf any update on this issue, and a fix for it?

matsduf commented 7 years ago

@aabdnn, not yet but we have identified it as prioritized.

sandoche2k commented 7 years ago

@aabdnn @vlevigneron will be applying a patch for the Engine. Then @mtoma will be look at the compatibility of the patch with the backend

aabdnn commented 7 years ago

Thanks for the update @sandoche2k. I will wait to hear from you guys next week.