Hello, Martin, Remi, All Im very excited about this feature, Thanks Martin, Please see Comments inline below
On 23 January 2018 at 18:06, Remi Locherer <[email protected]> wrote: > On Mon, Jan 22, 2018 at 04:23:59PM +0100, Martin Pieuchot wrote: >> Diff below adds a new feature to bridge(4), similar to Cisco's Protected >> Port but with more possibilities. >> >> The idea is to prevent traffic to flow between some members of a bridge(4). >> For example: >> - you want to prevent some of your servers to talk to each others, or >> - you want employees/students/clients to have access to internet and a >> printer but not to each other, or >> - you want VMs to have access to an uplink but not to each other > > I think the last example is really useful and makes sense. Agreed > > For the other cases there would most likely be a switch between the other > host and the OpenBSD box. Then you need to enforce the policy on that switch. Other examples where it would be useful if the OpenBSD box is terminating alot of Tunnels and you want to limit broadcast domains. > >> With the diff below it is now possible to defined "protected groups". >> Interface that are part of such groups cannot send/receive traffic >> to/from any other member of the group. > > Why several protected groups and not just a single one? Having multiple protected groups can be useful in the following circumstances: Where you want to isolate clients / vms from each other, and where you have 2 redundant up-link ports in the bridge that could other wise create a loop. (where spanning tree cannot be deployed (due to incompatible or limited switches) e.g a) loop prevention in 2 up link ports on a bridge so if the 2 up-link ports are added to the same protected group they cannot form a loop as the 2 up-links are isolated from each other Uplink ports would be em1 and em2 and we would add them to protected group1 em1 and em2 cannot communicate with each other and cant create a loop b)preventing vms from talking to each other by adding them to another protected group e.g. tap1 and tap2 would be added to protected group 2 so tap1 and tap2 would be isolated from each other, and because they are members of a different protected group to em1 and em2 in the same bridge they can communicate freely with either em1 or em2 I have used this technique on certain Layer 2 networks where STP was not possible or desirable,, and it would give the administrator granular control of traffic flow in the bridge Other Added benefits (control broadcast domains etc) increase security for access networks (prevent rogue dhcp Servers) I hope this helps and Im really excited with this diff :) Thanks Guys > >> >> In the example below, em1 and em2 can only send/receive traffic through >> em0 because they are both in the protected group 2. >> >> bridge0: flags=41<UP,RUNNING> >> index 9 llprio 3 >> groups: bridge >> priority 32768 hellotime 2 fwddelay 15 maxage 20 holdcnt 6 proto rstp >> designated: id 00:00:00:00:00:00 priority 0 >> em0 flags=3<LEARNING,DISCOVER> >> port 6 ifpriority 0 ifcost 0 >> em1 flags=3<LEARNING,DISCOVER> >> port 7 ifpriority 0 ifcost 0 protected 2 >> em2 flags=3<LEARNING,DISCOVER> >> port 8 ifpriority 0 ifcost 0 protected 2 >> >> Adding an interface to a protected group is as simple as: >> >> # ifconfig bridge0 protected em1 2 >> >> Removing it: >> >> # ifconfig bridge0 -protected em1 >> >> Comments? Oks? >> >> Index: sys/net/if_bridge.c >> =================================================================== >> RCS file: /cvs/src/sys/net/if_bridge.c,v >> retrieving revision 1.301 >> diff -u -p -r1.301 if_bridge.c >> --- sys/net/if_bridge.c 10 Jan 2018 23:50:39 -0000 1.301 >> +++ sys/net/if_bridge.c 22 Jan 2018 14:27:41 -0000 >> @@ -409,6 +409,7 @@ bridge_ioctl(struct ifnet *ifp, u_long c >> } >> req->ifbr_ifsflags = p->bif_flags; >> req->ifbr_portno = p->ifp->if_index & 0xfff; >> + req->ifbr_protected = p->bif_protected; >> if (p->bif_flags & IFBIF_STP) { >> bp = p->bif_stp; >> req->ifbr_state = bstp_getstate(bs, bp); >> @@ -496,6 +497,19 @@ bridge_ioctl(struct ifnet *ifp, u_long c >> brop->ifbop_last_tc_time.tv_sec = bs->bs_last_tc_time.tv_sec; >> brop->ifbop_last_tc_time.tv_usec = bs->bs_last_tc_time.tv_usec; >> break; >> + case SIOCBRDGSIFPROT: >> + ifs = ifunit(req->ifbr_ifsname); >> + if (ifs == NULL) { >> + error = ENOENT; >> + break; >> + } >> + p = (struct bridge_iflist *)ifs->if_bridgeport; >> + if (p == NULL || p->bridge_sc != sc) { >> + error = ESRCH; >> + break; >> + } >> + p->bif_protected = req->ifbr_protected; >> + break; >> case SIOCBRDGRTS: >> case SIOCBRDGGCACHE: >> case SIOCBRDGGPRI: >> @@ -594,6 +608,7 @@ bridge_bifconf(struct bridge_softc *sc, >> strlcpy(breq->ifbr_ifsname, p->ifp->if_xname, IFNAMSIZ); >> breq->ifbr_ifsflags = p->bif_flags; >> breq->ifbr_portno = p->ifp->if_index & 0xfff; >> + breq->ifbr_protected = p->bif_protected; >> if (p->bif_flags & IFBIF_STP) { >> bp = p->bif_stp; >> breq->ifbr_state = bstp_getstate(sc->sc_stp, bp); >> @@ -848,6 +863,7 @@ bridgeintr_frame(struct bridge_softc *sc >> struct bridge_rtnode *dst_p; >> struct ether_addr *dst, *src; >> struct ether_header eh; >> + u_int32_t protected; >> int len; >> >> >> @@ -960,6 +976,7 @@ bridgeintr_frame(struct bridge_softc *sc >> bridge_broadcast(sc, src_if, &eh, m); >> return; >> } >> + protected = ifl->bif_protected; >> >> /* >> * At this point, we're dealing with a unicast frame going to a >> @@ -975,6 +992,14 @@ bridgeintr_frame(struct bridge_softc *sc >> m_freem(m); >> return; >> } >> + /* >> + * Do not transmit if both ports are part of the same protected >> + * group. >> + */ >> + if (protected != 0 && protected == ifl->bif_protected) { >> + m_freem(m); >> + return; >> + } >> if (bridge_filterrule(&ifl->bif_brlout, &eh, m) == BRL_ACTION_BLOCK) { >> m_freem(m); >> return; >> @@ -1159,6 +1184,10 @@ bridge_broadcast(struct bridge_softc *sc >> struct mbuf *mc; >> struct ifnet *dst_if; >> int len, used = 0; >> + u_int32_t protected; >> + >> + p = (struct bridge_iflist *)ifp->if_bridgeport; >> + protected = p->bif_protected; >> >> TAILQ_FOREACH(p, &sc->sc_iflist, next) { >> dst_if = p->ifp; >> @@ -1177,6 +1206,13 @@ bridge_broadcast(struct bridge_softc *sc >> /* Drop non-IP frames if the appropriate flag is set. */ >> if (p->bif_flags & IFBIF_BLOCKNONIP && >> bridge_blocknonip(eh, m)) >> + continue; >> + >> + /* >> + * Do not transmit if both ports are part of the same >> + * protected group. >> + */ >> + if (protected != 0 && protected == p->bif_protected) >> continue; >> >> if (bridge_filterrule(&p->bif_brlout, eh, m) == >> BRL_ACTION_BLOCK) >> Index: sys/net/if_bridge.h >> =================================================================== >> RCS file: /cvs/src/sys/net/if_bridge.h,v >> retrieving revision 1.55 >> diff -u -p -r1.55 if_bridge.h >> --- sys/net/if_bridge.h 20 Jan 2017 05:03:48 -0000 1.55 >> +++ sys/net/if_bridge.h 22 Jan 2018 15:13:07 -0000 >> @@ -46,6 +46,7 @@ struct ifbreq { >> char ifbr_ifsname[IFNAMSIZ]; /* member ifs name */ >> u_int32_t ifbr_ifsflags; /* member ifs flags */ >> u_int32_t ifbr_portno; /* member port number */ >> + u_int32_t ifbr_protected; /* protected group number */ >> >> u_int8_t ifbr_state; /* member stp state */ >> u_int8_t ifbr_priority; /* member stp priority */ >> @@ -398,6 +399,7 @@ struct bridge_iflist { >> struct brl_head bif_brlout; /* output rules */ >> struct ifnet *ifp; /* member interface */ >> u_int32_t bif_flags; /* member flags */ >> + u_int32_t bif_protected; /* protected group */ >> void *bif_dhcookie; >> }; >> #define bif_state bif_stp->bp_state >> Index: sys/sys/sockio.h >> =================================================================== >> RCS file: /cvs/src/sys/sys/sockio.h,v >> retrieving revision 1.72 >> diff -u -p -r1.72 sockio.h >> --- sys/sys/sockio.h 24 Oct 2017 09:36:13 -0000 1.72 >> +++ sys/sys/sockio.h 22 Jan 2018 14:09:09 -0000 >> @@ -89,6 +89,7 @@ >> #define SIOCBRDGDADDR _IOW('i', 71, struct ifbareq) /* delete addr >> */ >> #define SIOCBRDGFLUSH _IOW('i', 72, struct ifbreq) /* flush addr >> cache */ >> #define SIOCBRDGADDL _IOW('i', 73, struct ifbreq) /* add local >> port */ >> +#define SIOCBRDGSIFPROT _IOW('i', 74, struct ifbreq) /* set >> protected grp */ >> >> #define SIOCBRDGARL _IOW('i', 77, struct ifbrlreq) /* add bridge rule */ >> #define SIOCBRDGFRL _IOW('i', 78, struct ifbrlreq) /* flush brdg rules */ >> Index: sbin/ifconfig/brconfig.c >> =================================================================== >> RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v >> retrieving revision 1.16 >> diff -u -p -r1.16 brconfig.c >> --- sbin/ifconfig/brconfig.c 31 Jul 2017 02:32:11 -0000 1.16 >> +++ sbin/ifconfig/brconfig.c 22 Jan 2018 14:30:38 -0000 >> @@ -336,6 +336,8 @@ bridge_list(char *delim) >> printf("port %u ifpriority %u ifcost %u", >> reqp->ifbr_portno, reqp->ifbr_priority, >> reqp->ifbr_path_cost); >> + if (reqp->ifbr_protected) >> + printf(" protected %u", reqp->ifbr_protected); >> if (reqp->ifbr_ifsflags & IFBIF_STP) >> printf(" %s role %s", >> stpstates[reqp->ifbr_state], >> @@ -452,6 +454,41 @@ bridge_priority(const char *arg, int d) >> bp.ifbrp_prio = v; >> if (ioctl(s, SIOCBRDGSPRI, (caddr_t)&bp) < 0) >> err(1, "%s", name); >> +} >> + >> +void >> +bridge_protect(const char *ifname, const char *val) >> +{ >> + struct ifbreq breq; >> + unsigned long v; >> + char *endptr; >> + >> + strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name)); >> + strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname)); >> + >> + errno = 0; >> + v = strtoul(val, &endptr, 0); >> + if (val[0] == '\0' || endptr[0] != '\0' || v == 0 || v > UINT32_MAX || >> + (errno == ERANGE && v == ULONG_MAX)) >> + err(1, "invalid arg for protected group: %s", val); >> + breq.ifbr_protected = v; >> + >> + if (ioctl(s, SIOCBRDGSIFPROT, (caddr_t)&breq) < 0) >> + err(1, "%s: %s", name, val); >> +} >> + >> +void >> +bridge_unprotect(const char *ifname, int d) >> +{ >> + struct ifbreq breq; >> + >> + strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name)); >> + strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname)); >> + >> + breq.ifbr_protected = 0; >> + >> + if (ioctl(s, SIOCBRDGSIFPROT, (caddr_t)&breq) < 0) >> + err(1, "%s: %d", name, 0); >> } >> >> void >> Index: sbin/ifconfig/brconfig.h >> =================================================================== >> RCS file: /cvs/src/sbin/ifconfig/brconfig.h,v >> retrieving revision 1.12 >> diff -u -p -r1.12 brconfig.h >> --- sbin/ifconfig/brconfig.h 16 Jan 2018 10:33:55 -0000 1.12 >> +++ sbin/ifconfig/brconfig.h 22 Jan 2018 14:12:39 -0000 >> @@ -52,6 +52,8 @@ void bridge_addrs(const char *, int); >> void bridge_hellotime(const char *, int); >> void bridge_fwddelay(const char *, int); >> void bridge_maxage(const char *, int); >> +void bridge_protect(const char *, const char *); >> +void bridge_unprotect(const char *, int); >> void bridge_proto(const char *, int); >> void bridge_ifprio(const char *, const char *); >> void bridge_ifcost(const char *, const char *); >> Index: sbin/ifconfig/ifconfig.8 >> =================================================================== >> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v >> retrieving revision 1.292 >> diff -u -p -r1.292 ifconfig.8 >> --- sbin/ifconfig/ifconfig.8 16 Jan 2018 10:33:55 -0000 1.292 >> +++ sbin/ifconfig/ifconfig.8 22 Jan 2018 15:11:32 -0000 >> @@ -662,6 +662,13 @@ The default is 100 entries. >> .It Cm maxage Ar time >> Set the time (in seconds) that a spanning tree protocol configuration is >> valid. >> Defaults to 20 seconds, minimum of 6, maximum of 40. >> +.It Cm protected Ar interface Ar id >> +Put >> +.Ar interface >> +in protected mode. >> +A protected interface does not forward traffic to any other protected >> interface >> +having the same >> +.Ar id . >> .It Cm proto Ar value >> Force the spanning tree protocol version. >> The available values are >> Index: sbin/ifconfig/ifconfig.c >> =================================================================== >> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v >> retrieving revision 1.353 >> diff -u -p -r1.353 ifconfig.c >> --- sbin/ifconfig/ifconfig.c 16 Jan 2018 10:33:55 -0000 1.353 >> +++ sbin/ifconfig/ifconfig.c 22 Jan 2018 14:17:17 -0000 >> @@ -472,6 +472,8 @@ const struct cmd { >> { "-edge", NEXTARG, 0, unsetedge }, >> { "autoedge", NEXTARG, 0, setautoedge }, >> { "-autoedge", NEXTARG, 0, unsetautoedge }, >> + { "protected", NEXTARG2, 0, NULL, bridge_protect }, >> + { "-protected", NEXTARG, 0, bridge_unprotect }, >> { "ptp", NEXTARG, 0, setptp }, >> { "-ptp", NEXTARG, 0, unsetptp }, >> { "autoptp", NEXTARG, 0, setautoptp }, > -- Kindest regards, Tom Smyth Mobile: +353 87 6193172 The information contained in this E-mail is intended only for the confidential use of the named recipient. If the reader of this message is not the intended recipient or the person responsible for delivering it to the recipient, you are hereby notified that you have received this communication in error and that any review, dissemination or copying of this communication is strictly prohibited. If you have received this in error, please notify the sender immediately by telephone at the number above and erase the message You are requested to carry out your own virus check before opening any attachment.
