Scrypt Implementation #3117

Ayrx · 2016-08-27T13:50:44Z

Definitely not ready for reviews yet!

alex · 2016-08-27T13:53:16Z

src/cryptography/hazmat/backends/multibackend.py

+    def derive_scrypt(self, key_material, salt, length, N, r, p):
+        for b in self._filtered_backends(ScryptBackend):
+            return b.derive_scrypt(key_material, salt, length, N, r, p)
+        Raise UnsupportedAlgorithm("This backend does not support scrypt.")


You've got yourself a SyntaxError here.

alex · 2016-08-27T15:55:17Z

I think you can skip the scrypt_supported, and instead just conditionally register the interface.

Ayrx · 2016-08-27T15:56:50Z

@alex Hmm, fair idea. I'll give that a shot.

Ayrx · 2016-08-28T06:23:44Z

@reaperhulk, @alex I need some API advice on this.

So, EVP_PBE_scrypt has a parameter uint64_t maxmem which defines the maximum memory that the function can use. When this parameter is set to 0, which I have currently opted to do in this PR, it defaults to the value of SCRYPT_MAX_MEM. When this isn't configured at build time, it defaults to (1024 * 1024 * 32) (32MB). This value is too small to have one of the test vectors pass. (Here is the link to the part of the source code.)

