Merge pull request #255 from rod-hynes/master

psiphond enhancements

.travis.yml

@@ -9,10 +9,10 @@ install:
 - go get -t -d -v ./... && go build -v ./...
 script:
 - cd psiphon
-- go test -v -covermode=count -coverprofile=common.coverprofile ./common
-- go test -v -covermode=count -coverprofile=transferstats.coverprofile ./transferstats
-- go test -v -covermode=count -coverprofile=server.coverprofile ./server
-- go test -v -covermode=count -coverprofile=psiphon.coverprofile
+- go test -race -v -covermode=count -coverprofile=common.coverprofile ./common
+- go test -race -v -covermode=count -coverprofile=transferstats.coverprofile ./transferstats
+- go test -race -v -covermode=count -coverprofile=server.coverprofile ./server
+- go test -race -v -covermode=count -coverprofile=psiphon.coverprofile
 - $HOME/gopath/bin/gover
 - $HOME/gopath/bin/goveralls -coverprofile=gover.coverprofile -service=travis-ci -repotoken $COVERALLS_TOKEN
 before_install:

Server/make.bash

@@ -54,7 +54,7 @@ build_for_linux () {
     exit $?
   fi
 
-  GOOS=linux GOARCH=amd64 go build -ldflags "$LDFLAGS" -o psiphond main.go
+  GOOS=linux GOARCH=amd64 go build -race -ldflags "$LDFLAGS" -o psiphond main.go
   if [ $? != 0 ]; then
     echo "...'go build' failed, exiting"
     exit $?

psiphon/server/config.go

@@ -107,7 +107,7 @@ type Config struct {
 	// HostToConnect and PortToConnect when the client is making a
 	// tunneled connection to the web server. This address is always
 	// exempted from validation against SSH_DISALLOWED_PORT_FORWARD_HOSTS
-	// and AllowTCPPorts/DenyTCPPorts.
+	// and AllowTCPPorts.
 	WebServerPortForwardAddress string
 
 	// WebServerPortForwardRedirectAddress specifies an alternate
@@ -190,8 +190,8 @@ type Config struct {
 	//
 	// The intercept is applied before the port forward destination is
 	// validated against SSH_DISALLOWED_PORT_FORWARD_HOSTS and
-	// AllowTCPPorts/DenyTCPPorts. So the intercept address may be any
-	// otherwise prohibited destination.
+	// AllowTCPPorts. So the intercept address may be any otherwise
+	// prohibited destination.
 	UDPInterceptUdpgwServerAddress string
 
 	// DNSResolverIPAddress specifies the IP address of a DNS server
@@ -206,6 +206,13 @@ type Config struct {
 	// The default, 0, disables load logging.
 	LoadMonitorPeriodSeconds int
 
+	// ProcessProfileOutputDirectory is the path of a directory to which
+	// process profiles will be written when signaled with SIGUSR2. The
+	// files are overwritten on each invocation. When set to the default
+	// value, blank, no profiles are written on SIGUSR2. Profiles include
+	// the default profiles here: https://golang.org/pkg/runtime/pprof/#Profile.
+	ProcessProfileOutputDirectory string
+
 	// TrafficRulesFilename is the path of a file containing a
 	// JSON-encoded TrafficRulesSet, the traffic rules to apply to
 	// Psiphon client tunnels.
@@ -533,8 +540,6 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, error
 			MaxUDPPortForwardCount:                intPtr(32),
 			AllowTCPPorts:                         nil,
 			AllowUDPPorts:                         nil,
-			DenyTCPPorts:                          nil,
-			DenyUDPPorts:                          nil,
 		},
 	}
 

psiphon/server/server_test.go

@@ -659,8 +659,8 @@ func paveTrafficRulesFile(t *testing.T, trafficRulesFilename, sponsorID string,
                 "ReadBytesPerSecond": 16384,
                 "WriteBytesPerSecond": 16384
             },
-            "DenyTCPPorts" : [443],
-            "DenyUDPPorts" : [53]
+            "AllowTCPPorts" : [0],
+            "AllowUDPPorts" : [0]
         },
         "FilteredRules" : [
             {
@@ -675,9 +675,7 @@ func paveTrafficRulesFile(t *testing.T, trafficRulesFilename, sponsorID string,
                         "WriteUnthrottledBytes": 132352
                     },
                     "AllowTCPPorts" : [%s],
-                    "DenyTCPPorts" : [],
-                    "AllowUDPPorts" : [%s],
-                    "DenyUDPPorts" : []
+                    "AllowUDPPorts" : [%s]
                 }
             }
         ]

psiphon/server/services.go

@@ -26,7 +26,9 @@ package server
 import (
 	"os"
 	"os/signal"
+	"path/filepath"
 	"runtime"
+	"runtime/pprof"
 	"sync"
 	"syscall"
 	"time"
@@ -121,7 +123,7 @@ func RunServices(configJSON []byte) error {
 	reloadSupportServicesSignal := make(chan os.Signal, 1)
 	signal.Notify(reloadSupportServicesSignal, syscall.SIGUSR1)
 
-	// SIGUSR2 triggers an immediate load log
+	// SIGUSR2 triggers an immediate load log and optional profile dump
 	logServerLoadSignal := make(chan os.Signal, 1)
 	signal.Notify(logServerLoadSignal, syscall.SIGUSR2)
 
@@ -135,11 +137,17 @@ loop:
 			// Reset traffic rules for established clients to reflect reloaded config
 			// TODO: only update when traffic rules config has changed
 			tunnelServer.ResetAllClientTrafficRules()
+
 		case <-logServerLoadSignal:
+			// Profiles are dumped first to ensure some diagnostics are
+			// available in case logServerLoad deadlocks.
+			dumpProcessProfiles(supportServices.Config)
 			logServerLoad(tunnelServer)
+
 		case <-systemStopSignal:
 			log.WithContext().Info("shutdown by system")
 			break loop
+
 		case err = <-errors:
 			log.WithContextFields(LogFields{"error": err}).Error("service failed")
 			break loop
@@ -152,6 +160,35 @@ loop:
 	return err
 }
 
+func dumpProcessProfiles(config *Config) {
+
+	if config.ProcessProfileOutputDirectory != "" {
+
+		for _, profileName := range []string{
+			"goroutine", "heap", "threadcreate", "block"} {
+
+			fileName := filepath.Join(
+				config.ProcessProfileOutputDirectory, profileName+".profile")
+
+			writer, err := os.OpenFile(
+				fileName, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0666)
+
+			if err == nil {
+				err = pprof.Lookup(profileName).WriteTo(writer, 1)
+				writer.Close()
+			}
+
+			if err != nil {
+				log.WithContextFields(
+					LogFields{
+						"error":       err,
+						"profileName": profileName}).Error("write profile failed")
+			}
+		}
+	}
+
+}
+
 func logServerLoad(server *TunnelServer) {
 
 	// golang runtime stats

psiphon/server/trafficRules.go

@@ -21,7 +21,9 @@ package server
 
 import (
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
+	"net"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 )
@@ -113,15 +115,15 @@ type TrafficRules struct {
 	// in the list are accessible to clients.
 	AllowUDPPorts []int
 
-	// DenyTCPPorts specifies a blacklist of TCP ports that
-	// are not permitted for port forwarding. When set, the
-	// ports in the list are inaccessible to clients.
-	DenyTCPPorts []int
-
-	// DenyUDPPorts specifies a blacklist of UDP ports that
-	// are not permitted for port forwarding. When set, the
-	// ports in the list are inaccessible to clients.
-	DenyUDPPorts []int
+	// AllowSubnets specifies a list of IP address subnets for
+	// which all TCP and UDP ports are allowed. This list is
+	// consulted if a port is disallowed by the AllowTCPPorts
+	// or AllowUDPPorts configuration. Each entry is a IP subnet
+	// in CIDR notation.
+	// Limitation: currently, AllowSubnets only matches port
+	// forwards where the client sends an IP address. Domain
+	// names aren not resolved before checking AllowSubnets.
+	AllowSubnets []string
 }
 
 // RateLimits is a clone of common.RateLimits with pointers
@@ -161,13 +163,18 @@ func NewTrafficRulesSet(filename string) (*TrafficRulesSet, error) {
 				// On error, state remains the same
 				return common.ContextError(err)
 			}
-			err = json.Unmarshal(configJSON, &set)
+			var newSet TrafficRulesSet
+			err = json.Unmarshal(configJSON, &newSet)
+			if err != nil {
+				return common.ContextError(err)
+			}
+			err = newSet.Validate()
 			if err != nil {
-				// On error, state remains the same
-				// (Unmarshal first validates the provided
-				//  JOSN and then populates the interface)
 				return common.ContextError(err)
 			}
+			// Modify actual traffic rules only after validation
+			set.DefaultRules = newSet.DefaultRules
+			set.FilteredRules = newSet.FilteredRules
 			return nil
 		})
 
@@ -179,6 +186,50 @@ func NewTrafficRulesSet(filename string) (*TrafficRulesSet, error) {
 	return set, nil
 }
 
