Mail Auth Module - Auth-Server local unix socket support

[Maxim Dounin]

Hello!

On Tue, Apr 06, 2010 at 03:23:07PM +0200, Simon Lécaille wrote:

> Hi all,
> 
> Because I need it, I add the unix socket support to Mail Auth Module.
> Now if nginx mail auth module receives Auth-Server containing a sock
> path e.g :
> 
> HTTP/1.0 200 OK
> Auth-Status: OK
> Auth-Server: /tmp/cyrus.sock
> Auth-Port: [SomethingOrNot]
> Auth-User: user at domain.tld
> Auth-Pass: password
> 
> Nginx will be able to connect to the socket (e.g /tmp/cyrus.sock)
> 
> I'm writting the tests set for prove.
> 
> Patch in this mail (nginx-0.8.35)
> 
> For people who wonder why :
> Unix sockets allow me to restrict rights and permissions on cyrus.
> By chrooting a lot of services, bad local users could contact cyrus
> from localhost with tcp connections.
> With unix sockets, the problem is now solved.
> 
> Best regards,
> Simon LECAILLE.
> 
> -- 
> (Logo EmisFr)
> *Simon LECAILLE*
> EmisFR
> /Infogérance totale ou partagée, sur site ou distante,
> Développements sur mesure web 2.0/
> 10 rue Mazagran, 54000 NANCY, France
> http://www.emisfr.com
> Tel/Fax.: +33.3 83 32 25 75

> --- ./src/mail/ngx_mail_auth_http_module.c	2009-12-25 16:43:40.000000000 +0100
> +++ ./src/mail/ngx_mail_auth_http_module.c	2010-04-06 14:55:05.000000000 +0200
> @@ -458,7 +458,6 @@
>      size_t               len, size;
>      ngx_int_t            rc, port, n;
>      ngx_addr_t          *peer;
> -    struct sockaddr_in  *sin;
>  
>      ngx_log_debug0(NGX_LOG_DEBUG_MAIL, s->connection->log, 0,
>                     "mail auth http process headers");
> @@ -744,7 +743,7 @@
>                  return;
>              }
>  
> -            if (ctx->addr.len == 0 || ctx->port.len == 0) {
> +			if ((ctx->addr.len == 0 && ctx->port.len == 0) || (ctx->port.len == 0 && ngx_strncmp(ctx->addr.data,"/",1)!=0)) {

Whitespace damage (don't use tabs in nginx code, please).  Calling 
ngx_strncmp isn't needed here as it's enough to check just one 
character.  Pointer in ctx->addr.data used without checking if 
it's supposed to be valid (i.e. you have to check if addr.len 
isn't 0 first).

>                  ngx_log_error(NGX_LOG_ERR, s->connection->log, 0,
>                                "auth http server %V did not send server or port",
>                                ctx->peer.name);
> @@ -770,9 +769,38 @@
>                  ngx_mail_session_internal_server_error(s);
>                  return;
>              }
> +			/* AF_UNIX or AF_INET*/
> +			if(ngx_strncmp(ctx->addr.data,"/",1)==0){

Whitespace damage, no spaces where they are supposed to be, and 
same as the above about ngx_strncmp.

> +
> +				/* AF_UNIX */
> +				port = 0;
> +				struct sockaddr_un  *sun;
> +				sun = ngx_pcalloc(s->connection->pool, sizeof(struct sockaddr_un));
> +				if (sun == NULL) {
> +					ngx_destroy_pool(ctx->pool);
> +					ngx_mail_session_internal_server_error(s);
> +					return;
> +				}
>  
> -            /* AF_INET only */
> +				sun->sun_family = AF_UNIX;

Not all systems have unix sockets, it should be in #if's with
NGX_HAVE_UNIX_DOMAIN check as appropriate.  Not even mentioning 
whitespace damage.

Also, please don't define variables in the middle of function 
unless they are only used in #if'ed block.

> +				ngx_memcpy(sun->sun_path, ctx->addr.data, ctx->addr.len);
> +				peer->sockaddr = (struct sockaddr *) sun;
> +				peer->socklen = sizeof(struct sockaddr_un);
> +				len = ctx->addr.len;
> +				peer->name.len = len;
> +				peer->name.data = ngx_pnalloc(s->connection->pool, len);
> +				if (peer->name.data == NULL) {
> +					ngx_destroy_pool(ctx->pool);
> +					ngx_mail_session_internal_server_error(s);
> +					return;
> +				}
>  
> +				len = ctx->addr.len;
> +				ngx_memcpy(peer->name.data, ctx->addr.data, len);
> +			}
> +			else{
> +				/* AF_INET */
> +				struct sockaddr_in  *sin;
>              sin = ngx_pcalloc(s->connection->pool, sizeof(struct sockaddr_in));
>              if (sin == NULL) {
>                  ngx_destroy_pool(ctx->pool);
> @@ -823,10 +850,9 @@
>              len = ctx->addr.len;
>  
>              ngx_memcpy(peer->name.data, ctx->addr.data, len);
> -
>              peer->name.data[len++] = ':';
> -
>              ngx_memcpy(peer->name.data + len, ctx->port.data, ctx->port.len);
> +			}

You may want to re-indent else{} part and avoid unrelated 
whitespace changes.

But actually I think it's good idea to plug something like 
ngx_parse_url() here instead of re-inventing the wheel.

Maxim Dounin