[PATCH] Add client_body_temp_access configuration directive

[Maxim Dounin]

Hello!

On Fri, Oct 12, 2018 at 09:54:04PM +0200, Paul Pawlowski via nginx-devel wrote:

> # HG changeset patch
> # User Paul Pawlowski <paul at mrarm.io>
> # Date 1539371172 -7200
> #      Fri Oct 12 21:06:12 2018 +0200
> # Node ID 67cc676dbfaf56332bb6c61e635c0073c1e49907
> # Parent  8b68d50090e4f134a35da60146fefd5e63770759
> Add client_body_temp_access configuration directive

Style: there should be a trailing dot and a reference to ticket 
you've created, and please use past tense.  Also, I think that 
"client_body_access" might be a better name:

Added client_body_access directive (ticket #1653).

> Adds client_body_temp_access configuration directive, which sets the unix permissions of the temporary files holding client request bodies.
> 
> This makes it possible for a process running as another user to access the files stored using client_body_temp_path and client_body_in_file_only.
> This is useful when using the mentioned directives in order to have nginx store the request body to file and forward the file path to another server running on the same machine as another user.

Style: please wrap lines at 80 characters or less.

> 
> diff -r 8b68d50090e4 -r 67cc676dbfaf contrib/vim/syntax/nginx.vim
> --- a/contrib/vim/syntax/nginx.vim	Wed Oct 03 14:08:51 2018 +0300
> +++ b/contrib/vim/syntax/nginx.vim	Fri Oct 12 21:06:12 2018 +0200
> @@ -155,6 +155,7 @@
>  syn keyword ngxDirective contained chunked_transfer_encoding
>  syn keyword ngxDirective contained client_body_buffer_size
>  syn keyword ngxDirective contained client_body_in_file_only
> +syn keyword ngxDirective contained client_body_temp_access
>  syn keyword ngxDirective contained client_body_in_single_buffer
>  syn keyword ngxDirective contained client_body_temp_path
>  syn keyword ngxDirective contained client_body_timeout

This list is expected to be sorted alphabetically.

> diff -r 8b68d50090e4 -r 67cc676dbfaf src/http/ngx_http_core_module.c
> --- a/src/http/ngx_http_core_module.c	Wed Oct 03 14:08:51 2018 +0300
> +++ b/src/http/ngx_http_core_module.c	Fri Oct 12 21:06:12 2018 +0200
> @@ -370,6 +370,13 @@
>        offsetof(ngx_http_core_loc_conf_t, client_body_temp_path),
>        NULL },
>  
> +    { ngx_string("client_body_temp_access"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE123,
> +      ngx_conf_set_access_slot,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      offsetof(ngx_http_core_loc_conf_t, client_body_temp_access),
> +      NULL },
> +
>      { ngx_string("client_body_in_file_only"),
>        NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
>        ngx_conf_set_enum_slot,
> @@ -3373,6 +3380,7 @@
>      clcf->if_modified_since = NGX_CONF_UNSET_UINT;
>      clcf->max_ranges = NGX_CONF_UNSET_UINT;
>      clcf->client_body_in_file_only = NGX_CONF_UNSET_UINT;
> +    clcf->client_body_temp_access = NGX_CONF_UNSET_UINT;
>      clcf->client_body_in_single_buffer = NGX_CONF_UNSET;
>      clcf->internal = NGX_CONF_UNSET;
>      clcf->sendfile = NGX_CONF_UNSET;
> @@ -3594,6 +3602,8 @@
>      ngx_conf_merge_uint_value(conf->client_body_in_file_only,
>                                prev->client_body_in_file_only,
>                                NGX_HTTP_REQUEST_BODY_FILE_OFF);
> +    ngx_conf_merge_uint_value(conf->client_body_temp_access,
> +                              prev->client_body_temp_access, 0);

Explicitly using 0600 as the default should be a better option.

>      ngx_conf_merge_value(conf->client_body_in_single_buffer,
>                                prev->client_body_in_single_buffer, 0);
>      ngx_conf_merge_value(conf->internal, prev->internal, 0);
> diff -r 8b68d50090e4 -r 67cc676dbfaf src/http/ngx_http_core_module.h
> --- a/src/http/ngx_http_core_module.h	Wed Oct 03 14:08:51 2018 +0300
> +++ b/src/http/ngx_http_core_module.h	Fri Oct 12 21:06:12 2018 +0200
> @@ -375,6 +375,7 @@
>      ngx_uint_t    if_modified_since;       /* if_modified_since */
>      ngx_uint_t    max_ranges;              /* max_ranges */
>      ngx_uint_t    client_body_in_file_only; /* client_body_in_file_only */
> +    ngx_uint_t    client_body_temp_access; /* client_body_temp_access */
>  
>      ngx_flag_t    client_body_in_single_buffer;
>                                             /* client_body_in_singe_buffer */
> diff -r 8b68d50090e4 -r 67cc676dbfaf src/http/ngx_http_request_body.c
> --- a/src/http/ngx_http_request_body.c	Wed Oct 03 14:08:51 2018 +0300
> +++ b/src/http/ngx_http_request_body.c	Fri Oct 12 21:06:12 2018 +0200
> @@ -450,6 +450,7 @@
>          tf->file.fd = NGX_INVALID_FILE;
>          tf->file.log = r->connection->log;
>          tf->path = clcf->client_body_temp_path;
> +        tf->access = clcf->client_body_temp_access;
>          tf->pool = r->pool;
>          tf->warn = "a client request body is buffered to a temporary file";
>          tf->log_level = r->request_body_file_log_level;

The following block:

        if (r->request_body_file_group_access) {
            tf->access = 0660;
        }

raises the question on how it is expected to interact with 
r->request_body_file_group_access.  I suspect that 
r->request_body_file_group_access is not currently needed and can 
be removed altogether, but this needs an additional investigation.

Another question to consider is how this is expected to interact 
with directories created for various temp file levels.  These 
directories, much like the client_body_temp_path diretory itself, 
are unconditionally created with 0700 access, and will prevent 
access to temporary files from non-nginx user as well.  These can 
be pre-created with appropriate permissions - but as long as it is 
a recommended approach, this probably needs to be mentioned in the 
commit log.

-- 
Maxim Dounin
http://mdounin.ru/