+// Validate checks for correct input formats in a TrafficRulesSet.
+func (set *TrafficRulesSet) Validate() error {
+
+	validateTrafficRules := func(rules *TrafficRules) error {
+		for _, subnet := range rules.AllowSubnets {
+			_, _, err := net.ParseCIDR(subnet)
+			if err != nil {
+				return common.ContextError(
+					fmt.Errorf("invalid subnet: %s %s", subnet, err))
+			}
+		}
+		return nil
+	}
+
+	err := validateTrafficRules(&set.DefaultRules)
+	if err != nil {
+		return common.ContextError(err)
+	}
+
+	for _, filteredRule := range set.FilteredRules {
+
+		for paramName, _ := range filteredRule.Filter.HandshakeParameters {
+			validParamName := false
+			for _, paramSpec := range baseRequestParams {
+				if paramSpec.name == paramName {
+					validParamName = true
+					break
+				}
+			}
+			if !validParamName {
+				return common.ContextError(
+					fmt.Errorf("invalid parameter name: %s", paramName))
+			}
+		}
+
+		err := validateTrafficRules(&filteredRule.Rules)
+		if err != nil {
+			return common.ContextError(err)
+		}
+	}
+
+	return nil
+}
+
 // GetTrafficRules determines the traffic rules for a client based on its attributes.
 // For the return value TrafficRules, all pointer and slice fields are initialized,
 // so nil checks are not required. The caller must not modify the returned TrafficRules.
@@ -250,14 +301,6 @@ func (set *TrafficRulesSet) GetTrafficRules(
 		trafficRules.AllowUDPPorts = make([]int, 0)
 	}
 
-	if trafficRules.DenyTCPPorts == nil {
-		trafficRules.DenyTCPPorts = make([]int, 0)
-	}
-
-	if trafficRules.DenyUDPPorts == nil {
-		trafficRules.DenyUDPPorts = make([]int, 0)
-	}
-
 	// TODO: faster lookup?
 	for _, filteredRules := range set.FilteredRules {
 
@@ -341,14 +384,6 @@ func (set *TrafficRulesSet) GetTrafficRules(
 			trafficRules.AllowUDPPorts = filteredRules.Rules.AllowUDPPorts
 		}
 
-		if filteredRules.Rules.DenyTCPPorts != nil {
-			trafficRules.DenyTCPPorts = filteredRules.Rules.DenyTCPPorts
-		}
-
-		if filteredRules.Rules.DenyUDPPorts != nil {
-			trafficRules.DenyUDPPorts = filteredRules.Rules.DenyUDPPorts
-		}
-
 		break
 	}
 

psiphon/server/tunnelServer.go

@@ -955,14 +955,15 @@ func (sshClient *sshClient) isPortForwardPermitted(
 		return false
 	}
 
-	var allowPorts, denyPorts []int
+	var allowPorts []int
 	if portForwardType == portForwardTypeTCP {
 		allowPorts = sshClient.trafficRules.AllowTCPPorts
-		denyPorts = sshClient.trafficRules.AllowTCPPorts
 	} else {
 		allowPorts = sshClient.trafficRules.AllowUDPPorts
-		denyPorts = sshClient.trafficRules.AllowUDPPorts
+	}
 
+	if len(allowPorts) == 0 {
+		return true
 	}
 
 	// TODO: faster lookup?
@@ -972,18 +973,23 @@ func (sshClient *sshClient) isPortForwardPermitted(
 				return true
 			}
 		}
-		return false
 	}
 
-	if len(denyPorts) > 0 {
-		for _, denyPort := range denyPorts {
-			if port == denyPort {
-				return false
+	// TODO: AllowSubnets won't match when host is a domain.
+	// Callers should resolve domain host before checking
+	// isPortForwardPermitted.
+
+	if ip := net.ParseIP(host); ip != nil {
+		for _, subnet := range sshClient.trafficRules.AllowSubnets {
+			// Note: ignoring error as config has been validated
+			_, network, _ := net.ParseCIDR(subnet)
+			if network.Contains(ip) {
+				return true
 			}
 		}
 	}
 
-	return true
+	return false
 }
 
 func (sshClient *sshClient) isPortForwardLimitExceeded(
@@ -1150,7 +1156,7 @@ func (sshClient *sshClient) handleTCPChannel(
 	select {
 	case result = <-resultChannel:
 	case <-sshClient.stopBroadcast:
-		// Note: may leave dial in progress
+		// Note: may leave dial in progress (TODO: use DialContext to cancel)
 		return
 	}