p2p: limit node per single IP #537

r8d8 · 2018-03-14T14:16:53Z

Countermeasure 2 for eclipse attack

Countermeasure 2 for `eclipse attack`

tzdybal · 2018-03-19T14:07:44Z

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

p2p/discover/table.go, line 148 at r1 (raw file):

		}
	}
	for i := range tab.buckets {

Old loop needs to be removed, as it overwrites the buckets with initialized ips.

p2p/discover/table.go, line 148 at r1 (raw file):

		}
	}
	for i := range tab.buckets {

I can't see the initialization of tab.buckets - where is it? what's the size of the slice?

p2p/discover/table_test.go, line 173 at r1 (raw file):

		n := nodeAtDistance(tab.self.sha, d)
		n.IP = net.IP{172, 0, 1, byte(i)}
		tab.add(n)

what about the "ping" functionality from tab.add?

p2p/distip/net.go, line 23 at r1 (raw file):

// number of existing IPs in the defined range exceeds the limit.
func (s *DistinctNetSet) Add(ip net.IP) bool {
	key := s.key(ip)

stringify key here, once. We don't need key in original form.

p2p/distip/net.go, line 34 at r1 (raw file):

// Remove removes an IP from the set.
func (s *DistinctNetSet) Remove(ip net.IP) {
	key := s.key(ip)

Same here. We can convert key to string once, before assigning to variable.

p2p/distip/net.go, line 53 at r1 (raw file):

// Len returns the number of tracked IPs.
func (s DistinctNetSet) Len() int {
	n := uint(0)

Why uint and int?

Comments from Reviewable

r8d8 · 2018-03-19T14:37:03Z

Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions.

p2p/discover/table.go, line 148 at r1 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

Old loop needs to be removed, as it overwrites the buckets with initialized ips.

Done.

p2p/distip/net.go, line 23 at r1 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

stringify key here, once. We don't need key in original form.

Done.

p2p/distip/net.go, line 34 at r1 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

Same here. We can convert key to string once, before assigning to variable.

Done.

p2p/distip/net.go, line 53 at r1 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

Why uint and int?

Done.

Comments from Reviewable

r8d8 · 2018-03-19T14:40:40Z

Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

p2p/discover/table.go, line 148 at r1 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

I can't see the initialization of tab.buckets - where is it? what's the size of the slice?

https://github.com/ethereumproject/go-ethereum/pull/537/files#diff-a59ccf33a2c8693dad1b9532841a894bR45

Comments from Reviewable

tzdybal · 2018-03-19T15:39:39Z

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

whilei · 2018-03-19T19:42:02Z

p2p/discover/table_test.go

@@ -144,6 +144,39 @@ func fillBucket(tab *Table, ld int) (last *Node) {
 	return b.entries[bucketSize-1]
 }

+// This checks that the table-wide IP limit is applied correctly.
+func TestTable_IPLimit(t *testing.T) {


It is not clear to me what the difference should be between this test and TestTable_BucketIPLimit..?

FYI My proposed changes from whilei@dfbe261 fix the panic currently causing these tests to fail, but still this test fails because table ip limit is exceeded.

whilei · 2018-03-20T12:00:33Z

😕 Where are the other CI builds?

r8d8 · 2018-03-20T12:04:08Z

yeap, have the same question. @tzdybal, @ia do you have admin access to check hooks in Settings tab?

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

tzdybal · 2018-03-20T12:38:30Z

I changed settings right now, to enable running CI on PR's from forks. I'll try to trigger the build.

solution: initial tests for receiver functions

solution: wait for tab initDone

whilei · 2018-03-20T13:16:37Z

~~@r8d8 negatory, captain. Maybe try pushing the branch to upstream and changing the source branch for the PR?~~

whilei · 2018-03-20T13:18:47Z

And just bumping this question since it got "outdated" from merge -- #537 (comment)

r8d8 · 2018-03-20T14:49:56Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

p2p/discover/table_test.go, line 148 at r2 (raw file):

Previously, whilei (ia) wrote…

It is not clear to me what the difference should be between this test and TestTable_BucketIPLimit..?

FYI My proposed changes from whilei@dfbe261 fix the panic currently causing these tests to fail, but still this test fails because table ip limit is exceeded.

it calls nodeAtDistance, with d=3, which calculates Kamelia's distance (logdist(base, n.sha) == ld.)

Comments from Reviewable

tzdybal · 2018-03-20T15:40:29Z

Reviewed 49 of 50 files at r3.
Review status: 51 of 52 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

coverage.tmp, line 1 at r3 (raw file):

mode: atomic

I think we should remove this file.

accounts/testdata/keystore/accounts.db, line 0 at r3 (raw file):
Safe to remove?

p2p/distip/net_test.go, line 39 at r3 (raw file):

		t.Errorf("got: %v, want: %v", len, 0)
	}
}

Test with more then one IP would be cool.

Comments from Reviewable

whilei · 2018-03-20T22:13:17Z

accounts/testdata/keystore/accounts.db, line at r3 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

Safe to remove?

affirmative

~/gocode/src/github.com/ethereumproject/go-ethereum/.git/info GIT_DIR! ⟠ cat exclude
/.idea
/i
/accounts/testdata/benchmark_keystore*
/accounts/testdata/keystore/accounts.db

Comments from Reviewable

Node managment inside buckets

r8d8 · 2018-03-21T07:45:45Z

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions.

accounts/testdata/keystore/accounts.db, line at r3 (raw file):

Previously, whilei (ia) wrote…

affirmative

~/gocode/src/github.com/ethereumproject/go-ethereum/.git/info GIT_DIR! ⟠ cat exclude
/.idea
/i
/accounts/testdata/benchmark_keystore*
/accounts/testdata/keystore/accounts.db

Done

Comments from Reviewable

r8d8 · 2018-03-21T07:46:21Z

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions.

coverage.tmp, line 1 at r3 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

I think we should remove this file.

Done.

Comments from Reviewable

r8d8 · 2018-03-21T07:46:49Z

Reviewed 1 of 3 files at r1.
Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

) * Apply source code formating provided by `go` tool * solution: remove not-to-be versioned accounts.db testdata * problem: should test distinctNetSet with >1 address solution: create test for dynamic number of ips problem: test fails * Fix for p2p table Node managment inside buckets * solution: remove coverage.tmp * Updated findnode mechanism to filter out local IPs * problem: test fails because that's not how distinctNetSet is supposed to work solution: use EF test for EF code

whilei · 2018-03-21T13:19:44Z

p2p/discover/table.go

@@ -595,43 +610,69 @@ func (tab *Table) ping(id NodeID, addr *net.UDPAddr) error {
 	return nil
 }

+// add attempts to add the given node its corresponding bucket. If the


can we get rid of this?

whilei · 2018-03-21T13:20:43Z

Except for this questionable commented code hunk, LGTM

… fix/eclipse_attack

tzdybal · 2018-03-21T22:30:42Z

Reviewed 1 of 50 files at r3, 2 of 4 files at r4, 5 of 6 files at r5, 2 of 2 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

p2p/discover/udp.go, line 165 at r7 (raw file):

func (t *udp) nodeFromRPC(sender *net.UDPAddr, rn rpcNode) (*Node, error) {
	if rn.UDP <= 1024 {
		return nil, errors.New("low port")

Why error? This is definitely not common config, but restriction looks quite arbitral.

Comments from Reviewable

whilei · 2018-03-22T13:55:46Z

p2p/discover/udp.go, line 165 at r7 (raw file):

Previously, tzdybal (Tomasz Zdybał) wrote…

Why error? This is definitely not common config, but restriction looks quite arbitral.

Because ports <1024 are "reserved", see https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers

Comments from Reviewable

whilei · 2018-03-22T13:56:39Z

p2p/discover/udp.go, line 165 at r7 (raw file):

Previously, whilei (ia) wrote…

Because ports <1024 are "reserved", see https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers

*"many ports <1024"... but not all

Comments from Reviewable

r8d8 added 4 commits March 14, 2018 16:13

Add limit for node per single Ip

f09bc99

Countermeasure 2 for `eclipse attack`

Fix package path

33209b8

Fix package import path

ca54caf

Merge remote-tracking branch 'upstream/master' into fix/eclipse_attack

60186df

tzdybal requested review from tzdybal and whilei March 19, 2018 13:27

r8d8 changed the title ~~[WIP] p2p: limit node per single IP~~ p2p: limit node per single IP Mar 19, 2018

Refactor distinc ip handler

2191993

whilei reviewed Mar 19, 2018

View reviewed changes

r8d8 and others added 4 commits March 20, 2018 14:58

Apply source code formating provided by go tool

f3ae149

solution: remove unused function fillBucket

aeed713

problem: no test for DistinctNetSet

31faaa1

solution: initial tests for receiver functions

problem: panic when pingRecorder calls findnode

f630f78

solution: wait for tab initDone

r8d8 added 2 commits March 21, 2018 08:30

Fix for p2p table

61eaa9d

Node managment inside buckets

Merge remote-tracking branch 'upstream/master' into fix/eclipse_attack

22322b1

r8d8 and others added 2 commits March 21, 2018 12:22

Updated findnode mechanism to filter out local IPs

e57ea2f

whilei reviewed Mar 21, 2018

View reviewed changes

r8d8 added 2 commits March 21, 2018 15:31

Code cleanup

4db7ac2

Merge branch 'fix/eclipse_attack' of github.com:r8d8/go-ethereum into…

9e53061

… fix/eclipse_attack

whilei approved these changes Mar 21, 2018

View reviewed changes

tzdybal approved these changes Mar 21, 2018

View reviewed changes

whilei mentioned this pull request Mar 22, 2018

fix mlog wire casting cases with error #542

Merged

whilei merged commit 559ff66 into ethereumproject:master Mar 22, 2018

whilei mentioned this pull request Mar 22, 2018

make state sync independent #506

Merged

1 task

r8d8 deleted the fix/eclipse_attack branch April 3, 2018 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: limit node per single IP #537

p2p: limit node per single IP #537

r8d8 commented Mar 14, 2018

tzdybal commented Mar 19, 2018

r8d8 commented Mar 19, 2018

r8d8 commented Mar 19, 2018

tzdybal commented Mar 19, 2018

whilei Mar 19, 2018

whilei commented Mar 20, 2018 •

edited

Loading

r8d8 commented Mar 20, 2018 •

edited

Loading

tzdybal commented Mar 20, 2018

whilei commented Mar 20, 2018 •

edited

Loading

whilei commented Mar 20, 2018

r8d8 commented Mar 20, 2018

tzdybal commented Mar 20, 2018

whilei commented Mar 20, 2018

r8d8 commented Mar 21, 2018

r8d8 commented Mar 21, 2018

r8d8 commented Mar 21, 2018

whilei Mar 21, 2018

whilei commented Mar 21, 2018

tzdybal commented Mar 21, 2018

whilei commented Mar 22, 2018

whilei commented Mar 22, 2018

p2p: limit node per single IP #537

p2p: limit node per single IP #537

Conversation

r8d8 commented Mar 14, 2018

tzdybal commented Mar 19, 2018

r8d8 commented Mar 19, 2018

r8d8 commented Mar 19, 2018

tzdybal commented Mar 19, 2018

whilei Mar 19, 2018

Choose a reason for hiding this comment

whilei commented Mar 20, 2018 • edited Loading

r8d8 commented Mar 20, 2018 • edited Loading

tzdybal commented Mar 20, 2018

whilei commented Mar 20, 2018 • edited Loading

whilei commented Mar 20, 2018

r8d8 commented Mar 20, 2018

tzdybal commented Mar 20, 2018

whilei commented Mar 20, 2018

r8d8 commented Mar 21, 2018

r8d8 commented Mar 21, 2018

r8d8 commented Mar 21, 2018

whilei Mar 21, 2018

Choose a reason for hiding this comment

whilei commented Mar 21, 2018

tzdybal commented Mar 21, 2018

whilei commented Mar 22, 2018

whilei commented Mar 22, 2018

whilei commented Mar 20, 2018 •

edited

Loading

r8d8 commented Mar 20, 2018 •

edited

Loading

whilei commented Mar 20, 2018 •

edited

Loading