From: Junio C Hamano <hidden> Date: 2016-06-15 23:03:56
Junio C Hamano [off-list ref] writes:
I do not think v1 can be fixed by "send one ref with capability,
newer client may respond immediately so we can stop enumerating
remaining refs and older one will get stuck so we can have a timeout
to see if the connection is from the newer one, and send the rest
for the older client", because anything that involves such a timeout
would not reliably work over WAN.
Just for fun, I was trying to see if there is a hole in the current
protocol that allows a new client to talk a valid v1 protocol
exchange with existing, deployed servers without breaking, while
letting it to know a new server that it is a new client and it does
not want to get blasted by megabytes of ref advertisement.
The idea is to find a request that can be sent as the first
utterance by the client to an old server that is interpreted as a
no-op and can be recognised by a new server as such a "no-op probe".
If there is such a request, then the exchange can go like this with
(new client, old server) pair:
- new client connects and sends that no-op.
- old server starts blasting the ref advertisement
- new client monitors and notices that the other side
started speaking, and the ref advertisement lacks the
capability bit for new protocol.
- new client accepts the ref advertisement and does the v1
protocol thing as a follow-up to what it already sent.
As long as the first one turns out to be no-op for old server, we
would be OK. On the other hand, (new client, new server) pair
would go like this:
- new client connects and sends that no-op.
- new server notices that there is already a data from the
client, and recognises the "no-op probe".
- new server gives the first v2 protocol message with
capability.
- new client notices thqat the other side started speaking, and
it is the first v2 protocol message.
- both sides happily speak v2.
and (old client, new server) pair would go like this:
- old client connects and waits.
- new server notices that there is *no* data sent from the
client and decides that the other side is a v1 client. It
starts blasting the ref advertisement.
- both sides happily speak v1 from here on.
A misdetected case between (new client, new server) pair might go
like this:
- new client connects and sends that no-op.
- new server accepts the connection, but that no-op probe has
not arrived yet.". It misdetects the other side as a v1
client and it starts blasting the ref advertisement.
- new client notices that the ref advertisement has the
capability bit and the server is capable of v2 protocol. it
waits until the server sends "sorry, I misdetected" message.
- new server eventually notices the "no-op probe" while blasting
the ref advertisement and it can stop in the middle.
hopefully this can happen after only sending a few kilobytes
among megabytes of ref advertisement data ;-). The server
sends "sorry, I misdetected" message to synchronise.
- both sides happily speak v2 from here on.
So the topic of this exercise ("just for fun") is to see if there is
such a no-op request the client side can send as the first thing for
probing.
On the fetch side, the first response upload-pack expects are one
of:
- "want " followed by an object name.
- "shallow " followed by an object name.
- "deepen " followed by a positive integer.
And there _is_ a hole ;-). The parsing of "shallow " object name is
done in such a way that an object name that passes get_sha1_hex()
that results in a NULL return from parse_object() is _ignored_. So
a new client can use "shallow 0{40}" as a no-op probe.
It appears that on the push side, there is a similar hole that can
be used. receive-pack expects either "shallow ", "push-cert" or the
refname updates (i.e. two "[0-9a-f]{40}" followed by a refname); the
parsing of "shallow " is not as loose as the fetch side in that
using a "shallow 0{40}" as a no-op probe will end up causing
prepare_shallow_info() sift the "0{40}" object name into "theirs",
but I think it will be ignored at the end as "unreachable cruft"
without causing harm.
I am _not_ proposing that we should go this route, at least not yet.
I am merely pointing out that an in-place sidegrade from v1 to a
protocol that avoids the megabyte-advertisement-at-the-beginning
seems to be possible, as a food for thought.
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:56
On Fri, Feb 27, 2015 at 3:05 PM, Junio C Hamano [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:
quoted
I do not think v1 can be fixed by "send one ref with capability,
newer client may respond immediately so we can stop enumerating
remaining refs and older one will get stuck so we can have a timeout
to see if the connection is from the newer one, and send the rest
for the older client", because anything that involves such a timeout
would not reliably work over WAN.
Just for fun, I was trying to see if there is a hole in the current
protocol that allows a new client to talk a valid v1 protocol
exchange with existing, deployed servers without breaking, while
letting it to know a new server that it is a new client and it does
not want to get blasted by megabytes of ref advertisement.
The idea is to find a request that can be sent as the first
utterance by the client to an old server that is interpreted as a
no-op and can be recognised by a new server as such a "no-op probe".
If there is such a request, then the exchange can go like this with
(new client, old server) pair:
- new client connects and sends that no-op.
- old server starts blasting the ref advertisement
- new client monitors and notices that the other side
started speaking, and the ref advertisement lacks the
capability bit for new protocol.
- new client accepts the ref advertisement and does the v1
protocol thing as a follow-up to what it already sent.
As long as the first one turns out to be no-op for old server, we
would be OK. On the other hand, (new client, new server) pair
would go like this:
- new client connects and sends that no-op.
- new server notices that there is already a data from the
client, and recognises the "no-op probe".
- new server gives the first v2 protocol message with
capability.
- new client notices thqat the other side started speaking, and
it is the first v2 protocol message.
- both sides happily speak v2.
and (old client, new server) pair would go like this:
- old client connects and waits.
- new server notices that there is *no* data sent from the
client and decides that the other side is a v1 client. It
starts blasting the ref advertisement.
- both sides happily speak v1 from here on.
A misdetected case between (new client, new server) pair might go
like this:
- new client connects and sends that no-op.
- new server accepts the connection, but that no-op probe has
not arrived yet.". It misdetects the other side as a v1
client and it starts blasting the ref advertisement.
- new client notices that the ref advertisement has the
capability bit and the server is capable of v2 protocol. it
waits until the server sends "sorry, I misdetected" message.
- new server eventually notices the "no-op probe" while blasting
the ref advertisement and it can stop in the middle.
hopefully this can happen after only sending a few kilobytes
among megabytes of ref advertisement data ;-). The server
sends "sorry, I misdetected" message to synchronise.
- both sides happily speak v2 from here on.
So the topic of this exercise ("just for fun") is to see if there is
such a no-op request the client side can send as the first thing for
probing.
On the fetch side, the first response upload-pack expects are one
of:
- "want " followed by an object name.
- "shallow " followed by an object name.
- "deepen " followed by a positive integer.
And there _is_ a hole ;-). The parsing of "shallow " object name is
done in such a way that an object name that passes get_sha1_hex()
that results in a NULL return from parse_object() is _ignored_. So
a new client can use "shallow 0{40}" as a no-op probe.
It appears that on the push side, there is a similar hole that can
be used. receive-pack expects either "shallow ", "push-cert" or the
refname updates (i.e. two "[0-9a-f]{40}" followed by a refname); the
parsing of "shallow " is not as loose as the fetch side in that
using a "shallow 0{40}" as a no-op probe will end up causing
prepare_shallow_info() sift the "0{40}" object name into "theirs",
but I think it will be ignored at the end as "unreachable cruft"
without causing harm.
I am _not_ proposing that we should go this route, at least not yet.
I am merely pointing out that an in-place sidegrade from v1 to a
protocol that avoids the megabyte-advertisement-at-the-beginning
seems to be possible, as a food for thought.
This is a fun thing indeed, though I'd personally feel uneasy with
such a probe as
a serious proposal. (Remember somebody 10 years from now wants to enjoy
reading the source code). So let's keep the idea around if we don't find another
solution.
As far as I can tell we have
* native git protocol (git daemon)
* ssh
* http(s)
* ftp (deprecated!)
* rsync(deprecated)
For both native git as well as ssh, Duy presented a solution at [1, 2]
a year ago,
which essentially presents the desired client capabilites 'out of
band' to the server
via an argument to the server. So we'd only need to examine the
http(s) path how to
pass in arguments there.
Since 5 years an additional argument in the git protocol would cause
no harm, so it's
pretty safe to extend it with caution. We'd advertise the
client-may-ask-for-capabilities-out-of-band
per server and the client would eventually learn about it and only use
this for servers
which advertised they can do so. And nobody sane out there will
downgrade a version
of today to a git of 5 years ago.
I did rebase Duys' patches and I am trying to send out a patch series
today going that route
asking for comments.
Thanks a lot for the digging!
Stefan
[1] https://github.com/pclouds/git/commit/e26fa77c4d9ace06b9f2c80091af9eb7b63a1c95
[2] https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50
On Sat, Feb 28, 2015 at 6:05 AM, Junio C Hamano [off-list ref] wrote:
Just for fun, I was trying to see if there is a hole in the current
protocol that allows a new client to talk a valid v1 protocol
exchange with existing, deployed servers without breaking, while
letting it to know a new server that it is a new client and it does
not want to get blasted by megabytes of ref advertisement.
...
The idea is to find a request that can be sent as the first
utterance by the client to an old server that is interpreted as a
no-op and can be recognised by a new server as such a "no-op probe".
...
And there _is_ a hole ;-). The parsing of "shallow " object name is
done in such a way that an object name that passes get_sha1_hex()
that results in a NULL return from parse_object() is _ignored_. So
a new client can use "shallow 0{40}" as a no-op probe.
...
I am _not_ proposing that we should go this route, at least not yet.
I am merely pointing out that an in-place sidegrade from v1 to a
protocol that avoids the megabyte-advertisement-at-the-beginning
seems to be possible, as a food for thought.
There may be another hole, if we send "want <empty-tree>", it looks
like it will go through without causing errors. It's not exactly no-op
because an empty tree object will be bundled in result pack. But that
makes no difference in pratice. I didn't verify this though.
In the spirit of fun, I looked at how jgit handles this shallow line
(because this is more like an implementation hole than protocol hole).
I don't think jgit would ignore 0{40} the way C Git does. This SHA-1
will end up in shallowCommits set in upload-pack, then will be parsed
as a commit. But even if the parsing is through, a non-empty
shallowCommits set would disable pack bitmap. Fun is usually short..
PS. heh my "want empty-tree" hole is probably impl-specific too. Not
sure if jgit also keeps empty tree available even if it does not
exist.
--
Duy
From: Junio C Hamano <hidden> Date: 2016-06-15 23:03:56
On Fri, Feb 27, 2015 at 4:07 PM, Duy Nguyen [off-list ref] wrote:
There may be another hole, if we send "want <empty-tree>", it looks
like it will go through without causing errors. It's not exactly no-op
because an empty tree object will be bundled in result pack. But that
makes no difference in pratice. I didn't verify this though.
In addition to "that's not a no-op" problem, unless the old server has a
ref that has an emtpy tree at its tip, such a fetch request will be rejected,
unless the server is configured to serve any object, no?
If your new server does have a ref that points at an empty tree, a client
may request you to send that, but this is not a problem, because the
new server can tell if the client is sending it as a no-op probe or a serious
request by looking at its capability request. A serious old client will not
tell you that he is new, a probing new client does, and a serious new
client does. So your new server can tell and will not be confused.
as a commit. But even if the parsing is through, a non-empty
shallowCommits set would disable pack bitmap.
Performance penalty is fine. Over time we would upgrade and the
point of the exercise is not to cause the old-new or new-old pair to
die but keep talking the old protocol and getting correct results.
From: Junio C Hamano <hidden> Date: 2016-06-15 23:03:56
On Fri, Feb 27, 2015 at 3:44 PM, Stefan Beller [off-list ref] wrote:
On Fri, Feb 27, 2015 at 3:05 PM, Junio C Hamano [off-list ref] wrote:
quoted
I am _not_ proposing that we should go this route, at least not yet.
I am merely pointing out that an in-place sidegrade from v1 to a
protocol that avoids the megabyte-advertisement-at-the-beginning
seems to be possible, as a food for thought.
This is a fun thing indeed, though I'd personally feel uneasy with
such a probe as
a serious proposal. (Remember somebody 10 years from now wants to enjoy
reading the source code).
That cannot be a serious objection, once you realize that NUL + capability
was exactly the same kind of "yes, we have a hole to allow up customize
the protocol". The code to do so may not be pretty, but the code to implement
ended up being reasonably clean with parse_feature_request() and friends.
After all we live in a real world ;-)
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:56
On Fri, Feb 27, 2015 at 4:33 PM, Junio C Hamano [off-list ref] wrote:
On Fri, Feb 27, 2015 at 3:44 PM, Stefan Beller [off-list ref] wrote:
quoted
On Fri, Feb 27, 2015 at 3:05 PM, Junio C Hamano [off-list ref] wrote:
quoted
I am _not_ proposing that we should go this route, at least not yet.
I am merely pointing out that an in-place sidegrade from v1 to a
protocol that avoids the megabyte-advertisement-at-the-beginning
seems to be possible, as a food for thought.
This is a fun thing indeed, though I'd personally feel uneasy with
such a probe as
a serious proposal. (Remember somebody 10 years from now wants to enjoy
reading the source code).
That cannot be a serious objection, once you realize that NUL + capability
was exactly the same kind of "yes, we have a hole to allow up customize
the protocol". The code to do so may not be pretty, but the code to implement
ended up being reasonably clean with parse_feature_request() and friends.
After all we live in a real world ;-)
- new server accepts the connection, but that no-op probe has
not arrived yet.". It misdetects the other side as a v1
client and it starts blasting the ref advertisement.
A race condition may be a serious objection then? Once people believe the
refs can scale fairly well they will use it, which means blasting the ref
advertisement will become very worse over time.
I'll try to present a 'client asks for options first out of band' instead of the
way you describe.
Also we should not rely on having holes here and there. (We might run out of
holes over time), so I'd rather have the capabilities presented at first
which rather opens new holes instead of closing old ones.
(assuming we'll never run into megabytes of capabilities
over time to have the same trouble again ;)
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:56
From: Nguyễn Thái Ngọc Duy <redacted>
pack-protocol.txt says so and fetch-pack also follows it even though
upload-pack is a bit lax. Fix it.
Signed-off-by: Stefan Beller <redacted>
---
upload-pack.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
@@ -596,7 +597,11 @@ static void receive_needs(void)die("git upload-pack: protocol error, ""expected to get sha, not '%s'",line);-parse_features(line+45);+if(first_want){+parse_features(line+45);+first_want=0;+}elseif(line[45])+die("garbage at the end of 'want' line %s",line+45);o=parse_object(sha1_buf);if(!o)
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:56
From: Nguyễn Thái Ngọc Duy <redacted>
The only difference from the original protocol client capabilities are
negotiated before initial refs advertisment.
Client capabilities are sent out of band (upload-pack receives it as
the second command line argument). The server sends one pkt-line back
advertising its capabilities.
Signed-off-by: Stefan Beller <redacted>
---
Notes:
v1:
I am still undecided if the client should then accept/resend
the capabilities to confirm them, which would make the client the
ultimate decider which capabilities are used.
My gut feeling is to rather let the server make the final decision
for the capabilities, as it will use some requested capabilities
already to not send out all the refs.
Documentation/git-upload-pack.txt | 10 +++++++++-
upload-pack.c | 42 +++++++++++++++++++++++++++------------
2 files changed, 38 insertions(+), 14 deletions(-)
@@ -34,6 +34,14 @@ OPTIONS <directory>:: The repository to sync from.+capabilities::+ Historically the capabilities were exchanged inside the protocol of+ 'git-upload-pack' talking to 'git-fetch-pack'. It turned out this was+ too late as 'git-upload-pack' already did work, which may have been+ avoided. This allows to pass in the capabilities the client wants to+ use as one argument. The capabilites are separated by space.+ See technical/protocol-capabilities.txt (TODO: how to make it a link?)+ SEE ALSO -------- linkgit:gitnamespaces[7]
@@ -597,11 +602,14 @@ static void receive_needs(void)die("git upload-pack: protocol error, ""expected to get sha, not '%s'",line);-if(first_want){-parse_features(line+45);-first_want=0;-}elseif(line[45])-die("garbage at the end of 'want' line %s",line+45);+if(!capabilities_first){+if(first_want){+parse_features(line+45);+first_want=0;+}elseif(line[45]){+die("garbage at the end of 'want' line %s",line+45);+}+}o=parse_object(sha1_buf);if(!o)
@@ -840,17 +848,25 @@ int main(int argc, char **argv)}}-if(i!=argc-1)-usage(upload_pack_usage);+switch(argc-i){+case2:+capabilities_first=1;+parse_features(argv[i+1]);+/* fall through*/+case1:+setup_path();-setup_path();+dir=argv[i];-dir=argv[i];+if(!enter_repo(dir,strict))+die("'%s' does not appear to be a git repository",dir);-if(!enter_repo(dir,strict))-die("'%s' does not appear to be a git repository",dir);+git_config(upload_pack_config,NULL);+upload_pack();+break;+default:+usage(upload_pack_usage);+}-git_config(upload_pack_config,NULL);-upload_pack();return0;}
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:56
If this is over git protocol, the flags is appended as the next
parameter after host=. If it's ssh, a new argument is appended to the
command line.
None of the callers use this now though.
[sb: originally by pclouds, rebased as jk implemented 1823bea10,
(git_connect: use argv_array), so any error is mine]
Signed-off-by: Stefan Beller <redacted>
---
builtin/fetch-pack.c | 2 +-
builtin/send-pack.c | 2 +-
connect.c | 18 ++++++++++++------
connect.h | 2 +-
transport.c | 3 ++-
5 files changed, 17 insertions(+), 10 deletions(-)
@@ -495,6 +495,7 @@ static int connect_setup(struct transport *transport, int for_push, int verbose)data->conn=git_connect(data->fd,transport->url,for_push?data->options.receivepack:data->options.uploadpack,+NULL,verbose?CONNECT_VERBOSE:0);return0;
@@ -850,7 +851,7 @@ static int connect_git(struct transport *transport, const char *name,{structgit_transport_data*data=transport->data;data->conn=git_connect(data->fd,transport->url,-executable,0);+executable,NULL,0);fd[0]=data->fd[0];fd[1]=data->fd[1];return0;
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:56
Before 73bb33a (daemon: Strictly parse the "extra arg" part of the
command - 2009-06-04) a client sending extra arguments could DoS
git-daemon. 73bb33a fixed it by forbidding extra arguments.
Allow arguments other than "host=" again as a preparation step for
upload-pack2. "host=" if present must be the first argument
though. The remaining arguments are concatenated by whitespace.
So far none of supported services support extra arguments. Attempting
to do will abort the service, just like how it is before. We might
want to make them silently ignore extra arguments though.
Signed-off-by: Stefan Beller <redacted>
---
connect.c | 3 ---
daemon.c | 37 ++++++++++++++++++++++++-------------
2 files changed, 24 insertions(+), 16 deletions(-)
@@ -514,8 +519,13 @@ static void parse_host_arg(char *extra_args, int buflen)/* On to the next one */extra_args=val+vallen;}-if(extra_args<end&&*extra_args)-die("Invalid request");+}++if(remaining_args){+for(val=extra_args;val<end;val++)+if(!*val)+*val=' ';+*remaining_args=extra_args;}/*
@@ -577,6 +587,7 @@ static int execute(void){char*line=packet_buffer;intpktlen,len,i;+char*args=NULL;char*addr=getenv("REMOTE_ADDR"),*port=getenv("REMOTE_PORT");if(addr)
@@ -603,7 +614,7 @@ static int execute(void)hostname=canon_hostname=ip_address=tcp_port=NULL;if(len!=pktlen)-parse_host_arg(line+len+1,pktlen-len-1);+parse_host_arg(line+len+1,&args,pktlen-len-1);for(i=0;i<ARRAY_SIZE(daemon_service);i++){structdaemon_service*s=&(daemon_service[i]);
@@ -616,7 +627,7 @@ static int execute(void)*Note:Thedirectoryhereisprobablycontextsensitive,*andmightdependontheactualservicebeingperformed.*/-returnrun_service(arg,s);+returnrun_service(arg,s,args);}}
@@ -191,10 +191,10 @@ HTTP clients that support the "smart" protocol (or both the "smart" and "dumb" protocols) MUST discover references by making a parameterized request for the info/refs file of the repository.-The request MUST contain exactly one query parameter,+The request MAY contain parameters. Supported parameters includes `service=$servicename`, where `$servicename` MUST be the service name the client wishes to contact to complete the operation.-The request MUST NOT contain additional query parameters.+Further parameters are as described in protocol-capabilities.txt C: GET $GIT_URL/info/refs?service=git-upload-pack HTTP/1.0
@@ -3,6 +3,10 @@ Git Protocol Capabilities Servers SHOULD support all capabilities defined in this document.+The client MAY ask for capabilities first out of band to the server.+If so the server MUST NOT advertise any capabilities the client did+not ask for.+ On the very first line of the initial server response of either receive-pack and upload-pack the first reference is followed by a NUL byte and then a list of space delimited server capabilities.
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:56
Heavily inspired by the ideas of Duy, who wrote the first
patches nearly a year ago.
Nguyễn Thái Ngọc Duy (2):
upload-pack: only accept capabilities on the first "want" line
upload-pack: support out of band client capability requests
Stefan Beller (3):
connect.c: connect to a remote service with some flags
daemon.c: accept extra service arguments
WIP/Document the http protocol change
Documentation/git-upload-pack.txt | 10 +++++-
Documentation/technical/http-protocol.txt | 4 +--
Documentation/technical/protocol-capabilities.txt | 4 +++
builtin/fetch-pack.c | 2 +-
builtin/send-pack.c | 2 +-
connect.c | 21 ++++++------
connect.h | 2 +-
daemon.c | 37 +++++++++++++--------
transport.c | 3 +-
upload-pack.c | 39 +++++++++++++++++------
10 files changed, 86 insertions(+), 38 deletions(-)
--
2.3.0.81.gc37f363
From: Kyle J. McKay <hidden> Date: 2016-06-15 23:03:56
On Feb 27, 2015, at 17:01, Stefan Beller wrote:
quoted hunk
From: Nguyễn Thái Ngọc Duy <redacted>
The only difference from the original protocol client capabilities are
negotiated before initial refs advertisment.
Client capabilities are sent out of band (upload-pack receives it as
the second command line argument). The server sends one pkt-line back
advertising its capabilities.
Signed-off-by: Stefan Beller <redacted>
---
Notes:
v1:
I am still undecided if the client should then accept/resend
the capabilities to confirm them, which would make the client the
ultimate decider which capabilities are used.
My gut feeling is to rather let the server make the final decision
for the capabilities, as it will use some requested capabilities
already to not send out all the refs.
Documentation/git-upload-pack.txt | 10 +++++++++-
upload-pack.c | 42 ++++++++++++++++++++++++++
+------------
2 files changed, 38 insertions(+), 14 deletions(-)
Isn't the problem with this that passing the extra argument to ssh
servers will cause them to fail?
Having just looked at the upload-pack.c source it looks to me like
trying to send "git-upload-pack 'dir' 'capabilities'" to an ssh git
server running a current version of the code will just end up
failing. I realize the extra argument is optional, so does that mean
there's no out-of-band support for ssh connections since the extra
argument would have to be omitted to remain compatible?
On Feb 26, 2015, at 12:13, Junio C Hamano wrote:
The capability-based sidegrade does not solve the problem
when the problem to be solved is that the server side needs to spend
a lot of cycles and the network needs to carry megabytes of data
before capability exchange happens.
On Feb 27, 2015, at 16:46, Stefan Beller wrote:
I'll try to present a 'client asks for options first out of band'
instead of the
way you describe.
On Feb 27, 2015, at 15:44, Stefan Beller wrote:
For both native git as well as ssh, Duy presented a solution at [1, 2]
a year ago, which essentially presents the desired client capabilites
'out of band' to the server via an argument to the server. So we'd
only need to examine the http(s) path how to pass in arguments there.
I've looked at those links and it's unclear to me how they support an
out-of-band option for ssh, they seem to be targeted at git-daemon.
Maybe there's another reference?
But there does seem to be a way to pass the protocol information out-
of-band for each of the HTTP, git and ssh connections to stop the
initial ref advertisement.
As already suggested [1, 2], for git: another "Extended attribute" can
be added after the host=...\0 to become host=...\0protocol=extended\0
or similar:
For HTTP, just add a second parameter in the query string .../info/
refs?service=git-upload-pack&protocol=extended. Alternatively an "X-
Git-Protocol: extended" or similar header can be added by the client.
It looks to me like the current http-backend.c already ignores any
extra parameters/headers.
That leaves ssh. A bit more problematic, but if the server side adds
"AcceptEnv GIT_PROTOCOL" to its sshd_config and then the client does
setenv("GIT_PROTOCOL","extended") and adds a "-o SendEnv=GIT_PROTOCOL"
option to the ssh command line the GIT_PROTOCOL variable will be
passed along. This one might need a config option to always disable
adding the "-o ..." option to the command line in case connect.c
guesses wrongly about it being OpenSSH and such is not likely to be
supported other than with OpenSSH on both ends. I'm not seeing any
other way to pass out-of-band information to an ssh server configured
to run git-shell that is safely ignored by current versions of the code.
Worst case with SSH is the initial ref advertisement is not suppressed
even though the server does support protocol v2 and a capability-based
sidegrade is required which is unfortunate.
-Kyle
[1] https://github.com/pclouds/git/commit/e26fa77c4d9ace06b9f2c80091af9eb7b63a1c95
[2] https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50
If this is over git protocol, the flags is appended as the next
parameter after host=. If it's ssh, a new argument is appended to the
command line.
None of the callers use this now though.
[sb: originally by pclouds, rebased as jk implemented 1823bea10,
(git_connect: use argv_array), so any error is mine]
Signed-off-by: Stefan Beller <redacted>
---
builtin/fetch-pack.c | 2 +-
builtin/send-pack.c | 2 +-
connect.c | 18 ++++++++++++------
connect.h | 2 +-
transport.c | 3 ++-
5 files changed, 17 insertions(+), 10 deletions(-)
The name "service_flags" reminds me on a service:
We connect to a service, but we are not a service.
And "flags" is often used for different bits, collected in an int, but not a "string"
Options are used at the command line, or arguments.
How about "extra_arg", or "extra_option", or "option_str", or simply "options" (or "option") ?
@@ -685,10 +687,13 @@ struct child_process *git_connect(int fd[2], const char *url, * Note: Do not add any other headers here! Doing so * will cause older git-daemon servers to crash. */- packet_write(fd[1],- "%s %s%chost=%s%c",- prog, path, 0,- target_host, 0);+ if (!service_flags)+ packet_write(fd[1], "%s %s%chost=%s%c",+ prog, path, 0, target_host, 0);+ else+ packet_write(fd[1], "%s %s%chost=%s%c%s%c",+ prog, path, 0, target_host, 0,+ service_flags, 0);
We don't need this "big if" here, a simple "?" will do:
packet_write(fd[1], "%s %s%chost=%s%c%s%c",
prog, path, 0, target_host, 0,
options ? options : "", /* old service_flasgs */
0);
On Sat, Feb 28, 2015 at 2:47 PM, Kyle J. McKay [off-list ref] wrote:
On Feb 27, 2015, at 17:01, Stefan Beller wrote:
quoted
From: Nguyễn Thái Ngọc Duy <redacted>
The only difference from the original protocol client capabilities are
negotiated before initial refs advertisment.
Client capabilities are sent out of band (upload-pack receives it as
the second command line argument). The server sends one pkt-line back
advertising its capabilities.
Signed-off-by: Stefan Beller <redacted>
---
Notes:
v1:
I am still undecided if the client should then accept/resend
the capabilities to confirm them, which would make the client the
ultimate decider which capabilities are used.
My gut feeling is to rather let the server make the final decision
for the capabilities, as it will use some requested capabilities
already to not send out all the refs.
Documentation/git-upload-pack.txt | 10 +++++++++-
upload-pack.c | 42
+++++++++++++++++++++++++++------------
2 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-upload-pack.txt
b/Documentation/git-upload-pack.txt
index 0abc806..ad3a89d 100644
Isn't the problem with this that passing the extra argument to ssh servers
will cause them to fail?
Having just looked at the upload-pack.c source it looks to me like trying to
send "git-upload-pack 'dir' 'capabilities'" to an ssh git server running a
current version of the code will just end up failing. I realize the extra
argument is optional, so does that mean there's no out-of-band support for
ssh connections since the extra argument would have to be omitted to remain
compatible?
The client should only trigger this behavior when it knows the server
can deal with it. And that is possible because in the last fetch, the
server has told the client that it's capable of receiving this
capabilities argument. Backward compatibility is a concern at client
side, not server side.
I've looked at those links and it's unclear to me how they support an
out-of-band option for ssh, they seem to be targeted at git-daemon. Maybe
there's another reference?
For ssh, I think connect.c is the one that constructs and executes ssh command.
--
Duy
On Sat, Feb 28, 2015 at 8:01 AM, Stefan Beller [off-list ref] wrote:
Notes:
v1:
I am still undecided if the client should then accept/resend
the capabilities to confirm them, which would make the client the
ultimate decider which capabilities are used.
My gut feeling is to rather let the server make the final decision
for the capabilities, as it will use some requested capabilities
already to not send out all the refs.
pack-capabilities.txt says
"Client will then send a space separated list of capabilities it wants
to be in effect. The client MUST NOT ask for capabilities the server
did not say it supports."
What was sent out of band is what the client can support, not what it
wants. So perhaps drop this patch and let the client decide exactly
what it wants.
--
Duy
@@ -3,6 +3,10 @@ Git Protocol Capabilities Servers SHOULD support all capabilities defined in this document.+The client MAY ask for capabilities first out of band to the server.+If so the server MUST NOT advertise any capabilities the client did+not ask for.
I think what the current patches do is "For v2, the client must _tell_
the server its capabilities". The unwritten statement is "the server
must not use any capabilities not advertised by the client". v2 will
not send initial refs by default, and the client will need to send
"wantrefs refs/heads/* refs/tags/*" or similar to retrieve ref list.
So we don't have any client capability yet. But there are two caps
that come to mind:
- ref list compression (for sending full ref list in clone),
compression ratio may be 50%
- setting locale in upload-pack and receive-pack to support l10n.
On the very first line of the initial server response of either
receive-pack and upload-pack the first reference is followed by
a NUL byte and then a list of space delimited server capabilities.
This one should be prefixed with "in version 1, "
--
Duy
From: Kyle J. McKay <hidden> Date: 2016-06-15 23:03:56
On Feb 28, 2015, at 03:22, Duy Nguyen wrote:
The client should only trigger this behavior when it knows the server
can deal with it. And that is possible because in the last fetch, the
server has told the client that it's capable of receiving this
capabilities argument. Backward compatibility is a concern at client
side, not server side.
quoted
I've looked at those links and it's unclear to me how they support an
out-of-band option for ssh, they seem to be targeted at git-
daemon. Maybe
there's another reference?
For ssh, I think connect.c is the one that constructs and executes
ssh command.
This I assume you're referring to this change in connect.c from [1]:
char *url,
conn->use_shell = 1;
}
argv_array_push(&argv, cmd.buf);
+ if (service_flags)
+ argv_array_push(&argv, service_flags);
conn->argv = argv.argv;
if (start_command(conn))
die("unable to fork");
That's not going to work for ssh servers running a stock git-shell and
I haven't seen any updates to shell.c to match. git-shell does not
allow anything other than one argument to be passed to git-upload-pack/
git-receive-pack.
When shell.c calls do_generic_cmd and it calls sq_dequote on its
argument that contains "'dir' 'service-flags'" it's going to return
NULL and shell.c will die("bad argument"). So I don't see how this
supports ssh as-is even if you know in advance the server supports the
new protocol. I don't see any changes to shell.c in that uploadpack2
branch nor in this patch series.
-Kyle
[1] https://github.com/pclouds/git/commit/20d048e5fc650b20fdc7dd8bbe35cb8510ac9c50
On Sun, Mar 1, 2015 at 5:36 AM, Kyle J. McKay [off-list ref] wrote:
quoted hunk
On Feb 28, 2015, at 03:22, Duy Nguyen wrote:
quoted
The client should only trigger this behavior when it knows the server
can deal with it. And that is possible because in the last fetch, the
server has told the client that it's capable of receiving this
capabilities argument. Backward compatibility is a concern at client
side, not server side.
quoted
I've looked at those links and it's unclear to me how they support an
out-of-band option for ssh, they seem to be targeted at git-daemon.
Maybe
there's another reference?
For ssh, I think connect.c is the one that constructs and executes ssh
command.
This I assume you're referring to this change in connect.c from [1]:
*url,
conn->use_shell = 1;
}
argv_array_push(&argv, cmd.buf);
+ if (service_flags)
+ argv_array_push(&argv, service_flags);
conn->argv = argv.argv;
if (start_command(conn))
die("unable to fork");
That's not going to work for ssh servers running a stock git-shell and I
haven't seen any updates to shell.c to match. git-shell does not allow
anything other than one argument to be passed to
git-upload-pack/git-receive-pack.
When shell.c calls do_generic_cmd and it calls sq_dequote on its argument
that contains "'dir' 'service-flags'" it's going to return NULL and shell.c
will die("bad argument"). So I don't see how this supports ssh as-is even
if you know in advance the server supports the new protocol. I don't see
any changes to shell.c in that uploadpack2 branch nor in this patch series.
You're right. If we continue to pass client capabilities as an extra
argument, then git-shell needs updates.
--
Duy
From: Johannes Sixt <hidden> Date: 2016-06-15 23:03:56
Am 28.02.2015 um 02:01 schrieb Stefan Beller:
Heavily inspired by the ideas of Duy, who wrote the first
patches nearly a year ago.
Nguyễn Thái Ngọc Duy (2):
upload-pack: only accept capabilities on the first "want" line
upload-pack: support out of band client capability requests
Stefan Beller (3):
connect.c: connect to a remote service with some flags
daemon.c: accept extra service arguments
WIP/Document the http protocol change
Documentation/git-upload-pack.txt | 10 +++++-
Documentation/technical/http-protocol.txt | 4 +--
Documentation/technical/protocol-capabilities.txt | 4 +++
builtin/fetch-pack.c | 2 +-
builtin/send-pack.c | 2 +-
connect.c | 21 ++++++------
connect.h | 2 +-
daemon.c | 37 +++++++++++++--------
transport.c | 3 +-
upload-pack.c | 39 +++++++++++++++++------
10 files changed, 86 insertions(+), 38 deletions(-)
You may also consider an idea I proposed here:
http://thread.gmane.org/gmane.comp.version-control.git/206886/focus=207342
The idea is that the exchange begins as usual, but when the v2 client
sees that the server also supports v2, then it begins sending its
desired refs. When the server notices that the client spoke while it was
still sending out its megabytes of ref advertisments, it stops the v1
advertisements and continues with v2 protocol.
But Shawn pointed out that this would be difficult to implement in a
JGit server because there is no way to poll the incoming channel while
it is sending out refs.
-- Hannes