curl: add --suppress-proxy-headers #783

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@dochang
Contributor
dochang commented Apr 28, 2016

Based on this discussion.

@bagder bagder added the HTTP label Apr 28, 2016
Owner
bagder commented Apr 28, 2016

Do note that it failed the test, probably because you didn't add the new symbol to symbols-in-versions.

Also, you didn't provide any man page for the new option.

Finally, I think you need to phrase the documentation more carefully. You're only addressing proxy response headers from CONNECT requests, right? If you do "normal" requests over the proxy, this option won't suppress the headers that then also arrive from the proxy...

Contributor
dochang commented Apr 28, 2016

Do note that it failed the test, probably because you didn't add the new symbol to symbols-in-versions.

Fixed.

Contributor
dochang commented Apr 28, 2016

Also, you didn't provide any man page for the new option.

I added the documentation in docs/curl.1. Is there any other file which I have to fix?

Contributor
dochang commented Apr 28, 2016 edited

Finally, I think you need to phrase the documentation more carefully. You're only addressing proxy response headers from CONNECT requests, right? If you do "normal" requests over the proxy, this option won't suppress the headers that then also arrive from the proxy...

Thanks. I will fix the doc. And what do you think about the option name? Do we need a new name, such as --suppress-connect-headers or --suppress-proxy-connect-headers?

Owner
jay commented Apr 28, 2016

The discussion link doesn't work, and yes if this goes in it will need a more apt name like --no-include-connect-headers or something

Contributor
dochang commented Apr 28, 2016

The discussion link doesn't work

Gmane is down. Please visit the thread on the mailing list.

Contributor
dochang commented Apr 28, 2016

Also, you didn't provide any man page for the new option.

Ok, I think I need to add new manpages in docs/libcurl. I will do this tomorrow.

Contributor
dochang commented Apr 29, 2016

@bagder I have fixed all documentation issues. Thanks for your help. Please check it.

If a new option name is more appropriate, I'll fix.

Owner
bagder commented Apr 29, 2016

What do you think about --suppress-connect-headers ? I realize connect and CONNECT aren't the same things but we only do lowercase options and I think (hope?) "connect headers" most likely won't be that confusing to most users anyway.

@dochang dochang curl: add --suppress-connect-headers
525df32
Contributor
dochang commented Apr 30, 2016

@bagder The option name has been changed to --suppress-connect-headers. Thanks for your help!

@jay jay added a commit to jay/curl that referenced this pull request May 3, 2016
@jay jay CURLOPT_HEADER: Support output of a specific set of headers
- Retrofit CURLOPT_HEADER to use new CURLHDRBODY_ defines and support
showing only proxy headers and only server headers.

  0L: CURLHDRBODY_OFF:
  No headers in body.

  1L: CURLHDRBODY_ALL:
  All headers in body.

  2L: CURLHDRBODY_SERVER_ONLY:
  Only server headers in body, no proxy tunnel headers.

  3L: CURLHDRBODY_TUNNEL_ONLY
  Only proxy tunnel headers in body, no server headers.

- New tool option --include-tunnel to expose the new functionality.

  --proxytunnel --head
  Output all headers and no content.

  --proxytunnel --include
  Output all headers and server content.

  --proxytunnel --head --no-include --include-tunnel
  Output tunnel headers and no content.

  --proxytunnel --include-tunnel
  Output tunnel headers and server content.

  --proxytunnel --head --no-include-tunnel
  Output server headers and no content.

  --proxytunnel --include --no-include-tunnel
  Output server headers and server content.

---

Ref: curl#783
Assisted-by: Desmond O. Chang
99fb558
Owner
jay commented May 3, 2016

I have a counterproposal for this. I rewrote it from scratch so we can piggyback on CURLOPT_HEADER in a backwards-compatible way:

  0L: CURLHDRBODY_OFF:
  No headers in body.

  1L: CURLHDRBODY_ALL:
  All headers in body.

  2L: CURLHDRBODY_SERVER_ONLY:
  Only server headers in body, no proxy tunnel headers.

  3L: CURLHDRBODY_TUNNEL_ONLY
  Only proxy tunnel headers in body, no server headers.

And new tool option --include-tunnel which can be negated as --no-include-tunnel. To suppress proxytunnel headers would be --head --no-include-tunnel or --include --no-include-tunnel. This way we can also suppress server headers but show proxytunnel headers.

https://github.com/curl/curl/compare/master...jay:pr_783_counterproposal?expand=1

Owner
bagder commented May 3, 2016

I really like taking better advantage of CURLOPT_HEADER for a feature like this, but I think you're missing the target slightly at least in this first take. I got the impression the user wants to control what headers are sent to the header callback, as it isn't really possible for a user to distinguish between the different set of headers there.

Owner
jay commented May 5, 2016

I'm a little lost then. I read his request and I thought he wants to stop proxytunnel headers from being sent to the body.

Owner
bagder commented May 13, 2016

@dochang can you comment on this? Are you looking at changing what headers are passed as body (write callback) or do you want to change what headers are passed to the header callback?

Owner
jay commented May 13, 2016

Another way we could go which may be better is work off of his commit but make sure that suppressing the headers means they're suppressed from reaching both user callbacks. The option wouldn't have anything to do with CURLOPT_HEADER and instead just be a separate option to prevent the headers from being received by the callbacks. Like this:

diff --git a/lib/http_proxy.c b/lib/http_proxy.c
index 546dd43..24dc4ba 100644
--- a/lib/http_proxy.c
+++ b/lib/http_proxy.c
@@ -368,10 +368,11 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
                                line_start, (size_t)perline, conn);

                   /* send the header to the callback */
-                  writetype = CLIENTWRITE_HEADER;
-                  if(data->set.include_header &&
-                     !data->set.suppress_connect_headers)
-                    writetype |= CLIENTWRITE_BODY;
+                  if(!data->set.suppress_connect_headers) {
+                    writetype = CLIENTWRITE_HEADER;
+                    if(data->set.include_header)
+                      writetype |= CLIENTWRITE_BODY;
+                  }

                   result = Curl_client_write(conn, writetype, line_start,
                                              perline);

Also I think some of his documentation (even if it's adjusted for the behavior above) is superfluous, we really don't need a lot of it. It should be shorter and to the point, it could be as simple as 'When this option is enabled proxytunnel headers will not be sent to the user's writefunction and header function.'

Another idea compatible with that is change the option to suppress_headers and then flags for proxytunnel and server and we default to none. That way the user could suppress server headers if they wanted only the proxy tunnel headers for some reason.

Contributor
dochang commented May 15, 2016

@bagder I just want to suppress the headers created by the proxy. Where do they go? "write callback" or "header callback"?

Owner
bagder commented May 16, 2016

Headers always go to the header callback. They optionally also go to the write callback when --include (CURLOPT_HEADER) is used.

Contributor
dochang commented May 16, 2016

So, they should not go to the write callback because I want to prevent the proxy headers from displaying them. But I'm not sure whether they should go to the header callback.

I prefer to pass them to the header callback, because the purpose of this PR is just suppressing the headers.

What do you think? @bagder @jay

Hi, I'd like to have an option like this included in curl.
I think that in Curl_proxyCONNECT both user callbacks should be excluded: in the commandline tool this might not make any difference, but an application using libcurl usually doesn't need to receive the response headers from a CONNECT request.

If I develop an application which performs HTTPS requests using curl and then for whatever reason an HTTP proxy is needed in some setups I'd expect that setting CURLOPT_PROXY should be enough, but any code that handles the response headers wouldn't probably work properly with the additional headers from the proxy. Having an option like CURLOPT_SUPPRESS_CONNECT_HEADERS that basically makes the proxy invisible to the application would be really useful.

So, I'd wrap everything in an if, like this:

diff --git a/lib/http_proxy.c b/lib/http_proxy.c
index 8f5e9b4..5194301 100644
--- a/lib/http_proxy.c
+++ b/lib/http_proxy.c
@@ -371,16 +371,18 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
                     Curl_debug(data, CURLINFO_HEADER_IN,
                                line_start, (size_t)perline, conn);

-                  /* send the header to the callback */
-                  writetype = CLIENTWRITE_HEADER;
-                  if(data->set.include_header)
-                    writetype |= CLIENTWRITE_BODY;
+                  /* send the header to the callback if requested */
+                  if(!data->set.suppress_connect_headers){
+                    writetype = CLIENTWRITE_HEADER;
+                    if(data->set.include_header)
+                      writetype |= CLIENTWRITE_BODY;

-                  result = Curl_client_write(conn, writetype, line_start,
-                                             perline);
+                    result = Curl_client_write(conn, writetype, line_start,
+                                               perline);

-                  data->info.header_size += (long)perline;
-                  data->req.headerbytecount += (long)perline;
+                    data->info.header_size += (long)perline;
+                    data->req.headerbytecount += (long)perline;
+                  }

                   if(result)
                     return result;
Owner
jay commented Nov 2, 2016

@CarloCannas Thanks. I don't think we can skip req.headerbytecount calculation because that keeps track of the bytes received and is used elsewhere to see if we have received any bytes. Further there's info.header_size which is the internal variable for CURLINFO_HEADER_SIZE and it is a question if we suppress the proxy headers whether that option should still include them in the total. I would think no.

So to tweak it a little

diff --git a/lib/http_proxy.c b/lib/http_proxy.c
index 8f5e9b4..37ccccb 100644
--- a/lib/http_proxy.c
+++ b/lib/http_proxy.c
@@ -371,19 +371,22 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
                     Curl_debug(data, CURLINFO_HEADER_IN,
                                line_start, (size_t)perline, conn);

-                  /* send the header to the callback */
-                  writetype = CLIENTWRITE_HEADER;
-                  if(data->set.include_header)
-                    writetype |= CLIENTWRITE_BODY;
+                  data->req.headerbytecount += (long)perline;

-                  result = Curl_client_write(conn, writetype, line_start,
-                                             perline);
+                  if(!data->set.suppress_connect_headers) {
+                    /* send the header to the callback */
+                    writetype = CLIENTWRITE_HEADER;
+                    if(data->set.include_header)
+                      writetype |= CLIENTWRITE_BODY;

-                  data->info.header_size += (long)perline;
-                  data->req.headerbytecount += (long)perline;
+                    result = Curl_client_write(conn, writetype, line_start,
+                                               perline);

-                  if(result)
-                    return result;
+                    data->info.header_size += (long)perline;
+
+                    if(result)
+                      return result;
+                  }

                   /* Newlines are CRLF, so the CR is ignored as the line isn't
                      really terminated until the LF comes. Treat a following CR

Anyone else interested in this?

Owner
bagder commented Feb 11, 2017

This seems to have gone stale.

@bagder bagder closed this Feb 11, 2017
Owner
jay commented Feb 14, 2017

This seems to have gone stale.

Resurrected! I think this is a good idea and I've rebased @dochang's changes on master with the appropriate changes and added a test, see master...jay:suppress-proxy-headers_take2. It is as we discussed above except not discounting the suppressed headers from the total header byte statistic as I had originally suggested, since on second thought that seems misleading.

The behavior is basically;

  • Must suppress in --include and --dump-header
  • Must not suppress in --verbose and --trace
  • Must not suppress in statistics (eg received header byte total)

One minor concern with the name --suppress-connect-headers a user could think it is suppressing headers from being sent, not received. But --suppress-connect-response-headers or something seems like a mouthful.

Except for that minor name concern this would have 👍 from me for 7.54.0. At that time it will have to be updated with the appropriate option next LONG id and test id since the ids in the commit now may not be available at the time it goes in.

@jay jay reopened this Feb 14, 2017
Owner
jay commented Feb 27, 2017 edited

I'm going to move on landing this in the next few days unless there are any objections. As far as the ambiguity of the name goes --suppress-connect-headers re sent/received I'll add something to the manpage explaining that to control sent connect headers use --proxy-header

@jay jay added a commit that closed this pull request Mar 12, 2017
@dochang @jay dochang + jay url: add option CURLOPT_SUPPRESS_CONNECT_HEADERS
- Add new option CURLOPT_SUPPRESS_CONNECT_HEADERS to allow suppressing
  proxy CONNECT response headers from the user callback functions
  CURLOPT_HEADERFUNCTION and CURLOPT_WRITEFUNCTION.

- Add new tool option --suppress-connect-headers to expose
  CURLOPT_SUPPRESS_CONNECT_HEADERS and allow suppressing proxy CONNECT
  response headers from --dump-header and --include.

Assisted-by: Jay Satiro
Assisted-by: CarloCannas@users.noreply.github.com
Closes #783
d2bcf1e
@jay jay closed this in d2bcf1e Mar 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment