Project

General

Profile

Actions

Feature #2434

closed

[mod_proxy] Please make it possible to forward environment variables.

Added by KiBi about 10 years ago. Updated about 4 years ago.

Status:
Invalid
Priority:
Low
Category:
mod_proxy
Target version:
-
ASK QUESTIONS IN Forums:

Description

Hello,

this is a generalization/clean version of the patch I posted in:
http://redmine.lighttpd.net/boards/2/topics/3578?r=5277#message-5277

The initial purpose was to forward X509 properties through mod_proxy, but that can now be done with any (dynamic) environment variables, just by listing the wanted ones (separated by commas) in the proxy.forward-env-vars config variable.

Example:
proxy.forward-env-vars = "SSL_CLIENT_S_DN_x500UniqueIdentifier,SSL_CLIENT_S_DN_emailAddress"

Tests:
- Successful runtime tests against 1.4.28 with and without SSL client certificates.
- Successful make && make check against 1.4.31.

Thanks for considering.

Mraw,
KiBi.


Files

Actions #1

Updated by KiBi about 10 years ago

Here's a follow-up patch.

Let me quote the commit message, along with the consequences:

-----
[mod_proxy] Fix mistakes in the previous commit.

Several things went wrong:
- Wrong format string for log_error_write(). Copy/paste FTL!

--> Crashes in case the config variable is not a string.

- strchrnul() is better than cheating strchr().

--> Let's just avoid being nasty.

- Don't assume "prev" can't be a comma, this can happen:
+ with a leading comma
+ with a trailing comma
+ if there are two commas in a row

--> Crashes in case of empty substrings.
-----

I'm attaching the fix-up patch so that it's easy to spot the bug fixes, but it would be nice to apply both patches squashed as a single commit.

Tested in the same conditions as the first one, with or without empty substrings in the config variable.

If that's deemed desirable, we could error out if empty substrings are found (we can tolerate them, but that's probably an error the user would want to know about, right?). In that case, log_error_write/return if prev == ptr, but making sure all p->forward_env_vars[j] are set to NULL, or SIGSEV would happen in mod_proxy_free(). I can provide an incremental patch (“3”) for that.

Of course I can provide a squashed version of 1+2, or 1+2+3 if that's useful.

Mraw,
KiBi.

Actions #2

Updated by KiBi about 10 years ago

I realize I could have used an array directly, but then one would have to make sure each item is a string, so that would require some amount of work anyway, so splitting a single string might not be that bad after all.

I'm attaching a “third time lucky” patch, which is 1+2 above, plus a tiny fix for a c99 declaration (for int j=…); hopefully it'll be good enough for a review on your side.

Mraw,
KiBi.

Actions #3

Updated by stbuehler about 10 years ago

As the config can represent strings, i really think an array should be used instead of splitting (it is what a user familiar with the config style will expect).

Perhaps the "X-" prefix could be added only once (and the lookup be done with ptr+2); modifying a array from the config might be "unsafe" (+= might reuse entries).

Actions #4

Updated by tlange almost 10 years ago

I also needed functionality like that (forwarding authentication information from an SSL enabled lighttpd) and really would like a patch like this integrated in forthcoming releases.

But please note:
Especially when forwarding such SSL context information like the authenticated user (SSL_CLIENT_S_DN_CN), certificate fingerprints, E-Mail-Adress and so on (see initial usecase), it should be avoided that one can spoof this information by just posting the resulting
"X-%s", *forward_env_vars_ptr
header in the initial request.

By using
proxy_set_header(con, forward_env_header, key_ds->value->ptr); (see all patches below up to today)
old values from the original request are kept and the new information is appended comma-separated!

This might be
a) unexpected
b) insecure
if you rely on those information at the destination.

It would be good to implement another helper function like

static void proxy_replace_header(connection *con, const char *key, const char *value) {
data_string *ds_dst;
if (NULL == (ds_dst = (data_string *)array_get_unused_element(con->request.headers, TYPE_STRING))) {
ds_dst = data_string_init();
}
buffer_copy_string(ds_dst->key, key);
buffer_copy_string(ds_dst->value, value);
array_replace(con->request.headers, (data_unset *)ds_dst);
}

instead of the proxy_set_header() call at least for those headers that should not be "injectable". This function replaces any old header content when the new one is set.

For other environmental information to be forwarded one might see this "append to pre-existing X- Headers" more relaxed, maybe even wanted (?) behaviour.
For the SSL environemnt usecase it definitively isn't.

Actions #5

