8000 bpo-35045: Fix test_ssl.test_min_max_version() by vstinner · Pull Request #11508 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member
@vstinner vstinner commented Jan 10, 2019

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
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.
Copy link
Member
@tiran tiran left a comment

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

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
Copy link
Contributor
hroncok commented Jan 10, 2019

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

@tiran
Copy link
Member
tiran commented Jan 10, 2019

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

@hroncok
Copy link
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
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
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
Copy link
Contributor
hroncok commented Jan 15, 2019

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

@vstinner
Copy link
Member Author

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

Oh, I missed the PR #11510. Thanks.

@vstinner
Copy link
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
0