From 8ecb3ad6ecc8d636fb205895d736108cbc8083a1 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Fri, 9 Oct 2009 04:31:08 +0000 Subject: Bug 514913: Eliminate ssl="authenticated sessions" Patch by Max Kanat-Alexander r=dkl, a=mkanat --- Bugzilla/Auth/Login/CGI.pm | 11 ------- Bugzilla/Auth/Persist/Cookie.pm | 8 ++--- Bugzilla/CGI.pm | 43 +++++++++++++------------- Bugzilla/Config.pm | 5 +++ Bugzilla/Config/Core.pm | 7 ++--- Bugzilla/Mailer.pm | 5 +-- Bugzilla/Util.pm | 68 +++++++++++------------------------------ Bugzilla/WebService/Server.pm | 14 --------- 8 files changed, 52 insertions(+), 109 deletions(-) (limited to 'Bugzilla') 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 +=item C -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 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 +instead of calling this directly. =item C 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 Returns either the C or C parameter, depending on the -current setting for the C parameter. +current setting for the C parameter. =item C 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; -- cgit v1.2.3-24-g4f1b