Add rewrite_to_https_port setting support

[invidian]

In situation, when nginx is behind port multiplexer, ex. sslh, so nginx ssl is listening on 8443 and multiplexer on 443, I was unable to set proper redirect to SSL. With this patch, we can set on which port we want to be redirected to SSL.

First if in template is to prevent using @ssl_port when @rewrite_to_https_port is defined. Second if is the same as for @ssl_port

[bastelfreak]

Hi @invidian, can you add tests for this PR?

[wyardley]

I'm in favor of this, but just wondering, what do people think about changing the name of the rewrite_to_https_port that's easier for people to find when searching for TLS / SSL related directives, say:
ssl_force_redirect or similar? To me, 'redirect' and 'ssl' would probably be the things I'd look for first, and I'd expect it to be clustered along with the SSL related directives.

@invidian: are you able to add tests and are you still interested in pushing this PR along? If not, I can take my best stab at reworking it when there's time, but no promises.

[invidian]

Hi, sorry for not updating for a long time. Unfortunately, I'm not familiar with unit test, so I can't provide them until I find time to learn how to do it. But I just ticked "Allow edits from maintainers", so you should be able to push tests to that PR.

That would be nice to push this PR, as it covers missing functionality, which is quite often used in my opinion.

In my opinion, renaming it to ssl_force and ssl_force_port might be a good idea, but it's up to you.

[wyardley]

It would mostly be modifying this test: https://github.com/voxpupuli/puppet-nginx/blob/master/spec/defines/resource_vhost_spec.rb#L234
I'll leave this open for now, if I get a chance later, I'll try to make a replacement PR that handles this and renames both variables.

[wyardley]

The interesting thing is that this already seems to be there in vhost_ssl_header.erb

  if ($ssl_protocol = "") {
       return 301 https://$host<% if @ssl_port.to_i != 443 %>:<%= @ssl_port %><% end %>$request_uri;

So I think if @ssl_port is set, this will already behave as you expect.

[law]

In general, nginx frowns upon using 'if' statements, per:
https://www.nginx.com/resources/wiki/start/topics/depth/ifisevil/

They also have a recommended 'force-http-to-https' config statement, per:
https://www.nginx.com/resources/wiki/start/topics/tutorials/config_pitfalls/#taxing-rewrites

Given that, perhaps there should be a more stringent "force_all_to_ssl" or similar option? That would replace the previous if-statement and various :80 stanzas to just use:

{
  listen *:80;
  server_name           myvhostname;
  return 301 https://$host$request_uri;
}

[wyardley]

@invidian: rereading, and I see what you're saying now, I'll try to add this to my rework as well.

[invidian]

@wyardley Thank you for your effort resolving this. Your pull request looks good, I hope you will be able to finish it.

[wyardley]

@invidian: thanks -- take a look, I've made some additional changes.
I didn't include the previous commits from this PR since I reworked the logic quite a bit; hope that's Ok if that one ends up getting merged instead.

[invidian]

#957 looks OK. Feel free to close that one if your PR gets merged.

[wyardley]

This functionality now exists in the (merged) #957