Another approach: use python-cryptography instead#1606
Another approach: use python-cryptography instead#1606CdeMills wants to merge 1 commit intoros:melodic-develfrom CdeMills:melodic-devel
Conversation
|
Is this fully compatible with the original approach? Either way, it doesn't seem like we're gaining much in brevity with this new dependency. |
dirk-thomas
left a comment
There was a problem hiding this comment.
This PR aims to address #1599.
The new dependency proposed in this patch (which still needs to be added to the package manifest) is available as of Ubuntu Bionic: python-pycryptodome, python3-pycryptodome
Please describe the pros/cons over the proposed patch in #1609.
| encrypted_chunk = cipher.encrypt(_add_padding(chunk)) | ||
| # cipher = AES.new(self._symmetric_key, AES.MODE_CBC, iv) | ||
| cipher = Cipher(algorithms.AES(self._symmetric_key), modes.CBC(iv), backend=backend) | ||
| encryptor = cipher.encryptor() |
There was a problem hiding this comment.
What is the benefit of using this different API?
| iv = Random.new().read(AES.block_size) | ||
| # iv = Random.new().read(AES.block_size) | ||
| backend = default_backend() | ||
| iv = os.urandom(AES_BLOCK_SIZE) |
There was a problem hiding this comment.
What is the difference of os.urandom over Random?
|
Hello, Pascal |
|
Comparing the statistics of the two GitHub repos I would think cryptography is the more mature and active project compared to pycryptodome. Therefore I closed the other PR. @CdeMills Please answer the pending questions when you have a chance and also update the package manifest to add the new dependency (and replace the old one). |
| raise ROSBagFormatException('Error in encrypted chunk size: {}'.format(len(encrypted_chunk))) | ||
| if len(encrypted_chunk) < AES.block_size: | ||
| # if len(encrypted_chunk) < AES.block_size: | ||
| if len(encrypted_chunk) < AES.BLOCK_SIZE: |
There was a problem hiding this comment.
AES.BLOCK_SIZE doesn't exist. Either AES_BLOCK_SIZE or AES.block_size.
| return | ||
| self._symmetric_key = Random.new().read(AES.block_size) | ||
| # self._symmetric_key = Random.new().read(AES.block_size) | ||
| self._symmetric_key = os.urandom(AES.block_size) |
There was a problem hiding this comment.
Why AES.block_size here instead of AES_BLOCK_SIZE?
| cipher = Cipher(algorithms.AES(self._symmetric_key), modes.CBC(iv), backend=backend) | ||
| decryptor = cipher.decryptor() | ||
| # header = cipher.decrypt(encrypted_header) | ||
| header = decryptor_update(encrypted_header) + decryptor_finalize() |
There was a problem hiding this comment.
Should be decryptor.update(...
| #from Crypto import Random | ||
| # from Crypto.Cipher import AES | ||
| # taken from Crypto.Cipher.AES.block_size | ||
| AES_BLOCK_SIZE = 16 |
There was a problem hiding this comment.
Why use this constant over AES.block_size (which is 128)?
|
The passing CI clearly indicates that non of the touched code is covered by unit tests. Otherwise they would have failed due to the bugs in the current patch. |
|
Closing in favor of #1793 which redos this patch. |
This introduces a dependency w.r.t. https://cryptography.io/en/latest/