[v2,0/6,Outreachy] Introduce os-version Capability with Configurable Options

Message

Usman Akinyemi Jan. 17, 2025, 10:46 a.m. UTC

For debugging, statistical analysis, and security purposes, it can
be valuable for Git servers to know the operating system the clients
are using.

For example:
- A server noticing that a client is using an old Git version with
security issues on one platform, like macOS, could verify if the
user is indeed running macOS before sending a message to upgrade."
- Similarly, a server identifying a client that could benefit from
an upgrade (e.g., for performance reasons) could better customize the
message it sends to nudge the client to upgrade.

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.

By default this sends similar info as `git bugreport` is already sending,
which uses uname(2). The difference is that it is sanitized in the same
way as the Git version sent by the 'agent' capability is sanitized
(by replacing characters having an ascii code less than 32 or more
than 127 with '.'). Also, it only sends the result of `uname -s` i.e
just only the operating system name (e.g "Linux").

Due to privacy issues and concerns, let's add the `transfer.advertiseOSVersion`
config option. This boolean option is enabled by default, but allows users to
disable this feature completely by setting it to "false".

To provide flexibility and customization, let also add the `osversion.command`
config option. This allows users to specify a custom command whose output will
be used as the string exchanged via the "os-version" capability. If this option
is not set, the default behavior exchanges only the operating system name,
such as "Linux" or "Windows". This option was particularly suggested by Randall S. Becker
in a previous conversation. You can find the reference here
https://lore.kernel.org/git/000a01dac25c$df7b23e0$9e716ba0$@nexbridge.com/

Note that, due to differences between `uname(1)` (command-line
utility) and `uname(2)` (system call) outputs on Windows,
`transfer.advertiseOSVersion` is set to false on Windows during
testing. See the message part of patch 5/6 for more details.

My mentor, Christian Couder, sent a previous patch series about this
before. You can find it here
https://lore.kernel.org/git/[e-mail]/

Changes since v1
================
  - Refactored documentation for improved clarity.
  - Splitted patch "refactor get_uname_info()" into two patches with first
    part doing refactoring and the second part doing enhancement for code
    clearity and cleanliness.
  - Made test_redact_non_printables() to trim carriage-returns.
  - Fixed outdated commit message.
  - Splitted part of the "test capability advertisement" into a setup"-type
    to remove side-effect dependency.
  - Changed the name of some created files used in testing for better
    clearity of what their content is.
  - Added comment to os_version(), os_version_sanitized() and advertise_os_version()
    for improved clarity of what they do.

Usman Akinyemi (6):
  version: refactor redact_non_printables()
  version: refactor get_uname_info()
  version: extend get_uname_info() to hide system details
  t5701: add setup test to remove side-effect dependency
  connect: advertise OS version
  version: introduce osversion.command config for os-version output

 Documentation/config/transfer.txt |  16 ++++
 Documentation/gitprotocol-v2.txt  |  17 ++++
 builtin/bugreport.c               |  13 +--
 connect.c                         |   3 +
 serve.c                           |  14 ++++
 t/t5555-http-smart-common.sh      |  38 ++++++++-
 t/t5701-git-serve.sh              |  59 ++++++++++++-
 t/test-lib-functions.sh           |   8 ++
 version.c                         | 135 ++++++++++++++++++++++++++++--
 version.h                         |  28 +++++++
 10 files changed, 309 insertions(+), 22 deletions(-)

Range-diff versus v1:

1:  d23091031c ! 1:  97bccab6d5 version: refactor redact_non_printables()
    @@ Commit message
         For now the new redact_non_printables() 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 explicitly detach the string contained by
    -        the 'buf' strbuf.
    +    While at it, let's use strbuf_detach() to explicitly detach the string
    +    contained by the 'buf' strbuf.
     
         Mentored-by: Christian Couder <[e-mail]>
         Signed-off-by: Usman Akinyemi <[e-mail]>
2:  1336622be9 ! 2:  1f8a4024a4 version: refactor get_uname_info()
    @@ Commit message
         Let's refactor this code into a new get_uname_info() function, so
         that we can reuse it in a following commit.
     
    -    We may need to refactor this function in the future if an
    -    `osVersion.format` config option is added, but for now we only
    -    need it to accept a "full" flag that makes it switch between providing
    -    full OS information and providing only the OS name. The mode
    -    providing only the OS name is needed in a following commit
    -
         Mentored-by: Christian Couder <[e-mail]>
         Signed-off-by: Usman Akinyemi <[e-mail]>
     
    @@ builtin/bugreport.c: static void get_system_info(struct strbuf *sys_info)
     -			    uname_info.release,
     -			    uname_info.version,
     -			    uname_info.machine);
    -+	get_uname_info(sys_info, 1);
    ++	get_uname_info(sys_info);
      
      	strbuf_addstr(sys_info, _("compiler info: "));
      	get_compiler_info(sys_info);
    @@ version.c: const char *git_user_agent_sanitized(void)
      	return agent;
      }
     +
    -+int get_uname_info(struct strbuf *buf, unsigned int full)
    ++int get_uname_info(struct strbuf *buf)
     +{
     +	struct utsname uname_info;
     +
    @@ version.c: const char *git_user_agent_sanitized(void)
     +		return -1;
     +	}
     +
    -+	if (full)
    -+		strbuf_addf(buf, "%s %s %s %s\n",
    -+			    uname_info.sysname,
    -+			    uname_info.release,
    -+			    uname_info.version,
    -+			    uname_info.machine);
    -+	else
    -+		strbuf_addf(buf, "%s\n", uname_info.sysname);
    ++	strbuf_addf(buf, "%s %s %s %s\n",
    ++		    uname_info.sysname,
    ++		    uname_info.release,
    ++		    uname_info.version,
    ++		    uname_info.machine);
     +	return 0;
     +}
     
    @@ version.h: extern const char git_built_from_commit_string[];
     +  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, unsigned int full);
    ++int get_uname_info(struct strbuf *buf);
     +
      #endif /* VERSION_H */
