add support for git credential helpers #976

dtrifiro · 2022-06-07T12:20:42Z

add CredentialHelper class to run credential helper commands and retrieve values
add get_credentials_from_helper to retrieve credentials from helpers defined in provided config
moved (unused) get_credentials_from_store to credentials.py, refactor it to use CredentialHelper with store helper.

Also see https://www.git-scm.com/docs/gitcredentials

fixes #873

codecov-commenter · 2022-06-07T12:23:49Z

Codecov Report

Merging #976 (ba0fa0c) into master (ba0fa0c) will not change coverage.
The diff coverage is n/a.

❗ Current head ba0fa0c differs from pull request most recent head 452bc9a. Consider uploading reports for the commit 452bc9a to get more accurate results

@@           Coverage Diff           @@
##           master     #976   +/-   ##
=======================================
  Coverage   85.05%   85.05%           
=======================================
  Files          93       93           
  Lines       22901    22901           
  Branches     3359     3359           
=======================================
  Hits        19479    19479           
  Misses       2957     2957           
  Partials      465      465

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

dulwich/client.py

dtrifiro · 2022-06-13T08:57:25Z

Would appreciate if you could take a look @pmrowla

dulwich/credentials.py

pmrowla · 2022-06-15T05:35:23Z

dulwich/credentials.py

+        for line in res.stdout.decode("UTF-8").strip().split("\n"):
+            try:
+                key, value = line.split("=")
+                credentials[key] = value
+            except ValueError:
+                continue


I don't think we should be making assumptions about text encodings here, and just handle credentials as Dict[bytes, bytes] rather than [str, str].

Text encoding for HTTP basic auth username/password is technically undefined (https://www.rfc-editor.org/rfc/rfc7617#appendix-B.3), and depending on the credential manager/system locale settings/etc, I think it's possible that the returned credentials here may not be utf-8 or ascii.

Then later we can just do b64encode(username + b':' + password) where the username/pw are already bytes objects.

Agreed and corrected.

Note though that when username/password kwargs are provided to Urllib3HttpGitClient(), we use urllib3.utils.make_headers:

dulwich/dulwich/client.py

Line 2187 in bb83067

basic_auth = urllib3.util.make_headers(basic_auth=credentials)

which encodes the basic_auth string as latin-1:
https://github.com/urllib3/urllib3/blob/972e9f02cd219892701fa0a1129dda7f81dac535/src/urllib3/util/request.py#L101-L104

In practice though, Imost of the time user/pass will be provided as kwargs when using from_parsedurl and in this case username/password are already decoded through urlparse.

When using creds from a credential manager we should be generating the basic auth header ourselves. (We should be following the RFC, not urllib3)

dulwich/credentials.py

dulwich/client.py

dulwich/tests/test_client.py

jelmer · 2022-06-16T19:40:34Z

dulwich/client.py

+                    self.credentials = self.get_credentials(url)
+                try:
+                    credentials = next(self.credentials)
+                except StopIteration:


won't this keep retrying upon a 404?

It should not, there's a test for that here:
https://github.com/dtrifiro/dulwich/blob/f3ed0104e17c3b9c74952fe62f401008cb590579/dulwich/tests/test_client.py#L1205

jelmer · 2022-06-16T19:43:38Z

dulwich/credentials.py

+
+        argv = self.command.split(" ")
+        if not os.path.isabs(argv[0]):
+            cmd = ["git", f"credential-{argv[0]}"]


Dulwich is meant to be independent of C Git; please don't introduce dependencies on C git.

To clarify the intention here isn't to introduce any dependency on C Git, the expected behavior for 3rd party credential helpers is to follow the executable naming conventions set by C Git:

Generally speaking, rule (3) above is the simplest for users to specify. Authors of credential helpers should make an effort to assist their users by naming their program "git-credential-$NAME", and putting it in the $PATH or $GIT_EXEC_PATH during installation, which will allow a user to enable it with git config credential.helper $NAME.

Although that does mean we probably should just use git-credential-{name} instead of relying on git credential-{name} @dtrifiro

I agree.

The rationale for using git credential-foo instead of git-credential-foo here was that (I'm assuming) most users will be using cgit provided credential helpers (such as git-credential-store, git-credential-osxkeychain, git-credential-gnome-keyring, etc), which are in GIT_EXEC_PATH (which is not in PATH). Using git made sure that GIT_EXEC_PATH was also checked.

Pushed a patch which fixes the issue: if git-credential-foo is not in PATH and git cli is available, GIT_EXEC_PATH is retrieved using git --exec-path and we check if git-credential-foo is available in GIT_EXEC_PATH. If git cli is not available, nothing is done, and calling subprocess.run will just raise FileNotFoundError (which will result in CredentialNotFound exception).

dulwich/credentials.py

jelmer

.

dtrifiro · 2022-06-28T07:22:34Z

Hey @jelmer, this is ready for review btw 🙂

oslobowl · 2022-08-29T10:16:30Z

Hi guys :) Do you know when this branch will be merged, approximately? Thanks a lot :)

jelmer

Sorry for sitting on this for so long; I'm a bit concerned about how this makes the interface for authentication more complicated. Ideally there should just be one pythonic interface for authentication, with perhaps one implementation of that that can interact with C git.

jelmer · 2022-08-12T12:49:00Z

dulwich/credentials.py

+def match_partial_url(valid_url: "ParseResult", partial_url: str) -> bool:
+    """matches a parsed url with a partial url (no scheme/netloc)"""
+    if "://" not in partial_url:
+        parsed = urlparse("scheme://" + partial_url)


Shouldn't this be an error instead?

jelmer · 2022-08-12T12:50:00Z

dulwich/credentials.py

+            (parsed.port and valid_url.port != parsed.port),
+            (
+                parsed.path
+                and not valid_url.path.rstrip("/").startswith(parsed.path.rstrip("/"))


Wouldn't this consider http://example.com/bla to match http://example.com/blah ?

Indeed, fixed.

dulwich/credentials.py

jelmer · 2022-08-12T12:53:04Z

dulwich/credentials.py

+
+from dulwich.config import ConfigDict, StackedConfig, get_xdg_config_home_path
+
+if TYPE_CHECKING:


No need for this as we're already importing urllib.parse

jelmer · 2022-08-12T13:05:57Z

dulwich/credentials.py

+        return cls(command.decode(encoding))
+
+
+def get_credentials_from_store(


Ideally we'd just have one pythonic interface that's implemented by the static credentials files, the helper class and any other python code.

I moved get_credentials_from_store from client to here since it seemed the right place for it, but it is dead code. I would remove this if that's ok with you.

jelmer · 2022-08-12T13:07:21Z

dulwich/credentials.py

+
+def get_credentials_from_helper(base_url: str, config) -> Tuple[bytes, bytes]:
+    """Retrieves credentials for the given url from git credential helpers"""
+    if isinstance(config, StackedConfig):


Why does this need to peek under the covers, can't it just use the config API?

Given an URL, git loops though the available configs and matches urls in credentials sections with the provided URL, which is something we cannot do with the config API.

For example with a config like this:

[credential "https://github.com/jelmer/dulwich"] helper = githubhelper [credential] helper = generichelper

Running

config.get((b"credential", b"https://github.com))

would return generichelper instead of githubhelper, which is not what we want.

I refactored this a bit so that we use urlmatch_credential_sections to cycle though config sections that match the URL.
This could be extracted to become a config API method, although I'm not sure if there's any other use cases where one needs to match URLs in sections, apart from the credential helpers scenario.

dtrifiro · 2022-08-30T21:49:05Z

Sorry for sitting on this for so long

Hi @jelmer, thanks for taking the time to review this 🙂

I'm a bit concerned about how this makes the interface for authentication more complicated. Ideally there should just be one pythonic interface for authentication, with perhaps one implementation of that that can interact with C git.

I'm not sure I understand what you're proposing here, but below you'll find my thoughts. Please let me know if I misunderstood and/or if there's any other parts you find complicated you can point me to, so I can have another look and take action.

With my last push I streamlined get_credentials_from_helpers to explicitly how the configs sections and values are being used. The only other method to get credentials is get_credentials_from_store which, as far as I can tell, is unused. We could either refactor this or get rid of it (as I proposed in another comment).

Temporary patch while waiting for jelmer/dulwich#976

dtrifiro · 2022-10-12T16:47:48Z

Hey @jelmer, just a friendly ping: this is ready for review 🙂

jelmer · 2022-10-16T16:42:20Z

sorry about that, looking at it again now

jelmer · 2022-10-19T22:40:22Z

sorry about that, looking at it again now

Haven't forgotten about this, will probably put forward a PR with a subset of your changes later this week.

- add misc url-matching functions Extracted from #976

jelmer · 2022-10-23T23:39:52Z

Sorry again for the long delay. As a means of trying to move this forward, I've at least cherry-pick the config-related bits of this PR that are non-controversial.

efiop · 2022-10-26T14:22:22Z

Hey @jelmer , thanks for taking a look! 🙏 FYI: @dtrifiro broke his clavicle/collarbone and won't be able to reply for a few weeks. If there is anything he needs to address, he'll get back to it once he's recovered.

jelmer · 2022-10-26T14:49:04Z

Hey @jelmer , thanks for taking a look! 🙏 FYI: @dtrifiro broke his clavicle/collarbone and won't be able to reply for a few weeks. If there is anything he needs to address, he'll get back to it once he's recovered.

Ah, thanks - good to know. I hope his recovery goes well!

dtrifiro · 2023-01-12T16:34:25Z

Hey @jelmer, sorry for keeping this hanging so long.

I just rebased and added a new commit to update the get_credentials_from_store to use CredentialHelper.

If you have any other concerns and/or suggestions that would help this get merged, I'd greatly appreciate that.

Thanks!

- add `CredentialHelper` class to run credential helper commands and retrieve values - add `get_credentials_from_helper` to retrieve credentials from helpers defined in provided config - moved (unused) `get_credentials_from_store` to `credentials.py` Also see https://www.git-scm.com/docs/gitcredentials fixes jelmer#873

dtrifiro force-pushed the feature/credential-helper branch from c3e09d6 to eeea755 Compare June 7, 2022 12:21

dtrifiro force-pushed the feature/credential-helper branch from eeea755 to 4df8f7d Compare June 7, 2022 14:31

dtrifiro commented Jun 7, 2022

View reviewed changes

dulwich/client.py Outdated Show resolved Hide resolved

dtrifiro force-pushed the feature/credential-helper branch 3 times, most recently from 7c6a29f to 452bc9a Compare June 8, 2022 12:10

efiop mentioned this pull request Jun 9, 2022

import: does not work with repositories using params iterative/dvc#7307

Closed

dtrifiro marked this pull request as ready for review June 10, 2022 13:29

jelmer self-requested a review June 12, 2022 23:09

pmrowla reviewed Jun 15, 2022

View reviewed changes

dtrifiro force-pushed the feature/credential-helper branch from 452bc9a to f90afc7 Compare June 15, 2022 16:01

dtrifiro commented Jun 15, 2022

View reviewed changes

dulwich/credentials.py Outdated Show resolved Hide resolved

dtrifiro force-pushed the feature/credential-helper branch 2 times, most recently from a0aaf5a to 5068244 Compare June 15, 2022 16:42

pmrowla reviewed Jun 16, 2022

View reviewed changes

dulwich/client.py Outdated Show resolved Hide resolved

pmrowla reviewed Jun 16, 2022

View reviewed changes

dulwich/tests/test_client.py Outdated Show resolved Hide resolved

dtrifiro force-pushed the feature/credential-helper branch 2 times, most recently from 357cfd7 to 5e58402 Compare June 16, 2022 10:36

This comment was marked as outdated.

Sign in to view

dtrifiro mentioned this pull request Jun 16, 2022

dvc import broken authentication iterative/dvc#7898

Closed

jelmer reviewed Jun 16, 2022

View reviewed changes

dtrifiro force-pushed the feature/credential-helper branch 2 times, most recently from 159278f to f3ed010 Compare June 20, 2022 14:18

dtrifiro requested a review from jelmer June 20, 2022 15:30

dtrifiro force-pushed the feature/credential-helper branch 2 times, most recently from 1705fbf to e9cca29 Compare June 28, 2022 07:22

dtrifiro force-pushed the feature/credential-helper branch from 79fd07d to f1343d2 Compare August 23, 2022 10:20

jelmer requested changes Aug 30, 2022

View reviewed changes

dtrifiro force-pushed the feature/credential-helper branch from f1343d2 to a60bdb1 Compare August 30, 2022 18:41

jelmer force-pushed the feature/credential-helper branch from a60bdb1 to c2200c5 Compare September 5, 2022 01:29

dtrifiro added a commit to dtrifiro/scmrepo that referenced this pull request Sep 6, 2022

dulwich: add support for git credential helpers

8a9da51

Temporary patch while waiting for jelmer/dulwich#976

dtrifiro mentioned this pull request Sep 6, 2022

dulwich: add support for git credential helpers iterative/scmrepo#138

Merged

dtrifiro added a commit to dtrifiro/scmrepo that referenced this pull request Sep 6, 2022

dulwich: add support for git credential helpers

76ee05d

Temporary patch while waiting for jelmer/dulwich#976

dtrifiro added a commit to dtrifiro/scmrepo that referenced this pull request Sep 6, 2022

dulwich: add support for git credential helpers

4646872

Temporary patch while waiting for jelmer/dulwich#976

dtrifiro added a commit to dtrifiro/scmrepo that referenced this pull request Sep 6, 2022

dulwich: add support for git credential helpers

2e3f7da

Temporary patch while waiting for jelmer/dulwich#976

efiop pushed a commit to iterative/scmrepo that referenced this pull request Sep 7, 2022

dulwich: add support for git credential helpers

38a252e

Temporary patch while waiting for jelmer/dulwich#976

dtrifiro requested a review from jelmer October 7, 2022 12:31

jelmer mentioned this pull request Oct 23, 2022

Import misc url-matching functions for credentials helpers #1079

Merged

jelmer pushed a commit that referenced this pull request Oct 23, 2022

add support for git credential helpers

96588c5

- add misc url-matching functions Extracted from #976

dtrifiro force-pushed the feature/credential-helper branch from c2200c5 to 7bdc74a Compare January 12, 2023 16:29

dtrifiro added 5 commits January 26, 2023 12:36

ci: disable scheduled runs, python publish

adad034

fix typo

1534402

porcelain: get rid of pathlib.Path

5ba74dd

credentials: use CredentialHelper in get_credentials_from_store

c9e2d47

dtrifiro force-pushed the feature/credential-helper branch from 7bdc74a to c9e2d47 Compare January 26, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for git credential helpers #976

add support for git credential helpers #976

dtrifiro commented Jun 7, 2022 •

edited

codecov-commenter commented Jun 7, 2022 •

edited

dtrifiro commented Jun 13, 2022

pmrowla Jun 15, 2022

dtrifiro Jun 15, 2022

pmrowla Jun 16, 2022

This comment was marked as outdated.

jelmer Jun 16, 2022

dtrifiro Jun 20, 2022

jelmer Jun 16, 2022

pmrowla Jun 17, 2022

dtrifiro Jun 20, 2022 •

edited

jelmer left a comment

dtrifiro commented Jun 28, 2022 •

edited

oslobowl commented Aug 29, 2022

jelmer left a comment

jelmer Aug 12, 2022

jelmer Aug 12, 2022

dtrifiro Aug 30, 2022

jelmer Aug 12, 2022

jelmer Aug 12, 2022

dtrifiro Aug 30, 2022

jelmer Aug 12, 2022

dtrifiro Aug 30, 2022

dtrifiro commented Aug 30, 2022

dtrifiro commented Oct 12, 2022

jelmer commented Oct 16, 2022

jelmer commented Oct 19, 2022

jelmer commented Oct 23, 2022

efiop commented Oct 26, 2022

jelmer commented Oct 26, 2022

dtrifiro commented Jan 12, 2023


		from dulwich.config import ConfigDict, StackedConfig, get_xdg_config_home_path

		if TYPE_CHECKING:

		return cls(command.decode(encoding))


		def get_credentials_from_store(

add support for git credential helpers #976

Are you sure you want to change the base?

add support for git credential helpers #976

Conversation

dtrifiro commented Jun 7, 2022 • edited

codecov-commenter commented Jun 7, 2022 • edited

Codecov Report

dtrifiro commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtrifiro Jun 20, 2022 • edited

Choose a reason for hiding this comment

jelmer left a comment

Choose a reason for hiding this comment

dtrifiro commented Jun 28, 2022 • edited

oslobowl commented Aug 29, 2022

jelmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtrifiro commented Aug 30, 2022

dtrifiro commented Oct 12, 2022

jelmer commented Oct 16, 2022

jelmer commented Oct 19, 2022

jelmer commented Oct 23, 2022

efiop commented Oct 26, 2022

jelmer commented Oct 26, 2022

dtrifiro commented Jan 12, 2023

dtrifiro commented Jun 7, 2022 •

edited

codecov-commenter commented Jun 7, 2022 •

edited

dtrifiro Jun 20, 2022 •

edited

dtrifiro commented Jun 28, 2022 •

edited