diff mbox series

[Openvpn-devel,L] Change in openvpn[master]: dns: don't publish env vars to non-dns scripts

Message ID ecaea417456daae56787b0bceebd0a95b95f5dba-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,L] Change in openvpn[master]: dns: don't publish env vars to non-dns scripts | expand

Commit Message

ralf_lici (Code Review) Dec. 12, 2024, 7:47 a.m. UTC 
Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/840?usp=email

to review the following change.


Change subject: dns: don't publish env vars to non-dns scripts
......................................................................

dns: don't publish env vars to non-dns scripts

With --dns-script in place we no longer need DNS related vars in the
environment for other script hooks. Code for doing that is removed and
the function to set --dns stuff made static, for internal use only.

Another thing: since --dns setting overrule DNS related --dhcp-options,
remove the latter when we got some via --dns.

Change-Id: I3fb01ab76cf3df0874ba92e08f371d17607a8369
Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
---
M src/openvpn/dns.c
M src/openvpn/dns.h
M src/openvpn/options.c
3 files changed, 110 insertions(+), 257 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/40/840/1
diff mbox series

Patch

diff --git a/src/openvpn/dns.c b/src/openvpn/dns.c
index 14c1270..10f9bdc 100644
--- a/src/openvpn/dns.c
+++ b/src/openvpn/dns.c
@@ -349,93 +349,6 @@ 
     }
 }
 
-static void
-setenv_dns_option(struct env_set *es,
-                  const char *format, int i, int j,
-                  const char *value)
-{
-    char name[64];
-    bool name_ok = false;
-
-    if (j < 0)
-    {
-        name_ok = snprintf(name, sizeof(name), format, i);
-    }
-    else
-    {
-        name_ok = snprintf(name, sizeof(name), format, i, j);
-    }
-
-    if (!name_ok)
-    {
-        msg(M_WARN, "WARNING: dns option setenv name buffer overflow");
-    }
-
-    setenv_str(es, name, value);
-}
-
-void
-setenv_dns_options(const struct dns_options *o, struct env_set *es)
-{
-    struct gc_arena gc = gc_new();
-    const struct dns_server *s;
-    const struct dns_domain *d;
-    int i, j;
-
-    for (i = 1, d = o->search_domains; d != NULL; i++, d = d->next)
-    {
-        setenv_dns_option(es, "dns_search_domain_%d", i, -1, d->name);
-    }
-
-    for (i = 1, s = o->servers; s != NULL; i++, s = s->next)
-    {
-        for (j = 0; j < s->addr_count; ++j)
-        {
-            if (s->addr[j].family == AF_INET)
-            {
-                setenv_dns_option(es, "dns_server_%d_address_%d", i, j + 1,
-                                  print_in_addr_t(s->addr[j].in.a4.s_addr, IA_NET_ORDER, &gc));
-            }
-            else
-            {
-                setenv_dns_option(es, "dns_server_%d_address_%d", i, j + 1,
-                                  print_in6_addr(s->addr[j].in.a6, 0, &gc));
-            }
-            if (s->addr[j].port)
-            {
-                setenv_dns_option(es, "dns_server_%d_port_%d", i, j + 1,
-                                  print_in_port_t(s->addr[j].port, &gc));
-            }
-        }
-
-        if (s->domains)
-        {
-            for (j = 1, d = s->domains; d != NULL; j++, d = d->next)
-            {
-                setenv_dns_option(es, "dns_server_%d_resolve_domain_%d", i, j, d->name);
-            }
-        }
-
-        if (s->dnssec)
-        {
-            setenv_dns_option(es, "dns_server_%d_dnssec", i, -1,
-                              dnssec_value(s->dnssec));
-        }
-
-        if (s->transport)
-        {
-            setenv_dns_option(es, "dns_server_%d_transport", i, -1,
-                              transport_value(s->transport));
-        }
-        if (s->sni)
-        {
-            setenv_dns_option(es, "dns_server_%d_sni", i, -1, s->sni);
-        }
-    }
-
-    gc_free(&gc);
-}
-
 #ifdef _WIN32
 
 static void
@@ -524,6 +437,93 @@ 
 #else /* ifdef _WIN32 */
 
 static void
