From b60b74ef04faa5b7f0d8b5aef2ce496f1abe913c Mon Sep 17 00:00:00 2001 From: Rod Hynes Date: Wed, 31 Jul 2019 13:18:51 -0400 Subject: [PATCH 1/3] Update vendored bolt --- vendor/github.com/Psiphon-Labs/bolt/tx.go | 83 +++++++++++++---------- vendor/vendor.json | 6 +- 2 files changed, 51 insertions(+), 38 deletions(-) diff --git a/vendor/github.com/Psiphon-Labs/bolt/tx.go b/vendor/github.com/Psiphon-Labs/bolt/tx.go index 514d00f8d..a5c10046c 100644 --- a/vendor/github.com/Psiphon-Labs/bolt/tx.go +++ b/vendor/github.com/Psiphon-Labs/bolt/tx.go @@ -383,7 +383,16 @@ func (tx *Tx) CopyFile(path string, mode os.FileMode) error { // the same time. func (tx *Tx) Check() <-chan error { ch := make(chan error) - go tx.check(ch) + // [Psiphon] + // This code is modified to use the single-error check function while + // preserving the existing bolt Check API. + go func() { + err := tx.check() + if err != nil { + ch <- err + } + close(ch) + }() return ch } @@ -391,36 +400,23 @@ func (tx *Tx) Check() <-chan error { // SynchronousCheck performs the Check function in the current goroutine, // allowing the caller to recover from any panics or faults. func (tx *Tx) SynchronousCheck() error { - checkErrChannel := make(chan error) - - // tx.check may send multiple errors to the channel, and we must consume them - // all to ensure tx.check terminates. Only the first error is returned from - // SynchronousCheck. - firstErrChannel := make(chan error) - go func() { - var err error - for nextErr := range checkErrChannel { - if err != nil { - err = nextErr - } - } - firstErrChannel <- err - }() - - // Invoke bolt code that may panic/segfault in the current goroutine. - tx.check(checkErrChannel) - - return <-firstErrChannel + return tx.check() } -func (tx *Tx) check(ch chan error) { +// [Psiphon] +// check is modified to stop and return on the first error. This prevents some +// long running loops, perhaps due to looping based on corrupt data, that we +// have observed when testing check against corrupted database files. Since +// Psiphon will recover by resetting (deleting) the datastore on any error, +// more than one error is not useful information in our case. +func (tx *Tx) check() error { // Check if any pages are double freed. freed := make(map[pgid]bool) all := make([]pgid, tx.db.freelist.count()) tx.db.freelist.copyall(all) for _, id := range all { if freed[id] { - ch <- fmt.Errorf("page %d: already freed", id) + return fmt.Errorf("page %d: already freed", id) } freed[id] = true } @@ -434,53 +430,70 @@ func (tx *Tx) check(ch chan error) { } // Recursively check buckets. - tx.checkBucket(&tx.root, reachable, freed, ch) + err := tx.checkBucket(&tx.root, reachable, freed) + if err != nil { + return err + } // Ensure all pages below high water mark are either reachable or freed. for i := pgid(0); i < tx.meta.pgid; i++ { _, isReachable := reachable[i] if !isReachable && !freed[i] { - ch <- fmt.Errorf("page %d: unreachable unfreed", int(i)) + return fmt.Errorf("page %d: unreachable unfreed", int(i)) } } - // Close the channel to signal completion. - close(ch) + return nil } -func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bool, ch chan error) { +// [Psiphon] +// checkBucket is modified to stop and return on the first error. +func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bool) error { // Ignore inline buckets. if b.root == 0 { - return + return nil } + var err error + // Check every page used by this bucket. b.tx.forEachPage(b.root, 0, func(p *page, _ int) { if p.id > tx.meta.pgid { - ch <- fmt.Errorf("page %d: out of bounds: %d", int(p.id), int(b.tx.meta.pgid)) + err = fmt.Errorf("page %d: out of bounds: %d", int(p.id), int(b.tx.meta.pgid)) + return } // Ensure each page is only referenced once. for i := pgid(0); i <= pgid(p.overflow); i++ { var id = p.id + i if _, ok := reachable[id]; ok { - ch <- fmt.Errorf("page %d: multiple references", int(id)) + err = fmt.Errorf("page %d: multiple references", int(id)) + return } reachable[id] = p } // We should only encounter un-freed leaf and branch pages. if freed[p.id] { - ch <- fmt.Errorf("page %d: reachable freed", int(p.id)) + err = fmt.Errorf("page %d: reachable freed", int(p.id)) + return } else if (p.flags&branchPageFlag) == 0 && (p.flags&leafPageFlag) == 0 { - ch <- fmt.Errorf("page %d: invalid type: %s", int(p.id), p.typ()) + err = fmt.Errorf("page %d: invalid type: %s", int(p.id), p.typ()) + return } }) + if err != nil { + return err + } + // Check each bucket within this bucket. - _ = b.ForEach(func(k, v []byte) error { + return b.ForEach(func(k, v []byte) error { if child := b.Bucket(k); child != nil { - tx.checkBucket(child, reachable, freed, ch) + err := tx.checkBucket(child, reachable, freed) + if err != nil { + return err + } } return nil }) diff --git a/vendor/vendor.json b/vendor/vendor.json index 79d8e6734..6c5ac042d 100644 --- a/vendor/vendor.json +++ b/vendor/vendor.json @@ -33,10 +33,10 @@ "revisionTime": "2017-02-28T16:03:01Z" }, { - "checksumSHA1": "v2r1JWhVzxu8q099mzH+VNJ9GiI=", + "checksumSHA1": "I0Gc6+Xq9KQF6ifkVEet1leubD4=", "path": "github.com/Psiphon-Labs/bolt", - "revision": "7494fc3896a47cb6732b87e0b18312cb05ca241a", - "revisionTime": "2019-07-30T19:27:05Z" + "revision": "94750aa2185e6ee4217105064949acace0156564", + "revisionTime": "2019-07-31T17:17:12Z" }, { "checksumSHA1": "d3DwsdacdFn1/KCG/2uPV1PwR3s=", From a1485be9dc8e53d2dc83721fd8947369c3a54786 Mon Sep 17 00:00:00 2001 From: Rod Hynes Date: Fri, 2 Aug 2019 12:37:33 -0400 Subject: [PATCH 2/3] Add fast lookups for traffic rules filters and allowed ports - Also fix bugs in tactics fast lookups --- psiphon/common/tactics/tactics.go | 14 +-- psiphon/common/tactics/tactics_test.go | 4 +- psiphon/server/server_test.go | 7 +- psiphon/server/trafficRules.go | 139 ++++++++++++++++++++++++- psiphon/server/tunnelServer.go | 29 ++---- 5 files changed, 155 insertions(+), 38 deletions(-) diff --git a/psiphon/common/tactics/tactics.go b/psiphon/common/tactics/tactics.go index 2bb328dd1..c5d5769a4 100755 --- a/psiphon/common/tactics/tactics.go +++ b/psiphon/common/tactics/tactics.go @@ -450,8 +450,6 @@ func NewServer( return common.ContextError(err) } - newServer.initLookups() - // Modify actual traffic rules only after validation server.RequestPublicKey = newServer.RequestPublicKey server.RequestPrivateKey = newServer.RequestPrivateKey @@ -459,6 +457,8 @@ func NewServer( server.DefaultTactics = newServer.DefaultTactics server.FilteredTactics = newServer.FilteredTactics + server.initLookups() + server.loaded = true return nil @@ -571,7 +571,7 @@ func (server *Server) Validate() error { return nil } -const lookupThreshold = 5 +const stringLookupThreshold = 5 // initLookups creates map lookups for filters where the number // of string values to compare against exceeds a threshold where @@ -581,17 +581,17 @@ func (server *Server) initLookups() { for _, filteredTactics := range server.FilteredTactics { - if len(filteredTactics.Filter.Regions) >= lookupThreshold { + if len(filteredTactics.Filter.Regions) >= stringLookupThreshold { filteredTactics.Filter.regionLookup = make(map[string]bool) for _, region := range filteredTactics.Filter.Regions { filteredTactics.Filter.regionLookup[region] = true } } - if len(filteredTactics.Filter.ISPs) >= lookupThreshold { - filteredTactics.Filter.regionLookup = make(map[string]bool) + if len(filteredTactics.Filter.ISPs) >= stringLookupThreshold { + filteredTactics.Filter.ispLookup = make(map[string]bool) for _, ISP := range filteredTactics.Filter.ISPs { - filteredTactics.Filter.regionLookup[ISP] = true + filteredTactics.Filter.ispLookup[ISP] = true } } diff --git a/psiphon/common/tactics/tactics_test.go b/psiphon/common/tactics/tactics_test.go index 70cf1e0ec..ab9acdc78 100755 --- a/psiphon/common/tactics/tactics_test.go +++ b/psiphon/common/tactics/tactics_test.go @@ -116,8 +116,8 @@ func TestTactics(t *testing.T) { ] } ` - if lookupThreshold != 5 { - t.Fatalf("unexpected lookupThreshold") + if stringLookupThreshold != 5 { + t.Fatalf("unexpected stringLookupThreshold") } encodedRequestPublicKey, encodedRequestPrivateKey, encodedObfuscatedKey, err := GenerateKeys() diff --git a/psiphon/server/server_test.go b/psiphon/server/server_test.go index 510b3e004..9d26c03cb 100644 --- a/psiphon/server/server_test.go +++ b/psiphon/server/server_test.go @@ -1577,8 +1577,13 @@ func paveTrafficRulesFile( requireAuthorization, deny bool, livenessTestSize int) { + // Test both default and fast lookups + if intLookupThreshold != 10 { + t.Fatalf("unexpected intLookupThreshold") + } + allowTCPPorts := fmt.Sprintf("%d", mockWebServerPort) - allowUDPPorts := "53, 123" + allowUDPPorts := "53, 123, 10001, 10002, 10003, 10004, 10005, 10006, 10007, 10008, 10009, 10010" if deny { allowTCPPorts = "0" diff --git a/psiphon/server/trafficRules.go b/psiphon/server/trafficRules.go index 15560e7ee..93b5ee889 100644 --- a/psiphon/server/trafficRules.go +++ b/psiphon/server/trafficRules.go @@ -147,6 +147,9 @@ type TrafficRulesFilter struct { // must have been revoked. When true, authorizations must have been // revoked. When omitted or false, this field is ignored. AuthorizationsRevoked bool + + regionLookup map[string]bool + ispLookup map[string]bool } // TrafficRules specify the limits placed on client traffic. @@ -218,8 +221,11 @@ type TrafficRules struct { // 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. + // names are not resolved before checking AllowSubnets. AllowSubnets []string + + allowTCPPortsLookup map[int]bool + allowUDPPortsLookup map[int]bool } // RateLimits is a clone of common.RateLimits with pointers @@ -280,6 +286,8 @@ func NewTrafficRulesSet(filename string) (*TrafficRulesSet, error) { set.DefaultRules = newSet.DefaultRules set.FilteredRules = newSet.FilteredRules + set.initLookups() + return nil }) @@ -366,6 +374,58 @@ func (set *TrafficRulesSet) Validate() error { return nil } +const stringLookupThreshold = 5 +const intLookupThreshold = 10 + +// initLookups creates map lookups for filters where the number of string/int +// values to compare against exceeds a threshold where benchmarks show maps +// are faster than looping through a string/int slice. +func (set *TrafficRulesSet) initLookups() { + + initTrafficRulesLookups := func(rules *TrafficRules) { + + if len(rules.AllowTCPPorts) >= intLookupThreshold { + rules.allowTCPPortsLookup = make(map[int]bool) + for _, port := range rules.AllowTCPPorts { + rules.allowTCPPortsLookup[port] = true + } + } + + if len(rules.AllowUDPPorts) >= intLookupThreshold { + rules.allowUDPPortsLookup = make(map[int]bool) + for _, port := range rules.AllowUDPPorts { + rules.allowUDPPortsLookup[port] = true + } + } + } + + initTrafficRulesFilterLookups := func(filter *TrafficRulesFilter) { + + if len(filter.Regions) >= stringLookupThreshold { + filter.regionLookup = make(map[string]bool) + for _, region := range filter.Regions { + filter.regionLookup[region] = true + } + } + + if len(filter.ISPs) >= stringLookupThreshold { + filter.ispLookup = make(map[string]bool) + for _, ISP := range filter.ISPs { + filter.ispLookup[ISP] = true + } + } + } + + initTrafficRulesLookups(&set.DefaultRules) + + for i, _ := range set.FilteredRules { + initTrafficRulesFilterLookups(&set.FilteredRules[i].Filter) + initTrafficRulesLookups(&set.FilteredRules[i].Rules) + } + + // TODO: add lookups for MeekRateLimiter? +} + // 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. @@ -478,14 +538,26 @@ func (set *TrafficRulesSet) GetTrafficRules( } if len(filteredRules.Filter.Regions) > 0 { - if !common.Contains(filteredRules.Filter.Regions, geoIPData.Country) { - continue + if filteredRules.Filter.regionLookup != nil { + if !filteredRules.Filter.regionLookup[geoIPData.Country] { + continue + } + } else { + if !common.Contains(filteredRules.Filter.Regions, geoIPData.Country) { + continue + } } } if len(filteredRules.Filter.ISPs) > 0 { - if !common.Contains(filteredRules.Filter.ISPs, geoIPData.ISP) { - continue + if filteredRules.Filter.ispLookup != nil { + if !filteredRules.Filter.ispLookup[geoIPData.ISP] { + continue + } + } else { + if !common.Contains(filteredRules.Filter.ISPs, geoIPData.ISP) { + continue + } } } @@ -593,10 +665,12 @@ func (set *TrafficRulesSet) GetTrafficRules( if filteredRules.Rules.AllowTCPPorts != nil { trafficRules.AllowTCPPorts = filteredRules.Rules.AllowTCPPorts + trafficRules.allowTCPPortsLookup = filteredRules.Rules.allowTCPPortsLookup } if filteredRules.Rules.AllowUDPPorts != nil { trafficRules.AllowUDPPorts = filteredRules.Rules.AllowUDPPorts + trafficRules.allowUDPPortsLookup = filteredRules.Rules.allowUDPPortsLookup } if filteredRules.Rules.AllowSubnets != nil { @@ -616,6 +690,61 @@ func (set *TrafficRulesSet) GetTrafficRules( return trafficRules } +func (rules *TrafficRules) AllowTCPPort(remoteIP net.IP, port int) bool { + + if len(rules.AllowTCPPorts) == 0 { + return true + } + + if rules.allowTCPPortsLookup != nil { + if rules.allowTCPPortsLookup[port] == true { + return true + } + } else { + for _, allowPort := range rules.AllowTCPPorts { + if port == allowPort { + return true + } + } + } + + return rules.allowSubnet(remoteIP) +} + +func (rules *TrafficRules) AllowUDPPort(remoteIP net.IP, port int) bool { + + if len(rules.AllowUDPPorts) == 0 { + return true + } + + if rules.allowUDPPortsLookup != nil { + if rules.allowUDPPortsLookup[port] == true { + return true + } + } else { + for _, allowPort := range rules.AllowUDPPorts { + if port == allowPort { + return true + } + } + } + + return rules.allowSubnet(remoteIP) +} + +func (rules *TrafficRules) allowSubnet(remoteIP net.IP) bool { + + for _, subnet := range rules.AllowSubnets { + // Note: ignoring error as config has been validated + _, network, _ := net.ParseCIDR(subnet) + if network.Contains(remoteIP) { + return true + } + } + + return false +} + // GetMeekRateLimiterConfig gets a snapshot of the meek rate limiter // configuration values. func (set *TrafficRulesSet) GetMeekRateLimiterConfig() (int, int, []string, []string, int, int) { diff --git a/psiphon/server/tunnelServer.go b/psiphon/server/tunnelServer.go index 81acb60a7..a686b5813 100644 --- a/psiphon/server/tunnelServer.go +++ b/psiphon/server/tunnelServer.go @@ -2632,30 +2632,13 @@ func (sshClient *sshClient) isPortForwardPermitted( // Traffic rules checks. - var allowPorts []int - if portForwardType == portForwardTypeTCP { - allowPorts = sshClient.trafficRules.AllowTCPPorts - } else { - allowPorts = sshClient.trafficRules.AllowUDPPorts - } - - if len(allowPorts) == 0 { - return true - } - - // TODO: faster lookup? - if len(allowPorts) > 0 { - for _, allowPort := range allowPorts { - if port == allowPort { - return true - } + switch portForwardType { + case portForwardTypeTCP: + if sshClient.trafficRules.AllowTCPPort(remoteIP, port) { + return true } - } - - for _, subnet := range sshClient.trafficRules.AllowSubnets { - // Note: ignoring error as config has been validated - _, network, _ := net.ParseCIDR(subnet) - if network.Contains(remoteIP) { + case portForwardTypeUDP: + if sshClient.trafficRules.AllowUDPPort(remoteIP, port) { return true } } From 4c2cbfe56457bc7d0b49a62d79a63103db87e8cb Mon Sep 17 00:00:00 2001 From: Rod Hynes Date: Tue, 6 Aug 2019 11:44:48 -0400 Subject: [PATCH 3/3] In Client Library, use log file facility when EmitDiagnosticNotices is set --- ClientLibrary/clientlib/clientlib.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ClientLibrary/clientlib/clientlib.go b/ClientLibrary/clientlib/clientlib.go index 72b95ac68..d5330b818 100644 --- a/ClientLibrary/clientlib/clientlib.go +++ b/ClientLibrary/clientlib/clientlib.go @@ -154,6 +154,17 @@ func StartTunnel(ctx context.Context, } } + // As Client Library doesn't currently implement callbacks, diagnostic + // notices aren't relayed to the client application. So, when + // EmitDiagnosticNotices is set, initialize the rotating diagnostic log file + // facility. + if config.EmitDiagnosticNotices { + err := psiphon.SetNoticeFiles("", filepath.Join(config.DataStoreDirectory, "diagnostics.log"), 0, 0) + if err != nil { + return nil, common.ContextErrorMsg(err, "failed to initialize diagnostic logging") + } + } + err = psiphon.OpenDataStore(config) if err != nil { return nil, common.ContextErrorMsg(err, "failed to open data store")