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.

Sign up for GitHub

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

Jump to bottom

remove NPN bindings -- you should be using ALPN! #4765

Merged
merged 5 commits into from
Jul 6, 2020
Merged

remove NPN bindings -- you should be using ALPN! #4765

merged 5 commits into from
Jul 6, 2020

Conversation

reaperhulk
Copy link
Member

pyOpenSSL consumed these, but we've marked it as deprecated and it already handles the case where the bindings are not available.

@reaperhulk reaperhulk mentioned this pull request Feb 25, 2019
Closed
@alex
Copy link
Member

alex commented Feb 25, 2019

We need to uncondtionally set Cryptography_HAS_NEXTPROTONEG to false if you do this

@alex
Copy link
Member

alex commented Feb 25, 2019

What about SSL_select_next_proto and OPENSSL_NPN_NEGOTIATED

@reaperhulk
Copy link
Member Author

SSL_select_next_proto is apparently also usable in an ALPN context: https://www.openssl.org/docs/man1.0.2/man3/SSL_select_next_proto.html

OPENSSL_NPN_NEGOTIATED is declared even if OPENSSL_NO_NEXTPROTONEG is declared, but there's no reason for it to exist so we can remove it.

alex
alex previously approved these changes Feb 25, 2019
View reviewed changes
@reaperhulk reaperhulk mentioned this pull request Apr 6, 2019
Merged
glyph added a commit to twisted/twisted that referenced this pull request Apr 7, 2019
@glyph
Merge pull request #1121 from glyph/9614-npnalpn-skip
24e48fc 
Author: reaperhulk

Reviewer: glyph

Fixes: ticket:9614

NPNAndALPNAbsentTests skip iff both are absent

Previously these tests only skipped if they failed to detect NPN on the assumption that ALPN would never be present if NPN was missing. However, NPN is deprecated and we'd like to remove it from cryptography/pyOpenSSL so this test needs to be smarter.

refs ​pyca/cryptography#4765
@reaperhulk
Copy link
Member Author

reaperhulk commented Apr 8, 2019 •
edited
Loading

Damn, the fix I put into twisted's tests doesn't entirely fix the test failures. One left. Dropping it here for later investigation, but this could be a real thing that needs fixing on the twisted side

[ERROR]
Traceback (most recent call last):
  File "/home/travis/.venv/lib/python2.7/site-packages/twisted/protocols/test/test_tls.py", line 1943, in checkNegotiatedProtocol
    self.assertEqual(client.negotiatedProtocol, b'h2')
  File "/home/travis/.venv/lib/python2.7/site-packages/twisted/trial/_synctest.py", line 432, in assertEqual
    super(_Assertions, self).assertEqual(first, second, msg)
  File "/opt/python/2.7.15/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/opt/python/2.7.15/lib/python2.7/unittest/case.py", line 506, in _baseAssertEqual
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: None != 'h2'

twisted.protocols.test.test_tls.TLSMemoryBIOTests.test_disorderlyShutdown

@reaperhulk
Copy link
Member Author

On @glyph's recommendation I changed the test suite in the last push (a month ago) to just run the one test to see if it passed. It did. This is state leakage between tests, so twisted needs to sort that out before we can merge this.

@alex
Copy link
Member

alex commented Jun 3, 2019

Have we filed a bug with twisted for that?

@jefferyto jefferyto mentioned this pull request Jun 4, 2019
Closed
@reaperhulk
Copy link
Member Author

Ping @glyph plz fix state leakage in your testing

@reaperhulk reaperhulk added this to the Twenty Ninth release milestone Sep 7, 2019
@alex
Copy link
Member

alex commented Nov 10, 2019

I filed https://twistedmatrix.com/trac/ticket/9725 for this

reaperhulk added 5 commits July 5, 2020 20:55
@reaperhulk
remove NPN bindings -- you should be using ALPN!
1ac2df0 
pyOpenSSL consumed these, but we've marked it as deprecated and it
already handles the case where the bindings are not available.
@reaperhulk
set Cryptography_HAS_NEXTPROTONEG to 0 for pyOpenSSL
e92d9c5 
we can remove this symbol in like...5 years.
@reaperhulk
remove another NPN related definition
e1777f4
@reaperhulk
suspicious
1f256ba
@reaperhulk
Revert "remove another NPN related definition"
c1352fc 
This reverts commit d872a7d.

Revert "suspicious"

This reverts commit 5b76748.
@alex alex merged commit 99bf4e4 into pyca:master Jul 6, 2020
@reaperhulk reaperhulk deleted the remove-npn branch July 26, 2020 19:54
@jefferyto jefferyto mentioned this pull request Jul 26, 2020
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alex alex alex approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
Thirtieth release
Development

Successfully merging this pull request may close these issues.
2 participants
@reaperhulk @alex