bpo-37952: SSL: add support for export_keying_material #25255

wingel · 2021-04-07T13:57:38Z

Add support for the RFC5705 SSL_export_keying_material function to the Python SSL module.

OpenSSL has a function to "SSL_export_keying_material" as described in RFC5705. This functionality is needed to be able to support a bunch of other protocols such as "Network Time Security for the Network Time Protocol" which has now become a proper RFC as RFC8915. There are half a dozen other RFCs which also use this functionality.

This functionality is used in my implementation of the NTS procotol which can also be found on github:

https://github.com/Netnod/nts-poc-python

It would be very nice if mainline Python could support for t his function in the future so that I don't have to maintain a patched version of Python for this.

https://bugs.python.org/issue37952

tiran

Please include a blurb, tests, and more documentation. Documentation should explain what the function does, the meaning of each argument with examples and explain at which point of the handshake the function is available.

IMHO it would be helpfup to include an enum with most common export labels for standard TLS.

tiran · 2021-04-07T14:03:24Z

Modules/_ssl.c

+/* export_keying_material is present from 1.0.1 */
+#if OPENSSL_VERSION_NUMBER >= 0x10001000L
+# define HAVE_EXPORT_KEYING_MATERIAL 1
+#else
+# define HAVE_EXPORT_KEYING_MATERIAL 0
+#endif
+


Python 3.10 will require OpenSSL >= 1.1.1

tiran · 2021-04-07T14:07:32Z

Modules/_ssl.c

+    if (SSL_export_keying_material(self->ssl,
+                                   key, key_len,
+                                   label->buf, label->len,
+                                   context->buf, context->len,
+                                   context->buf != NULL) != 1) {


Please follow the error checking style of the module:

Suggested change

if (SSL_export_keying_material(self->ssl,

key, key_len,

label->buf, label->len,

context->buf, context->len,

context->buf != NULL) != 1) {

if (!SSL_export_keying_material(self->ssl,

key, key_len,

label->buf, label->len,

context->buf, context->len,

context->buf != NULL)) {

tiran · 2021-04-07T14:08:15Z

Modules/_ssl.c

+        Py_DECREF(out);
+        out = NULL;


Suggested change

Py_DECREF(out);

out = NULL;

Py_CLEAR(out);

tiran · 2021-04-07T14:09:30Z

Modules/_ssl.c

+#if HAVE_EXPORT_KEYING_MATERIAL
+    addbool(m, "HAS_EXPORT_KEYING_MATERIAL", 1);
+#else
+    addbool(m, "HAS_EXPORT_KEYING_MATERIAL", 0);
+#endif
+


Not needed, the feature is always present in OpenSSL 1.1.1

Suggested change

#if HAVE_EXPORT_KEYING_MATERIAL

addbool(m, "HAS_EXPORT_KEYING_MATERIAL", 1);

#else

addbool(m, "HAS_EXPORT_KEYING_MATERIAL", 0);

#endif

tiran · 2021-04-07T14:09:53Z

Lib/ssl.py

@@ -1295,6 +1295,14 @@ def verify_client_post_handshake(self):
        else:
            raise ValueError("No SSL wrapper around " + str(self))

+    def export_keying_material(self, label, key_len, context = None):


Suggested change

def export_keying_material(self, label, key_len, context = None):

def export_keying_material(self, label, key_len, context=None):

tiran · 2021-04-07T14:11:53Z

Modules/_ssl.c

+    if (key_len < 1) {
+        PyErr_SetString(PyExc_ValueError, "key_len must be positive");
+        return NULL;
+    }


key_len should probably be limited, too.

tiran · 2021-04-07T14:12:16Z

Doc/library/ssl.rst

@@ -1293,6 +1293,12 @@ SSL sockets also have the following additional methods and attributes:

   .. versionadded:: 3.3

+.. method:: SSLSocket.export_keying_material(label, key_len, context)


Suggested change

.. method:: SSLSocket.export_keying_material(label, key_len, context)

.. method:: SSLSocket.export_keying_material(label, key_len, context=None)

tiran · 2021-04-07T14:15:22Z

Modules/_ssl.c

+    label: Py_buffer(accept={buffer, str, NoneType})
+    key_len: int
+    context: Py_buffer(accept={buffer, str, NoneType})


Why do you accept buffer, str, and None? Is this suppose to be optional text or bytes? The method should either accept text or bytes.

bedevere-bot · 2021-04-07T14:23:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tiran · 2021-04-07T14:39:52Z

Test scenarios:

TLS 1.2 server and client side
TLS 1.3 server and client side
before handshake
during handshake
after handshake
after connection is closed
valid and invalid labels
with context, with zero length context, without context
multiple key lengths

Please also include edge cases for label and context values, e.g. embedded NULL byte, non-ASCII, ...

wingel · 2021-04-14T13:03:10Z

I've updated the patch and I believe that I've addressed all the review comments now.

tiran

Please fix all tests and then follow the instructions of bedevere-bot.

wingel · 2021-04-15T10:14:25Z

I was told that Python 3.10 will require OpenSSL 1.1.1. Some of these failures are because the tests are run with OpenSSL 1.1.0l which does not seem to support TLSv1.3. Oh well, I've added a check which skips the test if an older version of OpenSSL is used.

And now all tests pass.

tiran

See https://discuss.python.org/t/openssl-3-0-0-support-3-8-to-3-10/8205. I'll drop 1.1.0 and 1.0.2 support next week or the week after.

You need to split the tests and protect each protocol test with @requires_tls_version decorator for each used TLS version.

And please read and follow the instructions of bedevere-bot. The bot will trigger workflow changes and notify me when everything is ok.

wingel · 2021-04-16T10:50:24Z

All checks pass now. It there anything else that's needed?

wingel · 2021-04-22T09:41:30Z

Pull request updated to work after bpo-42333 and bpo-43669.

wingel · 2021-04-26T08:43:27Z

I have made the requested changes; please review again

wingel · 2021-08-19T09:41:27Z

@tiran Can you please take a look at this patch again?

wingel · 2021-08-19T09:48:06Z

I have made the requested changes; please review again

bedevere-bot · 2021-08-19T09:48:08Z

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

lowinger42 · 2021-10-12T14:49:05Z

@tiran Is there any way to get this pull request accepted?
/AL

Add support for the RFC5705 SSL_export_keying_material function to the Python SSL module.

wingel · 2022-01-11T10:14:26Z

Yet another year, yet another update of my patch.

I have made the requested changes; please review again

bedevere-bot · 2022-01-11T10:14:28Z

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

wingel · 2022-01-11T10:25:39Z

Have I missed anything? It would be nice to get some kind of feedback.

tiran · 2022-01-11T11:14:26Z

I no longer think that this feature should land in Python core. The ssl module provides a opinionated and limited API. Exporting key material a special feature for a narrow use case. Python core dev has limited resources. I'm basically the only maintainer of the ssl module and I don't want to burden myself with supporting this feature for the next five to ten years.

I would rather move https://bugs.python.org/issue43902 forward so third party developers can use ctypes, cffi, or C code to access and modify raw SSL_CTX* and SSL* pointers.

eighthave · 2022-03-20T13:51:32Z

@tiran thanks for your ongoing maintenance of ssl! Although I would like to see this pull request land, https://bugs.python.org/issue43902 would also work fine for my use case. Exporting keying material is being used more and more in IETF standards, and is pretty widely implemented:

pyOpenSSL
rustls
Go TLS
Conscrypt
nodejs
openssl, boringssl, NSS, Microsoft TLS, BouncyCastle, wolfSSL

TheZ3ro · 2023-03-11T16:03:41Z

Sad to see that Python still don't have this in its ssl module :(
Basically every language has this and its becoming more and more used.

Is there an alternative way to export keying material?

Neustradamus · 2023-07-30T03:11:29Z

@wingel, @tiran: Have you progressed on it since more one year?

wingel requested a review from tiran as a code owner April 7, 2021 13:57

the-knights-who-say-ni added the CLA signed label Apr 7, 2021

bedevere-bot added the awaiting review label Apr 7, 2021

tiran requested changes Apr 7, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Apr 7, 2021

wingel force-pushed the export_keying_material-master branch 2 times, most recently from 53c6c8b to 7725b00 Compare April 14, 2021 12:59

wingel requested a review from tiran April 15, 2021 06:51

tiran requested changes Apr 15, 2021

View reviewed changes

wingel force-pushed the export_keying_material-master branch 7 times, most recently from 04f2f2a to d20645a Compare April 15, 2021 09:47

wingel requested a review from tiran April 15, 2021 10:14

tiran requested changes Apr 15, 2021

View reviewed changes

wingel force-pushed the export_keying_material-master branch from d20645a to adb9ead Compare April 15, 2021 13:45

wingel force-pushed the export_keying_material-master branch 2 times, most recently from 398cd73 to 60f65cd Compare April 22, 2021 09:19

wingel requested a review from tiran April 22, 2021 09:40

wingel force-pushed the export_keying_material-master branch 4 times, most recently from 4e1865f to c23bd08 Compare June 14, 2021 08:32

wingel force-pushed the export_keying_material-master branch 2 times, most recently from 97884bb to c1b14f0 Compare August 18, 2021 11:46

bpo-37952: SSL: add support for export_keying_material

51a8a5c

Add support for the RFC5705 SSL_export_keying_material function to the Python SSL module.

wingel force-pushed the export_keying_material-master branch from c1b14f0 to 51a8a5c Compare January 11, 2022 10:05

AlexWaygood added type-feature and removed stale labels Jan 11, 2022

tiran removed their request for review January 11, 2022 10:53

tiran added the DO-NOT-MERGE label Jan 11, 2022

gst approved these changes Mar 20, 2022

View reviewed changes

wingel mannequin mentioned this pull request Apr 13, 2022

Add support for export_keying_material to SSL library #82133

Open

ezio-melotti removed the CLA signed label Jul 13, 2022

Neustradamus mentioned this pull request Jul 31, 2022

RFC 9266: Channel Bindings for TLS 1.3 support #95350

Closed

TheZ3ro mentioned this pull request May 17, 2023

Implement the pair command for Android 11+ devices JeffLIrion/adb_shell#206

Open

This was referenced Feb 9, 2024

RFC 9266: Channel Bindings for TLS 1.3 support #115193

Closed

tls-server-end-point RFC5929 (Channel Binding) missing support #115195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

bpo-37952: SSL: add support for export_keying_material #25255

bpo-37952: SSL: add support for export_keying_material #25255

wingel commented Apr 7, 2021 •

edited by bedevere-bot

Loading

tiran left a comment

tiran Apr 7, 2021

tiran Apr 7, 2021

tiran Apr 7, 2021

tiran Apr 7, 2021

tiran Apr 7, 2021

tiran Apr 7, 2021

tiran Apr 7, 2021

tiran Apr 7, 2021

bedevere-bot commented Apr 7, 2021

tiran commented Apr 7, 2021

wingel commented Apr 14, 2021

tiran left a comment

wingel commented Apr 15, 2021

tiran left a comment

wingel commented Apr 16, 2021 •

edited

Loading

wingel commented Apr 22, 2021 •

edited by bedevere-bot

Loading

wingel commented Apr 26, 2021

wingel commented Aug 19, 2021

wingel commented Aug 19, 2021

bedevere-bot commented Aug 19, 2021

lowinger42 commented Oct 12, 2021

wingel commented Jan 11, 2022

bedevere-bot commented Jan 11, 2022

wingel commented Jan 11, 2022

tiran commented Jan 11, 2022

eighthave commented Mar 20, 2022 •

edited

Loading

TheZ3ro commented Mar 11, 2023

Neustradamus commented Jul 30, 2023

	def export_keying_material(self, label, key_len, context = None):
	def export_keying_material(self, label, key_len, context=None):

		@@ -1293,6 +1293,12 @@ SSL sockets also have the following additional methods and attributes:

		.. versionadded:: 3.3

		.. method:: SSLSocket.export_keying_material(label, key_len, context)

bpo-37952: SSL: add support for export_keying_material #25255

Are you sure you want to change the base?

bpo-37952: SSL: add support for export_keying_material #25255

Conversation

wingel commented Apr 7, 2021 • edited by bedevere-bot Loading

tiran 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

bedevere-bot commented Apr 7, 2021

tiran commented Apr 7, 2021

wingel commented Apr 14, 2021

tiran left a comment

Choose a reason for hiding this comment

wingel commented Apr 15, 2021

tiran left a comment

Choose a reason for hiding this comment

wingel commented Apr 16, 2021 • edited Loading

wingel commented Apr 22, 2021 • edited by bedevere-bot Loading

wingel commented Apr 26, 2021

wingel commented Aug 19, 2021

wingel commented Aug 19, 2021

bedevere-bot commented Aug 19, 2021

lowinger42 commented Oct 12, 2021

wingel commented Jan 11, 2022

bedevere-bot commented Jan 11, 2022

wingel commented Jan 11, 2022

tiran commented Jan 11, 2022

eighthave commented Mar 20, 2022 • edited Loading

TheZ3ro commented Mar 11, 2023

Neustradamus commented Jul 30, 2023

wingel commented Apr 7, 2021 •

edited by bedevere-bot

Loading

wingel commented Apr 16, 2021 •

edited

Loading

wingel commented Apr 22, 2021 •

edited by bedevere-bot

Loading

eighthave commented Mar 20, 2022 •

edited

Loading