Merge pull request #210 from rod-hynes/master

Remove fail2ban logging

Server/main.go

@@ -33,12 +33,13 @@ import (
 
 func main() {
 
-	var generateTrafficRulesFilename, generateServerEntryFilename string
-	var generateServerIPaddress, generateServerNetworkInterface string
+	var generateTrafficRulesFilename string
+	var generateServerEntryFilename string
+	var generateLogFilename string
+	var generateServerIPaddress string
+	var generateServerNetworkInterface string
 	var generateWebServerPort int
 	var generateProtocolPorts stringListFlag
-	var generateLogFilename string
-	var generateFail2BanLogFilename string
 	var configFilename string
 
 	flag.StringVar(
@@ -53,6 +54,12 @@ func main() {
 		server.SERVER_ENTRY_FILENAME,
 		"generate with this server entry `filename`")
 
+	flag.StringVar(
+		&generateLogFilename,
+		"logFilename",
+		"",
+		"set application log file name and path; blank for stderr")
+
 	flag.StringVar(
 		&generateServerIPaddress,
 		"ipaddress",
@@ -76,18 +83,6 @@ func main() {
 		"protocol",
 		"generate with `protocol:port`; flag may be repeated to enable multiple protocols")
 
-	flag.StringVar(
-		&generateLogFilename,
-		"logFilename",
-		"",
-		"set application log file name and path; blank for stderr")
-
-	flag.StringVar(
-		&generateFail2BanLogFilename,
-		"fail2BanLogFilename",
-		"",
-		"set Fail2Ban log file name and path; blank for stderr")
-
 	flag.StringVar(
 		&configFilename,
 		"config",
@@ -137,13 +132,12 @@ func main() {
 		configJSON, trafficRulesJSON, encodedServerEntry, err :=
 			server.GenerateConfig(
 				&server.GenerateConfigParams{
+					LogFilename:          generateLogFilename,
 					ServerIPAddress:      serverIPaddress,
 					EnableSSHAPIRequests: true,
 					WebServerPort:        generateWebServerPort,
 					TunnelProtocolPorts:  tunnelProtocolPorts,
 					TrafficRulesFilename: generateTrafficRulesFilename,
-					LogFilename:          generateLogFilename,
-					Fail2BanLogFilename:  generateFail2BanLogFilename,
 				})
 		if err != nil {
 			fmt.Printf("generate failed: %s\n", err)

psiphon/server/api.go

@@ -95,7 +95,6 @@ func handshakeAPIRequestHandler(
 
 	err := validateRequestParams(support, params, baseRequestParams)
 	if err != nil {
-		// TODO: fail2ban?
 		return nil, psiphon.ContextError(err)
 	}
 
@@ -168,7 +167,6 @@ func connectedAPIRequestHandler(
 
 	err := validateRequestParams(support, params, connectedRequestParams)
 	if err != nil {
-		// TODO: fail2ban?
 		return nil, psiphon.ContextError(err)
 	}
 
@@ -214,7 +212,6 @@ func statusAPIRequestHandler(
 
 	err := validateRequestParams(support, params, statusRequestParams)
 	if err != nil {
-		// TODO: fail2ban?
 		return nil, psiphon.ContextError(err)
 	}
 
@@ -330,7 +327,6 @@ func clientVerificationAPIRequestHandler(
 
 	err := validateRequestParams(support, params, baseRequestParams)
 	if err != nil {
-		// TODO: fail2ban?
 		return nil, psiphon.ContextError(err)
 	}
 

psiphon/server/config.go

@@ -70,22 +70,6 @@ type Config struct {
 	// to. When blank, logs are written to stderr.
 	LogFilename string
 
-	// Fail2BanFormat is a string format specifier for the
-	// log message format to use for fail2ban integration for
-	// blocking abusive clients by source IP address.
-	// When set, logs with this format are made if clients fail
-	// to authenticate.
-	// The client's IP address is included with the log message.
-	// An example format specifier, which should be compatible
-	// with default SSH fail2ban configuration, is
-	// "Authentication failure for psiphon-client from %s".
-	Fail2BanFormat string
-
-	// LogFilename specifies the path of the file to log
-	// fail2ban messages to. When blank, logs are written to
-	// stderr.
-	Fail2BanLogFilename string
-
 	// DiscoveryValueHMACKey is the network-wide secret value
 	// used to determine a unique discovery strategy.
 	DiscoveryValueHMACKey string
@@ -226,12 +210,6 @@ func (config *Config) RunLoadMonitor() bool {
 	return config.LoadMonitorPeriodSeconds > 0
 }
 
-// UseFail2Ban indicates whether to log client IP addresses, in authentication
-// failure cases, for use by fail2ban.
-func (config *Config) UseFail2Ban() bool {
-	return config.Fail2BanFormat != ""
-}
-
 // LoadConfig loads and validates a JSON encoded server config.
 func LoadConfig(configJSON []byte) (*Config, error) {
 
@@ -241,10 +219,6 @@ func LoadConfig(configJSON []byte) (*Config, error) {
 		return nil, psiphon.ContextError(err)
 	}
 
-	if config.Fail2BanFormat != "" && strings.Count(config.Fail2BanFormat, "%s") != 1 {
-		return nil, errors.New("Fail2BanFormat must have one '%%s' placeholder")
-	}
-
 	if config.ServerIPAddress == "" {
 		return nil, errors.New("ServerIPAddress is missing from config file")
 	}
@@ -319,13 +293,12 @@ func LoadConfig(configJSON []byte) (*Config, error) {
 // GenerateConfigParams specifies customizations to be applied to
 // a generated server config.
 type GenerateConfigParams struct {
+	LogFilename          string
 	ServerIPAddress      string
 	WebServerPort        int
 	EnableSSHAPIRequests bool
 	TunnelProtocolPorts  map[string]int
 	TrafficRulesFilename string
-	LogFilename          string
-	Fail2BanLogFilename  string
 }
 
 // GenerateConfig creates a new Psiphon server config. It returns JSON
@@ -466,7 +439,7 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, error
 
 	config := &Config{
 		LogLevel:                       "info",
-		Fail2BanFormat:                 "Authentication failure for psiphon-client from %s",
+		LogFilename:                    params.LogFilename,
 		GeoIPDatabaseFilenames:         nil,
 		HostID:                         "example-host-id",
 		ServerIPAddress:                params.ServerIPAddress,
@@ -490,8 +463,6 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, error
 		MeekProxyForwardedForHeaders:   []string{"X-Forwarded-For"},
 		LoadMonitorPeriodSeconds:       300,
 		TrafficRulesFilename:           params.TrafficRulesFilename,
-		LogFilename:                    params.LogFilename,
-		Fail2BanLogFilename:            params.Fail2BanLogFilename,
 	}
 
 	encodedConfig, err := json.MarshalIndent(config, "\n", "    ")

psiphon/server/log.go

@@ -20,7 +20,6 @@
 package server
 
 import (
-	"fmt"
 	"io"
 	"os"
 
@@ -67,24 +66,11 @@ func NewLogWriter() *io.PipeWriter {
 	return log.Writer()
 }
 
-// LogFail2Ban logs a message using the format specified by
-// config.Fail2BanFormat and the given client IP address. This
-// is for integration with fail2ban for blocking abusive
-// clients by source IP address.
-func LogFail2Ban(clientIPAddress string) {
-	fail2BanLogger.Info(
-		fmt.Sprintf(fail2BanFormat, clientIPAddress))
-}
-
 var log *ContextLogger
-var fail2BanFormat string
-var fail2BanLogger *logrus.Logger
 
 // InitLogging configures a logger according to the specified
 // config params. If not called, the default logger set by the
 // package init() is used.
-// When configured, InitLogging also establishes a local syslog
-// logger specifically for fail2ban integration.
 // Concurrenty note: should only be called from the main
 // goroutine.
 func InitLogging(config *Config) error {
@@ -112,30 +98,6 @@ func InitLogging(config *Config) error {
 		},
 	}
 
-	if config.Fail2BanFormat != "" {
-
-		fail2BanFormat = config.Fail2BanFormat
-
-		fail2BanLogWriter := os.Stderr
-
-		if config.Fail2BanLogFilename != "" {
-			logWriter, err = os.OpenFile(
-				config.Fail2BanLogFilename, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0666)
-			if err != nil {
-				return psiphon.ContextError(err)
-			}
-		}
-
-		fail2BanLogger = &logrus.Logger{
-			Out: fail2BanLogWriter,
-			Formatter: &logrus.TextFormatter{
-				DisableColors: true,
-				FullTimestamp: true,
-			},
-			Level: level,
-		}
-	}
-
 	return nil
 }
 

psiphon/server/meek.go

@@ -303,7 +303,7 @@ func (server *MeekServer) getSession(
 	}
 
 	// Determine the client remote address, which is used for geolocation
-	// and stats. When an intermediate proxy of CDN is in use, we may be
+	// and stats. When an intermediate proxy or CDN is in use, we may be
 	// able to determine the original client address by inspecting HTTP
 	// headers such as X-Forwarded-For.
 
@@ -317,7 +317,7 @@ func (server *MeekServer) getSession(
 				// list of IPs (each proxy in a chain). The first IP should be
 				// the client IP.
 				proxyClientIP := strings.Split(header, ",")[0]
-				if net.ParseIP(clientIP) != nil {
+				if net.ParseIP(proxyClientIP) != nil {
 					clientIP = proxyClientIP
 					break
 				}
@@ -331,7 +331,7 @@ func (server *MeekServer) getSession(
 	// meek conn with a useful value to return when the tunnel
 	// server calls conn.RemoteAddr() to get the client's IP address.
 
-	// Assumes clientIP is a value IP address; the port value is a stub
+	// Assumes clientIP is a valid IP address; the port value is a stub
 	// and is expected to be ignored.
 	clientConn := newMeekConn(
 		&net.TCPAddr{

psiphon/server/tunnelServer.go

@@ -618,14 +618,27 @@ func (sshClient *sshClient) authLogCallback(conn ssh.ConnMetadata, method string
 			return
 		}
 
-		if sshClient.sshServer.support.Config.UseFail2Ban() {
-			clientIPAddress := psiphon.IPAddressFromAddr(conn.RemoteAddr())
-			if clientIPAddress != "" {
-				LogFail2Ban(clientIPAddress)
-			}
-		}
+		// Note: here we previously logged messages for fail2ban to act on. This is no longer
+		// done as the complexity outweighs the benefits.
+		//
+		// - The SSH credential is not secret -- it's in the server entry. Attackers targetting
+		//   the server likely already have the credential. On the other hand, random scanning and
+		//   brute forcing is mitigated with high entropy random passwords, rate limiting
+		//   (implemented on the host via iptables), and limited capabilities (the SSH session can
+		//   only port forward).
+		//
+		// - fail2ban coverage was inconsistent; in the case of an unfronted meek protocol through
+		//   an upstream proxy, the remote address is the upstream proxy, which should not be blocked.
+		//   The X-Forwarded-For header cant be used instead as it may be forged and used to get IPs
+		//   deliberately blocked; and in any case fail2ban adds iptables rules which can only block
+		//   by direct remote IP, not by original client IP. Fronted meek has the same iptables issue.
+		//
+		// TODO: random scanning and brute forcing of port 22 will result in log noise. To eliminate
+		// this, and to also cover meek protocols, and bad obfuscation keys, and bad inputs to the web
+		// server, consider implementing fail2ban-type logic directly in this server, with the ability
+		// to use X-Forwarded-For (when trustworthy; e.g, from a CDN).
 
-		log.WithContextFields(LogFields{"error": err, "method": method}).Error("authentication failed")
+		log.WithContextFields(LogFields{"error": err, "method": method}).Warning("authentication failed")
 
 	} else {