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.

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: function to verify sep10 challenge tx #1576

Merged
merged 3 commits into from
Aug 9, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
86 changes: 86 additions & 0 deletions txnbuild/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,3 +406,89 @@ 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.
// 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) {
abuiles marked this conversation as resolved.
Show resolved Hide resolved
poliha marked this conversation as resolved.
Show resolved Hide resolved
tx, err := TransactionFromXDR(challengeTx)
if err != nil {
return false, err
}
tx.Network = network

// verify transaction source
if tx.SourceAccount != nil {
poliha marked this conversation as resolved.
Show resolved Hide resolved
if tx.SourceAccount.GetAccountID() != serverAccountId {
return false, errors.New("transaction source account is not equal to server's account")
}
} else {
return false, errors.New("transaction requires a source account")
}

// verify timebounds
if tx.Timebounds.wasBuilt {
poliha marked this conversation as resolved.
Show resolved Hide resolved
// if timebound is not infinite
if tx.Timebounds.MinTime > 0 && tx.Timebounds.MaxTime > 0 {
poliha marked this conversation as resolved.
Show resolved Hide resolved
currentTime := time.Now().UTC().Unix()
if currentTime < tx.Timebounds.MinTime || currentTime > tx.Timebounds.MaxTime {
return false, errors.New("transaction timebounds has elapsed")
}
}
} else {
return false, errors.New("transaction requires timebounds")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return false, errors.New("transaction requires timebounds")
return false, errors.New("transaction requires non-infinite timebounds")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timebounds can be infinite. The spec only recommends a timebound, and the BuildChallengeTx allows it as well. This check is for transactions built with older versions of the SDK that do not have timebounds implemented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with this, so let's figure out where we're each coming from. I think a valid challenge must include timebounds.

I read this in the spec:

Response

On success the endpoint must return 200 OK HTTP status code and a JSON object with these fields:

transaction: an XDR-encoded Stellar transaction with the following:
...
time bounds: {min: now(), max: now() + 300 } (we recommend expiration of 5 minutes to give user time to sign transaction)

And then in the section on validation:

verify that transaction has time bounds set, and that current time is between the minimum and maximum bounds;

The reason timebounds are important is to protect the server from receiving responses after a long time has passed. I don't think they should be optional. We can raise this for clarification on the SEP though if you interpret it differently.

}

// verify operation
if len(tx.Operations) == 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, reverse. Fail fast if len(tx.Operations) != 1

op, ok := tx.Operations[0].(*ManageData)
if !ok {
return false, errors.New("operation type should be manage_data")
}
if op.SourceAccount == nil {
return false, errors.New("operation should have a source account")
}
// verify signature from operation source
ok, err = verifyTxSignature(tx, op.SourceAccount.GetAccountID())
if err != nil {
return ok, err
}
} else {
return false, errors.New("transaction requires a single manage_data operation")
}

// verify signature from server signing key
return verifyTxSignature(tx, serverAccountId)
}
abuiles marked this conversation as resolved.
Show resolved Hide resolved

// verifyTxSignature checks if a transaction has been signed by the provided stellar account.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stellar -> Stellar

func verifyTxSignature(tx Transaction, accountID string) (bool, error) {
signerFound := false
txHash, err := tx.Hash()
if err != nil {
return signerFound, err
}

kp, err := keypair.Parse(accountID)
if err != nil {
return signerFound, err
}

// find and verify signatures
if tx.xdrEnvelope != nil && len(tx.xdrEnvelope.Signatures) > 0 {
poliha marked this conversation as resolved.
Show resolved Hide resolved
for _, s := range tx.xdrEnvelope.Signatures {
e := kp.Verify(txHash[:], s.Signature)
if e == nil {
signerFound = true
break
}
}
} else {
return signerFound, errors.New("transaction has no signatures")
}

if !signerFound {
return signerFound, errors.Errorf("transaction not signed by %s", accountID)
}
return signerFound, nil
}
134 changes: 134 additions & 0 deletions txnbuild/transaction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1156,3 +1156,137 @@ func TestSignWithSecretKey(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, expected, actual, "base64 xdr should match")
}

func TestVerifyTxSignature(t *testing.T) {
kp0 := newKeypair0()
kp1 := newKeypair1()
txSource := NewSimpleAccount(kp0.Address(), int64(9605939170639897))
opSource := NewSimpleAccount(kp1.Address(), 0)
createAccount := CreateAccount{
Destination: "GCCOBXW2XQNUSL467IEILE6MMCNRR66SSVL4YQADUNYYNUVREF3FIV2Z",
Amount: "10",
SourceAccount: &opSource,
}
tx := Transaction{
SourceAccount: &txSource,
Operations: []Operation{&createAccount},
Timebounds: NewInfiniteTimeout(),
Network: network.TestNetworkPassphrase,
}

// verify unsigned tx
poliha marked this conversation as resolved.
Show resolved Hide resolved
err := tx.Build()
assert.NoError(t, err)
ok, err := verifyTxSignature(tx, kp0.Address())
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "transaction has no signatures")
}
assert.Equal(t, false, ok)

// verify tx with one signature
err = tx.Sign(kp0)
assert.NoError(t, err)
ok, err = verifyTxSignature(tx, kp0.Address())
assert.NoError(t, err)
assert.Equal(t, true, ok)

// verify tx with multiple signature
err = tx.Sign(kp1)
assert.NoError(t, err)
ok, err = verifyTxSignature(tx, kp0.Address())
assert.NoError(t, err)
assert.Equal(t, true, ok)
ok, err = verifyTxSignature(tx, kp1.Address())
assert.NoError(t, err)
assert.Equal(t, true, ok)

// verify invalid signer
ok, err = verifyTxSignature(tx, "GATBMIXTHXYKSUZSZUEJKACZ2OS2IYUWP2AIF3CA32PIDLJ67CH6Y5UY")
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "transaction not signed by GATBMIXTHXYKSUZSZUEJKACZ2OS2IYUWP2AIF3CA32PIDLJ67CH6Y5UY")
}
assert.Equal(t, false, ok)
}

func TestVerifyChallengeTx(t *testing.T) {
invalidTx := "AAAAACYWIvM98KlTMs0IlQBZ06WkYpZ+gILsQN6ega0++I/sAAAAZAAXeEkAAAABAAAAAAAAAAEAAAAQMkExVjZKNTcwM0c0N1hIWQAAAAEAAAABAAAAACYWIvM98KlTMs0IlQBZ06WkYpZ+gILsQN6ega0++I/sAAAAAQAAAADMSEvcRKXsaUNna++Hy7gWm/CfqTjEA7xoGypfrFGUHAAAAAAAAAACCPHRAAAAAAAAAAABPviP7AAAAEBu6BCKf4WZHPum5+29Nxf6SsJNN8bgjp1+e1uNBaHjRg3rdFZYgUqEqbHxVEs7eze3IeRbjMZxS3zPf/xwJCEI"

_, err := VerifyChallengeTx(invalidTx, "GATBMIXTHXYKSUZSZUEJKACZ2OS2IYUWP2AIF3CA32PIDLJ67CH6Y5UY", network.TestNetworkPassphrase)
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "transaction requires timebounds")
}

kp0 := newKeypair0()
kp1 := newKeypair1()

// transaction with elapsed timebound
poliha marked this conversation as resolved.
Show resolved Hide resolved
newChallenge, err := BuildChallengeTx(kp0.Seed(), kp1.Address(), "sdf", network.TestNetworkPassphrase, 1)
assert.NoError(t, err)
time.Sleep(2 * time.Second)
_, err = VerifyChallengeTx(newChallenge, kp0.Address(), network.TestNetworkPassphrase)
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "transaction timebounds has elapsed")
}

// transaction not signed by client
newChallenge, err = BuildChallengeTx(kp0.Seed(), kp1.Address(), "sdf", network.TestNetworkPassphrase, 300)
assert.NoError(t, err)
_, err = VerifyChallengeTx(newChallenge, kp0.Address(), network.TestNetworkPassphrase)
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "transaction not signed by "+kp1.Address())
}

// valid transaction signed by client
newChallenge, err = BuildChallengeTx(kp0.Seed(), kp1.Address(), "sdf", network.TestNetworkPassphrase, 300)
assert.NoError(t, err)
newTx, err := TransactionFromXDR(newChallenge)
assert.NoError(t, err)
newTx.Network = network.TestNetworkPassphrase
err = newTx.Sign(kp1)
assert.NoError(t, err)
newChallenge, err = newTx.Base64()
assert.NoError(t, err)
isValid, err := VerifyChallengeTx(newChallenge, kp0.Address(), network.TestNetworkPassphrase)
assert.NoError(t, err)
assert.Equal(t, true, isValid, "challenge should be valid")

// valid transaction with infinite timebound signed by client
newChallenge, err = BuildChallengeTx(kp0.Seed(), kp1.Address(), "sdf", network.TestNetworkPassphrase, 0)
assert.NoError(t, err)
newTx, err = TransactionFromXDR(newChallenge)
assert.NoError(t, err)
newTx.Network = network.TestNetworkPassphrase
err = newTx.Sign(kp1)
assert.NoError(t, err)
newChallenge, err = newTx.Base64()
assert.NoError(t, err)
isValid, err = VerifyChallengeTx(newChallenge, kp0.Address(), network.TestNetworkPassphrase)
assert.NoError(t, err)
assert.Equal(t, true, isValid, "challenge should be valid")

// invalid operation type
txSource := NewSimpleAccount(kp0.Address(), 0)
opSource := NewSimpleAccount(kp1.Address(), 0)
createAccount := CreateAccount{
Destination: "GCCOBXW2XQNUSL467IEILE6MMCNRR66SSVL4YQADUNYYNUVREF3FIV2Z",
Amount: "10",
SourceAccount: &opSource,
}
newTx = Transaction{
SourceAccount: &txSource,
Operations: []Operation{&createAccount},
Timebounds: NewInfiniteTimeout(),
Network: network.TestNetworkPassphrase,
}
err = newTx.Build()
assert.NoError(t, err)
err = newTx.Sign(kp0, kp1)
assert.NoError(t, err)
newChallenge, err = newTx.Base64()
assert.NoError(t, err)
isValid, err = VerifyChallengeTx(newChallenge, kp0.Address(), network.TestNetworkPassphrase)
if assert.Error(t, err) {
assert.Contains(t, err.Error(), "operation type should be manage_data")
}
assert.Equal(t, false, isValid, "challenge should be invalid")
}