Remove Python < 2.6 support #84
base: master
Are you sure you want to change the base?
Conversation
…and is not used by any part of the package itself. I think it's fine if we just support strings here. LP#1256172.
@@ -305,7 +303,7 @@ def _ignite(self, data): | |||
if self._mac: | |||
raise TypeError("_ignite() cannot be called twice") | |||
|
|||
self._buffer.insert(0, data) | |||
self._buffer.insert(0, bstr(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data
is already supposed to be a byte string here. Adding bstr
is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is from #81. The bstr is necessary to convert the data for the CMAC if a bytearray was input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change to the API then: CMAC.update()
is defined as taking a byte string only, not a byte array. I find this belongs to a separate change set: this PR is about removing support for Python<2.6 (and adding support for buffers in the process).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't make myself clear. This ensures that any input given to the block cipher is passed to the CMAC as bytes rather than in its given form. It does not alter what CMAC can accept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the block cipher methods update
, encrypt
and decrypt
(which currently feed CMAC in EAX mode) are currently defined as accepting byte strings too, not bytearrays. This change still does not belong here (not to say it is not an improvement, just noting that it changes the API, and if it does there should be unit tests and such).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Includes #81.