Add support for Signature class instances #409
Conversation
Returns signature object in create_signature() Accepts signature object in verify_signature() Update SSlibSigner sign method Update tests
Changes to docstring in verify_signature() and create_signature() method in effect to accept and return signature class instance in respective functions.
|
Nice work. I think the high level issue here is that this is public API that is being used. So while I think something like this is indeed a good path forward -- in my opinion public API arguments and return values should be better defined than "it's a dict!" -- securesystemslib might not want to just change the API... But I'm just assuming: hopefully actual maintainers have opinions here. |
Update test_keys() in tests.check_public_interfaces.py to send signature object instead of a dict
|
Impeccable work, @PradyumnaKrishna. There is nothing for me to comment on in the patch. 💯 But @jku is right that we need to consider carefully if we want to merge (now) and thus disrupt in-toto/tuf operations in the next release. I suggest we hold off with merging until we have a clear plan for #270. |
There was a problem hiding this comment.
I like the changes here, but I'm indeed wary about this breaking in-toto / python-tuf. That said, I suspect (I haven't had a chance to look) modifying both in parallel to switch away from the dict shouldn't be too difficult or a breaking change by any means. @PradyumnaKrishna, can I ask you to take a look at what it would mean to make that change? :)
| {'keyid': 'f30a0870d026980100c0573bd557394f8c1bbd6...', | ||
| 'sig': sig}. | ||
| keyid = 'f30a0870d026980100c0573bd557394f8c1bbd6...', | ||
| sig = sig |
There was a problem hiding this comment.
Perhaps a better description / example here? sig = sig just looks a little funny hehe.
|
@adityasaky In in-toto, |
|
@lukpueh @SantiagoTorres thoughts on modifying in-toto simultaneously? |
|
I gave this more thought and would like to suggest to not touch Good alternatives:
|
| @@ -803,14 +801,15 @@ def verify_signature(key_dict, signature, data): | |||
| formats.ANYKEY_SCHEMA.check_match(key_dict) | |||
|
|
|||
| # Does 'signature' have the correct format? | |||
| formats.SIGNATURE_SCHEMA.check_match(signature) | |||
| if not isinstance(signature, signer.Signature): | |||
| raise exceptions.FormatError("Wanted a Signature object.") | |||
There was a problem hiding this comment.
Nit, feel free to ignore if you decide:
A comment I once got a while ago: Don't add dots or exclamation marks at the end of an exception message.
|
My late review and links to related work -- I believe this mostly duplicates lukas last comment: I think modifying keys module API is not a good idea:
Links:
I think this PR should maybe be closed? |
|
Closing in favor of new API work (see #409 (comment), #409 (comment) and #270) |
Fixes: #360
Description of the changes being introduced by the pull request:
Currently,
verify_siganture()method takes a signature dictionary as parameter and there is no implementation forSignatureclass present insecuresystemslib.signer. This Pull Request adds support forSignatureclass object increate_signature()andverify_signature()methods.Please verify and check that the pull request fulfils the following requirements: