Abstract out the network IO #1250

sechkova · 2020-12-22T18:20:31Z

Description of the changes being introduced by the pull request:

Based on the discussion in #1213 and mainly the idea of @jku, implement an abstract FetcherInterface calss and
a concrete RequestsFetcher class .

The idea is to extract the network IO related code out of tuf.download and put it in RequestsFetcher class which is also the default FetcherInterface implementation which TUF uses if not given an external one.

RequestsFetcher implements only one public method 'fetch' which returns a bytes iterator. All file handling and verification
logic remains in tuf.download which is now wrapper in a FileDownload class for convenience.

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

sechkova · 2020-12-22T18:29:39Z

The new GitHub actions have been harsh on me :)) Will review the errors after the holiday break.

sechkova · 2021-01-05T10:07:56Z

The new GitHub actions have been harsh on me :)) Will review the errors after the holiday break.

Fixed the linter issues, however according to Windows "Download was too slow." 🙄

sechkova · 2021-01-06T08:55:03Z

Fixed a small issue and force-pushed but (un)fortunately I see no errors while testing on a local windows VM.

sechkova · 2021-01-12T11:37:19Z

First thing, apologize for the spam and using the CI as a test server but I could not reproduce the Windows tests failure locally.

The problem can be summarized in two parts:

Slow 'localhost' lookup on Windows tests: localhost name lookup takes 1sec on Windows in github actions #1257
A change in this PR which adds the time to connect to the server to the calculation of average_download_speed for the first downloaded chunk which raises SlowRetreivalError in the case of 1.

While 1. can quickly be fixed as suggested by @jku in #1257, 2. is related to the network abstraction api which may need some discussion. The main point is, given all the uncertainty around protection against slow retrieval attack and its recent removal from the specification, where should we implement the slow retrieval check:

Keep the fetcher unaware of it and implement the download speed calculations in its caller function.
- has the obvious pros that fetcher implementation is unaware of it and the check is performed by TUF itself
- cons: incorrectly adds connection time to the actual chunk download time
Keep the fetcher unaware of it but make fetcher API more complicated by splitting session establishment/connection and downloading data
- pros: solves the timing issue
- cons: undesirably complicated api
Move the download speed calculation and slow retrieval checks as part of TUF's fetcher implementation
- pros: replicates current behaviour
- cons: different fetcher implementations can(and most likely will) totally ignore the slow retrieval check

My choice is in favour of point 3 but I would like to hear more opinions before going ahead.

lukpueh · 2021-01-12T12:32:21Z

Thanks for the detailed comment, @sechkova (and of course for the patch, which I haven't looked at yet). Here are my thoughts:

I'd rule out (2). This attack, which is no longer part of the spec (Remove slow retrieval attacks from protections specification#111), does not seem to justify a more complicated API.
I think (3) is perfectly fine, maybe we can add a big disclaimer to the API docs that says that only the default fetcher provides this protection, and any custom implementation must do it themselves.
Regardless, wouldn't it be possible to achieve (1) by granting some grace period in the beginning, where the fetcher may drop below the rate? AFAIU a successful slow-retrieval attack requires quite some time, so if we start checking the average rate only after enough time has passed to likely smooth out the spike caused by connection/session establishment, we are probably able to detect the attack early enough.

trishankatdatadog · 2021-01-12T12:51:09Z

I agree with @lukpueh: (1) makes more sense to me, depending on the philosophical motivation for the fetcher. Are we simply trying to fetch bytes or do something more?

jku · 2021-01-12T13:21:55Z

I lean towards option 3 (mostly because I do not believe our slow retrieval protection works without false positives -- Teodoras test failures are a strange example of this IMO -- and I would like to not have these false positives in pip)

wouldn't it be possible to achieve (1) by granting some grace period in the beginning, where the fetcher may drop below the rate? AFAIU a successful slow-retrieval attack requires quite some time, so if we start checking the average rate only after enough time has passed to likely smooth out the spike caused by connection/session establishment, we are probably able to detect the attack early enough.

I originally suggested the same thing, as this might also help with the massive bias that the current implementation has. Example of the bias: Imagine a download of 10 chunks where 9 chunks transfer at "normal internet speeds" but one is slow:

if the slow chunk is the first one the download will be canceled if speed is < 50B/s
if the slow chunk is the last one the download will happily succeed even if the chunk speed is 1 B/s (a very long time later but it will succeed)

However, solving the above issue issue properly is no doubt complex -- probably the best we could do is special case the first chunk handling, just giving it a few extra seconds... I don't see why this is better than letting the fetcher handle it.

jku · 2021-01-12T13:29:05Z

Oh additional reason for choice 3 (that Teodora kind of mentioned): the patch will probably be cleaner as it will be easier to just move existing code from download.py to Fetcher instead of developing similar but not quite same code.

jku · 2021-01-12T13:34:17Z

I agree with @lukpueh: (1) makes more sense to me, depending on the philosophical motivation for the fetcher. Are we simply trying to fetch bytes or do something more?

The issue is that the first call to the fetch() generator also creates the connection because there is no separate initialization function... now that I said it aloud, I think it has to be possible to initialize the generator separately. I'll look this up. Thanks.

joshuagl · 2021-01-14T10:52:57Z

I am in favour of option three, both because we don't have a specification for slow retrieval attacks, and because we are not confident about the effectiveness of the current implementation of slow retrieval attacks.

First thing, apologize for the spam and using the CI as a test server but I could not reproduce the Windows tests failure locally.

I didn't receive any spam about CI failures, so I think no need to apologise here 😄

sechkova · 2021-01-18T12:59:18Z

Thanks all for giving your opinion and especially @jku for the ideas, most of which I think are included in the latest changes.
Feel free to review 😉

joshuagl · 2021-01-18T21:04:21Z

tuf/fetcher.py

-
-      # Check response status.
-      response.raise_for_status()
+    response = session.get(url, stream=True, timeout=self.SOCKET_TIMEOUT)


As there's no longer a context manager in use for response, do we need to call some cleanup method?

also maybe double check what happens when connect times out. I expect that's a ReadTimeoutError which should be handled

Did some cleanup in 77900af.

Connect time out will raise an exception that will be captured in NoWorkingMirrorError. This is the same as the current behaviour.

jku

I like it except for the fact that it rewrites download.py completely :) I understand it's mostly an indent change but I was really hoping that wasn't needed.

I have not yet reviewed download.py changes -- I will do that but I'll leave an early review for this question: What compromises would we need to make to keep download.py changes to a minimum?

jku · 2021-01-19T09:07:11Z

tuf/fetcher.py

+
+
+
+class FetcherSettings():


Can you explain the construct? I can see it should work but what is the advantage of a separate FetcherSettings and RequestsFetcher?

The idea was to separate FetcherSettings so that other FetcherInterface implementations can reuse them (by inheriting) or replace them with their own. But given that we cannot remove them from tuf.settings just like that, I dropped this change too.

jku · 2021-01-19T09:09:51Z

tuf/settings.py

-# Set a timeout value in seconds (float) for non-blocking socket operations.
-SOCKET_TIMEOUT = 4 #seconds
-
-# The maximum chunk of data, in bytes, we would download in every round.
-CHUNK_SIZE = 400000 #bytes
-


These are settings that unfortunately maybe used by someone so I don't think we should just silently delete them without deprecation.

I wonder if the new implementation should still use these as the default values for its properties... and possibly mark these as deprecated, explaining that these values are now fetcher specific and that the default RequestsFetcher exposes the same properties (but that those properties might not be considered API(?) and those changing them should consider making their own fetcher)

I totally forgot about settings being part of the API. I dropped the commit and just used tuf.settings in RequestsFetcher.

I suggest we revise tuf.settings in general and then apply the decisions here.

jku · 2021-01-19T09:11:51Z

tuf/client/updater.py

+      fetcher:
+        A conctrete 'FetcherInterface' implementation. If not provided,
+        tuf.fetcher.RequestsFetcher is used.


typo on concrete. Also it's lacking a sentence on what fetcher does.

Fixed in b0e8acb

sechkova · 2021-01-22T14:44:48Z

Reworked some changes after discussion with @jku. Download.py should be more appealing for a review now!

MVrachev · 2021-01-22T22:16:36Z

tests/test_fetcher.py

+    self.temp_file.seek(0)
+    temp_file_data = self.temp_file.read().decode('utf-8')
+    self.assertEqual(self.file_contents, temp_file_data)
+    self.assertEqual(self.file_length, len(temp_file_data))


Do you need to check that when you know that the self.file_contents is equal to temp_file_data from the previous assertEqual?

Nope. Thanks for noting, seems like I'll have to take another look on the test cases.

MVrachev · 2021-01-22T22:21:04Z

tests/test_fetcher.py

+
+  # Test incorrect URL parsing
+  def test_url_parsing(self):
+   with self.assertRaises(tuf.exceptions.URLParsingError) as cm:


What exactly will happen if this test fails? What about when it succeeds?
I just haven't seen the usage of a context manager with an exception...

with self.assertRaises(ExpectedError): function_under_test(argument1, argument2)

is functionally the same as

self.assertRaises(ExpectedError, function_under_test, argument1, argument2)

but more readable especially with longer names and more arguments, and also more extensible (as you can have more than one line in the block as test_url_parsing() does).

The as cm seems unneeded (but harmless) in this case as it's not used.

but more readable especially with longer names and more arguments, and also more extensible (as you can have more than one line in the block as test_url_parsing() does).

This is what I've meant when writing it but I guess I had plans to use cm that didn't happen ...

joshuagl · 2021-02-16T15:46:00Z

Would it be possible to ensure the new files (tuf/client/fetcher.py and tuf/requests_fetcher.py) adhere to the new coding style? Of particular note is the docstring format.

sechkova · 2021-02-17T11:50:41Z

Would it be possible to ensure the new files (tuf/client/fetcher.py and tuf/requests_fetcher.py) adhere to the new coding style? Of particular note is the docstring format.

The docstrings are updated. I haven't applied the rules from the new coding style which cause linter errors. Since the changes in the PR are meant to work with the existing code they follow the old style. Let's review the coding style during the transition to TUF 1.0.0

joshuagl

A few minor comments, which you may choose to address or not, and a couple of questions. I will be happy to merge this in its current state once we've released v0.17.0 (#1284).

Thank you for the solid work on this!

tests/test_fetcher.py

joshuagl · 2021-02-23T11:40:49Z

tests/test_fetcher.py

+    temp_file_data = self.temp_file.read().decode('utf-8')
+    self.assertEqual(self.file_contents, temp_file_data)
+    # Double check that we download more than one chunk
+    self.assertGreater(chunks_count, 1)


I think this check is unnecessary? We are checking for the expected number of chunks below, what additional value does this provide?

I reworked it a bit.

Does this line express my intention better:
https://github.com/theupdateframework/tuf/blob/c794e0b917ba79327277f2846eda0c9a005b6730/tests/test_fetcher.py#L122-L123

The goal is to make sure that the calculation of expected_chunks is correct and results in more than one chunk downloaded.

Oh, I had confused variables. Apologies. 🤦
The locality of the check makes the intention clearer, I don't think we should be using assert() over self.assert* here though.

I did change the variable to make the test clearer 🙄
Here it is, using the unittest's "assert" 50b3b19

tuf/download.py

The new class FetcherInterface defines an interface for abstract network download which can be implemented for a variety of network libraries and configurations. Signed-off-by: Teodora Sechkova <[email protected]>

A concrete implementation of FetcherInterface based on the Requests library. Signed-off-by: Teodora Sechkova <[email protected]>

Abstract the network IO. Move the network operations from tuf.download to the RequestsFercher class which is TUF's implementation of the abstract FetcherInterface. Signed-off-by: Teodora Sechkova <[email protected]>

Initialize Updater with an external implementation of FetcherInterface. If not provided, tuf.fetcher.RequestsFetcher is used. Signed-off-by: Teodora Sechkova <[email protected]>

Pass RequestsFetcher object to tuf.download functions. Signed-off-by: Teodora Sechkova <[email protected]>

Add unit test for requests_fetcher.py Signed-off-by: Teodora Sechkova <[email protected]>

- Update RequestsFetcher.fetch to return a generator object. - Update _download_file to skip connection time when calculating average download speed. - Write chunk to temp file before exiting the fetcher loop on error. Signed-off-by: Teodora Sechkova <[email protected]>

On Windows (Github Actions) the lookup for 'localhost' takes 1 second. This is because: - Windows retries connect() with a timeout - the machine has IPv6 and IPv4 but Testserver only binds the port on IPv4 - the test clients connect to 'localhost' Since socketserver.TCPServer does not seem to support IPv6 before 3.8, just replace 'localhost' with '127.0.0.1' in client-side URLs. See theupdateframework#1257 Signed-off-by: Teodora Sechkova <[email protected]>

Use '>=' as the defensive version of the equality check. Add a comment describing the need of a chunks() generator. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2021-02-23T16:16:15Z

Addressed the latest comments and rebased on the latest changes from the development branch.

Add test cases to test_fetcher and test_download that decrease default chunk size and download data in more than one chunk. Small code-style improvements. Signed-off-by: Teodora Sechkova <[email protected]>

Calls to safe_download and unsafe_download leave a temporary file unclosed. Signed-off-by: Teodora Sechkova <[email protected]>

A custom error is required so that updater is able to special case 403 & 404 status codes. Rewrite the test case a bit to be more readable. Signed-off-by: Jussi Kukkonen <[email protected]>

* Move FetcherInterface to tuf/client/ directory: This way everything inside that directory is clearly part of client API, and everything outside _may_ be more of an implementation detail (settings is still an unfortunate exception) * Keep RequestsFetcher in tuf/ for same reasons: it's just the default implementation, not explicitly part of client API An even clearer division would be if we moved all the client specific implementation details (download.py, mirrors.py, requests_fetcher.py) to tuf/client/_internal/ but that's a larger change... Signed-off-by: Jussi Kukkonen <[email protected]>

Use a common test level constant for defining the host address forming the download URL on the client side. Signed-off-by: Teodora Sechkova <[email protected]>

Ensure that the newly added files' docstrings adhere to the recently adopted code style guideline (theupdateframework#1232). Small code style improvements in comments and imports. Signed-off-by: Teodora Sechkova <[email protected]>

jku · 2021-03-01T11:29:46Z

Had another look, I agree with my previous self: LGTM.

Notes:

The whole thing is a mostly faithful reproduction of the earlier code, just in a RequestsFetcher.fetch() generator function
requests_fetcher.py, download.py and mirrors.py are implementation details of updater.py: this is why download.py "API change" was seen as reasonable in the PR
download.py does not provide much value anymore but I think keeping it is the correct call to keep the PR to a reasonable size
This PR fixes the issues that I can see with pip integration (ability to tweak network stack tweakables, ability to do progress notifications, ability to compress metadata on the wire)
The test server address fix could be more integrated (like TestServerProcess.get_base_url() that returns something like "http://127.0.0.1:1234/" but I think this is good enough for this PR)

There's an approval already so I'll merge this soon if no complaints appear.

lukpueh · 2021-03-01T20:10:53Z

Thanks for all the efforts you put into this PR, @sechkova and @jku! And kudos to the reviewers, @joshuagl and @MVrachev. ❤️

I won't add any meaningful comments here, but based on a quick glance through the diff and all the other comments and discussions above, I'm sure the approval is well justified. :)

sechkova requested a review from jku December 22, 2020 18:20

sechkova mentioned this pull request Dec 22, 2020

updater: abstract out the network IO #1213

Closed

sechkova mentioned this pull request Dec 22, 2020

PoC: Let user download targets #1171

Closed

3 tasks

joshuagl mentioned this pull request Jan 4, 2021

Client explicitly asks server to not compress data #1251

Closed

sechkova force-pushed the fetcher branch from 96ed767 to 6124c39 Compare January 5, 2021 09:55

sechkova force-pushed the fetcher branch from 6124c39 to b1cb6e8 Compare January 6, 2021 08:24

jku mentioned this pull request Jan 8, 2021

tests: flaky windows server startup failure on github action #1255

Closed

sechkova force-pushed the fetcher branch from c460df3 to 189ab0c Compare January 8, 2021 13:49

jku mentioned this pull request Jan 11, 2021

tests: localhost name lookup takes 1sec on Windows in github actions #1257

Closed

sechkova force-pushed the fetcher branch 3 times, most recently from ebef202 to e8be76b Compare January 18, 2021 12:51

joshuagl reviewed Jan 18, 2021

View reviewed changes

jku reviewed Jan 19, 2021

View reviewed changes

sechkova force-pushed the fetcher branch 3 times, most recently from a749a0d to abac87f Compare January 22, 2021 14:21

MVrachev reviewed Jan 22, 2021

View reviewed changes

joshuagl approved these changes Feb 23, 2021

View reviewed changes

sechkova added 9 commits February 23, 2021 17:58

Add FetcherInterface class

84957c8

The new class FetcherInterface defines an interface for abstract network download which can be implemented for a variety of network libraries and configurations. Signed-off-by: Teodora Sechkova <[email protected]>

Implement RequestsFetcher class

41ffe7a

A concrete implementation of FetcherInterface based on the Requests library. Signed-off-by: Teodora Sechkova <[email protected]>

Move network IO logic to RequestsFetcher

815fe24

Abstract the network IO. Move the network operations from tuf.download to the RequestsFercher class which is TUF's implementation of the abstract FetcherInterface. Signed-off-by: Teodora Sechkova <[email protected]>

Add fetcher as parameter to Updater class

33b89a8

Initialize Updater with an external implementation of FetcherInterface. If not provided, tuf.fetcher.RequestsFetcher is used. Signed-off-by: Teodora Sechkova <[email protected]>

Update tests importing tuf.download

6c49792

Pass RequestsFetcher object to tuf.download functions. Signed-off-by: Teodora Sechkova <[email protected]>

Add test_fetcher

7dc5ef6

Add unit test for requests_fetcher.py Signed-off-by: Teodora Sechkova <[email protected]>

Apply a defensive data length check in fetch()

29e3419

Use '>=' as the defensive version of the equality check. Add a comment describing the need of a chunks() generator. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the fetcher branch from e77cbd0 to 4ab26ca Compare February 23, 2021 15:59

sechkova and others added 6 commits February 24, 2021 11:41

Test downloading data in more than one chunk

50b3b19

Add test cases to test_fetcher and test_download that decrease default chunk size and download data in more than one chunk. Small code-style improvements. Signed-off-by: Teodora Sechkova <[email protected]>

Close temp file in test_proxy_use.py

055280b

Calls to safe_download and unsafe_download leave a temporary file unclosed. Signed-off-by: Teodora Sechkova <[email protected]>

Add an HTTP error for Fetcher interface

e9b294b

A custom error is required so that updater is able to special case 403 & 404 status codes. Rewrite the test case a bit to be more readable. Signed-off-by: Jussi Kukkonen <[email protected]>

Add host address as a test level constant

2af63cf

Use a common test level constant for defining the host address forming the download URL on the client side. Signed-off-by: Teodora Sechkova <[email protected]>

Apply the new code style to fetcher docstrings

93c6573

Ensure that the newly added files' docstrings adhere to the recently adopted code style guideline (theupdateframework#1232). Small code style improvements in comments and imports. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the fetcher branch from 4ab26ca to 93c6573 Compare February 24, 2021 09:41

sechkova mentioned this pull request Mar 1, 2021

WIP: Client refactor #1291

Merged

17 tasks

jku merged commit ab1520e into theupdateframework:develop Mar 3, 2021

sechkova mentioned this pull request Mar 12, 2021

Split Updater into logical components: redesign mirrors.py and download.py #1307

Closed

4 tasks

jku mentioned this pull request Mar 18, 2021

client/updater design: let user download targets? #1142

Closed

sechkova deleted the fetcher branch March 25, 2021 13:20

joshuagl mentioned this pull request Apr 7, 2021

Design client library #1135

Closed

effulgentsia mentioned this pull request Apr 12, 2021

Allow PreFileDownloadEvent to carry transport options for metadata composer/composer#9831

Merged

Abstract out the network IO #1250

Abstract out the network IO #1250

Uh oh!

Conversation

sechkova commented Dec 22, 2020

Uh oh!

sechkova commented Dec 22, 2020

Uh oh!

sechkova commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sechkova commented Jan 6, 2021

Uh oh!

sechkova commented Jan 12, 2021

Uh oh!

lukpueh commented Jan 12, 2021

Uh oh!

trishankatdatadog commented Jan 12, 2021

Uh oh!

jku commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Jan 12, 2021

Uh oh!

jku commented Jan 12, 2021

Uh oh!

joshuagl commented Jan 14, 2021

Uh oh!

sechkova commented Jan 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jku Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sechkova commented Jan 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sechkova Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuagl commented Feb 16, 2021

Uh oh!

sechkova commented Feb 17, 2021

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

sechkova commented Jan 5, 2021 •

edited

Loading

jku commented Jan 12, 2021 •

edited

Loading

jku left a comment •

edited

Loading

jku Jan 19, 2021 •

edited

Loading

sechkova Jan 26, 2021 •

edited

Loading

sechkova Feb 24, 2021 •

edited

Loading