Merge pull request #9880 from yyforyongyu/accessman

Improve access control in peer connections

server.go

@@ -193,6 +193,23 @@ const (
 	peerStatusProtected
 )
 
+// String returns a human-readable representation of the status code.
+func (p peerAccessStatus) String() string {
+	switch p {
+	case peerStatusRestricted:
+		return "restricted"
+
+	case peerStatusTemporary:
+		return "temporary"
+
+	case peerStatusProtected:
+		return "protected"
+
+	default:
+		return "unknown"
+	}
+}
+
 // peerSlotStatus determines whether a peer gets access to one of our free
 // slots or gets to bypass this safety mechanism.
 type peerSlotStatus struct {
@@ -1891,7 +1908,9 @@ func newServer(ctx context.Context, cfg *Config, listenAddrs []net.Addr,
 		// connection requests when we call NewListener.
 		listeners[i], err = brontide.NewListener(
 			nodeKeyECDH, listenAddr.String(),
-			s.peerAccessMan.checkIncomingConnBanScore,
+			// TODO(yy): remove this check and unify the inbound
+			// connection check inside `InboundPeerConnected`.
+			s.peerAccessMan.checkAcceptIncomingConn,
 		)
 		if err != nil {
 			return nil, err
@@ -4009,22 +4028,6 @@ func (s *server) InboundPeerConnected(conn net.Conn) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	// If the remote node's public key is banned, drop the connection.
-	access, err := s.peerAccessMan.assignPeerPerms(nodePub)
-	if err != nil {
-		// Clean up the persistent peer maps if we're dropping this
-		// connection.
-		s.bannedPersistentPeerConnection(pubStr)
-
-		srvrLog.Debugf("Dropping connection for %x since we are out "+
-			"of restricted-access connection slots: %v.", pubSer,
-			err)
-
-		conn.Close()
-
-		return
-	}
-
 	// If we already have an outbound connection to this peer, then ignore
 	// this new connection.
 	if p, ok := s.outboundPeers[pubStr]; ok {
@@ -4059,7 +4062,7 @@ func (s *server) InboundPeerConnected(conn net.Conn) {
 		// We were unable to locate an existing connection with the
 		// target peer, proceed to connect.
 		s.cancelConnReqs(pubStr, nil)
-		s.peerConnected(conn, nil, true, access)
+		s.peerConnected(conn, nil, true)
 
 	case nil:
 		// We already have a connection with the incoming peer. If the
@@ -4091,7 +4094,7 @@ func (s *server) InboundPeerConnected(conn net.Conn) {
 		s.removePeer(connectedPeer)
 		s.ignorePeerTermination[connectedPeer] = struct{}{}
 		s.scheduledPeerConnection[pubStr] = func() {
-			s.peerConnected(conn, nil, true, access)
+			s.peerConnected(conn, nil, true)
 		}
 	}
 }
@@ -4116,25 +4119,6 @@ func (s *server) OutboundPeerConnected(connReq *connmgr.ConnReq, conn net.Conn)
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	access, err := s.peerAccessMan.assignPeerPerms(nodePub)
-	if err != nil {
-		// Clean up the persistent peer maps if we're dropping this
-		// connection.
-		s.bannedPersistentPeerConnection(pubStr)
-
-		srvrLog.Debugf("Dropping connection for %x since we are out "+
-			"of restricted-access connection slots: %v.", pubSer,
-			err)
-
-		if connReq != nil {
-			s.connMgr.Remove(connReq.ID())
-		}
-
-		conn.Close()
-
-		return
-	}
-
 	// If we already have an inbound connection to this peer, then ignore
 	// this new connection.
 	if p, ok := s.inboundPeers[pubStr]; ok {
@@ -4169,7 +4153,7 @@ func (s *server) OutboundPeerConnected(connReq *connmgr.ConnReq, conn net.Conn)
 		return
 	}
 
-	srvrLog.Infof("Established connection to: %x@%v", pubStr,
+	srvrLog.Infof("Established outbound connection to: %x@%v", pubStr,
 		conn.RemoteAddr())
 
 	if connReq != nil {
@@ -4193,7 +4177,7 @@ func (s *server) OutboundPeerConnected(connReq *connmgr.ConnReq, conn net.Conn)
 	case ErrPeerNotConnected:
 		// We were unable to locate an existing connection with the
 		// target peer, proceed to connect.
-		s.peerConnected(conn, connReq, false, access)
+		s.peerConnected(conn, connReq, false)
 
 	case nil:
 		// We already have a connection with the incoming peer. If the
@@ -4227,7 +4211,7 @@ func (s *server) OutboundPeerConnected(connReq *connmgr.ConnReq, conn net.Conn)
 		s.removePeer(connectedPeer)
 		s.ignorePeerTermination[connectedPeer] = struct{}{}
 		s.scheduledPeerConnection[pubStr] = func() {
-			s.peerConnected(conn, connReq, false, access)
+			s.peerConnected(conn, connReq, false)
 		}
 	}
 }
@@ -4304,43 +4288,48 @@ func (s *server) SubscribeCustomMessages() (*subscribe.Client, error) {
 // notifyOpenChannelPeerEvent updates the access manager's maps and then calls
 // the channelNotifier's NotifyOpenChannelEvent.
 func (s *server) notifyOpenChannelPeerEvent(op wire.OutPoint,
-	remotePub *btcec.PublicKey) error {
+	remotePub *btcec.PublicKey) {
 
 	// Call newOpenChan to update the access manager's maps for this peer.
 	if err := s.peerAccessMan.newOpenChan(remotePub); err != nil {
-		return err
+		srvrLog.Errorf("Failed to update peer[%x] access status after "+
+			"channel[%v] open", remotePub.SerializeCompressed(), op)
 	}
 
 	// Notify subscribers about this open channel event.
 	s.channelNotifier.NotifyOpenChannelEvent(op)
-
-	return nil
 }
 
 // notifyPendingOpenChannelPeerEvent updates the access manager's maps and then
 // calls the channelNotifier's NotifyPendingOpenChannelEvent.
 func (s *server) notifyPendingOpenChannelPeerEvent(op wire.OutPoint,
-	pendingChan *channeldb.OpenChannel, remotePub *btcec.PublicKey) error {
+	pendingChan *channeldb.OpenChannel, remotePub *btcec.PublicKey) {
 
 	// Call newPendingOpenChan to update the access manager's maps for this
 	// peer.
 	if err := s.peerAccessMan.newPendingOpenChan(remotePub); err != nil {
-		return err
+		srvrLog.Errorf("Failed to update peer[%x] access status after "+
+			"channel[%v] pending open",
+			remotePub.SerializeCompressed(), op)
 	}
 
 	// Notify subscribers about this event.
 	s.channelNotifier.NotifyPendingOpenChannelEvent(op, pendingChan)
-
-	return nil
 }
 
 // notifyFundingTimeoutPeerEvent updates the access manager's maps and then
 // calls the channelNotifier's NotifyFundingTimeout.
 func (s *server) notifyFundingTimeoutPeerEvent(op wire.OutPoint,
-	remotePub *btcec.PublicKey) error {
+	remotePub *btcec.PublicKey) {
 
 	// Call newPendingCloseChan to potentially demote the peer.
 	err := s.peerAccessMan.newPendingCloseChan(remotePub)
+	if err != nil {
+		srvrLog.Errorf("Failed to update peer[%x] access status after "+
+			"channel[%v] pending close",
+			remotePub.SerializeCompressed(), op)
+	}
+
 	if errors.Is(err, ErrNoMoreRestrictedAccessSlots) {
 		// If we encounter an error while attempting to disconnect the
 		// peer, log the error.
@@ -4351,8 +4340,6 @@ func (s *server) notifyFundingTimeoutPeerEvent(op wire.OutPoint,
 
 	// Notify subscribers about this event.
 	s.channelNotifier.NotifyFundingTimeout(op)
-
-	return nil
 }
 
 // peerConnected is a function that handles initialization a newly connected
@@ -4360,12 +4347,35 @@ func (s *server) notifyFundingTimeoutPeerEvent(op wire.OutPoint,
 // starting all the goroutines the peer needs to function properly. The inbound
 // boolean should be true if the peer initiated the connection to us.
 func (s *server) peerConnected(conn net.Conn, connReq *connmgr.ConnReq,
-	inbound bool, access peerAccessStatus) {
+	inbound bool) {
 
 	brontideConn := conn.(*brontide.Conn)
 	addr := conn.RemoteAddr()
 	pubKey := brontideConn.RemotePub()
 
+	// Only restrict access for inbound connections, which means if the
+	// remote node's public key is banned or the restricted slots are used
+	// up, we will drop the connection.
+	//
+	// TODO(yy): Consider perform this check in
+	// `peerAccessMan.addPeerAccess`.
+	access, err := s.peerAccessMan.assignPeerPerms(pubKey)
+	if inbound && err != nil {
+		pubSer := pubKey.SerializeCompressed()
+
+		// Clean up the persistent peer maps if we're dropping this
+		// connection.
+		s.bannedPersistentPeerConnection(string(pubSer))
+
+		srvrLog.Debugf("Dropping connection for %x since we are out "+
+			"of restricted-access connection slots: %v.", pubSer,
+			err)
+
+		conn.Close()
+
+		return
+	}
+
 	srvrLog.Infof("Finalizing connection to %x@%s, inbound=%v",
 		pubKey.SerializeCompressed(), addr, inbound)
 
@@ -4503,7 +4513,7 @@ func (s *server) peerConnected(conn net.Conn, connReq *connmgr.ConnReq,
 	p := peer.NewBrontide(pCfg)
 
 	// Update the access manager with the access permission for this peer.
-	s.peerAccessMan.addPeerAccess(pubKey, access)
+	s.peerAccessMan.addPeerAccess(pubKey, access, inbound)
 
 	// TODO(roasbeef): update IP address for link-node
 	//  * also mark last-seen, do it one single transaction?
@@ -4965,7 +4975,8 @@ func (s *server) removePeer(p *peer.Brontide) {
 	}
 
 	// Remove the peer's access permission from the access manager.
-	s.peerAccessMan.removePeerAccess(p.IdentityKey())
+	peerPubStr := string(p.IdentityKey().SerializeCompressed())
+	s.peerAccessMan.removePeerAccess(peerPubStr)
 
 	// Copy the peer's error buffer across to the server if it has any items
 	// in it so that we can restore peer errors across connections.
@@ -4999,7 +5010,11 @@ func (s *server) ConnectToPeer(addr *lnwire.NetAddress,
 
 	// Ensure we're not already connected to this peer.
 	peer, err := s.findPeerByPubStr(targetPub)
-	if err == nil {
+
+	// When there's no error it means we already have a connection with this
+	// peer. If this is a dev environment with the `--unsafeconnect` flag
+	// set, we will ignore the existing connection and continue.
+	if err == nil && !s.cfg.Dev.GetUnsafeConnect() {
 		s.mu.Unlock()
 		return &errPeerAlreadyConnected{peer: peer}
 	}