Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ngclient rollback improvements #1524

Merged
merged 4 commits into from
Aug 24, 2021
Merged

ngclient rollback improvements #1524

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0911972
ngclient: Remove root_update_finished()
Aug 18, 2021
b515997
ngclient: Allow limited use of expired timestamp/snapshot
Aug 18, 2021
e3b789c
ngclient: allow limited use of wrong snapshot version
Aug 18, 2021
c8696d9
tests: Remove unhelpful helper
Aug 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 8 additions & 20 deletions tests/test_trusted_metadata_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ def _root_updated_and_update_timestamp(

"""
timestamp_bytes = timestamp_bytes or self.metadata["timestamp"]
self.trusted_set.root_update_finished()
self.trusted_set.update_timestamp(timestamp_bytes)


Expand All @@ -118,7 +117,6 @@ def _update_all_besides_targets(


def test_update(self):
self.trusted_set.root_update_finished()
self.trusted_set.update_timestamp(self.metadata["timestamp"])
self.trusted_set.update_snapshot(self.metadata["snapshot"])
self.trusted_set.update_targets(self.metadata["targets"])
Expand All @@ -139,24 +137,16 @@ def test_update(self):
self.assertTrue(count, 6)

def test_out_of_order_ops(self):
# Update timestamp before root is finished
with self.assertRaises(RuntimeError):
self.trusted_set.update_timestamp(self.metadata["timestamp"])

self.trusted_set.root_update_finished()
with self.assertRaises(RuntimeError):
self.trusted_set.root_update_finished()

# Update root after a previous successful root update
with self.assertRaises(RuntimeError):
self.trusted_set.update_root(self.metadata["root"])

# Update snapshot before timestamp
with self.assertRaises(RuntimeError):
self.trusted_set.update_snapshot(self.metadata["snapshot"])

self.trusted_set.update_timestamp(self.metadata["timestamp"])

# Update root after timestamp
with self.assertRaises(RuntimeError):
self.trusted_set.update_root(self.metadata["root"])

# Update targets before snapshot
with self.assertRaises(RuntimeError):
self.trusted_set.update_targets(self.metadata["targets"])
Expand Down Expand Up @@ -198,8 +188,6 @@ def test_update_with_invalid_json(self):
with self.assertRaises(exceptions.RepositoryError):
self.trusted_set.update_root(self.metadata["snapshot"])

self.trusted_set.root_update_finished()

top_level_md = [
(self.metadata["timestamp"], self.trusted_set.update_timestamp),
(self.metadata["snapshot"], self.trusted_set.update_snapshot),
Expand Down Expand Up @@ -241,15 +229,16 @@ def test_update_root_new_root_ver_same_as_trusted_root_ver(self):
with self.assertRaises(exceptions.ReplayedMetadataError):
self.trusted_set.update_root(self.metadata["root"])

def test_root_update_finished_expired(self):
def test_root_expired_final_root(self):
def root_expired_modifier(root: Root) -> None:
root.expires = datetime(1970, 1, 1)

# intermediate root can be expired
root = self.modify_metadata("root", root_expired_modifier)
tmp_trusted_set = TrustedMetadataSet(root)
# call root_update_finished when trusted root has expired
# update timestamp to trigger final root expiry check
with self.assertRaises(exceptions.ExpiredMetadataError):
tmp_trusted_set.root_update_finished()
tmp_trusted_set.update_timestamp(self.metadata["timestamp"])


def test_update_timestamp_new_timestamp_ver_below_trusted_ver(self):
Expand All @@ -275,7 +264,6 @@ def bump_snapshot_version(timestamp: Timestamp) -> None:
self.trusted_set.update_timestamp(self.metadata["timestamp"])

def test_update_timestamp_expired(self):
self.trusted_set.root_update_finished()
# new_timestamp has expired
def timestamp_expired_modifier(timestamp: Timestamp) -> None:
timestamp.expires = datetime(1970, 1, 1)
Expand Down
42 changes: 9 additions & 33 deletions tuf/ngclient/_internal/trusted_metadata_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
>>> # update root from remote until no more are available
>>> with download("root", trusted_set.root.signed.version + 1) as f:
>>> trusted_set.update_root(f.read())
>>> # ...
>>> trusted_set.root_update_finished()
>>>
>>> # load local timestamp, then update from remote
>>> try:
Expand All @@ -58,9 +56,6 @@
a generic RepositoryError that covers every issue that server provided
metadata could inflict (other errors would be user errors), but this is not
yet the case
* usefulness of root_update_finished() can be debated: it could be done
in the beginning of load_timestamp()...
* some metadata interactions might work better in Metadata itself
* Progress through Specification update process should be documented
(not sure yet how: maybe a spec_logger that logs specification events?)
"""
Expand Down Expand Up @@ -99,7 +94,6 @@ def __init__(self, root_data: bytes):
"""
self._trusted_set = {} # type: Dict[str: Metadata]
self.reference_time = datetime.utcnow()
self._root_update_finished = False

# Load and validate the local root metadata. Valid initial trusted root
# metadata is required
Expand Down Expand Up @@ -144,7 +138,7 @@ def update_root(self, data: bytes):
"""Verifies and loads 'data' as new root metadata.

Note that an expired intermediate root is considered valid: expiry is
only checked for the final root in root_update_finished().
only checked for the final root in update_timestamp().

Args:
data: unverified new root metadata as bytes
Expand All @@ -153,10 +147,8 @@ def update_root(self, data: bytes):
RepositoryError: Metadata failed to load or verify. The actual
error type and content will contain more details.
"""
if self._root_update_finished:
raise RuntimeError(
"Cannot update root after root update is finished"
)
if self.timestamp is not None:
raise RuntimeError("Cannot update root after timestamp")
logger.debug("Updating root")

try:
Expand All @@ -183,26 +175,6 @@ def update_root(self, data: bytes):
self._trusted_set["root"] = new_root
logger.debug("Updated root")

def root_update_finished(self):
"""Marks root metadata as final and verifies it is not expired

Raises:
ExpiredMetadataError: The final root metadata is expired.
"""
if self._root_update_finished:
raise RuntimeError("Root update is already finished")

if self.root.signed.is_expired(self.reference_time):
raise exceptions.ExpiredMetadataError("New root.json is expired")

# No need to delete timestamp/snapshot here as specification instructs
# for fast-forward attack recovery: timestamp/snapshot can not be
# loaded at this point and when loaded later they will be verified
# with current root keys.

self._root_update_finished = True
logger.debug("Verified final root.json")

def update_timestamp(self, data: bytes):
"""Verifies and loads 'data' as new timestamp metadata.

Expand All @@ -213,11 +185,15 @@ def update_timestamp(self, data: bytes):
RepositoryError: Metadata failed to load or verify. The actual
error type and content will contain more details.
"""
if not self._root_update_finished:
raise RuntimeError("Cannot update timestamp before root")
if self.snapshot is not None:
raise RuntimeError("Cannot update timestamp after snapshot")

# client workflow 5.3.10: Make sure final root is not expired.
if self.root.signed.is_expired(self.reference_time):
raise exceptions.ExpiredMetadataError("Final root.json is expired")
# No need to check for 5.3.11 (fast forward attack recovery):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me think: did we do that check before?

Copy link
Member Author

@jku jku Aug 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current ngclient code has a similar comment, and does not do the check. the legacy updater does do this check because it may have untrusted metadata in memory

# timestamp/snapshot can not yet be loaded at this point

try:
new_timestamp = Metadata.from_bytes(data)
except DeserializationError as e:
Expand Down
3 changes: 0 additions & 3 deletions tuf/ngclient/updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,9 +320,6 @@ def _load_root(self) -> None:
# 404/403 means current root is newest available
break

# Verify final root
self._trusted_set.root_update_finished()

def _load_timestamp(self) -> None:
"""Load local and remote timestamp metadata"""
try:
Expand Down