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 password-protected client keyfiles #1489

Merged
merged 8 commits into from
Jan 22, 2019

Conversation

sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Dec 1, 2018

The standard library supports passing text or binary passwords to SSLContext so we should also support that for PyOpenSSLContext/SecureTransportContext objects. This PR adds support for that and tests for all situations where this functionality is supported.

Currently the only way we support this is via creating your own SSLContext and calling load_cert_chain() yourself. Adding the key_password parameter to HTTPSConnectionPool allows this to be defined from the PoolManager level without having to create your own SSLContext.

Another problem that I encountered first-hand is that if OpenSSL encounters an encrypted keyfile but the password parameter is not given:

If the password argument is not specified and a password is required, OpenSSL’s built-in password prompting mechanism will be used to interactively prompt the user for a password.

which basically means if you try using an encrypted keyfile without a password your program just straight up blocks which breaks a lot of things in non-interactive contexts. (PyCharm locks up irreversibly, pytest appears to be blocking indefinitely due to swallowing stdout). I was wondering if there's a way for us to stop this bad behavior on our side and raise an exception instead? Besides detecting an encrypted keyfile within ssl_wrap_socket I don't know what else we can do here. :(

Closes #1275.

@codecov-io
Copy link

codecov-io commented Dec 1, 2018

Codecov Report

Merging #1489 into master will decrease coverage by 2%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1489      +/-   ##
==========================================
- Coverage   66.84%   64.84%   -2.01%     
==========================================
  Files          22       22              
  Lines        2760     2904     +144     
==========================================
+ Hits         1845     1883      +38     
- Misses        915     1021     +106
Impacted Files Coverage Δ
src/urllib3/poolmanager.py 61.39% <ø> (-15.93%) ⬇️
src/urllib3/connectionpool.py 59.72% <100%> (-2.75%) ⬇️
src/urllib3/connection.py 54.19% <100%> (-1.55%) ⬇️
src/urllib3/util/ssl_.py 35.8% <57.14%> (-0.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f80ff34...6756504. Read the comment docs.

@sethmlarson
Copy link
Member Author

The commit I just pushed implements simple logic to detect an encrypted key file and raise a helpful error message before OpenSSL has a chance to block indefinitely. If there's other ideas or we decide against landing that commit I can roll it back.

@sigmavirus24
Copy link
Contributor

cc @alex

Copy link
Member

@shazow shazow left a comment

Choose a reason for hiding this comment

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

Looks sensible to me.

I'm not super familiar with the latest developments of the pyopenssl/SSLContext APIs so I can't speak to whether there's a better way to do this.

Soft 👍

@alex
Copy link
Contributor

alex commented Jan 22, 2019

Quick skim looks fine to me.

@sethmlarson sethmlarson merged commit 791e9b4 into urllib3:master Jan 22, 2019
@sethmlarson sethmlarson deleted the load-cert-chain-password branch January 22, 2019 13:17
@sethmlarson
Copy link
Member Author

Thanks all!


def _is_key_file_encrypted(key_file):
"""Detects if a key file is encrypted or not."""
with open(key_file, 'r') as f:

Choose a reason for hiding this comment

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

@sethmlarson There would not be a way to put this in a try...except or something? We want to avoid files or temporary files and we put a special HTTPAdapter to pass the contents immediately instead of the files, but then it tracebacks at this point. Ideally, there would be support for passing the data immediately instead of opening files.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jco-odoo Can you open a separate issue for this?

Choose a reason for hiding this comment

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

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants