Discussion:
[privoxy-devel] [PATCH] Allow tagging requests with server address and/or port
p***@fake-box.com
2016-04-24 17:48:15 UTC
Permalink
Hi,Please find attached (in-line and as attachment) a patch which allows totag a client's request with the port and/or the ip-address of the privoxyserver which handles the request.Example use case:privoxy listens on two addresses, e.g. localhost:8118 and localhost:9119.By tagging the request withThis is for example useful to distinguish request made through differenthosts/ports.Example:---- user.filter ----CLIENT-HEADER-TAGGER: tag-server-ports@^\w*\s+.*\s+HTTP/\d\.\d\s*@TAG-SERVERPORT: $***@DCLIENT-HEADER-TAGGER: tag-server-addrs@^\w*\s+.*\s+HTTP/\d\.\d\s*@TAG-SERVERADDR: $***@D---- user.filter -------- user.action ----# Tag every request with the server port{+client-header-tagger{tag-server-port}}/# Fun only for those connections through 9119{+filter{fun}}TAG:TAG-SERVERPORT: 9119---- user.action ----While we are at it. I propose adding CLIENT-HEADER-TAGGER for all'special' substitutions ($origin, $url, $host, $path), not only for$origin.The patches, sure, needs some polishing (and I'm willing to help, note I'm not subscribed). In particular the spots marked with FIXME.  I alsotried to find all spots where the new introduced variablesserver_port_str, ... have to be freed, but I might have missed some.The naming of the newly introduced variables is up to discussion. Feelfree to change them.The patch itself is tested only on a Linux box, but with bothconfigurations: with and without HAVE_RFC2553 defined.Finally, in case it matters:Patch released under GNU General Public License as published by the FreeSoftware Foundation; either version 2 of the License, or (at youroption) any later version.Kind regardsdiff --git a/filters.c b/filters.cindex 245a9ce..e8b395b 100644--- a/filters.c+++ b/filters.c@@ -926,6 +926,8 @@ pcrs_job *compile_dynamic_pcrs_job_list(const struct client_state *csp, const st       {"path",   csp->http->path,  1},       {"host",   csp->http->host,  1},       {"origin", csp->ip_addr_str, 1},+      {"serverport",  csp->server_port_str, 1},+      {"serveraddr",  csp->server_ip_addr_str, 1},       {NULL,     NULL,             1}    };@@ -1762,6 +1764,8 @@ static void set_privoxy_variables(const struct client_state *csp)       { "PRIVOXY_PATH",   csp->http->path  },       { "PRIVOXY_HOST",   csp->http->host  },       { "PRIVOXY_ORIGIN", csp->ip_addr_str },+      { "PRIVOXY_SERVERPORT",  csp->server_port_str, 1},+      { "PRIVOXY_SERVERADDR",  csp->server_ip_addr_str, 1},    };    for (i = 0; i < SZ(env); i++)diff --git a/jbsockets.c b/jbsockets.cindex 2338423..7e7be77 100644--- a/jbsockets.c+++ b/jbsockets.c@@ -1348,6 +1348,78 @@ int accept_connection(struct client_state * csp, jb_socket fds[])    csp->ip_addr_long = ntohl(client.sin_addr.s_addr); #endif /* def HAVE_RFC2553 */++   /* Get the name/IP of the server which accepted the client and+    * store these in client_state.+    */+#ifdef HAVE_RFC2553+   struct sockaddr_storage server_sin;+#else+   struct sockaddr_in server_sin;+#endif /* def HAVE_RFC2553 */+#if defined(_WIN32) || defined(__OS2__) || defined(AMIGA)+   /* Wierdness - fix a warning. */+   int s_length;+#else+   socklen_t s_length;+#endif+   s_length = sizeof(server_sin);++   /* Resolve socket (a file descriptor) to sockaddr_* */+   if (getsockname(fd, (struct sockaddr *)&server_sin, &s_length) == -1)+   {+      log_error(LOG_LEVEL_ERROR, "Can not determine server socket for "+         "accepted connection.");+      /* FIXME free csp->ip_addr_str? */+   }+   else+   else+   {+      /* get the server port as number and as string */+      struct sockaddr_in *server_sin_in = (struct sockaddr_in *)&server_sin;+#ifndef _WIN32+      if (sizeof(server_sin_in->sin_port) == sizeof(short))+#endif /* ndef _WIN32 */+      {+         /* FIXME: Unnecessary conversation of integers? */+         csp->server_port_num = (int) ntohs(server_sin_in->sin_port);+      }+#ifndef _WIN32+      else+      {+         /* FIXME: Unnecessary conversation of integers? */+         csp->server_port_num = (int) ntohl(server_sin_in->sin_port);+      }+#endif /* ndef _WIN32 */+      char servnam[6];+      retval = snprintf(servnam, sizeof(servnam), "%d", csp->server_port_num);+      if ((-1 == retval) || (sizeof(servnam) <= retval))+      {+         log_error(LOG_LEVEL_ERROR,+            "Port number (%d) ASCII decimal representation doesn't fit into 6 bytes",+            csp->server_port_num);+        /* FIXME free csp->ip_addr_str? */+      }+      /* FIXME unecessary copying? */+      csp->server_port_str = strdup(servnam);++      /* Get the server name or IP as string and as sockaddr_storage/long. */+#ifdef HAVE_RFC2553+      csp->server_tcp_addr = server_sin;+      csp->server_ip_addr_str = malloc_or_die(NI_MAXHOST);+      retval = getnameinfo((struct sockaddr *) &server_sin, s_length,+            csp->server_ip_addr_str, NI_MAXHOST, NULL, 0, NI_NUMERICHOST);+      if (!csp->server_ip_addr_str || retval)+      {+         log_error(LOG_LEVEL_ERROR, "Can not save csp->server_ip_addr_str: %s",+            (csp->server_ip_addr_str) ? gai_strerror(retval) : "Insuffcient memory");+         freez(csp->server_ip_addr_str);+      }+#else+      csp->server_ip_addr_str  = strdup(inet_ntoa(server_sin.sin_addr));+      csp->server_ip_addr_long = ntohl(server_sin.sin_addr.s_addr);+#endif /* def HAVE_RFC2553 */+   } /* -END- if (getsockname(...) == -1) */+    return 1; }diff --git a/jcc.c b/jcc.cindex d8e785a..b97cc0b 100644--- a/jcc.c+++ b/jcc.c@@ -4038,6 +4038,8 @@ static void listen_loop(void)             "Connection from %s on socket %d dropped due to ACL", csp->ip_addr_str, csp->cfd);          close_socket(csp->cfd);          freez(csp->ip_addr_str);+         freez(csp->server_port_str);+         freez(csp->server_ip_addr_str);          freez(csp_list);          continue;       }@@ -4053,6 +4055,8 @@ static void listen_loop(void)             strlen(TOO_MANY_CONNECTIONS_RESPONSE));          close_socket(csp->cfd);          freez(csp->ip_addr_str);+         freez(csp->server_port_str);+         freez(csp->server_ip_addr_str);          freez(csp_list);          continue;       }diff --git a/loaders.c b/loaders.cindex fd3db36..8968895 100644--- a/loaders.c+++ b/loaders.c@@ -182,6 +182,8 @@ unsigned int sweep(void)          last_active->next = client_list->next;          freez(csp->ip_addr_str);+         freez(csp->server_port_str);+         freez(csp->server_ip_addr_str);          freez(csp->client_iob->buf);          freez(csp->iob->buf);          freez(csp->error_message);diff --git a/project.h b/project.hindex 0d23ef3..6daf178 100644--- a/project.h+++ b/project.h@@ -924,6 +924,31 @@ struct client_state    unsigned long ip_addr_long; #endif /* def HAVE_RFC2553 */+   /** The port on which the privoxy server which accepted the+       client's connection.+       As a number. */+   int server_port_num;+   /** The port on which the privoxy server which accepted the+       client's connection.+       As a string. */+   char *server_port_str;+   /** The IP address oft he privoxy server which accepted the+       client's connection.+       As a string. */+   char *server_ip_addr_str;+#ifdef HAVE_RFC2553+   /** The TCP address oft he privoxy server which accepted the+       client's connection.+       As a sockaddr. */+   struct sockaddr_storage server_tcp_addr;+#else+   /** The IP address oft he privoxy server which accepted the+       client's connection.+       As a number. */+   unsigned long server_ip_addr_long;+#endif /* def HAVE_RFC2553 */++    /** The URL that was requested */    struct http_request http[1];
Fabian Keil
2016-04-25 16:16:56 UTC
Permalink
Post by p***@fake-box.com
Please find attached (in-line and as attachment) a patch which allows to
tag a client's request with the port and/or the ip-address of the privoxy
server which handles the request.
privoxy listens on two addresses, e.g. localhost:8118 and localhost:9119.
By tagging the request with
This is for example useful to distinguish request made through different
hosts/ports.
Thanks a lot for the patch.

I agree that something like this would be useful but think it
could be simplified by using a single listen-address variable
that allows to tag the request with address and port together.

Instead of creating a text representation of the listen-address
for each connection, it seems preferable to do it once after
binding to the socket and simply look it up later on.

Variables should be declared at the beginning of the block
in which they are used.
Post by p***@fake-box.com
While we are at it. I propose adding CLIENT-HEADER-TAGGER for all
'special' substitutions ($origin, $url, $host, $path), not only for
$origin.
For variables that can contain special characters this is currently
blocked by TODO list item #118:

118) There should be "escaped" dynamic variables that are guaranteed
not to break filters.

Fabian
p***@fake-box.com
2016-04-25 19:38:15 UTC
Permalink
Dear Fabian,Fabian Keil <***@fabiankeil.de> wrote:> ***@fake-box.com wrote:>       > > Please find attached (in-line and as attachment) a patch which allows to> > tag a client's request with the port and/or the ip-address of the privoxy> > server which handles the request.> >> > Example use case:> > > > privoxy listens on two addresses, e.g. localhost:8118 and localhost:9119.> > By tagging the request with> >> > This is for example useful to distinguish request made through different> > hosts/ports.> > Thanks a lot for the patch.> > I agree that something like this would be useful but think it> could be simplified by using a single listen-address variable> that allows to tag the request with address and port together.Ok.> Instead of creating a text representation of the listen-address> for each connection, it seems preferable to do it once after> binding to the socket and simply look it up later on.The server socket is 'generated' in bind_port(), file jbsockets.c. Following up the path we arrive via bind_port_helper() andbind_ports_helper(), both file jcc.c, in listen_loop(), also jcc.c. Thesocket is passed as a pointer argument from function to function.Adding the additional information of the listen-address and listen-portwould require it to be replaced by an array of structs like struct server_state {        char *server_ip_addr_str;        jb_socket fd;}However, even if we have the text representation in listen_loop(),How should we pass the information to compile_dynamic_pcrs_job_list(),and set_privoxy_variables(), both file filters.c, where we actually wantto use that value? Both functions take a client_state as argument anduse it to access the needed bits of information. So this seemed for me alogical route to take.Another possibility is to add an extra parameter tocompile_dynamic_pcrs_job_list() and set_privoxy_variables(), call itserver_state, which contains all the information needed.Then, following the flow from compile_dynamic_pcrs_job_list() upwards wearrive (for one possible path) at- header_tagger(), parsers.c- scan_headers(), parsers.c- sed(), parsers.c- parse_client_request(), jcc.c- chat(), jcc.c- serve(), jcc.c- listen_loop() via pthread(), jcc.cso all these function get a additional server_state argument, just topass it onwards.Last problem to tackle would be how to determine inside listen_loop()which socket accepted the client. Note that accept_connection() takesthe complete array of available sockets from listen_loop(). So adding anadditional parameter server_state* in addition to client_state* seemsnecessary to tell listen_loop() which server socket it was.Yet another approach would be a hybrid of the one in the patch and theone discussed above:Only replacing the jb_socket pointers/arrays in bind_port(),bind_port_helper(), bind_ports_helper() and accept_connection() withpointers/arrays to struct server_state as described above, adding a newfield char *server_addr in client_state (similar as done in the patch)and setting this field in accept_connection based on the server_statewhich accepted the client connection.So what do you think which one should I pursue?> Variables should be declared at the beginning of the block> in which they are used.I'll fix that in the next version of the patch.Thanks for the response.
Fabian Keil
2016-04-26 15:35:01 UTC
Permalink
Post by p***@fake-box.com
Post by Fabian Keil
Post by p***@fake-box.com
Please find attached (in-line and as attachment) a patch which allows to
tag a client's request with the port and/or the ip-address of the privoxy
server which handles the request.
privoxy listens on two addresses, e.g. localhost:8118 and localhost:9119.
By tagging the request with
This is for example useful to distinguish request made through different
hosts/ports.
Thanks a lot for the patch.
I agree that something like this would be useful but think it
could be simplified by using a single listen-address variable
that allows to tag the request with address and port together.
Ok.
Post by Fabian Keil
Instead of creating a text representation of the listen-address
for each connection, it seems preferable to do it once after
binding to the socket and simply look it up later on.
The server socket is 'generated' in bind_port(), file jbsockets.c.
Following up the path we arrive via bind_port_helper() and
bind_ports_helper(), both file jcc.c, in listen_loop(), also jcc.c. The
socket is passed as a pointer argument from function to function.
Adding the additional information of the listen-address and listen-port
would require it to be replaced by an array of structs like
struct server_state {
char *server_ip_addr_str;
jb_socket fd;
}
However, even if we have the text representation in listen_loop(),
How should we pass the information to compile_dynamic_pcrs_job_list(),
and set_privoxy_variables(), both file filters.c, where we actually want
to use that value? Both functions take a client_state as argument and
use it to access the needed bits of information. So this seemed for me a
logical route to take.
Another possibility is to add an extra parameter to
compile_dynamic_pcrs_job_list() and set_privoxy_variables(), call it
server_state, which contains all the information needed.
Then, following the flow from compile_dynamic_pcrs_job_list() upwards we
arrive (for one possible path) at
- header_tagger(), parsers.c
- scan_headers(), parsers.c
- sed(), parsers.c
- parse_client_request(), jcc.c
- chat(), jcc.c
- serve(), jcc.c
- listen_loop() via pthread(), jcc.c
so all these function get a additional server_state argument, just to
pass it onwards.
Last problem to tackle would be how to determine inside listen_loop()
which socket accepted the client. Note that accept_connection() takes
the complete array of available sockets from listen_loop(). So adding an
additional parameter server_state* in addition to client_state* seems
necessary to tell listen_loop() which server socket it was.
Yet another approach would be a hybrid of the one in the patch and the
Only replacing the jb_socket pointers/arrays in bind_port(),
bind_port_helper(), bind_ports_helper() and accept_connection() with
pointers/arrays to struct server_state as described above, adding a new
field char *server_addr in client_state (similar as done in the patch)
and setting this field in accept_connection based on the server_state
which accepted the client connection.
Unless I miss something, accept_connection() should be able to set
the a csp->listen_addr by using the socket index (the i in fds[i]) on
csp->config->haddr[] and csp->config->hport[].

BTW, your MUA seems to declare HTML as text which causes rendering
issues like this:
https://sourceforge.net/p/ijbswa/mailman/ijbswa-developers/?viewmonth=201604

It would be great if you could fix this, preferable by using plain
text mails without any HTML.

Fabian
p***@fake-box.com
2016-04-27 19:25:27 UTC
Permalink
HiFabian Keil <***@fabiankeil.de> wrote:> Unless I miss something, accept_connection() should be able to set the a> csp->listen_addr by using the socket index (the i in fds[i]) on> csp->config->haddr[] and csp->config->hport[].Oh, I totally missed that csp has a pointer to the config. Sadly, it is NULLwhen passing the client_state to accept_connection(). I solved this problem bypassing the config directly as argument from listen_loop().  Please seeattached patch. I also added documentation for the new '$server' feature.At your liking, you might change the name from '$server' to something different, e.g. '$listen_address'. I'm quite unimaginative when it comes to name something.I also took the to liberty alter an error message in jcc.c using the server name and port in addition to the socket file descriptor.Fabian Keil <***@fabiankeil.de> wrote:> BTW, your MUA seems to declare HTML as text which causes rendering issues like> this:> https://sourceforge.net/p/ijbswa/mailman/ijbswa-developers/?viewmonth=201604> It would be great if you could fix this, preferable by using plain text mails> without any HTML.I'm sorry. I changed some settings, hope this the broken mails it.Kind regards
Fabian Keil
2016-05-01 13:06:33 UTC
Permalink
Post by p***@fake-box.com
Post by p***@fake-box.com
Unless I miss something, accept_connection() should be able to set the a
csp->listen_addr by using the socket index (the i in fds[i]) on
csp->config->haddr[] and csp->config->hport[].
Oh, I totally missed that csp has a pointer to the config. Sadly, it is NULL
when passing the client_state to accept_connection(). I solved this problem by
passing the config directly as argument from listen_loop(). Please see
attached patch. I also added documentation for the new '$server' feature.
Thanks for the updated patch. I intent to polish and commit it in the
next days.

Fabian
Fabian Keil
2016-05-24 15:34:53 UTC
Permalink
Post by Fabian Keil
Post by p***@fake-box.com
Post by p***@fake-box.com
Unless I miss something, accept_connection() should be able to set the a
csp->listen_addr by using the socket index (the i in fds[i]) on
csp->config->haddr[] and csp->config->hport[].
Oh, I totally missed that csp has a pointer to the config. Sadly, it is NULL
when passing the client_state to accept_connection(). I solved this problem by
passing the config directly as argument from listen_loop(). Please see
attached patch. I also added documentation for the new '$server' feature.
Thanks for the updated patch. I intent to polish and commit it in the
next days.
I attached the patch set I intend to commit before the weekend so
it can make it into the 3.0.25 release.

Please let me know if you see anything wrong with it or want to be
credited differently.

Fabian
Fabian Keil
2016-05-25 11:00:01 UTC
Permalink
Post by Fabian Keil
Post by Fabian Keil
Post by p***@fake-box.com
Post by p***@fake-box.com
Unless I miss something, accept_connection() should be able to set the a
csp->listen_addr by using the socket index (the i in fds[i]) on
csp->config->haddr[] and csp->config->hport[].
Oh, I totally missed that csp has a pointer to the config. Sadly, it is NULL
when passing the client_state to accept_connection(). I solved this problem by
passing the config directly as argument from listen_loop(). Please see
attached patch. I also added documentation for the new '$server' feature.
Thanks for the updated patch. I intent to polish and commit it in the
next days.
I attached the patch set I intend to commit before the weekend so
it can make it into the 3.0.25 release.
Please let me know if you see anything wrong with it or want to be
credited differently.
fake-box.com rejected my mail so I committed a slightly modified
version of the patch set without waiting for feedback.

Fabian

p***@fake-box.com
2016-05-03 15:39:39 UTC
Permalink
Hi FabianFabian Keil wrote:> Thanks for the updated patch. I intent to polish and commit it in the next days.Thanks to you for your support and your hints.Attached you find yet another patch, which adds an usage example to theuser-manual (in the section about client-header-tagger).Kind regards
Loading...