Support multistage authentication with a Git credential helper #5803
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bk2204
force-pushed
the
credential-authtype-state
branch
2 times, most recently
from
a14e76e
to
82b2acd
Compare
chrisd8088
approved these changes
Jun 19, 2024
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
force-pushed
the
credential-authtype-state
branch
from
82b2acd
to
6783078
Compare
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 booleancontinuefield, 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
statecapability. Additionally, we save and pass back thestate[]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.