Do not immediately verify latest root when rotating #885

trishankatdatadog · 2019-06-04T18:56:53Z

Fixes issue #:

N/A

Description of the changes being introduced by the pull request:

When rotating the root (say from v2 to v3, then v3 to v4), the reference implementation currently immediately verifies signatures on the latest root (say, v4). This will fail when v4 is signed using v3's but not v2's keys. This PR changes the implementation so that signatures on the latest root is not immediately verified.

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

JustinCappos · 2019-06-04T21:05:31Z

@tanishqjasoria Can you also take a look?

trishankatdatadog · 2019-06-04T21:12:50Z

Yeah, I should say that someone should triple-check this to make we are not skipping some important security check. But at that point, all we are trying to do is get the latest version number of the root.

Cc @lukpueh

tanishqjasoria · 2019-06-05T18:42:27Z

I guess there might be a problem. Form switching to _get_file() from _get_metadata_file(), the root.json we get is not verified.
Then, we are checking if the root metadata is changed or not by comparing the next_version and latest_version and the latest_version is extracted from the unverified root.json file.
So if the unverified root.json file points that the latest_version is same as the current_version, then the root metadata would not be updated even if there is an update available and we would continue using the old root metadata.

trishankatdatadog · 2019-06-05T18:46:27Z

@tanishqjasoria Good observation, but this would be true even if with a signed and verified "latest" root metadata file: attackers who control the repository can always perform a freeze attack (limited by the expiration of the previous root, which could be as long as years).

tanishqjasoria · 2019-06-05T19:53:14Z

@trishankatdatadog I missed that!

tuf/client/updater.py

lukpueh

Thanks for the patch, @trishankatdatadog! I am not sure I understand the full consequences of your changes. Could you provide answers to my questions below?

tuf/client/updater.py

lukpueh · 2019-06-10T14:21:47Z

tuf/client/updater.py

+      # should refresh them here as a protective measure.  See Issue #736.
+      self._rebuild_key_and_role_db()
+      self.consistent_snapshot = \
+          self.metadata['current']['root']['consistent_snapshot']


Comment in L1152-1154 claims that self.consistent_snapshot is set with _update_metadata().

https://github.com/theupdateframework/tuf/blob/c75a2b7fe1eaabc88f82a5f0bdfc8bd25cab7ab1/tuf/client/updater.py#L1152-L1154

Why set it here explicitly? Is the comment above inaccurate?

IDK, it's possible.

tuf/client/updater.py

lukpueh

I somehow missed the call to _update_metadata in the top line of below for-loop. It downloads all the root files again using _get_metadata_file exactly how it used to fetch latest_root_metadata_file, before your patch. That means, all the checks that I listed in my last review are performed eventually.

So I'd say, LGTM! :)

Do you have capacities to add a test that has at least one intermediate root and where the latest root is signed with keys that are not available in the beginning?

On a side-note, I re-read the relevant part of the spec and it seems a bit different from what we do here. As per the spec the client downloads root files with incrementing version numbers until the server 404s. So in the spec there is no initial download of the latest root.

And on another side-note, did you see @vladimir-v-diaz's TODO comment in the refresh, where _update_root_metadata is called?

https://github.com/theupdateframework/tuf/blob/cf6fdec5101fec5e2402dc8d073fa17ace1fc25f/tuf/client/updater.py#L1079-L1082

trishankatdatadog · 2019-06-13T21:29:23Z

@lukpueh Following our discussion, I have adjusted the code to follow the spec. Please review.

To answer your questions:

Could someone else please help add the tests? Maybe an intern...
That TODO requires a wider discussion on our mailing list. @JustinCappos

trishankatdatadog · 2019-06-13T21:33:33Z

Our spec should also be updated to say: once you have turned on consistent snapshots, you should always also write versioned besides unversioned root metadata files so that outdated clients can updated to your latest root. @JustinCappos

Update: theupdateframework/specification#52

lukpueh

Thanks for your update, @trishankatdatadog. It does look more like the spec now. However, the code does not work yet. @JustinCappos, do we have an intern who can take over, i.e. fix the things I noted below and add some tests?

tuf/client/updater.py

trishankatdatadog · 2019-06-18T15:59:50Z

@lukpueh Sorry, I wrote on one machine, and tested on another, so there were some mistakes in translation.

Does this work better for you?

lukpueh

Thanks for the updates! I'm unsure whether you need request.exceptions. Otherwise it looks good.

tuf/client/updater.py

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

lukpueh · 2019-07-02T08:19:47Z

Thanks for the update, @trishankatdatadog! This looks sound now. I'd still like to see tests before we merge. @JustinCappos, do we have a student you might want to volunteer? If not, I'll add some tests.

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

trishankatdatadog · 2019-10-02T19:42:52Z

This is very important to Datadog for our next update. Could we get this fixed ASAP? Thanks.

lukpueh · 2019-10-03T12:51:06Z

FYI. I am working on tests...

tuf/client/updater.py

trishankatdatadog · 2019-10-03T17:41:41Z

Thanks for helping to write the tests, @lukpueh...

lukpueh · 2019-10-04T14:43:11Z

And here they are: trishankatdatadog/tuf@b170601...lukpueh:correctly-rotate-root-test

@trishankatdatadog, could you please sign off your most recent commit? And would you mind adding my three tests-adding commits linked above on top of your branch? Then we can review the tests here on the same PR.

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

…trishankatdatadog/tuf into trishankatdatadog/correctly-rotate-root Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>

trishankatdatadog · 2019-10-04T21:45:43Z

