only allowing one connection per IP address

[Frederic Bregier]

Hi Trustin,

I'm much better, even if I'm still not at work...

OK, I come back on your comments:

>>  Only channelConnectedAccept and channelClosedWasBlocked (not so happy with this name
>>  but I couldn't figure something more acceptable rigt now) are still there.
>
> I think we can simplify this further.  Can we remove
> channelConnectedAccept and channelClosedWasBlocked, and just use
> accept() and the attachment?  We can move the code in
> channelConnectedAccept() to handleUpstream() and get rid of
> channelConnectedAccept() and channelClosedWasBlocked().


OK, I remove them. I keep :
-accept
-handleRefusedChannel
-isBlocked (see later on)

I also change the FutureChannel handling. Can you check if it seems ok ?

>> - I only trap by default CONNECTED and CLOSED events.
>>  However, if a channel was blocked (accept returning false), all events will not
>>  be passed to the next entry in the pipeline.
>
> This is somewhat inconsistent in that there's possibility that
> channelOpen() and channelBound() can be handed off to the next
> handlers while channelClosed() will never be handed off.  We should
> block all events or none.

OK, so I change a bit the logic. All events (except OPEN and BOUND since
they occur before CONNECT so before the filter) are blocked by default.
But the user can change it either by overridden isBlocked (in OneIpFilter
it can be a problem) or by overridden handleUpstream.
WDYT ?


> It look OK, but I think it should work fine with IPv6 addresses, too.
> Could you consider this?

I agreen but why not considering another IpSubnet implementation for IPV6.
CIDR for instance seems not ok for IPV6 (am I right ?).

> This needs to be independent from IpSubnet, which means, IpSubnet
> shouldn't decide its address should be allowed or denied.

I agree, so I introduce a new Interface IpSet which IpSubnet implements.
Then IpFilterRule is an interface of IpSet plus ALLOW DENY property.
And then we've got IpSubnetFilterRule which implements this interface
by extending IpSubnet.
WDYT ?

> Same here.  It will need some modification once IpSubnet and
> allow/deny flag are split from each other.  IpSubnet is already
> IpFilterRule, so we can remove IpBlackWhiteListHandler, right?  I
> don't see a reason to keep IpBlackWhiteListHandler.

I agree so I remove IBWLH.
I change as I understand...

> It's getting better and better.  Please keep up the great work!
I hope so ;-)

Cheers,
Frederic
----- Original Message ----- 
From: Trustin Lee-2 (via Nabble)
To: Frederic Bregier
Sent: Friday, April 03, 2009 8:18 AM
Subject: Re: only allowing one connection per IP address


Comments inline:

On Wed, Apr 1, 2009 at 3:15 PM, Frederic Bregier <fredbregier at ...> wrote:
> Sorry for the delay...

Hope you feel fine now.

> I commit on the branch located at
> http://anonsvn.jboss.org/repos/netty/branches/ipfilter
> a new proposal, trying to follow as much as possible your ideas and my understanding.
>
> I make a small resume:
>
> - channelOpenAccept / channelBoundAccept are completely removed.

Sweet.

>  Only channelConnectedAccept and channelClosedWasBlocked (not so happy with this name
>  but I couldn't figure something more acceptable rigt now) are still there.

I think we can simplify this further.  Can we remove
channelConnectedAccept and channelClosedWasBlocked, and just use
accept() and the attachment?  We can move the code in
channelConnectedAccept() to handleUpstream() and get rid of
channelConnectedAccept() and channelClosedWasBlocked().

> - handleRefusedChannel returns a ChannelFuture (may be null).
>  channelConnectedAccept therefore wait for this future to finish (except if null)
>  before closing the channel.

The following code is buggy:

            ChannelFuture future = this.handleRefusedChannel(ctx, e,
inetSocketAddress);
            if (future != null) {
                future.awaitUninterruptibly();
            }
            Channels.close(e.getChannel());

You have to to use ChannelFuture.addListener().  We can not assume IP
filter is behind an ExecutionHandler.

> - I only trap by default CONNECTED and CLOSED events.
>  However, if a channel was blocked (accept returning false), all events will not
>  be passed to the next entry in the pipeline.

This is somewhat inconsistent in that there's possibility that
channelOpen() and channelBound() can be handed off to the next
handlers while channelClosed() will never be handed off.  We should
block all events or none.

> - I create an interface IpFilterRule that one might want to implement to fit his need.
>  A default implementation IpSubnet is proposed (both standard and CIDR notations).
>  The main change is as the following.
>
>  An IpFilterRule have two kind of informations:
>  - A subnet (or whatever it is) where a InetAddress can be compared by the method
>    contains. This method returns true if the InetAddress is in this IpFilterRule.

It look OK, but I think it should work fine with IPv6 addresses, too.
Could you consider this?

>  - A DENY or ALLOW flag that says if the previous call to contains should be
>    considered as a DENY or ALLOW rule.
>  - A constructor enables to define a IpSubnet DENY/ALLOW ALL by just setting the flag.

This needs to be independent from IpSubnet, which means, IpSubnet
shouldn't decide its address should be allowed or denied.

> - I change a bit the OneIpFilterHandler (mainly HashMap to Set).

This broke compatibility with Java 1.5.  I fixed it by reverting it
back to ConcurrentHashMap.

> - I change a bit the IpBlackWhiteListHandler mainly to reflect other changes.

Looks good!  It will need some modification once IpSubnet and
allow/deny flag are split from each other.

> - I create a new handler named IpFilterRuleHandler which could replace the BlackWhite
>  one as it is more general. The BlackWhite is an handler that have only two lists:
>  Black or deny list then White or allow list in that order.
>  IpFilterRuleHandler is like a standard Firewall ruleset where you can mix in whatever
>  order you want IpFilter which are ALLOW or DENY.

Same here.  It will need some modification once IpSubnet and
allow/deny flag are split from each other.  IpSubnet is already
IpFilterRule, so we can remove IpBlackWhiteListHandler, right?  I
don't see a reason to keep IpBlackWhiteListHandler.

> I hope those changes will be more ok.
> Please take care that I didn't test them correctly, it is just a prototype!!!

It's getting better and better.  Please keep up the great work!

Cheers,

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

_______________________________________________
netty-users mailing list
netty-users at ...
https://lists.jboss.org/mailman/listinfo/netty-users





This email is a reply to your post @ 
http://n2.nabble.com/only-allowing-one-connection-per-IP-address-tp2495797p2579138.html
You can reply by email or by visting the link above. 



-----
Hardware/Software Architect
-- 
View this message in context: http://n2.nabble.com/only-allowing-one-connection-per-IP-address-tp2495797p2580827.html
Sent from the Netty User Group mailing list archive at Nabble.com.