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.

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

feat: all unverified inbound sessions #180

Merged
merged 9 commits into from
May 25, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"lint": "eslint --color --ext .ts src/",
"test": "yarn test:unit && yarn test:e2e",
"test:unit": "mocha 'test/unit/**/*.test.ts'",
"test:e2e": "mocha 'test/unit/**/*.test.ts'"
"test:e2e": "mocha 'test/e2e/**/*.test.ts'"
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

},
"pre-push": [
"lint"
Expand Down
1 change: 1 addition & 0 deletions src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,5 @@ export const defaultConfig: IDiscv5Config = {
lookupTimeout: 60 * 1000,
pingInterval: 300 * 1000,
enrUpdate: true,
allowUnverifiedSessions: false,
};
9 changes: 4 additions & 5 deletions src/service/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,10 +386,9 @@ export class Discv5 extends (EventEmitter as { new (): Discv5EventEmitter }) {
/**
* Send TALKRESP message to requesting node
*/
public async sendTalkResp(remote: ENR | Multiaddr, requestId: RequestId, payload: Uint8Array): Promise<void> {
public async sendTalkResp(remote: INodeAddress, requestId: RequestId, payload: Uint8Array): Promise<void> {
const msg = createTalkResponseMessage(requestId, payload);
const nodeAddr = getNodeAddress(createNodeContact(remote));
this.sendRpcResponse(nodeAddr, msg);
this.sendRpcResponse(remote, msg);
}

/**
Expand All @@ -402,8 +401,8 @@ export class Discv5 extends (EventEmitter as { new (): Discv5EventEmitter }) {
/**
* Sends a PING request to a node
*/
private sendPing(enr: ENR): void {
this.sendRpcRequest({ contact: createNodeContact(enr), request: createPingMessage(this.enr.seq) });
public sendPing(nodeAddr: ENR | Multiaddr): void {
this.sendRpcRequest({ contact: createNodeContact(nodeAddr), request: createPingMessage(this.enr.seq) });
}

/**
Expand Down
63 changes: 38 additions & 25 deletions src/session/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -460,13 +460,10 @@ export class SessionService extends (EventEmitter as { new (): StrictEventEmitte
}

/**
* Verifies a Node ENR to its observed address.
* If it fails, any associated session is also considered failed.
* If it succeeds, we notify the application
* Compares the ENR multiaddr to its observed address.
* Returns true if they match
*/
private verifyEnr(enr: ENR, nodeAddr: INodeAddress): boolean {
// If the ENR does not match the observed IP addresses,
// we consider the session failed.
const enrMultiaddr = enr.getLocationMultiaddr("udp");
return enr.nodeId === nodeAddr.nodeId && (enrMultiaddr?.equals(nodeAddr.socketAddr) ?? true);
}
Expand Down Expand Up @@ -510,28 +507,35 @@ export class SessionService extends (EventEmitter as { new (): StrictEventEmitte
);

// Receiving an AuthResponse must give us an up-to-date view of the node ENR.
// Verify the ENR is valid
if (this.verifyEnr(enr, nodeAddr)) {
// Session is valid
// Notify the application
// Verify the ENR endpoint matches observed node address
const verified = this.verifyEnr(enr, nodeAddr);

// Drop session if invalid ENR and session service not configured to allow unverified sessions
if (!verified && !this.config.allowUnverifiedSessions) {
log("ENR contains invalid socket address. Dropping session with %o", nodeAddr);
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be a this.failSession(nodeAddr, RequestErrorType.InvalidRemoteENR, true) after this log and the similar log below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

return;
}

if (verified) {
// If ENR is valid, notify application in order to add to routing table
// The session established here are from WHOAREYOU packets that we sent.
// This occurs when a node established a connection with us.
this.emit("established", enr, ConnectionDirection.Incoming);

this.newSession(nodeAddr, session);

// decrypt the message
this.handleMessage(src, {
maskingIv: packet.maskingIv,
header: createHeader(
PacketType.Message,
encodeMessageAuthdata({ srcId: nodeAddr.nodeId }),
packet.header.nonce
),
message: packet.message,
messageAd: encodeChallengeData(packet.maskingIv, packet.header),
});
}

this.newSession(nodeAddr, session);

// decrypt the message
this.handleMessage(src, {
maskingIv: packet.maskingIv,
header: createHeader(
PacketType.Message,
encodeMessageAuthdata({ srcId: nodeAddr.nodeId }),
packet.header.nonce
),
message: packet.message,
messageAd: encodeChallengeData(packet.maskingIv, packet.header),
});
} catch (e) {
if ((e as Error).name === ERR_INVALID_SIG) {
log("Authentication header contained invalid signature. Ignoring packet from: %o", nodeAddr);
Expand Down Expand Up @@ -619,15 +623,24 @@ export class SessionService extends (EventEmitter as { new (): StrictEventEmitte
if (message.type === MessageType.NODES) {
// Received the requested ENR
const enr = message.enrs.pop();

if (enr) {
if (this.verifyEnr(enr, nodeAddr)) {
// Verify the ENR endpoint matches observed node address
const verified = this.verifyEnr(enr, nodeAddr);

// Drop session if invalid ENR and session service not configured to allow unverified sessions
if (!verified && !this.config.allowUnverifiedSessions) {
log("ENR contains invalid socket address. Dropping session with %o", nodeAddr);
return;
}
if (verified) {
// Notify the application
// This can occur when we try to dial a node without an
// ENR. In this case we have attempted to establish the
// connection, so this is an outgoing connection.
this.emit("established", enr, ConnectionDirection.Outgoing);
return;
}
return;
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/session/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ export interface ISessionConfig {
* The maximum number of established sessions to maintain
*/
sessionCacheCapacity: number;
/**
* Allow sessions with unverified ENRs (i.e. either have no UDP endpoint specified or else reported
* UDP endpoint does not match observed socket address)
*/
allowUnverifiedSessions: boolean;
}

export enum RequestErrorType {
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/connect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,17 @@ describe("discv5 integration test", function () {

// test a TALKRESP with no response
try {
await node0.discv5.sendTalkReq(node1.enr.nodeId, Buffer.from([0, 1, 2, 3]), "foo");
await node0.discv5.sendTalkReq(node1.enr, Buffer.from([0, 1, 2, 3]), "foo");
expect.fail("TALKREQ response should throw when no response is given");
} catch (e) {
}

// test a TALKRESP with a response
const expectedResp = Buffer.from([4, 5, 6, 7]);
node1.discv5.on("talkReqReceived", (nodeAddr, enr, request) => {
node1.discv5.sendTalkResp(nodeAddr.nodeId, request.id, expectedResp);
node1.discv5.sendTalkResp(nodeAddr, request.id, expectedResp);
});
const resp = await node0.discv5.sendTalkReq(node1.enr.nodeId, Buffer.from([0, 1, 2, 3]), "foo");
const resp = await node0.discv5.sendTalkReq(node1.enr, Buffer.from([0, 1, 2, 3]), "foo");
expect(resp).to.deep.equal(expectedResp);

});
Expand Down