You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2017/01/10 11:39:56 UTC
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
On 12/30/2016 03:20 PM, druggeri@apache.org wrote:
> 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
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 2016
> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
> return OK;
> }
>
> +static int remoteip_is_server_port(apr_port_t port)
> +{
> + ap_listen_rec *lr;
> +
> + for (lr = ap_listeners; lr; lr = lr->next) {
> + if (lr->bind_addr && lr->bind_addr->port == port) {
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Human readable format:
> + * PROXY {TCP4|TCP6|UNKNOWN} <client-ip-addr> <dest-ip-addr> <client-port> <dest-port><CR><LF>
> + */
> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
> + remoteip_conn_config_t *conn_conf,
> + proxy_header *hdr, apr_size_t len,
> + apr_size_t *hdr_len)
> +{
> + char *end, *word, *host, *valid_addr_chars, *saveptr;
> + char buf[sizeof(hdr->v1.line)];
> + apr_port_t port;
> + apr_status_t ret;
> + apr_int32_t family;
> +
> +#define GET_NEXT_WORD(field) \
> + word = apr_strtok(NULL, " ", &saveptr); \
> + if (!word) { \
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
> + "RemoteIPProxyProtocol: no " field " found in header '%s'", \
> + hdr->v1.line); \
> + return HDR_ERROR; \
> + }
> +
> + end = memchr(hdr->v1.line, '\r', len - 1);
> + if (!end || end[1] != '\n') {
> + return HDR_NEED_MORE; /* partial or invalid header */
> + }
> +
> + *end = '\0';
> + *hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
> +
> + /* parse in separate buffer so have the original for error messages */
> + strcpy(buf, hdr->v1.line);
> +
> + apr_strtok(buf, " ", &saveptr);
> +
> + /* parse family */
> + GET_NEXT_WORD("family")
> + if (strcmp(word, "UNKNOWN") == 0) {
> + conn_conf->client_addr = c->client_addr;
> + conn_conf->client_ip = c->client_ip;
> + return HDR_DONE;
> + }
> + else if (strcmp(word, "TCP4") == 0) {
> + family = APR_INET;
> + valid_addr_chars = "0123456789.";
> + }
> + else if (strcmp(word, "TCP6") == 0) {
> +#if APR_HAVE_IPV6
> + family = APR_INET6;
> + valid_addr_chars = "0123456789abcdefABCDEF:";
> +#else
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
> + "RemoteIPProxyProtocol: Unable to parse v6 address - APR is not compiled with IPv6 support",
> + word, hdr->v1.line);
> + return HDR_ERROR;
> +#endif
> + }
> + else {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
> + "RemoteIPProxyProtocol: unknown family '%s' in header '%s'",
> + word, hdr->v1.line);
> + return HDR_ERROR;
> + }
> +
> + /* parse client-addr */
> + GET_NEXT_WORD("client-address")
> +
> + if (strspn(word, valid_addr_chars) != strlen(word)) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
> + "RemoteIPProxyProtocol: invalid client-address '%s' found in "
> + "header '%s'", word, hdr->v1.line);
> + return HDR_ERROR;
> + }
> +
> + host = word;
> +
> + /* parse dest-addr */
> + GET_NEXT_WORD("destination-address")
> +
> + /* parse client-port */
> + GET_NEXT_WORD("client-port")
> + if (sscanf(word, "%hu", &port) != 1) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
> + "RemoteIPProxyProtocol: error parsing port '%s' in header '%s'",
> + word, hdr->v1.line);
> + return HDR_ERROR;
> + }
> +
> + /* parse dest-port */
> + /* GET_NEXT_WORD("destination-port") - no-op since we don't care about it */
> +
> + /* create a socketaddr from the info */
> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, host, family, port, 0,
> + c->pool);
> + if (ret != APR_SUCCESS) {
> + conn_conf->client_addr = NULL;
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03502)
> + "RemoteIPProxyProtocol: error converting family '%d', host '%s',"
> + " and port '%hu' to sockaddr; header was '%s'",
> + family, host, port, hdr->v1.line);
> + return HDR_ERROR;
> + }
> +
> + conn_conf->client_ip = apr_pstrdup(c->pool, host);
> +
> + return HDR_DONE;
> +}
> +
> +/** Add our filter to the connection if it is requested
> + */
> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
> +{
> + remoteip_config_t *conf;
> + remoteip_conn_config_t *conn_conf;
> + int optional;
> +
> + conf = ap_get_module_config(ap_server_conf->module_config,
> + &remoteip_module);
> +
> + /* Used twice - do the check only once */
> + optional = remoteip_addr_in_list(conf->proxy_protocol_optional, c->local_addr);
> +
> + /* check if we're enabled for this connection */
> + if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c->local_addr)
> + && !optional )
> + || remoteip_addr_in_list(conf->proxy_protocol_disabled, c->local_addr)) {
> + return DECLINED;
> + }
> +
> + /* mod_proxy creates outgoing connections - we don't want those */
> + if (!remoteip_is_server_port(c->local_addr->port)) {
> + return DECLINED;
> + }
Why is the c->local_addr->port set to a port we are listening on in case of proxy connections?
> +
> + /* add our filter */
> + if (!ap_add_input_filter(remoteip_filter_name, NULL, NULL, c)) {
> + /* XXX: Shouldn't this WARN in log? */
> + return DECLINED;
> + }
> +
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(03503)
> + "RemoteIPProxyProtocol: enabled on connection to %s:%hu",
> + c->local_ip, c->local_addr->port);
> +
> + /* this holds the resolved proxy info for this connection */
> + conn_conf = apr_pcalloc(c->pool, sizeof(*conn_conf));
> +
> + /* Propagate the optional flag so the connection handler knows not to
> + abort if the header is mising. NOTE: This means we must check after
> + we read the request that the header was NOT optional, too.
> + */
> + conn_conf->proxy_protocol_optional = optional;
> +
> + ap_set_module_config(c->conn_config, &remoteip_module, conn_conf);
> +
> + return OK;
> +}
> +
> +/* Binary format:
> + * <sig><cmd><proto><addr-len><addr>
> + * sig = \x0D \x0A \x0D \x0A \x00 \x0D \x0A \x51 \x55 \x49 \x54 \x0A
> + * cmd = <4-bits-version><4-bits-command>
> + * 4-bits-version = \x02
> + * 4-bits-command = {\x00|\x01} (\x00 = LOCAL: discard con info; \x01 = PROXY)
> + * proto = <4-bits-family><4-bits-protocol>
> + * 4-bits-family = {\x00|\x01|\x02|\x03} (AF_UNSPEC, AF_INET, AF_INET6, AF_UNIX)
> + * 4-bits-protocol = {\x00|\x01|\x02} (UNSPEC, STREAM, DGRAM)
> + */
> +static remoteip_parse_status_t remoteip_process_v2_header(conn_rec *c,
> + remoteip_conn_config_t *conn_conf,
> + proxy_header *hdr)
> +{
> + apr_status_t ret;
> +
> + switch (hdr->v2.ver_cmd & 0xF) {
> + case 0x01: /* PROXY command */
> + switch (hdr->v2.fam) {
> + case 0x11: /* TCPv4 */
> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
> + APR_INET,
> + ntohs(hdr->v2.addr.ip4.src_port),
> + 0, c->pool);
> + if (ret != APR_SUCCESS) {
> + conn_conf->client_addr = NULL;
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03504)
> + "RemoteIPPProxyProtocol: error creating sockaddr");
> + return HDR_ERROR;
> + }
> +
> + conn_conf->client_addr->sa.sin.sin_addr.s_addr =
> + hdr->v2.addr.ip4.src_addr;
> + break;
> +
> + case 0x21: /* TCPv6 */
> +#if APR_HAVE_IPV6
> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
> + APR_INET6,
> + ntohs(hdr->v2.addr.ip6.src_port),
> + 0, c->pool);
> + if (ret != APR_SUCCESS) {
> + conn_conf->client_addr = NULL;
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03505)
> + "RemoteIPProxyProtocol: error creating sockaddr");
> + return HDR_ERROR;
> + }
> + memcpy(&conn_conf->client_addr->sa.sin6.sin6_addr.s6_addr,
> + hdr->v2.addr.ip6.src_addr, 16);
> + break;
> +#else
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03506)
> + "RemoteIPProxyProtocol: APR is not compiled with IPv6 support");
> + return HDR_ERROR;
> +#endif
> + default:
> + /* unsupported protocol, keep local connection address */
> + return HDR_DONE;
> + }
> + break; /* we got a sockaddr now */
> +
> + case 0x00: /* LOCAL command */
> + /* keep local connection address for LOCAL */
> + return HDR_DONE;
> +
> + default:
> + /* not a supported command */
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03507)
> + "RemoteIPProxyProtocol: unsupported command %.2hx",
> + hdr->v2.ver_cmd);
> + return HDR_ERROR;
> + }
> +
> + /* got address - compute the client_ip from it */
> + ret = apr_sockaddr_ip_get(&conn_conf->client_ip, conn_conf->client_addr);
> + if (ret != APR_SUCCESS) {
> + conn_conf->client_addr = NULL;
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03508)
> + "RemoteIPProxyProtocol: error converting address to string");
> + return HDR_ERROR;
> + }
> +
> + return HDR_DONE;
> +}
> +
> +/** Determine if this is a v1 or v2 PROXY header.
> + */
> +static int remoteip_determine_version(conn_rec *c, const char *ptr)
> +{
> + proxy_header *hdr = (proxy_header *) ptr;
> +
> + /* assert len >= 14 */
> +
> + if (memcmp(&hdr->v2, v2sig, sizeof(v2sig)) == 0 &&
> + (hdr->v2.ver_cmd & 0xF0) == 0x20) {
> + return 2;
> + }
> + else if (memcmp(hdr->v1.line, "PROXY ", 6) == 0) {
> + return 1;
> + }
> + else {
> + return -1;
> + }
> +}
> +
> +/* Capture the first bytes on the protocol and parse the proxy protocol header.
> + * Removes itself when the header is complete.
> + */
> +static apr_status_t remoteip_input_filter(ap_filter_t *f,
> + apr_bucket_brigade *bb_out,
> + ap_input_mode_t mode,
> + apr_read_type_e block,
> + apr_off_t readbytes)
> +{
> + apr_status_t ret;
> + remoteip_filter_context *ctx = f->ctx;
> + remoteip_conn_config_t *conn_conf;
> + apr_bucket *b;
> + remoteip_parse_status_t psts;
> + const char *ptr;
> + apr_size_t len;
> +
> + if (f->c->aborted) {
> + return APR_ECONNABORTED;
> + }
> +
> + /* allocate/retrieve the context that holds our header */
> + if (!ctx) {
> + ctx = f->ctx = apr_palloc(f->c->pool, sizeof(*ctx));
> + ctx->rcvd = 0;
> + ctx->need = MIN_HDR_LEN;
> + ctx->version = 0;
> + ctx->mode = AP_MODE_READBYTES;
> + ctx->bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
> + ctx->done = 0;
> + }
> +
> + if (ctx->done) {
> + /* Note: because we're a connection filter we can't remove ourselves
> + * when we're done, so we have to stay in the chain and just go into
> + * passthrough mode.
> + */
> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
> + }
> +
> + conn_conf = ap_get_module_config(f->c->conn_config, &remoteip_module);
> +
> + /* try to read a header's worth of data */
> + while (!ctx->done) {
> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
> + ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
> + ctx->need - ctx->rcvd);
> + if (ret != APR_SUCCESS) {
> + return ret;
> + }
> + }
> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
What about the case of an non blocking read where the upstream filter returns an empty brigade
and APR_SUCCESS. This is equal to returning EAGAIN.
> + return APR_EOF;
> + }
> +
> + while (!ctx->done && !APR_BRIGADE_EMPTY(ctx->bb)) {
> + b = APR_BRIGADE_FIRST(ctx->bb);
> +
> + ret = apr_bucket_read(b, &ptr, &len, block);
> + if (APR_STATUS_IS_EAGAIN(ret) && block == APR_NONBLOCK_READ) {
> + return APR_SUCCESS;
> + }
> + if (ret != APR_SUCCESS) {
> + return ret;
> + }
> +
> + memcpy(ctx->header + ctx->rcvd, ptr, len);
> + ctx->rcvd += len;
> +
> + /* Remove instead of delete - we may put this bucket
> + back into bb_out if the header was optional and we
> + pass down the chain */
> + APR_BUCKET_REMOVE(b);
> + psts = HDR_NEED_MORE;
> +
> + if (ctx->version == 0) {
> + /* reading initial chunk */
> + if (ctx->rcvd >= MIN_HDR_LEN) {
> + ctx->version = remoteip_determine_version(f->c, ctx->header);
> + if (ctx->version < 0) {
> + psts = HDR_MISSING;
> + }
> + else if (ctx->version == 1) {
> + ctx->mode = AP_MODE_GETLINE;
> + ctx->need = sizeof(proxy_v1);
> + }
> + else if (ctx->version == 2) {
> + ctx->need = MIN_V2_HDR_LEN;
> + }
> + }
> + }
> + else if (ctx->version == 1) {
> + psts = remoteip_process_v1_header(f->c, conn_conf,
> + (proxy_header *) ctx->header,
> + ctx->rcvd, &ctx->need);
> + }
> + else if (ctx->version == 2) {
> + if (ctx->rcvd >= MIN_V2_HDR_LEN) {
> + ctx->need = MIN_V2_HDR_LEN +
> + ntohs(((proxy_header *) ctx->header)->v2.len);
> + }
> + if (ctx->rcvd >= ctx->need) {
> + psts = remoteip_process_v2_header(f->c, conn_conf,
> + (proxy_header *) ctx->header);
> + }
> + }
> + else {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03509)
> + "RemoteIPProxyProtocol: internal error: unknown version "
> + "%d", ctx->version);
> + f->c->aborted = 1;
> + apr_bucket_delete(b);
> + apr_brigade_destroy(ctx->bb);
> + return APR_ECONNABORTED;
> + }
> +
> + switch (psts) {
> + case HDR_MISSING:
> + if (conn_conf->proxy_protocol_optional) {
> + /* Same as DONE, but don't delete the bucket. Rather, put it
> + back into the brigade and move the request along the stack */
> + ctx->done = 1;
> + APR_BRIGADE_INSERT_HEAD(bb_out, b);
See below. We need to restore all buckets. What if the original read was speculative?
> + return ap_pass_brigade(f->next, ctx->bb);
Why using ap_pass_brigade on f->next? We are an input filter and f->next is our upstream input filter.
> + }
> + else {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03510)
> + "RemoteIPProxyProtocol: no valid PROXY header found");
> + /* fall through to error case */
> + }
> + case HDR_ERROR:
> + f->c->aborted = 1;
> + apr_bucket_delete(b);
> + apr_brigade_destroy(ctx->bb);
> + return APR_ECONNABORTED;
> +
> + case HDR_DONE:
> + apr_bucket_delete(b);
> + ctx->done = 1;
> + break;
> +
> + case HDR_NEED_MORE:
> + /* It is safe to delete this bucket if we get here since we know
> + that we are definitely processing a header (we've read enough to
> + know if the signatures exist on the line) */
No. We might not even received MIN_HDR_LEN data so far and thus did not check for the signature
so far. We need to safe all the buckets that we might need to restore in a separate bucket brigade,
that we need to be set aside before doing the next ap_get_brigade to avoid killing transient buckets.
> + apr_bucket_delete(b);
> + break;
> + }
> + }
> + }
> +
> + /* we only get here when done == 1 */
> + if (psts == HDR_DONE) {
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03511)
> + "RemoteIPProxyProtocol: received valid PROXY header: %s:%hu",
> + conn_conf->client_ip, conn_conf->client_addr->port);
> + }
> + else {
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03512)
> + "RemoteIPProxyProtocol: PROXY header was missing");
> + }
> +
> + if (ctx->rcvd > ctx->need || !APR_BRIGADE_EMPTY(ctx->bb)) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03513)
> + "RemoteIPProxyProtocol: internal error: have data left over; "
> + " need=%lu, rcvd=%lu, brigade-empty=%d", ctx->need,
> + ctx->rcvd, APR_BRIGADE_EMPTY(ctx->bb));
> + f->c->aborted = 1;
> + apr_brigade_destroy(ctx->bb);
> + return APR_ECONNABORTED;
> + }
> +
> + /* clean up */
> + apr_brigade_destroy(ctx->bb);
> + ctx->bb = NULL;
> +
> + /* now do the real read for the upper layer */
> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
> +}
> +
> static const command_rec remoteip_cmds[] =
> {
> AP_INIT_TAKE1("RemoteIPHeader", header_name_set, NULL, RSRC_CONF,
Regards
R�diger
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 01/28/2017 06:59 PM, Daniel Ruggeri wrote:
> Ruediger Pluem wrote:
>> + /* try to read a header's worth of data */
>> + while (!ctx->done) {
>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
>> + ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
>> + ctx->need - ctx->rcvd);
>> + if (ret != APR_SUCCESS) {
>> + return ret;
>> + }
>> + }
>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
> What about the case of an non blocking read where the upstream filter
> returns an empty brigade
> and APR_SUCCESS. This is equal to returning EAGAIN.
>
>> + return APR_EOF;
>> + }
>
> Coming back to this one after correcting the setaside stuff... Is this
> what you have in mind or should we actually return APR_EAGAIN?
>
> return block == APR_NONBLOCK_READ ? APR_SUCCESS : APR_EOF;
>
IMHO you will hit the APR_BRIGADE_EMPTY(ctx->bb) with APR_SUCCESS returned case only
in the non blocking case. So it would be sufficient to replace
return APR_EOF
with
return APR_SUCCESS.
You proposal is a little bit more fail safe and hence probably a good idea.
Furthermore I think you could move the second check for an empty brigade inside
the block checking it firstly (at the end).
Regards
R�diger
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Daniel Ruggeri <DR...@primary.net>.
Ruediger Pluem wrote:
> + /* try to read a header's worth of data */
> + while (!ctx->done) {
> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
> + ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
> + ctx->need - ctx->rcvd);
> + if (ret != APR_SUCCESS) {
> + return ret;
> + }
> + }
> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
What about the case of an non blocking read where the upstream filter
returns an empty brigade
and APR_SUCCESS. This is equal to returning EAGAIN.
> + return APR_EOF;
> + }
Coming back to this one after correcting the setaside stuff... Is this
what you have in mind or should we actually return APR_EAGAIN?
return block == APR_NONBLOCK_READ ? APR_SUCCESS : APR_EOF;
--
Daniel Ruggeri
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Daniel Ruggeri <DR...@primary.net>.
To clarify, the issues with handling of the buckets and the fact that
buckets are not being set aside came from the original code. The
question about what to do regarding f->next versus passing the data
along some other way is the only one that came from the code I added to
make the header optional.
Being honest, I don't fully grok all the details of the filter stack and
what kinds of buckets to expect emanating from core as the request for
data propagates up through the input filters, but what Rudiger has
pointed out seems like a structural problem with the module that could
bite users under certain circumstances. What I'm particularly unclear
about is what those circumstances would be.
I'll try to reply to the other thread to provide more clarity.
--
Daniel Ruggeri
On 1/24/2017 8:36 AM, Jim Jagielski wrote:
> ++1. I know that Daniel is out of pocket for a little bit so I'l
> give it a coupla more days before I "restore" to the original filter
> code...
>> On Jan 24, 2017, at 3:46 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>>
>> On 01/17/2017 02:48 PM, Jim Jagielski wrote:
>>>> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri <DR...@primary.net> wrote:
>>>>
>>>> For the most part, yes except the portions that make the header presence
>>>> optional (the HDR_MISSING case). Those were added as it came into the
>>>> code base to handle a use case I was working on. I've added some
>>>> comments inline since I won't have time to poke around myself for a
>>>> while yet.
>>>>
>>>>
>>>> For convenience, here's a link to the original code
>>>>
>>>> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
>>>>
>>> Would it make sense to have the "stable" version available
>>> for backport, and keep in the WIP in trunk?
>>>
>> This would be an option, but apart from this I would like to see the WIP in trunk
>> somehow fixed. Otherwise it is a perfect candidate for falling through the cracks
>> and giving yet another surprise once we branch "whatever we name it" from trunk.
>> IMHO easier to fix that now or even revert that part for the time being while people
>> remember what is going on then later digging through it while hitting the issues.
>>
>> Regards
>>
>> R�diger
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
++1. I know that Daniel is out of pocket for a little bit so I'l
give it a coupla more days before I "restore" to the original filter
code...
> On Jan 24, 2017, at 3:46 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> On 01/17/2017 02:48 PM, Jim Jagielski wrote:
>>
>>> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri <DR...@primary.net> wrote:
>>>
>>> For the most part, yes except the portions that make the header presence
>>> optional (the HDR_MISSING case). Those were added as it came into the
>>> code base to handle a use case I was working on. I've added some
>>> comments inline since I won't have time to poke around myself for a
>>> while yet.
>>>
>>>
>>> For convenience, here's a link to the original code
>>>
>>> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
>>>
>>
>> Would it make sense to have the "stable" version available
>> for backport, and keep in the WIP in trunk?
>>
>
> This would be an option, but apart from this I would like to see the WIP in trunk
> somehow fixed. Otherwise it is a perfect candidate for falling through the cracks
> and giving yet another surprise once we branch "whatever we name it" from trunk.
> IMHO easier to fix that now or even revert that part for the time being while people
> remember what is going on then later digging through it while hitting the issues.
>
> Regards
>
> Rüdiger
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 01/17/2017 02:48 PM, Jim Jagielski wrote:
>
>> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri <DR...@primary.net> wrote:
>>
>> For the most part, yes except the portions that make the header presence
>> optional (the HDR_MISSING case). Those were added as it came into the
>> code base to handle a use case I was working on. I've added some
>> comments inline since I won't have time to poke around myself for a
>> while yet.
>>
>>
>> For convenience, here's a link to the original code
>>
>> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
>>
>
> Would it make sense to have the "stable" version available
> for backport, and keep in the WIP in trunk?
>
This would be an option, but apart from this I would like to see the WIP in trunk
somehow fixed. Otherwise it is a perfect candidate for falling through the cracks
and giving yet another surprise once we branch "whatever we name it" from trunk.
IMHO easier to fix that now or even revert that part for the time being while people
remember what is going on then later digging through it while hitting the issues.
Regards
Rdiger
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jan 16, 2017, at 6:57 PM, Daniel Ruggeri <DR...@primary.net> wrote:
>
> For the most part, yes except the portions that make the header presence
> optional (the HDR_MISSING case). Those were added as it came into the
> code base to handle a use case I was working on. I've added some
> comments inline since I won't have time to poke around myself for a
> while yet.
>
>
> For convenience, here's a link to the original code
>
> https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
>
Would it make sense to have the "stable" version available
for backport, and keep in the WIP in trunk?
Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Daniel Ruggeri <DR...@primary.net>.
On 1/30/2017 4:45 AM, Ruediger Pluem wrote:
> Thinking of all the above it might be best if you read in mode AP_MODE_SPECULATIVE on your own from upstream until
> you have MIN_HDR_LEN data. If the PROXY header is present, read MIN_HDR_LEN in AP_MODE_READBYTES to finally consume the
> data and move on. If the PROXY header is not present, well then just forward the original request and you are fine.
> This way you leave all the hassle to the upstream filters.
Yes, definitely. I was contemplating the same thing given the
permutations of modes it may be called in and the various cases to deal
with. That's the approach I've taken in the latest commit because the
hassle is definitely best left to upstream :-)
At a high level, it no longer stores the data to pass along. When
optional processing is enabled, the filter starts in speculative read
mode. Once MIN_HDR_LEN is read and we know if a header is there or not
we can discard ctx->bb, reinitialize ctx and move to READBYTES mode.
--
Daniel Ruggeri
Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 01/28/2017 07:14 PM, Daniel Ruggeri wrote:
>
> On 1/25/2017 9:02 PM, Daniel Ruggeri wrote:
>> On 1/25/2017 6:53 PM, Daniel Ruggeri wrote:
>>> I'd say that not returning until ctx->bb has enough information to
>>> determine if the header is present or not would be sufficient. Isn't
>>> this already done in the potentially repeated calls to ap_get_brigade on
>>> line no 1056 inside the loop verifying that ctx->done is not set? If
>>> we're not intentionally holding onto the data until we know it's safe,
>>> then I think there's a structural problem in this module that would
>>> require us to start doing so.
>> Sorry - I neglected to notice the obvious check that the brigade is not
>> empty JUST before this line. Nevermind this silly comment...
>>
>> Indeed, if the brigade runs dry before the minimum number of bytes
>> needed has been read, the data should not be deleted.
>> I'm thinking apr_bucket_setaside(b, f->c->pool) in place of
>> apr_bucket_delete(b) would be good here... I will also experiment with
>> having the filter ask for less data than it needs to verify these
>> multiple calls through the filter work as expected.
>>
>
> Changes to set aside the bucket data are in r1780725. I'll await
> updating the 2.4 backport patch with this and the compiler warning fix
> when we're good on how to proceed regarding the other email I just sent.
>
> To verify behavior, I forced the filter to receive only a few bytes at a
> time until it "built up" enough data to evaluate the header type and
> pass all data set aside in bb_out when RemoteIPProxyProtocol is set to
> Optional. Thanks for the pointers.
>
I think there are still several edge cases open, especially in the optional
case. Let me try to list.
1. I guess you should step aside (for this particular call) if the filter is called with the following modes:
AP_MODE_INIT
AP_MODE_EATCRLF ???
2. All the real fun starts with the optional case if you do not find the PROXY header:
1. The original call to your filter requested less bytes than MIN_HDR_LEN. You can only
return the amount of data requested and you need to keep the remaining stuff in your ctx->store_bb. For each
following call you need to hand out your remaining data in ctx->store_bb first.
2. 1. gets even more fun if the the reader requested mode AP_MODE_SPECULATIVE. In this case you can proceed as in 1.,
but you need to hand out copies of the data as you need to be able to return that data later again in case of a non
speculative read.
3. In AP_MODE_GETLINE you need to ensure that you don't return data past a LF character. If there is no LF in your
ctx->store_bb data you need to add the missing data by doing a ap_get_brigade on your own.
4. There might be intermittent calls of 1., 2. and 3.
Thinking of all the above it might be best if you read in mode AP_MODE_SPECULATIVE on your own from upstream until
you have MIN_HDR_LEN data. If the PROXY header is present, read MIN_HDR_LEN in AP_MODE_READBYTES to finally consume the
data and move on. If the PROXY header is not present, well then just forward the original request and you are fine.
This way you leave all the hassle to the upstream filters.
Regards
R�diger
Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Daniel Ruggeri <DR...@primary.net>.
On 1/25/2017 9:02 PM, Daniel Ruggeri wrote:
> On 1/25/2017 6:53 PM, Daniel Ruggeri wrote:
>> I'd say that not returning until ctx->bb has enough information to
>> determine if the header is present or not would be sufficient. Isn't
>> this already done in the potentially repeated calls to ap_get_brigade on
>> line no 1056 inside the loop verifying that ctx->done is not set? If
>> we're not intentionally holding onto the data until we know it's safe,
>> then I think there's a structural problem in this module that would
>> require us to start doing so.
> Sorry - I neglected to notice the obvious check that the brigade is not
> empty JUST before this line. Nevermind this silly comment...
>
> Indeed, if the brigade runs dry before the minimum number of bytes
> needed has been read, the data should not be deleted.
> I'm thinking apr_bucket_setaside(b, f->c->pool) in place of
> apr_bucket_delete(b) would be good here... I will also experiment with
> having the filter ask for less data than it needs to verify these
> multiple calls through the filter work as expected.
>
Changes to set aside the bucket data are in r1780725. I'll await
updating the 2.4 backport patch with this and the compiler warning fix
when we're good on how to proceed regarding the other email I just sent.
To verify behavior, I forced the filter to receive only a few bytes at a
time until it "built up" enough data to evaluate the header type and
pass all data set aside in bb_out when RemoteIPProxyProtocol is set to
Optional. Thanks for the pointers.
--
Daniel Ruggeri
Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Daniel Ruggeri <DR...@primary.net>.
On 1/25/2017 6:53 PM, Daniel Ruggeri wrote:
> I'd say that not returning until ctx->bb has enough information to
> determine if the header is present or not would be sufficient. Isn't
> this already done in the potentially repeated calls to ap_get_brigade on
> line no 1056 inside the loop verifying that ctx->done is not set? If
> we're not intentionally holding onto the data until we know it's safe,
> then I think there's a structural problem in this module that would
> require us to start doing so.
Sorry - I neglected to notice the obvious check that the brigade is not
empty JUST before this line. Nevermind this silly comment...
Indeed, if the brigade runs dry before the minimum number of bytes
needed has been read, the data should not be deleted.
I'm thinking apr_bucket_setaside(b, f->c->pool) in place of
apr_bucket_delete(b) would be good here... I will also experiment with
having the filter ask for less data than it needs to verify these
multiple calls through the filter work as expected.
--
Daniel Ruggeri
Re: AW: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Daniel Ruggeri <DR...@primary.net>.
First, my apologies, but it looks like line wrapping is going to exceed
the usual number of columns so formatting may get wonky in this reply.
On 1/17/2017 3:48 AM, Pl�m, R�diger, Vodafone Group wrote:
>
>> -----Urspr�ngliche Nachricht-----
>> Von: Daniel Ruggeri [mailto:DRuggeri@primary.net]
>> Gesendet: Dienstag, 17. Januar 2017 00:57
>> An: dev@httpd.apache.org
>> Betreff: Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-
>> message-tags/next-number docs/manual/mod/mod_remoteip.xml
>> modules/metadata/mod_remoteip.c
>>
>> For the most part, yes except the portions that make the header presence
>> optional (the HDR_MISSING case). Those were added as it came into the
>> code base to handle a use case I was working on. I've added some
>> comments inline since I won't have time to poke around myself for a
>> while yet.
>>
>>
>> For convenience, here's a link to the original code
>>
>> https://github.com/roadrunner2/mod-proxy-
>> protocol/blob/master/mod_proxy_protocol.c
>>
>>
>> On 1/16/2017 10:14 AM, Jim Jagielski wrote:
>>> Let me take a look... afaict, this is a copy of what
>>> was donated, which has been working for numerous people.
>>> But that doesn't mean it can't have bugs ;)
>>>
>>>> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem <rp...@apache.org>
>> wrote:
>>>> Anyone?
>> Apologies for missing the original reply
>>
>>>> Regards
>>>>
>>>> R�diger
>>>>
>>>> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>>>>> On 12/30/2016 03:20 PM, druggeri@apache.org wrote:
>>>>>> 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
>>>>>>
>>>>>>
>> ========================================================================
>> ======
>>>>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>>>>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30
>> 14:20:48 2016
>>>>>> + */
>>>>>> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
>>>>>> +{
>>>>>> + remoteip_config_t *conf;
>>>>>> + remoteip_conn_config_t *conn_conf;
>>>>>> + int optional;
>>>>>> +
>>>>>> + conf = ap_get_module_config(ap_server_conf->module_config,
>>>>>> + &remoteip_module);
>>>>>> +
>>>>>> + /* Used twice - do the check only once */
>>>>>> + optional = remoteip_addr_in_list(conf-
>>> proxy_protocol_optional, c->local_addr);
>>>>>> +
>>>>>> + /* check if we're enabled for this connection */
>>>>>> + if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c-
>>> local_addr)
>>>>>> + && !optional )
>>>>>> + || remoteip_addr_in_list(conf->proxy_protocol_disabled, c-
>>> local_addr)) {
>>>>>> + return DECLINED;
>>>>>> + }
>>>>>> +
>>>>>> + /* mod_proxy creates outgoing connections - we don't want
>> those */
>>>>>> + if (!remoteip_is_server_port(c->local_addr->port)) {
>>>>>> + return DECLINED;
>>>>>> + }
>>>>> Why is the c->local_addr->port set to a port we are listening on in
>> case of proxy connections?
>>
>> This is from the original code, but wouldn't this be the local port on
>> the outbound connection (some high number)? The remoteip_is_server_port
>> should therefore fail this check.
> My bad I missed the ! in the condition. So this should work.
>
>>>>>> +
>>>>>> + switch (psts) {
>>>>>> + case HDR_MISSING:
>>>>>> + if (conn_conf->proxy_protocol_optional) {
>>>>>> + /* Same as DONE, but don't delete the bucket. Rather, put it
>>>>>> + back into the brigade and move therequest along the stack */
>>>>>> + ctx->done = 1;
>>>>>> + APR_BRIGADE_INSERT_HEAD(bb_out, b);
>>>>> See below. We need to restore all buckets. What if the original read
>> was speculative?
>>>>>> + return ap_pass_brigade(f->next, ctx->bb);
>>>>> Why using ap_pass_brigade on f->next? We are an input filter and f-
>>> next is our upstream input filter.
>> This is probably my own misunderstanding... what would be the preferred
>> way to move this down the stack, then?
> What do you want to do here? Return what you have in ctx->bb to the caller that pulls
> from you? You pull from input filter while you push to output filters.
Yes, as you say. Enough information was read to determine that the
header is not present. Since nothing was consumed by this function, all
data read up until this point (which should reside in ctx->bb) should be
passed to the next filter. My understanding is that the "next" filter
would be mod_ssl or even core
>
>>>>>> + }
>>>>>> + else {
>>>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f-
>>> c, APLOGNO(03510)
>>>>>> + "RemoteIPProxyProtocol: no
>> valid PROXY header found");
>>>>>> + /* fall through to error case */
>>>>>> + }
>>>>>> + case HDR_ERROR:
>>>>>> + f->c->aborted = 1;
>>>>>> + apr_bucket_delete(b);
>>>>>> + apr_brigade_destroy(ctx->bb);
>>>>>> + return APR_ECONNABORTED;
>>>>>> +
>>>>>> + case HDR_DONE:
>>>>>> + apr_bucket_delete(b);
>>>>>> + ctx->done = 1;
>>>>>> + break;
>>>>>> +
>>>>>> + case HDR_NEED_MORE:
>>>>>> + /* It is safe to delete this bucket if we get here since we know
>>>>>> + that we are definitely processing a header (we've read enough to
>>>>>> + know if the signatures exist on the line) */
>>>>> No. We might not even received MIN_HDR_LEN data so far and thus didnot check for the signature
>>>>> so far. We need to safe all the buckets that we might need to restore in a separate bucket brigade,
>>>>> that we need to be set aside before doing the next ap_get_brigade to avoid killing transient buckets.
>>>>>
>>>>> This is a good point. As a side note, in the original code, this delete
>>>>> didn't exist on this line, but was done earlier (noted above).
>>>>>
>>>>> Editorially, while I was testing a patch I was writing to do this same
>>>>> work with mod_ssl down stack, I was consistently getting only 11 bytes
>>>>> read and I would have expected that same thing to be an issue here
>>>>> (since MIN_HDR_LEN is 16)... but never actually observed that. I'm sure
>>>>> that's because ap_get_brigade on the first round through is called
>>>>> requesting MIN_HDR_LEN bytes (as assigned to ctx->need). Would
>>>>> ap_get_brigade ever turn less than what was requested?
> Of course it can. It depends on the mode. In AP_MODE_READBYTES it can return less
> bytes even in blocking mode provided that bytes are available at all. But it
> never returns more bytes than the requested amount.
> in AP_MODE_GETLINE mode and blocking mode it reads until it gets the LF or if it has read
> more than HUGE_STRING_LEN.
> Another issue you have in the case of a missing header is a speculative read:
> Even if you return the data you used for checking the header (unsuccessfully), the caller
> expects the data still to be available for a non speculative read later on. So another complexity
> added here.
Understood, but it is unsafe to pass any data at all until we know that
the header is present (and removed when enabled) or not present when
configured to be optional. If it is present, but optional, any partial
read of the data would result in passing incomplete pieces of the header
down the stack. That data is just bytes that would put unexpected junk
in front of the HTTP request or SSL data.
> Or what do you do if the caller requested to read less bytes than you need for reading and detecting
> a possible header? In this case you can only deliver what the caller has asked for and you need to keep the
> remaining stuff in stock for later return.
> I guess having a look at
>
> ap_core_input_filter
> ssl_io_filter_input
>
> gives some ideas.
I'd say that not returning until ctx->bb has enough information to
determine if the header is present or not would be sufficient. Isn't
this already done in the potentially repeated calls to ap_get_brigade on
line no 1056 inside the loop verifying that ctx->done is not set? If
we're not intentionally holding onto the data until we know it's safe,
then I think there's a structural problem in this module that would
require us to start doing so.
>
> Regards
>
> R�diger
--
Daniel Ruggeri
AW: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: Daniel Ruggeri [mailto:DRuggeri@primary.net]
> Gesendet: Dienstag, 17. Januar 2017 00:57
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r1776575 - in /httpd/httpd/trunk: docs/log-
> message-tags/next-number docs/manual/mod/mod_remoteip.xml
> modules/metadata/mod_remoteip.c
>
> For the most part, yes except the portions that make the header presence
> optional (the HDR_MISSING case). Those were added as it came into the
> code base to handle a use case I was working on. I've added some
> comments inline since I won't have time to poke around myself for a
> while yet.
>
>
> For convenience, here's a link to the original code
>
> https://github.com/roadrunner2/mod-proxy-
> protocol/blob/master/mod_proxy_protocol.c
>
>
> On 1/16/2017 10:14 AM, Jim Jagielski wrote:
> > Let me take a look... afaict, this is a copy of what
> > was donated, which has been working for numerous people.
> > But that doesn't mean it can't have bugs ;)
> >
> >> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem <rp...@apache.org>
> wrote:
> >>
> >> Anyone?
>
> Apologies for missing the original reply
>
> >> Regards
> >>
> >> Rüdiger
> >>
> >> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
> >>>
> >>> On 12/30/2016 03:20 PM, druggeri@apache.org wrote:
> >>>> 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
> >>>>
> >>>>
> ========================================================================
> ======
> >>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
> >>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30
> 14:20:48 2016
> >>>> + */
> >>>> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
> >>>> +{
> >>>> + remoteip_config_t *conf;
> >>>> + remoteip_conn_config_t *conn_conf;
> >>>> + int optional;
> >>>> +
> >>>> + conf = ap_get_module_config(ap_server_conf->module_config,
> >>>> + &remoteip_module);
> >>>> +
> >>>> + /* Used twice - do the check only once */
> >>>> + optional = remoteip_addr_in_list(conf-
> >proxy_protocol_optional, c->local_addr);
> >>>> +
> >>>> + /* check if we're enabled for this connection */
> >>>> + if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c-
> >local_addr)
> >>>> + && !optional )
> >>>> + || remoteip_addr_in_list(conf->proxy_protocol_disabled, c-
> >local_addr)) {
> >>>> + return DECLINED;
> >>>> + }
> >>>> +
> >>>> + /* mod_proxy creates outgoing connections - we don't want
> those */
> >>>> + if (!remoteip_is_server_port(c->local_addr->port)) {
> >>>> + return DECLINED;
> >>>> + }
> >>> Why is the c->local_addr->port set to a port we are listening on in
> case of proxy connections?
>
> This is from the original code, but wouldn't this be the local port on
> the outbound connection (some high number)? The remoteip_is_server_port
> should therefore fail this check.
My bad I missed the ! in the condition. So this should work.
> >>>> +
> >>>> + switch (psts) {
> >>>> + case HDR_MISSING:
> >>>> + if (conn_conf->proxy_protocol_optional) {
> >>>> + /* Same as DONE, but don't delete the
> bucket. Rather, put it
> >>>> + back into the brigade and move the
> request along the stack */
> >>>> + ctx->done = 1;
> >>>> + APR_BRIGADE_INSERT_HEAD(bb_out, b);
> >>> See below. We need to restore all buckets. What if the original read
> was speculative?
> >>>
> >>>> + return ap_pass_brigade(f->next, ctx->bb);
> >>> Why using ap_pass_brigade on f->next? We are an input filter and f-
> >next is our upstream input filter.
>
> This is probably my own misunderstanding... what would be the preferred
> way to move this down the stack, then?
What do you want to do here? Return what you have in ctx->bb to the caller that pulls
from you? You pull from input filter while you push to output filters.
>
> >>>
> >>>> + }
> >>>> + else {
> >>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f-
> >c, APLOGNO(03510)
> >>>> + "RemoteIPProxyProtocol: no
> valid PROXY header found");
> >>>> + /* fall through to error case */
> >>>> + }
> >>>> + case HDR_ERROR:
> >>>> + f->c->aborted = 1;
> >>>> + apr_bucket_delete(b);
> >>>> + apr_brigade_destroy(ctx->bb);
> >>>> + return APR_ECONNABORTED;
> >>>> +
> >>>> + case HDR_DONE:
> >>>> + apr_bucket_delete(b);
> >>>> + ctx->done = 1;
> >>>> + break;
> >>>> +
> >>>> + case HDR_NEED_MORE:
> >>>> + /* It is safe to delete this bucket if we get
> here since we know
> >>>> + that we are definitely processing a header
> (we've read enough to
> >>>> + know if the signatures exist on the line)
> */
> >>> No. We might not even received MIN_HDR_LEN data so far and thus did
> not check for the signature
> >>> so far. We need to safe all the buckets that we might need to
> restore in a separate bucket brigade,
> >>> that we need to be set aside before doing the next ap_get_brigade to
> avoid killing transient buckets.
>
> This is a good point. As a side note, in the original code, this delete
> didn't exist on this line, but was done earlier (noted above).
>
> Editorially, while I was testing a patch I was writing to do this same
> work with mod_ssl down stack, I was consistently getting only 11 bytes
> read and I would have expected that same thing to be an issue here
> (since MIN_HDR_LEN is 16)... but never actually observed that. I'm sure
> that's because ap_get_brigade on the first round through is called
> requesting MIN_HDR_LEN bytes (as assigned to ctx->need). Would
> ap_get_brigade ever turn less than what was requested?
Of course it can. It depends on the mode. In AP_MODE_READBYTES it can return less
bytes even in blocking mode provided that bytes are available at all. But it
never returns more bytes than the requested amount.
in AP_MODE_GETLINE mode and blocking mode it reads until it gets the LF or if it has read
more than HUGE_STRING_LEN.
Another issue you have in the case of a missing header is a speculative read:
Even if you return the data you used for checking the header (unsuccessfully), the caller
expects the data still to be available for a non speculative read later on. So another complexity
added here.
Or what do you do if the caller requested to read less bytes than you need for reading and detecting
a possible header? In this case you can only deliver what the caller has asked for and you need to keep the
remaining stuff in stock for later return.
I guess having a look at
ap_core_input_filter
ssl_io_filter_input
gives some ideas.
Regards
Rüdiger
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Daniel Ruggeri <DR...@primary.net>.
For the most part, yes except the portions that make the header presence
optional (the HDR_MISSING case). Those were added as it came into the
code base to handle a use case I was working on. I've added some
comments inline since I won't have time to poke around myself for a
while yet.
For convenience, here's a link to the original code
https://github.com/roadrunner2/mod-proxy-protocol/blob/master/mod_proxy_protocol.c
On 1/16/2017 10:14 AM, Jim Jagielski wrote:
> Let me take a look... afaict, this is a copy of what
> was donated, which has been working for numerous people.
> But that doesn't mean it can't have bugs ;)
>
>> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>
>> Anyone?
Apologies for missing the original reply
>> Regards
>>
>> R�diger
>>
>> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>>>
>>> On 12/30/2016 03:20 PM, druggeri@apache.org wrote:
>>>> 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
>>>>
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 2016
>>>> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>>>> return OK;
>>>> }
>>>>
>>>> +static int remoteip_is_server_port(apr_port_t port)
>>>> +{
>>>> + ap_listen_rec *lr;
>>>> +
>>>> + for (lr = ap_listeners; lr; lr = lr->next) {
>>>> + if (lr->bind_addr && lr->bind_addr->port == port) {
>>>> + return 1;
>>>> + }
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Human readable format:
>>>> + * PROXY {TCP4|TCP6|UNKNOWN} <client-ip-addr> <dest-ip-addr> <client-port> <dest-port><CR><LF>
>>>> + */
>>>> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
>>>> + remoteip_conn_config_t *conn_conf,
>>>> + proxy_header *hdr, apr_size_t len,
>>>> + apr_size_t *hdr_len)
>>>> +{
>>>> + char *end, *word, *host, *valid_addr_chars, *saveptr;
>>>> + char buf[sizeof(hdr->v1.line)];
>>>> + apr_port_t port;
>>>> + apr_status_t ret;
>>>> + apr_int32_t family;
>>>> +
>>>> +#define GET_NEXT_WORD(field) \
>>>> + word = apr_strtok(NULL, " ", &saveptr); \
>>>> + if (!word) { \
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
>>>> + "RemoteIPProxyProtocol: no " field " found in header '%s'", \
>>>> + hdr->v1.line); \
>>>> + return HDR_ERROR; \
>>>> + }
>>>> +
>>>> + end = memchr(hdr->v1.line, '\r', len - 1);
>>>> + if (!end || end[1] != '\n') {
>>>> + return HDR_NEED_MORE; /* partial or invalid header */
>>>> + }
>>>> +
>>>> + *end = '\0';
>>>> + *hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
>>>> +
>>>> + /* parse in separate buffer so have the original for error messages */
>>>> + strcpy(buf, hdr->v1.line);
>>>> +
>>>> + apr_strtok(buf, " ", &saveptr);
>>>> +
>>>> + /* parse family */
>>>> + GET_NEXT_WORD("family")
>>>> + if (strcmp(word, "UNKNOWN") == 0) {
>>>> + conn_conf->client_addr = c->client_addr;
>>>> + conn_conf->client_ip = c->client_ip;
>>>> + return HDR_DONE;
>>>> + }
>>>> + else if (strcmp(word, "TCP4") == 0) {
>>>> + family = APR_INET;
>>>> + valid_addr_chars = "0123456789.";
>>>> + }
>>>> + else if (strcmp(word, "TCP6") == 0) {
>>>> +#if APR_HAVE_IPV6
>>>> + family = APR_INET6;
>>>> + valid_addr_chars = "0123456789abcdefABCDEF:";
>>>> +#else
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
>>>> + "RemoteIPProxyProtocol: Unable to parse v6 address - APR is not compiled with IPv6 support",
>>>> + word, hdr->v1.line);
>>>> + return HDR_ERROR;
>>>> +#endif
>>>> + }
>>>> + else {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
>>>> + "RemoteIPProxyProtocol: unknown family '%s' in header '%s'",
>>>> + word, hdr->v1.line);
>>>> + return HDR_ERROR;
>>>> + }
>>>> +
>>>> + /* parse client-addr */
>>>> + GET_NEXT_WORD("client-address")
>>>> +
>>>> + if (strspn(word, valid_addr_chars) != strlen(word)) {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
>>>> + "RemoteIPProxyProtocol: invalid client-address '%s' found in "
>>>> + "header '%s'", word, hdr->v1.line);
>>>> + return HDR_ERROR;
>>>> + }
>>>> +
>>>> + host = word;
>>>> +
>>>> + /* parse dest-addr */
>>>> + GET_NEXT_WORD("destination-address")
>>>> +
>>>> + /* parse client-port */
>>>> + GET_NEXT_WORD("client-port")
>>>> + if (sscanf(word, "%hu", &port) != 1) {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
>>>> + "RemoteIPProxyProtocol: error parsing port '%s' in header '%s'",
>>>> + word, hdr->v1.line);
>>>> + return HDR_ERROR;
>>>> + }
>>>> +
>>>> + /* parse dest-port */
>>>> + /* GET_NEXT_WORD("destination-port") - no-op since we don't care about it */
>>>> +
>>>> + /* create a socketaddr from the info */
>>>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, host, family, port, 0,
>>>> + c->pool);
>>>> + if (ret != APR_SUCCESS) {
>>>> + conn_conf->client_addr = NULL;
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03502)
>>>> + "RemoteIPProxyProtocol: error converting family '%d', host '%s',"
>>>> + " and port '%hu' to sockaddr; header was '%s'",
>>>> + family, host, port, hdr->v1.line);
>>>> + return HDR_ERROR;
>>>> + }
>>>> +
>>>> + conn_conf->client_ip = apr_pstrdup(c->pool, host);
>>>> +
>>>> + return HDR_DONE;
>>>> +}
>>>> +
>>>> +/** Add our filter to the connection if it is requested
>>>> + */
>>>> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
>>>> +{
>>>> + remoteip_config_t *conf;
>>>> + remoteip_conn_config_t *conn_conf;
>>>> + int optional;
>>>> +
>>>> + conf = ap_get_module_config(ap_server_conf->module_config,
>>>> + &remoteip_module);
>>>> +
>>>> + /* Used twice - do the check only once */
>>>> + optional = remoteip_addr_in_list(conf->proxy_protocol_optional, c->local_addr);
>>>> +
>>>> + /* check if we're enabled for this connection */
>>>> + if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c->local_addr)
>>>> + && !optional )
>>>> + || remoteip_addr_in_list(conf->proxy_protocol_disabled, c->local_addr)) {
>>>> + return DECLINED;
>>>> + }
>>>> +
>>>> + /* mod_proxy creates outgoing connections - we don't want those */
>>>> + if (!remoteip_is_server_port(c->local_addr->port)) {
>>>> + return DECLINED;
>>>> + }
>>> Why is the c->local_addr->port set to a port we are listening on in case of proxy connections?
This is from the original code, but wouldn't this be the local port on
the outbound connection (some high number)? The remoteip_is_server_port
should therefore fail this check.
>>>
>>>> +
>>>> + /* add our filter */
>>>> + if (!ap_add_input_filter(remoteip_filter_name, NULL, NULL, c)) {
>>>> + /* XXX: Shouldn't this WARN in log? */
>>>> + return DECLINED;
>>>> + }
>>>> +
>>>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(03503)
>>>> + "RemoteIPProxyProtocol: enabled on connection to %s:%hu",
>>>> + c->local_ip, c->local_addr->port);
>>>> +
>>>> + /* this holds the resolved proxy info for this connection */
>>>> + conn_conf = apr_pcalloc(c->pool, sizeof(*conn_conf));
>>>> +
>>>> + /* Propagate the optional flag so the connection handler knows not to
>>>> + abort if the header is mising. NOTE: This means we must check after
>>>> + we read the request that the header was NOT optional, too.
>>>> + */
>>>> + conn_conf->proxy_protocol_optional = optional;
>>>> +
>>>> + ap_set_module_config(c->conn_config, &remoteip_module, conn_conf);
>>>> +
>>>> + return OK;
>>>> +}
>>>> +
>>>> +/* Binary format:
>>>> + * <sig><cmd><proto><addr-len><addr>
>>>> + * sig = \x0D \x0A \x0D \x0A \x00 \x0D \x0A \x51 \x55 \x49 \x54 \x0A
>>>> + * cmd = <4-bits-version><4-bits-command>
>>>> + * 4-bits-version = \x02
>>>> + * 4-bits-command = {\x00|\x01} (\x00 = LOCAL: discard con info; \x01 = PROXY)
>>>> + * proto = <4-bits-family><4-bits-protocol>
>>>> + * 4-bits-family = {\x00|\x01|\x02|\x03} (AF_UNSPEC, AF_INET, AF_INET6, AF_UNIX)
>>>> + * 4-bits-protocol = {\x00|\x01|\x02} (UNSPEC, STREAM, DGRAM)
>>>> + */
>>>> +static remoteip_parse_status_t remoteip_process_v2_header(conn_rec *c,
>>>> + remoteip_conn_config_t *conn_conf,
>>>> + proxy_header *hdr)
>>>> +{
>>>> + apr_status_t ret;
>>>> +
>>>> + switch (hdr->v2.ver_cmd & 0xF) {
>>>> + case 0x01: /* PROXY command */
>>>> + switch (hdr->v2.fam) {
>>>> + case 0x11: /* TCPv4 */
>>>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
>>>> + APR_INET,
>>>> + ntohs(hdr->v2.addr.ip4.src_port),
>>>> + 0, c->pool);
>>>> + if (ret != APR_SUCCESS) {
>>>> + conn_conf->client_addr = NULL;
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03504)
>>>> + "RemoteIPPProxyProtocol: error creating sockaddr");
>>>> + return HDR_ERROR;
>>>> + }
>>>> +
>>>> + conn_conf->client_addr->sa.sin.sin_addr.s_addr =
>>>> + hdr->v2.addr.ip4.src_addr;
>>>> + break;
>>>> +
>>>> + case 0x21: /* TCPv6 */
>>>> +#if APR_HAVE_IPV6
>>>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
>>>> + APR_INET6,
>>>> + ntohs(hdr->v2.addr.ip6.src_port),
>>>> + 0, c->pool);
>>>> + if (ret != APR_SUCCESS) {
>>>> + conn_conf->client_addr = NULL;
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03505)
>>>> + "RemoteIPProxyProtocol: error creating sockaddr");
>>>> + return HDR_ERROR;
>>>> + }
>>>> + memcpy(&conn_conf->client_addr->sa.sin6.sin6_addr.s6_addr,
>>>> + hdr->v2.addr.ip6.src_addr, 16);
>>>> + break;
>>>> +#else
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03506)
>>>> + "RemoteIPProxyProtocol: APR is not compiled with IPv6 support");
>>>> + return HDR_ERROR;
>>>> +#endif
>>>> + default:
>>>> + /* unsupported protocol, keep local connection address */
>>>> + return HDR_DONE;
>>>> + }
>>>> + break; /* we got a sockaddr now */
>>>> +
>>>> + case 0x00: /* LOCAL command */
>>>> + /* keep local connection address for LOCAL */
>>>> + return HDR_DONE;
>>>> +
>>>> + default:
>>>> + /* not a supported command */
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03507)
>>>> + "RemoteIPProxyProtocol: unsupported command %.2hx",
>>>> + hdr->v2.ver_cmd);
>>>> + return HDR_ERROR;
>>>> + }
>>>> +
>>>> + /* got address - compute the client_ip from it */
>>>> + ret = apr_sockaddr_ip_get(&conn_conf->client_ip, conn_conf->client_addr);
>>>> + if (ret != APR_SUCCESS) {
>>>> + conn_conf->client_addr = NULL;
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03508)
>>>> + "RemoteIPProxyProtocol: error converting address to string");
>>>> + return HDR_ERROR;
>>>> + }
>>>> +
>>>> + return HDR_DONE;
>>>> +}
>>>> +
>>>> +/** Determine if this is a v1 or v2 PROXY header.
>>>> + */
>>>> +static int remoteip_determine_version(conn_rec *c, const char *ptr)
>>>> +{
>>>> + proxy_header *hdr = (proxy_header *) ptr;
>>>> +
>>>> + /* assert len >= 14 */
>>>> +
>>>> + if (memcmp(&hdr->v2, v2sig, sizeof(v2sig)) == 0 &&
>>>> + (hdr->v2.ver_cmd & 0xF0) == 0x20) {
>>>> + return 2;
>>>> + }
>>>> + else if (memcmp(hdr->v1.line, "PROXY ", 6) == 0) {
>>>> + return 1;
>>>> + }
>>>> + else {
>>>> + return -1;
>>>> + }
>>>> +}
>>>> +
>>>> +/* Capture the first bytes on the protocol and parse the proxy protocol header.
>>>> + * Removes itself when the header is complete.
>>>> + */
>>>> +static apr_status_t remoteip_input_filter(ap_filter_t *f,
>>>> + apr_bucket_brigade *bb_out,
>>>> + ap_input_mode_t mode,
>>>> + apr_read_type_e block,
>>>> + apr_off_t readbytes)
>>>> +{
>>>> + apr_status_t ret;
>>>> + remoteip_filter_context *ctx = f->ctx;
>>>> + remoteip_conn_config_t *conn_conf;
>>>> + apr_bucket *b;
>>>> + remoteip_parse_status_t psts;
>>>> + const char *ptr;
>>>> + apr_size_t len;
>>>> +
>>>> + if (f->c->aborted) {
>>>> + return APR_ECONNABORTED;
>>>> + }
>>>> +
>>>> + /* allocate/retrieve the context that holds our header */
>>>> + if (!ctx) {
>>>> + ctx = f->ctx = apr_palloc(f->c->pool, sizeof(*ctx));
>>>> + ctx->rcvd = 0;
>>>> + ctx->need = MIN_HDR_LEN;
>>>> + ctx->version = 0;
>>>> + ctx->mode = AP_MODE_READBYTES;
>>>> + ctx->bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
>>>> + ctx->done = 0;
>>>> + }
>>>> +
>>>> + if (ctx->done) {
>>>> + /* Note: because we're a connection filter we can't remove ourselves
>>>> + * when we're done, so we have to stay in the chain and just go into
>>>> + * passthrough mode.
>>>> + */
>>>> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
>>>> + }
>>>> +
>>>> + conn_conf = ap_get_module_config(f->c->conn_config, &remoteip_module);
>>>> +
>>>> + /* try to read a header's worth of data */
>>>> + while (!ctx->done) {
>>>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
>>>> + ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
>>>> + ctx->need - ctx->rcvd);
>>>> + if (ret != APR_SUCCESS) {
>>>> + return ret;
>>>> + }
>>>> + }
>>>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
>>> What about the case of an non blocking read where the upstream filter returns an empty brigade
>>> and APR_SUCCESS. This is equal to returning EAGAIN.
Also from the original code
>>>> + return APR_EOF;
>>>> + }
>>>> +
>>>> + while (!ctx->done && !APR_BRIGADE_EMPTY(ctx->bb)) {
>>>> + b = APR_BRIGADE_FIRST(ctx->bb);
>>>> +
>>>> + ret = apr_bucket_read(b, &ptr, &len, block);
>>>> + if (APR_STATUS_IS_EAGAIN(ret) && block == APR_NONBLOCK_READ) {
>>>> + return APR_SUCCESS;
>>>> + }
>>>> + if (ret != APR_SUCCESS) {
>>>> + return ret;
>>>> + }
>>>> +
>>>> + memcpy(ctx->header + ctx->rcvd, ptr, len);
>>>> + ctx->rcvd += len;
>>>> +
>>>> + /* Remove instead of delete - we may put this bucket
>>>> + back into bb_out if the header was optional and we
>>>> + pass down the chain */
>>>> + APR_BUCKET_REMOVE(b);
Note: In the original code this was apr_bucket_delete.
>>>> + psts = HDR_NEED_MORE;
>>>> +
>>>> + if (ctx->version == 0) {
>>>> + /* reading initial chunk */
>>>> + if (ctx->rcvd >= MIN_HDR_LEN) {
>>>> + ctx->version = remoteip_determine_version(f->c, ctx->header);
>>>> + if (ctx->version < 0) {
>>>> + psts = HDR_MISSING;
>>>> + }
>>>> + else if (ctx->version == 1) {
>>>> + ctx->mode = AP_MODE_GETLINE;
>>>> + ctx->need = sizeof(proxy_v1);
>>>> + }
>>>> + else if (ctx->version == 2) {
>>>> + ctx->need = MIN_V2_HDR_LEN;
>>>> + }
>>>> + }
>>>> + }
>>>> + else if (ctx->version == 1) {
>>>> + psts = remoteip_process_v1_header(f->c, conn_conf,
>>>> + (proxy_header *) ctx->header,
>>>> + ctx->rcvd, &ctx->need);
>>>> + }
>>>> + else if (ctx->version == 2) {
>>>> + if (ctx->rcvd >= MIN_V2_HDR_LEN) {
>>>> + ctx->need = MIN_V2_HDR_LEN +
>>>> + ntohs(((proxy_header *) ctx->header)->v2.len);
>>>> + }
>>>> + if (ctx->rcvd >= ctx->need) {
>>>> + psts = remoteip_process_v2_header(f->c, conn_conf,
>>>> + (proxy_header *) ctx->header);
>>>> + }
>>>> + }
>>>> + else {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03509)
>>>> + "RemoteIPProxyProtocol: internal error: unknown version "
>>>> + "%d", ctx->version);
>>>> + f->c->aborted = 1;
>>>> + apr_bucket_delete(b);
>>>> + apr_brigade_destroy(ctx->bb);
>>>> + return APR_ECONNABORTED;
>>>> + }
>>>> +
>>>> + switch (psts) {
>>>> + case HDR_MISSING:
>>>> + if (conn_conf->proxy_protocol_optional) {
>>>> + /* Same as DONE, but don't delete the bucket. Rather, put it
>>>> + back into the brigade and move the request along the stack */
>>>> + ctx->done = 1;
>>>> + APR_BRIGADE_INSERT_HEAD(bb_out, b);
>>> See below. We need to restore all buckets. What if the original read was speculative?
>>>
>>>> + return ap_pass_brigade(f->next, ctx->bb);
>>> Why using ap_pass_brigade on f->next? We are an input filter and f->next is our upstream input filter.
This is probably my own misunderstanding... what would be the preferred
way to move this down the stack, then?
>>>
>>>> + }
>>>> + else {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03510)
>>>> + "RemoteIPProxyProtocol: no valid PROXY header found");
>>>> + /* fall through to error case */
>>>> + }
>>>> + case HDR_ERROR:
>>>> + f->c->aborted = 1;
>>>> + apr_bucket_delete(b);
>>>> + apr_brigade_destroy(ctx->bb);
>>>> + return APR_ECONNABORTED;
>>>> +
>>>> + case HDR_DONE:
>>>> + apr_bucket_delete(b);
>>>> + ctx->done = 1;
>>>> + break;
>>>> +
>>>> + case HDR_NEED_MORE:
>>>> + /* It is safe to delete this bucket if we get here since we know
>>>> + that we are definitely processing a header (we've read enough to
>>>> + know if the signatures exist on the line) */
>>> No. We might not even received MIN_HDR_LEN data so far and thus did not check for the signature
>>> so far. We need to safe all the buckets that we might need to restore in a separate bucket brigade,
>>> that we need to be set aside before doing the next ap_get_brigade to avoid killing transient buckets.
This is a good point. As a side note, in the original code, this delete
didn't exist on this line, but was done earlier (noted above).
Editorially, while I was testing a patch I was writing to do this same
work with mod_ssl down stack, I was consistently getting only 11 bytes
read and I would have expected that same thing to be an issue here
(since MIN_HDR_LEN is 16)... but never actually observed that. I'm sure
that's because ap_get_brigade on the first round through is called
requesting MIN_HDR_LEN bytes (as assigned to ctx->need). Would
ap_get_brigade ever turn less than what was requested?
>>>> + apr_bucket_delete(b);
>>>> + break;
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> + /* we only get here when done == 1 */
>>>> + if (psts == HDR_DONE) {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03511)
>>>> + "RemoteIPProxyProtocol: received valid PROXY header: %s:%hu",
>>>> + conn_conf->client_ip, conn_conf->client_addr->port);
>>>> + }
>>>> + else {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03512)
>>>> + "RemoteIPProxyProtocol: PROXY header was missing");
>>>> + }
>>>> +
>>>> + if (ctx->rcvd > ctx->need || !APR_BRIGADE_EMPTY(ctx->bb)) {
>>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03513)
>>>> + "RemoteIPProxyProtocol: internal error: have data left over; "
>>>> + " need=%lu, rcvd=%lu, brigade-empty=%d", ctx->need,
>>>> + ctx->rcvd, APR_BRIGADE_EMPTY(ctx->bb));
>>>> + f->c->aborted = 1;
>>>> + apr_brigade_destroy(ctx->bb);
>>>> + return APR_ECONNABORTED;
>>>> + }
>>>> +
>>>> + /* clean up */
>>>> + apr_brigade_destroy(ctx->bb);
>>>> + ctx->bb = NULL;
>>>> +
>>>> + /* now do the real read for the upper layer */
>>>> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
>>>> +}
>>>> +
>>>> static const command_rec remoteip_cmds[] =
>>>> {
>>>> AP_INIT_TAKE1("RemoteIPHeader", header_name_set, NULL, RSRC_CONF,
>>> Regards
>>>
>>> R�diger
>>>
--
Daniel Ruggeri
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
Let me take a look... afaict, this is a copy of what
was donated, which has been working for numerous people.
But that doesn't mean it can't have bugs ;)
> On Jan 16, 2017, at 7:20 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
> Anyone?
>
> Regards
>
> Rüdiger
>
> On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>>
>>
>> On 12/30/2016 03:20 PM, druggeri@apache.org wrote:
>>> 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
>>>
>>
>>> ==============================================================================
>>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 2016
>>
>>> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>>> return OK;
>>> }
>>>
>>> +static int remoteip_is_server_port(apr_port_t port)
>>> +{
>>> + ap_listen_rec *lr;
>>> +
>>> + for (lr = ap_listeners; lr; lr = lr->next) {
>>> + if (lr->bind_addr && lr->bind_addr->port == port) {
>>> + return 1;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * Human readable format:
>>> + * PROXY {TCP4|TCP6|UNKNOWN} <client-ip-addr> <dest-ip-addr> <client-port> <dest-port><CR><LF>
>>> + */
>>> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
>>> + remoteip_conn_config_t *conn_conf,
>>> + proxy_header *hdr, apr_size_t len,
>>> + apr_size_t *hdr_len)
>>> +{
>>> + char *end, *word, *host, *valid_addr_chars, *saveptr;
>>> + char buf[sizeof(hdr->v1.line)];
>>> + apr_port_t port;
>>> + apr_status_t ret;
>>> + apr_int32_t family;
>>> +
>>> +#define GET_NEXT_WORD(field) \
>>> + word = apr_strtok(NULL, " ", &saveptr); \
>>> + if (!word) { \
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
>>> + "RemoteIPProxyProtocol: no " field " found in header '%s'", \
>>> + hdr->v1.line); \
>>> + return HDR_ERROR; \
>>> + }
>>> +
>>> + end = memchr(hdr->v1.line, '\r', len - 1);
>>> + if (!end || end[1] != '\n') {
>>> + return HDR_NEED_MORE; /* partial or invalid header */
>>> + }
>>> +
>>> + *end = '\0';
>>> + *hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
>>> +
>>> + /* parse in separate buffer so have the original for error messages */
>>> + strcpy(buf, hdr->v1.line);
>>> +
>>> + apr_strtok(buf, " ", &saveptr);
>>> +
>>> + /* parse family */
>>> + GET_NEXT_WORD("family")
>>> + if (strcmp(word, "UNKNOWN") == 0) {
>>> + conn_conf->client_addr = c->client_addr;
>>> + conn_conf->client_ip = c->client_ip;
>>> + return HDR_DONE;
>>> + }
>>> + else if (strcmp(word, "TCP4") == 0) {
>>> + family = APR_INET;
>>> + valid_addr_chars = "0123456789.";
>>> + }
>>> + else if (strcmp(word, "TCP6") == 0) {
>>> +#if APR_HAVE_IPV6
>>> + family = APR_INET6;
>>> + valid_addr_chars = "0123456789abcdefABCDEF:";
>>> +#else
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
>>> + "RemoteIPProxyProtocol: Unable to parse v6 address - APR is not compiled with IPv6 support",
>>> + word, hdr->v1.line);
>>> + return HDR_ERROR;
>>> +#endif
>>> + }
>>> + else {
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
>>> + "RemoteIPProxyProtocol: unknown family '%s' in header '%s'",
>>> + word, hdr->v1.line);
>>> + return HDR_ERROR;
>>> + }
>>> +
>>> + /* parse client-addr */
>>> + GET_NEXT_WORD("client-address")
>>> +
>>> + if (strspn(word, valid_addr_chars) != strlen(word)) {
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
>>> + "RemoteIPProxyProtocol: invalid client-address '%s' found in "
>>> + "header '%s'", word, hdr->v1.line);
>>> + return HDR_ERROR;
>>> + }
>>> +
>>> + host = word;
>>> +
>>> + /* parse dest-addr */
>>> + GET_NEXT_WORD("destination-address")
>>> +
>>> + /* parse client-port */
>>> + GET_NEXT_WORD("client-port")
>>> + if (sscanf(word, "%hu", &port) != 1) {
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
>>> + "RemoteIPProxyProtocol: error parsing port '%s' in header '%s'",
>>> + word, hdr->v1.line);
>>> + return HDR_ERROR;
>>> + }
>>> +
>>> + /* parse dest-port */
>>> + /* GET_NEXT_WORD("destination-port") - no-op since we don't care about it */
>>> +
>>> + /* create a socketaddr from the info */
>>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, host, family, port, 0,
>>> + c->pool);
>>> + if (ret != APR_SUCCESS) {
>>> + conn_conf->client_addr = NULL;
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03502)
>>> + "RemoteIPProxyProtocol: error converting family '%d', host '%s',"
>>> + " and port '%hu' to sockaddr; header was '%s'",
>>> + family, host, port, hdr->v1.line);
>>> + return HDR_ERROR;
>>> + }
>>> +
>>> + conn_conf->client_ip = apr_pstrdup(c->pool, host);
>>> +
>>> + return HDR_DONE;
>>> +}
>>> +
>>> +/** Add our filter to the connection if it is requested
>>> + */
>>> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
>>> +{
>>> + remoteip_config_t *conf;
>>> + remoteip_conn_config_t *conn_conf;
>>> + int optional;
>>> +
>>> + conf = ap_get_module_config(ap_server_conf->module_config,
>>> + &remoteip_module);
>>> +
>>> + /* Used twice - do the check only once */
>>> + optional = remoteip_addr_in_list(conf->proxy_protocol_optional, c->local_addr);
>>> +
>>> + /* check if we're enabled for this connection */
>>> + if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c->local_addr)
>>> + && !optional )
>>> + || remoteip_addr_in_list(conf->proxy_protocol_disabled, c->local_addr)) {
>>> + return DECLINED;
>>> + }
>>> +
>>> + /* mod_proxy creates outgoing connections - we don't want those */
>>> + if (!remoteip_is_server_port(c->local_addr->port)) {
>>> + return DECLINED;
>>> + }
>>
>> Why is the c->local_addr->port set to a port we are listening on in case of proxy connections?
>>
>>> +
>>> + /* add our filter */
>>> + if (!ap_add_input_filter(remoteip_filter_name, NULL, NULL, c)) {
>>> + /* XXX: Shouldn't this WARN in log? */
>>> + return DECLINED;
>>> + }
>>> +
>>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(03503)
>>> + "RemoteIPProxyProtocol: enabled on connection to %s:%hu",
>>> + c->local_ip, c->local_addr->port);
>>> +
>>> + /* this holds the resolved proxy info for this connection */
>>> + conn_conf = apr_pcalloc(c->pool, sizeof(*conn_conf));
>>> +
>>> + /* Propagate the optional flag so the connection handler knows not to
>>> + abort if the header is mising. NOTE: This means we must check after
>>> + we read the request that the header was NOT optional, too.
>>> + */
>>> + conn_conf->proxy_protocol_optional = optional;
>>> +
>>> + ap_set_module_config(c->conn_config, &remoteip_module, conn_conf);
>>> +
>>> + return OK;
>>> +}
>>> +
>>> +/* Binary format:
>>> + * <sig><cmd><proto><addr-len><addr>
>>> + * sig = \x0D \x0A \x0D \x0A \x00 \x0D \x0A \x51 \x55 \x49 \x54 \x0A
>>> + * cmd = <4-bits-version><4-bits-command>
>>> + * 4-bits-version = \x02
>>> + * 4-bits-command = {\x00|\x01} (\x00 = LOCAL: discard con info; \x01 = PROXY)
>>> + * proto = <4-bits-family><4-bits-protocol>
>>> + * 4-bits-family = {\x00|\x01|\x02|\x03} (AF_UNSPEC, AF_INET, AF_INET6, AF_UNIX)
>>> + * 4-bits-protocol = {\x00|\x01|\x02} (UNSPEC, STREAM, DGRAM)
>>> + */
>>> +static remoteip_parse_status_t remoteip_process_v2_header(conn_rec *c,
>>> + remoteip_conn_config_t *conn_conf,
>>> + proxy_header *hdr)
>>> +{
>>> + apr_status_t ret;
>>> +
>>> + switch (hdr->v2.ver_cmd & 0xF) {
>>> + case 0x01: /* PROXY command */
>>> + switch (hdr->v2.fam) {
>>> + case 0x11: /* TCPv4 */
>>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
>>> + APR_INET,
>>> + ntohs(hdr->v2.addr.ip4.src_port),
>>> + 0, c->pool);
>>> + if (ret != APR_SUCCESS) {
>>> + conn_conf->client_addr = NULL;
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03504)
>>> + "RemoteIPPProxyProtocol: error creating sockaddr");
>>> + return HDR_ERROR;
>>> + }
>>> +
>>> + conn_conf->client_addr->sa.sin.sin_addr.s_addr =
>>> + hdr->v2.addr.ip4.src_addr;
>>> + break;
>>> +
>>> + case 0x21: /* TCPv6 */
>>> +#if APR_HAVE_IPV6
>>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
>>> + APR_INET6,
>>> + ntohs(hdr->v2.addr.ip6.src_port),
>>> + 0, c->pool);
>>> + if (ret != APR_SUCCESS) {
>>> + conn_conf->client_addr = NULL;
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03505)
>>> + "RemoteIPProxyProtocol: error creating sockaddr");
>>> + return HDR_ERROR;
>>> + }
>>> + memcpy(&conn_conf->client_addr->sa.sin6.sin6_addr.s6_addr,
>>> + hdr->v2.addr.ip6.src_addr, 16);
>>> + break;
>>> +#else
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03506)
>>> + "RemoteIPProxyProtocol: APR is not compiled with IPv6 support");
>>> + return HDR_ERROR;
>>> +#endif
>>> + default:
>>> + /* unsupported protocol, keep local connection address */
>>> + return HDR_DONE;
>>> + }
>>> + break; /* we got a sockaddr now */
>>> +
>>> + case 0x00: /* LOCAL command */
>>> + /* keep local connection address for LOCAL */
>>> + return HDR_DONE;
>>> +
>>> + default:
>>> + /* not a supported command */
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03507)
>>> + "RemoteIPProxyProtocol: unsupported command %.2hx",
>>> + hdr->v2.ver_cmd);
>>> + return HDR_ERROR;
>>> + }
>>> +
>>> + /* got address - compute the client_ip from it */
>>> + ret = apr_sockaddr_ip_get(&conn_conf->client_ip, conn_conf->client_addr);
>>> + if (ret != APR_SUCCESS) {
>>> + conn_conf->client_addr = NULL;
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03508)
>>> + "RemoteIPProxyProtocol: error converting address to string");
>>> + return HDR_ERROR;
>>> + }
>>> +
>>> + return HDR_DONE;
>>> +}
>>> +
>>> +/** Determine if this is a v1 or v2 PROXY header.
>>> + */
>>> +static int remoteip_determine_version(conn_rec *c, const char *ptr)
>>> +{
>>> + proxy_header *hdr = (proxy_header *) ptr;
>>> +
>>> + /* assert len >= 14 */
>>> +
>>> + if (memcmp(&hdr->v2, v2sig, sizeof(v2sig)) == 0 &&
>>> + (hdr->v2.ver_cmd & 0xF0) == 0x20) {
>>> + return 2;
>>> + }
>>> + else if (memcmp(hdr->v1.line, "PROXY ", 6) == 0) {
>>> + return 1;
>>> + }
>>> + else {
>>> + return -1;
>>> + }
>>> +}
>>> +
>>> +/* Capture the first bytes on the protocol and parse the proxy protocol header.
>>> + * Removes itself when the header is complete.
>>> + */
>>> +static apr_status_t remoteip_input_filter(ap_filter_t *f,
>>> + apr_bucket_brigade *bb_out,
>>> + ap_input_mode_t mode,
>>> + apr_read_type_e block,
>>> + apr_off_t readbytes)
>>> +{
>>> + apr_status_t ret;
>>> + remoteip_filter_context *ctx = f->ctx;
>>> + remoteip_conn_config_t *conn_conf;
>>> + apr_bucket *b;
>>> + remoteip_parse_status_t psts;
>>> + const char *ptr;
>>> + apr_size_t len;
>>> +
>>> + if (f->c->aborted) {
>>> + return APR_ECONNABORTED;
>>> + }
>>> +
>>> + /* allocate/retrieve the context that holds our header */
>>> + if (!ctx) {
>>> + ctx = f->ctx = apr_palloc(f->c->pool, sizeof(*ctx));
>>> + ctx->rcvd = 0;
>>> + ctx->need = MIN_HDR_LEN;
>>> + ctx->version = 0;
>>> + ctx->mode = AP_MODE_READBYTES;
>>> + ctx->bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
>>> + ctx->done = 0;
>>> + }
>>> +
>>> + if (ctx->done) {
>>> + /* Note: because we're a connection filter we can't remove ourselves
>>> + * when we're done, so we have to stay in the chain and just go into
>>> + * passthrough mode.
>>> + */
>>> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
>>> + }
>>> +
>>> + conn_conf = ap_get_module_config(f->c->conn_config, &remoteip_module);
>>> +
>>> + /* try to read a header's worth of data */
>>> + while (!ctx->done) {
>>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
>>> + ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
>>> + ctx->need - ctx->rcvd);
>>> + if (ret != APR_SUCCESS) {
>>> + return ret;
>>> + }
>>> + }
>>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
>>
>> What about the case of an non blocking read where the upstream filter returns an empty brigade
>> and APR_SUCCESS. This is equal to returning EAGAIN.
>>
>>> + return APR_EOF;
>>> + }
>>> +
>>> + while (!ctx->done && !APR_BRIGADE_EMPTY(ctx->bb)) {
>>> + b = APR_BRIGADE_FIRST(ctx->bb);
>>> +
>>> + ret = apr_bucket_read(b, &ptr, &len, block);
>>> + if (APR_STATUS_IS_EAGAIN(ret) && block == APR_NONBLOCK_READ) {
>>> + return APR_SUCCESS;
>>> + }
>>> + if (ret != APR_SUCCESS) {
>>> + return ret;
>>> + }
>>> +
>>> + memcpy(ctx->header + ctx->rcvd, ptr, len);
>>> + ctx->rcvd += len;
>>> +
>>> + /* Remove instead of delete - we may put this bucket
>>> + back into bb_out if the header was optional and we
>>> + pass down the chain */
>>> + APR_BUCKET_REMOVE(b);
>>> + psts = HDR_NEED_MORE;
>>> +
>>> + if (ctx->version == 0) {
>>> + /* reading initial chunk */
>>> + if (ctx->rcvd >= MIN_HDR_LEN) {
>>> + ctx->version = remoteip_determine_version(f->c, ctx->header);
>>> + if (ctx->version < 0) {
>>> + psts = HDR_MISSING;
>>> + }
>>> + else if (ctx->version == 1) {
>>> + ctx->mode = AP_MODE_GETLINE;
>>> + ctx->need = sizeof(proxy_v1);
>>> + }
>>> + else if (ctx->version == 2) {
>>> + ctx->need = MIN_V2_HDR_LEN;
>>> + }
>>> + }
>>> + }
>>> + else if (ctx->version == 1) {
>>> + psts = remoteip_process_v1_header(f->c, conn_conf,
>>> + (proxy_header *) ctx->header,
>>> + ctx->rcvd, &ctx->need);
>>> + }
>>> + else if (ctx->version == 2) {
>>> + if (ctx->rcvd >= MIN_V2_HDR_LEN) {
>>> + ctx->need = MIN_V2_HDR_LEN +
>>> + ntohs(((proxy_header *) ctx->header)->v2.len);
>>> + }
>>> + if (ctx->rcvd >= ctx->need) {
>>> + psts = remoteip_process_v2_header(f->c, conn_conf,
>>> + (proxy_header *) ctx->header);
>>> + }
>>> + }
>>> + else {
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03509)
>>> + "RemoteIPProxyProtocol: internal error: unknown version "
>>> + "%d", ctx->version);
>>> + f->c->aborted = 1;
>>> + apr_bucket_delete(b);
>>> + apr_brigade_destroy(ctx->bb);
>>> + return APR_ECONNABORTED;
>>> + }
>>> +
>>> + switch (psts) {
>>> + case HDR_MISSING:
>>> + if (conn_conf->proxy_protocol_optional) {
>>> + /* Same as DONE, but don't delete the bucket. Rather, put it
>>> + back into the brigade and move the request along the stack */
>>> + ctx->done = 1;
>>> + APR_BRIGADE_INSERT_HEAD(bb_out, b);
>>
>> See below. We need to restore all buckets. What if the original read was speculative?
>>
>>> + return ap_pass_brigade(f->next, ctx->bb);
>>
>> Why using ap_pass_brigade on f->next? We are an input filter and f->next is our upstream input filter.
>>
>>> + }
>>> + else {
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03510)
>>> + "RemoteIPProxyProtocol: no valid PROXY header found");
>>> + /* fall through to error case */
>>> + }
>>> + case HDR_ERROR:
>>> + f->c->aborted = 1;
>>> + apr_bucket_delete(b);
>>> + apr_brigade_destroy(ctx->bb);
>>> + return APR_ECONNABORTED;
>>> +
>>> + case HDR_DONE:
>>> + apr_bucket_delete(b);
>>> + ctx->done = 1;
>>> + break;
>>> +
>>> + case HDR_NEED_MORE:
>>> + /* It is safe to delete this bucket if we get here since we know
>>> + that we are definitely processing a header (we've read enough to
>>> + know if the signatures exist on the line) */
>>
>> No. We might not even received MIN_HDR_LEN data so far and thus did not check for the signature
>> so far. We need to safe all the buckets that we might need to restore in a separate bucket brigade,
>> that we need to be set aside before doing the next ap_get_brigade to avoid killing transient buckets.
>>
>>> + apr_bucket_delete(b);
>>> + break;
>>> + }
>>> + }
>>> + }
>>> +
>>> + /* we only get here when done == 1 */
>>> + if (psts == HDR_DONE) {
>>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03511)
>>> + "RemoteIPProxyProtocol: received valid PROXY header: %s:%hu",
>>> + conn_conf->client_ip, conn_conf->client_addr->port);
>>> + }
>>> + else {
>>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03512)
>>> + "RemoteIPProxyProtocol: PROXY header was missing");
>>> + }
>>> +
>>> + if (ctx->rcvd > ctx->need || !APR_BRIGADE_EMPTY(ctx->bb)) {
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03513)
>>> + "RemoteIPProxyProtocol: internal error: have data left over; "
>>> + " need=%lu, rcvd=%lu, brigade-empty=%d", ctx->need,
>>> + ctx->rcvd, APR_BRIGADE_EMPTY(ctx->bb));
>>> + f->c->aborted = 1;
>>> + apr_brigade_destroy(ctx->bb);
>>> + return APR_ECONNABORTED;
>>> + }
>>> +
>>> + /* clean up */
>>> + apr_brigade_destroy(ctx->bb);
>>> + ctx->bb = NULL;
>>> +
>>> + /* now do the real read for the upper layer */
>>> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
>>> +}
>>> +
>>> static const command_rec remoteip_cmds[] =
>>> {
>>> AP_INIT_TAKE1("RemoteIPHeader", header_name_set, NULL, RSRC_CONF,
>>
>> Regards
>>
>> Rüdiger
>>
Re: svn commit: r1776575 - in /httpd/httpd/trunk:
docs/log-message-tags/next-number docs/manual/mod/mod_remoteip.xml
modules/metadata/mod_remoteip.c
Posted by Ruediger Pluem <rp...@apache.org>.
Anyone?
Regards
R�diger
On 01/10/2017 12:39 PM, Ruediger Pluem wrote:
>
>
> On 12/30/2016 03:20 PM, druggeri@apache.org wrote:
>> 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
>>
>
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
>> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Fri Dec 30 14:20:48 2016
>
>> @@ -427,6 +730,464 @@ static int remoteip_modify_request(reque
>> return OK;
>> }
>>
>> +static int remoteip_is_server_port(apr_port_t port)
>> +{
>> + ap_listen_rec *lr;
>> +
>> + for (lr = ap_listeners; lr; lr = lr->next) {
>> + if (lr->bind_addr && lr->bind_addr->port == port) {
>> + return 1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Human readable format:
>> + * PROXY {TCP4|TCP6|UNKNOWN} <client-ip-addr> <dest-ip-addr> <client-port> <dest-port><CR><LF>
>> + */
>> +static remoteip_parse_status_t remoteip_process_v1_header(conn_rec *c,
>> + remoteip_conn_config_t *conn_conf,
>> + proxy_header *hdr, apr_size_t len,
>> + apr_size_t *hdr_len)
>> +{
>> + char *end, *word, *host, *valid_addr_chars, *saveptr;
>> + char buf[sizeof(hdr->v1.line)];
>> + apr_port_t port;
>> + apr_status_t ret;
>> + apr_int32_t family;
>> +
>> +#define GET_NEXT_WORD(field) \
>> + word = apr_strtok(NULL, " ", &saveptr); \
>> + if (!word) { \
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03497) \
>> + "RemoteIPProxyProtocol: no " field " found in header '%s'", \
>> + hdr->v1.line); \
>> + return HDR_ERROR; \
>> + }
>> +
>> + end = memchr(hdr->v1.line, '\r', len - 1);
>> + if (!end || end[1] != '\n') {
>> + return HDR_NEED_MORE; /* partial or invalid header */
>> + }
>> +
>> + *end = '\0';
>> + *hdr_len = end + 2 - hdr->v1.line; /* skip header + CRLF */
>> +
>> + /* parse in separate buffer so have the original for error messages */
>> + strcpy(buf, hdr->v1.line);
>> +
>> + apr_strtok(buf, " ", &saveptr);
>> +
>> + /* parse family */
>> + GET_NEXT_WORD("family")
>> + if (strcmp(word, "UNKNOWN") == 0) {
>> + conn_conf->client_addr = c->client_addr;
>> + conn_conf->client_ip = c->client_ip;
>> + return HDR_DONE;
>> + }
>> + else if (strcmp(word, "TCP4") == 0) {
>> + family = APR_INET;
>> + valid_addr_chars = "0123456789.";
>> + }
>> + else if (strcmp(word, "TCP6") == 0) {
>> +#if APR_HAVE_IPV6
>> + family = APR_INET6;
>> + valid_addr_chars = "0123456789abcdefABCDEF:";
>> +#else
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03498)
>> + "RemoteIPProxyProtocol: Unable to parse v6 address - APR is not compiled with IPv6 support",
>> + word, hdr->v1.line);
>> + return HDR_ERROR;
>> +#endif
>> + }
>> + else {
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03499)
>> + "RemoteIPProxyProtocol: unknown family '%s' in header '%s'",
>> + word, hdr->v1.line);
>> + return HDR_ERROR;
>> + }
>> +
>> + /* parse client-addr */
>> + GET_NEXT_WORD("client-address")
>> +
>> + if (strspn(word, valid_addr_chars) != strlen(word)) {
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03500)
>> + "RemoteIPProxyProtocol: invalid client-address '%s' found in "
>> + "header '%s'", word, hdr->v1.line);
>> + return HDR_ERROR;
>> + }
>> +
>> + host = word;
>> +
>> + /* parse dest-addr */
>> + GET_NEXT_WORD("destination-address")
>> +
>> + /* parse client-port */
>> + GET_NEXT_WORD("client-port")
>> + if (sscanf(word, "%hu", &port) != 1) {
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03501)
>> + "RemoteIPProxyProtocol: error parsing port '%s' in header '%s'",
>> + word, hdr->v1.line);
>> + return HDR_ERROR;
>> + }
>> +
>> + /* parse dest-port */
>> + /* GET_NEXT_WORD("destination-port") - no-op since we don't care about it */
>> +
>> + /* create a socketaddr from the info */
>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, host, family, port, 0,
>> + c->pool);
>> + if (ret != APR_SUCCESS) {
>> + conn_conf->client_addr = NULL;
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03502)
>> + "RemoteIPProxyProtocol: error converting family '%d', host '%s',"
>> + " and port '%hu' to sockaddr; header was '%s'",
>> + family, host, port, hdr->v1.line);
>> + return HDR_ERROR;
>> + }
>> +
>> + conn_conf->client_ip = apr_pstrdup(c->pool, host);
>> +
>> + return HDR_DONE;
>> +}
>> +
>> +/** Add our filter to the connection if it is requested
>> + */
>> +static int remoteip_hook_pre_connection(conn_rec *c, void *csd)
>> +{
>> + remoteip_config_t *conf;
>> + remoteip_conn_config_t *conn_conf;
>> + int optional;
>> +
>> + conf = ap_get_module_config(ap_server_conf->module_config,
>> + &remoteip_module);
>> +
>> + /* Used twice - do the check only once */
>> + optional = remoteip_addr_in_list(conf->proxy_protocol_optional, c->local_addr);
>> +
>> + /* check if we're enabled for this connection */
>> + if ((!remoteip_addr_in_list(conf->proxy_protocol_enabled, c->local_addr)
>> + && !optional )
>> + || remoteip_addr_in_list(conf->proxy_protocol_disabled, c->local_addr)) {
>> + return DECLINED;
>> + }
>> +
>> + /* mod_proxy creates outgoing connections - we don't want those */
>> + if (!remoteip_is_server_port(c->local_addr->port)) {
>> + return DECLINED;
>> + }
>
> Why is the c->local_addr->port set to a port we are listening on in case of proxy connections?
>
>> +
>> + /* add our filter */
>> + if (!ap_add_input_filter(remoteip_filter_name, NULL, NULL, c)) {
>> + /* XXX: Shouldn't this WARN in log? */
>> + return DECLINED;
>> + }
>> +
>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(03503)
>> + "RemoteIPProxyProtocol: enabled on connection to %s:%hu",
>> + c->local_ip, c->local_addr->port);
>> +
>> + /* this holds the resolved proxy info for this connection */
>> + conn_conf = apr_pcalloc(c->pool, sizeof(*conn_conf));
>> +
>> + /* Propagate the optional flag so the connection handler knows not to
>> + abort if the header is mising. NOTE: This means we must check after
>> + we read the request that the header was NOT optional, too.
>> + */
>> + conn_conf->proxy_protocol_optional = optional;
>> +
>> + ap_set_module_config(c->conn_config, &remoteip_module, conn_conf);
>> +
>> + return OK;
>> +}
>> +
>> +/* Binary format:
>> + * <sig><cmd><proto><addr-len><addr>
>> + * sig = \x0D \x0A \x0D \x0A \x00 \x0D \x0A \x51 \x55 \x49 \x54 \x0A
>> + * cmd = <4-bits-version><4-bits-command>
>> + * 4-bits-version = \x02
>> + * 4-bits-command = {\x00|\x01} (\x00 = LOCAL: discard con info; \x01 = PROXY)
>> + * proto = <4-bits-family><4-bits-protocol>
>> + * 4-bits-family = {\x00|\x01|\x02|\x03} (AF_UNSPEC, AF_INET, AF_INET6, AF_UNIX)
>> + * 4-bits-protocol = {\x00|\x01|\x02} (UNSPEC, STREAM, DGRAM)
>> + */
>> +static remoteip_parse_status_t remoteip_process_v2_header(conn_rec *c,
>> + remoteip_conn_config_t *conn_conf,
>> + proxy_header *hdr)
>> +{
>> + apr_status_t ret;
>> +
>> + switch (hdr->v2.ver_cmd & 0xF) {
>> + case 0x01: /* PROXY command */
>> + switch (hdr->v2.fam) {
>> + case 0x11: /* TCPv4 */
>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
>> + APR_INET,
>> + ntohs(hdr->v2.addr.ip4.src_port),
>> + 0, c->pool);
>> + if (ret != APR_SUCCESS) {
>> + conn_conf->client_addr = NULL;
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03504)
>> + "RemoteIPPProxyProtocol: error creating sockaddr");
>> + return HDR_ERROR;
>> + }
>> +
>> + conn_conf->client_addr->sa.sin.sin_addr.s_addr =
>> + hdr->v2.addr.ip4.src_addr;
>> + break;
>> +
>> + case 0x21: /* TCPv6 */
>> +#if APR_HAVE_IPV6
>> + ret = apr_sockaddr_info_get(&conn_conf->client_addr, NULL,
>> + APR_INET6,
>> + ntohs(hdr->v2.addr.ip6.src_port),
>> + 0, c->pool);
>> + if (ret != APR_SUCCESS) {
>> + conn_conf->client_addr = NULL;
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03505)
>> + "RemoteIPProxyProtocol: error creating sockaddr");
>> + return HDR_ERROR;
>> + }
>> + memcpy(&conn_conf->client_addr->sa.sin6.sin6_addr.s6_addr,
>> + hdr->v2.addr.ip6.src_addr, 16);
>> + break;
>> +#else
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03506)
>> + "RemoteIPProxyProtocol: APR is not compiled with IPv6 support");
>> + return HDR_ERROR;
>> +#endif
>> + default:
>> + /* unsupported protocol, keep local connection address */
>> + return HDR_DONE;
>> + }
>> + break; /* we got a sockaddr now */
>> +
>> + case 0x00: /* LOCAL command */
>> + /* keep local connection address for LOCAL */
>> + return HDR_DONE;
>> +
>> + default:
>> + /* not a supported command */
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03507)
>> + "RemoteIPProxyProtocol: unsupported command %.2hx",
>> + hdr->v2.ver_cmd);
>> + return HDR_ERROR;
>> + }
>> +
>> + /* got address - compute the client_ip from it */
>> + ret = apr_sockaddr_ip_get(&conn_conf->client_ip, conn_conf->client_addr);
>> + if (ret != APR_SUCCESS) {
>> + conn_conf->client_addr = NULL;
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, c, APLOGNO(03508)
>> + "RemoteIPProxyProtocol: error converting address to string");
>> + return HDR_ERROR;
>> + }
>> +
>> + return HDR_DONE;
>> +}
>> +
>> +/** Determine if this is a v1 or v2 PROXY header.
>> + */
>> +static int remoteip_determine_version(conn_rec *c, const char *ptr)
>> +{
>> + proxy_header *hdr = (proxy_header *) ptr;
>> +
>> + /* assert len >= 14 */
>> +
>> + if (memcmp(&hdr->v2, v2sig, sizeof(v2sig)) == 0 &&
>> + (hdr->v2.ver_cmd & 0xF0) == 0x20) {
>> + return 2;
>> + }
>> + else if (memcmp(hdr->v1.line, "PROXY ", 6) == 0) {
>> + return 1;
>> + }
>> + else {
>> + return -1;
>> + }
>> +}
>> +
>> +/* Capture the first bytes on the protocol and parse the proxy protocol header.
>> + * Removes itself when the header is complete.
>> + */
>> +static apr_status_t remoteip_input_filter(ap_filter_t *f,
>> + apr_bucket_brigade *bb_out,
>> + ap_input_mode_t mode,
>> + apr_read_type_e block,
>> + apr_off_t readbytes)
>> +{
>> + apr_status_t ret;
>> + remoteip_filter_context *ctx = f->ctx;
>> + remoteip_conn_config_t *conn_conf;
>> + apr_bucket *b;
>> + remoteip_parse_status_t psts;
>> + const char *ptr;
>> + apr_size_t len;
>> +
>> + if (f->c->aborted) {
>> + return APR_ECONNABORTED;
>> + }
>> +
>> + /* allocate/retrieve the context that holds our header */
>> + if (!ctx) {
>> + ctx = f->ctx = apr_palloc(f->c->pool, sizeof(*ctx));
>> + ctx->rcvd = 0;
>> + ctx->need = MIN_HDR_LEN;
>> + ctx->version = 0;
>> + ctx->mode = AP_MODE_READBYTES;
>> + ctx->bb = apr_brigade_create(f->c->pool, f->c->bucket_alloc);
>> + ctx->done = 0;
>> + }
>> +
>> + if (ctx->done) {
>> + /* Note: because we're a connection filter we can't remove ourselves
>> + * when we're done, so we have to stay in the chain and just go into
>> + * passthrough mode.
>> + */
>> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
>> + }
>> +
>> + conn_conf = ap_get_module_config(f->c->conn_config, &remoteip_module);
>> +
>> + /* try to read a header's worth of data */
>> + while (!ctx->done) {
>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
>> + ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
>> + ctx->need - ctx->rcvd);
>> + if (ret != APR_SUCCESS) {
>> + return ret;
>> + }
>> + }
>> + if (APR_BRIGADE_EMPTY(ctx->bb)) {
>
> What about the case of an non blocking read where the upstream filter returns an empty brigade
> and APR_SUCCESS. This is equal to returning EAGAIN.
>
>> + return APR_EOF;
>> + }
>> +
>> + while (!ctx->done && !APR_BRIGADE_EMPTY(ctx->bb)) {
>> + b = APR_BRIGADE_FIRST(ctx->bb);
>> +
>> + ret = apr_bucket_read(b, &ptr, &len, block);
>> + if (APR_STATUS_IS_EAGAIN(ret) && block == APR_NONBLOCK_READ) {
>> + return APR_SUCCESS;
>> + }
>> + if (ret != APR_SUCCESS) {
>> + return ret;
>> + }
>> +
>> + memcpy(ctx->header + ctx->rcvd, ptr, len);
>> + ctx->rcvd += len;
>> +
>> + /* Remove instead of delete - we may put this bucket
>> + back into bb_out if the header was optional and we
>> + pass down the chain */
>> + APR_BUCKET_REMOVE(b);
>> + psts = HDR_NEED_MORE;
>> +
>> + if (ctx->version == 0) {
>> + /* reading initial chunk */
>> + if (ctx->rcvd >= MIN_HDR_LEN) {
>> + ctx->version = remoteip_determine_version(f->c, ctx->header);
>> + if (ctx->version < 0) {
>> + psts = HDR_MISSING;
>> + }
>> + else if (ctx->version == 1) {
>> + ctx->mode = AP_MODE_GETLINE;
>> + ctx->need = sizeof(proxy_v1);
>> + }
>> + else if (ctx->version == 2) {
>> + ctx->need = MIN_V2_HDR_LEN;
>> + }
>> + }
>> + }
>> + else if (ctx->version == 1) {
>> + psts = remoteip_process_v1_header(f->c, conn_conf,
>> + (proxy_header *) ctx->header,
>> + ctx->rcvd, &ctx->need);
>> + }
>> + else if (ctx->version == 2) {
>> + if (ctx->rcvd >= MIN_V2_HDR_LEN) {
>> + ctx->need = MIN_V2_HDR_LEN +
>> + ntohs(((proxy_header *) ctx->header)->v2.len);
>> + }
>> + if (ctx->rcvd >= ctx->need) {
>> + psts = remoteip_process_v2_header(f->c, conn_conf,
>> + (proxy_header *) ctx->header);
>> + }
>> + }
>> + else {
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03509)
>> + "RemoteIPProxyProtocol: internal error: unknown version "
>> + "%d", ctx->version);
>> + f->c->aborted = 1;
>> + apr_bucket_delete(b);
>> + apr_brigade_destroy(ctx->bb);
>> + return APR_ECONNABORTED;
>> + }
>> +
>> + switch (psts) {
>> + case HDR_MISSING:
>> + if (conn_conf->proxy_protocol_optional) {
>> + /* Same as DONE, but don't delete the bucket. Rather, put it
>> + back into the brigade and move the request along the stack */
>> + ctx->done = 1;
>> + APR_BRIGADE_INSERT_HEAD(bb_out, b);
>
> See below. We need to restore all buckets. What if the original read was speculative?
>
>> + return ap_pass_brigade(f->next, ctx->bb);
>
> Why using ap_pass_brigade on f->next? We are an input filter and f->next is our upstream input filter.
>
>> + }
>> + else {
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03510)
>> + "RemoteIPProxyProtocol: no valid PROXY header found");
>> + /* fall through to error case */
>> + }
>> + case HDR_ERROR:
>> + f->c->aborted = 1;
>> + apr_bucket_delete(b);
>> + apr_brigade_destroy(ctx->bb);
>> + return APR_ECONNABORTED;
>> +
>> + case HDR_DONE:
>> + apr_bucket_delete(b);
>> + ctx->done = 1;
>> + break;
>> +
>> + case HDR_NEED_MORE:
>> + /* It is safe to delete this bucket if we get here since we know
>> + that we are definitely processing a header (we've read enough to
>> + know if the signatures exist on the line) */
>
> No. We might not even received MIN_HDR_LEN data so far and thus did not check for the signature
> so far. We need to safe all the buckets that we might need to restore in a separate bucket brigade,
> that we need to be set aside before doing the next ap_get_brigade to avoid killing transient buckets.
>
>> + apr_bucket_delete(b);
>> + break;
>> + }
>> + }
>> + }
>> +
>> + /* we only get here when done == 1 */
>> + if (psts == HDR_DONE) {
>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03511)
>> + "RemoteIPProxyProtocol: received valid PROXY header: %s:%hu",
>> + conn_conf->client_ip, conn_conf->client_addr->port);
>> + }
>> + else {
>> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, f->c, APLOGNO(03512)
>> + "RemoteIPProxyProtocol: PROXY header was missing");
>> + }
>> +
>> + if (ctx->rcvd > ctx->need || !APR_BRIGADE_EMPTY(ctx->bb)) {
>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(03513)
>> + "RemoteIPProxyProtocol: internal error: have data left over; "
>> + " need=%lu, rcvd=%lu, brigade-empty=%d", ctx->need,
>> + ctx->rcvd, APR_BRIGADE_EMPTY(ctx->bb));
>> + f->c->aborted = 1;
>> + apr_brigade_destroy(ctx->bb);
>> + return APR_ECONNABORTED;
>> + }
>> +
>> + /* clean up */
>> + apr_brigade_destroy(ctx->bb);
>> + ctx->bb = NULL;
>> +
>> + /* now do the real read for the upper layer */
>> + return ap_get_brigade(f->next, bb_out, mode, block, readbytes);
>> +}
>> +
>> static const command_rec remoteip_cmds[] =
>> {
>> AP_INIT_TAKE1("RemoteIPHeader", header_name_set, NULL, RSRC_CONF,
>
> Regards
>
> R�diger
>