add support for update_into on CipherContext #3190

reaperhulk · 2016-10-08T06:34:17Z

This allows you to provide your own buffer (like recv_into) to improve performance when repeatedly calling encrypt/decrypt on large payloads.

Even if you're calling update millions of times this is not necessarily a performance win at very small sizes (e.g. 16 bytes per call). In testing it is 21% faster when calling update repeatedly with 32KB payloads.

This is still WIP as at least one branch will not be covered (update_into on OpenSSL 1.0.0 AES CTR) and I'm not sure I'm happy with testing yet.

fixes #3119

mention-bot · 2016-10-08T06:34:17Z

@reaperhulk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @public, @alex and @Ayrx to be potential reviewers.

reaperhulk · 2016-11-06T02:10:44Z

(I am considering this blocked until we drop 1.0.0 because I don't want to modify our CI to cover that branch, so this PR will languish until we start the dev cycle for 1.8)

reaperhulk · 2016-11-12T08:31:12Z

Jenkins, retest this please

codecov-io · 2016-11-13T16:18:38Z

Current coverage is 99.94% (diff: 94.32%)

Merging #3190 into master will decrease coverage by 0.05%

@@           master      #3190   diff @@
========================================
  Files         134        134          
  Lines       15245      15380   +135   
  Methods         0          0          
  Messages        0          0          
  Branches     1570       1585    +15   
========================================
+ Hits        15245      15372   +127   
- Misses          0          5     +5   
- Partials        0          3     +3

Powered by Codecov. Last update c7ab931...4b4ed14

alex · 2016-11-13T22:32:46Z

Failing tests relevant.

reaperhulk · 2016-11-15T08:44:25Z

To get full coverage on this I had to add a new tox target py35-cffi-min. This installs a cffi < 1.7 and runs on Mac so it can trigger the NotImplementedError branches in both openssl (which is actually covered by our pypy 4.0.1 linux job) and commoncrypto.

reaperhulk · 2017-02-11T14:17:56Z

src/cryptography/utils.py

@@ -10,6 +10,8 @@
 import sys
 import warnings

+from pkg_resources import parse_version


Doing this will trigger the import side effects we tried to avoid in #3347. Maybe we should do a simpler version check?

Is there any other not-hacky version parser lying around we can use?

cc @dstufft who might know

https://packaging.pypa.io/en/latest/version/ ?

Thanks @dstufft! @sigmavirus24 also suggested it in IRC so that looks like the path to go.

alex · 2017-02-14T15:26:54Z

.travis.yml

@@ -67,6 +67,10 @@ matrix:
          env: TOXENV=py27 CRYPTOGRAPHY_OSX_NO_LINK_FLAGS=0
        - language: generic
          os: osx
+          osx_image: xcode8
+          env: TOXENV=py35-cffi-min CRYPTOGRAPHY_OSX_NO_LINK_FLAGS=1


Does this need to be an OS X job?

Yes, but now that I look at the code that's because we have conditional branches for update_into on both the commoncrypto and openssl backends. We could just assume we have a sufficiently new version in those classes but then do the version check at the CipherContext layer in hazmat.primitives.ciphers.base. That would simplify this whole thing.

(And it would make it so this doesn't have to be a macOS job)

alex · 2017-02-14T15:27:43Z

docs/hazmat/primitives/symmetric-encryption.rst

+            This method allows you to avoid a memory copy by passing a writable
+            buffer and reading the resulting data. You are responsible for
+            correctly sizing the buffer and properly handling the data. Failure
+            to do so correctly can result in crashes. This method should only


Why crashes? Can't we check the buffer sizes and error cleanly?

We actually do, this msg should be updated

alex · 2017-02-14T15:28:12Z

docs/hazmat/primitives/symmetric-encryption.rst

+
+        :param bytes data: The data you wish to pass into the context.
+        :param buf: A writable Python buffer that the data will be written
+            into. This buffer should be ``n - 1`` bytes bigger than the size of


I think this would be clearer if you wrote it as len(data) + n - 1

alex · 2017-02-14T15:32:25Z

src/cryptography/utils.py

@@ -10,6 +10,8 @@
 import sys
 import warnings

+from pkg_resources import parse_version


Is there any other not-hacky version parser lying around we can use?

alex · 2017-02-14T15:34:21Z

tox.ini

@@ -48,6 +48,14 @@ commands =
    pip list
    py.test --capture=no --strict {posargs}

+# This target downgrades cffi to the minimum supported in setup.py


Please add a comment to setup.py saying this needs to be updated if the minimimum cffi version is changed.

alex · 2017-02-14T15:34:40Z

tox.ini

+    coverage
+    ./vectors
+    cffi==1.4.1
+basepython = python3.5


At this point if we want this to be a py3 build maybe it should be 3.6?

This allows you to provide your own buffer (like recv_into) to improve performance when repeatedly calling encrypt/decrypt on large payloads.

This tests against macOS so we capture some commoncrypto branches

alex · 2017-02-14T15:56:09Z

french toast is a great version

…

On Tue, Feb 14, 2017 at 7:53 AM, Donald Stufft ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/cryptography/utils.py <#3190>: > @@ -10,6 +10,8 @@ import sys import warnings +from pkg_resources import parse_version https://packaging.pypa.io/en/latest/version/ ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3190>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAADBCd0ZigloEx7hD-ahvOAqbBmF5hGks5rcc4MgaJpZM4KRo5B> .

-- "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: D1B3 ADC0 E023 8CA6

reaperhulk · 2017-02-15T20:23:25Z

Jenkins, retest this please

alex · 2017-02-16T04:49:25Z

docs/hazmat/primitives/symmetric-encryption.rst

+            >>> iv = os.urandom(16)
+            >>> cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=backend)
+            >>> encryptor = cipher.encryptor()
+            >>> buf = bytearray(31)


Can you add comments throughout this example?

I took a swing at adding some comments. Hopefully they're useful.

alex

very close!

alex · 2017-02-16T13:57:42Z

docs/hazmat/primitives/symmetric-encryption.rst

@@ -488,12 +488,12 @@ Interfaces
            >>> iv = os.urandom(16)
            >>> cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=backend)
            >>> encryptor = cipher.encryptor()
-            >>> buf = bytearray(31)
+            >>> buf = bytearray(31)  # size the buffer to b len(data) + n - 1


b len(data)?

alex · 2017-02-16T13:57:58Z

docs/hazmat/primitives/symmetric-encryption.rst

            >>> len_encrypted = encryptor.update_into(b"a secret message", buf)
-            >>> ct = bytes(buf[:len_encrypted]) + encryptor.finalize()
+            >>> ct = bytes(buf[:len_encrypted]) + encryptor.finalize()  # get the ciphertext from the buffer reading only the bytes written to it (len_encrypted)


Maybe put the comment on the line above the code, so it doesn't wrap so long?

alex · 2017-02-17T00:43:23Z

docs/hazmat/primitives/symmetric-encryption.rst

+            >>> iv = os.urandom(16)
+            >>> cipher = Cipher(algorithms.AES(key), modes.CBC(iv), backend=backend)
+            >>> encryptor = cipher.encryptor()
+            >>> buf = bytearray(31)  # size the buffer to b len(data) + n - 1


I still don't understand this comment :-)

reaperhulk force-pushed the update-into branch 2 times, most recently from e414eef to 3b875b5 Compare October 12, 2016 17:18

reaperhulk force-pushed the update-into branch from 3b875b5 to 40077be Compare November 12, 2016 08:24

reaperhulk force-pushed the update-into branch from 53f6e01 to dc19fe7 Compare November 13, 2016 06:33

reaperhulk changed the title ~~[WIP] add support for update_into on CipherContext~~ add support for update_into on CipherContext Nov 14, 2016

