Welcome! Log In Create A New Profile

Advanced

Re: Making http_auth_request_module a first-class citizen? [patch]

Maxim Dounin
February 17, 2012 06:20AM
Hello!

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

>
> Hello!
>
> 16 февраля 2012, 20:07 от Maxim Dounin <mdounin@mdounin.ru>:
> > Hello!
> >
> > On Thu, Feb 16, 2012 at 08:16:03AM +0400, Max wrote:
> >
> > >
> > > 15 февраля 2012, 18:50 от Maxim Dounin <mdounin@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

_______________________________________________
nginx mailing list
nginx@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx
Subject Author Posted

Making http_auth_request_module a first-class citizen?

Maxim Khitrov February 15, 2012 08:58AM

Re: Making http_auth_request_module a first-class citizen?

Maxim Dounin February 15, 2012 09:52AM

Re[2]: Making http_auth_request_module a first-class citizen? [patch]

Max February 15, 2012 11:18PM

Re: Making http_auth_request_module a first-class citizen? [patch]

Piotr Sikora February 16, 2012 07:40AM

Re: Making http_auth_request_module a first-class citizen? [patch]

Maxim Dounin February 16, 2012 11:08AM

Re[2]: Making http_auth_request_module a first-class citizen? [patch]

Max February 17, 2012 03:30AM

Re: Making http_auth_request_module a first-class citizen? [patch]

Maxim Dounin February 17, 2012 06:20AM

Re[2]: Making http_auth_request_module a first-class citizen? [patch]

Max February 18, 2012 08:14PM

Re: Making http_auth_request_module a first-class citizen? [patch]

Maxim Dounin February 18, 2012 10:10PM



Sorry, only registered users may post in this forum.

Click here to login

Online Users

Guests: 154
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready