Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Feb 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
670a6f5
txnbuild: ignore txhash (preauth) and xhash signers in sep-10
leighmcculloch Feb 1, 2020
3fe9dfa
Move the check to earlier in the flow closer to user data and fix test
leighmcculloch Feb 1, 2020
e969079
Remove old comment
leighmcculloch Feb 1, 2020
9aacbb6
Ignore future other types of strkeys
leighmcculloch Feb 1, 2020
982dfaa
Fix shadow lint error
leighmcculloch Feb 1, 2020
92a6ea3
Merge branch 'master' into sep10ignorehashsigners
leighmcculloch Feb 3, 2020
2111ceb
Remove comments that are too much information
leighmcculloch Feb 3, 2020
7445bb2
Remove more TMI
leighmcculloch Feb 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions txnbuild/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -499,12 +500,17 @@ 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, however signers that that are prefixed but are corrupted in some
// other way will cause an error to be returned.
//
// 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.
// - Any signers appearing to be an accountID/address strkey (start with G) are corrupt.
func VerifyChallengeTxThreshold(challengeTx, serverAccountID, network string, threshold Threshold, signerSummary SignerSummary) (signersFound []string, err error) {
signers := make([]string, 0, len(signerSummary))
for s := range signerSummary {
Expand Down Expand Up @@ -536,16 +542,17 @@ 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, however signers that that are prefixed but are corrupted in some
// other way will cause an error to be returned.
//
// 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.
// - Any signers appearing to be an accountID/address strkey (start with G) are corrupt.
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 {
Expand All @@ -571,13 +578,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
Expand Down
84 changes: 81 additions & 3 deletions txnbuild/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down