Update TUF to handle HTTPS proxies #781

trishankatdatadog · 2018-08-30T23:53:42Z

Fixes issue #:

Unfortunately, TUF fails to work via HTTPS proxies, whereas the requests library does.

The problem is this well-intentioned but outdated code.

As the comment hints, I had borrowed that VerifiedHTTPSConnection code from pip back then, because I knew that python2 back then was not verifying hostnames on SSL certs. Unfortunately, this code is now severely outdated.

Description of the changes being introduced by the pull request:

The tuf.download module now uses the requests library. I have manually tested that it handles HTTPS proxies.

Changes:

Install requests and its dependencies by default.
Change the networking code to use requests instead of custom, outdated networking code.
Remove option to specify custom SSL certs via a TUF setting. Instead, we depend on clients exporting the REQUESTS_CA_BUNDLE environment variable to requests.
Remove tuf.settings.SLOW_START_GRACE_PERIOD, as this no longer seems terribly useful, because requests allow for setting connect and / or read timeouts.
We no longer explicitly check for supported schemes (e.g., http and https only). This was originally done to prevent potentially unsafe requests to files on disk, but requests does not handle opening local files anyway.

TODO:

Unfortunately, a few slow retrieval attack tests are failing. I'm too tired right now to figure out what is going on.
Use a separate requests.Session per repository mirror to avoid sharing state, and accidentally falling for unknown / unforeseen security issues.
Add unit tests to see whether: (1) hostnames are verified on SSL certs, and (2) HTTPS proxies are handled transparently
Add version number to the TUF user agent
Add documentation about: (1) exporting the REQUESTS_CA_BUNDLE and / or HTTPS_PROXY variables in order to handle HTTPS proxies, and (2) ensuring that different sessions are used per hostname
Add coverage tests, if necessary

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

trishankatdatadog · 2018-08-30T23:56:06Z

Cc @JustinCappos @SantiagoTorres @awwad

Need your eyes to carefully review, edit, and merge! 👀

JustinCappos

Jotted some quick thoughts down. Someone with more recent eyeballs in the code will likely find better things to comment on.

JustinCappos · 2018-08-31T00:49:28Z

ci-requirements.txt

 pylint
+requests


Can we just change the requests line in this commit unless the rest are relevant? I don't mind you doing the alpha-reorder, but please let's split the commits so we can see why things are done.vv(Same for requirements.in too please)

I'm happy to make the edits, but want to double check that these files aren't order sensitive...

JustinCappos · 2018-08-31T00:56:24Z

tuf/exceptions.py

@@ -173,7 +173,7 @@ def __init__(self, expected_length, observed_length):

  def __str__(self):
    return 'Observed length (' + repr(self.observed_length)+\
-           ') <= expected length (' + repr(self.expected_length) + ').'
+           ') != expected length (' + repr(self.expected_length) + ').'


I think we had this before because some adopters wanted to effectively turn this off and list max length allowed instead of the expected length for files. Why is this being changed? Is this change needed now?

The reason why I changed itfrom<=to!= is because if there is a bug in the way the download function works, nothing actually prevents it from downloading more than it should. This happened to me, and I found the error message very misleading (e.g., 9 <= 5).

Behavior here has not changed. It should not be possible to get a length greater than expected due to the way the download code was written (and now as well, I think). If the code is changed and it becomes possible to end up with more, we can change the error message, but '<' currently communicates an expectation that has some value.

JustinCappos · 2018-08-31T00:57:08Z

tuf/settings.py

-# A PEM (RFC 1422) file where you may find SSL certificate authorities
-# https://en.wikipedia.org/wiki/Certificate_authority
-# http://docs.python.org/2/library/ssl.html#certificates
-ssl_certificates = None


Does this mean that there are no trusted certs by default? What does None do in this context?

This used to be "use the default bundle that urllib resolves", instead we are going to use the environment variable that requests uses: REQUESTS_CA_BUNDLE that's here

Correct. I figured we don't need two ways to configure the same thing right now.

JustinCappos · 2018-08-31T00:57:44Z

tuf/settings.py

@@ -89,16 +84,6 @@
 # avoid being considered as a slow retrieval attack.
 MIN_AVERAGE_DOWNLOAD_SPEED = 50 #bytes/second

-# The time (in seconds) we ignore a server with a slow initial retrieval speed.
-SLOW_START_GRACE_PERIOD = 0.1 #seconds


