protocol version 2

Changes in v2:
* Added documentation for fetch
* changes #defines for state variables to be enums
* couple code changes to pkt-line functions and documentation
* Added unit tests for the git-serve binary as well as for ls-refs

Areas for improvement
* Push isn't implemented, right now this is ok because if v2 is requested the server can just default to v0. Before this can be merged we may want to change how the client request a new protocol, and not allow for sending "version=2" when pushing even though the user has it configured. Or maybe its fine to just have an older client who doesn't understand how to push (and request v2) to die if the server tries to speak v2 at it.

Fixing this essentially would just require piping through a bit more information to the function which ultimately runs connect (for both builtins and remote-curl)

* I want to make sure that the docs are well written before this gets merged so I'm hoping that someone can do a through review on the docs themselves to make sure they are clear.

* Right now there is a capability 'stateless-rpc' which essentially makes sure that a server command completes after a single round (this is to make sure http works cleanly). After talking with some folks it may make more sense to just have v2 be stateless in nature so that all commands terminate after a single round trip. This makes things a bit easier if a server wants to have ssh just be a proxy for http.

One potential thing would be to flip this so that by default the protocol is stateless and if a server/command has a state-full mode that can be implemented as a capability at a later point. Thoughts?

* Shallow repositories and shallow clones aren't supported yet. I'm working on it and it can be either added to v2 by default if people think it needs to be in there from the start, or we can add it as a capability at a later point.

Sorry for chiming in with mostly nitpicks so late since sending this version. Mostly, I tried to read it to see if I could understand the scope of the patch and how this code worked before. It looks very polished, so I the nits were the best I could do.

Quoted 5 lines · Brandon Williams

Changes in v2:
* Added documentation for fetch
* changes #defines for state variables to be enums
* couple code changes to pkt-line functions and documentation
* Added unit tests for the git-serve binary as well as for ls-refs

I'm a fan of more unit-level testing, and I think that will be more important as we go on with these multiple configuration options.

Quoted 11 lines

Areas for improvement
* Push isn't implemented, right now this is ok because if v2 is requested the server can just default to v0. Before this can be merged we may want to change how the client request a new protocol, and not allow for sending "version=2" when pushing even though the user has it configured. Or maybe its fine to just have an older client who doesn't understand how to push (and request v2) to die if the server tries to speak v2 at it.

Fixing this essentially would just require piping through a bit more information to the function which ultimately runs connect (for both builtins and remote-curl)

Definitely save push for a later patch. Getting 'fetch' online did require 'ls-refs' at the same time. Future reviews will be easier when adding one command at a time.

Quoted 4 lines

* I want to make sure that the docs are well written before this gets merged so I'm hoping that someone can do a through review on the docs themselves to make sure they are clear.

I made a comment in the docs about the architectural changes. While I think a discussion on that topic would be valuable, I'm not sure that's the point of the document (i.e. documenting what v2 does versus selling the value of the patch). I thought the docs were clear for how the commands work.

Quoted 7 lines

* Right now there is a capability 'stateless-rpc' which essentially makes sure that a server command completes after a single round (this is to make sure http works cleanly). After talking with some folks it may make more sense to just have v2 be stateless in nature so that all commands terminate after a single round trip. This makes things a bit easier if a server wants to have ssh just be a proxy for http.

Quoted 3 lines

One potential thing would be to flip this so that by default the protocol is stateless and if a server/command has a state-full mode that can be implemented as a capability at a later point. Thoughts?

At minimum, all commands should be designed with a "stateless first" philosophy since a large number of users communicate via HTTP[S] and any decisions that make stateless communication painful should be rejected.

Quoted 5 lines

* Shallow repositories and shallow clones aren't supported yet. I'm working on it and it can be either added to v2 by default if people think it needs to be in there from the start, or we can add it as a capability at a later point.

I'm happy to say the following:

1. Shallow repositories should not be used for servers, since they cannot service all requests.

2. Since v2 has easy capability features, I'm happy to leave shallow for later. We will want to verify that a shallow clone command reverts to v1.

I fetched bw/protocol-v2 with tip 13c70148, built, set 'protocol.version=2' in the config, and tested fetches against GitHub and VSTS just as a compatibility test. Everything worked just fine.

Is there an easy way to test the existing test suite for clone and fetch using protocol v2 to make sure there are no regressions with protocol.version=2 in the config?

Thanks,
-Stolee

Changes in v2:
* Added documentation for fetch
* changes #defines for state variables to be enums
* couple code changes to pkt-line functions and documentation
* Added unit tests for the git-serve binary as well as for ls-refs

The current pkt-line API encodes the status of a pkt-line read in the length of the read content. An error is indicated with '-1', a flush with '0' (which can be confusing since a return value of '0' can also indicate an empty pkt-line), and a positive integer for the length of the read content otherwise. This doesn't leave much room for allowing the addition of additional special packets in the future.

To solve this introduce 'packet_read_with_status()' which reads a packet and returns the status of the read encoded as an 'enum packet_status' type. This allows for easily identifying between special and normal packets as well as errors. It also enables easily adding a new special packet in the future.

pkt-line.c +15 -1

@@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
 
 	buffer[len] = 0;
 	packet_trace(buffer, len, 0);
-	return len;
+	*pktlen = len;
+	return PACKET_READ_NORMAL;
+}
+
+int packet_read(int fd, char **src_buffer, size_t *src_len,
+		char *buffer, unsigned size, int options)
+{
+	enum packet_read_status status;
+	int pktlen;
+
+	status = packet_read_with_status(fd, src_buffer, src_len,
+					 buffer, size, &pktlen,
+					 options);
+	switch (status) {
+	case PACKET_READ_EOF:

pkt-line.h +1 -0

@@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);
 
+/*

Subject: pkt-line: introduce struct packet_reader

Sometimes it is advantageous to be able to peek the next packet line without consuming it (e.g. to be able to determine the protocol version a server is speaking). In order to do that introduce 'struct packet_reader' which is an abstraction around the normal packet reading logic. This enables a caller to be able to peek a single line at a time using 'packet_reader_peek()' and having a caller consume a line by calling 'packet_reader_read()'.

+/*
+ * Perform a packet read and return the status of the read.

+ * The values of 'pktlen' and 'line' are updated based on the status of the
+ * read as follows:
+ *
+ * PACKET_READ_ERROR: 'pktlen' is set to '-1' and 'line' is set to NULL
+ * PACKET_READ_NORMAL: 'pktlen' is set to the number of bytes read
+ * 'line' is set to point at the read line
+ * PACKET_READ_FLUSH: 'pktlen' is set to '0' and 'line' is set to NULL
+ */
+extern enum packet_read_status packet_reader_read(struct packet_reader *reader);

+ * The next call to 'packet_reader_read()' will perform a read of the same line
+ * that was peeked, consuming the line.

+ *
+ * Peeking multiple times without calling 'packet_reader_read()' will return
+ * the same result.
+ */
+extern enum packet_read_status packet_reader_peek(struct packet_reader *reader);

pkt-line.c +19 -0

+
+	reader->status = packet_read_with_status(reader->fd,
+						 &reader->src_buffer,
+						 &reader->src_len,
+						 reader->buffer,
+						 reader->buffer_size,
+						 &reader->pktlen,
+						 reader->options);
+
+	switch (reader->status) {
+	case PACKET_READ_EOF:
+		reader->pktlen = -1;
+		reader->line = NULL;
+		break;
+	case PACKET_READ_NORMAL:
+		reader->line = reader->buffer;
+		break;
+	case PACKET_READ_FLUSH:
+		reader->pktlen = 0;

[...]

pkt-line.h char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); · line 120

Quoted 13 lines

pkt-line.h +7 -0

@@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+struct packet_reader {
+	/* source file descriptor */
+	int fd;
+
+	/* source buffer and its size */
+	char *src_buffer;
+	size_t src_len;

Can or should this be a strbuf?

pkt-line.h char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); · line 124

Quoted 4 lines

pkt-line.h +4 -0

+
+	/* buffer that pkt-lines are read into and its size */
+	char *buffer;
+	unsigned buffer_size;

Likewise.

Quoted 16 lines · Jonathan Nieder

pkt-line.h char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); · line 120

Quoted 13 lines

pkt-line.h +7 -0

@@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+struct packet_reader {
+	/* source file descriptor */
+	int fd;
+
+	/* source buffer and its size */
+	char *src_buffer;
+	size_t src_len;

Can or should this be a strbuf?

Quoted 6 lines

pkt-line.h char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size); · line 124

Quoted 4 lines

pkt-line.h +4 -0

+
+	/* buffer that pkt-lines are read into and its size */
+	char *buffer;
+	unsigned buffer_size;

Likewise.

This struct is setup to be a drop in replacement for the existing read_packet() family of functions. Because of this I tried to make the interface as similar as possible to make it easy to convert to using it as well as having no need to clean anything up (because the struct is really just a wrapper and doesn't own anything).

Sometimes it is advantageous to be able to peek the next packet line without consuming it (e.g. to be able to determine the protocol version a server is speaking). In order to do that introduce 'struct packet_reader' which is an abstraction around the normal packet reading logic. This enables a caller to be able to peek a single line at a time using 'packet_reader_peek()' and having a caller consume a line by calling 'packet_reader_read()'.

Signed-off-by Brandon Williams <[email protected]>

---
 pkt-line.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pkt-line.h | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

Quoted 8 lines · Brandon Williams

Sometimes it is advantageous to be able to peek the next packet line without consuming it (e.g. to be able to determine the protocol version a server is speaking). In order to do that introduce 'struct packet_reader' which is an abstraction around the normal packet reading logic. This enables a caller to be able to peek a single line at a time using 'packet_reader_peek()' and having a caller consume a line by calling 'packet_reader_read()'.

Quoted 5 lines

Signed-off-by Brandon Williams <[email protected]>

---
 pkt-line.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 pkt-line.h | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

I like it!

The questions and nits from
https://public-inbox.org/git/[email protected]/
still apply. In particular, the ownership of the buffers inside the 'struct packet_reader' is still unclear; could the packet_reader create its own (strbuf) buffers so that the contract around them (who is allowed to write to them; who is responsible for freeing them) is more obvious?

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

To do this, introduce the special deliminator packet '0001'. A delim packet can then be used as a deliminator between lists of packet lines while flush packets can be reserved to indicate the end of a response.

Signed-off-by Brandon Williams <[email protected]>

Quoted 7 lines · Brandon Williams

One of the design goals of protocol-v2 is to improve the semantics of flush packets. Currently in protocol-v1, flush packets are used both to indicate a break in a list of packet lines as well as an indication that one side has finished speaking. This makes it particularly difficult to implement proxies as a proxy would need to completely understand git protocol instead of simply looking for a flush packet.

Quoted 3 lines

To do this, introduce the special deliminator packet '0001'. A delim packet can then be used as a deliminator between lists of packet lines while flush packets can be reserved to indicate the end of a response.

Please mention where this can be found in the documentation. (Defer to later patch?)

As the commit message states, this is only to be used for v2, in v0 it is still an illegal pkt.

Quoted 2 lines

Signed-off-by Brandon Williams <[email protected]>

The code is

Reviewed-by Stefan Beller <[email protected]>

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Signed-off-by Brandon Williams <[email protected]>

upload-pack.c +0 -16

-static void upload_pack(void)
-{
-	struct string_list symref = STRING_LIST_INIT_DUP;
-
-	head_ref_namespaced(find_symref, &symref);
-
-	if (advertise_refs || !stateless_rpc) {
-		reset_timeout();
-		head_ref_namespaced(send_ref, &symref);
-		for_each_namespaced_ref(send_ref, &symref);
-		advertise_shallow_grafts(1);
-		packet_flush(1);
-	} else {
-		head_ref_namespaced(check_ref, NULL);
-		for_each_namespaced_ref(check_ref, NULL);
-	}

Quoted 4 lines · On Tue, 6 Feb 2018 17:12:41 -0800 Brandon Williams <[email protected]> wrote:

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Signed-off-by Brandon Williams <[email protected]>

As Stefan mentioned in [1], also mention in the commit message that this means that the "git-upload-pack" invocation gains additional capabilities (for example, invoking a pager for --help).

Having said that, the main purpose of this patch seems to be to libify upload-pack, and the move to builtin is just a way of putting the program somewhere - we could have easily renamed upload-pack.c and created a new upload-pack.c containing the main(), preserving the non-builtin-ness of upload-pack, while still gaining the benefits of libifying upload-pack.

Quoted 6 lines · Jonathan Tan

Quoted 3 lines · On Tue, 6 Feb 2018 17:12:41 -0800 Brandon Williams <[email protected]> wrote:

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Quoted 5 lines

Quoted 1 line

Signed-off-by Brandon Williams <[email protected]>

As Stefan mentioned in [1], also mention in the commit message that this means that the "git-upload-pack" invocation gains additional capabilities (for example, invoking a pager for --help).

And possibly respecting pager.upload-pack, which would violate our rule that it is safe to run upload-pack in untrusted repositories.

(This actually doesn't work right now because pager.* is broken for builtins that don't specify RUN_SETUP; but I think with the fixes last year to the config code, we can now drop that restriction).

Obviously we can work around this with an extra RUN_NO_PAGER_CONFIG flag. But I think it points to a general danger in making upload-pack a builtin. I'm not sure what other features it would want to avoid (or what might grow in the future).

Quoted 6 lines

Having said that, the main purpose of this patch seems to be to libify upload-pack, and the move to builtin is just a way of putting the program somewhere - we could have easily renamed upload-pack.c and created a new upload-pack.c containing the main(), preserving the non-builtin-ness of upload-pack, while still gaining the benefits of libifying upload-pack.

Yeah, this seems like a better route to me.

-Peff

Quoted 14 lines · Jeff King

Quoted 11 lines · Jonathan Tan

Quoted 4 lines · On Tue, 6 Feb 2018 17:12:41 -0800 Brandon Williams <[email protected]> wrote:

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Signed-off-by Brandon Williams <[email protected]>

As Stefan mentioned in [1], also mention in the commit message that this means that the "git-upload-pack" invocation gains additional capabilities (for example, invoking a pager for --help).

Quoted 2 lines

And possibly respecting pager.upload-pack, which would violate our rule that it is safe to run upload-pack in untrusted repositories.

And this isn't an issue with receive-pack because this same guarantee doesn't exist?

Quoted 7 lines · Brandon Williams

Quoted 6 lines · Jeff King

Quoted 5 lines · Jonathan Tan

Quoted 2 lines · On Tue, 6 Feb 2018 17:12:41 -0800 Brandon Williams <[email protected]> wrote:

In order to allow for code sharing with the server-side of fetch in protocol-v2 convert upload-pack to be a builtin.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

As Stefan mentioned in [1], also mention in the commit message that this means that the "git-upload-pack" invocation gains additional capabilities (for example, invoking a pager for --help).

And possibly respecting pager.upload-pack, which would violate our rule that it is safe to run upload-pack in untrusted repositories.

And this isn't an issue with receive-pack because this same guarantee doesn't exist?

Yes, exactly (which is confusing and weird, yes, but that's how it is).

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

And possibly respecting pager.upload-pack, which would violate our rule that it is safe to run upload-pack in untrusted repositories.

And this isn't an issue with receive-pack because this same guarantee doesn't exist?

Yes, exactly (which is confusing and weird, yes, but that's how it is).

To be clear, which of the following are you (most) worried about?

1. being invoked with --help and spawning a pager
2. receiving and acting on options between 'git' and 'upload-pack'
3. repository discovery
4. pager config
5. alias discovery
6. increased code surface / unknown threats

Although in most setups the user does not control the config files on a server, item (4) looks like a real issue worth solving. I think we should introduce a flag to skip looking for pager config. We could use it for receive-pack, too.

Quoted 8 lines

To be clear, which of the following are you (most) worried about?

1. being invoked with --help and spawning a pager
2. receiving and acting on options between 'git' and 'upload-pack'
3. repository discovery
4. pager config
5. alias discovery
6. increased code surface / unknown threats

My immediate concern is (4). But my greater concern is that people who work on git.c should not have to worry about accidentally violating this principle when they add a new feature or config option.

In other words, it seems like an accident waiting to happen. I'd be more amenable to it if there was some compelling reason for it to be a builtin, but I don't see one listed in the commit message. I see only "let's make it easier to share the code", which AFAICT is equally served by just lib-ifying the code and calling it from the standalone upload-pack.c.

The current property is that it's safe to fetch from an
untrusted repository, even over ssh. If we're keeping that for protocol v1, we'd want it to apply to protocol v2, as well.

Keep in mind that git upload-archive (a read-only command, just like git upload-pack) also already has the same issues.

Quoted 2 lines · Jonathan Nieder

Keep in mind that git upload-archive (a read-only command, just like git upload-pack) also already has the same issues.

Yuck. I don't think we've ever made a historical promise about that. But then, I don't think the promise about upload-pack has ever really been documented, except in mailing list discussions.

Hi,

Quoted 8 lines · Jeff King

Quoted 2 lines · Jonathan Nieder

Keep in mind that git upload-archive (a read-only command, just like git upload-pack) also already has the same issues.

Yuck. I don't think we've ever made a historical promise about that. But then, I don't think the promise about upload-pack has ever really been documented, except in mailing list discussions.

Sorry to revive this old side-thread. Good news: for a dashed command like git-upload-archive, the pager selection code only runs for commands with RUN_SETUP or RUN_SETUP_GENTLY:

	if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
	    !(p->option & DELAY_PAGER_CONFIG))
		use_pager = check_pager_config(p->cmd);

None of upload-pack, receive-pack,git-serve, or upload-archive set those flags, so we (narrowly) escape trouble here.

Later today I should be able to send a cleanup to make the behavior more obvious.

We could even give it an environment variable, which would allow something like:

tar xf maybe-evil.git.tar
cd maybe-evil
export GIT_TRUST_REPO=false
git log

1. We have to manually annotate any "dangerous" code to act more safely when it sees the flag. Which means it's highly likely to a spot, or to add a new feature which doesn't respect it. And suddenly that's a security hole. So I'm concerned it may create a false sense of security and actually make things worse.

Quoted 7 lines · Jeff King

We could even give it an environment variable, which would allow something like:

tar xf maybe-evil.git.tar
cd maybe-evil
export GIT_TRUST_REPO=false
git log

[...]
As an internal implementation detail, this is so obviously fragile that it wouldn't give me any feeling of security. ;-) So it should be strictly an improvement.

As a public-facing feature, I suspect it's a bad idea for exactly that reason.

FWIW for pager specifically I am going for a whitelisting approach:
new commands would have to explicitly set ALLOW_PAGER if they want to respect pager config. That doesn't guarantee people think about it again as things evolve but it should at least help with getting the right setting for new plumbing.

Quoted 13 lines · Jeff King

Quoted 8 lines

To be clear, which of the following are you (most) worried about?

1. being invoked with --help and spawning a pager
2. receiving and acting on options between 'git' and 'upload-pack'
3. repository discovery
4. pager config
5. alias discovery
6. increased code surface / unknown threats

My immediate concern is (4). But my greater concern is that people who work on git.c should not have to worry about accidentally violating this principle when they add a new feature or config option.

Quoted 6 lines

In other words, it seems like an accident waiting to happen. I'd be more amenable to it if there was some compelling reason for it to be a builtin, but I don't see one listed in the commit message. I see only "let's make it easier to share the code", which AFAICT is equally served by just lib-ifying the code and calling it from the standalone upload-pack.c.

By the way, any decision here would presumably need to be extended to git-serve, etc. The current property is that it's safe to fetch from an untrusted repository, even over ssh. If we're keeping that for protocol v1, we'd want it to apply to protocol v2, as well.

-Peff

Quoted 4 lines

By the way, any decision here would presumably need to be extended to git-serve, etc. The current property is that it's safe to fetch from an untrusted repository, even over ssh. If we're keeping that for protocol v1, we'd want it to apply to protocol v2, as well.

This may be more complicated. Right now (for backward compatibility) all fetches for v2 are issued to the upload-pack endpoint. So even though I've introduced git-serve it doesn't have requests issued to it and no requests can be issued to it currently (support isn't added to http-backend or git-daemon). This just means that the command already exists to make it easy for testing specific v2 stuff and if we want to expose it as an endpoint (like when we have a brand new server command that is completely incompatible with v1) its already there and support just needs to be plumbed in.

This whole notion of treating upload-pack differently from receive-pack has bad consequences for v2 though. The idea for v2 is to be able to run any number of commands via the same endpoint, so at the end of the day the endpoint you used is irrelevant. So you could issue both fetch and push commands via the same endpoint in v2 whether its git-serve, receive-pack, or upload-pack. So really, like Jonathan has said elsewhere, we need to figure out how to be ok with having receive-pack and upload-pack builtins, or having neither of them builtins, because it doesn't make much sense for v2 to straddle that line.

Quoted 2 lines · Jonathan Nieder

To be clear, which of the following are you (most) worried about?

Quoted 6 lines

1. being invoked with --help and spawning a pager
2. receiving and acting on options between 'git' and 'upload-pack'
3. repository discovery
4. pager config
5. alias discovery
6. increased code surface / unknown threats

My immediate concern is (4).

But my greater concern is that people who
work on git.c should not have to worry about accidentally violating this principle when they add a new feature or config option.

In other words, it seems like an accident waiting to happen. I'd be more amenable to it if there was some compelling reason for it to be a builtin, but I don't see one listed in the commit message. I see only "let's make it easier to share the code", which AFAICT is equally served by just lib-ifying the code and calling it from the standalone upload-pack.c.

Quoted 4 lines

Although in most setups the user does not control the config files on a server, item (4) looks like a real issue worth solving. I think we should introduce a flag to skip looking for pager config. We could use it for receive-pack, too.

There's not much point for receive-pack. It respects hooks, so any security ship has already sailed there.

Quoted 3 lines

But my greater concern is that people who
work on git.c should not have to worry about accidentally violating this principle when they add a new feature or config option.

That sounds like a combination of (6) and insufficient documentation or tests. Ideas for how we can help prevent such accidents?

Quoted 6 lines

In other words, it seems like an accident waiting to happen. I'd be more amenable to it if there was some compelling reason for it to be a builtin, but I don't see one listed in the commit message. I see only "let's make it easier to share the code", which AFAICT is equally served by just lib-ifying the code and calling it from the standalone upload-pack.c.

If we have so little control of the common code used by git commands that could be invoked by a remote user, I think we're in trouble already. I don't think being a builtin vs not makes that significantly different, since there are plenty of builtins that can be triggered by remote users. Further, if we have so little control over the security properties of git.c, what hope do we have of making the rest of libgit.a usable in secure code?

In other words, having to pay more attention to the git wrapper from a security pov actually feels to me like a *good* thing. The git wrapper is the entry point to almost all git commands. If it is an accident waiting to happen, then anything that calls git commands is already an accident waiting to happen. So how can we make it not an accident waiting to happen? :)

Quoted 2 lines

There's not much point for receive-pack. It respects hooks, so any security ship has already sailed there.

Yet there are plenty of cases where people who can push are not supposed to have root privilege. I am not worried about hooks specifically (although the changes described at [1] might help and I still plan to work on those) but I am worried about e.g. commandline injection issues. I don't think we can treat receive-pack as out of scope.

And to be clear, I don't think you were saying receive-pack *is* out of scope. But you seem to be trying to draw some boundary, where I only see something fuzzier (e.g. if a bug only applies to receive-pack, then that certainly decreases its impact, but it doesn't make the impact go away).

But I don't think it _is_ an accident waiting to happen for the rest of the commands. upload-pack is special. The point is that people may touch git.c thinking they are adding a nice new feature (like pager config, or aliases, or default options, or whatever). And it _would_ be a nice new feature for most commands, but not for upload-pack, because its requirements are different.

So thinking about security in the git wrapper is just a burden for those other commands.

All of that said, I think the current code is quite dangerous already, and maybe even broken. upload-pack may run sub-commands like rev-list or pack-objects, which are themselves builtins.

I couldn't quite get it to work, but I think it's because I'm doing something wrong with the submodules. But I also think this attack would _have_ to be done over ssh, because on a local system the submodule clone would a hard-link rather than a real fetch.

Quoted 4 lines

I couldn't quite get it to work, but I think it's because I'm doing something wrong with the submodules. But I also think this attack would _have_ to be done over ssh, because on a local system the submodule clone would a hard-link rather than a real fetch.

What happens if the submodule URL starts with file://?

Quoted 6 lines · Jonathan Nieder

Quoted 4 lines

I couldn't quite get it to work, but I think it's because I'm doing something wrong with the submodules. But I also think this attack would _have_ to be done over ssh, because on a local system the submodule clone would a hard-link rather than a real fetch.

What happens if the submodule URL starts with file://?

Ah, that would do it. Or I guess any follow-up fetch.

I'm still having trouble convincing submodules to fetch _just_ the desired sha1, though. It always just fetches everything. I know there's a way that this kicks in (that's why we have things like allowReachableSHA1InWant), but I'm not sufficiently well-versed in submodules to know how to trigger it.

Factor out the logic for processing shallow, deepen, deepen_since, and deepen_not lines into their own functions to simplify the 'receive_needs()' function in addition to making it easier to reuse some of this logic when implementing protocol_v2.

Signed-off-by Brandon Williams <[email protected]>

+ *depth = (int)strtol(arg, &end, 0);

Quoted 5 lines · Brandon Williams

Factor out the logic for processing shallow, deepen, deepen_since, and deepen_not lines into their own functions to simplify the 'receive_needs()' function in addition to making it easier to reuse some of this logic when implementing protocol_v2.

Quoted 1 line

Signed-off-by Brandon Williams <[email protected]>

Reviewed-by Stefan Beller <[email protected]>

for the stated purpose of just refactoring existing code for better reuse later.

I do have a few comments on the code in general, which might be out of scope for this series.

A close review would have been fastest if we had some sort of
https://public-inbox.org/git/[email protected]/
which I might revive soon for this purpose. (it showed that I would need it)

Quoted 1 line

+ *depth = (int)strtol(arg, &end, 0);

strtol is not used quite correctly here IMHO, as we do not inspect errno for ERANGE

Thanks,
Stefan

Remove code duplication and use the existing 'get_refs_via_connect()' function to retrieve a remote's heads in 'fetch_refs_via_pack()' and 'git_transport_push()'.

Signed-off-by Brandon Williams <[email protected]>

---
 transport.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

transport.c +0 -0

@@ -541,14 +537,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	struct send_pack_args args;
 	int ret;
 

Quoted 8 lines · Brandon Williams

Remove code duplication and use the existing 'get_refs_via_connect()' function to retrieve a remote's heads in 'fetch_refs_via_pack()' and 'git_transport_push()'.

Signed-off-by Brandon Williams <[email protected]>

---
 transport.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

I like the diffstat.

[...]

transport.c static int fetch_refs_via_pack(struct transport *transport, · line 234

Quoted 13 lines

+++ b/transport.c

transport.c +2 -6

@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.cloning = transport->cloning;
 	args.update_shallow = data->options.update_shallow;
 
-	if (!data->got_remote_heads) {
-		connect_setup(transport, 0);
-		get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,
-				 NULL, &data->shallow);
-		data->got_remote_heads = 1;
-	}
+	if (!data->got_remote_heads)
+		refs_tmp = get_refs_via_connect(transport, 0);

The only difference between the old and new code is that the old code passes NULL as 'extra_have' and the new code passes &data->extra_have.

That means this populates the data->extra_have oid_array. Does it matter?

Quoted 4 lines

transport.c +0 -0

@@ -541,14 +537,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	struct send_pack_args args;
 	int ret;
 

transport.c static int git_transport_push(struct transport *transport, struct ref *remote_re · line 541

Quoted 10 lines

transport.c +2 -8

-	if (!data->got_remote_heads) {
-		struct ref *tmp_refs;
-		connect_setup(transport, 1);
-
-		get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,
-				 NULL, &data->shallow);
-		data->got_remote_heads = 1;
-	}
+	if (!data->got_remote_heads)
+		get_refs_via_connect(transport, 1);

not a new problem, just curious: Does this leak tmp_refs?

Same question as the other caller about whether we mind getting extra_have populated.

Quoted 21 lines · Jonathan Nieder

transport.c static int fetch_refs_via_pack(struct transport *transport, · line 234

Quoted 13 lines · Brandon Williams

+++ b/transport.c

transport.c +2 -6

@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.cloning = transport->cloning;
 	args.update_shallow = data->options.update_shallow;
 
-	if (!data->got_remote_heads) {
-		connect_setup(transport, 0);
-		get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,
-				 NULL, &data->shallow);
-		data->got_remote_heads = 1;
-	}
+	if (!data->got_remote_heads)
+		refs_tmp = get_refs_via_connect(transport, 0);

The only difference between the old and new code is that the old code passes NULL as 'extra_have' and the new code passes &data->extra_have.

That means this populates the data->extra_have oid_array. Does it matter?

I don't think its a problem to have extra_have populated, least I haven't seen anything to lead me to believe it would be a problem.

Quoted 4 lines

Quoted 4 lines

transport.c +0 -0

@@ -541,14 +537,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	struct send_pack_args args;
 	int ret;
 

Quoted 12 lines

transport.c static int git_transport_push(struct transport *transport, struct ref *remote_re · line 541

Quoted 10 lines

transport.c +2 -8

-	if (!data->got_remote_heads) {
-		struct ref *tmp_refs;
-		connect_setup(transport, 1);
-
-		get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,
-				 NULL, &data->shallow);
-		data->got_remote_heads = 1;
-	}
+	if (!data->got_remote_heads)
+		get_refs_via_connect(transport, 1);

not a new problem, just curious: Does this leak tmp_refs?

Maybe, though its removed by this patch.

Quoted 22 lines · Brandon Williams

Quoted 21 lines · Jonathan Nieder

transport.c static int fetch_refs_via_pack(struct transport *transport, · line 234

Quoted 13 lines · Brandon Williams

+++ b/transport.c

transport.c +2 -6

@@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport *transport,
 	args.cloning = transport->cloning;
 	args.update_shallow = data->options.update_shallow;
 
-	if (!data->got_remote_heads) {
-		connect_setup(transport, 0);
-		get_remote_heads(data->fd[0], NULL, 0, &refs_tmp, 0,
-				 NULL, &data->shallow);
-		data->got_remote_heads = 1;
-	}
+	if (!data->got_remote_heads)
+		refs_tmp = get_refs_via_connect(transport, 0);

The only difference between the old and new code is that the old code passes NULL as 'extra_have' and the new code passes &data->extra_have.

That means this populates the data->extra_have oid_array. Does it matter?

[...]

Quoted 2 lines

I don't think its a problem to have extra_have populated, least I haven't seen anything to lead me to believe it would be a problem.

Assuming it gets properly freed later, the only effect I can imagine is some increased memory usage.

I'm inclined to agree with you that the simplicity is worth it. It seems worth mentioning in the commit message, though.

[...]

Quoted 17 lines

Quoted 16 lines

Quoted 4 lines

transport.c +0 -0

@@ -541,14 +537,8 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	struct send_pack_args args;
 	int ret;
 

transport.c static int git_transport_push(struct transport *transport, struct ref *remote_re · line 541

Quoted 10 lines

transport.c +2 -8

-	if (!data->got_remote_heads) {
-		struct ref *tmp_refs;
-		connect_setup(transport, 1);
-
-		get_remote_heads(data->fd[0], NULL, 0, &tmp_refs, REF_NORMAL,
-				 NULL, &data->shallow);
-		data->got_remote_heads = 1;
-	}
+	if (!data->got_remote_heads)
+		get_refs_via_connect(transport, 1);

not a new problem, just curious: Does this leak tmp_refs?

Quoted 1 line

Maybe, though its removed by this patch.

Sorry for the lack of clarity. If it was leaked before, then it is still leaked now, via the discarded return value from get_refs_via_connect.

Any idea how we can track that down? E.g. are there ways to tell leak checkers "just tell me about this particular allocation"?

connect.c +11 -0

@@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)
                      "and the repository exists."));
 }
 
+static enum protocol_version discover_version(struct packet_reader *reader)
+{
+       enum protocol_version version = protocol_unknown_version;
+
+       /*
+        * Peek the first line of the server's response to
+        * determine the protocol version the server is speaking.
+        */
+       switch (packet_reader_peek(reader)) {
+       case PACKET_READ_EOF:
+               die_initial_contact(0);

connect.c +2 -0

+static enum protocol_version discover_version(struct packet_reader *reader)
+{

Quoted 2 lines

connect.c +2 -0

+static enum protocol_version discover_version(struct packet_reader *reader)
+{

...

connect.c static void die_initial_contact(int unexpected) · line 94

Quoted 11 lines

connect.c +11 -0

+
+       /* Maybe process capabilities here, at least for v2 */
+       switch (version) {
+       case protocol_v1:
+               /* Read the peeked version line */
+               packet_reader_read(reader);
+               break;
+       case protocol_v0:
+               break;
+       case protocol_unknown_version:
+               die("unknown protocol version: '%s'\n", reader->line);

The following patches introduce more of the switch(version) cases. And there it actually is a
BUG("protocol version unknown? should have been set in discover_version")
but here it is a mere
die (_("The server uses a different protocol version than we can
speak: %s\n"),

      reader->line);

so I would think here it is reasonable to add _(translation).

Quoted 15 lines · Stefan Beller

Quoted 2 lines

connect.c +2 -0

+static enum protocol_version discover_version(struct packet_reader *reader)
+{

...

connect.c static void die_initial_contact(int unexpected) · line 94

Quoted 11 lines

connect.c +11 -0

+
+       /* Maybe process capabilities here, at least for v2 */
+       switch (version) {
+       case protocol_v1:
+               /* Read the peeked version line */
+               packet_reader_read(reader);
+               break;
+       case protocol_v0:
+               break;
+       case protocol_unknown_version:
+               die("unknown protocol version: '%s'\n", reader->line);

Quoted 8 lines

The following patches introduce more of the switch(version) cases. And there it actually is a
BUG("protocol version unknown? should have been set in discover_version")
but here it is a mere
die (_("The server uses a different protocol version than we can
speak: %s\n"),

      reader->line);

so I would think here it is reasonable to add _(translation).

This should be a BUG as it shouldn't ever be unknown at this point. And I'll also drop that comment.

Quoted 25 lines · Brandon Williams

Quoted 15 lines · Stefan Beller

Quoted 2 lines

connect.c +2 -0

+static enum protocol_version discover_version(struct packet_reader *reader)
+{

...

connect.c static void die_initial_contact(int unexpected) · line 94

Quoted 11 lines

connect.c +11 -0

+
+       /* Maybe process capabilities here, at least for v2 */
+       switch (version) {
+       case protocol_v1:
+               /* Read the peeked version line */
+               packet_reader_read(reader);
+               break;
+       case protocol_v0:
+               break;
+       case protocol_unknown_version:
+               die("unknown protocol version: '%s'\n", reader->line);

Quoted 8 lines

The following patches introduce more of the switch(version) cases. And there it actually is a
BUG("protocol version unknown? should have been set in discover_version")
but here it is a mere
die (_("The server uses a different protocol version than we can
speak: %s\n"),

      reader->line);

so I would think here it is reasonable to add _(translation).

Quoted 2 lines

This should be a BUG as it shouldn't ever be unknown at this point. And I'll also drop that comment.

Huh?
Then I miss-understood the flow of code. When the server announces its answer is version 42, but the client cannot handle it, which die call is responsible for reporting it to the user?
(That is technically a BUG on the server side, as we probably never asked for v42, so I would not want to print BUG locally at the client?)