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.

Sign up for GitHub

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

refactor(p2p): JSON encoding to use protobuf instead #2061

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

refactor(p2p): JSON encoding to use protobuf instead #2061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a979e9
Protocol's JSON encoding to use protobuf instead
Jan 11, 2021
c7cb339
npm run lint
Jan 11, 2021
725f380
move json-stable-stringify from dependencies to devDependencies
Jan 11, 2021
6d167d9
move `checksum` to a util function
Jan 14, 2021
04c0452
npm run lint
Jan 14, 2021
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
3 changes: 2 additions & 1 deletion lib/p2p/Framer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { randomBytes } from '../utils/cryptoUtils';
import errors from './errors';
import Network from './Network';
import Packet from './packets/Packet';
import { calcChecksum } from './packets/utils';

type WireMsgHeader = {
magic?: number;
Expand Down Expand Up @@ -69,7 +70,7 @@ class Framer {
msg.writeUInt32LE(packet.type, 8);

// checksum
msg.writeUInt32LE(packet.checksum(), 12);
msg.writeUInt32LE(calcChecksum(packetRaw), 12);

// payload
packetRaw.copy(msg, 16);
Expand Down
3 changes: 2 additions & 1 deletion lib/p2p/Parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import Framer, { WireMsgHeader } from './Framer';
import { PacketType } from './packets';
import Packet, { isPacket } from './packets/Packet';
import * as packetTypes from './packets/types';
import { calcChecksum } from './packets/utils';

interface Parser {
on(event: 'packet', packet: (order: Packet) => void): this;
Expand Down Expand Up @@ -181,7 +182,7 @@ class Parser extends EventEmitter {
}

const packet = packetOrPbObj;
if (header.checksum && header.checksum !== packet.checksum()) {
if (header.checksum && header.checksum !== calcChecksum(payload)) {
throw errors.PARSER_DATA_INTEGRITY_ERR(`${PacketType[header.type]} ${JSON.stringify(packet)}`);
}

Expand Down
11 changes: 5 additions & 6 deletions lib/p2p/Peer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import assert from 'assert';
import { createECDH, createHash } from 'crypto';
import { EventEmitter } from 'events';
import stringify from 'json-stable-stringify';
import net, { Socket } from 'net';
import secp256k1 from 'secp256k1';
import { SocksClient, SocksClientOptions } from 'socks';
Expand Down Expand Up @@ -870,7 +869,7 @@ class Peer extends EventEmitter {
expectedNodePubKey?: string,
) => {
const body = packet.body!;
const { sign, ...bodyWithoutSign } = body;
const { sign, ...innerBody } = body;
/** The pub key of the node that sent the init packet. */
const sourceNodePubKey = body.nodePubKey;
/** The pub key of the node that the init packet is intended for. */
Expand All @@ -890,7 +889,7 @@ class Peer extends EventEmitter {
}

// verify that the msg was signed by the peer
const msg = stringify(bodyWithoutSign);
const msg = packets.SessionInitPacket.serializeInnerBody(innerBody);
const msgHash = createHash('sha256').update(msg).digest();
const verified = secp256k1.verify(msgHash, Buffer.from(sign, 'hex'), Buffer.from(sourceNodePubKey, 'hex'));

Expand Down Expand Up @@ -1041,19 +1040,19 @@ class Peer extends EventEmitter {
ownVersion: string;
expectedNodePubKey: string;
}): packets.SessionInitPacket => {
let body: any = {
const innerBody: any = {
ephemeralPubKey,
version: ownVersion,
peerPubKey: expectedNodePubKey,
nodePubKey: ownNodeKey.pubKey,
nodeState: ownNodeState,
};

const msg = stringify(body);
const msg = packets.SessionInitPacket.serializeInnerBody(innerBody);
const msgHash = createHash('sha256').update(msg).digest();
const { signature } = secp256k1.sign(msgHash, ownNodeKey.privKey);

body = { ...body, sign: signature.toString('hex') };
const body = { ...innerBody, sign: signature.toString('hex') };

return new packets.SessionInitPacket(body);
};
Expand Down
13 changes: 0 additions & 13 deletions lib/p2p/packets/Packet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { createHash } from 'crypto';
import uuidv1 from 'uuid/v1';
import stringify from 'json-stable-stringify';
import PacketType from './PacketType';

type PacketHeader = {
Expand Down Expand Up @@ -91,24 +89,13 @@ abstract class Packet<T = any> implements PacketInterface {

public abstract serialize(): Uint8Array;

public toJSON = () => {
return stringify({ header: this.header, body: this.body });
};

/**
* Serialize this packet to binary Buffer.
* @returns Buffer representation of the packet
*/
public toRaw = (): Buffer => {
return Buffer.from(this.serialize().buffer as ArrayBuffer);
};

/**
* Calculating the packet checksum using its JSON representation hash first 4 bytes.
*/
public checksum = (): number => {
return createHash('sha256').update(this.toJSON()).digest().readUInt32LE(0);
};
}

function isPacket(val: any): val is Packet {
Expand Down
18 changes: 16 additions & 2 deletions lib/p2p/packets/types/SessionInitPacket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ import { NodeState } from '../../types';
import * as pb from '../../../proto/xudp2p_pb';
import { validateNodeState, convertNodeState, serializeNodeState } from '../utils';

export type SessionInitPacketBody = {
sign: string;
export type InnerBody = {
ephemeralPubKey: string;
/** The node pub key of the peer we are connecting to. */
peerPubKey: string;
Expand All @@ -17,6 +16,10 @@ export type SessionInitPacketBody = {
nodePubKey: string;
};

export type SessionInitPacketBody = InnerBody & {
sign: string;
};

class SessionInitPacket extends Packet<SessionInitPacketBody> {
public get type(): PacketType {
return PacketType.SessionInit;
Expand Down Expand Up @@ -61,6 +64,17 @@ class SessionInitPacket extends Packet<SessionInitPacketBody> {
});
};

public static serializeInnerBody = (body: InnerBody): Uint8Array => {
const msg = new pb.SessionInitPacket();
msg.setPeerPubKey(body.peerPubKey);
msg.setEphemeralPubKey(body.ephemeralPubKey);
msg.setVersion(body.version);
msg.setNodePubKey(body.nodePubKey);
msg.setNodeState(serializeNodeState(body.nodeState));

return msg.serializeBinary();
};

public serialize = (): Uint8Array => {
const msg = new pb.SessionInitPacket();
msg.setId(this.header.id);
Expand Down
8 changes: 8 additions & 0 deletions lib/p2p/packets/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { createHash } from 'crypto';
import * as pb from '../../proto/xudp2p_pb';
import { removeUndefinedProps, convertKvpArrayToKvps, setObjectToMap } from '../../utils/utils';
import { NodeState } from '../types';
Expand Down Expand Up @@ -67,3 +68,10 @@ export const serializeNodeState = (nodeState: NodeState): pb.NodeState => {
}
return pbNodeState;
};

/**
* Calculate checksum using the bytes sha256-hash first 4 bytes.
*/
export const calcChecksum = (bytes: Uint8Array): number => {
return createHash('sha256').update(bytes).digest().readUInt32LE(0);
};
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@
"fastpriorityqueue": "^0.6.3",
"google-protobuf": "^3.14.0",
"gulp": "^4.0.2",
"json-stable-stringify": "^1.0.1",
"keccak": "^2.1.0",
"moment": "^2.29.1",
"node-forge": "^0.10.0",
Expand Down Expand Up @@ -215,6 +214,7 @@
"grpc-tools": "^1.10.0",
"grpc_tools_node_protoc_ts": "^5.1.0",
"jest": "^26.6.3",
"json-stable-stringify": "^1.0.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is only being used for the Parser.spec.ts suite, but it doesn't make much sense to me to use it for tests when it's not used in the actual functioning of xud. I think we can just change the line that expects the stringify outputs to match to instead check that the packet checksums match, and then get rid of this package entirely, right? If you'd like though I could do this in a follow-up PR instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of this as well, but there’s a small problem - protobuf scheme (in which checksum now relies on) is fixed (unlike JSON), and so it will only compare between the define fields, while in some of the negative test cases these are just a subset, because an additional (undefined in protobuf) field is added (e.g., reqId).
Also, there’s a small benefit in comparing between the constructed POJO, and not the binary representation, which is an intermediate stage. I tried to use the built-inJSON.stringify, but the output isn’t deterministic and so many tests fail.

So the usage of stringify here isn’t related to its previous usage in xud. It’s just a tool for comparing between nested objects reliably.
Let me know what you agree. If not, I can just change some of the negative tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation. That makes sense and I think it's OK to leave it for now, it's not a big deal to have as a dev dependency. I might take another look at this test suite some time to see if there's a sensible way to run it without this stringify function but no need to hold this PR up for that.

"mocha": "^8.2.1",
"nodemon": "^2.0.6",
"prettier": "2.2.1",
Expand Down