Skip to content

Commit

Permalink
Merge pull request #957 from wyardley/rework_ssl_redirects
Browse files Browse the repository at this point in the history 
Rename rewrite_to_https => ssl_redirect (backwards-incompatible change)
  • Loading branch information
@bastelfreak
bastelfreak committed Nov 7, 2016
2 parents 7a88a11 + 857bc07 commit bcb17f2
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 32 deletions.
44 changes: 29 additions & 15 deletions manifests/resource/vhost.pp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
61 changes: 49 additions & 12 deletions spec/defines/resource_vhost_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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' } }

Expand Down
8 changes: 3 additions & 5 deletions templates/vhost/vhost_header.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 -%>
Expand Down

0 comments on commit bcb17f2

Please sign in to comment.