From 9172ccd23139442f74b2395f706a5f7a67cad762 Mon Sep 17 00:00:00 2001 From: William Yardley Date: Tue, 1 Nov 2016 15:25:52 -0700 Subject: [PATCH] Rename rewrite_to_https => ssl_force_redirect (breaking / backwards-incompatible change), take out needless "if" statement before return. Also forces integer for $ssl_port and $listen_port, which previously only generated a warning. --- manifests/resource/vhost.pp | 44 ++++++++++++++------- spec/defines/resource_vhost_spec.rb | 61 +++++++++++++++++++++++------ templates/vhost/vhost_header.erb | 8 ++-- 3 files changed, 81 insertions(+), 32 deletions(-) diff --git a/manifests/resource/vhost.pp b/manifests/resource/vhost.pp index d06e9bb7e..6fca138ca 100644 --- a/manifests/resource/vhost.pp +++ b/manifests/resource/vhost.pp @@ -63,6 +63,10 @@ # [*ssl_dhparam*] - This directive specifies a file containing # Diffie-Hellman key agreement protocol cryptographic parameters, in PEM # format, utilized for exchanging session keys between server and client. +# [*ssl_redirect*] - Adds a server directive and return statement to +# force ssl redirect. Will honor ssl_port if it's set. +# [*ssl_redirect_port*] - Overrides $ssl_port in the SSL redirect set by +# ssl_redirect # [*ssl_key*] - Pre-generated SSL Key file to reference for SSL # Support. This is not generated by this module. Set to `false` to inherit # from the http section, which improves performance by conserving memory. @@ -144,8 +148,6 @@ # put after everything else inside vhost ssl # [*vhost_cfg_ssl_prepend*] - It expects a hash with custom directives to # put before everything else inside vhost ssl -# [*rewrite_to_https*] - Adds a server directive and rewrite rule to -# rewrite to ssl # [*include_files*] - Adds include files to vhost # [*access_log*] - Where to write access log (log format can be # set with $format_log). This can be either a string or an array; in the @@ -215,6 +217,8 @@ $ssl_client_cert = undef, $ssl_verify_client = 'on', $ssl_dhparam = undef, + $ssl_redirect = false, + $ssl_redirect_port = undef, $ssl_key = undef, $ssl_port = 443, $ssl_protocols = $::nginx::ssl_protocols, @@ -263,7 +267,6 @@ $server_name = [$name], $www_root = undef, $rewrite_www_to_non_www = false, - $rewrite_to_https = undef, $location_custom_cfg = undef, $location_cfg_prepend = undef, $location_cfg_append = undef, @@ -312,10 +315,7 @@ if !(is_array($listen_ip) or is_string($listen_ip)) { fail('$listen_ip must be a string or array.') } - if is_string($listen_port) { - warning('DEPRECATION: String $listen_port must be converted to an integer. Integer string support will be removed in a future release.') - } - elsif !is_integer($listen_port) { + if !is_integer($listen_port) { fail('$listen_port must be an integer.') } if ($listen_options != undef) { @@ -365,13 +365,16 @@ if ($ssl_dhparam != undef) { validate_string($ssl_dhparam) } + validate_bool($ssl_redirect) + if ($ssl_redirect_port != undef) { + if !is_integer($ssl_redirect_port) { + fail('$ssl_redirect_port must be an integer.') + } + } if $ssl_key { validate_string($ssl_key) } - if is_string($ssl_port) { - warning('DEPRECATION: String $ssl_port must be converted to an integer. Integer string support will be removed in a future release.') - } - elsif !is_integer($ssl_port) { + if !is_integer($ssl_port) { fail('$ssl_port must be an integer.') } validate_string($ssl_protocols) @@ -462,9 +465,6 @@ validate_string($www_root) } validate_bool($rewrite_www_to_non_www) - if ($rewrite_to_https != undef) { - validate_bool($rewrite_to_https) - } if ($raw_prepend != undef) { if (is_array($raw_prepend)) { validate_array($raw_prepend) @@ -618,7 +618,21 @@ require => File[$vhost_dir], } - $ssl_only = ($ssl == true) and (($ssl_port + 0) == ($listen_port + 0)) + # This deals with a situation where the listen directive for SSL doesn't match + # the port we want to force the SSL redirect to. + if ($ssl_redirect_port) { + $_ssl_redirect_port = $ssl_redirect_port + } elsif ($ssl_port) { + $_ssl_redirect_port = $ssl_port + } + + # Suppress unneded stuff in non-SSL location block when certain conditions are + # met. + if (($ssl == true) and ($ssl_port == $listen_port)) or ($ssl_redirect) { + $ssl_only = true + } else { + $ssl_only = false + } if $use_default_location == true { # Create the default location reference for the vHost diff --git a/spec/defines/resource_vhost_spec.rb b/spec/defines/resource_vhost_spec.rb index 1ac3275b3..8f337c215 100644 --- a/spec/defines/resource_vhost_spec.rb +++ b/spec/defines/resource_vhost_spec.rb @@ -235,22 +235,16 @@ notmatch: %r{ root /;} }, { - title: 'should rewrite to HTTPS', - attr: 'rewrite_to_https', + title: 'should force https (SSL) redirect', + attr: 'ssl_redirect', value: true, - match: [ - ' if ($ssl_protocol = "") {', - ' return 301 https://$host$request_uri;' - ] + match: %r{ return 301 https://\$host\$request_uri;} }, { - title: 'should not rewrite to HTTPS', - attr: 'rewrite_to_https', + title: 'should not force https (SSL) redirect', + attr: 'ssl_redirect', value: false, - notmatch: [ - %r'if \(\$ssl_protocol = ""\) \{', - %r{\s+return 301 https://\$host\$request_uri;} - ] + notmatch: %r{\s*return\s+301} }, { title: 'should set access_log', @@ -855,6 +849,49 @@ end end + context 'ssl_redirect' do + let(:params) { { ssl_redirect: true } } + + it { is_expected.to contain_concat__fragment("#{title}-header").without_content(%r{^\s*index\s+}) } + it { is_expected.to contain_concat__fragment("#{title}-header").without_content(%r{^\s*location\s+}) } + end + + context 'ssl_redirect with alternate port' do + let(:params) { { ssl_redirect: true, ssl_port: 8888 } } + + it { is_expected.to contain_concat__fragment("#{title}-header").with_content(%r{ return 301 https://\$host:8888\$request_uri;}) } + end + + context 'ssl_redirect with standard port set explicitly' do + let(:params) { { ssl_redirect: true, ssl_port: 443 } } + + it { is_expected.to contain_concat__fragment("#{title}-header").with_content(%r{ return 301 https://\$host\$request_uri;}) } + end + + context 'ssl_redirect with overridden port' do + let(:params) { { ssl_redirect: true, ssl_redirect_port: 8878 } } + + it { is_expected.to contain_concat__fragment("#{title}-header").with_content(%r{ return 301 https://\$host:8878\$request_uri;}) } + end + + context 'ssl_redirect with ssl_port set and overridden redirect port' do + let(:params) do + { + ssl_redirect: true, + ssl_redirect_port: 9787, + ssl_port: 9783 + } + end + + it { is_expected.to contain_concat__fragment("#{title}-header").with_content(%r{ return 301 https://\$host:9787\$request_uri;}) } + end + + context 'ssl_redirect should set ssl_only' do + let(:params) { { ssl_redirect: true } } + + it { is_expected.to contain_nginx__resource__location("#{title}-default").with_ssl_only(true) } + end + context 'SSL cert missing' do let(:params) { { ssl: true, ssl_key: 'key' } } diff --git a/templates/vhost/vhost_header.erb b/templates/vhost/vhost_header.erb index 65c32fd86..14b90036c 100644 --- a/templates/vhost/vhost_header.erb +++ b/templates/vhost/vhost_header.erb @@ -134,12 +134,10 @@ server { <% if @maintenance -%> <%= @maintenance_value %>; <% end -%> -<% if @rewrite_to_https -%> - if ($ssl_protocol = "") { - return 301 https://$host<% if @ssl_port.to_i != 443 %>:<%= @ssl_port %><% end %>$request_uri; - } +<% if @ssl_redirect -%> + return 301 https://$host<% if @_ssl_redirect_port.to_i != 443 %>:<%= @_ssl_redirect_port %><% end %>$request_uri; <% end -%> -<% if @index_files.count > 0 -%> +<% if @index_files.count > 0 and not @ssl_only -%> index <% Array(@index_files).each do |i| %> <%= i %><% end %>; <% end -%>