Seems like a very tight bound. Should this be higher?

It is gone now. I think requests has a built-in start grace period.

Correct, requests has a connect() timeout.

JustinCappos · 2018-08-31T00:57:57Z

tuf/settings.py

@@ -80,7 +75,7 @@
 DEFAULT_TARGETS_REQUIRED_LENGTH = 5000000 #bytes

 # Set a timeout value in seconds (float) for non-blocking socket operations.
-SOCKET_TIMEOUT = 4 #seconds
+SOCKET_TIMEOUT = 5 #seconds


Why did this change?

Because network connections can be quite lossy in the real world. We found a customer who was timing out over HTTP, but that was more likely due to the stringest SLOW_START_GRACE_PERIOD of 0.1 seconds. This latter setting is gone now, because requests handles both connect() and read() timeouts.

JustinCappos · 2018-08-31T01:03:36Z

tuf/download.py

+# mirror, and it would pass the Session to be used in this module, but that
+# requires a much more invasive change.
+#
+_session = requests.Session()


Key to fix before merging. Using a separate session for each hostname part of the URL should be easy. Perhaps make this a dict?

Each hostname might be a bit overkill --- I was thinking a separate session per repository mirror, but better to be safe than sorry. Let me work on this.

JustinCappos · 2018-08-31T01:05:17Z

tuf/download.py

@@ -172,20 +163,6 @@ def unsafe_download(url, required_length):
  securesystemslib.formats.URL_SCHEMA.check_match(url)
  securesystemslib.formats.LENGTH_SCHEMA.check_match(required_length)

-  # Ensure 'url' specifies one of the URI schemes in


We probably should retain a comment explaining that this is where the http / https prefix from the settings.py file is checked (assuming that is the case) and that an ???Exception may be thrown as a result...

I removed the check for whether a given URL matches http or https. Originally, we did this to avoid processing file:/// requests, but requests doesn't open files anyway, so we should be okay. I'm more in favour of simplicity. I can't imagine requests opening more dangerous protocols than http or https, but please correct me if wrong.

JustinCappos · 2018-08-31T01:06:32Z

tuf/download.py

+        # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding#Directives
+        'Accept-Encoding': 'identity',
+        # The TUF user agent.
+        # TODO: Attach version number, which cannot be easily found right now.


What does this mean? Do you mean that TUF's communication protocol should have a version of its own so we might use different versions at different times? Do we want the version of TUF to be here instead?

I think it's just overwriting the User-Agent to something that's not requests or python urrlibX. It'd be cleaner (and easier to analyze traffic) if we used something like TUF vX

Correct. Sometimes you want to exactly which piece of software was downloading from your server. To be fair, it should be something like tuf-{tuf-version-number}-over-requests-{requests-version-number}.

SantiagoTorres

There are some very minor details. Otherwise it LGTM once the failing tests are addressed.

SantiagoTorres · 2018-08-31T16:25:51Z

tests/test_slow_retrieval_attack.py

    extra_bytes = 8
-    total_bytes = tuf.settings.SLOW_START_GRACE_PERIOD + extra_bytes
+    total_bytes = 100 * extra_bytes


I'd be happier if re-used that variable or assigned a constant for this 100

@awwad, could you help me fix this? I think this might be related to the problems you've been seeing with bad hashes, and so on. Thanks in advance!

SantiagoTorres · 2018-08-31T16:28:44Z

tuf/download.py

+        # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding#Directives
+        'Accept-Encoding': 'identity',
+        # The TUF user agent.
+        # TODO: Attach version number, which cannot be easily found right now.


I think it's just overwriting the User-Agent to something that's not requests or python urrlibX. It'd be cleaner (and easier to analyze traffic) if we used something like TUF vX

SantiagoTorres · 2018-08-31T16:32:23Z

tuf/settings.py

-# A PEM (RFC 1422) file where you may find SSL certificate authorities
-# https://en.wikipedia.org/wiki/Certificate_authority
-# http://docs.python.org/2/library/ssl.html#certificates
-ssl_certificates = None


This used to be "use the default bundle that urllib resolves", instead we are going to use the environment variable that requests uses: REQUESTS_CA_BUNDLE that's here

SantiagoTorres · 2018-08-31T16:33:19Z

tuf/settings.py

@@ -89,16 +84,6 @@
 # avoid being considered as a slow retrieval attack.
 MIN_AVERAGE_DOWNLOAD_SPEED = 50 #bytes/second

-# The time (in seconds) we ignore a server with a slow initial retrieval speed.
-SLOW_START_GRACE_PERIOD = 0.1 #seconds


It is gone now. I think requests has a built-in start grace period.

SantiagoTorres · 2018-08-31T16:36:12Z

tuf/download.py

-    connection.close()
+    # This else block returns and skips closing the response in the finally
+    # block, so close the response here.
+    response.close()


Not related to this PR, but this finally clause with the response.close is then useless. I don't think finally works like this at all...

Resolved now (with the conflict). The strange try/except/finally was removed in #771.

SantiagoTorres · 2018-08-31T16:37:29Z

tuf/download.py

+        # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding#Directives
+        'Accept-Encoding': 'identity',
+        # The TUF user agent.
+        # TODO: Attach version number, which cannot be easily found right now.


We definitely need something defined here, and add it to part of our release process...

SantiagoTorres · 2018-08-31T16:38:44Z

tests/test_download.py

@@ -168,44 +170,29 @@ def test_download_url_to_tempfileobj_and_urls(self):
    self.assertRaises(securesystemslib.exceptions.FormatError,
                      download_file, None, self.target_data_length)

-    self.assertRaises(securesystemslib.exceptions.FormatError,
+    self.assertRaises(requests.exceptions.MissingSchema,


Again, not related to this PR, but moving to assertRaises as a context processor would make this code more readable...

Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>

awwad · 2018-08-31T17:24:23Z

Rebased.
Lots of tests fail here, and some comments explaining the changes would help. For example, the URL validation (such as it was) is pretty much all removed (I think all that's left is just a check that the URL is a string.). This might be fine if the requests call handles that, but that would be good to mention in the commit message or a comment.

Edit: most of the failures I was seeing had to do with the virtual box I was running it in. 😆 Saw 24 failures and balked. My bad.

(sorry -- just to keep things simple) Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad · 2018-08-31T17:44:46Z

I'll finish looking through it to make sure I haven't missed anything subtle and then fix the tests.

trishankatdatadog · 2018-08-31T17:46:36Z

@awwad AFAICT with tox at least, there were 2 test_slow_retrieval_attack tests failing for sure. There was 1 nondeterministic failure which made 1 map file test error out, which was weird. Thanks for fixing the tests, I'll address the rest of the comments!

trishankatdatadog · 2018-08-31T20:56:42Z

I have a PR for the sessions issue, but I can't push it now for some network reason, and I gotta run now. See you guys Monday!

awwad · 2018-08-31T20:57:21Z

setup.py

 setup(
  name = 'tuf',
-  version = '0.11.1',
+  version = find_version("tuf", "__init__.py"),


This isn't going to work: read isn't defined. This is code from here and is a little more complicated than is ideal. It'll also match comment lines if they exist. Single-sourcing version number isn't necessary for this pull request, but if I was going to do it, I'd probably add a VERSION file and have tuf/__init__.py and setup.py each read that in. There could be problems with that, too. I'm going to punt on this and keep the version in two places and we can fix that less urgently. (Also, the user agent reporting a version seems less critical in any case than the rest of the PR.)

awwad · 2018-08-31T21:53:47Z

SlowRetrievalError is essentially part of the API (see ATTACKS.md), so I'll adjust this a bit to still raise that when we catch urllib3.exceptions.ReadTimeoutError().

trishankatdatadog · 2018-09-04T16:11:50Z

I am still having some strange network issues in my VM trying to push to / pull from GitHub. Let me send my updated PR as soon as I figure out what's going on.

awwad · 2018-09-04T16:29:51Z

Keep an eye out for any history differences? It wouldn't look like a network issue, but I did have to strip one of your commits (self-merge -- merging remote branch into local branch and then pushing) from the commit history.

Meanwhile, I'm looking at why bad hash errors are occurring in one of the tests where I don't expect them to occur. (Slow retrieval should cause an error that indicates that retrieval was slow/disrupted, not a bad hash.) The download code is a little kinky and probably could use some tidying up (in another PR, later, tbc).

trishankatdatadog · 2018-09-04T18:15:51Z

Okay, guys, I've added code to use a different session per hostname. Let me know what you think, and I'm happy to help with anything else!

P.S. There is not so much time pressure on this anymore, but the sooner we can fix this, the better :)

awwad · 2018-09-04T19:43:17Z

So, unlike the code without this PR merged, this branch downloads a target file all in the code that claims to make a connection. This makes the slow retrieval, endless data, etc. code moot (and also invalidates all the docstrings -- e.g. _download_fixed_amount_of_data is called on the response provided by requests, slowly reading out data from the request already retrieved from the server, and claims to be "where the download really happens").

Using stream=True in the requests.get() call might fix this. I'll tinker more.

(UPDATE: Hang on, I may have misunderstood something.....)

trishankatdatadog · 2018-09-04T20:14:53Z

We already use request.get(stream=True). Not sure what is going on to break the tests, I'm guessing some fragile assumptions were made...

awwad · 2018-09-05T16:08:57Z

Fixed some bad assumptions in the tests that resulted -- when the test failed -- in misleading errors (and clarified behavior with comments in the tests). Also raised ReadTimeoutError (from urllib) as an expected tuf.exceptions.SlowRetrievalError.

The test fails, however, because a slow retrieval attack is not prevented: streaming one byte per second for 400 (or, previously, 800) seconds is permitted to complete. I'll tinker and figure out how to prevent slow retrieval attacks with the requests calls.

trishankatdatadog · 2018-09-05T17:26:13Z

I wonder what's going on. Have you looked at the doc, Sebastien?

awwad · 2018-09-05T19:04:49Z

Yes. The current settings include a chunk size of 40k and a 5s timeout that is now interpreted by requests as a maximum gap between bytes (and also the timeout of the connect interval). This means that a transfer of 40k could take 50+ hours and still pass. We could tinker with those settings*, but what we really want is to interrupt the <=40k transfer if it's taking too long. I'm not sure if there's an option like that somewhere in requests.

* Barring a total-time-spent timeout for each chunk, I guess we could reduce the expected interval between bytes below one second (🙁), but we could also very substantially reduce the size of chunks and allow the existing means of measuring the rate of transfer to decide to raise SlowRetrievalError. I assume that'll lead to a slowdown. If we can't get a different behavior from requests, then max acceptable delay will be given by inter-byte timeout * chunk size. We can pick a sensible value (1 minute per chunk and a chunk size of 10kb?) and set tuf settings accordingly and add comments by the settings explaining their significance (and similar comments in the rate testing code).

trishankatdatadog · 2018-09-05T19:12:16Z

I think I sort of understand what's going on. Would you mind terribly much showing what you mean in the updated unit tests? Thanks in advance!

awwad · 2018-09-05T19:45:44Z

Not clear on the question, so in case you mean explaining the stack as it is tested, then:

updater.download_target() provides fpath len hashes to:
  -> updater._get_target_file() provides verifyfunc, fpath, len, safely=True to
    -> updater._get_file() provides mirrors and len to:
      -> tuf.download.safe_download provides len to:
        -> tuf.download._download_file(), which calls:      # the part we care about now
          -> tuf.download._download_fixed_amount_of_data()  # the part we care about now
        then tuf.download._check_downloaded_length
    then updater._get_file() continues by calling:
      -> verifyfunc (which is passed as verify_target_file, which calls updater._hard_check_file_length() and updater._check_hashes())

tuf.settings.SOCKET_TIMEOUT (5s) is used by tuf.download._download_file() to initialize a request object with a "timeout" that sets the aforementioned maximum time before the first byte and between further bytes. The request object is passed down to _download_fixed_amount_of_data(), which uses request.raw.read() with a chunk size of 40k (tuf.settings.CHUNK_SIZE) to read 40k at a time. Requests will time out if the first byte takes 5s to come, or any of the gaps between further bytes exceed 5s. After this first chunk is downloaded, _download_fixed_amount_of_data() checks to see if it's done. (It is; the test file provides 400b which is less than the chunk size of 40k, so it's done after one chunk.)
The call returns the data after 400+s and the test fails because no slow retrieval error was raised.

If there had been more than one chunk (if target file size > 40k), then _download_fixed_amount_of_data(), before continuing with another request.raw.read call for the second chunk, would check the amount of data requests provided and the time that passed during the call to requests against tuf.settings.MIN_AVERAGE_DOWNLOAD_SPEED. This check would fail in our case and an error would be raised, but only after the 400s for the first chunk. (If the file size were, say, 40k, this would have taken ~ 40,000 * delay between each byte).

Change Required:

The change that's called for (I'll make one or the other in a bit assuming I'm not off my rocker) is to either reduce chunk size and continue to use request.raw.read() or, instead of using request.raw.read(), call request.iter_content() with a much smaller chunk size (I think I prefer this option? Is there a reason raw.read() is preferable?), say 128b, resulting in the largest possible delay that can be caused by an attacker delivering at speeds less than that acceptable (tuf.settings.MIN_AVERAGE_DOWNLOAD_SPEED) being something like 11min (128 * tuf.settings.SOCKET_TIMEOUT) instead of 55 hours. That's probably still too long...? We can allow a 5s delay for the first byte and no more than 2s for further bytes, maybe (4.3min)? Smaller iter_content sizes are also possible. I'm not sure what the loss to efficiency would be, but I could test it, I guess.

TBC, ideally we could call request.iter_content(40k, max_duration=X seconds) or something, but I don't see that option anywhere after looking through some feature requests.

awwad · 2018-09-05T19:55:53Z

I feel like Vlad toyed with requests and elected not to use it? I don't really recall at this point -- maybe we used to use it and moved away.... This (no total timeout option for an individual read) would be a decent reason for that. Maybe there's a way to do this via urllib3? (don't see that yet, but haven't really looked properly).

UPDATE: don't see it in urllib3 after moderate search

trishankatdatadog · 2018-09-05T19:58:00Z

The problem with response.iter_content() is that it's a generator that uses a fixed-size chunk in every loop. That means you can't control downloading exactly the number of bytes you want, and avoid endless data attacks (admittedly small with a small chunk size, but still).

trishankatdatadog · 2018-09-05T19:59:20Z

I strongly recommend using requests as a backbone, because custom networking code is prone to problems that have been solved in production-tested libraries such as requests. requests also does a lot of things that urllib3 does not (such as by-default verification of SSL requests using certifi), and I would strongly recommend against writing our own networking stack, as this stuff is tricky, and easy to miss a lot of things that arise in the real world.

trishankatdatadog · 2018-09-05T20:08:48Z

Having said all this, the way we used to handle slow retrieval errors appears to be broken using requests, and so we need to think of satisfactory solutions. Your idea of using a small chunk size seems feasible, but it does result in a lot of network calls. Let's think a bit more, and see.

aaaaalbert · 2018-10-02T09:14:53Z

tests/test_proxy_use.py

+    HTTP proxy, and perform an HTTP connection with the final server.
+    """
+
+    logger.info('Trying http download with no proxy: ' + self.url)


If there isn't a striking reason for not capitalizing HTTP, I would suggest you do throughout.

aaaaalbert · 2018-10-02T09:15:39Z

tests/test_proxy_use.py

+
+
+
+  def test_httpS_dl_via_smart_http_proxy(self):


(I think I get the idea of capitalization ~ highlighting, but this looks terrible to me.)

aaaaalbert · 2018-10-02T09:20:46Z

tests/test_proxy_use.py

+      # recently overwrote it with.
+      pass
+
+    elif key in os.environ:


Perhaps remove the elif/else and do self.old_env_values[key] = os.environ.get(key, None).

(BTW, what about os.execveing things and supplying a modified environment directly, instead of saving/restoring the current one?)

Your suggestion reduced 6 lines to 2. Thanks!
As for execve: seems like a messy option. I'm not sure I'd want to fully replace the environment variables (instead of tweaking them) and I'm not sure I want to close the existing process and transition to a new one (temp files?). Popen execve? Not sure. I think this is probably easier and more acceptable.

aaaaalbert · 2018-10-02T09:21:46Z

tests/test_proxy_use.py

+      # del os.environ[key] should unset the variable. Otherwise, we'll just
+      # have to settle for setting it to an empty string.
+      # See os.environ in:
+      #    https://docs.python.org/2/library/os.html#process-parameters)


Remove trailing )

aaaaalbert · 2018-10-02T09:23:35Z

tests/test_proxy_use.py

+    #   This is only relevant if the proxy is in intercept mode.
+    cls.https_proxy_port = cls.http_port + 3
+    cls.https_proxy_proc = subprocess.Popen(
+        ['python', 'proxy_server.py', str(cls.https_proxy_port), 'intercept',


Use sys.executable instead of python. (It could be python3, python2.7 etc. depending on your setup.)

And/or consider Popening a Python interpreter a common function and spin it off into a separate function. (The code seems to do this a lot, across files in the PR).

- two reversions to unnecessary changes - some typo fixes - capitalization of HTTP/S where reasonable - commenting out code section with ''' rather than # Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

that draws from sys.executable (the currently running Python interpreter) instead of assuming 'python' is correct. Use this function instead of having many individual subprocess calls written out. Slightly simplifies code, too. This should eventually be moved to a common test module instead of appearing in two places in the test code. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

trishankatdatadog · 2018-10-02T19:34:57Z

Hi Sebastien,

Sorry, don't mean to press you, but could we please get a dev release at least ASAP? We have the Agent 6.6 release frozen at the end of next week, and I'd like to test this ASAP, so that we can update TUF in the Agent next week if all goes well. Thanks very much in advance!

Cc: @nmuesch @olivielpeau

awwad · 2018-10-02T19:36:46Z

No problem. I'll merge sometime today (and release).

trishankatdatadog · 2018-10-02T19:43:16Z

@nmuesch Can we get your customer to install the latest stable Agent, and then update TUF manually to see if that fixes the problem?

to make sure that the test uses the intended certificate. (There's some indirect indication that the updated environment variable might not always have been used.) Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

@aaaaalbert

After seeing some AppVeyor failures, I've increased the wait after starting test HTTP, HTTPS, and proxy servers from 0.5s to 1s, to make it less likely that tests will fail because the servers weren't done starting up yet. After some review comments by @aaaaalbert, I've tightened the logic in aggregate_tests.py around which tests to skip unless a certain Python version is running, and added some consistency checks. This also involved a bit of clarification of comments and variable names. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad · 2018-10-03T03:12:18Z

Merged and an alpha released produced: https://github.com/theupdateframework/tuf/releases/tag/v0.11.2-alpha

Oh, I should mention that I wasn't expecting to put this on PyPI until the vulnerability is resolved. I hope that's OK. I could be convinced otherwise, so if this is an issue, LMK. For now, it's just on GitHub.

awwad · 2018-10-03T03:12:29Z

@trishankatdatadog

trishankatdatadog · 2018-10-03T15:19:19Z

@awwad Could we please get a tuf-0.11.2.dev1 release on PyPI? Otherwise, it's hard for us to beta test this. The dev release ensures users cannot install it by accident (need to pass --pre flag to pip). Thanks!

awwad · 2018-10-03T16:35:45Z

https://pypi.org/project/tuf/0.11.2.dev1/
There's some confusion in the way we have dev version numbers listed, so mind the version.

trishankatdatadog · 2018-10-03T17:09:44Z

Thanks @awwad !

Since theupdateframework#781 we only provide limited protection against slow retrieval attacks. So far this has only been discussed in above issue and hinted at by a disabled test and a code comment in that test. This change adds a corresponding disclaimer to a more prominent place, i.e. the list of attacks in SECURITY.md.

Since theupdateframework#781 we only provide limited protection against slow retrieval attacks. So far this has only been discussed in above issue and hinted at by a disabled test and a code comment in that test. This change adds a corresponding disclaimer to a more prominent place, i.e. the list of attacks in SECURITY.md. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Since theupdateframework#781 we only provide limited protection against slow retrieval attacks. So far this has only been discussed in above issue and hinted at by a disabled test and a code comment in that test. This change adds a corresponding disclaimer to a more prominent place, i.e. the list of attacks in SECURITY.md. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu> Co-Authored-By: Trishank K Kuppusamy <33133073+trishankatdatadog@users.noreply.github.com>

trishankatdatadog force-pushed the trishankatdatadog/fix-for-https-proxies branch from 1e86d74 to 4863868 Compare August 30, 2018 23:55

JustinCappos reviewed Aug 31, 2018

View reviewed changes

SantiagoTorres previously requested changes Aug 31, 2018

View reviewed changes

Update TUF to handle HTTPS proxies

b9bc860

Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>

awwad force-pushed the trishankatdatadog/fix-for-https-proxies branch from 4863868 to b9bc860 Compare August 31, 2018 17:20

minor: undo some import/dependency re-ordering

1e97275

(sorry -- just to keep things simple) Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

trishankatdatadog force-pushed the trishankatdatadog/fix-for-https-proxies branch from 063c2c2 to c3a01bd Compare August 31, 2018 19:03

awwad force-pushed the trishankatdatadog/fix-for-https-proxies branch from c3a01bd to b059fc3 Compare August 31, 2018 20:29

awwad reviewed Aug 31, 2018

View reviewed changes

awwad force-pushed the trishankatdatadog/fix-for-https-proxies branch from 08ac96e to 7df8e46 Compare September 4, 2018 19:55

aaaaalbert reviewed Oct 2, 2018

View reviewed changes

awwad added 3 commits October 2, 2018 15:01

minor: PR tweaks based on review: doc, casing, typos, updates

ec27630

- two reversions to unnecessary changes - some typo fixes - capitalization of HTTP/S where reasonable - commenting out code section with ''' rather than # Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Test: simplify env variable overwrite/restore code in test_proxy_use

e8a1ab1

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Test: clear requests sessions when updating expected ssl certs

ebcb17b

to make sure that the test uses the intended certificate. (There's some indirect indication that the updated environment variable might not always have been used.) Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad force-pushed the trishankatdatadog/fix-for-https-proxies branch from dbc5338 to ab8990b Compare October 2, 2018 21:00

awwad force-pushed the trishankatdatadog/fix-for-https-proxies branch from ab8990b to 01d8d9e Compare October 2, 2018 21:07

awwad merged commit 01d8d9e into theupdateframework:develop Oct 2, 2018

trishankatdatadog deleted the trishankatdatadog/fix-for-https-proxies branch October 3, 2018 15:13

awwad mentioned this pull request Oct 3, 2018

Bump version to 0.11.2-dev1 for PyPI dev release #789

Merged

awwad mentioned this pull request Dec 18, 2018

TUF client requests TUF server through proxy #814

Closed

lukpueh mentioned this pull request Sep 20, 2019

Should we replace urllib(2) with requests? #209

Closed

lukpueh mentioned this pull request Oct 3, 2019

Add disclaimer about limited slow retrieval attack protection #928

Merged

3 tasks

lukpueh mentioned this pull request Oct 10, 2019

Reinstate full protection against slow retrieval attacks #932

Open

Update TUF to handle HTTPS proxies #781

Update TUF to handle HTTPS proxies #781

Conversation

trishankatdatadog commented Aug 30, 2018 • edited by awwad Loading

trishankatdatadog commented Aug 30, 2018

JustinCappos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trishankatdatadog Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantiagoTorres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad commented Aug 31, 2018 • edited Loading

awwad commented Aug 31, 2018

trishankatdatadog commented Aug 31, 2018

trishankatdatadog commented Aug 31, 2018

Choose a reason for hiding this comment

awwad commented Aug 31, 2018

trishankatdatadog commented Sep 4, 2018

awwad commented Sep 4, 2018 • edited Loading

trishankatdatadog commented Sep 4, 2018

awwad commented Sep 4, 2018 • edited Loading

trishankatdatadog commented Sep 4, 2018

awwad commented Sep 5, 2018 • edited Loading

trishankatdatadog commented Sep 5, 2018 • edited Loading

awwad commented Sep 5, 2018

trishankatdatadog commented Sep 5, 2018

awwad commented Sep 5, 2018 • edited Loading

Change Required:

awwad commented Sep 5, 2018 • edited Loading

trishankatdatadog commented Sep 5, 2018 • edited Loading

trishankatdatadog commented Sep 5, 2018 • edited Loading

trishankatdatadog commented Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trishankatdatadog commented Oct 2, 2018

awwad commented Oct 2, 2018 • edited Loading

trishankatdatadog commented Oct 2, 2018

awwad commented Oct 3, 2018 • edited Loading

awwad commented Oct 3, 2018

trishankatdatadog commented Oct 3, 2018

awwad commented Oct 3, 2018

trishankatdatadog commented Oct 3, 2018

trishankatdatadog commented Aug 30, 2018 •

edited by awwad

Loading

trishankatdatadog Aug 31, 2018 •

edited

Loading

awwad Aug 31, 2018 •

edited

Loading

awwad commented Aug 31, 2018 •

edited

Loading

awwad commented Sep 4, 2018 •

edited

Loading

awwad commented Sep 4, 2018 •

edited

Loading

awwad commented Sep 5, 2018 •

edited

Loading

trishankatdatadog commented Sep 5, 2018 •

edited

Loading

awwad commented Sep 5, 2018 •

edited

Loading

awwad commented Sep 5, 2018 •

edited

Loading

trishankatdatadog commented Sep 5, 2018 •

edited

Loading

trishankatdatadog commented Sep 5, 2018 •

edited

Loading

trishankatdatadog commented Sep 5, 2018 •

edited

Loading

awwad commented Oct 2, 2018 •

edited

Loading

awwad commented Oct 3, 2018 •

edited

Loading