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

>
> > 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.