From 27fe9151986dccf52c1618ca200ad021a2da2ae6 Mon Sep 17 00:00:00 2001 From: Jake Urban Date: Tue, 2 Nov 2021 17:44:26 -0700 Subject: [PATCH 1/4] added client attribution to SEP-10 helpers --- src/utils.ts | 47 ++++++++++++- test/unit/utils_test.js | 146 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 188 insertions(+), 5 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 2019f7cbd..9fa3ba810 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -49,6 +49,8 @@ export namespace Utils { networkPassphrase: string, webAuthDomain: string, memo: string | null = null, + clientDomain: string | null = null, + clientSigningKey: string | null = null, ): string { if (clientAccountID.startsWith("M") && memo) { throw Error("memo cannot be used if clientAccountID is a muxed account"); @@ -88,6 +90,19 @@ export namespace Utils { }), ); + if (clientDomain) { + if (!clientSigningKey) { + throw Error("clientSigningKey is required if clientDomain is provided"); + } + builder.addOperation( + Operation.manageData({ + name: `client_domain`, + value: clientDomain, + source: clientSigningKey, + }), + ); + } + if (memo) { builder.addMemo(Memo.id(memo)); } @@ -284,7 +299,7 @@ export namespace Utils { "The transaction has operations that are not of type 'manageData'", ); } - if (op.source !== serverAccountID) { + if (op.source !== serverAccountID && op.name !== "client_domain") { throw new InvalidSep10ChallengeError( "The transaction has operations that are unrecognized", ); @@ -533,6 +548,14 @@ export namespace Utils { ); } + let clientSigningKey; + for (const op of tx.operations) { + if (op.type === "manageData" && op.name === "client_domain") { + clientSigningKey = op.source; + break; + } + } + // 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 @@ -541,16 +564,32 @@ export namespace Utils { serverKP.publicKey(), ...Array.from(clientSigners), ]; + if (clientSigningKey) { allSigners.push(clientSigningKey); } const signersFound: string[] = gatherTxSigners(tx, allSigners); + let serverSignatureFound = false; + let clientSigningKeySignatureFound = false; + for (const signer of signersFound) { + if (signer === serverKP.publicKey()) { serverSignatureFound = true; } + if (signer === clientSigningKey) { clientSigningKeySignatureFound = true; } + } + // Confirm we matched a signature to the server signer. - if (signersFound.indexOf(serverKP.publicKey()) === -1) { + if (!serverSignatureFound) { throw new InvalidSep10ChallengeError( "Transaction not signed by server: '" + serverKP.publicKey() + "'", ); } + // Confirm we matched a signature to the client domain's signer + if (clientSigningKey && !clientSigningKeySignatureFound) { + throw new InvalidSep10ChallengeError( + "Transaction not signed by the source account of the 'client_domain' " + + "ManageData operation", + ); + } + // Confirm we matched at least one given signer with the transaction signatures if (signersFound.length === 1) { throw new InvalidSep10ChallengeError( @@ -567,6 +606,10 @@ export namespace Utils { // Remove the server public key before returning signersFound.splice(signersFound.indexOf(serverKP.publicKey()), 1); + if (clientSigningKey) { + // Remove the client domain public key public key before returning + signersFound.splice(signersFound.indexOf(clientSigningKey), 1); + } return signersFound; } diff --git a/test/unit/utils_test.js b/test/unit/utils_test.js index 9dfda9f13..4a6b81cf5 100644 --- a/test/unit/utils_test.js +++ b/test/unit/utils_test.js @@ -95,6 +95,7 @@ describe('Utils', function() { it('returns challenge which follows SEP0010 spec', function() { let keypair = StellarSdk.Keypair.random(); + let clientSigningKeypair = StellarSdk.Keypair.random(); const challenge = StellarSdk.Utils.buildChallengeTx( keypair, @@ -102,20 +103,23 @@ describe('Utils', function() { "testanchor.stellar.org", 300, StellarSdk.Networks.TESTNET, - "testanchor.stellar.org" + "testanchor.stellar.org", + null, + "testdomain", + clientSigningKeypair.publicKey() ); const transaction = new StellarSdk.Transaction(challenge, StellarSdk.Networks.TESTNET); expect(transaction.sequence).to.eql("0"); expect(transaction.source).to.eql(keypair.publicKey()); - expect(transaction.operations.length).to.eql(2); + expect(transaction.operations.length).to.eql(3); const { maxTime, minTime } = transaction.timeBounds; expect(parseInt(maxTime) - parseInt(minTime)).to.eql(300); - const [ operation1, operation2 ] = transaction.operations; + const [ operation1, operation2, operation3 ] = transaction.operations; expect(operation1.name).to.eql("testanchor.stellar.org auth"); expect(operation1.source).to.eql("GBDIT5GUJ7R5BXO3GJHFXJ6AZ5UQK6MNOIDMPQUSMXLIHTUNR2Q5CFNF"); @@ -127,6 +131,11 @@ describe('Utils', function() { expect(operation2.source).to.eql(keypair.publicKey()); expect(operation2.type).to.eql("manageData"); expect(operation2.value.toString()).to.eql("testanchor.stellar.org"); + + expect(operation3.name).to.eql("client_domain"); + expect(operation3.source).to.eql(clientSigningKeypair.publicKey()); + expect(operation3.type).to.eql("manageData"); + expect(operation3.value.toString()).to.eql("testdomain"); }); it('uses the passed-in timeout', function() { @@ -169,6 +178,24 @@ describe('Utils', function() { ); }); + it("throws an error if clientSigningKey is not passed", function() { + expect(() => + StellarSdk.Utils.buildChallengeTx( + StellarSdk.Keypair.random(), + StellarSdk.Keypair.random().publicKey(), + "testanchor.stellar.org", + 600, + StellarSdk.Networks.TESTNET, + "testanchor.stellar.org", + null, + "testdomain", + null + ) + ).to.throw( + /clientSigningKey is required if clientDomain is provided/ + ); + }); + }); describe("Utils.readChallengeTx", function() { @@ -1424,6 +1451,31 @@ describe('Utils', function() { memo: null }); }); + + it("validates a challenge containing a 'client_domain' manageData operation", () => { + let keypair = StellarSdk.Keypair.random(); + let clientSigningKeypair = StellarSdk.Keypair.random(); + + const challenge = StellarSdk.Utils.buildChallengeTx( + keypair, + "GBDIT5GUJ7R5BXO3GJHFXJ6AZ5UQK6MNOIDMPQUSMXLIHTUNR2Q5CFNF", + "testanchor.stellar.org", + 300, + StellarSdk.Networks.TESTNET, + "testanchor.stellar.org", + null, + "testdomain", + clientSigningKeypair.publicKey() + ); + + StellarSdk.Utils.readChallengeTx( + challenge, + keypair.publicKey(), + StellarSdk.Networks.TESTNET, + "testanchor.stellar.org", + "testanchor.stellar.org" + ); + }); }); describe("Utils.verifyChallengeTxThreshold", function() { @@ -2387,6 +2439,94 @@ describe('Utils', function() { /No verifiable client signers provided, at least one G... address must be provided/, ); }); + + it("validates challenges containing client domain signers", () => { + const serverKP = StellarSdk.Keypair.random(); + const clientKP = StellarSdk.Keypair.random(); + const clientSigningKey = StellarSdk.Keypair.random(); + const challenge = StellarSdk.Utils.buildChallengeTx( + serverKP, + clientKP.publicKey(), + "SDF", + 300, + StellarSdk.Networks.TESTNET, + "testanchor.stellar.org", + null, + "testdomain", + clientSigningKey.publicKey() + ); + + clock.tick(200); + + const transaction = new StellarSdk.Transaction( + challenge, + StellarSdk.Networks.TESTNET + ); + + transaction.sign(clientKP); + transaction.sign(clientSigningKey); + + const signedChallenge = transaction + .toEnvelope() + .toXDR("base64") + .toString(); + + const signersFound = StellarSdk.Utils.verifyChallengeTxSigners( + signedChallenge, + serverKP.publicKey(), + StellarSdk.Networks.TESTNET, + [clientKP.publicKey()], + "SDF", + "testanchor.stellar.org" + ); + + expect(signersFound.indexOf(clientSigningKey.publicKey())).to.eql(-1); + }); + + it("throws an error if a challenge with a client_domain operation doesn't have a matching signature", () => { + const serverKP = StellarSdk.Keypair.random(); + const clientKP = StellarSdk.Keypair.random(); + const clientSigningKeypair = StellarSdk.Keypair.random(); + const challenge = StellarSdk.Utils.buildChallengeTx( + serverKP, + clientKP.publicKey(), + "SDF", + 300, + StellarSdk.Networks.TESTNET, + "testanchor.stellar.org", + null, + "testdomain", + clientSigningKeypair.publicKey() + ); + + clock.tick(200); + + const transaction = new StellarSdk.Transaction( + challenge, + StellarSdk.Networks.TESTNET + ); + + transaction.sign(clientKP); + + const signedChallenge = transaction + .toEnvelope() + .toXDR("base64") + .toString(); + + expect(() => + StellarSdk.Utils.verifyChallengeTxSigners( + signedChallenge, + serverKP.publicKey(), + StellarSdk.Networks.TESTNET, + [clientKP.publicKey()], + "SDF", + "testanchor.stellar.org" + ) + ).to.throw( + StellarSdk.InvalidSep10ChallengeError, + /Transaction not signed by the source account of the 'client_domain' ManageData operation/ + ); + }); }); describe('Utils.verifyTxSignedBy', function() { From 96fb1ab043be0c44b985ad79f48f9818703ff367 Mon Sep 17 00:00:00 2001 From: Jake Urban <10968980+JakeUrban@users.noreply.github.com> Date: Thu, 4 Nov 2021 09:54:05 -0700 Subject: [PATCH 2/4] Update src/utils.ts Co-authored-by: George --- src/utils.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 9fa3ba810..d2615ce48 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -548,13 +548,14 @@ export namespace Utils { ); } - let clientSigningKey; - for (const op of tx.operations) { - if (op.type === "manageData" && op.name === "client_domain") { - clientSigningKey = op.source; - break; - } - } + let clientSigningKey; + for (const op of tx.operations) { + if (op.type === "manageData" && op.name === "client_domain") { + if (clientSigningKey) + throw new InvalidSep10ChallengeError("Found more than one client_domain signing key"); + clientSigningKey = op.source; + } + } // 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 From 9278edc51554ed24e830d81dbad867911dbfcd6b Mon Sep 17 00:00:00 2001 From: Jake Urban Date: Thu, 4 Nov 2021 10:18:37 -0700 Subject: [PATCH 3/4] make PR adjustments --- src/utils.ts | 31 +++++++----- test/unit/utils_test.js | 104 +++++++++++++++++++++++++++++++++++----- 2 files changed, 111 insertions(+), 24 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index d2615ce48..2ec15bb71 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -548,14 +548,17 @@ export namespace Utils { ); } - let clientSigningKey; - for (const op of tx.operations) { - if (op.type === "manageData" && op.name === "client_domain") { - if (clientSigningKey) - throw new InvalidSep10ChallengeError("Found more than one client_domain signing key"); - clientSigningKey = op.source; - } - } + let clientSigningKey; + for (const op of tx.operations) { + if (op.type === "manageData" && op.name === "client_domain") { + if (clientSigningKey) { + throw new InvalidSep10ChallengeError( + "Found more than one client_domain operation", + ); + } + clientSigningKey = op.source; + } + } // 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 @@ -565,15 +568,21 @@ export namespace Utils { serverKP.publicKey(), ...Array.from(clientSigners), ]; - if (clientSigningKey) { allSigners.push(clientSigningKey); } + if (clientSigningKey) { + allSigners.push(clientSigningKey); + } const signersFound: string[] = gatherTxSigners(tx, allSigners); let serverSignatureFound = false; let clientSigningKeySignatureFound = false; for (const signer of signersFound) { - if (signer === serverKP.publicKey()) { serverSignatureFound = true; } - if (signer === clientSigningKey) { clientSigningKeySignatureFound = true; } + if (signer === serverKP.publicKey()) { + serverSignatureFound = true; + } + if (signer === clientSigningKey) { + clientSigningKeySignatureFound = true; + } } // Confirm we matched a signature to the server signer. diff --git a/test/unit/utils_test.js b/test/unit/utils_test.js index 4a6b81cf5..5f9fa7c11 100644 --- a/test/unit/utils_test.js +++ b/test/unit/utils_test.js @@ -1453,24 +1453,41 @@ describe('Utils', function() { }); it("validates a challenge containing a 'client_domain' manageData operation", () => { - let keypair = StellarSdk.Keypair.random(); + let serverKP = StellarSdk.Keypair.random(); + let clientKP = StellarSdk.Keypair.random(); + const serverAccount = new StellarSdk.Account(serverKP.publicKey(), "-1"); let clientSigningKeypair = StellarSdk.Keypair.random(); - const challenge = StellarSdk.Utils.buildChallengeTx( - keypair, - "GBDIT5GUJ7R5BXO3GJHFXJ6AZ5UQK6MNOIDMPQUSMXLIHTUNR2Q5CFNF", - "testanchor.stellar.org", - 300, - StellarSdk.Networks.TESTNET, - "testanchor.stellar.org", - null, - "testdomain", - clientSigningKeypair.publicKey() - ); + const transaction = new StellarSdk.TransactionBuilder( + serverAccount, + txBuilderOpts, + ) + .addOperation( + StellarSdk.Operation.manageData({ + source: clientKP.publicKey(), + name: "testanchor.stellar.org auth", + value: randomBytes(48).toString("base64"), + }), + ) + .addOperation( + StellarSdk.Operation.manageData({ + source: clientSigningKeypair.publicKey(), + name: "client_domain", + value: "testdomain" + }), + ) + .setTimeout(30) + .build(); + + transaction.sign(serverKP); + const challenge = transaction + .toEnvelope() + .toXDR("base64") + .toString(); StellarSdk.Utils.readChallengeTx( challenge, - keypair.publicKey(), + serverKP.publicKey(), StellarSdk.Networks.TESTNET, "testanchor.stellar.org", "testanchor.stellar.org" @@ -2527,6 +2544,67 @@ describe('Utils', function() { /Transaction not signed by the source account of the 'client_domain' ManageData operation/ ); }); + + it("throws an error if a challenge has multiple client_domain operations", () => { + const serverKP = StellarSdk.Keypair.random(); + const clientKP = StellarSdk.Keypair.random(); + const clientSigningKeypair = StellarSdk.Keypair.random(); + + const serverAccount = new StellarSdk.Account(serverKP.publicKey(), "-1"); + + const transaction = new StellarSdk.TransactionBuilder( + serverAccount, + txBuilderOpts, + ) + .addOperation( + StellarSdk.Operation.manageData({ + source: clientKP.publicKey(), + name: "testanchor.stellar.org auth", + value: randomBytes(48).toString("base64"), + }), + ) + .addOperation( + StellarSdk.Operation.manageData({ + source: clientSigningKeypair.publicKey(), + name: "client_domain", + value: "testdomain" + }), + ) + .addOperation( + StellarSdk.Operation.manageData({ + source: clientSigningKeypair.publicKey(), + name: "client_domain", + value: "testdomain2" + }), + ) + .setTimeout(30) + .build(); + + clock.tick(200); + + transaction.sign(serverKP); + transaction.sign(clientKP); + transaction.sign(clientSigningKeypair); + + const signedChallenge = transaction + .toEnvelope() + .toXDR("base64") + .toString(); + + expect(() => + StellarSdk.Utils.verifyChallengeTxSigners( + signedChallenge, + serverKP.publicKey(), + StellarSdk.Networks.TESTNET, + [clientKP.publicKey()], + "testanchor.stellar.org", + "testanchor.stellar.org" + ) + ).to.throw( + StellarSdk.InvalidSep10ChallengeError, + /Found more than one client_domain operation/ + ); + }); }); describe('Utils.verifyTxSignedBy', function() { From 2f1db823f4a9b25cdc7233a80fe2ed36ecf4e065 Mon Sep 17 00:00:00 2001 From: Jake Urban Date: Thu, 4 Nov 2021 10:27:41 -0700 Subject: [PATCH 4/4] update CHANGELOG.md --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index da882b319..ac8e4b2ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ A breaking change will get clearly marked in this log. ## Unreleased +### Add + +- Adds [client domain verification](https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0010.md#verifying-the-client-domain) to the SEP-10 helper functions ([#720](https://github.com/stellar/js-stellar-sdk/pull/720)): + - `Utils.buildChallengeTx()` + - `Utils.readChallengeTx()` + - `Utils.verifyChallengeTxSigners()` ## [v9.0.0](https://github.com/stellar/js-stellar-sdk/compare/v9.0.0-beta.1...v9.0.0)