Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-35045: Fix test_ssl.test_min_max_version()#11508

Closed
vstinner wants to merge 1 commit into
python:masterpython/cpython:masterfrom
vstinner:test_ssl_min_verCopy head branch name to clipboard
Closed

bpo-35045: Fix test_ssl.test_min_max_version()#11508
vstinner wants to merge 1 commit into
python:masterpython/cpython:masterfrom
vstinner:test_ssl_min_verCopy head branch name to clipboard

Conversation

@vstinner

@vstinner vstinner commented Jan 10, 2019

Copy link
Copy Markdown
Member

test_ssl.test_min_max_version() no longer tests the default
minimum_version: it depends on the OpenSSL configuration, it is not
always equal to TLSVersion.MINIMUM_SUPPORTED.

https://bugs.python.org/issue35045

@vstinner

Copy link
Copy Markdown
Member Author

cc @stratakis

test_ssl.test_min_max_version() no longer tests the default
minimum_version: it depends on the OpenSSL configuration, it is not
always equal to TLSVersion.MINIMUM_SUPPORTED.

@tiran tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1

Fedora's crypto policy modifies the settings. You have to disable the crypto policy for your test session.

@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

@hroncok

hroncok commented Jan 10, 2019

Copy link
Copy Markdown
Contributor

can we change the environment variable as part of that test instead?

@tiran

tiran commented Jan 10, 2019

Copy link
Copy Markdown
Member

Yes, that's my plan. I'm working on a PR right now.

@hroncok

hroncok commented Jan 10, 2019

Copy link
Copy Markdown
Contributor
script = '''
import ssl
ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
print(ctx.minimum_version)
'''

proc = subprocess.run([sys.executable, '-c', script],
                      capture_output=True,
                      text=True,
                      check=True,
                      env={**os.environ, 'OPENSSL_CONF': '/non-existing-file'})
assert proc.stdout.strip() == 'TLSVersion.MINIMUM_SUPPORTED'

@vstinner

Copy link
Copy Markdown
Member Author

11 lines of code just to test the default value of an OpenSSL constant, is it really worth it? Well, I rely on @tiran for ssl changes :-)

@vstinner

Copy link
Copy Markdown
Member Author

Yes, that's my plan. I'm working on a PR right now.

If nobody comes with a better fix for this test on Fedora, I will merge this change at the end of the week.

Note: Even if I merge my change, it i will be trivial to revert my change later for a better solution ;-)

@hroncok

hroncok commented Jan 15, 2019

Copy link
Copy Markdown
Contributor

@vstinner IIRC there is another PR open by @tiran for this.

@vstinner

Copy link
Copy Markdown
Member Author

@vstinner IIRC there is another PR open by @tiran for this.

Oh, I missed the PR #11510. Thanks.

@vstinner

Copy link
Copy Markdown
Member Author

I abandon my PR in favor of PR #11510 which is a better fix.

@vstinner vstinner closed this Jan 15, 2019
@vstinner vstinner deleted the test_ssl_min_ver branch January 15, 2019 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting changes tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.