Abstract base class for hashlib

[tiran]

BPO	18742
Nosy	@rhettinger, @gpshead, @pitrou, @tiran
Files	hashlib_abc3.patch hashlib_abc4.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-feature'] title = 'Abstract base class for hashlib' updated_at = user = 'https://github.com/tiran' ``` bugs.python.org fields: ```python activity = actor = 'christian.heimes' assignee = 'none' closed = False closed_date = None closer = None components = [] creation = creator = 'christian.heimes' dependencies = [] files = ['31308', '32302'] hgrepos = [] issue_num = 18742 keywords = ['patch'] message_count = 16.0 messages = ['195226', '195259', '195301', '195308', '195561', '200746', '200940', '200941', '200953', '200988', '200996', '203164', '203169', '203197', '203198', '203201'] nosy_count = 5.0 nosy_names = ['rhettinger', 'gregory.p.smith', 'pitrou', 'christian.heimes', 'python-dev'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue18742' versions = ['Python 3.5'] ```

[tiran]

All hashlib types provide a common interfaces but there is no common super class. The patch implements provides hashlib.CryptoHash abstract base class as common virtual class for all hash types.

The patch also exposes all internal types of the internal hash C modules so I don't have to jump throw the type(constructor()) hoop.

I have also changed __get_builtin_constructor() to use a lookup cache instead of importing the module every time. It is necessary to avoid multiple calls to CryptoHash.register().

[tiran]

Updated patch.

I'm going to add documentation when everybody is happy with the patch.

[tiran]

Now with block_size

[gpshead]

your patch makes sense to me.

[tiran]

Thanks Gregory!

Have you had a chance to read http://www.python.org/dev/peps/pep-0452/ , too?

[tiran]

I'm going to add CryptoKeyedHash ABC and register hmac as keyed hash algorithm, too.

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 72d7b2185771 by Christian Heimes in branch 'default': Issue bpo-18742: Rework the internal hashlib construtor to pave the road for ABCs. http://hg.python.org/cpython/rev/72d7b2185771

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 27da6e790c41 by Christian Heimes in branch 'default': Issue bpo-18742: Expose the internal hash type object for ABCs. http://hg.python.org/cpython/rev/27da6e790c41

[tiran]

I have checked in some of the basic and non-controversal parts of the PEP (optimization, expose type object). The ABCs are still missing. In order to cope with keyed and non-keyed CHF I have added a common base class for both and separate ABCs for keyed and non-keyed CHF. They are really different beasts but they provide a common API except for they constructor.

class AbstractCryptoHashFunction(metaclass=_abc.ABCMeta):
    """Common ABC for keyed and non-keyed cryptographic hash functions"""

class CryptoHashFunction(AbstractCryptoHashFunction):
    """Abstract base class for cryptographic hash functions (PEP 247)"""

class KeyedCryptoHashFunction(AbstractCryptoHashFunction):
    """Abstract base class for keyed cryptographic hashs function (PEP 247)"""

[gpshead]

Have you had a chance to read http://www.python.org/dev/peps/pep-0452/ , too?

Overall PEP-452 looks good but one thing popped out at me:

        >>> import hashlib
        >>> from Crypto.Hash import MD5
        >>> m = MD5.new()
        >>> isinstance(m, hashlib.CryptoHash)
        True

This suggests that there is an ABC hashlib.CryptoHash that non hashlib implementations sound use but that is not spelled out anywhere as required or recommended for third party hash implementations or why using it is a good thing. I believe that is what you want to do with the ABC being defined in this issue so I'd mention that explicitly in the pep.

[rhettinger]

Consider inheriting from abc.ABC rather than specifying metaclass=abc.ABCMeta

[tiran]

Raymond: Good idea!

to all: Do we need more discussion about the ABC, e.g. on the pycrypto mailing list?

[pitrou]

Some comments:

• AbstractCryptoHashFunction should be called CryptoHashBase or something (but does it warrant being public? I don't think so)
• having "Function" in a class name is a bit confusing to me. Why not simply "CryptoHash"?
• you don't need to add a __slots__ to your ABCs, IMO
• the default hexdigest() implementation looks a bit suboptimal to me, why not use binascii?

[tiran]

• "Cryptographic Hash Function" is the correct technical term for algorithms like SHA-1. http://en.wikipedia.org/wiki/Cryptographic_hash_function

• PEP-452 is going to suggest that 3rd party libraries register their hash function as subclasses of the ABC.

• __slots are required for subclassing. All our ABCs in Python 3.x have __slots

• I don't want to import yet another module just for the ABC. I'd rather not provide a sample implementation of hexdigest() if you think it is too slow.

[pitrou]

• "Cryptographic Hash Function" is the correct technical term for algorithms like SHA-1.

Sure, but in a programming context, it's confusing when the "function" is implemented by a class with additional properties and methods. Why not simply "CryptoHash"? It sounds descriptive enough to me.

• I don't want to import yet another module just for the ABC. I'd rather not provide a sample implementation of hexdigest() if you think it is too slow.

Well, it's probably not excruciatingly slow, so if you don't want another import, I guess the current implementation is ok :)

[tiran]

It seems the name of the ABC needs more discussion. I'll address it in PEP-452 for Python 3.5.