Skip to content

Conversation

@pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Mar 28, 2023

  • Adds support for interactive username/password prompt
  • Interactive support is disabled when GIT_TERMINAL_PROMPT env var is set to 0 (same as CLI Git)

dulwich changes:

  • Now follows proper auth workflow for HTTP
    1. Unauthenticated request is tried first
    2. If request fails, credentials are read from credential helpers and interactive prompt (in that order)
    3. Request is retried with credentials from (2), if request fails, we raise the auth error
  • asyncssh vendor now supports interactive prompt for username/password when the server requests it
    • Note that this is not always available depending on the server
    • Github/Gitlab/etc only allow key authentication over SSH
    • Mainly only useful for private/self-hosted git over SSH

pygit2 changes:

  • Interactive username/pw auth supported for HTTP

(Note that SSH support is still disabled in pygit2 backend)

@pmrowla pmrowla self-assigned this Mar 28, 2023
@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 28, 2023

This PR does not require upstreaming callback support in dulwich. HTTP is handled in our HTTPGitClient implementation and SSH is handled in our asyncssh vendor. It would be better to have a unified approach in dulwich (similar to libgit2/pygit2 RemoteCallbacks), but this should be sufficient to avoid re-introducing any default gitpython usage.

@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2023

Codecov Report

Patch coverage: 25.00% and project coverage change: -1.12 ⚠️

Comparison is base (4605cd3) 81.61% compared to head (cf0b166) 80.49%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
- Coverage   81.61%   80.49%   -1.12%     
==========================================
  Files          26       26              
  Lines        3677     3743      +66     
  Branches      646      660      +14     
==========================================
+ Hits         3001     3013      +12     
- Misses        573      627      +54     
  Partials      103      103              
