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

Signer uses python2 "__metaclass__" #463

Closed
jku opened this issue Nov 25, 2022 · 4 comments · Fixed by #473 or #546
Closed

Signer uses python2 "__metaclass__" #463

jku opened this issue Nov 25, 2022 · 4 comments · Fixed by #473 or #546
Assignees
Labels

Comments

@jku
Copy link
Collaborator

jku commented Nov 25, 2022

signer.py does __metaclass__ = abc.ABCMeta. This is python2 syntax and does nothing. So calling Signer() works fine but the resulting object does not implement the Signer API (the sign method is unimplemented)

The actually supported methods are

from abc import ABC
class MyABC(ABC)

and

from abc import ABCMeta
class MyABC(metaclass=ABCMeta)

the latter may be safer (WRT multiple inheritance issues)

Found in #456

@jku jku added the bug label Nov 25, 2022
@jku
Copy link
Collaborator Author

jku commented Nov 25, 2022

also StorageBackendInterface

@lukpueh
Copy link
Member

lukpueh commented Nov 29, 2022

This might be a bit of a digression, but I think we should be using typing.Protocol in most cases, where we now use abc. This is at least true for Signer and StorageBackendInterface), where the base classes don't provide any re-usable code and we don't care if the implementations are actually subclasses of these interfaces. We just want to make sure that the implementations quack like Signers or StorageBackendInterface.

typing.Protocol is not in Python 3.7, but a backport exists. What do others think?

@jku
Copy link
Collaborator Author

jku commented Nov 29, 2022

In general I like the protocol idea for things like Signer (where the intent is to document to 3rd parties what they need to do). Caveats though:

  • signer dispatch would still need to be somewhere
  • anything with serialize/deserialize (like Key) seems trickier

The one issue I have with Protocol is that the implementations never declare themselves (compare with rust Traits which you always declare with a impl ThisTrait for ThatType). This is very pythonic of course but it's also annoying in a very pythonic way: Does MySpecialClass implement the Signer protocol? Who knows! Go ahead and read the full source code to find out...

@joshuagl
Copy link
Collaborator

I agree that typing.Protocol seems like a good fit for our ABC's which exist only to define an interface and do not provide any base or shared functionality.

lukpueh added a commit to lukpueh/securesystemslib that referenced this issue Mar 22, 2023
Every now and then this test fails, because pyca/cryptography
raises an unexpected error message (see pyca/cryptography#8563)

Regardless of whether it is a good idea to assert error messages of
3rd party libraries, this patch provides a quick fix by asserting
that is one of both known error messages.

closes secure-systems-lab#463

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment