<div dir="auto"><div>Hi Maxim,</div><div dir="auto"><br></div><div dir="auto">Thanks for your reply. In fact I want to discuss is that I think nginx should help user to check the configuration and ensure the configuration will follow user's expectations. <br></div><div dir="auto"><br></div><div dir="auto">When user set config like <span style="font-family:sans-serif">auth_request_set "$email $upstream_http_x_auth_request_</span><span style="font-family:sans-serif">email;" will effect on all server, I think user can't image this. Avoid doing mistakes like this by users themselves I think maybe is difficult? </span></div><div dir="auto"><font face="sans-serif"><br></font></div><div dir="auto"><font face="sans-serif">So I think if can interrupt these kind of variable, or we should raise a warn. Or we should miracle these variables only work for specific context? Could you share more advice for this case? </font> Thanks. </div><div dir="auto"><br></div><div dir="auto">Best Regards,</div><div dir="auto">Jinhua</div><div dir="auto"><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr"> <<a href="mailto:nginx-devel-request@nginx.org">nginx-devel-request@nginx.org</a>> 于 2021年1月21日周四 20:01写道：</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Date: Wed, 20 Jan 2021 19:50:02 +0300<br>
From: Maxim Dounin <<a href="mailto:mdounin@mdounin.ru" target="_blank" rel="noreferrer">mdounin@mdounin.ru</a>><br>
To: <a href="mailto:nginx-devel@nginx.org" target="_blank" rel="noreferrer">nginx-devel@nginx.org</a><br>
Subject: Re: Http: protect prefix variable when add variable<br>
Message-ID: <<a href="mailto:20210120165002.GW1147@mdounin.ru" target="_blank" rel="noreferrer">20210120165002.GW1147@mdounin.ru</a>><br>
Content-Type: text/plain; charset=us-ascii<br>
<br>
Hello!<br>
<br>
On Wed, Jan 20, 2021 at 07:59:50PM +0800, Jim T wrote:<br>
<br>
> Hello!<br>
> <br>
> There is a incident occur in our team, when we use auth_request_set like<br>
> this in many server, and print $upstream_http_x_auth_request_email in log:<br>
> <br>
> server {<br>
> listen 8080 reuseport;<br>
> server_name <a href="http://test.io" rel="noreferrer noreferrer" target="_blank">test.io</a>;<br>
> location / {<br>
> auth_request /oauth2/auth;<br>
> auth_request_set $email $upstream_http_x_auth_request_email;<br>
> }<br>
> }<br>
> <br>
> But when we add a bad auth_request_set like below:<br>
> server {<br>
> listen 8080 reuseport;<br>
> server_name <a href="http://test2.io" rel="noreferrer noreferrer" target="_blank">test2.io</a>;<br>
> location / {<br>
> auth_request /oauth2/auth;<br>
> auth_request_set $upstream_http_x_auth_request_email $email;<br>
> }<br>
> }<br>
> <br>
> We will lost all $upstream_http_x_auth_request_email even the server<br>
> haven't use, because there is a new variable<br>
> $upstream_http_x_auth_request_email, and the prefix variable can't be read<br>
> any more.<br>
> <br>
> So I think we can fix it like this, to avoid the wrong configuration:<br>
<br>
Thank you for your suggestion and patch.<br>
See comments below.<br>
<br>
> # HG changeset patch<br>
> # User Jinhua Tan <<a href="mailto:312841925@qq.com" target="_blank" rel="noreferrer">312841925@qq.com</a>><br>
> # Date 1611143620 -28800<br>
> # Wed Jan 20 19:53:40 2021 +0800<br>
> # Node ID fd7e9432a59abcfcf380ddedb1e892098a54a845<br>
> # Parent 61d0df8fcc7c630da35e832ba8e983db0061a3be<br>
> Http: protect prefix variable when add variable<br>
> <br>
> diff -r 61d0df8fcc7c -r fd7e9432a59a src/http/ngx_http_variables.c<br>
> --- a/src/http/ngx_http_variables.c Tue Jan 19 20:35:17 2021 +0300<br>
> +++ b/src/http/ngx_http_variables.c Wed Jan 20 19:53:40 2021 +0800<br>
> @@ -393,6 +393,20 @@<br>
> };<br>
> <br>
> <br>
> +static ngx_str_t ngx_http_protect_variables_prefix[] = {<br>
> + ngx_string("arg_"),<br>
> + ngx_string("http_"),<br>
> + ngx_string("sent_http_"),<br>
> + ngx_string("sent_trailer_"),<br>
> + ngx_string("cookie_"),<br>
> + ngx_string("arg_"),<br>
> + ngx_string("upstream_http_"),<br>
> + ngx_string("upstream_trailer_"),<br>
> + ngx_string("upstream_cookie_"),<br>
> + ngx_null_string<br>
> +};<br>
<br>
Using a static list of prefixes is certainly wrong: there can be <br>
arbitrary prefix variables added by various modules, and limiting <br>
checks to a predefied list is not going to work correctly.<br>
<br>
> +<br>
> +<br>
> ngx_http_variable_value_t ngx_http_variable_null_value =<br>
> ngx_http_variable("");<br>
> ngx_http_variable_value_t ngx_http_variable_true_value =<br>
> @@ -410,6 +424,7 @@<br>
> ngx_hash_key_t *key;<br>
> ngx_http_variable_t *v;<br>
> ngx_http_core_main_conf_t *cmcf;<br>
> + ngx_str_t *p;<br>
> <br>
> if (name->len == 0) {<br>
> ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,<br>
> @@ -421,6 +436,18 @@<br>
> return ngx_http_add_prefix_variable(cf, name, flags);<br>
> }<br>
> <br>
> + if (flags & NGX_HTTP_VAR_CHANGEABLE) {<br>
> + for (p = ngx_http_protect_variables_prefix; p->len; p++) {<br>
> + if (name->len >= p.len<br>
> + && ngx_strncasecmp(name->data, p->data, p->len) == 0)<br>
> + {<br>
> + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,<br>
> + "similar to prefix variable \"%V\"",<br>
> *p);<br>
> + return NULL;<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> cmcf = ngx_http_conf_get_module_main_conf(cf, ngx_http_core_module);<br>
> <br>
> key = cmcf->variables_keys->keys.elts;<br>
<br>
Prefix variables are intentionally implemented in a way which <br>
allows one to overwrite them: for example, this is used in nginx <br>
itself to provide custom handler for some $http_* variables, such <br>
as $http_host (which can be effectively retrieved, since nginx has <br>
a pointer to the Host header explicitly stored). That is, it is <br>
quite normal that the ngx_http_add_variable() function you modify <br>
is called with a variable which is more specific than a registered <br>
prefix variable. And using the NGX_HTTP_VAR_CHANGEABLE flag to <br>
distinguish when to fail looks wrong, as this is an unrelated <br>
flag. You probably mean to detect user-added variables, such as <br>
introduced by "set" or "auth_request_set". There is no way to <br>
detect such variables except in a particular directive used to <br>
define the variable.<br>
<br>
Further, I tend to think there are valid use cases when one may <br>
want to actually redefine a particular variable.<br>
<br>
Overall, the patch certainly needs more work, and I very much <br>
doubt we want to introduce such checks at all and hence this work <br>
needs to be done. A better solution might be to avoid doing <br>
mistakes like the one you've described above.<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer noreferrer" target="_blank">http://mdounin.ru/</a><br>
<br>
<br>
------------------------------<br>
</blockquote></div></div></div>