[systemd-devel] [PATCH] resolved: re-add support for getting local domain from DHCP

[Michael Marineau]

When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.
---

This is a resend, rebased since some recent changes changed how this
patch needed to be implemented.

src/network/networkd-link.c | 13 +++++++++++++
src/network/sd-network.c | 24 ++++++++++++++++++++++++
src/resolve/resolved-link.c | 20 ++++++++++++++++++++
src/resolve/resolved-link.h | 2 ++
src/resolve/resolved-manager.c | 10 +++++++++-
src/systemd/sd-network.h | 3 +++
6 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 3b8b7ed..827c428 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -2451,6 +2451,19 @@ int link_save(Link *link) {
(address + 1 ? " " : ""));

fputs("\n", f);
+
+ fprintf(f, "DOMAINNAME=");
+
+ if (link->network->dhcp_domainname &&
+ link->dhcp_lease) {
+ const char *domainname;
+
+ r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);
+ if (r >= 0)
+ fputs(domainname, f);
+ }
+
+ fputs("\n", f);
}

if (link->dhcp_lease) {
diff --git a/src/network/sd-network.c b/src/network/sd-network.c
index bfb8321..a427a27 100644
--- a/src/network/sd-network.c
+++ b/src/network/sd-network.c
@@ -175,6 +175,30 @@ _public_ int sd_network_get_ntp(int ifindex, char ***ret) {
return network_get_strv("NTP", ifindex, ret);
}

+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;
+
+ r = parse_env_file(p, NEWLINE, "DOMAINNAME", &s, NULL);
+ if (r == -ENOENT)
+ return -ENODATA;
+ else if (r < 0)
+ return r;
+ else if (!s)
+ return -EIO;
+
+ *domainname = s;
+ s = NULL;
+
+ return 0;
+}
+
static inline int MONITOR_TO_FD(sd_network_monitor *m) {
return (int) (unsigned long) m - 1;
}
diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c
index 6ac7c5b..f6b7f6a 100644
--- a/src/resolve/resolved-link.c
+++ b/src/resolve/resolved-link.c
@@ -77,6 +77,7 @@ Link *link_free(Link *l) {
while (l->dns_servers)
dns_server_free(l->dns_servers);

+ free(l->domainname);
free(l);
return NULL;
}
@@ -191,10 +192,29 @@ clear:
return r;
}

+static int link_update_domainname(Link *l) {
+ char *domainname = NULL;
+ int r;
+
+ assert(l);
+
+ free(l->domainname);
+ l->domainname = NULL;
+
+ r = sd_network_get_domainname(l->ifindex, &domainname);
+ if (r < 0)
+ return r;
+
+ l->domainname = domainname;
+
+ return 0;
+}
+
int link_update_monitor(Link *l) {
assert(l);

link_update_dns_servers(l);
+ link_update_domainname(l);
link_allocate_scopes(l);
link_add_rrs(l);

diff --git a/src/resolve/resolved-link.h b/src/resolve/resolved-link.h
index f58bd54..9730aec 100644
--- a/src/resolve/resolved-link.h
+++ b/src/resolve/resolved-link.h
@@ -68,6 +68,8 @@ struct Link {

RateLimit mdns_ratelimit;
RateLimit llmnr_ratelimit;
+
+ char *domainname;
};

int link_new(Manager *m, Link **ret, int ifindex);
diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index a8715bd..253a97e 100644
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -522,6 +522,7 @@ int manager_write_resolv_conf(Manager *m) {
const char *path = "/run/systemd/resolve/resolv.conf";
_cleanup_free_ char *temp_path = NULL;
_cleanup_fclose_ FILE *f = NULL;
+ const char *domainname = NULL;
unsigned count = 0;
DnsServer *s;
Iterator i;
@@ -542,13 +543,20 @@ int manager_write_resolv_conf(Manager *m) {
"# resolv.conf(5) in a different way, replace the symlink by a\n"
"# static file or a different symlink.\n\n", f);

- HASHMAP_FOREACH(l, m->links, i)
+ HASHMAP_FOREACH(l, m->links, i) {
LIST_FOREACH(servers, s, l->dns_servers)
write_resolve_conf_server(s, f, &count);

+ if (!domainname && l->domainname)
+ domainname = l->domainname;
+ }
+
LIST_FOREACH(servers, s, m->dns_servers)
write_resolve_conf_server(s, f, &count);

+ if (domainname)
+ fprintf(f, "domain %s\n", domainname);
+
r = fflush_and_check(f);
if (r < 0)
goto fail;
diff --git a/src/systemd/sd-network.h b/src/systemd/sd-network.h
index ec01e07..06f147d 100644
--- a/src/systemd/sd-network.h
+++ b/src/systemd/sd-network.h
@@ -84,6 +84,9 @@ int sd_network_get_dns(int ifindex, char ***addr);
* reperesentations of IP addresses */
int sd_network_get_ntp(int ifindex, char ***addr);

+/* Get the DNS domain name for a given link. */
+int sd_network_get_domainname(int ifindex, char **domainname);
+
/* Monitor object */
typedef struct sd_network_monitor sd_network_monitor;

[Zbigniew Jędrzejewski-Szmek]

Post by Michael Marineau
When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.
---
This is a resend, rebased since some recent changes changed how this
patch needed to be implemented.
src/network/networkd-link.c | 13 +++++++++++++
src/network/sd-network.c | 24 ++++++++++++++++++++++++
src/resolve/resolved-link.c | 20 ++++++++++++++++++++
src/resolve/resolved-link.h | 2 ++
src/resolve/resolved-manager.c | 10 +++++++++-
src/systemd/sd-network.h | 3 +++
6 files changed, 71 insertions(+), 1 deletion(-)
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 3b8b7ed..827c428 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -2451,6 +2451,19 @@ int link_save(Link *link) {
(address + 1 ? " " : ""));
fputs("\n", f);
+
+ fprintf(f, "DOMAINNAME=");
+
+ if (link->network->dhcp_domainname &&
+ link->dhcp_lease) {
+ const char *domainname;
+
+ r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);
+ if (r >= 0)
+ fputs(domainname, f);
+ }
+
+ fputs("\n", f);

Is it really necessary to write anything if the name is not available?
Other parts of this function don't write anyting in similar cases.

Post by Michael Marineau
if (link->dhcp_lease) {
diff --git a/src/network/sd-network.c b/src/network/sd-network.c
index bfb8321..a427a27 100644
--- a/src/network/sd-network.c
+++ b/src/network/sd-network.c
@@ -175,6 +175,30 @@ _public_ int sd_network_get_ntp(int ifindex, char ***ret) {
return network_get_strv("NTP", ifindex, ret);
}
+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;

Not terribly important, but please spell that as:

char p[sizeof("/run/systemd/netif/links/") + DECIMAL_STRING_MAX(int)];
snprintf(p, sizeof(p), "/run/systemd/netif/links/%d", ifindex);

Post by Michael Marineau
+ r = parse_env_file(p, NEWLINE, "DOMAINNAME", &s, NULL);
+ if (r == -ENOENT)
+ return -ENODATA;
+ else if (r < 0)
+ return r;
+ else if (!s)
+ return -EIO;
+
+ *domainname = s;
+ s = NULL;
+
+ return 0;
+}
+
static inline int MONITOR_TO_FD(sd_network_monitor *m) {
return (int) (unsigned long) m - 1;
}
diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c
index 6ac7c5b..f6b7f6a 100644
--- a/src/resolve/resolved-link.c
+++ b/src/resolve/resolved-link.c
@@ -77,6 +77,7 @@ Link *link_free(Link *l) {
while (l->dns_servers)
dns_server_free(l->dns_servers);
+ free(l->domainname);
free(l);
return NULL;
}
return r;
}
+static int link_update_domainname(Link *l) {
+ char *domainname = NULL;
+ int r;
+
+ assert(l);
+
+ free(l->domainname);
+ l->domainname = NULL;
+
+ r = sd_network_get_domainname(l->ifindex, &domainname);
+ if (r < 0)
+ return r;
+
+ l->domainname = domainname;
+
+ return 0;
+}
+
int link_update_monitor(Link *l) {
assert(l);
link_update_dns_servers(l);
+ link_update_domainname(l);
link_allocate_scopes(l);
link_add_rrs(l);
diff --git a/src/resolve/resolved-link.h b/src/resolve/resolved-link.h
index f58bd54..9730aec 100644
--- a/src/resolve/resolved-link.h
+++ b/src/resolve/resolved-link.h
@@ -68,6 +68,8 @@ struct Link {
RateLimit mdns_ratelimit;
RateLimit llmnr_ratelimit;
+
+ char *domainname;
};
int link_new(Manager *m, Link **ret, int ifindex);
diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index a8715bd..253a97e 100644
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -522,6 +522,7 @@ int manager_write_resolv_conf(Manager *m) {
const char *path = "/run/systemd/resolve/resolv.conf";
_cleanup_free_ char *temp_path = NULL;
_cleanup_fclose_ FILE *f = NULL;
+ const char *domainname = NULL;
unsigned count = 0;
DnsServer *s;
Iterator i;
@@ -542,13 +543,20 @@ int manager_write_resolv_conf(Manager *m) {
"# resolv.conf(5) in a different way, replace the symlink by a\n"
"# static file or a different symlink.\n\n", f);
- HASHMAP_FOREACH(l, m->links, i)
+ HASHMAP_FOREACH(l, m->links, i) {
LIST_FOREACH(servers, s, l->dns_servers)
write_resolve_conf_server(s, f, &count);
+ if (!domainname && l->domainname)
+ domainname = l->domainname;
+ }
+
LIST_FOREACH(servers, s, m->dns_servers)
write_resolve_conf_server(s, f, &count);
+ if (domainname)
+ fprintf(f, "domain %s\n", domainname);
+

How does this deal with empty domain names? Let's say the server is
misconfigure and returns "". Will this still be valid syntax? Also,
are the contents checked for special characters, etc, earlier?

Zbyszek

[Michael Marineau]

On Tue, Jul 29, 2014 at 3:37 PM, Zbigniew Jędrzejewski-Szmek

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.
---
This is a resend, rebased since some recent changes changed how this
patch needed to be implemented.
src/network/networkd-link.c | 13 +++++++++++++
src/network/sd-network.c | 24 ++++++++++++++++++++++++
src/resolve/resolved-link.c | 20 ++++++++++++++++++++
src/resolve/resolved-link.h | 2 ++
src/resolve/resolved-manager.c | 10 +++++++++-
src/systemd/sd-network.h | 3 +++
6 files changed, 71 insertions(+), 1 deletion(-)
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 3b8b7ed..827c428 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -2451,6 +2451,19 @@ int link_save(Link *link) {
(address + 1 ? " " : ""));
fputs("\n", f);
+
+ fprintf(f, "DOMAINNAME=");
+
+ if (link->network->dhcp_domainname &&
+ link->dhcp_lease) {
+ const char *domainname;
+
+ r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);
+ if (r >= 0)
+ fputs(domainname, f);
+ }
+
+ fputs("\n", f);

Is it really necessary to write anything if the name is not available?
Other parts of this function don't write anyting in similar cases.

I was just matching the above lines which may write DNS= or NTP= with
blank values. I don't think it matters either way. Omitting
DOMAINNAME= if it is blank certainly looks a little cleaner since the
writes get squashed into a single fprintf. Will update.

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
if (link->dhcp_lease) {
diff --git a/src/network/sd-network.c b/src/network/sd-network.c
index bfb8321..a427a27 100644
--- a/src/network/sd-network.c
+++ b/src/network/sd-network.c
@@ -175,6 +175,30 @@ _public_ int sd_network_get_ntp(int ifindex, char ***ret) {
return network_get_strv("NTP", ifindex, ret);
}
+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;

char p[sizeof("/run/systemd/netif/links/") + DECIMAL_STRING_MAX(int)];
snprintf(p, sizeof(p), "/run/systemd/netif/links/%d", ifindex);

This was copied verbatim from similar functions in this file, should I
update the style of the others to match your suggestion? Why the
preference of manually calculating a buffer length than using
asprintf?

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
+ r = parse_env_file(p, NEWLINE, "DOMAINNAME", &s, NULL);
+ if (r == -ENOENT)
+ return -ENODATA;
+ else if (r < 0)
+ return r;
+ else if (!s)
+ return -EIO;
+
+ *domainname = s;
+ s = NULL;
+
+ return 0;
+}
+
static inline int MONITOR_TO_FD(sd_network_monitor *m) {
return (int) (unsigned long) m - 1;
}
diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c
index 6ac7c5b..f6b7f6a 100644
--- a/src/resolve/resolved-link.c
+++ b/src/resolve/resolved-link.c
@@ -77,6 +77,7 @@ Link *link_free(Link *l) {
while (l->dns_servers)
dns_server_free(l->dns_servers);
+ free(l->domainname);
free(l);
return NULL;
}
return r;
}
+static int link_update_domainname(Link *l) {
+ char *domainname = NULL;
+ int r;
+
+ assert(l);
+
+ free(l->domainname);
+ l->domainname = NULL;
+
+ r = sd_network_get_domainname(l->ifindex, &domainname);
+ if (r < 0)
+ return r;
+
+ l->domainname = domainname;
+
+ return 0;
+}
+
int link_update_monitor(Link *l) {
assert(l);
link_update_dns_servers(l);
+ link_update_domainname(l);
link_allocate_scopes(l);
link_add_rrs(l);
diff --git a/src/resolve/resolved-link.h b/src/resolve/resolved-link.h
index f58bd54..9730aec 100644
--- a/src/resolve/resolved-link.h
+++ b/src/resolve/resolved-link.h
@@ -68,6 +68,8 @@ struct Link {
RateLimit mdns_ratelimit;
RateLimit llmnr_ratelimit;
+
+ char *domainname;
};
int link_new(Manager *m, Link **ret, int ifindex);
diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index a8715bd..253a97e 100644
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -522,6 +522,7 @@ int manager_write_resolv_conf(Manager *m) {
const char *path = "/run/systemd/resolve/resolv.conf";
_cleanup_free_ char *temp_path = NULL;
_cleanup_fclose_ FILE *f = NULL;
+ const char *domainname = NULL;
unsigned count = 0;
DnsServer *s;
Iterator i;
@@ -542,13 +543,20 @@ int manager_write_resolv_conf(Manager *m) {
"# resolv.conf(5) in a different way, replace the symlink by a\n"
"# static file or a different symlink.\n\n", f);
- HASHMAP_FOREACH(l, m->links, i)
+ HASHMAP_FOREACH(l, m->links, i) {
LIST_FOREACH(servers, s, l->dns_servers)
write_resolve_conf_server(s, f, &count);
+ if (!domainname && l->domainname)
+ domainname = l->domainname;
+ }
+
LIST_FOREACH(servers, s, m->dns_servers)
write_resolve_conf_server(s, f, &count);
+ if (domainname)
+ fprintf(f, "domain %s\n", domainname);
+

How does this deal with empty domain names? Let's say the server is
misconfigure and returns "". Will this still be valid syntax? Also,
are the contents checked for special characters, etc, earlier?

sd_network_get_domainname() returns NULL for empty strings following
the behavior of parse_env_file() so simply checking for NULL elsewhere
is sufficient.

[Zbigniew Jędrzejewski-Szmek]

Post by Michael Marineau
On Tue, Jul 29, 2014 at 3:37 PM, Zbigniew Jędrzejewski-Szmek

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.
---
This is a resend, rebased since some recent changes changed how this
patch needed to be implemented.
src/network/networkd-link.c | 13 +++++++++++++
src/network/sd-network.c | 24 ++++++++++++++++++++++++
src/resolve/resolved-link.c | 20 ++++++++++++++++++++
src/resolve/resolved-link.h | 2 ++
src/resolve/resolved-manager.c | 10 +++++++++-
src/systemd/sd-network.h | 3 +++
6 files changed, 71 insertions(+), 1 deletion(-)
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 3b8b7ed..827c428 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -2451,6 +2451,19 @@ int link_save(Link *link) {
(address + 1 ? " " : ""));
fputs("\n", f);
+
+ fprintf(f, "DOMAINNAME=");
+
+ if (link->network->dhcp_domainname &&
+ link->dhcp_lease) {
+ const char *domainname;
+
+ r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);
+ if (r >= 0)
+ fputs(domainname, f);
+ }
+
+ fputs("\n", f);

Is it really necessary to write anything if the name is not available?
Other parts of this function don't write anyting in similar cases.

I was just matching the above lines which may write DNS= or NTP= with
blank values. I don't think it matters either way. Omitting
DOMAINNAME= if it is blank certainly looks a little cleaner since the
writes get squashed into a single fprintf. Will update.

You're right, I misread the surrounding code. It would probably be nice
to not write anything in thos cases but it should be a separate patch anyway,
that does all the places at once.

Post by Michael Marineau

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
if (link->dhcp_lease) {
diff --git a/src/network/sd-network.c b/src/network/sd-network.c
index bfb8321..a427a27 100644
--- a/src/network/sd-network.c
+++ b/src/network/sd-network.c
@@ -175,6 +175,30 @@ _public_ int sd_network_get_ntp(int ifindex, char ***ret) {
return network_get_strv("NTP", ifindex, ret);
}
+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;

char p[sizeof("/run/systemd/netif/links/") + DECIMAL_STRING_MAX(int)];
snprintf(p, sizeof(p), "/run/systemd/netif/links/%d", ifindex);

This was copied verbatim from similar functions in this file, should I
update the style of the others to match your suggestion? Why the
preference of manually calculating a buffer length than using
asprintf?

You're right, in this case asprintf is totally appropriate.

Post by Michael Marineau

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
+ r = parse_env_file(p, NEWLINE, "DOMAINNAME", &s, NULL);
+ if (r == -ENOENT)
+ return -ENODATA;
+ else if (r < 0)
+ return r;
+ else if (!s)
+ return -EIO;
+
+ *domainname = s;
+ s = NULL;
+
+ return 0;
+}
+
static inline int MONITOR_TO_FD(sd_network_monitor *m) {
return (int) (unsigned long) m - 1;
}
diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c
index 6ac7c5b..f6b7f6a 100644
--- a/src/resolve/resolved-link.c
+++ b/src/resolve/resolved-link.c
@@ -77,6 +77,7 @@ Link *link_free(Link *l) {
while (l->dns_servers)
dns_server_free(l->dns_servers);
+ free(l->domainname);
free(l);
return NULL;
}
return r;
}
+static int link_update_domainname(Link *l) {
+ char *domainname = NULL;
+ int r;
+
+ assert(l);
+
+ free(l->domainname);
+ l->domainname = NULL;
+
+ r = sd_network_get_domainname(l->ifindex, &domainname);
+ if (r < 0)
+ return r;
+
+ l->domainname = domainname;
+
+ return 0;
+}
+
int link_update_monitor(Link *l) {
assert(l);
link_update_dns_servers(l);
+ link_update_domainname(l);
link_allocate_scopes(l);
link_add_rrs(l);
diff --git a/src/resolve/resolved-link.h b/src/resolve/resolved-link.h
index f58bd54..9730aec 100644
--- a/src/resolve/resolved-link.h
+++ b/src/resolve/resolved-link.h
@@ -68,6 +68,8 @@ struct Link {
RateLimit mdns_ratelimit;
RateLimit llmnr_ratelimit;
+
+ char *domainname;
};
int link_new(Manager *m, Link **ret, int ifindex);
diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index a8715bd..253a97e 100644
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -522,6 +522,7 @@ int manager_write_resolv_conf(Manager *m) {
const char *path = "/run/systemd/resolve/resolv.conf";
_cleanup_free_ char *temp_path = NULL;
_cleanup_fclose_ FILE *f = NULL;
+ const char *domainname = NULL;
unsigned count = 0;
DnsServer *s;
Iterator i;
@@ -542,13 +543,20 @@ int manager_write_resolv_conf(Manager *m) {
"# resolv.conf(5) in a different way, replace the symlink by a\n"
"# static file or a different symlink.\n\n", f);
- HASHMAP_FOREACH(l, m->links, i)
+ HASHMAP_FOREACH(l, m->links, i) {
LIST_FOREACH(servers, s, l->dns_servers)
write_resolve_conf_server(s, f, &count);
+ if (!domainname && l->domainname)
+ domainname = l->domainname;
+ }
+
LIST_FOREACH(servers, s, m->dns_servers)
write_resolve_conf_server(s, f, &count);
+ if (domainname)
+ fprintf(f, "domain %s\n", domainname);
+

How does this deal with empty domain names? Let's say the server is
misconfigure and returns "". Will this still be valid syntax? Also,
are the contents checked for special characters, etc, earlier?

sd_network_get_domainname() returns NULL for empty strings following
the behavior of parse_env_file() so simply checking for NULL elsewhere
is sufficient.

OK. It seems that technically the patch is correct. But I'm less sure that
we actually want that. I mean your patch would promote one of the (potentially)
many domain names as special. It feels as if either any or none should be
used, ie. maybe 'search' is more appropriate... I'll let Tom or Lennart
comment on this.

Zbyszek

[Michael Marineau]

When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.
---

This is a refresh of the patch on recent master with a little bit of
cleanup from the last. Regarding the robustness/correctness/etc of
setting the "domain" resolv.conf attribute from DNS, I don't think that
is practical to address in this patch. The implementation is already
clearly incomplete because networkd doesn't handle "search" domains
which is actually an entirely different option. My goal here is to just
fix the regression from when resolved was first introduced.

The most common setup is for "domain" to correspond to the domain suffix
for the local host name. If you are concerned about which interface's
domain attribute wins and lands in resolv.conf, there is a related issue
of which interface's host name winds up being applied as the host's
transient host name. What ever interface wins the two should probably
match but this is complicated significantly by the two being handled by
different daemons, resolved vs hostnamed, with two different
integration points with networkd, reading state files in /run vs dbus
method calls. I don't have a good recommendation to make sense of any of
this right now.

src/network/networkd-link.c | 9 +++++++++
src/network/sd-network.c | 24 ++++++++++++++++++++++++
src/resolve/resolved-link.c | 20 ++++++++++++++++++++
src/resolve/resolved-link.h | 2 ++
src/resolve/resolved-manager.c | 10 +++++++++-
src/systemd/sd-network.h | 3 +++
6 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 172be64..42d528f 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -2385,6 +2385,15 @@ int link_save(Link *link) {
(address + 1 ? " " : ""));

fputs("\n", f);
+
+ if (link->network->dhcp_domainname &&
+ link->dhcp_lease) {
+ const char *domainname;
+
+ r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);
+ if (r >= 0)
+ fprintf(f, "DOMAINNAME=%s\n", domainname);
+ }
}

if (link->dhcp_lease) {
diff --git a/src/network/sd-network.c b/src/network/sd-network.c
index bfb8321..a427a27 100644
--- a/src/network/sd-network.c
+++ b/src/network/sd-network.c
@@ -175,6 +175,30 @@ _public_ int sd_network_get_ntp(int ifindex, char ***ret) {
return network_get_strv("NTP", ifindex, ret);
}

+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;
+
+ r = parse_env_file(p, NEWLINE, "DOMAINNAME", &s, NULL);
+ if (r == -ENOENT)
+ return -ENODATA;
+ else if (r < 0)
+ return r;
+ else if (!s)
+ return -EIO;
+
+ *domainname = s;
+ s = NULL;
+
+ return 0;
+}
+
static inline int MONITOR_TO_FD(sd_network_monitor *m) {
return (int) (unsigned long) m - 1;
}
diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c
index 2c02f09..9d582e4 100644
--- a/src/resolve/resolved-link.c
+++ b/src/resolve/resolved-link.c
@@ -74,6 +74,7 @@ Link *link_free(Link *l) {
while (l->dns_servers)
dns_server_free(l->dns_servers);

+ free(l->domainname);
free(l);
return NULL;
}
@@ -188,10 +189,29 @@ clear:
return r;
}

+static int link_update_domainname(Link *l) {
+ char *domainname = NULL;
+ int r;
+
+ assert(l);
+
+ free(l->domainname);
+ l->domainname = NULL;
+
+ r = sd_network_get_domainname(l->ifindex, &domainname);
+ if (r < 0)
+ return r;
+
+ l->domainname = domainname;
+
+ return 0;
+}
+
int link_update_monitor(Link *l) {
assert(l);

link_update_dns_servers(l);
+ link_update_domainname(l);
link_allocate_scopes(l);
link_add_rrs(l, false);

diff --git a/src/resolve/resolved-link.h b/src/resolve/resolved-link.h
index 38bb392..eed9f42 100644
--- a/src/resolve/resolved-link.h
+++ b/src/resolve/resolved-link.h
@@ -68,6 +68,8 @@ struct Link {

RateLimit mdns_ratelimit;
RateLimit llmnr_ratelimit;
+
+ char *domainname;
};

int link_new(Manager *m, Link **ret, int ifindex);
diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index 1b6dc8a..8f28eaf 100644
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -648,6 +648,7 @@ int manager_write_resolv_conf(Manager *m) {
static const char path[] = "/run/systemd/resolve/resolv.conf";
_cleanup_free_ char *temp_path = NULL;
_cleanup_fclose_ FILE *f = NULL;
+ const char *domainname = NULL;
unsigned count = 0;
DnsServer *s;
Iterator i;
@@ -674,15 +675,22 @@ int manager_write_resolv_conf(Manager *m) {
LIST_FOREACH(servers, s, m->dns_servers)
write_resolve_conf_server(s, f, &count);

- HASHMAP_FOREACH(l, m->links, i)
+ HASHMAP_FOREACH(l, m->links, i) {
LIST_FOREACH(servers, s, l->dns_servers)
write_resolve_conf_server(s, f, &count);

+ if (!domainname && l->domainname)
+ domainname = l->domainname;
+ }
+
if (count == 0) {
LIST_FOREACH(servers, s, m->fallback_dns_servers)
write_resolve_conf_server(s, f, &count);
}

+ if (domainname)
+ fprintf(f, "domain %s\n", domainname);
+
r = fflush_and_check(f);
if (r < 0)
goto fail;
diff --git a/src/systemd/sd-network.h b/src/systemd/sd-network.h
index ec01e07..06f147d 100644
--- a/src/systemd/sd-network.h
+++ b/src/systemd/sd-network.h
@@ -84,6 +84,9 @@ int sd_network_get_dns(int ifindex, char ***addr);
* reperesentations of IP addresses */
int sd_network_get_ntp(int ifindex, char ***addr);

+/* Get the DNS domain name for a given link. */
+int sd_network_get_domainname(int ifindex, char **domainname);
+
/* Monitor object */
typedef struct sd_network_monitor sd_network_monitor;

[Lennart Poettering]

On Mon, 04.08.14 10:05, Michael Marineau (***@coreos.com) wrote:

Patch looks pretty good, though I'd really prefer if we could do the
UseDomain= thing as discussed in the other mail, and not propagate
DHCP-supplied domain names unless explicitly requested.

This would means we probably mean we'd need two new sd-network.h calls:

int sd_network_get_link_route_domains(int ifindex, char **domains);
int sd_network_get_link_search_domains(int ifindex, char **domains);

The former would return the list of domains whose requests shall be
routed to the specified interface, and the latter would be the list of
domains we actively use for searching single-label domains in.

Any domains configured statically for a link in the .network files would
be listed in both lists. And depending on the UseDomains= settings the
dhcp provides domains might be listed on none, both or only one of
them. or something like that...

Post by Michael Marineau
src/network/networkd-link.c | 9 +++++++++
src/network/sd-network.c | 24 ++++++++++++++++++++++++
src/resolve/resolved-link.c | 20 ++++++++++++++++++++
src/resolve/resolved-link.h | 2 ++
src/resolve/resolved-manager.c | 10 +++++++++-
src/systemd/sd-network.h | 3 +++
6 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 172be64..42d528f 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -2385,6 +2385,15 @@ int link_save(Link *link) {
(address + 1 ? " " : ""));
fputs("\n", f);
+
+ if (link->network->dhcp_domainname &&
+ link->dhcp_lease) {
+ const char *domainname;
+
+ r = sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);
+ if (r >= 0)
+ fprintf(f, "DOMAINNAME=%s\n",
domainname);

THis should be plural really, from the beginning. After all the newer
DHCP specs allow a full list... and we want to allow a full list to be
provided in the .network files too...

Lennart

[Michael Marineau]

Post by Lennart Poettering
Patch looks pretty good, though I'd really prefer if we could do the
UseDomain= thing as discussed in the other mail, and not propagate
DHCP-supplied domain names unless explicitly requested.
int sd_network_get_link_route_domains(int ifindex, char **domains);
int sd_network_get_link_search_domains(int ifindex, char **domains);
The former would return the list of domains whose requests shall be
routed to the specified interface, and the latter would be the list of
domains we actively use for searching single-label domains in.
Any domains configured statically for a link in the .network files would
be listed in both lists. And depending on the UseDomains= settings the
dhcp provides domains might be listed on none, both or only one of
them. or something like that...

Post by Michael Marineau
src/network/networkd-link.c | 9 +++++++++
src/network/sd-network.c | 24 ++++++++++++++++++++++++
src/resolve/resolved-link.c | 20 ++++++++++++++++++++
src/resolve/resolved-link.h | 2 ++
src/resolve/resolved-manager.c | 10 +++++++++-
src/systemd/sd-network.h | 3 +++
6 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c
index 172be64..42d528f 100644
--- a/src/network/networkd-link.c
+++ b/src/network/networkd-link.c
@@ -2385,6 +2385,15 @@ int link_save(Link *link) {
(address + 1 ? " " : ""));
fputs("\n", f);
+
+ if (link->network->dhcp_domainname &&
+ link->dhcp_lease) {
+ const char *domainname;
+
+ r =

sd_dhcp_lease_get_domainname(link->dhcp_lease, &domainname);

Post by Lennart Poettering

Post by Michael Marineau
+ if (r >= 0)
+ fprintf(f, "DOMAINNAME=%s\n", domainname);

THis should be plural really, from the beginning. After all the newer
DHCP specs allow a full list... and we want to allow a full list to be
provided in the .network files too...
Lennart
--
Lennart Poettering, Red Hat

Right now the search domains DHCP option is unsupported so it is indeed
singular. Also I had assumed since search domains is both a different DHCP
option and a different resolve.conf option that they would be recorded
separately but I suppose the two options is more of a legacy artifact than
meaningful distinction so it is equally as valid to squash them together
into the search domain list. I am happy to write a follow up patch to
implement the search domains option and support providing additional
domains in the .network file but I think this patch can stand alone since
it fixes a regression.

[Lennart Poettering]

Post by Lennart Poettering
Patch looks pretty good, though I'd really prefer if we could do the
UseDomain= thing as discussed in the other mail, and not propagate
DHCP-supplied domain names unless explicitly requested.
int sd_network_get_link_route_domains(int ifindex, char **domains);
int sd_network_get_link_search_domains(int ifindex, char **domains);
The former would return the list of domains whose requests shall be
routed to the specified interface, and the latter would be the list of
domains we actively use for searching single-label domains in.
Any domains configured statically for a link in the .network files would
be listed in both lists. And depending on the UseDomains= settings the
dhcp provides domains might be listed on none, both or only one of
them. or something like that...

So, I have thought about this a bit more over night... I think this
would still be too complicated, and I don't really see the need to make
the dhcp search thing that configurable. I see no risk in strictly
making single-label host names searchable in all configured domains of
the interface and nothing else...

I think I am back at suggesting that there should be the follow .network settings:

[Network]
Domains=0pointer.de redhat.com

[DHCP]
UseDomains=yes|no

And one sd-network function:

int sd_network_get_link_domains(int ifindex, char **domains);

And that should be it.

UseDomain= should have the effect of adding the domains from dhcp option
15 and 119 to the list of domains for the interface. And
sd_network_get_link_domains() should then return a single list, of
deduplicated entries, with the domains specified in Domains= first, and
then the dhcp domains possible added in at the end.

Zbigniew, I think this simplification would be beneficial, as I really
don't see the need to make the search vs. route domain thing
configurable....

Tom, what's your take on all of this?

The domain data from DHCP should be thoroughly verified btw. For
example, if is_hostname() is true for some domain acquired via DHCP we
should ignore it, and we should make sure that the domain name is
actually valid, and so on.

Lennart

[Tom Gundersen]

On Thu, Aug 14, 2014 at 1:11 PM, Lennart Poettering

Post by Lennart Poettering
UseDomain= should have the effect of adding the domains from dhcp option
15 and 119 to the list of domains for the interface. And
sd_network_get_link_domains() should then return a single list, of
deduplicated entries, with the domains specified in Domains= first, and
then the dhcp domains possible added in at the end.
Zbigniew, I think this simplification would be beneficial, as I really
don't see the need to make the search vs. route domain thing
configurable....
Tom, what's your take on all of this?

Sorry for taking forever to answer to this thread. I have been going
back and forth in my mind about how this should look.

I think in the end I essentially agree with Lennart's last suggestion.
Let's make this dead-simple and collapse the search/route domains for
each link into a single list. I think this is fine given that we
restrict the search behaviour to single-labels.

My only hesitation has been that I can imagine someone wanting to add
search domains that do not imply anything about routing. However, I
think in this case it does not make much sense to make this per-link,
but it should rather be a global SearchDomains= option (in
resolved.conf) or something to that effect.

Zbigniew, Michael, what do you think?

Cheers,

Tom

[Lennart Poettering]

Post by Tom Gundersen
On Thu, Aug 14, 2014 at 1:11 PM, Lennart Poettering

Post by Lennart Poettering
UseDomain= should have the effect of adding the domains from dhcp option
15 and 119 to the list of domains for the interface. And
sd_network_get_link_domains() should then return a single list, of
deduplicated entries, with the domains specified in Domains= first, and
then the dhcp domains possible added in at the end.
Zbigniew, I think this simplification would be beneficial, as I really
don't see the need to make the search vs. route domain thing
configurable....
Tom, what's your take on all of this?

Sorry for taking forever to answer to this thread. I have been going
back and forth in my mind about how this should look.
I think in the end I essentially agree with Lennart's last suggestion.
Let's make this dead-simple and collapse the search/route domains for
each link into a single list. I think this is fine given that we
restrict the search behaviour to single-labels.
My only hesitation has been that I can imagine someone wanting to add
search domains that do not imply anything about routing. However, I
think in this case it does not make much sense to make this per-link,
but it should rather be a global SearchDomains= option (in
resolved.conf) or something to that effect.
Zbigniew, Michael, what do you think?

Tom reminded me of the fact now, that at the systemd hackfest in Brno
last week (which really was more a "talkfest") people suggested we
should actually make it possible that if you go to lets say
"xhamster.com" you never ever want this to be resolved via the redhat
VPN. That probably makes a lot of sense.

Hence, I would suggest adding a syntax of:

[Network]
Domains=*

which would have the effect to route all DNS traffic that is not
explicitly routed somewhereelse to this interface.

Internally, this would just set a boolean, which could be queried with:

int sd_network_link_get_wildcard_domain(int ifindex);

or so, which would return 0 or 1 or negative -errno...

But then again, this doesn't have to be there from day one, we can add
that later... But of course, I'd love to see this done early on, too,
after all the porn usecase is a major one.

Lennart

[Tom Gundersen]

On Thu, Aug 14, 2014 at 1:47 PM, Lennart Poettering

Post by Lennart Poettering

Post by Tom Gundersen
On Thu, Aug 14, 2014 at 1:11 PM, Lennart Poettering

Post by Lennart Poettering
UseDomain= should have the effect of adding the domains from dhcp option
15 and 119 to the list of domains for the interface. And
sd_network_get_link_domains() should then return a single list, of
deduplicated entries, with the domains specified in Domains= first, and
then the dhcp domains possible added in at the end.
Zbigniew, I think this simplification would be beneficial, as I really
don't see the need to make the search vs. route domain thing
configurable....
Tom, what's your take on all of this?

Sorry for taking forever to answer to this thread. I have been going
back and forth in my mind about how this should look.
I think in the end I essentially agree with Lennart's last suggestion.
Let's make this dead-simple and collapse the search/route domains for
each link into a single list. I think this is fine given that we
restrict the search behaviour to single-labels.
My only hesitation has been that I can imagine someone wanting to add
search domains that do not imply anything about routing. However, I
think in this case it does not make much sense to make this per-link,
but it should rather be a global SearchDomains= option (in
resolved.conf) or something to that effect.
Zbigniew, Michael, what do you think?

Tom reminded me of the fact now, that at the systemd hackfest in Brno
last week (which really was more a "talkfest") people suggested we
should actually make it possible that if you go to lets say
"xhamster.com" you never ever want this to be resolved via the redhat
VPN. That probably makes a lot of sense.
[Network]
Domains=*
which would have the effect to route all DNS traffic that is not
explicitly routed somewhereelse to this interface.
int sd_network_link_get_wildcard_domain(int ifindex);
or so, which would return 0 or 1 or negative -errno...
But then again, this doesn't have to be there from day one, we can add
that later... But of course, I'd love to see this done early on, too,
after all the porn usecase is a major one.

As discussed off-list, I agree with adding this API / behaviour.

Cheers,

Tom

[Zbigniew Jędrzejewski-Szmek]

Post by Tom Gundersen
On Thu, Aug 14, 2014 at 1:11 PM, Lennart Poettering

Post by Lennart Poettering
UseDomain= should have the effect of adding the domains from dhcp option
15 and 119 to the list of domains for the interface. And
sd_network_get_link_domains() should then return a single list, of
deduplicated entries, with the domains specified in Domains= first, and
then the dhcp domains possible added in at the end.
Zbigniew, I think this simplification would be beneficial, as I really
don't see the need to make the search vs. route domain thing
configurable....
Tom, what's your take on all of this?

Sorry for taking forever to answer to this thread. I have been going
back and forth in my mind about how this should look.
I think in the end I essentially agree with Lennart's last suggestion.
Let's make this dead-simple and collapse the search/route domains for
each link into a single list. I think this is fine given that we
restrict the search behaviour to single-labels.

Sorry for taking forever to answer to this thread. I have been going
back and forth in my mind about how this should look. ;)

I now agree with what Lennart proposed too. This is partially implemented
now, and with UseDomains=yes, option 15 is used to to set 'search' field.

I think we should go a step further, and set UseDomains=yes by default,
to have 'search' populated in /etc/resolv.conf. I think the security
reservations are overstated:
iiuc, the concern was that multi-level domain names (i.e. those with at least
one dot) could be spoofed by controlling the search suffix. But for
names with at least two levels glibc only uses the search list as a fallback.
So to mount a successful spoof, the attacker needs to
a) control the dhcp server domain option to return a domain under attacker control
b) arrange for the user to resolve an invalid domain name

Considering that the attack can only work for domain names which would
not resolve otherwise, and requires either a misconfigured dhcp server
or control of some dns server, this doesn't seem very serious. After all,
there are more direct avenues of attack if one controls dhcp or dns traffic.

(Above was about traditional libc resolver. For systemd-resolved clients
I don't think we should ever suffix non-single-level domain names with anything.)

The story is sligthly different for single-level names. By setting UseDomains=yes
we allow the dhcp server some control over the resolution of those names.
But that seems natural too. If we want to allow LLMR or avahi, allowing
the dhcp server to also control local name resolution seems a natural extension.

Any reservations for making UseDomains=yes the default?

https://bugs.freedesktop.org/show_bug.cgi?id=85397

Post by Tom Gundersen
My only hesitation has been that I can imagine someone wanting to add
search domains that do not imply anything about routing. However, I
think in this case it does not make much sense to make this per-link,
but it should rather be a global SearchDomains= option (in
resolved.conf) or something to that effect.
Zbigniew, Michael, what do you think?
Cheers,
Tom

Zbyszek

[Lennart Poettering]

Post by Zbigniew Jędrzejewski-Szmek
I now agree with what Lennart proposed too. This is partially implemented
now, and with UseDomains=yes, option 15 is used to to set 'search' field.
I think we should go a step further, and set UseDomains=yes by default,
to have 'search' populated in /etc/resolv.conf. I think the security
iiuc, the concern was that multi-level domain names (i.e. those with at least
one dot) could be spoofed by controlling the search suffix. But for
names with at least two levels glibc only uses the search list as a fallback.

Well, sure, being able to influence things at the beginning of the
search logic is more problematic than influencing things at the end of
the search logic, but i still think it's problematic, since it still
allows you to insert "home.foobar.com" into a domain "foobar.com" that
doesn't have "home.foobar.com" itself but only "www.bar.com"...

Sure, classic (non-DNSSEC) DNS is not ever going to be fully secure,
but it I still believe we should default to the safer options, and
allow the others.

Altering the search paths is inherently something that makes no sense
on public networks, it only makes sense if you know your network well,
and trust it to some level. Hence opt-in sounds like the better option
to me.

Post by Zbigniew Jędrzejewski-Szmek
The story is sligthly different for single-level names. By setting UseDomains=yes
we allow the dhcp server some control over the resolution of those names.
But that seems natural too. If we want to allow LLMR or avahi, allowing
the dhcp server to also control local name resolution seems a natural extension.
Any reservations for making UseDomains=yes the default?

I'd really prefer if this stays opt-in. That said, I think it would be
a really good idea to improve the documentation of DHCP= to suggest
people to set UseDomains=yes if they need it.

Lennart

[Zbigniew Jędrzejewski-Szmek]

Post by Lennart Poettering

Post by Zbigniew Jędrzejewski-Szmek
I now agree with what Lennart proposed too. This is partially implemented
now, and with UseDomains=yes, option 15 is used to to set 'search' field.
I think we should go a step further, and set UseDomains=yes by default,
to have 'search' populated in /etc/resolv.conf. I think the security
iiuc, the concern was that multi-level domain names (i.e. those with at least
one dot) could be spoofed by controlling the search suffix. But for
names with at least two levels glibc only uses the search list as a fallback.

Well, sure, being able to influence things at the beginning of the
search logic is more problematic than influencing things at the end of
the search logic, but i still think it's problematic, since it still
allows you to insert "home.foobar.com" into a domain "foobar.com" that
doesn't have "home.foobar.com" itself but only "www.bar.com"...
Sure, classic (non-DNSSEC) DNS is not ever going to be fully secure,
but it I still believe we should default to the safer options, and
allow the others.
Altering the search paths is inherently something that makes no sense
on public networks, it only makes sense if you know your network well,
and trust it to some level. Hence opt-in sounds like the better option
to me.

Ok, this makes a lot of sense... but currently this policy is not very
consistent. UseDNS defaults to true, which gives control of *all* names
to the dhcp server, not just over the prefix. When we have DNSSEC by default,
this will be different though, so that's fine I guess.

OTOH, I really don't see why UseHostname default to true. Why trust the
server in this regard?

Post by Lennart Poettering

Post by Zbigniew Jędrzejewski-Szmek
The story is sligthly different for single-level names. By setting UseDomains=yes
we allow the dhcp server some control over the resolution of those names.
But that seems natural too. If we want to allow LLMR or avahi, allowing
the dhcp server to also control local name resolution seems a natural extension.
Any reservations for making UseDomains=yes the default?

I'd really prefer if this stays opt-in. That said, I think it would be
a really good idea to improve the documentation of DHCP= to suggest
people to set UseDomains=yes if they need it.

I now pushed a commit which does that.

Zbyszek

[Lennart Poettering]

Post by Zbigniew Jędrzejewski-Szmek

Post by Lennart Poettering

Post by Zbigniew Jędrzejewski-Szmek
I now agree with what Lennart proposed too. This is partially implemented
now, and with UseDomains=yes, option 15 is used to to set 'search' field.
I think we should go a step further, and set UseDomains=yes by default,
to have 'search' populated in /etc/resolv.conf. I think the security
iiuc, the concern was that multi-level domain names (i.e. those with at least
one dot) could be spoofed by controlling the search suffix. But for
names with at least two levels glibc only uses the search list as a fallback.

Well, sure, being able to influence things at the beginning of the
search logic is more problematic than influencing things at the end of
the search logic, but i still think it's problematic, since it still
allows you to insert "home.foobar.com" into a domain "foobar.com" that
doesn't have "home.foobar.com" itself but only "www.bar.com"...
Sure, classic (non-DNSSEC) DNS is not ever going to be fully secure,
but it I still believe we should default to the safer options, and
allow the others.
Altering the search paths is inherently something that makes no sense
on public networks, it only makes sense if you know your network well,
and trust it to some level. Hence opt-in sounds like the better option
to me.

Ok, this makes a lot of sense... but currently this policy is not very
consistent. UseDNS defaults to true, which gives control of *all* names
to the dhcp server, not just over the prefix. When we have DNSSEC by default,
this will be different though, so that's fine I guess.
OTOH, I really don't see why UseHostname default to true. Why trust the
server in this regard?

Well, we use it to initialize the "transient" hostname managed by
hostnamed with. And the "transient" hostname is irrelevant if there's
a static one defined, since the static hostname overrides the
transient one.

or in other words: usehostname has only very weak effects: it
propagates to the kernel hostname only if you have no manual
configuration...

Lennart

[Tom Gundersen]

It seems this thread was already resolved, just wanted to add one comment:

On Mon, May 18, 2015 at 2:26 PM, Zbigniew Jędrzejewski-Szmek

Post by Zbigniew Jędrzejewski-Szmek
So to mount a successful spoof, the attacker needs to
a) control the dhcp server domain option to return a domain under attacker control

Notice that this is entirely trivial, as the attacker does not need to
control _the_ DHCP server, all they have to do is to provide a
competing DHCP server on the local link and hope they get picked over
the real one.

Cheers,

Tom

[Lennart Poettering]

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;

char p[sizeof("/run/systemd/netif/links/") + DECIMAL_STRING_MAX(int)];
snprintf(p, sizeof(p), "/run/systemd/netif/links/%d",
ifindex);

Actually, I'd even use normal sprintf() here, not snprintf(), after all,
we carefully sized the string anyway, hence no need to enforce the size
limit here. In particular since snprintf() doesn't add a trailing NULL
if it truncates the string, which makes the whole excercise kinda
pointless... an snprintf() without something like char_array_0() invoked
right after it always raises my eyebrows. Using sprintf() here makes is
clear to me that the buffer was carefully sized before, so I think would
be preferrable...

Lennart

[Zbigniew Jędrzejewski-Szmek]

Post by Lennart Poettering

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;

char p[sizeof("/run/systemd/netif/links/") + DECIMAL_STRING_MAX(int)];
snprintf(p, sizeof(p), "/run/systemd/netif/links/%d",
ifindex);

Actually, I'd even use normal sprintf() here, not snprintf(), after all,
we carefully sized the string anyway, hence no need to enforce the size
limit here. Using sprintf() here makes is
clear to me that the buffer was carefully sized before, so I think would
be preferrable...

On my TODO list is adding snprintf_check (or snprintf_assert, or snprintf_sized,
not sure about the name), which would wrap the snprintf in an assert
to check that there was no truncation.

Post by Lennart Poettering
In particular since snprintf() doesn't add a trailing NULL
if it truncates the string, which makes the whole excercise kinda
pointless... an snprintf() without something like char_array_0() invoked
right after it always raises my eyebrows.

Are you sure?

snprintf(3) implies that the terminating byte is always written. Testing
confirms that fwiw.

Zbyszek

[Lennart Poettering]

Post by Zbigniew Jędrzejewski-Szmek

Post by Lennart Poettering

Post by Zbigniew Jędrzejewski-Szmek

Post by Michael Marineau
+_public_ int sd_network_get_domainname(int ifindex, char **domainname) {
+ _cleanup_free_ char *s = NULL, *p = NULL;
+ int r;
+
+ assert_return(ifindex > 0, -EINVAL);
+ assert_return(domainname, -EINVAL);
+
+ if (asprintf(&p, "/run/systemd/netif/links/%d", ifindex) < 0)
+ return -ENOMEM;

char p[sizeof("/run/systemd/netif/links/") + DECIMAL_STRING_MAX(int)];
snprintf(p, sizeof(p), "/run/systemd/netif/links/%d",
ifindex);

Actually, I'd even use normal sprintf() here, not snprintf(), after all,
we carefully sized the string anyway, hence no need to enforce the size
limit here. Using sprintf() here makes is
clear to me that the buffer was carefully sized before, so I think would
be preferrable...

On my TODO list is adding snprintf_check (or snprintf_assert, or snprintf_sized,
not sure about the name), which would wrap the snprintf in an assert
to check that there was no truncation.

Post by Lennart Poettering
In particular since snprintf() doesn't add a trailing NULL
if it truncates the string, which makes the whole excercise kinda
pointless... an snprintf() without something like char_array_0() invoked
right after it always raises my eyebrows.

Are you sure?
snprintf(3) implies that the terminating byte is always written. Testing
confirms that fwiw.

Hmm, indeed. Apparently glibc always does that. Old solaris and windows
don't however, but we don't care about that...

So I figure we can drop char_array_0() invocations at most places,
possibly even get rid of the macro definition entirely...

Lennart

[Lennart Poettering]

Post by Michael Marineau
When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.

Hmm, we really should figure out how we want to support all of this in
the long run, between networkd and resolved.

Quite frankly I believe the entire "domain" and "search list" logic is
idiotic. It's an invitation for a security breach without bounds. In a
world where DNSSEC is supposed to validate full hostnames, and what they
refer to it's simply wrong to mangle user input like that.

But then again, I figure we cannot just not support it. Administrators
traditionally used that and most would probably defend it as the most
useful thing on earth. But I guess we can at least try to make this less
broken...

For example, I don't really understand where the effective difference
between resolv.conf's "domain" and "search" setting is supposed to be
for resolvers. Why would anyone configure "domain", if he could just
as well configure "search"?

resolved's DNS resolver is actually aware of multiple interfaces and
their specific DNS servers (at least when used in conjuncion with
networkd). It will issue DNS requests in parallel on all interfaces, in
order to deal with the VPN vs. local LAN problem where a company VPN and
the local LAN might both define local, private names, and we should be
able to resolve them both at the same time. For this kind of setup it is
sometimes useful however to bind a specific domain exlcusively to one
interface though, i.e. to disable the logic of simulatenously asking all
interfaces after all. For example, declaring that *.redhat.com should
strictly go into the VPN is a good thing, in order not leak information
about redhat-internal hosts onto public DNS servers... Now, for this
kind of stuff we should allow defining a list of exclusive domains per
interface. This is different from a search list however, as this is
simply about where to route DNS requests to, and not about appending
suffixes.

I think if we apply search lists then we should probably restrict this
to single-label names, for security reasons. That way it will only
compete against LLMNR (since llmnr is used exclusively for single-label
names, too), but never be applied to fqdns. Given that one can trust
LLMNR and DHCP pretty much to the same degree that should be OK.

So, maybe we should have just two options:

[Network]
Domains=list of domains

[DHCP]
UseDomains=yes/no (default: no)

Domains= configures a list of domains specific to the interface. This
would be used for DNS routing by resolved, as pointed out above. It
would also be used as search list for single-label names.

And UseDomains= in the [DHCP] section would have the result of adding a
search list supplied via DHCP (either option 119 or 15) to the manually
configured search list (at the end). It would be off by default, for
security reasons.

Does this make any sense? Opinions?

Note that this would be different from glibc's resolver, where the
search list is applied to *all* domains names, regardless if they are
single-label or not. Also, it will be different from existing DHCP
clients, as they all tend to apply the search list supplied from DHCP
without any restrictions.

Lennart

[Mantas Mikulėnas]

Post by Lennart Poettering

Post by Michael Marineau
When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.

Hmm, we really should figure out how we want to support all of this in
the long run, between networkd and resolved.
Quite frankly I believe the entire "domain" and "search list" logic is
idiotic. It's an invitation for a security breach without bounds. In a
world where DNSSEC is supposed to validate full hostnames, and what they
refer to it's simply wrong to mangle user input like that.
But then again, I figure we cannot just not support it. Administrators
traditionally used that and most would probably defend it as the most
useful thing on earth. But I guess we can at least try to make this less
broken...

On the one hand, it certainly saves a lot of time typing, and I don't see
how it affects DNSSEC given that the validator always sees full names.

On the other hand, it *does* break TLS certificate validation, as well as
Kerberos authentication, both of which want an exact FQDN match.

So 'ping' and 'mtr' is probably 99% of my use of the search list; full
domain name everywhere else. So I probably wouldn't miss the search list
much, myself.

(glibc has a nice alternative, $HOSTALIASES, allowing each user to
configure alias › domain name mappings. Interestingly, setuid programs
ignore that completely for security reasons, so it's unusable with
ping/mtr/traceroute, precisely where I'd want it most.)

Post by Lennart Poettering
For example, I don't really understand where the effective difference
between resolv.conf's "domain" and "search" setting is supposed to be
for resolvers. Why would anyone configure "domain", if he could just
as well configure "search"?

If I remember correctly from the resolv.conf manpage or somewhere such,
there's *no* difference other than 'domain' being limited to one suffix.

Post by Lennart Poettering
resolved's DNS resolver is actually aware of multiple interfaces and
their specific DNS servers (at least when used in conjuncion with
networkd). It will issue DNS requests in parallel on all interfaces, in
order to deal with the VPN vs. local LAN problem where a company VPN and
the local LAN might both define local, private names, and we should be
able to resolve them both at the same time. For this kind of setup it is
sometimes useful however to bind a specific domain exlcusively to one
interface though, i.e. to disable the logic of simulatenously asking all
interfaces after all. For example, declaring that *.redhat.com should
strictly go into the VPN is a good thing, in order not leak information
about redhat-internal hosts onto public DNS servers... Now, for this
kind of stuff we should allow defining a list of exclusive domains per
interface. This is different from a search list however, as this is
simply about where to route DNS requests to, and not about appending
suffixes.

In other words, the Linux resolver is finally as smart as Windows has been
for a decade :D

Post by Lennart Poettering
I think if we apply search lists then we should probably restrict this
to single-label names, for security reasons. That way it will only
compete against LLMNR (since llmnr is used exclusively for single-label
names, too), but never be applied to fqdns. Given that one can trust
LLMNR and DHCP pretty much to the same degree that should be OK.

Well, I imagine admins *do* filter DHCP packets from untrusted devices.
I've even seen switches capable of this.

Post by Lennart Poettering
[Network]
Domains=list of domains
[DHCP]
UseDomains=yes/no (default: no)
Domains= configures a list of domains specific to the interface. This
would be used for DNS routing by resolved, as pointed out above. It
would also be used as search list for single-label names.
And UseDomains= in the [DHCP] section would have the result of adding a
search list supplied via DHCP (either option 119 or 15) to the manually
configured search list (at the end). It would be off by default, for
security reasons.
Does this make any sense? Opinions?
Note that this would be different from glibc's resolver, where the
search list is applied to *all* domains names, regardless if they are
single-label or not.

The one label restriction sounds like an improvement, though I wonder if
someone actually relies on the old behavior.

I always disliked how Windows would first attempt resolving
google.com.example.com. and only then google.com. unless I manually added
"." at the top of the list...

[Zbigniew Jędrzejewski-Szmek]

Post by Lennart Poettering

Post by Michael Marineau
When the code for generating resolv.conf was moved from networkd to
resolved the DHCP domain name code was dropped.

Hmm, we really should figure out how we want to support all of this in
the long run, between networkd and resolved.
Quite frankly I believe the entire "domain" and "search list" logic is
idiotic. It's an invitation for a security breach without bounds. In a
world where DNSSEC is supposed to validate full hostnames, and what they
refer to it's simply wrong to mangle user input like that.
But then again, I figure we cannot just not support it. Administrators
traditionally used that and most would probably defend it as the most
useful thing on earth. But I guess we can at least try to make this less
broken...
For example, I don't really understand where the effective difference
between resolv.conf's "domain" and "search" setting is supposed to be
for resolvers. Why would anyone configure "domain", if he could just
as well configure "search"?
resolved's DNS resolver is actually aware of multiple interfaces and
their specific DNS servers (at least when used in conjuncion with
networkd). It will issue DNS requests in parallel on all interfaces, in
order to deal with the VPN vs. local LAN problem where a company VPN and
the local LAN might both define local, private names, and we should be
able to resolve them both at the same time. For this kind of setup it is
sometimes useful however to bind a specific domain exlcusively to one
interface though, i.e. to disable the logic of simulatenously asking all
interfaces after all. For example, declaring that *.redhat.com should
strictly go into the VPN is a good thing, in order not leak information
about redhat-internal hosts onto public DNS servers... Now, for this
kind of stuff we should allow defining a list of exclusive domains per
interface. This is different from a search list however, as this is
simply about where to route DNS requests to, and not about appending
suffixes.
I think if we apply search lists then we should probably restrict this
to single-label names, for security reasons. That way it will only
compete against LLMNR (since llmnr is used exclusively for single-label
names, too), but never be applied to fqdns. Given that one can trust
LLMNR and DHCP pretty much to the same degree that should be OK.
[Network]
Domains=list of domains
[DHCP]
UseDomains=yes/no (default: no)
Domains= configures a list of domains specific to the interface. This
would be used for DNS routing by resolved, as pointed out above. It
would also be used as search list for single-label names.
And UseDomains= in the [DHCP] section would have the result of adding a
search list supplied via DHCP (either option 119 or 15) to the manually
configured search list (at the end). It would be off by default, for
security reasons.
Does this make any sense? Opinions?

Yes, totally makes sense. But the name UseDomains is confusing though.
IIUC, we have two separate concepts:
1. using a specific interface (and a set of DNS resolvers tied to it)
when resolving specific fqdns (resolve list)
2. using specifc fqdns when a single-label name is given (search list)

Your description sounds like DHCP.UseDomains=yes would mean using
the DHCP-supplied list for 2. I think it should be used for 1 too.
So maybe there should be

UseDomains=resolve|search|all

(all in case we add futher options later on).

Post by Lennart Poettering
Note that this would be different from glibc's resolver, where the
search list is applied to *all* domains names, regardless if they are
single-label or not. Also, it will be different from existing DHCP
clients, as they all tend to apply the search list supplied from DHCP
without any restrictions.

Zbyszek

[Lennart Poettering]

Post by Zbigniew Jędrzejewski-Szmek

Post by Lennart Poettering
Does this make any sense? Opinions?

Yes, totally makes sense. But the name UseDomains is confusing though.
1. using a specific interface (and a set of DNS resolvers tied to it)
when resolving specific fqdns (resolve list)
2. using specifc fqdns when a single-label name is given (search list)
Your description sounds like DHCP.UseDomains=yes would mean using
the DHCP-supplied list for 2. I think it should be used for 1 too.
So maybe there should be
UseDomains=resolve|search|all
(all in case we add futher options later on).

Makes sense, I agree.

Tom, can we get the domain list as an API into sd-network?

Lennart

[Lennart Poettering]

Post by Lennart Poettering

Post by Zbigniew Jędrzejewski-Szmek

Post by Lennart Poettering
Does this make any sense? Opinions?

Yes, totally makes sense. But the name UseDomains is confusing though.
1. using a specific interface (and a set of DNS resolvers tied to it)
when resolving specific fqdns (resolve list)
2. using specifc fqdns when a single-label name is given (search list)
Your description sounds like DHCP.UseDomains=yes would mean using
the DHCP-supplied list for 2. I think it should be used for 1 too.
So maybe there should be
UseDomains=resolve|search|all
(all in case we add futher options later on).

Makes sense, I agree.

Hmm, actually thinking about this, I think this would end up being quite
complex, since networkd would then ultimately have to pass four bits
information to resolved, in individual fields: the manually
configured per-interface domain list + its resolve/search/all
settings + the dhcp supplied domain list + its resolve/search/all. I'd
really prefer if we simplify that, so that sd-network doesn't have to
have any dhcp-specific APIs, but instead would always combine the dhcp
data with the statically configured data and pass it as one to
resolved...

Lennart