Conversation
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.
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!).
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 |
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