-:  ---------- > 3:  962b42702f version: extend get_uname_info() to hide system details
-:  ---------- > 4:  7f0ec75a0d t5701: add setup test to remove side-effect dependency
3:  b90a24813f ! 5:  499eda49cf connect: advertise OS version
    @@ Commit message
         controlled by the new `transfer.advertiseOSVersion` config option.
     
         Add the `transfer.advertiseOSVersion` config option to address
    -    privacy concerns issue. It defaults to `true` and can be changed to
    +    privacy concerns. It defaults to `true` and can be changed to
         `false`. When enabled, this option makes clients and servers send each
         other the OS name (e.g., "Linux" or "Windows"). The information is
         retrieved using the 'sysname' field of the `uname(2)` system call.
    @@ Commit message
           .2024-02-14.20:17.UTC.x86_64
           - `uname(2)` output: Windows.10.0.20348
     
    -    Until a good way to test the feature on Windows is found, the
    -    transfer.advertiseOSVersion is set to false on Windows during testing.
    +    On Windows, uname(2) is not actually system-supplied but is instead
    +    already faked up by Git itself. We could have overcome the test issue
    +    on Windows by implementing a new `uname` subcommand in `test-tool`
    +    using uname(2), but except uname(2), which would be tested against
    +    itself, there would be nothing platform specific, so it's just simpler
    +    to disable the tests on Windows.
     
         Mentored-by: Christian Couder <[e-mail]>
         Signed-off-by: Usman Akinyemi <[e-mail]>
    @@ Documentation/config/transfer.txt: transfer.bundleURI::
     +	When `true`, the `os-version` capability is advertised by clients and
     +	servers. It makes clients and servers send to each other a string
     +	representing the operating system name, like "Linux" or "Windows".
    -+	This string is retrieved from the 'sysname' field of the struct returned
    ++	This string is retrieved from the `sysname` field of the struct returned
     +	by the uname(2) system call. Defaults to true.
     
      ## Documentation/gitprotocol-v2.txt ##
    @@ Documentation/gitprotocol-v2.txt: printable ASCII characters except space (i.e.,
     +~~~~~~~~~~
     +
     +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
    ++advertise the `os-version` capability to notify the client the
    ++kind of operating system it is running on. The client may optionally
    ++send its own `os-version` capability, to notify the server the kind of
    ++operating system it is also running on in its request to the server
    ++(but it MUST NOT do so if the server did not advertise the os-version
    ++capability). The value of this capability may consist of ASCII printable
    ++characters(from 33 to 126 inclusive) and are typically made from the result of
     +`uname -s`(OS name e.g Linux). The os-version capability can be disabled
     +entirely by setting the `transfer.advertiseOSVersion` config option
     +to `false`. The `os-version` strings are purely informative for
    @@ t/t5555-http-smart-common.sh: test_expect_success 'git receive-pack --advertise-
      '
      
      test_expect_success 'git upload-pack --advertise-refs: v2' '
    -+	printf "agent=FAKE" >agent_and_os_name &&
    ++	printf "agent=FAKE" >agent_and_osversion &&
     +	if test_have_prereq WINDOWS
     +	then
    -+		# We do not use test_config here so that any tests below can reuse
    -+		# the "expect" file from this test
     +		git config transfer.advertiseOSVersion false
     +	else
    -+		printf "\nos-version=%s\n" $(uname -s | test_redact_non_printables) >>agent_and_os_name
    ++		printf "\nos-version=%s\n" $(uname -s | test_redact_non_printables) >>agent_and_osversion
     +	fi &&
     +
      	cat >expect <<-EOF &&
      	version 2
     -	agent=FAKE
    -+	$(cat agent_and_os_name)
    ++	$(cat agent_and_osversion)
      	ls-refs=unborn
      	fetch=shallow wait-for-done
      	server-option
     
      ## t/t5701-git-serve.sh ##
    -@@ t/t5701-git-serve.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
    - . ./test-lib.sh
    - 
    - test_expect_success 'test capability advertisement' '
    -+	printf "agent=git/$(git version | cut -d" " -f3)" >agent_and_os_name &&
    +@@ t/t5701-git-serve.sh: test_expect_success 'setup to generate files with expected content' '
    + 	cat >expect.trailer <<-EOF &&
    + 	0000
    + 	EOF
    ++
     +	if test_have_prereq WINDOWS
     +	then
    -+		# We do not use test_config here so that tests below will be able to reuse
    -+		# the expect.base and expect.trailer files
     +		git config transfer.advertiseOSVersion false
     +	else
    -+		printf "\nos-version=%s\n" $(uname -s | test_redact_non_printables) >>agent_and_os_name
    ++		printf "\nos-version=%s\n" $(uname -s | test_redact_non_printables) >>agent_and_osversion
     +	fi &&
     +
    - 	test_oid_cache <<-EOF &&
    - 	wrong_algo sha1:sha256
    - 	wrong_algo sha256:sha1
    ++	cat >expect_osversion.base <<-EOF
    ++	version 2
    ++	$(cat agent_and_osversion)
    ++	ls-refs=unborn
    ++	fetch=shallow wait-for-done
    ++	server-option
    ++	object-format=$(test_oid algo)
    ++	EOF
    + '
    + 
    + test_expect_success 'test capability advertisement' '
    +-	cat expect.base expect.trailer >expect &&
    ++	cat expect_osversion.base expect.trailer >expect &&
    + 
    + 	GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
    + 		--advertise-capabilities >out &&
    +@@ t/t5701-git-serve.sh: test_expect_success 'test capability advertisement with uploadpack.advertiseBund
    + 	cat >expect.extra <<-EOF &&
    + 	bundle-uri
      	EOF
    - 	cat >expect.base <<-EOF &&
    - 	version 2
    --	agent=git/$(git version | cut -d" " -f3)
    -+	$(cat agent_and_os_name)
    - 	ls-refs=unborn
    - 	fetch=shallow wait-for-done
    - 	server-option
    +-	cat expect.base \
    ++	cat expect_osversion.base \
    + 	    expect.extra \
    + 	    expect.trailer >expect &&
    + 
     
      ## t/test-lib-functions.sh ##
     @@ t/test-lib-functions.sh: test_trailing_hash () {
    @@ t/test-lib-functions.sh: test_trailing_hash () {
     +# Octal intervals \001-\040 and \177-\377
     +# corresponds to decimal intervals 1-32 and 127-255
     +test_redact_non_printables () {
    -+    tr -d "\n" | tr "[\001-\040][\177-\377]" "."
    ++    tr -d "\n\r" | tr "[\001-\040][\177-\377]" "."
     +}
     
      ## version.c ##
    @@ version.c
      const char git_version_string[] = GIT_VERSION;
      const char git_built_from_commit_string[] = GIT_BUILT_FROM_COMMIT;
     @@ version.c: int get_uname_info(struct strbuf *buf, unsigned int full)
    - 		strbuf_addf(buf, "%s\n", uname_info.sysname);
    + 	     strbuf_addf(buf, "%s\n", uname_info.sysname);
      	return 0;
      }
     +
    @@ version.h: const char *git_user_agent_sanitized(void);
      */
      int get_uname_info(struct strbuf *buf, unsigned int full);
      
    ++/*
    ++  Retrieve and cache system information for subsequent calls.
    ++  Return a pointer to the cached system information string.
    ++*/
     +const char *os_version(void);
    ++
    ++/*
    ++  Retrieve system information string from os_version(). Then
    ++  sanitize and cache it. Return a pointer to the sanitized
    ++  system information string.
    ++*/
     +const char *os_version_sanitized(void);
    ++
    ++/*
    ++  Retrieve and cache whether os-version capability is enabled.
    ++  Return 1 if enabled, 0 if disabled.
    ++*/
     +int advertise_os_version(struct repository *r);
     +
      #endif /* VERSION_H */
4:  745e63060e ! 6:  a1637dc7cf version: introduce osversion.command config for os-version output
    @@ Commit message
         Let's introduce a new configuration option, `osversion.command`, to handle
         the string exchange between servers and clients. This option allows
         customization of the exchanged string by leveraging the output of the
    -    specified command. If this is not set, the `os-version` capability
    -    exchange just the operating system name.
    +    specified command. This customization might be especially useful on some
    +    quite uncommon platforms like NonStop where interesting OS information is
    +    available from other means than uname(2).
     
    +    If this new configuration option is not set, the `os-version` capability
    +    exchanges just the operating system name.
    +
    +    Helped-by: Randall S. Becker <[e-mail]>
         Mentored-by: Christian Couder <[e-mail]>
         Signed-off-by: Usman Akinyemi <[e-mail]>
     
    @@ Documentation/config/transfer.txt
     @@ Documentation/config/transfer.txt: transfer.advertiseOSVersion::
      	servers. It makes clients and servers send to each other a string
      	representing the operating system name, like "Linux" or "Windows".
    - 	This string is retrieved from the 'sysname' field of the struct returned
    + 	This string is retrieved from the `sysname` field of the struct returned
     -	by the uname(2) system call. Defaults to true.
     +	by the uname(2) system call. If the `osVersion.command` is set, the
     +	output of the command specified will be the string exchanged by the clients
    @@ Documentation/config/transfer.txt: transfer.advertiseOSVersion::
     +	`transfer.advertiseOSVersion` config option.
     
      ## Documentation/gitprotocol-v2.txt ##
    -@@ Documentation/gitprotocol-v2.txt: 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
    +@@ Documentation/gitprotocol-v2.txt: 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 to notify the client the
    +-kind of operating system it is running on. The client may optionally
    +-send its own `os-version` capability, to notify the server the kind of
    +-operating system it is also running on in its request to the server
    +-(but it MUST NOT do so if the server did not advertise the os-version
    +-capability). The value of this capability may consist of ASCII printable
    ++In the same way as the `agent` capability above, the server can advertise
    ++the `os-version` capability to notify the client the kind of operating system
    ++it is running on. The client may optionally send its own `os-version` capability,
    ++to notify the server the kind of operating system it is also running on in its
    ++request to the server (but it MUST NOT do so if the server did not advertise the
    ++os-version capability). The value of this capability may consist of ASCII printable
    + characters(from 33 to 126 inclusive) and are typically made from the result of
     -`uname -s`(OS name e.g Linux). The os-version capability can be disabled
     -entirely by setting the `transfer.advertiseOSVersion` config option
     -to `false`. 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.
    -+`uname -s`(OS name e.g Linux).  If the `osVersion.command` is set,
    -+the `X` and `Y` are made from the ouput of the command specified.
    -+The os-version capability can be disabled entirely by setting the
    -+`transfer.advertiseOSVersion` config option to `false`. 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.
    ++`uname -s`(OS name e.g Linux). If the `osVersion.command` is set, the value of this
    ++capability are made from the ouput of the command specified. The os-version capability
    ++can be disabled entirely by setting the `transfer.advertiseOSVersion` config option
    ++to `false`. 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
      ~~~~~~~
    @@ t/t5555-http-smart-common.sh: test_expect_success 'git upload-pack --advertise-r
      '
      
     +test_expect_success 'git upload-pack --advertise-refs: v2 with osVersion.command config set' '
    -+	# test_config is used here as we are not reusing any file output from here
     +	test_config osVersion.command "uname -srvm" &&
    -+	printf "agent=FAKE" >agent_and_long_os_name &&
    ++	printf "agent=FAKE" >agent_and_long_osversion &&
     +
     +	if test_have_prereq !WINDOWS
     +	then
    -+		printf "\nos-version=%s\n" $(uname -srvm | test_redact_non_printables) >>agent_and_long_os_name
    ++		printf "\nos-version=%s\n" $(uname -srvm | test_redact_non_printables) >>agent_and_long_osversion
     +	fi &&
     +
     +	cat >expect <<-EOF &&
     +	version 2
    -+	$(cat agent_and_long_os_name)
    ++	$(cat agent_and_long_osversion)
     +	ls-refs=unborn
     +	fetch=shallow wait-for-done
     +	server-option
    @@ t/t5701-git-serve.sh: test_expect_success 'test capability advertisement' '
      '
      
     +test_expect_success 'test capability advertisement with osVersion.command config set' '
    -+	# test_config is used here as we are not reusing any file output from here
     +	test_config osVersion.command "uname -srvm" &&
    -+	printf "agent=git/$(git version | cut -d" " -f3)" >agent_and_long_os_name &&
    ++	printf "agent=git/$(git version | cut -d" " -f3)" >agent_and_long_osversion &&
     +
     +	if test_have_prereq !WINDOWS
     +	then
    -+		printf "\nos-version=%s\n" $(uname -srvm | test_redact_non_printables) >>agent_and_long_os_name
    ++		printf "\nos-version=%s\n" $(uname -srvm | test_redact_non_printables) >>agent_and_long_osversion
     +	fi &&
     +
     +	test_oid_cache <<-EOF &&
     +	wrong_algo sha1:sha256
     +	wrong_algo sha256:sha1
     +	EOF
    -+	cat >expect.base_long <<-EOF &&
    ++	cat >expect_long.base <<-EOF &&
     +	version 2
    -+	$(cat agent_and_long_os_name)
    ++	$(cat agent_and_long_osversion)
     +	ls-refs=unborn
     +	fetch=shallow wait-for-done
     +	server-option
     +	object-format=$(test_oid algo)
     +	EOF
    -+	cat >expect.trailer_long <<-EOF &&
    -+	0000
    -+	EOF
    -+	cat expect.base_long expect.trailer_long >expect &&
    ++	cat expect_long.base expect.trailer >expect &&
     +
     +	GIT_TEST_SIDEBAND_ALL=0 test-tool serve-v2 \
     +		--advertise-capabilities >out &&