Making http_auth_request_module a first-class citizen? [patch]

[Max]

17 февраля 2012, 15:19 от Maxim Dounin <mdounin at mdounin.ru>:
> On Fri, Feb 17, 2012 at 12:28:14PM +0400, Max wrote:
> > Maxim, you haven't even taken a look at my patch, have you? Because
> > if you had, you wouldn't have made such unsubstantiated claims.
> 
> I have, despite the fact that it was provided as a link only.

I usually send in patches on postcards, but I had run out of postcards.
I'm glad this Internet thing seems to work, although your replies seem
to indicate your download request had header_only set to 1. :-P

> I stand corrected. Your patch broke only $request variable, not
> the $request_method (which always comes from the main request).

You're wrong, my patch did not break anything. Go ahead and put
any variables in those /private/ and /auth location blocks and
see for yourself. I know it must be difficult to decipher, but
could you try to guess what the following part of my patch does?

64 +        /*
65 +         * 1) Allocate a new request line string
66 +         *    (4 extra bytes for future compatibility just in case
67 +         *    a single letter HTTP request method is introduced).
68 +         */
69 +
70 +        request_line.data = ngx_pcalloc(r->pool, sr->request_line.len + 4);
71 +        if (request_line.data == NULL) {
72 +            return NGX_ERROR;
73 +        }


Feel free to verify this for yourself AGAIN:

location /private/ {
    auth_request /auth;
    set $request_private $request;
    set $request_uri_private $request_uri;
    set $request_method_private $request_method;
}

location = /auth {
    set $request_auth $request;
    set $request_uri_auth $request_uri;
    set $request_method_auth $request_method;
}

Here's the debug log output:

1) Inside the /private/ location block:

http script var: "GET /private/test HTTP/1.1"
http script set $request_private

http script var: "/private/test"
http script set $request_uri_private

http script var: "GET"
http script set $request_method_private


2) Inside the /auth location block:

http script var: "GET /private/test HTTP/1.1"
http script set $request_auth

http script var: "/private/test"
http script set $request_uri_auth

http script var: "GET"
http script set $request_method_auth

Here's the gdb session output:

322         sr->header_only = 1; /* <--- Look, it's your old friend! */
(gdb)
324         ctx->subrequest = sr;
(gdb)
326         ngx_http_set_ctx(r, ctx, ngx_http_auth_request_module);
(gdb)
328         return NGX_AGAIN;

(gdb) print &r->request_line
$1 = (ngx_str_t *) 0x48b06d88 <--- Take a good look.
(gdb) print r->request_line
$2 = {len = 26, data = 0x48b67000 "GET /private/test HTTP/1.1\r\nUser-Agent"}
                       ^^^^^^^^^^- Take a good look at this, too.

(gdb) print &sr->request_line
$3 = (ngx_str_t *) 0x48b96874 <--- Does that look like 0x48b06d88 to you?
(gdb) print sr->request_line
$4 = {len = 27, data = 0x48b96c64 "HEAD /private/test HTTP/1.1"}
                       ^^^^^^^^^^- Does that look like 0x48b67000 to you?

(gdb) print sizeof("GET /private/test HTTP/1.1")-1
$5 = 26
(gdb) print sizeof("HEAD /private/test HTTP/1.1")-1
$6 = 27

(gdb) print r->method
$7 = 2
(gdb) print r->method_name
$8 = {len = 3, data = 0x48b67000 "GET /private/test HTTP/1.1\r\nUser-Agent"}

(gdb) print sr->method
$9 = 4
(gdb) print sr->method_name
$10 = {len = 4, data = 0x81733f2 "HEAD "}

In your previous reply to my post you wrote:

> If you handle auth subrequests with proxy_pass, you may use
> proxy_set_method to issue HEAD requests to backend.  Or you may
> use correct auth endpoint which doesn't return unneeded data.

And in your latest reply you wrote:

> Again: the sr->header_only workaround is anyway required, as
> static file, or memcached, or fastcgi may be used as handler for
> auth subrequest (and, actually, even some broken http backends may
> return data to HEAD, not even talking about intended changes of
> proxy_method). Without sr->header_only explicitly set you
> will get response content before headers of the real response:

First you argued that one should use "correct" authentication endpoints
that do not return unneeded data, and now you're trying to make it seem
like your workaround is there to allow the auth_request module to
work with "incorrect" authentication endpoints that violate HTTP itself
and other established protocols, but the main reason your workaround
is there has to do with the fact that you don't know how to handle
the request body without crashing nginx. That is also why you need
to redesign your module if you want it to work with caching.

> And, BTW, as far as I recall your code, it won't set
> sr->header_only in case of HEAD requests. This is wrong, you
> still need it even for HEADs.
>
> > I left your workaround the way it is to prevent people from shooting
> > themselves in the foot by setting the proxy method to GET, but those
> > who know about the proxy_method directive (BTW, you got the name wrong,
> > there is no proxy_set_method directive), should know what they are doing.

You're wrong, AGAIN. I left the workaround the way it is - do I hear
an echo? Make a claim. Have your claim proven wrong by the next
paragraph you quote. Keep the wrong claim and the quoted paragraph
that proves you wrong in your reply anyway. Priceless.

> Your patch tries to make the workaround a bit more smart, and
> tries to make arbitrary configuration more efficient, but this is
> wrong aproach: instead, it should be made less intrusive.
>
> The major problem with the workaround as of now is that it
> prevents caching. And *this* should be addressed.

My patch solves the GET initiated flood problem nicely without
breaking anything, but it does not address the broken design
of your module that's preventing caching in the first place.

So instead of trying to prove me wrong and getting yourself
proven wrong again and again (which you might find personally
intrusive and embarrassing), maybe you could address the broken
design of your module?

> On the other hand, setting method/request line increase comlexity
> and overhead in normal case, as well as subject to bugs (at least
> two were identified above).

Are you serious? You're comparing the complexity and overhead of
completely in-memory operations (memory allocation, assignment,
request line scanning and two memcpy() calls) on a few dozen
bytes to reading from disk, processing, buffering and sending
hundreds of kilobytes on a closed connection?

Just to put things in perspective I did some profiling with
nanosecond resolution and it turns out that on the oldest single-core
CPU server I have access to, my patch adds 2550 ns (on average) to the
overall processing time. Using the memchr() function instead of the
ngx_strlchr() function to scan for the space character and scanning
only up to the 8th byte (because currently the longest HTTP method
request name is 7 characters long) gives roughly the same results
(2600 ns on average), while using an optimized for loop directly
shaves off another 200 ns, on average. Even without any
micro-optimization, my patch adds less than 3 microseconds
(0.003 ms) to the overall processing time on a very old server
that you're unlikely to find in any production environment.
3000 nanoseconds. Horrible, isn't it?

As for bugs, please do feel free to point them out.

> Hope my position is clear enough

It's clear that you keep making unsubstantiated claims that keep
getting proven wrong, and that you'd rather spend 10 minutes
writing a post to try to make it look like I broke your beloved
module with my patch instead of taking 5 minutes to apply my patch,
recompile nginx, and step through the patched function in a debugger
to see what my patch really does, because you obviously do not
understand what is going on in there.

You seem to prefer conjecture to verifiable facts, so I see no point in
discussing this further.

Max