Add signature verification to xdr viewer for transaction envelopes #348

robertDurst · 2018-08-06T23:34:14Z

This may need some work making it proper React code. It also might need some touchup on design. I kept it pretty simple.

Basically, whenever the user types or pastes in an xdr of type Transaction Envelope, this will asynchronously query for all signers on that transaction (base source account and all operation source accounts). It will then, for each signature on the transaction check to see if it is valid for a given signer -- the red means invalid, green means valid, and during loading, black means not yet loaded.

Here is what it looks like for a transaction has two valid signers and one invalid signer:

Closes #343

robertDurst · 2018-08-07T00:19:22Z

Fails for hash(x)

robertDurst · 2018-08-07T21:51:56Z

Now passes for pre-auth tx and hash x signers, should be good for review @bartekn

bartekn

Sorry for a late review. Overall the PR is good. We should leverage the existing code in stellar-base more and shouldn't mix presentation and model layers. We can talk about this tomorrow.

bartekn · 2018-08-07T22:12:09Z

src/actions/xdrViewer.js

+        network.networkId(),
+        xdr.EnvelopeType.envelopeTypeTx().toXDR(),
+        txXdr
+      ]));


Everything above can be changed to:

let tx = new StellarSdk.Transaction(input); const signatureBase = tx.signatureBase();

bartekn · 2018-08-07T22:52:32Z

src/actions/xdrViewer.js

+          // We define a verification as ternary:
+          // 0 - no verification
+          // 1 - false verification
+          // 2 - true verification


These should be constants like:

const SIGNATURE_NOT_VERIFIED_YET = 0; const SIGNATURE_INVALID = 1; const SIGNATURE_VALID = 1;

bartekn · 2018-08-07T22:53:33Z

src/actions/xdrViewer.js

+      .catch(err => dispatch({
+          type: FETCHED_SIGNERS_FAIL,
+        }));
+    } catch(error) {}


This should be handled as well.

bartekn · 2018-08-07T23:01:32Z

src/actions/xdrViewer.js

+  });
+
+  return srcAccounts;
+}


We should leverage stellar-base more here. When you create Transaction object like:

let tx = new Transaction(xdrObject);

you have:

transaction source in tx.source,

each operation source in tx.operations[i].source.

bartekn · 2018-08-07T23:08:45Z

src/actions/xdrViewer.js

+    // Utilize same strategy as ExtrapolateXDR file, reference there for how this works
+    const hintBytes =  new Buffer(x._attributes.hint.toString("base64"), "base64");
+    const partialPublicKey = Buffer.concat([new Buffer(28).fill(0), hintBytes]);
+    const hint = StrKey.encodeEd25519PublicKey(partialPublicKey, 'base64').substr(47, 5);


I think we shouldn't do it. We're mixing presentation and model layers. Why not compare raw bytes? If you check this big comment in extrapolateFromXdr.js file about hints you'll see that the base58 representation of a hint is actually missing a few bits of data because of the way base58 works:

// byte 28: #### #### full b32 symbols // byte 29: # ##### ## |--------------------------| // byte 30: ### 48### | // byte 31: Signature Hint start | 49### 50# | Signature Hint end // byte 32: ## 51### 5 | | // byte 33: 2### 53##

Decided to ignore the hint. In my discussion with Tomer last week, we don't need to utilize the hint optimization for this.

bartekn · 2018-08-07T23:12:35Z

src/actions/xdrViewer.js

+          resp.data.signers.forEach(signer => {
+            // Get key hint for matching
+            const key = signer.key.substring(signer.key.length-9, signer.key.length-4);
+            if (!obj.hasOwnProperty(key)) {


What if there are two keys with the same hint? Hint is only 4 bytes so it's likely to happen.

I believe stellar core uses this for optimization -- some trade off between likely-hood of collision vs space saved.

Similar to below, decided to ignore the hint.

robertDurst · 2018-08-08T22:53:59Z

@bartekn thanks for the feedback. Utilizing Stellar-base and removing trivial hint optimization lead to much cleaner code.

Also squashed.

bartekn

OK, in this review I focused on model layer, next week will check presentation layer more.

bartekn · 2018-08-10T18:35:25Z

src/actions/xdrViewer.js

+  return dispatch => {
+    try {
+      // Capture network for determining signature base
+      StellarSdk.Network.use(new Network(networkPassphrase))


Note to self: we should fix stellar/js-stellar-base#112 :)

bartekn · 2018-08-10T18:41:42Z

src/actions/xdrViewer.js

+    if (!sourceAccounts.hasOwnProperty(key)) {
+      sourceAccounts[key] = true;
+    }
+  });


Can we exchange the function above with:

if (op.source) { sourceAccounts[op.source] = true; }

3 vs 7 lines, doing the same stuff :).

bartekn · 2018-08-10T18:55:11Z

src/actions/xdrViewer.js

+            switch (signer.type) {
+              case 'sha256_hash':
+                // Convert to string since you can't compare two buffer arrays
+                isValid = signer.value.toString() === sigObj.hashx.toString();


I think it won't work. signer.value is taken directly from Horizon API in line 65 and the key is in strkey format: https://horizon-testnet.stellar.org/accounts/GADKHRMUM76DVVW5M6FESVQ4DHYLJN42LNJXTVZDNABBPEXDA2AJWTRY. So you should use StrKey.decodeSha256Hash (or encodeSha256Hash somewhere else) function.

Also, don't use toString, check my other comment. Use Buffer.equals or compare byte by byte.

bartekn · 2018-08-10T19:06:54Z

src/actions/xdrViewer.js

+    const signature =  x._attributes.signature;
+    return {
+      // In case of hashX
+      hashx: hash(signature.toString()),


We shouldn't use toString() here. More info: nodejs/node#3982

bartekn · 2018-08-10T19:10:42Z

src/actions/xdrViewer.js

+                    keypair: Keypair.fromPublicKey(signer.key),
+                  };
+                break;
+              }


I think it will be much easier to read after changing this function to a simple one-liner:

resp.data.signers.forEach(signer => obj[signer.key] = signer);

and do all necessary conversions from one type to another in the loops below. Currently conversions happen here and there and it complicates everything.

bartekn · 2018-08-10T19:16:08Z

src/actions/xdrViewer.js

+
+            if (isValid) {
+              break;
+            }


This if is redundant.

Not sure this is invalid, at least the way the code is currently written. Here the check on isValid exits the loop once a valid key is found. Not only does this optimize (skipping a few rounds of checks once a valid key is seen) it also breaks since isValid is re-written during each pass of the internally nested loop.

You are right, my mistake.

bartekn · 2018-08-10T19:18:52Z

src/actions/xdrViewer.js

+      // In case of hashX
+      hashx: hash(signature.toString()),
+      // Used for matching against ExtrapolateXDR's signature formatting
+      value: new Buffer(signature).toString('base64'),


I think it's better to have all low level binary data as Buffer and convert when needed. Currently it's really hard to remember which values are buffers/hex/byte64.

ah yes, nicolas opened an issue about that

Fixed: as you said above, I kept signature as base64 as it is in TreeViewer then convert it back to binary and compare that signature against my fetchSigners list (buff.equals).

robertDurst · 2018-08-14T03:38:58Z

Fixed per your latest comments... you were right, hash(x) still failed under the previous commit. It should be fixed now with buffer/hex/bin sorted out a bit more.

bartekn

src/actions/xdrViewer.js looks good. Added comments to presentation layer.

bartekn · 2018-08-14T12:39:17Z

src/actions/xdrViewer.js

+      const hashedSignatureBase = hash(tx.signatureBase());
+
+      // Extract all signatures on transaction
+      const signatures = tx.signatures.map(x => ({ sig: x._attributes.signature }));


I missed that one. js-xdr lacks good documentation but I think it has autogenerated getter functions so instead x._attributes.signature we should use x.signature() or x.get("signature") whichever works.

It is x.signature()... much nicer 😄

bartekn · 2018-08-14T12:58:03Z

src/reducers/xdrViewer.js

@@ -37,3 +37,13 @@ function type(state = 'TransactionEnvelope', action) {

  return state;
 }
+
+function fetchedSigners(state = "", action) {


Shouldn't default state be []?

changed default to null -- I want a way to make sure this is run at least once, say if you were to copy-paste a link with xdr I want this to trigger

bartekn · 2018-08-14T13:32:22Z

src/components/TreeView.js

    let rootClass = 'TreeView ' + (className) ? className : '';

+    // First check in case of transaction with no signatures, i.e. pre-auth
+    if(nodes[0] && nodes[0].type == "hint" && fetchedSigners) {


I think this condition is a little complicated. fetchedSigners is present if the type is a TransactionEnvelope (checked in state.type === "TransactionEnvelope" condition). So we really want to check if we are in TransactionEnvelope view but we're doing it by checking the other variable set somewhere else.

Also I believe that in a very near future we'll want to fix #336 and it will be much easier to do if we know the exact location we're currently at in a tree.

So I propose to construct a location string. We always have key name in nodes[i].type. So we can construct a location string like TransactionEnvelope.signatures[i] by appending the current key name in each child and trigger an action when we are when we want to be. For location with a variable part ( arrays like TransactionEnvelope.signatures[0], TransactionEnvelope.signatures[1]...) we can use RegExp.

Not quite following here -- can discuss tomorrow AM (UTC -7).

I see the key name, but where does the location string live? Is it attached to the node? I know that the TreeView has some recursion, so does this key persist through iterations?

We need to build a location ourselves. TreeView renders itself recursively so if we send a current (then parent) name to the child we can construct the full location string. Check the quick patch below:

diff --git a/src/components/TreeView.js b/src/components/TreeView.js index e7ed97b..8a0e4d4 100644 --- a/src/components/TreeView.js +++ b/src/components/TreeView.js @@ -9,7 +9,7 @@ import SIGNATURE from '../constants/signature'; // @param {array} props.nodes - Array of TreeView compatible nodes export default class TreeView extends React.Component { render() { - let {nodes, className, fetchedSigners} = this.props; + let {nodes, className, fetchedSigners, parent} = this.props; let rootClass = 'TreeView ' + (className) ? className : ''; if(nodes[1] && nodes[1].type == "signatures") { @@ -20,15 +20,22 @@ export default class TreeView extends React.Component { {_.map(Array.prototype.slice.call(nodes), (node, index) => { let childNodes; + let sep = '.'; + if (!parent || node.type.charAt(0) == '[') { + sep = ''; + } + + let position = (parent ? parent+sep : '') + node.type; + if (typeof node.nodes !== 'undefined') { childNodes = <div className="TreeView__child"> - <TreeView nodes={node.nodes} fetchedSigners={fetchedSigners}/> + <TreeView nodes={node.nodes} fetchedSigners={fetchedSigners} parent={position} /> </div>; } return <div className="TreeView__set" key={index}> <div className="TreeView__row" key={index + node.type}> - <RowValue node={node} /> + <RowValue node={node} position={position} /> </div> {childNodes} </div> @@ -67,7 +74,7 @@ function checkSignatures(nodes, fetchedSigners) { function RowValue(props) { let value, childNodes, separatorNeeded, separator; - let {node} = props; + let {node, position} = props; if (typeof node.value === 'string') { value = String(node.value); @@ -95,7 +102,7 @@ function RowValue(props) { return formatSignature(node, separator); } - return <span><strong>{node.type}</strong>{separator}{value}</span> + return <span><strong>{node.type} {position}</strong>{separator}{value}</span> } // Types values are values that will be displayed with special formatting to

Then instead of:

if(nodes[1] && nodes[1].type == "signatures")

We can check if we are in the exact location in a tree:

if (position == 'TransactionEnvelope.signatures')

bartekn · 2018-08-14T13:47:51Z

src/components/TreeView.js

+  let isValid = SIGNATURE.INVALID;
+  if (sig) {
+    isValid = sig.isValid;
+  }


isValid defaults to SIGNATURE.INVALID so in case of errors (ex. connection error) the signature will be displayed as invalid (you can go to Chrome Dev tools, switch to Offline mode and load a TransactionEnvelope to reproduce it).

What do you think something like this:

When checking the signatures (loading signers etc) we add "Checking signatures..." to TransactionEnvelope.signatures node:

If there was an error we display it at the TransactionEnvelope.signatures level:

If checking was successful we display the statuses (like we do now):

Looks good! So basically a sort of visual of the state of fetched signatures:

loading

failed

success (same as now)

bartekn · 2018-08-14T13:50:43Z

src/components/TreeView.js

@@ -34,6 +40,18 @@ export default class TreeView extends React.Component {
  }
 }

+function checkSignature(nodes, fetchedSigners) {
+  const signatureBuffer = Buffer.from(nodes[1].value.value, 'base64');
+  const sig = fetchedSigners.filter(x => x.sig.equals(signatureBuffer))[0];


Better to use find instead of filter here.

bartekn · 2018-08-14T13:55:28Z

src/components/XdrViewer.js

@@ -39,13 +44,18 @@ function XdrViewer(props) {
          <p>The XDR Viewer is a tool that displays contents of a Stellar XDR blob in a human readable format.</p>
        </div>
        <p className="XdrViewer__label">
-          Input a base-64 encoded XDR blob, or <a onClick={() => dispatch(fetchLatestTx(baseURL))}>fetch the latest transaction to try it out</a>:
+        Input a base-64 encoded XDR blob, or <a onClick={() => dispatch(fetchLatestTx(baseURL))}>fetch the latest transaction to try it out</a>:


If you click this link it doesn't trigger signers check.

robertDurst · 2018-08-15T20:37:46Z

Fixed for presentation level feedback. However, left a reply on a comment where I was a bit confused.

bartekn · 2018-08-23T17:15:17Z

src/components/TreeView.js

+function checkSignatures(nodes, fetchedSigners) {
+  // catch fetch signature errors
+  if (fetchedSigners === null) {
+    nodes[1].state = SIGNATURE.NOT_VERIFIED_YET;


I think something is wrong here. I set connection latency to 10s in devtools. I expected to see "Checking signatures..." but instead I got "Error checking signatures..." and after 10s it disappeared and updated each signature state.

Ah I see. The error is that I have an initial state of null with only two states, fail and success. However, there is in fact also a third state (different from initial state) for loading.

bartekn · 2018-08-23T17:16:40Z

src/components/TreeView.js

+        let position = (parent ? parent + sep : '') + node.type;
+
+        if (position == 'TransactionEnvelope.signatures') {
+          checkSignatures(nodes, fetchedSigners);


Use node here so that you don't need to use nodes[1] inside checkSignatures..

bartekn · 2018-08-23T17:18:32Z

src/components/TreeView.js

@@ -56,6 +96,14 @@ function RowValue(props) {
    separator = ': ';
  }

+  if (node.type === 'signatures') {


Use position prop here.

bartekn · 2018-08-23T17:19:27Z

src/components/TreeView.js

+    return formatSignatureCheckState(node, separator)
+  }
+
+  if (node.type === "signature") {


Use position prop here too but use regexp to check if you are in TransactionEnvelope.signatures[x].signature.

bartekn

This is the final round of comments. Will merge right away after these changes. Good job 👍 !

bartekn · 2018-08-24T12:29:31Z

src/components/TreeView.js

+          sep = '';
+        }
+
+        let position = (parent ? parent + sep : '') + node.type;


Let's move it to a separate function.

bartekn · 2018-08-24T12:32:58Z

src/reducers/xdrViewer.js

@@ -37,3 +37,15 @@ function type(state = 'TransactionEnvelope', action) {

  return state;
 }
+
+function fetchedSigners(state = "uninitialized", action) {


We can use undefined here (or even remove value as it will be undefined by default?). Or maybe add:

export const FETCHED_SIGNERS_PENDING = 'FETCHED_SIGNERS_PENDING'; // or a better name?

Originally that was undefined, specifically an empty string, however in js land "" == [] which failed for this:

// Fetch signers on initial load if (state.type === "TransactionEnvelope" && state.fetchedSigners === 'uninitialized') { dispatch(fetchSigners(state.input, baseURL, networkPassphrase)) }

since initial state == case with no signers ([])

Also, default state cannot be undefined:

combineReducers.js:56 Uncaught Error: Reducer "fetchedSigners" returned undefined during initialization. If the state passed to the reducer is undefined, you must explicitly return the initial state. The initial state may not be undefined. If you don't want to set a value for this reducer, you can use null instead of undefined.

Decided to go with:

INITIAL --> null
SUCCESS --> result (type array)
FAILURE --> "ERROR"
PENDING --> "PENDING"

bartekn · 2018-08-24T12:38:44Z

src/components/TreeView.js

+  } else if (node.value.isValid === SIGNATURE.VALID) {
+    style = {color: "green"};
+  }
+  return <span style={style}><strong>{node.type}</strong>{separator}{node.value.value}</span>


Let's add add a small icon here too, like ❌ / ✅ (can be unicode).

bartekn · 2018-08-24T12:40:10Z

src/constants/signature.js

@@ -0,0 +1,9 @@
+// This standardizes all the slug strings into a simple map
+// These are used in to build the url and are the "page name"


This comment seems wrong.

bartekn · 2018-08-24T12:48:12Z

src/components/TreeView.js

+    return formatSignatureCheckState(node, separator)
+  }
+
+  if (position.match(/(\.signature)+$/)) {


We really need a full regexp here. No idea how TransactionEnvelope will looks like in a future and it will break if there's another signature field.

bartekn · 2018-08-27T14:04:07Z

src/actions/xdrViewer.js

+      })
+      .catch(err => dispatch({ type: FETCHED_SIGNERS_FAIL }));
+    } catch(error) {
+      dispatch({ type: FETCHED_SIGNERS_FAIL });


Please add console.error in both catch functions.

bartekn · 2018-08-27T14:09:52Z

src/components/TreeView.js

  return <span><strong>{node.type}</strong>{separator}{value}</span>
 }

+function checkSignatures({signatures, fetchedSigners}) {


Any reason for destructuring here and other methods? I think it's just copied from convertTypedValue({type, value}) definition where it's helpful because you can pass a node value there: convertTypedValue(node.value).

bartekn · 2018-08-27T14:15:18Z

src/components/TreeView.js

+  signatures.state = SIGNATURE.VALID;
+
+  for (var i = 0; i < signatures.nodes.length; i ++) {
+    const sig = signatures.nodes[i].nodes[1];


This can be confusing if you don't remember that signatures.nodes are decorated signatures. Let's change it to:

const sig = signatures.nodes[i].find(n => n.type == 'signature');

bartekn · 2018-08-27T14:39:30Z

src/components/TreeView.js

+
+  for (var i = 0; i < signatures.nodes.length; i ++) {
+    const sig = signatures.nodes[i].nodes[1];
+    const signatureBuffer = Buffer.from(sig.value.value, 'base64');


As a part of a separate PR we should probably move all the extra presentation functions here from extrapolateFromXdr. Right now we should probably leave the original value in a resulting object like:

if (object && object._isBuffer) { return {type: 'code', raw: object, value: new Buffer(object).toString('base64')}; }

Without it we need to convert it back to bytes from base64 representation. Then we can do:

const fetchedSignature = fetchedSigners.find(x => x.sig.equals(sig.raw));

robertDurst · 2018-08-27T17:31:44Z

@bartekn ok updated for latest comments. Also squashed.

robertDurst requested a review from bartekn August 6, 2018 23:34

robertDurst changed the title ~~Add signature verification to xdr viewer for transaction envelopes~~ [WIP] Add signature verification to xdr viewer for transaction envelopes Aug 7, 2018

robertDurst changed the title ~~[WIP] Add signature verification to xdr viewer for transaction envelopes~~ Add signature verification to xdr viewer for transaction envelopes Aug 7, 2018

bartekn reviewed Aug 7, 2018

View reviewed changes

bartekn suggested changes Aug 10, 2018

View reviewed changes

bartekn suggested changes Aug 14, 2018

View reviewed changes

bartekn reviewed Aug 23, 2018

View reviewed changes

bartekn reviewed Aug 24, 2018

View reviewed changes

bartekn reviewed Aug 27, 2018

View reviewed changes

Add signature verification to xdr viewer for transaction envelopes

b7ab1cf

bartekn approved these changes Aug 27, 2018

View reviewed changes

bartekn merged commit eaff872 into stellar:master Aug 27, 2018

		@@ -0,0 +1,9 @@
		// This standardizes all the slug strings into a simple map
		// These are used in to build the url and are the "page name"

Add signature verification to xdr viewer for transaction envelopes #348

Add signature verification to xdr viewer for transaction envelopes #348

Conversation

robertDurst commented Aug 6, 2018

robertDurst commented Aug 7, 2018

robertDurst commented Aug 7, 2018

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented Aug 8, 2018

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

robertDurst commented Aug 14, 2018

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented Aug 27, 2018

robertDurst Aug 14, 2018 •

edited

Loading

bartekn left a comment •

edited

Loading