Re: [PATCH v5 6/6] agent: advertise OS name via agent capability

[Junio C Hamano <gitster@pobox.com>]

Usman Akinyemi <usmanakinyemi202@gmail.com> writes:

> 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.
>
> Our current agent capability is in the form of "package/version" (e.g.,
> "git/1.8.3.1"). Let's extend it to include the operating system name (os)
> i.e in the form "package/version os" (e.g., "git/1.8.3.1 Linux").

Shouldn't this be "git/1.8.3.1-Linux" or something to avoid SP?  The
capability list in protocol v1 is on a single line that is whitespace 
separated (cf. connect.c:parse_feature_value()) without any escape
mechanism.

	Side note.  Does it pose a security hole, when we can set
	agent to any value?  I do not think so, as it controls what
	this end sends to the other.  If you are attacker in control
	of your own agent string to be sent to the other end, and
	use a string with a whitespace in it after "agent=" to claim
	that you support a capability you actually don't, that is
	not a new way to attack the other side available to you---you
	can write your own Git client to talk to the other side to
	send such a bogus capablity list anyway.

> diff --git a/Documentation/gitprotocol-v2.txt b/Documentation/gitprotocol-v2.txt
> index 1652fef3ae..f4831a8787 100644
> --- a/Documentation/gitprotocol-v2.txt
> +++ b/Documentation/gitprotocol-v2.txt
> @@ -184,11 +184,14 @@ form `agent=X`) to notify the client that the server is running version
>  the `agent` capability with a value `Y` (in the form `agent=Y`) in its
>  request to the server (but it MUST NOT do so if the server did not
>  advertise the agent 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 of the form "package/version" (e.g.,
> -"git/1.8.3.1"). The agent strings are purely informative for statistics
> -and debugging purposes, and MUST NOT be used to programmatically assume
> -the presence or absence of particular features.
> +printable ASCII characters (i.e., the byte range 31 < x < 127), and are

Patches 1 & 2 redacted non-printables and SP separately, because SP
is considered printable.  With this change you are allowing SP to be
passed without getting redacted?  I do not think it is a good idea
(see above).

While I'd prefer to keep the range the same as before, i.e. "any
printable ASCII characters except space", "33 <= x <= 126" may be
more readily recognisable that we are doing something unusual, as
"32 <= x <= 126" is fairly easily recognisable as "ASCII printable".

> +typically of the form "package/version os" (e.g., "git/1.8.3.1 Linux")

So, I'd suggest using something other than " " between "version" and
"os".  Dot (as if the byte there were redacted) or slash or dash or
whatever, anything that is not whitespace.

> +where `os` is the operating system name (e.g., "Linux"). `X` and `Y` can
> +be configured using the GIT_USER_AGENT environment variable and it takes
> +priority. The `os` is retrieved using the 'sysname' field of the `uname(2)`
> +system call or its equivalent. The agent strings are purely informative for
> +statistics and debugging purposes, and MUST NOT be used to programmatically
> +assume the presence or absence of particular features.

Other than these nits, I find the above very well done.

As to the additional implementation of git_user_agent_sanitized(),
except for that same "do we really want SP there?" question, I see
nothing questionable there, either.

Overall very nicely done and presented.

Thanks.