Abstract methods added to Key for signing scheme dissection #837
Conversation
…ing_name_str, get_padding_name added to Key class
securesystemslib/signer/_key.py
Outdated
| """Return payload padding name used for this key as a AsymmetricPadding""" | ||
|
|
||
| raise NotImplementedError | ||
|
|
There was a problem hiding this comment.
I can't really see a benefit of adding these to the Key interface. The only subclass, which implements them, is SSlibKey. Why don't make these SSlibKey-only methods?
There was a problem hiding this comment.
In _azure_signer.py and _gcp_signer.py both AzureSigner and GCPSigner take in a public key which is a Key object, on which they use the get hash method. Can this be changed to SSlibKey?
There was a problem hiding this comment.
Hm. Good question. It looks like all Signer implementations take Keys, even though they can only handle specific Key implementations. That's probably okay. And I definitely don't want to disrupt anyone, by changing it.
But adding new behaviour to an abstract base class, which is specific to one subclass only, and raising NotImplementedError everywhere else, does not feel right.
Would it be okay to add regular functions, e.g. to _utils?
Thoughts, @jku?
There was a problem hiding this comment.
I admit I didn't look at the specific uses in AzureSigner and GCPSigner yet but I would say it's fine for them to handle SSlibKey only -- that sounds correct to me, what else could the key be?
Whether that should be done in practice by type checking inside the AzureSigner and GCPSigner functions or by changing some public argument types I can't say without having a closer look...
There was a problem hiding this comment.
There was a problem hiding this comment.
Looks great! If the change isn't likely to bother any user, then I'd go with it. I'll give a detailed review after #836 is merged.
…the SSlibKey subclass, added strict typing for SSlibKey to both AzureSigner and GCPSigner
| class UnsupportedKeyType(Exception): # noqa: N818 | ||
| pass |
There was a problem hiding this comment.
this could be marked private (_UnsupportedKeyType) since I don't think we intend to leak it outside... but that's a nit
| raise NotImplementedError | ||
|
|
||
| def get_padding_name(self, hash_algorithm: None, salt_length: None) -> None: | ||
| raise NotImplementedError |
There was a problem hiding this comment.
These stubs are no longer needed, right?
Description of the changes being introduced by the pull request:
get_hash_algorithm_strabstract method added to Key classget_hash_algorithmabstract method added to Key classget_padding_name_strabstract method added to Key classget_padding_nameabstract method added to Key classThis PR tries to make classes using the Key instance independent of the specific Key implementation.
It also includes the changes from PR #836.
Fixes #594