bugfix: make Sep10Challenge.verifyTransactionSignatures resilient against account IDs that are not compliant with ed25519
#440
Conversation
…reshold` is called with an account id signer that's not compliant with ed25519.
marcelosalloum
requested review from
sreuland and
lijamie98
and removed request for
sreuland
Maybe my description of the PR is not precise enough... What I actually changed was the method After that, these signers will validated in the caller method, where the verification you suggested is being performed: If no signer is found or if the number of signers found is not sufficient, an exception will still be thrown.
|
marcelosalloum
changed the title
Sep10Challenge.verifyChallengeTransactionThreshold resilient against account IDs that are not compliant with ed25519Sep10Challenge. verifyTransactionSignatures resilient against account IDs that are not compliant with ed25519
marcelosalloum
changed the title
Sep10Challenge. verifyTransactionSignatures resilient against account IDs that are not compliant with ed25519Sep10Challenge.verifyTransactionSignatures resilient against account IDs that are not compliant with ed25519
| @@ -1608,6 +1608,43 @@ public void testVerifyChallengeTransactionThresholdValidServerAndMultipleClientK | |||
| assertEquals(new HashSet<String>(Arrays.asList(masterClient.getAccountId(), signerClient1.getAccountId())), signersFound); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testVerifyChallengeTransactionThresholdValidServerAndMultipleClientKeyMeetingThresholdSomeUnusedAndOneNotEd25519Compliant() throws IOException, InvalidSep10ChallengeException { | |||
There was a problem hiding this comment.
now that's a test case name, good coverage to assert the change! maybe condense the name down to just the non-compliant aspect.
| try { | ||
| keyPair = KeyPair.fromAccountId(signer); | ||
| } catch (RuntimeException e) { | ||
| System.out.printf("Couldn't parse the signer \"%s\" as a valid Public Key.", signer); |
There was a problem hiding this comment.
this console output is non-standard for the sdk which by default does not generate that type of output or logger outputs. So, to maintain consistency, would remove this and the stacktrace prints. the callers of this method could check the returned set, and determine which signers are missing, then it could deduce something was wrong and log something from the app layer, etc.
marcelosalloum
deleted the
439-fix-crash-in-sep10challenge-for-invalid-ed25519-public-key
branch
What
Make
Sep10Challenge.verifyTransactionSignaturesresilient against account IDs that are not compliant with ed25519. If a non-compliant account was provided in the list of signers, this account is simply ignored.Accounts whose ID is not a valid ed25519 public key won't have a signing key, so they can't have signed the challenge transaction.
After the
Sep10Challenge.verifyTransactionSignaturesmethod finds the intersection betweenaccount IDs that signed the transaction∩signers provided in the list, it will return to the caller and the number of accounts and their weight will be verified accordingly.Why
Close #439.