Hi Antonio,
Thanks for the feedback.
[Re: [PATCH] vpnc-script: add dnsmasq split-horizon DNS support] On 14.06.29 (Sun 13:04) Antonio Borneo wrote:
> Hi Joe,
>
> On Tue, Jun 24, 2014 at 10:56 PM, Joe MacDonald
> <joe_macdonald at mentor.com> wrote:
> > Attempt to determine if dnsmasq is being used to manage name resolution
> > and, if so, update the configuration with VPN-supplied DNS information.
> >
> > Signed-off-by: Joe MacDonald <joe_macdonald at mentor.com>
> > ---
> >
> > Adding any level of dnsmasq interaction makes it pretty easy to set up
> > best-effort split-horizon DNS. This does depend on having resolvconf
> > managing things as well, but the concept should apply equally well to a
> > non-resolvconf scenario. It just seemed easiest and least invasive to
> > create / remove the dnsmasq.d configuration piece in the
> > *_resolvconf_manager() functions. I try to err on the side of asking the
> > VPN DNS more often rather than less but still use the
> > previously-configured resolver for what looks like non-VPN traffic.
>
> I did not tested your patch since I'm not using dnsmasq
>
> I suppose this will work when VPN sets a split network and dnsmasq can
> still access DNS, inside and outside VPN channel.
> What would happen if VPN setting forces all the traffic in the VPN
> tunnel? Do you need to remove previous configuration from dnsmasq?
Interesting question. It sent me off to do more reading because I
thought I knew the answer when I wrote the patch but I began to doubt
myself.
dnsmasq, by default, round-robins between available servers. Using the
server= directive the way I have below limits that somewhat, such that
requests for those domains are never sent to any other DNS server,
regardless of the round-robin policy. All other queries are sent
wherever happens to be convenient. So they may go to the VPN server(s)
or to the external one(s).
If the VPN traps all traffic in the tunnel, this should still work,
though the first lookup may take longer since dnsmasq will need to
determine that the non-VPN server is unreachable. Of course that's only
true if your other server was local to your LAN and not one that would
be reachable anyway, like the Google ones.
Ultimately, I'm just going to do some testing and see what the behaviour
is before sending out a v2 of the patch, but I've not been able to come
up with a scenario in the different types of VPNs I've worked on in the
past where this wouldn't continue to behave as expected.
> > vpnc-script | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 47 insertions(+), 7 deletions(-)
> >
> > diff --git a/vpnc-script b/vpnc-script
> > index 79cf6e2..bbde4cd 100755
> > --- a/vpnc-script
> > +++ b/vpnc-script
> > @@ -360,6 +360,22 @@ fi
> >
> > # =========== resolv.conf handling for any OS =========================
> >
> > +do_restart_dnsmasq() {
> > + SERVICE=`which service`
>
> You need to redirect to /dev/null the stderr of "which".
I've corrected this (and the other 'which') to follow the style used for
the IPROUTE assignment.
> > + if [ -x $SERVICE ] ; then
> > + $SERVICE dnsmasq restart
>
> If "service" is not installed, then "$SERVICE"==""
> The condition would turn true and script will execute "dnsmasq
> restart", probably rising error.
> You need to write
> if [ -x "$SERVICE" ] ; then
>
> > + else
> > + if [ -x /etc/init.d/dnsmasq ] ; then
>
> use "elif" to limit indentation
Corrected.
>
> > + /etc/init.d/dnsmasq restart
> > + else
> > + SYSTEMCTL=`which systemctl`
> > + if [ -x $SYSTEMCTL ] ; then
>
> Same as above, redirect stderr and "" around the variable
>
> > + $SYSTEMCTL restart dnsmasq
> > + fi
> > + fi
>
> Embedded system could start dnsmasq by some custom script.
>
> We are here because you have detected that dnsmasq is running, but you
> cannot find a way to restart it.
> Should we print an error message ?
> Or return an error so caller will remove /etc/dnsmasq.d/$TUNDEV.conf ?
This is a very good point. I've updated my patch such that
do_restart_dnsmasq returns 1 for this case and the caller then displays
an error message appropriate to the action (modifying/restoring). In
the modifying case, I will not leave the dnsmasq configuration fragment
lying around, that can do more harm than good, but include what would've
been in it in the error message. Then, since DNS is still in an
unusable state for the VPN, it falls back to using whatever vpnc-script
would've used if dnsmasq hadn't been discovered.
Hopefully it'll be clear when I send out the patch.
> > + fi
> > +}
> > +
> > modify_resolvconf_generic() {
> > grep '^#@VPNC_GENERATED@' /etc/resolv.conf > /dev/null 2>&1 || cp -- /etc/resolv.conf "$RESOLV_CONF_BACKUP"
> > NEW_RESOLVCONF="#@VPNC_GENERATED@ -- this file is generated by vpnc
> > @@ -542,19 +558,43 @@ restore_resolvconf_openwrt() {
> >
> > modify_resolvconf_manager() {
>
> We are in this function because the script have detected that
> "/sbin/resolvconf" is installed and it plans to use it.
> But then you skip resolvconf and play with dnsmasq.
> I think you should detect dnsmasq before and add dedicated
> MODIFYRESOLVCONF / RESTORERESOLVCONF for dnsmasq.
You're right. I created modify/restore_resolvconf_dnsmasq() and
restored the original state of modify/restore_resolvconf_manager().
>
> > NEW_RESOLVCONF=""
> > - for i in $INTERNAL_IP4_DNS; do
> > - NEW_RESOLVCONF="$NEW_RESOLVCONF
> > + if [[ -d /etc/dnsmasq.d/ && `pgrep dnsmasq` ]] ;
> > + then
>
> Put "then" in the same line with "if"
> "[[" is bash specific. Replace "[[" with "[" and "&&" with "-a"
Corrected.
> "pgrep" will match any running application named "*dnsmasq*". Used
> `pgrep '^dnsmasq$'`
> But then, you have added a new dependency from pgrep. Any replacement
> for system that does not have it installed?
Of course. I should've avoided pgrep anyway. This construct does
the same thing:
ps -e | awk '/\<dnsmasq\>/ { print $1 }'
does that seem like a better option? That's what I've got in my version
of the script that I'm testing now.
-J.
>
> > + # the system has dnsmasq installed and appears to be using it so inform
> > + # dnsmasq about the new servers and domains
> > + if [ -n "$CISCO_DEF_DOMAIN" ]; then
> > + # limit searching the VPN servers for only VPN addresses, but it is
> > + # better to cast too wide a net on this than miss VPN address lookups.
> > + VPN_DNS=`echo $CISCO_DEF_DOMAIN | awk -F. '{ print "/" $(NF-1) "." $NF "/" }'`
> > + fi
> > + for i in $INTERNAL_IP4_DNS; do
> > + NEW_RESOLVCONF="$NEW_RESOLVCONF
> > +server=$VPN_DNS$i"
> > + done
> > + echo "$NEW_RESOLVCONF" > /etc/dnsmasq.d/$TUNDEV.conf
> > + # inform dnsmasq that there is a new configuraiton fragment to consider.
>
> s/configuraiton/configuration/
>
> > + do_restart_dnsmasq
> > + else
> > + for i in $INTERNAL_IP4_DNS; do
> > + NEW_RESOLVCONF="$NEW_RESOLVCONF
> > nameserver $i"
> > - done
> > - if [ -n "$CISCO_DEF_DOMAIN" ]; then
> > - NEW_RESOLVCONF="$NEW_RESOLVCONF
> > + done
> > + if [ -n "$CISCO_DEF_DOMAIN" ]; then
> > + NEW_RESOLVCONF="$NEW_RESOLVCONF
> > domain $CISCO_DEF_DOMAIN"
> > + fi
> > + echo "$NEW_RESOLVCONF" | /sbin/resolvconf -a $TUNDEV
> > fi
> > - echo "$NEW_RESOLVCONF" | /sbin/resolvconf -a $TUNDEV
> > }
> >
> > restore_resolvconf_manager() {
> > - /sbin/resolvconf -d $TUNDEV
> > + if [[ -d /etc/dnsmasq.d/ && `pgrep dnsmasq` ]] ;
> > + then
>
> As above; "then" in same line, replace "[[" and all said about "pgrep"
>
> Best Regards,
> Antonio
>
> > + rm -f /etc/dnsmasq.d/$TUNDEV.conf
> > + do_restart_dnsmasq
> > + else
> > + /sbin/resolvconf -d $TUNDEV
> > + fi
> > }
> >
> > # ========= Toplevel state handling =======================================
> > --
> > 1.7.10.4
> >
> >
> > _______________________________________________
> > openconnect-devel mailing list
> > openconnect-devel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/openconnect-devel
--
-Joe MacDonald.
:wq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20140704/47d9ef0f/attachment.sig>
