From 7dfd2c3e1d151b8fbbfe012e07f704d552683c37 Mon Sep 17 00:00:00 2001 From: Sreedhar Yengalasetti Date: Thu, 20 Aug 2026 14:56:09 +0530 Subject: [PATCH 1/2] server: fix wrong message wired into fsmStateReason on bad-peer-AS/invalid-msg handleOpen() builds the actual outgoing NOTIFICATION in notif via bgp.NewBGPNotificationMessage() and correctly sends it on the wire (sendNotification(fsm.conn, notif)), but passed the received OPEN message (m, bound by the outer type switch) into newfsmStateReason(...) instead of notif, for both the bad-peer-AS and generic invalid-message paths. This means fsmStateReason.BGPNotification held a *bgp.BGPOpen rather than a *bgp.BGPNotification for these two transitions, silently diverging from every other FSM path in this file (hold-timer-expired, admin-down) which correctly pass the actual notification. Besides being simply wrong, this also meant bmp.go's BMP Peer Down Notification embedded the wrong message content for a peer torn down via a bad-peer-AS or invalid-message OPEN rejection. Pass notif instead, matching the convention used everywhere else in this file. --- pkg/server/fsm.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/server/fsm.go b/pkg/server/fsm.go index 811e0b469..f961250e2 100644 --- a/pkg/server/fsm.go +++ b/pkg/server/fsm.go @@ -1465,9 +1465,9 @@ func (fsm *fsm) handleOpen(fmsg *fsmMsg) (bgp.FSMState, *fsmStateReason, *bgp.BG err := err.(*bgp.MessageError) notif := bgp.NewBGPNotificationMessage(err.TypeCode, err.SubTypeCode, err.Data) if err.TypeCode == bgp.BGP_ERROR_OPEN_MESSAGE_ERROR && err.SubTypeCode == bgp.BGP_ERROR_SUB_BAD_PEER_AS { - return bgp.BGP_FSM_IDLE, newfsmStateReason(fsmBadPeerAS, m, nil), notif + return bgp.BGP_FSM_IDLE, newfsmStateReason(fsmBadPeerAS, notif, nil), notif } - return bgp.BGP_FSM_IDLE, newfsmStateReason(fsmInvalidMsg, m, nil), notif + return bgp.BGP_FSM_IDLE, newfsmStateReason(fsmInvalidMsg, notif, nil), notif } return bgp.BGP_FSM_OPENCONFIRM, newfsmStateReason(fsmOpenMsgReceived, nil, nil), nil } From 29212f31bc1a0f6b4d9069ecd4dd3a47770e3408 Mon Sep 17 00:00:00 2001 From: Sreedhar Yengalasetti Date: Thu, 20 Aug 2026 14:56:31 +0530 Subject: [PATCH 2/2] api, server: expose raw NOTIFICATION error code/subcode on PeerState MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DisconnectReason/DisconnectMessage (added for peer disconnect reason reporting) describe why a session went down as an enum plus a formatted string, but don't expose the underlying RFC 4271 §6 NOTIFICATION error code/subcode as typed integers -- callers that need the raw numeric values have to fragile-parse the message string. Add PeerState.notification_code / notification_subcode (proto + generated Go), threaded through apiutil.PeerState and the WatchEvent peer-event builder via a new extractNotificationCodeSubcode helper. This is intentionally not gated on any particular DisconnectReason: fsmBadPeerAS, fsmInvalidMsg, fsmHoldTimerExpired, and fsmAdminDown can all carry a real notification, so any of them should surface a numeric code/subcode when one is actually present. Both fields are 0/0 when no notification was involved. Adds a test driving a genuine bad-peer-AS NOTIFICATION exchange between two live *BgpServer instances and asserting the resulting PeerState carries the real RFC 4271 OPEN Message Error / Bad Peer AS code/subcode. --- api/gobgp.pb.go | 35 ++++++++++++-- pkg/apiutil/util.go | 7 +++ pkg/server/server.go | 46 ++++++++++++++----- pkg/server/server_test.go | 96 +++++++++++++++++++++++++++++++++++++++ proto/api/gobgp.proto | 9 ++++ 5 files changed, 177 insertions(+), 16 deletions(-) diff --git a/api/gobgp.pb.go b/api/gobgp.pb.go index a60b72c95..6ec1002d1 100644 --- a/api/gobgp.pb.go +++ b/api/gobgp.pb.go @@ -22,7 +22,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: // protoc-gen-go v1.36.6 -// protoc (unknown) +// protoc v7.35.1 // source: api/gobgp.proto package api @@ -7994,8 +7994,17 @@ type PeerState struct { DisconnectReason PeerState_DisconnectReason `protobuf:"varint,21,opt,name=disconnect_reason,json=disconnectReason,proto3,enum=api.PeerState_DisconnectReason" json:"disconnect_reason,omitempty"` DisconnectMessage string `protobuf:"bytes,22,opt,name=disconnect_message,json=disconnectMessage,proto3" json:"disconnect_message,omitempty"` BfdState *BfdPeerState `protobuf:"bytes,23,opt,name=bfd_state,json=bfdState,proto3" json:"bfd_state,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + // Raw RFC 4271 §6 NOTIFICATION error code/subcode, extracted from the FSM's + // underlying BGP NOTIFICATION whenever one is present. This is not limited + // to DISCONNECT_REASON_NOTIFICATION_SENT/_RECEIVED -- fsmBadPeerAS, + // fsmInvalidMsg, fsmHoldTimerExpired, and fsmAdminDown can all carry a real + // notification too. 0/0 when no notification was involved. Needed because + // disconnect_reason/disconnect_message alone cannot expose these as typed + // integers without fragile string-parsing. + NotificationCode uint32 `protobuf:"varint,24,opt,name=notification_code,json=notificationCode,proto3" json:"notification_code,omitempty"` + NotificationSubcode uint32 `protobuf:"varint,25,opt,name=notification_subcode,json=notificationSubcode,proto3" json:"notification_subcode,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *PeerState) Reset() { @@ -8182,6 +8191,20 @@ func (x *PeerState) GetBfdState() *BfdPeerState { return nil } +func (x *PeerState) GetNotificationCode() uint32 { + if x != nil { + return x.NotificationCode + } + return 0 +} + +func (x *PeerState) GetNotificationSubcode() uint32 { + if x != nil { + return x.NotificationSubcode + } + return 0 +} + type Messages struct { state protoimpl.MessageState `protogen:"open.v1"` Received *Message `protobuf:"bytes,1,opt,name=received,proto3" json:"received,omitempty"` @@ -13661,7 +13684,7 @@ const file_api_gobgp_proto_rawDesc = "" + "\fmultihop_ttl\x18\x02 \x01(\rR\vmultihopTtl\"\x83\x01\n" + "\x0eRouteReflector\x124\n" + "\x16route_reflector_client\x18\x01 \x01(\bR\x14routeReflectorClient\x12;\n" + - "\x1aroute_reflector_cluster_id\x18\x02 \x01(\tR\x17routeReflectorClusterId\"\x98\x0e\n" + + "\x1aroute_reflector_cluster_id\x18\x02 \x01(\tR\x17routeReflectorClusterId\"\xf8\x0e\n" + "\tPeerState\x12#\n" + "\rauth_password\x18\x01 \x01(\tR\fauthPassword\x12 \n" + "\vdescription\x18\x02 \x01(\tR\vdescription\x12\x1b\n" + @@ -13688,7 +13711,9 @@ const file_api_gobgp_proto_rawDesc = "" + "\trouter_id\x18\x14 \x01(\tR\brouterId\x12L\n" + "\x11disconnect_reason\x18\x15 \x01(\x0e2\x1f.api.PeerState.DisconnectReasonR\x10disconnectReason\x12-\n" + "\x12disconnect_message\x18\x16 \x01(\tR\x11disconnectMessage\x12.\n" + - "\tbfd_state\x18\x17 \x01(\v2\x11.api.BfdPeerStateR\bbfdState\"\xd4\x01\n" + + "\tbfd_state\x18\x17 \x01(\v2\x11.api.BfdPeerStateR\bbfdState\x12+\n" + + "\x11notification_code\x18\x18 \x01(\rR\x10notificationCode\x121\n" + + "\x14notification_subcode\x18\x19 \x01(\rR\x13notificationSubcode\"\xd4\x01\n" + "\fSessionState\x12\x1d\n" + "\x19SESSION_STATE_UNSPECIFIED\x10\x00\x12\x16\n" + "\x12SESSION_STATE_IDLE\x10\x01\x12\x19\n" + diff --git a/pkg/apiutil/util.go b/pkg/apiutil/util.go index 41a85e236..91da4a81b 100644 --- a/pkg/apiutil/util.go +++ b/pkg/apiutil/util.go @@ -143,6 +143,13 @@ type PeerState struct { LocalCap []bgp.ParameterCapabilityInterface DisconnectReason api.PeerState_DisconnectReason DisconnectMessage string + // NotificationCode/NotificationSubcode carry the raw RFC 4271 §6 + // NOTIFICATION error code/subcode whenever one is present -- not limited + // to a NOTIFICATION-specific DisconnectReason, since several other FSM + // transitions can carry a real notification too. 0/0 when none was + // involved. + NotificationCode uint32 + NotificationSubcode uint32 } type Transport struct { LocalAddress netip.Addr diff --git a/pkg/server/server.go b/pkg/server/server.go index dc393c786..b9c4006d3 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -4631,6 +4631,27 @@ func convertFSMStateReasonToAPI(stateReason *fsmStateReason) (api.PeerState_Disc return disconnectReason, stateReason.String() } +// extractNotificationCodeSubcode returns the raw RFC 4271 §6 NOTIFICATION +// error code/subcode whenever stateReason carries a real *bgp.BGPNotification +// -- intentionally not gated on stateReason.Type (e.g. fsmNotificationSent/ +// fsmNotificationRecv only): fsmBadPeerAS, fsmInvalidMsg, fsmHoldTimerExpired, +// and fsmAdminDown can all carry a real notification too (see their +// newfsmStateReason call sites in fsm.go), and any of them should surface a +// numeric code/subcode when one is actually present. Returns 0, 0 when no +// notification was involved. +func extractNotificationCodeSubcode(stateReason *fsmStateReason) (uint32, uint32) { + if stateReason == nil || stateReason.BGPNotification == nil { + return 0, 0 + } + + body, ok := stateReason.BGPNotification.Body.(*bgp.BGPNotification) + if !ok { + return 0, 0 + } + + return uint32(body.ErrorCode), uint32(body.ErrorSubcode) +} + func toPathApiUtil(path *table.Path) *apiutil.Path { // Best and SendMaxFiltered are set in ListPath API p := &apiutil.Path{ @@ -4715,6 +4736,7 @@ func (s *BgpServer) WatchEvent(ctx context.Context, callbacks WatchEventMessageC } disconnectReason, disconnectMessage := convertFSMStateReasonToAPI(msg.StateReason) + notificationCode, notificationSubcode := extractNotificationCodeSubcode(msg.StateReason) callbacks.OnPeerUpdate(&apiutil.WatchEventMessage_PeerEvent{ Type: msg.Type, @@ -4727,17 +4749,19 @@ func (s *BgpServer) WatchEvent(ctx context.Context, callbacks WatchEventMessageC PeerGroup: msg.PeerGroup, }, State: apiutil.PeerState{ - PeerASN: msg.PeerAS, - LocalASN: msg.LocalAS, - NeighborAddress: msg.PeerAddress, - SessionState: msg.State, - AdminState: admin_state, - RouterID: msg.PeerID, - PeerGroup: msg.PeerGroup, - RemoteCap: msg.RemoteCap, - LocalCap: msg.LocalCap, - DisconnectReason: disconnectReason, - DisconnectMessage: disconnectMessage, + PeerASN: msg.PeerAS, + LocalASN: msg.LocalAS, + NeighborAddress: msg.PeerAddress, + SessionState: msg.State, + AdminState: admin_state, + RouterID: msg.PeerID, + PeerGroup: msg.PeerGroup, + RemoteCap: msg.RemoteCap, + LocalCap: msg.LocalCap, + DisconnectReason: disconnectReason, + DisconnectMessage: disconnectMessage, + NotificationCode: notificationCode, + NotificationSubcode: notificationSubcode, }, Transport: apiutil.Transport{ LocalAddress: msg.LocalAddress, diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 10e2f831a..f46adb396 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -4444,3 +4444,99 @@ func TestRTCShouldNotAdvertiseVPNRouteWhenRTCIsNotPassImportPolicies(t *testing. require.Never(t, vpnPresentAtS2AdjIn, 10*time.Second, 100*time.Millisecond, "VPN route should not appear at s2 adj-in from s1 after second VPN prefix is added") } + +// TestWatchEventPeerNotificationCodeSubcode verifies PeerState.NotificationCode +// and PeerState.NotificationSubcode -- the raw RFC 4271 §6 NOTIFICATION error +// code/subcode, extracted directly from the FSM's underlying notification. +// DisconnectReason/DisconnectMessage alone cannot expose these as typed +// integers without fragile string-parsing. +func TestWatchEventPeerNotificationCodeSubcode(t *testing.T) { + assert := assert.New(t) + + // s expects its peer to be ASN 99, but the peer that actually connects + // (peerServer, below) is ASN 2 -- a genuine peer-AS mismatch, driving + // fsm.go's opensent() into the fsmBadPeerAS branch, with a real + // NOTIFICATION (OPEN_MESSAGE_ERROR / BAD_PEER_AS) sent to the peer. + s := runNewServer(t, 1, "1.1.1.1", 10182) + defer s.StopBgp(context.Background(), &api.StopBgpRequest{}) + + err := s.AddPeer(context.Background(), &api.AddPeerRequest{ + Peer: &api.Peer{ + Conf: &api.PeerConf{ + NeighborAddress: "127.0.0.1", + PeerAsn: 99, + }, + Transport: &api.Transport{ + PassiveMode: true, + }, + }, + }) + assert.NoError(err) + + peerServer := runNewServer(t, 2, "2.2.2.2", -1) + defer peerServer.StopBgp(context.Background(), &api.StopBgpRequest{}) + + var ( + badPeerASReason api.PeerState_DisconnectReason + badPeerASMessage string + notificationCode, notifSubcode uint32 + once sync.Once + ) + badPeerASCh := make(chan struct{}) + watchCtx, watchCancel := context.WithCancel(context.Background()) + defer watchCancel() + + err = s.WatchEvent(watchCtx, WatchEventMessageCallbacks{ + OnPeerUpdate: func(peer *apiutil.WatchEventMessage_PeerEvent, _ time.Time) { + if peer == nil || peer.Type != apiutil.PEER_EVENT_STATE { + return + } + if api.PeerState_SessionState(int(peer.Peer.State.SessionState)+1) != api.PeerState_SESSION_STATE_IDLE { + return + } + once.Do(func() { + badPeerASReason = peer.Peer.State.DisconnectReason + badPeerASMessage = peer.Peer.State.DisconnectMessage + notificationCode = peer.Peer.State.NotificationCode + notifSubcode = peer.Peer.State.NotificationSubcode + close(badPeerASCh) + }) + }, + }, WatchPeer()) + assert.NoError(err) + + err = peerServer.AddPeer(context.Background(), &api.AddPeerRequest{ + Peer: &api.Peer{ + Conf: &api.PeerConf{ + NeighborAddress: "127.0.0.1", + PeerAsn: 1, + }, + Transport: &api.Transport{ + RemotePort: 10182, + }, + Timers: &api.Timers{ + Config: &api.TimersConfig{ + ConnectRetry: 1, + IdleHoldTimeAfterReset: 1, + }, + }, + }, + }) + assert.NoError(err) + + select { + case <-badPeerASCh: + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for bad-peer-AS IDLE event") + } + + assert.Equal(api.PeerState_DISCONNECT_REASON_BAD_PEER_AS, badPeerASReason) + // fsmBadPeerAS's String() case returns the literal "bad-peer-as" -- + // unlike fsmNotificationSent/fsmNotificationRecv, it does not format the + // raw notification code/subcode into the message string. That's exactly + // why NotificationCode/NotificationSubcode exist as separate fields. + assert.Equal("bad-peer-as", badPeerASMessage) + // RFC 4271 §6: OPEN Message Error (2) / Bad Peer AS (2). + assert.Equal(uint32(bgp.BGP_ERROR_OPEN_MESSAGE_ERROR), notificationCode) + assert.Equal(uint32(bgp.BGP_ERROR_SUB_BAD_PEER_AS), notifSubcode) +} diff --git a/proto/api/gobgp.proto b/proto/api/gobgp.proto index b0354be32..68c509adc 100644 --- a/proto/api/gobgp.proto +++ b/proto/api/gobgp.proto @@ -869,6 +869,15 @@ message PeerState { DisconnectReason disconnect_reason = 21; string disconnect_message = 22; BfdPeerState bfd_state = 23; + // Raw RFC 4271 §6 NOTIFICATION error code/subcode, extracted from the FSM's + // underlying BGP NOTIFICATION whenever one is present. This is not limited + // to DISCONNECT_REASON_NOTIFICATION_SENT/_RECEIVED -- fsmBadPeerAS, + // fsmInvalidMsg, fsmHoldTimerExpired, and fsmAdminDown can all carry a real + // notification too. 0/0 when no notification was involved. Needed because + // disconnect_reason/disconnect_message alone cannot expose these as typed + // integers without fragile string-parsing. + uint32 notification_code = 24; + uint32 notification_subcode = 25; } message Messages {