Implement __hash__ on all extensions we've implemented __eq__ on

[reaperhulk]

We've fixed this haphazardly in the past, but we should probably consistently implement __hash__ so extensions don't have odd behaviors like not being able to be inserted into hashable collections.

• CRLNumber (Refs #2578 -- implement __hash__ on CRLNumber #2580)
• AuthorityKeyIdentifier
• SubjectKeyIdentifier (Refs #2578 -- implement __hash__ on SubjectKeyIdentifier #2583)
• AuthorityInformationAccess
• AccessDescription
• BasicConstraints (implement hash on basicconstraints #2594)
• CRLDistributionPoints
• DistributionPoint
• CertificatePolicies
• PolicyInformation
• UserNotice
• NoticeReference
• ExtendedKeyUsage
• InhibitAnyPolicy
• KeyUsage
• NameConstraints
• Extension
• PolicyConstraints
• GeneralNames
• SubjectAlternativeName
• IssuerAlternativeName
• CRLReason (Refs #2578 -- implement __hash__ on CRLReason #2595)
• InvalidityDate
• CertificateIssuer
• PrecertificateSignedCertificateTimestamps
• TLSFeature

[reaperhulk]

Many of the remaining extensions use a list of items as their internal storage. Do we want to do something like hash(tuple(list))...? This could potentially be pretty expensive.

[alex]

bleh. let's take some time and think for those, I don't think we have
external requests for these features yet, so we can take our time

On Sun, Dec 27, 2015 at 5:00 PM, Paul Kehrer [email protected]
wrote:

Many of the remaining extensions use a list of items as their internal
storage. Do we want to do something like hash(tuple(list))...? This could
potentially be pretty expensive.

—
Reply to this email directly or view it on GitHub
#2578 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

[Badg]

I'm going to preemptively apologize for an unintentionally long question, but: for those classes using lists for internal storage, is it a big deal if __hash__ just reinstates object.__hash__?

This would have the consequence that if, for example, two GeneralNames instances had the same internal self._general_names list, they would hash differently, and therefore be put into different buckets in dicts, etc. I'm asking because I honestly don't know: is it likely that this could result in problems?

I'm thinking about chipping away at a few of these (at what could only be described as a glacial pace, but hey, anything helps, and if I ended up blocking anyone I'd speed up), but I'm concerned about not violating the edict of "thou shalt not have changing hashes". If the internal storage iterables are ever mutated, it seems to me that the only safe option would be either to use object.__hash__ directly, or something functionally equivalent (a unique hash for each instance regardless of internal state).

I suppose there are some other strategies that could be used, too, but they all seem fairly dangerous. At a certain point, if two equivalent objects really, really should be put in the same hash bucket, it seems to me like it'd be better off to convert the internal list (ex GeneralNames._general_names) into a tuple to enforce immutability. But that has the potential to break things, and I'm definitely not familiar enough with the project internals to actually suggest that.

Thoughts?

[sejr]

@alex I saw this issue as a starter project, and I would like to work on implementing __hash__ for the remaining extensions. Would you prefer each extension as its own PR, or would you like to group them together as a PR addressing this particular issue (#2578)?

[alex]

Individual PRs please, it makes them much easier to review and get merged quickly! Thanks for looking into contributing!

[sejr]

@alex I have been thinking about the comment from @Badg regarding __hash__() on classes with lists. I also consulted the Python documentation:

If a class defines mutable objects and implements a __cmp__() or __eq__() method, it should not implement __hash__(), since hashable collection implementations require that an object’s hash value is immutable (if the object’s hash value changes, it will be in the wrong hash bucket).

My thoughts: since the data members of the AuthorityKeyIdentifier class (for example) are read-only, why don't we just construct the problematic members as tuples instead of lists?

In the AuthorityKeyIdentifier example; instead of:

if authority_cert_issuer is not None:
            authority_cert_issuer = list(authority_cert_issuer)

We could just do:

if authority_cert_issuer is not None:
            authority_cert_issuer = tuple(authority_cert_issuer)

This way, we wouldn't have to convert the list to a tuple every time __hash__() is called on these classes.

[alex]

The attribute really shouldn't be a tuple -- tuples are not just immutable lists, they should not be treated as general purpose sequences. Until someone has a specific performance complaint, I'd much rather we convert to tuple in __hash__.

[reaperhulk]

Completing this is now blocked on finishing #3461