Add __eq__ to HashAlgorithm and padding instances

[mathiasertl]

This PR:

1. Adds __eq__() to all subclasses of HashAlgorithm.
2. Adds __eq__() to all padding classes.

My primary use case is testing. First, it's really un-pythonic to me (obviously very subjective, but it feels just not right to me) to write assert isinstance(cert.cert.signature_hash_algorithm, hashes.SHA256()), when I could write assert cert.cert.signature_hash_algorithm == hashes.SHA256(). But once you start comparing to other data structures, it gets even uglier:

def test_something() -> None:
    ...
    assert isinstance(cert.signature_hash_algorithm, type(other.signature_hash_algorithm))

I think this just looks a lot better and readable:

def test_something() -> None:
    ...
    assert cert.signature_hash_algorithm == other.signature_hash_algorithm

[reaperhulk]

Let's put these in a separate PR. Whether or not we want to do __eq__ is possibly debateable, but if you want these properties to be public that's a separate discussion.

[mathiasertl]

Hi @reaperhulk

Thanks for the feedback - I've updated this PR to only include the __eq__ instances. I'll make a separate PR for the extra properties.

kr, Mat

[reaperhulk]

Looking at the test failures here the issue is that scapy creates its own HashAlgorithm, which now fails because they don't implement __eq__. We could avoid this by making __eq__ a concrete method that raises NotImplementedError, but we'd need to cover those lines in a test as well.

[mathiasertl]

Or it could be a breaking change and that lib would need to implement the method.

Open to implement any solution, just let me know what you prefer!

[reaperhulk]

@alex do you have an opinion here? I'm reluctant to spend breakage budget on this when we could avoid it with a concrete implementation, but I'm interested in other opinions.

[alex]

I'm pretty ambivalent about this. We have a few choices:

1. Simply implement __eq__ for our own hashes, don't change the contract (all objects always have __eq__).
2. Add __eq__ to the ABC and require people to implement it: breaking change for people defining their own hashes
3. Add __eq__ to the ABC and provide a default impl: works for some users with their own hashes, but is wrong for anyone with their own hash which has any attributes (e.g. blake2).

Of these I think (1) is probably best.

[reaperhulk]

For 3 the default impl could raise NotImplementedError so you wouldn’t need to have a potentially incorrect impl

[alex]

That can break existing applications, to the extent they're relying on the default object.__eq__ behavior!

[mathiasertl]

I'm also ambivalent, I defer to your call and will adapt as requested.

Just option three (default Implementation) seems like a bad idea to me.

Mat

[reaperhulk]

After thinking for too long, I'm okay with option 1 here.

[reaperhulk]

Are you still interested in this PR? Happy to review/land with option 1 + a rebase to fix the changelog conflict

[alex]

I think a question is whether we should be returning NotImplemented or False for diffefrent types

[alex]

We need to do this check first (otherwise you get an AttributeError) and return NotImplemented

[mathiasertl]

Right, on it!

[alex]

Same issue here.

[mathiasertl]

Actually not correct in this case:

>>> h = hashes.SHA256()
>>> mgf = MGF1(h)
>>> OAEP(mgf, h, None) == h
False

[alex]

This isn't quite right for the behavior we implemented. I'd probably document it as "builtin hash classes can now be compared with =="

[mathiasertl]

agreed. will be updated.

[mathiasertl]

I think a question is whether we should be returning NotImplemented or False for diffefrent types

I don't know of any classes that do that. Standard python certainly doesn't:

>>> {} == 0
False

[alex]

>>> {}.__eq__(0)
NotImplemented

:-)

[mathiasertl]

>>> {}.__eq__(0)
NotImplemented

:-)

To quote official documentation - emphasis mine:

A rich comparison method may return the singleton NotImplemented if it does not implement the operation for a given pair of arguments. By convention, False and True are returned for a successful comparison.

So I can implement it, it's just lots of extra code with no benefit that I'm aware of.

[mathiasertl]

This class is more-or-less duplicated in test_openssl as well!

I would offer to move it to tests.doubles.py for shorter code - if you approve.

[alex]

Sounds great, thanks