core: Add a "b" flag to ErrorLogFormat's %a/%A for RFC 5952 bracketed IPv6 addresses (#1016)

changes-entries/65696.txt

@@ -0,0 +1,5 @@
+  *) core: Add a "b" flag to the ErrorLogFormat "%a" and "%A" directives
+     (as "%{b}a" and "%{b}A") that writes an IPv6 address in brackets,
+     "[address]:port", per RFC 5952 section 6, instead of the existing
+     "address:port" that is ambiguous for IPv6. The default "%a"/"%A"
+     output is unchanged. PR 65696 [Arturo Bernal]

docs/manual/mod/core.xml

@@ -1761,9 +1761,20 @@ ErrorLogFormat "[%t] [%l] [pid %P] %F: %E: [client %a] %M"
         <td>Underlying peer IP address and port of the connection (see the
             <module>mod_remoteip</module> module)</td></tr>
 
+    <tr><td><code>%{b}a</code></td>
+        <td>Like <code>%a</code>, but an IPv6 address is written in
+            brackets, as <code>[address]:port</code>, per
+            <a href="https://www.rfc-editor.org/rfc/rfc5952#section-6">RFC
+            5952, section 6</a>. The <code>b</code> and <code>c</code>
+            flags can be combined, as in <code>%{bc}a</code></td></tr>
+
     <tr><td><code>%A</code></td>
         <td>Local IP-address and port</td></tr>
 
+    <tr><td><code>%{b}A</code></td>
+        <td>Like <code>%A</code>, but an IPv6 address is written in
+            brackets, as with <code>%{b}a</code></td></tr>
+
     <tr><td><code>%{<em>name</em>}e</code></td>
         <td>Request environment variable <em>name</em></td></tr>
 

server/log.c

@@ -595,15 +595,31 @@ static int cpystrn(char *buf, const char *arg, int buflen)
 }
 
 
+static int log_formatted_address(const char *ip, apr_port_t port,
+                                  const char *arg, char *buf, int buflen)
+{
+    /* RFC 5952, section 6: an IPv6 address combined with a port is
+     * written as "[address]:port" to keep the port's colon from being
+     * read as part of the address. Opt-in via the 'b' arg flag, since
+     * existing ErrorLogFormat consumers may already parse the
+     * bracket-less "address:port" this directive has always produced.
+     */
+    if (arg && ap_strchr_c(arg, 'b') && ap_strchr_c(ip, ':'))
+        return apr_snprintf(buf, buflen, "[%s]:%d", ip, port);
+    return apr_snprintf(buf, buflen, "%s:%d", ip, port);
+}
+
 static int log_remote_address(const ap_errorlog_info *info, const char *arg,
                               char *buf, int buflen)
 {
-    if (info->r && !(arg && *arg == 'c'))
-        return apr_snprintf(buf, buflen, "%s:%d", info->r->useragent_ip,
-                            info->r->useragent_addr ? info->r->useragent_addr->port : 0);
+    if (info->r && !(arg && ap_strchr_c(arg, 'c')))
+        return log_formatted_address(info->r->useragent_ip,
+                                     info->r->useragent_addr ? info->r->useragent_addr->port : 0,
+                                     arg, buf, buflen);
     else if (info->c)
-        return apr_snprintf(buf, buflen, "%s:%d", info->c->client_ip,
-                            info->c->client_addr ? info->c->client_addr->port : 0);
+        return log_formatted_address(info->c->client_ip,
+                                     info->c->client_addr ? info->c->client_addr->port : 0,
+                                     arg, buf, buflen);
     else
         return 0;
 }
@@ -612,8 +628,9 @@ static int log_local_address(const ap_errorlog_info *info, const char *arg,
                              char *buf, int buflen)
 {
     if (info->c)
-        return apr_snprintf(buf, buflen, "%s:%d", info->c->local_ip,
-                            info->c->local_addr->port);
+        return log_formatted_address(info->c->local_ip,
+                                     info->c->local_addr->port,
+                                     arg, buf, buflen);
     else
         return 0;
 }

test/modules/core/test_016_errorlogformat_ipv6_bracket.py

@@ -0,0 +1,72 @@
+"""ErrorLogFormat's %a/%A print an IPv6 address and port as
+"address:port" -- e.g. "::1:34567" -- which is ambiguous: a log consumer
+cannot tell where the address ends and the port begins, since both use
+':' as a separator (BZ 65696). %{b}a/%{b}A opt into the RFC 5952,
+section 6 bracket notation instead: "[::1]:34567", matching the
+convention other HTTP libraries (e.g. Netty, OkHttp) already use for
+the same host:port ambiguity.
+"""
+import re
+import socket
+
+import pytest
+
+from pyhttpd.conf import HttpdConf
+
+
+IPV6_PORT = 5221
+
+
+def _has_ipv6():
+    try:
+        s = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
+        s.bind(('::1', 0))
+        s.close()
+        return True
+    except (socket.error, OSError):
+        return False
+
+
+@pytest.fixture(autouse=True, scope='class')
+def _class_scope(env):
+    if not _has_ipv6():
+        pytest.skip('IPv6 (::1) not available in this environment')
+
+    conf = HttpdConf(env)
+    conf.add([
+        f'Listen [::1]:{IPV6_PORT}',
+        'ErrorLogFormat "a=%a ba=%{b}a A=%A bA=%{b}A %M"',
+    ])
+    conf.start_vhost(domains=[f'ipv6.{env.http_tld}'], port=IPV6_PORT,
+                      with_ssl=False)
+    conf.add(['<Location "/missing">', 'Require all denied', '</Location>'])
+    conf.end_vhost()
+    conf.install()
+    assert env.apache_restart() == 0
+
+
+class TestErrorLogFormatIPv6Bracket:
+
+    def test_core_016_01_default_format_unchanged(self, env):
+        """%a/%A keep their existing, unbracketed "address:port" output
+        -- this must not regress for any consumer already parsing it."""
+        start = env.httpd_error_log.current_pos()
+        r = env.curl_get(f'http://[::1]:{IPV6_PORT}/missing', insecure=True)
+        assert r.response['status'] == 403
+
+        env.httpd_error_log.add_ignored_matches([r'.*AH01630.*'])
+        line = env.httpd_error_log.wait_for(
+            re.compile(r'.*a=::1:\d+ .*A=::1:\d+ .*'), start, timeout=10)
+        assert line, 'expected unbracketed a=::1:<port> and A=::1:<port>'
+
+    def test_core_016_02_b_flag_adds_brackets(self, env):
+        """%{b}a/%{b}A bracket the same IPv6 address per RFC 5952."""
+        start = env.httpd_error_log.current_pos()
+        r = env.curl_get(f'http://[::1]:{IPV6_PORT}/missing', insecure=True)
+        assert r.response['status'] == 403
+
+        env.httpd_error_log.add_ignored_matches([r'.*AH01630.*'])
+        line = env.httpd_error_log.wait_for(
+            re.compile(r'.*ba=\[::1\]:\d+ .*bA=\[::1\]:\d+ .*'), start,
+            timeout=10)
+        assert line, 'expected bracketed ba=[::1]:<port> and bA=[::1]:<port>'