Bug 51497

Summary:	Use canonical IPv6 text representation in logs
Product:	Tomcat 8	Reporter:	Ognjen Blagojevic <ognjen.d.blagojevic>
Component:	Catalina	Assignee:	Tomcat Developers Mailing List <dev>
Status:	RESOLVED FIXED
Severity:	enhancement
Priority:	P2
Version:	8.0.x-trunk
Target Milestone:	----
Hardware:	All
OS:	All
Attachments:	Adds RFC 5952 canonical IPv6 representation to AccessLogValve. AccessLogValve patch AccessLogValve patch Patch to enable canonical IPv6

Description Ognjen Blagojevic 2011-07-11 17:51:03 UTC

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

Comment 1 Konstantin Kolinko 2011-07-11 19:32:48 UTC

Discussion on dev@, 2011-07-11:
http://tomcat.markmail.org/thread/jwysfldcsmulkytb
http://marc.info/?t=131040340100002&r=1&w=2

Comment 2 Ognjen Blagojevic 2011-07-25 14:11:54 UTC

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?

Comment 3 Sebb 2013-02-04 12:21:52 UTC

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().

Comment 4 Sebb 2013-02-04 12:37:23 UTC

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.

Comment 5 Sebb 2013-02-04 16:11:13 UTC

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

Comment 6 Sebb 2013-02-04 16:49:07 UTC

Also mayBeIPv6Address does not recognise IPv6 scoped addresses (%scope) or IPv4 mapped addresses (e.g. ::FFFF:d.d.d.d).

Comment 7 Ognjen Blagojevic 2013-02-04 19:00:26 UTC

Sebb,

I will fix problems you noticed, and upload new patch.

Comment 8 Ognjen Blagojevic 2013-02-05 12:19:00 UTC

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

Comment 9 Konstantin Kolinko 2013-02-05 23:28:28 UTC

> 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)

Comment 10 Ognjen Blagojevic 2013-02-06 11:39:25 UTC

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.

Comment 11 Ognjen Blagojevic 2014-02-11 00:07:11 UTC

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.

Comment 12 Mark Thomas 2018-05-23 14:25:41 UTC

Added with some minor modifications (including disabled by default) to 9.0.x for 9.0.9 onwards.

Comment 13 Mark Thomas 2019-07-30 21:38:21 UTC

Added to 8.5.x for 8.5.44 onwards.