Making http_auth_request_module a first-class citizen? [patch]

[Max]

Hello!

16 февраля 2012, 20:07 от Maxim Dounin <mdounin at mdounin.ru>:
> Hello!
> 
> On Thu, Feb 16, 2012 at 08:16:03AM +0400, Max wrote:
> 
> > 
> > 15 февраля 2012, 18:50 от Maxim Dounin <mdounin at mdounin.ru>:
> > > Hello!
> > > 
> > > On Wed, Feb 15, 2012 at 08:56:49AM -0500, Maxim Khitrov wrote:
> > > 
> > > > Hello Maxim,
> > > > 
> > > > Back in 2010 you wrote that it's not likely that your
> > > > http_auth_request_module would make it into nginx core. I'm curious if
> > > > anything has changed over the past two years?
> > > > 
> > > > It's not that compiling this module into nginx is a problem
> > > > (especially on FreeBSD), but I think a lot of people are inherently
> > > > weary of depending on 3rd-party modules, since there is no guarantee
> > > > of continued support.
> > > > 
> > > > What do you think about adding your module to the main nginx repository?
> > > 
> > > There are no immediate plans, but this may happen somewhere in the 
> > > future.
> > 
> > Hello fellow Maxims and others,
> > 
> > I took a closer look at the auth_request module source code today and
> > realized that I was partially wrong about auth_request authorization
> > subrequests causing the entire requested file to be retrieved from the
> > backend server. I apologize for the confusion my posts may have
> > caused. Due to sr->header_only being set to 1, the connection to the
> > backend server is terminated from within ngx_http_upstream_send_response()
> > as soon as the HTTP request status code is received.
> 
> Yes.  This is basically a workaround for cases when people 
> unintentionally return data to auth subrequest, it makes sure that 
> no unexpected data are sent to client in any case.
> 
> [...]
> 
> > All of these issues can be avoided simply by using HEAD method
> > requests for authorization subrequests. According to my
> 
> Using HEAD is not an option in auth_request itself, as it doesn't 
> know how auth subrequest will be handled.  E.g. it may be passed to 
> fastcgi, or even hit static file.
> 
> 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.
> 
> [...]
> 
> > I have also modified the auth_request module to use HEAD method
> > authorization subrequests by default. This setting can be
> > overridden in the configuration file by using the proxy_method
> > directive, of course.
> > 
> > You can find my auth_request module patch here:
> > 
> >
> https://nginxyzpro.berlios.de/patch-head.ngx_http_auth_request_module.c.20120215.diff
> 
> The patch is wrong by design, see above.  Moreover, it makes it 
> impossible to correctly pass original request method to auth 
> endpoint.

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.

First of all, I am referring to subrequests in the context of
subquests created by the ngx_http_subrequest() function.
As you might recall, the auth_request function
ngx_http_auth_request_handler() calls the ngx_http_subrequest()
function to create a subrequest, which inherits most of the values
from the original request, BUT the method values are not
inherited by the subrequest - they are explicitly set to the
GET method:

nginx-1.1.15/src/http/ngx_http_core_module.c:

2361 ngx_http_subrequest(ngx_http_request_t *r,
2362     ngx_str_t *uri, ngx_str_t *args, ngx_http_request_t **psr,
2363     ngx_http_post_subrequest_t *ps, ngx_uint_t flags)
2364 {
...
2417     sr->method = NGX_HTTP_GET;
2434     sr->method_name = ngx_http_core_get_method;
...
2487 }

So your auth_request module NEVER DID pass original request
methods on to subrequests in the first place! By default all
auth_request subrequests using the proxy_pass directive have been,
still are, and will be (until my patch is applied) GET method requests.

My patch changes the default method from GET to HEAD by changing
sr->method, sr->method_name and sr->request_line accordingly.
It does NOT in any way interfere with anything else, you can
use whatever authentication mechanism and endpoint you
want - static files, fastcgi_pass, postgres_pass, etc.

Moreover, you are wrong about my patch making it impossible
to correctly pass the original request method to authentication
endpoints. The original request is fully preserved (along with
the entire original request) and can be accessed through the
$request_method variable inside the subrequest location block.
Feel free to verify this for yourself, if you don't believe me:

location /private/ {
    auth_request /auth;
    set $request_method_private $request_method;
}

location = /auth {
    set $request_method_auth $request_method;
}

Here's what you'll see:

Location   Original module (v0.2)     Patched module (v20120215)
---------  ----------------------     -----------------------------
           GET /private/ HTTP/1.0     GET /private/ HTTP/1.0
/private/  $request_method: GET       $request_method: GET
/auth      $request_method: GET       $request_method: GET (intact)
           subrequest method: GET     subrequest method: HEAD

           HEAD /private/ HTTP/1.0    HEAD /private/ HTTP/1.0
/private/  $request_method: HEAD      $request_method: HEAD
/auth      $request_method: HEAD      $request_method: HEAD (intact)
           subrequest method: GET     subrequest method: HEAD

My patch simply makes the proxy_pass directive use the HEAD request
method by default in auth_request subrequest location blocks.

The same can be achieved by using the proxy_method directive
("proxy_method HEAD;"), but I believe the HEAD method should be used
by default. Why? Because, IMNSHO, it does everything the GET method does
(including Basic access authentication WWW-Authenticate / Authorization
header exchange), but in a much more elegant and efficient way, which
also makes your sr->header_only=1 workaround unnecessary.

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.

BTW, the proxy_method directive is missing from the official
documentation, so you may want to fix that:

http://nginx.org/en/docs/http/ngx_http_proxy_module.html

Most people might be using other authentication methods that this
patch in no way affects, but those who do use the pass_proxy directive
for subrequests would benefit from my patch without losing any of the
functionality. Moreover, if they really need to use the GET method,
or any other method, they still can.

I did some more testing and it turns out that even under moderate load
the backend server keeps sending another 150-200 kb before detecting
the frontend server had closed the connection on its end. Compare
that to 1200-1400 BYTES using the HEAD method even under heavy
load. With large files and per-file access rules on the backend server
that means ten GET method subrequests per second are wasting at least
10 Mbps worth of bandwidth when 100 kbps would have done the job in a
way that would also help reduce the load on the backend server.

So my question to you is: why would you not want to optimize your module?
I thought nginx was supposed to be about efficiency.

All my claims made here are based on facts and can be verified by
anyone who has the time, the willingness, and the capacity to
understand and apply what I've written. I know you understand
what I've written, but if you're too busy or can't be bothered to deal
with this, just say so, but please don't jump to conclusions or make
unsubstantiated claims based on what you think I MIGHT have
meant instead of checking the facts because such claims will
only discredit you and alienate potential developers.

Max