[PATCH] http: Support sending custom HTTP headers

classic Classic list List threaded Threaded
119 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|

[PATCH] http: Support sending custom HTTP headers

Johannes Schindelin
To make communication for `git fetch`, `git ls-remote` and friends extra
secure, we introduce a way to send custom HTTP headers with all
requests.

This allows us, for example, to send an extra token that the server
tests for. The server could use this token e.g. to ensure that only
certain operations or refs are allowed, or allow the token to be used
only once.

This feature can be used like this:

        git -c http.extraheader='Secret: sssh!' fetch $URL $REF

Signed-off-by: Johannes Schindelin <[hidden email]>
Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v1
---
 http-push.c   | 10 +++++-----
 http.c        | 28 +++++++++++++++++++++++++---
 http.h        |  1 +
 remote-curl.c |  4 ++--
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/http-push.c b/http-push.c
index bd60668..04eef17 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
  struct strbuf buf = STRBUF_INIT;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_get_default_headers();
 
  if (options & DAV_HEADER_IF) {
  strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
  struct active_request_slot *slot;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_get_default_headers();
 
  slot = get_active_slot();
  slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
  char *ep;
  char timeout_header[25];
  struct remote_lock *lock = NULL;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_get_default_headers();
  struct xml_ctx ctx;
  char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_get_default_headers();
  struct xml_ctx ctx;
  struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_get_default_headers();
  struct xml_ctx ctx;
  int lock_flags = 0;
  char *escaped;
diff --git a/http.c b/http.c
index 4304b80..02d7147 100644
--- a/http.c
+++ b/http.c
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
+static struct curl_slist *extra_http_headers;
 
 static struct active_request_slot *active_queue_head;
 
@@ -323,6 +324,12 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
  }
 
+ if (!strcmp("http.extraheader", var)) {
+ extra_http_headers =
+ curl_slist_append(extra_http_headers, value);
+ return 0;
+ }
+
  /* Fall back on the default ones */
  return git_default_config(var, value, cb);
 }
@@ -678,8 +685,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
  if (remote)
  var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
 
- pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
- no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
+ pragma_header = curl_slist_append(http_get_default_headers(),
+ "Pragma: no-cache");
+ no_pragma_header = curl_slist_append(http_get_default_headers(),
+ "Pragma:");
 
 #ifdef USE_CURL_MULTI
  {
@@ -765,6 +774,9 @@ void http_cleanup(void)
 #endif
  curl_global_cleanup();
 
+ curl_slist_free_all(extra_http_headers);
+ extra_http_headers = NULL;
+
  curl_slist_free_all(pragma_header);
  pragma_header = NULL;
 
@@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
  return handle_curl_result(results);
 }
 
+struct curl_slist *http_get_default_headers()
+{
+ struct curl_slist *headers = NULL, *h;
+
+ for (h = extra_http_headers; h; h = h->next)
+ headers = curl_slist_append(headers, h->data);
+
+ return headers;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
  char *ptr;
@@ -1380,7 +1402,7 @@ static int http_request(const char *url,
 {
  struct active_request_slot *slot;
  struct slot_results results;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_get_default_headers();
  struct strbuf buf = STRBUF_INIT;
  const char *accept_language;
  int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..b0927de 100644
--- a/http.h
+++ b/http.h
@@ -106,6 +106,7 @@ extern void step_active_slots(void);
 extern void http_init(struct remote *remote, const char *url,
       int proactive_auth);
 extern void http_cleanup(void);
+extern struct curl_slist *http_get_default_headers();
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..86ba787 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
  struct active_request_slot *slot;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_get_default_headers();
  struct strbuf buf = STRBUF_INIT;
  int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
  struct active_request_slot *slot;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_get_default_headers();
  int use_gzip = rpc->gzip_request;
  char *gzip_body = NULL;
  size_t gzip_size = 0;
--
2.8.1.306.gff998f2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Shawn Pearce
On Mon, Apr 25, 2016 at 6:13 AM, Johannes Schindelin
<[hidden email]> wrote:
> To make communication for `git fetch`, `git ls-remote` and friends extra
> secure, we introduce a way to send custom HTTP headers with all
> requests.

Hmm. Its not Apr 1 2016. So I guess you are serious. :)

> This allows us, for example, to send an extra token that the server
> tests for. The server could use this token e.g. to ensure that only
> certain operations or refs are allowed, or allow the token to be used
> only once.
>
> This feature can be used like this:
>
>         git -c http.extraheader='Secret: sssh!' fetch $URL $REF

Its not very secure to be adding secure data to the command line, e.g.
on Linux you can see that data in /proc.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Jeff King
In reply to this post by Johannes Schindelin
On Mon, Apr 25, 2016 at 03:13:08PM +0200, Johannes Schindelin wrote:

> diff --git a/http.c b/http.c
> index 4304b80..02d7147 100644
> --- a/http.c
> +++ b/http.c
> @@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
>  
>  static struct curl_slist *pragma_header;
>  static struct curl_slist *no_pragma_header;
> +static struct curl_slist *extra_http_headers;
>  
>  static struct active_request_slot *active_queue_head;
>  
> @@ -323,6 +324,12 @@ static int http_options(const char *var, const char *value, void *cb)
>  #endif
>   }
>  
> + if (!strcmp("http.extraheader", var)) {
> + extra_http_headers =
> + curl_slist_append(extra_http_headers, value);
> + return 0;
> + }

I wondered if this would trigger for "http.*.extraheader", too. And it
should, as that is all handled in the caller of http_options. Good.

> @@ -678,8 +685,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>   if (remote)
>   var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
>  
> - pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
> - no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
> + pragma_header = curl_slist_append(http_get_default_headers(),
> + "Pragma: no-cache");
> + no_pragma_header = curl_slist_append(http_get_default_headers(),
> + "Pragma:");

This looked wrong to me at first, because we are appending to the
default header list in each case. But the secret sauce is that calling
http_get_default_headers() actually creates a _new_ list that is a copy
of the default headers (and the caller can do what they will with it,
and must free it).

I think that's really the only sane way to do it because of curl's
interfaces. But maybe it is worth a comment either here, or along with
http_get_default_headers(), or both.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> writes:

> To make communication for `git fetch`, `git ls-remote` and friends extra
> secure, we introduce a way to send custom HTTP headers with all
> requests.

I think an ability to send custom headers may be a good addition and
have no problem with it, but I tend to agree with Shawn that its log
message that advertises it as if it has anything to do with security
is probably a bad idea in both ways (i.e. it isn't very secure, and
the usefulness of the feature is not limited to security).

> This allows us, for example, to send an extra token that the server
> tests for. The server could use this token e.g. to ensure that only
> certain operations or refs are allowed, or allow the token to be used
> only once.
>
> This feature can be used like this:
>
> git -c http.extraheader='Secret: sssh!' fetch $URL $REF
>
> Signed-off-by: Johannes Schindelin <[hidden email]>


> Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v1

Move this after "---".

> ---

This obviously needs documentation updates and tests, no?

>  http-push.c   | 10 +++++-----
>  http.c        | 28 +++++++++++++++++++++++++---
>  http.h        |  1 +
>  remote-curl.c |  4 ++--
>  4 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index bd60668..04eef17 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
>  static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
>  {
>   struct strbuf buf = STRBUF_INIT;
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_get_default_headers();
>  
>   if (options & DAV_HEADER_IF) {
>   strbuf_addf(&buf, "If: (<%s>)", lock->token);
> @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
>  static void start_move(struct transfer_request *request)
>  {
>   struct active_request_slot *slot;
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_get_default_headers();
>  
>   slot = get_active_slot();
>   slot->callback_func = process_response;
> @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
>   char *ep;
>   char timeout_header[25];
>   struct remote_lock *lock = NULL;
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_get_default_headers();
>   struct xml_ctx ctx;
>   char *escaped;
>  
> @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
>   struct slot_results results;
>   struct strbuf in_buffer = STRBUF_INIT;
>   struct buffer out_buffer = { STRBUF_INIT, 0 };
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_get_default_headers();
>   struct xml_ctx ctx;
>   struct remote_ls_ctx ls;
>  
> @@ -1204,7 +1204,7 @@ static int locking_available(void)
>   struct slot_results results;
>   struct strbuf in_buffer = STRBUF_INIT;
>   struct buffer out_buffer = { STRBUF_INIT, 0 };
> - struct curl_slist *dav_headers = NULL;
> + struct curl_slist *dav_headers = http_get_default_headers();
>   struct xml_ctx ctx;
>   int lock_flags = 0;
>   char *escaped;
> diff --git a/http.c b/http.c
> index 4304b80..02d7147 100644
> --- a/http.c
> +++ b/http.c
> @@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
>  
>  static struct curl_slist *pragma_header;
>  static struct curl_slist *no_pragma_header;
> +static struct curl_slist *extra_http_headers;
>  
>  static struct active_request_slot *active_queue_head;
>  
> @@ -323,6 +324,12 @@ static int http_options(const char *var, const char *value, void *cb)
>  #endif
>   }
>  
> + if (!strcmp("http.extraheader", var)) {
> + extra_http_headers =
> + curl_slist_append(extra_http_headers, value);
> + return 0;
> + }
> +
>   /* Fall back on the default ones */
>   return git_default_config(var, value, cb);
>  }
> @@ -678,8 +685,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>   if (remote)
>   var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
>  
> - pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
> - no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
> + pragma_header = curl_slist_append(http_get_default_headers(),
> + "Pragma: no-cache");
> + no_pragma_header = curl_slist_append(http_get_default_headers(),
> + "Pragma:");
>  
>  #ifdef USE_CURL_MULTI
>   {
> @@ -765,6 +774,9 @@ void http_cleanup(void)
>  #endif
>   curl_global_cleanup();
>  
> + curl_slist_free_all(extra_http_headers);
> + extra_http_headers = NULL;
> +
>   curl_slist_free_all(pragma_header);
>   pragma_header = NULL;
>  
> @@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
>   return handle_curl_result(results);
>  }
>  
> +struct curl_slist *http_get_default_headers()
> +{
> + struct curl_slist *headers = NULL, *h;
> +
> + for (h = extra_http_headers; h; h = h->next)
> + headers = curl_slist_append(headers, h->data);
> +
> + return headers;
> +}
> +
>  static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
>  {
>   char *ptr;
> @@ -1380,7 +1402,7 @@ static int http_request(const char *url,
>  {
>   struct active_request_slot *slot;
>   struct slot_results results;
> - struct curl_slist *headers = NULL;
> + struct curl_slist *headers = http_get_default_headers();
>   struct strbuf buf = STRBUF_INIT;
>   const char *accept_language;
>   int ret;
> diff --git a/http.h b/http.h
> index 4ef4bbd..b0927de 100644
> --- a/http.h
> +++ b/http.h
> @@ -106,6 +106,7 @@ extern void step_active_slots(void);
>  extern void http_init(struct remote *remote, const char *url,
>        int proactive_auth);
>  extern void http_cleanup(void);
> +extern struct curl_slist *http_get_default_headers();
>  
>  extern long int git_curl_ipresolve;
>  extern int active_requests;
> diff --git a/remote-curl.c b/remote-curl.c
> index 15e48e2..86ba787 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
>  static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  {
>   struct active_request_slot *slot;
> - struct curl_slist *headers = NULL;
> + struct curl_slist *headers = http_get_default_headers();
>   struct strbuf buf = STRBUF_INIT;
>   int err;
>  
> @@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  static int post_rpc(struct rpc_state *rpc)
>  {
>   struct active_request_slot *slot;
> - struct curl_slist *headers = NULL;
> + struct curl_slist *headers = http_get_default_headers();
>   int use_gzip = rpc->gzip_request;
>   char *gzip_body = NULL;
>   size_t gzip_size = 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Johannes Schindelin
Hi Junio,

On Mon, 25 Apr 2016, Junio C Hamano wrote:

> Johannes Schindelin <[hidden email]> writes:
>
> > To make communication for `git fetch`, `git ls-remote` and friends
> > extra secure, we introduce a way to send custom HTTP headers with all
> > requests.
>
> I think an ability to send custom headers may be a good addition and
> have no problem with it, but I tend to agree with Shawn that its log
> message that advertises it as if it has anything to do with security is
> probably a bad idea in both ways (i.e. it isn't very secure, and the
> usefulness of the feature is not limited to security).

You know, it never occurred to me that anybody could even *think* that I
was talking about the security of the client setup.

You see, it is much easier to read $HOME/.netrc than /proc/, especially if
you are looking outside of Linux, where the proc filesystem does not even
exist.  And it is almost as easy to query the credential helper for a
plain text password as looking at $HOME/.netrc.

So I took it for granted that everybody knows that they have to keep their
own computer safe.

Instead, I was thinking of server side security (with the clear
expectation that the users will keep their client setups secure).

I will rephrase the commit message to describe the actual use case I have
here: build agents need temporary access to private repositories, and I'd
like to do that via sort of One-Time-Passwords, sent as additional HTTP
headers (via HTTPS, I should not need to point out, but now feel I have to
spell out).

> > Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v1
>
> Move this after "---".

Whoops. That is what I intended, but overlooked. Will fix.

> This obviously needs documentation updates and tests, no?

Documentation, yes. I have that already, but somehow it slipped out of the
patch.

Testing the headers? I dunno, do we have tests for that already? I thought
we did not: it requires an HTTP server (so that the headers are actually
sent) that we can force to check the header...

So I see we have some tests that use Apache, and one that uses our own
http-backend. But is there already anything that logs HTTP requests? I did
not think so, please correct me if I am wrong.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Johannes Schindelin
In reply to this post by Jeff King
Hi Peff,

On Mon, 25 Apr 2016, Jeff King wrote:

> On Mon, Apr 25, 2016 at 03:13:08PM +0200, Johannes Schindelin wrote:
>
> > diff --git a/http.c b/http.c
> > index 4304b80..02d7147 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
> >  
> >  static struct curl_slist *pragma_header;
> >  static struct curl_slist *no_pragma_header;
> > +static struct curl_slist *extra_http_headers;
> >  
> >  static struct active_request_slot *active_queue_head;
> >  
> > @@ -323,6 +324,12 @@ static int http_options(const char *var, const char *value, void *cb)
> >  #endif
> >   }
> >  
> > + if (!strcmp("http.extraheader", var)) {
> > + extra_http_headers =
> > + curl_slist_append(extra_http_headers, value);
> > + return 0;
> > + }
>
> I wondered if this would trigger for "http.*.extraheader", too. And it
> should, as that is all handled in the caller of http_options. Good.

Yes, I was surprised about that, too, but all the other http.* settings
are handled via the urlmatch mechanism (which rewrites the matching
http.<URL>.* settings).

> > @@ -678,8 +685,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
> >   if (remote)
> >   var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
> >  
> > - pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
> > - no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
> > + pragma_header = curl_slist_append(http_get_default_headers(),
> > + "Pragma: no-cache");
> > + no_pragma_header = curl_slist_append(http_get_default_headers(),
> > + "Pragma:");
>
> This looked wrong to me at first, because we are appending to the
> default header list in each case. But the secret sauce is that calling
> http_get_default_headers() actually creates a _new_ list that is a copy
> of the default headers (and the caller can do what they will with it,
> and must free it).
>
> I think that's really the only sane way to do it because of curl's
> interfaces. But maybe it is worth a comment either here, or along with
> http_get_default_headers(), or both.

I chose to rename it to http_copy_default_headers(); That should make it
easier to understand.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] http: support sending custom HTTP headers

Johannes Schindelin
In reply to this post by Johannes Schindelin
We introduce a way to send custom HTTP headers with all requests.

This allows us, for example, to send an extra token from build agents
for temporary access to private repositories. (This is the use case that
triggered this patch.)

This feature can be used like this:

        git -c http.extraheader='Secret: sssh!' fetch $URL $REF

As `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` overrides previous
calls' headers (instead of appending the headers, as this unsuspecting
developer thought initially), we piggyback onto the `Pragma:` setting by
default, and introduce the global helper `http_copy_default_headers()`
to help functions that want to specify HTTP headers themselves.

Signed-off-by: Johannes Schindelin <[hidden email]>
---

Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v2

 Documentation/config.txt |  6 ++++++
 http-push.c              | 10 +++++-----
 http.c                   | 28 +++++++++++++++++++++++++---
 http.h                   |  1 +
 remote-curl.c            |  4 ++--
 5 files changed, 39 insertions(+), 10 deletions(-)

Interdiff vs v1:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 42d2b50..37b9af7 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1655,6 +1655,12 @@ http.emptyAuth::
  a username in the URL, as libcurl normally requires a username for
  authentication.
 
 +http.extraHeader::
 + Pass an additional HTTP header when communicating with a server.  If
 + more than one such entry exists, all of them are added as extra headers.
 + This feature is useful e.g. to increase security, or to allow
 + time-limited access based on expiring tokens.
 +
  http.cookieFile::
  File containing previously stored cookie lines which should be used
  in the Git http session, if they match the server. The file format
 diff --git a/http-push.c b/http-push.c
 index 04eef17..ae2b7f1 100644
 --- a/http-push.c
 +++ b/http-push.c
 @@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
  static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
  {
  struct strbuf buf = STRBUF_INIT;
 - struct curl_slist *dav_headers = http_get_default_headers();
 + struct curl_slist *dav_headers = http_copy_default_headers();
 
  if (options & DAV_HEADER_IF) {
  strbuf_addf(&buf, "If: (<%s>)", lock->token);
 @@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
  static void start_move(struct transfer_request *request)
  {
  struct active_request_slot *slot;
 - struct curl_slist *dav_headers = http_get_default_headers();
 + struct curl_slist *dav_headers = http_copy_default_headers();
 
  slot = get_active_slot();
  slot->callback_func = process_response;
 @@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
  char *ep;
  char timeout_header[25];
  struct remote_lock *lock = NULL;
 - struct curl_slist *dav_headers = http_get_default_headers();
 + struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  char *escaped;
 
 @@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
 - struct curl_slist *dav_headers = http_get_default_headers();
 + struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  struct remote_ls_ctx ls;
 
 @@ -1204,7 +1204,7 @@ static int locking_available(void)
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
 - struct curl_slist *dav_headers = http_get_default_headers();
 + struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  int lock_flags = 0;
  char *escaped;
 diff --git a/http.c b/http.c
 index 02d7147..3d662bb 100644
 --- a/http.c
 +++ b/http.c
 @@ -685,9 +685,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
  if (remote)
  var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
 
 - pragma_header = curl_slist_append(http_get_default_headers(),
 + pragma_header = curl_slist_append(http_copy_default_headers(),
  "Pragma: no-cache");
 - no_pragma_header = curl_slist_append(http_get_default_headers(),
 + no_pragma_header = curl_slist_append(http_copy_default_headers(),
  "Pragma:");
 
  #ifdef USE_CURL_MULTI
 @@ -1175,7 +1175,7 @@ int run_one_slot(struct active_request_slot *slot,
  return handle_curl_result(results);
  }
 
 -struct curl_slist *http_get_default_headers()
 +struct curl_slist *http_copy_default_headers()
  {
  struct curl_slist *headers = NULL, *h;
 
 @@ -1402,7 +1402,7 @@ static int http_request(const char *url,
  {
  struct active_request_slot *slot;
  struct slot_results results;
 - struct curl_slist *headers = http_get_default_headers();
 + struct curl_slist *headers = http_copy_default_headers();
  struct strbuf buf = STRBUF_INIT;
  const char *accept_language;
  int ret;
 diff --git a/http.h b/http.h
 index b0927de..5f13695 100644
 --- a/http.h
 +++ b/http.h
 @@ -106,7 +106,7 @@ extern void step_active_slots(void);
  extern void http_init(struct remote *remote, const char *url,
       int proactive_auth);
  extern void http_cleanup(void);
 -extern struct curl_slist *http_get_default_headers();
 +extern struct curl_slist *http_copy_default_headers();
 
  extern long int git_curl_ipresolve;
  extern int active_requests;
 diff --git a/remote-curl.c b/remote-curl.c
 index 86ba787..672b382 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
  static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
  {
  struct active_request_slot *slot;
 - struct curl_slist *headers = http_get_default_headers();
 + struct curl_slist *headers = http_copy_default_headers();
  struct strbuf buf = STRBUF_INIT;
  int err;
 
 @@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
  static int post_rpc(struct rpc_state *rpc)
  {
  struct active_request_slot *slot;
 - struct curl_slist *headers = http_get_default_headers();
 + struct curl_slist *headers = http_copy_default_headers();
  int use_gzip = rpc->gzip_request;
  char *gzip_body = NULL;
  size_t gzip_size = 0;


diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..37b9af7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1655,6 +1655,12 @@ http.emptyAuth::
  a username in the URL, as libcurl normally requires a username for
  authentication.
 
+http.extraHeader::
+ Pass an additional HTTP header when communicating with a server.  If
+ more than one such entry exists, all of them are added as extra headers.
+ This feature is useful e.g. to increase security, or to allow
+ time-limited access based on expiring tokens.
+
 http.cookieFile::
  File containing previously stored cookie lines which should be used
  in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
  struct strbuf buf = STRBUF_INIT;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
 
  if (options & DAV_HEADER_IF) {
  strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
  struct active_request_slot *slot;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
 
  slot = get_active_slot();
  slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
  char *ep;
  char timeout_header[25];
  struct remote_lock *lock = NULL;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  int lock_flags = 0;
  char *escaped;
diff --git a/http.c b/http.c
index 4304b80..3d662bb 100644
--- a/http.c
+++ b/http.c
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
+static struct curl_slist *extra_http_headers;
 
 static struct active_request_slot *active_queue_head;
 
@@ -323,6 +324,12 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
  }
 
+ if (!strcmp("http.extraheader", var)) {
+ extra_http_headers =
+ curl_slist_append(extra_http_headers, value);
+ return 0;
+ }
+
  /* Fall back on the default ones */
  return git_default_config(var, value, cb);
 }
@@ -678,8 +685,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
  if (remote)
  var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
 
- pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
- no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
+ pragma_header = curl_slist_append(http_copy_default_headers(),
+ "Pragma: no-cache");
+ no_pragma_header = curl_slist_append(http_copy_default_headers(),
+ "Pragma:");
 
 #ifdef USE_CURL_MULTI
  {
@@ -765,6 +774,9 @@ void http_cleanup(void)
 #endif
  curl_global_cleanup();
 
+ curl_slist_free_all(extra_http_headers);
+ extra_http_headers = NULL;
+
  curl_slist_free_all(pragma_header);
  pragma_header = NULL;
 
@@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
  return handle_curl_result(results);
 }
 
+struct curl_slist *http_copy_default_headers()
+{
+ struct curl_slist *headers = NULL, *h;
+
+ for (h = extra_http_headers; h; h = h->next)
+ headers = curl_slist_append(headers, h->data);
+
+ return headers;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
  char *ptr;
@@ -1380,7 +1402,7 @@ static int http_request(const char *url,
 {
  struct active_request_slot *slot;
  struct slot_results results;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_copy_default_headers();
  struct strbuf buf = STRBUF_INIT;
  const char *accept_language;
  int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..5f13695 100644
--- a/http.h
+++ b/http.h
@@ -106,6 +106,7 @@ extern void step_active_slots(void);
 extern void http_init(struct remote *remote, const char *url,
       int proactive_auth);
 extern void http_cleanup(void);
+extern struct curl_slist *http_copy_default_headers();
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..672b382 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
  struct active_request_slot *slot;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_copy_default_headers();
  struct strbuf buf = STRBUF_INIT;
  int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
  struct active_request_slot *slot;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_copy_default_headers();
  int use_gzip = rpc->gzip_request;
  char *gzip_body = NULL;
  size_t gzip_size = 0;
--
2.8.1.306.gff998f2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> writes:

> Testing the headers? I dunno, do we have tests for that already? I thought
> we did not: it requires an HTTP server (so that the headers are actually
> sent) that we can force to check the header...
>
> So I see we have some tests that use Apache, and one that uses our own
> http-backend. But is there already anything that logs HTTP requests? I did
> not think so, please correct me if I am wrong.

I suspect that no codepath in the current system has cared about
what http headers are sent; auth stuff might have but even then I
would imagine that a test for auth would observe the end result
(i.e. it would ask "did the server accept or reject us?") not the
mechanism (i.e. it would not ask "did we correctly send an
Authorization header?").

So I wouldn't be surprised if this topic is the first one that cares
exactly what headers are sent out (eh, rather, "we told Git to send
this and that header, do they appear at the server end?"), in which
case it is very likely that we do not have any existing test that
can be imitated for that purpose X-<.

In other words, the answer to "Do we already have a test so that I
can mimick it instead of thinking of a way to test this?" would
probably be "No".

Do we care about this feature deeply enough to devise a mechanism
to prevent it from getting broken by careless others in the future?


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Jeff King
In reply to this post by Johannes Schindelin
On Tue, Apr 26, 2016 at 05:37:32PM +0200, Johannes Schindelin wrote:

> > I think that's really the only sane way to do it because of curl's
> > interfaces. But maybe it is worth a comment either here, or along with
> > http_get_default_headers(), or both.
>
> I chose to rename it to http_copy_default_headers(); That should make it
> easier to understand.

Thanks, I think that makes it clearer.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] http: support sending custom HTTP headers

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> writes:

> We introduce a way to send custom HTTP headers with all requests.
>
> This allows us, for example, to send an extra token from build agents
> for temporary access to private repositories. (This is the use case that
> triggered this patch.)
>
> This feature can be used like this:
>
> git -c http.extraheader='Secret: sssh!' fetch $URL $REF
>
> As `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` overrides previous
> calls' headers (instead of appending the headers, as this unsuspecting
> developer thought initially), we piggyback onto the `Pragma:` setting by
> default, and introduce the global helper `http_copy_default_headers()`
> to help functions that want to specify HTTP headers themselves.

My reading stuttered at "we piggyback onto the `Pragma:` setting by
default", which made me stop and wonder if a description about a
knob that changes this behaviour and makes us piggyback onto
something else follows.

I guess "by default" you meant that the majority of codepaths set
the headers using no_pragma_header or pragma_header variables, and
by [no_]pragma_header to mean more than just about "Pragma:" by
adding the extra headers to them, you did not have to touch them.
Other codepaths that do not use these two variables but start from
NULL you made them start from these extra headers.

Which makes sense.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 42d2b50..37b9af7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1655,6 +1655,12 @@ http.emptyAuth::
>   a username in the URL, as libcurl normally requires a username for
>   authentication.
>  
> +http.extraHeader::
> + Pass an additional HTTP header when communicating with a server.  If
> + more than one such entry exists, all of them are added as extra headers.
> + This feature is useful e.g. to increase security, or to allow
> + time-limited access based on expiring tokens.
> +

I think one-time/short-lived use case does not want to have this in
a configuration file, and instead want to do the command line thing
you illustrated in the proposed log message.  I however wonder if
there are other use cases where having this in $GIT_DIR/config for
repeated use is useful.  If there is, not being able to override a
configured value per invocation would become a problem.

Peff, what do you think?  I vaguely recollect that you did a hack to
one variable that declares "an empty value means discard accumulated
values so far" or something like that, and this variable deserves a
mechanism like that, too.

> diff --git a/http.c b/http.c
> index 4304b80..3d662bb 100644
> --- a/http.c
> +++ b/http.c
> @@ -1163,6 +1175,16 @@ int run_one_slot(struct active_request_slot *slot,
>   return handle_curl_result(results);
>  }
>  
> +struct curl_slist *http_copy_default_headers()

struct curl_slist *http_copy_default_headers(void)

> +{
> + struct curl_slist *headers = NULL, *h;
> +
> + for (h = extra_http_headers; h; h = h->next)
> + headers = curl_slist_append(headers, h->data);
> +
> + return headers;
> +}
> +
> diff --git a/http.h b/http.h
> index 4ef4bbd..5f13695 100644
> --- a/http.h
> +++ b/http.h
> @@ -106,6 +106,7 @@ extern void step_active_slots(void);
>  extern void http_init(struct remote *remote, const char *url,
>        int proactive_auth);
>  extern void http_cleanup(void);
> +extern struct curl_slist *http_copy_default_headers();

extern struct curl_slist *http_copy_default_headers(void);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] http: support sending custom HTTP headers

Jeff King
On Tue, Apr 26, 2016 at 10:03:14AM -0700, Junio C Hamano wrote:

> > +http.extraHeader::
> > + Pass an additional HTTP header when communicating with a server.  If
> > + more than one such entry exists, all of them are added as extra headers.
> > + This feature is useful e.g. to increase security, or to allow
> > + time-limited access based on expiring tokens.
> > +
>
> I think one-time/short-lived use case does not want to have this in
> a configuration file, and instead want to do the command line thing
> you illustrated in the proposed log message.  I however wonder if
> there are other use cases where having this in $GIT_DIR/config for
> repeated use is useful.  If there is, not being able to override a
> configured value per invocation would become a problem.
>
> Peff, what do you think?  I vaguely recollect that you did a hack to
> one variable that declares "an empty value means discard accumulated
> values so far" or something like that, and this variable deserves a
> mechanism like that, too.

Yes, it was for credential.helper. I think the _implementation_ is a
hack (because each callback has to handle it individually), but the
user-facing part is reasonably elegant, given the constraint of not
introducing new syntax.

In this case it would be something like:

diff --git a/http.c b/http.c
index 3d662bb..a7a4be5 100644
--- a/http.c
+++ b/http.c
@@ -325,8 +325,13 @@ static int http_options(const char *var, const char *value, void *cb)
  }
 
  if (!strcmp("http.extraheader", var)) {
- extra_http_headers =
- curl_slist_append(extra_http_headers, value);
+ if (*value)
+ extra_http_headers =
+ curl_slist_append(extra_http_headers, value);
+ else {
+ curl_slist_free_all(extra_http_headers);
+ extra_http_headers = NULL;
+ }
  return 0;
  }
 

But I think this block (even before my patch) also needs to handle the
case where "value" is NULL (presumably by complaining with
config_error_nonbool).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] http: support sending custom HTTP headers

Junio C Hamano
Jeff King <[hidden email]> writes:

> But I think this block (even before my patch) also needs to handle the
> case where "value" is NULL (presumably by complaining with
> config_error_nonbool).

OK, so squashes found to be necessary so far amounts to the attached
patch.  I still haven't figured out the best way to rephrase the "by
default" in the proposed log message that made me stutter while
reading it, though.


 http.c | 13 ++++++++++---
 http.h |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index 6c4d2ed..aae9944 100644
--- a/http.c
+++ b/http.c
@@ -325,8 +325,15 @@ static int http_options(const char *var, const char *value, void *cb)
  }
 
  if (!strcmp("http.extraheader", var)) {
- extra_http_headers =
- curl_slist_append(extra_http_headers, value);
+ if (!value) {
+ return config_error_nonbool(var);
+ } else if (!*value) {
+ curl_slist_free_all(extra_http_headers);
+ extra_http_headers = NULL;
+ } else {
+ extra_http_headers =
+ curl_slist_append(extra_http_headers, value);
+ }
  return 0;
  }
 
@@ -1172,7 +1179,7 @@ int run_one_slot(struct active_request_slot *slot,
  return handle_curl_result(results);
 }
 
-struct curl_slist *http_copy_default_headers()
+struct curl_slist *http_copy_default_headers(void)
 {
  struct curl_slist *headers = NULL, *h;
 
diff --git a/http.h b/http.h
index 5f13695..36f558b 100644
--- a/http.h
+++ b/http.h
@@ -106,7 +106,7 @@ extern void step_active_slots(void);
 extern void http_init(struct remote *remote, const char *url,
       int proactive_auth);
 extern void http_cleanup(void);
-extern struct curl_slist *http_copy_default_headers();
+extern struct curl_slist *http_copy_default_headers(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Jeff King
In reply to this post by Johannes Schindelin
On Tue, Apr 26, 2016 at 05:33:33PM +0200, Johannes Schindelin wrote:

> Testing the headers? I dunno, do we have tests for that already? I thought
> we did not: it requires an HTTP server (so that the headers are actually
> sent) that we can force to check the header...
>
> So I see we have some tests that use Apache, and one that uses our own
> http-backend. But is there already anything that logs HTTP requests? I did
> not think so, please correct me if I am wrong.

You can ask apache to check for specific headers. Like this:

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9317ba0..de5a8fe 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,6 +102,12 @@ Alias /auth/dumb/ www/auth/dumb/
  SetEnv GIT_HTTP_EXPORT_ALL
  Header set Set-Cookie name=value
 </LocationMatch>
+<LocationMatch /smart_headers/>
+ Require expr %{HTTP:x-magic-one} == 'abra'
+ Require expr %{HTTP:x-magic-two} == 'cadabra'
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+</LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 58207d8..e44fe72 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
  test_line_count = 100000 tags
 '
 
+test_expect_success 'custom http headers' '
+ test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+ git -c http.extraheader="x-magic-one: abra" \
+    -c http.extraheader="x-magic-two: cadabra" \
+    fetch "$HTTPD_URL/smart_headers/repo.git"
+'
+
 stop_httpd
 test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] http: support sending custom HTTP headers

Jeff King
In reply to this post by Junio C Hamano
On Tue, Apr 26, 2016 at 10:20:19AM -0700, Junio C Hamano wrote:

> Jeff King <[hidden email]> writes:
>
> > But I think this block (even before my patch) also needs to handle the
> > case where "value" is NULL (presumably by complaining with
> > config_error_nonbool).
>
> OK, so squashes found to be necessary so far amounts to the attached
> patch.  I still haven't figured out the best way to rephrase the "by
> default" in the proposed log message that made me stutter while
> reading it, though.

I think that part is just trying to explain the implementation hackery.
Maybe something like:

  Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
  a single list, overriding any previous call. This means we have to
  collect _all_ of the headers we want to use into a single list, and
  feed it to curl in one shot. Since we already unconditionally set a
  "pragma" header when initializing the curl handles, we can add our new
  headers to that list.

  For callers which override the default header list (like probe_rpc),
  we provide `http_copy_default_headers()` so they can do the same
  trick.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] http: support sending custom HTTP headers

Junio C Hamano
In reply to this post by Johannes Schindelin
Johannes Schindelin <[hidden email]> writes:

>  diff --git a/Documentation/config.txt b/Documentation/config.txt
>  index 42d2b50..37b9af7 100644
>  --- a/Documentation/config.txt
>  +++ b/Documentation/config.txt
>  @@ -1655,6 +1655,12 @@ http.emptyAuth::
>   a username in the URL, as libcurl normally requires a username for
>   authentication.
>  
>  +http.extraHeader::
>  + Pass an additional HTTP header when communicating with a server.  If
>  + more than one such entry exists, all of them are added as extra headers.
>  + This feature is useful e.g. to increase security, or to allow
>  + time-limited access based on expiring tokens.

I am unsure about the last two lines.  Is it necessary to have them?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] http: support sending custom HTTP headers

Johannes Schindelin
In reply to this post by Jeff King
Hi Peff,

On Tue, 26 Apr 2016, Jeff King wrote:

> On Tue, Apr 26, 2016 at 10:20:19AM -0700, Junio C Hamano wrote:
>
> > Jeff King <[hidden email]> writes:
> >
> > > But I think this block (even before my patch) also needs to handle the
> > > case where "value" is NULL (presumably by complaining with
> > > config_error_nonbool).
> >
> > OK, so squashes found to be necessary so far amounts to the attached
> > patch.  I still haven't figured out the best way to rephrase the "by
> > default" in the proposed log message that made me stutter while
> > reading it, though.
>
> I think that part is just trying to explain the implementation hackery.
> Maybe something like:
>
>   Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
>   a single list, overriding any previous call. This means we have to
>   collect _all_ of the headers we want to use into a single list, and
>   feed it to curl in one shot. Since we already unconditionally set a
>   "pragma" header when initializing the curl handles, we can add our new
>   headers to that list.
>
>   For callers which override the default header list (like probe_rpc),
>   we provide `http_copy_default_headers()` so they can do the same
>   trick.

Thank you very much for this. I replaced that part of the commit message
with your version.

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] http: support sending custom HTTP headers

Johannes Schindelin
In reply to this post by Johannes Schindelin
We introduce a way to send custom HTTP headers with all requests.

This allows us, for example, to send an extra token from build agents
for temporary access to private repositories. (This is the use case that
triggered this patch.)

This feature can be used like this:

        git -c http.extraheader='Secret: sssh!' fetch $URL $REF

Note that `curl_easy_setopt(..., CURLOPT_HTTPHEADER, ...)` takes only
a single list, overriding any previous call. This means we have to
collect _all_ of the headers we want to use into a single list, and
feed it to cURL in one shot. Since we already unconditionally set a
"pragma" header when initializing the curl handles, we can add our new
headers to that list.

For callers which override the default header list (like probe_rpc),
we provide `http_copy_default_headers()` so they can do the same
trick.

Big thanks to Jeff King and Junio Hamano for their outstanding help and
patient reviews.

Signed-off-by: Johannes Schindelin <[hidden email]>
---

Changes since v2:
        - now using Peff's much improved wording in the commit message
        - skipped the last two lines from the documentation of the feature
        - fixed function declaration/definition by using `(void)`
        - we now allow resetting extra headers with an empty value
        - added a test

Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v3
 Documentation/config.txt   |  6 ++++++
 http-push.c                | 10 +++++-----
 http.c                     | 35 ++++++++++++++++++++++++++++++++---
 http.h                     |  1 +
 remote-curl.c              |  4 ++--
 t/t5550-http-fetch-dumb.sh |  8 ++++++++
 6 files changed, 54 insertions(+), 10 deletions(-)
Interdiff vs v2:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index 37b9af7..c7bbe98 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1657,9 +1657,9 @@ http.emptyAuth::
 
  http.extraHeader::
  Pass an additional HTTP header when communicating with a server.  If
 - more than one such entry exists, all of them are added as extra headers.
 - This feature is useful e.g. to increase security, or to allow
 - time-limited access based on expiring tokens.
 + more than one such entry exists, all of them are added as extra
 + headers.  To allow overriding the settings inherited from the system
 + config, an empty value will reset the extra headers to the empty list.
 
  http.cookieFile::
  File containing previously stored cookie lines which should be used
 diff --git a/http.c b/http.c
 index 3d662bb..985b995 100644
 --- a/http.c
 +++ b/http.c
 @@ -325,8 +325,15 @@ static int http_options(const char *var, const char *value, void *cb)
  }
 
  if (!strcmp("http.extraheader", var)) {
 - extra_http_headers =
 - curl_slist_append(extra_http_headers, value);
 + if (!value) {
 + return config_error_nonbool(var);
 + } else if (!*value) {
 + curl_slist_free_all(extra_http_headers);
 + extra_http_headers = NULL;
 + } else {
 + extra_http_headers =
 + curl_slist_append(extra_http_headers, value);
 + }
  return 0;
  }
 
 @@ -1175,7 +1182,7 @@ int run_one_slot(struct active_request_slot *slot,
  return handle_curl_result(results);
  }
 
 -struct curl_slist *http_copy_default_headers()
 +struct curl_slist *http_copy_default_headers(void)
  {
  struct curl_slist *headers = NULL, *h;
 
 diff --git a/http.h b/http.h
 index 5f13695..36f558b 100644
 --- a/http.h
 +++ b/http.h
 @@ -106,7 +106,7 @@ extern void step_active_slots(void);
  extern void http_init(struct remote *remote, const char *url,
       int proactive_auth);
  extern void http_cleanup(void);
 -extern struct curl_slist *http_copy_default_headers();
 +extern struct curl_slist *http_copy_default_headers(void);
 
  extern long int git_curl_ipresolve;
  extern int active_requests;
 diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
 index 48e2ab6..96425b1 100755
 --- a/t/t5550-http-fetch-dumb.sh
 +++ b/t/t5550-http-fetch-dumb.sh
 @@ -267,5 +267,13 @@ test_expect_success 'git client does not send an empty Accept-Language' '
  ! grep "^Accept-Language:" stderr
  '
 
 +test_expect_success 'extra HTTP headers are sent' '
 + GIT_CURL_VERBOSE=1 \
 + git -c http.extraheader="Hello: World" \
 + ls-remote "$HTTPD_URL/dumb/repo.git" >out 2>err &&
 + test_i18ngrep "Hello: World" err >hello.txt &&
 + test_line_count = 2 hello.txt
 +'
 +
  stop_httpd
  test_done


diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..c7bbe98 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1655,6 +1655,12 @@ http.emptyAuth::
  a username in the URL, as libcurl normally requires a username for
  authentication.
 
+http.extraHeader::
+ Pass an additional HTTP header when communicating with a server.  If
+ more than one such entry exists, all of them are added as extra
+ headers.  To allow overriding the settings inherited from the system
+ config, an empty value will reset the extra headers to the empty list.
+
 http.cookieFile::
  File containing previously stored cookie lines which should be used
  in the Git http session, if they match the server. The file format
diff --git a/http-push.c b/http-push.c
index bd60668..ae2b7f1 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,7 +211,7 @@ static void curl_setup_http(CURL *curl, const char *url,
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
  struct strbuf buf = STRBUF_INIT;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
 
  if (options & DAV_HEADER_IF) {
  strbuf_addf(&buf, "If: (<%s>)", lock->token);
@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
  struct active_request_slot *slot;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
 
  slot = get_active_slot();
  slot->callback_func = process_response;
@@ -845,7 +845,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
  char *ep;
  char timeout_header[25];
  struct remote_lock *lock = NULL;
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  char *escaped;
 
@@ -1126,7 +1126,7 @@ static void remote_ls(const char *path, int flags,
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  struct remote_ls_ctx ls;
 
@@ -1204,7 +1204,7 @@ static int locking_available(void)
  struct slot_results results;
  struct strbuf in_buffer = STRBUF_INIT;
  struct buffer out_buffer = { STRBUF_INIT, 0 };
- struct curl_slist *dav_headers = NULL;
+ struct curl_slist *dav_headers = http_copy_default_headers();
  struct xml_ctx ctx;
  int lock_flags = 0;
  char *escaped;
diff --git a/http.c b/http.c
index 4304b80..985b995 100644
--- a/http.c
+++ b/http.c
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
+static struct curl_slist *extra_http_headers;
 
 static struct active_request_slot *active_queue_head;
 
@@ -323,6 +324,19 @@ static int http_options(const char *var, const char *value, void *cb)
 #endif
  }
 
+ if (!strcmp("http.extraheader", var)) {
+ if (!value) {
+ return config_error_nonbool(var);
+ } else if (!*value) {
+ curl_slist_free_all(extra_http_headers);
+ extra_http_headers = NULL;
+ } else {
+ extra_http_headers =
+ curl_slist_append(extra_http_headers, value);
+ }
+ return 0;
+ }
+
  /* Fall back on the default ones */
  return git_default_config(var, value, cb);
 }
@@ -678,8 +692,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
  if (remote)
  var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
 
- pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
- no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
+ pragma_header = curl_slist_append(http_copy_default_headers(),
+ "Pragma: no-cache");
+ no_pragma_header = curl_slist_append(http_copy_default_headers(),
+ "Pragma:");
 
 #ifdef USE_CURL_MULTI
  {
@@ -765,6 +781,9 @@ void http_cleanup(void)
 #endif
  curl_global_cleanup();
 
+ curl_slist_free_all(extra_http_headers);
+ extra_http_headers = NULL;
+
  curl_slist_free_all(pragma_header);
  pragma_header = NULL;
 
@@ -1163,6 +1182,16 @@ int run_one_slot(struct active_request_slot *slot,
  return handle_curl_result(results);
 }
 
+struct curl_slist *http_copy_default_headers(void)
+{
+ struct curl_slist *headers = NULL, *h;
+
+ for (h = extra_http_headers; h; h = h->next)
+ headers = curl_slist_append(headers, h->data);
+
+ return headers;
+}
+
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
  char *ptr;
@@ -1380,7 +1409,7 @@ static int http_request(const char *url,
 {
  struct active_request_slot *slot;
  struct slot_results results;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_copy_default_headers();
  struct strbuf buf = STRBUF_INIT;
  const char *accept_language;
  int ret;
diff --git a/http.h b/http.h
index 4ef4bbd..36f558b 100644
--- a/http.h
+++ b/http.h
@@ -106,6 +106,7 @@ extern void step_active_slots(void);
 extern void http_init(struct remote *remote, const char *url,
       int proactive_auth);
 extern void http_cleanup(void);
+extern struct curl_slist *http_copy_default_headers(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 15e48e2..672b382 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -474,7 +474,7 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
  struct active_request_slot *slot;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_copy_default_headers();
  struct strbuf buf = STRBUF_INIT;
  int err;
 
@@ -503,7 +503,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 static int post_rpc(struct rpc_state *rpc)
 {
  struct active_request_slot *slot;
- struct curl_slist *headers = NULL;
+ struct curl_slist *headers = http_copy_default_headers();
  int use_gzip = rpc->gzip_request;
  char *gzip_body = NULL;
  size_t gzip_size = 0;
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 48e2ab6..96425b1 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -267,5 +267,13 @@ test_expect_success 'git client does not send an empty Accept-Language' '
  ! grep "^Accept-Language:" stderr
 '
 
+test_expect_success 'extra HTTP headers are sent' '
+ GIT_CURL_VERBOSE=1 \
+ git -c http.extraheader="Hello: World" \
+ ls-remote "$HTTPD_URL/dumb/repo.git" >out 2>err &&
+ test_i18ngrep "Hello: World" err >hello.txt &&
+ test_line_count = 2 hello.txt
+'
+
 stop_httpd
 test_done
--
2.8.1.306.gff998f2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] http: support sending custom HTTP headers

Johannes Schindelin
In reply to this post by Junio C Hamano
Hi Junio,

On Tue, 26 Apr 2016, Junio C Hamano wrote:

> Jeff King <[hidden email]> writes:
>
> > But I think this block (even before my patch) also needs to handle the
> > case where "value" is NULL (presumably by complaining with
> > config_error_nonbool).
>
> OK, so squashes found to be necessary so far amounts to the attached
> patch.

Thanks.
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Johannes Schindelin
In reply to this post by Jeff King
Hi Peff,

On Tue, 26 Apr 2016, Jeff King wrote:

> On Tue, Apr 26, 2016 at 05:33:33PM +0200, Johannes Schindelin wrote:
>
> > Testing the headers? I dunno, do we have tests for that already? I thought
> > we did not: it requires an HTTP server (so that the headers are actually
> > sent) that we can force to check the header...
> >
> > So I see we have some tests that use Apache, and one that uses our own
> > http-backend. But is there already anything that logs HTTP requests? I did
> > not think so, please correct me if I am wrong.
>
> You can ask apache to check for specific headers. Like this:
>
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 9317ba0..de5a8fe 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -102,6 +102,12 @@ Alias /auth/dumb/ www/auth/dumb/
>   SetEnv GIT_HTTP_EXPORT_ALL
>   Header set Set-Cookie name=value
>  </LocationMatch>
> +<LocationMatch /smart_headers/>
> + Require expr %{HTTP:x-magic-one} == 'abra'
> + Require expr %{HTTP:x-magic-two} == 'cadabra'
> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
> + SetEnv GIT_HTTP_EXPORT_ALL
> +</LocationMatch>
>  ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
>  ScriptAlias /broken_smart/ broken-smart-http.sh/
>  ScriptAlias /error/ error.sh/
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 58207d8..e44fe72 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -282,5 +282,12 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
>   test_line_count = 100000 tags
>  '
>  
> +test_expect_success 'custom http headers' '
> + test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> + git -c http.extraheader="x-magic-one: abra" \
> +    -c http.extraheader="x-magic-two: cadabra" \
> +    fetch "$HTTPD_URL/smart_headers/repo.git"
> +'
> +
>  stop_httpd
>  test_done

That's pretty easy.

After sleeping over the issue, I realized, though, that the test can use
the very same method *I* used to verify that the headers are sent: using
GIT_CURL_VERBOSE.

I hope you do not mind that I used this method instead.

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] http: Support sending custom HTTP headers

Jeff King
On Wed, Apr 27, 2016 at 08:31:50AM +0200, Johannes Schindelin wrote:

> > +test_expect_success 'custom http headers' '
> > + test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
> > + git -c http.extraheader="x-magic-one: abra" \
> > +    -c http.extraheader="x-magic-two: cadabra" \
> > +    fetch "$HTTPD_URL/smart_headers/repo.git"
> > +'
> > +
> >  stop_httpd
> >  test_done
>
> That's pretty easy.
>
> After sleeping over the issue, I realized, though, that the test can use
> the very same method *I* used to verify that the headers are sent: using
> GIT_CURL_VERBOSE.
>
> I hope you do not mind that I used this method instead.

TBH, I think mine is a little more robust, but I don't overly care
either way.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
1234 ... 6