What do you think we should pass in to maxmem to ensure the most consistent experience overall? I don't think this should be exposed as part of the user facing API as it isn't defined in the specification and it seems like a OpenSSL specific thing (libscrypt doesn't have it for example...).

reaperhulk · 2016-08-28T13:20:40Z

It looks like if SCRYPT_MAX_MEM is set to zero during compile then it defines it to ((size_t)-1)/2. How about setting it to that by default (to be conservative we can use a 32-bit size_t)?

Ayrx · 2016-08-28T13:24:24Z

@reaperhulk What I don't like about that is that it breaks expectations if the user compiles OpenSSL with that particular option enabled.

After some hours of deliberation, my thoughts will be to handle that error gracefully (which we'd need to do anyway), and have our CI compile OpenSSL with SCRYPT_MAX_MEM = 0 which sets it to ((size_t)-1). Thoughts?

alex · 2016-08-28T13:26:30Z

src/cryptography/hazmat/backends/openssl/backend.py


+# If the backend has support for Scrypt, dynamically register the backend and
+# implement the interface.
+def _derive_scrypt(self, key_material, salt, length, n, r, p):


This method should just be on the class normally.

Won't this break if the user uses the OpenSSL backend directly instead of going through default_backend()?

No, because https://github.com/pyca/cryptography/pull/3117/files#diff-2b076058f22fe576413155fbf8ebafbdR19 will catch it if ScryptBackend is not registered so even though the method is present it won't be called.

D'oh. Right. :)

reaperhulk · 2016-08-28T13:27:02Z

Users are going to use whatever the default of their distro is in 99.9% of cases. If that number is low enough to cause failures when setting scrypt variables that's problematic...and 32MB is not a particularly huge number.

We definitely should handle the error gracefully, but I'm 👎 on anything that requires a custom compile flag to make our tests pass.

Ayrx · 2016-08-28T13:33:09Z

On the contrary I'd say that in practice almost no one is going to be using such high memory settings in production because that is 32MB every time you derive a key with Scrypt and very little systems can support that with any kind of practical load.

reaperhulk · 2016-08-28T13:37:38Z

On a system with 64GB of RAM that's 1/2000th of available memory, and 64GB is not hard to come by. That said, maybe you can convince me that "large enough to pass the test vectors" is a good starting point and then we can change it in the future if users complain.

alex · 2016-08-28T17:18:48Z

Ultimately ram usage is controlled by the 3 parameters scrypt takes, so if people want it to use less RAM they should change those. I have no problem setting max memory to sys.maxsize

alex · 2016-08-28T17:20:36Z

src/cryptography/hazmat/backends/openssl/backend.py

+# implement the interface.
+def _derive_scrypt(self, key_material, salt, length, n, r, p):
+    buf = self._ffi.new("unsigned char[]", length)
+    self._lib.EVP_PBE_scrypt(key_material, len(key_material), salt,


You need to check the return value here.

alex · 2016-08-28T17:22:14Z

(Will also need docs and a changelog entry)

reaperhulk · 2016-08-29T06:26:54Z

Looks like there's a small pep8 issue to clear up too :)

reaperhulk · 2016-08-29T06:36:12Z

tests/hazmat/primitives/test_scrypt.py

+from tests.utils import load_nist_vectors, load_vectors_from_file
+
+vectors = load_vectors_from_file(
+    "KDF/scrypt.txt", load_nist_vectors)


This should be os.path.join("KDF", "scrypt.txt")

alex · 2016-08-29T12:21:54Z

register_interface_if merged

reaperhulk · 2016-08-29T13:48:11Z

docs/hazmat/primitives/key-derivation-functions.rst

+    :param int r: Block size parameter.
+    :param int p: Parallelization parameter.
+    :param backend: An instance of
+        :class:`~cryptography.hazmat.backends.interfaces.ScryptBackend`.


This isn't documented, that's why the tests are failing.

Ayrx · 2016-08-30T12:32:38Z

Working on a better description for N, r and p now. Will push it up when I'm done.

Ayrx · 2016-08-30T14:34:46Z

Alright, I hope that explanation makes sense to people as much as it does in my head. Suggestions welcomed. :)

Conflicts: CHANGELOG.rst

Ayrx · 2016-09-01T02:34:06Z

@alex @reaperhulk ping

reaperhulk · 2016-09-01T09:18:13Z

docs/hazmat/primitives/key-derivation-functions.rst

 .. _`key stretching`: https://en.wikipedia.org/wiki/Key_stretching
 .. _`HKDF`: https://en.wikipedia.org/wiki/HKDF
 .. _`HKDF paper`: https://eprint.iacr.org/2010/264
+.. _`RFC 7914`: https://tools.ietf.org/html/rfc7914


This link is not required now that you're using the :rfc: setup.

reaperhulk · 2016-09-01T09:29:58Z

Time for one quick question in ignorance: We have an AlreadyFinalized for our other KDFs that prevents calling derive more than once. Should we do that here?

Ayrx · 2016-09-01T12:03:58Z

I think it should for consistency sake. Added.

reaperhulk · 2016-09-01T12:39:26Z

Anybody remember why we added AlreadyFinalized as an exception?

alex · 2016-09-01T12:40:48Z

Because salt is in the constructor and we don't want to make it easy to
reuse a salt.

On Thu, Sep 1, 2016 at 8:39 AM, Paul Kehrer notifications@github.com
wrote:

Anybody remember why we added AlreadyFinalized as an exception?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3117 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAADBPHxGOaIYGMS9KENuvDn5GMHXgI9ks5qlsd-gaJpZM4JuswS
.

"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

Ayrx added 5 commits August 27, 2016 13:40

Scrypt implementation.

06288ab

Docs stuff.

ba96e7e

Make example just an example and not a doctest.

d5d0c83

Add changelog entry.

4e85946

Docs cleanup.

70a7ed0

alex reviewed Aug 27, 2016
View reviewed changes

reaperhulk mentioned this pull request Aug 27, 2016

1.1.0 Features To Add #2968

Closed

5 tasks

alex added this to the Sixteenth Release milestone Aug 27, 2016

Add more tests.

ca40cf9

Ayrx added 3 commits August 28, 2016 00:34

Add multibackend tests.

5133441

PEP8.

79ffdac

Add docs about Scrypt parameters.

271c4fa

alex reviewed Aug 28, 2016
View reviewed changes

reaperhulk reviewed Aug 29, 2016
View reviewed changes

Ayrx mentioned this pull request Aug 29, 2016

Add a register_interface_if decorator. #3120

Merged

reaperhulk reviewed Aug 29, 2016
View reviewed changes

Ayrx changed the title ~~[WIP] Scrypt Implementation~~ Scrypt Implementation Aug 30, 2016

Merge branch 'master' into scrypt-interface

5f6a21b

Conflicts: CHANGELOG.rst

reaperhulk reviewed Sep 1, 2016
View reviewed changes

Ayrx added 2 commits September 1, 2016 19:44

Docs cleanup.

9e19cd8

Add AlreadyFinalized.

39c46f2

reaperhulk merged commit d8a27df into pyca:master Sep 1, 2016

reaperhulk mentioned this pull request Dec 15, 2016

scrypt failures on 32-bit arches #3323

Closed

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

Scrypt Implementation #3117

Scrypt Implementation #3117

Uh oh!

Conversation

Ayrx commented Aug 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex commented Aug 27, 2016

Uh oh!

Ayrx commented Aug 27, 2016

Uh oh!

Ayrx commented Aug 28, 2016

Uh oh!

reaperhulk commented Aug 28, 2016

Uh oh!

Ayrx commented Aug 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reaperhulk commented Aug 28, 2016

Uh oh!

Ayrx commented Aug 28, 2016

Uh oh!

reaperhulk commented Aug 28, 2016

Uh oh!

alex commented Aug 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex commented Aug 28, 2016

Uh oh!

reaperhulk commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayrx commented Aug 30, 2016

Uh oh!

Ayrx commented Aug 30, 2016

Uh oh!

Ayrx commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reaperhulk commented Sep 1, 2016

Uh oh!

Ayrx commented Sep 1, 2016

Uh oh!

reaperhulk commented Sep 1, 2016

Uh oh!

alex commented Sep 1, 2016

Uh oh!

Uh oh!