VLAN-Tagging

As '--vlan-accept raw' is a global behavioural modifier, it makes more sense to break it out into a global '--vlan-tagging' boolean switch. (Which matches the approach chosen in the initial patch-set.)

'--vlan-accept tagged|untagged|all' now only specifies the behaviour of the tap device, while '--vlan-tagging' specifies whether OpenVPN should take a closer look at the tagging of packets.

The patch is only a configuration option change. The behaviour should remain the same.

New Old
------------------------------------------------------------------

not specifying anything is equivalent to not specifying anything

not specifying anything is equivalent to '--vlan-accept raw'

'--vlan-tagging' +
'--vlan-accept tagged' is equivalent to '--vlan-accept tagged'

'--vlan-tagging' +
'--vlan-accept untagged' is equivalent to '--vlan-accept untagged'

'--vlan-tagging' is equivalent to '--vlan-accept all'

'--vlan-tagging' +
'--vlan-accept all' is equivalent to '--vlan-accept all'

Hi,

changes since the last round of patches:

- Added Signed-Off-By to all patches.

- Properly mentioned "--vlan-accept all" mode in options.c (it was missing in a few places).

- Split out '--vlan-accept raw' to '--vlan-tagging' boolean switch. Quoting from the patch description:

Quoted 21 lines

As '--vlan-accept raw' is a global behavioural modifier, it makes more sense to break it out into a global '--vlan-tagging' boolean switch. (Which matches the approach chosen in the initial patch-set.)

'--vlan-accept tagged|untagged|all' now only specifies the behaviour of the tap device, while '--vlan-tagging' specifies whether OpenVPN should take a closer look at the tagging of packets.

The patch is only a configuration option change. The behaviour should remain the same.

New Old
------------------------------------------------------------------

not specifying anything is equivalent to not specifying anything

not specifying anything is equivalent to '--vlan-accept raw'

'--vlan-tagging' +
'--vlan-accept tagged' is equivalent to '--vlan-accept tagged'

Quoted 7 lines

'--vlan-tagging' +
'--vlan-accept untagged' is equivalent to '--vlan-accept untagged'

'--vlan-tagging' is equivalent to '--vlan-accept all'

'--vlan-tagging' +
'--vlan-accept all' is equivalent to '--vlan-accept all'

All further work will be bug-fix only.

The rebased changes are available from my git repo [1] on branch 'feat_vlan'. On branch 'feat_vlan_tagging' the same changes are available as incremental patches to the 'feat_vlan_tagging' branch on openvpn-testing.git. (Only the non-incremental patches provide all missing Signed-Off-Bys).

For reviewers who haven't had a look at the other patches yet, I've attached a diff containing all changes introduced by the current patch-set.

Cheers
Fabian

1: git://fsmi-dev.fsmi.uni-karlsruhe.de/openvpn.git

I've finally found some time to dig into this again. After some consideration, I decided to rebase your work on your feat_vlan_tagging branch against the openvpn-testing.git feat_vlan_tagging branch.

This means that your earlier patches without signed-off-by tags are not

merged in. I am fine with that, as I've become stricter on those tags later on.

The alternative is to scratch the feat_vlan_tagging branch now in openvpn-testing.git and re-establish it on your feat_vlan branch, which has those tags all from the beginning.

So I will leave it up to you now how you want it. But in the moment I this branch gets merged into allmerged, its too late to change your opinion. I will wait for your reply on which approach you would like.

When this is settled, the only missing thing is to get someone who can understand the code path being changed in your feature branch a bit better than me to give an official ACK. When I get that ACK, it goes into allmerged.

Hi David,

Quoted 5 lines · David Sommerseth

I've finally found some time to dig into this again. After some consideration, I decided to rebase your work on your feat_vlan_tagging branch against the openvpn-testing.git feat_vlan_tagging branch.

This means that your earlier patches without signed-off-by tags are not

                                       ^^^^^^^--- with?

Quoted 7 lines

merged in. I am fine with that, as I've become stricter on those tags later on.

The alternative is to scratch the feat_vlan_tagging branch now in openvpn-testing.git and re-establish it on your feat_vlan branch, which has those tags all from the beginning.

Quoted 3 lines

So I will leave it up to you now how you want it. But in the moment I this branch gets merged into allmerged, its too late to change your opinion. I will wait for your reply on which approach you would like.

If I may choose freely, I'd prefer the fresh patch-set that has the proper signed-of-by lines. But I'm fine with both approaches.

I'll do all future changes incrementally so that this decision doesn't come up again. :)

I've attached a diff containing all changes introduced by the current patch-set.

