From 3e6aef00b26e33181e907c9a27357003ad497b82 Mon Sep 17 00:00:00 2001 From: Tobi Smethurst <31960611+tsmethurst@users.noreply.github.com> Date: Sun, 27 Jun 2021 11:46:07 +0200 Subject: [PATCH] fix the annoying infinite handshake bug (tested) (#69) --- internal/federation/federator.go | 6 +++ internal/federation/handshake.go | 80 +++++++++++++++++++++++++++++++ internal/federation/util.go | 2 + internal/processing/federation.go | 66 ++++++++++++++----------- 4 files changed, 125 insertions(+), 29 deletions(-) create mode 100644 internal/federation/handshake.go diff --git a/internal/federation/federator.go b/internal/federation/federator.go index 016a6fb68..2ee01697f 100644 --- a/internal/federation/federator.go +++ b/internal/federation/federator.go @@ -21,6 +21,7 @@ package federation import ( "net/http" "net/url" + "sync" "github.com/go-fed/activity/pub" "github.com/sirupsen/logrus" @@ -54,6 +55,8 @@ type Federator interface { // // If username is an empty string, our instance user's credentials will be used instead. GetTransportForUser(username string) (transport.Transport, error) + // Handshaking returns true if the given username is currently in the process of dereferencing the remoteAccountID. + Handshaking(username string, remoteAccountID *url.URL) bool pub.CommonBehavior pub.FederatingProtocol } @@ -67,6 +70,8 @@ type federator struct { transportController transport.Controller actor pub.FederatingActor log *logrus.Logger + handshakes map[string][]*url.URL + handshakeSync *sync.Mutex // mutex to lock/unlock when checking or updating the handshakes map } // NewFederator returns a new federator @@ -81,6 +86,7 @@ func NewFederator(db db.DB, federatingDB federatingdb.DB, transportController tr typeConverter: typeConverter, transportController: transportController, log: log, + handshakeSync: &sync.Mutex{}, } actor := newFederatingActor(f, f, federatingDB, clock) f.actor = actor diff --git a/internal/federation/handshake.go b/internal/federation/handshake.go new file mode 100644 index 000000000..af720403a --- /dev/null +++ b/internal/federation/handshake.go @@ -0,0 +1,80 @@ +package federation + +import "net/url" + +func (f *federator) Handshaking(username string, remoteAccountID *url.URL) bool { + f.handshakeSync.Lock() + defer f.handshakeSync.Unlock() + + if f.handshakes == nil { + // handshakes isn't even initialized yet so we can't be handshaking with anyone + return false + } + + remoteIDs, ok := f.handshakes[username]; + if !ok { + // user isn't handshaking with anyone, bail + return false + } + + for _, id := range remoteIDs { + if id.String() == remoteAccountID.String() { + // we are currently handshaking with the remote account, yep + return true + } + } + + // didn't find it which means we're not handshaking + return false +} + +func (f *federator) startHandshake(username string, remoteAccountID *url.URL) { + f.handshakeSync.Lock() + defer f.handshakeSync.Unlock() + + // lazily initialize handshakes + if f.handshakes == nil { + f.handshakes = make(map[string][]*url.URL) + } + + remoteIDs, ok := f.handshakes[username] + if !ok { + // there was nothing in there yet, so just add this entry and return + f.handshakes[username] = []*url.URL{remoteAccountID} + return + } + + // add the remote ID to the slice + remoteIDs = append(remoteIDs, remoteAccountID) + f.handshakes[username] = remoteIDs +} + +func (f *federator) stopHandshake(username string, remoteAccountID *url.URL) { + f.handshakeSync.Lock() + defer f.handshakeSync.Unlock() + + if f.handshakes == nil { + return + } + + remoteIDs, ok := f.handshakes[username] + if !ok { + // there was nothing in there yet anyway so just bail + return + } + + newRemoteIDs := []*url.URL{} + for _, id := range remoteIDs { + if id.String() != remoteAccountID.String() { + newRemoteIDs = append(newRemoteIDs, id) + } + } + + if len(newRemoteIDs) == 0 { + // there are no handshakes so just remove this user entry from the map and save a few bytes + delete(f.handshakes, username) + } else { + // there are still other handshakes ongoing + f.handshakes[username] = newRemoteIDs + } +} diff --git a/internal/federation/util.go b/internal/federation/util.go index 7be92e13d..9ec0770f6 100644 --- a/internal/federation/util.go +++ b/internal/federation/util.go @@ -213,6 +213,8 @@ func (f *federator) AuthenticateFederatedRequest(username string, r *http.Reques } func (f *federator) DereferenceRemoteAccount(username string, remoteAccountID *url.URL) (typeutils.Accountable, error) { + f.startHandshake(username, remoteAccountID) + defer f.stopHandshake(username, remoteAccountID) transport, err := f.GetTransportForUser(username) if err != nil { diff --git a/internal/processing/federation.go b/internal/processing/federation.go index a154034db..3bcda866b 100644 --- a/internal/processing/federation.go +++ b/internal/processing/federation.go @@ -26,7 +26,6 @@ import ( "github.com/go-fed/activity/streams" "github.com/go-fed/activity/streams/vocab" - "github.com/sirupsen/logrus" apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" "github.com/superseriousbusiness/gotosocial/internal/db" "github.com/superseriousbusiness/gotosocial/internal/gtserror" @@ -35,23 +34,16 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/util" ) -// authenticateAndDereferenceFediRequest authenticates the HTTP signature of an incoming federation request, using the given +// dereferenceFediRequest authenticates the HTTP signature of an incoming federation request, using the given // username to perform the validation. It will *also* dereference the originator of the request and return it as a gtsmodel account // for further processing. NOTE that this function will have the side effect of putting the dereferenced account into the database, // and passing it into the processor through a channel for further asynchronous processing. -func (p *processor) authenticateAndDereferenceFediRequest(username string, r *http.Request) (*gtsmodel.Account, error) { - - // first authenticate - requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(username, r) - if err != nil { - return nil, fmt.Errorf("couldn't authenticate request for username %s: %s", username, err) - } - +func (p *processor) dereferenceFediRequest(username string, requestingAccountURI *url.URL) (*gtsmodel.Account, error) { // OK now we can do the dereferencing part // we might already have an entry for this account so check that first requestingAccount := >smodel.Account{} - err = p.db.GetWhere([]db.Where{{Key: "uri", Value: requestingAccountURI.String()}}, requestingAccount) + err := p.db.GetWhere([]db.Where{{Key: "uri", Value: requestingAccountURI.String()}}, requestingAccount) if err == nil { // we do have it yay, return it return requestingAccount, nil @@ -98,12 +90,6 @@ func (p *processor) authenticateAndDereferenceFediRequest(username string, r *ht } func (p *processor) GetFediUser(requestedUsername string, request *http.Request) (interface{}, gtserror.WithCode) { - l := p.log.WithFields(logrus.Fields{ - "func": "GetFediUser", - "requestedUsername": requestedUsername, - "requestURL": request.URL.String(), - }) - // get the account the request is referring to requestedAccount := >smodel.Account{} if err := p.db.GetLocalAccountByUsername(requestedUsername, requestedAccount); err != nil { @@ -113,28 +99,35 @@ func (p *processor) GetFediUser(requestedUsername string, request *http.Request) var requestedPerson vocab.ActivityStreamsPerson var err error if util.IsPublicKeyPath(request.URL) { - l.Debug("serving from public key path") // if it's a public key path, we don't need to authenticate but we'll only serve the bare minimum user profile needed for the public key requestedPerson, err = p.tc.AccountToASMinimal(requestedAccount) if err != nil { return nil, gtserror.NewErrorInternalError(err) } } else if util.IsUserPath(request.URL) { - l.Debug("serving from user path") // if it's a user path, we want to fully authenticate the request before we serve any data, and then we can serve a more complete profile - requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request) + requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request) if err != nil { return nil, gtserror.NewErrorNotAuthorized(err) } - blocked, err := p.db.Blocked(requestedAccount.ID, requestingAccount.ID) - if err != nil { - return nil, gtserror.NewErrorInternalError(err) + // if we're already handshaking/dereferencing a remote account, we can skip the dereferencing part + if !p.federator.Handshaking(requestedUsername, requestingAccountURI) { + requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI) + if err != nil { + return nil, gtserror.NewErrorNotAuthorized(err) + } + + blocked, err := p.db.Blocked(requestedAccount.ID, requestingAccount.ID) + if err != nil { + return nil, gtserror.NewErrorInternalError(err) + } + + if blocked { + return nil, gtserror.NewErrorNotAuthorized(fmt.Errorf("block exists between accounts %s and %s", requestedAccount.ID, requestingAccount.ID)) + } } - if blocked { - return nil, gtserror.NewErrorNotAuthorized(fmt.Errorf("block exists between accounts %s and %s", requestedAccount.ID, requestingAccount.ID)) - } requestedPerson, err = p.tc.AccountToAS(requestedAccount) if err != nil { return nil, gtserror.NewErrorInternalError(err) @@ -159,7 +152,12 @@ func (p *processor) GetFediFollowers(requestedUsername string, request *http.Req } // authenticate the request - requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request) + requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request) + if err != nil { + return nil, gtserror.NewErrorNotAuthorized(err) + } + + requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI) if err != nil { return nil, gtserror.NewErrorNotAuthorized(err) } @@ -199,7 +197,12 @@ func (p *processor) GetFediFollowing(requestedUsername string, request *http.Req } // authenticate the request - requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request) + requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request) + if err != nil { + return nil, gtserror.NewErrorNotAuthorized(err) + } + + requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI) if err != nil { return nil, gtserror.NewErrorNotAuthorized(err) } @@ -239,7 +242,12 @@ func (p *processor) GetFediStatus(requestedUsername string, requestedStatusID st } // authenticate the request - requestingAccount, err := p.authenticateAndDereferenceFediRequest(requestedUsername, request) + requestingAccountURI, err := p.federator.AuthenticateFederatedRequest(requestedUsername, request) + if err != nil { + return nil, gtserror.NewErrorNotAuthorized(err) + } + + requestingAccount, err := p.dereferenceFediRequest(requestedUsername, requestingAccountURI) if err != nil { return nil, gtserror.NewErrorNotAuthorized(err) }