Bug 51497 - Use canonical IPv6 text representation in logs
Summary: Use canonical IPv6 text representation in logs
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.x-trunk
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 17:51 UTC by Ognjen Blagojevic
Modified: 2018-05-23 14:25 UTC (History)
0 users



Attachments
Adds RFC 5952 canonical IPv6 representation to AccessLogValve. (18.13 KB, patch)
2011-07-25 14:11 UTC, Ognjen Blagojevic
Details | Diff
AccessLogValve patch (23.20 KB, patch)
2013-02-05 12:19 UTC, Ognjen Blagojevic
Details | Diff
AccessLogValve patch (22.92 KB, patch)
2013-02-06 11:39 UTC, Ognjen Blagojevic
Details | Diff
Patch to enable canonical IPv6 (22.75 KB, patch)
2014-02-11 00:07 UTC, Ognjen Blagojevic
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.