Bug 51953

Created attachment 27686 [details]
Netmask class

Created attachment 27687 [details]
Netmask filter

Created attachment 27688 [details]
Netmask valve

Created attachment 27689 [details]
Whole patch (git diff origin/trunk..)

I might make this filter/valve a bit more generic: there's no reason to go through all the trouble of doing an InetAddress lookup, creating a BigInteger, shifting it, and then comparing it if there is no CIDR spec in the IP specified by the configuration.

You could create a fairly generic IP-matching interface and then two implementations: one simple string-equals one and a more complicated netmask-matching one.

(In reply to comment #5)
> I might make this filter/valve a bit more generic: there's no reason to go
> through all the trouble of doing an InetAddress lookup, creating a BigInteger,
> shifting it, and then comparing it if there is no CIDR spec in the IP specified
> by the configuration.
> 

Well, the remote host is always an IP address in string form, so there is no lookup performed  at all. When supplied with an IP address in string form, InetAddress.getByName() only validates the validity of the IP address (whether it be IPv4 and IPv6).

Also, the current implementation also does simple IP matching, since if there is no CIDR the right shift is 0, thus an exact match is required.

More to the point, I don't see how I could make this generic? It would mean dispatching at some point, but how?

This code is executed unconditionally:

"
final InetAddress addr = InetAddress.getByName(property);

for (final NetMask nm: deny)
     if (nm.matches(addr))
"

NetMask.matches does a new BigInteger().shiftRight (though the shift should be 0 bytes, and so should be pretty quick).

Remember that this code may be executed for every request, include, and forward, so you'd better take every opportunity to reduce the amount of time required to run it.

In terms of making it more generic, what I meant was that you would be able to quickly match exact-matches (using a simple String.equals) and only do the extra work when there was a netmask to actually check.

(In reply to comment #7)
> This code is executed unconditionally:
> 
> "
> final InetAddress addr = InetAddress.getByName(property);
> 
> for (final NetMask nm: deny)
>      if (nm.matches(addr))
> "
> 
> NetMask.matches does a new BigInteger().shiftRight (though the shift should be
> 0 bytes, and so should be pretty quick).
> 
> Remember that this code may be executed for every request, include, and
> forward, so you'd better take every opportunity to reduce the amount of time
> required to run it.
> 

Good point. I was tempted at first to do a byte[] manipulation but found BigInteger to be very practical, especially since it requires the byte array to be in network order -- which is what I wanted.

I'll have that second implementation working and compare the results in speed.

> In terms of making it more generic, what I meant was that you would be able to
> quickly match exact-matches (using a simple String.equals) and only do the
> extra work when there was a netmask to actually check.

That's true, but consider ::ffff:127.0.0.1: it actually is the same than 127.0.0.1. An .equals() won't work here. OK, that's going far, but still.

Created attachment 27691 [details]
Unified patch for RemoteAddrNetmaskFilter

Take a look at this implementation (Filter only, for now), which I believe is a bit simpler and also allows fast string-matching when there is no "/" in the allow/deny spec.

Created attachment 27694 [details]
Alternative netmask class, using byte arrays

Here is another implementation using byte arrays only.

I believe it is even faster than a string compare on exact byte matching, _and_ it doesn't get fooled by ::ffff:* addresses.

This crude test using the following code shows indeed that the byte array based implementation is indeed MUCH faster:

----
    private static final int COUNT = 50000;

    public static void main(final String[] args) throws UnknownHostException
    {
        final NetMask nm = new NetMask("22.3.25.8/24");
        final NetMask2 nm2 = new NetMask2("22.3.25.8/24");

        final InetAddress in1 = InetAddress.getByName("22.3.25.10"),
            in2 = InetAddress.getByName("cf83::ef:13:1"),
            in3 = InetAddress.getByName("12.143.110.1");

        int i;
        long start, end;

        start = System.currentTimeMillis();
        for (i = 0; i < COUNT; i++) {
            nm.matches(in1);
            nm.matches(in2);
            nm.matches(in3);
        }
        end = System.currentTimeMillis();

        System.out.println("impl1: " + (end - start));

        start = System.currentTimeMillis();
        for (i = 0; i < COUNT; i++) {
            nm2.matches(in1);
            nm2.matches(in2);
            nm2.matches(in3);
        }
        end = System.currentTimeMillis();

        System.out.println("impl2: " + (end - start));

    }
----

Results:

----
impl1: 147
impl2: 11
----

So, it's definitely the byte-based class. I'll try and throw the string compare into the mix and see how it fares for good measure.

I like this NetMask implementation *much* better. It took me a while to convince myself that it was correct, but it looks good to me, now. :)

There are only a few nits of coding style that I would change. I'll post an updated patch that includes this implementation.

Did you get a chance to look at my patch?

(In reply to comment #12)
> I like this NetMask implementation *much* better. It took me a while to
> convince myself that it was correct, but it looks good to me, now. :)
> 
> There are only a few nits of coding style that I would change. I'll post an
> updated patch that includes this implementation.
> 
> Did you get a chance to look at my patch?

Yes I did. But I'm really not convinced that string matching is a good idea. It may be for IPv4, but for IPv6 it will be a mess.

If you remember, I've had the problem that ::1 wouldn't match because the address reported by request.getRemoteHost() (is it the name?) was actually 0:0:0:0:0:0:0:1, which means you have to write it all. Furthermore, it is quite easy to miss a :0  in the mix...

I'll continue the work when I get time.

(In reply to comment #13)
> But I'm really not convinced that string matching is a good idea. It
> may be for IPv4, but for IPv6 it will be a mess.

I think you're right. With your faster implementation of NetMask, it makes this more reasonable.

OK, I need advice.

How should errors be handled and at which level? What should happen if one specified netmask is invalid? Should the Valve/Filter fail completely? What should be logged and how?

Created attachment 27726 [details]
Tests for the NetMask class

Here are the first tests for this class. Only errors at object initialization time for now.

Can I have hints for error handling?

Still waiting for input...

Created attachment 27782 [details]
Version "0.1" of the patch

Full patch over Apache's tomcat70 git repo (therefore over tomcat's trunk if I get it right).

This patch addresses the following remarks raised on the devel mailing list:

* .get*() now return a suitable input for .set*();
* as the process of filling lists for deny and allow is the same, for the filter and the valve, factorize it;
* more comments in NetMask;
* coding style issues;
* javadoc;
* throw IllegalArgumentException from the Valve and Filter if a wrongly formatted NetMask is passed in.

Now, I have a problem which I cannot understand, wrt error handling...

I use the base tar.gz as built by "ant release", in output/release/v8.0.0-dev/bin/apache-tomcat-8.0.0-dev.tar.gz. I unarchive in a directory, do startup.sh, it starts.

I then edit the web.xml so as to apply the RemoteCIDRFilter: Tomcat reloads the webapp, the filter applies. Then I edit web.xml so as to apply an ILLEGAL netmask: even though the base NetMask tests work (constructor refuses the input), when reloading the webapp, it seems that nothing is even LOGGED (I do log errors about bad netmasks) and the previous filter stays in place :/

What am I doing wrong? I have a list of TODOs but don't even want to mention that list before I understand what the problem is with the filter. Note: I have not debugged yet... I use IDEA, so if someone knows how to debug that I'd be happy.

Well, my filter is not the only one affected.

I also added this to web.xml:

----
  <filter>
    <filter-name>host</filter-name>
    <filter-class>org.apache.catalina.filters.RemoteHostFilter</filter-class>
    <init-param>
      <param-name>deny</param-name>
      <param-value>$127.0.0.1</param-value>
    </init-param>
  </filter>

  <filter-mapping>
    <filter-name>host</filter-name>
    <url-pattern>/*</url-pattern>
  </filter-mapping>
----

Of course, the regex is invalid. BUT THE CONTEXT RELOADS!!

(In reply to comment #19)
>       <param-value>$127.0.0.1</param-value>
> Of course, the regex is invalid. BUT THE CONTEXT RELOADS!!

The regex is valid. There is nothing syntactically wrong with it. It just does not match anything (because you expect some text after the end of string).

<%= java.util.regex.Pattern.compile("$127.0.0.1") %>
compiles successfully without any exceptions.

OK, I don't understand what is happening at all.

The filter does work, exceptions are thrown, but nothing is logged with the default Tomcat log configuration, and a bad filter doesn't prevent the context from reloading!

Is that expected?

Well, I can now confirm that both the filter and valve work, however the messages I log are NOT shown at all. As to exceptions, when running the valve, they are not raised AT ALL. Error messages are shown, ie:

----
SEVERE: 1127.0.0.1: invalid address specification
----

but only for the Valve :(

Is that the expected behavior? If so, then my patch is basically ready...

Comments?

It has been rather too long since this was last looked at. Apologies for that. I'm looking at this now.

The good news is that the patch applies cleanly to trunk (9.0.x). There are some compilation issues to take care of (e.g. Comet has been removed) but nothing major.

I'm planning on starting with the NetMask and associated unit tests. I've tidied up some Checkstyle / formatting issues and I'm now looking at refactoring the unit test coverage to a) use a parameterized test and b) expand the test cases. I also want to look at the Exception messages as the current message look slightly odd and I want to switch them over to the StringManager for i18n support.

I'll look at the Valve and Filter once the netmask work is complete.

I've just committed the NetMask class and associated test case.

Can I just say "Nice code". An elegant solution and very clearly commented.

Fixed in:
- trunk for 9.0.9 onwards
- 8.5.x for 8.5.32 onwards
- 7.0.x for 7.0.89 onwards