@lukpueh I had to manually set DCO to pass, should be OK (only one commit done directly from GitHub didn't have sign off). Great test, BTW. Let me know if you need anything else!

…ectly-rotate-root Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh · 2019-10-07T10:03:59Z

Merged manually via 3d342e6. Tests will be added with #930.

lukpueh · 2019-10-07T10:05:15Z

Thanks for the fix, @trishankatdatadog!! 💯

Since theupdateframework#885 the tests in TestUpdater and TestKeyRevocation fail on Appveyor Python 2.7 builds. After some live debugging, it turns out that the tests fail due to the extra amount of http requests to the simple http server (see tests/simple_server.py) that were added in theupdateframework#885. The simple server runs in a subprocess and is re-used for the entire TestCase. After a certain amount of requests it becomes unresponsive. Note that neither the subprocess exits (ps -W), nor does the port get closed (netstat -a). It just doesn't serve the request, making it time out and fail the test. The following script can be used to reproduce the issue (run in tests directory): ```python import subprocess import requests import random counter = 0 port = random.randint(30000, 45000) command = ['python', 'simple_server.py', str(port)] server_process = subprocess.Popen(command, stderr=subprocess.PIPE) url = 'http://localhost:'+str(port) + '/' sess = requests.Session() try: while True: sess.get(url, timeout=3) counter +=1 finally: print(counter) server_process.kill() ``` It fails repeatedly on the 69th request, but only if `stderr=subprocess.PIPE` is passed to Popen. Given that for each request the simple server writes about ~60 characters to stderr, e.g. ``` 127.0.0.1 - - [24/Feb/2020 12:01:23] "GET / HTTP/1.1" 200 - ``` ... it looks a lot like a full pipe buffer of size 4096. Note that the `bufsize` argument to Popen does not change anything. As a simple work around we silence the test server on Windows/Python2 to not fill the buffer.

Since theupdateframework#885 the tests in TestUpdater and TestKeyRevocation fail on Appveyor Python 2.7 builds. After some live debugging, it turns out that the tests fail due to the extra amount of http requests to the simple http server (see tests/simple_server.py) that were added in theupdateframework#885. The simple server runs in a subprocess and is re-used for the entire TestCase. After a certain amount of requests it becomes unresponsive. Note that neither the subprocess exits (ps -W), nor does the port get closed (netstat -a). It just doesn't serve the request, making it time out and fail the test. The following script can be used to reproduce the issue (run in tests directory): ```python import subprocess import requests import random counter = 0 port = random.randint(30000, 45000) command = ['python', 'simple_server.py', str(port)] server_process = subprocess.Popen(command, stderr=subprocess.PIPE) url = 'http://localhost:'+str(port) + '/' sess = requests.Session() try: while True: sess.get(url, timeout=3) counter +=1 finally: print(counter) server_process.kill() ``` It fails repeatedly on the 69th request, but only if `stderr=subprocess.PIPE` is passed to Popen. Given that for each request the simple server writes about ~60 characters to stderr, e.g. ``` 127.0.0.1 - - [24/Feb/2020 12:01:23] "GET / HTTP/1.1" 200 - ``` ... it looks a lot like a full pipe buffer of size 4096. Note that the `bufsize` argument to Popen does not change anything. As a simple work around we silence the test server on Windows/Python2 to not fill the buffer. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Since theupdateframework#885 the tests in TestUpdater and TestKeyRevocation fail on Appveyor Python 2.7 builds. After some live debugging, it turns out that the tests fail due to the extra amount of http requests to the simple http server (see tests/simple_server.py) that were added in theupdateframework#885. The simple server runs in a subprocess and is re-used for the entire TestCase. After a certain amount of requests it becomes unresponsive. Note that neither the subprocess exits (ps -W), nor does the port get closed (netstat -a). It just doesn't serve the request, making it time out and fail the test. The following script can be used to reproduce the issue (run in tests directory): ```python import subprocess import requests import random counter = 0 port = random.randint(30000, 45000) command = ['python', 'simple_server.py', str(port)] server_process = subprocess.Popen(command, stderr=subprocess.PIPE) url = 'http://localhost:'+str(port) + '/' sess = requests.Session() try: while True: sess.get(url, timeout=3) counter +=1 finally: print(counter) server_process.kill() ``` It fails repeatedly on the 69th request, but only if `stderr=subprocess.PIPE` is passed to Popen. Given that for each request the simple server writes about ~60 characters to stderr, e.g. ... ``` 127.0.0.1 - - [24/Feb/2020 12:01:23] "GET / HTTP/1.1" 200 - ``` ... it looks a lot like a full pipe buffer of size 4096. Note that the `bufsize` argument to Popen does not change anything. As a simple work around we silence the test server on Windows/Python2 to not fill the buffer. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

JustinCappos requested a review from lukpueh June 4, 2019 21:03

trishankatdatadog commented Jun 6, 2019

View reviewed changes

tuf/client/updater.py Outdated Show resolved Hide resolved

lukpueh reviewed Jun 10, 2019

View reviewed changes

lukpueh reviewed Jun 13, 2019

View reviewed changes

lukpueh requested changes Jun 17, 2019

View reviewed changes

trishankatdatadog force-pushed the trishankatdatadog/correctly-rotate-root branch 2 times, most recently from d6359e1 to eb81246 Compare June 18, 2019 19:56

trishankatdatadog mentioned this pull request Jun 18, 2019

Limit on the # of root metadata files downloaded theupdateframework/specification#45

Merged

lukpueh reviewed Jun 19, 2019

View reviewed changes

tuf/client/updater.py Outdated Show resolved Hide resolved

trishankatdatadog added 9 commits July 1, 2019 13:13

do not immediately verify latest root when rotating

92ae4eb

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

after verification, next root key is loaded as current

851913b

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

some minor updates

d8c6680

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

follow the spec

b0be9a2

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

address @lukpueh comments

704b90c

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

safely ignore only HTTP 403 / 404 errors when updating root

7e77327

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

better handling of HTTP errors

4839f20

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

fix a bug where we return incorrectly

3e05cfb

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

remove unnecessary conditional check

5fc3d90

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

trishankatdatadog force-pushed the trishankatdatadog/correctly-rotate-root branch from fbff603 to 5fc3d90 Compare July 1, 2019 17:21

reduce the excessive # of rotation checks

3bb4f73

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

trishankatdatadog force-pushed the trishankatdatadog/correctly-rotate-root branch from 54a101b to 3bb4f73 Compare July 2, 2019 14:50

JustinCappos assigned tanishqjasoria Jul 3, 2019

trishankatdatadog added this to the 0.12.0 milestone Oct 2, 2019

lukpueh assigned lukpueh and unassigned tanishqjasoria Oct 3, 2019

lukpueh mentioned this pull request Oct 3, 2019

Bump version 0.12.0 #921

Merged

trishankatdatadog commented Oct 3, 2019

View reviewed changes

tuf/client/updater.py Outdated Show resolved Hide resolved

Update tuf/client/updater.py

e34bdcc

trishankatdatadog added 2 commits October 4, 2019 17:40

reduce the excessive # of rotation checks

224bba7

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

Merge branch 'trishankatdatadog/correctly-rotate-root' of github.com:…

cd2a361

…trishankatdatadog/tuf into trishankatdatadog/correctly-rotate-root Signed-off-by: Trishank K Kuppusamy <trishank.kuppusamy@datadoghq.com>

trishankatdatadog force-pushed the trishankatdatadog/correctly-rotate-root branch from 7f32a1e to cd2a361 Compare October 4, 2019 21:42

lukpueh added a commit that referenced this pull request Oct 7, 2019

Merge pull request #885 from trishankatdatadog:trishankatdatadog/corr…

3d342e6

…ectly-rotate-root Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

lukpueh mentioned this pull request Oct 7, 2019

Add tests for multiple root key rotation #930

Merged

3 tasks

lukpueh closed this Oct 7, 2019

lukpueh mentioned this pull request Oct 14, 2019

Update securesystemslib to 0.12.0 #934

Merged

3 tasks

lukpueh mentioned this pull request Oct 24, 2019

Ambiguity between section 5.1.3 and 6.1 regarding updating root keys theupdateframework/specification#56

Closed

lukpueh mentioned this pull request Feb 24, 2020

Fix failing AppVeyor Python2.7 tests #985

Merged

3 tasks

trishankatdatadog deleted the trishankatdatadog/correctly-rotate-root branch July 13, 2020 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not immediately verify latest root when rotating #885

Do not immediately verify latest root when rotating #885

trishankatdatadog commented Jun 4, 2019

JustinCappos commented Jun 4, 2019

trishankatdatadog commented Jun 4, 2019

tanishqjasoria commented Jun 5, 2019

trishankatdatadog commented Jun 5, 2019 •

edited

Loading

tanishqjasoria commented Jun 5, 2019

lukpueh left a comment

lukpueh Jun 10, 2019

trishankatdatadog Jun 10, 2019

lukpueh left a comment

trishankatdatadog commented Jun 13, 2019

trishankatdatadog commented Jun 13, 2019 •

edited

Loading

lukpueh left a comment

trishankatdatadog commented Jun 18, 2019

lukpueh left a comment

lukpueh commented Jul 2, 2019

trishankatdatadog commented Oct 2, 2019

lukpueh commented Oct 3, 2019

trishankatdatadog commented Oct 3, 2019

lukpueh commented Oct 4, 2019

trishankatdatadog commented Oct 4, 2019

lukpueh commented Oct 7, 2019

lukpueh commented Oct 7, 2019

Do not immediately verify latest root when rotating #885

Do not immediately verify latest root when rotating #885

Conversation

trishankatdatadog commented Jun 4, 2019

JustinCappos commented Jun 4, 2019

trishankatdatadog commented Jun 4, 2019

tanishqjasoria commented Jun 5, 2019

trishankatdatadog commented Jun 5, 2019 • edited Loading

tanishqjasoria commented Jun 5, 2019

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh Jun 10, 2019

Choose a reason for hiding this comment

trishankatdatadog Jun 10, 2019

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Jun 13, 2019

trishankatdatadog commented Jun 13, 2019 • edited Loading

lukpueh left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Jun 18, 2019

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh commented Jul 2, 2019

trishankatdatadog commented Oct 2, 2019

lukpueh commented Oct 3, 2019

trishankatdatadog commented Oct 3, 2019

lukpueh commented Oct 4, 2019

trishankatdatadog commented Oct 4, 2019

lukpueh commented Oct 7, 2019

lukpueh commented Oct 7, 2019

trishankatdatadog commented Jun 5, 2019 •

edited

Loading

trishankatdatadog commented Jun 13, 2019 •

edited

Loading