reaperhulk added this to the Seventeenth Release milestone Nov 22, 2016

reaperhulk modified the milestones: Seventeenth Release, Eighteenth Release Dec 6, 2016

reaperhulk force-pushed the update-into branch from b7624e0 to b49318d Compare December 14, 2016 02:08

reaperhulk commented Feb 11, 2017

View reviewed changes

alex reviewed Feb 14, 2017

View reviewed changes

reaperhulk added 13 commits February 14, 2017 09:48

add support for update_into on CipherContext

b5e23b2

This allows you to provide your own buffer (like recv_into) to improve performance when repeatedly calling encrypt/decrypt on large payloads.

another skip_if

93c8fad

more skip_if complexity

b6026d6

maybe do this right

4efd7c3

correct number of args

143b188

coverage for the coverage gods

09b7326

add a cffi minimum test tox target and travis builder

1f2ac83

This tests against macOS so we capture some commoncrypto branches

extra arg

41c9910

need to actually install py35

bca7f1f

fix

bb8b152

coverage for GCM decrypt in CC

35a5fa1

no longer relevant

964aa62

1.8 now

9db7d9b

pep8

cd07d52

reaperhulk added 2 commits February 14, 2017 12:18

dramatically simplify

0e7e747

update docs

eb2ed24

reaperhulk force-pushed the update-into branch from ed21475 to eb2ed24 Compare February 14, 2017 18:18

reaperhulk added 3 commits February 14, 2017 12:23

remove unneeded test

21e12ae

changelog entry

915cffc

test improvements

6ccdba3

reaperhulk force-pushed the update-into branch from 01d30da to 6ccdba3 Compare February 15, 2017 00:43

coverage fix

827b4e0

alex reviewed Feb 16, 2017

View reviewed changes

add some comments to example

e78e98a

alex reviewed Feb 16, 2017

View reviewed changes

move the comments to their own line

2e57ba2

alex reviewed Feb 17, 2017

View reviewed changes

fix and move comment

1bb3b34

alex approved these changes Feb 17, 2017

View reviewed changes

alex merged commit 9b34ca9 into pyca:master Feb 17, 2017

reaperhulk deleted the update-into branch February 18, 2017 14:46

reaperhulk mentioned this pull request Apr 5, 2017

use setuptools.parse_version in _version_check #3488

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for update_into on CipherContext #3190

add support for update_into on CipherContext #3190

reaperhulk commented Oct 8, 2016

mention-bot commented Oct 8, 2016

reaperhulk commented Nov 6, 2016

reaperhulk commented Nov 12, 2016

codecov-io commented Nov 13, 2016

alex commented Nov 13, 2016

reaperhulk commented Nov 15, 2016

reaperhulk Feb 11, 2017

alex Feb 14, 2017

reaperhulk Feb 14, 2017

dstufft Feb 14, 2017

reaperhulk Feb 14, 2017

alex Feb 14, 2017

reaperhulk Feb 14, 2017

reaperhulk Feb 14, 2017

alex Feb 14, 2017

reaperhulk Feb 14, 2017

alex Feb 14, 2017

alex Feb 14, 2017

alex Feb 14, 2017

alex Feb 14, 2017

alex commented Feb 14, 2017 via email

reaperhulk commented Feb 15, 2017

alex Feb 16, 2017

reaperhulk Feb 16, 2017

alex left a comment

alex Feb 16, 2017

alex Feb 16, 2017

alex Feb 17, 2017

add support for update_into on CipherContext #3190

add support for update_into on CipherContext #3190

Conversation

reaperhulk commented Oct 8, 2016

mention-bot commented Oct 8, 2016

reaperhulk commented Nov 6, 2016

reaperhulk commented Nov 12, 2016

codecov-io commented Nov 13, 2016

Current coverage is 99.94% (diff: 94.32%)

alex commented Nov 13, 2016

reaperhulk commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex commented Feb 14, 2017 via email

reaperhulk commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment