feat: Allowlist #47

MarcoPolo · 2022-06-10T05:39:43Z

This adds the Allowlist to Resource manager. See docs/allowlist.md for the design doc and more details.

This change requires changes to be merged first in go-libp2p-core and go-multiaddr (to support the ipcidr protocol in the multiaddr).

After this is merged, go-libp2p will need to be plumbed with these changes as well.

Before Merge:

Update resource manager interface in go-libp2p-core
- Add endpoint parameter to the OpenConnection method for ResourceManager go-libp2p-core#257
Update go-multiaddr + multiaddr repo with ipcidr
- Add ipcidr support multiformats/go-multiaddr#177
- Add ipcidr multiformats/multiaddr#129

After Merge:

Another PR for the config input to define an allowlist. The allowlist supports dynamic updating, but it's not exposed yet.
Update go-libp2p to pass in the endpoint information

Accepts multiaddrs that define who is allowed.

MarcoPolo · 2022-06-13T23:45:50Z

@marten-seemann / @vyzo this is ready to review. Please take a look.

Before merging I'll make sure to finish this checkboxed things in the PR description.

marten-seemann

Is my understanding correct, that we allow two types of addresses to be whitelisted?

/ip{4,6}/<ip>/
/ip{4,6}/<ip>/p2p/<peerid>
But not /p2p/<peerid>?

We also need to add configurable limits for the newly introduced allowlist scopes, right?

allowlist.go

marten-seemann · 2022-06-14T18:12:04Z

allowlist.go

+	allowedNetworks []*net.IPNet
+
+	// Only the specified peers can use these IPs
+	allowedPeerByNetwork map[peer.ID][]*net.IPNet


Have you considered unifying these into a single map? map[net.IPNet]struct{ allowsAll bool; map[peer.ID]struct{} }
allowsAll would be set if a multiaddr without /p2p is added.

Can I use net.IPNet as a key? The struct holds two byteslices, so I don't think I can.

Just checked, you can't. That's annoying. You could use net.IPNet.String(), but not sure if that's nicer.

I don’t think so because:

allow checking is harder. you have to convert between the string and bytes.

You need to allocate the string on insertion.

you need to allocate at least once when doing an allowlist check.

(Depending on check implementation) you lose the quick lookup by peerid. Which may be a pretty common use case.

marten-seemann · 2022-06-14T18:13:50Z

rcmgr.go

-	peer  *peerScope
+	dir           network.Direction
+	usefd         bool
+	isAllowlisted bool


Nit: isAllowListed. Or newAllowlist instead of newAllowList.

I meant to call this allowlist or Allowlist. Purposely not capitalizing the l. I realize I should change newAllowList to newAllowlist to be consistent.

rcmgr_test.go

allowlist.go

marten-seemann · 2022-06-14T18:22:35Z

Requesting a review from @vyzo. Can you check how the scopes are getting shifted around?

…dTransient

MarcoPolo · 2022-06-14T18:53:41Z

We also need to add configurable limits for the newly introduced allowlist scopes, right?

Yep! Thank you for reminding me. I just added those with the same limits as the standard scopes.

Is my understanding correct, that we allow two types of addresses to be whitelisted?
1. `/ip{4,6}/<ip>/`

2. `/ip{4,6}/<ip>/p2p/<peerid>`
   But not `/p2p/<peerid>`?

Correct. I'm not sure what value the /p2p/<peerid> would give you since we'd have no way of knowing a connection was for this peerid until after we get the peerid information. Maybe if we knew that this address was signed by the <peerid> then we could allowlist the connection attempt. But I think you can do that with this by Adding the addresses you discover are signed by the peer you trust to the allowlist with the peer's peer id appended.

marten-seemann · 2022-06-15T09:19:32Z

Correct. I'm not sure what value the /p2p/ would give you since we'd have no way of knowing a connection was for this peerid until after we get the peerid information.

I agree. I was misled by the ipNet != nil check into assuming that we can have addresses that don't have an IP address.

MarcoPolo · 2022-06-16T16:58:20Z

Friendly ping @vyzo :)

marten-seemann · 2022-06-19T08:14:44Z

@MarcoPolo Do you want to include the config changes in this PR, or are you planning to create a follow-up PR for that?

MarcoPolo · 2022-06-19T15:05:13Z

@MarcoPolo Do you want to include the config changes in this PR, or are you planning to create a follow-up PR for that?

My plan was after this (see pr description). Since this will work fine without it and the config change is pretty small. And you have some changes around config in #48 in progress.

vyzo

We need a public interface and accessor in the rcmgr for the allowlist, plus an allowlist option to initialize.

vyzo · 2022-06-24T08:01:37Z

allowlist.go

+	manet "github.com/multiformats/go-multiaddr/net"
+)
+
+type allowlist struct {


Can we have an interface type assertion for this, right after the def?
Or is there no public interface?

I think we need to make a public interface for it in extapi, and provide an accessor so that users can modify after initialization.
The RWMutex is a must in this case.

I don't follow the reason for a public interface? I thought generally interfaces belonged to the caller.

Interfaces declare the behaviour the caller requires not the behaviour the type will provide. Let callers define an interface that describes the behaviour they expect. The interface belongs to them, the consumer, not you.

From: https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_let_callers_define_the_interface_they_require

ah is it because we return an interface and we can't have an interface function return this unexported type?

What's the downside of exporting this type then?

allowlist.go

rcmgr.go

vyzo · 2022-06-27T17:43:22Z

Another important consideration here: we should only check the allow list if and only if a regular connection reservation fails.
This way it only triggers at the limit and we avoid the computational cost in the happy path.

MarcoPolo · 2022-06-27T22:48:29Z

We need a public interface and accessor in the rcmgr for the allowlist, plus an allowlist option to initialize.

I exported the Allowlist type, added an accessor to rcmgr, and added the WithAllowlistedMultiaddrs option.

marten-seemann · 2022-06-29T09:25:23Z

allowlist.go

+	var allowedPeer peer.ID
+	var isIPV4 bool
+
+	multiaddr.ForEach(ma, func(c multiaddr.Component) bool {


Can we use ma.MultiaddrToIPNet here?

Probably not, if we want to extract the p2p component at the same time, right? We could get that one without iterating again by splitting off the last component though.

I also need the isIPV4 to save some time below

allowlist.go

marten-seemann · 2022-06-29T09:40:50Z

allowlist.go

+	}
+
+	i := len(ipNetList)
+	for i > 0 {


Any reason not to use for i, ipnet := range ipNetList? Do we gain anything from going through the list in reverse order?

For swap remove. If we go through in normal order then we would skip an entry after swap remove or do some other messy logic that I think boils down to reverse iterating

That only applies if you're trying to make multiple deletions from the list. Should be fine if you're breaking after the first deletion, right?

ah that's true. Although I'm a bit hesitant to change this so that instead of working for N removals it only works for 1 removal and would subtly break if you try to remove more than 1.

And the only reason to make the change is so that we use the syntactic sugar of for ... range. Is there any other benefit?

Fair enough. I'd consider range more idiomatic (makes it harder to produce out-of-bounds access). No need to change it here.

allowlist.go

allowlist_test.go

MarcoPolo · 2022-06-30T23:54:26Z

@marten-seemann anything left?

MarcoPolo added 7 commits June 8, 2022 15:55

Add design doc

ee3cc3b

Add allowlist impl

dd0cde1

Accepts multiaddrs that define who is allowed.

Add allowlist to rcmgr

1fcd41c

Simplify by removing ip addr, only networks. Adds a benchmark

23ec39d

Add comment

78498fb

Cleanup

7ecf0fc

Fix comment

297cd00

MarcoPolo marked this pull request as ready for review June 13, 2022 23:39

marten-seemann self-requested a review June 14, 2022 18:00

marten-seemann reviewed Jun 14, 2022

View reviewed changes

marten-seemann requested a review from vyzo June 14, 2022 18:22

MarcoPolo added 2 commits June 14, 2022 11:46

PR comments

84ca9d3

Use the same standard limits for the allowlistedSystem and allowliste…

539b147

…dTransient

MarcoPolo added 2 commits June 15, 2022 15:22

Fix allowlist system instantiation

dc09a97

Persist endpoint

a410332

vyzo reviewed Jun 27, 2022

View reviewed changes

MarcoPolo added 6 commits June 27, 2022 13:59

Fuse with if above

1bf5e93

Protect inner data with RWMutex

fd82a9f

Only use allowlist if the normal resource scopes would have failed

61008b3

Update comment

5f5fdb3

Allowlist test with rcmgr

a5e8e04

Export Allowlist type

2bb066a

MarcoPolo added 2 commits June 27, 2022 15:42

Add GetAllowlist()

8c5643a

Add WithAllowlistedMultiaddr option

374b297

Update allowlist design doc with no extra cost for happy path

600191a

MarcoPolo requested a review from marten-seemann June 27, 2022 22:55

BigLep linked an issue Jun 28, 2022 that may be closed by this pull request

Defend against eclipse attacks with ALLOW-list support #29

Closed

BigLep mentioned this pull request Jun 28, 2022

go-libp2p v0.21.0 libp2p/go-libp2p#1514

Closed

41 tasks

marten-seemann reviewed Jun 29, 2022

View reviewed changes

MarcoPolo added 2 commits June 29, 2022 21:18

PR nits

7aecdc1

Remove branch

6edfbe4

MarcoPolo mentioned this pull request Jun 30, 2022

Release v0.19.0 libp2p/go-libp2p-core#271

Merged

MarcoPolo added 2 commits June 30, 2022 15:34

Update deps

8eac037

Mod tidy

0487d3e

MarcoPolo requested a review from marten-seemann June 30, 2022 23:54

marten-seemann approved these changes Jul 1, 2022

View reviewed changes

MarcoPolo merged commit 6607ffe into master Jul 1, 2022

MarcoPolo deleted the marco/allowlist branch July 1, 2022 18:59

BigLep mentioned this pull request Jul 1, 2022

Defend against eclipse attacks with ALLOW-list support #29

Closed

ajnavarro mentioned this pull request Aug 24, 2022

Release 0.15 ipfs/kubo#9152

Closed

72 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allowlist #47

feat: Allowlist #47

MarcoPolo commented Jun 10, 2022 •

edited

Loading

MarcoPolo commented Jun 13, 2022

marten-seemann left a comment

marten-seemann Jun 14, 2022

MarcoPolo Jun 14, 2022

marten-seemann Jun 15, 2022

MarcoPolo Jun 15, 2022

marten-seemann Jun 14, 2022 •

edited

Loading

MarcoPolo Jun 14, 2022

marten-seemann commented Jun 14, 2022

MarcoPolo commented Jun 14, 2022

marten-seemann commented Jun 15, 2022

MarcoPolo commented Jun 16, 2022

marten-seemann commented Jun 19, 2022

MarcoPolo commented Jun 19, 2022

vyzo left a comment

vyzo Jun 24, 2022

vyzo Jun 27, 2022

MarcoPolo Jun 27, 2022

MarcoPolo Jun 27, 2022

vyzo commented Jun 27, 2022

MarcoPolo commented Jun 27, 2022

marten-seemann Jun 29, 2022

marten-seemann Jun 29, 2022

MarcoPolo Jun 29, 2022

marten-seemann Jun 29, 2022

MarcoPolo Jun 29, 2022

marten-seemann Jun 30, 2022

MarcoPolo Jun 30, 2022

marten-seemann Jul 1, 2022

MarcoPolo commented Jun 30, 2022

feat: Allowlist #47

feat: Allowlist #47

Conversation

MarcoPolo commented Jun 10, 2022 • edited Loading

MarcoPolo commented Jun 13, 2022

marten-seemann 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

marten-seemann Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann commented Jun 14, 2022

MarcoPolo commented Jun 14, 2022

marten-seemann commented Jun 15, 2022

MarcoPolo commented Jun 16, 2022

marten-seemann commented Jun 19, 2022

MarcoPolo commented Jun 19, 2022

vyzo 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

vyzo commented Jun 27, 2022

MarcoPolo commented Jun 27, 2022

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

MarcoPolo commented Jun 30, 2022

MarcoPolo commented Jun 10, 2022 •

edited

Loading

marten-seemann Jun 14, 2022 •

edited

Loading