Skip to content

Commit

Permalink
Allow testing multiple GoBGP instances on localhost.
Browse files Browse the repository at this point in the history 
Currently GoBGP does not accept UPDATE messages with nexthops pointing
to a loopback address. This disallows multiple GoBGP instances from
running at the same time on 127.0.0.0/8.

This PR proposes removing this constraint when the RouterID of the
current GoBGP instance itself resides within the testing subnet of
127.0.0.0/8.
  • Loading branch information
@wenovus @fujita
wenovus authored and fujita committed Oct 15, 2023
1 parent 146b2b8 commit 16a9c95
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 44 deletions.
8 changes: 4 additions & 4 deletions pkg/packet/bgp/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
)

// Validator for BGPUpdate
func ValidateUpdateMsg(m *BGPUpdate, rfs map[RouteFamily]BGPAddPathMode, isEBGP bool, isConfed bool) (bool, error) {
func ValidateUpdateMsg(m *BGPUpdate, rfs map[RouteFamily]BGPAddPathMode, isEBGP bool, isConfed bool, loopbackAllowed bool) (bool, error) {
var strongestError error

eCode := uint8(BGP_ERROR_UPDATE_MESSAGE_ERROR)
Expand All @@ -31,7 +31,7 @@ func ValidateUpdateMsg(m *BGPUpdate, rfs map[RouteFamily]BGPAddPathMode, isEBGP
seen[a.GetType()] = a
newAttrs = append(newAttrs, a)
//check specific path attribute
ok, err := ValidateAttribute(a, rfs, isEBGP, isConfed)
ok, err := ValidateAttribute(a, rfs, isEBGP, isConfed, loopbackAllowed)
if !ok {
msgErr := err.(*MessageError)
if msgErr.ErrorHandling == ERROR_HANDLING_SESSION_RESET {
Expand Down Expand Up @@ -81,7 +81,7 @@ func ValidateUpdateMsg(m *BGPUpdate, rfs map[RouteFamily]BGPAddPathMode, isEBGP
return strongestError == nil, strongestError
}

func ValidateAttribute(a PathAttributeInterface, rfs map[RouteFamily]BGPAddPathMode, isEBGP bool, isConfed bool) (bool, error) {
func ValidateAttribute(a PathAttributeInterface, rfs map[RouteFamily]BGPAddPathMode, isEBGP bool, isConfed bool, loopbackAllowed bool) (bool, error) {
var strongestError error

eCode := uint8(BGP_ERROR_UPDATE_MESSAGE_ERROR)
Expand Down Expand Up @@ -169,7 +169,7 @@ func ValidateAttribute(a PathAttributeInterface, rfs map[RouteFamily]BGPAddPathM
}

//check IP address represents host address
if p.Value.IsLoopback() || isZero(p.Value) || isClassDorE(p.Value) {
if (!loopbackAllowed && p.Value.IsLoopback()) || isZero(p.Value) || isClassDorE(p.Value) {
eMsg := "invalid nexthop address"
data, _ := a.Serialize()
e := NewMessageErrorWithErrorHandling(eCode, eSubCodeBadNextHop, data, getErrorHandlingFromPathAttribute(p.GetType()), nil, eMsg)
Expand Down
101 changes: 62 additions & 39 deletions pkg/packet/bgp/validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,31 +43,31 @@ func bgpupdateV6() *BGPMessage {
func Test_Validate_CapV4(t *testing.T) {
assert := assert.New(t)
message := bgpupdate().Body.(*BGPUpdate)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv6_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv6_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(false, res)
assert.Error(err)

res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
require.NoError(t, err)
assert.Equal(true, res)
}

func Test_Validate_CapV6(t *testing.T) {
assert := assert.New(t)
message := bgpupdateV6().Body.(*BGPUpdate)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv6_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv6_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.NoError(err)
assert.True(res)

res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Error(err)
assert.False(res)
}

func Test_Validate_OK(t *testing.T) {
assert := assert.New(t)
message := bgpupdate().Body.(*BGPUpdate)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(true, res)
assert.NoError(err)

Expand Down Expand Up @@ -155,7 +155,7 @@ func Test_Validate_duplicate_attribute(t *testing.T) {
origin.DecodeFromBytes(originBytes)
message.PathAttributes = append(message.PathAttributes, origin)

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
Expand All @@ -169,7 +169,7 @@ func Test_Validate_mandatory_missing(t *testing.T) {
assert := assert.New(t)
message := bgpupdate().Body.(*BGPUpdate)
message.PathAttributes = message.PathAttributes[1:]
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
Expand All @@ -186,7 +186,7 @@ func Test_Validate_mandatory_missing_nocheck(t *testing.T) {
message.PathAttributes = message.PathAttributes[1:]
message.NLRI = nil

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(true, res)
assert.NoError(err)
}
Expand All @@ -200,7 +200,7 @@ func Test_Validate_invalid_origin(t *testing.T) {
origin.DecodeFromBytes(originBytes)
message.PathAttributes[0] = origin

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
Expand All @@ -222,7 +222,7 @@ func Test_Validate_invalid_nexthop_zero(t *testing.T) {
nexthop.DecodeFromBytes(nexthopBytes)
message.PathAttributes[2] = nexthop

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
Expand All @@ -233,25 +233,48 @@ func Test_Validate_invalid_nexthop_zero(t *testing.T) {
}

func Test_Validate_invalid_nexthop_lo(t *testing.T) {
assert := assert.New(t)
message := bgpupdate().Body.(*BGPUpdate)

// invalid nexthop
addr := net.ParseIP("127.0.0.1").To4()
nexthopBytes := []byte{byte(PathAttrFlags[BGP_ATTR_TYPE_NEXT_HOP]), 3, 4}
nexthopBytes = append(nexthopBytes, addr...)
nexthop := &PathAttributeNextHop{}
nexthop.DecodeFromBytes(nexthopBytes)
message.PathAttributes[2] = nexthop

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
assert.Equal(uint8(BGP_ERROR_UPDATE_MESSAGE_ERROR), e.TypeCode)
assert.Equal(uint8(BGP_ERROR_SUB_INVALID_NEXT_HOP_ATTRIBUTE), e.SubTypeCode)
assert.Equal(ERROR_HANDLING_TREAT_AS_WITHDRAW, e.ErrorHandling)
assert.Equal(nexthopBytes, e.Data)
tests := []struct {
desc string
inLoopbackAllowed bool
wantErr bool
}{{
desc: "loopback-disallowed",
inLoopbackAllowed: false,
wantErr: true,
}, {
desc: "loopback-allowed",
inLoopbackAllowed: true,
wantErr: false,
}}

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
assert := assert.New(t)
message := bgpupdate().Body.(*BGPUpdate)

// invalid nexthop
addr := net.ParseIP("127.0.0.1").To4()
nexthopBytes := []byte{byte(PathAttrFlags[BGP_ATTR_TYPE_NEXT_HOP]), 3, 4}
nexthopBytes = append(nexthopBytes, addr...)
nexthop := &PathAttributeNextHop{}
nexthop.DecodeFromBytes(nexthopBytes)
message.PathAttributes[2] = nexthop

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, tt.inLoopbackAllowed)
if tt.wantErr {
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
assert.Equal(uint8(BGP_ERROR_UPDATE_MESSAGE_ERROR), e.TypeCode)
assert.Equal(uint8(BGP_ERROR_SUB_INVALID_NEXT_HOP_ATTRIBUTE), e.SubTypeCode)
assert.Equal(ERROR_HANDLING_TREAT_AS_WITHDRAW, e.ErrorHandling)
assert.Equal(nexthopBytes, e.Data)
} else {
assert.Equal(true, res)
assert.NoError(err)
}
})
}
}

func Test_Validate_invalid_nexthop_de(t *testing.T) {
Expand All @@ -266,7 +289,7 @@ func Test_Validate_invalid_nexthop_de(t *testing.T) {
nexthop.DecodeFromBytes(nexthopBytes)
message.PathAttributes[2] = nexthop

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
Expand All @@ -287,7 +310,7 @@ func Test_Validate_unrecognized_well_known(t *testing.T) {
unknown.DecodeFromBytes(unknownBytes)
message.PathAttributes = append(message.PathAttributes, unknown)

res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, false, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
Expand All @@ -302,7 +325,7 @@ func Test_Validate_aspath(t *testing.T) {
message := bgpupdate().Body.(*BGPUpdate)

// VALID AS_PATH
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, false)
res, err := ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, false, false)
require.NoError(t, err)
assert.Equal(true, res)

Expand All @@ -321,7 +344,7 @@ func Test_Validate_aspath(t *testing.T) {
}

message.PathAttributes = newAttrs
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, false)
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, false, false)
assert.Equal(false, res)
assert.Error(err)
e := err.(*MessageError)
Expand All @@ -330,7 +353,7 @@ func Test_Validate_aspath(t *testing.T) {
assert.Equal(ERROR_HANDLING_TREAT_AS_WITHDRAW, e.ErrorHandling)
assert.Nil(e.Data)

res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, true)
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, true, false)
assert.Equal(false, res)
assert.Error(err)
e = err.(*MessageError)
Expand All @@ -353,7 +376,7 @@ func Test_Validate_aspath(t *testing.T) {
}

message.PathAttributes = newAttrs
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, false)
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, false, false)
assert.Equal(false, res)
assert.Error(err)
e = err.(*MessageError)
Expand All @@ -362,7 +385,7 @@ func Test_Validate_aspath(t *testing.T) {
assert.Equal(ERROR_HANDLING_TREAT_AS_WITHDRAW, e.ErrorHandling)
assert.Nil(e.Data)

res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, true)
res, err = ValidateUpdateMsg(message, map[RouteFamily]BGPAddPathMode{RF_IPv4_UC: BGP_ADD_PATH_BOTH}, true, true, false)
require.NoError(t, err)
assert.Equal(true, res)
}
Expand Down Expand Up @@ -393,7 +416,7 @@ func Test_Validate_flowspec(t *testing.T) {
n1 := NewFlowSpecIPv4Unicast(cmp)
a := NewPathAttributeMpReachNLRI("", []AddrPrefixInterface{n1})
m := map[RouteFamily]BGPAddPathMode{RF_FS_IPv4_UC: BGP_ADD_PATH_NONE}
_, err := ValidateAttribute(a, m, false, false)
_, err := ValidateAttribute(a, m, false, false, false)
assert.Nil(err)

cmp = make([]FlowSpecComponentInterface, 0)
Expand All @@ -403,7 +426,7 @@ func Test_Validate_flowspec(t *testing.T) {
a = NewPathAttributeMpReachNLRI("", []AddrPrefixInterface{n1})
// Swaps components order to reproduce the rules order violation.
n1.Value[0], n1.Value[1] = n1.Value[1], n1.Value[0]
_, err = ValidateAttribute(a, m, false, false)
_, err = ValidateAttribute(a, m, false, false, false)
assert.NotNil(err)
}

Expand All @@ -417,7 +440,7 @@ func TestValidateLargeCommunities(t *testing.T) {
assert.Nil(err)
a := NewPathAttributeLargeCommunities([]*LargeCommunity{c1, c2, c3})
assert.True(len(a.Values) == 3)
_, err = ValidateAttribute(a, nil, false, false)
_, err = ValidateAttribute(a, nil, false, false, false)
assert.Nil(err)
assert.True(len(a.Values) == 2)
}
Expand Down
12 changes: 11 additions & 1 deletion pkg/server/fsm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"io"
"math/rand"
"net"
"net/netip"
"os"
"strconv"
"sync"
Expand Down Expand Up @@ -1074,7 +1075,16 @@ func (h *fsmHandler) recvMessageWithError() (*fsmMsg, error) {
h.fsm.lock.RLock()
rfMap := h.fsm.rfMap
h.fsm.lock.RUnlock()
ok, err := bgp.ValidateUpdateMsg(body, rfMap, isEBGP, isConfed)

// Allow updates from loopback addresses if the GoBGP instance
// itself is assigned to 127.0.0.0/8, since this can happen when
// testing, where multiple GoBGP instances might be created within
// 127.0.0.0/8.
var allowLoopback bool
if routerIDAddr, err := netip.ParseAddr(h.fsm.gConf.Config.RouterId); err == nil && routerIDAddr.Is4() {
allowLoopback = routerIDAddr.IsLoopback()
}
ok, err := bgp.ValidateUpdateMsg(body, rfMap, isEBGP, isConfed, allowLoopback)
if !ok {
handling = h.handlingError(m, err, useRevisedError)
}
Expand Down

0 comments on commit 16a9c95

Please sign in to comment.