From 7f8d793ddbf6852b9a81f979cb13236491ae1dd6 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 17 Dec 2019 09:11:27 -0800 Subject: [PATCH 01/29] txnbuild: update challenge tx helpers for sep-10 changes --- txnbuild/transaction.go | 166 ++++- .../transaction_challenge_example_test.go | 229 ++++++ txnbuild/transaction_test.go | 691 ++++++++++++++++++ 3 files changed, 1046 insertions(+), 40 deletions(-) create mode 100644 txnbuild/transaction_challenge_example_test.go diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 1210e73158..d947984e21 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -16,6 +16,7 @@ import ( "encoding/base64" "encoding/hex" "fmt" + "strings" "time" "github.com/stellar/go/keypair" @@ -413,77 +414,142 @@ func (tx *Transaction) SignWithKeyString(keys ...string) error { return tx.Sign(signers...) } -// VerifyChallengeTx is a factory method that verifies a SEP 10 challenge transaction, -// for use in web authentication. It can be used by a server to verify that the challenge -// has been signed by the client. -// More details on SEP 10: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0010.md -func VerifyChallengeTx(challengeTx, serverAccountID, network string) (bool, error) { - tx, err := TransactionFromXDR(challengeTx) +// ReadChallengeTx verifies that a SEP 10 challenge transaction for use in web +// authentication is signed by the server and returns the transaction and +// client account ID. +// +// It does not verify that the transaction has been signed by the client or +// that any other signatures other than the servers on the transaction are +// valid. Use VerifyChallengeTxSigners to verify a set of client signers have +// signed the transaction. +func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transaction, clientAccountID string, err error) { + tx, err = TransactionFromXDR(challengeTx) if err != nil { - return false, err + return tx, clientAccountID, err } tx.Network = network // verify transaction source if tx.SourceAccount == nil { - return false, errors.New("transaction requires a source account") + return tx, clientAccountID, errors.New("transaction requires a source account") } if tx.SourceAccount.GetAccountID() != serverAccountID { - return false, errors.New("transaction source account is not equal to server's account") + return tx, clientAccountID, errors.New("transaction source account is not equal to server's account") } //verify sequence number txSourceAccount, ok := tx.SourceAccount.(*SimpleAccount) if !ok { - return false, errors.New("source account is not of type SimpleAccount unable to verify sequence number") + return tx, clientAccountID, errors.New("source account is not of type SimpleAccount unable to verify sequence number") } if txSourceAccount.Sequence != 0 { - return false, errors.New("transaction sequence number must be 0") + return tx, clientAccountID, errors.New("transaction sequence number must be 0") } // verify timebounds if tx.Timebounds.MaxTime == TimeoutInfinite { - return false, errors.New("transaction requires non-infinite timebounds") + return tx, clientAccountID, errors.New("transaction requires non-infinite timebounds") } currentTime := time.Now().UTC().Unix() if currentTime < tx.Timebounds.MinTime || currentTime > tx.Timebounds.MaxTime { - return false, errors.Errorf("transaction is not within range of the specified timebounds (currentTime=%d, MinTime=%d, MaxTime=%d)", + return tx, clientAccountID, errors.Errorf("transaction is not within range of the specified timebounds (currentTime=%d, MinTime=%d, MaxTime=%d)", currentTime, tx.Timebounds.MinTime, tx.Timebounds.MaxTime) } // verify operation if len(tx.Operations) != 1 { - return false, errors.New("transaction requires a single manage_data operation") + return tx, clientAccountID, errors.New("transaction requires a single manage_data operation") } op, ok := tx.Operations[0].(*ManageData) if !ok { - return false, errors.New("operation type should be manage_data") + return tx, clientAccountID, errors.New("operation type should be manage_data") } if op.SourceAccount == nil { - return false, errors.New("operation should have a source account") + return tx, clientAccountID, errors.New("operation should have a source account") } + clientAccountID = op.SourceAccount.GetAccountID() // verify manage data value nonceB64 := string(op.Value) if len(nonceB64) != 64 { - return false, errors.New("random nonce encoded as base64 should be 64 bytes long") + return tx, clientAccountID, errors.New("random nonce encoded as base64 should be 64 bytes long") } nonceBytes, err := base64.StdEncoding.DecodeString(nonceB64) if err != nil { - return false, errors.Wrap(err, "failed to decode random nonce provided in manage_data operation") + return tx, clientAccountID, errors.Wrap(err, "failed to decode random nonce provided in manage_data operation") } if len(nonceBytes) != 48 { - return false, errors.New("random nonce before encoding as base64 should be 48 bytes long") + return tx, clientAccountID, errors.New("random nonce before encoding as base64 should be 48 bytes long") + } + + err = verifyTxSignature(tx, serverAccountID) + if err != nil { + return tx, clientAccountID, err + } + + return tx, clientAccountID, nil +} + +// VerifyChallengeTxSigners verifies that for a SEP 10 challenge transaction +// all signatures on the transaction are accounted for. A transaction is +// verified if it is signed by the server account, and all other signatures +// match a signer that has been provided as an argument. Additional signers can +// be provided that do not have a signature, but all signatures must be matched +// to a signer for verification to succeed. If verification succeeds a list of +// signers that were found is returned, excluding the server account ID. +// +// Errors will be raised if: +// - The transaction is invalid according to ReadChallengeTx. +// - No client signatures are found on the transaction. +// - One or more signatures in the transaction are not identifiable as the server account or one of the signers provided in the arguments. +func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, signers ...string) ([]string, error) { + tx, _, err := ReadChallengeTx(challengeTx, serverAccountID, network) + if err != nil { + return nil, err + } + + serverKP, err := keypair.ParseAddress(serverAccountID) + if err != nil { + return nil, err + } + + // Remove the server signer from the signers list if it is present. It's + // important when verifying signers of a challenge transaction that we only + // verify and return client signers. If an account has the server as a + // signer the server should not play a part in the authentication of the + // client. + clientSigners := make([]string, 0, len(signers)-1) + for _, signer := range signers { + if signer == serverKP.Address() { + continue + } + clientSigners = append(clientSigners, signer) + } + + signersFound, err := verifyTxSignatures(tx, clientSigners...) + if err != nil { + return nil, err } + if len(signersFound) != len(tx.xdrEnvelope.Signatures)-1 { + return signersFound, errors.Errorf("transaction has unrecognized signatures") + } + + return signersFound, nil +} - // verify signature from operation source - err = verifyTxSignature(tx, op.SourceAccount.GetAccountID()) +// VerifyChallengeTx is a factory method that verifies a SEP 10 challenge transaction, +// for use in web authentication. It can be used by a server to verify that the challenge +// has been signed by the client. +// More details on SEP 10: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0010.md +// +// Deprecated: Use VerifyChallengeTxSigners. +func VerifyChallengeTx(challengeTx, serverAccountID, network string) (bool, error) { + tx, clientAccountID, err := ReadChallengeTx(challengeTx, serverAccountID, network) if err != nil { return false, err } - // verify signature from server signing key - err = verifyTxSignature(tx, serverAccountID) + err = verifyTxSignature(tx, clientAccountID) if err != nil { return false, err } @@ -492,33 +558,53 @@ func VerifyChallengeTx(challengeTx, serverAccountID, network string) (bool, erro } // verifyTxSignature checks if a transaction has been signed by the provided Stellar account. -func verifyTxSignature(tx Transaction, accountID string) error { +func verifyTxSignature(tx Transaction, signer string) error { + _, err := verifyTxSignatures(tx, signer) + return err +} + +// verifyTxSignature checks if a transaction has been signed by one or more of +// the signers, returning a list of signers that were found to have signed the +// transaction. +func verifyTxSignatures(tx Transaction, signers ...string) ([]string, error) { if tx.xdrEnvelope == nil { - return errors.New("transaction has no signatures") + return nil, errors.New("transaction has no signatures") } txHash, err := tx.Hash() if err != nil { - return err - } - - kp, err := keypair.Parse(accountID) - if err != nil { - return err + return nil, err } // find and verify signatures - signerFound := false - for _, s := range tx.xdrEnvelope.Signatures { - e := kp.Verify(txHash[:], s.Signature) - if e == nil { - signerFound = true - break + signersFound := map[string]struct{}{} + for _, signer := range signers { + kp, err := keypair.ParseAddress(signer) + if err != nil { + return nil, err + } + + for _, decSig := range tx.xdrEnvelope.Signatures { + if decSig.Hint != kp.Hint() { + continue + } + err := kp.Verify(txHash[:], decSig.Signature) + if err == nil { + signersFound[signer] = struct{}{} + break + } } } - if !signerFound { - return errors.Errorf("transaction not signed by %s", accountID) + if len(signersFound) == 0 { + return nil, errors.Errorf("transaction not signed by %s", strings.Join(signers, ", ")) } - return nil + signersFoundList := make([]string, 0, len(signersFound)) + for _, signer := range signers { + if _, ok := signersFound[signer]; ok { + signersFoundList = append(signersFoundList, signer) + delete(signersFound, signer) + } + } + return signersFoundList, nil } diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go new file mode 100644 index 0000000000..f62ec0895e --- /dev/null +++ b/txnbuild/transaction_challenge_example_test.go @@ -0,0 +1,229 @@ +package txnbuild_test + +import ( + "fmt" + "time" + + "github.com/stellar/go/keypair" + "github.com/stellar/go/network" + "github.com/stellar/go/txnbuild" +) + +func ExampleVerifyChallengeTxSigners_verifyAllClientSigners() { + serverAccount := keypair.MustRandom() + clientAccount := keypair.MustRandom() + clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") + clientSigner2, _ := keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") + + // Server builds challenge transaction + var challengeTx string + { + tx, err := txnbuild.BuildChallengeTx(serverAccount.Seed(), clientAccount.Address(), "test", network.TestNetworkPassphrase, time.Minute) + if err != nil { + fmt.Println("Error:", err) + return + } + challengeTx = tx + } + + // Client reads and signs challenge transaction + var signedChallengeTx string + { + tx, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) + if err != nil { + fmt.Println("Error:", err) + return + } + if txClientAccountID != clientAccount.Address() { + fmt.Println("Error: challenge tx is not for expected client account") + return + } + err = tx.Sign(clientSigner1, clientSigner2) + if err != nil { + fmt.Println("Error:", err) + return + } + signedChallengeTx, err = tx.Base64() + if err != nil { + fmt.Println("Error:", err) + return + } + } + + // Server verifies signed challenge transaction + { + // Server gets list of account's signers + clientSigners := []string{clientSigner1.Address(), clientSigner2.Address()} + fmt.Println("Client Signers:") + for _, clientSigner := range clientSigners { + fmt.Println(clientSigner) + } + + // Server finds which client signers are found on transaction + signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, clientSigners...) + if err != nil { + fmt.Println("Error:", err) + return + } + + // Server checks that all client signers were found on transaction + if len(signersFound) != len(clientSigners) { + fmt.Println("Error: not all client signers signed the challenge tx") + return + } + + fmt.Println("Client Signers Verified:") + for _, signerFound := range signersFound { + fmt.Println(signerFound) + } + } + + // Output: + // Client Signers: + // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 + // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP + // Client Signers Verified: + // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 + // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP +} + +func ExampleVerifyChallengeTxSigners_verifyAnyClientSigners() { + serverAccount := keypair.MustRandom() + clientAccount := keypair.MustRandom() + clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") + clientSigner2, _ := keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") + + // Server builds challenge transaction + var challengeTx string + { + tx, err := txnbuild.BuildChallengeTx(serverAccount.Seed(), clientAccount.Address(), "test", network.TestNetworkPassphrase, time.Minute) + if err != nil { + fmt.Println("Error:", err) + return + } + challengeTx = tx + } + + // Client reads and signs challenge transaction + var signedChallengeTx string + { + tx, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) + if err != nil { + fmt.Println("Error:", err) + return + } + if txClientAccountID != clientAccount.Address() { + fmt.Println("Error: challenge tx is not for expected client account") + return + } + // Signs with only one of the account's signers + err = tx.Sign(clientSigner2) + if err != nil { + fmt.Println("Error:", err) + return + } + signedChallengeTx, err = tx.Base64() + if err != nil { + fmt.Println("Error:", err) + return + } + } + + // Server verifies signed challenge transaction + { + // Server gets list of account's signers + clientSigners := []string{clientSigner1.Address(), clientSigner2.Address()} + fmt.Println("Client Signers:") + for _, clientSigner := range clientSigners { + fmt.Println(clientSigner) + } + + // Server finds which client signers are found on transaction + signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, clientSigners...) + if err != nil { + fmt.Println("Error:", err) + return + } + + fmt.Println("Client Signers Verified:") + for _, signerFound := range signersFound { + fmt.Println(signerFound) + } + } + + // Output: + // Client Signers: + // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 + // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP + // Client Signers Verified: + // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP +} + +func ExampleVerifyChallengeTxSigners_verifyClientMasterKeySigned() { + serverAccount := keypair.MustRandom() + clientAccount, _ := keypair.ParseFull("SDZQ46SVA4VEE4WGBKZMN76JDMFRNEICSLWPIGNINLEOFOSB7AVMV2PL") + + // Server builds challenge transaction + var challengeTx string + { + tx, err := txnbuild.BuildChallengeTx(serverAccount.Seed(), clientAccount.Address(), "test", network.TestNetworkPassphrase, time.Minute) + if err != nil { + fmt.Println("Error:", err) + return + } + challengeTx = tx + } + + // Client reads and signs challenge transaction + var signedChallengeTx string + { + tx, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) + if err != nil { + fmt.Println("Error:", err) + return + } + if txClientAccountID != clientAccount.Address() { + fmt.Println("Error: challenge tx is not for expected client account") + return + } + // Signs with account master key + err = tx.Sign(clientAccount) + if err != nil { + fmt.Println("Error:", err) + return + } + signedChallengeTx, err = tx.Base64() + if err != nil { + fmt.Println("Error:", err) + return + } + } + + // Server verifies signed challenge transaction + { + // Server gets list of account's signers + clientSigners := []string{clientAccount.Address()} + fmt.Println("Client Signers:") + for _, clientSigner := range clientSigners { + fmt.Println(clientSigner) + } + + // Server finds which client signers are found on transaction + signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, clientSigners...) + if err != nil { + fmt.Println("Error:", err) + return + } + + fmt.Println("Client Signers Verified:") + for _, signerFound := range signersFound { + fmt.Println(signerFound) + } + } + + // Output: + // Client Signers: + // GDTEJZDQADCQZ4U6AFPWCOO55DYZCQ332KLYHHGEJIOFFL5HZN5MEGVY + // Client Signers Verified: + // GDTEJZDQADCQZ4U6AFPWCOO55DYZCQ332KLYHHGEJIOFFL5HZN5MEGVY +} diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index 7a4f7fa296..5b65e1d01e 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -3,6 +3,7 @@ package txnbuild import ( "crypto/sha256" "encoding/base64" + "strings" "testing" "time" @@ -1163,6 +1164,696 @@ func TestSignWithSecretKey(t *testing.T) { assert.Equal(t, expected, actual, "base64 xdr should match") } +func TestReadChallengeTx_validSignedByServerAndClient(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, clientKP.Address(), readClientAccountID) + assert.NoError(t, err) +} + +func TestReadChallengeTx_validSignedByServer(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, clientKP.Address(), readClientAccountID) + assert.NoError(t, err) +} + +func TestReadChallengeTx_invalidNotSignedByServer(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, clientKP.Address(), readClientAccountID) + assert.EqualError(t, err, "transaction not signed by "+serverKP.Address()) +} + +func TestReadChallengeTx_invalidCorrupted(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + tx64 = strings.ReplaceAll(tx64, "A", "B") + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, Transaction{}, readTx) + assert.Equal(t, "", readClientAccountID) + assert.EqualError(t, err, "unable to unmarshal transaction envelope: xdr:decode: switch '68174084' is not valid enum value for union") +} + +func TestReadChallengeTx_invalidServerAccountIDMismatch(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(newKeypair2().Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, "", readClientAccountID) + assert.EqualError(t, err, "transaction source account is not equal to server's account") +} + +func TestReadChallengeTx_invalidSeqNoNotZero(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), 1234) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, "", readClientAccountID) + assert.EqualError(t, err, "transaction sequence number must be 0") +} + +func TestReadChallengeTx_invalidTimeboundsInfinite(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewInfiniteTimeout(), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, "", readClientAccountID) + assert.EqualError(t, err, "transaction requires non-infinite timebounds") +} + +func TestReadChallengeTx_invalidTimeboundsOutsideRange(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimebounds(0, 100), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, "", readClientAccountID) + assert.Error(t, err) + assert.Regexp(t, "transaction is not within range of the specified timebounds", err.Error()) +} + +func TestReadChallengeTx_invalidTooManyOperations(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op, &op}, + Timebounds: NewTimeout(300), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, "", readClientAccountID) + assert.EqualError(t, err, "transaction requires a single manage_data operation") +} + +func TestReadChallengeTx_invalidOperationWrongType(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := BumpSequence{ + SourceAccount: &opSource, + BumpTo: 0, + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(300), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, "", readClientAccountID) + assert.EqualError(t, err, "operation type should be manage_data") +} + +func TestReadChallengeTx_invalidOperationNoSourceAccount(t *testing.T) { + serverKP := newKeypair0() + txSource := NewSimpleAccount(serverKP.Address(), -1) + op := ManageData{ + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(300), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, "", readClientAccountID) + assert.EqualError(t, err, "operation should have a source account") +} + +func TestReadChallengeTx_invalidDataValueWrongEncodedLength(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 45))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(300), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, clientKP.Address(), readClientAccountID) + assert.EqualError(t, err, "random nonce encoded as base64 should be 64 bytes long") +} + +func TestReadChallengeTx_invalidDataValueCorruptBase64(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA?AAAAAAAAAAAAAAAAAAAAAAAAAA"), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(300), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, clientKP.Address(), readClientAccountID) + assert.EqualError(t, err, "failed to decode random nonce provided in manage_data operation: illegal base64 data at input byte 37") +} + +func TestReadChallengeTx_invalidDataValueWrongByteLength(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 47))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(300), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + readTx, readClientAccountID, err := ReadChallengeTx(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.Equal(t, tx, readTx) + assert.Equal(t, clientKP.Address(), readClientAccountID) + assert.EqualError(t, err, "random nonce before encoding as base64 should be 48 bytes long") +} + +func TestVerifyChallengeTxSigners_validServerAndClientMasterKey(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP.Address()) + assert.Equal(t, []string{clientKP.Address()}, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxSigners_invalidServerAndNoClient(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP.Address()) + assert.Empty(t, signersFound) + assert.EqualError(t, err, "transaction not signed by "+clientKP.Address()) +} + +func TestVerifyChallengeTxSigners_invalidServerAndUnrecognizedClient(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + unrecognizedKP := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, unrecognizedKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP.Address()) + assert.Empty(t, signersFound) + assert.EqualError(t, err, "transaction not signed by "+clientKP.Address()) +} + +func TestVerifyChallengeTxSigners_validServerAndMultipleClientSigners(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP.Address(), clientKP2.Address()) + assert.Equal(t, []string{clientKP.Address(), clientKP2.Address()}, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxSigners_validServerAndMultipleClientSignersReverseOrder(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP2, clientKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP.Address(), clientKP2.Address()) + assert.Equal(t, []string{clientKP.Address(), clientKP2.Address()}, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxSigners_validServerAndClientSignersNotMasterKey(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP2.Address()) + assert.Equal(t, []string{clientKP2.Address()}, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxSigners_validServerAndClientSignersIgnoresServerSigner(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, serverKP.Address(), clientKP2.Address()) + assert.Equal(t, []string{clientKP2.Address()}, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxSigners_invalidServerNoClientSignersIgnoresServerSigner(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, serverKP.Address(), clientKP2.Address()) + assert.Empty(t, signersFound) + assert.EqualError(t, err, "transaction not signed by "+clientKP2.Address()) +} + +func TestVerifyChallengeTxSigners_validServerAndClientSignersIgnoresDuplicateSigner(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP2.Address(), clientKP2.Address()) + assert.Equal(t, []string{clientKP2.Address()}, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxSigners_invalidServerAndClientSignersFailsDuplicateSignatures(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP2, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP2.Address()) + assert.Equal(t, []string{clientKP2.Address()}, signersFound) + assert.EqualError(t, err, "transaction has unrecognized signatures") +} + func TestVerifyTxSignatureUnsignedTx(t *testing.T) { kp0 := newKeypair0() kp1 := newKeypair1() From 1bd551f58d21d829f8affd8b2f100243f01baa98 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 17 Dec 2019 17:40:34 -0800 Subject: [PATCH 02/29] feedback from @ire-and-curses --- txnbuild/transaction.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index d947984e21..54d110dad5 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -437,7 +437,7 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti return tx, clientAccountID, errors.New("transaction source account is not equal to server's account") } - //verify sequence number + // verify sequence number txSourceAccount, ok := tx.SourceAccount.(*SimpleAccount) if !ok { return tx, clientAccountID, errors.New("source account is not of type SimpleAccount unable to verify sequence number") @@ -501,7 +501,8 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti // Errors will be raised if: // - The transaction is invalid according to ReadChallengeTx. // - No client signatures are found on the transaction. -// - One or more signatures in the transaction are not identifiable as the server account or one of the signers provided in the arguments. +// - One or more signatures in the transaction are not identifiable as the +// server account or one of the signers provided in the arguments. func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, signers ...string) ([]string, error) { tx, _, err := ReadChallengeTx(challengeTx, serverAccountID, network) if err != nil { @@ -518,7 +519,7 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign // verify and return client signers. If an account has the server as a // signer the server should not play a part in the authentication of the // client. - clientSigners := make([]string, 0, len(signers)-1) + clientSigners := make([]string, 0, len(signers)) for _, signer := range signers { if signer == serverKP.Address() { continue From ad17003cc1a869662c5691fcce699b08f12adaf6 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Fri, 20 Dec 2019 17:42:59 -0800 Subject: [PATCH 03/29] Add examples of using Horizon during verification --- .../transaction_challenge_example_test.go | 91 ++++++++++++++----- 1 file changed, 67 insertions(+), 24 deletions(-) diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index f62ec0895e..6c8dc02e49 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -4,14 +4,36 @@ import ( "fmt" "time" + "github.com/stellar/go/clients/horizon" + "github.com/stellar/go/clients/horizonclient" "github.com/stellar/go/keypair" "github.com/stellar/go/network" "github.com/stellar/go/txnbuild" ) +var clientAccount, _ = keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") +var clientSigner1, _ = keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") +var clientSigner2, _ = keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") +var horizonClient = func() horizonclient.ClientInterface { + client := &horizonclient.MockClient{} + client. + On("AccountDetail", horizonclient.AccountRequest{AccountID: clientAccount.Address()}). + Return( + horizon.Account{ + Thresholds: horizon.AccountThresholds{LowThreshold: 1, MedThreshold: 10, HighThreshold: 100}, + Signers: []horizon.Signer{ + {Key: clientSigner1.Address(), Weight: 40}, + {Key: clientSigner2.Address(), Weight: 60}, + }, + }, + nil, + ) + return client +}() + func ExampleVerifyChallengeTxSigners_verifyAllClientSigners() { - serverAccount := keypair.MustRandom() - clientAccount := keypair.MustRandom() + serverAccount, _ := keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") + clientAccount, _ := keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") clientSigner2, _ := keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") @@ -52,11 +74,23 @@ func ExampleVerifyChallengeTxSigners_verifyAllClientSigners() { // Server verifies signed challenge transaction { + _, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) + if err != nil { + fmt.Println("Error:", err) + return + } + // Server gets list of account's signers - clientSigners := []string{clientSigner1.Address(), clientSigner2.Address()} + horizonClientAccount, err := horizonClient.AccountDetail(horizonclient.AccountRequest{AccountID: txClientAccountID}) + if err != nil { + fmt.Println("Error:", err) + return + } + clientSigners := []string{} fmt.Println("Client Signers:") - for _, clientSigner := range clientSigners { - fmt.Println(clientSigner) + for _, horizonSigner := range horizonClientAccount.Signers { + fmt.Println(horizonSigner.Key) + clientSigners = append(clientSigners, horizonSigner.Key) } // Server finds which client signers are found on transaction @@ -88,9 +122,9 @@ func ExampleVerifyChallengeTxSigners_verifyAllClientSigners() { } func ExampleVerifyChallengeTxSigners_verifyAnyClientSigners() { - serverAccount := keypair.MustRandom() - clientAccount := keypair.MustRandom() - clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") + serverAccount, _ := keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") + clientAccount, _ := keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") + //clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") clientSigner2, _ := keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") // Server builds challenge transaction @@ -131,11 +165,23 @@ func ExampleVerifyChallengeTxSigners_verifyAnyClientSigners() { // Server verifies signed challenge transaction { + _, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) + if err != nil { + fmt.Println("Error:", err) + return + } + // Server gets list of account's signers - clientSigners := []string{clientSigner1.Address(), clientSigner2.Address()} + horizonClientAccount, err := horizonClient.AccountDetail(horizonclient.AccountRequest{AccountID: txClientAccountID}) + if err != nil { + fmt.Println("Error:", err) + return + } + clientSigners := []string{} fmt.Println("Client Signers:") - for _, clientSigner := range clientSigners { - fmt.Println(clientSigner) + for _, horizonSigner := range horizonClientAccount.Signers { + fmt.Println(horizonSigner.Key) + clientSigners = append(clientSigners, horizonSigner.Key) } // Server finds which client signers are found on transaction @@ -160,8 +206,8 @@ func ExampleVerifyChallengeTxSigners_verifyAnyClientSigners() { } func ExampleVerifyChallengeTxSigners_verifyClientMasterKeySigned() { - serverAccount := keypair.MustRandom() - clientAccount, _ := keypair.ParseFull("SDZQ46SVA4VEE4WGBKZMN76JDMFRNEICSLWPIGNINLEOFOSB7AVMV2PL") + serverAccount, _ := keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") + clientAccount, _ := keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") // Server builds challenge transaction var challengeTx string @@ -201,29 +247,26 @@ func ExampleVerifyChallengeTxSigners_verifyClientMasterKeySigned() { // Server verifies signed challenge transaction { - // Server gets list of account's signers - clientSigners := []string{clientAccount.Address()} - fmt.Println("Client Signers:") - for _, clientSigner := range clientSigners { - fmt.Println(clientSigner) + _, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) + if err != nil { + fmt.Println("Error:", err) + return } // Server finds which client signers are found on transaction - signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, clientSigners...) + signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, txClientAccountID) if err != nil { fmt.Println("Error:", err) return } - fmt.Println("Client Signers Verified:") + fmt.Println("Client Master Key Verified:") for _, signerFound := range signersFound { fmt.Println(signerFound) } } // Output: - // Client Signers: - // GDTEJZDQADCQZ4U6AFPWCOO55DYZCQ332KLYHHGEJIOFFL5HZN5MEGVY - // Client Signers Verified: - // GDTEJZDQADCQZ4U6AFPWCOO55DYZCQ332KLYHHGEJIOFFL5HZN5MEGVY + // Client Master Key Verified: + // GBOHBZB3Q3RMKXO3OSLJRDJNUSAPOXDVBDOMA52VHIGCIQEVZSXQ44CW } From c3a71698b9d13eb9c8e1dd8cf0243489a591dce1 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Fri, 20 Dec 2019 20:47:42 -0800 Subject: [PATCH 04/29] Simplify the interface and make it easier to implement a check correctly --- txnbuild/set_options.go | 20 ++++ txnbuild/set_options_test.go | 18 +++ txnbuild/transaction.go | 43 +++++++ .../transaction_challenge_example_test.go | 113 ++---------------- 4 files changed, 90 insertions(+), 104 deletions(-) diff --git a/txnbuild/set_options.go b/txnbuild/set_options.go index 1ec99970fe..ce2a875721 100644 --- a/txnbuild/set_options.go +++ b/txnbuild/set_options.go @@ -1,6 +1,7 @@ package txnbuild import ( + "github.com/stellar/go/clients/horizon" "github.com/stellar/go/support/errors" "github.com/stellar/go/xdr" ) @@ -32,6 +33,25 @@ type Signer struct { Weight Threshold } +// SignerFromHorizon converts a signer retrieved from Horizon to a txnbuild +// Signer. +func SignerFromHorizon(horizonSigner horizon.Signer) Signer { + return Signer{ + Address: horizonSigner.Key, + Weight: Threshold(horizonSigner.Weight), + } +} + +// SignersFromHorizon converts a list of signers retrieved from Horizon to +// txnbuild Signers. +func SignersFromHorizon(horizonSigners []horizon.Signer) []Signer { + signers := make([]Signer, 0, len(horizonSigners)) + for _, hs := range horizonSigners { + signers = append(signers, SignerFromHorizon(hs)) + } + return signers +} + // NewHomeDomain is syntactic sugar that makes instantiating SetOptions more convenient. func NewHomeDomain(hd string) *string { return &hd diff --git a/txnbuild/set_options_test.go b/txnbuild/set_options_test.go index 0398f37257..6f0ef3913c 100644 --- a/txnbuild/set_options_test.go +++ b/txnbuild/set_options_test.go @@ -3,6 +3,7 @@ package txnbuild import ( "testing" + "github.com/stellar/go/clients/horizon" "github.com/stellar/go/xdr" "github.com/stretchr/testify/assert" ) @@ -136,3 +137,20 @@ func TestEmptyHomeDomainOK(t *testing.T) { assert.Equal(t, string(*options.xdrOp.HomeDomain), "", "empty string home domain is set") } + +func TestSignersFromHorizon(t *testing.T) { + horizonSigners := []horizon.Signer{ + {Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 0}, + {Key: "GAT4FUGQNKOIDLOIXCJJYFSAFJHQY5MZEZLRBXXFKDCXGUHJQ63XZFTD", Weight: 10}, + {Key: "GCB35H32SU5ME4OALQUPOM4AADJICL2H2NLWAGLOMMTSYOVTXWYP6Q4T", Weight: 100}, + {Key: "GAVJHRCK5CEFE3MHL4JALMNX35Z5NLUIODSJIC44VRRLQQZGTDJWANV4", Weight: 255}, + } + wantSigners := []Signer{ + {Address: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 0}, + {Address: "GAT4FUGQNKOIDLOIXCJJYFSAFJHQY5MZEZLRBXXFKDCXGUHJQ63XZFTD", Weight: 10}, + {Address: "GCB35H32SU5ME4OALQUPOM4AADJICL2H2NLWAGLOMMTSYOVTXWYP6Q4T", Weight: 100}, + {Address: "GAVJHRCK5CEFE3MHL4JALMNX35Z5NLUIODSJIC44VRRLQQZGTDJWANV4", Weight: 255}, + } + signers := SignersFromHorizon(horizonSigners) + assert.Equal(t, wantSigners, signers) +} diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 54d110dad5..14448ba8f8 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -490,6 +490,49 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti return tx, clientAccountID, nil } +// VerifyChallengeTxThreshold verifies that for a SEP 10 challenge transaction +// all signatures on the transaction are accounted for and that the signatures +// meet a threshold on an account. A transaction is verified if it is signed by +// the server account, and all other signatures match a signer that has been +// provided as an argument, and those signatures meet a threshold on the +// account. +// +// Errors will be raised if: +// - The transaction is invalid according to ReadChallengeTx. +// - No client signatures are found on the transaction. +// - One or more signatures in the transaction are not identifiable as the +// server account or one of the signers provided in the arguments. +// - The signatures are all valid but do not meet the threshold. +func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, threshold Threshold, signers []Signer) (signersFound []Signer, err error) { + signerMap := map[string]Signer{} + signerAddresses := make([]string, 0, len(signers)) + for _, s := range signers { + signerMap[s.Address] = s + signerAddresses = append(signerAddresses, s.Address) + } + + signerAddressesFound, err := VerifyChallengeTxSigners(challengeTx, serverAccountID, network, signerAddresses...) + if err != nil { + return nil, err + } + + weight := Threshold(0) + for _, s := range signerAddressesFound { + weight += signerMap[s].Weight + } + + if weight < threshold { + return nil, errors.Errorf("signers with weight %d do not meet threshold %d", weight, threshold) + } + + signersFound = make([]Signer, 0, len(signerAddressesFound)) + for _, s := range signerAddressesFound { + signersFound = append(signersFound, signerMap[s]) + } + + return signersFound, nil +} + // VerifyChallengeTxSigners verifies that for a SEP 10 challenge transaction // all signatures on the transaction are accounted for. A transaction is // verified if it is signed by the server account, and all other signatures diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index 6c8dc02e49..78488e5c56 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -31,7 +31,7 @@ var horizonClient = func() horizonclient.ClientInterface { return client }() -func ExampleVerifyChallengeTxSigners_verifyAllClientSigners() { +func ExampleVerifyChallengeTxThreshold() { serverAccount, _ := keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") clientAccount, _ := keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") @@ -80,112 +80,19 @@ func ExampleVerifyChallengeTxSigners_verifyAllClientSigners() { return } - // Server gets list of account's signers + // Server gets list of account's signers and thresholds horizonClientAccount, err := horizonClient.AccountDetail(horizonclient.AccountRequest{AccountID: txClientAccountID}) if err != nil { fmt.Println("Error:", err) return } - clientSigners := []string{} - fmt.Println("Client Signers:") - for _, horizonSigner := range horizonClientAccount.Signers { - fmt.Println(horizonSigner.Key) - clientSigners = append(clientSigners, horizonSigner.Key) - } - - // Server finds which client signers are found on transaction - signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, clientSigners...) - if err != nil { - fmt.Println("Error:", err) - return - } - - // Server checks that all client signers were found on transaction - if len(signersFound) != len(clientSigners) { - fmt.Println("Error: not all client signers signed the challenge tx") - return - } + signers := txnbuild.SignersFromHorizon(horizonClientAccount.Signers) - fmt.Println("Client Signers Verified:") - for _, signerFound := range signersFound { - fmt.Println(signerFound) - } - } - - // Output: - // Client Signers: - // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 - // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP - // Client Signers Verified: - // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 - // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP -} - -func ExampleVerifyChallengeTxSigners_verifyAnyClientSigners() { - serverAccount, _ := keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") - clientAccount, _ := keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") - //clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") - clientSigner2, _ := keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") - - // Server builds challenge transaction - var challengeTx string - { - tx, err := txnbuild.BuildChallengeTx(serverAccount.Seed(), clientAccount.Address(), "test", network.TestNetworkPassphrase, time.Minute) - if err != nil { - fmt.Println("Error:", err) - return - } - challengeTx = tx - } - - // Client reads and signs challenge transaction - var signedChallengeTx string - { - tx, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) - if err != nil { - fmt.Println("Error:", err) - return - } - if txClientAccountID != clientAccount.Address() { - fmt.Println("Error: challenge tx is not for expected client account") - return - } - // Signs with only one of the account's signers - err = tx.Sign(clientSigner2) - if err != nil { - fmt.Println("Error:", err) - return - } - signedChallengeTx, err = tx.Base64() - if err != nil { - fmt.Println("Error:", err) - return - } - } - - // Server verifies signed challenge transaction - { - _, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) - if err != nil { - fmt.Println("Error:", err) - return - } - - // Server gets list of account's signers - horizonClientAccount, err := horizonClient.AccountDetail(horizonclient.AccountRequest{AccountID: txClientAccountID}) - if err != nil { - fmt.Println("Error:", err) - return - } - clientSigners := []string{} - fmt.Println("Client Signers:") - for _, horizonSigner := range horizonClientAccount.Signers { - fmt.Println(horizonSigner.Key) - clientSigners = append(clientSigners, horizonSigner.Key) - } + // Server chooses the threshold to require: low, med or high + threshold := txnbuild.Threshold(horizonClientAccount.Thresholds.MedThreshold) // Server finds which client signers are found on transaction - signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, clientSigners...) + signersFound, err := txnbuild.VerifyChallengeTxThreshold(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, threshold, signers) if err != nil { fmt.Println("Error:", err) return @@ -193,16 +100,14 @@ func ExampleVerifyChallengeTxSigners_verifyAnyClientSigners() { fmt.Println("Client Signers Verified:") for _, signerFound := range signersFound { - fmt.Println(signerFound) + fmt.Println(signerFound.Address, "weight:", signerFound.Weight) } } // Output: - // Client Signers: - // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 - // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP // Client Signers Verified: - // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP + // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 weight: 40 + // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP weight: 60 } func ExampleVerifyChallengeTxSigners_verifyClientMasterKeySigned() { From 828d6ec77e4505874f44a81659b7375f3c9b2015 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Fri, 20 Dec 2019 21:21:02 -0800 Subject: [PATCH 05/29] Make example comments clearer --- txnbuild/transaction_challenge_example_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index 78488e5c56..1e9eb96f21 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -80,12 +80,14 @@ func ExampleVerifyChallengeTxThreshold() { return } - // Server gets list of account's signers and thresholds + // Server gets account horizonClientAccount, err := horizonClient.AccountDetail(horizonclient.AccountRequest{AccountID: txClientAccountID}) if err != nil { fmt.Println("Error:", err) return } + + // Server gets list of signers from account signers := txnbuild.SignersFromHorizon(horizonClientAccount.Signers) // Server chooses the threshold to require: low, med or high From 3aacce3bfb2eb9ac75d537ce12e6e5e740d3e632 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Fri, 20 Dec 2019 21:33:09 -0800 Subject: [PATCH 06/29] Make the example more realistic by handling account not existing --- .../transaction_challenge_example_test.go | 129 ++++++------------ 1 file changed, 42 insertions(+), 87 deletions(-) diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index 1e9eb96f21..bfa7711f33 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -81,28 +81,49 @@ func ExampleVerifyChallengeTxThreshold() { } // Server gets account + clientAccountExists := false horizonClientAccount, err := horizonClient.AccountDetail(horizonclient.AccountRequest{AccountID: txClientAccountID}) - if err != nil { - fmt.Println("Error:", err) - return - } - - // Server gets list of signers from account - signers := txnbuild.SignersFromHorizon(horizonClientAccount.Signers) - - // Server chooses the threshold to require: low, med or high - threshold := txnbuild.Threshold(horizonClientAccount.Thresholds.MedThreshold) - - // Server finds which client signers are found on transaction - signersFound, err := txnbuild.VerifyChallengeTxThreshold(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, threshold, signers) - if err != nil { - fmt.Println("Error:", err) - return - } - - fmt.Println("Client Signers Verified:") - for _, signerFound := range signersFound { - fmt.Println(signerFound.Address, "weight:", signerFound.Weight) + if err == nil { + clientAccountExists = true + } else { + if hErr, ok := err.(*horizonclient.Error); ok && hErr.Problem.Type == "https://stellar.org/horizon-errors/not_found" { + fmt.Println("Account does not exist, use master key to verify") + } else { + fmt.Println("Error:", err) + return + } + } + + if clientAccountExists { + // Server gets list of signers from account + signers := txnbuild.SignersFromHorizon(horizonClientAccount.Signers) + + // Server chooses the threshold to require: low, med or high + threshold := txnbuild.Threshold(horizonClientAccount.Thresholds.MedThreshold) + + // Server finds which client signers are found on transaction + signersFound, err := txnbuild.VerifyChallengeTxThreshold(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, threshold, signers) + if err != nil { + fmt.Println("Error:", err) + return + } + + fmt.Println("Client Signers Verified:") + for _, signerFound := range signersFound { + fmt.Println(signerFound.Address, "weight:", signerFound.Weight) + } + } else { + // Server finds which client signers are found on transaction + signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, txClientAccountID) + if err != nil { + fmt.Println("Error:", err) + return + } + + fmt.Println("Client Master Key Verified:") + for _, signerFound := range signersFound { + fmt.Println(signerFound) + } } } @@ -111,69 +132,3 @@ func ExampleVerifyChallengeTxThreshold() { // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 weight: 40 // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP weight: 60 } - -func ExampleVerifyChallengeTxSigners_verifyClientMasterKeySigned() { - serverAccount, _ := keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") - clientAccount, _ := keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") - - // Server builds challenge transaction - var challengeTx string - { - tx, err := txnbuild.BuildChallengeTx(serverAccount.Seed(), clientAccount.Address(), "test", network.TestNetworkPassphrase, time.Minute) - if err != nil { - fmt.Println("Error:", err) - return - } - challengeTx = tx - } - - // Client reads and signs challenge transaction - var signedChallengeTx string - { - tx, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) - if err != nil { - fmt.Println("Error:", err) - return - } - if txClientAccountID != clientAccount.Address() { - fmt.Println("Error: challenge tx is not for expected client account") - return - } - // Signs with account master key - err = tx.Sign(clientAccount) - if err != nil { - fmt.Println("Error:", err) - return - } - signedChallengeTx, err = tx.Base64() - if err != nil { - fmt.Println("Error:", err) - return - } - } - - // Server verifies signed challenge transaction - { - _, txClientAccountID, err := txnbuild.ReadChallengeTx(challengeTx, serverAccount.Address(), network.TestNetworkPassphrase) - if err != nil { - fmt.Println("Error:", err) - return - } - - // Server finds which client signers are found on transaction - signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, txClientAccountID) - if err != nil { - fmt.Println("Error:", err) - return - } - - fmt.Println("Client Master Key Verified:") - for _, signerFound := range signersFound { - fmt.Println(signerFound) - } - } - - // Output: - // Client Master Key Verified: - // GBOHBZB3Q3RMKXO3OSLJRDJNUSAPOXDVBDOMA52VHIGCIQEVZSXQ44CW -} From 2432aed42a9201258bb84a9275a705acbb86cf93 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Fri, 20 Dec 2019 21:34:18 -0800 Subject: [PATCH 07/29] Reduce dupe code --- txnbuild/transaction_challenge_example_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index bfa7711f33..007f96f739 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -11,6 +11,7 @@ import ( "github.com/stellar/go/txnbuild" ) +var serverAccount, _ = keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") var clientAccount, _ = keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") var clientSigner1, _ = keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") var clientSigner2, _ = keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") @@ -32,11 +33,6 @@ var horizonClient = func() horizonclient.ClientInterface { }() func ExampleVerifyChallengeTxThreshold() { - serverAccount, _ := keypair.ParseFull("SCDXPYDGKV5HOAGVZN3FQSS5FKUPP5BAVBWH4FXKTAWAC24AE4757JSI") - clientAccount, _ := keypair.ParseFull("SANVNCABRBVISCV7KH4SZVBKPJWWTT4424OVWUHUHPH2MVSF6RC7HPGN") - clientSigner1, _ := keypair.ParseFull("SBPQUZ6G4FZNWFHKUWC5BEYWF6R52E3SEP7R3GWYSM2XTKGF5LNTWW4R") - clientSigner2, _ := keypair.ParseFull("SBMSVD4KKELKGZXHBUQTIROWUAPQASDX7KEJITARP4VMZ6KLUHOGPTYW") - // Server builds challenge transaction var challengeTx string { From b051ebce7512270b5db7db1a59b37cf870337732 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Fri, 20 Dec 2019 21:36:54 -0800 Subject: [PATCH 08/29] Improve comments --- txnbuild/transaction_challenge_example_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index 007f96f739..de30ed6fe5 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -97,25 +97,23 @@ func ExampleVerifyChallengeTxThreshold() { // Server chooses the threshold to require: low, med or high threshold := txnbuild.Threshold(horizonClientAccount.Thresholds.MedThreshold) - // Server finds which client signers are found on transaction + // Server verifies threshold is met signersFound, err := txnbuild.VerifyChallengeTxThreshold(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, threshold, signers) if err != nil { fmt.Println("Error:", err) return } - fmt.Println("Client Signers Verified:") for _, signerFound := range signersFound { fmt.Println(signerFound.Address, "weight:", signerFound.Weight) } } else { - // Server finds which client signers are found on transaction + // Server verifies that master key has signed challenge transaction signersFound, err := txnbuild.VerifyChallengeTxSigners(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, txClientAccountID) if err != nil { fmt.Println("Error:", err) return } - fmt.Println("Client Master Key Verified:") for _, signerFound := range signersFound { fmt.Println(signerFound) From b419bad0880706ddfeb89429c80bbdf58bcc0837 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Wed, 15 Jan 2020 15:20:48 -0800 Subject: [PATCH 09/29] add test --- txnbuild/set_options_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/txnbuild/set_options_test.go b/txnbuild/set_options_test.go index 6f0ef3913c..7cb0967e5d 100644 --- a/txnbuild/set_options_test.go +++ b/txnbuild/set_options_test.go @@ -138,6 +138,13 @@ func TestEmptyHomeDomainOK(t *testing.T) { } +func TestSignerFromHorizon(t *testing.T) { + horizonSigner := horizon.Signer{Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 10} + wantSigner := Signer{Address: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 10} + signer := SignerFromHorizon(horizonSigner) + assert.Equal(t, wantSigner, signer) +} + func TestSignersFromHorizon(t *testing.T) { horizonSigners := []horizon.Signer{ {Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 0}, From 56097e4f6d50795d676c717b0bec3ae185eb4c60 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Wed, 15 Jan 2020 15:30:29 -0800 Subject: [PATCH 10/29] update comments --- txnbuild/transaction.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 14448ba8f8..b1efb6c6af 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -414,14 +414,16 @@ func (tx *Transaction) SignWithKeyString(keys ...string) error { return tx.Sign(signers...) } -// ReadChallengeTx verifies that a SEP 10 challenge transaction for use in web -// authentication is signed by the server and returns the transaction and -// client account ID. +// ReadChallengeTx reads a SEP 10 challenge transaction and returns the decoded +// transaction and client account ID contained within. +// +// It also verifies that transaction is signed by the server. // // It does not verify that the transaction has been signed by the client or -// that any other signatures other than the servers on the transaction are -// valid. Use VerifyChallengeTxSigners to verify a set of client signers have -// signed the transaction. +// that any signatures other than the servers on the transaction are valid. Use +// one of the following functions to completely verify the transaction: +// - VerifyChallengeTxThreshold +// - VerifyChallengeTxSigners func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transaction, clientAccountID string, err error) { tx, err = TransactionFromXDR(challengeTx) if err != nil { From fe76f9e7ea8bb8c76736fde08099a57574ec6086 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Wed, 15 Jan 2020 17:21:59 -0800 Subject: [PATCH 11/29] Add clarifying comment to deprecated func --- txnbuild/transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index b1efb6c6af..5f4cd3a94e 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -585,7 +585,7 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign // VerifyChallengeTx is a factory method that verifies a SEP 10 challenge transaction, // for use in web authentication. It can be used by a server to verify that the challenge -// has been signed by the client. +// has been signed by the client account's master key. // More details on SEP 10: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0010.md // // Deprecated: Use VerifyChallengeTxSigners. From 649275941b08710318c92098f4b1e2f8a7524e5d Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Wed, 15 Jan 2020 17:22:30 -0800 Subject: [PATCH 12/29] Add tests for VerifyChallengeTxThreshold --- txnbuild/transaction_test.go | 176 +++++++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+) diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index 5b65e1d01e..64de9fe501 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/stellar/go/keypair" "github.com/stellar/go/network" "github.com/stellar/go/strkey" "github.com/stellar/go/xdr" @@ -1566,6 +1567,181 @@ func TestReadChallengeTx_invalidDataValueWrongByteLength(t *testing.T) { assert.EqualError(t, err, "random nonce before encoding as base64 should be 48 bytes long") } +func TestVerifyChallengeTxThreshold_validServerAndClientKeyMeetingThreshold(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(1) + signers := []Signer{ + {Address: clientKP.Address(), Weight: 1}, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + assert.Equal(t, signers, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThreshold(t *testing.T) { + serverKP := newKeypair0() + clientKP1 := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP1.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(3) + signers := []Signer{ + {Address: clientKP1.Address(), Weight: 1}, + {Address: clientKP2.Address(), Weight: 2}, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP1, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + assert.Equal(t, signers, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresholdSomeUnused(t *testing.T) { + serverKP := newKeypair0() + clientKP1 := newKeypair1() + clientKP2 := newKeypair2() + clientKP3 := keypair.MustRandom() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP1.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(3) + signers := []Signer{ + {Address: clientKP1.Address(), Weight: 1}, + {Address: clientKP2.Address(), Weight: 2}, + {Address: clientKP3.Address(), Weight: 2}, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP1, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + wantSignersFound := []Signer{ + {Address: clientKP1.Address(), Weight: 1}, + {Address: clientKP2.Address(), Weight: 2}, + } + assert.Equal(t, wantSignersFound, signersFound) + assert.NoError(t, err) +} + +func TestVerifyChallengeTxThreshold_invalidServerAndMultipleClientKeyNotMeetingThreshold(t *testing.T) { + serverKP := newKeypair0() + clientKP1 := newKeypair1() + clientKP2 := newKeypair2() + clientKP3 := keypair.MustRandom() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP1.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(10) + signers := []Signer{ + {Address: clientKP1.Address(), Weight: 1}, + {Address: clientKP2.Address(), Weight: 2}, + {Address: clientKP3.Address(), Weight: 2}, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP1, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + assert.EqualError(t, err, "signers with weight 3 do not meet threshold 10") +} + +func TestVerifyChallengeTxThreshold_invalidClientKeyUnrecognized(t *testing.T) { + serverKP := newKeypair0() + clientKP1 := newKeypair1() + clientKP2 := newKeypair2() + clientKP3 := keypair.MustRandom() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP1.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(10) + signers := []Signer{ + {Address: clientKP1.Address(), Weight: 1}, + {Address: clientKP2.Address(), Weight: 2}, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP1, clientKP2, clientKP3) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + assert.EqualError(t, err, "transaction has unrecognized signatures") +} + func TestVerifyChallengeTxSigners_validServerAndClientMasterKey(t *testing.T) { serverKP := newKeypair0() clientKP := newKeypair1() From 846e3965878c036296291cce7e336b57f9dfda2d Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Wed, 15 Jan 2020 17:26:13 -0800 Subject: [PATCH 13/29] Add check that some signers are provided --- txnbuild/transaction.go | 4 +++ txnbuild/transaction_test.go | 58 ++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 5f4cd3a94e..1c9c11cf72 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -549,6 +549,10 @@ func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, th // - One or more signatures in the transaction are not identifiable as the // server account or one of the signers provided in the arguments. func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, signers ...string) ([]string, error) { + if len(signers) == 0 { + return nil, errors.New("no signers provided") + } + tx, _, err := ReadChallengeTx(challengeTx, serverAccountID, network) if err != nil { return nil, err diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index 64de9fe501..cab55a8017 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -1742,6 +1742,37 @@ func TestVerifyChallengeTxThreshold_invalidClientKeyUnrecognized(t *testing.T) { assert.EqualError(t, err, "transaction has unrecognized signatures") } +func TestVerifyChallengeTxThreshold_invalidNoSigners(t *testing.T) { + serverKP := newKeypair0() + clientKP1 := newKeypair1() + clientKP2 := newKeypair2() + clientKP3 := keypair.MustRandom() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP1.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(10) + signers := []Signer{} + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP1, clientKP2, clientKP3) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + assert.EqualError(t, err, "no signers provided") +} + func TestVerifyChallengeTxSigners_validServerAndClientMasterKey(t *testing.T) { serverKP := newKeypair0() clientKP := newKeypair1() @@ -2030,6 +2061,33 @@ func TestVerifyChallengeTxSigners_invalidServerAndClientSignersFailsDuplicateSig assert.EqualError(t, err, "transaction has unrecognized signatures") } +func TestVerifyChallengeTxSigners_invalidNoSigners(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + _, err = VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase) + assert.EqualError(t, err, "no signers provided") +} + func TestVerifyTxSignatureUnsignedTx(t *testing.T) { kp0 := newKeypair0() kp1 := newKeypair1() From f20703f3cd5b6743952a0b23c14ef300311e2489 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Thu, 16 Jan 2020 14:39:42 -0800 Subject: [PATCH 14/29] Remove dependency on clients/horizon --- txnbuild/set_options.go | 6 +++--- txnbuild/set_options_test.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/txnbuild/set_options.go b/txnbuild/set_options.go index ce2a875721..e094bda010 100644 --- a/txnbuild/set_options.go +++ b/txnbuild/set_options.go @@ -1,7 +1,7 @@ package txnbuild import ( - "github.com/stellar/go/clients/horizon" + hProtocol "github.com/stellar/go/protocols/horizon" "github.com/stellar/go/support/errors" "github.com/stellar/go/xdr" ) @@ -35,7 +35,7 @@ type Signer struct { // SignerFromHorizon converts a signer retrieved from Horizon to a txnbuild // Signer. -func SignerFromHorizon(horizonSigner horizon.Signer) Signer { +func SignerFromHorizon(horizonSigner hProtocol.Signer) Signer { return Signer{ Address: horizonSigner.Key, Weight: Threshold(horizonSigner.Weight), @@ -44,7 +44,7 @@ func SignerFromHorizon(horizonSigner horizon.Signer) Signer { // SignersFromHorizon converts a list of signers retrieved from Horizon to // txnbuild Signers. -func SignersFromHorizon(horizonSigners []horizon.Signer) []Signer { +func SignersFromHorizon(horizonSigners []hProtocol.Signer) []Signer { signers := make([]Signer, 0, len(horizonSigners)) for _, hs := range horizonSigners { signers = append(signers, SignerFromHorizon(hs)) diff --git a/txnbuild/set_options_test.go b/txnbuild/set_options_test.go index 7cb0967e5d..1fae0c55fc 100644 --- a/txnbuild/set_options_test.go +++ b/txnbuild/set_options_test.go @@ -3,7 +3,7 @@ package txnbuild import ( "testing" - "github.com/stellar/go/clients/horizon" + hProtocol "github.com/stellar/go/protocols/horizon" "github.com/stellar/go/xdr" "github.com/stretchr/testify/assert" ) @@ -139,14 +139,14 @@ func TestEmptyHomeDomainOK(t *testing.T) { } func TestSignerFromHorizon(t *testing.T) { - horizonSigner := horizon.Signer{Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 10} + horizonSigner := hProtocol.Signer{Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 10} wantSigner := Signer{Address: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 10} signer := SignerFromHorizon(horizonSigner) assert.Equal(t, wantSigner, signer) } func TestSignersFromHorizon(t *testing.T) { - horizonSigners := []horizon.Signer{ + horizonSigners := []hProtocol.Signer{ {Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 0}, {Key: "GAT4FUGQNKOIDLOIXCJJYFSAFJHQY5MZEZLRBXXFKDCXGUHJQ63XZFTD", Weight: 10}, {Key: "GCB35H32SU5ME4OALQUPOM4AADJICL2H2NLWAGLOMMTSYOVTXWYP6Q4T", Weight: 100}, From b7bec9f3d0b4b0d75438bb47465d8b4f1a29970e Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 21 Jan 2020 15:33:59 -0800 Subject: [PATCH 15/29] Remove addition of protocols/horizon dependency --- txnbuild/set_options.go | 20 --------------- txnbuild/set_options_test.go | 25 ------------------- .../transaction_challenge_example_test.go | 8 +++++- 3 files changed, 7 insertions(+), 46 deletions(-) diff --git a/txnbuild/set_options.go b/txnbuild/set_options.go index e094bda010..1ec99970fe 100644 --- a/txnbuild/set_options.go +++ b/txnbuild/set_options.go @@ -1,7 +1,6 @@ package txnbuild import ( - hProtocol "github.com/stellar/go/protocols/horizon" "github.com/stellar/go/support/errors" "github.com/stellar/go/xdr" ) @@ -33,25 +32,6 @@ type Signer struct { Weight Threshold } -// SignerFromHorizon converts a signer retrieved from Horizon to a txnbuild -// Signer. -func SignerFromHorizon(horizonSigner hProtocol.Signer) Signer { - return Signer{ - Address: horizonSigner.Key, - Weight: Threshold(horizonSigner.Weight), - } -} - -// SignersFromHorizon converts a list of signers retrieved from Horizon to -// txnbuild Signers. -func SignersFromHorizon(horizonSigners []hProtocol.Signer) []Signer { - signers := make([]Signer, 0, len(horizonSigners)) - for _, hs := range horizonSigners { - signers = append(signers, SignerFromHorizon(hs)) - } - return signers -} - // NewHomeDomain is syntactic sugar that makes instantiating SetOptions more convenient. func NewHomeDomain(hd string) *string { return &hd diff --git a/txnbuild/set_options_test.go b/txnbuild/set_options_test.go index 1fae0c55fc..0398f37257 100644 --- a/txnbuild/set_options_test.go +++ b/txnbuild/set_options_test.go @@ -3,7 +3,6 @@ package txnbuild import ( "testing" - hProtocol "github.com/stellar/go/protocols/horizon" "github.com/stellar/go/xdr" "github.com/stretchr/testify/assert" ) @@ -137,27 +136,3 @@ func TestEmptyHomeDomainOK(t *testing.T) { assert.Equal(t, string(*options.xdrOp.HomeDomain), "", "empty string home domain is set") } - -func TestSignerFromHorizon(t *testing.T) { - horizonSigner := hProtocol.Signer{Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 10} - wantSigner := Signer{Address: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 10} - signer := SignerFromHorizon(horizonSigner) - assert.Equal(t, wantSigner, signer) -} - -func TestSignersFromHorizon(t *testing.T) { - horizonSigners := []hProtocol.Signer{ - {Key: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 0}, - {Key: "GAT4FUGQNKOIDLOIXCJJYFSAFJHQY5MZEZLRBXXFKDCXGUHJQ63XZFTD", Weight: 10}, - {Key: "GCB35H32SU5ME4OALQUPOM4AADJICL2H2NLWAGLOMMTSYOVTXWYP6Q4T", Weight: 100}, - {Key: "GAVJHRCK5CEFE3MHL4JALMNX35Z5NLUIODSJIC44VRRLQQZGTDJWANV4", Weight: 255}, - } - wantSigners := []Signer{ - {Address: "GAABGBW5DINUS456OTHH6IUPTQSQZVVFCZGAO467OLIPFUWTMV6XR5XS", Weight: 0}, - {Address: "GAT4FUGQNKOIDLOIXCJJYFSAFJHQY5MZEZLRBXXFKDCXGUHJQ63XZFTD", Weight: 10}, - {Address: "GCB35H32SU5ME4OALQUPOM4AADJICL2H2NLWAGLOMMTSYOVTXWYP6Q4T", Weight: 100}, - {Address: "GAVJHRCK5CEFE3MHL4JALMNX35Z5NLUIODSJIC44VRRLQQZGTDJWANV4", Weight: 255}, - } - signers := SignersFromHorizon(horizonSigners) - assert.Equal(t, wantSigners, signers) -} diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index de30ed6fe5..e5924c7768 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -92,7 +92,13 @@ func ExampleVerifyChallengeTxThreshold() { if clientAccountExists { // Server gets list of signers from account - signers := txnbuild.SignersFromHorizon(horizonClientAccount.Signers) + signers := make([]txnbuild.Signer, 0, len(horizonClientAccount.Signers)) + for _, hs := range horizonClientAccount.Signers { + signers = append(signers, txnbuild.Signer{ + Address: hs.Key, + Weight: txnbuild.Threshold(hs.Weight), + }) + } // Server chooses the threshold to require: low, med or high threshold := txnbuild.Threshold(horizonClientAccount.Thresholds.MedThreshold) From 731c0d4a042be3b9380a92a1132a9136dac1f02c Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 21 Jan 2020 16:06:02 -0800 Subject: [PATCH 16/29] Change how the signer data is copied from horizon to txnbuild --- protocols/horizon/main.go | 9 +++ txnbuild/transaction.go | 24 +++---- .../transaction_challenge_example_test.go | 14 ++-- txnbuild/transaction_test.go | 67 ++++++++++--------- 4 files changed, 60 insertions(+), 54 deletions(-) diff --git a/protocols/horizon/main.go b/protocols/horizon/main.go index b1c034051d..198f19930c 100644 --- a/protocols/horizon/main.go +++ b/protocols/horizon/main.go @@ -128,6 +128,15 @@ func (a *Account) GetData(key string) ([]byte, error) { return base64.StdEncoding.DecodeString(a.Data[key]) } +// SignerSummary returns a map of signer's keys to weights. +func (a *Account) SignerSummary() map[string]int32 { + m := map[string]int32{} + for _, s := range a.Signers { + m[s.Key] = s.Weight + } + return m +} + // AccountSigner is the account signer information. type AccountSigner struct { Links struct { diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 1c9c11cf72..23142ac198 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -492,6 +492,9 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti return tx, clientAccountID, nil } +// SignerSummary is a map of signers to their weights. +type SignerSummary map[string]int32 + // VerifyChallengeTxThreshold verifies that for a SEP 10 challenge transaction // all signatures on the transaction are accounted for and that the signatures // meet a threshold on an account. A transaction is verified if it is signed by @@ -505,33 +508,26 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti // - One or more signatures in the transaction are not identifiable as the // server account or one of the signers provided in the arguments. // - The signatures are all valid but do not meet the threshold. -func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, threshold Threshold, signers []Signer) (signersFound []Signer, err error) { - signerMap := map[string]Signer{} - signerAddresses := make([]string, 0, len(signers)) - for _, s := range signers { - signerMap[s.Address] = s - signerAddresses = append(signerAddresses, s.Address) +func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, threshold Threshold, signerSummary SignerSummary) (signersFound []string, err error) { + signerAddresses := make([]string, 0, len(signerSummary)) + for s := range signerSummary { + signerAddresses = append(signerAddresses, s) } - signerAddressesFound, err := VerifyChallengeTxSigners(challengeTx, serverAccountID, network, signerAddresses...) + signersFound, err = VerifyChallengeTxSigners(challengeTx, serverAccountID, network, signerAddresses...) if err != nil { return nil, err } weight := Threshold(0) - for _, s := range signerAddressesFound { - weight += signerMap[s].Weight + for _, s := range signersFound { + weight += Threshold(signerSummary[s]) } if weight < threshold { return nil, errors.Errorf("signers with weight %d do not meet threshold %d", weight, threshold) } - signersFound = make([]Signer, 0, len(signerAddressesFound)) - for _, s := range signerAddressesFound { - signersFound = append(signersFound, signerMap[s]) - } - return signersFound, nil } diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index e5924c7768..db77ab1f72 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -92,26 +92,20 @@ func ExampleVerifyChallengeTxThreshold() { if clientAccountExists { // Server gets list of signers from account - signers := make([]txnbuild.Signer, 0, len(horizonClientAccount.Signers)) - for _, hs := range horizonClientAccount.Signers { - signers = append(signers, txnbuild.Signer{ - Address: hs.Key, - Weight: txnbuild.Threshold(hs.Weight), - }) - } + signerSummary := horizonClientAccount.SignerSummary() // Server chooses the threshold to require: low, med or high threshold := txnbuild.Threshold(horizonClientAccount.Thresholds.MedThreshold) // Server verifies threshold is met - signersFound, err := txnbuild.VerifyChallengeTxThreshold(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, threshold, signers) + signers, err := txnbuild.VerifyChallengeTxThreshold(signedChallengeTx, serverAccount.Address(), network.TestNetworkPassphrase, threshold, signerSummary) if err != nil { fmt.Println("Error:", err) return } fmt.Println("Client Signers Verified:") - for _, signerFound := range signersFound { - fmt.Println(signerFound.Address, "weight:", signerFound.Weight) + for _, signer := range signers { + fmt.Println(signer, "weight:", signerSummary[signer]) } } else { // Server verifies that master key has signed challenge transaction diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index cab55a8017..a790a837ec 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -1584,8 +1584,11 @@ func TestVerifyChallengeTxThreshold_validServerAndClientKeyMeetingThreshold(t *t Network: network.TestNetworkPassphrase, } threshold := Threshold(1) - signers := []Signer{ - {Address: clientKP.Address(), Weight: 1}, + signerSummary := SignerSummary{ + clientKP.Address(): 1, + } + wantSigners := []string{ + clientKP.Address(), } err := tx.Build() @@ -1594,8 +1597,8 @@ func TestVerifyChallengeTxThreshold_validServerAndClientKeyMeetingThreshold(t *t assert.NoError(t, err) tx64, err := tx.Base64() require.NoError(t, err) - signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) - assert.Equal(t, signers, signersFound) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) + assert.Equal(t, wantSigners, signersFound) assert.NoError(t, err) } @@ -1617,9 +1620,13 @@ func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresh Network: network.TestNetworkPassphrase, } threshold := Threshold(3) - signers := []Signer{ - {Address: clientKP1.Address(), Weight: 1}, - {Address: clientKP2.Address(), Weight: 2}, + signerSummary := map[string]int32{ + clientKP1.Address(): 1, + clientKP2.Address(): 2, + } + wantSigners := []string{ + clientKP1.Address(), + clientKP2.Address(), } err := tx.Build() @@ -1628,8 +1635,8 @@ func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresh assert.NoError(t, err) tx64, err := tx.Base64() require.NoError(t, err) - signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) - assert.Equal(t, signers, signersFound) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) + assert.Equal(t, wantSigners, signersFound) assert.NoError(t, err) } @@ -1652,10 +1659,14 @@ func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresh Network: network.TestNetworkPassphrase, } threshold := Threshold(3) - signers := []Signer{ - {Address: clientKP1.Address(), Weight: 1}, - {Address: clientKP2.Address(), Weight: 2}, - {Address: clientKP3.Address(), Weight: 2}, + signerSummary := SignerSummary{ + clientKP1.Address(): 1, + clientKP2.Address(): 2, + clientKP3.Address(): 2, + } + wantSigners := []string{ + clientKP1.Address(), + clientKP2.Address(), } err := tx.Build() @@ -1664,12 +1675,8 @@ func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresh assert.NoError(t, err) tx64, err := tx.Base64() require.NoError(t, err) - signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) - wantSignersFound := []Signer{ - {Address: clientKP1.Address(), Weight: 1}, - {Address: clientKP2.Address(), Weight: 2}, - } - assert.Equal(t, wantSignersFound, signersFound) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) + assert.Equal(t, wantSigners, signersFound) assert.NoError(t, err) } @@ -1692,10 +1699,10 @@ func TestVerifyChallengeTxThreshold_invalidServerAndMultipleClientKeyNotMeetingT Network: network.TestNetworkPassphrase, } threshold := Threshold(10) - signers := []Signer{ - {Address: clientKP1.Address(), Weight: 1}, - {Address: clientKP2.Address(), Weight: 2}, - {Address: clientKP3.Address(), Weight: 2}, + signerSummary := SignerSummary{ + clientKP1.Address(): 1, + clientKP2.Address(): 2, + clientKP3.Address(): 2, } err := tx.Build() @@ -1704,7 +1711,7 @@ func TestVerifyChallengeTxThreshold_invalidServerAndMultipleClientKeyNotMeetingT assert.NoError(t, err) tx64, err := tx.Base64() require.NoError(t, err) - _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) assert.EqualError(t, err, "signers with weight 3 do not meet threshold 10") } @@ -1727,9 +1734,9 @@ func TestVerifyChallengeTxThreshold_invalidClientKeyUnrecognized(t *testing.T) { Network: network.TestNetworkPassphrase, } threshold := Threshold(10) - signers := []Signer{ - {Address: clientKP1.Address(), Weight: 1}, - {Address: clientKP2.Address(), Weight: 2}, + signerSummary := map[string]int32{ + clientKP1.Address(): 1, + clientKP2.Address(): 2, } err := tx.Build() @@ -1738,7 +1745,7 @@ func TestVerifyChallengeTxThreshold_invalidClientKeyUnrecognized(t *testing.T) { assert.NoError(t, err) tx64, err := tx.Base64() require.NoError(t, err) - _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) assert.EqualError(t, err, "transaction has unrecognized signatures") } @@ -1761,7 +1768,7 @@ func TestVerifyChallengeTxThreshold_invalidNoSigners(t *testing.T) { Network: network.TestNetworkPassphrase, } threshold := Threshold(10) - signers := []Signer{} + signerSummary := SignerSummary{} err := tx.Build() require.NoError(t, err) @@ -1769,7 +1776,7 @@ func TestVerifyChallengeTxThreshold_invalidNoSigners(t *testing.T) { assert.NoError(t, err) tx64, err := tx.Base64() require.NoError(t, err) - _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signers) + _, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) assert.EqualError(t, err, "no signers provided") } From d53c0b7cd9a35c54e80877f4c446693514e37250 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 21 Jan 2020 16:49:19 -0800 Subject: [PATCH 17/29] Fix the order of the signers that are outputted --- txnbuild/transaction_challenge_example_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/txnbuild/transaction_challenge_example_test.go b/txnbuild/transaction_challenge_example_test.go index db77ab1f72..8358cfc5ec 100644 --- a/txnbuild/transaction_challenge_example_test.go +++ b/txnbuild/transaction_challenge_example_test.go @@ -2,6 +2,7 @@ package txnbuild_test import ( "fmt" + "sort" "time" "github.com/stellar/go/clients/horizon" @@ -104,6 +105,7 @@ func ExampleVerifyChallengeTxThreshold() { return } fmt.Println("Client Signers Verified:") + sort.Strings(signers) for _, signer := range signers { fmt.Println(signer, "weight:", signerSummary[signer]) } @@ -123,6 +125,6 @@ func ExampleVerifyChallengeTxThreshold() { // Output: // Client Signers Verified: - // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 weight: 40 // GAS4V4O2B7DW5T7IQRPEEVCRXMDZESKISR7DVIGKZQYYV3OSQ5SH5LVP weight: 60 + // GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3 weight: 40 } From 0a9dbc88c90afb10048ecd574ae2a8ee24179cd8 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 21 Jan 2020 17:21:17 -0800 Subject: [PATCH 18/29] Moved SignerSummary to its own file --- txnbuild/signer_summary.go | 4 ++++ txnbuild/transaction.go | 3 --- 2 files changed, 4 insertions(+), 3 deletions(-) create mode 100644 txnbuild/signer_summary.go diff --git a/txnbuild/signer_summary.go b/txnbuild/signer_summary.go new file mode 100644 index 0000000000..269e65a8f7 --- /dev/null +++ b/txnbuild/signer_summary.go @@ -0,0 +1,4 @@ +package txnbuild + +// SignerSummary is a map of signers to their weights. +type SignerSummary map[string]int32 diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 23142ac198..d9aff3f669 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -492,9 +492,6 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti return tx, clientAccountID, nil } -// SignerSummary is a map of signers to their weights. -type SignerSummary map[string]int32 - // VerifyChallengeTxThreshold verifies that for a SEP 10 challenge transaction // all signatures on the transaction are accounted for and that the signatures // meet a threshold on an account. A transaction is verified if it is signed by From 99529e70c81d2b0806928698f45f03110f874040 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 21 Jan 2020 17:32:12 -0800 Subject: [PATCH 19/29] Fix bug in weight >= threshold check --- txnbuild/transaction.go | 6 +++--- txnbuild/transaction_test.go | 38 ++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index d9aff3f669..01c7a71384 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -516,12 +516,12 @@ func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, th return nil, err } - weight := Threshold(0) + weight := int32(0) for _, s := range signersFound { - weight += Threshold(signerSummary[s]) + weight += signerSummary[s] } - if weight < threshold { + if weight < int32(threshold) { return nil, errors.Errorf("signers with weight %d do not meet threshold %d", weight, threshold) } diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index a790a837ec..457be94488 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -1780,6 +1780,44 @@ func TestVerifyChallengeTxThreshold_invalidNoSigners(t *testing.T) { assert.EqualError(t, err, "no signers provided") } +func TestVerifyChallengeTxThreshold_weightsAddToMoreThan8Bits(t *testing.T) { + serverKP := newKeypair0() + clientKP1 := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP1.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(1) + signerSummary := SignerSummary{ + clientKP1.Address(): 255, + clientKP2.Address(): 1, + } + wantSigners := []string{ + clientKP1.Address(), + clientKP2.Address(), + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP1, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) + assert.Equal(t, wantSigners, signersFound) + assert.NoError(t, err) +} + func TestVerifyChallengeTxSigners_validServerAndClientMasterKey(t *testing.T) { serverKP := newKeypair0() clientKP := newKeypair1() From e2519cf4001c9d7a6667c03db0caff9fc50af160 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 21 Jan 2020 16:43:35 -0800 Subject: [PATCH 20/29] Fix tests failing due to slice ordering (cherry picked from commit edecbbd7bb91430fdab6c1fc29e334c96005693d) --- txnbuild/transaction_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index 457be94488..ab8cd736c4 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -1598,7 +1598,7 @@ func TestVerifyChallengeTxThreshold_validServerAndClientKeyMeetingThreshold(t *t tx64, err := tx.Base64() require.NoError(t, err) signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) - assert.Equal(t, wantSigners, signersFound) + assert.ElementsMatch(t, wantSigners, signersFound) assert.NoError(t, err) } @@ -1636,7 +1636,7 @@ func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresh tx64, err := tx.Base64() require.NoError(t, err) signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) - assert.Equal(t, wantSigners, signersFound) + assert.ElementsMatch(t, wantSigners, signersFound) assert.NoError(t, err) } @@ -1676,7 +1676,7 @@ func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresh tx64, err := tx.Base64() require.NoError(t, err) signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) - assert.Equal(t, wantSigners, signersFound) + assert.ElementsMatch(t, wantSigners, signersFound) assert.NoError(t, err) } From d3420d834e4d9799d3b091f06bba11d8af954beb Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Wed, 22 Jan 2020 12:44:17 -0800 Subject: [PATCH 21/29] Fix test expectation --- txnbuild/transaction_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index ab8cd736c4..cac7f58170 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -1814,7 +1814,7 @@ func TestVerifyChallengeTxThreshold_weightsAddToMoreThan8Bits(t *testing.T) { tx64, err := tx.Base64() require.NoError(t, err) signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) - assert.Equal(t, wantSigners, signersFound) + assert.ElementsMatch(t, wantSigners, signersFound) assert.NoError(t, err) } From 330f6922e9c92a1af68a378c980d22e1b65f4692 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Mon, 27 Jan 2020 11:26:37 -0800 Subject: [PATCH 22/29] Improve deprecated comment --- txnbuild/transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 01c7a71384..cd57d8fe34 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -585,7 +585,7 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign // has been signed by the client account's master key. // More details on SEP 10: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0010.md // -// Deprecated: Use VerifyChallengeTxSigners. +// Deprecated: Use VerifyChallengeTxThreshold or VerifyChallengeTxSigners. func VerifyChallengeTx(challengeTx, serverAccountID, network string) (bool, error) { tx, clientAccountID, err := ReadChallengeTx(challengeTx, serverAccountID, network) if err != nil { From 30a4ef91ee61e40f9245776289ea623635871d8e Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Mon, 27 Jan 2020 20:15:14 -0800 Subject: [PATCH 23/29] Restructured some of the challenge transaction verification process to ensure that the code never allows a signature to be consumed more than once, and as a side effect of that some of the guards and checks had to move, which I felt made this require more comments also. The comments might have been required anyway really. --- txnbuild/transaction.go | 60 ++++++++++++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index cd57d8fe34..0002bc3e66 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -546,34 +546,63 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign return nil, errors.New("no signers provided") } + // Read the transaction which validates its structure. tx, _, err := ReadChallengeTx(challengeTx, serverAccountID, network) if err != nil { return nil, err } + // Ensure the server account ID is an address and not a seed. serverKP, err := keypair.ParseAddress(serverAccountID) if err != nil { return nil, err } - // Remove the server signer from the signers list if it is present. It's - // important when verifying signers of a challenge transaction that we only - // verify and return client signers. If an account has the server as a - // signer the server should not play a part in the authentication of the - // client. - clientSigners := make([]string, 0, len(signers)) + // Deduplicate the client signers and ensure the server is not included + // anywhere we check or output the list of signers. + clientSignersSet := map[string]struct{}{} for _, signer := range signers { + // Ignore the server signer if it is in the signers list. It's + // important when verifying signers of a challenge transaction that we + // only verify and return client signers. If an account has the server + // as a signer the server should not play a part in the authentication + // of the client. if signer == serverKP.Address() { continue } + clientSignersSet[signer] = struct{}{} + } + clientSigners := make([]string, 0, len(clientSignersSet)) + for signer := range clientSignersSet { clientSigners = append(clientSigners, signer) } - signersFound, err := verifyTxSignatures(tx, clientSigners...) + // Verify that all the transaction's signers (server and client) in one + // hit. We do this in one hit here even though the server signature was + // checked in the ReadChallengeTx to ensure that every signature and signer + // are consumed only once on the transaction. + allSigners := append([]string{serverKP.Address()}, clientSigners...) + allSignersFound, err := verifyTxSignatures(tx, allSigners...) if err != nil { return nil, err } - if len(signersFound) != len(tx.xdrEnvelope.Signatures)-1 { + + // Remove the server from the list of signers found. + signersFound := make([]string, 0, len(allSignersFound)-1) + for _, signer := range allSignersFound { + if signer == serverKP.Address() { + continue + } + signersFound = append(signersFound, signer) + } + + // Confirm we found at least one client signer. + if len(signersFound) == 0 { + return nil, errors.Errorf("transaction not signed by %s", strings.Join(clientSigners, ", ")) + } + + // Confirm all signatures were consumed by a signer. + if len(allSignersFound) != len(tx.xdrEnvelope.Signatures) { return signersFound, errors.Errorf("transaction has unrecognized signatures") } @@ -602,7 +631,10 @@ func VerifyChallengeTx(challengeTx, serverAccountID, network string) (bool, erro // verifyTxSignature checks if a transaction has been signed by the provided Stellar account. func verifyTxSignature(tx Transaction, signer string) error { - _, err := verifyTxSignatures(tx, signer) + signersFound, err := verifyTxSignatures(tx, signer) + if len(signersFound) == 0 { + return errors.Errorf("transaction not signed by %s", signer) + } return err } @@ -620,6 +652,7 @@ func verifyTxSignatures(tx Transaction, signers ...string) ([]string, error) { } // find and verify signatures + signatureUsed := map[int]bool{} signersFound := map[string]struct{}{} for _, signer := range signers { kp, err := keypair.ParseAddress(signer) @@ -627,20 +660,21 @@ func verifyTxSignatures(tx Transaction, signers ...string) ([]string, error) { return nil, err } - for _, decSig := range tx.xdrEnvelope.Signatures { + for i, decSig := range tx.xdrEnvelope.Signatures { + if signatureUsed[i] { + continue + } if decSig.Hint != kp.Hint() { continue } err := kp.Verify(txHash[:], decSig.Signature) if err == nil { + signatureUsed[i] = true signersFound[signer] = struct{}{} break } } } - if len(signersFound) == 0 { - return nil, errors.Errorf("transaction not signed by %s", strings.Join(signers, ", ")) - } signersFoundList := make([]string, 0, len(signersFound)) for _, signer := range signers { From 3fedc4c6cbb94eb573e16b5052a211f500fb5feb Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 28 Jan 2020 10:12:06 -0800 Subject: [PATCH 24/29] Remove unnecessary word in comment Co-Authored-By: Eric Saunders --- txnbuild/transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 0002bc3e66..91888d82e3 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -577,7 +577,7 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign clientSigners = append(clientSigners, signer) } - // Verify that all the transaction's signers (server and client) in one + // Verify all the transaction's signers (server and client) in one // hit. We do this in one hit here even though the server signature was // checked in the ReadChallengeTx to ensure that every signature and signer // are consumed only once on the transaction. From 916a4cf816419d6045ba13d0b83b7e0b4e4163eb Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 28 Jan 2020 10:05:33 -0800 Subject: [PATCH 25/29] More consistent variable names --- txnbuild/transaction.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 91888d82e3..47995f2db6 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -506,12 +506,12 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti // server account or one of the signers provided in the arguments. // - The signatures are all valid but do not meet the threshold. func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, threshold Threshold, signerSummary SignerSummary) (signersFound []string, err error) { - signerAddresses := make([]string, 0, len(signerSummary)) + signers := make([]string, 0, len(signerSummary)) for s := range signerSummary { - signerAddresses = append(signerAddresses, s) + signers = append(signers, s) } - signersFound, err = VerifyChallengeTxSigners(challengeTx, serverAccountID, network, signerAddresses...) + signersFound, err = VerifyChallengeTxSigners(challengeTx, serverAccountID, network, signers...) if err != nil { return nil, err } From fb8198b8c8f8d5ba1490406de81bec70eeb2936d Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 28 Jan 2020 10:09:20 -0800 Subject: [PATCH 26/29] Preserve the order of client signers throughout the whole process --- txnbuild/transaction.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 47995f2db6..9477848133 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -560,7 +560,8 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign // Deduplicate the client signers and ensure the server is not included // anywhere we check or output the list of signers. - clientSignersSet := map[string]struct{}{} + clientSigners := []string{} + clientSignersSeen := map[string]struct{}{} for _, signer := range signers { // Ignore the server signer if it is in the signers list. It's // important when verifying signers of a challenge transaction that we @@ -570,11 +571,11 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign if signer == serverKP.Address() { continue } - clientSignersSet[signer] = struct{}{} - } - clientSigners := make([]string, 0, len(clientSignersSet)) - for signer := range clientSignersSet { + if _, seen := clientSignersSeen[signer]; seen { + continue + } clientSigners = append(clientSigners, signer) + clientSignersSeen[signer] = struct{}{} } // Verify all the transaction's signers (server and client) in one From 2a903e0da11340432cc7a564aeea950b255ca46a Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 28 Jan 2020 10:24:14 -0800 Subject: [PATCH 27/29] Add test proving duplicate signers in request are not included in errors --- txnbuild/transaction_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index cac7f58170..dde6607d12 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -2077,6 +2077,35 @@ func TestVerifyChallengeTxSigners_validServerAndClientSignersIgnoresDuplicateSig assert.NoError(t, err) } +func TestVerifyChallengeTxSigners_invalidServerAndClientSignersIgnoresDuplicateSignerInError(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP.Address(), clientKP.Address()) + assert.Empty(t, signersFound) + assert.EqualError(t, err, "transaction not signed by "+clientKP.Address()) +} + func TestVerifyChallengeTxSigners_invalidServerAndClientSignersFailsDuplicateSignatures(t *testing.T) { serverKP := newKeypair0() clientKP := newKeypair1() From 0914d38ab12fd48c68f8ca726c6502c5f038c48f Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 28 Jan 2020 10:49:17 -0800 Subject: [PATCH 28/29] Add tests to all functions to ensure server key missing will fail, and add some code to ensure the server key is rematched when we're matching the client keys. This is a very unlikely thing protecting against. --- txnbuild/transaction.go | 11 +++++-- txnbuild/transaction_test.go | 60 ++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 9477848133..8537586439 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -588,16 +588,23 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign return nil, err } - // Remove the server from the list of signers found. + // Confirm the server is in the list of signers found and remove it. + serverSignerFound := false signersFound := make([]string, 0, len(allSignersFound)-1) for _, signer := range allSignersFound { if signer == serverKP.Address() { + serverSignerFound = true continue } signersFound = append(signersFound, signer) } - // Confirm we found at least one client signer. + // Confirm we matched a signature to the server signer. + if !serverSignerFound { + return nil, errors.Errorf("transaction not signed by %s", serverKP.Address()) + } + + // Confirm we matched signatures to the client signers. if len(signersFound) == 0 { return nil, errors.Errorf("transaction not signed by %s", strings.Join(clientSigners, ", ")) } diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index dde6607d12..fc47f3c2ef 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -1567,6 +1567,38 @@ func TestReadChallengeTx_invalidDataValueWrongByteLength(t *testing.T) { assert.EqualError(t, err, "random nonce before encoding as base64 should be 48 bytes long") } +func TestVerifyChallengeTxThreshold_invalidServer(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + threshold := Threshold(1) + signerSummary := SignerSummary{ + clientKP.Address(): 1, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(clientKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary) + assert.Empty(t, signersFound) + assert.EqualError(t, err, "transaction not signed by "+serverKP.Address()) +} + func TestVerifyChallengeTxThreshold_validServerAndClientKeyMeetingThreshold(t *testing.T) { serverKP := newKeypair0() clientKP := newKeypair1() @@ -1818,6 +1850,34 @@ func TestVerifyChallengeTxThreshold_weightsAddToMoreThan8Bits(t *testing.T) { assert.NoError(t, err) } +func TestVerifyChallengeTxSigners_invalidServer(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(clientKP) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP.Address()) + assert.Empty(t, signersFound) + assert.EqualError(t, err, "transaction not signed by "+serverKP.Address()) +} + func TestVerifyChallengeTxSigners_validServerAndClientMasterKey(t *testing.T) { serverKP := newKeypair0() clientKP := newKeypair1() From a7e38389fb5ddd28a93f07e539d11f1fb01661c9 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Tue, 28 Jan 2020 11:04:19 -0800 Subject: [PATCH 29/29] More meaningful error message if signer is a seed --- txnbuild/transaction.go | 2 +- txnbuild/transaction_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/txnbuild/transaction.go b/txnbuild/transaction.go index 8537586439..aae64dc11c 100644 --- a/txnbuild/transaction.go +++ b/txnbuild/transaction.go @@ -665,7 +665,7 @@ func verifyTxSignatures(tx Transaction, signers ...string) ([]string, error) { for _, signer := range signers { kp, err := keypair.ParseAddress(signer) if err != nil { - return nil, err + return nil, errors.Wrap(err, "signer not address") } for i, decSig := range tx.xdrEnvelope.Signatures { diff --git a/txnbuild/transaction_test.go b/txnbuild/transaction_test.go index fc47f3c2ef..5cf0178e03 100644 --- a/txnbuild/transaction_test.go +++ b/txnbuild/transaction_test.go @@ -2195,6 +2195,35 @@ func TestVerifyChallengeTxSigners_invalidServerAndClientSignersFailsDuplicateSig assert.EqualError(t, err, "transaction has unrecognized signatures") } +func TestVerifyChallengeTxSigners_invalidServerAndClientSignersFailsSignerSeed(t *testing.T) { + serverKP := newKeypair0() + clientKP := newKeypair1() + clientKP2 := newKeypair2() + txSource := NewSimpleAccount(serverKP.Address(), -1) + opSource := NewSimpleAccount(clientKP.Address(), 0) + op := ManageData{ + SourceAccount: &opSource, + Name: "testserver auth", + Value: []byte(base64.StdEncoding.EncodeToString(make([]byte, 48))), + } + tx := Transaction{ + SourceAccount: &txSource, + Operations: []Operation{&op}, + Timebounds: NewTimeout(1000), + Network: network.TestNetworkPassphrase, + } + + err := tx.Build() + require.NoError(t, err) + err = tx.Sign(serverKP, clientKP2, clientKP2) + assert.NoError(t, err) + tx64, err := tx.Base64() + require.NoError(t, err) + signersFound, err := VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase, clientKP2.Seed()) + assert.Empty(t, signersFound) + assert.EqualError(t, err, "signer not address: invalid version byte") +} + func TestVerifyChallengeTxSigners_invalidNoSigners(t *testing.T) { serverKP := newKeypair0() clientKP := newKeypair1()