Skip to content

Support multistage authentication with a Git credential helper#5803

Merged
bk2204 merged 7 commits intogit-lfs:mainfrom
bk2204:credential-authtype-state
Jun 26, 2024
Merged

Support multistage authentication with a Git credential helper#5803
bk2204 merged 7 commits intogit-lfs:mainfrom
bk2204:credential-authtype-state

Conversation

@bk2204
Copy link
Member

@bk2204 bk2204 commented Jun 17, 2024

In 2.46, Git will support multistage authentication from credential helpers. This will allow Git and Git LFS to implement functionality like NTLM and Kerberos in the credential helper, which lets this functionality to work even if Git LFS doesn't support it natively.

This requires two separate pieces of data. First, it involves a state[] field, which each credential helper can add to keep track of state. Second, it allows the usage of a boolean continue field, which indicates that the response is multistage and this is not the final stage.

In order to make this work, we adjust a few things. First, we advertise the state capability. Additionally, we save and pass back the state[] fields that the credential helper may send to us. We also don't change the authentication scheme if the helper told us that this was a multistage response. Finally, we add a check to avoid a credential helper getting stuck in an infinite loop if it keeps handing back the same credentials.

There are also a variety of preparatory steps which are necessary and are present in their own commits for easy review.

@bk2204 bk2204 force-pushed the credential-authtype-state branch 2 times, most recently from a14e76e to 82b2acd Compare June 17, 2024 16:26
@bk2204 bk2204 marked this pull request as ready for review June 17, 2024 19:11
@bk2204 bk2204 requested a review from a team as a code owner June 17, 2024 19:11
bk2204 added 7 commits June 24, 2024 19:48
In a future commit, we'll want to use the `slices` package, so let's
bump the required version of Go to 1.21.  This is already the oldest
supported version we test with anyway.
We're going to start processing more complex sets of credentials in the
future, so to keep our code nice and tidy, move the credential handling
into a struct so that we can simplify things.  This allows us to return
only two values from `credsForHostAndPath` instead of the current four,
a number which would only increase in the future.
The new functionality in Git allows persisting state across requests and
providing multistage responses.  To allow testing that, let's implement
the `state[]` and `continue` arguments.

The `state[]` entry contains one or more items of state that are
prefixed by the credential helper to distinguish them from those from
other credential helpers.  These are passed back to the credential
helper on success, failure, or a second round.

The `continue` boolean value indicates whether, upon receiving a 401
response, authentication should continue for another round.  This flag
allows three-legged auth such as NTLM or Kerberos (via either NTLM or
Negotiate schemes) to be successfully implemented by the credential
helper.

Now that we're writing a serialized credential directly into a variable,
let's make sure we copy the relevant pieces from the original credential
structure, including host, path, protocol, and capabilities, so that
these are passed along.  Except for the capabilities, none of these are
strictly needed since Git will fill them in, but it's still nicer to
provide them in the output for completeness.

We document the possible values of a line in the configuration file to
make them easier to understand as well.
In the future, we'll want to parse multiple credentials per file, so
let's move the parsing of a single credential to its own function.
Now that we're implementing multistage authentication, let's allow
multiple credentials in the same file so that we can test the multistage
functionality.  Add some tests to verify that the individual commands
work as expected.
We'd like to support multistage authentication that Git now supports in
credential helpers, so let's add some support in our fake backend
server.  We create a fake authentication scheme, Multistage, that
requires two sets of round trips to successfully authenticate.  If the
appropriate credential is sent in each stage, then succeed; otherwise,
fail.
In 2.46, Git will support multistage authentication from credential
helpers.  This will allow Git and Git LFS to implement functionality
like NTLM and Kerberos in the credential helper, which lets this
functionality to work even if Git LFS doesn't support it natively.

This requires two separate pieces of data.  First, it involves a
`state[]` field, which each credential helper can add to keep track of
state.  Second, it allows the usage of a boolean `continue` field, which
indicates that the response is multistage and this is not the final
stage.

In order to make this work, we adjust a few things.  First, we advertise
the `state` capability.  Additionally, we save and pass back the
`state[]` fields that the credential helper may send to us.  We also
don't change the authentication scheme if the helper told us that this
was a multistage response.  Finally, we add a check to avoid a
credential helper getting stuck in an infinite loop if it keeps handing
back the same credentials.
@bk2204 bk2204 force-pushed the credential-authtype-state branch from 82b2acd to 6783078 Compare June 24, 2024 19:58
@bk2204 bk2204 merged commit 8e36a03 into git-lfs:main Jun 26, 2024
@bk2204 bk2204 deleted the credential-authtype-state branch June 26, 2024 15:31
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 3, 2024
In commit 8e6641b of PR git-lfs#5803 we
refactored our git-credential-lfstest test utility program so as to
use a new internal "credential" structure to store the details of
a given credential, as part of that PR's changes to support multi-stage
authentication in a manner similar to that introduced in Git version
2.46.0.

The git-credential-lfstest program acts as a Git credential helper,
and therefore accepts "get" and "store" command-line arguments along
with input data as described in the Git documentation:

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267

The Serialize() method of our new "credential" structure now encapsulates
the logic that determines which credential fields to output in response
to a command invocation with the "get" argument.  Before commit
8e6641b, this logic was implemented
in the fill() function, which handles "get" requests.

When handling "get" requests for which the "authtype" credential field
was not supplied (and no "authtype" capability was announced by the
caller), and the credential record file read by the helper program
does not indicate a response should be skipped, the current logic of
the Serialize() method only outputs "username" and "password" fields
if they were both found in the record file and are non-empty strings.

However, the "clone ClientCert" test in our t/t-clone.sh script
depends on credential record files we create in the directory identified
by the CREDSDIR variable and which have empty values for the "username"
field.  These are used to supply the password for the encrypted
certificate key file generated by our gitserver-lfstest utility
program.  In particular, when this test is run to completion, it
executes a "git lfs clone" command using the URL on which our
gitserver-lfstest program requires a client TLS/SSL certificate.
We use the http.<url>.ssl{Cert,Key} configuration options to
specify the client certificate files to Git.

To decrypt the certificate, Git makes a request to the configured
credential helper, sending empty "host" and "username" fields,
a "path" field with the path to the certificate file, and a "protocol"
field with the "cert" scheme.  It expects our git-credential-lfstest
helper to then return a "password" field with the passphrase for
the encrypted client certificate file.

Because the Serialize() method in our helper program now only returns
"username" and "password" fields if they are both non-empty, it
returns only the "host", "protocol", and "path" fields, without
a "password" field.  This prevents the test from succeeding, as Git then
requests a password from the terminal since no helper provided one.

Note, though, that due to the issue described in git-lfs#5658, this test does
not actually run to completion at the moment, so we don't experience
this problem in our CI jobs.  We will address this issue in a subsequent
commit in this PR, but as a first step, we need to at least restore the
behaviour of our git-credential-lfstest helper program in the case
where a credential record's file contains an empty "username" field and
a non-empty "password" field.

The original version of this code was introduced in 2015 in commit
5914d7d of PR git-lfs#306; it simply returned
a "username" field if the input fields contained a "username" field,
and the same for the "password" field.  Prior to commit
8e6641b of PR git-lfs#5803 the helper program
had evolved to handle some credential record files with formats
other than simple "username" and "password" fields, but when
returning those fields, continued to function like the original code
in that it would return each field if there was a matching field
among the input key/value pairs.

Neither this approach, nor the current one, actually reflects how Git's
own basic git-credential-store helper functions, nor others such as the
git-credential-osxkeychain helper program.  The git-credential-store
program uses Git's credential_match() function to determine whether
to return "username" and "password" fields for a given request, passing
a zero (false) value for the match_password flag.  For each of the
"protocol", "host", "path", and "username" fields (but not the "password"
field, because match_password is false) the credential_match() function
checks if the field was provided in the request, and if so, that it matches
the corresponding field in the credential record under consideration,
assuming that field is also defined.  Note that these fields may be
defined but empty (i.e., be a pointer to zero-length string).  If all
the possible matches are met, then the function returns a non-zero
(true) value.

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98

The git-credential-osxkeychain helper program is similar in that
if a "username" field is provided, it must match the stored record
before anything is returned, even if both are empty strings.  But if
a "username" field is not provided, both it and the "password" field
will be returned, assuming the other required fields like "host" and
"protocol" match.

To make our git-credential-lfstest helper more accurately reflect
the behaviour of these real credential helpers, and to resolve the
problem with our "clone ClientCert" test and Git's inability to
retrieve the passphrase for our encrypted test TLS/SSL client
certificate, we therefore revise the Serialize() method of our
"credential" Go structure so that before returning a "password" field,
it checks whether a "username" field was provided, and if it was,
that it matches the "username" field of the credential record
(even if both are empty).  As well, if a "username" field was not
provided, then the one in the credential record is returned along with
the "password" field.  Further, we do not require that either the
"username" or "password" fields in the credential record be non-empty.

This brings our test credential helper program into closer alignment
with the actions of Git's own helpers, and again allows a request for
a credential with an empty "username" field in the input to match a
credential record with an empty "username" field.

One consequence of these changes on their own is that we might now
return empty "username" and "password" fields even when the
credential record file does not specify either, but had a leading
non-empty field and so is a record we parse as one with fields
for "authtype" and "credential" data.

For example, if the credential record file contains the line "foo::bar",
then we understand it has having an "authtype" field with the value
"foo" and a "credential" field with the value of "bar".  If a request
is made which lacks "authtype" and "capability[]" fields, but whose
"host" field (and possibly also "path" field) map to this credential
record file, we will now return "host", "username", and "password"
fields, with the latter having two empty values, and nothing else.

This is because the Serialize() method will only return "authtype"
and "credential" fields if the request contains an "authtype" field,
and a "capability[]" field with the value "authtype", and if the
credential record also contains non-empty "authtype" and "credential"
fields.  When these conditions are not met, the method falls through
to the conditional governing whether "username" and "password" fields
are output, and with our new conditions for handling those, we
now allow for a missing "username" input field.  Hence, without
additional changes, we would output empty "username" and "password"
fields in such a case as described above.

To be clear, this is not something which occurs with our test suite
as it stands now.  Neverthless, we should ensure that we only output
"username" and "password" fields if we have parsed the credential
record file as having them.

Therefore we add a check to the final conditional in the Serialze()
method's logic, to ensure that we require an empty "authtype" field
(i.e, a record such as ":foo:bar", with a blank leading field) before
we will output "username" and "password" fields.  In other words, we
must have parsed the credential record file as one containing "username"
and "password" field data before we will send those fields back to
a caller.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 6, 2024
In commit 8e6641b of PR git-lfs#5803 we
refactored our git-credential-lfstest test utility program so as to
use a new internal "credential" structure to store the details of
a given credential, as part of that PR's changes to support multi-stage
authentication in a manner similar to that introduced in Git version
2.46.0.

The git-credential-lfstest program acts as a Git credential helper,
and therefore accepts "get" and "store" command-line arguments along
with input data as described in the Git documentation:

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267

The Serialize() method of our new "credential" structure now encapsulates
the logic that determines which credential fields to output in response
to a command invocation with the "get" argument.  Before commit
8e6641b, this logic was implemented
in the fill() function, which handles "get" requests.

When handling "get" requests for which the "authtype" credential field
was not supplied (and no "authtype" capability was announced by the
caller), and the credential record file read by the helper program
does not indicate a response should be skipped, the current logic of
the Serialize() method only outputs "username" and "password" fields
if they were both found in the record file and are non-empty strings.

However, the "clone ClientCert" test in our t/t-clone.sh script
depends on credential record files we create in the directory identified
by the CREDSDIR variable and which have empty values for the "username"
field.  These are used to supply the password for the encrypted
certificate key file generated by our gitserver-lfstest utility
program.  In particular, when this test is run to completion, it
executes a "git lfs clone" command using the URL on which our
gitserver-lfstest program requires a client TLS/SSL certificate.
We use the http.<url>.ssl{Cert,Key} configuration options to
specify the client certificate files to Git.

To decrypt the certificate, Git makes a request to the configured
credential helper, sending empty "host" and "username" fields,
a "path" field with the path to the certificate file, and a "protocol"
field with the "cert" scheme.  It expects our git-credential-lfstest
helper to then return a "password" field with the passphrase for
the encrypted client certificate file.

Because the Serialize() method in our helper program now only returns
"username" and "password" fields if they are both non-empty, it
returns only the "host", "protocol", and "path" fields, without
a "password" field.  This prevents the test from succeeding, as Git then
requests a password from the terminal since no helper provided one.

Note, though, that due to the issue described in git-lfs#5658, this test does
not actually run to completion at the moment, so we don't experience
this problem in our CI jobs.  We will address this issue in a subsequent
commit in this PR, but as a first step, we need to at least restore the
behaviour of our git-credential-lfstest helper program in the case
where a credential record's file contains an empty "username" field and
a non-empty "password" field.

The original version of this code was introduced in 2015 in commit
5914d7d of PR git-lfs#306; it simply returned
a "username" field if the input fields contained a "username" field,
and the same for the "password" field.  Prior to commit
8e6641b of PR git-lfs#5803 the helper program
had evolved to handle some credential record files with formats
other than simple "username" and "password" fields, but when
returning those fields, continued to function like the original code
in that it would return each field if there was a matching field
among the input key/value pairs.

Neither this approach, nor the current one, actually reflects how Git's
own basic git-credential-store helper functions, nor others such as the
git-credential-osxkeychain helper program.  The git-credential-store
program uses Git's credential_match() function to determine whether
to return "username" and "password" fields for a given request, passing
a zero (false) value for the match_password flag.  For each of the
"protocol", "host", "path", and "username" fields (but not the "password"
field, because match_password is false) the credential_match() function
checks if the field was provided in the request, and if so, that it matches
the corresponding field in the credential record under consideration,
assuming that field is also defined.  Note that these fields may be
defined but empty (i.e., be a pointer to zero-length string).  If all
the possible matches are met, then the function returns a non-zero
(true) value.

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98

The git-credential-osxkeychain helper program is similar in that
if a "username" field is provided, it must match the stored record
before anything is returned, even if both are empty strings.  But if
a "username" field is not provided, both it and the "password" field
will be returned, assuming the other required fields like "host" and
"protocol" match.

To make our git-credential-lfstest helper more accurately reflect
the behaviour of these real credential helpers, and to resolve the
problem with our "clone ClientCert" test and Git's inability to
retrieve the passphrase for our encrypted test TLS/SSL client
certificate, we therefore revise the Serialize() method of our
"credential" Go structure so that before returning a "password" field,
it checks whether a "username" field was provided, and if it was,
that it matches the "username" field of the credential record
(even if both are empty).  As well, if a "username" field was not
provided, then the one in the credential record is returned along with
the "password" field.  Further, we do not require that either the
"username" or "password" fields in the credential record be non-empty.

This brings our test credential helper program into closer alignment
with the actions of Git's own helpers, and again allows a request for
a credential with an empty "username" field in the input to match a
credential record with an empty "username" field.

One consequence of these changes on their own is that we might now
return empty "username" and "password" fields even when the
credential record file does not specify either, but had a leading
non-empty field and so is a record we parse as one with fields
for "authtype" and "credential" data.

For example, if the credential record file contains the line "foo::bar",
then we understand it has having an "authtype" field with the value
"foo" and a "credential" field with the value of "bar".  If a request
is made which lacks "authtype" and "capability[]" fields, but whose
"host" field (and possibly also "path" field) map to this credential
record file, we will now return "host", "username", and "password"
fields, with the latter having two empty values, and nothing else.

This is because the Serialize() method will only return "authtype"
and "credential" fields if the request contains an "authtype" field,
and a "capability[]" field with the value "authtype", and if the
credential record also contains non-empty "authtype" and "credential"
fields.  When these conditions are not met, the method falls through
to the conditional governing whether "username" and "password" fields
are output, and with our new conditions for handling those, we
now allow for a missing "username" input field.  Hence, without
additional changes, we would output empty "username" and "password"
fields in such a case as described above.

To be clear, this is not something which occurs with our test suite
as it stands now.  Neverthless, we should ensure that we only output
"username" and "password" fields if we have parsed the credential
record file as having them.

Therefore we add a check to the final conditional in the Serialze()
method's logic, to ensure that we require an empty "authtype" field
(i.e, a record such as ":foo:bar", with a blank leading field) before
we will output "username" and "password" fields.  In other words, we
must have parsed the credential record file as one containing "username"
and "password" field data before we will send those fields back to
a caller.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 8, 2024
In commit 8e6641b of PR git-lfs#5803 we
refactored our git-credential-lfstest test utility program so as to
use a new internal "credential" structure to store the details of
a given credential, as part of that PR's changes to support multi-stage
authentication in a manner similar to that introduced in Git version
2.46.0.

The git-credential-lfstest program acts as a Git credential helper,
and therefore accepts "get" and "store" command-line arguments along
with input data as described in the Git documentation:

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267

The Serialize() method of our new "credential" structure now encapsulates
the logic that determines which credential fields to output in response
to a command invocation with the "get" argument.  Before commit
8e6641b, this logic was implemented
in the fill() function, which handles "get" requests.

When handling "get" requests for which the "authtype" credential field
was not supplied (and no "authtype" capability was announced by the
caller), and the credential record file read by the helper program
does not indicate a response should be skipped, the current logic of
the Serialize() method only outputs "username" and "password" fields
if they were both found in the record file and are non-empty strings.

However, the "clone ClientCert" test in our t/t-clone.sh script
depends on credential record files we create in the directory identified
by the CREDSDIR variable and which have empty values for the "username"
field.  These are used to supply the password for the encrypted
certificate key file generated by our gitserver-lfstest utility
program.  In particular, when this test is run to completion, it
executes a "git lfs clone" command using the URL on which our
gitserver-lfstest program requires a client TLS/SSL certificate.
We use the http.<url>.ssl{Cert,Key} configuration options to
specify the client certificate files to Git.

To decrypt the certificate, Git makes a request to the configured
credential helper, sending empty "host" and "username" fields,
a "path" field with the path to the certificate file, and a "protocol"
field with the "cert" scheme.  It expects our git-credential-lfstest
helper to then return a "password" field with the passphrase for
the encrypted client certificate file.

Because the Serialize() method in our helper program now only returns
"username" and "password" fields if they are both non-empty, it
returns only the "host", "protocol", and "path" fields, without
a "password" field.  This prevents the test from succeeding, as Git then
requests a password from the terminal since no helper provided one.

Note, though, that due to the issue described in git-lfs#5658, this test does
not actually run to completion at the moment, so we don't experience
this problem in our CI jobs.  We will address this issue in a subsequent
commit in this PR, but as a first step, we need to at least restore the
behaviour of our git-credential-lfstest helper program in the case
where a credential record's file contains an empty "username" field and
a non-empty "password" field.

The original version of this code was introduced in 2015 in commit
5914d7d of PR git-lfs#306; it simply returned
a "username" field if the input fields contained a "username" field,
and the same for the "password" field.  Prior to commit
8e6641b of PR git-lfs#5803 the helper program
had evolved to handle some credential record files with formats
other than simple "username" and "password" fields, but when
returning those fields, continued to function like the original code
in that it would return each field if there was a matching field
among the input key/value pairs.

Neither this approach, nor the current one, actually reflects how Git's
own basic git-credential-store helper functions, nor others such as the
git-credential-osxkeychain helper program.  The git-credential-store
program uses Git's credential_match() function to determine whether
to return "username" and "password" fields for a given request, passing
a zero (false) value for the match_password flag.  For each of the
"protocol", "host", "path", and "username" fields (but not the "password"
field, because match_password is false) the credential_match() function
checks if the field was provided in the request, and if so, that it matches
the corresponding field in the credential record under consideration,
assuming that field is also defined.  Note that these fields may be
defined but empty (i.e., be a pointer to zero-length string).  If all
the possible matches are met, then the function returns a non-zero
(true) value.

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98

The git-credential-osxkeychain helper program is similar in that
if a "username" field is provided, it must match the stored record
before anything is returned, even if both are empty strings.  But if
a "username" field is not provided, both it and the "password" field
will be returned, assuming the other required fields like "host" and
"protocol" match.

To make our git-credential-lfstest helper more accurately reflect
the behaviour of these real credential helpers, and to resolve the
problem with our "clone ClientCert" test and Git's inability to
retrieve the passphrase for our encrypted test TLS/SSL client
certificate, we therefore revise the Serialize() method of our
"credential" Go structure so that before returning a "password" field,
it checks whether a "username" field was provided, and if it was,
that it matches the "username" field of the credential record
(even if both are empty).  As well, if a "username" field was not
provided, then the one in the credential record is returned along with
the "password" field.  Further, we do not require that either the
"username" or "password" fields in the credential record be non-empty.

This brings our test credential helper program into closer alignment
with the actions of Git's own helpers, and again allows a request for
a credential with an empty "username" field in the input to match a
credential record with an empty "username" field.

One consequence of these changes on their own is that we might now
return empty "username" and "password" fields even when the
credential record file does not specify either, but had a leading
non-empty field and so is a record we parse as one with fields
for "authtype" and "credential" data.

For example, if the credential record file contains the line "foo::bar",
then we understand it has having an "authtype" field with the value
"foo" and a "credential" field with the value of "bar".  If a request
is made which lacks "authtype" and "capability[]" fields, but whose
"host" field (and possibly also "path" field) map to this credential
record file, we will now return "host", "username", and "password"
fields, with the latter having two empty values, and nothing else.

This is because the Serialize() method will only return "authtype"
and "credential" fields if the request contains an "authtype" field,
and a "capability[]" field with the value "authtype", and if the
credential record also contains non-empty "authtype" and "credential"
fields.  When these conditions are not met, the method falls through
to the conditional governing whether "username" and "password" fields
are output, and with our new conditions for handling those, we
now allow for a missing "username" input field.  Hence, without
additional changes, we would output empty "username" and "password"
fields in such a case as described above.

To be clear, this is not something which occurs with our test suite
as it stands now.  Neverthless, we should ensure that we only output
"username" and "password" fields if we have parsed the credential
record file as having them.

Therefore we add a check to the final conditional in the Serialze()
method's logic, to ensure that we require an empty "authtype" field
(i.e, a record such as ":foo:bar", with a blank leading field) before
we will output "username" and "password" fields.  In other words, we
must have parsed the credential record file as one containing "username"
and "password" field data before we will send those fields back to
a caller.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Sep 15, 2024
In commit 8e6641b of PR git-lfs#5803 we
refactored our git-credential-lfstest test utility program so as to
use a new internal "credential" structure to store the details of
a given credential, as part of that PR's changes to support multi-stage
authentication in a manner similar to that introduced in Git version
2.46.0.

The git-credential-lfstest program acts as a Git credential helper,
and therefore accepts "get" and "store" command-line arguments along
with input data as described in the Git documentation:

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267

The Serialize() method of our new "credential" structure now encapsulates
the logic that determines which credential fields to output in response
to a command invocation with the "get" argument.  Before commit
8e6641b, this logic was implemented
in the fill() function, which handles "get" requests.

When handling "get" requests for which the "authtype" credential field
was not supplied (and no "authtype" capability was announced by the
caller), and the credential record file read by the helper program
does not indicate a response should be skipped, the current logic of
the Serialize() method only outputs "username" and "password" fields
if they were both found in the record file and are non-empty strings.

However, the "clone ClientCert" test in our t/t-clone.sh script
depends on credential record files we create in the directory identified
by the CREDSDIR variable and which have empty values for the "username"
field.  These are used to supply the password for the encrypted
certificate key file generated by our gitserver-lfstest utility
program.  In particular, when this test is run to completion, it
executes a "git lfs clone" command using the URL on which our
gitserver-lfstest program requires a client TLS/SSL certificate.
We use the http.<url>.ssl{Cert,Key} configuration options to
specify the client certificate files to Git.

To decrypt the certificate, Git makes a request to the configured
credential helper, sending empty "host" and "username" fields,
a "path" field with the path to the certificate file, and a "protocol"
field with the "cert" scheme.  It expects our git-credential-lfstest
helper to then return a "password" field with the passphrase for
the encrypted client certificate file.

Because the Serialize() method in our helper program now only returns
"username" and "password" fields if they are both non-empty, it
returns only the "host", "protocol", and "path" fields, without
a "password" field.  This prevents the test from succeeding, as Git then
requests a password from the terminal since no helper provided one.

Note, though, that due to the issue described in git-lfs#5658, this test does
not actually run to completion at the moment, so we don't experience
this problem in our CI jobs.  We will address this issue in a subsequent
commit in this PR, but as a first step, we need to at least restore the
behaviour of our git-credential-lfstest helper program in the case
where a credential record's file contains an empty "username" field and
a non-empty "password" field.

The original version of this code was introduced in 2015 in commit
5914d7d of PR git-lfs#306; it simply returned
a "username" field if the input fields contained a "username" field,
and the same for the "password" field.  Prior to commit
8e6641b of PR git-lfs#5803 the helper program
had evolved to handle some credential record files with formats
other than simple "username" and "password" fields, but when
returning those fields, continued to function like the original code
in that it would return each field if there was a matching field
among the input key/value pairs.

Neither this approach, nor the current one, actually reflects how Git's
own basic git-credential-store helper functions, nor others such as the
git-credential-osxkeychain helper program.  The git-credential-store
program uses Git's credential_match() function to determine whether
to return "username" and "password" fields for a given request, passing
a zero (false) value for the match_password flag.  For each of the
"protocol", "host", "path", and "username" fields (but not the "password"
field, because match_password is false) the credential_match() function
checks if the field was provided in the request, and if so, that it matches
the corresponding field in the credential record under consideration,
assuming that field is also defined.  Note that these fields may be
defined but empty (i.e., be a pointer to zero-length string).  If all
the possible matches are met, then the function returns a non-zero
(true) value.

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98

The git-credential-osxkeychain helper program is similar in that
if a "username" field is provided, it must match the stored record
before anything is returned, even if both are empty strings.  But if
a "username" field is not provided, both it and the "password" field
will be returned, assuming the other required fields like "host" and
"protocol" match.

To make our git-credential-lfstest helper more accurately reflect
the behaviour of these real credential helpers, and to resolve the
problem with our "clone ClientCert" test and Git's inability to
retrieve the passphrase for our encrypted test TLS/SSL client
certificate, we therefore revise the Serialize() method of our
"credential" Go structure so that before returning a "password" field,
it checks whether a "username" field was provided, and if it was,
that it matches the "username" field of the credential record
(even if both are empty).  As well, if a "username" field was not
provided, then the one in the credential record is returned along with
the "password" field.  Further, we do not require that either the
"username" or "password" fields in the credential record be non-empty.

This brings our test credential helper program into closer alignment
with the actions of Git's own helpers, and again allows a request for
a credential with an empty "username" field in the input to match a
credential record with an empty "username" field.

One consequence of these changes on their own is that we might now
return empty "username" and "password" fields even when the
credential record file does not specify either, but had a leading
non-empty field and so is a record we parse as one with fields
for "authtype" and "credential" data.

For example, if the credential record file contains the line "foo::bar",
then we understand it has having an "authtype" field with the value
"foo" and a "credential" field with the value of "bar".  If a request
is made which lacks "authtype" and "capability[]" fields, but whose
"host" field (and possibly also "path" field) map to this credential
record file, we will now return "host", "username", and "password"
fields, with the latter having two empty values, and nothing else.

This is because the Serialize() method will only return "authtype"
and "credential" fields if the request contains an "authtype" field,
and a "capability[]" field with the value "authtype", and if the
credential record also contains non-empty "authtype" and "credential"
fields.  When these conditions are not met, the method falls through
to the conditional governing whether "username" and "password" fields
are output, and with our new conditions for handling those, we
now allow for a missing "username" input field.  Hence, without
additional changes, we would output empty "username" and "password"
fields in such a case as described above.

To be clear, this is not something which occurs with our test suite
as it stands now.  Neverthless, we should ensure that we only output
"username" and "password" fields if we have parsed the credential
record file as having them.

Therefore we add a check to the final conditional in the Serialze()
method's logic, to ensure that we require an empty "authtype" field
(i.e, a record such as ":foo:bar", with a blank leading field) before
we will output "username" and "password" fields.  In other words, we
must have parsed the credential record file as one containing "username"
and "password" field data before we will send those fields back to
a caller.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 7, 2024
In commit 8e6641b of PR git-lfs#5803 we
refactored our git-credential-lfstest test utility program so as to
use a new internal "credential" structure to store the details of
a given credential, as part of that PR's changes to support multi-stage
authentication in a manner similar to that introduced in Git version
2.46.0.

The git-credential-lfstest program acts as a Git credential helper,
and therefore accepts "get" and "store" command-line arguments along
with input data as described in the Git documentation:

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267

The Serialize() method of our new "credential" structure now encapsulates
the logic that determines which credential fields to output in response
to a command invocation with the "get" argument.  Before commit
8e6641b, this logic was implemented
in the fill() function, which handles "get" requests.

When handling "get" requests for which the "authtype" credential field
was not supplied (and no "authtype" capability was announced by the
caller), and the credential record file read by the helper program
does not indicate a response should be skipped, the current logic of
the Serialize() method only outputs "username" and "password" fields
if they were both found in the record file and are non-empty strings.

However, the "clone ClientCert" test in our t/t-clone.sh script
depends on credential record files we create in the directory identified
by the CREDSDIR variable and which have empty values for the "username"
field.  These are used to supply the password for the encrypted
certificate key file generated by our gitserver-lfstest utility
program.  In particular, when this test is run to completion, it
executes a "git lfs clone" command using the URL on which our
gitserver-lfstest program requires a client TLS/SSL certificate.
We use the http.<url>.ssl{Cert,Key} configuration options to
specify the client certificate files to Git.

To decrypt the certificate, Git makes a request to the configured
credential helper, sending empty "host" and "username" fields,
a "path" field with the path to the certificate file, and a "protocol"
field with the "cert" scheme.  It expects our git-credential-lfstest
helper to then return a "password" field with the passphrase for
the encrypted client certificate file.

Because the Serialize() method in our helper program now only returns
"username" and "password" fields if they are both non-empty, it
returns only the "host", "protocol", and "path" fields, without
a "password" field.  This prevents the test from succeeding, as Git then
requests a password from the terminal since no helper provided one.

Note, though, that due to the issue described in git-lfs#5658, this test does
not actually run to completion at the moment, so we don't experience
this problem in our CI jobs.  We will address this issue in a subsequent
commit in this PR, but as a first step, we need to at least restore the
behaviour of our git-credential-lfstest helper program in the case
where a credential record's file contains an empty "username" field and
a non-empty "password" field.

The original version of this code was introduced in 2015 in commit
5914d7d of PR git-lfs#306; it simply returned
a "username" field if the input fields contained a "username" field,
and the same for the "password" field.  Prior to commit
8e6641b of PR git-lfs#5803 the helper program
had evolved to handle some credential record files with formats
other than simple "username" and "password" fields, but when
returning those fields, continued to function like the original code
in that it would return each field if there was a matching field
among the input key/value pairs.

Neither this approach, nor the current one, actually reflects how Git's
own basic git-credential-store helper functions, nor others such as the
git-credential-osxkeychain helper program.  The git-credential-store
program uses Git's credential_match() function to determine whether
to return "username" and "password" fields for a given request, passing
a zero (false) value for the match_password flag.  For each of the
"protocol", "host", "path", and "username" fields (but not the "password"
field, because match_password is false) the credential_match() function
checks if the field was provided in the request, and if so, that it matches
the corresponding field in the credential record under consideration,
assuming that field is also defined.  Note that these fields may be
defined but empty (i.e., be a pointer to zero-length string).  If all
the possible matches are met, then the function returns a non-zero
(true) value.

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98

The git-credential-osxkeychain helper program is similar in that
if a "username" field is provided, it must match the stored record
before anything is returned, even if both are empty strings.  But if
a "username" field is not provided, both it and the "password" field
will be returned, assuming the other required fields like "host" and
"protocol" match.

To make our git-credential-lfstest helper more accurately reflect
the behaviour of these real credential helpers, and to resolve the
problem with our "clone ClientCert" test and Git's inability to
retrieve the passphrase for our encrypted test TLS/SSL client
certificate, we therefore revise the Serialize() method of our
"credential" Go structure so that before returning a "password" field,
it checks whether a "username" field was provided, and if it was,
that it matches the "username" field of the credential record
(even if both are empty).  As well, if a "username" field was not
provided, then the one in the credential record is returned along with
the "password" field.  Further, we do not require that either the
"username" or "password" fields in the credential record be non-empty.

This brings our test credential helper program into closer alignment
with the actions of Git's own helpers, and again allows a request for
a credential with an empty "username" field in the input to match a
credential record with an empty "username" field.

One consequence of these changes on their own is that we might now
return empty "username" and "password" fields even when the
credential record file does not specify either, but had a leading
non-empty field and so is a record we parse as one with fields
for "authtype" and "credential" data.

For example, if the credential record file contains the line "foo::bar",
then we understand it has having an "authtype" field with the value
"foo" and a "credential" field with the value of "bar".  If a request
is made which lacks "authtype" and "capability[]" fields, but whose
"host" field (and possibly also "path" field) map to this credential
record file, we will now return "host", "username", and "password"
fields, with the latter having two empty values, and nothing else.

This is because the Serialize() method will only return "authtype"
and "credential" fields if the request contains an "authtype" field,
and a "capability[]" field with the value "authtype", and if the
credential record also contains non-empty "authtype" and "credential"
fields.  When these conditions are not met, the method falls through
to the conditional governing whether "username" and "password" fields
are output, and with our new conditions for handling those, we
now allow for a missing "username" input field.  Hence, without
additional changes, we would output empty "username" and "password"
fields in such a case as described above.

To be clear, this is not something which occurs with our test suite
as it stands now.  Neverthless, we should ensure that we only output
"username" and "password" fields if we have parsed the credential
record file as having them.

Therefore we add a check to the final conditional in the Serialze()
method's logic, to ensure that we require an empty "authtype" field
(i.e, a record such as ":foo:bar", with a blank leading field) before
we will output "username" and "password" fields.  In other words, we
must have parsed the credential record file as one containing "username"
and "password" field data before we will send those fields back to
a caller.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 9, 2024
In commit 8e6641b of PR git-lfs#5803 we
refactored our git-credential-lfstest test utility program so as to
use a new internal "credential" structure to store the details of
a given credential, as part of that PR's changes to support multi-stage
authentication in a manner similar to that introduced in Git version
2.46.0.

The git-credential-lfstest program acts as a Git credential helper,
and therefore accepts "get" and "store" command-line arguments along
with input data as described in the Git documentation:

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/gitcredentials.txt#L261-L286
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/Documentation/git-credential.txt#L103-L267

The Serialize() method of our new "credential" structure now encapsulates
the logic that determines which credential fields to output in response
to a command invocation with the "get" argument.  Before commit
8e6641b, this logic was implemented
in the fill() function, which handles "get" requests.

When handling "get" requests for which the "authtype" credential field
was not supplied (and no "authtype" capability was announced by the
caller), and the credential record file read by the helper program
does not indicate a response should be skipped, the current logic of
the Serialize() method only outputs "username" and "password" fields
if they were both found in the record file and are non-empty strings.

However, the "clone ClientCert" test in our t/t-clone.sh script
depends on credential record files we create in the directory identified
by the CREDSDIR variable and which have empty values for the "username"
field.  These are used to supply the password for the encrypted
certificate key file generated by our gitserver-lfstest utility
program.  In particular, when this test is run to completion, it
executes a "git lfs clone" command using the URL on which our
gitserver-lfstest program requires a client TLS/SSL certificate.
We use the http.<url>.ssl{Cert,Key} configuration options to
specify the client certificate files to Git.

To decrypt the certificate, Git makes a request to the configured
credential helper, sending empty "host" and "username" fields,
a "path" field with the path to the certificate file, and a "protocol"
field with the "cert" scheme.  It expects our git-credential-lfstest
helper to then return a "password" field with the passphrase for
the encrypted client certificate file.

Because the Serialize() method in our helper program now only returns
"username" and "password" fields if they are both non-empty, it
returns only the "host", "protocol", and "path" fields, without
a "password" field.  This prevents the test from succeeding, as Git then
requests a password from the terminal since no helper provided one.

Note, though, that due to the issue described in git-lfs#5658, this test does
not actually run to completion at the moment, so we don't experience
this problem in our CI jobs.  We will address this issue in a subsequent
commit in this PR, but as a first step, we need to at least restore the
behaviour of our git-credential-lfstest helper program in the case
where a credential record's file contains an empty "username" field and
a non-empty "password" field.

The original version of this code was introduced in 2015 in commit
5914d7d of PR git-lfs#306; it simply returned
a "username" field if the input fields contained a "username" field,
and the same for the "password" field.  Prior to commit
8e6641b of PR git-lfs#5803 the helper program
had evolved to handle some credential record files with formats
other than simple "username" and "password" fields, but when
returning those fields, continued to function like the original code
in that it would return each field if there was a matching field
among the input key/value pairs.

Neither this approach, nor the current one, actually reflects how Git's
own basic git-credential-store helper functions, nor others such as the
git-credential-osxkeychain helper program.  The git-credential-store
program uses Git's credential_match() function to determine whether
to return "username" and "password" fields for a given request, passing
a zero (false) value for the match_password flag.  For each of the
"protocol", "host", "path", and "username" fields (but not the "password"
field, because match_password is false) the credential_match() function
checks if the field was provided in the request, and if so, that it
matches the corresponding field in the credential record under
consideration, assuming that field is also defined.  Note that these
fields may be defined but empty (i.e., be a pointer to zero-length
string).  If all the possible matches are met, then the function
returns a non-zero (true) value.

  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/builtin/credential-store.c#L34
  https://github.com/git/git/blob/4590f2e9412378c61eac95966709c78766d326ba/credential.c#L87-L98

The git-credential-osxkeychain helper program is similar in that
if a "username" field is provided, it must match the stored record
before anything is returned, even if both are empty strings.  But if
a "username" field is not provided, both it and the "password" field
will be returned, assuming the other required fields like "host" and
"protocol" match.

To make our git-credential-lfstest helper more accurately reflect
the behaviour of these real credential helpers, and to resolve the
problem with our "clone ClientCert" test and Git's inability to
retrieve the passphrase for our encrypted test TLS/SSL client
certificate, we therefore revise the Serialize() method of our
"credential" Go structure so that before returning a "password" field,
it checks whether a "username" field was provided, and if it was,
that it matches the "username" field of the credential record
(even if both are empty).  As well, if a "username" field was not
provided, then the one in the credential record is returned along with
the "password" field.  Further, we do not require that either the
"username" or "password" fields in the credential record be non-empty.

This brings our test credential helper program into closer alignment
with the actions of Git's own helpers, and again allows a request for
a credential with an empty "username" field in the input to match a
credential record with an empty "username" field.

One consequence of these changes on their own is that we might now
return empty "username" and "password" fields even when the
credential record file does not specify either, but had a leading
non-empty field and so is a record we parse as one with fields
for "authtype" and "credential" data.

For example, if the credential record file contains the line "foo::bar",
then we understand it has having an "authtype" field with the value
"foo" and a "credential" field with the value of "bar".  If a request
is made which lacks "authtype" and "capability[]" fields, but whose
"host" field (and possibly also "path" field) map to this credential
record file, we will now return "host", "username", and "password"
fields, with the latter two having empty values, and nothing else.

This is because the Serialize() method will only return "authtype"
and "credential" fields if the request contains an "authtype" field,
and a "capability[]" field with the value "authtype", and if the
credential record also contains non-empty "authtype" and "credential"
fields.  When these conditions are not met, the method falls through
to the conditional governing whether "username" and "password" fields
are output, and with our new conditions for handling those, we
now allow for a missing "username" input field.  Hence, without
additional changes, we would output empty "username" and "password"
fields in such a case as described above.

To be clear, this is not something which occurs with our test suite
as it stands now.  Nevertheless, we should ensure that we only output
"username" and "password" fields if we have parsed the credential
record file as having them.

Therefore we add a check to the final conditional in the Serialize()
method's logic, to ensure that we require an empty "authtype" field
(i.e, a record such as ":foo:bar", with a blank leading field) before
we will output "username" and "password" fields.  In other words, we
must have parsed the credential record file as one containing "username"
and "password" field data before we will send those fields back to
a caller.
chrisd8088 added a commit that referenced this pull request Jan 14, 2025
The Creds structure in our "creds" package was first introduced in
commit e8de7a3, where it was part of
a different package in an early pre-alpha version of the Git LFS client.
A single method was defined for the structure in that commit, the
Buffer() method, which formatted lines of key/value pairs to be sent
to the git-credential(1) command.

In commit 3a271b1 of PR #1839 this
method was converted into the current bufferCreds() function, a change
made to avoid exporting the method out of the "lfsapi" package, where
the structure was defined at that time.

We next moved the Creds structure (which was then a simple map of
strings to strings) and its one function into the current "creds"
package in commit c752dcd of PR #3270.
Later, in commits 5d5f90e of PR #5381
and 6783078 of PR #5803 we revised the
Creds structure so its map contains arrays of strings, and added the
IsMultistage() method, so we again have a single method defined for
this structure.

In subsequent commits in this PR we will refactor the bufferCreds()
function to accept and return additional values.  As a first step, in
order to keep our code as consistent as possible, we convert it back into
a method, as it was originally defined, except with the lowercase name of
buffer() so it is not exported outside of the "creds" package.  This
retains the intent of commit 3a271b1
from PR #1839, but uses a naming pattern that is more idiomatic for the
Go language.

As well, we add a test function named TestCredsBufferFormat() to the
accompanying creds/creds_test.go source file.  This test function simply
verifies the behaviour of the former bufferCreds() function that we have
now renamed to the buffer() method.  To permit comparison of the contents
of the method's output buffer with the lines of key/value pairs expected
by the test function, we also add an assertCredsLinesMatch() helper
function which splits and sorts the lines in the buffer and compares
them with the sorted set of expected output lines.
chrisd8088 added a commit to manturovDan/git-lfs that referenced this pull request Mar 5, 2026
In prior commits in this PR we revised the DoWithAuth() method of the
Client structure in our "lfsapi" package so that it should no longer
enter an infinite loop if a Git LFS API implementation repeatedly
responds to requests with a 401 Unauthorized status code.

To help verify that these changes are effective, we now add a test to
our "t/t-credentials.sh" shell test script.  This test is similar to the
existing "credentials can authenticate with multistage auth" test, which
was introduced along with our support for multi-stage authentication
schemes in PR git-lfs#5803.  Both the existing test and our new test make use
of the example "Multistage" authentication scheme used in our shell
test suite, which also first appeared in PR git-lfs#5803.

Our new test simulates a Git credential helper that is misconfigured or
broken in such a way that after a certain stage, it begins to always
return the same credentials and state.  To establish this intentional
misconfiguration we simply create a credential record file for our
"git-credential-lfstest" utility that contains two entries, one to
define the transition from the initial stage to the "state1" stage,
and another to define a self-referential transition from the "state1"
stage to the same "state1" stage.

Note that the "git-credential-lfstest" utility searches for a matching
entry in its credential record files in a sequential fashion, and that
entries with an empty fourth field (the "MATCH" field) are considered to
match against all states.  As a result, we must place the entry for the
transition from the initial stage to the "state1" stage at the end of
the credential record file, or else it would always match the current
stage.

To help clarify this detail of our test's setup steps we include a
comment which explains why the entry for the first transition must appear
last in the file.  We also take the opportunity to add similar comments
to the two existing tests in the "t/t-credential.sh" script that create
credential record files for multi-stage authentication, as they likewise
place the entry for their first transitions at the end of these files.

(As well, we slightly adjust the whitespace formatting of the existing
"credentials can authenticate with multistage auth" test so that it more
closely resembles our new "credentials with multistage auth loop fails"
test, as well as that of another multi-stage authentication test we expect
to introduce in a subsequent commit in this PR.)

After our new test establishes its misconfigured credential record file,
it performs a "git push" command and then performs the same set of checks
as are found in the "credentials with useHttpPath, with wrong password
and 401 response" test that we added in a prior commit in this PR, plus
one additional check specific to multi-stage authentication.

As we described in that previous commit, we expect the Git LFS client to
make two sets of requests during the "git push" command, one set to the
Locking API and another to the Batch API.  The initial request to the
Locking API is made without an Authorization header, and so no credentials
are retrieved from the local configuration for this request.

Because a 401 status code is received after the initial request, the
client then retrieves the first stage's credential and state from the
"git-credential-lfstest" helper and makes a second request, which also
receives a 401 status code response.  The client then retrieves the next
stage's credential and state from the helper, but due to our intentional
misconfiguration, these are the same as for the first state.

Our "lfstest-gitserver" test utility's skipIfBadAuth() function, which
determines whether to accept or reject the credentials presented in
a request's Authorization header, handles our example "Multistage"
authentication scheme by checking whether the credentials match either
the value "cred1" or the value "cred2".  The "cred2" credential value
will result in the function returning "false" and accepting the
credential, while the "cred1" value causes the function to return "true"
and the utility to respond with a 401 status code.

Since the client's second request to the Locking API also contains the
"cred1" credential, it again receives a 401 status code, which due to the
revisions we made to the DoWithAuth() method in prior commits in this PR
should cause the client to exit from the method and report that the
Locking API is not supported by the Git LFS endpoint.

We then expect the client to make an initial request to the Batch API,
but unlike the first request to the Locking API, an Authorization header
will be included because the access mode required by the Git LFS endpoint
has been cached by the requests to the Locking API.  When this request
receives a 401 status code response, it should then be retried two more
times before the DoWithAuth() method reports an error and exits.

This expected sequence of events explains why our new test checks that
the "git push" command retrieved credentials five times rather than six,
and why the test checks for five instances of the "cred1" credential in
an Authorization header and not six.

Note that we anticipate that we will further refine the behaviour of the
DoWithAuth() method in a subsequent commit in this PR so that requests
without an Authorization header are not counted towards the total number
of attempts to request authentication.
chrisd8088 added a commit to manturovDan/git-lfs that referenced this pull request Mar 5, 2026
In prior commits in this PR we revised the DoWithAuth() method of the
Client structure in our "lfsapi" package so that it should no longer
enter an infinite loop if a Git LFS API implementation repeatedly
responds to requests with a 401 Unauthorized status code.  We have also
added tests which demonstrate that our changes to the DoWithAuth() method
are effective, including when a server only returns 401 status codes.

However, a misbehaving server is not the only condition which might
cause the DoWithAuth() method to perform the maximum allowed number of
requests without successfully authenticating or being rejected.  In
PR git-lfs#5803 we introduced support for multi-stage authentication, in which
a Git credential helper returns a series of credentials and states,
and Git LFS client makes a request for each state using the supplied
credentials.  The final state is expected to complete the authentication
sequence and either be accepted or rejected.

We have already added tests in previous commits in this PR which simulate
a broken or misconfigured credential helper that never returns the final
state in a multi-stage authentication sequence, and instead keeps looping
through the intermediate states.  Both the new "credentials with
multistage auth loop fails" test in our "t/t-credentials.sh" shell script
and the new TestDoWithAuthMultistageRetryLimitExceeded() function in our
Go test suite simulate a credential helper which never advances past an
intermediate authentication state.  These tests demonstrate that our
changes to the DoWithAuth() method ensure it will return after making the
maximum allowed number of requests, even if a multi-stage authentication
sequence is not complete.

