openvpn-devel

[Openvpn-devel] [PATCH 0/7] Vlan support for openvpn

[Mike A. <mik...@gm...>]

Hi there,

This is the start of a 7-patch set to add vlan support to openvpn, as
authored by Fabian Knittel.

This a squashed patchset (originally numbering 21 patches, but now
reduced to 7).  These were originally written up to four years ago, and
have been maintained (at sparse intervals) by the original author.  He's
made use of them in production at a large university for the past four
years, and I'm keen to implement them in a medium sized international
organization.

These were slated for inclusion in openvpn four years ago but were never
fully reviewed.  I tried asking for these to be included back in
January, but was told that the patches were too numerous to be reviewed.
 I've therefore squashed the patches down and merged the comments in
order to create this patchset.

I contacted the original author asking if he could do the squashing, but
got no reply for a month.  I then contacted him again with my attempt
asking him if he was happy with the attribution and merging of the
comments and gave him a month to review them and provide a response.
I've still had no reply and am therefore submitting them to the list.

The original author's repository is available at [1].  I'm currently
maintaining the original patchset against the current master at [2], but
I'm relatively new to using git for interacting with large projects, so
didn't add the patches into a branch unfortunately.  The recent
conversations (including previous history) are available at [3].

Please let me know what further I'll need to do in order to help get
these patches included upstream?

Thanks,

Mike  5:)

[1] https://github.com/fknittel/openvpn
[2] https://github.com/ikelos/openvpn
[3] https://sourceforge.net/p/openvpn/mailman/message/34750695/

Re: [Openvpn-devel] [PATCH 0/7] Vlan support for openvpn

[Gert D. <ge...@gr...>]

Hi,

On Sun, Apr 03, 2016 at 07:51:07PM +0100, Mike Auty wrote:
> This is the start of a 7-patch set to add vlan support to openvpn, as
> authored by Fabian Knittel.
> 
> This a squashed patchset (originally numbering 21 patches, but now
> reduced to 7).
[..]
> Please let me know what further I'll need to do in order to help get
> these patches included upstream?

Well.  Thanks for maintaining, and squashing this.

Overall, I think it's still too many patches - like the doc patch and the
configure patch, these do not make sense on their own, so maybe squashing
the patch set even further (after some feedback) might be the way to go.

Usually we try to have patches in logical units - like, "one bug fix", or
"one manageable chunk of refactoring", or "one new feature, with header,
code, configure, and documentation".  Very complex new features do get
spread across multiple patches, of course, but splitting off configure and
documentatation does not really make sense - without documentation, the
patch is incomplete, and the configure change without the code change is
not useful.  Of course this is not a hard rule, but you might look at
the commit log to get an idea how "this is usually done".

I have not done a full review, but I noticed that at least for the 
"is_ipv4()" patch, the documentation isn't correct anymore - it talks
about is_ipv4(), but the function has been merged with IPv6 and is called
is_ipv_X() - which the code change acknowledges, but the commit message
doesn't.  Further, that particular change creates quite a bit of messy
code - we do have a style guide, and the code should follow it (I'm aware
that the original code is more "compact" than according to style guide,
but when extending by extra branch levels, this gets non-pretty quickly).


But these are all minor things - "the big thing" missing here is "the success
story".  

So - what is it that you can do with it that you can not do with tun mode?  
How would the configuration look like on the host side?  etc.  - please 
describe this to an audience that has no idea whatsoever about vlan 
stuff(*), and has mostly moved from tap mode to tun mode in the previous 
years (since tun has less overhead, *and* both Android and iOS do not 
support tap mode unless rooted).

So we have somewhat of a niche feature for a niche corner of OpenVPN 
(tap mode) - which will land on our plates for maintenance if we take it,
and the balance between "useful new code" and "maintenance chore" needs
to be on the side of "useful"...

Right now, we have about 47123 strange options and compile-time features
that hardly anyone is using, but every time we change one of the more
mainstream features in a bigger way, one of the exotic things breaks
(and we feel stupid about it).  Which makes us a bit reluctant about adding
new "special feature" code.

gert

(*) I do understand VLANs - being a network guy - but I'm missing the 
big picture "how would the configuration on the host and client (and ccd/)
side look like?  what can you do with it?  why is this cool?"

PS: I promise a more detailed review per-chunk, so don't send new patches
yet.  I'm not making promises which month this is going to happen - sorry.
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             ge...@gr...
fax: +49-89-35655025                        ge...@ne...

Re: [Openvpn-devel] [PATCH 0/7] Vlan support for openvpn

[Mike A. <mik...@gm...>]

On 04/04/16 07:54, Gert Doering wrote:
> Hi,

Hello!

> On Sun, Apr 03, 2016 at 07:51:07PM +0100, Mike Auty wrote:
> Well.  Thanks for maintaining, and squashing this.

My pleasure!  5:)

> Overall, I think it's still too many patches - like the doc patch and the
> configure patch, these do not make sense on their own, so maybe squashing
> the patch set even further (after some feedback) might be the way to go.

Ah, I was worried about the patches being too big, the granularity for
other patches that have been past recently seemed very small, sorry, I'm
happy to squash it more.

> Usually we try to have patches in logical units - like, "one bug fix", or
> "one manageable chunk of refactoring", or "one new feature, with header,
> code, configure, and documentation".  Very complex new features do get
> spread across multiple patches, of course, but splitting off configure and
> documentatation does not really make sense - without documentation, the
> patch is incomplete, and the configure change without the code change is
> not useful.  Of course this is not a hard rule, but you might look at
> the commit log to get an idea how "this is usually done".

My reason for splitting the configure options and the documentation was
that they were extremely easy self-contained units (and they'd work
as standalone commits without disrupting any of the other code, but I
guess the #ifdefs do that too).

> I have not done a full review, but I noticed that at least for the 
> "is_ipv4()" patch, the documentation isn't correct anymore - it talks
> about is_ipv4(), but the function has been merged with IPv6 and is called
> is_ipv_X() - which the code change acknowledges, but the commit message
> doesn't.  Further, that particular change creates quite a bit of messy
> code - we do have a style guide, and the code should follow it (I'm aware
> that the original code is more "compact" than according to style guide,
> but when extending by extra branch levels, this gets non-pretty quickly).

Ok, thanks for spotting that, I must've grabbed the commit message from
an earlier commit because it seemed relevant.  I'll take a look at
what's going on and the style guide to see if I can sort the code (as
well as the documentation).

> 
> But these are all minor things - "the big thing" missing here is "the success
> story".  
> 
> So - what is it that you can do with it that you can not do with tun mode?  
> How would the configuration look like on the host side?  etc.  - please 
> describe this to an audience that has no idea whatsoever about vlan 
> stuff(*), and has mostly moved from tap mode to tun mode in the previous 
> years (since tun has less overhead, *and* both Android and iOS do not 
> support tap mode unless rooted).
> 
> So we have somewhat of a niche feature for a niche corner of OpenVPN 
> (tap mode) - which will land on our plates for maintenance if we take it,
> and the balance between "useful new code" and "maintenance chore" needs
> to be on the side of "useful"...
> 
> Right now, we have about 47123 strange options and compile-time features
> that hardly anyone is using, but every time we change one of the more
> mainstream features in a bigger way, one of the exotic things breaks
> (and we feel stupid about it).  Which makes us a bit reluctant about adding
> new "special feature" code.

Yep, I understand there's a maintenance cost associated with adding the
code to the project, but it's the maintenance cost of layer 3 management
that this is trying to offset somewhat.  Layer 2 VPNs, whilst
potentially less efficient due to additional traffic (is there another
reason the tap adaptor is less efficient?), allow for simpler management
and configuration, and do not require significant changes to the VPN
configuration if the network topology changes.

We've implemented 802.1x with dynamic vlan assignment already, meaning
that when a user connects either wired or wirelessly they get shunted
into the appropriate segment of the network (and potentially into
overlapping subnets).  This ensures strong segregation based on their
identity/certificate.  So we really want to plug openvpn into that and
allow users a single certificate to always end up in their same
segregated section of the network without having to create separate VPN
subnets for them.  This can be achieved using Fabian's patches and the
following server config (with no changes to the client software or config!):

vlan-tagging
vlan-accept tagged
client-connect /path/to/radius_auth_script.sh

The radius auth script would echo out "vlan-pvid 2", for example, for a
user who belongs in vlan 2, whilst it will push a different user into
vlan 3 if appropriate.  It means that users can talk within their own
VLAN but not with everybody using the VPN (which at the moment
client-to-client won't allow, and would be complex firewall rules
otherwise).  This also allows us to use a single trunk port, rather than
having to split all the various networks out into their own connections
to feed into the VPN server or for us to run multiple VPN servers on
different ports/IPs for different sets of users.

So our users see a single VPN configuration, connect with their existing
certificates, and end up in the network they expected to with DHCP
provided by the network itself (allowing us to change DNS servers,
routers, etc from a single central point rather than distributing that
information out to DHCP servers and openvpn servers, etc).

It would be possible, but in our opinion much more of a burden, to
implement, manage and maintain a single endpoint in tun mode that could
achieve the same effect, and make use of the existing network topology
that we're already using.  I'd like to think this capability ranks over
some of the existing 47123 strange options that have already been
accepted into OpenVPN in terms of usefulness, and that's just my
use-case scenario alone.

Allowing bridged mode to have knowledge of vlan tags and be able to
manipulate them based on the user is an advanced, but extremely valuable
facility that I was genuinely surprised OpenVPN didn't already have,
given its flexibility.  These existing, tested patches would add that
support.

Hopefully that sounds like a success story?  5:)

> gert
> 
> (*) I do understand VLANs - being a network guy - but I'm missing the 
> big picture "how would the configuration on the host and client (and ccd/)
> side look like?  what can you do with it?  why is this cool?"
> 
> PS: I promise a more detailed review per-chunk, so don't send new patches
> yet.  I'm not making promises which month this is going to happen - sorry.
> 

It'll take me a bit of time to get the changes requested done too, but
thank you for reviewing these, it's much appreciated.  5:)

Mike  5:)

Re: [Openvpn-devel] [PATCH 0/7] Vlan support for openvpn

[Mike A. <mik...@gm...>]

Hiya Gert,

On 04/04/16 07:54, Gert Doering wrote:
> PS: I promise a more detailed review per-chunk, so don't send new patches
> yet.  I'm not making promises which month this is going to happen - sorry.

I was just wondering if there'd been any progress on this?  Not pushing,
since you didn't promise a time, but wanted to ask if there was anything
I could be doing to help progress this?

Thanks,

Mike  5:)