aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authormkanat%bugzilla.org <>2009-10-09 04:31:08 +0000
committermkanat%bugzilla.org <>2009-10-09 04:31:08 +0000
commit8ecb3ad6ecc8d636fb205895d736108cbc8083a1 (patch)
tree69b5da83e47b1fc8481227f2ec46aba1261e84ad
parent4671e0ffd9920d000fb6191999288ed12d4dac52 (diff)
downloadbugs-8ecb3ad6ecc8d636fb205895d736108cbc8083a1.tar
bugs-8ecb3ad6ecc8d636fb205895d736108cbc8083a1.tar.gz
bugs-8ecb3ad6ecc8d636fb205895d736108cbc8083a1.tar.bz2
bugs-8ecb3ad6ecc8d636fb205895d736108cbc8083a1.tar.xz
bugs-8ecb3ad6ecc8d636fb205895d736108cbc8083a1.zip
Bug 514913: Eliminate ssl="authenticated sessions"
Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=dkl, a=mkanat
-rw-r--r--Bugzilla.pm24
-rw-r--r--Bugzilla/Auth/Login/CGI.pm11
-rw-r--r--Bugzilla/Auth/Persist/Cookie.pm8
-rw-r--r--Bugzilla/CGI.pm43
-rw-r--r--Bugzilla/Config.pm5
-rw-r--r--Bugzilla/Config/Core.pm7
-rw-r--r--Bugzilla/Mailer.pm5
-rw-r--r--Bugzilla/Util.pm68
-rw-r--r--Bugzilla/WebService/Server.pm14
-rw-r--r--docs/en/xml/administration.xml8
-rwxr-xr-xindex.cgi8
-rw-r--r--template/en/default/account/auth/login-small.html.tmpl7
-rw-r--r--template/en/default/admin/params/core.html.tmpl8
-rw-r--r--template/en/default/bug/edit.html.tmpl3
-rwxr-xr-xtoken.cgi8
15 files changed, 74 insertions, 153 deletions
diff --git a/Bugzilla.pm b/Bugzilla.pm
index 43a9b39ae..62b1af659 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -85,7 +85,6 @@ use constant SHUTDOWNHTML_EXIT_SILENTLY => [
sub init_page {
(binmode STDOUT, ':utf8') if Bugzilla->params->{'utf8'};
-
if (${^TAINT}) {
# Some environment variables are not taint safe
delete @::ENV{'PATH', 'IFS', 'CDPATH', 'ENV', 'BASH_ENV'};
@@ -94,6 +93,12 @@ sub init_page {
$ENV{'PATH'} = '';
}
+ # Because this function is run live from perl "use" commands of
+ # other scripts, we're skipping the rest of this function if we get here
+ # during a perl syntax check (perl -c, like we do during the
+ # 001compile.t test).
+ return if $^C;
+
# IIS prints out warnings to the webpage, so ignore them, or log them
# to a file if the file exists.
if ($ENV{SERVER_SOFTWARE} && $ENV{SERVER_SOFTWARE} =~ /microsoft-iis/i) {
@@ -108,18 +113,15 @@ sub init_page {
};
}
+ do_ssl_redirect_if_required();
+
# If Bugzilla is shut down, do not allow anything to run, just display a
# message to the user about the downtime and log out. Scripts listed in
# SHUTDOWNHTML_EXEMPT are exempt from this message.
#
- # Because this is code which is run live from perl "use" commands of other
- # scripts, we're skipping this part if we get here during a perl syntax
- # check -- runtests.pl compiles scripts without running them, so we
- # need to make sure that this check doesn't apply to 'perl -c' calls.
- #
# This code must go here. It cannot go anywhere in Bugzilla::CGI, because
# it uses Template, and that causes various dependency loops.
- if (!$^C && Bugzilla->params->{"shutdownhtml"}
+ if (Bugzilla->params->{"shutdownhtml"}
&& lsearch(SHUTDOWNHTML_EXEMPT, basename($0)) == -1)
{
# Allow non-cgi scripts to exit silently (without displaying any
@@ -318,14 +320,6 @@ sub login {
$class->set_user($authenticated_user);
}
- # We run after the login has completed since
- # some of the checks in ssl_require_redirect
- # look for Bugzilla->user->id to determine
- # if redirection is required.
- if (i_am_cgi() && ssl_require_redirect()) {
- $class->cgi->require_https($class->params->{'sslbase'});
- }
-
return $class->user;
}
diff --git a/Bugzilla/Auth/Login/CGI.pm b/Bugzilla/Auth/Login/CGI.pm
index 5be98aa7a..a93bc3d3a 100644
--- a/Bugzilla/Auth/Login/CGI.pm
+++ b/Bugzilla/Auth/Login/CGI.pm
@@ -65,17 +65,6 @@ sub fail_nodata {
->faultstring('Login Required');
}
- # If system is not configured to never require SSL connections
- # we want to always redirect to SSL since passing usernames and
- # passwords over an unprotected connection is a bad idea. If we
- # get here then a login form will be provided to the user so we
- # want this to be protected if possible.
- if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
- && Bugzilla->params->{'ssl'} ne 'never')
- {
- $cgi->require_https(Bugzilla->params->{'sslbase'});
- }
-
print $cgi->header();
$template->process("account/auth/login.html.tmpl",
{ 'target' => $cgi->url(-relative=>1) })
diff --git a/Bugzilla/Auth/Persist/Cookie.pm b/Bugzilla/Auth/Persist/Cookie.pm
index c533252d3..60f90925e 100644
--- a/Bugzilla/Auth/Persist/Cookie.pm
+++ b/Bugzilla/Auth/Persist/Cookie.pm
@@ -89,11 +89,9 @@ sub persist_login {
# Not a session cookie, so set an infinite expiry
$cookieargs{'-expires'} = 'Fri, 01-Jan-2038 00:00:00 GMT';
}
- if (Bugzilla->params->{'ssl'} ne 'never'
- && Bugzilla->params->{'sslbase'} ne '')
- {
- # Bugzilla->login will automatically redirect to https://,
- # so it's safe to turn on the 'secure' bit.
+ if (Bugzilla->params->{'ssl_redirect'}) {
+ # Make these cookies only be sent to us by the browser during
+ # HTTPS sessions, if we're using SSL.
$cookieargs{'-secure'} = 1;
}
diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm
index a00d632c3..c30e13618 100644
--- a/Bugzilla/CGI.pm
+++ b/Bugzilla/CGI.pm
@@ -368,22 +368,23 @@ sub remove_cookie {
'-value' => 'X');
}
-# Redirect to https if required
-sub require_https {
- my ($self, $url) = @_;
- # Do not create query string if data submitted via XMLRPC
- # since we want the data to be resubmitted over POST method.
- my $query = Bugzilla->usage_mode == USAGE_MODE_XMLRPC ? 0 : 1;
- # XMLRPC clients (SOAP::Lite at least) requires 301 to redirect properly
- # and do not work with 302.
- my $status = Bugzilla->usage_mode == USAGE_MODE_XMLRPC ? 301 : 302;
- if (defined $url) {
- $url .= $self->url('-path_info' => 1, '-query' => $query, '-relative' => 1);
- } else {
- $url = $self->self_url;
- $url =~ s/^http:/https:/i;
- }
- print $self->redirect(-location => $url, -status => $status);
+sub redirect_to_https {
+ my $self = shift;
+ my $sslbase = Bugzilla->params->{'sslbase'};
+ # If this is a POST, we don't want ?POSTDATA in the query string.
+ # We expect the client to re-POST, which may be a violation of
+ # the HTTP spec, but the only time we're expecting it often is
+ # in the WebService, and WebService clients usually handle this
+ # correctly.
+ $self->delete('POSTDATA');
+ my $url = $sslbase . $self->url('-path_info' => 1, '-query' => 1,
+ '-relative' => 1);
+
+ # XML-RPC clients (SOAP::Lite at least) require a 301 to redirect properly
+ # and do not work with 302. Our redirect really is permanent anyhow, so
+ # it doesn't hurt to make it a 301.
+ print $self->redirect(-location => $url, -status => 301);
+
# When using XML-RPC with mod_perl, we need the headers sent immediately.
$self->r->rflush if $ENV{MOD_PERL};
exit;
@@ -459,13 +460,13 @@ effectively removing the cookie.
As its only argument, it takes the name of the cookie to expire.
-=item C<require_https($baseurl)>
+=item C<redirect_to_https>
-This routine redirects the client to a different location using the https protocol.
-If the client is using XMLRPC, it will not retain the QUERY_STRING since XMLRPC uses POST.
+This routine redirects the client to the https version of the page that
+they're looking at, using the C<sslbase> parameter for the redirection.
-It takes an optional argument which will be used as the base URL. If $baseurl
-is not provided, the current URL is used.
+Generally you should use L<Bugzilla::Util/do_ssl_redirect_if_required>
+instead of calling this directly.
=item C<redirect_to_urlbase>
diff --git a/Bugzilla/Config.pm b/Bugzilla/Config.pm
index a20751737..f56ffd78a 100644
--- a/Bugzilla/Config.pm
+++ b/Bugzilla/Config.pm
@@ -192,6 +192,11 @@ sub update_params {
$param->{'mail_delivery_method'} = $translation{$method};
}
+ # Convert the old "ssl" parameter to the new "ssl_redirect" parameter.
+ # Both "authenticated sessions" and "always" turn on "ssl_redirect"
+ # when upgrading.
+ $param->{'ssl_redirect'} = 1 if $param->{'ssl'} ne 'never';
+
# --- DEFAULTS FOR NEW PARAMS ---
_load_params unless %params;
diff --git a/Bugzilla/Config/Core.pm b/Bugzilla/Config/Core.pm
index 6d413b965..91426b30a 100644
--- a/Bugzilla/Config/Core.pm
+++ b/Bugzilla/Config/Core.pm
@@ -68,10 +68,9 @@ sub get_param_list {
},
{
- name => 'ssl',
- type => 's',
- choices => ['never', 'authenticated sessions', 'always'],
- default => 'never'
+ name => 'ssl_redirect',
+ type => 'b',
+ default => 0
},
diff --git a/Bugzilla/Mailer.pm b/Bugzilla/Mailer.pm
index 610523b8a..83ae5a600 100644
--- a/Bugzilla/Mailer.pm
+++ b/Bugzilla/Mailer.pm
@@ -82,10 +82,7 @@ sub MessageToMTA {
#
# We don't use correct_urlbase, because we want this URL to
# *always* be the same for this Bugzilla, in every email,
- # and some emails we send when we're logged out (in which case
- # some emails might get urlbase while the logged-in emails might
- # get sslbase). Also, we want this to stay the same even if
- # the admin changes the "ssl" parameter.
+ # even if the admin changes the "ssl_redirect" parameter some day.
$email->header_set('X-Bugzilla-URL', Bugzilla->params->{'urlbase'});
# We add this header to mark the mail as "auto-generated" and
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 55ec6dcf8..90525b9d4 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -36,7 +36,7 @@ use base qw(Exporter);
html_quote url_quote xml_quote
css_class_quote html_light_quote url_decode
i_am_cgi get_netaddr correct_urlbase
- lsearch ssl_require_redirect use_attachbase
+ lsearch do_ssl_redirect_if_required use_attachbase
diff_arrays
trim wrap_hard wrap_comment find_wrap_point
format_time format_time_decimal validate_date
@@ -264,60 +264,28 @@ sub i_am_cgi {
return exists $ENV{'SERVER_SOFTWARE'} ? 1 : 0;
}
-sub ssl_require_redirect {
- my $method = shift;
-
- # If currently not in a protected SSL
- # connection, determine if a redirection is
- # needed based on value in Bugzilla->params->{ssl}.
- # If we are already in a protected connection or
- # sslbase is not set then no action is required.
- if (uc($ENV{'HTTPS'}) ne 'ON'
- && $ENV{'SERVER_PORT'} != 443
- && Bugzilla->params->{'sslbase'} ne '')
- {
- # System is configured to never require SSL
- # so no redirection is needed.
- return 0
- if Bugzilla->params->{'ssl'} eq 'never';
-
- # System is configured to always require a SSL
- # connection so we need to redirect.
- return 1
- if Bugzilla->params->{'ssl'} eq 'always';
-
- # System is configured such that if we are inside
- # of an authenticated session, then we need to make
- # sure that all of the connections are over SSL. Non
- # authenticated sessions SSL is not mandatory.
- # For XMLRPC requests, if the method is User.login
- # then we always want the connection to be over SSL
- # if the system is configured for authenticated
- # sessions since the user's username and password
- # will be passed before the user is logged in.
- return 1
- if Bugzilla->params->{'ssl'} eq 'authenticated sessions'
- && (Bugzilla->user->id
- || (defined $method && $method eq 'User.login'));
- }
+# This exists as a separate function from Bugzilla::CGI::redirect_to_https
+# because we don't want to create a CGI object during XML-RPC calls
+# (doing so can mess up XML-RPC).
+sub do_ssl_redirect_if_required {
+ return if !i_am_cgi();
+ return if !Bugzilla->params->{'ssl_redirect'};
- return 0;
+ my $sslbase = Bugzilla->params->{'sslbase'};
+
+ # If we're already running under SSL, never redirect.
+ return if uc($ENV{HTTPS} || '') eq 'ON';
+ # Never redirect if there isn't an sslbase.
+ return if !$sslbase;
+ Bugzilla->cgi->redirect_to_https();
}
sub correct_urlbase {
- my $ssl = Bugzilla->params->{'ssl'};
- return Bugzilla->params->{'urlbase'} if $ssl eq 'never';
-
+ my $ssl = Bugzilla->params->{'ssl_redirect'};
+ my $urlbase = Bugzilla->params->{'urlbase'};
my $sslbase = Bugzilla->params->{'sslbase'};
- if ($sslbase) {
- return $sslbase if $ssl eq 'always';
- # Authenticated Sessions
- return $sslbase if Bugzilla->user->id;
- }
- # Set to "authenticated sessions" but nobody's logged in, or
- # sslbase isn't set.
- return Bugzilla->params->{'urlbase'};
+ return ($ssl && $sslbase) ? $sslbase : $urlbase;
}
sub use_attachbase {
@@ -830,7 +798,7 @@ cookies) to only some addresses.
=item C<correct_urlbase()>
Returns either the C<sslbase> or C<urlbase> parameter, depending on the
-current setting for the C<ssl> parameter.
+current setting for the C<ssl_redirect> parameter.
=item C<use_attachbase()>
diff --git a/Bugzilla/WebService/Server.pm b/Bugzilla/WebService/Server.pm
index dfb9f559a..2db182fd4 100644
--- a/Bugzilla/WebService/Server.pm
+++ b/Bugzilla/WebService/Server.pm
@@ -17,26 +17,12 @@
package Bugzilla::WebService::Server;
use strict;
-use Bugzilla::Util qw(ssl_require_redirect);
sub handle_login {
my ($self, $class, $method, $full_method) = @_;
eval "require $class";
return if $class->login_exempt($method);
Bugzilla->login();
-
- # Even though we check for the need to redirect in
- # Bugzilla->login() we check here again since Bugzilla->login()
- # does not know what the current XMLRPC method is. Therefore
- # ssl_require_redirect in Bugzilla->login() will have returned
- # false if system was configured to redirect for authenticated
- # sessions and the user was not yet logged in.
- # So here we pass in the method name to ssl_require_redirect so
- # it can then check for the extra case where the method equals
- # User.login, which we would then need to redirect if not
- # over a secure connection.
- Bugzilla->cgi->require_https(Bugzilla->params->{'sslbase'})
- if ssl_require_redirect($full_method);
}
1;
diff --git a/docs/en/xml/administration.xml b/docs/en/xml/administration.xml
index 6b6ff5fda..0c9a60ce2 100644
--- a/docs/en/xml/administration.xml
+++ b/docs/en/xml/administration.xml
@@ -100,13 +100,13 @@
<varlistentry>
<term>
- ssl
+ ssl_redirect
</term>
<listitem>
<para>
- Determines when Bugzilla will force HTTPS (SSL) connections, using
- the URL defined in <command>sslbase</command>.
- Options include "always", "never", and "authenticated sessions".
+ If enabled, Bugzilla will force HTTPS (SSL) connections, by
+ automatically redirecting any users who try to use a non-SSL
+ connection.
</para>
</listitem>
</varlistentry>
diff --git a/index.cgi b/index.cgi
index 660909452..fac26434a 100755
--- a/index.cgi
+++ b/index.cgi
@@ -56,14 +56,6 @@ if ($cgi->param('logout')) {
# Main Body Execution
###############################################################################
-# Force to use HTTPS unless Bugzilla->params->{'ssl'} equals 'never'.
-# This is required because the user may want to log in from here.
-if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
- && Bugzilla->params->{'ssl'} ne 'never')
-{
- $cgi->require_https(Bugzilla->params->{'sslbase'});
-}
-
# Return the appropriate HTTP response headers.
print $cgi->header();
diff --git a/template/en/default/account/auth/login-small.html.tmpl b/template/en/default/account/auth/login-small.html.tmpl
index 752aa64df..710d7d0a5 100644
--- a/template/en/default/account/auth/login-small.html.tmpl
+++ b/template/en/default/account/auth/login-small.html.tmpl
@@ -28,12 +28,7 @@
[% login_target = "index.cgi" %]
[% END %]
-[%# If SSL is in use, use 'sslbase', else use 'urlbase'. %]
-[% IF Param("sslbase") != "" && Param("ssl") != "never" %]
- [% login_target = Param("sslbase") _ login_target %]
-[% ELSE %]
- [% login_target = Param("urlbase") _ login_target %]
-[% END %]
+[% login_target = urlbase _ login_target %]
<li id="mini_login_container[% qs_suffix %]">
<span class="separator">| </span>
diff --git a/template/en/default/admin/params/core.html.tmpl b/template/en/default/admin/params/core.html.tmpl
index d66c4a51b..b65dde233 100644
--- a/template/en/default/admin/params/core.html.tmpl
+++ b/template/en/default/admin/params/core.html.tmpl
@@ -42,8 +42,12 @@
sslbase => "The URL that is the common initial leading part of all HTTPS " _
"(SSL) $terms.Bugzilla URLs.",
- ssl => "Controls when $terms.Bugzilla should enforce sessions to use HTTPS by " _
- "using <tt>sslbase</tt>.",
+ ssl_redirect =>
+ "When this is enabled, $terms.Bugzilla will ensure that every page is"
+ _ " accessed over SSL, by redirecting any plain HTTP requests to HTTPS"
+ _ " using the <tt>sslbase</tt> parameter. Also, when this is enabled,"
+ _ " $terms.Bugzilla will send out links using <tt>sslbase</tt> in emails"
+ _ " instead of <tt>urlbase</tt>.",
cookiedomain => "The domain for $terms.Bugzilla cookies. Normally blank. " _
"If your website is at 'www.foo.com', setting this to " _
diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl
index 76ca259e5..d5a345182 100644
--- a/template/en/default/bug/edit.html.tmpl
+++ b/template/en/default/bug/edit.html.tmpl
@@ -277,7 +277,8 @@
<legend>Note</legend>
<p>
You need to
- <a href="[% IF Param('ssl') != 'never' %][% Param('sslbase') %][% END %]show_bug.cgi?id=[% bug.bug_id %]&amp;GoAheadAndLogIn=1">log in</a>
+ <a href="show_bug.cgi?id=
+ [%- bug.bug_id %]&amp;GoAheadAndLogIn=1">log in</a>
before you can comment on or make changes to this [% terms.bug %].
</p>
</fieldset>
diff --git a/token.cgi b/token.cgi
index 614feefa9..d4298cde7 100755
--- a/token.cgi
+++ b/token.cgi
@@ -360,15 +360,7 @@ sub request_create_account {
$vars->{'email'} = $login_name . Bugzilla->params->{'emailsuffix'};
$vars->{'expiration_ts'} = ctime(str2time($date) + MAX_TOKEN_AGE * 86400);
- # When 'ssl' equals 'always' or 'authenticated sessions',
- # we want this form to always be over SSL.
- if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
- && Bugzilla->params->{'ssl'} ne 'never')
- {
- $cgi->require_https(Bugzilla->params->{'sslbase'});
- }
print $cgi->header();
-
$template->process('account/email/confirm-new.html.tmpl', $vars)
|| ThrowTemplateError($template->error());
}