only allowing one connection per IP address

[Trustin Lee]

Hi Frederic,

On Mon, Mar 23, 2009 at 7:36 PM, Frederic Bregier <fredbregier at free.fr> wrote:
>
> Hi Trustin,
>
> I come back on some points. Mainly the core of IpFilteringHandler
>
>>> * for handleRefusedChannel(), did you mean that any actions
>>> other than closing could be done here? Or say in another way,
>>> closing is still outside of this method in order to be factorized
>>> whatever the action (or no action) is done in the method ?
>>I mean even the close request should be done in
>>handlerRefusedChannel() and therefore its default implementation
>>should be:
>>
>>    protected void handleRefusedChannel(ctx, evt) {
>>        ctx.getChannel().close();
>>    }
>
>>and a user could override this method if necessary.
>
> I feel a bit different. I implement in fact three methods:
> - channelConnectedAccept : this one is called from the handleUpstream (CONNECT event).
>    It first calls accept, then if true, it calls handleRefusedChannel
>
> - accept : this one has to be given by the implementation (ex IpBlackWhiteList)
>
> - handleRefusedChannel : this one has to be given by the implementation and by default do nothing

I'm actually talking about channelOpenAccept / channelBoundAccept /
channelConnectedAccept.  Why do we need the three methods where accpet
can just do the job?  IpBlackWhiteListHandler can just implement
IpFilteringHandler.accept(), no?

> My idea is that any implementation has not to re-write or call super() if possible in order to handle the steps accept then handleRefused then close.
> And my argument (but of course I could be wrong, so my proposal) is that when the close signal is on going, the fourth method channelClosedContinue (another name could be better) is called and can block the chain in the pipeline.
> Of course, why doing that ?
> I feel, (I may be wrong), that when a connection is refused, it should not go to the next entry in the pipeline but stop as early as possible. Of course, if one wants however to continue, he can call in handleRefused the next entry in the pipeline by itself, and the same he can rewrite channelClosedContinue in order to return always false (so goes to the next entry for CLOSED event too).
> That's was one of the reason I was thinking to block as early as possible (if wanted) the connection (from OPEN or BOUND).
> But as you said sometimes there could be no remoteAddress for sure, it is a bad idea.
> However, what I would like is to prevent any message or whatever except connect and closed to be passed through.
> Because I feel like this kind of handler should block as much as possible when the connection is clearly indentified as a bad one.
>
> Again I may be wrong, and I understand that we are not completely on the same line. WDYT?

IIUC, your point is that the connection must be closed once accept()
returned false for the connection, right?  I agree with you, and my
proposal doesn't work for that case.  Perhaps we could modify
handleRefused() to return ChannelFuture so that IpFilteringHandler can
add ChannelFutureListener.CLOSE to the returned future?  Writing a bye
bye message before closure would look like this:

  protected ChannelFuture handleRefusedChannel(...) {
      retrun channel.write(BYE);
  }

WDYT?

>> * I don't think we need this many *Accept() methods.  My initial idea
>> was just to make IpFilterHandler to monitor all events so that any
>> connections can be closed even when messageReceived event is
>> triggered.  It could be like this:
>>
>>     public void handlerUpstream(ctx, evt) {
>>         if (accept(..., ctx.getChannel().getRemoteAddress())) {
>>             ctx.sendUpstream(evt);
>>         } else {
>>             handleRefusedChannel(...);
>>         }
>>     }
>
> I understand your point. However, it seems we are not on the same base.
> Of course, your point is more general than mine, I fully agree.
> But I can't imagine (perhaps because still a bit sick) :(
> a situation where you want to block something after the connexion was done.
> And moreover, you will call this accept method on every event, which could be not efficient
> (depending of course on the implementation of the accept method).
> However, with my proposal I've got a value already computed (attachment) that can enable blocking all event to be passed to the next entry in the pipeline. Is it ok for you this way ?

I agree with you that it's an overengineering.  We will be fine with
just monitoring channelOpen, channelBound, and channelConnected.
However, the point I'm trying to make is something different.  I am
saying there are too many accept methods (accept(),
channelOpenAccept(), channelBoundAccept(), channelConnectedAccept()).
Why do we need that many?

> Another subject is IpSubnet. I feel like it could be as complete as possible.
> Currently, it implements both CIDR and Subet.

I think merging CIDR and Subnet mask expression into one class is a
good idea.  However, a user might want to implement more complicated
rules like using the filtered IP list fetched from JDBC connection
periodically.  Imagine you are implementing an SMTP server and you
want to block connections from a certain country.  So it should be
extensible.

> About IpRangeRule, what exactly do you mean?
> I cannot see right now what is the exact meaning of 192.168.0.0-128 .

It means 192.168.0.0, 192.168.0.1, 192.168.0.2, ... 192.168.0.128.  I
don't think this can be implemented as a subnet mask.  Anyway, this is
just a good-to-have feature.  You don't need to implement it.  Subnet
mask will just work fine in almost every case.

> My proposal is to have both black and white lists at the same time,
> so enabling the user to have only one filter that handle all connections.
> Of course, one can have two handlers in its implementation, letting one list empty (for instance first white list is empty then in the second this would be the black list).

That's what I thought, too.

Actually, my proposal was to have a list of tuples where each tuple
has a allow/deny flag and IpFilterRule (e.g. first element: ALLOW or
DENY, second element: CIDR expression).  By doing so, we can let user
combine a set of different rules rather than specifying just one
allowing rule and one denying rule.  For example:

  rule #1 - ALLOW:127.0.0.1/32 (accept connections from localhost)
  rule #2 - DENY:192.168.1.0/24 (deny connections from 192.168.1.*)
  rule #3 - ALLOW:192.168.0.0/24 (accept connections from 192.168.0.*)
  rule #4 - DENY:0.0.0.0/0 (deny all other connections)

I know the rule 2 is unnecessary, but I think a user might load many
rules from different sources and therefore it should be tolerated.

> PS: excuse me if I am not clear or quick in understanding your points, probably due to my personal condition...

I've got cold too :)

Cheers,

— Trustin Lee, http://gleamynode.net/