Impacted Files Coverage Δ
src/scmrepo/git/backend/dulwich/client.py 33.96% <20.00%> (-34.22%) ⬇️
src/scmrepo/git/backend/pygit2/callbacks.py 40.00% <26.92%> (-13.34%) ⬇️
src/scmrepo/git/backend/dulwich/asyncssh_vendor.py 74.60% <33.33%> (-5.58%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmrowla pmrowla merged commit 4f28b3f into treeverse:main Mar 29, 2023
@pmrowla pmrowla deleted the interactive-auth branch March 29, 2023 05:02
@dberenbaum
Copy link
Contributor

@pmrowla Looks great.

I tried to test it on an http repo but still find I'm getting an error. Is it expected?

$ dvc get -v https://dagshub.com/dberenbaum/example-get-started model.pkl
2023-03-29 16:32:14,232 DEBUG: v2.51.1.dev32+ga6c89cac8, CPython 3.10.2 on macOS-13.2.1-arm64-arm-64bit
2023-03-29 16:32:14,232 DEBUG: command: /Users/dave/miniforge3/envs/dvc/bin/dvc get -v https://dagshub.com/dberenbaum/example-get-started model.pkl
2023-03-29 16:32:14,457 DEBUG: Creating external repo https://dagshub.com/dberenbaum/example-get-started@None
2023-03-29 16:32:14,457 DEBUG: erepo: git clone 'https://dagshub.com/dberenbaum/example-get-started' to a temporary dir
2023-03-29 16:32:14,692 DEBUG: Removing '/private/tmp/.AouoqMnjQkTnyjQwYEhS94'
2023-03-29 16:32:14,692 ERROR: failed to get 'model.pkl' - SCM error: Failed to clone repo 'https://dagshub.com/dberenbaum/example-get-started' to '/var/folders/24/99_tf1xj3vx8k1k_jkdmnhq00000gn/T/tmpdwri2ftndvc-clone': No valid credentials provided
Traceback (most recent call last):
  File "/Users/dave/Code/scmrepo/src/scmrepo/git/backend/dulwich/client.py", line 48, in _http_request
    result = super()._http_request(url, headers=headers, data=data)
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dulwich/client.py", line 2219, in _http_request
    raise HTTPUnauthorized(resp.headers.get("WWW-Authenticate"), url)
dulwich.client.HTTPUnauthorized: No valid credentials provided

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/dave/Code/scmrepo/src/scmrepo/git/backend/dulwich/__init__.py", line 219, in clone
    repo = clone_from()
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dulwich/porcelain.py", line 508, in clone
    return client.clone(
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dulwich/client.py", line 705, in clone
    result = self.fetch(path, target, progress=progress, depth=depth)
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dulwich/client.py", line 782, in fetch
    result = self.fetch_pack(
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dulwich/client.py", line 2085, in fetch_pack
    refs, server_capabilities, url = self._discover_references(
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dulwich/client.py", line 1941, in _discover_references
    resp, read = self._http_request(url, headers)
  File "/Users/dave/Code/scmrepo/src/scmrepo/git/backend/dulwich/client.py", line 57, in _http_request
    result = super()._http_request(url, headers=headers, data=data)
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/dulwich/client.py", line 2219, in _http_request
    raise HTTPUnauthorized(resp.headers.get("WWW-Authenticate"), url)
dulwich.client.HTTPUnauthorized: No valid credentials provided

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/dave/Code/dvc/dvc/scm.py", line 160, in clone
    git = Git.clone(url, to_path, progress=pbar.update_git, **kwargs)
  File "/Users/dave/Code/scmrepo/src/scmrepo/git/__init__.py", line 142, in clone
    backend.clone(url, to_path, **kwargs)
  File "/Users/dave/Code/scmrepo/src/scmrepo/git/backend/dulwich/__init__.py", line 224, in clone
    raise CloneError(url, to_path) from exc
scmrepo.exceptions.CloneError: Failed to clone repo 'https://dagshub.com/dberenbaum/example-get-started' to '/var/folders/24/99_tf1xj3vx8k1k_jkdmnhq00000gn/T/tmpdwri2ftndvc-clone'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/dave/Code/dvc/dvc/commands/get.py", line 33, in _get_file_from_repo
    Repo.get(
  File "/Users/dave/Code/dvc/dvc/repo/get.py", line 54, in get
    with external_repo(
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/contextlib.py", line 135, in __enter__
    return next(self.gen)
  File "/Users/dave/Code/dvc/dvc/external_repo.py", line 34, in external_repo
    path = _cached_clone(url, rev, for_write=for_write)
  File "/Users/dave/Code/dvc/dvc/external_repo.py", line 144, in _cached_clone
    clone_path, shallow = _clone_default_branch(url, rev, for_write=for_write)
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/funcy/decorators.py", line 45, in wrapper
    return deco(call, *dargs, **dkwargs)
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/funcy/flow.py", line 274, in wrap_with
    return call()
  File "/Users/dave/miniforge3/envs/dvc/lib/python3.10/site-packages/funcy/decorators.py", line 66, in __call__
    return self._func(*self._args, **self._kwargs)
  File "/Users/dave/Code/dvc/dvc/external_repo.py", line 212, in _clone_default_branch
    git = clone(url, clone_path)
  File "/Users/dave/Code/dvc/dvc/scm.py", line 165, in clone
    raise CloneError("SCM error") from exc
dvc.scm.CloneError: SCM error

$ dvc doctor
DVC version: 2.51.1.dev32+ga6c89cac8
------------------------------------
Platform: Python 3.10.2 on macOS-13.2.1-arm64-arm-64bit
Subprojects:
        dvc_data = 0.46.0
        dvc_objects = 0.21.1
        dvc_render = 0.3.1
        dvc_task = 0.2.0
        scmrepo = 0.1.18.dev7+g4f28b3f
Supports:
        azure (adlfs = 2022.9.1, knack = 0.9.0, azure-identity = 1.7.1),
        gdrive (pydrive2 = 1.15.0),
        gs (gcsfs = 2022.11.0),
        hdfs (fsspec = 2022.11.0+0.gacad158.dirty, pyarrow = 7.0.0),
        http (aiohttp = 3.8.1, aiohttp-retry = 2.8.3),
        https (aiohttp = 3.8.1, aiohttp-retry = 2.8.3),
        oss (ossfs = 2021.8.0),
        s3 (s3fs = 2022.11.0+6.g804057f.dirty, boto3 = 1.24.59),
        ssh (sshfs = 2023.1.0),
        webdav (webdav4 = 0.9.4),
        webdavs (webdav4 = 0.9.4),
        webhdfs (fsspec = 2022.11.0+0.gacad158.dirty)

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 30, 2023

If the credentials are wrong then the error would be expected. I'm not sure how dagshub's git server works, I've only been testing against private github repos.

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 30, 2023

@dberenbaum with the latest scmrepo@main I get
asciicast

(using GH personal access token instead of password)

@skshetry
Copy link
Collaborator

@pmrowla, what do you think about supporting a callback that dvc provides? That would allow us to disable that progress bar while prompting.

@skshetry
Copy link
Collaborator

skshetry commented Mar 30, 2023

Also in these cases, I am not sure where we should prompt. git prompts on /dev/tty as far I remember.

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 30, 2023

@skshetry I think it would be better to have an actual callbacks that get passed all the way up to DVC.

This implementation is very naive, and is just meant to be a quick replacement for what we used to provide through gitpython (which just exposed stdin/stdout/stderr to the git subprocess and did not have any actual callback API at all for either progress or the interactive input)

@pmrowla
Copy link
Contributor Author

pmrowla commented Mar 30, 2023

getpass does prompt on /dev/tty on posix, I'll change the username prompt so it behaves the same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants