feat(core-p2p): Validate GET replies from other peers #2102

vasild · 2019-02-14T17:46:05Z

Proposed changes

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring (improve a current implementation without adding a new feature or fixing a bug)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Build (changes that affect the build system)
Docs (documentation only changes)
Test (adding missing tests or fixing existing tests)
Other... Please describe:

Checklist

I have read the CONTRIBUTING documentation
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

ghost · 2019-02-14T17:46:09Z

@air1one @faustbrian @supaiku0 - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

ghost · 2019-02-14T17:49:30Z

@vasild The ci/circleci: test-node11-2 job is failing as of 94761b29e288c4c1a6e4dfed652219d7cf64516b. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

faustbrian · 2019-02-15T04:18:18Z

This is a breaking change in how the legacy P2P API responds, some people still rely on it so that is a no go. This is a change that will be made for websockets.

vasild · 2019-02-15T09:21:05Z

This is a breaking change in how the legacy P2P API responds, some people still rely on it so that is a no go. This is a change that will be made for websockets.

We agreed that I add a schema validation as a followup PR after #2032:
#2032 (comment)
#2032 (comment)

Now, should I relax the validation so that it allows "legacy P2P API" replies, or completely ditch this PR
@supaiku0, @kristjank?

vasild · 2019-02-15T10:10:40Z

By the way I get the following with this patch:

[ERROR]: Got unexpected reply from http://137.74.237.196:4002/peer/list: {"peers":[{...},{"ip":"2130706433",...}: child "peers" fails because ["peers" at position 1 fails because [child "ip" fails because ["ip" must be a valid ip address with a forbidden CIDR]]]

and the node keeps working by fetching the peer list from another peer. This looks like a good way to get rid of the IP addresses like "2130706433" with which devnet is poisoned.

faustbrian · 2019-02-16T14:08:24Z

If you change it so that the responses don't change we can keep it open, otherwise I would wait until 2.6 with it and talk to @air1one as he currently is working on the websockets.

vasild · 2019-02-18T08:47:21Z

If you change it so that the responses don't change we can keep it open, otherwise I would wait until 2.6 with it and talk to @air1one as he currently is working on the websockets.

Yes, sure. The idea was never to change the responses. The idea is to check that the responses are the expected ones. So, I guess, the rules should be relaxed. @air1one how would this fit with websockets? I guess with websockets we still send JSONs back and forth, right? So the validation is still relevant, no matter whether we received the reply via HTTP or websockets.

faustbrian · 2019-02-20T15:01:40Z

@vasild try to finish this until the end of the week so we can put it on devnet and freeze the develop branch for the 2.2 release.

…ply-validation * ArkEcosystem/core/develop: chore(release): 2.2.0-beta.5 (#2132) refactor(core-p2p): reduce logging noise (#2129)

codecov-io · 2019-02-22T10:52:05Z

Codecov Report

Merging #2102 into develop will decrease coverage by 0.06%.
The diff coverage is 90.62%.

@@             Coverage Diff             @@
##           develop    #2102      +/-   ##
===========================================
- Coverage    79.55%   79.48%   -0.07%     
===========================================
  Files          329      330       +1     
  Lines         7900     7913      +13     
  Branches      1123     1114       -9     
===========================================
+ Hits          6285     6290       +5     
- Misses        1582     1590       +8     
  Partials        33       33

Impacted Files	Coverage Δ
packages/core-p2p/src/peer-verifier.ts	`85.61% <100%> (-1.35%)`	⬇️
packages/core-p2p/src/reply-schemas.ts	`100% <100%> (ø)`
packages/core-p2p/src/peer.ts	`85% <89.65%> (+0.78%)`	⬆️
packages/core-p2p/src/errors.ts	`36.36% <0%> (-45.46%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7be0cf...b4f1d2e. Read the comment docs.

spkjp · 2019-02-22T13:43:46Z

packages/core-p2p/src/peer.ts

 import util from "util";
 import { config as localConfig } from "./config";
 import { PeerPingTimeoutError, PeerStatusResponseError, PeerVerificationFailedError } from "./errors";
 import { PeerVerificationResult, PeerVerifier } from "./peer-verifier";

 export class Peer implements P2P.IPeer {
+    private static replySchemas: any = {


Please move the schemas into a separate file.

Moved in abb0242.

…ply-validation * ArkEcosystem/core/develop: chore(release): 2.2.0-beta.6 (#2136) refactor(core-container): throw an error if the peers or plugins file are missing (#2135)

…ply-validation * ArkEcosystem/core/develop: chore(release): 2.2.0-beta.7 (#2141)

* ArkEcosystem/core/2.3: chore: move core-graphql to the deprecated folder (#2169) refactor(crypto): benchmarks (#2167) refactor: replace micromatch with nanomatch and remove heavy deps (#2165) feat(crypto): increase vendor field length to 255 bytes (#2159) feat(core-api): search delegates by usernames (#2143) feat(core-logger-pino): initial implementation (#2134) perf(crypto): integrate bcrypto (#2158) feat(core): ask for process restarts after updating (#2155) refactor(core): replace pm2 with process manager (#2154) refactor(core): require the user to take action for updates (#2153) feat(core-p2p): Fetch list of peers from at least a few others (#2152) refactor(core): more robust check for ensureDefaults (#2151) fix(core): ensure file and defaults before reading fix(core): return correct suffix for core:restart command (#2150) fix(core-database): properly sort BigNumber values (#2144) feat(core): configuration and channel support for the CLI (#2145) feat(core): merge core-snapshot-cli commands into core (#2149) fix(core-api): pass query to findAllByVote method (#2142) feat(core-p2p): Validate GET replies from other peers (#2102) chore(release): 2.2.0-beta.7 (#2141) fix(core-blockchain): stuck at not ready to accept new block (#2139) refactor(core-p2p): Improve selection of peer for downloading blocks (#2137) fix(core): overwrite the config path if an env variable is provided (#2140) fix(core): do not ignore the network flag in parseWithNetwork (#2138) chore(release): 2.2.0-beta.6 (#2136) refactor(core-container): throw an error if the peers or plugins file are missing (#2135) chore(release): 2.2.0-beta.5 (#2132) refactor(core-p2p): reduce logging noise (#2129) misc(core-p2p): remove superfluous log message (#2128) refactor(core-p2p): Improve fork handling in updatePeersOnMissingBlocks (#2125)

feat(core-p2p): Validate GET replies from other peers

eb1a98d

vasild requested review from faustbrian, kristjank and spkjp as code owners February 14, 2019 17:46

ghost requested a review from air1one February 14, 2019 17:46

ghost added Status: Needs Review labels Feb 14, 2019

ghost mentioned this pull request Feb 18, 2019

[Weekly Digest] Feb 11, 2019 - Feb 17, 2019 #2109

Closed

Merge branch 'develop' into peer-reply-validation

078a34f

faustbrian and others added 5 commits February 21, 2019 20:07

Merge branch 'develop' into peer-reply-validation

7a0de8d

Merge branch 'develop' into peer-reply-validation

305f4e5

Merge remote-tracking branch 'ArkEcosystem/core/develop' into peer-re…

94761b2

…ply-validation * ArkEcosystem/core/develop: chore(release): 2.2.0-beta.5 (#2132) refactor(core-p2p): reduce logging noise (#2129)

chore(core-p2p): Remove call to a nonexistent function

e1bfb8a

fix(core-p2p): adjust tests to mock proper responses

da6b63e

vasild added this to the 2.2.0 milestone Feb 22, 2019

chore(core-p2p): allow "common: null" in reply for common blocks

7790151

spkjp reviewed Feb 22, 2019

View reviewed changes

vasild and others added 3 commits February 22, 2019 15:27

Merge remote-tracking branch 'ArkEcosystem/core/develop' into peer-re…

e423f88

…ply-validation * ArkEcosystem/core/develop: chore(release): 2.2.0-beta.6 (#2136) refactor(core-container): throw an error if the peers or plugins file are missing (#2135)

chore(core-p2p): Move replySchemas to a separate file

abb0242

Merge branch 'develop' into peer-reply-validation

0936cc1

spkjp self-requested a review February 22, 2019 16:33

spkjp approved these changes Feb 22, 2019

View reviewed changes

Merge remote-tracking branch 'ArkEcosystem/core/develop' into peer-re…

b4f1d2e

…ply-validation * ArkEcosystem/core/develop: chore(release): 2.2.0-beta.7 (#2141)

vasild merged commit bbca40c into develop Feb 22, 2019

ghost deleted the peer-reply-validation branch February 22, 2019 16:54

ghost removed the Status: Needs Review label Feb 22, 2019

ghost mentioned this pull request Feb 24, 2019

[Weekly Digest] Feb 18, 2019 - Feb 24, 2019 #2148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core-p2p): Validate GET replies from other peers #2102

feat(core-p2p): Validate GET replies from other peers #2102

vasild commented Feb 14, 2019 •

edited

Loading

ghost commented Feb 14, 2019

ghost commented Feb 14, 2019 •

edited by ghost

Loading

faustbrian commented Feb 15, 2019

vasild commented Feb 15, 2019

vasild commented Feb 15, 2019 •

edited

Loading

faustbrian commented Feb 16, 2019

vasild commented Feb 18, 2019

faustbrian commented Feb 20, 2019

codecov-io commented Feb 22, 2019 •

edited

Loading

spkjp Feb 22, 2019

vasild Feb 22, 2019

feat(core-p2p): Validate GET replies from other peers #2102

feat(core-p2p): Validate GET replies from other peers #2102

Conversation

vasild commented Feb 14, 2019 • edited Loading

Proposed changes

Types of changes

Checklist

ghost commented Feb 14, 2019

ghost commented Feb 14, 2019 • edited by ghost Loading

faustbrian commented Feb 15, 2019

vasild commented Feb 15, 2019

vasild commented Feb 15, 2019 • edited Loading

faustbrian commented Feb 16, 2019

vasild commented Feb 18, 2019

faustbrian commented Feb 20, 2019

codecov-io commented Feb 22, 2019 • edited Loading

Codecov Report

spkjp Feb 22, 2019

Choose a reason for hiding this comment

vasild Feb 22, 2019

Choose a reason for hiding this comment

vasild commented Feb 14, 2019 •

edited

Loading

ghost commented Feb 14, 2019 •

edited by ghost

Loading

vasild commented Feb 15, 2019 •

edited

Loading

codecov-io commented Feb 22, 2019 •

edited

Loading