httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Plüm, Rüdiger, Vodafone Group <ruediger.pl...@vodafone.com>
Subject 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
Date Tue, 17 Jan 2017 09:48:48 GMT


> -----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 <rpluem@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
Mime
View raw message