I'm not sure if my mail client mangled the message or my email provider did, but I couldn't read this except from lists.a.o so my reply may appear as a new thread in your email client if it's following thread IDs.
Christophe JAILLET wrote: > First of all, http://blog.haproxy.com/haproxy/proxy-protocol/ list > another module implementation for Apache: > https://github.com/ggrandes/apache24-modules/blob/master/mod_myfixip.c > > If anyone wants to give it a look. > > > > Anyway, a few minor comments below. > > CJ > > > Le 30/12/2016 à 15:20, drugg...@apache.org a écrit : >> Author: druggeri >> Date: Fri Dec 30 14:20:48 2016 >> New Revision: 1776575 >> >> URL:http://svn.apache.org/viewvc?rev=1776575&view=rev >> Log: >> Merge new PROXY protocol code into mod_remoteip >> >> Modified: >> httpd/httpd/trunk/docs/log-message-tags/next-number >> httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml >> httpd/httpd/trunk/modules/metadata/mod_remoteip.c >> >> Modified: httpd/httpd/trunk/docs/log-message-tags/next-number >> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1776575&r1=1776574&r2=1776575&view=diff >> ============================================================================== >> --- httpd/httpd/trunk/docs/log-message-tags/next-number (original) >> +++ httpd/httpd/trunk/docs/log-message-tags/next-number Fri Dec 30 14:20:48 >> 2016 >> @@ -1 +1 @@ >> -3491 >> +3514 >> >> Modified: httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml >> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml?rev=1776575&r1=1776574&r2=1776575&view=diff >> ============================================================================== >> --- httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml (original) >> +++ httpd/httpd/trunk/docs/manual/mod/mod_remoteip.xml Fri Dec 30 14:20:48 >> 2016 >> @@ -42,6 +42,12 @@ via the request headers. >> with the useragent IP address reported in the request header configured >> with the <directive module="mod_remoteip">RemoteIPHeader</directive> >> directive.</p> >> >> + <p>Additionally, this module implements the server side of >> + HAProxy's >> + <a href="http://blog.haproxy.com/haproxy/proxy-protocol/";>Proxy >> Protocol</a> when >> + using the <directive >> module="mod_remoteip">RemoteIPProxyProtocolEnable</directive> >> + directive.</p> >> + >> <p>Once replaced as instructed, this overridden useragent IP address is >> then used for the <module>mod_authz_host</module> >> <directive module="mod_authz_core" name="Require">Require >> ip</directive> >> @@ -59,6 +65,7 @@ via the request headers. >> <seealso><module>mod_authz_host</module></seealso> >> <seealso><module>mod_status</module></seealso> >> <seealso><module>mod_log_config</module></seealso> >> +<seealso><a >> href="http://www.haproxy.org/download/1.5/doc/proxy-protocol.txt";>Proxy >> Protocol Spec</a></seealso> >> >> <section id="processing"><title>Remote IP Processing</title> >> >> @@ -214,6 +221,70 @@ RemoteIPProxiesHeader X-Forwarded-By >> </usage> >> </directivesynopsis> >> >> +<directivesynopsis> >> +<name>RemoteIPProxyProtocol</name> > s/RemoteIPProxyProtocol/RemoteIPProxyProtocolEnable/ Right - this was addressed in a subsequent commit after discussing the name. It's now RemoteIPProxyProtocol > >> +<description>Enable, optionally enable or disable the proxy protocol >> handling</description> >> +<syntax>ProxyProtocol On|Optional|Off</syntax> >> +<contextlist><context>server config</context><context>virtual host</context> >> +</contextlist> > Compatibility note missing. Added - thanks! > >> .... >> >> Modified: httpd/httpd/trunk/modules/metadata/mod_remoteip.c >> URL:http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_remoteip.c?rev=1776575&r1=1776574&r2=1776575&view=diff >> ============================================================================== >> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original) >> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 >> 2016 >> @@ -12,15 +12,20 @@ >> * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. >> * See the License for the specific language governing permissions and >> * limitations under the License. >> + * >> + * The majority of the input filter code for PROXY protocol support is >> + * Copyright 2014 Cloudzilla Inc. >> */ >> >> #include "ap_config.h" >> #include "ap_mmn.h" >> +#include "ap_listen.h" >> #include "httpd.h" >> #include "http_config.h" >> #include "http_connection.h" >> #include "http_protocol.h" >> #include "http_log.h" >> +#include "http_main.h" >> #include "apr_strings.h" >> #include "apr_lib.h" >> #define APR_WANT_BYTEFUNC >> @@ -36,6 +41,12 @@ typedef struct { >> void *internal; >> } remoteip_proxymatch_t; >> >> +typedef struct remoteip_addr_info { >> + struct remoteip_addr_info *next; >> + apr_sockaddr_t *addr; >> + server_rec *source; >> +} remoteip_addr_info; >> + >> typedef struct { >> /** The header to retrieve a proxy-via IP list */ >> const char *header_name; >> @@ -48,6 +59,17 @@ typedef struct { >> * with the most commonly encountered listed first >> */ >> apr_array_header_t *proxymatch_ip; >> + >> + remoteip_addr_info *proxy_protocol_enabled; >> + remoteip_addr_info *proxy_protocol_optional; >> + remoteip_addr_info *proxy_protocol_disabled; >> + >> + /** A flag indicating whether or not proxyprotocol >> + * is optoinal for this specific server >> + */ >> + int pp_optional; >> + >> + apr_pool_t *pool; > Do we really need to keep the config pool here? if badly used", wouldn't > it generate a kind of leak? See response below to your follow up question > >> } remoteip_config_t; >> >> typedef struct { >> @@ -59,12 +81,92 @@ typedef struct { >> const char *proxied_remote; >> } remoteip_req_t; >> >> +/* For PROXY protocol processing */ >> +static const char *remoteip_filter_name = "REMOTEIP_INPUT"; >> + >> +typedef struct { >> + char line[108]; >> +} proxy_v1; >> + >> +typedef union { >> + struct { /* for TCP/UDP over IPv4, len = 12 */ >> + uint32_t src_addr; >> + uint32_t dst_addr; >> + uint16_t src_port; >> + uint16_t dst_port; >> + } ip4; >> + struct { /* for TCP/UDP over IPv6, len = 36 */ >> + uint8_t src_addr[16]; >> + uint8_t dst_addr[16]; >> + uint16_t src_port; >> + uint16_t dst_port; >> + } ip6; >> + struct { /* for AF_UNIX sockets, len = 216 */ >> + uint8_t src_addr[108]; >> + uint8_t dst_addr[108]; >> + } unx; >> +} proxy_v2_addr; >> + >> +typedef struct { >> + uint8_t sig[12]; /* hex 0D 0A 0D 0A 00 0D 0A 51 55 49 54 0A */ >> + uint8_t ver_cmd; /* protocol version and command */ >> + uint8_t fam; /* protocol family and address */ >> + uint16_t len; /* number of following bytes part of the header */ >> + proxy_v2_addr addr; >> +} proxy_v2; >> + >> +typedef union { >> + proxy_v1 v1; >> + proxy_v2 v2; >> +} proxy_header; >> + >> +static const char v2sig[12] = >> "\x0D\x0A\x0D\x0A\x00\x0D\x0A\x51\x55\x49\x54\x0A"; >> +#define MIN_V1_HDR_LEN 15 >> +#define MIN_V2_HDR_LEN 16 >> +#define MIN_HDR_LEN MIN_V1_HDR_LEN >> + >> +/* XXX: Unsure if this is needed if v6 support is not available on >> + this platform */ >> +#ifndef INET6_ADDRSTRLEN >> +#define INET6_ADDRSTRLEN 46 >> +#endif >> + >> +typedef struct { >> + char header[sizeof(proxy_header)]; >> + apr_size_t rcvd; >> + apr_size_t need; >> + int version; >> + ap_input_mode_t mode; >> + apr_bucket_brigade *bb; >> + int done; >> +} remoteip_filter_context; >> + >> +/** Holds the resolved proxy info for this connection and any addition >> + configurable parameters >> +*/ >> +typedef struct { >> + /** The parsed client address in native format */ >> + apr_sockaddr_t *client_addr; >> + /** Character representation of the client */ >> + char *client_ip; >> + /** Flag indicating that the PROXY header may be omitted on this >> + connection (do not abort if it is missing). */ >> + int proxy_protocol_optional; >> +} remoteip_conn_config_t; >> + >> +typedef enum { HDR_DONE, HDR_ERROR, HDR_MISSING, HDR_NEED_MORE } >> remoteip_parse_status_t; >> + >> static void *create_remoteip_server_config(apr_pool_t *p, server_rec *s) >> { >> remoteip_config_t *config = apr_pcalloc(p, sizeof *config); >> /* config->header_name = NULL; >> * config->proxies_header_name = NULL; >> */ >> + config->proxy_protocol_enabled = NULL; >> + config->proxy_protocol_optional = NULL; >> + config->proxy_protocol_disabled = NULL; >> + config->pp_optional = 0; > Useless init because of apr_pcalloc. Should be consistent with > header_name and proxies_header_name. All commented or all NULL/0 > assigned or dropped completely. NP - moved to comments in latest commit > >> + config->pool = p; >> return config; >> } >> >> @@ -85,6 +187,9 @@ static void *merge_remoteip_server_confi >> config->proxymatch_ip = server->proxymatch_ip >> ? server->proxymatch_ip >> : global->proxymatch_ip; >> + config->pp_optional = server->pp_optional >> + ? server->pp_optional >> + : global->pp_optional; >> return config; >> } >> >> @@ -215,13 +320,184 @@ static const char *proxylist_read(cmd_pa >> return NULL; >> } >> >> +/** Similar apr_sockaddr_equal, except that it compares ports too. */ >> +static int remoteip_sockaddr_equal(apr_sockaddr_t *addr1, apr_sockaddr_t >> *addr2) >> +{ >> + return (addr1->port == addr2->port && apr_sockaddr_equal(addr1, addr2)); >> +} >> + >> +/** Similar remoteip_sockaddr_equal, except that it handles wildcard >> addresses >> + * and ports too. >> + */ >> +static int remoteip_sockaddr_compat(apr_sockaddr_t *addr1, apr_sockaddr_t >> *addr2) >> +{ >> + /* test exact address equality */ >> + if (apr_sockaddr_equal(addr1, addr2) && >> + (addr1->port == addr2->port || addr1->port == 0 || addr2->port == >> 0)) { >> + return 1; >> + } >> + >> + /* test address wildcards */ >> + if (apr_sockaddr_is_wildcard(addr1) && >> + (addr1->port == 0 || addr1->port == addr2->port)) { >> + return 1; >> + } >> + >> + if (apr_sockaddr_is_wildcard(addr2) && >> + (addr2->port == 0 || addr2->port == addr1->port)) { >> + return 1; >> + } >> + >> + return 0; >> +} >> + >> +static int remoteip_addr_in_list(remoteip_addr_info *list, apr_sockaddr_t >> *addr) >> +{ >> + for (; list; list = list->next) { >> + if (remoteip_sockaddr_compat(list->addr, addr)) { >> + return 1; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static void remoteip_warn_enable_conflict(remoteip_addr_info *prev, >> server_rec *new, const char* arg) >> +{ >> + char buf[INET6_ADDRSTRLEN]; >> + >> + apr_sockaddr_ip_getbuf(buf, sizeof(buf), prev->addr); >> + >> + ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, new, APLOGNO(03491) >> + "RemoteIPProxyProtocolEnable: previous setting for %s:%hu >> from virtual " >> + "host {%s:%hu in %s} is being overriden by virtual host " >> + "{%s:%hu in %s}; new setting is '%s'", >> + buf, prev->addr->port, prev->source->server_hostname, >> + prev->source->addrs->host_port, prev->source->defn_name, >> + new->server_hostname, new->addrs->host_port, >> new->defn_name, >> + arg); >> +} >> + >> +static const char *remoteip_enable_proxy_protocol(cmd_parms *cmd, void >> *config, >> + const char *arg) >> +{ >> + remoteip_config_t *global_conf; >> + remoteip_config_t *server_conf; >> + server_addr_rec *addr; >> + remoteip_addr_info **add; >> + int list_len = 2; >> + remoteip_addr_info **rem_list[list_len]; >> + remoteip_addr_info *list; >> + int i; >> + >> + global_conf = ap_get_module_config(ap_server_conf->module_config, >> + &remoteip_module); >> + server_conf = ap_get_module_config(cmd->server->module_config, >> + &remoteip_module); >> + >> + if (strcasecmp(arg, "On") == 0) { >> + add = &global_conf->proxy_protocol_enabled; >> + rem_list[0] = &global_conf->proxy_protocol_optional; >> + rem_list[1] = &global_conf->proxy_protocol_disabled; >> + } >> + else if (strcasecmp(arg, "Optional") == 0) { >> + add = &global_conf->proxy_protocol_optional; >> + rem_list[0] = &global_conf->proxy_protocol_enabled; >> + rem_list[1] = &global_conf->proxy_protocol_disabled; >> + server_conf->pp_optional = 1; >> + } >> + else if (strcasecmp(arg, "Off") == 0 ) { >> + add = &global_conf->proxy_protocol_disabled; >> + rem_list[0] = &global_conf->proxy_protocol_enabled; >> + rem_list[1] = &global_conf->proxy_protocol_optional; >> + } >> + else { >> + return apr_pstrcat(cmd->pool, "Unrecognized option for >> ProxyProtocolEnable `%s'", arg, NULL); > s/ProxyProtocolEnable/RemoteIPProxyProtocolEnable/ > or cmd->cmd->name to be safe. I like that even better - updated to the safest case > >> + } >> + >> + for (addr = cmd->server->addrs; addr; addr = addr->next) { >> + /* remove address from other lists */ >> + for (i = 0; i < list_len ; i++) { >> + remoteip_addr_info **rem = rem_list[i]; >> + if (*rem) { >> + if (remoteip_sockaddr_equal((*rem)->addr, addr->host_addr)) >> { >> + remoteip_warn_enable_conflict(*rem, cmd->server, arg); >> + *rem = (*rem)->next; >> + } >> + else { >> + for (list = *rem; list->next; list = list->next) { >> + if (remoteip_sockaddr_equal(list->next->addr, >> addr->host_addr)) { >> + remoteip_warn_enable_conflict(list->next, >> cmd->server, arg); >> + list->next = list->next->next; >> + break; >> + } >> + } >> + } >> + } >> + } >> + >> + /* add address to desired list */ >> + if (!remoteip_addr_in_list(*add, addr->host_addr)) { >> + remoteip_addr_info *info = apr_palloc(global_conf->pool, >> sizeof(*info)); > Could cmd->pool be used here, instead? This came from the original authors of the code, but I think it's correct. This is the only place remoteip_config_t->pool is allocated into. A collection of all enabled, disabled and optional remoteip_addr_info structs is kept and examined pre-connection to determine if the filter should be inserted for the connection. Since the server is not known pre-connection, this must be stored in the global server. The lifetime of cmd->pool would prevent using it here. > >> . . . >> static const command_rec remoteip_cmds[] = >> { >> AP_INIT_TAKE1("RemoteIPHeader", header_name_set, NULL, RSRC_CONF, >> @@ -450,11 +1211,21 @@ static const command_rec remoteip_cmds[] >> RSRC_CONF | EXEC_ON_READ, >> "The filename to read the list of internal proxies, " >> "see the RemoteIPInternalProxy directive"), >> + AP_INIT_TAKE1("RemoteIPProxyProtocolEnable", >> remoteip_enable_proxy_protocol, NULL, >> + RSRC_CONF, "Enable proxy-protocol handling (`on', >> `off')"), > `optional' is missing Fixed - thanks! > >> { NULL } >> }; >> >> static void register_hooks(apr_pool_t *p) >> { >> + /* mod_ssl is CONNECTION + 5, so we want something higher (earlier); >> + * mod_reqtimeout is CONNECTION + 8, so we want something lower (later) >> */ >> + ap_register_input_filter(remoteip_filter_name, remoteip_input_filter, >> NULL, >> + AP_FTYPE_CONNECTION + 7); >> + >> + ap_hook_pre_config(remoteip_hook_pre_config, NULL, NULL, >> APR_HOOK_MIDDLE); >> + ap_hook_post_config(remoteip_hook_post_config, NULL, NULL, >> APR_HOOK_MIDDLE); >> + ap_hook_pre_connection(remoteip_hook_pre_connection, NULL, NULL, >> APR_HOOK_MIDDLE); >> ap_hook_post_read_request(remoteip_modify_request, NULL, NULL, >> APR_HOOK_FIRST); >> } >> -- Daniel Ruggeri
