> -----Ursprüngliche Nachricht-----
> Von: Daniel Ruggeri [mailto:drugg...@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 <rpl...@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, drugg...@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
