RFC patch, 3 partsAllow empty user name in HTTP authentication

From: Simon Richter <Simon.Richter@hogyros.de>

Hi,

this adds a configuration option to set the authentication method curl uses when authenticating to a server.

The method is always configured, not just when a username is set, to allow failing early if the server does not support the selected method; otherwise, this mechanism is orthogonal to the proxy authentication method handling, and I've liberally copied code from there.

This introduces http.authmethod and remote.<name>.authmethod configuration options and an environment variable GIT_HTTP_AUTHMETHOD, with ascending precedence.

There are three patches in this series, one that just renames a constant list of options as it is used outside the proxy configuration scope now, one that contains the main patch, and one I'm unsure about (hence no Signed-Off-By yet) that allows empty user names during authentication.

The latter avoids surprises when people half-follow Microsoft's documentation, which suggests that users configure a custom header containing a hand-crafted Basic authentication string with an empty user name. This is not strictly required by the "DevOps" server, any string will do here, but simply pressing return on the username prompt will otherwise fail to present the credentials at all, and give an error message indicating that the given token is invalid.

I haven't investigated fully whether this is of any use outside the interactive case, so the third patch is more of a request for comments.

With these changes, I can successfully authenticate to MS DevOps server over HTTP using a Personal Access Token, without using the custom header workaround[1], which allows me to use git-lfs (which in turn doesn't work over ssh) from Jenkins with a limited token that is stored in the Jenkins credential store, solving a problem for approximately tens of users[2].

   Simon

[1] https://docs.microsoft.com/en-us/azure/devops/organizations/accounts/use-personal-access-tokens-to-authenticate?view=azure-devops&tabs=Windows#use-a-pat

[2] https://stackoverflow.com/q/64800010

Simon Richter (3):
  Rename proxy_authmethods -> authmethods
  Add config option/env var to limit HTTP auth methods
  Allow empty user name in HTTP authentication

 Documentation/config/http.txt   | 19 ++++++++++++++
 Documentation/config/remote.txt |  4 +++
 http.c                          | 45 ++++++++++++++++++++++++++++-----
 remote.c                        |  4 +++
 remote.h                        |  3 +++
 5 files changed, 68 insertions(+), 7 deletions(-)

From: Simon Richter <Simon.Richter@hogyros.de>

When using a Personal Access Token in Microsoft DevOps server, the username
can be empty, so users might expect that pressing return on an username
prompt will work.
---
 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 2022-05-13 at 07:04:16, Simon.Richter@hogyros.de wrote:

I don't think this is a good idea. libcurl relies on CURLOPT_USERPWD being set to enable authentication, and before the appearance of http.emptyAuth, it was extremely common for Kerberos users to specify an empty username to get Git to authenticate properly. I probably still have some repositories on my system configured that way.

I believe GitHub can also accept an empty username with a PAT, but it can also accept a dummy (e.g., "token"), which I would hope Azure DevOps can do as well. In such a case, the documentation for Azure DevOps should just be updated to tell people to specify something like "token" or their username.

From: Simon Richter <Simon.Richter@hogyros.de>

Curl also allows specifying a list of acceptable auth methods for the request itself, so this isn't specific to proxy authentication.

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 http.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Simon.Richter@hogyros.de writes:

While that is true, given that it is ONLY used to sanity check the http_proxy_authmethod variable and use CURLOPT_PROXYAUTH thing, the above alone is not a good excuse to rename this array.

I haven't read the later patches, but I would imagine that this is so that you'd create another consumer that is about authentication method that is not for the proxyauth. And if that is the case, the proposed log message for this change should explicitly say so to justify this change.

    We are about to reuse this table of authmethods to parse the
    non-proxy authentication in a later step in this series.  Let's
    rename it to just "authmethod[]".

appended as the second paragraph after the above might be sufficient.

Two things that comes to mind:

 * In general, an array whose elements are accessed individually is
   better named in singular, i.e. "type thing[]", not "type
   things[]" (an exception is when the most prevalent use of the
   array is to pass it as a whole to functions as a bag of things,
   instead of accessing individual element).  This is because it is
   more natural to see the zeroth thing to be spelled "thing[0]",
   and not "things[0]".  If we are renaming this array anyway, it
   may make sense to rename it to authmethod[].

 * If the reason why this rename is warranted is because there will
   be another user of this table that maps a string name to its
   corresponding CURLAUTH_* constant, it would probably make sense
   to extract a helper function out of this loop to do just that,
   something along the lines of ...

   static int parse_authmethod(const char *name, long *auth_param)
   {
       int i;

       for (i = 0; i < ARRAY_SIZE(authmethod); i++)
           if (!strcmp(name, authmethod[i].name)) {
		*auth_param = authmethod[i].curlauth_param;
                return i;
           }
       return -1;
   }

   Then the existing code can become

   if (http_proxy_authmethod) {
        long auth_param;

	if (parse_authmethod(http_proxy_authmethod, &auth_param) < 0) {
	    warning("unsupported ... %s: using anyauth", http_proxy_authmethod);
            auth_param = CURLAUTH_ANY;
	}
	curl_easy_setopt(result, CURLOPT_PROXYAUTH, auth_param);
   }

   It is probably OK to do so in the same patch as renaming of the
   table, but the focus of the step will then become "factor out
   parsing of authmethod from string to CURLAUTH_* constants" and
   the patch should be retitled accordingly.  Then you do not have
   to justify the rename of the table based on the future plan.

The title of a patch in this project follows certain convention. cf. Documentation/SubmittingPatches.

    Subject: [PATCH 1/n] http: factor out parsing of authmethod

    In order to support CURLOPT_PROXYAUTH, there is a code to parse
    the name of an authentication method given as a string into one
    of the CURLAUTH_* constant.  The next step of this series wants
    to reuse the same parser to support CURLOPT_HTTPAUTH in a
    similar way.

    Factor out the loop into a separate helper function.  Since the
    table of authentication methods no longer is only for proxy
    authentication, drop "proxy" prefix from its name while we are
    at it.

or something like that, perhaps.

From: Simon Richter <Simon.Richter@hogyros.de>

This allows forcing an authentication mechanism when the available credentials do not match the automatically selected "best" mechanism.

For example, MS DevOps server supports both NTLM and Basic authentication, but the NTLM backend is connected to the user database only and does not accept Personal Access Tokens; curl however selects NTLM over Basic if both are available.

Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
---
 Documentation/config/http.txt   | 19 +++++++++++++++++++
 Documentation/config/remote.txt |  4 ++++
 http.c                          | 33 ++++++++++++++++++++++++++++++++-
 remote.c                        |  4 ++++
 remote.h                        |  3 +++
 5 files changed, 62 insertions(+), 1 deletion(-)

Simon.Richter@hogyros.de writes:

The above makes sense.

Configuring this variable per URL, just like all other variables in "http.*" namespace, we should be able to use the "http.<url>.*" mechanism that the users are already familiar with.

IOW, this looks out of place.