Fix manual prompt in pyopenssl adapter for private key password #798
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #798 +/- ##
==========================================
+ Coverage 78.01% 78.18% +0.16%
==========================================
Files 41 41
Lines 4749 4786 +37
Branches 542 547 +5
==========================================
+ Hits 3705 3742 +37
+ Misses 905 904 -1
- Partials 139 140 +1 |
|
@jatalahd may I ask why does your checklist and up having |
cheroot/ssl/pyopenssl.py
Outdated
| if self.private_key_password is not None: | ||
| c.set_passwd_cb(self._password_callback, self.private_key_password) |
There was a problem hiding this comment.
Was #752 (comment) incorrect? Why? How?
Can this be tested? Could you add a change log fragment?
There was a problem hiding this comment.
Why don't we have _password_callback() raise RuntimeError if password is None? If we don't set the callback, passing an encrypted private key will probably traceback in some weird place. It seems more reasonable to have our own error reporting for this case.
There was a problem hiding this comment.
The comment you are referring to was correct in that context and the code works in that scenario. The current fix is for a scenario where the private key is encrypted, but the password is not provided via private_key_password argument. In that case when starting the server, it should ask the user to input the password manually, but in this case, as defined, the set_passwd_callback() is called every time it encounters an encrypted private key. If the password is not given as argument it returns the empty string and fails due to password mismatch. That is why the callback should not be called if there is no password given in arguments.
This cannot be tested here in cheroot side, but I am assuming that it could be tested in cherrypy side when starting the server in a subprocess and communicating the user input to the process.
There was a problem hiding this comment.
Before we proceed, could you fact-check that it actually works as the doc describes? cherrypy/cherrypy#1583 (comment).
There was a problem hiding this comment.
The password prompt arises from the underlying ssl implementation and is unaffected by cherrypy/cheroot code. The official documentation for the ssl library (used in the builtin adapter) for the method SSLContext.load_cert_chain() says (reference: https://docs.python.org/3/library/ssl.html#ssl-contexts)
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.
As the pyOpenSSL is only a wrapper on top of OpenSSL, it behaves the same way. This can be verified by making an OpenSSL commandline operation on an encrypted private key: the result is Enter PEM pass phrase: command line prompt.
I had tested both adapters working this way from cherrypy (and actually using bottle app, along with cheroot to provide the https layer with the adapters) before starting my initial commit. That is why I originally had the if in the initial commit, but forgot about its meaning during the long process of finetuning the code through the review process.
There was a problem hiding this comment.
Alright. Thanks for clearing that up!
Seeing how this confused me (since I never used it like this myself), I think it's a reasonable approach and it deserves a code comment. Could you add one? We need something that would stop people from trying to refactor this seemingly unnecessary conditional at some point in the future — a sufficient justification explanation. Such breakcrumbs are important — I've been looking into the past two decades of history of this module just the other day and trying to understand people's motivation is challenging sometimes.
There was a problem hiding this comment.
Added code comment in latest push.
There was a problem hiding this comment.
@jatalahd what if instead we put a getpass.getpass() into the callback and have more control over retrieving said password? I thought we'd do this when it was first implemented but forgot to ask then. And now I'm remembering this idea. WDYT?
There was a problem hiding this comment.
We could tie this into the idea I suggested @ cherrypy/cherrypy#2074 (comment). Something like this would do:
from getpass import getpass as _ask_for_password_interactively
...
def _prompt_for_tls_password(prompt_prefix: str = '', /) -> str:
password_prompt = (
f'{prompt_prefix}Enter the password for deciphering '
'the TLS private key PEM file: '
)
return _ask_for_password_interactively(password_prompt)
...
class pyOpenSSLAdapter(Adapter):
...
def _password_callback(
self,
password_max_length,
verify_twice,
password_or_callback,
/,
):
...
if callable(password_or_callback):
password = password_or_callback()
if verify_twice and password != password_or_callback('Once again. '):
raise ValueError(
'Verification failed: entered passwords do not match',
) from None
else:
password = password_or_callback
...
...
def get_context(self) -> SSL.Context:
...
if self.private_key_password is None:
self.private_key_password = _prompt_for_tls_password
c.set_passwd_cb(self._password_callback, self.private_key_password)
...There was a problem hiding this comment.
Yes, good idea, but now you forced me to push the builtin adapter to accept Callable type. So this simple one liner will most likely turn into many weeks of pruning :)
Check out the latest push.
jatalahd
force-pushed
the
fix_openssl_private_key_none_passwd
branch
from
339ca88 to
bb640b0
Compare
I truly do not know. I have to pay attention to this next time. |
|
@webknjaz ; added change log fragment and some answers to your questions. Added also explanation to the main umbrella issue: |
Let me know. I've seen this happening when other people fill out the form but never learned what the cause is. |
jatalahd
force-pushed
the
fix_openssl_private_key_none_passwd
branch
from
bb640b0 to
3f6f038
Compare
jatalahd
force-pushed
the
fix_openssl_private_key_none_passwd
branch
from
3f6f038 to
20885c6
Compare
|
@webknjaz ; In the latest push I have made both adapters to accept callable type for the |
Cool. I was hoping it'd help like that. With the comments I left inline, I think the builtin adapter should be able to mirror the same idea. |
jatalahd
force-pushed
the
fix_openssl_private_key_none_passwd
branch
from
20885c6 to
afc39cd
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@webknjaz ; Made some progress in the latest push and overall things start to line up. I am not sure if the password prompting callback should be in |
cheroot/ssl/__init__.py
Outdated
|
|
||
| def prompt_for_tls_password(self) -> str: | ||
| """Define interactive prompt for encrypted private key password.""" | ||
| prompt = 'Enter PEM pass phrase:' |
There was a problem hiding this comment.
A whitespace is missing here:
| prompt = 'Enter PEM pass phrase:' | |
| prompt = 'Enter PEM pass phrase: ' |
Usually, prompts have it there so that the cursor wouldn't look weirdly misplaced.
I originally suggested a different signature: #798 (comment) — did you have a comment on why you decided to implement it differently?
There was a problem hiding this comment.
I did not comment on that previously. I just don't see the reason to use anything else than the default prompt the internal ssl libraries are using. If you really want it to be different, we can still change the prompt.
There was a problem hiding this comment.
No, it's okay. I just wanted to make sure it's not being missed.
There was a problem hiding this comment.
No, it's okay. I just wanted to make sure it's not being missed.
Good call.
Yep, I feel the same. P.S. Rebase the PR to absorb the fixes on |
|
@jatalahd you'll need fix a conflict when rebasing. |
jatalahd
force-pushed
the
fix_openssl_private_key_none_passwd
branch
from
afc39cd to
529dd97
Compare
|
@webknjaz ; Latest push is closer towards the goal, but I did not manage to resolve all your comments. Let's continue from here. |
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
Just two small test improvements left.
- If pyopenssl adapter was used with password protected private key, the manual entry option was not given, only a fail due to invalid password. The password callback was triggered also in the case where the private_key_password was None. - Added Callable type as possible private_key_password argument
jatalahd
force-pushed
the
fix_openssl_private_key_none_passwd
branch
from
529dd97 to
2692451
Compare
| when the password in not set in the :py:attr:`private_key_password attribute | ||
| <cheroot.ssl.pyopenssl.pyOpenSSLAdapter.private_key_password>` in the | ||
| :py:class:`pyOpenSSL TLS adapter <cheroot.ssl.pyopenssl.pyOpenSSLAdapter>`. | ||
| Also improved the private key password to accept the :py:class:`~typing.Callable` type. |
There was a problem hiding this comment.
Just realized that typing.Callable is deprecated and moved to a new location:
| Also improved the private key password to accept the :py:class:`~typing.Callable` type. | |
| Also improved the private key password to accept the | |
| :py:class:`~collections.abc.Callable` type. |
There was a problem hiding this comment.
Looks good (short of the callable link): https://cheroot--798.org.readthedocs.build/en/798/history/#to-be-included-in-the-next-release
| raise NotImplementedError # pragma: no cover | ||
|
|
||
| def _prompt_for_tls_password(self) -> str: | ||
| """Define interactive prompt for encrypted private key password.""" |
There was a problem hiding this comment.
(because this does actual prompting)
| """Define interactive prompt for encrypted private key password.""" | |
| """Prompt for encrypted private key password interactively.""" |
| ciphers: _t.Any | None = ..., | ||
| *, | ||
| private_key_password: str | bytes | None = ..., | ||
| private_key_password: _t.Callable[[], bytes | str] |
There was a problem hiding this comment.
Add import collections.abc as _c at the top and use it instead of the deprecated type:
| private_key_password: _t.Callable[[], bytes | str] | |
| private_key_password: _c.Callable[[], bytes | str] |
| ciphers: _t.Any | None = ..., | ||
| *, | ||
| private_key_password: str | bytes | None = ..., | ||
| private_key_password: _t.Callable[[], bytes | str] |
There was a problem hiding this comment.
Add import collections.abc as _c at the top and use it instead of the deprecated type:
| private_key_password: _t.Callable[[], bytes | str] | |
| private_key_password: _c.Callable[[], bytes | str] |
| _verify_twice: bool, | ||
| password: bytes | str | None, | ||
| verify_twice: bool, | ||
| password_or_callback: _t.Callable[[], bytes | str] |
There was a problem hiding this comment.
Add import collections.abc as _c at the top and use it instead of the deprecated type:
| password_or_callback: _t.Callable[[], bytes | str] | |
| password_or_callback: _c.Callable[[], bytes | str] |
|
A few fixes left are mostly cosmetic. |
2 participants
If
pyopenssladapter is used with password protected private key and the manual entry option was not given, only a fail due to invalid password. The password callback used to also be triggered in the case where theprivate_key_passwordwasNone.❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)Resolves #
❓ What is the current behavior? (You can also link to an open issue here)
Manual prompt to enter private key password is not given in server startup
in the case where private_key_password = None. Functionality conflicts with
documentation and with builtin adapter
❓ What is the new behavior (if this is a feature change)?
With this fix the functionality is matching the documentation
and both ssl adapters work in similar fashion.
📋 Other information:
📋 Contribution checklist:
(If you're a first-timer, check out
this guide on making great pull requests)
the changes have been approved
and description in grammatically correct, complete sentences