stop autocreating d-lines for throttle violations

This didn't work correctly for IPv6 or custom nets.
/UNDLINE IP can temporarily be used to reset the throttle.

default.yaml

@@ -247,9 +247,6 @@ server:
         window: 10m
         # maximum number of new connections per IP/CIDR within the given duration
         max-connections-per-window: 32
-        # how long to ban offenders for. after banning them, the number of connections is
-        # reset, which lets you use /UNDLINE to unban people
-        throttle-ban-duration: 10m
 
         # how wide the CIDR should be for IPv4 (a /32 is a fully specified IPv4 address)
         cidr-len-ipv4: 32

irc/client.go

@@ -21,6 +21,7 @@ import (
 	ident "github.com/oragono/go-ident"
 	"github.com/oragono/oragono/irc/caps"
 	"github.com/oragono/oragono/irc/connection_limits"
+	"github.com/oragono/oragono/irc/flatip"
 	"github.com/oragono/oragono/irc/history"
 	"github.com/oragono/oragono/irc/modes"
 	"github.com/oragono/oragono/irc/sno"
@@ -1477,7 +1478,7 @@ func (client *Client) destroy(session *Session) {
 			if session.proxiedIP != nil {
 				ip = session.proxiedIP
 			}
-			client.server.connectionLimiter.RemoveClient(ip)
+			client.server.connectionLimiter.RemoveClient(flatip.FromNetIP(ip))
 			source = ip.String()
 		}
 		client.server.logger.Info("connect-ip", fmt.Sprintf("disconnecting session of %s from %s", details.nick, source))

irc/connection_limits/limiter.go

@@ -7,7 +7,6 @@ import (
 	"crypto/md5"
 	"errors"
 	"fmt"
-	"net"
 	"sync"
 	"time"
 
@@ -48,8 +47,7 @@ type rawLimiterConfig struct {
 
 	Throttle     bool
 	Window       time.Duration
-	MaxPerWindow int           `yaml:"max-connections-per-window"`
+	MaxPerWindow int `yaml:"max-connections-per-window"`
-	BanDuration  time.Duration `yaml:"throttle-ban-duration"`
 
 	CidrLenIPv4 int `yaml:"cidr-len-ipv4"`
 	CidrLenIPv6 int `yaml:"cidr-len-ipv6"`
@@ -126,44 +124,49 @@ type Limiter struct {
 
 // addrToKey canonicalizes `addr` to a string key, and returns
 // the relevant connection limit and throttle max-per-window values
-func (cl *Limiter) addrToKey(flat flatip.IP) (key limiterKey, limit int, throttle int) {
+func (cl *Limiter) addrToKey(addr flatip.IP) (key limiterKey, limit int, throttle int) {
 	for _, custom := range cl.config.customLimits {
 		for _, net := range custom.nets {
-			if net.Contains(flat) {
+			if net.Contains(addr) {
 				return limiterKey{maskedIP: custom.name, prefixLen: 0}, custom.maxConcurrent, custom.maxPerWindow
 			}
 		}
 	}
 
 	var prefixLen int
-	if flat.IsIPv4() {
+	if addr.IsIPv4() {
 		prefixLen = cl.config.CidrLenIPv4
-		flat = flat.Mask(prefixLen, 32)
+		addr = addr.Mask(prefixLen, 32)
 		prefixLen += 96
 	} else {
 		prefixLen = cl.config.CidrLenIPv6
-		flat = flat.Mask(prefixLen, 128)
+		addr = addr.Mask(prefixLen, 128)
 	}
 
-	return limiterKey{maskedIP: flat, prefixLen: uint8(prefixLen)}, cl.config.MaxConcurrent, cl.config.MaxPerWindow
+	return limiterKey{maskedIP: addr, prefixLen: uint8(prefixLen)}, cl.config.MaxConcurrent, cl.config.MaxPerWindow
 }
 
 // AddClient adds a client to our population if possible. If we can't, throws an error instead.
-func (cl *Limiter) AddClient(addr net.IP) error {
+func (cl *Limiter) AddClient(addr flatip.IP) error {
-	flat := flatip.FromNetIP(addr)
-
 	cl.Lock()
 	defer cl.Unlock()
 
 	// we don't track populations for exempted addresses or nets - this is by design
-	if flatip.IPInNets(flat, cl.config.exemptedNets) {
+	if flatip.IPInNets(addr, cl.config.exemptedNets) {
 		return nil
 	}
 
-	addrString, maxConcurrent, maxPerWindow := cl.addrToKey(flat)
+	addrString, maxConcurrent, maxPerWindow := cl.addrToKey(addr)
+
+	// check limiter
+	var count int
+	if cl.config.Count {
+		count = cl.limiter[addrString] + 1
+		if count > maxConcurrent {
+			return ErrLimitExceeded
+		}
+	}
 
-	// XXX check throttle first; if we checked limit first and then checked throttle,
-	// we'd have to decrement the limit on an unsuccessful throttle check
 	if cl.config.Throttle {
 		details := cl.throttler[addrString] // retrieve mutable throttle state from the map
 		// add in constant state to process the limiting operation
@@ -175,16 +178,13 @@ func (cl *Limiter) AddClient(addr net.IP) error {
 		throttled, _ := g.Touch()                    // actually check the limit
 		cl.throttler[addrString] = g.ThrottleDetails // store modified mutable state
 		if throttled {
+			// back out the limiter add
 			return ErrThrottleExceeded
 		}
 	}
 
-	// now check limiter
+	// success, record in limiter
 	if cl.config.Count {
-		count := cl.limiter[addrString] + 1
-		if count > maxConcurrent {
-			return ErrLimitExceeded
-		}
 		cl.limiter[addrString] = count
 	}
 
@@ -192,17 +192,15 @@ func (cl *Limiter) AddClient(addr net.IP) error {
 }
 
 // RemoveClient removes the given address from our population
-func (cl *Limiter) RemoveClient(addr net.IP) {
+func (cl *Limiter) RemoveClient(addr flatip.IP) {
-	flat := flatip.FromNetIP(addr)
-
 	cl.Lock()
 	defer cl.Unlock()
 
-	if !cl.config.Count || flatip.IPInNets(flat, cl.config.exemptedNets) {
+	if !cl.config.Count || flatip.IPInNets(addr, cl.config.exemptedNets) {
 		return
 	}
 
-	addrString, _, _ := cl.addrToKey(flat)
+	addrString, _, _ := cl.addrToKey(addr)
 	count := cl.limiter[addrString]
 	count -= 1
 	if count < 0 {
@@ -212,17 +210,15 @@ func (cl *Limiter) RemoveClient(addr net.IP) {
 }
 
 // ResetThrottle resets the throttle count for an IP
-func (cl *Limiter) ResetThrottle(addr net.IP) {
+func (cl *Limiter) ResetThrottle(addr flatip.IP) {
-	flat := flatip.FromNetIP(addr)
-
 	cl.Lock()
 	defer cl.Unlock()
 
-	if !cl.config.Throttle || flatip.IPInNets(flat, cl.config.exemptedNets) {
+	if !cl.config.Throttle || flatip.IPInNets(addr, cl.config.exemptedNets) {
 		return
 	}
 
-	addrString, _, _ := cl.addrToKey(flat)
+	addrString, _, _ := cl.addrToKey(addr)
 	delete(cl.throttler, addrString)
 }
 

irc/connection_limits/limiter_test.go

@@ -5,26 +5,20 @@ package connection_limits
 
 import (
 	"crypto/md5"
-	"net"
 	"testing"
 	"time"
 
 	"github.com/oragono/oragono/irc/flatip"
 )
 
-func easyParseIP(ipstr string) (result net.IP) {
+func easyParseIP(ipstr string) (result flatip.IP) {
-	result = net.ParseIP(ipstr)
+	result, err := flatip.ParseIP(ipstr)
-	if result == nil {
+	if err != nil {
-		panic(ipstr)
+		panic(err)
 	}
 	return
 }
 
-func easyParseFlat(ipstr string) (result flatip.IP) {
-	r1 := easyParseIP(ipstr)
-	return flatip.FromNetIP(r1)
-}
-
 var baseConfig = LimiterConfig{
 	rawLimiterConfig: rawLimiterConfig{
 		Count:         true,
@@ -56,20 +50,20 @@ func TestKeying(t *testing.T) {
 	limiter.ApplyConfig(&config)
 
 	// an ipv4 /32 looks like a /128 to us after applying the 4-in-6 mapping
-	key, maxConc, maxWin := limiter.addrToKey(easyParseFlat("1.1.1.1"))
+	key, maxConc, maxWin := limiter.addrToKey(easyParseIP("1.1.1.1"))
 	assertEqual(key.prefixLen, uint8(128), t)
 	assertEqual(key.maskedIP[12:], []byte{1, 1, 1, 1}, t)
 	assertEqual(maxConc, 4, t)
 	assertEqual(maxWin, 8, t)
 
-	testIPv6 := easyParseFlat("2607:5301:201:3100::7426")
+	testIPv6 := easyParseIP("2607:5301:201:3100::7426")
 	key, maxConc, maxWin = limiter.addrToKey(testIPv6)
 	assertEqual(key.prefixLen, uint8(64), t)
-	assertEqual(key.maskedIP[:], []byte(easyParseIP("2607:5301:201:3100::")), t)
+	assertEqual(flatip.IP(key.maskedIP), easyParseIP("2607:5301:201:3100::"), t)
 	assertEqual(maxConc, 4, t)
 	assertEqual(maxWin, 8, t)
 
-	key, maxConc, maxWin = limiter.addrToKey(easyParseFlat("8.8.4.4"))
+	key, maxConc, maxWin = limiter.addrToKey(easyParseIP("8.8.4.4"))
 	assertEqual(key.prefixLen, uint8(0), t)
 	assertEqual([16]byte(key.maskedIP), md5.Sum([]byte("google")), t)
 	assertEqual(maxConc, 128, t)

irc/connection_limits/throttler_test.go

@@ -4,7 +4,6 @@
 package connection_limits
 
 import (
-	"net"
 	"reflect"
 	"testing"
 	"time"
@@ -83,7 +82,7 @@ func makeTestThrottler(v4len, v6len int) *Limiter {
 
 func TestConnectionThrottle(t *testing.T) {
 	throttler := makeTestThrottler(32, 64)
-	addr := net.ParseIP("8.8.8.8")
+	addr := easyParseIP("8.8.8.8")
 
 	for i := 0; i < 3; i += 1 {
 		err := throttler.AddClient(addr)
@@ -97,14 +96,14 @@ func TestConnectionThrottleIPv6(t *testing.T) {
 	throttler := makeTestThrottler(32, 64)
 
 	var err error
-	err = throttler.AddClient(net.ParseIP("2001:0db8::1"))
+	err = throttler.AddClient(easyParseIP("2001:0db8::1"))
 	assertEqual(err, nil, t)
-	err = throttler.AddClient(net.ParseIP("2001:0db8::2"))
+	err = throttler.AddClient(easyParseIP("2001:0db8::2"))
 	assertEqual(err, nil, t)
-	err = throttler.AddClient(net.ParseIP("2001:0db8::3"))
+	err = throttler.AddClient(easyParseIP("2001:0db8::3"))
 	assertEqual(err, nil, t)
 
-	err = throttler.AddClient(net.ParseIP("2001:0db8::4"))
+	err = throttler.AddClient(easyParseIP("2001:0db8::4"))
 	assertEqual(err, ErrThrottleExceeded, t)
 }
 
@@ -112,13 +111,13 @@ func TestConnectionThrottleIPv4(t *testing.T) {
 	throttler := makeTestThrottler(24, 64)
 
 	var err error
-	err = throttler.AddClient(net.ParseIP("192.168.1.101"))
+	err = throttler.AddClient(easyParseIP("192.168.1.101"))
 	assertEqual(err, nil, t)
-	err = throttler.AddClient(net.ParseIP("192.168.1.102"))
+	err = throttler.AddClient(easyParseIP("192.168.1.102"))
 	assertEqual(err, nil, t)
-	err = throttler.AddClient(net.ParseIP("192.168.1.103"))
+	err = throttler.AddClient(easyParseIP("192.168.1.103"))
 	assertEqual(err, nil, t)
 
-	err = throttler.AddClient(net.ParseIP("192.168.1.104"))
+	err = throttler.AddClient(easyParseIP("192.168.1.104"))
 	assertEqual(err, ErrThrottleExceeded, t)
 }

irc/dline.go

@@ -226,8 +226,7 @@ func (dm *DLineManager) RemoveIP(addr net.IP) error {
 }
 
 // CheckIP returns whether or not an IP address was banned, and how long it is banned for.
-func (dm *DLineManager) CheckIP(netAddr net.IP) (isBanned bool, info IPBanInfo) {
+func (dm *DLineManager) CheckIP(addr flatip.IP) (isBanned bool, info IPBanInfo) {
-	addr := flatip.FromNetIP(netAddr)
 	if addr.IsLoopback() {
 		return // #671
 	}

irc/flatip/adhoc.go

@@ -0,0 +1,33 @@
+// Copyright 2020 Shivaram Lingamneni <slingamn@cs.stanford.edu>
+// Released under the MIT license
+
+package flatip
+
+// begin ad-hoc utilities
+
+// ParseToNormalizedNet attempts to interpret a string either as an IP
+// network in CIDR notation, returning an IPNet, or as an IP address,
+// returning an IPNet that contains only that address.
+func ParseToNormalizedNet(netstr string) (ipnet IPNet, err error) {
+	_, ipnet, err = ParseCIDR(netstr)
+	if err == nil {
+		return
+	}
+	ip, err := ParseIP(netstr)
+	if err == nil {
+		ipnet.IP = ip
+		ipnet.PrefixLen = 128
+	}
+	return
+}
+
+// IPInNets is a convenience function for testing whether an IP is contained
+// in any member of a slice of IPNet's.
+func IPInNets(addr IP, nets []IPNet) bool {
+	for _, net := range nets {
+		if net.Contains(addr) {
+			return true
+		}
+	}
+	return false
+}

irc/flatip/flatip.go

@@ -1,5 +1,6 @@
 // Copyright 2020 Shivaram Lingamneni <slingamn@cs.stanford.edu>
 // Copyright 2009 The Go Authors
+// Released under the MIT license
 
 package flatip
 
@@ -13,6 +14,8 @@ var (
 	v4InV6Prefix = []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff}
 
 	IPv6loopback = IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}
+	IPv6zero     = IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}
+	IPv4zero     = IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0, 0, 0, 0}
 
 	ErrInvalidIPString = errors.New("String could not be interpreted as an IP address")
 )
@@ -20,8 +23,8 @@ var (
 // packed versions of net.IP and net.IPNet; these are pure value types,
 // so they can be compared with == and used as map keys.
 
-// IP is the 128-bit representation of the IPv6 address, using the 4-in-6 mapping
+// IP is a 128-bit representation of an IP address, using the 4-in-6 mapping
-// if necessary:
+// to represent IPv4 addresses.
 type IP [16]byte
 
 // IPNet is a IP network. In a valid value, all bits after PrefixLen are zeroes.
@@ -95,6 +98,10 @@ func (ip IP) IsLoopback() bool {
 	}
 }
 
+func (ip IP) IsUnspecified() bool {
+	return ip == IPv4zero || ip == IPv6zero
+}
+
 func rawCidrMask(length int) (m IP) {
 	n := uint(length)
 	for i := 0; i < 16; i++ {
@@ -176,6 +183,13 @@ func (cidr IPNet) String() string {
 	return ipnet.String()
 }
 
+// IsZero tests whether ipnet is the zero value of an IPNet, 0::0/0.
+// Although this is a valid subnet, it can still be used as a sentinel
+// value in some contexts.
+func (ipnet IPNet) IsZero() bool {
+	return ipnet == IPNet{}
+}
+
 // ParseCIDR parses a string representation of an IP network in CIDR notation,
 // then returns it as an IPNet (along with the original, unmasked address).
 func ParseCIDR(netstr string) (ip IP, ipnet IPNet, err error) {
@@ -186,32 +200,3 @@ func ParseCIDR(netstr string) (ip IP, ipnet IPNet, err error) {
 	}
 	return FromNetIP(nip), FromNetIPNet(*nipnet), nil
 }
-
-// begin ad-hoc utilities
-
-// ParseToNormalizedNet attempts to interpret a string either as an IP
-// network in CIDR notation, returning an IPNet, or as an IP address,
-// returning an IPNet that contains only that address.
-func ParseToNormalizedNet(netstr string) (ipnet IPNet, err error) {
-	_, ipnet, err = ParseCIDR(netstr)
-	if err == nil {
-		return
-	}
-	ip, err := ParseIP(netstr)
-	if err == nil {
-		ipnet.IP = ip
-		ipnet.PrefixLen = 128
-	}
-	return
-}
-
-// IPInNets is a convenience function for testing whether an IP is contained
-// in any member of a slice of IPNet's.
-func IPInNets(addr IP, nets []IPNet) bool {
-	for _, net := range nets {
-		if net.Contains(addr) {
-			return true
-		}
-	}
-	return false
-}

irc/gateways.go

@@ -9,6 +9,7 @@ import (
 	"errors"
 	"net"
 
+	"github.com/oragono/oragono/irc/flatip"
 	"github.com/oragono/oragono/irc/modes"
 	"github.com/oragono/oragono/irc/utils"
 )
@@ -87,7 +88,7 @@ func (client *Client) ApplyProxiedIP(session *Session, proxiedIP net.IP, tls boo
 	}
 	// successfully added a limiter entry for the proxied IP;
 	// remove the entry for the real IP if applicable (#197)
-	client.server.connectionLimiter.RemoveClient(session.realIP)
+	client.server.connectionLimiter.RemoveClient(flatip.FromNetIP(session.realIP))
 
 	// given IP is sane! override the client's current IP
 	client.server.logger.Info("connect-ip", "Accepted proxy IP for client", proxiedIP.String())

irc/handlers.go

@@ -24,6 +24,7 @@ import (
 	"github.com/goshuirc/irc-go/ircmsg"
 	"github.com/oragono/oragono/irc/caps"
 	"github.com/oragono/oragono/irc/custime"
+	"github.com/oragono/oragono/irc/flatip"
 	"github.com/oragono/oragono/irc/history"
 	"github.com/oragono/oragono/irc/jwt"
 	"github.com/oragono/oragono/irc/modes"
@@ -2798,6 +2799,11 @@ func unDLineHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *R
 	// get host
 	hostString := msg.Params[0]
 
+	// TODO(#1447) consolidate this into the "unban" command
+	if flatip, ipErr := flatip.ParseIP(hostString); ipErr == nil {
+		server.connectionLimiter.ResetThrottle(flatip)
+	}
+
 	// check host
 	hostNet, err := utils.NormalizedNetFromString(hostString)
 

irc/server.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/oragono/oragono/irc/caps"
 	"github.com/oragono/oragono/irc/connection_limits"
+	"github.com/oragono/oragono/irc/flatip"
 	"github.com/oragono/oragono/irc/history"
 	"github.com/oragono/oragono/irc/logger"
 	"github.com/oragono/oragono/irc/modes"
@@ -160,31 +161,23 @@ func (server *Server) checkBans(config *Config, ipaddr net.IP, checkScripts bool
 		}
 	}
 
+	flat := flatip.FromNetIP(ipaddr)
+
 	// check DLINEs
-	isBanned, info := server.dlines.CheckIP(ipaddr)
+	isBanned, info := server.dlines.CheckIP(flat)
 	if isBanned {
-		server.logger.Info("connect-ip", fmt.Sprintf("Client from %v rejected by d-line", ipaddr))
+		server.logger.Info("connect-ip", "Client rejected by d-line", ipaddr.String())
 		return true, false, info.BanMessage("You are banned from this server (%s)")
 	}
 
 	// check connection limits
-	err := server.connectionLimiter.AddClient(ipaddr)
+	err := server.connectionLimiter.AddClient(flat)
 	if err == connection_limits.ErrLimitExceeded {
 		// too many connections from one client, tell the client and close the connection
-		server.logger.Info("connect-ip", fmt.Sprintf("Client from %v rejected for connection limit", ipaddr))
+		server.logger.Info("connect-ip", "Client rejected for connection limit", ipaddr.String())
 		return true, false, "Too many clients from your network"
 	} else if err == connection_limits.ErrThrottleExceeded {
-		duration := config.Server.IPLimits.BanDuration
+		server.logger.Info("connect-ip", "Client exceeded connection throttle", ipaddr.String())
-		if duration != 0 {
-			server.dlines.AddIP(ipaddr, duration, throttleMessage,
-				"Exceeded automated connection throttle", "auto.connection.throttler")
-			// 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.connectionLimiter.ResetThrottle(ipaddr)
-		}
-		server.logger.Info(
-			"connect-ip",
-			fmt.Sprintf("Client from %v exceeded connection throttle, d-lining for %v", ipaddr, duration))
 		return true, false, throttleMessage
 	} else if err != nil {
 		server.logger.Warning("internal", "unexpected ban result", err.Error())
@@ -211,7 +204,7 @@ func (server *Server) checkBans(config *Config, ipaddr net.IP, checkScripts bool
 		}
 		if output.Result == IPBanned {
 			// XXX roll back IP connection/throttling addition for the IP
-			server.connectionLimiter.RemoveClient(ipaddr)
+			server.connectionLimiter.RemoveClient(flat)
 			server.logger.Info("connect-ip", "Rejected client due to ip-check-script", ipaddr.String())
 			return true, false, output.BanMessage
 		} else if output.Result == IPRequireSASL {

traditional.yaml

@@ -220,9 +220,6 @@ server:
         window: 10m
         # maximum number of new connections per IP/CIDR within the given duration
         max-connections-per-window: 32
-        # how long to ban offenders for. after banning them, the number of connections is
-        # reset, which lets you use /UNDLINE to unban people
-        throttle-ban-duration: 10m
 
         # how wide the CIDR should be for IPv4 (a /32 is a fully specified IPv4 address)
         cidr-len-ipv4: 32