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.

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

Add Key.from_securesystemslib_key #1520

Merged
merged 1 commit into from Aug 31, 2021

Conversation

avelichka
Copy link
Contributor

The securesystemslib key dictionary representation includes
the private key in keyval. TUF key doesn't handle it in any way,
but considering that we allow unrecognized symbols in the format,
we should exclude the private key.
A call to securesystemslib.keys.format_keyval_to_metadata
with the default private=False would do exactly that.

Signed-off-by: Velichka Atanasova avelichka@vmware.com

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Fixes #1458

Description of the changes being introduced by the pull request:

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

@MVrachev
Copy link
Collaborator

MVrachev commented Aug 16, 2021

As someone who didn't read the issue solved by this pr, it was a little unclear why we need to remove
the private part of the securesystemslib key. I had to read Jussi's issue to understand the problem.
Maybe you can modify your second sentence in your pr description and commit message like this:

TUF key doesn't handle it in any way,
but considering that we allow unrecognized symbols in the format,
we should exclude the private key otherwise this could lead to unexpected
behavior.

For example, those steps could lead to a mistake:
1. create a Metadata object with a root instance for Metadata.signed:
md = Metadata.from_file(<root_path>)
2. create a securesystemslib key:
sslib_key = generate_ed25519_key()
3. create a TUF Key using the dictionary representing the securesystemslib key:
key = Key.from_dict(sslib_key["keyid"], sslib_key)
4. add the new TUF key to the root instance
md.signed.add_key("root", key)
5. save the metadata back to a file by calling Metadata.to_file():
md.to_file("root_test.json")

this will result in a new "root_test.json" file containing the private portion of a key.

Not sure if this will help, but a use case like this can highlight the issue without looking at the GitHub issue.
Otherwise, the code looks good to me.

PS: I tried the steps from 1 to 5 and they produce the expected result.

tuf/api/metadata.py Outdated Show resolved Hide resolved
tests/test_api.py Outdated Show resolved Hide resolved
The securesystemslib key dictionary representation includes
the private key in keyval. TUF key doesn't handle it in any way,
but considering that we allow unrecognized symbols in the format,
we should exclude the private key otherwise this could lead to
misuse.
A call to securesystemslib.keys.format_keyval_to_metadata
with the default private=False would do exactly that.

Signed-off-by: Velichka Atanasova <avelichka@vmware.com>
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. I think this makes sense so all of our example code and repository code can easily create new keys and use them without added code just to handle this issue.

The thing that makes me wonder about this is adding more and more functionality into Metadata API that the client does not need. We'll need to be careful not to overdo this (or figure out some RepositoryMixin classes or something ...)

@joshuagl
Copy link
Member

The thing that makes me wonder about this is adding more and more functionality into Metadata API that the client does not need. We'll need to be careful not to overdo this (or figure out some RepositoryMixin classes or something ...)

💯 Agree with this train of thought, metadata.py is very large.

@jku jku merged commit e1ec782 into theupdateframework:develop Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata API: Add Key.from_securesystemslib_key()?
4 participants