From 7c3aee4abe11c180e1db9843f63ced41cb6ac3f3 Mon Sep 17 00:00:00 2001 From: Chris Darroch Date: Sun, 1 Sep 2024 22:51:00 -0700 Subject: [PATCH] t/cmd/git-credential-lfstest.go: allow empty user In commit 8e6641be1c95231673dbd5327736b872ee778d39 of PR #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 8e6641be1c95231673dbd5327736b872ee778d39, 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..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 #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 5914d7da68226d4e9a0ca2bb2795e5b266a9b983 of PR #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 8e6641be1c95231673dbd5327736b872ee778d39 of PR #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. --- t/cmd/git-credential-lfstest.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/t/cmd/git-credential-lfstest.go b/t/cmd/git-credential-lfstest.go index bef2872822..8bb4644c0e 100644 --- a/t/cmd/git-credential-lfstest.go +++ b/t/cmd/git-credential-lfstest.go @@ -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) @@ -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 @@ -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 }