Added GitAuthError

[daavoo]

• ❗ I have followed the Contributing to DVC checklist.

• 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

To match updates in docs. From iterative/dvc.org#2731 (review)

[pmrowla]

It doesn't have to be an SSH URL in every case though.

The SSH requirement is specifically for pushing (writing) to a remote git server that requires authentication (meaning pushing to github/gitlab). exp pull and exp list will work with any git URL (including HTTP) as long as the git server is public. If you are hosting a personal git server somewhere that was configured to allow pushing over unauthenticated HTTP, you could use the HTTP git URL format (and would not be required to use SSH).

And yes, read operations that require authentication (pull/list for private github/gitlab repos) would require using SSH.

[daavoo]

It doesn't have to be an SSH URL in every case though.

The SSH requirement is specifically for pushing (writing) to a remote git server that requires authentication (meaning pushing to github/gitlab). exp pull and exp list will work with any git URL (including HTTP) as long as the git server is public. If you are hosting a personal git server somewhere that was configured to allow pushing over unauthenticated HTTP, you could use the HTTP git URL format (and would not be required to use SSH).

And yes, read operations that require authentication (pull/list for private github/gitlab repos) would require using SSH.

Not sure if there is a way of synthesizing those cases for the CLI help or if we should just say any valid URL and leave the detailed info for the docs.

cc @jorgeorpinel

[pmrowla]

IMO the CLI help text should just use a valid Git URL, and a more detailed explanation about the SSH requirement when server auth is required belongs in the online docs.

[skshetry]

@pmrowla, is it possible to catch the exception and provide the hint (maybe with the link to the proper documentation)?

[pmrowla]

@pmrowla, is it possible to catch the exception and provide the hint (maybe with the link to the proper documentation)?

Yes, in the dulwich backend we need to check for these exceptions when we do a GitClient operation
https://github.com/dulwich/dulwich/blob/5559440256dc231637896d97f1016f9052cc7dc3/dulwich/client.py#L2101

[daavoo]

IMO the CLI help text should just use a valid Git URL, and a more detailed explanation about the SSH requirement when server auth is required belongs in the online docs.

So I just added a minor change here including the word valid. At the very least it implies that there some URLs could be invalid. The proper definition of what valid implies would be in the docs.

@pmrowla, is it possible to catch the exception and provide the hint (maybe with the link to the proper documentation)?

This would be a good improvement

[dberenbaum]

Agree that giving a hint seems much more useful. Seems like we can just add another exception handler for HTTPUnauthorized in

dvc/dvc/scm/git/backend/dulwich.py

Line 429 in 4ee9829

try:

?

Even better would be to prompt for username and password and try again. Is there a reason we wouldn't want to do that?

[pmrowla]

Even better would be to prompt for username and password and try again. Is there a reason we wouldn't want to do that?

Using prompts won't work in automated contexts (like the CML auto-push feature) but it would be possible for us to support it. The main issue will just be implementing proper support for git HTTP auth (see dulwich tests for examples: https://github.com/dulwich/dulwich/blob/76dd8356f9d8870f44226e4232c24fbc1e573e93/dulwich/tests/test_client.py#L565)

But it should be noted that even if we add the prompts and username/password HTTP support for git operations, it won't work for pushing to github (https://github.blog/2020-12-15-token-authentication-requirements-for-git-operations/)

[dberenbaum]

Using prompts won't work in automated contexts (like the CML auto-push feature) but it would be possible for us to support it.

Right, if we make it easier in non-automated scenarios, there could be confusion if people try to migrate to something like CML. Hopefully they would realize any manual steps would need to be automated? Is it better to give flexibility or require the preferred workflow? It seems like generally we try to let users make these decisions themselves. On the other hand, we don't prompt for remote credentials, but that may be because it would get too complex to handle.

The main issue will just be implementing proper support for git HTTP auth (see dulwich tests for examples: https://github.com/dulwich/dulwich/blob/76dd8356f9d8870f44226e4232c24fbc1e573e93/dulwich/tests/test_client.py#L565)

Can we do this:

1. Catch the HTTPUnauthorized exception
2. Prompt for the username and password
3. Recreate the git client with those keyword args
4. Retry
5. Throw if there is still an exception

Does dvc need to worry about the details of http auth beyond asking for username and password?

But it should be noted that even if we add the prompts and username/password HTTP support for git operations, it won't work for pushing to github (https://github.blog/2020-12-15-token-authentication-requirements-for-git-operations/)

From a quick test, it looks like users should be able to push to github with http if they replace their passwords with their tokens.

[daavoo]

Can we do this:

1. Catch the `HTTPUnauthorized` exception

2. Prompt for the username and password

3. Recreate the git client with those keyword args

4. Retry

5. Throw if there is still an exception

Does dvc need to worry about the details of http auth beyond asking for username and password?

This would be prompted on each command run (i.e. dvc exp push), right? I feel like that could be somehow annoying.

Not sure if that would be a good user experience or if it would be more helpful to just raise an exception with a link to the docs.

[dberenbaum]

This would be prompted on each command run (i.e. dvc exp push), right? I feel like that could be somehow annoying.

Not sure if that would be a good user experience or if it would be more helpful to just raise an exception with a link to the docs.

It could be annoying, but is it any more annoying than doing git push via HTTP? SSH seems like the way to go, but I worry that it will cause trouble for some users (for example, if SSH port is blocked).

We could start with the warning and keep the HTTP authorization open as a separate issue to see if users do encounter it (or wait to see if someone opens that issue themselves).

Edit: I meant we can start with raising an exception for unauthorized http for now.

[skshetry]

For autopush, we may need some kind of memoization like the way we do for http(s) filesystems.

dvc/dvc/fs/http.py

Lines 12 to 18 in 6b582bd

	@wrap_with(threading.Lock())
	@memoize
	def ask_password(host, user):
	return prompt.password(
	"Enter a password for "
	"host '{host}' user '{user}'".format(host=host, user=user)
	)

[dberenbaum]

Not sure if there is a way of synthesizing those cases for the CLI help or if we should just say any valid URL and leave the detailed info for the docs.

Is "any valid URL" correct? It seems more like it's "any supported URL" since I think the HTTP URL is also valid according to Git. I also think we could probably summarize behavior as not supporting HTTP authentication. Cc @jorgeorpinel

[jorgeorpinel]

💅

See also iterative/dvc.org#2731

[jorgeorpinel]

Is "any valid URL" correct? It seems more like it's "any supported URL"
summarize behavior as not supporting HTTP authentication

Good points @dberenbaum. I updated my suggestions ☝️

[daavoo]

Need to add tests catching the exception

[dberenbaum]

Pending discussion in iterative/dvc.org#2731 (comment).

[karajan1001]

So when is it ready? Still a draft one?

[daavoo]

So when is it ready? Still a draft one?

I guess that we can now consider this ready (docs PR was also approved iterative/dvc.org#2731 (comment)) and have #6586 as a follow-up.