From: Johannes Schindelin <hidden> Date: 2016-06-16 02:18:58
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 <redacted>
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(-)
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;staticstructcurl_slist*pragma_header;staticstructcurl_slist*no_pragma_header;+staticstructcurl_slist*extra_http_headers;staticstructactive_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);+return0;+}+/* Fall back on the default ones */returngit_default_config(var,value,cb);}
On Mon, Apr 25, 2016 at 6:13 AM, Johannes Schindelin
[off-list ref] 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.
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
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).
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
@@ -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
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;staticstructcurl_slist*pragma_header;staticstructcurl_slist*no_pragma_header;+staticstructcurl_slist*extra_http_headers;staticstructactive_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);+return0;+}+/* Fall back on the default ones */returngit_default_config(var,value,cb);}
From: Jeff King <hidden> Date: 2016-06-16 02:19:00
On Tue, Apr 26, 2016 at 05:37:32PM +0200, Johannes Schindelin wrote:
quoted
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.
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:01
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 <redacted>
---
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
@@ -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
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;staticstructcurl_slist*pragma_header;staticstructcurl_slist*no_pragma_header;+staticstructcurl_slist*extra_http_headers;staticstructactive_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){+returnconfig_error_nonbool(var);+}elseif(!*value){+curl_slist_free_all(extra_http_headers);+extra_http_headers=NULL;+}else{+extra_http_headers=+curl_slist_append(extra_http_headers,value);+}+return0;+}+/* Fall back on the default ones */returngit_default_config(var,value,cb);}
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:01
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 <redacted>
---
Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v4
The only change vs v3 is that I replaced my flimsical test by Peff's (with
*one* change: I realized that we need to group the Require statements in a
<RequireAll> block when I tried to verify that the test fails when I
modify the first header).
Documentation/config.txt | 6 ++++++
http-push.c | 10 +++++-----
http.c | 35 ++++++++++++++++++++++++++++++++---
http.h | 1 +
remote-curl.c | 4 ++--
t/lib-httpd/apache.conf | 8 ++++++++
t/t5551-http-fetch-smart.sh | 7 +++++++
7 files changed, 61 insertions(+), 10 deletions(-)
Interdiff vs v3:
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9317ba0..b8ed96f 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -102,6 +102,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_HTTP_EXPORT_ALL
Header set Set-Cookie name=value
</LocationMatch>
+<LocationMatch /smart_headers/>
+ <RequireAll>
+ Require expr %{HTTP:x-magic-one} == 'abra'
+ Require expr %{HTTP:x-magic-two} == 'cadabra'
+ </RequireAll>
+ 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/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 96425b1..48e2ab6 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -267,13 +267,5 @@ 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/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
@@ -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
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;staticstructcurl_slist*pragma_header;staticstructcurl_slist*no_pragma_header;+staticstructcurl_slist*extra_http_headers;staticstructactive_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){+returnconfig_error_nonbool(var);+}elseif(!*value){+curl_slist_free_all(extra_http_headers);+extra_http_headers=NULL;+}else{+extra_http_headers=+curl_slist_append(extra_http_headers,value);+}+return0;+}+/* Fall back on the default ones */returngit_default_config(var,value,cb);}
From: Jeff King <hidden> Date: 2016-06-16 02:19:02
On Wed, Apr 27, 2016 at 02:20:37PM +0200, Johannes Schindelin wrote:
The only change vs v3 is that I replaced my flimsical test by Peff's (with
*one* change: I realized that we need to group the Require statements in a
<RequireAll> block when I tried to verify that the test fails when I
modify the first header).
Whoops, I didn't actually test that case. Thanks for catching (as you
might guess, I wanted to make sure we handle multiple values correctly).
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:03
To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.
This patch allows us to configure these extra HTTP headers for use with
`git submodule update`, too. It requires somewhat special handling:
submodules do not share the parent project's config. It would be
incorrect to simply reuse that specific part of the parent's config.
Instead, the config option needs to be specified on the command-line or
in ~/.gitconfig or friends.
Example: git -c http.extraheader="Secret: Sauce" submodule update --init
Signed-off-by: Johannes Schindelin <redacted>
---
builtin/submodule--helper.c | 4 +++-
t/t5551-http-fetch-smart.sh | 11 ++++++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:03
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 <redacted>
---
Documentation/config.txt | 6 ++++++
http-push.c | 10 +++++-----
http.c | 35 ++++++++++++++++++++++++++++++++---
http.h | 1 +
remote-curl.c | 4 ++--
t/lib-httpd/apache.conf | 8 ++++++++
t/t5551-http-fetch-smart.sh | 7 +++++++
7 files changed, 61 insertions(+), 10 deletions(-)
@@ -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
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;staticstructcurl_slist*pragma_header;staticstructcurl_slist*no_pragma_header;+staticstructcurl_slist*extra_http_headers;staticstructactive_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){+returnconfig_error_nonbool(var);+}elseif(!*value){+curl_slist_free_all(extra_http_headers);+extra_http_headers=NULL;+}else{+extra_http_headers=+curl_slist_append(extra_http_headers,value);+}+return0;+}+/* Fall back on the default ones */returngit_default_config(var,value,cb);}
Should we consider just white-listing all of "http.*"?
That would help other cases which have come up, like:
http://thread.gmane.org/gmane.comp.version-control.git/264840
which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).
-Peff
Should we consider just white-listing all of "http.*"?
That would help other cases which have come up, like:
http://thread.gmane.org/gmane.comp.version-control.git/264840
which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).
I considered that, and thought that it might be dangerous, what with me
not vetting carefully which http.* variables are safe to pass on to the
submodules' update and which are not.
So I had a look now, and the most prominent potential problem is the
http.cookieFile setting, which could be reused all of a sudden if we
made my patch more general.
But then, we are talking about the code that filters what gets passed via
the *command-line*. And to be quite honest, I am not sure that we should
actually filter out *any* of these settings.
The commit message that introduced this particular filtering has this
rationale to let only credential.* through:
GIT_CONFIG_PARAMETERS is special, and we actually do want to
preserve these settings. However, we do not want to preserve all
configuration as many things should be left specific to the parent
project.
Add a git submodule--helper function, sanitize-config, which shall be
used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
except a small subset that are known to be safe and necessary.
Dunno. I tried to err on the side of caution... But this sounds maybe a
bit *too* cautious?
Jacob, Junio?
Ciao,
Dscho
From: Jeff King <hidden> Date: 2016-06-16 02:19:03
On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:
quoted
Should we consider just white-listing all of "http.*"?
That would help other cases which have come up, like:
http://thread.gmane.org/gmane.comp.version-control.git/264840
which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).
I considered that, and thought that it might be dangerous, what with me
not vetting carefully which http.* variables are safe to pass on to the
submodules' update and which are not.
So I had a look now, and the most prominent potential problem is the
http.cookieFile setting, which could be reused all of a sudden if we
made my patch more general.
But then, we are talking about the code that filters what gets passed via
the *command-line*. And to be quite honest, I am not sure that we should
actually filter out *any* of these settings.
The intent of the whitelist (from my recollection of the discussion) is
to filter out config that must be repo-specific. E.g., core.worktree or
core.bare should definitely _not_ be passed to a submodule.
I don't know if there are others. We started with a whitelist because it
was the smallest and safest change away from the status quo. A blacklist
would also work, with the risk that we might let through nonsense in
some cases (but only if the user triggers us to do so).
The commit message that introduced this particular filtering has this
rationale to let only credential.* through:
GIT_CONFIG_PARAMETERS is special, and we actually do want to
preserve these settings. However, we do not want to preserve all
configuration as many things should be left specific to the parent
project.
Add a git submodule--helper function, sanitize-config, which shall be
used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
except a small subset that are known to be safe and necessary.
Dunno. I tried to err on the side of caution... But this sounds maybe a
bit *too* cautious?
So if we all agree that the sanitizing is really about preventing
repo-specific variables from leaking, and not any kind of security
boundary, I think we should generally be pretty liberal in whitelisting
things.
I can certainly come up with a pathological case where using it as a
security boundary may have some practical use, but in general I think it
is mostly getting in the way of what users are trying to do.
-Peff
From: Jeff King <hidden> Date: 2016-06-16 02:19:03
On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:
quoted
Should we consider just white-listing all of "http.*"?
That would help other cases which have come up, like:
http://thread.gmane.org/gmane.comp.version-control.git/264840
which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).
I considered that, and thought that it might be dangerous, what with me
not vetting carefully which http.* variables are safe to pass on to the
submodules' update and which are not.
BTW, just in case you or anybody else ends up playing around with this
and finds your tests do not work as expected: the config pass-through
feature is somewhat broken for anything except cloning. I just posted
some fixes in:
http://thread.gmane.org/gmane.comp.version-control.git/292466/focus=292875
-Peff
From: Jacob Keller <hidden> Date: 2016-06-16 02:19:03
On Thu, Apr 28, 2016 at 6:49 AM, Jeff King [off-list ref] wrote:
On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:
quoted
quoted
Should we consider just white-listing all of "http.*"?
That would help other cases which have come up, like:
http://thread.gmane.org/gmane.comp.version-control.git/264840
which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).
I considered that, and thought that it might be dangerous, what with me
not vetting carefully which http.* variables are safe to pass on to the
submodules' update and which are not.
So I had a look now, and the most prominent potential problem is the
http.cookieFile setting, which could be reused all of a sudden if we
made my patch more general.
But then, we are talking about the code that filters what gets passed via
the *command-line*. And to be quite honest, I am not sure that we should
actually filter out *any* of these settings.
The intent of the whitelist (from my recollection of the discussion) is
to filter out config that must be repo-specific. E.g., core.worktree or
core.bare should definitely _not_ be passed to a submodule.
I don't know if there are others. We started with a whitelist because it
was the smallest and safest change away from the status quo. A blacklist
would also work, with the risk that we might let through nonsense in
some cases (but only if the user triggers us to do so).
quoted
The commit message that introduced this particular filtering has this
rationale to let only credential.* through:
GIT_CONFIG_PARAMETERS is special, and we actually do want to
preserve these settings. However, we do not want to preserve all
configuration as many things should be left specific to the parent
project.
Add a git submodule--helper function, sanitize-config, which shall be
used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
except a small subset that are known to be safe and necessary.
Dunno. I tried to err on the side of caution... But this sounds maybe a
bit *too* cautious?
So if we all agree that the sanitizing is really about preventing
repo-specific variables from leaking, and not any kind of security
boundary, I think we should generally be pretty liberal in whitelisting
things.
I can certainly come up with a pathological case where using it as a
security boundary may have some practical use, but in general I think it
is mostly getting in the way of what users are trying to do.
-Peff
I think I prefer a blacklist approach, since it reduces the need for
future changes, since most cases will either not put config on the
environment or (based on feedback on the mailing list and bug reports)
the user will believe it should be applied.
A black list which only removed configurations we know are harmful
would be easier to maintain but risks new additions forgetting to do
so. A whitelist means we only fix things as they come up but also
means we aren't "breaking" anything that works today, where as a
blacklist could break something that works today.
Thanks,
Jake
From: Jeff King <hidden> Date: 2016-06-16 02:19:03
On Thu, Apr 28, 2016 at 08:37:10AM -0700, Jacob Keller wrote:
I think I prefer a blacklist approach, since it reduces the need for
future changes, since most cases will either not put config on the
environment or (based on feedback on the mailing list and bug reports)
the user will believe it should be applied.
A black list which only removed configurations we know are harmful
would be easier to maintain but risks new additions forgetting to do
so. A whitelist means we only fix things as they come up but also
means we aren't "breaking" anything that works today, where as a
blacklist could break something that works today.
I think the key thing with a blacklist is somebody has to go to the work
to audit the existing keys.
-Peff
From: Stefan Beller <hidden> Date: 2016-06-16 02:19:03
On Thu, Apr 28, 2016 at 8:39 AM, Jeff King [off-list ref] wrote:
On Thu, Apr 28, 2016 at 08:37:10AM -0700, Jacob Keller wrote:
quoted
I think I prefer a blacklist approach, since it reduces the need for
future changes, since most cases will either not put config on the
environment or (based on feedback on the mailing list and bug reports)
the user will believe it should be applied.
A black list which only removed configurations we know are harmful
would be easier to maintain but risks new additions forgetting to do
so. A whitelist means we only fix things as they come up but also
means we aren't "breaking" anything that works today, where as a
blacklist could break something that works today.
I think the key thing with a blacklist is somebody has to go to the work
to audit the existing keys.
Would it be sufficient to wait until someone screams at the mailing list
for some key to be blacklisted? (I mean in the short term that would be
of less quality, but relying on the larger community would result in a better
end result? So your going through is just a jump start this process of
listening to the community?)
Thanks,
Stefan
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jeff King <hidden> Date: 2016-06-16 02:19:04
On Thu, Apr 28, 2016 at 09:09:44AM -0700, Stefan Beller wrote:
quoted
I think the key thing with a blacklist is somebody has to go to the work
to audit the existing keys.
Would it be sufficient to wait until someone screams at the mailing list
for some key to be blacklisted? (I mean in the short term that would be
of less quality, but relying on the larger community would result in a better
end result? So your going through is just a jump start this process of
listening to the community?)
Yeah, I think ultimately we will rely on the community. But I would feel
a lot more comfortable if somebody made at least a single pass.
I'll be curious what Junio says, too. I generally defer to him on how
conservative we want to be in cases like this.
-Peff
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:10
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 <redacted>
---
Documentation/config.txt | 6 ++++++
http-push.c | 10 +++++-----
http.c | 35 ++++++++++++++++++++++++++++++++---
http.h | 1 +
remote-curl.c | 4 ++--
t/lib-httpd/apache.conf | 8 ++++++++
t/t5551-http-fetch-smart.sh | 7 +++++++
7 files changed, 61 insertions(+), 10 deletions(-)
@@ -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
@@ -114,6 +114,7 @@ static unsigned long http_auth_methods = CURLAUTH_ANY;staticstructcurl_slist*pragma_header;staticstructcurl_slist*no_pragma_header;+staticstructcurl_slist*extra_http_headers;staticstructactive_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){+returnconfig_error_nonbool(var);+}elseif(!*value){+curl_slist_free_all(extra_http_headers);+extra_http_headers=NULL;+}else{+extra_http_headers=+curl_slist_append(extra_http_headers,value);+}+return0;+}+/* Fall back on the default ones */returngit_default_config(var,value,cb);}
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:10
To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.
This patch allows us to configure these extra HTTP headers for use with
`git submodule update`, too. It requires somewhat special handling:
submodules do not share the parent project's config. It would be
incorrect to simply reuse that specific part of the parent's config.
Instead, the config option needs to be specified on the command-line or
in ~/.gitconfig or friends.
Example: git -c http.extraheader="Secret: Sauce" submodule update --init
Signed-off-by: Johannes Schindelin <redacted>
---
builtin/submodule--helper.c | 3 ++-
t/t5551-http-fetch-smart.sh | 11 ++++++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2016-06-16 02:19:10
On Wed, May 04, 2016 at 08:14:07AM +0200, Johannes Schindelin wrote:
My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.
v6 supports submodules better by allowing
git -c http.extraheader submodule update
to work as one would expect intuitively.
Johannes Schindelin (2):
http: support sending custom HTTP headers
I think this one is in "next", with "will merge to master" in the latest
What's Cooking. So fortunately it does not have any changes in this
version of the series. :)
submodule: pass on http.extraheader config settings
IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
surprised at first that your test worked at all, but that is because it
is using "clone", which is the one code path that works).
But I think we are waiting on going one of two paths:
1. drop sanitizing entirely
2. fix sanitizing and add more variables to it
If we go the route of (2), then we'd want my fix topic and this patch.
And if not, then we don't need any of it (just a patch dropping the
filtering, which AFAIK nobody has written yet).
-Peff
From: Jeff King <hidden> Date: 2016-06-16 02:19:10
On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
quoted
submodule: pass on http.extraheader config settings
IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
surprised at first that your test worked at all, but that is because it
is using "clone", which is the one code path that works).
But I think we are waiting on going one of two paths:
1. drop sanitizing entirely
2. fix sanitizing and add more variables to it
If we go the route of (2), then we'd want my fix topic and this patch.
And if not, then we don't need any of it (just a patch dropping the
filtering, which AFAIK nobody has written yet).
Actually, I think this last bit is not quite true. If we want to go back
to "nothing gets passed to submodules", we can drop all of my patches,
but I don't think anybody wants to do that.
But if we want "everything gets passed to submodules", then we do need
something like my patch series, because every use of local_repo_env
needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
don't think we want to simply drop that variable from local_repo_env
(which would also mean that it would be propagated to a local
git-upload-pack, for example, along with any third-party scripts that
use rev-parse --local-env-vars).
So I think we'd actually want my series as a preliminary fix, followed
by dropping the whitelist entirely on top of that, and then probably
simplifying the shell sanitize_submodule_env() on top of that (it would
be correct without the whitelist, but you can also trivially implement
it without having to call submodule--helper at all).
-Peff
From: Jeff King <hidden> Date: 2016-06-16 02:19:10
[+cc Stefan and Jacob since this is really resuming that earlier thread]
On Wed, May 04, 2016 at 03:45:59AM -0400, Jeff King wrote:
On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
quoted
quoted
submodule: pass on http.extraheader config settings
IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
surprised at first that your test worked at all, but that is because it
is using "clone", which is the one code path that works).
But I think we are waiting on going one of two paths:
1. drop sanitizing entirely
2. fix sanitizing and add more variables to it
If we go the route of (2), then we'd want my fix topic and this patch.
And if not, then we don't need any of it (just a patch dropping the
filtering, which AFAIK nobody has written yet).
Actually, I think this last bit is not quite true. If we want to go back
to "nothing gets passed to submodules", we can drop all of my patches,
but I don't think anybody wants to do that.
But if we want "everything gets passed to submodules", then we do need
something like my patch series, because every use of local_repo_env
needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
don't think we want to simply drop that variable from local_repo_env
(which would also mean that it would be propagated to a local
git-upload-pack, for example, along with any third-party scripts that
use rev-parse --local-env-vars).
So I think we'd actually want my series as a preliminary fix, followed
by dropping the whitelist entirely on top of that, and then probably
simplifying the shell sanitize_submodule_env() on top of that (it would
be correct without the whitelist, but you can also trivially implement
it without having to call submodule--helper at all).
I think we'd actually do it all in one, and that patch looks something
like the one below (on top of jk/submodule-config-sanitize-fix).
I don't feel that strongly about going either direction with this, but I
figure it doesn't hurt to make the patch so we know what the actual
option looks like.
-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options
The point of having a whitelist of command-line config
options to pass to submodules was two-fold:
1. It prevented obvious nonsense like using core.worktree
for multiple repos.
2. It could prevent surprise when the user did not mean
for the options to leak to the submodules (e.g.,
http.sslverify=false).
For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).
So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.
Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).
We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).
Signed-off-by: Jeff King <redacted>
---
builtin/submodule--helper.c | 17 -----------------
git-submodule.sh | 4 ++--
submodule.c | 40 +---------------------------------------
t/t7412-submodule--helper.sh | 26 --------------------------
4 files changed, 3 insertions(+), 84 deletions(-)
delete mode 100755 t/t7412-submodule--helper.sh
@@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)return0;}-staticintmodule_sanitize_config(intargc,constchar**argv,constchar*prefix)-{-structstrbufsanitized_config=STRBUF_INIT;--if(argc>1)-usage(_("git submodule--helper sanitize-config"));--git_config_from_parameters(sanitize_submodule_config,&sanitized_config);-if(sanitized_config.len)-printf("%s\n",sanitized_config.buf);--strbuf_release(&sanitized_config);--return0;-}-structsubmodule_update_clone{/* index into 'list', the list of submodules to look into for cloning */intcurrent;
@@ -197,9 +197,9 @@ isnumber()# of the settings from GIT_CONFIG_PARAMETERS. sanitize_submodule_env(){-sanitized_config=$(gitsubmodule--helpersanitize-config)+save_config=$GIT_CONFIG_PARAMETERSclear_local_git_env-GIT_CONFIG_PARAMETERS=$sanitized_config+GIT_CONFIG_PARAMETERS=$save_configexportGIT_CONFIG_PARAMETERS}
From: Stefan Beller <hidden> Date: 2016-06-16 02:19:10
On Wed, May 4, 2016 at 1:00 AM, Jeff King [off-list ref] wrote:
[+cc Stefan and Jacob since this is really resuming that earlier thread]
On Wed, May 04, 2016 at 03:45:59AM -0400, Jeff King wrote:
quoted
On Wed, May 04, 2016 at 02:26:18AM -0400, Jeff King wrote:
quoted
quoted
submodule: pass on http.extraheader config settings
IMHO this should come on top of jk/submodule-config-sanitize-fix (I was
surprised at first that your test worked at all, but that is because it
is using "clone", which is the one code path that works).
But I think we are waiting on going one of two paths:
1. drop sanitizing entirely
2. fix sanitizing and add more variables to it
If we go the route of (2), then we'd want my fix topic and this patch.
And if not, then we don't need any of it (just a patch dropping the
filtering, which AFAIK nobody has written yet).
Actually, I think this last bit is not quite true. If we want to go back
to "nothing gets passed to submodules", we can drop all of my patches,
but I don't think anybody wants to do that.
But if we want "everything gets passed to submodules", then we do need
something like my patch series, because every use of local_repo_env
needs to be come "local_repo_env excluding GIT_CONFIG_PARAMETERS". I
don't think we want to simply drop that variable from local_repo_env
(which would also mean that it would be propagated to a local
git-upload-pack, for example, along with any third-party scripts that
use rev-parse --local-env-vars).
So I think we'd actually want my series as a preliminary fix, followed
by dropping the whitelist entirely on top of that, and then probably
simplifying the shell sanitize_submodule_env() on top of that (it would
be correct without the whitelist, but you can also trivially implement
it without having to call submodule--helper at all).
I think we'd actually do it all in one, and that patch looks something
like the one below (on top of jk/submodule-config-sanitize-fix).
I don't feel that strongly about going either direction with this, but I
figure it doesn't hurt to make the patch so we know what the actual
option looks like.
-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options
The point of having a whitelist of command-line config
options to pass to submodules was two-fold:
1. It prevented obvious nonsense like using core.worktree
for multiple repos.
2. It could prevent surprise when the user did not mean
for the options to leak to the submodules (e.g.,
http.sslverify=false).
For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).
So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.
Just to recap:
Before jk/submodule-config-sanitize-fix (jk/submodule-c-credential actually)
we passed nothing down to the commands operating on submodules.
Then we decided to pass on some of it based on a curated list.
Curating the list is too hard, so we pass on everything now, because
it is easy to maintain and easy to explain. And when the user is hurt,
they're holding it wrong?
Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).
So when running `git -c foo=bar command --recurse-submodules`
the `-c` parsing calls git_config_push_parameter, which
exports that string `foo=baz` into the environment variable
GIT_CONFIG_PARAMETERS.
When the submodule command is called, sanitize_submodule_env
just wipes all the Git related configurations except those in
GIT_CONFIG_PARAMETERS as they are set again after
clear_local_git_env wiped it.
I wonder about the implementation detail, if we rather want to introduce
a `git rev-parse --repo-only-local-env-vars` which is
`git rev-parse --local-env-vars` without the GIT_CONFIG_PARAMETERS.
such that clear_local_git_env does the right thing and we don't
have to have 2 functions for it (i.e. clear_local_git_env and
sanitize_submodule_env, which is the newer not as strict version of it)
But as Jeff once put it, rev-parse is already a messy kitchensink,
so adding another option there may also not the right way?
We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).
Yeah that -- program may change any time in no backwards
compatible way.
Do we want to add documentation for the new behavior though?
Before: pass not -c arguments to submodules
Now: Pass all the -c arguments to submodules
@@ -246,22 +246,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)return0;}-staticintmodule_sanitize_config(intargc,constchar**argv,constchar*prefix)-{-structstrbufsanitized_config=STRBUF_INIT;--if(argc>1)-usage(_("git submodule--helper sanitize-config"));--git_config_from_parameters(sanitize_submodule_config,&sanitized_config);-if(sanitized_config.len)-printf("%s\n",sanitized_config.buf);--strbuf_release(&sanitized_config);--return0;-}-structsubmodule_update_clone{/* index into 'list', the list of submodules to look into for cloning */intcurrent;
@@ -197,9 +197,9 @@ isnumber()# of the settings from GIT_CONFIG_PARAMETERS. sanitize_submodule_env(){-sanitized_config=$(gitsubmodule--helpersanitize-config)+save_config=$GIT_CONFIG_PARAMETERSclear_local_git_env-GIT_CONFIG_PARAMETERS=$sanitized_config+GIT_CONFIG_PARAMETERS=$save_configexportGIT_CONFIG_PARAMETERS}
From: Jeff King <hidden> Date: 2016-06-16 02:19:10
On Wed, May 04, 2016 at 10:58:26AM -0700, Stefan Beller wrote:
quoted
So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.
Just to recap:
Before jk/submodule-config-sanitize-fix (jk/submodule-c-credential actually)
we passed nothing down to the commands operating on submodules.
Then we decided to pass on some of it based on a curated list.
Curating the list is too hard, so we pass on everything now, because
it is easy to maintain and easy to explain. And when the user is hurt,
they're holding it wrong?
Yes, I think that sums it up (the last paragraph is what's under
discussion, so it's up for debate).
quoted
Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).
So when running `git -c foo=bar command --recurse-submodules`
the `-c` parsing calls git_config_push_parameter, which
exports that string `foo=baz` into the environment variable
GIT_CONFIG_PARAMETERS.
When the submodule command is called, sanitize_submodule_env
just wipes all the Git related configurations except those in
GIT_CONFIG_PARAMETERS as they are set again after
clear_local_git_env wiped it.
Right.
I wonder about the implementation detail, if we rather want to introduce
a `git rev-parse --repo-only-local-env-vars` which is
`git rev-parse --local-env-vars` without the GIT_CONFIG_PARAMETERS.
such that clear_local_git_env does the right thing and we don't
have to have 2 functions for it (i.e. clear_local_git_env and
sanitize_submodule_env, which is the newer not as strict version of it)
I don't think that really buys you much. The policy is technically
duplicated in prepare_submodule_repo_env() in submodule.c and
sanitize_submodule_env in submodule.h. But without the whitelist, it's
such a simple policy that I'm not sure it's worth the boilerplate of
having the shell function call into the C code.
If we did want to go that route, the more appropriate interface would
probably be:
git submodule--helper sanitize-env
or something. That avoids making it a public interface, and makes it
clear that we are invoking the submodule rules.
Do we want to add documentation for the new behavior though?
Before: pass not -c arguments to submodules
Now: Pass all the -c arguments to submodules
I don't think there was any documentation for the _old_ behavior, and
certainly jk/submodule-c-credential didn't add any. But it probably is
worth document, maybe as part of "-c"? Care to roll a patch on top?
-Peff
From: Stefan Beller <hidden> Date: 2016-06-16 02:19:11
I don't think there was any documentation for the _old_ behavior, and
certainly jk/submodule-c-credential didn't add any. But it probably is
worth document, maybe as part of "-c"? Care to roll a patch on top?
Sure.
I think we'd actually do it all in one, and that patch looks something
like the one below (on top of jk/submodule-config-sanitize-fix).
$ git checkout origin/jk/submodule-config-sanitize-fix
$ git am p
Applying: submodule: stop sanitizing config options
error: patch failed: builtin/submodule--helper.c:246
error: builtin/submodule--helper.c: patch does not apply
error: patch failed: submodule.c:1131
error: submodule.c: patch does not apply
Patch failed at 0001 submodule: stop sanitizing config options
So if you want some documentation on top of that, where would I base it on?
From: Jeff King <hidden> Date: 2016-06-16 02:19:11
On Wed, May 04, 2016 at 03:53:26PM -0700, Stefan Beller wrote:
quoted
I think we'd actually do it all in one, and that patch looks something
like the one below (on top of jk/submodule-config-sanitize-fix).
$ git checkout origin/jk/submodule-config-sanitize-fix
$ git am p
Applying: submodule: stop sanitizing config options
error: patch failed: builtin/submodule--helper.c:246
error: builtin/submodule--helper.c: patch does not apply
error: patch failed: submodule.c:1131
error: submodule.c: patch does not apply
Patch failed at 0001 submodule: stop sanitizing config options
So if you want some documentation on top of that, where would I base it on?
I build the patches for jk/submodule-config-sanitize-fix on top of
master as of the other day, and then built this most recent patch on top
of that.
Looks like Junio applied them directly on the tip of
jk/submodule-c-credential, and had to wiggle the code in submodule.c,
which conflicted with the parallel-process stuff that was merged in
between. Since the new patch updates that code, it will likewise run
into conflicts.
I don't think there's a strict right answer here; if the original buggy
submodule-c-credential code had been released, we would definitely want
to build off of it for "maint" releases. But it wasn't, so master is
"just as good" in a sense. But I think Junio makes it a habit to apply
fixes as far back as the introduced bug, even when it's not going to
maint.
So since that's what published, it makes sense to build on that. Here's
a version of my patch that should apply for you (no semantic changes,
just differences in the surrounding context):
-- >8 --
Subject: [PATCH] submodule: stop sanitizing config options
The point of having a whitelist of command-line config
options to pass to submodules was two-fold:
1. It prevented obvious nonsense like using core.worktree
for multiple repos.
2. It could prevent surprise when the user did not mean
for the options to leak to the submodules (e.g.,
http.sslverify=false).
For case 1, the answer is mostly "if it hurts, don't do
that". For case 2, we can note that any such example has a
matching inverted surprise (e.g., a user who meant
http.sslverify=true to apply everywhere, but it didn't).
So this whitelist is probably not giving us any benefit, and
is already creating a hassle as people propose things to put
on it. Let's just drop it entirely.
Note that we still need to keep a special code path for
"prepare the submodule environment", because we still have
to take care to pass through $GIT_CONFIG_PARAMETERS (and
block the rest of the repo-specific environment variables).
We can do this easily from within the submodule shell
script, which lets us drop the submodule--helper option
entirely (and it's OK to do so because as a "--" program, it
is entirely a private implementation detail).
Signed-off-by: Jeff King <redacted>
---
builtin/submodule--helper.c | 17 -----------------
git-submodule.sh | 4 ++--
submodule.c | 39 +--------------------------------------
t/t7412-submodule--helper.sh | 26 --------------------------
4 files changed, 3 insertions(+), 83 deletions(-)
delete mode 100755 t/t7412-submodule--helper.sh
@@ -197,9 +197,9 @@ isnumber()# of the settings from GIT_CONFIG_PARAMETERS. sanitize_submodule_env(){-sanitized_config=$(gitsubmodule--helpersanitize-config)+save_config=$GIT_CONFIG_PARAMETERSclear_local_git_env-GIT_CONFIG_PARAMETERS=$sanitized_config+GIT_CONFIG_PARAMETERS=$save_configexportGIT_CONFIG_PARAMETERS}
From: Lars Schneider <hidden> Date: 2016-06-16 02:19:12
On 04 May 2016, at 08:14, Johannes Schindelin [off-list ref] wrote:
quoted hunk
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 <redacted>
---
Documentation/config.txt | 6 ++++++
http-push.c | 10 +++++-----
http.c | 35 ++++++++++++++++++++++++++++++++---
http.h | 1 +
remote-curl.c | 4 ++--
t/lib-httpd/apache.conf | 8 ++++++++
t/t5551-http-fetch-smart.sh | 7 +++++++
7 files changed, 61 insertions(+), 10 deletions(-)
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
I think "<RequireAll>" depends on mod_authz_core which is only
available in Apache HTTPD 2.3 or later [1].
Right now the test only checks if Apache version greater 2
is installed. Should we guard this test with a special version
check? Or do you see a way to check the magic values without
"<RequireAll>"?
I only noticed this because I enabled these tests on Travis-CI
and the Travis Linux box comes with Apache 2.2.22 installed...
- Lars
[1] https://httpd.apache.org/docs/trunk/mod/mod_authz_core.html
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
--
2.8.1.306.gff998f2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
I think "<RequireAll>" depends on mod_authz_core which is only
available in Apache HTTPD 2.3 or later [1].
Right now the test only checks if Apache version greater 2
is installed. Should we guard this test with a special version
check? Or do you see a way to check the magic values without
"<RequireAll>"?
I think you can get rid of RequireAll with:
Require expr %{HTTP:x-magic-one} == 'abra' && %{HTTP:x-magic-two} == 'cadabra'
But I am also not sure that "expr" existed in Apache 2.2.
I think the older way of checking headers was to do some trickery with
RewriteCond; I tried briefly to make that work when I wrote the test,
but never did (but I'm far from an expert in Apache; the only reason I
have touched it in the last 15 years is for Git's test suite).
The nuclear option would be to put a shell script between Apache and
git-http-backend that checks for those headers (I suspect we'd still
need some magic in the Apache config to pass the headers out in the
environment, though).
-Peff
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:15
To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.
This patch allows us to configure these extra HTTP headers for use with
`git submodule update`, too. It requires somewhat special handling:
submodules do not share the parent project's config. It would be
incorrect to simply reuse that specific part of the parent's config.
Instead, the config option needs to be specified on the command-line or
in ~/.gitconfig or friends.
Example: git -c http.extraheader="Secret: Sauce" submodule update --init
Signed-off-by: Johannes Schindelin <redacted>
---
builtin/submodule--helper.c | 3 ++-
t/t5551-http-fetch-smart.sh | 11 ++++++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:15
My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.
The first part already made it into `master`, this is the remainder.
This iteration still has the specific patch to make `git -c
http.extraHeader=... submodule update` work; I plan to keep only the
test (and adjust the commit message) as soon as Peff's patch is applied
that skips -c ... sanitizing for submodules.
Johannes Schindelin (3):
tests: Adjust the configuration for Apache 2.2
t5551: make the test for extra HTTP headers more robust
submodule: pass on http.extraheader config settings
builtin/submodule--helper.c | 3 ++-
t/lib-httpd/apache.conf | 12 ++++++++----
t/t5551-http-fetch-smart.sh | 14 ++++++++++++--
3 files changed, 22 insertions(+), 7 deletions(-)
Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v7
Interdiff vs v6:
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index b8ed96f..29b34bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/
Header set Set-Cookie name=value
</LocationMatch>
<LocationMatch /smart_headers/>
- <RequireAll>
- Require expr %{HTTP:x-magic-one} == 'abra'
- Require expr %{HTTP:x-magic-two} == 'cadabra'
- </RequireAll>
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
</LocationMatch>
@@ -136,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
+# And as RewriteCond unfortunately lacks "not equal" matching, we use this
+# ugly trick to fail *unless* the two headers are present.
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
<IfDefine SSL>
LoadModule ssl_module modules/mod_ssl.so
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1794168..2f375eb 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -283,7 +283,8 @@ test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
'
test_expect_success 'custom http headers' '
- test_must_fail git fetch "$HTTPD_URL/smart_headers/repo.git" &&
+ test_must_fail git -c http.extraheader="x-magic-two: cadabra" \
+ 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" &&
--
2.8.2.463.g99156ee
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:15
Lars Schneider noticed that the configuration introduced to test the extra
HTTP headers cannot be used with Apache 2.2 (which is still actively
maintained, as pointed out by Junio Hamano).
To let the tests pass with Apache 2.2 again, let's substitute the
offending <RequireAll> and `expr` by using old school RewriteCond
statements.
Signed-off-by: Johannes Schindelin <redacted>
---
t/lib-httpd/apache.conf | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
@@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/HeadersetSet-Cookiename=value</LocationMatch><LocationMatch/smart_headers/>-<RequireAll>-Requireexpr%{HTTP:x-magic-one}=='abra'-Requireexpr%{HTTP:x-magic-two}=='cadabra'-</RequireAll>SetEnvGIT_EXEC_PATH${GIT_EXEC_PATH}SetEnvGIT_HTTP_EXPORT_ALL</LocationMatch>
@@ -136,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]RewriteRule^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*)/$1[R=302]RewriteRule^/loop-redir/(.*)$/loop-redir/x-$1[R=302]+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.+# And as RewriteCond unfortunately lacks "not equal" matching, we use this+# ugly trick to fail *unless* the two headers are present.+RewriteCond%{HTTP:x-magic-one}=abra+RewriteCond%{HTTP:x-magic-two}=cadabra+RewriteRule^/smart_headers/.*-[L]+RewriteRule^/smart_headers/.*-[F]+<IfDefineSSL>LoadModulessl_modulemodules/mod_ssl.so
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:15
To test that extra HTTP headers are passed correctly, t5551 verifies that
a fetch succeeds when two required headers are passed, and that the fetch
does not succeed when those headers are not passed.
However, this test would also succeed if the configuration required only
one header. As Apache's configuration is notoriously tricky (this
developer frequently requires StackOverflow's help to understand Apache's
documentation), especially when still supporting the 2.2 line, let's just
really make sure that the test verifies what we want it to verify.
Signed-off-by: Johannes Schindelin <redacted>
---
t/t5551-http-fetch-smart.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Lars Schneider <hidden> Date: 2016-06-16 02:19:15
On 09 May 2016, at 08:19, Johannes Schindelin [off-list ref] wrote:
To test that extra HTTP headers are passed correctly, t5551 verifies that
a fetch succeeds when two required headers are passed, and that the fetch
does not succeed when those headers are not passed.
However, this test would also succeed if the configuration required only
one header. As Apache's configuration is notoriously tricky (this
developer frequently requires StackOverflow's help to understand Apache's
documentation), especially when still supporting the 2.2 line, let's just
really make sure that the test verifies what we want it to verify.
From: Jeff King <hidden> Date: 2016-06-16 02:19:15
On Mon, May 09, 2016 at 08:18:52AM +0200, Johannes Schindelin wrote:
+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
+# And as RewriteCond unfortunately lacks "not equal" matching, we use this
+# ugly trick to fail *unless* the two headers are present.
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
Thanks, this is the magic that eluded me earlier. I had to look up the
flags, so for any observers in the same boat, this works because:
- the '[L]' flag says "stop doing any more rewrite rules"; it triggers
only when the RewriteConds above match
- the '[F]' flag says "return 403 Forbidden"; it triggers always,
because after a RewriteRule, all RewriteConds are reset
I'm sure that is all apparent to somebody who is familiar with Apache
config, but I think that does not include most people on this project. I
dunno if it is worth a comment here or in the commit message.
-Peff
From: Jeff King <hidden> Date: 2016-06-16 02:19:15
On Mon, May 09, 2016 at 08:19:00AM +0200, Johannes Schindelin wrote:
To test that extra HTTP headers are passed correctly, t5551 verifies that
a fetch succeeds when two required headers are passed, and that the fetch
does not succeed when those headers are not passed.
However, this test would also succeed if the configuration required only
one header. As Apache's configuration is notoriously tricky (this
developer frequently requires StackOverflow's help to understand Apache's
documentation), especially when still supporting the 2.2 line, let's just
really make sure that the test verifies what we want it to verify.
This loses the 0-header check, but I don't think that is particularly
interesting to us (I had originally wanted to double-check that our
apache config worked at all in the absence of this feature, but I think
it is OK for the 1-header case to cover this; if our code is so buggy we
accidentally send 0 headers in the first command, we'll catch that,
too).
So looks good to me.
-Peff
This loses the 0-header check, but I don't think that is particularly
interesting to us (I had originally wanted to double-check that our
apache config worked at all in the absence of this feature, but I think
it is OK for the 1-header case to cover this; if our code is so buggy we
accidentally send 0 headers in the first command, we'll catch that,
too).
Yeah, a faulty Apache config will unfortunately *skip* the entire test, as
httpd refuses to start.
And I also considered testing 0-header and 1st header only. But as you
know, Git's test suite takes already 3.5h in a moderately sized Windows
VM, so I am really reluctant to add overly extensive tests.
From: Jeff King <hidden> Date: 2016-06-16 02:19:15
On Mon, May 09, 2016 at 10:13:51AM +0200, Johannes Schindelin wrote:
do you sleep at all?
Actually, I just woke up. Nothing like some Git ML to get the blood
pumping in the morning.
Yeah, a faulty Apache config will unfortunately *skip* the entire test, as
httpd refuses to start.
If you care about this, you can set GIT_TEST_HTTPD=true. The default is
"auto", which will skip whenever apache setup fails. But with "true", it
will show a hard error (so at least your "make test" will report the
failure).
-Peff
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:15
Hi Peff,
On Mon, 9 May 2016, Jeff King wrote:
On Mon, May 09, 2016 at 08:18:52AM +0200, Johannes Schindelin wrote:
quoted
+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
+# And as RewriteCond unfortunately lacks "not equal" matching, we use this
+# ugly trick to fail *unless* the two headers are present.
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
Thanks, this is the magic that eluded me earlier. I had to look up the
flags, so for any observers in the same boat, this works because:
- the '[L]' flag says "stop doing any more rewrite rules"; it triggers
only when the RewriteConds above match
- the '[F]' flag says "return 403 Forbidden"; it triggers always,
because after a RewriteRule, all RewriteConds are reset
I'm sure that is all apparent to somebody who is familiar with Apache
config, but I think that does not include most people on this project. I
dunno if it is worth a comment here or in the commit message.
Oh, you're absolutely correct, I should have described this better. It
took me quite a couple of iterations to get it right, after all.
How about this:
As RewriteCond does not allow testing for *non*-matches, we simply
match the desired case first and let it pass by marking the
RewriteRule as '[L]' ("last rule, do not process any other
matching RewriteRules after this"), and then have another
RewriteRule that matches all other cases and lets them fail via
'[F]' ("fail").
Good enough?
Ciao,
Dscho
From: Jeff King <hidden> Date: 2016-06-16 02:19:15
On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:
How about this:
As RewriteCond does not allow testing for *non*-matches, we simply
match the desired case first and let it pass by marking the
RewriteRule as '[L]' ("last rule, do not process any other
matching RewriteRules after this"), and then have another
RewriteRule that matches all other cases and lets them fail via
'[F]' ("fail").
Good enough?
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:15
Hi Peff,
On Mon, 9 May 2016, Jeff King wrote:
On Mon, May 09, 2016 at 04:03:48PM +0200, Johannes Schindelin wrote:
quoted
How about this:
As RewriteCond does not allow testing for *non*-matches, we simply
match the desired case first and let it pass by marking the
RewriteRule as '[L]' ("last rule, do not process any other
matching RewriteRules after this"), and then have another
RewriteRule that matches all other cases and lets them fail via
'[F]' ("fail").
Good enough?
Yep, I think that explains it. Thanks.
Okay, I already force-pushed my extra-http-header branch and the next
iteration will sport this paragraph.
Hopefully your patch to remove the -c ... sanitizing makes it to `master`
soon, then I can submit my next iteration.
Ciao,
Dscho
From: Lars Schneider <hidden> Date: 2016-06-16 02:19:17
On 09 May 2016, at 08:18, Johannes Schindelin [off-list ref] wrote:
Lars Schneider noticed that the configuration introduced to test the extra
HTTP headers cannot be used with Apache 2.2 (which is still actively
maintained, as pointed out by Junio Hamano).
To let the tests pass with Apache 2.2 again, let's substitute the
offending <RequireAll> and `expr` by using old school RewriteCond
statements.
RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
+# And as RewriteCond unfortunately lacks "not equal" matching, we use this
+# ugly trick to fail *unless* the two headers are present.
+RewriteCond %{HTTP:x-magic-one} =abra
+RewriteCond %{HTTP:x-magic-two} =cadabra
+RewriteRule ^/smart_headers/.* - [L]
+RewriteRule ^/smart_headers/.* - [F]
+
<IfDefine SSL>
LoadModule ssl_module modules/mod_ssl.so
--
2.8.2.463.g99156ee
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:17
Lars Schneider noticed that the configuration introduced to test the
extra HTTP headers cannot be used with Apache 2.2 (which is still
actively maintained, as pointed out by Junio Hamano).
To let the tests pass with Apache 2.2 again, let's substitute the
offending <RequireAll> and `expr` by using old school RewriteCond
statements.
As RewriteCond does not allow testing for *non*-matches, we simply match
the desired case first and let it pass by marking the RewriteRule as
'[L]' ("last rule, do not process any other matching RewriteRules after
this"), and then have another RewriteRule that matches all other cases
and lets them fail via '[F]' ("fail").
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
t/lib-httpd/apache.conf | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
@@ -103,10 +103,6 @@ Alias /auth/dumb/ www/auth/dumb/HeadersetSet-Cookiename=value</LocationMatch><LocationMatch/smart_headers/>-<RequireAll>-Requireexpr%{HTTP:x-magic-one}=='abra'-Requireexpr%{HTTP:x-magic-two}=='cadabra'-</RequireAll>SetEnvGIT_EXEC_PATH${GIT_EXEC_PATH}SetEnvGIT_HTTP_EXPORT_ALL</LocationMatch>
@@ -136,6 +132,18 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]RewriteRule^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*)/$1[R=302]RewriteRule^/loop-redir/(.*)$/loop-redir/x-$1[R=302]+# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.+# And as RewriteCond does not allow testing for non-matches, we match+# the desired case first (one has abra, two has cadabra), and let it+# pass by marking the RewriteRule as [L], "last rule, do not process+# any other matching RewriteRules after this"), and then have another+# RewriteRule that matches all other cases and lets them fail via '[F]',+# "fail the request".+RewriteCond%{HTTP:x-magic-one}=abra+RewriteCond%{HTTP:x-magic-two}=cadabra+RewriteRule^/smart_headers/.*-[L]+RewriteRule^/smart_headers/.*-[F]+<IfDefineSSL>LoadModulessl_modulemodules/mod_ssl.so
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:17
My use case is an army of build agents that need only limited and
selective access to otherwise private repositories.
The first part already made it into `master`, this is the remainder.
This iteration is based on 'jk/submodule-c-credential' and therefore
converted the original config-sanitizing patch into a test-only patch.
This iteration also replaces the "ugly" comment with the explanation
preferred by Junio.
Johannes Schindelin (3):
tests: adjust the configuration for Apache 2.2
t5551: make the test for extra HTTP headers more robust
submodule: ensure that -c http.extraheader is heeded
t/lib-httpd/apache.conf | 16 ++++++++++++----
t/t5551-http-fetch-smart.sh | 14 ++++++++++++--
2 files changed, 24 insertions(+), 6 deletions(-)
Published-As: https://github.com/dscho/git/releases/tag/extra-http-headers-v8
--
Interdiff vs v7:
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
[no longer applies; skipped]
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 29b34bb..018a83a 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -133,8 +133,12 @@ RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
-# And as RewriteCond unfortunately lacks "not equal" matching, we use this
-# ugly trick to fail *unless* the two headers are present.
+# And as RewriteCond does not allow testing for non-matches, we match
+# the desired case first (one has abra, two has cadabra), and let it
+# pass by marking the RewriteRule as [L], "last rule, do not process
+# any other matching RewriteRules after this"), and then have another
+# RewriteRule that matches all other cases and lets them fail via '[F]',
+# "fail the request".
RewriteCond %{HTTP:x-magic-one} =abra
RewriteCond %{HTTP:x-magic-two} =cadabra
RewriteRule ^/smart_headers/.* - [L]
2.8.2.463.g99156ee
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:17
To test that extra HTTP headers are passed correctly, t5551 verifies that
a fetch succeeds when two required headers are passed, and that the fetch
does not succeed when those headers are not passed.
However, this test would also succeed if the configuration required only
one header. As Apache's configuration is notoriously tricky (this
developer frequently requires StackOverflow's help to understand Apache's
documentation), especially when still supporting the 2.2 line, let's just
really make sure that the test verifies what we want it to verify.
Signed-off-by: Johannes Schindelin <redacted>
---
t/t5551-http-fetch-smart.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Johannes Schindelin <hidden> Date: 2016-06-16 02:19:17
To support this developer's use case of allowing build agents token-based
access to private repositories, we introduced the http.extraheader
feature, allowing extra HTTP headers to be sent along with every HTTP
request.
This patch verifies that we can configure these extra HTTP headers via the
command-line for use with `git submodule update`, too. Example: git -c
http.extraheader="Secret: Sauce" submodule update --init
Signed-off-by: Johannes Schindelin <redacted>
---
t/t5551-http-fetch-smart.sh | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
On 10.05.16 09:08, Johannes Schindelin wrote:
- I'm not sure, if this is the right thread to report on -
It seems as if t5551 is hanging ?
This is the last line from the log:
ok 25 - large fetch-pack requests can be split across POSTs
I have 7 such processes running:
/trash directory.t5551-http-fetch-smart/httpd -f
/Users/tb/projects/git/git.pu/t/lib-httpd/apache.conf -DDarwin -c Listen
127.0.0.1:5551 -k start
This happens both under Mac OS X and Debian.
Does anybody have the same hanging ?
From: Jeff King <hidden> Date: 2016-06-16 02:19:20
On Wed, May 11, 2016 at 07:13:56PM +0200, Torsten Bögershausen wrote:
On 10.05.16 09:08, Johannes Schindelin wrote:
- I'm not sure, if this is the right thread to report on -
It seems as if t5551 is hanging ?
This is the last line from the log:
ok 25 - large fetch-pack requests can be split across POSTs
Are you running the tests with "--long" or GIT_TEST_LONG in the
environment? The next line should show it skipping test 26 unless one of
those is set.
If you are, can you confirm that it's actually hanging, and not just
slow? On my system, test 26 takes about a minute to run (which is why we
don't do it by default).
I have 7 such processes running:
/trash directory.t5551-http-fetch-smart/httpd -f
/Users/tb/projects/git/git.pu/t/lib-httpd/apache.conf -DDarwin -c Listen
127.0.0.1:5551 -k start
That's normal while the test is running; apache pre-forks a bunch of
worker threads.
-Peff
On Wed, May 11, 2016 at 07:13:56PM +0200, Torsten Bögershausen wrote:
quoted
On 10.05.16 09:08, Johannes Schindelin wrote:
- I'm not sure, if this is the right thread to report on -
It seems as if t5551 is hanging ?
This is the last line from the log:
ok 25 - large fetch-pack requests can be split across POSTs
Are you running the tests with "--long" or GIT_TEST_LONG in the
environment? The next line should show it skipping test 26 unless one of
those is set.
Yes
If you are, can you confirm that it's actually hanging, and not just
slow? On my system, test 26 takes about a minute to run (which is why we
don't do it by default).
Nearly sure. After 10 minutes, the test was still running.
Yesterday another machine was running even longer.
Any tips, how to debug, are welcome.
From: Jeff King <hidden> Date: 2016-06-16 02:19:20
On Wed, May 11, 2016 at 10:03:45PM +0200, Torsten Bögershausen wrote:
quoted
If you are, can you confirm that it's actually hanging, and not just
slow? On my system, test 26 takes about a minute to run (which is why we
don't do it by default).
Nearly sure. After 10 minutes, the test was still running.
Yesterday another machine was running even longer.
Any tips, how to debug, are welcome.
Try running with "-x" to see what the test is doing. It will probably be
in:
+ git -C too-many-refs fetch -q --tags
after a while. Check "ps" to see if you have a fetch-pack sub-process
running. It should be writing "have" lines and reading lots of ACK
lines, which you can check via strace.
If it's blocked on read() or write(), then it's probably some kind of
I/O deadlock.
-Peff
On Wed, May 11, 2016 at 10:03:45PM +0200, Torsten Bögershausen wrote:
quoted
quoted
If you are, can you confirm that it's actually hanging, and not just
slow? On my system, test 26 takes about a minute to run (which is why we
don't do it by default).
Nearly sure. After 10 minutes, the test was still running.
Yesterday another machine was running even longer.
Any tips, how to debug, are welcome.
Try running with "-x" to see what the test is doing. It will probably be
in:
+ git -C too-many-refs fetch -q --tags
after a while. Check "ps" to see if you have a fetch-pack sub-process
running. It should be writing "have" lines and reading lots of ACK
lines, which you can check via strace.
If it's blocked on read() or write(), then it's probably some kind of
I/O deadlock.
-Peff
This is the last log that I see:
---------------------------------------------------------------------
pack_report: getpagesize() = 4096
pack_report: core.packedGitWindowSize = 1073741824
pack_report: core.packedGitLimit = 8589934592
pack_report: pack_used_ctr = 96001
pack_report: pack_mmap_calls = 48002
pack_report: pack_open_windows = 2 / 2
pack_report: pack_mapped = 6605494 / 6605494
---------------------------------------------------------------------
+++ perl -e 'print "bla" x 30'+++ command /usr/bin/perl -e 'print "bla" x 30'+++ /usr/bin/perl -e 'print "bla" x 30'
++ tag=blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla
++ sed -e 's|^:\([^ ]*\) \(.*\)$|\2 refs/tags/blablablablablablablablablablablablablablablablablablablablablablablablablablablablablabla-\1|'
++ git -C too-many-refs fetch -q --tags
And this may be the processes :
(Not sure, probaly need to reboot & clean ?)
/bin/sh ./t5551-http-fetch-smart.sh -x
73459 ttys010 0:21.45 /Users/tb/projects/git/git.pu/git -C too-many-refs fetch -q --tags
73460 ttys010 0:00.40 git-remote-http origin http://127.0.0.1:5551/smart/repo.git
ps | grep fetch
73540 ttys006 0:00.00 grep fetch
73025 ttys010 0:00.14 /bin/sh ./t5551-http-fetch-smart.sh -x
73459 ttys010 3:40.70 /Users/tb/projects/git/git.pu/git -C too-many-refs fetch -q --tags
Beside that, reverting the last 2 commits on 5551 doesn't seem to help:
Revert "t5551: make the test for extra HTTP headers more robust"
Revert "submodule: ensure that -c http.extraheader is heeded"
I'm surprised not to see fetch-pack in that list. And to see so much CPU
going to fetch itself. But perhaps you are simply at a different stage
in the test. 3:40 of CPU time is a lot (the whole thing runs in under a
minute on my machine).
Hmm. Switching to "pu" seems to make things slow on my machine, too, and
the time all goes to fetch. So perhaps there is some recent regression
there. It should be bisectable.
-Peff
From: Jeff King <hidden> Date: 2016-06-16 02:19:20
On Thu, May 12, 2016 at 02:40:39AM -0400, Jeff King wrote:
Hmm. Switching to "pu" seems to make things slow on my machine, too, and
the time all goes to fetch. So perhaps there is some recent regression
there. It should be bisectable.
It's 66d33af21bd1e398973414435af43d06f2e2099c. I don't think it's
hanging, but it is _really_ slow. I'll reply to that patch separately
with a report.
-Peff