Add private key password option to ssl adapters #752
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #752 +/- ##
==========================================
+ Coverage 79.36% 79.62% +0.26%
==========================================
Files 29 29
Lines 4201 4275 +74
Branches 538 544 +6
==========================================
+ Hits 3334 3404 +70
- Misses 723 727 +4
Partials 144 144 |
|
Fixed linter issues with second commit, but now it seems that I need to do the git squash... Anyway, it would be important for us to get this improvement into some upcoming cheroot release, which we can then reference in our pipfile so that we can take these changes into use. |
|
@webknjaz ; I understand that you are busy, but I am pinging you because you seem to be the only administrator actively working on this repository. I am hoping that we could proceed with this pull request until I forget what have I done. I assume at least the review is required, but maybe some other tasks need to be still done until this can be merged? |
|
@jatalahd thanks for the contrib! You don't have to squash commits unconditionally. Only if the commits are non-atomic. If they aren't, it's usually a good idea to combine them to keep Git history clean. One thing that's definitely missing is a change note. Read https://cheroot.cherrypy.dev/en/latest/contributing/guidelines/#adding-change-notes-with-your-prs and follow the guidelines. Do you best and if it needs editing, I'll tell you. I can't give you a timeline right now. I'm a bit unhappy about the state of the TLS adapters in general and wanted to redesign them eventually. And so I'm a bit hesitant on what changes would be acceptable in the public API. I'll need to think about it first. I'll leave a few notes in the diff but that'll be an incomplete review. |
jatalahd
force-pushed
the
add_private_key_password
branch
from
8bf36ee to
ebf6cf1
Compare
|
@webknjaz ; Thanks for reviewing the code. I fixed all problems you mentioned (as well as I could). Added also change note and had to do the git squash to clean up the git history. Due to that, I had to force push and therefore your review comments disappeared from the "files changed" -tab. However I answered all those comments in this conversation-tab. Please let me know if there is something that still needs fixing. |
jatalahd
force-pushed
the
add_private_key_password
branch
3 times, most recently
from
95bc21c to
6d4f3d2
Compare
|
With the latest two pushes, I fixed one linter issue and one test fail that was occuring on Ubuntu arm platform. To me the CI pipeline results look fine, the remaining test failures are not due to my commit. I consider this now ready for hopefully final review. |
|
@webknjaz ; I am still hoping that this improvement will be included in some upcoming cheroot release. We are currently using a workaround of creating a local wheel of cheroot with this change included and that cannot be a sustainable solution in long term. |
|
@jatalahd I've added a few comments + you need to rebase to pick up the recent |
- It is now possible to use password protected private keys in both builtin and openssl ssl-adapters - Added also positive and negative unit test cases - With reference to #1583
jatalahd
force-pushed
the
add_private_key_password
branch
from
6d4f3d2 to
ae8e1bc
Compare
|
@webknjaz ; Thank you for review and feedback. I have made fixes as you suggested with my best effort. Made a few additions to unit tests as well. The code is rebased on top of main with the fixes included and force pushed for (hopefully) final review. |
|
|
||
| if len(b_password) > password_max_length: | ||
| _warn( | ||
| f'User-provided password is {len(b_password)} bytes long and will ' |
There was a problem hiding this comment.
I'd rather not have length computed twice. Also, complex expressions reduce readability. Could you move this to a var?
| f'User-provided password is {len(b_password)} bytes long and will ' | ||
| f'be truncated since it exceeds the maximum of {password_max_length}.', | ||
| UserWarning, | ||
| stacklevel=2, |
There was a problem hiding this comment.
Do you have an example of where this is pointing to on the stack? Any output? Trace?
| 'builtin', | ||
| 'pyopenssl', | ||
| ) | ||
| * 2, |
There was a problem hiding this comment.
What are you trying to achieve with this? Double test runs? This seems wrong.
| keyfile_temp_path.write_bytes(encrypted_key_as_bytes) | ||
|
|
||
| yield keyfile_temp_path |
There was a problem hiding this comment.
This should probably be under with so that the .tempile() CM would delete it by itlsef. OTOH, do we actually need to call .tempfile()? what are you after? I'd rather make a key next to the original one, in the same dir.
| encrypted_key_as_bytes = private_key_object.private_bytes( | ||
| encoding=Encoding.PEM, | ||
| format=PrivateFormat.PKCS8, | ||
| encryption_algorithm=BestAvailableEncryption( |
There was a problem hiding this comment.
Instead of best encryption, it's better to use the worst. Less compute would contributed to tests running faster.
| [ | ||
| private_key_password, | ||
| private_key_password.encode('utf-8'), | ||
| ], |
There was a problem hiding this comment.
Going for a random password will make the test inconsistent. We should check the predictable thing. Instead, you can parametrize the test to run against one variant at a time.
| return httpserver | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
There's no reason to test against multiple passwords. So this should run just once:
| @pytest.fixture | |
| @pytest.fixture(scope='session') |
OTOH, we could probably just hardcode a silly easter egg value and go with it.
| """Provide random 10 character password for private key.""" | ||
| return ''.join( | ||
| secrets.choice(string.ascii_letters + string.digits) for _ in range(10) | ||
| ) |
There was a problem hiding this comment.
Let's just keep it simple
| """Provide random 10 character password for private key.""" | |
| return ''.join( | |
| secrets.choice(string.ascii_letters + string.digits) for _ in range(10) | |
| ) | |
| """Return a password for private key.""" | |
| return 'криївка' |
| 'pyopenssl', | ||
| ), | ||
| ) | ||
| def test_ssl_adapters_key_without_password( |
There was a problem hiding this comment.
Why not combine this with the previous function through parametrize?
| httpserver.ssl_adapter = tls_adapter | ||
|
|
||
| with pytest.raises(OpenSSL.SSL.Error, match=r'.+bad decrypt.+'): | ||
| httpserver.prepare() |
There was a problem hiding this comment.
Why does pyOpenSSL fail in a different place?
There was a problem hiding this comment.
Could you also test that password length warning with pytest.warns()?
| tls_adapter_cls( | ||
| certificate=tls_certificate_chain_pem_path, | ||
| private_key=tls_certificate_passwd_private_key_pem_path, | ||
| private_key_password=str(uuid.uuid4()), |
There was a problem hiding this comment.
We could hardcode a long value here too. Like
| private_key_password=str(uuid.uuid4()), | |
| private_key_password='x' * 256, |
2 participants
❓ What kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)cherrypy/cherrypy#1583
❓ What is the current behavior? (You can also link to an open issue here)
With the current functionality it is only possible to use ssl adapters with private keys without password protection
❓ What is the new behavior (if this is a feature change)?
With this change, there is a new option to give the ssl adapter a "private_key_password" argument, which can be in either string or bytestring format.
📋 Other information:
Added also unit tests to test the new functionality
📋 Contribution checklist:
the changes have been approved
and description in grammatically correct, complete sentences
This change is