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

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://nginx.org/mailman/listinfo/nginx-devel