From 9cf66bf29871dad6523f421ae72125d584ac4c9a Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Tue, 26 Apr 2022 18:10:11 +0200 Subject: [PATCH] [chore] Return more useful errors from auth failure (#494) * try rsa_sha256 sig algo first * return more informative errors from auth * adapt to reworked auth function --- internal/federation/authenticate.go | 88 +++++++++++++------ internal/federation/federatingprotocol.go | 20 +++-- internal/federation/federator.go | 3 +- .../processing/federation/getfollowers.go | 7 +- .../processing/federation/getfollowing.go | 7 +- internal/processing/federation/getoutbox.go | 7 +- internal/processing/federation/getstatus.go | 7 +- .../processing/federation/getstatusreplies.go | 7 +- internal/processing/federation/getuser.go | 11 +-- 9 files changed, 92 insertions(+), 65 deletions(-) diff --git a/internal/federation/authenticate.go b/internal/federation/authenticate.go index 84044890..380fa144 100644 --- a/internal/federation/authenticate.go +++ b/internal/federation/authenticate.go @@ -38,6 +38,7 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/ap" "github.com/superseriousbusiness/gotosocial/internal/config" "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" ) @@ -115,7 +116,7 @@ func getPublicKeyFromResponse(c context.Context, b []byte, keyID *url.URL) (voca // // 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, bool, error) { +func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedUsername string) (*url.URL, gtserror.WithCode) { l := logrus.WithField("func", "AuthenticateFederatedRequest") var publicKey interface{} @@ -125,33 +126,43 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU // thanks to signaturecheck.go in the security package, we should already have a signature verifier set on the context vi := ctx.Value(ap.ContextRequestingPublicKeyVerifier) if vi == nil { - l.Debug("request wasn't signed") - return nil, false, nil // request wasn't signed + err := errors.New("http request wasn't signed or http signature was invalid") + errWithCode := gtserror.NewErrorNotAuthorized(err, err.Error()) + l.Debug(errWithCode) + return nil, errWithCode } verifier, ok := vi.(httpsig.Verifier) if !ok { - l.Debug("couldn't extract sig verifier") - return nil, false, nil // couldn't extract the verifier + err := errors.New("http request wasn't signed or http signature was invalid") + errWithCode := gtserror.NewErrorNotAuthorized(err, err.Error()) + l.Debug(errWithCode) + return nil, errWithCode } // we should have the signature itself set too si := ctx.Value(ap.ContextRequestingPublicKeySignature) - if vi == nil { - l.Debug("request wasn't signed") - return nil, false, nil // request wasn't signed + if si == nil { + err := errors.New("http request wasn't signed or http signature was invalid") + errWithCode := gtserror.NewErrorNotAuthorized(err, err.Error()) + l.Debug(errWithCode) + return nil, errWithCode } signature, ok := si.(string) if !ok { - l.Debug("couldn't extract signature") - return nil, false, nil // couldn't extract the signature + err := errors.New("http request wasn't signed or http signature was invalid") + errWithCode := gtserror.NewErrorNotAuthorized(err, err.Error()) + l.Debug(errWithCode) + return nil, errWithCode } + // now figure out who actually signed it requestingPublicKeyID, err := url.Parse(verifier.KeyId()) if err != nil { - l.Debug("couldn't parse public key URL") - return nil, false, err // couldn't parse the public key ID url + errWithCode := gtserror.NewErrorBadRequest(err, fmt.Sprintf("couldn't parse public key URL %s", verifier.KeyId())) + l.Debug(errWithCode) + return nil, errWithCode } requestingRemoteAccount := >smodel.Account{} @@ -163,12 +174,16 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU // the request is coming from INSIDE THE HOUSE so skip the remote dereferencing l.Tracef("proceeding without dereference for local public key %s", requestingPublicKeyID) if err := f.db.GetWhere(ctx, []db.Where{{Key: "public_key_uri", Value: requestingPublicKeyID.String()}}, requestingLocalAccount); err != nil { - return nil, false, fmt.Errorf("couldn't get local account with public key uri %s from the database: %s", requestingPublicKeyID.String(), err) + errWithCode := gtserror.NewErrorInternalError(fmt.Errorf("couldn't get account with public key uri %s from the database: %s", requestingPublicKeyID.String(), err)) + l.Debug(errWithCode) + return nil, errWithCode } publicKey = requestingLocalAccount.PublicKey pkOwnerURI, err = url.Parse(requestingLocalAccount.URI) if err != nil { - return nil, false, fmt.Errorf("error parsing url %s: %s", requestingLocalAccount.URI, err) + errWithCode := gtserror.NewErrorBadRequest(err, fmt.Sprintf("couldn't parse public key owner URL %s", requestingLocalAccount.URI)) + l.Debug(errWithCode) + return nil, errWithCode } } else if err := f.db.GetWhere(ctx, []db.Where{{Key: "public_key_uri", Value: requestingPublicKeyID.String()}}, requestingRemoteAccount); err == nil { // REMOTE ACCOUNT REQUEST WITH KEY CACHED LOCALLY @@ -177,7 +192,9 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU publicKey = requestingRemoteAccount.PublicKey pkOwnerURI, err = url.Parse(requestingRemoteAccount.URI) if err != nil { - return nil, false, fmt.Errorf("error parsing url %s: %s", requestingRemoteAccount.URI, err) + errWithCode := gtserror.NewErrorBadRequest(err, fmt.Sprintf("couldn't parse public key owner URL %s", requestingRemoteAccount.URI)) + l.Debug(errWithCode) + return nil, errWithCode } } else { // REMOTE ACCOUNT REQUEST WITHOUT KEY CACHED LOCALLY @@ -186,56 +203,72 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU l.Tracef("proceeding with dereference for uncached public key %s", requestingPublicKeyID) transport, err := f.transportController.NewTransportForUsername(ctx, requestedUsername) if err != nil { - return nil, false, fmt.Errorf("transport err: %s", err) + errWithCode := gtserror.NewErrorInternalError(fmt.Errorf("error creating transport for %s: %s", requestedUsername, err)) + l.Debug(errWithCode) + return nil, errWithCode } // The actual http call to the remote server is made right here in the Dereference function. b, err := transport.Dereference(ctx, requestingPublicKeyID) if err != nil { - return nil, false, fmt.Errorf("error deferencing key %s: %s", requestingPublicKeyID.String(), err) + errWithCode := gtserror.NewErrorNotAuthorized(fmt.Errorf("error dereferencing public key %s: %s", requestingPublicKeyID, err)) + l.Debug(errWithCode) + return nil, errWithCode } // if the key isn't in the response, we can't authenticate the request requestingPublicKey, err := getPublicKeyFromResponse(ctx, b, requestingPublicKeyID) if err != nil { - return nil, false, fmt.Errorf("error getting key %s from response %s: %s", requestingPublicKeyID.String(), string(b), err) + errWithCode := gtserror.NewErrorNotAuthorized(fmt.Errorf("error parsing public key %s: %s", requestingPublicKeyID, err)) + l.Debug(errWithCode) + return nil, errWithCode } // we should be able to get the actual key embedded in the vocab.W3IDSecurityV1PublicKey pkPemProp := requestingPublicKey.GetW3IDSecurityV1PublicKeyPem() if pkPemProp == nil || !pkPemProp.IsXMLSchemaString() { - return nil, false, errors.New("publicKeyPem property is not provided or it is not embedded as a value") + errWithCode := gtserror.NewErrorNotAuthorized(errors.New("publicKeyPem property is not provided or it is not embedded as a value")) + l.Debug(errWithCode) + return nil, errWithCode } // and decode the PEM so that we can parse it as a golang public key pubKeyPem := pkPemProp.Get() block, _ := pem.Decode([]byte(pubKeyPem)) if block == nil || block.Type != "PUBLIC KEY" { - return nil, false, errors.New("could not decode publicKeyPem to PUBLIC KEY pem block type") + errWithCode := gtserror.NewErrorNotAuthorized(errors.New("could not decode publicKeyPem to PUBLIC KEY pem block type")) + l.Debug(errWithCode) + return nil, errWithCode } publicKey, err = x509.ParsePKIXPublicKey(block.Bytes) if err != nil { - return nil, false, fmt.Errorf("could not parse public key from block bytes: %s", err) + errWithCode := gtserror.NewErrorNotAuthorized(fmt.Errorf("could not parse public key %s from block bytes: %s", requestingPublicKeyID, err)) + l.Debug(errWithCode) + return nil, errWithCode } // all good! we just need the URI of the key owner to return pkOwnerProp := requestingPublicKey.GetW3IDSecurityV1Owner() if pkOwnerProp == nil || !pkOwnerProp.IsIRI() { - return nil, false, errors.New("publicKeyOwner property is not provided or it is not embedded as a value") + errWithCode := gtserror.NewErrorNotAuthorized(errors.New("publicKeyOwner property is not provided or it is not embedded as a value")) + l.Debug(errWithCode) + return nil, errWithCode } pkOwnerURI = pkOwnerProp.GetIRI() } // after all that, public key should be defined if publicKey == nil { - return nil, false, errors.New("returned public key was empty") + errWithCode := gtserror.NewErrorInternalError(errors.New("returned public key was empty")) + l.Debug(errWithCode) + return nil, errWithCode } // do the actual authentication here! algos := []httpsig.Algorithm{ - httpsig.RSA_SHA512, httpsig.RSA_SHA256, + httpsig.RSA_SHA512, httpsig.ED25519, } @@ -244,11 +277,12 @@ func (f *federator) AuthenticateFederatedRequest(ctx context.Context, requestedU err := verifier.Verify(publicKey, algo) if err == nil { l.Tracef("authentication for %s PASSED with algorithm %s", pkOwnerURI, algo) - return pkOwnerURI, true, nil + return pkOwnerURI, nil } l.Tracef("authentication for %s NOT PASSED with algorithm %s: %s", pkOwnerURI, algo, err) } - l.Infof("authentication not passed for public key owner %s; signature value was '%s'", pkOwnerURI, signature) - return nil, false, nil + errWithCode := gtserror.NewErrorNotAuthorized(fmt.Errorf("authentication not passed for public key owner %s; signature value was '%s'", pkOwnerURI, signature)) + l.Debug(errWithCode) + return nil, errWithCode } diff --git a/internal/federation/federatingprotocol.go b/internal/federation/federatingprotocol.go index 78995981..7bcefc14 100644 --- a/internal/federation/federatingprotocol.go +++ b/internal/federation/federatingprotocol.go @@ -119,15 +119,17 @@ func (f *federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr return nil, false, fmt.Errorf("could not fetch receiving account with username %s: %s", username, err) } - publicKeyOwnerURI, authenticated, err := f.AuthenticateFederatedRequest(ctx, receivingAccount.Username) - if err != nil { - l.Debugf("request not authenticated: %s", err) - return ctx, false, err - } - - if !authenticated { - w.WriteHeader(http.StatusForbidden) - return ctx, false, nil + publicKeyOwnerURI, errWithCode := f.AuthenticateFederatedRequest(ctx, receivingAccount.Username) + if errWithCode != nil { + switch errWithCode.Code() { + case http.StatusUnauthorized, http.StatusForbidden, http.StatusBadRequest: + // if 400, 401, or 403, obey the interface by writing the header and bailing + w.WriteHeader(errWithCode.Code()) + return ctx, false, nil + default: + // if not, there's been a proper error + return ctx, false, err + } } // authentication has passed, so add an instance entry for this instance if it hasn't been done already diff --git a/internal/federation/federator.go b/internal/federation/federator.go index 4359e17d..61d7f3e1 100644 --- a/internal/federation/federator.go +++ b/internal/federation/federator.go @@ -27,6 +27,7 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/db" "github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing" "github.com/superseriousbusiness/gotosocial/internal/federation/federatingdb" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/media" "github.com/superseriousbusiness/gotosocial/internal/transport" @@ -50,7 +51,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, bool, error) + AuthenticateFederatedRequest(ctx context.Context, username string) (*url.URL, gtserror.WithCode) // FingerRemoteAccount performs a webfinger lookup for a remote account, using the .well-known path. It will return the ActivityPub URI for that // account, or an error if it doesn't exist or can't be retrieved. diff --git a/internal/processing/federation/getfollowers.go b/internal/processing/federation/getfollowers.go index c15b2b6c..a4903739 100644 --- a/internal/processing/federation/getfollowers.go +++ b/internal/processing/federation/getfollowers.go @@ -20,7 +20,6 @@ package federation import ( "context" - "errors" "fmt" "net/url" @@ -36,9 +35,9 @@ func (p *processor) GetFollowers(ctx context.Context, requestedUsername string, } // authenticate the request - requestingAccountURI, authenticated, err := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) - if err != nil || !authenticated { - return nil, gtserror.NewErrorNotAuthorized(errors.New("not authorized"), "not authorized") + requestingAccountURI, errWithCode := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) + if errWithCode != nil { + return nil, errWithCode } requestingAccount, err := p.federator.GetRemoteAccount(ctx, requestedUsername, requestingAccountURI, false, false) diff --git a/internal/processing/federation/getfollowing.go b/internal/processing/federation/getfollowing.go index d2beaada..a38c049f 100644 --- a/internal/processing/federation/getfollowing.go +++ b/internal/processing/federation/getfollowing.go @@ -20,7 +20,6 @@ package federation import ( "context" - "errors" "fmt" "net/url" @@ -36,9 +35,9 @@ func (p *processor) GetFollowing(ctx context.Context, requestedUsername string, } // authenticate the request - requestingAccountURI, authenticated, err := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) - if err != nil || !authenticated { - return nil, gtserror.NewErrorNotAuthorized(errors.New("not authorized"), "not authorized") + requestingAccountURI, errWithCode := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) + if errWithCode != nil { + return nil, errWithCode } requestingAccount, err := p.federator.GetRemoteAccount(ctx, requestedUsername, requestingAccountURI, false, false) diff --git a/internal/processing/federation/getoutbox.go b/internal/processing/federation/getoutbox.go index 2c7511e4..455f427f 100644 --- a/internal/processing/federation/getoutbox.go +++ b/internal/processing/federation/getoutbox.go @@ -20,7 +20,6 @@ package federation import ( "context" - "errors" "fmt" "net/url" @@ -37,9 +36,9 @@ func (p *processor) GetOutbox(ctx context.Context, requestedUsername string, pag } // authenticate the request - requestingAccountURI, authenticated, err := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) - if err != nil || !authenticated { - return nil, gtserror.NewErrorNotAuthorized(errors.New("not authorized"), "not authorized") + requestingAccountURI, errWithCode := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) + if errWithCode != nil { + return nil, errWithCode } requestingAccount, err := p.federator.GetRemoteAccount(ctx, requestedUsername, requestingAccountURI, false, false) diff --git a/internal/processing/federation/getstatus.go b/internal/processing/federation/getstatus.go index 3a32ffa5..820f1a19 100644 --- a/internal/processing/federation/getstatus.go +++ b/internal/processing/federation/getstatus.go @@ -20,7 +20,6 @@ package federation import ( "context" - "errors" "fmt" "net/url" @@ -38,9 +37,9 @@ func (p *processor) GetStatus(ctx context.Context, requestedUsername string, req } // authenticate the request - requestingAccountURI, authenticated, err := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) - if err != nil || !authenticated { - return nil, gtserror.NewErrorNotAuthorized(errors.New("not authorized"), "not authorized") + requestingAccountURI, errWithCode := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) + if errWithCode != nil { + return nil, errWithCode } requestingAccount, err := p.federator.GetRemoteAccount(ctx, requestedUsername, requestingAccountURI, false, false) diff --git a/internal/processing/federation/getstatusreplies.go b/internal/processing/federation/getstatusreplies.go index c6db4dd3..984f3a40 100644 --- a/internal/processing/federation/getstatusreplies.go +++ b/internal/processing/federation/getstatusreplies.go @@ -20,7 +20,6 @@ package federation import ( "context" - "errors" "fmt" "net/url" @@ -38,9 +37,9 @@ func (p *processor) GetStatusReplies(ctx context.Context, requestedUsername stri } // authenticate the request - requestingAccountURI, authenticated, err := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) - if err != nil || !authenticated { - return nil, gtserror.NewErrorNotAuthorized(errors.New("not authorized"), "not authorized") + requestingAccountURI, errWithCode := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) + if errWithCode != nil { + return nil, errWithCode } requestingAccount, err := p.federator.GetRemoteAccount(ctx, requestedUsername, requestingAccountURI, false, false) diff --git a/internal/processing/federation/getuser.go b/internal/processing/federation/getuser.go index b201bea4..f870baa1 100644 --- a/internal/processing/federation/getuser.go +++ b/internal/processing/federation/getuser.go @@ -20,7 +20,6 @@ package federation import ( "context" - "errors" "fmt" "net/url" @@ -46,13 +45,9 @@ func (p *processor) GetUser(ctx context.Context, requestedUsername string, reque } } else { // if it's any other path, we want to fully authenticate the request before we serve any data, and then we can serve a more complete profile - requestingAccountURI, authenticated, err := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) - if err != nil { - return nil, gtserror.NewErrorNotAuthorized(err, "not authorized") - } - - if !authenticated { - return nil, gtserror.NewErrorNotAuthorized(errors.New("not authorized"), "not authorized") + requestingAccountURI, errWithCode := p.federator.AuthenticateFederatedRequest(ctx, requestedUsername) + if errWithCode != nil { + return nil, errWithCode } // if we're not already handshaking/dereferencing a remote account, dereference it now