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

nginx module: only turn on HTTP2 when SSL is enabled #33118

Merged
merged 1 commit into from Dec 27, 2017

Conversation

hrdinka
Copy link
Contributor

@hrdinka hrdinka commented Dec 27, 2017

Motivation for this change

#32858 did break all my nginx setups. With that patch "http2" gets appended to all vhosts by default, also to http only port 80 server blocks (like the ones generated by forceSSL=true). Nginx will then try to communicate securely with clients over port 80 and will return some binary garbage if no TLS certificate was set.

The fix is quite simple, it will now only activate HTTP2 when the ssl flag was set as well.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

CC @nh2 @fpletz @globin

Signed-off-by: Christoph Hrdinka <c.github@hrdinka.at>
@nh2
Copy link
Contributor

nh2 commented Dec 28, 2017

@hrdinka Thank you very much for finding this.

I've spent the last 2 hours trying to find out why the one rarely-used non-SSL default vhost I have returns garbage.

The HTTP2 spec supports running without SSL, but apparently nginx's implementation doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants