txnbuild: ignore txhash (preauth) and xhash signers in sep-10 (#2215)

### What
Ignore txhash (T...) signers, used for preauth transactions, xhash (X...)
signers, and other future types of signers the server might not know
about, during SEP-10 verification.

### Why
Developers will likely pass through to the verification functions the
signers on accounts as provided by Horizon. Accounts can have other
non-ed25519 signers and they're likely going to be passed through
verbatim. The verification logic's goal is to confirm the transaction
has been signed by the signers and so ignoring unsupported types like
txhash and xhash seems like a safe thing to do given that the
verification function will also ignore ed25519 signers that don't match
a signature.

Without this in a typical SEP-10 implementation any account with a
txhash or xhash signer will likely fail SEP-10 verification.

Issues that might be caused by this new behavior is if a user passes in
an account seed (S...) or some other string they won't see an error.
I think that's unlikely and hopefully a smaller impact than is worth
making this solution more complex.

This issue was first identified by @overcat in lightsail-network/java-stellar-sdk#264,
but solved in a way that depends on data from Horizon. This solution
does not depend on data from Horizon and should be portable to all our
SDKs. This was previously discussed at:
lightsail-network/java-stellar-sdk#264 (comment).

txnbuild/transaction.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/stellar/go/keypair"
 	"github.com/stellar/go/network"
+	"github.com/stellar/go/strkey"
 	"github.com/stellar/go/support/errors"
 	"github.com/stellar/go/xdr"
 )
@@ -499,6 +500,9 @@ func ReadChallengeTx(challengeTx, serverAccountID, network string) (tx Transacti
 // provided as an argument, and those signatures meet a threshold on the
 // account.
 //
+// Signers that are not prefixed as an address/account ID strkey (G...) will be
+// ignored.
+//
 // Errors will be raised if:
 //  - The transaction is invalid according to ReadChallengeTx.
 //  - No client signatures are found on the transaction.
@@ -536,16 +540,15 @@ func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, th
 // to a signer for verification to succeed. If verification succeeds a list of
 // signers that were found is returned, excluding the server account ID.
 //
+// Signers that are not prefixed as an address/account ID strkey (G...) will be
+// ignored.
+//
 // 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) {
-	if len(signers) == 0 {
-		return nil, errors.New("no signers provided")
-	}
-
 	// Read the transaction which validates its structure.
 	tx, _, err := ReadChallengeTx(challengeTx, serverAccountID, network)
 	if err != nil {
@@ -571,13 +574,27 @@ func VerifyChallengeTxSigners(challengeTx, serverAccountID, network string, sign
 		if signer == serverKP.Address() {
 			continue
 		}
+		// Deduplicate.
 		if _, seen := clientSignersSeen[signer]; seen {
 			continue
 		}
+		// Ignore non-G... account/address signers.
+		strkeyVersionByte, strkeyErr := strkey.Version(signer)
+		if strkeyErr != nil {
+			continue
+		}
+		if strkeyVersionByte != strkey.VersionByteAccountID {
+			continue
+		}
 		clientSigners = append(clientSigners, signer)
 		clientSignersSeen[signer] = struct{}{}
 	}
 
+	// Don't continue if none of the signers provided are in the final list.
+	if len(clientSigners) == 0 {
+		return nil, errors.New("no verifiable signers provided, at least one G... address must be provided")
+	}
+
 	// 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

txnbuild/transaction_test.go

@@ -1712,6 +1712,52 @@ func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresh
 	assert.NoError(t, err)
 }
 
+func TestVerifyChallengeTxThreshold_validServerAndMultipleClientKeyMeetingThresholdSomeUnusedIgnorePreauthTxHashAndXHash(t *testing.T) {
+	serverKP := newKeypair0()
+	clientKP1 := newKeypair1()
+	clientKP2 := newKeypair2()
+	clientKP3 := keypair.MustRandom()
+	preauthTxHash := "TAQCSRX2RIDJNHFIFHWD63X7D7D6TRT5Y2S6E3TEMXTG5W3OECHZ2OG4"
+	xHash := "XDRPF6NZRR7EEVO7ESIWUDXHAOMM2QSKIQQBJK6I2FB7YKDZES5UCLWD"
+	unknownSignerType := "?ARPF6NZRR7EEVO7ESIWUDXHAOMM2QSKIQQBJK6I2FB7YKDZES5UCLWD"
+	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)
+	signerSummary := SignerSummary{
+		clientKP1.Address(): 1,
+		clientKP2.Address(): 2,
+		clientKP3.Address(): 2,
+		preauthTxHash:       10,
+		xHash:               10,
+		unknownSignerType:   10,
+	}
+	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.ElementsMatch(t, wantSigners, signersFound)
+	assert.NoError(t, err)
+}
+
 func TestVerifyChallengeTxThreshold_invalidServerAndMultipleClientKeyNotMeetingThreshold(t *testing.T) {
 	serverKP := newKeypair0()
 	clientKP1 := newKeypair1()
@@ -1809,7 +1855,7 @@ func TestVerifyChallengeTxThreshold_invalidNoSigners(t *testing.T) {
 	tx64, err := tx.Base64()
 	require.NoError(t, err)
 	_, err = VerifyChallengeTxThreshold(tx64, serverKP.Address(), network.TestNetworkPassphrase, threshold, signerSummary)
-	assert.EqualError(t, err, "no signers provided")
+	assert.EqualError(t, err, "no verifiable signers provided, at least one G... address must be provided")
 }
 
 func TestVerifyChallengeTxThreshold_weightsAddToMoreThan8Bits(t *testing.T) {
@@ -2137,6 +2183,38 @@ func TestVerifyChallengeTxSigners_validServerAndClientSignersIgnoresDuplicateSig
 	assert.NoError(t, err)
 }
 
+func TestVerifyChallengeTxSigners_validIgnorePreauthTxHashAndXHash(t *testing.T) {
+	serverKP := newKeypair0()
+	clientKP := newKeypair1()
+	clientKP2 := newKeypair2()
+	preauthTxHash := "TAQCSRX2RIDJNHFIFHWD63X7D7D6TRT5Y2S6E3TEMXTG5W3OECHZ2OG4"
+	xHash := "XDRPF6NZRR7EEVO7ESIWUDXHAOMM2QSKIQQBJK6I2FB7YKDZES5UCLWD"
+	unknownSignerType := "?ARPF6NZRR7EEVO7ESIWUDXHAOMM2QSKIQQBJK6I2FB7YKDZES5UCLWD"
+	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(), preauthTxHash, xHash, unknownSignerType)
+	assert.Equal(t, []string{clientKP2.Address()}, signersFound)
+	assert.NoError(t, err)
+}
+
 func TestVerifyChallengeTxSigners_invalidServerAndClientSignersIgnoresDuplicateSignerInError(t *testing.T) {
 	serverKP := newKeypair0()
 	clientKP := newKeypair1()
@@ -2221,7 +2299,7 @@ func TestVerifyChallengeTxSigners_invalidServerAndClientSignersFailsSignerSeed(t
 	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")
+	assert.EqualError(t, err, "no verifiable signers provided, at least one G... address must be provided")
 }
 
 func TestVerifyChallengeTxSigners_invalidNoSigners(t *testing.T) {
@@ -2248,7 +2326,7 @@ func TestVerifyChallengeTxSigners_invalidNoSigners(t *testing.T) {
 	tx64, err := tx.Base64()
 	require.NoError(t, err)
 	_, err = VerifyChallengeTxSigners(tx64, serverKP.Address(), network.TestNetworkPassphrase)
-	assert.EqualError(t, err, "no signers provided")
+	assert.EqualError(t, err, "no verifiable signers provided, at least one G... address must be provided")
 }
 
 func TestVerifyTxSignatureUnsignedTx(t *testing.T) {