OpenSSL DH backend implementation [Second attempt] #2914

palaviv · 2016-05-20T12:25:23Z

This PR is mainly based on PR #1721.
I made the following changes:

The key exchange API is very similar to the ECDH API. The DHPrivateKey has an exchange method that receives a DHPublicKey.
The default_backend support all DHBackend methods.

…mentation

…alization

alex · 2016-05-20T12:26:08Z

Jenkins, ok to test.

alex · 2016-05-20T12:27:39Z

src/cryptography/hazmat/backends/multibackend.py

+
+    def generate_dh_private_key_and_parameters(self, generator, key_size):
+        for b in self._filtered_backends(DHBackend):
+            return b.generate_dh_private_key_and_parameters( generator, key_size)


No space before generator

palaviv · 2016-05-23T22:28:04Z

It seems that all the tests that have failed in this PR are not caused by the changes. Are the InvocationError known issue?

reaperhulk · 2016-05-23T22:44:26Z

No, not sure what's going on there just yet...

reaperhulk · 2016-05-23T22:44:32Z

Jenkins, retest this please.

alex · 2016-05-29T21:30:06Z

Jenkins, retest this please.

palaviv · 2016-06-03T11:56:44Z

I can't understand why my patch fail the coverage tests. If anyone can help me understand I would appreciate it.

alex · 2016-06-03T14:59:49Z

The data out of codecov is making no sense here, I wouldn't worry about it for now.

alex · 2016-06-03T17:01:39Z

src/_cffi_src/openssl/dh.py

+    BIGNUM *q;
+    BIGNUM *j;
+    ...;
+} DH;


mmm, so this definitiion, using the struct fields, won't work in OpenSSL 1.1.0, so this will have to be ported to use the various accessors that OpenSSL exposes.

In OpenSSL 1.1.0 the DH_get0_pqg, DH_set0_pqg, DH_get0_key, DH_set0_key (https://www.openssl.org/docs/manmaster/crypto/DH_get0_pqg.html) functions were added. But I think that porting this struct will be more practical then using them.

Those functions will need to be declared and used. You can see examples in the rsa bindings. Going forward all bindings need to be compatible with 1.1.0 as well as earlier versions.

alex · 2016-06-04T23:49:45Z

There's merge conflicts here :-(

palaviv · 2016-06-05T15:52:24Z

Fixed the merge conflicts :-)

alex · 2016-06-05T21:42:17Z

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

+        assert res == 1
+
+        if codes[0] != 0:
+            raise ValueError("DH private numbers did not pass safety checks.")


This is the line missing coverage.

reaperhulk · 2016-06-06T15:27:19Z

Since this was based off an old PR it has the old assert model. Consequently all the asserts need to be replaced with calls to the openssl_assert method. You can see how this works in other parts of the openssl backend's backend.py. (Thanks for working on this)

palaviv · 2016-06-06T18:31:36Z

I changed the PR to the openssl_assert @reaperhulk. Thank you for the CR :)

reaperhulk · 2016-11-20T01:36:34Z

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

+        self.openssl_assert(res == 1)
+
+        codes = self._ffi.new("int[]", 1)
+        res = self._lib.DH_check(dh_cdata, codes)


This will fail with DH_UNABLE_TO_CHECK_GENERATOR for any generator that's not 2 or 5. However, we don't limit to 2/5 in the parameter creation, which means you can write code that fails this check like so:

params = dh.generate_parameters(7, 512, backend) params.generate_private_key().private_numbers().private_key(backend)

To fix this let's limit the generator to 2 or 5 for now (we can revisit whether we need to change how we use DH_check at a later date if users show up asking for more generator flexibility)

reaperhulk

We're getting close now, thanks for working so hard on this PR!

reaperhulk · 2016-11-21T03:21:31Z

docs/hazmat/primitives/asymmetric/dh.rst

+        :return bytes: The agreed key. The bytes are ordered in 'big' endian.
+
+
+.. class:: DHPrivateNumbers(x, public_numbers)


The 3 numbers classes should be grouped under a

Numbers ~~~~~~~

section (like RSA).

reaperhulk · 2016-11-21T03:25:24Z

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

+def _handle_dh_compute_key_error(errors, backend):
+    lib = backend._lib
+
+    backend.openssl_assert(errors[0][1:] == (


(This is still an outstanding question)

reaperhulk · 2016-11-21T03:28:35Z

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

+        buf = self._backend._ffi.new("unsigned char[]", self._key_size)
+        res = self._backend._lib.DH_compute_key(
+            buf,
+            self._backend._int_to_bn(peer_public_key.public_numbers().y),


I'd prefer to get the public value using DH_get0_key in this case. No need to do BN -> int -> BN conversion.

DH_get0_key should allow you to pass null for priv_key btw

reaperhulk · 2016-11-21T03:29:51Z

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

+            key = self._backend._ffi.buffer(buf)[:res]
+            pad = self._key_size - len(key)
+
+            if pad > 0:


DH_compute_key doesn't do this for you? (Clearly it doesn't, just want to make sure I understand why we need to do this)

Yes you can see test_symmetric_key_padding that actually cover this specific flow.

reaperhulk · 2016-11-21T03:32:20Z

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

+            dh_cdata, self._backend._lib.DH_free
+        )
+
+        p = self._backend._ffi.new("BIGNUM **")


DHParams_dup can give you a new DH * that already has p and g populated, which will simplify a lot of this code (you'll still need to register it for gc and set the pub_key though)

reaperhulk · 2016-11-21T03:34:54Z

tests/hazmat/primitives/test_dh.py

+        assert backend.dh_parameters_supported(23, 5)
+        assert not backend.dh_parameters_supported(23, 18)
+
+    def test_convert_to_serialized(self, backend):


Could we change the tests with the name "serialized" to say "numbers" instead? In general we tend to use serialization for PEM/DER serialization to bytes rather than the *Numbers integer repr classes

reaperhulk · 2016-11-21T03:38:56Z

tests/hazmat/primitives/test_dh.py

+        key = private.private_key(backend)
+        symkey = key.exchange(public.public_key(backend))
+
+        assert int_from_bytes(symkey, 'big') == int(vector["k"], 16)


A bit nitpicky, but could we have this be assert symkey == int_to_bytes(int(vector["k"], 16))? I haven't tested that so please confirm I did it right!

I can do this but I think this way is better because of the padding. In case that any of the vectors need padding your test will fail because the symkey buffer is padded.

reaperhulk · 2016-11-21T03:41:02Z

tests/hazmat/primitives/test_dh.py

+        assert len(symkey) == 512 // 8
+        assert symkey[:1] == b'\x00'
+
+    bad_tls_exchange_params = [(


Could you move these values into a vector file (with a few small comments noting in what way each one is broken)?

reaperhulk · 2016-11-21T23:05:20Z

jenkins, retest this please

alex · 2016-11-22T01:09:58Z

(Moved this to the 17th release, which we hope to get done in 2-3 weeks)

reaperhulk · 2016-11-22T01:05:30Z

vectors/cryptography_vectors/asymmetric/DH/bad_exchange.txt

@@ -0,0 +1,19 @@
+# This are pairs of DH vectors that an exchange between them will result in an error.


s/this/these

Please add a comment about what type of failure each one causes and that these were custom generated.

This vector file also needs an entry in test-vectors.rst.

reaperhulk

Hopefully just one last round here! Sorry for the scattershot comments, the PR is so large that I end up reviewing the entire thing over and over and I'm catching new things...

reaperhulk · 2016-11-23T02:41:30Z

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

+    def __init__(self, backend, dh_cdata):
+        self._backend = backend
+        self._dh_cdata = dh_cdata
+        self._key_size = self._backend._lib.DH_size(dh_cdata)


We allow non-byte aligned bit lengths when generating parameters. DH_size returns the byte length. For key_size multiplying by 8 is not a safe assumption because p could be 1021 bits, which would show up as 128 bytes. I think we need to rename this to _key_size_bytes.

reaperhulk · 2016-11-23T02:43:19Z

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

+
+    @property
+    def key_size(self):
+        return self._key_size * 8


With the change to _key_size_bytes this no longer works. But you can get the value by doing a get0_pqg with null for q and g and then calling BN_num_bits on p[0] (you can memoize this in init if you want, I don't have a preference)

reaperhulk · 2016-11-23T02:43:41Z

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

+
+    def exchange(self, peer_public_key):
+
+        buf = self._backend._ffi.new("unsigned char[]", self._key_size)


This becomes _key_size_bytes

reaperhulk · 2016-11-23T02:45:01Z

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

+            self._backend.openssl_assert(res >= 1)
+
+            key = self._backend._ffi.buffer(buf)[:res]
+            pad = self._key_size - len(key)


Also _key_size_bytes now. Padding should still work as expected for non-byte aligned keys since the data written to key by necessity must already be padded?

reaperhulk · 2016-11-23T02:45:33Z

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

+    def __init__(self, backend, dh_cdata):
+        self._backend = backend
+        self._dh_cdata = dh_cdata
+        self._key_size = self._backend._lib.DH_size(dh_cdata) * 8


Same problem with bits vs bytes. Since this is only used for key_size I'd suggest renaming it to _key_size_bits and doing get0_pqg. You can factor this out into a function _get_dh_num_bits since we're using it in two places.

reaperhulk · 2016-11-23T02:51:14Z

docs/development/test-vectors.rst

@@ -463,3 +470,4 @@ header format (substituting the correct information):
 .. _`Russian CA`: https://e-trust.gosuslugi.ru/MainCA
 .. _`test/evptests.txt`: https://github.com/openssl/openssl/blob/2d0b44126763f989a4cbffbffe9d0c7518158bb7/test/evptests.txt
 .. _`unknown signature OID`: https://bugzilla.mozilla.org/show_bug.cgi?id=405966
+.. _`botan`: https://raw.githubusercontent.com/randombit/botan/master/src/tests/data/pubkey/dh.vec


Let's make this link https://github.com/randombit/botan/blob/57789bdfc55061002b2727d0b32587612829a37c/src/tests/data/pubkey/dh.vec

reaperhulk · 2016-11-23T02:52:17Z

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

@@ -0,0 +1,182 @@
+# Licensed under the Apache License, Version 2.0 (the "License");


This license needs to be replaced with the following:

# This file is dual licensed under the terms of the Apache License, Version # 2.0, and the BSD License. See the LICENSE file in the root of this repository # for complete details.

palaviv · 2016-11-24T20:04:55Z

Thanks for the CR @reaperhulk I just hope that it will be ready in time for 1.7 :)

reaperhulk · 2016-11-25T14:50:55Z

One last request @palaviv: please add a changelog entry describing the new feature! Once that's there and the tests have passed I'm ready to merge. 🎉

reaperhulk · 2016-11-25T14:52:14Z

For the record, I have verified the vectors against the botan link in question and will do so again once the changelog commit is pushed.

palaviv · 2016-11-25T15:54:15Z

😃😃😃

public and others added 6 commits April 12, 2015 19:55

Start of OpenSSL DH backend implementation

8ef2666

Merge branch 'master' into dh-impl-2

f7c7a10

Supporting DH in MultiBackend

8823a07

DHBackend has dh_parameters_supported method

7276b3c

Removed DHParametersWithNumbers and DHPrivateKeyWithNumbers from docu…

7ba06f3

…mentation

Removed ExchangeContext. exchange is a method of DHPrivateKeyWithSeri…

e83c682

…alization

alex added the primitives label May 20, 2016

alex reviewed May 20, 2016
View reviewed changes

palaviv added 3 commits May 22, 2016 18:51

PEP8 fixes

bb29c9e

Fixed TestDH.test_bad_tls_exchange

141d675

Fixed generate_private_key reference in dh documentation

46693c8

palaviv added 3 commits June 1, 2016 23:35

test DH multibackend support

4b78e14

testing DH coversion to serialized

bf47b37

Validating that we receive serialized class in test_generate_dh

38c13e7

palaviv force-pushed the dh-impl-2 branch from 6ba3934 to 38c13e7 Compare June 2, 2016 15:32

Testing DH exchange symmetric key padding

9efa147

alex reviewed Jun 3, 2016
View reviewed changes

Merge branch 'master' of github.com:pyca/cryptography into dh-impl-2

4e1dffd

alex reviewed Jun 5, 2016
View reviewed changes

Fix key type to unsigned char as expected by DH_compute_key

c1deb02

reaperhulk reviewed Nov 20, 2016

View reviewed changes

palaviv added 2 commits November 20, 2016 20:53

Validate that DH generator is 2 or 5

e94382e

test dh exchange using botan vectors

0a7dd59

palaviv force-pushed the dh-impl-2 branch from 2e4cba0 to 0a7dd59 Compare November 20, 2016 21:24

reaperhulk requested changes Nov 21, 2016

View reviewed changes

palaviv added 3 commits November 21, 2016 18:20

group all numbers classes

2980cdd

Simplify _DHPrivateKey

94dc95b

Rename test with serialized to numbers

1a0483c

alex modified the milestones: Seventeenth Release, Sixteenth Release Nov 22, 2016

reaperhulk requested changes Nov 22, 2016

View reviewed changes

palaviv force-pushed the dh-impl-2 branch from d0e02b8 to 5b51c8a Compare November 22, 2016 16:54

palaviv added 2 commits November 22, 2016 22:31

Move bad exchange params to external vector file

b0b44a4

update exchange versionadded to 1.7

8e10551

palaviv force-pushed the dh-impl-2 branch from 5b51c8a to 8e10551 Compare November 22, 2016 20:31

reaperhulk requested changes Nov 23, 2016

View reviewed changes

palaviv added 2 commits November 24, 2016 22:02

Make key_size bit accurate

9b2b694

Change botan link

df4231b

reaperhulk approved these changes Nov 25, 2016

View reviewed changes

palaviv added 2 commits November 25, 2016 17:28

Merge branch 'master' into dh-impl-2

1d0fee5

Added CHANGELOG entry

a7d6815

reaperhulk merged commit 495f21a into pyca:master Nov 25, 2016

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

		:return bytes: The agreed key. The bytes are ordered in 'big' endian.


		.. class:: DHPrivateNumbers(x, public_numbers)

		@@ -0,0 +1,19 @@
		# This are pairs of DH vectors that an exchange between them will result in an error.


		def exchange(self, peer_public_key):

		buf = self._backend._ffi.new("unsigned char[]", self._key_size)

		@@ -0,0 +1,182 @@
		# Licensed under the Apache License, Version 2.0 (the "License");

OpenSSL DH backend implementation [Second attempt] #2914

OpenSSL DH backend implementation [Second attempt] #2914

Conversation

palaviv commented May 20, 2016

alex commented May 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palaviv commented May 23, 2016

reaperhulk commented May 23, 2016

reaperhulk commented May 23, 2016

alex commented May 29, 2016

palaviv commented Jun 3, 2016

alex commented Jun 3, 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

alex commented Jun 4, 2016

palaviv commented Jun 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk commented Jun 6, 2016

palaviv commented Jun 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk 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

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

reaperhulk commented Nov 21, 2016

alex commented Nov 22, 2016

Choose a reason for hiding this comment

reaperhulk 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

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

palaviv commented Nov 24, 2016

reaperhulk commented Nov 25, 2016

reaperhulk commented Nov 25, 2016

palaviv commented Nov 25, 2016