mroute.c +1 -0

+++ b/mroute.c

@@ -164,12 +164,28 @@ mroute_extract_addr_ipv4 (struct mroute_addr *src,
   return ret;
 }
 

(quoted diff) +1 -0

+++ b/multi.c

multi.c +11 -0

@@ -1918,6 +1924,28 @@ multi_process_post (struct multi_context *m, struct multi_instance *mi, const un
   return ret;
 }
 
+#ifdef ENABLE_VLAN_TAGGING
+bool
+buf_filter_incoming_vlan_tags (const struct buffer *buf)
+{
+  if (BLEN (buf) >= (int) sizeof (struct openvpn_8021qhdr))
+    {
+      const struct openvpn_8021qhdr *vlanhdr = (const struct openvpn_8021qhdr *) BPTR (buf);
+
+      if (ntohs (vlanhdr->tpid) == OPENVPN_ETH_P_8021Q)
+        {
+	  const int16_t vid = vlanhdr_get_vid(vlanhdr);

(quoted diff) +2 -0

+remove_vlan_tag (const struct context *c, struct buffer *buf)
+{

(quoted diff) +2 -0

+multi_prepend_vlan_tag (const struct context *c, struct buffer *buf)
+{

(quoted diff) +0 -0

@@ -415,6 +418,26 @@ multi_process_outgoing_tun (struct multi_context *m, const unsigned int mpp_flag

+++ b/openvpn.8

+++ b/options.c

options.c +1 -0

@@ -1742,6 +1774,17 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 
 	if ((options->ssl_flags & SSLF_NO_NAME_REMAPPING) && script_method == SM_SYSTEM)
 	  msg (M_USAGE, "--script-security method='system' cannot be combined with --no-name-remapping");
+#ifdef ENABLE_VLAN_TAGGING

(quoted diff) +0 -0

@@ -5730,6 +5776,45 @@ add_option (struct options *options,

options.c +22 -0

+  else if (streq (p[0], "vlan-accept") && p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      if (streq (p[1], "tagged"))
+	{
+	  options->vlan_accept = VAF_ONLY_VLAN_TAGGED;
+	}
+      else if (streq (p[1], "untagged"))
+	{
+	  options->vlan_accept = VAF_ONLY_UNTAGGED_OR_PRIORITY;
+	}
+      else if (streq (p[1], "all"))
+	{
+	  options->vlan_accept = VAF_ALL;
+	}
+      else
+	{
+	  msg (msglevel, "--vlan-accept must be 'tagged', 'untagged' or 'all'");
+	  goto err;
+	}
+    }
+  else if (streq (p[0], "vlan-pvid") && p[1])

+++ b/proto.h

Fabian Knittel wrote:

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2069

Quoted 9 lines

multi.c +5 -0

@@ -2033,10 +2062,27 @@ multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins
 	    }
 	  else if (TUNNEL_TYPE (m->top.c1.tuntap) == DEV_TYPE_TAP)
 	    {
+#ifdef ENABLE_VLAN_TAGGING
+	      int16_t vid = 0;
+#else
+	      const int16_t vid = 0;
+#endif

I like how you use const here.

Quoted 2 lines

(quoted diff) +2 -0

+multi_prepend_vlan_tag (const struct context *c, struct buffer *buf)
+{

..

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2277

Quoted 3 lines

multi.c +3 -0

+  /* Frame too small? */
+  if (BLEN (buf) < (int) sizeof (struct openvpn_ethhdr))
+    goto drop;

I like forward goto for error handling very much too!

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2286

Quoted 3 lines

multi.c +3 -0

+      /* Frame too small for header type? */
+      if (BLEN (buf) < (int) (sizeof (struct openvpn_8021qhdr)))
+	goto drop;

This looks like a whitespace oops?

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2296

Quoted 3 lines

multi.c +3 -0

+      /* Not enough head room for VLAN tag? */
+      if (buf_reverse_capacity (buf) < SIZE_ETH_TO_8021Q_HDR)
+	goto drop;

Here too? Hmm. Am I missing something?

multi.c multi_process_incoming_tun (struct multi_context *m, const unsigned int mpp_flag · line 2374

Quoted 13 lines

multi.c +9 -0

@@ -2158,6 +2363,16 @@ multi_process_incoming_tun (struct multi_context *m, const unsigned int mpp_flag
        * the appropriate multi_instance object.
        */
 
+#ifdef ENABLE_VLAN_TAGGING
+      if (dev_type == DEV_TYPE_TAP && m->top.options.vlan_tagging)
+        {
+	  if ((vid = remove_vlan_tag (&m->top, &m->top.c2.buf)) == -1)
+	    {
+	      return false;
+	    }
+        }
+#endif

This indent also looks off - but it may just be me not knowing the coding style. (I hope not though, this is horrible.)

Quoted 1 line

(quoted diff) +0 -0

@@ -415,6 +418,26 @@ multi_process_outgoing_tun (struct multi_context *m, const unsigned int mpp_flag

..

Review

Quoted 2 lines

(quoted diff) +2 -0

+      if (m->top.options.vlan_pvid != mi->context.options.vlan_pvid)
+	{

Again with the weird indenting.

Quoted 6 lines

+++ b/options.c

options.c +1 -0

@@ -1742,6 +1774,17 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
 
 	if ((options->ssl_flags & SSLF_NO_NAME_REMAPPING) && script_method == SM_SYSTEM)
 	  msg (M_USAGE, "--script-security method='system' cannot be combined with --no-name-remapping");
+#ifdef ENABLE_VLAN_TAGGING

options.c options_postprocess_verify_ce (const struct options *options, const struct conne · line 1787

Quoted 10 lines

options.c +10 -0

+      if (options->vlan_tagging && dev != DEV_TYPE_TAP)
+	msg (M_USAGE, "--vlan-tagging must be used with --dev tap");
+      if (!options->vlan_tagging)
+	{
+	  if (options->vlan_accept != defaults.vlan_accept)
+	    msg (M_USAGE, "--vlan-accept requires --vlan-tagging");
+	  if (options->vlan_pvid != defaults.vlan_pvid)
+	    msg (M_USAGE, "--vlan-pvid requires --vlan-tagging");
+	}
+#endif

Funky indent again.

options.c options_postprocess_verify_ce (const struct options *options, const struct conne · line 1837

Quoted 9 lines

options.c +4 -1

@@ -1788,7 +1831,10 @@ options_postprocess_verify_ce (const struct options *options, const struct conne
       if (options->port_share_host || options->port_share_port)
 	msg (M_USAGE, "--port-share requires TCP server mode (--mode server --proto tcp-server)");
 #endif
-
+#ifdef ENABLE_VLAN_TAGGING
+      if (options->vlan_tagging)
+	msg (M_USAGE, "--vlan-tagging requires --mode server");
+#endif

Here too.

Quoted 1 line

(quoted diff) +0 -0

@@ -5730,6 +5776,45 @@ add_option (struct options *options,

..

Quoted 22 lines

options.c +22 -0

+  else if (streq (p[0], "vlan-accept") && p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      if (streq (p[1], "tagged"))
+	{
+	  options->vlan_accept = VAF_ONLY_VLAN_TAGGED;
+	}
+      else if (streq (p[1], "untagged"))
+	{
+	  options->vlan_accept = VAF_ONLY_UNTAGGED_OR_PRIORITY;
+	}
+      else if (streq (p[1], "all"))
+	{
+	  options->vlan_accept = VAF_ALL;
+	}
+      else
+	{
+	  msg (msglevel, "--vlan-accept must be 'tagged', 'untagged' or 'all'");
+	  goto err;
+	}
+    }
+  else if (streq (p[0], "vlan-pvid") && p[1])

options.c add_option (struct options *options, · line 5816

Quoted 10 lines

options.c +10 -0

+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL|OPT_P_INSTANCE);
+      options->vlan_pvid = positive_atoi (p[1]);
+      if (options->vlan_pvid < OPENVPN_8021Q_MIN_VID ||
+	  options->vlan_pvid > OPENVPN_8021Q_MAX_VID)
+	{
+	  msg (msglevel, "the parameter of --vlan-pvid parameters must be >= %d and <= %d", OPENVPN_8021Q_MIN_VID, OPENVPN_8021Q_MAX_VID);
+	  goto err;
+	}
+    }

This indent stuff is really killing me. I find it extremely hard to read. :)

proto.c is_ipv4 (int tunnel_type, struct buffer *buf) · line 62

Quoted 11 lines

+++ b/proto.c

proto.c +3 -0

@@ -54,6 +54,9 @@ is_ipv4 (int tunnel_type, struct buffer *buf)
 	return false;
       eh = (const struct openvpn_ethhdr *) BPTR (buf);
       if (ntohs (eh->proto) == OPENVPN_ETH_P_8021Q) {
+        if (BLEN (buf) < (int)(sizeof (struct openvpn_8021qhdr)
+	    + sizeof (struct openvpn_iphdr)))
+	  return false;
         const struct openvpn_8021qhdr *evh;
         evh = (const struct openvpn_8021qhdr *) BPTR (buf);
         if (ntohs (evh->proto) != OPENVPN_ETH_P_IPV4)

Indenting here too. And I think the if needs to go after the struct to compile very broadly, even if C99 permits variables like this.

Quoted 5 lines · Fabian Knittel

+++ b/configure.ac

configure.ac +0 -0

@@ -212,6 +212,12 @@ AC_ARG_ENABLE(selinux,
    [SELINUX="yes"]
 )
 

configure.ac AC_ARG_ENABLE(selinux, · line 219

Quoted 5 lines

configure.ac +5 -0

+AC_ARG_ENABLE(vlan-tagging,
+   [  --disable-vlan-tagging  Disable support for VLAN tagging/untagging],
+   [VLAN_TAGGING="$enableval"],
+   [VLAN_TAGGING="yes"]
+)

Maybe explicitly mention that it's 802.1Q ?

configure.ac if test "$SELINUX" = "yes"; then · line 898

Quoted 4 lines

configure.ac +4 -0

+dnl enable --vlan-tagging
+if test "$VLAN_TAGGING" = "yes"; then
+   AC_DEFINE(ENABLE_VLAN_TAGGING, 1, [Enable VLAN tagging/untagging])
+fi

Maybe also here.

Quoted 5 lines

mroute.c +1 -0

+++ b/mroute.c

@@ -164,12 +164,28 @@ mroute_extract_addr_ipv4 (struct mroute_addr *src,
   return ret;
 }
 

mroute.c mroute_extract_addr_ipv4 (struct mroute_addr *src, · line 176

Quoted 10 lines

mroute.c +10 -0

+static void mroute_copy_ether_to_addr(struct mroute_addr *maddr,
+				      const uint8_t *eth_addr,
+				      int16_t vid)
+{
+  maddr->type = MR_ADDR_ETHER;
+  maddr->netbits = 0;
+  memcpy (maddr->addr, eth_addr, 6);
+#ifdef ENABLE_VLAN_TAGGING
+  maddr->len = 8;
+  memcpy (maddr->addr + 6, &vid, 2);

Does this need to consider htons() ?

Also, I have a weak preference for uint16_t, just to indicate that negative values are never correct.

mroute.c mroute_addr_print_ex (const struct mroute_addr *ma, · line 327

Quoted 7 lines

mroute.c +3 -0

@@ -303,6 +322,9 @@ mroute_addr_print_ex (const struct mroute_addr *ma,
 	{
 	case MR_ADDR_ETHER:
 	  buf_printf (&out, "%s", format_hex_ex (ma->addr, 6, 0, 1, ":", gc)); 
+#ifdef ENABLE_VLAN_TAGGING
+	  buf_printf (&out, "@%d", *(int16_t*)(ma->addr + 6));
+#endif

Also here, ntohs() ?

Quoted 1 line

(quoted diff) +1 -0

+++ b/multi.c

..

Quoted 20 lines

multi.c +16 -0

@@ -1918,6 +1924,28 @@ multi_process_post (struct multi_context *m, struct multi_instance *mi, const un
   return ret;
 }
 
+#ifdef ENABLE_VLAN_TAGGING
+bool
+buf_filter_incoming_vlan_tags (const struct buffer *buf)
+{
+  if (BLEN (buf) >= (int) sizeof (struct openvpn_8021qhdr))
+    {
+      const struct openvpn_8021qhdr *vlanhdr = (const struct openvpn_8021qhdr *) BPTR (buf);
+
+      if (ntohs (vlanhdr->tpid) == OPENVPN_ETH_P_8021Q)
+        {
+	  const int16_t vid = vlanhdr_get_vid(vlanhdr);
+	  if (vid != 0)
+	    {
+	      msg (D_VLAN_DEBUG, "dropping tagged incoming frame, vid: %d", vid);
+	      return true;
+	    }

multi.c multi_process_post (struct multi_context *m, struct multi_instance *mi, const un · line 1947

Quoted 5 lines

multi.c +5 -0

+	}
+    }
+  return false;
+}
+#endif

What a horrible coding style this is!! But oh well, that's certainly not your fault. :)

However, I really like code that checks for "break" conditions first, and returns early, over code that nests multiple levels of conditions, something like:

{
struct openvpn_8021qhdr *vlanhdr;
int16_t vid;
if (BLEN (buf) < (int) sizeof (struct openvpn_8021qhdr))

    return false;
  vlanhdr = (const struct openvpn_8021qhdr *) BPTR (buf);
  if (ntohs (vlanhdr->tpid) != OPENVPN_ETH_P_8021Q)
    return false;
  vid = vlanhdr_get_vid(vlanhdr);
  if (0 == vid)
    return false;

msg (D_VLAN_DEBUG, "dropping .. ");
return true;
}

But don't let this start some kind of argument about coding style. I have no technical complaints about the hunk above.

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2069

Quoted 9 lines

multi.c +5 -0

@@ -2033,10 +2062,27 @@ multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins
 	    }
 	  else if (TUNNEL_TYPE (m->top.c1.tuntap) == DEV_TYPE_TAP)
 	    {
+#ifdef ENABLE_VLAN_TAGGING
+	      int16_t vid = 0;
+#else
+	      const int16_t vid = 0;
+#endif

I like how you use const here.

Quoted 4 lines

mroute.c +0 -0

@@ -2116,6 +2163,159 @@ multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins
   return ret;
 }
 

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2170

Quoted 5 lines

multi.c +5 -0

+#ifdef ENABLE_VLAN_TAGGING
+/*
+ * For vlan_accept == VAF_ONLY_UNTAGGED_OR_PRIORITY:
+ *   If a frame is VLAN-tagged, it is dropped.  Otherwise, the global
+ *   vlan_pvid is returned as VID.

This is just a comment in the code, but maybe mention that vid=0 in a tag does not mean that it's VLAN-tagged. Yes, _OR_PRIORITY indicates this, but it might be nice to be super consistent also in the text.

Quoted 2 lines

(quoted diff) +2 -0

+remove_vlan_tag (const struct context *c, struct buffer *buf)
+{

..

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2217

Quoted 3 lines

multi.c +3 -0

+  /* Tagged packet. */
+
+  vid = ntohs (vlanhdr_get_vid (&vlanhdr));

Hmm - does ntohs() here mean that it shouldn't be done in mroute.c?

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2286

Quoted 3 lines

multi.c +3 -0

+      /* Frame too small for header type? */
+      if (BLEN (buf) < (int) (sizeof (struct openvpn_8021qhdr)))
+	goto drop;

This looks like a whitespace oops?

multi.c multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins · line 2296

Quoted 3 lines

multi.c +3 -0

+      /* Not enough head room for VLAN tag? */
+      if (buf_reverse_capacity (buf) < SIZE_ETH_TO_8021Q_HDR)
+	goto drop;

Here too? Hmm. Am I missing something?

This indent also looks off - but it may just be me not knowing the coding style. (I hope not though, this is horrible.)

Quoted 1 line

+++ b/openvpn.8

..

openvpn.8 other protocols such as ssh. · line 3247

Quoted 9 lines

openvpn.8 +9 -0

+Incoming untagged packets from clients are assigned with the client's Port
+VLAN Identifier (PVID) as their VID. In
+.B untagged
+mode, incoming untagged packets on the tap device are associated with the
+global
+.B \-\-vlan\-pvid
+setting. In
+.B tagged
+mode, any incoming untagged packets are dropped.

Would it be desirable to support both tagged and untagged at once?

On Linux maybe there could be a workaround involving a bridge or so.

Quoted 6 lines

+++ b/proto.c

proto.c +1 -0

@@ -54,6 +54,9 @@ is_ipv4 (int tunnel_type, struct buffer *buf)
 	return false;
       eh = (const struct openvpn_ethhdr *) BPTR (buf);
       if (ntohs (eh->proto) == OPENVPN_ETH_P_8021Q) {
+        if (BLEN (buf) < (int)(sizeof (struct openvpn_8021qhdr)

proto.c is_ipv4 (int tunnel_type, struct buffer *buf) · line 62

Quoted 5 lines

proto.c +2 -0

+	    + sizeof (struct openvpn_iphdr)))
+	  return false;
         const struct openvpn_8021qhdr *evh;
         evh = (const struct openvpn_8021qhdr *) BPTR (buf);
         if (ntohs (evh->proto) != OPENVPN_ETH_P_IPV4)

Indenting here too. And I think the if needs to go after the struct to compile very broadly, even if C99 permits variables like this.

Quoted 1 line

+++ b/proto.h

..

proto.h void ipv4_packet_size_verify (const uint8_t *data, · line 234

Quoted 8 lines

proto.h +8 -0

+/*
+ * Retrieve the Priority Code Point (PCP) from the IEEE 802.1Q header.
+ */
+static inline int
+vlanhdr_get_pcp (const struct openvpn_8021qhdr *hdr)
+{
+  return hdr->pcp_cfi_vid & OPENVPN_8021Q_MASK_PCP;
+}

What about byte order for these guys? Is it OK to use host byte order?