[feature] Allow admins to expire remote public keys; refetch expired keys on demand (#2183)

5 files changed, 228 insertions, 65 deletions

diff --git a/internal/federation/authenticate.go b/internal/federation/authenticate.go
index 80998680e..12d6f459a 100644
--- a/internal/federation/authenticate.go
+++ b/internal/federation/authenticate.go
@@ -25,14 +25,17 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"time"
 
 	"codeberg.org/gruf/go-kv"
 	"github.com/go-fed/httpsig"
 	"github.com/superseriousbusiness/activity/streams"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/config"
+	"github.com/superseriousbusiness/gotosocial/internal/db"
 	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
+	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
 )
 
@@ -45,11 +48,47 @@ var (
 	}
 )
 
+// PubKeyAuth models authorization information for a remote
+// Actor making a signed HTTP request to this GtS instance
+// using a public key.
+type PubKeyAuth struct {
+	// CachedPubKey is the public key found in the db
+	// for the Actor whose request we're now authenticating.
+	// Will be set only in cases where we had the Owner
+	// of the key stored in the database already.
+	CachedPubKey *rsa.PublicKey
+
+	// FetchedPubKey is an up-to-date public key fetched
+	// from the remote instance. Will be set in cases
+	// where EITHER we hadn't seen the Actor before whose
+	// request we're now authenticating, OR a CachedPubKey
+	// was found in our database, but was expired.
+	FetchedPubKey *rsa.PublicKey
+
+	// OwnerURI is the ActivityPub id of the owner of
+	// the public key used to sign the request we're
+	// now authenticating. This will always be set
+	// even if Owner isn't, so that callers can use
+	// this URI to go fetch the Owner from remote.
+	OwnerURI *url.URL
+
+	// Owner is the account corresponding to OwnerURI.
+	//
+	// Owner will only be defined if the account who
+	// owns the public key was already cached in the
+	// database when we received the request we're now
+	// authenticating (ie., we've seen it before).
+	//
+	// If it's not defined, callers should use OwnerURI
+	// to go and dereference it.
+	Owner *gtsmodel.Account
+}
+
 // AuthenticateFederatedRequest authenticates any kind of incoming federated
 // request from a remote server. This includes things like GET requests for
 // dereferencing our users or statuses etc, and POST requests for delivering
-// new Activities. The function returns the URL of the owner of the public key
-// used in the requesting http signature.
+// new Activities. The function returns details of the public key(s) used to
+// authenticate the requesting http signature.
 //
 // 'Authenticate' in this case is defined as making sure that the http request
 // is actually signed by whoever claims to have signed it, by fetching the public
@@ -70,7 +109,7 @@ var (
 // Also note that this function *does not* dereference the remote account that
 // the signature key is associated with. Other functions should use the returned
 // URL to dereference the remote account, if required.
-func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedUsername string) (*url.URL, gtserror.WithCode) {
+func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedUsername string) (*PubKeyAuth, gtserror.WithCode) {
 	// Thanks to the signature check middleware,
 	// we should already have an http signature
 	// verifier set on the context. If we don't,
@@ -102,10 +141,10 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU
 	// so now we need to validate the signature.
 
 	var (
-		pubKeyIDStr          = pubKeyID.String()
-		requestingAccountURI *url.URL
-		pubKey               interface{}
-		errWithCode          gtserror.WithCode
+		pubKeyIDStr = pubKeyID.String()
+		local       = (pubKeyID.Host == config.GetHost())
+		pubKeyAuth  *PubKeyAuth
+		errWithCode gtserror.WithCode
 	)
 
 	l := log.
@@ -115,37 +154,49 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU
 			{"pubKeyID", pubKeyIDStr},
 		}...)
 
-	if pubKeyID.Host == config.GetHost() {
-		l.Trace("public key is ours, no dereference needed")
-		requestingAccountURI, pubKey, errWithCode = f.derefDBOnly(ctx, pubKeyIDStr)
+	if local {
+		l.Trace("public key is local, no dereference needed")
+		pubKeyAuth, errWithCode = f.derefPubKeyDBOnly(ctx, pubKeyIDStr)
 	} else {
-		l.Trace("public key is not ours, checking if we need to dereference")
-		requestingAccountURI, pubKey, errWithCode = f.deref(ctx, requestedUsername, pubKeyIDStr, pubKeyID)
+		l.Trace("public key is remote, checking if we need to dereference")
+		pubKeyAuth, errWithCode = f.derefPubKey(ctx, requestedUsername, pubKeyIDStr, pubKeyID)
 	}
 
 	if errWithCode != nil {
 		return nil, errWithCode
 	}
 
-	// Ensure public key now defined.
-	if pubKey == nil {
-		err := gtserror.New("public key was nil")
+	if local && pubKeyAuth == nil {
+		// We signed this request, apparently, but
+		// local lookup didn't find anything. This
+		// is an almost impossible error condition!
+		err := gtserror.Newf("local public key %s could not be found; "+
+			"has the account been manually removed from the db?", pubKeyIDStr)
 		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	// Try to authenticate using permitted algorithms in
-	// order of most -> least common. Return OK as soon
-	// as one passes.
-	for _, algo := range signingAlgorithms {
-		l.Tracef("trying %s", algo)
-
-		err := verifier.Verify(pubKey, algo)
-		if err == nil {
-			l.Tracef("authentication PASSED with %s", algo)
-			return requestingAccountURI, nil
+	// order of most -> least common, checking each defined
+	// pubKey for this Actor. Return OK as soon as one passes.
+	for _, pubKey := range [2]*rsa.PublicKey{
+		pubKeyAuth.FetchedPubKey,
+		pubKeyAuth.CachedPubKey,
+	} {
+		if pubKey == nil {
+			continue
 		}
 
-		l.Tracef("authentication NOT PASSED with %s: %q", algo, err)
+		for _, algo := range signingAlgorithms {
+			l.Tracef("trying %s", algo)
+
+			err := verifier.Verify(pubKey, algo)
+			if err == nil {
+				l.Tracef("authentication PASSED with %s", algo)
+				return pubKeyAuth, nil
+			}
+
+			l.Tracef("authentication NOT PASSED with %s: %q", algo, err)
+		}
 	}
 
 	// At this point no algorithms passed.
@@ -157,36 +208,52 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU
 	return nil, gtserror.NewErrorUnauthorized(err, err.Error())
 }
 
-// derefDBOnly tries to dereference the given public
-// key using only entries already in the database.
-func (f *federator) derefDBOnly(
+// derefPubKeyDBOnly tries to dereference the given
+// pubKey using only entries already in the database.
+//
+// In case of a db or URL error, will return the error.
+//
+// In case an entry for the pubKey owner just doesn't
+// exist in the db (yet), will return nil, nil.
+func (f *federator) derefPubKeyDBOnly(
 	ctx context.Context,
 	pubKeyIDStr string,
-) (*url.URL, interface{}, gtserror.WithCode) {
-	reqAcct, err := f.db.GetAccountByPubkeyID(ctx, pubKeyIDStr)
+) (*PubKeyAuth, gtserror.WithCode) {
+	owner, err := f.db.GetAccountByPubkeyID(ctx, pubKeyIDStr)
 	if err != nil {
+		if errors.Is(err, db.ErrNoEntries) {
+			// We don't have this
+			// account stored (yet).
+			return nil, nil
+		}
+
 		err = gtserror.Newf("db error getting account with pubKeyID %s: %w", pubKeyIDStr, err)
-		return nil, nil, gtserror.NewErrorInternalError(err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	reqAcctURI, err := url.Parse(reqAcct.URI)
+	ownerURI, err := url.Parse(owner.URI)
 	if err != nil {
 		err = gtserror.Newf("error parsing account uri with pubKeyID %s: %w", pubKeyIDStr, err)
-		return nil, nil, gtserror.NewErrorInternalError(err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	return reqAcctURI, reqAcct.PublicKey, nil
+	return &PubKeyAuth{
+		CachedPubKey: owner.PublicKey,
+		OwnerURI:     ownerURI,
+		Owner:        owner,
+	}, nil
 }
 
-// deref tries to dereference the given public key by first
-// checking in the database, and then (if no entries found)
-// calling the remote pub key URI and extracting the key.
-func (f *federator) deref(
+// derefPubKey tries to dereference the given public key by first
+// checking in the database, and then (if no entry found, or entry
+// found but pubKey expired) calling the remote pub key URI and
+// extracting the key.
+func (f *federator) derefPubKey(
 	ctx context.Context,
 	requestedUsername string,
 	pubKeyIDStr string,
 	pubKeyID *url.URL,
-) (*url.URL, interface{}, gtserror.WithCode) {
+) (*PubKeyAuth, gtserror.WithCode) {
 	l := log.
 		WithContext(ctx).
 		WithFields(kv.Fields{
@@ -196,42 +263,101 @@ func (f *federator) deref(
 
 	// Try a database only deref first. We may already
 	// have the requesting account cached locally.
-	reqAcctURI, pubKey, errWithCode := f.derefDBOnly(ctx, pubKeyIDStr)
-	if errWithCode == nil {
-		l.Trace("public key cached, no dereference needed")
-		return reqAcctURI, pubKey, nil
+	pubKeyAuth, errWithCode := f.derefPubKeyDBOnly(ctx, pubKeyIDStr)
+	if errWithCode != nil {
+		return nil, errWithCode
 	}
 
-	l.Trace("public key not cached, trying dereference")
+	var (
+		// Just haven't seen this
+		// Actor + their pubkey yet.
+		uncached = (pubKeyAuth == nil)
+
+		// Have seen this Actor + their
+		// pubkey but latter is now expired.
+		expired = (!uncached && pubKeyAuth.Owner.PubKeyExpired())
+	)
+
+	switch {
+	case uncached:
+		l.Trace("public key was not cached, trying dereference of public key")
+	case !expired:
+		l.Trace("public key cached and up to date, no dereference needed")
+		return pubKeyAuth, nil
+	case expired:
+		// This is fairly rare and it may be helpful for
+		// admins to see what's going on, so log at info.
+		l.Infof(
+			"public key was cached, but expired at %s, trying dereference of new public key",
+			pubKeyAuth.Owner.PublicKeyExpiresAt,
+		)
+	}
 
 	// If we've tried to get this account before and we
 	// now have a tombstone for it (ie., it's been deleted
 	// from remote), don't try to dereference it again.
 	gone, err := f.CheckGone(ctx, pubKeyID)
 	if err != nil {
-		err := gtserror.Newf("error checking for tombstone for %s: %w", pubKeyIDStr, err)
-		return nil, nil, gtserror.NewErrorInternalError(err)
+		err := gtserror.Newf("error checking for tombstone (%s): %w", pubKeyIDStr, err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
 	if gone {
-		err := gtserror.Newf("account with public key %s is gone", pubKeyIDStr)
-		return nil, nil, gtserror.NewErrorGone(err)
+		err := gtserror.Newf("account with public key is gone (%s)", pubKeyIDStr)
+		return nil, gtserror.NewErrorGone(err)
 	}
 
-	// Make an http call to get the pubkey.
+	// Make an http call to get the (refreshed) pubkey.
 	pubKeyBytes, errWithCode := f.callForPubKey(ctx, requestedUsername, pubKeyID)
 	if errWithCode != nil {
-		return nil, nil, errWithCode
+		return nil, errWithCode
 	}
 
 	// Extract the key and the owner from the response.
 	pubKey, pubKeyOwner, err := parsePubKeyBytes(ctx, pubKeyBytes, pubKeyID)
 	if err != nil {
-		err := fmt.Errorf("error parsing public key %s: %w", pubKeyID, err)
-		return nil, nil, gtserror.NewErrorUnauthorized(err)
+		err := fmt.Errorf("error parsing public key (%s): %w", pubKeyID, err)
+		return nil, gtserror.NewErrorUnauthorized(err)
+	}
+
+	if !expired {
+		// PubKeyResponse was nil before because
+		// we had nothing cached; return the key
+		// we just fetched, and nothing else.
+		return &PubKeyAuth{
+			FetchedPubKey: pubKey,
+			OwnerURI:      pubKeyOwner,
+		}, nil
+	}
+
+	// Add newly-fetched key to response.
+	pubKeyAuth.FetchedPubKey = pubKey
+
+	// If key was expired, that means we already
+	// had an owner stored for it locally. Since
+	// we now successfully refreshed the pub key,
+	// we should update the account to reflect that.
+	ownerAcct := pubKeyAuth.Owner
+	ownerAcct.PublicKey = pubKeyAuth.FetchedPubKey
+	ownerAcct.PublicKeyExpiresAt = time.Time{}
+
+	l.Info("obtained a new public key to replace expired key, caching now; " +
+		"authorization for this request will be attempted with both old and new keys")
+
+	if err := f.db.UpdateAccount(
+		ctx,
+		ownerAcct,
+		"public_key",
+		"public_key_expires_at",
+	); err != nil {
+		err := gtserror.Newf("db error updating account with refreshed public key (%s): %w", pubKeyIDStr, err)
+		return nil, gtserror.NewErrorInternalError(err)
 	}
 
-	return pubKeyOwner, pubKey, nil
+	// Return both new and cached (now
+	// expired) keys, authentication
+	// will be attempted with both.
+	return pubKeyAuth, nil
 }
 
 // callForPubKey handles the nitty gritty of actually
diff --git a/internal/federation/federatingprotocol.go b/internal/federation/federatingprotocol.go
index ef42639ed..fb4e5bfb9 100644
--- a/internal/federation/federatingprotocol.go
+++ b/internal/federation/federatingprotocol.go
@@ -209,7 +209,7 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 	}
 
 	// Check who's trying to deliver to us by inspecting the http signature.
-	pubKeyOwner, errWithCode := f.AuthenticateFederatedRequest(ctx, receivingAccount.Username)
+	pubKeyAuth, errWithCode := f.AuthenticateFederatedRequest(ctx, receivingAccount.Username)
 	if errWithCode != nil {
 		switch errWithCode.Code() {
 		case http.StatusUnauthorized, http.StatusForbidden, http.StatusBadRequest:
@@ -232,12 +232,14 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 		}
 	}
 
+	pubKeyOwnerURI := pubKeyAuth.OwnerURI
+
 	// Authentication has passed, check if we need to create a
 	// new instance entry for the Host of the requesting account.
-	if _, err := f.db.GetInstance(ctx, pubKeyOwner.Host); err != nil {
+	if _, err := f.db.GetInstance(ctx, pubKeyOwnerURI.Host); err != nil {
 		if !errors.Is(err, db.ErrNoEntries) {
 			// There's been an actual error.
-			err = gtserror.Newf("error getting instance %s: %w", pubKeyOwner.Host, err)
+			err = gtserror.Newf("error getting instance %s: %w", pubKeyOwnerURI.Host, err)
 			return ctx, false, err
 		}
 
@@ -247,17 +249,17 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 			gtscontext.SetFastFail(ctx),
 			username,
 			&url.URL{
-				Scheme: pubKeyOwner.Scheme,
-				Host:   pubKeyOwner.Host,
+				Scheme: pubKeyOwnerURI.Scheme,
+				Host:   pubKeyOwnerURI.Host,
 			},
 		)
 		if err != nil {
-			err = gtserror.Newf("error dereferencing instance %s: %w", pubKeyOwner.Host, err)
+			err = gtserror.Newf("error dereferencing instance %s: %w", pubKeyOwnerURI.Host, err)
 			return nil, false, err
 		}
 
 		if err := f.db.PutInstance(ctx, instance); err != nil && !errors.Is(err, db.ErrAlreadyExists) {
-			err = gtserror.Newf("error inserting instance entry for %s: %w", pubKeyOwner.Host, err)
+			err = gtserror.Newf("error inserting instance entry for %s: %w", pubKeyOwnerURI.Host, err)
 			return nil, false, err
 		}
 	}
@@ -268,7 +270,7 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 	requestingAccount, _, err := f.GetAccountByURI(
 		gtscontext.SetFastFail(ctx),
 		username,
-		pubKeyOwner,
+		pubKeyOwnerURI,
 	)
 	if err != nil {
 		if gtserror.StatusCode(err) == http.StatusGone {
@@ -282,7 +284,7 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 			return ctx, false, nil
 		}
 
-		err = gtserror.Newf("couldn't get requesting account %s: %w", pubKeyOwner, err)
+		err = gtserror.Newf("couldn't get requesting account %s: %w", pubKeyOwnerURI, err)
 		return nil, false, err
 	}
 
diff --git a/internal/federation/federatingprotocol_test.go b/internal/federation/federatingprotocol_test.go
index 8da6859dd..7a8343048 100644
--- a/internal/federation/federatingprotocol_test.go
+++ b/internal/federation/federatingprotocol_test.go
@@ -257,6 +257,33 @@ func (suite *FederatingProtocolTestSuite) TestAuthenticatePostInbox() {
 	suite.Equal(http.StatusOK, code)
 }
 
+func (suite *FederatingProtocolTestSuite) TestAuthenticatePostInboxKeyExpired() {
+	var (
+		ctx              = context.Background()
+		activity         = suite.testActivities["dm_for_zork"]
+		receivingAccount = suite.testAccounts["local_account_1"]
+	)
+
+	// Update remote account to mark key as expired.
+	remoteAcct := &gtsmodel.Account{}
+	*remoteAcct = *suite.testAccounts["remote_account_1"]
+	remoteAcct.PublicKeyExpiresAt = testrig.TimeMustParse("2022-06-10T15:22:08Z")
+	if err := suite.state.DB.UpdateAccount(ctx, remoteAcct, "public_key_expires_at"); err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	ctx, authed, resp, code := suite.authenticatePostInbox(
+		ctx,
+		receivingAccount,
+		activity,
+	)
+
+	suite.NotNil(gtscontext.RequestingAccount(ctx))
+	suite.True(authed)
+	suite.Equal([]byte{}, resp)
+	suite.Equal(http.StatusOK, code)
+}
+
 func (suite *FederatingProtocolTestSuite) TestAuthenticatePostGoneWithTombstone() {
 	var (
 		activity         = suite.testActivities["delete_https://somewhere.mysterious/users/rest_in_piss#main-key"]
diff --git a/internal/federation/federator.go b/internal/federation/federator.go
index 40af08d25..ad6db8ff7 100644
--- a/internal/federation/federator.go
+++ b/internal/federation/federator.go
@@ -19,7 +19,6 @@ package federation
 
 import (
 	"context"
-	"net/url"
 
 	"github.com/superseriousbusiness/activity/pub"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
@@ -49,7 +48,7 @@ type Federator interface {
 	// If the request does not pass authentication, or there's a domain block, nil, false, nil will be returned.
 	//
 	// If something goes wrong during authentication, nil, false, and an error will be returned.
-	AuthenticateFederatedRequest(ctx context.Context, username string) (*url.URL, gtserror.WithCode)
+	AuthenticateFederatedRequest(ctx context.Context, username string) (*PubKeyAuth, gtserror.WithCode)
 
 	pub.CommonBehavior
 	pub.FederatingProtocol
diff --git a/internal/federation/federator_test.go b/internal/federation/federator_test.go
index a80d590a4..3287cd11a 100644
--- a/internal/federation/federator_test.go
+++ b/internal/federation/federator_test.go
@@ -18,6 +18,8 @@
 package federation_test
 
 import (
+	"context"
+
 	"github.com/stretchr/testify/suite"
 
 	"github.com/superseriousbusiness/gotosocial/internal/federation"
@@ -71,7 +73,14 @@ func (suite *FederatorStandardTestSuite) SetupTest() {
 		suite.typeconverter,
 	)
 
-	suite.httpClient = testrig.NewMockHTTPClient(nil, "../../testrig/media")
+	// Ensure it's possible to deref
+	// main key of foss satan.
+	fossSatanPerson, err := suite.typeconverter.AccountToAS(context.Background(), suite.testAccounts["remote_account_1"])
+	if err != nil {
+		suite.FailNow(err.Error())
+	}
+
+	suite.httpClient = testrig.NewMockHTTPClient(nil, "../../testrig/media", fossSatanPerson)
 	suite.httpClient.TestRemotePeople = testrig.NewTestFediPeople()
 	suite.httpClient.TestRemoteStatuses = testrig.NewTestFediStatuses()