Roles and Delegations are still confused in parts of the implementation #660

awwad · 2018-03-23T19:18:25Z

Several issues are related to this, including at least: #589, #646, #658, #736, spec issue 19.

Summary

The reference implementation continues to try to provide a 1-to-1 mapping of roles to keyids-the-role-should-be-signed-by-in-order-to-be-valid. This is not correct: the same role may need to be validated expecting different sets of keys, based on how the role was reached in the depth-first search while looking for target information.

The bottom line here is that code should be rewritten in places like roledb, to avoid things like get_role_keyids(), or roleinfo['threshold'] or roleinfo['keyids'], etc.

Detailed explanation

I think this all comes from a deeper issue:

Per the design of TUF, roles and delegations should never be regarded as the same thing. While in the last few years, there have emerged a few additional reasons that this separation is important (TAPs 3's multi-role delegations and TAP 5's alternative repository roots, for example), my understanding of the design history is that there was not originally an expectation of a one-to-one mapping of roles to delegations pointing to them.

So let's be pedantic:

A role is not the same thing as a delegation: a delegation is a relationship between two roles.
A delegation is a property of the delegating role, not the delegated-to role.
Therefore, for delegated roles, key IDs and thresholds are properties of a delegation, not properties of the delegated role.
You cannot simply map delegated roles to the keyids that validate them: you must map keys to delegations (that is, delegator + delegatee, if we're assuming no parallel edges in the graph (which, btw, are we?)).

The graph of delegations used to be guaranteed -- in the reference implementation -- to be a simple directed graph where Root is a node with an indegree of 0 and all other nodes have an indegree of 1. I.e.: No roles delegate to Root. Root delegates to Targets. Targets can delegate to various roles, but two or more roles never delegate to the same role. The design intention, however, did not guarantee maximum indegree of 1: in the design, delegations could be promiscuous: a single role can be delegated to by multiple roles.

A result of this confusion is that roledb still represents roles as flat in certain ways, when it should not assume this. This flatness resulted in #589, #658, #736, probably #646, and presumably other forgotten issues.

Take the function roledb.get_role_keyids() as an example: get_role_keyids takes only the name of a role (and a repository name), purporting to return the keys that that role should be signed by. The trouble, though, is that different roles delegating to that role can expect different keys and validate or reject role C's target information based on those different expectations.

Specification / Avoiding Future Issues

The spec itself should be a bit clearer -- see spec issue 19 -- about this, to help prevent these issues from arising in implementations. We may also need materials elsewhere to make the role/delegation distinction clearer. When editing the metadata formats in TAPs and the like, I've tried to make sure not to reinforce this confusion (mixing roles and delegations), and we should be mindful in that way, too.

As a sidenote, note that parallel edges would also cause this issue, not just promiscuous delegations from different origin nodes -- any role with indegree > 1 would have been an issue.

The text was updated successfully, but these errors were encountered:

add_restricted_paths was renamed to add_path; however, this function represents a problematic element of TUF that assumes that roles are have a single delegator and delegatee, and that one can refer to a role's expected keys without being concerned about any delegation metadata.... So this is being removed from the tutorial. In time, add_paths will either be removed or changed (to expect a delegator role and a delegatee role, not just a delegatee role). This comment does not do justice to the issue: please see TUF GitHub Issue #660: #660 Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

saving changes pending commit here, to switch tracks from ASN.1 support itself to finally resolve #660, which is causing problems here. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

- Rename and alter some schemas that really address delegations, to make that clear. - Do away with the ROLEDB_SCHEMA, an intermediate metadata format that is not necessary and which incorrectly flattens the delegation graph, and similar schemas. - Rewrite getters/setters in roledb to respect the delegation graph rather than assuming that delegated targets roles have only one delegation pointing to them (see Issue #660). - Add a variety of TODOs for later. - Clarify docstrings as a result of the above. reinterpreting metadata Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Kills classes MetaFile, RootFile, TargetsFile, MirrorsFile, SnapshotFile, and TimestampFile. They each had an unused from_ method and a used make_ method. They were all additional, unnecessary representations of the same metadata, and it is very important that metadata formats be defined once in the reference implementation, in the schemas that are already used more broadly, in foramts.py. Replaces the classes, their methods, and some associated variables with a single short function called build_dict_conforming_to_schema that takes keyword arguments and builds a dictionary, then checks to make sure that the result conforms to the given schema. This commit shifts repository_lib from use of the old classes to the new function. In later commits, we should use this function more broadly, since it can be of use in all schema construction. There are several TODOs added to the code, mostly for post-#660 tasks. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

- Rename and alter some schemas that really address delegations, to make that clear. - Do away with the ROLEDB_SCHEMA, an intermediate metadata format that is not necessary and which incorrectly flattens the delegation graph, and similar schemas. - Rewrite getters/setters in roledb to respect the delegation graph rather than assuming that delegated targets roles have only one delegation pointing to them (see Issue #660). - Add a variety of TODOs for later. - Clarify docstrings as a result of the above. reinterpreting metadata Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Kills classes MetaFile, RootFile, TargetsFile, MirrorsFile, SnapshotFile, and TimestampFile. They each had an unused from_ method and a used make_ method. They were all additional, unnecessary representations of the same metadata, and it is very important that metadata formats be defined once in the reference implementation, in the schemas that are already used more broadly, in foramts.py. Replaces the classes, their methods, and some associated variables with a single short function called build_dict_conforming_to_schema that takes keyword arguments and builds a dictionary, then checks to make sure that the result conforms to the given schema. This commit shifts repository_lib from use of the old classes to the new function. In later commits, we should use this function more broadly, since it can be of use in all schema construction. There are several TODOs added to the code, mostly for post-#660 tasks. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

- Rename and alter some schemas that really address delegations, to make that clear. - Do away with the ROLEDB_SCHEMA, an intermediate metadata format that is not necessary and which incorrectly flattens the delegation graph, and similar schemas. - Rewrite getters/setters in roledb to respect the delegation graph rather than assuming that delegated targets roles have only one delegation pointing to them (see Issue #660). - Add a variety of TODOs for later. - Clarify docstrings as a result of the above. reinterpreting metadata Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

- remove duplicate ROLENAME_SCHEMA and ROLEDICT_SCHEMA - remove outdated and duplicate ROLE_SCHEMA Note that this is a quick fix that may be overridden with refactoring work in theupdateframework#660/theupdateframework#846. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

add_restricted_paths was renamed to add_path; however, this function represents a problematic element of TUF that assumes that roles are have a single delegator and delegatee, and that one can refer to a role's expected keys without being concerned about any delegation metadata.... So this is being removed from the tutorial. In time, add_paths will either be removed or changed (to expect a delegator role and a delegatee role, not just a delegatee role). This comment does not do justice to the issue: please see TUF GitHub Issue theupdateframework#660: theupdateframework#660 Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

add_restricted_paths was renamed to add_path; however, this function represents a problematic element of TUF that assumes that roles are have a single delegator and delegatee, and that one can refer to a role's expected keys without being concerned about any delegation metadata.... So this is being removed from the tutorial. In time, add_paths will either be removed or changed (to expect a delegator role and a delegatee role, not just a delegatee role). This comment does not do justice to the issue: please see TUF GitHub Issue #660: #660 Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

This commit performs restructuring on the recently added metadata class model architecture, which shall be part of a new simple TUF API. The key change is that the Metadata class is now used as container for inner TUF metadata (Root, Timestamp, Snapshot, Targets) instead of serving as base class for these, that means we use 'composition' instead of 'inheritance'. Still, in order to aggregate common attributes of the inner Metadata (expires, version, spec_version), we use a new baseclass 'Signed', which also corresponds to the signed field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to more closely represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). Note that the proposed changes require us to now access some attributes/methods via the signed attribute of a Metadata object and not directly on the Metadata object, but it would be possible to add short-cuts. (see todo notes in doc header). Further changes include: - Add minimal doc header with TODO notes - Make attributes that correspond to fields in TUF JSON metadata public again. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code. - Generally try to reduce code. - Remove keyring and consistent_snapshot attributes from metadata class. As discussed in #1060 they are a better fit for extra management code (also see #660) - Remove sslib schema checks (see TODO notes about validation in doc header) - Drop usage of build_dict_conforming_to_schema, it seems a lot simpler and more explicit to just code this here. - ... same goes for make_metadata_fileinfo - Adapt tests accordingly TODO: Document!!! Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **Some more design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python2 **TODO:** See doc header TODO list

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **Additional design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python =< 3.5 **TODO: See doc header TODO list** Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Co-authored-by: Joshua Lock <jlock@vmware.com> Co-authored-by: Teodora Sechkova <tsechkova@vmware.com> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

Add metadata module with container classes for TUF role metadata, including methods to read/serialize/write from and to JSON, perform TUF-compliant metadata updates, and create and verify signatures. The 'Metadata' class provides a container for inner TUF metadata objects (Root, Timestamp, Snapshot, Targets) (i.e. OOP composition) The 'Signed' class provides a base class to aggregate common attributes (i.e. version, expires, spec_version) of the inner metadata classes. (i.e. OOP inheritance). The name of the class also aligns with the 'signed' field of the outer metadata container. Based on prior observations in TUF's sister project in-toto, this architecture seems to well represent the metadata model as it is defined in the specification (see in-toto/in-toto#98 and in-toto/in-toto#142 for related discussions). This commits also adds tests. **TODO: See doc header TODO list** **Additional design considerations** (also in regards to prior sketches of this module) - Aims at simplicity, brevity and recognizability of the wireline metadata format. - All attributes that correspond to fields in TUF JSON metadata are public. There doesn't seem to be a good reason to protect them with leading underscores and use setters/getters instead, it just adds more code, and impedes recognizability of the wireline metadata format. - Although, it might be convenient to have short-cuts on the Metadata class that point to methods and attributes that are common to all subclasses of the contained Signed class (e.g. Metadata.version instead of Metadata.signed.version, etc.), this also conflicts with goal of recognizability of the wireline metadata. Thus we won't add such short-cuts for now. See: theupdateframework#1060 (comment) - Signing keys and a 'consistent_snapshot' boolean are not on the targets metadata class. They are a better fit for management code. See: theupdateframework#1060 (comment), and theupdateframework#660. - Does not use sslib schema checks (see TODO notes about validation in doc header) - Does not use existing tuf utils, such as make_metadata_fileinfo, build_dict_conforming_to_schema, if it is easy and more explicit to just re-implement the desired behavior on the metadata classes. - All datetime's are treated as UTC. Since timezone info is not captured in the wireline metadata format it should not be captured in the internal representation either. - Does not use 3rd-party dateutil package, in order to minimize dependency footprint, which is especially important for update clients which often have to vendor their dependencies. However, compatibility between the more advanced dateutil.relativedelta (e.g handles leap years automatically) and timedelta is tested. - Uses PEP8 indentation (4 space) and Google-style doc string instead of sslab-style. See secure-systems-lab/code-style-guidelines#20 - Does not support Python =< 3.5 Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Co-authored-by: Joshua Lock <jlock@vmware.com> Co-authored-by: Teodora Sechkova <tsechkova@vmware.com> Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>

ivanayov · 2022-02-16T14:32:34Z

Closing this issue as it was filed against (what is now known as) the legacy codebase: issue seems to not be relevant anymore. Please re-open or file a new issue if you feel that the issue is revelant to current python-tuf.

More details

Current source code (and upcoming 1.0 release) only contains the modern components

a low-level Metadata API (tuf.api) and
tuf.ngclient that implements the client workflow,

Legacy components (e.g. tuf.client, tuf.repository_tool, tuf.repository_lib as well as the repo and client scripts) are no longer included. See announcement and API reference for more details.

awwad added bug security documentation Documentation of the project as well as procedural documentation client Related to the client (updater) implementation labels Mar 23, 2018

awwad mentioned this issue Mar 23, 2018

Promiscuous delegation not supported correctly #658

Closed

awwad mentioned this issue May 30, 2018

Bad assumption in roledb leads to persistent freeze attack #736

Closed

awwad mentioned this issue Jun 13, 2018

WIP: Fixes #589. Allows 2+ roles to delegate to same role #590

Closed

3 tasks

awwad mentioned this issue Jun 20, 2018

Fix tuf/tuf#736: Rebuild roledb in refresh() even if root unchanged awwad/tuf#13

Merged

vladimir-v-diaz mentioned this issue Jun 25, 2018

Use the keyids and threshold set by delegating roles to verify delegated metadata #757

Closed

3 tasks

This was referenced Jan 2, 2019

Avoid unnecessary delegated role downloads #811

Merged

Create roles independently from tuf.repository_tool.Targets.delegate() #820

Closed

awwad mentioned this issue Feb 11, 2019

Full Verification algorithm does not verify delegated roles (5.4.4.2:11.2) uptane/uptane-standard#36

Closed

awwad self-assigned this Feb 25, 2019

awwad added a commit that referenced this issue Feb 25, 2019

Add helper funcs for roledb: get_delegation, _is_top_level_role

e3e9322

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad mentioned this issue Mar 15, 2019

Destroy unnecessary MetaFile classes in formats.py #836

Merged

3 tasks

awwad added a commit that referenced this issue Mar 28, 2019

Add helper funcs for roledb: get_delegation, _is_top_level_role

8c32c35

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad added a commit that referenced this issue Apr 2, 2019

Add helper funcs for roledb: get_delegation, _is_top_level_role

17809a3

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad mentioned this issue Apr 2, 2019

(WIP/Collaboration) rewrites for delegation graph issues #846

Closed

awwad added a commit that referenced this issue Apr 3, 2019

Add helper funcs for roledb: get_delegation, _is_top_level_role

fc7308a

These aid in the roledb rewrite to start to address Issue #660. Also add two minor TODOs. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

This was referenced Apr 30, 2019

Sub-PR to 846: Modification of Updater and tests as part of #660 #867

Closed

Sub-PR of 846: Modification of Updater and tests as part of #660 #868

Closed

lukpueh mentioned this issue Oct 3, 2019

Duplicate definitions of schemas #927

Closed

lukpueh mentioned this issue Oct 4, 2019

Remove duplicate role-related schemas #929

Merged

3 tasks

lukpueh mentioned this issue Nov 8, 2019

Documenting or creating a TAP to describe keyid_hash_algorithms #848

Closed

mnm678 mentioned this issue Mar 19, 2020

Add TAP for keyid flexibility theupdateframework/taps#112

Merged

lukpueh mentioned this issue Jun 11, 2020

Load correctly the delegated Targets objects hierarchy #1052

Merged

3 tasks

joshuagl added this to the Refactor milestone Jul 7, 2020

lukpueh mentioned this issue Jul 8, 2020

A simple, per-file metadata CRUD API #1060

Closed

3 tasks

joshuagl mentioned this issue Jul 17, 2020

Implement support for TAP 3: Multi-role delegations #1082

Open

joshuagl removed this from the Refactor milestone Sep 8, 2020

jku added the legacy label Nov 30, 2021

ivanayov closed this as completed Feb 16, 2022

lukpueh mentioned this issue Mar 29, 2022

Correct and clarify "6.3.1 Updating targets metadata" for delegated targets roles theupdateframework/specification#214

Open

adityasaky mentioned this issue Dec 6, 2023

Added a termination check for delegation search gittuf/gittuf#199

Closed

adityasaky mentioned this issue Dec 18, 2023

Refactor state.Verify() to avoid promiscuous delegations gittuf/gittuf#219

Merged

lukpueh mentioned this issue Feb 12, 2024

repository: maybe provide a Repository.get_delegating_role() #2552

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roles and Delegations are still confused in parts of the implementation #660

Roles and Delegations are still confused in parts of the implementation #660

awwad commented Mar 23, 2018 •

edited

Loading

ivanayov commented Feb 16, 2022

Roles and Delegations are still confused in parts of the implementation #660

Roles and Delegations are still confused in parts of the implementation #660

Comments

awwad commented Mar 23, 2018 • edited Loading

Summary

Detailed explanation

Specification / Avoiding Future Issues

ivanayov commented Feb 16, 2022

awwad commented Mar 23, 2018 •

edited

Loading