Skip to content
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

Merged
merged 3 commits into from
Jun 24, 2015
Merged

add in basic address dial filtering #1378

merged 3 commits into from
Jun 24, 2015

Conversation

whyrusleeping
Copy link
Member

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 jeromyj@gmail.com

return ipn, nil
}
return nil, errors.New("invalid format")
}
Copy link
Member

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.

Copy link
Member Author

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.

@jbenet
Copy link
Member

jbenet commented Jun 17, 2015

comments above.

This may be overkill, but may be nice to have both block + allow. harder, but something like chaining:

filters:
  0.0.0.0/0      // allow everything at the beginning. implicit, dont need to state it.
  10.0.0.0/8     // allow only 10.0.0.0/8
  !10.10.0.0/16  // disallow 10.10.0.0/16

@@ -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)
Copy link
Member

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
}

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. 👍

@jbenet
Copy link
Member

jbenet commented Jun 17, 2015

This prevents dialing out but AFAICT, not listening to a conn from a filtered addr. will want to do this to listeners too

@whyrusleeping
Copy link
Member Author

@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?

@jbenet
Copy link
Member

jbenet commented Jun 17, 2015

@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.

@whyrusleeping
Copy link
Member Author

oh!! that makes more sense. Will do.

@whyrusleeping
Copy link
Member Author

alright, addressed comments mostly. Do we want to try and modularize multiaddr? or worry about that later?

@jbenet
Copy link
Member

jbenet commented Jun 18, 2015

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:

  • being able to dial out addresses not filtered
  • not being able to dial out addresses filtered
  • being able to accept addresses not filtered
  • not being able to accept addresses filtered

@whyrusleeping
Copy link
Member Author

@jbenet ping

@jbenet
Copy link
Member

jbenet commented Jun 23, 2015

@whyrusleeping does this pass tests? (gpe)

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
- 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 <jeromyj@gmail.com>
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
@GitCop
Copy link

GitCop commented Jun 23, 2015

There were the following issues with your Pull Request

  • Commit: 0bf6b39
    • Invalid signoff. Commit message must end with License: MIT
      Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue


This message was auto-generated by https://gitcop.com

@jbenet
Copy link
Member

jbenet commented Jun 23, 2015

ddc7aac and 1064110 didn't build so i squashed them.

@jbenet
Copy link
Member

jbenet commented Jun 23, 2015

I also rebased on master

@whyrusleeping whyrusleeping mentioned this pull request Jun 23, 2015
34 tasks
jbenet added a commit that referenced this pull request Jun 24, 2015
add in basic address dial filtering
@jbenet jbenet merged commit 65874eb into master Jun 24, 2015
@jbenet jbenet removed the status/in-progress In progress label Jun 24, 2015
@jbenet jbenet deleted the feat/filter branch June 24, 2015 22:12
@whyrusleeping whyrusleeping mentioned this pull request Jul 1, 2015
44 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants