Skip to content

Commit

Permalink
t/cmd/git-credential-lfstest.go: allow empty user
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
chrisd8088 committed Sep 5, 2024
1 parent a0621c2 commit 7c3aee4
Showing 1 changed file with 6 additions and 4 deletions.
10 changes: 6 additions & 4 deletions t/cmd/git-credential-lfstest.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type credential struct {
skip bool
}

func (c *credential) Serialize(capabilities map[string]struct{}, state []string) map[string][]string {
func (c *credential) Serialize(capabilities map[string]struct{}, state []string, username []string) map[string][]string {
formattedState := fmt.Sprintf("lfstest:%s", c.state)
formattedMatchState := fmt.Sprintf("lfstest:%s", c.matchState)
creds := make(map[string][]string)
Expand All @@ -51,8 +51,10 @@ func (c *credential) Serialize(capabilities map[string]struct{}, state []string)
}
}
}
} else if len(c.username) != 0 && len(c.password) != 0 {
creds["username"] = []string{c.username}
} else if len(c.authtype) == 0 && (len(username) == 0 || username[0] == c.username) {
if len(username) == 0 {
creds["username"] = []string{c.username}
}
creds["password"] = []string{c.password}
}
return creds
Expand Down Expand Up @@ -115,7 +117,7 @@ func fill() {
result := map[string][]string{}
capas := discoverCapabilities(creds)
for _, cred := range credentials {
result = cred.Serialize(capas, creds["state[]"])
result = cred.Serialize(capas, creds["state[]"], creds["username"])
if len(result) != 0 {
break
}
Expand Down

0 comments on commit 7c3aee4

Please sign in to comment.