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

adding parameter --ssl-password to decrypt ssl key file #776

Closed
wants to merge 5 commits into from

Conversation

remster85
Copy link
Contributor

@remster85 remster85 commented Sep 14, 2020

Add parameter --ssl-password to be able to decrypt the ssl key
If provided, this parameter will be passed to load_cert_chain as password.

The password argument may be a function to call to get the password for decrypting the private key. It will only be called if the private key is encrypted and a password is necessary. It will be called with no arguments, and it should return a string, bytes, or bytearray. If the return value is a string it will be encoded as UTF-8 before using it to decrypt the key. Alternatively a string, bytes, or bytearray value may be supplied directly as the password argument. It will be ignored if the private key is not encrypted and no password is needed.

https://docs.python.org/3.3/library/ssl.html#ssl.SSLContext.load_cert_chain

This issue required a pull request #581

Fixes #581

@remster85
Copy link
Contributor Author

Let's go! Could you please merge?

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Sounds like an acceptable feature to add, yup. I left a naming suggestion which I think we should address before merging this.

Also, looks like we should add a test to our test suite, at least to verify that the config ingestion logic works well?

Eg we've got fixtures for a certfile and a keyfile in conftest.py. So we can probably add an ENCRYPTED_PRIVATE_KEY that's an encrypted version of PRIVATE_KEY, and then make sure our config is able to load that properly into the ssl_context, by updating test_config.py?

@@ -79,6 +79,7 @@ Options:
5]
--ssl-keyfile TEXT SSL key file
--ssl-certfile TEXT SSL certificate file
--ssl-password TEXT SSL key file password
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on naming this --ssl-keyfile-password? Both for extra explicitness that this is related to --ssl-keyfile, as well as visually hinting that "this is a more advanced option, since its name is longer" :-)

Suggested change
--ssl-password TEXT SSL key file password
--ssl-keyfile-password TEXT SSL key file password

Comment on lines +108 to +115
if password:

def getpassword():
return password

ctx.load_cert_chain(certfile, keyfile, getpassword)
else:
ctx.load_cert_chain(certfile, keyfile)
Copy link
Member

Choose a reason for hiding this comment

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

(Nit) Dunno if we could go for a more compact form - hopefully this is readable enough? Or can we not pass None as the password argument? (The Python docs aren't very explicit about that.)

Suggested change
if password:
def getpassword():
return password
ctx.load_cert_chain(certfile, keyfile, getpassword)
else:
ctx.load_cert_chain(certfile, keyfile)
password = (lambda: password) if password else None
ctx.load_cert_chain(certfile, keyfile, password)

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Marking as "Changes requested" since I don't think this should be merged as-is for now, at least not without a basic test to prevent future regressions. :-)

@remster85
Copy link
Contributor Author

Thanks @florimondmanca for the review , allow me some time to get back to you.

@euri10
Copy link
Member

euri10 commented Oct 12, 2020

thanks a lot @remster85, I built on your PR in #808 and added the tests etc using trustme.

@euri10 euri10 closed this Oct 12, 2020
@remster85
Copy link
Contributor Author

Thanks so much @euri10 !

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.

Load certificates with custom function
3 participants