Bug 1269 - inconsistent use of built-in local zones with views
inconsistent use of built-in local zones with views
Status: ASSIGNED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-25 19:19 CEST by JINMEI Tatuya
Modified: 2017-05-31 19:16 CEST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description JINMEI Tatuya 2017-05-25 19:19:40 CEST
I've noticed some counter-intuitive behavior in the following configuration
(it's the case as of svn trunk revision 4190):

server:
[...]
	local-zone: a.onion redirect
	local-data: 'a.onion A 192.0.2.1'

	access-control-view: 127.0.0.1 view4
	access-control-view: ::1 view6

view:
	name: view4
	view-first: yes

view:
	name: view6
	view-first: yes
	local-zone: example.com always_nxdomain

That is, there's a global local-zone to override a built-in local-zone
"onion." (Note: it's the same for other built-in local-zones such as
"localhost.") Also, 2 views are defined, one with a view-specific local-zone
and the other without any view-specific local-zone.

If 'view4' is first consulted for a.onion/A, no view-specific local-zone
is applied and the overriding global local-zone is used.  So the answer
will be '192.0.2.1'.

If 'view6' is first consulted for a.onion/A, a built-in view-specific
local-zone for "onion." is applied and the answer will be NXDOMAIN.

The difference between these two cases seem to come from the fact
that view-specific built-in local-zones are not configured unless
there is at least one user-defined local-zone or local-data for the view.

This might be called a "feature", but I think this is quite
counter-intuitive from the user's point of view, since neither view
defines any explicit local zone for "onion.".

If the difference is accidental rather than intentional, I can think of
two alternatives:
1. always define built-in local-zones regardless of whether there's
   user-defined local-zone/data for the view
2. if view-first is yes, do not define built-in local-zones for any view.
   if view-first is no, always define built-in local-zones for the view
   like option 1.

Either way the behavior will be more "consistent".  I also personally think
option 2 will be more intuitive for users since any overriding by a global
local-zone/data will work naturally (and it's also a bit more memory
efficient if a huge number of views are defined with view-first being yes).

If, on the other hand, the "inconsistent" behavior is intentional for
a reason, I think it should at least be explicitly documented somewhere.
Comment 1 Ralph Dolmans 2017-05-30 15:13:15 CEST
Hi Jinmei,

Thank you for this bug report. Although intentional, I do recognize that this behavior can be counter-intuitive.

It is indeed not necessary to have the defaults in the view specific local-zone, as long as the global local-zone is used. This happens when view-first is "yes", or when the view does not have a local zone. Therefore, the default local-zones in the view are only needed when there is a local-zone and view-first is set to "no". I just committed code to change this behavior.

I also made some changes so that local-zones with type nodefault will be added to the view local zone, when there are no defaults in the view-specific local-zone. This way it is still be possible to exclude a default for a specific view.

Besides that, I made a change that the defaults are added to a view local-zone, when that local-zone creation is triggered by an unbound-control command, and view-first is "no".

Regards,
-- Ralph
Comment 2 JINMEI Tatuya 2017-05-30 23:20:20 CEST
Thanks for the response and for the update.

As of revision 4199 the behavior for 'view-first=yes' looks reasonable
to me.

I've noticed it might still be confusing when view-first is no.  If you
define the two views in the original problem description with view-first
being no:

view:
	name: view4
	view-first: no

view:
	name: view6
	view-first: no
	local-zone: example.com always_nxdomain

The default zones are used for 'view6' (because it has a user-defined
local-zone) but not for 'view4'.  I probably don't need to set view-first
to no, so that's not a big deal for my own purposes, but I guess this
can confuse users who need to have views with view-first=no.  If the behavior
is intentional you'll probably want to document it somewhere.

I also noticed one minor possible glitch in the code.
In services/view.c:views_apply_cfg():

					cfg_str2list_insert(&lz_cfg.local_zones,
						strdup(nd->str),
						strdup("nodefault"));

Failure of strdup() calls and of cfg_str2list_insert() is ignored.
It may be intentional, but even if so, if one call of strdup() succeeds
but the other fails the copied string will leak.  You'll probably at
least want to prevent the leak.
Comment 3 Ralph Dolmans 2017-05-31 13:48:41 CEST
Hi Jinmei,

You are right, this is not correct behaviour.

The issue is in the logic on when to use the global local-zones. I think these should also be used when view-first is no, but there is no local-zone in the view. This is in my opinion the correct behaviour because we like to add non local-zone related options to views in the future. The view should then only override the specified options, not all possible view option.

This was also what I had in mind while making last changes, hence the unclear behaviour. Do you agree? I just committed the code for this:

Index: services/localzone.c
===================================================================
--- services/localzone.c	(revision 4200)
+++ services/localzone.c	(working copy)
@@ -1590,7 +1590,7 @@
 			lock_rw_rdlock(&z->lock);
 			lzt = z->type;
 		}
-		if(!z && !view->isfirst){
+		if(view->local_zones && !z && !view->isfirst){
 			lock_rw_unlock(&view->lock);
 			return 0;
 		}

You are right that whatever behaviour we decide on, it has to be documented. I added some text to the unbound.conf man page.

Thanks for the remark about checking the strdup() return value. This can indeed result in a memory leak. Fixed.

Regards,
-- Ralph
Comment 4 JINMEI Tatuya 2017-05-31 19:16:58 CEST
Hi Ralph,

> This was also what I had in mind while making last changes, hence the
> unclear behaviour. Do you agree?

Okay, I now see the intent.  I suspect 'view-first' might become harder
to understand and use with that semantics, but I don't have a strong
opinion on the decision itself.  So, yes, I do agree in that sense.

I also confirmed the leak has been fixed.

Please feel free to close this report.