httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Ruggeri <DRugg...@primary.net>
Subject 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
Date Thu, 26 Jan 2017 00:53:46 GMT
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 <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 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



Mime
View raw message