Repository navigation
don't use snakeoil certs in nginx - #10465
Conversation
ohemorange
left a comment
ohemorange
left a comment
There was a problem hiding this comment.
looks good to me!
i think we could also do additional refactoring in certbot-nginx by moving the _make_server_ssl call out of choose_or_make_vhost and make deploy_cert responsible for calling it if the returned vhosts aren't ssl. in this case, we could then skip updating cert and key directives a second time as this is duplicate work if we just made the server ssl
i considered doing this, but it's a bigger refactor, breaks more tests, and i'm not sure it really buys us much so i skipped it. i could take this on or create an issue for it if you think it's important for us to do for some reason tho ohemorange
I don't think it's particularly important, although if you did want to do it I wouldn't be opposed. the duplicate work is a little silly but not much in the grand scheme of things, and at this point I feel the code is much clearer than before and agree that wouldn't be a significant improvement.
|
thanks for your thoughts! in that case, i think i won't worry about the additional refactoring in certbot-nginx here and just clean up the make_self_signed_cert function |
with #10465, make_self_signed_cert is no longer used outside of our tests so let's move it there and remove it from our public API
this PR finally removes all uses of self-signed certificates from certbot-nginx
i plan to create one last PR related to this deprecating
acme.crypto_util.make_self_signed_certand moving the function to certbot-compatibility-test which is the only place it's currently usedi think we could also do additional refactoring in certbot-nginx by moving the _make_server_ssl call out of choose_or_make_vhost and make deploy_cert responsible for calling it if the returned vhosts aren't ssl. in this case, we could then skip updating cert and key directives a second time as this is duplicate work if we just made the server ssl
i considered doing this, but it's a bigger refactor, breaks more tests, and i'm not sure it really buys us much so i skipped it. i could take this on or create an issue for it if you think it's important for us to do for some reason tho ohemorange