This was the intended behaviour of the DoWithAuth() method and the other
Client structure methods at the time when PR git-lfs#5803 was developed.  In
commit 6783078 of that PR we introduced
a "count" variable to the DoWithAuth() method, which the method then
passed to the doWithAuth() method as a pointer.  The doWithAuth() method
incremented the "count" variable after making a multi-stage authentication
request, unless the maximum number of permitted requests had been made,
in which case it was supposed to report that the request was rejected.

The stated intent of this design was "to avoid a credential helper getting
stuck in an infinite loop if it keeps handing back the same credentials",
per the description in commit 6783078.

Unfortunately, this implementation did not work as expected because the
DoWithAuth() method called itself recusively if the current request
received a 401 status code response, and because each invocation of the
method would instantiate a new "count" variable with a zero value.
Thus every time the doWithAuth() method was called it could only ever
increase the variable's value to one, and so it would never find that
the maximum number of allowed requests had been made.

Technically, as we noted in a comment on this PR, a rare combination of
HTTP redirections and 401 status code responses could result in the
"count" variable being incremented past one.  With a sufficient number of
redirections the "count" variable could reach the maximum request limit
and the doWithAuth() method would then inform the credential helper that
the current credentials should be rejected.  However, because the
doWithAuth() method still returned an error indicating that the response
to the most-recent request contained a 401 status code, the DoWithAuth()
method would call itself recursively and the Git LFS client would enter
an infinite loop:

  git-lfs#6018 (comment)

The revisions we have made to the DoWithAuth() and doWithAuth() methods
in this PR should resolve the problems described above and ensure that
misconfigured multi-stage authentication sequences will not cause the
Git LFS client to repeat the same requests without ceasing.

Despite these changes, the client may still exhibit unintended and
unexpected behaviour if a credential helper is configured with a valid
multi-stage authentication sequence, but the number of stages in that
sequence exceeds the maximum number of requests we allow during an
authentication sequence.

In such a case, if the client makes several distinct types of requests
to the same Git LFS endpoint, then the client may improperly continue
the incomplete authentication sequence across the different types of
requests.

In particular, various Git LFS commands start by making requests to
the Locking API of the remote Git LFS endpoint, but Git LFS services are
not required to implement that API.  If these requests fail, the client
just reports that the remote does not support the Locking API and then
proceeds to perform the given command, which may require one or more
requests to the Batch API of the endpoint.

The client may therefore proceed through the initial stages of a long
multi-stage authentication sequence, making requests to the Locking API
of an endpoint, but reach the maximum number of requests we allow
before being either accepted or rejected because the sequence is not
yet complete.  However, if the client then begins to make requests
to the Batch API of the endpoint, it will continue where it left off
in the sequence, and may be able to successfully authenticate.

While that may appear to be beneficial, this behaviour was not our
intention when we added multi-stage authentication support in PR git-lfs#5803.
If we permit the client to continue multi-stage authentication across
different types of requests, the client may behave inconsistently for
users who have identical configurations except for the number of stages
in their authentication sequences.

We therefore update the DoWithAuth() method so that when it returns
after making the maximum allowed number of requests without successfully
authenticating or being rejected, it first resets the current state of
any multi-stage authentication sequence so that any subsequent requests
will restart the sequence from its first stage.  To reset the current
state we simply pass a "nil" parameter to the SetStateFields() method
of the CredentialHelperContext structure from our "creds" package, which
sets the structure's "state" field back to its post-initialization value
of "nil".

To confirm that our changes are effective we then add a new "credentials
with multistage auth above limit fails and resets" test to our
"t/t-credentials.sh" shell test script.  This test establishes a
four-stage authentication sequence by creating a credential record file
for our "git-credential-lfstest" utility that contains four entries,
each of which defines a transition from one stage of four to the next.

For this four-stage authentication sequence to be fully supported by
our "lfstest-gitserver" test utility, we have to modify its
skipIfBadAuth() function, which determines whether to accept or reject
the credentials presented in a request's Authorization header.

Since commit a3429c3 of PR git-lfs#5803, the
skipIfBadAuth() function handles our example "Multistage" authentication
scheme by checking whether the credentials match either the value
"cred1" or the value "cred2".  The "cred2" credential value will result
in the function returning "false" and accepting the credential, while
the "cred1" value causes the function to return "true" and the utility
to respond with a 401 status code.

Several of the existing tests in our "t/t-credentials.sh" script
depend on the "lfstest-gitserver" utility accepting the "cred2" value
in an Authorization header to complete a multi-stage authentication
sequence.  As our new test requires the utility to not accept the first
three of four credentials in a sequence, we alter the skipIfBadAuth()
function so that it expects the values in an Authorization header with
a "Multistage" scheme to match the pattern "cred<m>of<n>".

If a "Multistage" scheme credential matches this pattern and the "<m>"
and "<n>" values are equal, the skipIfBadAuth() function will return
"false", indicating that the credentials are valid; otherwise, it will
return "true" and cause the "lfstest-gitserver" utility to respond
to the request with a 401 status code.

We then update our existing tests in our "t/t-credentials.sh" script
so that they create credential record files whose entries follow the
revised pattern, e.g., "cred1of2" and "cred2of2".

Our new "credentials with multistage auth above limit fails and resets"
test, however, creates entries in its credential record file of the
form "cred1of4", "cred2of4", etc.  Because the defaultMaxAuthAttempts
variable in the "lfsapi" package is assigned a value of three, we allow
at most three requests during an authentication sequence.  By defining
a four-stage authentication sequence for our new test, we guarantee
that each distinct type of request made by a Git LFS command should
reach the three-request limit without successfully authenticating.

Like the other tests we have introduced in previous commits in this PR,
our new test establishes its credential record file and then runs a
"git push" command.  As we described in those commits, we expect the
Git LFS client to make two sets of requests during the "git push"
command, one set to the Locking API and another to the Batch API.

Because the initial request to the Locking API is made without an
Authorization header, we expect that no credentials will be retrieved
from the local configuration for this request.  The client should then
make two more requests to the Locking API, both with credentials,
before reaching the per-sequence limit of three requests.  The client
should then proceed to make three requests to the Batch API, all with
credentials, before again reaching the per-sequence limit.

This expected sequence of events explains why our new test checks that
the "git push" command retrieved credentials five times rather than six,
and why the test checks for two instances each of the "cred1of4" and
"cred2of4" credentials in Authorization headers, but for only one
instance of the "cred3of4" credential and none of the "cred4of4"
credential.

Note that we anticipate that we will further refine the behaviour of the
DoWithAuth() method in a subsequent commit in this PR so that requests
without an Authorization header are not counted towards the total number
of attempts to request authentication.

Further, note that our new test would fail without our change to the
DoWithAuth() method in this commit, because the Git LFS client would
successfully authenticate on its second request to the Batch API.
The client would send the "cred3of4" credential on its first request to
that API, and then send the "cred4of4" credential on its next request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants