refactor: SEP-0010, confirm all signatures were consumed by a signer. (#252)

Thanks to Leigh McCulloch (@leighmcculloch).

Author: overcat

stellar_sdk/sep/ed25519_public_key_signer.py

@@ -1,5 +1,5 @@
 class Ed25519PublicKeySigner:
-    def __init__(self, account_id: str, weight: int) -> None:
+    def __init__(self, account_id: str, weight: int = 0) -> None:
         """The :class:`Signer` object, which represents represents the signer for the client account.
 
         :param account_id: Account ID (ex. GBYNR2QJXLBCBTRN44MRORCMI4YO7FZPFBCNOKTOBCAAFC7KC3LNPRYS)

stellar_sdk/sep/stellar_web_authentication.py

@@ -191,19 +191,44 @@ def verify_challenge_transaction_signers(
     )
     server_keypair = Keypair.from_public_key(server_account_id)
 
-    # 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.
+    # Deduplicate the client signers and ensure the server is not included
+    # anywhere we check or output the list of signers.
     client_signers = []
     for signer in 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 == server_keypair.public_key:
             continue
         client_signers.append(signer)
 
-    signers_found = _verify_transaction_signatures(te, client_signers)
-    if len(signers_found) != len(te.signatures) - 1:
+    # 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 read_challenge_transaction to ensure that every signature and signer
+    # are consumed only once on the transaction.
+    all_signers = client_signers + [Ed25519PublicKeySigner(server_keypair.public_key)]
+    all_signers_found = _verify_transaction_signatures(te, all_signers)
+
+    signers_found = []
+    server_signer_found = False
+    for signer in all_signers_found:
+        if signer.account_id == server_keypair.public_key:
+            server_signer_found = True
+            continue
+        if signer in signers_found:
+            continue
+        signers_found.append(signer)
+
+    # Confirm we matched a signature to the server signer.
+    if not server_signer_found:
+        raise InvalidSep10ChallengeError(
+            "Transaction not signed by server: {}.".format(server_keypair.public_key)
+        )
+
+    # Confirm all signatures were consumed by a signer.
+    if len(all_signers_found) != len(te.signatures):
         raise InvalidSep10ChallengeError("Transaction has unrecognized signatures.")
 
     return signers_found

tests/sep/test_stellar_web_authentication.py

@@ -509,6 +509,44 @@ def test_verify_challenge_transaction_signers_raise_no_signers(self):
                 challenge_tx, server_kp.public_key, network_passphrase, signers
             )
 
+    def test_verify_challenge_transaction_signers_raise_no_server_signature(self):
+        server_kp = Keypair.random()
+        client_kp_a = Keypair.random()
+        client_kp_b = Keypair.random()
+        client_kp_c = Keypair.random()
+        timeout = 600
+        network_passphrase = Network.PUBLIC_NETWORK_PASSPHRASE
+        anchor_name = "SDF"
+
+        challenge = build_challenge_transaction(
+            server_secret=server_kp.secret,
+            client_account_id=client_kp_a.public_key,
+            anchor_name=anchor_name,
+            network_passphrase=network_passphrase,
+            timeout=timeout,
+        )
+
+        transaction = TransactionEnvelope.from_xdr(challenge, network_passphrase)
+        transaction.signatures = []
+        transaction.sign(client_kp_a)
+        transaction.sign(client_kp_b)
+        transaction.sign(client_kp_c)
+
+        challenge_tx = transaction.to_xdr()
+        signers = [
+            Ed25519PublicKeySigner(client_kp_a.public_key, 1),
+            Ed25519PublicKeySigner(client_kp_a.public_key, 3),
+            Ed25519PublicKeySigner(client_kp_a.public_key, 4),
+            Ed25519PublicKeySigner(Keypair.random().public_key, 255),
+        ]
+        with pytest.raises(
+            InvalidSep10ChallengeError,
+            match="Transaction not signed by server: {}.".format(server_kp.public_key),
+        ):
+            verify_challenge_transaction_signers(
+                challenge_tx, server_kp.public_key, network_passphrase, signers
+            )
+
     def test_verify_challenge_transaction_signers_raise_unrecognized_signatures(self):
         server_kp = Keypair.random()
         client_kp_a = Keypair.random()