Making http_auth_request_module a first-class citizen? [patch]

[Maxim Dounin]

Hello!

On Fri, Feb 17, 2012 at 12:28:14PM +0400, Max wrote:

> 
> 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.

I have, despite the fact that it was provided as a link only.

[...]

> 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:

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

[...]

> 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.

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:

HEAD / HTTP/1.0

BOOM
HTTP/1.1 200 OK
Server: nginx/1.1.15
Date: Fri, 17 Feb 2012 10:11:35 GMT
Content-Type: text/html
Content-Length: 1047
Connection: close
Last-Modified: Mon, 13 Feb 2012 01:20:52 GMT
Accept-Ranges: bytes

The "BOOM" string is from static file used as auth_request 
handler.  (Obviously the sr->header_only workaround was removed 
for testing.)

If the question was about proxy only, I wouldn't added the 
workaround in the first place, but recommended proxy_method in 
docs instead.

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.

As I already said before, the whole sr->header_only thing is a 
workaround to prevent people from unintentionally breaking 
protocol.

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.

> 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

This will be addressed in near future.

[...]

> 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.

Both using proxy_method and using auth endpoint which doesn't 
return data do the same if you are talking about efficiency.

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).

Hope my position is clear enough.

[...]

Maxim Dounin