Skip to content

Add missing method args docs in metadata API #1620

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

Merged
merged 1 commit into from
Oct 20, 2021
Merged

Add missing method args docs in metadata API #1620

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 24 additions & 3 deletions tuf/api/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,9 +575,11 @@ def to_securesystemslib_key(self) -> Dict[str, Any]:

@classmethod
def from_securesystemslib_key(cls, key_dict: Dict[str, Any]) -> "Key":
"""
Creates a Key object from a securesystemlib key dict representation
"""Creates a Key object from a securesystemlib key dict representation
removing the private key from keyval.

Args:
key_dict: A key in securesystemlib dict representation.
Copy link
Member Author

Choose a reason for hiding this comment

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

I admit that the description I added is not particularly helpful, but I'm unsure what to do. Can we maybe quickly rethink the API design here?

From what I see, the relevant difference between from_dict and from_securesystemslib_key is that the former accepts any keyval (including private ones ... in the long run we will probably want to choke here, right?), whereas the latter removes keyval["private"], if present.

So maybe we can rename from_securesystemslib_key to from_private_key?

Also, the method becomes a lot easier if we just pop keyval["private"] instead of calling the mystic format_keyval_to_metadata function, which does other things that we don't really care for (e.g. vet values using schema, temporarily assign keyid_hash_algorithms, etc.).

@avelichka, would you care to weigh in? I'm happy to re-phrase the docs here or with #1600, once I understand the purpose of this method better. :)

Copy link
Member

Choose a reason for hiding this comment

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

Adding the method was my suggestion, and avoiding poking the securesystemslib key_dict internals may have been as well: none of us were probably entirely sure how much of the dict return value is sort of "implicitly API" or if it's a secursystemslib implementation detail -- are we going to be in trouble later if we just start removing things there. I think you've got a much better idea of this so feel free to propose something.

The purpose is that we have needed roughly the following code in multiple places (this is from test RepositorySimulator where we generate new Metadata):

    def create_key(self) -> Tuple[Key, SSlibSigner]:
        sslib_key = generate_ed25519_key()
        return Key.from_securesystemslib_key(sslib_key), SSlibSigner(sslib_key)

It turned out quite easy to make the mistake of including the private key in the TUF metadata :) and I wanted to make it easier to avoid that...

Other solutions for this are totally fine too.

Copy link
Member

Choose a reason for hiding this comment

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

oh, and if it wasn't clear: some of the places where this is needed are outside of python-tuf: so developers using python-tuf need to be able to do this so it felt like a function in Metadata API was a good idea even if del sslib_key["private"] probably would have worked fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the context, @jku! I'll try to condense some of this in the docstring with #1600.

"""
key_meta = sslib_keys.format_keyval_to_metadata(
key_dict["keytype"],
Expand Down Expand Up @@ -762,10 +764,13 @@ def to_dict(self) -> Dict[str, Any]:
)
return root_dict

# Update key for a role.
def add_key(self, role: str, key: Key) -> None:
"""Adds new signing key for delegated role 'role'.

Args:
role: The name of the role, for which 'key' is added.
key: The signing key to be added for 'role'.

Raises:
ValueError: If 'role' doesn't exist.
"""
Expand All @@ -777,6 +782,10 @@ def add_key(self, role: str, key: Key) -> None:
def remove_key(self, role: str, keyid: str) -> None:
"""Removes key from 'role' and updates the key store.

Args:
role: The name of the role, for which a signing key is removed.
key: The identifier of the key to be removed for 'role'.

Raises:
ValueError: If 'role' doesn't exist or if 'role' doesn't include
the key.
Expand Down Expand Up @@ -1151,6 +1160,10 @@ def is_delegated_path(self, target_filepath: str) -> bool:
supported as target path separator. Leading separators are not handled
as special cases (see `TUF specification on targetpath
<https://theupdateframework.github.io/specification/latest/#targetpath>`_).

Args:
target_filepath: URL path to a target file, relative to a base
targets URL.
"""

if self.path_hash_prefixes is not None:
Expand Down Expand Up @@ -1440,6 +1453,10 @@ def update(self, fileinfo: TargetFile) -> None:
def add_key(self, role: str, key: Key) -> None:
"""Adds new signing key for delegated role 'role'.

Args:
role: The name of the role, for which 'key' is added.
key: The signing key to be added for 'role'.

Raises:
ValueError: If there are no delegated roles or if 'role' is not
delegated by this Target.
Expand All @@ -1453,6 +1470,10 @@ def remove_key(self, role: str, keyid: str) -> None:
"""Removes key from delegated role 'role' and updates the delegations
key store.

Args:
role: The name of the role, for which a signing key is removed.
key: The identifier of the key to be removed for 'role'.

Raises:
ValueError: If there are no delegated roles or if 'role' is not
delegated by this Target or if key is not used by 'role'.
Expand Down