Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for export_keying_material to SSL library #82133

Open
wingel mannequin opened this issue Aug 26, 2019 · 11 comments
Open

Add support for export_keying_material to SSL library #82133

wingel mannequin opened this issue Aug 26, 2019 · 11 comments
Labels
3.10 only security fixes topic-SSL type-feature A feature request or enhancement

Comments

@wingel
Copy link
Mannequin

wingel mannequin commented Aug 26, 2019

BPO 37952
Nosy @wingel, @eighthave
PRs
  • bpo-37952: SSL: add support for export_keying_material #25255
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2019-08-26.08:49:04.430>
    labels = ['expert-SSL', 'type-feature', '3.10']
    title = 'Add support for export_keying_material to SSL library'
    updated_at = <Date 2022-03-20.14:18:50.161>
    user = 'https://github.com/wingel'

    bugs.python.org fields:

    activity = <Date 2022-03-20.14:18:50.161>
    actor = 'eighthave'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['SSL']
    creation = <Date 2019-08-26.08:49:04.430>
    creator = 'wingel71'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37952
    keywords = ['patch']
    message_count = 9.0
    messages = ['350512', '350513', '350514', '390433', '415553', '415569', '415579', '415585', '415604']
    nosy_count = 2.0
    nosy_names = ['wingel71', 'eighthave']
    pr_nums = ['25255']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue37952'
    versions = ['Python 3.10']

    @wingel
    Copy link
    Mannequin Author

    wingel mannequin commented Aug 26, 2019

    Add support for the export_keying_material function to the SSL library.

    Tested with Python 3.7.4 and Python master branch:

    https://github.com/wingel/cpython/tree/export_keying_material-3.7.4
    https://github.com/wingel/cpython/tree/export_keying_material-master

    Is this the correct format for a patch? Should I include the automatically generated clinic changes in my patch or not? What about the "versionadded::" string in the documentation? Should I include a line like that or does it only generate unneccessary conflicts? Anything else I need to do?

    @wingel wingel mannequin added 3.7 (EOL) end of life 3.9 only security fixes labels Aug 26, 2019
    @wingel wingel mannequin assigned tiran Aug 26, 2019
    @wingel wingel mannequin added topic-SSL type-feature A feature request or enhancement labels Aug 26, 2019
    @tiran
    Copy link
    Member

    tiran commented Aug 26, 2019

    Could you please explain the purpose of the feature and why you want to expose the interface? What's the use case?

    As this is a new feature, Python 3.7 and 3.8 are out of scope.

    @tiran tiran removed the 3.7 (EOL) end of life label Aug 26, 2019
    @wingel
    Copy link
    Mannequin Author

    wingel mannequin commented Aug 26, 2019

    I'm doing an implementation of the NTS protocol for my customer Netnod:

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

    NTS is draft RFC on its way to become a standard:

    https://datatracker.ietf.org/doc/draft-ietf-ntp-using-nts-for-ntp/

    NTS requires the export_keying_material functionality as described in RFC5705.

    Basically it's a part of the TLS standard, is used by 10 existing protocols with more on the way. And I can't implement a NTS key establishment server or client without the function. That's why I added the functionality and verified that it works both with the stable 3.7.4 release and with the master branch of the cpython repository.

    I tested with 3.7.4 first on my machine because that's the release of Python that comes with Ubuntu and I wanted to have as few differences as as possible compared to the distribution version. I then forward ported the patch to the master branch and verified that my NTS implementation still works with that branch.

    @wingel
    Copy link
    Mannequin Author

    wingel mannequin commented Apr 7, 2021

    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.

    I have written a patch to add support for this function which can be found on github:

    https://github.com/wingel/cpython

    And it 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 this function in the future so that I don't have to maintain a patched version of Python for this.

    @wingel wingel mannequin added 3.10 only security fixes and removed 3.9 only security fixes labels Apr 7, 2021
    @eighthave
    Copy link
    Mannequin

    eighthave mannequin commented Mar 19, 2022

    We're working on the HTTP Transport Auth draft (https://www.ietf.org/archive/id/draft-schinazi-httpbis-transport-auth-05.html) in the IETF that also needs this method. I would really love to see this land, any advice? If it is just a matter of updating the patch for the current Python, I can probably handle that.

    @wingel
    Copy link
    Mannequin Author

    wingel mannequin commented Mar 19, 2022

    Hi,

    unfortunately the maintainer of the openssl library in Python doesn't
    want to take my patch. He says that he doesn't want the burden of
    supporting more functions in the API. I'm a bit frustrated about the
    whole situation, I've redone my patch over and over again for at least
    six months just to receive no feedback at all and to finally be told
    that it was all in vain. If you add a comment to the merge request
    saying that you also need that functionality it might help to change
    his mind, but probably not. But it would show that it's not only me
    that would like to be able to use that function.

    I have kept my patch up to date up to a few weeks ago so unless
    something major has happened it ought to apply fairly cleanly to the
    latest mainline branch of python.

    https://github.com/wingel/cpython/tree/export_keying_material-master

    Usually there will be conflict due to an automatically generated
    checksum at the end of the file _ssl.c.h but to get around that, just
    skip that part of the patch and rerun "clinic" to regenerate the
    checksum. Here's what I usually do to build and test my patch:

    ./configure --prefix=/opt/python-master

    python3 Tools/clinic/clinic.py -f Modules/_ssl.c
    Modules/clinic/_ssl.c.h
    make -j24
    make install

    Regards,
    Christer

    On Sat, 2022-03-19 at 14:32 +0000, Hans-Christoph Steiner wrote:

    Hans-Christoph Steiner <hans@eds.org> added the comment:

    We're working on the HTTP Transport Auth draft
    (https://www.ietf.org/archive/id/draft-schinazi-httpbis-transport-auth-05.html
    ) in the IETF that also needs this method.  I would really love to
    see this land, any advice?  If it is just a matter of updating the
    patch for the current Python, I can probably handle that.

    ----------
    nosy: +eighthave


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue37952\>


    @tiran
    Copy link
    Member

    tiran commented Mar 19, 2022

    Neither venting frustration at my expense nor emotional blackmail is going to increase the likeliness, that I will spend my limited personal time to review a patch for a new feature. Feel free to find another core dev who is willing to land and maintain your patch.

    @tiran tiran removed their assignment Mar 19, 2022
    @wingel
    Copy link
    Mannequin Author

    wingel mannequin commented Mar 19, 2022

    Sorry about the venting, but it is kind of frustrating to spend months
    working on something with no feedback just to be told that it all was
    for nothing. But that's how it is. I'll just keep updating my path
    every now and then since I need it anyway and don't want my application
    to fall too far behind compared to mainstream Python.

    My point is mostly that that export_keying_material is starting to be
    used in more IETF RFCs. The most recent one was accepted just a few
    weeks ago. I think that is a bit of a shame that Python doesn't have
    support for that functionality out of the box. If enough people say
    it's useful for them maybe that would influence your decision.

    As for the rest of my mail. Since I am trying to keep my patch sort of
    up date, I might as well point to it and explain how to use it.
    Hopefully that will reduce your support burden since it will allow
    those who need that functionality to build a Python interpreter on
    their own.

    @eighthave
    Copy link
    Mannequin

    eighthave mannequin commented Mar 20, 2022

    I understand the frustrations here, but this is really not a place to vent, since that only harms everyone's interests. When a core maintainer voices concerns or questions, they need to be addressed. This goes for any project.

    I'll see if I can contribute to https://bugs.python.org/issue43902, that would also work for exporting keying material.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @jchampio
    Copy link

    jchampio commented Apr 13, 2022

    Could you please explain the purpose of the feature and why you want to expose the interface? What's the use case?

    Another use case: implementing the upcoming tls-exporter channel binding for testing.

    In my case I was able to work around the missing API by setting SSLContext.keylog_filename, and peeling apart that file for the exporter master secret needed to compute the EKM manually. (This is a testing rig, so there's no concern about the secrets leaking to disk.) For a general-purpose (production) library that would probably not be an acceptable solution, so this API would be really helpful to have.

    @Neustradamus
    Copy link

    To follow this ticket

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants