-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, it looks like Apache has this same problem. Filed an issue to fix for both: #954
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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