commit 9cf66bf29871dad6523f421ae72125d584ac4c9a
parent 728c4a5e380cd2feef6f0f49422b3dd8297c71db
Author: tobi <31960611+tsmethurst@users.noreply.github.com>
Date: Tue, 26 Apr 2022 18:10:11 +0200
[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
Diffstat:
9 files changed, 92 insertions(+), 65 deletions(-)
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
