Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

feat: Allowlist #47

Merged
merged 24 commits into from
Jul 1, 2022
Merged
Show file tree
Hide file tree
Changes from 11 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
187 changes: 187 additions & 0 deletions allowlist.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
package rcmgr

import (
"bytes"
"errors"
"fmt"
"net"

"github.com/libp2p/go-libp2p-core/peer"
"github.com/multiformats/go-multiaddr"
manet "github.com/multiformats/go-multiaddr/net"
)

type allowlist struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

// a simple structure of lists of networks. There is probably a faster way
// to check if an IP address is in this network than iterating over this
// list, but this is good enough for small numbers of networks (<1_000).
// Analyze the benchmark before trying to optimize this.

// Any peer with these IPs are allowed
allowedNetworks []*net.IPNet

// Only the specified peers can use these IPs
allowedPeerByNetwork map[peer.ID][]*net.IPNet
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think so because:

  1. allow checking is harder. you have to convert between the string and bytes.
  2. You need to allocate the string on insertion.
  3. you need to allocate at least once when doing an allowlist check.
  4. (Depending on check implementation) you lose the quick lookup by peerid. Which may be a pretty common use case.

}

func newAllowlist() allowlist {
return allowlist{
allowedPeerByNetwork: make(map[peer.ID][]*net.IPNet),
}
}

func toIPNet(ma multiaddr.Multiaddr) (*net.IPNet, peer.ID, error) {
var ipString string
var mask string
var allowedPeerStr string
var allowedPeer peer.ID
var isIPV4 bool

multiaddr.ForEach(ma, func(c multiaddr.Component) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use ma.MultiaddrToIPNet here?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also need the isIPV4 to save some time below

if c.Protocol().Code == multiaddr.P_IP4 || c.Protocol().Code == multiaddr.P_IP6 {
isIPV4 = c.Protocol().Code == multiaddr.P_IP4
ipString = c.Value()
}
if c.Protocol().Code == multiaddr.P_IPCIDR {
mask = c.Value()
}
if c.Protocol().Code == multiaddr.P_P2P {
allowedPeerStr = c.Value()
}
return ipString == "" || mask == "" || allowedPeerStr == ""
})

if ipString == "" {
return nil, allowedPeer, errors.New("missing ip address")
}

if allowedPeerStr != "" {
var err error
allowedPeer, err = peer.Decode(allowedPeerStr)
if err != nil {
return nil, allowedPeer, fmt.Errorf("failed to decode allowed peer: %w", err)
}
}

if mask == "" {
ip := net.ParseIP(ipString)
if ip == nil {
return nil, allowedPeer, errors.New("invalid ip address")
}
var mask net.IPMask
if isIPV4 {
mask = net.CIDRMask(32, 32)
} else {
mask = net.CIDRMask(128, 128)
}

net := &net.IPNet{IP: ip, Mask: mask}
return net, allowedPeer, nil
}

_, ipnet, err := net.ParseCIDR(ipString + "/" + mask)
return ipnet, allowedPeer, err

}

// Add takes a multiaddr and adds it to the allowlist. The multiaddr should be
// an ip address of the peer with or without a `/p2p` protocol.
// e.g. /ip4/1.2.3.4/p2p/QmFoo and /ip4/1.2.3.4 are valid.
// /p2p/QmFoo is not valid.
func (al *allowlist) Add(ma multiaddr.Multiaddr) error {
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
ipnet, allowedPeer, err := toIPNet(ma)
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}

if allowedPeer != peer.ID("") {
// We have a peerID constraint
al.allowedPeerByNetwork[allowedPeer] = append(al.allowedPeerByNetwork[allowedPeer], ipnet)
} else {
al.allowedNetworks = append(al.allowedNetworks, ipnet)
}
return nil
}

func (al *allowlist) Remove(ma multiaddr.Multiaddr) error {
ipnet, allowedPeer, err := toIPNet(ma)
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}
ipNetList := al.allowedNetworks

if allowedPeer != peer.ID("") {
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
// We have a peerID constraint
ipNetList = al.allowedPeerByNetwork[allowedPeer]
}

i := len(ipNetList)
for i > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

i--
if ipNetList[i].IP.Equal(ipnet.IP) && bytes.Equal(ipNetList[i].Mask, ipnet.Mask) {
if i == len(ipNetList)-1 {
// Trim this element from the end
ipNetList = ipNetList[:i]
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
} else {
// swap remove
ipNetList[i] = ipNetList[len(ipNetList)-1]
ipNetList = ipNetList[:len(ipNetList)-1]
}
}
}

if allowedPeer != "" {
al.allowedPeerByNetwork[allowedPeer] = ipNetList
} else {
al.allowedNetworks = ipNetList
}

return nil
}

func (al *allowlist) Allowed(ma multiaddr.Multiaddr) bool {
ip, err := manet.ToIP(ma)
MarcoPolo marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return false
}

for _, network := range al.allowedNetworks {
if network.Contains(ip) {
return true
}
}

for _, allowedNetworks := range al.allowedPeerByNetwork {
for _, network := range allowedNetworks {
if network.Contains(ip) {
return true
}
}
}

return false
}

func (al *allowlist) AllowedPeerAndMultiaddr(peerID peer.ID, ma multiaddr.Multiaddr) bool {
ip, err := manet.ToIP(ma)
if err != nil {
return false
}

for _, network := range al.allowedNetworks {
if network.Contains(ip) {
// We found a match that isn't constrained by a peerID
return true
}
}

if expectedNetworks, ok := al.allowedPeerByNetwork[peerID]; ok {
for _, expectedNetwork := range expectedNetworks {
if expectedNetwork.Contains(ip) {
return true
}
}
}

return false
}
Loading