Implement support for whitelists, default-deny/allow #1

arrdem · 2017-03-30T06:16:57Z

This makes ipfs/kubo#1972 possible by allowing the Filter to implement last-matching-term-wins policies. This allows for default-deny policies with exceptions or default-allow policies with large deny masks and selective permission in the same style as SQUID and other ACL systems.

coveralls · 2017-03-30T06:31:38Z

Coverage decreased (-1.7%) to 52.83% when pulling 2a8da79 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-30T06:31:39Z

Coverage decreased (-1.7%) to 52.83% when pulling 2a8da79 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-30T06:34:56Z

Coverage decreased (-1.7%) to 52.83% when pulling 0bf6dc0 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-30T06:34:56Z

Coverage decreased (-1.7%) to 52.83% when pulling 0bf6dc0 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-30T06:47:34Z

Coverage increased (+7.7%) to 62.264% when pulling 26ab0b9 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-30T07:17:14Z

Coverage decreased (-4.5%) to 50.0% when pulling c14e093 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-30T07:17:15Z

Coverage decreased (-4.5%) to 50.0% when pulling c14e093 on arrdem:master into 90aacb5 on libp2p:master.

hsanjuan · 2017-03-30T10:40:12Z

README.md

+
+// the default for a filter is accept, but we can change that
+f.Remove(ipnet)
+f.filterDefault = true


Should be an exported field, otherwise it's useless.

Also, I'd prefer something more descriptive like AcceptByDefault

Hmm you forgot to update line 44 here with the new name.

hsanjuan · 2017-03-30T10:44:50Z

filter.go

-	filters map[string]*net.IPNet
+	mu            sync.RWMutex
+	filterDefault bool
+	filters       []*FilterEntry


What happens when there are to entries for the same IPNet? Seems behaviour now depends on how search is implemented.

Why not using `map[string]*FilterEntry ? Also simplifies searching and deleting.

Ah, if this is about ipfs/kubo#1972 (comment) disregard this comment.

hsanjuan · 2017-03-30T10:45:13Z

filter.go

@@ -8,21 +8,34 @@ import (
 	manet "github.com/multiformats/go-multiaddr-net"
 )

+type FilterEntry struct {


should be unexported as it seems to have no use externally.

hsanjuan

Can you also make sure to include godoc/golint comments in the functions you are modifying. I know they were not there before, but it's good to take the chance and improve that.

coveralls · 2017-03-30T18:44:09Z

Coverage decreased (-3.8%) to 50.769% when pulling 4c905c2 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-31T00:37:31Z

Coverage increased (+21.3%) to 75.806% when pulling 95de304 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-31T00:37:31Z

Coverage increased (+21.3%) to 75.806% when pulling 95de304 on arrdem:master into 90aacb5 on libp2p:master.

arrdem · 2017-03-31T02:05:34Z

This should be ready for boats now.

hsanjuan

Hi

sorry to go back to it, can you explain why using a slice of filters is better than keeping the map that was used before?

I read too quickly the yesteday and now I don't see the point of keeping several rules for the same IPNet if only one of them is going to matter.

Thanks!!

hsanjuan · 2017-03-31T10:05:32Z

README.md

+
+// the default for a filter is accept, but we can change that
+f.Remove(ipnet)
+f.filterDefault = true


Hmm you forgot to update line 44 here with the new name.

hsanjuan · 2017-03-31T10:09:00Z

filter.go

 }

+// AddrBlocked parses a ma.Multiaddr, and if it can get a valid netip


and, if it can get a valid net.IPNet back, applies the Filters,...

(I'm no native English speaker so I'm not 100% sure but there's something weird with the commas in the original formulation).

hsanjuan · 2017-03-31T10:20:07Z

filter.go

 }

+// AddrBlocked parses a ma.Multiaddr, and if it can get a valid netip
+// back applies the Filters, returning true if the given address
+// should be rejected, and false if the given address is allowed.


Now I'm going around reading grammar explanations about commas. It's probably wrong to use a comma before and in this particular case.

You are correct. I missplaced those commas.

hsanjuan · 2017-03-31T10:30:16Z

filter_test.go

@@ -34,18 +34,90 @@ func TestFilter(t *testing.T) {
 		}
 	}

-	for _, notBlocked := range []string{
+	// test that other net intervals are not blocked


Can you split this into multiple test functions, one for each exported method?

hsanjuan · 2017-03-31T10:30:33Z

filter.go

 	}

 	f.mu.RLock()
 	defer f.mu.RUnlock()
+
+	var reject bool = f.RejectByDefault


Above there is a comment saying that the last policy added to the filter is authoritative.

AddDialFilter and AddAllowFilter append policies to the filter slice.

And here there is a for loop which looks for a matching rule from the beginning of the slice, so actually the first policy added to the filter is authoritative?. Also, see my comment above about going back to using a map.

"By here I mean 2 lines below"

Right. So we set the default result (f.RejectByDefault), then iterate through all filters. The filters slice is in insertion order, so the first filter inserted is applied first. We apply all filters to the address, and if the filter matches the address we change the reject outcome to be the outcome specified by the matching rule.

To be specific about the expected behavior:

DEFAULT DENY ALLOW 192.168.0.0/16 DENY 192.160.0.0/24 ALLOW 192.168.0.5/32

These rules specify to reject all connections, allowing a /16, blocking a /24 out of that, and and making exception for a single /32. These filters work because they're insertion ordered. This how other ACL systems tend to operate with the last rule to match winning. Better ACL systems make some attempt to sort, compact & optimize the ACL table, but this is usually Good Enough.

So as an exercise, given an attempted connection to 192.168.0.5, we will first take the default outcome which is DENY. Then we apply the first rule, which matches and says ALLOW presumably because that /16 is trusted address space. But there may be other relevant policies so we have to keep going. We apply the next rule, which also matches and produces a DENY. But another rule could express an exception so we keep going, and we find that the particular /32 is ALLOWed explicitly.

We can't go back to a map because maps aren't insertion ordered. The unordered map system works fine when you just want to see if there's a reject rule which matches and there's no possibility of countermanding a reject decision once one has been made. However because the map has no concept of ordering it's impossible to impose a hierarchy between rules allowing for exceptions as in this example. It's also not possible to achieve this sort of behavior with two maps (which was the second thing I tried) because again the ordering is ambiguous as to whether the ALLOW or DENY directive should win given even a simple conflict.

Yeah you're right. Just saw you're not breaking out of the loop :S. Sorry.

hsanjuan · 2017-03-31T10:39:16Z

filter.go

+// Filters returns the list of net.IPNet structs in the given filter
+// rules.
+//
+// FIXME: This function doesn't really make any sense now that filters


Right, can you change this to return only the reject filters as originally? Behaviour of this library should not be different than before.

Adding an AcceptFilters() which returns the others would not hurt.

Sure. The semantics of the two don't really make a ton of sense anymore since the filters are order dependent and taking either list independently looses all the semantics but sure.

coveralls · 2017-03-31T23:55:22Z

Coverage increased (+14.1%) to 68.675% when pulling a191744 on arrdem:master into 90aacb5 on libp2p:master.

coveralls · 2017-03-31T23:55:22Z

Coverage increased (+14.1%) to 68.675% when pulling a191744 on arrdem:master into 90aacb5 on libp2p:master.

hsanjuan · 2017-04-03T13:13:54Z

@whyrusleeping can you have a look here?

LGTM, but note the comments on Filters():

The semantics of the two don't really make a ton of sense anymore since the filters are order dependent and taking either list independently looses all the semantics but sure.

tvi · 2017-07-26T11:08:24Z

Is there any update on this? I would like to use these filters.

anacrolix · 2018-12-24T01:42:54Z

@arrdem Do you still have interest in this PR?

arrdem · 2018-12-24T21:24:39Z

@anacrolix My understanding was that, as a result of other IPFS-ey conversations rather than supporting whitelists of hosts with whom to peer or blacklists from whom to drop traffic which it was the intent of this PR to enable, other plans were being toyed with for private networks based on different bootstrap servers.

Personally I'm not using IPFS atm, and no longer have a burning need for this feature, but I'd be happy to see it merged. That said looks like there are conflicts, this needs revisiting and frankly my next moth is a mess so really this is a project direction question for you.

hsanjuan · 2018-12-25T12:40:27Z

@libp2p/go-team are there any chances/interest to rescue this?

bigs · 2019-01-07T23:51:58Z

@hsanjuan if we still want this, i can review/rebase/merge this tomorrow!

nodegin · 2019-02-03T01:23:38Z

Whitelist is really a must... Please implement this

anacrolix · 2019-02-03T02:30:31Z

This is now tracked in #8. @nodegin

arrdem force-pushed the master branch from b0ae6af to 2a8da79 Compare March 30, 2017 06:28

Implement support for whitelists, default-deny/allow

0bf6dc0

arrdem force-pushed the master branch from 2a8da79 to 0bf6dc0 Compare March 30, 2017 06:32

Expand test coverage

26ab0b9

arrdem added 2 commits March 29, 2017 23:54

Update README with extended API

5f11539

Refactor to use slices so ACLs are in-order

c14e093

hsanjuan reviewed Mar 30, 2017

View reviewed changes

hsanjuan suggested changes Mar 30, 2017

View reviewed changes

arrdem added 2 commits March 30, 2017 11:30

Unexport, add godocs

518c39d

Fix plurality to reflect behavior

4c905c2

arrdem force-pushed the master branch from 31b83e2 to 4c905c2 Compare March 30, 2017 18:31

Make Find internal, improve coverage to include Remove

95de304

hsanjuan suggested changes Mar 31, 2017

View reviewed changes

arrdem added 4 commits March 31, 2017 16:04

Update filter default flag

4bef9b6

Fix commas

71daec9

Fix Filters() to only return DENYs, add AllowFilters()

a29eec5

Prevent straight CIDR dupes by flipping the bit as appropriate

cbc531a

arrdem added 3 commits March 31, 2017 16:47

Hoist this for logical order

1f51415

Don't need the for loop anymore

1929808

Factor tests better

a191744

flyingzumwalt mentioned this pull request Apr 30, 2017

ipfs swarm filters seems blacklist-only ipfs/kubo#1972

Open

hsanjuan mentioned this pull request Dec 25, 2018

Permissioned RPC endpoints libp2p/go-libp2p-gorpc#35

Closed

bigs mentioned this pull request Jan 8, 2019

Implement support for whitelists, default-deny/allow (rebase) #8

Merged

anacrolix closed this Feb 3, 2019

		}

		// AddrBlocked parses a ma.Multiaddr, and if it can get a valid netip

Implement support for whitelists, default-deny/allow #1

Implement support for whitelists, default-deny/allow #1

Conversation

arrdem commented Mar 30, 2017

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017 • edited Loading

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017

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

hsanjuan left a comment

Choose a reason for hiding this comment

coveralls commented Mar 30, 2017 • edited Loading

coveralls commented Mar 31, 2017

coveralls commented Mar 31, 2017

arrdem commented Mar 31, 2017

hsanjuan 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

Choose a reason for hiding this comment

hsanjuan Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

hsanjuan Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

arrdem Mar 31, 2017 • edited Loading

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

coveralls commented Mar 31, 2017

coveralls commented Mar 31, 2017

hsanjuan commented Apr 3, 2017

tvi commented Jul 26, 2017

anacrolix commented Dec 24, 2018

arrdem commented Dec 24, 2018

hsanjuan commented Dec 25, 2018

bigs commented Jan 7, 2019

nodegin commented Feb 3, 2019

anacrolix commented Feb 3, 2019

coveralls commented Mar 30, 2017 •

edited

Loading

coveralls commented Mar 30, 2017 •

edited

Loading

hsanjuan Mar 31, 2017 •

edited

Loading

hsanjuan Mar 31, 2017 •

edited

Loading

arrdem Mar 31, 2017 •

edited

Loading