ngclient: review download code

[jku]

(this is about code currently in experimental-client branch, hopefully soon in develop)

After we removed mirrors support, we should look through the download code

• Add type annotations
• replace old style docstrings
• see if code can be simplified
• consider removing average download speed check (it is useless in practice, will trigger false positives if the limit was usefully low and makes the code complex). It also prevents setting chunk size to a reasonable value (it's currently way too large but I'm afraid to lower it because the average speed check then becomes even more volatile for the first chunks -- if the limit is even close to reasonable, it could trigger for the first chunk much easier than later chunks just because that's how statistics works)
• consider getting rid of strict_required_length: I do not see the value, and it makes the code more complex (if on the other hand there is actual value in length checks, we should implement them in MetadataBundle: the spec does not require them but they would be easy to add)

[jku]

Additional reasoning for length check opinion:

if on the other hand there is actual value in length checks, we should implement them in MetadataBundle

• MetadataBundle ensures new metadata is correct: it checks hashes, signatures and data validity. checking that length is correct fits right in there (even though spec does not require it)
• the download code on the other hand should worry about A) getting the requested bytes B) not downloading more than is needed

[sechkova]

• consider removing average download speed check (it is useless in practice, will trigger false positives if the limit was usefully low and makes the code complex). It also prevents setting chunk size to a reasonable value (it's currently way too large but I'm afraid to lower it because the average speed check then becomes even more volatile for the first chunks -- if the limit is even close to reasonable, it could trigger for the first chunk much easier than later chunks just because that's how statistics works)

Given that defense against slow retrieval attack is considered optional and hard to specify (theupdateframework/specification#124), this sounds like a fair decision.

• consider getting rid of strict_required_length: I do not see the value, and it makes the code more complex (if on the other hand there is actual value in length checks, we should implement them in MetadataBundle: the spec does not require them but they would be easy to add)

I agree about strict length check. I think we can still keep an upper boundary length check without much cost.

[sechkova]

In addition we can consider creating a Download class with a Download object owned by Updater and instantiated with a Fetcher (default or user-provided).
We can probably keep Fetcher as it is now (the plug-in networking) and Download will add the file object handling.