-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add in basic address dial filtering #1378
Conversation
e31b873
to
4edd115
Compare
return ipn, nil | ||
} | ||
return nil, errors.New("invalid format") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can probably pull this into multiaddr
or multiaddr-net
. though i do like the modularity. multiaddr right now isn't very modular. maybe should have a pluggable table or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice if there was an AddProtocol
method that extension libs could call in an init function. So importing my lib would add my libs protocol to multiaddrs parsing table.
comments above. This may be overkill, but may be nice to have both block + allow. harder, but something like chaining:
|
@@ -303,6 +303,8 @@ func (s *Swarm) dial(ctx context.Context, p peer.ID) (*Conn, error) { | |||
ila, _ := s.InterfaceListenAddresses() | |||
remoteAddrs = addrutil.Subtract(remoteAddrs, ila) | |||
remoteAddrs = addrutil.Subtract(remoteAddrs, s.peers.Addrs(s.local)) | |||
remoteAddrs = s.filterAddrs(remoteAddrs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to special case the error here, where we may have addrs, but if we filter them all out, state that as a different error. basically:
remoteAddrs = addrutil.Subtract(remoteAddrs, ila)
remoteAddrs = addrutil.Subtract(remoteAddrs, s.peers.Addrs(s.local))
if len(remoteAddrs) == 0 {
err := errors.New("peer has no addresses")
logdial["error"] = err
return nil, err
}
remoteAddrs = s.filterAddrs(remoteAddrs)
if len(remoteAddrs) == 0 {
err := errors.New("all peer addresses filtered out")
logdial["error"] = err
return nil, err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. 👍
This prevents dialing out but AFAICT, not listening to a conn from a filtered addr. will want to do this to listeners too |
@jbenet that feels a little out of scope for this PR. Listening on a given addr isnt really causing us issues right now. Whats the reasoning? |
i dont mean "listening on a particular addr" i mean "if we get dialed by an addr in our filter". its not what we want all of the time (i.e. not for the people who're running into host scanning problems), but it is what users who want to setup entirely private networks want. |
oh!! that makes more sense. Will do. |
alright, addressed comments mostly. Do we want to try and modularize multiaddr? or worry about that later? |
i'd worry about that later. Also, we should have tests for this:
|
4e92b5b
to
263b587
Compare
@jbenet ping |
@whyrusleeping does this pass tests? (gpe) |
License: MIT Signed-off-by: Jeromy <[email protected]>
License: MIT Signed-off-by: Jeromy <[email protected]>
- add extra check to dialblock test - move filter to separate package - also improved tests - sunk filters down into p2p/net/conn/listener License: MIT Signed-off-by: Jeromy <[email protected]> Signed-off-by: Juan Batiz-Benet <[email protected]>
There were the following issues with your Pull Request
Guidelines are available to help. Your feedback on GitCop is welcome on this issue This message was auto-generated by https://gitcop.com |
I also rebased on master |
add in basic address dial filtering
So this works but i'm not sure if i like the construction. Its probably fine, because we are going to redo the daemon init/construction at some point anyways.
License: MIT
Signed-off-by: Jeromy [email protected]