Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

dochang
Copy link
Contributor

@dochang dochang commented Apr 28, 2016

Based on this discussion.

@bagder bagder added the HTTP label Apr 28, 2016
@bagder
Copy link
Member

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...

@dochang
Copy link
Contributor Author

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.

@dochang
Copy link
Contributor Author

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?

@dochang
Copy link
Contributor Author

dochang commented Apr 28, 2016

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?

@jay
Copy link
Member

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

@dochang
Copy link
Contributor Author

dochang commented Apr 28, 2016

The discussion link doesn't work

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

@dochang dochang force-pushed the suppress-proxy-headers branch 3 times, most recently from 10c473f to 140cae8 Compare April 28, 2016 19:05
@dochang
Copy link
Contributor Author

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.

@dochang dochang force-pushed the suppress-proxy-headers branch 2 times, most recently from dd38a68 to 71a08cf Compare April 29, 2016 07:22
@dochang
Copy link
Contributor Author

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.

@bagder
Copy link
Member

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
Copy link
Contributor Author

dochang commented Apr 30, 2016

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

jay added a commit to jay/curl that referenced this pull request May 3, 2016
- 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
@jay
Copy link
Member

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

@bagder
Copy link
Member

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.

@jay
Copy link
Member

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.

@bagder
Copy link
Member

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?

@jay
Copy link
Member

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.

@dochang
Copy link
Contributor Author

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"?

@bagder
Copy link
Member

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.

@dochang
Copy link
Contributor Author

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

@CarloCannas
Copy link

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;

@jay
Copy link
Member

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?

@bagder
Copy link
Member

bagder commented Feb 11, 2017

This seems to have gone stale.

@bagder bagder closed this Feb 11, 2017
@jay
Copy link
Member

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
@jay jay added the feature-window A merge of this requires an open feature window label Feb 14, 2017
@jay
Copy link
Member

jay commented Feb 27, 2017

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 closed this in d2bcf1e Mar 12, 2017
@jay jay removed the feature-window A merge of this requires an open feature window label Mar 12, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants