Merge pull request #151 from slingamn/limits_refactor.3

refactor connection limits and throttling

irc/client.go

@@ -542,9 +542,7 @@ func (client *Client) destroy() {
 	ipaddr := client.IP()
 	// this check shouldn't be required but eh
 	if ipaddr != nil {
-		client.server.connectionLimitsMutex.Lock()
+		client.server.connectionLimiter.RemoveClient(ipaddr)
-		client.server.connectionLimits.RemoveClient(ipaddr)
-		client.server.connectionLimitsMutex.Unlock()
 	}
 
 	// alert monitors

irc/config.go

@@ -15,6 +15,7 @@ import (
 	"time"
 
 	"code.cloudfoundry.org/bytefmt"
+	"github.com/oragono/oragono/irc/connection_limits"
 	"github.com/oragono/oragono/irc/custime"
 	"github.com/oragono/oragono/irc/logger"
 	"github.com/oragono/oragono/irc/passwd"
@@ -108,29 +109,6 @@ func (conf *OperConfig) PasswordBytes() []byte {
 	return bytes
 }
 
-// ConnectionLimitsConfig controls the automated connection limits.
-type ConnectionLimitsConfig struct {
-	Enabled     bool
-	CidrLenIPv4 int `yaml:"cidr-len-ipv4"`
-	CidrLenIPv6 int `yaml:"cidr-len-ipv6"`
-	IPsPerCidr  int `yaml:"ips-per-subnet"`
-	Exempted    []string
-}
-
-// ConnectionThrottleConfig controls the automated connection throttling.
-type ConnectionThrottleConfig struct {
-	Enabled            bool
-	CidrLenIPv4        int           `yaml:"cidr-len-ipv4"`
-	CidrLenIPv6        int           `yaml:"cidr-len-ipv6"`
-	ConnectionsPerCidr int           `yaml:"max-connections"`
-	DurationString     string        `yaml:"duration"`
-	Duration           time.Duration `yaml:"duration-time"`
-	BanDurationString  string        `yaml:"ban-duration"`
-	BanDuration        time.Duration
-	BanMessage         string `yaml:"ban-message"`
-	Exempted           []string
-}
-
 // LineLenConfig controls line lengths.
 type LineLenConfig struct {
 	Tags int
@@ -184,8 +162,8 @@ type Config struct {
 		ProxyAllowedFrom    []string `yaml:"proxy-allowed-from"`
 		MaxSendQString      string   `yaml:"max-sendq"`
 		MaxSendQBytes       uint64
-		ConnectionLimits   ConnectionLimitsConfig   `yaml:"connection-limits"`
+		ConnectionLimiter   connection_limits.LimiterConfig   `yaml:"connection-limits"`
-		ConnectionThrottle ConnectionThrottleConfig `yaml:"connection-throttling"`
+		ConnectionThrottler connection_limits.ThrottlerConfig `yaml:"connection-throttling"`
 	}
 
 	Datastore struct {
@@ -405,12 +383,12 @@ func LoadConfig(filename string) (config *Config, err error) {
 			return nil, fmt.Errorf("STS port is incorrect, should be 0 if disabled: %d", config.Server.STS.Port)
 		}
 	}
-	if config.Server.ConnectionThrottle.Enabled {
+	if config.Server.ConnectionThrottler.Enabled {
-		config.Server.ConnectionThrottle.Duration, err = time.ParseDuration(config.Server.ConnectionThrottle.DurationString)
+		config.Server.ConnectionThrottler.Duration, err = time.ParseDuration(config.Server.ConnectionThrottler.DurationString)
 		if err != nil {
 			return nil, fmt.Errorf("Could not parse connection-throttle duration: %s", err.Error())
 		}
-		config.Server.ConnectionThrottle.BanDuration, err = time.ParseDuration(config.Server.ConnectionThrottle.BanDurationString)
+		config.Server.ConnectionThrottler.BanDuration, err = time.ParseDuration(config.Server.ConnectionThrottler.BanDurationString)
 		if err != nil {
 			return nil, fmt.Errorf("Could not parse connection-throttle ban-duration: %s", err.Error())
 		}

irc/connection_limits/limiter.go

@@ -1,20 +1,32 @@
 // Copyright (c) 2016-2017 Daniel Oaks <daniel@danieloaks.net>
 // released under the MIT license
 
-package irc
+package connection_limits
 
 import (
 	"errors"
 	"fmt"
 	"net"
+	"sync"
 )
 
+// LimiterConfig controls the automated connection limits.
+type LimiterConfig struct {
+	Enabled     bool
+	CidrLenIPv4 int `yaml:"cidr-len-ipv4"`
+	CidrLenIPv6 int `yaml:"cidr-len-ipv6"`
+	IPsPerCidr  int `yaml:"ips-per-subnet"`
+	Exempted    []string
+}
+
 var (
 	errTooManyClients = errors.New("Too many clients in subnet")
 )
 
-// ConnectionLimits manages the automated client connection limits.
+// Limiter manages the automated client connection limits.
-type ConnectionLimits struct {
+type Limiter struct {
+	sync.Mutex
+
 	enabled  bool
 	ipv4Mask net.IPMask
 	ipv6Mask net.IPMask
@@ -30,7 +42,7 @@ type ConnectionLimits struct {
 }
 
 // maskAddr masks the given IPv4/6 address with our cidr limit masks.
-func (cl *ConnectionLimits) maskAddr(addr net.IP) net.IP {
+func (cl *Limiter) maskAddr(addr net.IP) net.IP {
 	if addr.To4() == nil {
 		// IPv6 addr
 		addr = addr.Mask(cl.ipv6Mask)
@@ -44,7 +56,10 @@ func (cl *ConnectionLimits) maskAddr(addr net.IP) net.IP {
 
 // AddClient adds a client to our population if possible. If we can't, throws an error instead.
 // 'force' is used to add already-existing clients (i.e. ones that are already on the network).
-func (cl *ConnectionLimits) AddClient(addr net.IP, force bool) error {
+func (cl *Limiter) AddClient(addr net.IP, force bool) error {
+	cl.Lock()
+	defer cl.Unlock()
+
 	if !cl.enabled {
 		return nil
 	}
@@ -74,7 +89,10 @@ func (cl *ConnectionLimits) AddClient(addr net.IP, force bool) error {
 }
 
 // RemoveClient removes the given address from our population
-func (cl *ConnectionLimits) RemoveClient(addr net.IP) {
+func (cl *Limiter) RemoveClient(addr net.IP) {
+	cl.Lock()
+	defer cl.Unlock()
+
 	if !cl.enabled {
 		return
 	}
@@ -88,35 +106,48 @@ func (cl *ConnectionLimits) RemoveClient(addr net.IP) {
 	}
 }
 
-// NewConnectionLimits returns a new connection limit handler.
+// NewLimiter returns a new connection limit handler.
-func NewConnectionLimits(config ConnectionLimitsConfig) (*ConnectionLimits, error) {
+// The handler is functional, but disabled; it can be enabled via `ApplyConfig`.
-	var cl ConnectionLimits
+func NewLimiter() *Limiter {
-	cl.enabled = config.Enabled
+	var cl Limiter
 
+	// initialize empty population; all other state is configurable
 	cl.population = make(map[string]int)
-	cl.exemptedIPs = make(map[string]bool)
 
-	cl.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
+	return &cl
-	cl.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
+}
-	// subnetLimit is explicitly NOT capped at a minimum of one.
-	// this is so that CL config can be used to allow ONLY clients from exempted IPs/nets
-	cl.subnetLimit = config.IPsPerCidr
 
+// ApplyConfig atomically applies a config update to a connection limit handler
+func (cl *Limiter) ApplyConfig(config LimiterConfig) error {
 	// assemble exempted nets
+	exemptedIPs := make(map[string]bool)
+	var exemptedNets []net.IPNet
 	for _, cidr := range config.Exempted {
 		ipaddr := net.ParseIP(cidr)
 		_, netaddr, err := net.ParseCIDR(cidr)
 
 		if ipaddr == nil && err != nil {
-			return nil, fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
+			return fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
 		}
 
 		if ipaddr != nil {
-			cl.exemptedIPs[ipaddr.String()] = true
+			exemptedIPs[ipaddr.String()] = true
 		} else {
-			cl.exemptedNets = append(cl.exemptedNets, *netaddr)
+			exemptedNets = append(exemptedNets, *netaddr)
 		}
 	}
 
-	return &cl, nil
+	cl.Lock()
+	defer cl.Unlock()
+
+	cl.enabled = config.Enabled
+	cl.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
+	cl.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
+	// subnetLimit is explicitly NOT capped at a minimum of one.
+	// this is so that CL config can be used to allow ONLY clients from exempted IPs/nets
+	cl.subnetLimit = config.IPsPerCidr
+	cl.exemptedIPs = exemptedIPs
+	cl.exemptedNets = exemptedNets
+
+	return nil
 }

irc/connection_limits/throttler.go

@@ -1,22 +1,39 @@
 // Copyright (c) 2016-2017 Daniel Oaks <daniel@danieloaks.net>
 // released under the MIT license
 
-package irc
+package connection_limits
 
 import (
 	"fmt"
 	"net"
+	"sync"
 	"time"
 )
 
+// ThrottlerConfig controls the automated connection throttling.
+type ThrottlerConfig struct {
+	Enabled            bool
+	CidrLenIPv4        int           `yaml:"cidr-len-ipv4"`
+	CidrLenIPv6        int           `yaml:"cidr-len-ipv6"`
+	ConnectionsPerCidr int           `yaml:"max-connections"`
+	DurationString     string        `yaml:"duration"`
+	Duration           time.Duration `yaml:"duration-time"`
+	BanDurationString  string        `yaml:"ban-duration"`
+	BanDuration        time.Duration
+	BanMessage         string `yaml:"ban-message"`
+	Exempted           []string
+}
+
 // ThrottleDetails holds the connection-throttling details for a subnet/IP.
 type ThrottleDetails struct {
 	Start       time.Time
 	ClientCount int
 }
 
-// ConnectionThrottle manages automated client connection throttling.
+// Throttler manages automated client connection throttling.
-type ConnectionThrottle struct {
+type Throttler struct {
+	sync.RWMutex
+
 	enabled     bool
 	ipv4Mask    net.IPMask
 	ipv6Mask    net.IPMask
@@ -25,9 +42,8 @@ type ConnectionThrottle struct {
 	population  map[string]ThrottleDetails
 
 	// used by the server to ban clients that go over this limit
-	BanDuration     time.Duration
+	banDuration time.Duration
-	BanMessage      string
+	banMessage  string
-	BanMessageBytes []byte
 
 	// exemptedIPs holds IPs that are exempt from limits
 	exemptedIPs map[string]bool
@@ -36,7 +52,7 @@ type ConnectionThrottle struct {
 }
 
 // maskAddr masks the given IPv4/6 address with our cidr limit masks.
-func (ct *ConnectionThrottle) maskAddr(addr net.IP) net.IP {
+func (ct *Throttler) maskAddr(addr net.IP) net.IP {
 	if addr.To4() == nil {
 		// IPv6 addr
 		addr = addr.Mask(ct.ipv6Mask)
@@ -49,7 +65,10 @@ func (ct *ConnectionThrottle) maskAddr(addr net.IP) net.IP {
 }
 
 // ResetFor removes any existing count for the given address.
-func (ct *ConnectionThrottle) ResetFor(addr net.IP) {
+func (ct *Throttler) ResetFor(addr net.IP) {
+	ct.Lock()
+	defer ct.Unlock()
+
 	if !ct.enabled {
 		return
 	}
@@ -61,7 +80,10 @@ func (ct *ConnectionThrottle) ResetFor(addr net.IP) {
 }
 
 // AddClient introduces a new client connection if possible. If we can't, throws an error instead.
-func (ct *ConnectionThrottle) AddClient(addr net.IP) error {
+func (ct *Throttler) AddClient(addr net.IP) error {
+	ct.Lock()
+	defer ct.Unlock()
+
 	if !ct.enabled {
 		return nil
 	}
@@ -97,38 +119,63 @@ func (ct *ConnectionThrottle) AddClient(addr net.IP) error {
 	return nil
 }
 
-// NewConnectionThrottle returns a new client connection throttler.
+func (ct *Throttler) BanDuration() time.Duration {
-func NewConnectionThrottle(config ConnectionThrottleConfig) (*ConnectionThrottle, error) {
+	ct.RLock()
-	var ct ConnectionThrottle
+	defer ct.RUnlock()
-	ct.enabled = config.Enabled
 
+	return ct.banDuration
+}
+
+func (ct *Throttler) BanMessage() string {
+	ct.RLock()
+	defer ct.RUnlock()
+
+	return ct.banMessage
+}
+
+// NewThrottler returns a new client connection throttler.
+// The throttler is functional, but disabled; it can be enabled via `ApplyConfig`.
+func NewThrottler() *Throttler {
+	var ct Throttler
+
+	// initialize empty population; all other state is configurable
 	ct.population = make(map[string]ThrottleDetails)
-	ct.exemptedIPs = make(map[string]bool)
 
-	ct.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
+	return &ct
-	ct.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
+}
-	ct.subnetLimit = config.ConnectionsPerCidr
-
-	ct.duration = config.Duration
-
-	ct.BanDuration = config.BanDuration
-	ct.BanMessage = config.BanMessage
 
+// ApplyConfig atomically applies a config update to a throttler
+func (ct *Throttler) ApplyConfig(config ThrottlerConfig) error {
 	// assemble exempted nets
+	exemptedIPs := make(map[string]bool)
+	var exemptedNets []net.IPNet
 	for _, cidr := range config.Exempted {
 		ipaddr := net.ParseIP(cidr)
 		_, netaddr, err := net.ParseCIDR(cidr)
 
 		if ipaddr == nil && err != nil {
-			return nil, fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
+			return fmt.Errorf("Could not parse exempted IP/network [%s]", cidr)
 		}
 
 		if ipaddr != nil {
-			ct.exemptedIPs[ipaddr.String()] = true
+			exemptedIPs[ipaddr.String()] = true
 		} else {
-			ct.exemptedNets = append(ct.exemptedNets, *netaddr)
+			exemptedNets = append(exemptedNets, *netaddr)
 		}
 	}
 
-	return &ct, nil
+	ct.Lock()
+	defer ct.Unlock()
+
+	ct.enabled = config.Enabled
+	ct.ipv4Mask = net.CIDRMask(config.CidrLenIPv4, 32)
+	ct.ipv6Mask = net.CIDRMask(config.CidrLenIPv6, 128)
+	ct.subnetLimit = config.ConnectionsPerCidr
+	ct.duration = config.Duration
+	ct.banDuration = config.BanDuration
+	ct.banMessage = config.BanMessage
+	ct.exemptedIPs = exemptedIPs
+	ct.exemptedNets = exemptedNets
+
+	return nil
 }

irc/server.go

@@ -24,6 +24,7 @@ import (
 	"github.com/goshuirc/irc-go/ircfmt"
 	"github.com/goshuirc/irc-go/ircmsg"
 	"github.com/oragono/oragono/irc/caps"
+	"github.com/oragono/oragono/irc/connection_limits"
 	"github.com/oragono/oragono/irc/isupport"
 	"github.com/oragono/oragono/irc/logger"
 	"github.com/oragono/oragono/irc/passwd"
@@ -86,10 +87,8 @@ type Server struct {
 	commands                     chan Command
 	configFilename               string
 	configurableStateMutex       sync.RWMutex // generic protection for server state modified by rehash()
-	connectionLimits             *ConnectionLimits
+	connectionLimiter            *connection_limits.Limiter
-	connectionLimitsMutex        sync.Mutex // used when affecting the connection limiter, to make sure rehashing doesn't make things go out-of-whack
+	connectionThrottler          *connection_limits.Throttler
-	connectionThrottle           *ConnectionThrottle
-	connectionThrottleMutex      sync.Mutex // used when affecting the connection limiter, to make sure rehashing doesn't make things go out-of-whack
 	ctime                        time.Time
 	defaultChannelModes          Modes
 	dlines                       *DLineManager
@@ -149,6 +148,8 @@ func NewServer(config *Config, logger *logger.Manager) (*Server, error) {
 		channels:            *NewChannelNameMap(),
 		clients:             NewClientLookupSet(),
 		commands:            make(chan Command),
+		connectionLimiter:   connection_limits.NewLimiter(),
+		connectionThrottler: connection_limits.NewThrottler(),
 		listeners:           make(map[string]*ListenerWrapper),
 		logger:              logger,
 		monitorManager:      NewMonitorManager(),
@@ -297,36 +298,38 @@ func (server *Server) checkBans(ipaddr net.IP) (banned bool, message string) {
 	// check DLINEs
 	isBanned, info := server.dlines.CheckIP(ipaddr)
 	if isBanned {
+		server.logger.Info("localconnect-ip", fmt.Sprintf("Client from %v rejected by d-line", ipaddr))
 		return true, info.BanMessage("You are banned from this server (%s)")
 	}
 
 	// check connection limits
-	server.connectionLimitsMutex.Lock()
+	err := server.connectionLimiter.AddClient(ipaddr, false)
-	err := server.connectionLimits.AddClient(ipaddr, false)
-	server.connectionLimitsMutex.Unlock()
 	if err != nil {
 		// too many connections from one client, tell the client and close the connection
+		server.logger.Info("localconnect-ip", fmt.Sprintf("Client from %v rejected for connection limit", ipaddr))
 		return true, "Too many clients from your network"
 	}
 
 	// check connection throttle
-	server.connectionThrottleMutex.Lock()
+	err = server.connectionThrottler.AddClient(ipaddr)
-	err = server.connectionThrottle.AddClient(ipaddr)
-	server.connectionThrottleMutex.Unlock()
 	if err != nil {
 		// too many connections too quickly from client, tell them and close the connection
+		duration := server.connectionThrottler.BanDuration()
 		length := &IPRestrictTime{
-			Duration: server.connectionThrottle.BanDuration,
+			Duration: duration,
-			Expires:  time.Now().Add(server.connectionThrottle.BanDuration),
+			Expires:  time.Now().Add(duration),
 		}
-		server.dlines.AddIP(ipaddr, length, server.connectionThrottle.BanMessage, "Exceeded automated connection throttle")
+		server.dlines.AddIP(ipaddr, length, server.connectionThrottler.BanMessage(), "Exceeded automated connection throttle")
 
 		// they're DLINE'd for 15 minutes or whatever, so we can reset the connection throttle now,
 		// and once their temporary DLINE is finished they can fill up the throttler again
-		server.connectionThrottle.ResetFor(ipaddr)
+		server.connectionThrottler.ResetFor(ipaddr)
 
 		// this might not show up properly on some clients, but our objective here is just to close it out before it has a load impact on us
-		return true, server.connectionThrottle.BanMessage
+		server.logger.Info(
+			"localconnect-ip",
+			fmt.Sprintf("Client from %v exceeded connection throttle, d-lining for %v", ipaddr, duration))
+		return true, server.connectionThrottler.BanMessage()
 	}
 
 	return false, ""
@@ -1229,18 +1232,6 @@ func (server *Server) applyConfig(config *Config, initial bool) error {
 		return fmt.Errorf("Server name isn't valid [%s]: %s", config.Server.Name, err.Error())
 	}
 
-	// confirm connectionLimits are fine
-	connectionLimits, err := NewConnectionLimits(config.Server.ConnectionLimits)
-	if err != nil {
-		return fmt.Errorf("Error rehashing config file connection-limits: %s", err.Error())
-	}
-
-	// confirm connectionThrottler is fine
-	connectionThrottle, err := NewConnectionThrottle(config.Server.ConnectionThrottle)
-	if err != nil {
-		return fmt.Errorf("Error rehashing config file connection-throttle: %s", err.Error())
-	}
-
 	// confirm operator stuff all exists and is fine
 	operclasses, err := config.OperatorClasses()
 	if err != nil {
@@ -1272,22 +1263,15 @@ func (server *Server) applyConfig(config *Config, initial bool) error {
 	// apply new PROXY command restrictions
 	server.proxyAllowedFrom = config.Server.ProxyAllowedFrom
 
-	// apply new connectionlimits
+	err = server.connectionLimiter.ApplyConfig(config.Server.ConnectionLimiter)
-	server.connectionLimitsMutex.Lock()
+	if err != nil {
-	server.connectionLimits = connectionLimits
+		return err
-	server.connectionThrottleMutex.Lock()
+	}
-	server.connectionThrottle = connectionThrottle
 
-	server.clients.ByNickMutex.RLock()
+	err = server.connectionThrottler.ApplyConfig(config.Server.ConnectionThrottler)
-	for _, client := range server.clients.ByNick {
+	if err != nil {
-		ipaddr := client.IP()
+		return err
-		if ipaddr != nil {
-			server.connectionLimits.AddClient(ipaddr, true)
 	}
-	}
-	server.clients.ByNickMutex.RUnlock()
-	server.connectionThrottleMutex.Unlock()
-	server.connectionLimitsMutex.Unlock()
 
 	// setup new and removed caps
 	addedCaps := caps.NewSet()

oragono.yaml

@@ -71,7 +71,7 @@ server:
 
     # maximum number of connections per subnet
     connection-limits:
-        # whether to throttle limits or not
+        # whether to enforce connection limits or not
         enabled: true
 
         # how wide the cidr should be for IPv4