Updated by KiBi almost 10 years ago

Thanks for your comments, tlange.

Indeed, our initial testing was insufficient, and we didn't catch that spoofing possibility (joys of HTTP_, X-, X_ etc. prefixes).

I have an upcoming patch to also set SSL_CLIENT_I_* based on the the certificate issuer's data (in addition to SSL_CLIENT_S_* for the subject). Given the sensitive nature of those SSL_CLIENT_* variables, I'm wondering if I shouldn't simply propose a patch which implement an “I am a reverse SSL proxy” flag, which would kill any SSL_CLIENT_* in the headers (to be implemented), set those from the certificate if there's anything to extract from there (which is already done), and forward those (which was the purpose of that patch).

Here's what we could do:
1. Implement environment variable forwarding in mod_proxy, as originally intended; but using an array instead of a string holding a comma-separated list of variables to export.
2. Implement that “I'm a reverse SSL proxy” flag, which would kill SSL_CLIENT_* in the incoming headers (but where? I didn't check where that would make sense), and automatically export SSL_CLIENT_* in mod_proxy (in addition to any configured variable, using the first mechanism).

What do you think?

Mraw,
KiBi.

Actions #6

Updated by gstrauss over 6 years ago

  • Status changed from Patch Pending to Need Feedback
  • Priority changed from Normal to Low

Is the addition of internal environment variables into proxied request headers a feature available in other popular web server? What conventions are used? If not a feature in other popular web servers, then what solutions to this issue are used with those web servers?

Repeating some of the above feedback to the submitted patch, and adding some more:
  • For every header "headername" that you intend to forward, the case-insensitive "headername" and "X-headername" should be removed from existing request headers before potentially inserting "headername" from the internal con->environment, if "headername" exists in con->environment.
  • The contents of internal con->environment should not be fabricated by mod_proxy. In other words, a separate patch would be required to put SSL_* values into con->environment. For example, see https://redmine.lighttpd.net/issues/2693
  • It is preferable for proxy.forward-env-vars to be an array of strings instead of a comma-separate list.
  • strchrnul() is a GNU extension and is not portable.

Overall, I do not think this patch is a great idea.

I think the information provided in
https://tools.ietf.org/html/rfc7239 Forwarded HTTP Extension
would be more appropriate for HTTP proxy. (There are numerous open tickets where implementing RFC7239 in lighttpd might be a desirable solution.)

Separately, I could see adding various SSL_* values to the environment, and these would be passed to FastCGI, SCGI, CGI, SSI.

Actions #7

Updated by KiBi over 6 years ago

If by popular web servers you mean apache, yes it supports such setups, see e.g.

%{VARNAME}e     The contents of the environment variable VARNAME.
%{VARNAME}s     The contents of the SSL environment variable VARNAME, if mod_ssl is enabled.

in https://httpd.apache.org/docs/current/en/mod/mod_headers.html

Overall, due to various limitations and lack of feedback, we did not think lighttpd was a great idea.

I'm not exactly sure how we implemented it when we switched to apache though; that was years ago. And I'm no longer working for that company.

Feel free to close this bug report if nobody is interesting in picking up this topic.

KiBi.

Actions #8

Updated by gstrauss over 6 years ago

Thank you for your feedback.

I understand that lighttpd did not meet your needs.

I'll close this in another week or two if there is no additional interest.

Actions #9

Updated by gstrauss almost 6 years ago

  • Status changed from Need Feedback to Invalid

It is already possible to use some custom lua code and mod_magnet to modify the request headers sent to backend by mod_proxy. See Absoluation. Custom lua code could inspect and remove various combinations of the desired fields (FOO, X-FOO, X_FOO) from request headers sent by client, and then could insert request headers with values created by the server.

Note that not all SSL env variables available in Apache are added to con->environment in lighttpd, so a very limited set is available to mod_magnet. If you need the entire SSL certificate parsed and available, please file a separate enhancement request and I'll consider implementing it as something which can be generated on-demand in mod_magnet.

Actions #10

Updated by gstrauss almost 6 years ago

  • Target version deleted (1.4.x)
Actions #11

Updated by gstrauss about 4 years ago

Recent lighttpd versions provide various SSL variables to CGI env but not to mod_proxy. However, lighttpd 1.4.46 and later support receiving HAProxy PROXY protocol.

If a patch were written to lighttpd mod_proxy to support sending HAProxy PROXY protocol to proxy backends, the patch would be considered for inclusion in upstream lighttpd mod_proxy.

Actions

Also available in: Atom