From 81810ed5316bd647459e2de49a3e89f315dcb1a4 Mon Sep 17 00:00:00 2001 From: Sarah Jamie Lewis Date: Mon, 27 Sep 2021 14:38:01 -0700 Subject: [PATCH 1/4] Refactor WaitForCapabilityOrClose --- networks/tor/BaseOnionService.go | 116 +++++++++++++++++++++---------- 1 file changed, 81 insertions(+), 35 deletions(-) diff --git a/networks/tor/BaseOnionService.go b/networks/tor/BaseOnionService.go index c3c6383..6d49ec6 100644 --- a/networks/tor/BaseOnionService.go +++ b/networks/tor/BaseOnionService.go @@ -10,6 +10,7 @@ import ( "git.openprivacy.ca/openprivacy/log" "golang.org/x/crypto/ed25519" "sync" + "time" ) // BaseOnionService is a concrete implementation of the service interface over Tor onion services. @@ -62,59 +63,98 @@ func (s *BaseOnionService) SetPort(port int) { // WaitForCapabilityOrClose blocks until the connection has the given capability or the underlying connection is closed // (through error or user action) func (s *BaseOnionService) WaitForCapabilityOrClose(cid string, name tapir.Capability) (tapir.Connection, error) { - conn, err := s.GetConnection(cid) - // If there is at least one active connection, then check the one that is returned and use it - for conn != nil { + attempts := 0 + for { + if attempts > 4 { + s.connections.Range(func(key, value interface{}) bool { + connection := value.(tapir.Connection) + if connection.Hostname() == cid { + if !connection.IsClosed() { + connection.Close() + s.connections.Delete(key) + } + } + return true + }) + log.Debugf("WaitForCapabilityOrClose attempts exceeded for %v all, connections closed", cid) + return nil, errors.New("failed to acquire capability after multiple attempts, forcibly closing all connections with the peer") + } - // if we have only one and it has the right capability then return - if conn.HasCapability(name) && err == nil { - return conn, nil + if attempts > 0 { + // Allow connections to be torn down / closed before checking again + // There is no point in busy looping... + time.Sleep(time.Second * time.Duration(attempts)) + } + + // Increment Attempts + attempts++ + + log.Debugf("Lookup up a connection %v...", cid) + // Lookup the connection... + conn, err := s.GetConnection(cid) + + log.Debugf("Found %v %v", conn, err) + // If there are no active connections then return an error... + if conn == nil { + log.Debugf("Now active connection found for %v", cid) + return nil, err + } + + if err == nil { + // if we have only one connection and it has the desired capability then return the connection with + // no error... + if conn.HasCapability(name) { + return conn, nil + } + + log.Debugf("Found 1 connections for %v, but it lacks the desired capability %v", cid, name) + continue } // If We have 2 connections for the same hostname... if err != nil { log.Debugf("found duplicate connections for %v <-> %v %v", s.id.Hostname(), cid, err) + + inboundCount := 0 + // By convention the lowest lexicographical hostname purges all their outbounds to the higher // hostname // Which should only leave a single connection remaining (as we dedupe on connect too) // This does allow people to attempt to guarantee being the outbound connection but due to the bidirectional // authentication this should never result in an advantage in the protocol. - inboundCount := 0 - if s.id.Hostname() < cid { - // Close all outbound connections to connection + // Close all outbound connections to connection + s.connections.Range(func(key, value interface{}) bool { + connection := value.(tapir.Connection) + if connection.Hostname() == cid { + if !connection.IsClosed() && connection.IsOutbound() && s.id.Hostname() < cid { + connection.Close() + s.connections.Delete(key) + } + + if !connection.IsClosed() && !connection.IsOutbound() { + inboundCount++ + } + } + return true + }) + + // If we have more than 1 inbound count then forcibly close all connections... + // This shouldn't happen honestly, but if it does then it can cause an infinite check here + if inboundCount > 1 { s.connections.Range(func(key, value interface{}) bool { connection := value.(tapir.Connection) if connection.Hostname() == cid { - if !connection.IsClosed() && connection.IsOutbound() { + if !connection.IsClosed() { connection.Close() - } else if !connection.IsClosed() && !connection.IsOutbound() { - inboundCount++ + s.connections.Delete(key) } } return true }) - - // This shouldn't happen honestly, but if it does then it can cause an infinite check here - if inboundCount > 1 { - s.connections.Range(func(key, value interface{}) bool { - connection := value.(tapir.Connection) - if connection.Hostname() == cid { - if !connection.IsClosed() { - connection.Close() - } - } - return true - }) - return nil, errors.New("multiple inbound connections found and closed; the only resolution to this is to close them all and try connecting again") - } + return nil, errors.New("multiple inbound connections found and closed; the only resolution to this is to close them all and try connecting again") } } - - // We had a connection but it didn't have a capability...try again - conn, err = s.GetConnection(cid) } - // There are no active connections with that hostname - return nil, err } // GetConnection returns a connection for a given hostname. @@ -125,11 +165,14 @@ func (s *BaseOnionService) GetConnection(hostname string) (tapir.Connection, err if connection.Hostname() == hostname { if !connection.IsClosed() { conn = append(conn, connection) - return true + } else { + // Delete this Closed Connection + s.connections.Delete(key) } } return true }) + if len(conn) == 0 { return nil, errors.New("no connection found") } @@ -143,7 +186,9 @@ func (s *BaseOnionService) GetConnection(hostname string) (tapir.Connection, err // Connect initializes a new outbound connection to the given peer, using the defined Application func (s *BaseOnionService) Connect(hostname string, app tapir.Application) (bool, error) { - currconn, _ := s.GetConnection(hostname) + currconn, err := s.GetConnection(hostname) + + // We already have a connection if currconn != nil { // Note: This check is not 100% reliable. And we may end up with two connections between peers // This can happen when a client connects to a server as the server is connecting to the client @@ -152,6 +197,7 @@ func (s *BaseOnionService) Connect(hostname string, app tapir.Application) (bool // We mitigate this by performing multiple checks when Connect'ing return true, errors.New("already connected to " + hostname) } + // connects to a remote server // spins off to a connection struct log.Debugf("Connecting to %v", hostname) @@ -162,8 +208,8 @@ func (s *BaseOnionService) Connect(hostname string, app tapir.Application) (bool // Second check. If we didn't catch a double connection attempt before the Open we *should* catch it now because // the auth protocol is quick and Open over onion connections can take some time. // Again this isn't 100% reliable. - _, err := s.GetConnection(hostname) - if err == nil { + tconn, _ := s.GetConnection(hostname) + if tconn != nil { conn.Close() return true, errors.New("already connected to " + hostname) } From f1fe281ab42cf36d048069ba3363e596924d4f28 Mon Sep 17 00:00:00 2001 From: Sarah Jamie Lewis Date: Mon, 27 Sep 2021 14:43:35 -0700 Subject: [PATCH 2/4] Fixup debug messages --- networks/tor/BaseOnionService.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/networks/tor/BaseOnionService.go b/networks/tor/BaseOnionService.go index 6d49ec6..f1930f2 100644 --- a/networks/tor/BaseOnionService.go +++ b/networks/tor/BaseOnionService.go @@ -76,7 +76,7 @@ func (s *BaseOnionService) WaitForCapabilityOrClose(cid string, name tapir.Capab } return true }) - log.Debugf("WaitForCapabilityOrClose attempts exceeded for %v all, connections closed", cid) + log.Debugf("WaitForCapabilityOrClose attempts exceeded for %v, all connections closed", cid) return nil, errors.New("failed to acquire capability after multiple attempts, forcibly closing all connections with the peer") } @@ -96,7 +96,7 @@ func (s *BaseOnionService) WaitForCapabilityOrClose(cid string, name tapir.Capab log.Debugf("Found %v %v", conn, err) // If there are no active connections then return an error... if conn == nil { - log.Debugf("Now active connection found for %v", cid) + log.Debugf("no active connection found for %v", cid) return nil, err } From 12156065c3106fd062b08b903b294e7296c14550 Mon Sep 17 00:00:00 2001 From: Sarah Jamie Lewis Date: Mon, 27 Sep 2021 14:50:42 -0700 Subject: [PATCH 3/4] Upgrade Logging --- go.mod | 2 +- networks/tor/BaseOnionService.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 91690cc..f6204f5 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,7 @@ module git.openprivacy.ca/cwtch.im/tapir require ( git.openprivacy.ca/openprivacy/connectivity v1.4.5 - git.openprivacy.ca/openprivacy/log v1.0.2 + git.openprivacy.ca/openprivacy/log v1.0.3 github.com/davecgh/go-spew v1.1.1 // indirect github.com/gtank/merlin v0.1.1 github.com/gtank/ristretto255 v0.1.2 diff --git a/networks/tor/BaseOnionService.go b/networks/tor/BaseOnionService.go index f1930f2..b412ab2 100644 --- a/networks/tor/BaseOnionService.go +++ b/networks/tor/BaseOnionService.go @@ -186,7 +186,7 @@ func (s *BaseOnionService) GetConnection(hostname string) (tapir.Connection, err // Connect initializes a new outbound connection to the given peer, using the defined Application func (s *BaseOnionService) Connect(hostname string, app tapir.Application) (bool, error) { - currconn, err := s.GetConnection(hostname) + currconn,_ := s.GetConnection(hostname) // We already have a connection if currconn != nil { From b32cad2c27bbfd386812467bd7dc521b4ae89bee Mon Sep 17 00:00:00 2001 From: Sarah Jamie Lewis Date: Mon, 27 Sep 2021 14:57:06 -0700 Subject: [PATCH 4/4] Don't Log Conn --- networks/tor/BaseOnionService.go | 1 - 1 file changed, 1 deletion(-) diff --git a/networks/tor/BaseOnionService.go b/networks/tor/BaseOnionService.go index b412ab2..2fb2d90 100644 --- a/networks/tor/BaseOnionService.go +++ b/networks/tor/BaseOnionService.go @@ -93,7 +93,6 @@ func (s *BaseOnionService) WaitForCapabilityOrClose(cid string, name tapir.Capab // Lookup the connection... conn, err := s.GetConnection(cid) - log.Debugf("Found %v %v", conn, err) // If there are no active connections then return an error... if conn == nil { log.Debugf("no active connection found for %v", cid)