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. #184

Closed
wants to merge 1 commit into from

Conversation

richmoore
Copy link
Contributor

Note this requires pyca/cryptography#1619 to be merged first.

@richmoore
Copy link
Contributor Author

Failure is expected - the travis run was before the cryptography change was merged (it is now).

@exarkun
Copy link
Member

exarkun commented Jan 13, 2015

Thanks. I think this change will also need to include a bump to the declared required cryptography version in setup.py (which is easier after cryptography releases the new feature that's required, but I think you can construct a dev-style version in the interim if you want to get this merged before the next cryptography release).

@reaperhulk
Copy link
Member

We'll be releasing 0.7.2 with the binding required for this feature in the next day or two.

@richmoore richmoore force-pushed the session-tls-version branch from 99398ee to fb73e0b Compare January 17, 2015 10:56
@richmoore
Copy link
Contributor Author

I've updated setup.py in my branch.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling fb73e0b on richmoore:session-tls-version into 014c0cc on pyca:master.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be hard for most people to generalize from "0x303 means TLS 1.2" to an understanding of other values. Or maybe that's just mean. Anyway, can you expand this documentation or make it easier to understand these values some other way? An example of a non-documentation solution might be to make the result self-documenting by defining a collection of symbolic constants and referring to that collection here. Or another solution might be to refer to some existing OpenSSL documentation about this value.

Which brings me to another point, where is the documentation for SSL_version? I can't find any. I did find SSL_get_version which apparently returns a string instead. Exposing that instead might be another way to solve the documentation issue - since "TLS1.2" doesn't need as much explanation as 0x303 (though just going by the OpenSSL documentation, I don't know if "TLSv1.2" is a value that will ever be returned by SSL_get_version! It only documents "SSLv2", "SSLv3", and "TLSv1" - oh, and, awesomely, "unknown").

@exarkun
Copy link
Member

exarkun commented Jan 18, 2015

Thanks. Can you also add documentation to docs/api/ and an entry to ChangeLog?

@exarkun
Copy link
Member

exarkun commented Mar 21, 2015

Hi richmoore. Any interest in following up on this or would it be better if someone else finished it up? Thanks again for your work so far.

@ihamburglar
Copy link
Contributor

I took a shot at finishing this one up. When I switch to using SSL_get_versionI get this

..............................................................................
======================================================================
ERROR: test_get_protocol_version (__main__.ConnectionTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "OpenSSL/test/test_ssl.py", line 2755, in test_get_protocol_version
    server.get_protocol_version(), client.get_protocol_version()
  File "build/bdist.linux-x86_64/egg/OpenSSL/SSL.py", line 1894, in get_protocol_version
    version = _lib.SSL_get_version(self._ssl)
AttributeError: 'FFILibrary' object has no attribute 'SSL_get_version'

----------------------------------------------------------------------
Ran 193 tests in 6.109s

FAILED (errors=1)

When I try to print it from a simple ssl connect I get None as the value. Is this a problem in this not being implemented in cffi?

@alex
Copy link
Member

alex commented Apr 26, 2015

SSL_get_version doesn't appear to be bound in cryptography, adding it
should be a pretty simple PR.

On Sun, Apr 26, 2015 at 12:22 PM, elitest [email protected] wrote:

I took a shot at finishing this one up. When I switch to using
SSL_get_versionI get this

..............................................................................

ERROR: test_get_protocol_version (main.ConnectionTests)

Traceback (most recent call last):
File "OpenSSL/test/test_ssl.py", line 2755, in test_get_protocol_version
server.get_protocol_version(), client.get_protocol_version()
File "build/bdist.linux-x86_64/egg/OpenSSL/SSL.py", line 1894, in get_protocol_version
version = _lib.SSL_get_version(self._ssl)
AttributeError: 'FFILibrary' object has no attribute 'SSL_get_version'


Ran 193 tests in 6.109s

FAILED (errors=1)

When I try to print it from a simple ssl connect I get None as the value.
Is this a problem in this not being implemented in cffi?


Reply to this email directly or view it on GitHub
#184 (comment).

"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

@ihamburglar
Copy link
Contributor

We can probably close? These changes were (effectively) merged in #244

@alex alex closed this May 30, 2015
@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.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants