[v2,07/11] transport: log received server trace2 SID

Commit Message

Josh Steadmon Nov. 2, 2020, 10:31 p.m. UTC

When a client receives a trace2-sid capability from a protocol v0, v1,
or v2 server, log the received session ID via a trace2 data event.

Signed-off-by: Josh Steadmon <[e-mail]>
---
 t/t5705-trace2-sid-in-capabilities.sh | 64 +++++++++++++++++++++++++++
 transport.c                           | 10 +++++
 2 files changed, 74 insertions(+)
 create mode 100755 t/t5705-trace2-sid-in-capabilities.sh

Comments

Junio C Hamano Nov. 4, 2020, 9:14 p.m. UTC | #1

Josh Steadmon <[e-mail]> writes:

> When a client receives a trace2-sid capability from a protocol v0, v1,
> or v2 server, log the received session ID via a trace2 data event.

Would this pose a new security threat surface?  Just wondering if we
want to ignore the capability if it is not enabled on our end with
the configuration.

Thanks.

> diff --git a/transport.c b/transport.c
> index 47da955e4f..d16be597bd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -286,6 +286,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
>  	struct git_transport_data *data = transport->data;
>  	struct ref *refs = NULL;
>  	struct packet_reader reader;
> +	int sid_len;
> +	const char *server_trace2_sid;
>  
>  	connect_setup(transport, for_push);
>  
> @@ -297,6 +299,8 @@ static struct ref *handshake(struct transport *transport, int for_push,
>  	data->version = discover_version(&reader);
>  	switch (data->version) {
>  	case protocol_v2:
> +		if (server_feature_v2("trace2-sid", &server_trace2_sid))
> +			trace2_data_string("trace2", NULL, "server-sid", server_trace2_sid);
>  		if (must_list_refs)
>  			get_remote_refs(data->fd[1], &reader, &refs, for_push,
>  					ref_prefixes,
> @@ -310,6 +314,12 @@ static struct ref *handshake(struct transport *transport, int for_push,
>  				 for_push ? REF_NORMAL : 0,
>  				 &data->extra_have,
>  				 &data->shallow);
> +		server_trace2_sid = server_feature_value("trace2-sid", &sid_len);
> +		if (server_trace2_sid) {
> +			char *server_sid = xstrndup(server_trace2_sid, sid_len);
> +			trace2_data_string("trace2", NULL, "server-sid", server_sid);
> +			free(server_sid);
> +		}
>  		break;
>  	case protocol_unknown_version:
>  		BUG("unknown protocol version");

Josh Steadmon Nov. 11, 2020, 10:53 p.m. UTC | #2

On 2020.11.04 13:14, Junio C Hamano wrote:
> Josh Steadmon <[e-mail]> writes:
> 
> > When a client receives a trace2-sid capability from a protocol v0, v1,
> > or v2 server, log the received session ID via a trace2 data event.
> 
> Would this pose a new security threat surface?  Just wondering if we
> want to ignore the capability if it is not enabled on our end with
> the configuration.
> 
> Thanks.

As Jeff pointed out, the json-writer handles string escapes, so I don't
think we could cause any trouble with the trace2 "event" target. The
"normal" target ignores data events, so this would not show up in a
normal trace2 log. The "perf" target doesn't seem to do any escaping,
but it's intended to be human readable rather than parseable, so I'm not
sure if we need to worry there. Jeff, any thoughts?

Jeff Hostetler Nov. 12, 2020, 9:30 p.m. UTC | #3

On 11/11/20 5:53 PM, Josh Steadmon wrote:
> On 2020.11.04 13:14, Junio C Hamano wrote:
>> Josh Steadmon <[e-mail]> writes:
>>
>>> When a client receives a trace2-sid capability from a protocol v0, v1,
>>> or v2 server, log the received session ID via a trace2 data event.
>>
>> Would this pose a new security threat surface?  Just wondering if we
>> want to ignore the capability if it is not enabled on our end with
>> the configuration.
>>
>> Thanks.
> 
> As Jeff pointed out, the json-writer handles string escapes, so I don't
> think we could cause any trouble with the trace2 "event" target. The
> "normal" target ignores data events, so this would not show up in a
> normal trace2 log. The "perf" target doesn't seem to do any escaping,
> but it's intended to be human readable rather than parseable, so I'm not
> sure if we need to worry there. Jeff, any thoughts?
> 

Only the "event" target prints the SID and it is JSON quoted there.

Neither the "perf" nor "normal" target print them.  The "perf" target
does print the SID "depth" parameter (which is the number of slashes
in the complete SID).

My earlier concerns were about whitespace, CL/LF and other
non-printables in the wire protocol and etc.

Jeff

Patch

diff --git a/t/t5705-trace2-sid-in-capabilities.sh b/t/t5705-trace2-sid-in-capabilities.sh
new file mode 100755
index 0000000000..0870e78f7c
--- /dev/null
+++ b/t/t5705-trace2-sid-in-capabilities.sh
@@ -0,0 +1,64 @@ 
+#!/bin/sh
+
+test_description='trace2 SID in capabilities'
+
+. ./test-lib.sh
+
+REPO="$(pwd)/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for trace2 SID capability tests' '
+	git init "$REPO" &&
+	test_commit -C "$REPO" a &&
+	git clone "file://$REPO" "$LOCAL_PRISTINE" &&
+	test_commit -C "$REPO" b
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "trace2 session IDs not advertised by default (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+
+	test_expect_success "trace2 session IDs not advertised by default (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		test -z "$(grep \"key\":\"server-sid\" tr2-client-events)"
+	'
+done
+
+test_expect_success 'enable SID advertisement' '
+	git -C "$REPO" config trace2.advertiseSID true &&
+	git -C "$LOCAL_PRISTINE" config trace2.advertiseSID true
+'
+
+for PROTO in 0 1 2
+do
+	test_expect_success "trace2 session IDs advertised (fetch v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local fetch origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+
+	test_expect_success "trace2 session IDs advertised (push v${PROTO})" '
+		test_when_finished "rm -rf local tr2-client-events" &&
+		cp -r "$LOCAL_PRISTINE" local &&
+		git -C local pull --no-rebase origin &&
+		GIT_TRACE2_EVENT_NESTING=5 \
+		GIT_TRACE2_EVENT="$(pwd)/tr2-client-events" \
+		git -c protocol.version=$PROTO -C local push origin &&
+		grep \"key\":\"server-sid\" tr2-client-events
+	'
+done
+
+test_done
diff --git a/transport.c b/transport.c
index 47da955e4f..d16be597bd 100644
--- a/transport.c
+++ b/transport.c
@@ -286,6 +286,8 @@  static struct ref *handshake(struct transport *transport, int for_push,
 	struct git_transport_data *data = transport->data;
 	struct ref *refs = NULL;
 	struct packet_reader reader;
+	int sid_len;
+	const char *server_trace2_sid;
 
 	connect_setup(transport, for_push);
 
@@ -297,6 +299,8 @@  static struct ref *handshake(struct transport *transport, int for_push,
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
+		if (server_feature_v2("trace2-sid", &server_trace2_sid))
+			trace2_data_string("trace2", NULL, "server-sid", server_trace2_sid);
 		if (must_list_refs)
 			get_remote_refs(data->fd[1], &reader, &refs, for_push,
 					ref_prefixes,
@@ -310,6 +314,12 @@  static struct ref *handshake(struct transport *transport, int for_push,
 				 for_push ? REF_NORMAL : 0,
 				 &data->extra_have,
 				 &data->shallow);
+		server_trace2_sid = server_feature_value("trace2-sid", &sid_len);
+		if (server_trace2_sid) {
+			char *server_sid = xstrndup(server_trace2_sid, sid_len);
+			trace2_data_string("trace2", NULL, "server-sid", server_sid);
+			free(server_sid);
+		}
 		break;
 	case protocol_unknown_version:
 		BUG("unknown protocol version");