+setenv_dns_option(struct env_set *es,
+                  const char *format, int i, int j,
+                  const char *value)
+{
+    char name[64];
+    bool name_ok = false;
+
+    if (j < 0)
+    {
+        name_ok = snprintf(name, sizeof(name), format, i);
+    }
+    else
+    {
+        name_ok = snprintf(name, sizeof(name), format, i, j);
+    }
+
+    if (!name_ok)
+    {
+        msg(M_WARN, "WARNING: dns option setenv name buffer overflow");
+    }
+
+    setenv_str(es, name, value);
+}
+
+static void
+setenv_dns_options(const struct dns_options *o, struct env_set *es)
+{
+    struct gc_arena gc = gc_new();
+    const struct dns_server *s;
+    const struct dns_domain *d;
+    int i, j;
+
+    for (i = 1, d = o->search_domains; d != NULL; i++, d = d->next)
+    {
+        setenv_dns_option(es, "dns_search_domain_%d", i, -1, d->name);
+    }
+
+    for (i = 1, s = o->servers; s != NULL; i++, s = s->next)
+    {
+        for (j = 0; j < s->addr_count; ++j)
+        {
+            if (s->addr[j].family == AF_INET)
+            {
+                setenv_dns_option(es, "dns_server_%d_address_%d", i, j + 1,
+                                  print_in_addr_t(s->addr[j].in.a4.s_addr, IA_NET_ORDER, &gc));
+            }
+            else
+            {
+                setenv_dns_option(es, "dns_server_%d_address_%d", i, j + 1,
+                                  print_in6_addr(s->addr[j].in.a6, 0, &gc));
+            }
+            if (s->addr[j].port)
+            {
+                setenv_dns_option(es, "dns_server_%d_port_%d", i, j + 1,
+                                  print_in_port_t(s->addr[j].port, &gc));
+            }
+        }
+
+        if (s->domains)
+        {
+            for (j = 1, d = s->domains; d != NULL; j++, d = d->next)
+            {
+                setenv_dns_option(es, "dns_server_%d_resolve_domain_%d", i, j, d->name);
+            }
+        }
+
+        if (s->dnssec)
+        {
+            setenv_dns_option(es, "dns_server_%d_dnssec", i, -1,
+                              dnssec_value(s->dnssec));
+        }
+
+        if (s->transport)
+        {
+            setenv_dns_option(es, "dns_server_%d_transport", i, -1,
+                              transport_value(s->transport));
+        }
+        if (s->sni)
+        {
+            setenv_dns_option(es, "dns_server_%d_sni", i, -1, s->sni);
+        }
+    }
+
+    gc_free(&gc);
+}
+
+static void
 script_env_set(bool up, const struct dns_options *o, const struct tuntap *tt, struct env_set *es)
 {
     setenv_str(es, "dev", tt->actual_name);
diff --git a/src/openvpn/dns.h b/src/openvpn/dns.h
index 47f7e5d..bdf49fd 100644
--- a/src/openvpn/dns.h
+++ b/src/openvpn/dns.h
@@ -167,14 +167,6 @@ 
                      struct dns_script_runner_info *dsri);
 
 /**
- * Puts the DNS options into an environment set.
- *
- * @param   o           Pointer to the DNS options to set
- * @param   es          Pointer to the env_set to set the options into
- */
-void setenv_dns_options(const struct dns_options *o, struct env_set *es);
-
-/**
  * Prints configured DNS options.
  *
  * @param   o           Pointer to the DNS options to print
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 319f370..27a2476 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1029,11 +1029,6 @@ 
     {
         setenv_connection_entry(es, &o->ce, 1);
     }
-
-    if (!o->pull)
-    {
-        setenv_dns_options(&o->dns_options, es);
-    }
 }
 
 #ifndef _WIN32
@@ -1347,149 +1342,6 @@ 
         }
     }
 }
-
-/*
- * If DNS options are set use these for TUN/TAP options as well.
- * Applies to DNS, DNS6 and DOMAIN-SEARCH.
- * Existing options will be discarded.
- */
-static void
-tuntap_options_copy_dns(struct options *o)
-{
-    struct tuntap_options *tt = &o->tuntap_options;
-    struct dns_options *dns = &o->dns_options;
-
-    if (dns->search_domains)
-    {
-        tt->domain_search_list_len = 0;
-        const struct dns_domain *domain = dns->search_domains;
-        while (domain && tt->domain_search_list_len < N_SEARCH_LIST_LEN)
-        {
-            tt->domain_search_list[tt->domain_search_list_len++] = domain->name;
-            domain = domain->next;
-        }
-        if (domain)
-        {
-            msg(M_WARN, "WARNING: couldn't copy all --dns search-domains to --dhcp-option");
-        }
-        tt->dhcp_options |= DHCP_OPTIONS_DHCP_REQUIRED;
-    }
-
-    if (dns->servers)
-    {
-        tt->dns_len = 0;
-        tt->dns6_len = 0;
-        bool overflow = false;
-        const struct dns_server *server = dns->servers;
-        while (server)
-        {
-            for (int i = 0; i < server->addr_count; ++i)
-            {
-                if (server->addr[i].family == AF_INET)
-                {
-                    if (tt->dns_len >= N_DHCP_ADDR)
-                    {
-                        overflow = true;
-                        continue;
-                    }
-                    tt->dns[tt->dns_len++] = server->addr[i].in.a4.s_addr;
-                }
-                else
-                {
-                    if (tt->dns6_len >= N_DHCP_ADDR)
-                    {
-                        overflow = true;
-                        continue;
-                    }
-                    tt->dns6[tt->dns6_len++] = server->addr[i].in.a6;
-                }
-            }
-            server = server->next;
-        }
-        if (overflow)
-        {
-            msg(M_WARN, "WARNING: couldn't copy all --dns server addresses to --dhcp-option");
-        }
-        tt->dhcp_options |= DHCP_OPTIONS_DHCP_OPTIONAL;
-    }
-}
-#else /* if defined(_WIN32) || defined(TARGET_ANDROID) */
-static void
-foreign_options_copy_dns(struct options *o, struct env_set *es)
-{
-    const struct dns_domain *domain = o->dns_options.search_domains;
-    const struct dns_server *server = o->dns_options.servers;
-    if (!domain && !server)
-    {
-        return;
-    }
-
-    /* reset the index since we're starting all over again */
-    int opt_max = o->foreign_option_index;
-    o->foreign_option_index = 0;
-
-    for (int i = 1; i <= opt_max; ++i)
-    {
-        char name[32];
-        snprintf(name, sizeof(name), "foreign_option_%d", i);
-
-        const char *env_str = env_set_get(es, name);
-        const char *value = strchr(env_str, '=') + 1;
-        if ((domain && strstr(value, "dhcp-option DOMAIN-SEARCH") == value)
-            || (server && strstr(value, "dhcp-option DNS") == value))
-        {
-            setenv_del(es, name);
-        }
-        else
-        {
-            setenv_foreign_option(o, &value, 1, es);
-        }
-    }
-
-    struct gc_arena gc = gc_new();
-
-    while (server)
-    {
-        for (int i = 0; i < server->addr_count; ++i)
-        {
-            if (server->addr[i].family == AF_INET)
-            {
-                const char *argv[] = {
-                    "dhcp-option",
-                    "DNS",
-                    print_in_addr_t(server->addr[i].in.a4.s_addr, 0, &gc)
-                };
-                setenv_foreign_option(o, argv, 3, es);
-            }
-            else
-            {
-                const char *argv[] = {
-                    "dhcp-option",
-                    "DNS6",
-                    print_in6_addr(server->addr[i].in.a6, 0, &gc)
-                };
-                setenv_foreign_option(o, argv, 3, es);
-            }
-        }
-        server = server->next;
-    }
-    while (domain)
-    {
-        const char *argv[] = { "dhcp-option", "DOMAIN-SEARCH", domain->name };
-        setenv_foreign_option(o, argv, 3, es);
-        domain = domain->next;
-    }
-
-    gc_free(&gc);
-
-    /* remove old leftover entries */
-    while (o->foreign_option_index < opt_max)
-    {
-        char name[32];
-        snprintf(name, sizeof(name), "foreign_option_%d", opt_max--);
-        setenv_del(es, name);
-    }
-}
 #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */
 
 #ifndef ENABLE_SMALL
@@ -3829,14 +3681,6 @@ 
     {
         dns_options_preprocess_pull(&o->dns_options);
     }
-    else
-    {
-#if defined(_WIN32) || defined(TARGET_ANDROID)
-        tuntap_options_copy_dns(o);
-#else
-        foreign_options_copy_dns(o, es);
-#endif
-    }
     if (o->auth_token_generate && !o->auth_token_renewal)
     {
         o->auth_token_renewal = o->renegotiate_seconds;
@@ -4207,7 +4051,6 @@ 
 
 /*
  * Sanity check on options after more options were pulled from server.
- * Also time to modify some options based on other options.
  */
 bool
 options_postprocess_pull(struct options *o, struct env_set *es)
@@ -4216,12 +4059,30 @@ 
     if (success)
     {
         dns_options_postprocess_pull(&o->dns_options);
-        setenv_dns_options(&o->dns_options, es);
+
 #if defined(_WIN32) || defined(TARGET_ANDROID)
-        tuntap_options_copy_dns(o);
-#else
-        foreign_options_copy_dns(o, es);
-#endif
+        /* If there's --dns servers, remove dns related --dhcp-options */
+        if (o->dns_options.servers)
+        {
+            o->tuntap_options.dns_len = 0;
+            o->tuntap_options.dns6_len = 0;
+            o->tuntap_options.domain = NULL;
+            o->tuntap_options.domain_search_list_len = 0;
+        }
+        /* Override search domains with the ones from --dns */
+        else if (o->dns_options.search_domains)
+        {
+            int i = 0;
+            struct dns_domain *domain = o->dns_options.search_domains;
+            while (i < N_SEARCH_LIST_LEN && domain)
+            {
+                o->tuntap_options.domain_search_list[i] = domain->name;
+                domain = domain->next;
+                ++i;
+            }
+            o->tuntap_options.domain_search_list_len = i;
+        }
+#endif /* defined(_WIN32) || defined(TARGET_ANDROID) */
     }
     return success;
 }