[PATCH 0/3] Advertise OS version

[PATCH 0/3] Advertise OS version

[Christian Couder]

For debugging and statistical purposes, it can be useful for Git
servers to know the OS the client are using.

So let's add a new 'os-version' capability to the v2 protocol, in the
same way as the existing 'agent' capability that lets clients and
servers exchange the Git version they are running.

This sends the same info as `git bugreport` is already sending, which
uses uname(2). It should be the same as what `uname -srvm` returns,
except that it is sanitized in the same way as the Git version sent by
the 'agent' capability is sanitized (by replacing character having an
ascii code less than 32 or more than 127 with '.').

CI tests are currently failing on Windows as it looks like uname(1)
and uname(2) don't report the same thing:

  -os-version=MINGW64_NT-10.0-20348.3.4.10-87d57229.x86_64.2024-02-14.20:17.UTC.x86_64
  +os-version=Windows.10.0.20348

(See: https://github.com/chriscool/git/actions/runs/9581822699)

Thoughts?

Christian Couder (3):
  version: refactor strbuf_sanitize()
  version: refactor get_uname_info()
  connect: advertise OS version

 Documentation/gitprotocol-v2.txt | 18 +++++++++
 builtin/bugreport.c              | 13 +------
 connect.c                        |  3 ++
 serve.c                          | 12 ++++++
 t/t5555-http-smart-common.sh     |  3 ++
 t/t5701-git-serve.sh             |  3 ++
 version.c                        | 67 ++++++++++++++++++++++++++++----
 version.h                        | 10 +++++
 8 files changed, 111 insertions(+), 18 deletions(-)

-- 
2.45.2.563.g6aa460b3cb

[PATCH 1/3] version: refactor strbuf_sanitize()

[Christian Couder]

The git_user_agent_sanitized() function performs some sanitizing to
avoid special characters being sent over the line and possibly messing
up with the protocol or with the parsing on the other side.

Let's extract this sanitizing into a new strbuf_sanitize() function, as
we will want to reuse it in a following patch.

For now the new strbuf_sanitize() function is still static as it's only
needed locally.

While at it, let's also make a few small improvements:
  - use 'size_t' for 'i' instead of 'int',
  - move the declaration of 'i' inside the 'for ( ... )',
  - use strbuf_detach() to explicitely detach the string contained by
    the 'buf' strbuf.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 version.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/version.c b/version.c
index 41b718c29e..331ee6c372 100644
--- a/version.c
+++ b/version.c
@@ -5,6 +5,15 @@
 const char git_version_string[] = GIT_VERSION;
 const char git_built_from_commit_string[] = GIT_BUILT_FROM_COMMIT;
 
+static void strbuf_sanitize(struct strbuf *buf)
+{
+	strbuf_trim(buf);
+	for (size_t i = 0; i < buf->len; i++) {
+		if (buf->buf[i] <= 32 || buf->buf[i] >= 127)
+			buf->buf[i] = '.';
+	}
+}
+
 const char *git_user_agent(void)
 {
 	static const char *agent = NULL;
@@ -24,15 +33,10 @@ const char *git_user_agent_sanitized(void)
 
 	if (!agent) {
 		struct strbuf buf = STRBUF_INIT;
-		int i;
 
 		strbuf_addstr(&buf, git_user_agent());
-		strbuf_trim(&buf);
-		for (i = 0; i < buf.len; i++) {
-			if (buf.buf[i] <= 32 || buf.buf[i] >= 127)
-				buf.buf[i] = '.';
-		}
-		agent = buf.buf;
+		strbuf_sanitize(&buf);
+		agent = strbuf_detach(&buf, NULL);
 	}
 
 	return agent;
-- 
2.45.2.563.g6aa460b3cb

[PATCH 2/3] version: refactor get_uname_info()

[Christian Couder]

Some code from "builtin/bugreport.c" uses uname(2) to get system
information.

Let's refactor this code into a new get_uname_info() function, so
that we can reuse it in a following commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/bugreport.c | 13 ++-----------
 version.c           | 20 ++++++++++++++++++++
 version.h           |  7 +++++++
 3 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index b3cc77af53..b24f876c41 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -11,10 +11,10 @@
 #include "diagnose.h"
 #include "object-file.h"
 #include "setup.h"
+#include "version.h"
 
 static void get_system_info(struct strbuf *sys_info)
 {
-	struct utsname uname_info;
 	char *shell = NULL;
 
 	/* get git version from native cmd */
@@ -23,16 +23,7 @@ static void get_system_info(struct strbuf *sys_info)
 
 	/* system call for other version info */
 	strbuf_addstr(sys_info, "uname: ");
-	if (uname(&uname_info))
-		strbuf_addf(sys_info, _("uname() failed with error '%s' (%d)\n"),
-			    strerror(errno),
-			    errno);
-	else
-		strbuf_addf(sys_info, "%s %s %s %s\n",
-			    uname_info.sysname,
-			    uname_info.release,
-			    uname_info.version,
-			    uname_info.machine);
+	get_uname_info(sys_info);
 
 	strbuf_addstr(sys_info, _("compiler info: "));
 	get_compiler_info(sys_info);
diff --git a/version.c b/version.c
index 331ee6c372..10b9fa77d1 100644
--- a/version.c
+++ b/version.c
@@ -1,6 +1,7 @@
 #include "git-compat-util.h"
 #include "version.h"
 #include "strbuf.h"
+#include "gettext.h"
 
 const char git_version_string[] = GIT_VERSION;
 const char git_built_from_commit_string[] = GIT_BUILT_FROM_COMMIT;
@@ -41,3 +42,22 @@ const char *git_user_agent_sanitized(void)
 
 	return agent;
 }
+
+int get_uname_info(struct strbuf *buf)
+{
+	struct utsname uname_info;
+
+	if (uname(&uname_info)) {
+		strbuf_addf(buf, _("uname() failed with error '%s' (%d)\n"),
+			    strerror(errno),
+			    errno);
+		return -1;
+	}
+
+	strbuf_addf(buf, "%s %s %s %s\n",
+		    uname_info.sysname,
+		    uname_info.release,
+		    uname_info.version,
+		    uname_info.machine);
+	return 0;
+}
diff --git a/version.h b/version.h
index 7c62e80577..afe3dbbab7 100644
--- a/version.h
+++ b/version.h
@@ -7,4 +7,11 @@ extern const char git_built_from_commit_string[];
 const char *git_user_agent(void);
 const char *git_user_agent_sanitized(void);
 
+/*
+  Try to get information about the system using uname(2).
+  Return -1 and put an error message into 'buf' in case of uname()
+  error. Return 0 and put uname info into 'buf' otherwise.
+*/
+int get_uname_info(struct strbuf *buf);
+
 #endif /* VERSION_H */
-- 
2.45.2.563.g6aa460b3cb

[PATCH 3/3] connect: advertise OS version

[Christian Couder]

As some issues that can happen with a Git client can be operating system
specific, it can be useful for a server to know which OS a client is
using. In the same way it can be useful for a client to know which OS
a server is using.

Let's add OS information exchange to the protocol in the same way some
git version exchange is performed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/gitprotocol-v2.txt | 18 ++++++++++++++++++
 connect.c                        |  3 +++
 serve.c                          | 12 ++++++++++++
 t/t5555-http-smart-common.sh     |  3 +++
 t/t5701-git-serve.sh             |  3 +++
 version.c                        | 29 +++++++++++++++++++++++++++++
 version.h                        |  3 +++
 7 files changed, 71 insertions(+)

diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt
index 414bc625d5..f676b2dc7a 100644
--- a/Documentation/gitprotocol-v2.txt
+++ b/Documentation/gitprotocol-v2.txt
@@ -190,6 +190,24 @@ printable ASCII characters except space (i.e., the byte range 32 < x <
 and debugging purposes, and MUST NOT be used to programmatically assume
 the presence or absence of particular features.
 
+os-version
+~~~~~~~~~~
+
+In the same way as the `agent` capability above, the server can
+advertise the `os-version` capability with a value `X` (in the form
+`os-version=X`) to notify the client that the server is running an
+operating system that can be identified by `X`. The client may
+optionally send its own `os-version` string by including the
+`os-version` capability with a value `Y` (in the form `os-version=Y`)
+in its request to the server (but it MUST NOT do so if the server did
+not advertise the os-version capability). The `X` and `Y` strings may
+contain any printable ASCII characters except space (i.e., the byte
+range 32 < x < 127), and are typically made from the result of
+`uname -srvm`. The os-version strings are purely informative for
+statistics and debugging purposes, and MUST NOT be used to
+programmatically assume the presence or absence of particular
+features.
+
 ls-refs
 ~~~~~~~
 
diff --git a/connect.c b/connect.c
index 0d77737a53..3a48806ddc 100644
--- a/connect.c
+++ b/connect.c
@@ -489,6 +489,9 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)
 	if (server_supports_v2("agent"))
 		packet_write_fmt(fd_out, "agent=%s", git_user_agent_sanitized());
 
+	if (server_supports_v2("os-version"))
+		packet_write_fmt(fd_out, "os-version=%s", os_version_sanitized());
+
 	if (server_feature_v2("object-format", &hash_name)) {
 		int hash_algo = hash_algo_by_name(hash_name);
 		if (hash_algo == GIT_HASH_UNKNOWN)
diff --git a/serve.c b/serve.c
index aa651b73e9..77eb5ebdaa 100644
--- a/serve.c
+++ b/serve.c
@@ -29,6 +29,14 @@ static int agent_advertise(struct repository *r UNUSED,
 	return 1;
 }
 
+static int os_version_advertise(struct repository *r UNUSED,
+			   struct strbuf *value)
+{
+	if (value)
+		strbuf_addstr(value, os_version_sanitized());
+	return 1;
+}
+
 static int object_format_advertise(struct repository *r,
 				   struct strbuf *value)
 {
@@ -121,6 +129,10 @@ static struct protocol_capability capabilities[] = {
 		.name = "agent",
 		.advertise = agent_advertise,
 	},
+	{
+		.name = "os-version",
+		.advertise = os_version_advertise,
+	},
 	{
 		.name = "ls-refs",
 		.advertise = ls_refs_advertise,
diff --git a/t/t5555-http-smart-common.sh b/t/t5555-http-smart-common.sh
index 3dcb3340a3..c67739236f 100755
--- a/t/t5555-http-smart-common.sh
+++ b/t/t5555-http-smart-common.sh
@@ -124,9 +124,12 @@ test_expect_success 'git receive-pack --advertise-refs: v1' '
 '
 
 test_expect_success 'git upload-pack --advertise-refs: v2' '
+	# Octal intervals \001-\040 and \177-\377
+	# corresponds to decimal intervals 1-32 and 127-255
 	cat >expect <<-EOF &&
 	version 2
 	agent=FAKE
+	os-version=$(uname -srvm | tr -d "\n" | tr "[\001-\040][\177-\377]" ".")
 	ls-refs=unborn
 	fetch=shallow wait-for-done
 	server-option
diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh
index c48830de8f..9c9a707e6a 100755
--- a/t/t5701-git-serve.sh
+++ b/t/t5701-git-serve.sh
@@ -13,9 +13,12 @@ test_expect_success 'test capability advertisement' '
 	wrong_algo sha1:sha256
 	wrong_algo sha256:sha1
 	EOF
+	# Octal intervals \001-\040 and \177-\377
+	# corresponds to decimal intervals 1-32 and 127-255
 	cat >expect.base <<-EOF &&
 	version 2
 	agent=git/$(git version | cut -d" " -f3)
+	os-version=$(uname -srvm | tr -d "\n" | tr "[\001-\040][\177-\377]" ".")
 	ls-refs=unborn
 	fetch=shallow wait-for-done
 	server-option
diff --git a/version.c b/version.c
index 10b9fa77d1..5b20ea0d7c 100644
--- a/version.c
+++ b/version.c
@@ -61,3 +61,32 @@ int get_uname_info(struct strbuf *buf)
 		    uname_info.machine);
 	return 0;
 }
+
+const char *os_version(void)
+{
+	static const char *os = NULL;
+
+	if (!os) {
+		struct strbuf buf = STRBUF_INIT;
+
+		get_uname_info(&buf);
+		os = strbuf_detach(&buf, NULL);
+	}
+
+	return os;
+}
+
+const char *os_version_sanitized(void)
+{
+	static const char *os_sanitized = NULL;
+
+	if (!os_sanitized) {
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_addstr(&buf, os_version());
+		strbuf_sanitize(&buf);
+		os_sanitized = strbuf_detach(&buf, NULL);
+	}
+
+	return os_sanitized;
+}
diff --git a/version.h b/version.h
index afe3dbbab7..349952c8f2 100644
--- a/version.h
+++ b/version.h
@@ -14,4 +14,7 @@ const char *git_user_agent_sanitized(void);
 */
 int get_uname_info(struct strbuf *buf);
 
+const char *os_version(void);
+const char *os_version_sanitized(void);
+
 #endif /* VERSION_H */
-- 
2.45.2.563.g6aa460b3cb

Re: [PATCH 0/3] Advertise OS version

[Dragan Simic]

Hello Christian,

On 2024-06-19 14:57, Christian Couder wrote:
> For debugging and statistical purposes, it can be useful for Git
> servers to know the OS the client are using.
> 
> So let's add a new 'os-version' capability to the v2 protocol, in the
> same way as the existing 'agent' capability that lets clients and
> servers exchange the Git version they are running.
> 
> This sends the same info as `git bugreport` is already sending, which
> uses uname(2). It should be the same as what `uname -srvm` returns,
> except that it is sanitized in the same way as the Git version sent by
> the 'agent' capability is sanitized (by replacing character having an
> ascii code less than 32 or more than 127 with '.').

This may probably be a useful debugging feature, but I strongly
suggest that a configuration knob exists that makes disabling it
possible.  For security reasons, some users may not want to
publicly advertise their OSes and kernel versions.  Count me in
as one of such users. :)

> CI tests are currently failing on Windows as it looks like uname(1)
> and uname(2) don't report the same thing:
> 
> 
> -os-version=MINGW64_NT-10.0-20348.3.4.10-87d57229.x86_64.2024-02-14.20:17.UTC.x86_64
>   +os-version=Windows.10.0.20348
> 
> (See: https://github.com/chriscool/git/actions/runs/9581822699)
> 
> Thoughts?
> 
> Christian Couder (3):
>   version: refactor strbuf_sanitize()
>   version: refactor get_uname_info()
>   connect: advertise OS version
> 
>  Documentation/gitprotocol-v2.txt | 18 +++++++++
>  builtin/bugreport.c              | 13 +------
>  connect.c                        |  3 ++
>  serve.c                          | 12 ++++++
>  t/t5555-http-smart-common.sh     |  3 ++
>  t/t5701-git-serve.sh             |  3 ++
>  version.c                        | 67 ++++++++++++++++++++++++++++----
>  version.h                        | 10 +++++
>  8 files changed, 111 insertions(+), 18 deletions(-)

Re: [PATCH 0/3] Advertise OS version

[Jeff King]

On Wed, Jun 19, 2024 at 03:18:40PM +0200, Dragan Simic wrote:

> Hello Christian,
> 
> On 2024-06-19 14:57, Christian Couder wrote:
> > For debugging and statistical purposes, it can be useful for Git
> > servers to know the OS the client are using.
> > 
> > So let's add a new 'os-version' capability to the v2 protocol, in the
> > same way as the existing 'agent' capability that lets clients and
> > servers exchange the Git version they are running.
> > 
> > This sends the same info as `git bugreport` is already sending, which
> > uses uname(2). It should be the same as what `uname -srvm` returns,
> > except that it is sanitized in the same way as the Git version sent by
> > the 'agent' capability is sanitized (by replacing character having an
> > ascii code less than 32 or more than 127 with '.').
> 
> This may probably be a useful debugging feature, but I strongly
> suggest that a configuration knob exists that makes disabling it
> possible.  For security reasons, some users may not want to
> publicly advertise their OSes and kernel versions.  Count me in
> as one of such users. :)

Agreed. We do send the Git version, which is already a slight privacy
issue (though it can be overridden at both build-time and run-time). But
OS details seems like crossing a line to me.

I don't mind if this is present but disabled by default, but then I
guess it is not really serving much of a purpose, as hardly anybody
would enable it. Which makes collecting large-scale statistics by
hosting providers pretty much useless (and I don't think it is all that
useful for debugging individual cases).

-Peff

Re: [PATCH 0/3] Advertise OS version

[Dragan Simic]

Hello Jeff,

On 2024-06-19 15:45, Jeff King wrote:
> On Wed, Jun 19, 2024 at 03:18:40PM +0200, Dragan Simic wrote:
>> On 2024-06-19 14:57, Christian Couder wrote:
>> > For debugging and statistical purposes, it can be useful for Git
>> > servers to know the OS the client are using.
>> >
>> > So let's add a new 'os-version' capability to the v2 protocol, in the
>> > same way as the existing 'agent' capability that lets clients and
>> > servers exchange the Git version they are running.
>> >
>> > This sends the same info as `git bugreport` is already sending, which
>> > uses uname(2). It should be the same as what `uname -srvm` returns,
>> > except that it is sanitized in the same way as the Git version sent by
>> > the 'agent' capability is sanitized (by replacing character having an
>> > ascii code less than 32 or more than 127 with '.').
>> 
>> This may probably be a useful debugging feature, but I strongly
>> suggest that a configuration knob exists that makes disabling it
>> possible.  For security reasons, some users may not want to
>> publicly advertise their OSes and kernel versions.  Count me in
>> as one of such users. :)
> 
> Agreed. We do send the Git version, which is already a slight privacy
> issue (though it can be overridden at both build-time and run-time). 
> But
> OS details seems like crossing a line to me.
> 
> I don't mind if this is present but disabled by default, but then I
> guess it is not really serving much of a purpose, as hardly anybody
> would enable it. Which makes collecting large-scale statistics by
> hosting providers pretty much useless (and I don't think it is all that
> useful for debugging individual cases).

I agree that it should actually be disabled by default, for privacy
and security reasons, but that would actually defeat its purpose, so
I'm not really sure should it be merged.

Re: [PATCH 0/3] Advertise OS version

[Christian Couder]

On Wed, Jun 19, 2024 at 3:50 PM Dragan Simic <dsimic@manjaro.org> wrote:

> > I don't mind if this is present but disabled by default, but then I
> > guess it is not really serving much of a purpose, as hardly anybody
> > would enable it. Which makes collecting large-scale statistics by
> > hosting providers pretty much useless (and I don't think it is all that
> > useful for debugging individual cases).
>
> I agree that it should actually be disabled by default, for privacy
> and security reasons, but that would actually defeat its purpose, so
> I'm not really sure should it be merged.

One possibility is to send just the `sysname`, described as 'Operating
system name (e.g., "Linux")', field of the struct utsname filled out
by uname(2) by default.

It should be the same as what `uname -s` prints, so "Linux" for a
Linux machine, and might be acceptable regarding privacy concerns.

And then there might be a knob to deactivate it completely or to make
it more verbose (which might be useful for example in a corporate
context).

Re: [PATCH 0/3] Advertise OS version

[Dragan Simic]

On 2024-06-19 16:01, Christian Couder wrote:
> On Wed, Jun 19, 2024 at 3:50 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
> 
>> > I don't mind if this is present but disabled by default, but then I
>> > guess it is not really serving much of a purpose, as hardly anybody
>> > would enable it. Which makes collecting large-scale statistics by
>> > hosting providers pretty much useless (and I don't think it is all that
>> > useful for debugging individual cases).
>> 
>> I agree that it should actually be disabled by default, for privacy
>> and security reasons, but that would actually defeat its purpose, so
>> I'm not really sure should it be merged.
> 
> One possibility is to send just the `sysname`, described as 'Operating
> system name (e.g., "Linux")', field of the struct utsname filled out
> by uname(2) by default.
> 
> It should be the same as what `uname -s` prints, so "Linux" for a
> Linux machine, and might be acceptable regarding privacy concerns.
> 
> And then there might be a knob to deactivate it completely or to make
> it more verbose (which might be useful for example in a corporate
> context).

I'd be fine with advertising "Linux" (or "Windows") only by default,
because it doesn't reveal much from the privacy and security standpoint,
but allows rather usable statistics to be collected.

A configuration knob that would allow it to be disabled entirely, or
be enabled with more details to be sent would also be fine with me.

RE: [PATCH 0/3] Advertise OS version

[rsbecker]

On Wednesday, June 19, 2024 10:20 AM, Dragan Simic wrote:
>On 2024-06-19 16:01, Christian Couder wrote:
>> On Wed, Jun 19, 2024 at 3:50 PM Dragan Simic <dsimic@manjaro.org>
>> wrote:
>>
>>> > I don't mind if this is present but disabled by default, but then I
>>> > guess it is not really serving much of a purpose, as hardly anybody
>>> > would enable it. Which makes collecting large-scale statistics by
>>> > hosting providers pretty much useless (and I don't think it is all
>>> > that useful for debugging individual cases).
>>>
>>> I agree that it should actually be disabled by default, for privacy
>>> and security reasons, but that would actually defeat its purpose, so
>>> I'm not really sure should it be merged.
>>
>> One possibility is to send just the `sysname`, described as 'Operating
>> system name (e.g., "Linux")', field of the struct utsname filled out
>> by uname(2) by default.
>>
>> It should be the same as what `uname -s` prints, so "Linux" for a
>> Linux machine, and might be acceptable regarding privacy concerns.
>>
>> And then there might be a knob to deactivate it completely or to make
>> it more verbose (which might be useful for example in a corporate
>> context).
>
>I'd be fine with advertising "Linux" (or "Windows") only by default, because it
>doesn't reveal much from the privacy and security standpoint, but allows rather
>usable statistics to be collected.
>
>A configuration knob that would allow it to be disabled entirely, or be enabled with
>more details to be sent would also be fine with me.

While in the code, can I suggest including the OpenSSL version used in the build? This came up in at a customer a few weeks ago and they could not answer the question of what git build they were using. Turned out it used the wrong OpenSSL header compared to what they had installed.

Re: [PATCH 0/3] Advertise OS version

[Jeff King]

On Wed, Jun 19, 2024 at 04:01:57PM +0200, Christian Couder wrote:

> On Wed, Jun 19, 2024 at 3:50 PM Dragan Simic <dsimic@manjaro.org> wrote:
> 
> > > I don't mind if this is present but disabled by default, but then I
> > > guess it is not really serving much of a purpose, as hardly anybody
> > > would enable it. Which makes collecting large-scale statistics by
> > > hosting providers pretty much useless (and I don't think it is all that
> > > useful for debugging individual cases).
> >
> > I agree that it should actually be disabled by default, for privacy
> > and security reasons, but that would actually defeat its purpose, so
> > I'm not really sure should it be merged.
> 
> One possibility is to send just the `sysname`, described as 'Operating
> system name (e.g., "Linux")', field of the struct utsname filled out
> by uname(2) by default.

That would be better to me. I still don't love it, but I admit it's
coming more from a knee-jerk response than from some rational argument
against people knowing I run Linux.

Since HTTP user-agent fields are common, we can look at those for prior
art. curl sends its own version but nothing else. Most browsers do seem
to include some OS information. My version of firefox gives its own
version along with "Linux x86_64". So basically "uname -sm".

> And then there might be a knob to deactivate it completely or to make
> it more verbose (which might be useful for example in a corporate
> context).

Yes, I think we should definitely have an option to suppress or override
it, just like we do for the user-agent string.

-Peff

Re: [PATCH 0/3] Advertise OS version

[Jeff King]

On Wed, Jun 19, 2024 at 10:41:54AM -0400, rsbecker@nexbridge.com wrote:

> >A configuration knob that would allow it to be disabled entirely, or
> >be enabled with more details to be sent would also be fine with me.
> 
> While in the code, can I suggest including the OpenSSL version used in
> the build? This came up in at a customer a few weeks ago and they
> could not answer the question of what git build they were using.
> Turned out it used the wrong OpenSSL header compared to what they had
> installed.

At the point you are dealing one-on-one with somebody, I don't think
protocol-level messages like this are the best spot for debugging. But
it might make sense to teach "git version --build-options" to report the
openssl version.

-Peff

RE: [PATCH 0/3] Advertise OS version

[rsbecker]

On Wednesday, June 19, 2024 10:53 AM, Peff wrote:
>On Wed, Jun 19, 2024 at 10:41:54AM -0400, rsbecker@nexbridge.com wrote:
>
>> >A configuration knob that would allow it to be disabled entirely, or
>> >be enabled with more details to be sent would also be fine with me.
>>
>> While in the code, can I suggest including the OpenSSL version used in
>> the build? This came up in at a customer a few weeks ago and they
>> could not answer the question of what git build they were using.
>> Turned out it used the wrong OpenSSL header compared to what they had
>> installed.
>
>At the point you are dealing one-on-one with somebody, I don't think protocol-level
>messages like this are the best spot for debugging. But it might make sense to teach
>"git version --build-options" to report the openssl version.

Much better idea. Will look into it. Thanks.

RE: [PATCH 0/3] Advertise OS version

[rsbecker]

On Wednesday, June 19, 2024 10:51 AM, Peff wrote:
>On Wed, Jun 19, 2024 at 04:01:57PM +0200, Christian Couder wrote:
>
>> On Wed, Jun 19, 2024 at 3:50 PM Dragan Simic <dsimic@manjaro.org> wrote:
>>
>> > > I don't mind if this is present but disabled by default, but then
>> > > I guess it is not really serving much of a purpose, as hardly
>> > > anybody would enable it. Which makes collecting large-scale
>> > > statistics by hosting providers pretty much useless (and I don't
>> > > think it is all that useful for debugging individual cases).
>> >
>> > I agree that it should actually be disabled by default, for privacy
>> > and security reasons, but that would actually defeat its purpose, so
>> > I'm not really sure should it be merged.
>>
>> One possibility is to send just the `sysname`, described as 'Operating
>> system name (e.g., "Linux")', field of the struct utsname filled out
>> by uname(2) by default.
>
>That would be better to me. I still don't love it, but I admit it's coming more from a
>knee-jerk response than from some rational argument against people knowing I run
>Linux.
>
>Since HTTP user-agent fields are common, we can look at those for prior art. curl
>sends its own version but nothing else. Most browsers do seem to include some OS
>information. My version of firefox gives its own version along with "Linux x86_64".
>So basically "uname -sm".
>
>> And then there might be a knob to deactivate it completely or to make
>> it more verbose (which might be useful for example in a corporate
>> context).
>
>Yes, I think we should definitely have an option to suppress or override it, just like
>we do for the user-agent string.

Instead of an override, what about a knob that specifies the uname command to use to build the value. Personally, I would use `uname -s -r -v` on NonStop to get the kernel version used in the build. The difficulty on my platform is that this is not truly useful info. The effective build OS compatibility version is in a #define __L_Series_RVU and __H_Series_RVU, so the knob might be needed in git_compat_util.h or similar. This comes from the compiler arguments, which are not yet captured.

Re: [PATCH 0/3] Advertise OS version

[Dragan Simic]

On 2024-06-19 16:41, rsbecker@nexbridge.com wrote:
> On Wednesday, June 19, 2024 10:20 AM, Dragan Simic wrote:
>> On 2024-06-19 16:01, Christian Couder wrote:
>>> On Wed, Jun 19, 2024 at 3:50 PM Dragan Simic <dsimic@manjaro.org>
>>> wrote:
>>> 
>>>> > I don't mind if this is present but disabled by default, but then I
>>>> > guess it is not really serving much of a purpose, as hardly anybody
>>>> > would enable it. Which makes collecting large-scale statistics by
>>>> > hosting providers pretty much useless (and I don't think it is all
>>>> > that useful for debugging individual cases).
>>>> 
>>>> I agree that it should actually be disabled by default, for privacy
>>>> and security reasons, but that would actually defeat its purpose, so
>>>> I'm not really sure should it be merged.
>>> 
>>> One possibility is to send just the `sysname`, described as 
>>> 'Operating
>>> system name (e.g., "Linux")', field of the struct utsname filled out
>>> by uname(2) by default.
>>> 
>>> It should be the same as what `uname -s` prints, so "Linux" for a
>>> Linux machine, and might be acceptable regarding privacy concerns.
>>> 
>>> And then there might be a knob to deactivate it completely or to make
>>> it more verbose (which might be useful for example in a corporate
>>> context).
>> 
>> I'd be fine with advertising "Linux" (or "Windows") only by default, 
>> because it
>> doesn't reveal much from the privacy and security standpoint, but 
>> allows rather
>> usable statistics to be collected.
>> 
>> A configuration knob that would allow it to be disabled entirely, or 
>> be enabled with
>> more details to be sent would also be fine with me.
> 
> While in the code, can I suggest including the OpenSSL version used in
> the build? This came up in at a customer a few weeks ago and they
> could not answer the question of what git build they were using.
> Turned out it used the wrong OpenSSL header compared to what they had
> installed.

Makes sense to me, but only in the non-default "advertise more details"
mode of the new configuration knob.

Re: [PATCH 1/3] version: refactor strbuf_sanitize()

[Eric Sunshine]

On Wed, Jun 19, 2024 at 8:57 AM Christian Couder
<christian.couder@gmail.com> wrote:
> The git_user_agent_sanitized() function performs some sanitizing to
> avoid special characters being sent over the line and possibly messing
> up with the protocol or with the parsing on the other side.
>
> Let's extract this sanitizing into a new strbuf_sanitize() function, as
> we will want to reuse it in a following patch.
>
> For now the new strbuf_sanitize() function is still static as it's only
> needed locally.
>
> While at it, let's also make a few small improvements:
>   - use 'size_t' for 'i' instead of 'int',
>   - move the declaration of 'i' inside the 'for ( ... )',
>   - use strbuf_detach() to explicitely detach the string contained by
>     the 'buf' strbuf.

s/explicitely/explicitly/

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

Re: [PATCH 0/3] Advertise OS version

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

On 2024-06-19 at 14:50:42, Jeff King wrote:
> On Wed, Jun 19, 2024 at 04:01:57PM +0200, Christian Couder wrote:
> 
> > One possibility is to send just the `sysname`, described as 'Operating
> > system name (e.g., "Linux")', field of the struct utsname filled out
> > by uname(2) by default.
> 
> That would be better to me. I still don't love it, but I admit it's
> coming more from a knee-jerk response than from some rational argument
> against people knowing I run Linux.
> 
> Since HTTP user-agent fields are common, we can look at those for prior
> art. curl sends its own version but nothing else. Most browsers do seem
> to include some OS information. My version of firefox gives its own
> version along with "Linux x86_64". So basically "uname -sm".

If we choose to enable this, this is the right level of detail, yeah.
We could also allow a distributor to set this value at compile time,
much like Debian does for Postfix and OpenSSH.  For Postfix, it's simply
"(Debian)", which doesn't give much information.

To me as a server administrator interested in statistics, it's useful to
me to know OS name and version (as in, how many users are still using an
ancient version of CentOS?), since that tells me about things like
supported TLS versions which is helpful, but as a user I don't think
that's an appropriate level of detail to share.  And I also worry about
fingerprinting and tracking, which is a giant problem with HTTP
user-agents.  This is especially true if you're using something like
FreeBSD RISC-V, which is just not that common.

> > And then there might be a knob to deactivate it completely or to make
> > it more verbose (which might be useful for example in a corporate
> > context).
> 
> Yes, I think we should definitely have an option to suppress or override
> it, just like we do for the user-agent string.

I definitely think we should have both.  I'm sure we'll have some server
maintainer or repository administrator who tries to reject "bad" OSes
(like someone who doesn't like their employees using WSL, for example).
We've already had people propose to reject access based on the version
number in the name of "security", despite the fact that most Linux
distros just backport security patches and thus the version number is
not usually interesting in that regard.  Again, HTTP user-agents tell us
that people will make access control decisions here even though they
should not.

We'll want to honour people's decisions to remain a mystery or to work
around broken server implementations, or just to make it harder to track
or fingerprint them.

I also think the documentation should state that for the user-agent and
os-version fields that they are merely informative, can be changed, and
MUST NOT be used for access control.  That doesn't mean people will
honour it, but it does mean that we can and should feel free to break
implementations that don't comply.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 0/3] Advertise OS version

[Jeff King]

On Wed, Jun 19, 2024 at 10:46:13PM +0000, brian m. carlson wrote:

> We'll want to honour people's decisions to remain a mystery or to work
> around broken server implementations, or just to make it harder to track
> or fingerprint them.

Yep, I agree with everything you said, especially this part.

> I also think the documentation should state that for the user-agent and
> os-version fields that they are merely informative, can be changed, and
> MUST NOT be used for access control.  That doesn't mean people will
> honour it, but it does mean that we can and should feel free to break
> implementations that don't comply.

I think Christian's proposed documentation did have something along
these lines.

I do kind of wonder if we even need a separate "os-version" field, and
if it couldn't simply be plugged into the user-agent string (making it
"git/1.2.3 Linux x86_64" or something). But maybe that introduces more
hassles with respect to configuring/overriding the two parts separately.

-Peff

Re: [PATCH 0/3] Advertise OS version

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> For debugging and statistical purposes, it can be useful for Git
> servers to know the OS the client are using.
>
> So let's add a new 'os-version' capability to the v2 protocol, in the
> same way as the existing 'agent' capability that lets clients and
> servers exchange the Git version they are running.
>
> This sends the same info as `git bugreport` is already sending, which
> uses uname(2). It should be the same as what `uname -srvm` returns,
> except that it is sanitized in the same way as the Git version sent by
> the 'agent' capability is sanitized (by replacing character having an
> ascii code less than 32 or more than 127 with '.').
>
> CI tests are currently failing on Windows as it looks like uname(1)
> and uname(2) don't report the same thing:
>
>   -os-version=MINGW64_NT-10.0-20348.3.4.10-87d57229.x86_64.2024-02-14.20:17.UTC.x86_64
>   +os-version=Windows.10.0.20348
>
> (See: https://github.com/chriscool/git/actions/runs/9581822699)

I think we already heard from enough people to cover the spectrum of
opinions.  I'd have to say that needs to be carefully kept to the
minimum what we send in the 'user-agent' like manner.  The "git
bugreport" is an opt-in "these should help you in helping me"
feature, designed to allow further redacting by the user before
sending it out, and should not be compared with "on by default for
everybody" telemetry data.

I personally like the idea to add to user-agent, instead of adding a
new capability.  What is the true motivation behind this?  Is this
thing meant to gather statistics from potentially non-paying general
public from hosting providers, or is this primarily for $CORP IT
folks to make sure that nobody is being too stale?

Re: [PATCH 0/3] Advertise OS version

[Usman Akinyemi]

Hello,

Thank you to everyone who participated in this discussion. I am Usman
Akinyemi, one of the two selected Outreachy interns. I have been
selected to work on the project “Finish adding an 'os-version'
capability to Git protocol v2,” which involves implementing the
features discussed in this thread.

You can find the full discussion about my proposal for this project
here: https://public-inbox.org/git/CAPSxiM_rvt-tkQjHYmYNv-Wyr0=X4+123dt=vZKtc++PGRjQMQ@mail.gmail.com/

In summary, this is an outline of my proposal and what I plan to
implement, which has been influenced by the discussion in this thread:

- Send only the OS name by default while allowing a knob (custom
configuration) to specify other information (e.g., version details) and disable
 sending OS names and any other information entirely.

After discussing with my mentor, @Christian, we think that adding
this as a new capability (os-version) is a better option compared to
appending it to the user-agent. This ensures that we do not disrupt
people's scripts that collect statistics from the user-agent or
perform other actions.

Intentions of implementing this project:
- For statistical purposes.
- Most importantly, for security and debugging purposes. This will
allow servers to instruct users to upgrade or perform specific
debugging actions when necessary.

For example:-
A server seeing that a client is using an old Git version
that has security issues on one platform, like MacOS, could check if
the user is indeed running MacOS before sending it a message to
upgrade.

Also a server seeing a client that could benefit from an upgrade, for
example for performance reasons, could better customize the message it
sends to the client to nudge it to upgrade. If the client is on
Windows for example the server could send it a link to
https://gitforwindows.org/ as part of the message.

Please, if anyone has any suggestion or addition or concerns that
might, kindly add. Thank you very much.

Thank you very much!

Usman

RE: [PATCH 0/3] Advertise OS version

[rsbecker]

On December 9, 2024 11:15 AM, Usman Akinyemi wrote:
>Thank you to everyone who participated in this discussion. I am Usman Akinyemi,
>one of the two selected Outreachy interns. I have been selected to work on the
>project “Finish adding an 'os-version'
>capability to Git protocol v2,” which involves implementing the features discussed in
>this thread.
>
>You can find the full discussion about my proposal for this project
>here: https://public-inbox.org/git/CAPSxiM_rvt-tkQjHYmYNv-
>Wyr0=X4+123dt=vZKtc++PGRjQMQ@mail.gmail.com/
>
>In summary, this is an outline of my proposal and what I plan to implement, which
>has been influenced by the discussion in this thread:
>
>- Send only the OS name by default while allowing a knob (custom
>configuration) to specify other information (e.g., version details) and disable
>sending OS names and any other information entirely.
>
>After discussing with my mentor, @Christian, we think that adding this as a new
>capability (os-version) is a better option compared to appending it to the user-
>agent. This ensures that we do not disrupt people's scripts that collect statistics
>from the user-agent or perform other actions.
>
>Intentions of implementing this project:
>- For statistical purposes.
>- Most importantly, for security and debugging purposes. This will allow servers to
>instruct users to upgrade or perform specific debugging actions when necessary.
>
>For example:-
>A server seeing that a client is using an old Git version that has security issues on
>one platform, like MacOS, could check if the user is indeed running MacOS before
>sending it a message to upgrade.
>
>Also a server seeing a client that could benefit from an upgrade, for example for
>performance reasons, could better customize the message it sends to the client to
>nudge it to upgrade. If the client is on Windows for example the server could send it
>a link to https://gitforwindows.org/ as part of the message.
>
>Please, if anyone has any suggestion or addition or concerns that might, kindly add.
>Thank you very much.

Is this build-time or runtime? If run-time, please make sure the code is portable or provides
hooks so that non-linux systems can contribute content.

Thanks,
Randall

--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.

Re: [PATCH 0/3] Advertise OS version

[Usman Akinyemi]

On Mon, Dec 9, 2024 at 10:04 PM <rsbecker@nexbridge.com> wrote:
>
> On December 9, 2024 11:15 AM, Usman Akinyemi wrote:
> >Thank you to everyone who participated in this discussion. I am Usman Akinyemi,
> >one of the two selected Outreachy interns. I have been selected to work on the
> >project “Finish adding an 'os-version'
> >capability to Git protocol v2,” which involves implementing the features discussed in
> >this thread.
> >
> >You can find the full discussion about my proposal for this project
> >here: https://public-inbox.org/git/CAPSxiM_rvt-tkQjHYmYNv-
> >Wyr0=X4+123dt=vZKtc++PGRjQMQ@mail.gmail.com/
> >
> >In summary, this is an outline of my proposal and what I plan to implement, which
> >has been influenced by the discussion in this thread:
> >
> >- Send only the OS name by default while allowing a knob (custom
> >configuration) to specify other information (e.g., version details) and disable
> >sending OS names and any other information entirely.
> >
> >After discussing with my mentor, @Christian, we think that adding this as a new
> >capability (os-version) is a better option compared to appending it to the user-
> >agent. This ensures that we do not disrupt people's scripts that collect statistics
> >from the user-agent or perform other actions.
> >
> >Intentions of implementing this project:
> >- For statistical purposes.
> >- Most importantly, for security and debugging purposes. This will allow servers to
> >instruct users to upgrade or perform specific debugging actions when necessary.
> >
> >For example:-
> >A server seeing that a client is using an old Git version that has security issues on
> >one platform, like MacOS, could check if the user is indeed running MacOS before
> >sending it a message to upgrade.
> >
> >Also a server seeing a client that could benefit from an upgrade, for example for
> >performance reasons, could better customize the message it sends to the client to
> >nudge it to upgrade. If the client is on Windows for example the server could send it
> >a link to https://gitforwindows.org/ as part of the message.
> >
> >Please, if anyone has any suggestion or addition or concerns that might, kindly add.
> >Thank you very much.
>
Hello Randall,

> Is this build-time or runtime? If run-time, please make sure the code is portable or provides
> hooks so that non-linux systems can contribute content.
>
Thanks for pointing this out.

Yeah, the aim is to have it at runtime and to have hooks that can be
used by non-linux systems.

Thank you.
Usman.
> Thanks,
> Randall
>
> --
> Brief whoami: NonStop&UNIX developer since approximately
> UNIX(421664400)
> NonStop(211288444200000000)
> -- In real life, I talk too much.
>
>
>