Make new api compatible with the Signing interface #1272
Conversation
There was a problem hiding this comment.
Cool stuff, @MVrachev! I only noticed one thing (see inline comment), but we can revisit once secure-systems-lab/securesystemslib#319 has landed.
On a general note, I appreciate slightly more informative commit messages.
Sorry for the lack of a good commit message, you are right. I will update this pr when the securesystemslib signer interface is merged and I will add a better commit message before mark it |
MVrachev
force-pushed
the
signer-interface
branch
2 times, most recently
from
3062307 to
19e08d0
Compare
|
I updated the pr addressing Lukas comment #1272 (comment), adding a new commit fixing one comment in the code, and making the commit message more descriptive. |
MVrachev
force-pushed
the
signer-interface
branch
2 times, most recently
from
a5e64e1 to
ce655ac
Compare
|
Updated the pr after we have bumped securesystemslib version to 0.20.0 which supports the new Signer interface. |
|
Thanks for this PR Martin, I'm hoping to give it a thorough review ASAP. One quick observation: we should bump the minimum version of securesystemslib in the |
MVrachev
force-pushed
the
signer-interface
branch
4 times, most recently
from
1856040 to
977f0d4
Compare
|
I addressed your comments @joshuagl and fixed the conflicts. |
MVrachev
force-pushed
the
signer-interface
branch
from
977f0d4 to
3050fb6
Compare
|
I updated according to the last @lukpueh remarks. |
In the securesystemslib pr secure-systems-lab/securesystemslib#319 I added a new Signer interface with the purpose of supporting multiple signing implementations. Additionally, I added the SSlibSigner implementation of that interface which implements the signing operation for rsa, ed25519 and ecdsa schemes. With this commit, I integrate the SSlibSigner into the new API in tuf. Signed-off-by: Martin Vrachev <[email protected]>
lukpueh
force-pushed
the
signer-interface
branch
from
3050fb6 to
49aa0fc
Compare
There was a problem hiding this comment.
Hey @MVrachev, I took the liberty to rebase your PR on top of #1279, fixing conflicts and making minimal changes. See comments inline...
To get a full diff between your old diff and my new rebased diff you can do:
git range-diff develop 3050fb6 49aa0fc
I think we can merge. But maybe should consult with a 3rd-party, now that we are both authors? (ping @joshuagl, @jku, @sechkova)
| from securesystemslib.util import persist_temp_file | ||
| from securesystemslib.signer import Signer, Signature |
There was a problem hiding this comment.
Re-ordered imports alphabetically.
| # metadata['signatures'], call Signature.from_dict for each item, and | ||
| # pass a list of Signature objects to the Metadata constructor instead. | ||
| signatures = [] | ||
| for signature in metadata.pop('signatures'): |
There was a problem hiding this comment.
Now popping the signatures list for consistently destructing the passed dictionary as described in the newly added "Side Effect" docstring section.
| signed_serializer: Optional[SignedSerializer] = None | ||
| ) -> Dict[str, Any]: | ||
| """Creates signature over 'signed' and assigns it to 'signatures'. | ||
|
|
||
| Arguments: | ||
| key: A securesystemslib-style private key object used for signing. | ||
| signer: An object implementing the securesystemslib.signer.Signer |
There was a problem hiding this comment.
Did s/singer/signer to fix a typo I had missed in my review.
|
I agree with your changes @lukpueh. |
|
Ping @woodruffw! |
|
Very cool, thanks for all the great work, guys! Will, you should be able to reuse a lot of the Vault-specific code I wrote as part of this PR... |
Related to: #1263
Description of the changes being introduced by the pull request:
This pr makes TUF compatible with changes introduced in secure-systems-lab/securesystemslib#319.
Please wait for the securesystemlib pr to be merged first!
Signed-off-by: Martin Vrachev [email protected]