Nginx improvements #950
Nginx improvements #950
Conversation
| @@ -163,7 +163,8 @@ def prepare(self): | |||
|
|
|||
| temp_install(self.mod_ssl_conf) | |||
|
|
|||
| def deploy_cert(self, domain, cert_path, key_path, chain_path=None): | |||
| def deploy_cert(self, domain, cert_path, key_path, | |||
| chain_path=None, fullchain_path=None): # pylint: disable=unused-argument | |||
There was a problem hiding this comment.
Indentation seems to be broken here, you should add 4 more spaces. I believe this was not caught due to 6604a0f and its indent-after-paren change. Same elsewhere.
| # Nginx can take a moment to recognize a newly added TLS SNI servername, so sleep | ||
| # for a second. TODO: Check for expected servername and loop until it | ||
| # appears or return an error if looping too long. | ||
| time.sleep(1) |
There was a problem hiding this comment.
TODO comment seems pretty easy to do. Could we implement it in this PR to avoid unnecessary race condition debugging in the future?
There was a problem hiding this comment.
I think it's a little harder than it sounds, but I'd be interested in being proved wrong. My main concerns were:
(a) Can we be sure connecting to localhost is always the right thing? (probably yes)
(b) what method do I use to connect?
For (b), urllib seems to be the obvious answer, but we'd have to disable cert checking because it would be a snakeoil cert. However, if the new config hasn't loaded, we will get a default cert, so we won't get a nice failure like we wanted. Is there an easy to use connect method that will give me back the DNSNames of the provided cert regardless of cert validity?
There was a problem hiding this comment.
BTW, it looks like Apache has this same problem. Filed an issue to fix for both: #954
There was a problem hiding this comment.
(a) maybe just connect to the domain in question?
(b) you can use acme.crypto_util.probe_sni for that :)
(a) and (b) together: probe_sni(domain, socket.gethostbyname(domain), config.dvsni_port)
There was a problem hiding this comment.
Awesome, thanks! I updated the linked tickets with these. I'd like to land this as-is, since I can only really fit in client time on the weekends. But I definitely want to fix this the nice way ASAP (especially for Apache!).
| body = entry[1] | ||
| if directive not in body: | ||
| body.append(directive) | ||
| for key, body in main: |
There was a problem hiding this comment.
This assumes every directive has exactly two elements. I think this is okay for now, but in the future we might add something to nginxparser that does not produce directives of size 2.
|
lgtm |
|
@bmw @jdkasten @pde: What's the merge policy on this repo? Should I merge based on @diracdeltas's review? Wait for a second review? |
|
The current policy is just one "LGTM" from a core contributor. Based on @diracdeltas review, you're good to go. Merging this now. Thanks for the PR @jsha. |
|
I noticed that certbot is not adding the |
|
Nope, it's not expected to do that right now. |
|
I'm confused then. This pull request writes out those settings somewhere.
Where's that?
…On Sun, Sep 23, 2018, 1:22 AM Jacob Hoffman-Andrews < ***@***.***> wrote:
Nope, it's not expected to do that right now.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#950 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABr6dqjLRWwE5FOcK5i04fgfMoOX6fZ2ks5udyivgaJpZM4GMyMj>
.
|
|
Check the latest version of this code, not this old PR; things have changed in the last three years. |
Add a server_names_hash_bucket_size directive during challenges to fix an nginx
crash on restart (Fixes #922).
Use fullchain instead of chain (Fixes #610).
Implement OCSP stapling (Fixes #937, Fixes #931).
This pull request replaces two earlier ones I sent.
cc @diracdeltas