From 15de5ffd450223e1bc5abf80575d906dc2eabb6e Mon Sep 17 00:00:00 2001 From: Marcelo Salloum dos Santos Date: Wed, 18 Nov 2020 17:35:19 -0300 Subject: [PATCH] exp/services/recoverysigner: allow recoverysigner to sign transactions with operations containing a whitelisted source account (#3227) ### What Allow recoverysigner to sign transactions with operations containing another source account if it's listed in the config variables. ### Why The validation below prevents recoverysigner from signing CAP-33 `BeginSponsoringFutureReserves` operations https://github.com/stellar/go/blob/c2c77fb6066b7a06a0c31835039926ae7d9d3970/exp/services/recoverysigner/internal/serve/account_sign.go#L132-L138 For a `BeginSponsoringFutureReserves` operation we shouldn't be validating its account address but the sponsored id. --- exp/services/recoverysigner/README.md | 4 +- exp/services/recoverysigner/cmd/serve.go | 7 + .../internal/serve/account_sign.go | 26 ++- .../account_sign_signing_address_test.go | 154 ++++++++++++++++++ .../recoverysigner/internal/serve/serve.go | 58 ++++--- 5 files changed, 218 insertions(+), 31 deletions(-) diff --git a/exp/services/recoverysigner/README.md b/exp/services/recoverysigner/README.md index b3fbef1faa..9f6d28a922 100644 --- a/exp/services/recoverysigner/README.md +++ b/exp/services/recoverysigner/README.md @@ -1,7 +1,7 @@ # Recovery Signer This is an incomplete and work-in-progress implementation of the [SEP-30] -Recovery Signer protocol v0.4.0. +Recovery Signer protocol v0.7.0. A Recovery Signer is a server that can help a user regain control of a Stellar account if they have lost their secret key. A user registers their account with @@ -74,5 +74,5 @@ Flags: Use "recoverysigner db [command] --help" for more information about a command. ``` -[SEP-30]: https://github.com/stellar/stellar-protocol/blob/600c326b210d71ee031d7f3a40ca88191b4cdf9c/ecosystem/sep-0030.md +[SEP-30]: https://github.com/stellar/stellar-protocol/blob/3e05bb668f94793545588106af74699b8d6b02d6/ecosystem/sep-0030.md [README-Firebase.md]: README-Firebase.md diff --git a/exp/services/recoverysigner/cmd/serve.go b/exp/services/recoverysigner/cmd/serve.go index 6081bb4b11..4ee73ee812 100644 --- a/exp/services/recoverysigner/cmd/serve.go +++ b/exp/services/recoverysigner/cmd/serve.go @@ -95,6 +95,13 @@ func (c *ServeCommand) Command() *cobra.Command { FlagDefault: "recoverysigner", Required: false, }, + { + Name: "allowed-source-accounts", + Usage: "Stellar account(s) allowed as source accounts in transactions signed for all users in addition to the registered account comma separated (important: these accounts must never be registered accounts and must never have the signer configured that is a signing key used by this server)", + OptType: types.String, + ConfigKey: &opts.AllowedSourceAccounts, + Required: false, + }, } cmd := &cobra.Command{ Use: "serve", diff --git a/exp/services/recoverysigner/internal/serve/account_sign.go b/exp/services/recoverysigner/internal/serve/account_sign.go index a3d46a5897..97d3a805e3 100644 --- a/exp/services/recoverysigner/internal/serve/account_sign.go +++ b/exp/services/recoverysigner/internal/serve/account_sign.go @@ -13,10 +13,11 @@ import ( ) type accountSignHandler struct { - Logger *supportlog.Entry - SigningKeys []*keypair.Full - NetworkPassphrase string - AccountStore account.Store + Logger *supportlog.Entry + SigningKeys []*keypair.Full + NetworkPassphrase string + AccountStore account.Store + AllowedSourceAccounts []*keypair.FromAddress } type accountSignRequest struct { @@ -141,10 +142,21 @@ func (h accountSignHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if opSourceAccount == nil { continue } + if op.GetSourceAccount().GetAccountID() != req.Address.Address() { - l.Info("Operation's source account is not the account.") - badRequest.Render(w) - return + var opHasAllowedAccount bool + for _, sa := range h.AllowedSourceAccounts { + if sa.Address() == op.GetSourceAccount().GetAccountID() { + opHasAllowedAccount = true + break + } + } + + if !opHasAllowedAccount { + l.Info("Operation's source account is not the account in the request and not any account that is configured to be allowed.") + badRequest.Render(w) + return + } } } diff --git a/exp/services/recoverysigner/internal/serve/account_sign_signing_address_test.go b/exp/services/recoverysigner/internal/serve/account_sign_signing_address_test.go index 760332c113..a3cf6df73d 100644 --- a/exp/services/recoverysigner/internal/serve/account_sign_signing_address_test.go +++ b/exp/services/recoverysigner/internal/serve/account_sign_signing_address_test.go @@ -2,6 +2,7 @@ package serve import ( "context" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -985,6 +986,159 @@ func TestAccountSign_signingAddressEmailOwnerAuthenticated(t *testing.T) { assert.JSONEq(t, wantBody, string(body)) } +// Test that when the source account of the operation is listed in the allowed +// source accounts a successful response is returned. +func TestAccountSign_signingAddressEmailOwnerAuthenticatedOpSourceAccountIsAllowedSourceAccount(t *testing.T) { + s := &account.DBStore{DB: dbtest.Open(t).Open()} + s.Add(account.Account{ + Address: "GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4", + Identities: []account.Identity{ + { + Role: "sender", + AuthMethods: []account.AuthMethod{ + {Type: account.AuthMethodTypeEmail, Value: "user1@example.com"}, + }, + }, + }, + }) + h := accountSignHandler{ + Logger: supportlog.DefaultLogger, + AccountStore: s, + SigningKeys: []*keypair.Full{ + keypair.MustParseFull("SBIB72S6JMTGJRC6LMKLC5XMHZ2IOHZSZH4SASTN47LECEEJ7QEB6EYK"), // GBOG4KF66M4AFRBUHOTJQJRO7BGGFCSGIICTI5BHXHKXCWV2C67QRN5H + keypair.MustParseFull("SBJGZKZ7LU2FQNEFBUOBW4LHCA5BOZCABIJTR7BQIFWQ3P763ZW7MYDD"), // GAPE22DOMALCH42VOR4S3HN6KIZZ643G7D3GNTYF4YOWWXP6UVRAF5JS + }, + NetworkPassphrase: network.TestNetworkPassphrase, + AllowedSourceAccounts: []*keypair.FromAddress{ + keypair.MustParseAddress("GDR3RJVOHYR5A4RSLZ7D3GOSTPBGD2FY7KJD7ZB7363ROOQHWYDVVULS"), + }, + } + + tx, err := txnbuild.NewTransaction( + txnbuild.TransactionParams{ + SourceAccount: &txnbuild.SimpleAccount{AccountID: "GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4"}, + IncrementSequenceNum: true, + Operations: []txnbuild.Operation{ + &txnbuild.BeginSponsoringFutureReserves{ + SponsoredID: "GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4", + SourceAccount: &txnbuild.SimpleAccount{AccountID: "GDR3RJVOHYR5A4RSLZ7D3GOSTPBGD2FY7KJD7ZB7363ROOQHWYDVVULS"}, + }, + &txnbuild.SetOptions{ + Signer: &txnbuild.Signer{ + Address: "GD7CGJSJ5OBOU5KOP2UQDH3MPY75UTEY27HVV5XPSL2X6DJ2VGTOSXEU", + Weight: 20, + }, + }, + &txnbuild.EndSponsoringFutureReserves{}, + }, + BaseFee: txnbuild.MinBaseFee, + Timebounds: txnbuild.NewTimebounds(0, 1), + }, + ) + require.NoError(t, err) + txEnc, err := tx.Base64() + require.NoError(t, err) + t.Log("Tx:", txEnc) + + ctx := context.Background() + ctx = auth.NewContext(ctx, auth.Auth{Email: "user1@example.com"}) + req := fmt.Sprintf(`{"transaction": "%s"}`, txEnc) + r := httptest.NewRequest("POST", "/GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4/sign/GBOG4KF66M4AFRBUHOTJQJRO7BGGFCSGIICTI5BHXHKXCWV2C67QRN5H", strings.NewReader(req)) + r = r.WithContext(ctx) + + w := httptest.NewRecorder() + m := chi.NewMux() + m.Post("/{address}/sign/{signing-address}", h.ServeHTTP) + m.ServeHTTP(w, r) + resp := w.Result() + + require.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, "application/json; charset=utf-8", resp.Header.Get("Content-Type")) + + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + + wantBody := `{ + "signature": "Tpl/yZoKkahakaX4fSrdIeBLL2oi4uKegs5bLXFj5fG6Rcfe2D4EeSHcjJmmO2ZscuY8pX8+YPo70AvCtfw9Ag==", + "network_passphrase": "Test SDF Network ; September 2015" + }` + assert.JSONEq(t, wantBody, string(body)) +} + +// Test that when the source account of the operation is not the account sign +// the request is calling sign on a bad request response is returned. +func TestAccountSign_signingAddressEmailOwnerAuthenticatedOpSourceAccountInvalid(t *testing.T) { + s := &account.DBStore{DB: dbtest.Open(t).Open()} + s.Add(account.Account{ + Address: "GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4", + Identities: []account.Identity{ + { + Role: "sender", + AuthMethods: []account.AuthMethod{ + {Type: account.AuthMethodTypeEmail, Value: "user1@example.com"}, + }, + }, + }, + }) + h := accountSignHandler{ + Logger: supportlog.DefaultLogger, + AccountStore: s, + SigningKeys: []*keypair.Full{ + keypair.MustParseFull("SBIB72S6JMTGJRC6LMKLC5XMHZ2IOHZSZH4SASTN47LECEEJ7QEB6EYK"), // GBOG4KF66M4AFRBUHOTJQJRO7BGGFCSGIICTI5BHXHKXCWV2C67QRN5H + keypair.MustParseFull("SBJGZKZ7LU2FQNEFBUOBW4LHCA5BOZCABIJTR7BQIFWQ3P763ZW7MYDD"), // GAPE22DOMALCH42VOR4S3HN6KIZZ643G7D3GNTYF4YOWWXP6UVRAF5JS + }, + NetworkPassphrase: network.TestNetworkPassphrase, + AllowedSourceAccounts: nil, + } + + tx, err := txnbuild.NewTransaction( + txnbuild.TransactionParams{ + SourceAccount: &txnbuild.SimpleAccount{AccountID: "GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4"}, + IncrementSequenceNum: true, + Operations: []txnbuild.Operation{ + &txnbuild.BeginSponsoringFutureReserves{ + SponsoredID: "GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4", + SourceAccount: &txnbuild.SimpleAccount{AccountID: "GDR3RJVOHYR5A4RSLZ7D3GOSTPBGD2FY7KJD7ZB7363ROOQHWYDVVULS"}, + }, + &txnbuild.SetOptions{ + Signer: &txnbuild.Signer{ + Address: "GD7CGJSJ5OBOU5KOP2UQDH3MPY75UTEY27HVV5XPSL2X6DJ2VGTOSXEU", + Weight: 20, + }, + }, + &txnbuild.EndSponsoringFutureReserves{}, + }, + BaseFee: txnbuild.MinBaseFee, + Timebounds: txnbuild.NewTimebounds(0, 1), + }, + ) + require.NoError(t, err) + txEnc, err := tx.Base64() + require.NoError(t, err) + t.Log("Tx:", txEnc) + + ctx := context.Background() + ctx = auth.NewContext(ctx, auth.Auth{Email: "user1@example.com"}) + req := fmt.Sprintf(`{"transaction": "%s"}`, txEnc) + r := httptest.NewRequest("POST", "/GA6HNE7O2N2IXIOBZNZ4IPTS2P6DSAJJF5GD5PDLH5GYOZ6WMPSKCXD4/sign/GBOG4KF66M4AFRBUHOTJQJRO7BGGFCSGIICTI5BHXHKXCWV2C67QRN5H", strings.NewReader(req)) + r = r.WithContext(ctx) + + w := httptest.NewRecorder() + m := chi.NewMux() + m.Post("/{address}/sign/{signing-address}", h.ServeHTTP) + m.ServeHTTP(w, r) + resp := w.Result() + + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Equal(t, "application/json; charset=utf-8", resp.Header.Get("Content-Type")) + + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + + wantBody := `{"error": "The request was invalid in some way."}` + assert.JSONEq(t, wantBody, string(body)) +} + // Test that when authenticated with a email signing is possible. func TestAccountSign_signingAddressEmailOtherAuthenticated(t *testing.T) { s := &account.DBStore{DB: dbtest.Open(t).Open()} diff --git a/exp/services/recoverysigner/internal/serve/serve.go b/exp/services/recoverysigner/internal/serve/serve.go index ffd7149482..88a4200377 100644 --- a/exp/services/recoverysigner/internal/serve/serve.go +++ b/exp/services/recoverysigner/internal/serve/serve.go @@ -33,6 +33,8 @@ type Options struct { AdminPort int MetricsNamespace string + + AllowedSourceAccounts string } func Serve(opts Options) { @@ -63,15 +65,16 @@ func Serve(opts Options) { } type handlerDeps struct { - Logger *supportlog.Entry - NetworkPassphrase string - SigningKeys []*keypair.Full - SigningAddresses []*keypair.FromAddress - AccountStore account.Store - SEP10JWKS jose.JSONWebKeySet - SEP10JWTIssuer string - FirebaseAuthClient *firebaseauth.Client - MetricsRegistry *prometheus.Registry + Logger *supportlog.Entry + NetworkPassphrase string + SigningKeys []*keypair.Full + SigningAddresses []*keypair.FromAddress + AccountStore account.Store + SEP10JWKS jose.JSONWebKeySet + SEP10JWTIssuer string + FirebaseAuthClient *firebaseauth.Client + MetricsRegistry *prometheus.Registry + AllowedSourceAccounts []*keypair.FromAddress } func getHandlerDeps(opts Options) (handlerDeps, error) { @@ -141,16 +144,26 @@ func getHandlerDeps(opts Options) (handlerDeps, error) { opts.Logger.Warn("Error registering metric for accounts count: ", err) } + allowedSourceAccounts := []*keypair.FromAddress{} + for _, addressStr := range strings.Split(opts.AllowedSourceAccounts, ",") { + accountAddress, err := keypair.ParseAddress(addressStr) + if err != nil { + return handlerDeps{}, errors.Wrap(err, "parsing allowed source accounts") + } + allowedSourceAccounts = append(allowedSourceAccounts, accountAddress) + } + deps := handlerDeps{ - Logger: opts.Logger, - NetworkPassphrase: opts.NetworkPassphrase, - SigningKeys: signingKeys, - SigningAddresses: signingAddresses, - AccountStore: accountStore, - SEP10JWKS: sep10JWKS, - SEP10JWTIssuer: opts.SEP10JWTIssuer, - FirebaseAuthClient: firebaseAuthClient, - MetricsRegistry: metricsRegistry, + Logger: opts.Logger, + NetworkPassphrase: opts.NetworkPassphrase, + SigningKeys: signingKeys, + SigningAddresses: signingAddresses, + AccountStore: accountStore, + SEP10JWKS: sep10JWKS, + SEP10JWTIssuer: opts.SEP10JWTIssuer, + FirebaseAuthClient: firebaseAuthClient, + MetricsRegistry: metricsRegistry, + AllowedSourceAccounts: allowedSourceAccounts, } return deps, nil @@ -193,10 +206,11 @@ func handler(deps handlerDeps) http.Handler { AccountStore: deps.AccountStore, }.ServeHTTP) signHandler := accountSignHandler{ - Logger: deps.Logger, - SigningKeys: deps.SigningKeys, - NetworkPassphrase: deps.NetworkPassphrase, - AccountStore: deps.AccountStore, + Logger: deps.Logger, + SigningKeys: deps.SigningKeys, + NetworkPassphrase: deps.NetworkPassphrase, + AccountStore: deps.AccountStore, + AllowedSourceAccounts: deps.AllowedSourceAccounts, } mux.Post("/sign", signHandler.ServeHTTP) mux.Post("/sign/{signing-address}", signHandler.ServeHTTP)