In AccessLogValve and on other places where IPv6 address is logged or printed, it would bi good if Tomcat would use canonical IPv6 format as described in RFC 5952 [1] (especially note section "3.2.2. Logging"), e.g: 1. instead of logging 2001:4000:0:5:0:0:0:66, it should log 2001:4000:0:5::66, 2. instead of logging 0:0:0:0:0:0:0:1, it should log ::1. Class Inet6Address method getHostAddress confirms to RFC recommendations, in everything except in zero groups handling. It simply prints full form with all zeroes. In Java API I don't see any method to convert it to canonical form. [1] http://tools.ietf.org/html/rfc5952
Discussion on dev@, 2011-07-11: http://tomcat.markmail.org/thread/jwysfldcsmulkytb http://marc.info/?t=131040340100002&r=1&w=2
Created attachment 27312 [details] Adds RFC 5952 canonical IPv6 representation to AccessLogValve. Here is the proposed patch: - Added IPv6Utils.java to org.apache.tomcat.util.net - Added IPv6UtilsTest.java to org.apache.tomcat.util.net - Added parameter 'canonical' to AccessLogValve, and changed logic for parameters %a (remote addr), %A (local addr), %h (remote host name), and %v (local host name) - Updated AccessLogValve docs AccessLogValve parameter 'canonical' is set to true by default. Not sure if this shoud be the case, but since Apache httpd, Linux and Windows all use canonical representation, I believe it is reasonable assumption. Other than that, in order to access 'canonical' parameter, I needed to remove static modifier from LocalServerNameElement and LocalAddressElement inner class declarations, and to replace static initialization of LocalServerNameElement with lazy initialization. Will this be an issue?
Minor nit: the tests use "assertTrue(result.equals(expected))" throughout. This will detect errors, but won't show what any detail if anything goes wrong. It would be better to use "assertEquals(expected, result)" as that shows the actual result if it differs from expected. For checking against null, use assertNull().
The private method mayBeIPv6Address counts colons, but fails to check the value. Also, there's no need to continue checking characters once an invalid char has been found.
(In reply to comment #4) > The private method mayBeIPv6Address counts colons, but fails to check the > value. Which means that abcdef:80 is treated as an IPv6 address, and results in a call to DNS to resolve the host name.
Also mayBeIPv6Address does not recognise IPv6 scoped addresses (%scope) or IPv4 mapped addresses (e.g. ::FFFF:d.d.d.d).
Sebb, I will fix problems you noticed, and upload new patch.
Created attachment 29921 [details] AccessLogValve patch (In reply to comment #3) > It would be better to use "assertEquals(expected, result)" Fixed. (In reply to comment #4) > The private method mayBeIPv6Address counts colons, > but fails to check the value. Fixed. (In reply to comment #5) > Which means that abcdef:80 is treated as an IPv6 address, > and results in a call to DNS to resolve the host name. Fixed, it is not treated as IPv6 address anymore. I changed method mayBeIPv6Address visibility to "protected", and added JUnit test cases. (In reply to comment #6) > Also mayBeIPv6Address does not recognise IPv6 scoped addresses > (%scope) or IPv4 mapped addresses (e.g. ::FFFF:d.d.d.d). Fixed. Both mayBeIPv6Address and canonize(String) support zone ID (%1, %eth0...), and IPv4-in-IPv6 (::FFFF:d.d.d.d, ::d.d.d.d, etc).
> IPv6UtilsTest extends TestCase Trunk uses junit4-style tests. Checkstyle there is configured that junit3 ones will not pass validation. (Enabling checkstyle is documented in BUILDING.txt)
Created attachment 29924 [details] AccessLogValve patch (In reply to comment #9) > Trunk uses junit4-style tests. > Checkstyle there is configured that junit3 ones will not pass validation. I ran the checkstyle and corrected all errors: trailing spaces and JUnit version. Thank you both for reviewing the patch.
Created attachment 31300 [details] Patch to enable canonical IPv6 Most of the AccessLogValve logic is moved to AbstractAccessLogValve class. Updated patch to reflect those changes.
Added with some minor modifications (including disabled by default) to 9.0.x for 9.0.9 onwards.
Added to 8.5.x for 8.5.44 onwards.