remotes.http: Support dvc push for http remotes #3343

pmrowla · 2020-02-17T06:46:24Z

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
remote: Document HTTP remote push changes dvc.org#1006
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

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

WIP PR for #3247

Adds support for POST uploads to http remotes
Adds support for HTTP Basic and Digest authentication methods (for both upload and download operations)
- basic_auth and digest_auth boolean options added to the http(s) remote configurations (digest takes precedence if both are set to true)
- When no auth method is specified (or both are set to false), existing behavior is used (no auth method, assumes all HTTP endpoints are public for both upload and download)
- Username/password combinations can be configured in the same way as ssh remotes
- Password can be set in config or prompted on command line via ask_password config option

Still TODO:

Add functional/remote tests
Docs PR

pmrowla · 2020-02-17T06:48:05Z

I have been testing locally against a simple flask app with /public /basic /digest endpoints for each possible auth method: https://gist.github.com/pmrowla/6ed79cc0c4d20e26ab8e86d25570c983

dvc/config.py

dvc/remote/http.py

efiop

Looks great!

tests/utils/httpd.py

dvc/remote/http.py

efiop · 2020-02-20T01:25:26Z

For the record: test failures are unrelated and will be fixed in #3368

- uploaded files are sent as chunked encoding POST data Fixes iterative#3247

- username, password, ask_password options work the same way as for ssh remotes - basic_auth (bool) and digest_auth (bool) options added for http(s) remotes - digest_auth takes precedence over basic_auth if both are enabled

- HTTP remotes now tested locally using a SimpleHTTPServer instance that allows reading/writing to a temp directory

Co-Authored-By: Saugat Pachhai <suagatchhetri@outlook.com> Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>

tests/func/test_data_cloud.py

pared

LGTM, one comment.

pared · 2020-02-20T12:21:06Z

tests/func/test_data_cloud.py

+        self.http_server = http_server
+        self.method_name = request.function.__name__
+
+    def get_url(self):


I think DS has a valid point here, the original method is static, shouldn't we make all of them non-static now, that it is required by this particular change?

For what it's worth, I wrote it like this because it was done the same way in TestRemoteSSHMocked (which overrides static SSHMocked.get_url() from tests/remotes.py).

Well, it was only a question, its not severe, because any potential problems should be detected on the build stage. For me, it can stay as is.

skshetry · 2020-02-21T10:47:51Z

dvc/remote/http.py

@@ -100,7 +163,13 @@ def _request(self, method, url, **kwargs):
        kwargs.setdefault("timeout", self.REQUEST_TIMEOUT)

        try:
-            res = self._session.request(method, url, **kwargs)
+            res = self._session.request(


@pmrowla, I'm unable to make it work with Digest auth (I'm using the script that you provided on the gist).

But, when I change this to:

Suggested change

res = self._session.request(

res = requests.Session().request(

it works. Something's wrong in our cached session perhaps?

I think I figured it out. When I try to push, we send a HEAD request, after which the script sets a cookie. And, then all POST requests fails. So, I first tried clearing cookies with self._session.cookies.clear() and it worked.

Probably, we need to set auth when on creating sessions?

I'm wondering if this is a side effect of how my test app in the gist was configured?

By default, flask uses client-side cookies for session data, so flask-httpauth does as well. But for a web server running digest auth to be properly secured, it needs to be handled server-side: https://flask-httpauth.readthedocs.io/en/latest/#security-concerns-with-digest-authentication

If I modify the test app to actually use server-side sessions (https://gist.github.com/pmrowla/0615f162d1308cab4f429b6efafe276a) the existing remote code works without needing to clear any cached cookies

If I set session.auth in the http remote at the time we first create the session instead of setting it per request, I still see the same issue making requests against the original test app. So I'm not sure if it's just that requests.auth.HTTPDigestAuth won't work properly when talking to improperly configured flask apps?

If I set session.auth in the http remote at the time we first create the session instead of setting it per request, I still see the same issue making requests against the original test app.

@pmrowla, I tried that too but, does not work. It's unclear what's the best thing to do here.

If it's probably only flask-httpauth, let's ignore for now then.

pmrowla changed the title ~~WIP: Support dvc push for http remotes~~ [WIP] remotes.http: Support dvc push for http remotes Feb 17, 2020

pmrowla requested a review from efiop February 17, 2020 06:52

pmrowla mentioned this pull request Feb 18, 2020

remote: Document HTTP remote push changes iterative/dvc.org#1006

Merged

pared self-requested a review February 18, 2020 15:49

efiop reviewed Feb 18, 2020

View reviewed changes

dvc/config.py Outdated Show resolved Hide resolved

efiop reviewed Feb 19, 2020

View reviewed changes

dvc/remote/http.py Outdated Show resolved Hide resolved

efiop reviewed Feb 19, 2020

View reviewed changes

dvc/remote/http.py Outdated Show resolved Hide resolved

efiop approved these changes Feb 19, 2020

View reviewed changes

efiop requested a review from skshetry February 19, 2020 13:22

skshetry reviewed Feb 19, 2020

View reviewed changes

tests/utils/httpd.py Outdated Show resolved Hide resolved

skshetry reviewed Feb 19, 2020

View reviewed changes

dvc/remote/http.py Outdated Show resolved Hide resolved

efiop requested a review from skshetry February 20, 2020 00:42

pmrowla changed the title ~~[WIP] remotes.http: Support dvc push for http remotes~~ remotes.http: Support dvc push for http remotes Feb 20, 2020

pmrowla and others added 8 commits February 20, 2020 10:47

remote.http: support _upload() via HTTP POST

bf7f0ad

- uploaded files are sent as chunked encoding POST data Fixes iterative#3247

remote.http: add support for HTTP basic and digest auth

3e73a59

- username, password, ask_password options work the same way as for ssh remotes - basic_auth (bool) and digest_auth (bool) options added for http(s) remotes - digest_auth takes precedence over basic_auth if both are enabled

remote.http: auth_method() unit tests

c55fb3d

tests: Add functional tests for HTTP remotes

b45b9f7

- HTTP remotes now tested locally using a SimpleHTTPServer instance that allows reading/writing to a temp directory

tests: fix http tests on py3.5, 3.6

5a92a8b

remote.http: add custom auth method

fa69951

s/custom_header/custom_auth_header/

c193be3

Apply suggestions from code review

b893874

Co-Authored-By: Saugat Pachhai <suagatchhetri@outlook.com> Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>

pmrowla force-pushed the http-push branch from 58e94c8 to b893874 Compare February 20, 2020 01:49

skshetry reviewed Feb 20, 2020

View reviewed changes

tests/func/test_data_cloud.py Show resolved Hide resolved

pared reviewed Feb 20, 2020

View reviewed changes

efiop requested review from skshetry and pared February 21, 2020 03:18

pared approved these changes Feb 21, 2020

View reviewed changes

efiop merged commit 68e543f into iterative:master Feb 21, 2020

skshetry reviewed Feb 21, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Feb 23, 2020

Weekly Digest (16 February, 2020 - 23 February, 2020) #3387

Closed

pmrowla deleted the http-push branch March 25, 2020 06:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remotes.http: Support dvc push for http remotes #3343

remotes.http: Support dvc push for http remotes #3343

pmrowla commented Feb 17, 2020 •

edited

Loading

pmrowla commented Feb 17, 2020

efiop left a comment

efiop commented Feb 20, 2020

pared left a comment

pared Feb 20, 2020

pmrowla Feb 20, 2020

pared Feb 21, 2020

skshetry Feb 21, 2020

skshetry Feb 21, 2020 •

edited

Loading

skshetry Feb 21, 2020

skshetry Feb 21, 2020

pmrowla Feb 21, 2020 •

edited

Loading

skshetry Feb 21, 2020

	res = self._session.request(
	res = requests.Session().request(

remotes.http: Support dvc push for http remotes #3343

remotes.http: Support dvc push for http remotes #3343

Conversation

pmrowla commented Feb 17, 2020 • edited Loading

pmrowla commented Feb 17, 2020

efiop left a comment

Choose a reason for hiding this comment

efiop commented Feb 20, 2020

pared left a comment

Choose a reason for hiding this comment

pared Feb 20, 2020

Choose a reason for hiding this comment

pmrowla Feb 20, 2020

Choose a reason for hiding this comment

pared Feb 21, 2020

Choose a reason for hiding this comment

skshetry Feb 21, 2020

Choose a reason for hiding this comment

skshetry Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

skshetry Feb 21, 2020

Choose a reason for hiding this comment

skshetry Feb 21, 2020

Choose a reason for hiding this comment

pmrowla Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

skshetry Feb 21, 2020

Choose a reason for hiding this comment

pmrowla commented Feb 17, 2020 •

edited

Loading

skshetry Feb 21, 2020 •

edited

Loading

pmrowla Feb 21, 2020 •

edited

Loading