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 querying the negotiated TLS version. The Quickening #244

Merged
merged 18 commits into from May 30, 2015

Conversation

@elitest
Copy link
Contributor

@elitest elitest commented Apr 27, 2015

This PR hopefully finished up the work done by @richmoore in #184. Thanks to @alex for helping me figure out how to get SSL_get_version implemented. This PR requires the merge that happened in pyca/cryptography#1869, so do we wait until the next release of cryptography and then bump the required version number in setup.py?

I'm happy to hear feedback on whether the change to strings requires better/different testing?

Also the OpenSSL docs have been updated since the original PR in January, and this does actually support TLS1.1 and TLS1.2 :)

@@ -598,6 +598,11 @@ Connection objects have the following methods:
but not it returns the entire list in one go.


.. py:method:: Connection.get_protocol_version()
Retrieve the version of the SSL or TLS protocol used by the Connection

This comment has been minimized.

@alex

alex Apr 27, 2015
Member

This should include an example of what the result looks like. Also should end with a . :-)

@coveralls
Copy link

@coveralls coveralls commented Apr 27, 2015

Coverage Status

Coverage decreased (-2.42%) to 92.9% when pulling 676e868 on elitest:session-tls-version into f3fc99e on pyca:master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Apr 27, 2015

Coverage Status

Coverage decreased (-2.42%) to 92.9% when pulling 676e868 on elitest:session-tls-version into f3fc99e on pyca:master.

@coveralls
Copy link

@coveralls coveralls commented Apr 27, 2015

Coverage Status

Coverage decreased (-0.07%) to 95.25% when pulling a7c3acb on elitest:session-tls-version into f3fc99e on pyca:master.

1 similar comment
@coveralls
Copy link

@coveralls coveralls commented Apr 27, 2015

Coverage Status

Coverage decreased (-0.07%) to 95.25% when pulling a7c3acb on elitest:session-tls-version into f3fc99e on pyca:master.

giving the protocol version of the current connection.
"""
server, client = self._loopback()
server_protocol_version, client_protocol_version = \

This comment has been minimized.

@hynek

hynek Apr 27, 2015
Contributor

Please split this into two proper lines. The current version takes the same amount of lines and is less readable.

This comment has been minimized.

@elitest

elitest Apr 27, 2015
Author Contributor

Excellent point and done in 50d7d15.

:rtype: :py:class:`unicode`
"""
version = _ffi.string(_lib.SSL_get_version(self._ssl))
return version.decode("utf-8")

This comment has been minimized.

@hynek

hynek Apr 27, 2015
Contributor

I’m not sure about this one. I believe we decided to keep pyOpenSSL’s APIs bytes-based except for paths.

So unless someone can enlighten me on what I’m missing, I’m gonna claim this ought to be bytes.

This comment has been minimized.

@alex

alex Apr 27, 2015
Member

Dunno, the name of the protocol version definitely seems textual to me.

On Mon, Apr 27, 2015 at 10:16 AM, Hynek Schlawack notifications@github.com
wrote:

In OpenSSL/SSL.py
#244 (comment):

@@ -1883,6 +1883,18 @@ def get_cipher_version(self):
return version.decode("utf-8")

  • def get_protocol_version(self):
  •    """
    
  •    Obtain the protocol version of the current connection.
    
  •    :returns: The TLS version of the current connection, for example
    
  •        the value for TLS 1.2 would be 0x303.
    
  •    :rtype: :py:class:`unicode`
    
  •    """
    
  •    version = _ffi.string(_lib.SSL_get_version(self._ssl))
    
  •    return version.decode("utf-8")
    

I’m not sure about this one. I believe we decided to keep pyOpenSSL’s APIs
bytes-based except for paths.

So unless someone can enlighten me on what I’m missing, I’m gonna claim
this ought to be bytes.


Reply to this email directly or view it on GitHub
https://github.com/pyca/pyopenssl/pull/244/files#r29149506.

"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: 125F 5C67 DFE9 4084

This comment has been minimized.

@elitest

elitest Apr 27, 2015
Author Contributor

If I understand your argument correctly, you are saying that when you query for TLS version you should receive something like 0x769 as a response? The whole python3 bytes v. unicode thing is still new to me... What was the thinking behind trying to stick with bytes?

Plenty of things in SSL.py are unicode strings though, including much of the related cipher information. Should a developer really have to get out their OpenSSL decoder ring everytime she wants to query a cipher? I think TLS_RSA_WITH_AES_256_CBC_SHA256 makes a lot more sense than 0x3d, as long as the upstream doesn't change the formatting of those strings.

This comment has been minimized.

@hynek

hynek Apr 28, 2015
Contributor

For the first paragraph; what I’m saying is that the docstring is plain wrong. There’s no way you get anything resembling “0x303” back. You get a string like "TLSv1" back. Try it and pdb in your test. :)

This comment has been minimized.

@elitest

elitest Apr 28, 2015
Author Contributor

My apologies, I should have looked at the code you were commenting on more closely! I thought I had updated it from the way it was initially proposed in #184. Fixed in 6ac2e2a.

@coveralls
Copy link

@coveralls coveralls commented Apr 27, 2015

Coverage Status

Coverage decreased (-0.09%) to 95.23% when pulling 50d7d15 on elitest:session-tls-version into f3fc99e on pyca:master.

@coveralls
Copy link

@coveralls coveralls commented Apr 27, 2015

Coverage Status

Coverage decreased (-0.2%) to 95.098% when pulling 50d7d15 on elitest:session-tls-version into f3fc99e on pyca:master.

@coveralls
Copy link

@coveralls coveralls commented Apr 28, 2015

Coverage Status

Coverage decreased (-2.43%) to 92.88% when pulling 6ac2e2a on elitest:session-tls-version into f3fc99e on pyca:master.

@coveralls
Copy link

@coveralls coveralls commented Apr 28, 2015

Coverage Status

Coverage decreased (-0.08%) to 95.23% when pulling 6ac2e2a on elitest:session-tls-version into f3fc99e on pyca:master.

@elitest
Copy link
Contributor Author

@elitest elitest commented May 4, 2015

When does Travis update the version of cryptography it tests against? Based on the way I read the .travis.yml, Travis is supposed to pull from cryptography's master branch? If that is the case why would the test fail?

@hynek
Copy link
Contributor

@hynek hynek commented May 4, 2015

Good question. @reaperhulk any ideas?

@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented May 4, 2015

When we switched to using tox in travis we didn't change it over to using the master branch properly, so it's installing it and then not using it.

@elitest
Copy link
Contributor Author

@elitest elitest commented May 4, 2015

Is that something we want to fix? If I understand .travis.yml#L71-83 properly the problem is that we aren't installing cryptography's master into the virtualenv? So when it installs pyOpenSSL into the virtualenv via setup.py it installs the latest released cryptography from PyPI?

@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented May 4, 2015

Yeah that needs to be fixed. Right now it installs it into a venv, but then invokes tox (which builds its own venv and doesn't use the master branch).

@hynek
Copy link
Contributor

@hynek hynek commented May 5, 2015

Please rebase on master; this issue should be fixed now.

@hynek
Copy link
Contributor

@hynek hynek commented May 5, 2015

Also, until cryptography 0.9 is out, we can bikeshed on the return type.

My impression is, that pyOpenSSL is bytes only. If you’d like to argue for unicode I would like to ask you for precedents.

pyOpenSSL is supposed to by a low-level layer around OpenSSL so I’m very apprehensive to introduce unicode-based APIs. We’ve just added a ton of warning if certain methods are called with unicode.

@elitest
Copy link
Contributor Author

@elitest elitest commented May 5, 2015

@hynek have you had a chance to look at the original PR? Would you prefer that instead?

@hynek
Copy link
Contributor

@hynek hynek commented May 6, 2015

Oi, that's where the incorrect doc string came from. :)

That's not what I meant (I just want to get rid of the decode call) but we should definitely expose both ways. I'm on my phone for the next hours but I'll look into it when I'm back home.

It'll be a different PR though. so don't worry about it here.

@elitest
Copy link
Contributor Author

@elitest elitest commented May 13, 2015

So I might have some time to work on this during the coming weekend in. Just to be clear, when you say we should expose both ways you mean exposing both SSL_get_version(a.k.a. "TLV1.2" in bytes) and SSL_version(a.k.a. 0x303)? In #184 it was discussed how to make it clear what is being returned to you, in the documentation, do you have any suggestions there?

@hynek
Copy link
Contributor

@hynek hynek commented May 13, 2015

So I’m saying we should expose both but we start with the string-based version. And I’m also saying it should return bytes (not unicode) unless you can make a good case for unicode based on existing APIs. :)

@hynek
Copy link
Contributor

@hynek hynek commented May 26, 2015

Can you rebase please?

@elitest elitest force-pushed the elitest:session-tls-version branch from 6ac2e2a to 6a9f27b May 27, 2015
@elitest
Copy link
Contributor Author

@elitest elitest commented May 27, 2015

WIP
I'm still having issues wrapping my mind around the whole bytes thing... So tests currently fail. Right now I'm getting stuff like:

AssertionError: <cdata 'char *' 0x7fbe700c6c96> is not an instance of <class 'bytes'>

do I need to wrap the result that I'm returning in bytes() or something?

@reaperhulk
Copy link
Member

@reaperhulk reaperhulk commented May 27, 2015

SSL_get_version returns a const char *. cffi represents that as a proxy object. If you want to obtain the value you'll need to use ffi.buffer() or ffi.string(). The former requires you either know the length of the char * or own all the bytes while the latter reads until it encounters a null (as it assumes a null terminated string). In this case ffi.string is the appropriate method

@elitest
Copy link
Contributor Author

@elitest elitest commented May 27, 2015

That is what I had in whatever 41ac0c4 was before the rebase. Are we saying that we just don't decode it to utf-8 when we return?

@elitest elitest force-pushed the elitest:session-tls-version branch from 6a9f27b to 15758b6 May 27, 2015
@elitest
Copy link
Contributor Author

@elitest elitest commented May 27, 2015

Tests should now pass.

Retrieve the version of the SSL or TLS protocol used by the Connection.
For example, it will return ``0x303`` for connections made over TLS
version 1.2, or ``Unknown`` for connections that were not successfully

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

that seems unlikely :)

This comment has been minimized.

@elitest

elitest May 28, 2015
Author Contributor

removed unknown in 208438c

:returns: The TLS version of the current connection, for example
the value for TLS 1.2 would be ``b'TLSv1.2'``.
:rtype: :py:class:`unicode`

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

I believe that’s not true anymore?

This comment has been minimized.

@elitest

elitest May 28, 2015
Author Contributor

208438c now bytes

@@ -1938,6 +1950,17 @@ def get_alpn_proto_negotiated(self):
return _ffi.buffer(data[0], data_len[0])[:]


def get_protocol_version(self):

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

please group those two methods together

This comment has been minimized.

@elitest

elitest May 28, 2015
Author Contributor

moved in 208438c

"""
version = _lib.SSL_version(self._ssl)
return version

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

this needs three empty lines

This comment has been minimized.

@elitest

elitest May 28, 2015
Author Contributor

there are three lines at the bottom, it has been moved 208438c

ChangeLog Outdated
* OpenSSL/SSL.py, : Add ``get_protocol_version()`` and
``get_protocol_version_name()`` to Connection
Based on work from Rich Moore
* OpenSSL/test/test_crypto.py: tests for ``get_protocol_version()``

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

just add OpenSSL/test/test_crypto.py to the list above. it’s implicit that we add tests for new code. :)

This comment has been minimized.

@elitest

elitest May 28, 2015
Author Contributor

ChangeLog Outdated
2015-05-27 Jim Shaver <dcypherd@gmail.com>

* OpenSSL/SSL.py, : Add ``get_protocol_version()`` and
``get_protocol_version_name()`` to Connection

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

I’d like Connection to be in `` too. Also please add some periods after sentences. :)

This comment has been minimized.

@elitest

elitest May 28, 2015
Author Contributor

@hynek
Copy link
Contributor

@hynek hynek commented May 28, 2015

I didn’t have opinions as much as I’m trying to figure out what the correct way to do is. :) As far as I can see, bytes is the way to go but if anyone can show me similar APIs that take/return unicode, I’ll change my mind.

@hynek hynek added this to the 0.16 milestone May 28, 2015
@elitest
Copy link
Contributor Author

@elitest elitest commented May 28, 2015

I'm fine either way. There is some Cipher stuff that is returned in unicode for example. But if a discussion has happened around it and that is the direction we are taking additions I'm fine with that.

@coveralls
Copy link

@coveralls coveralls commented May 28, 2015

Coverage Status

Coverage increased (+0.02%) to 95.05% when pulling 208438c on elitest:session-tls-version into 51dc335 on pyca:master.

@hynek
Copy link
Contributor

@hynek hynek commented May 28, 2015

Ugghhh, and those methods are actually pretty analogue to these ones. :-/ Same for Curve.name

sigh

Unicode it is then. Sorry.

@elitest
Copy link
Contributor Author

@elitest elitest commented May 28, 2015

LOL! It is my own fault I should have pointed it out earlier.

@coveralls
Copy link

@coveralls coveralls commented May 28, 2015

Coverage Status

Coverage increased (+0.02%) to 95.05% when pulling 58d2573 on elitest:session-tls-version into 51dc335 on pyca:master.

:returns: The TLS version of the current connection, for example
the value for TLS 1.2 would be ``TLSv1.2``or ``Unknown``
for connections that were not successfully.

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

successfully what?


.. py:method:: Connection.get_protocol_version_name()
Retrieve the version of the SSL or TLS protocol used by the Connection.

This comment has been minimized.

@hynek

hynek May 28, 2015
Contributor

“…as an unicode string.” would be nice to differentiate it from the other method, no?

@coveralls
Copy link

@coveralls coveralls commented May 28, 2015

Coverage Status

Coverage increased (+0.02%) to 95.05% when pulling b5b6b0e on elitest:session-tls-version into 51dc335 on pyca:master.

@coveralls
Copy link

@coveralls coveralls commented May 29, 2015

Coverage Status

Coverage increased (+0.02%) to 95.05% when pulling 46f2891 on elitest:session-tls-version into 51dc335 on pyca:master.

hynek added a commit that referenced this pull request May 30, 2015
Add support for querying the negotiated TLS version
@hynek hynek merged commit b92c8a9 into pyca:master May 30, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek
Copy link
Contributor

@hynek hynek commented May 30, 2015

Thank you for your work and your patience!

@elitest
Copy link
Contributor Author

@elitest elitest commented May 30, 2015

Thanks for your guidance!

@elitest elitest deleted the elitest:session-tls-version branch May 30, 2015
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Apr 20, 2016
Changes:
16.0.0 (2016-03-19)
-------------------
This is the first release under full stewardship of PyCA.
We have made *many* changes to make local development more pleasing.
The test suite now passes both on Linux and OS X with OpenSSL 0.9.8,
1.0.1, and 1.0.2.  It has been moved to `py.test <https://pytest.org/>`_,
all CI test runs are part of `tox <https://testrun.org/tox/>`_ and
the source code has been made fully `flake8
<https://flake8.readthedocs.org/>`_ compliant.

We hope to have lowered the barrier for contributions significantly
but are open to hear about any remaining frustrations.

Backward-incompatible changes:
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Python 3.2 support has been dropped.
  It never had significant real world usage and has been dropped
  by our main dependency ``cryptography``.  Affected users should
  upgrade to Python 3.3 or later.

Deprecations:
^^^^^^^^^^^^^
- The support for EGD has been removed.
  The only affected function ``OpenSSL.rand.egd()`` now uses
  ``os.urandom()`` to seed the internal PRNG instead.  Please see
  `pyca/cryptography#1636
  <https://github.com/pyca/cryptography/pull/1636>`_ for more
  background information on this decision.  In accordance with our
  backward compatibility policy ``OpenSSL.rand.egd()`` will be
  *removed* no sooner than a year from the release of 16.0.0.
  Please note that you should `use urandom
  <http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/>`_
  for all your secure random number needs.
- Python 2.6 support has been deprecated.
  Our main dependency ``cryptography`` deprecated 2.6 in version
  0.9 (2015-05-14) with no time table for actually dropping it.
  pyOpenSSL will drop Python 2.6 support once ``cryptography``
  does.

Changes:
^^^^^^^^
- Fixed ``OpenSSL.SSL.Context.set_session_id``,
  ``OpenSSL.SSL.Connection.renegotiate``,
  ``OpenSSL.SSL.Connection.renegotiate_pending``, and
  ``OpenSSL.SSL.Context.load_client_ca``.
  They were lacking an implementation since 0.14.  `#422
  <https://github.com/pyca/pyopenssl/pull/422>`_
- Fixed segmentation fault when using keys larger than 4096-bit to sign data.
  `#428 <https://github.com/pyca/pyopenssl/pull/428>`_
- Fixed ``AttributeError`` when ``OpenSSL.SSL.Connection.get_app_data()``
  was called before setting any app data.
  `#304 <https://github.com/pyca/pyopenssl/pull/304>`_
- Added ``OpenSSL.crypto.dump_publickey()`` to dump ``OpenSSL.crypto.PKey``
  objects that represent public keys, and ``OpenSSL.crypto.load_publickey()``
  to load such objects from serialized representations.
  `#382 <https://github.com/pyca/pyopenssl/pull/382>`_
- Added ``OpenSSL.crypto.dump_crl()`` to dump a certificate revocation
  list out to a string buffer.
  `#368 <https://github.com/pyca/pyopenssl/pull/368>`_
- Added ``OpenSSL.SSL.Connection.get_state_string()`` using the
  OpenSSL binding ``state_string_long``.
  `#358 <https://github.com/pyca/pyopenssl/pull/358>`_
- Added support for the ``socket.MSG_PEEK`` flag to
  ``OpenSSL.SSL.Connection.recv()`` and
  ``OpenSSL.SSL.Connection.recv_into()``.
  `#294 <https://github.com/pyca/pyopenssl/pull/294>`_
- Added ``OpenSSL.SSL.Connection.get_protocol_version()`` and
  ``OpenSSL.SSL.Connection.get_protocol_version_name()``.
  `#244 <https://github.com/pyca/pyopenssl/pull/244>`_
- Switched to ``utf8string`` mask by default.
  OpenSSL formerly defaulted to a ``T61String`` if there were UTF-8
  characters present.  This was changed to default to ``UTF8String``
  in the config around 2005, but the actual code didn't change it
  until late last year.  This will default us to the setting that
  actually works.  To revert this you can call
  ``OpenSSL.crypto._lib.ASN1_STRING_set_default_mask_asc(b"default")``.
  `#234 <https://github.com/pyca/pyopenssl/pull/234>`_
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.