260682 - Support redirecting to HTTPS always or for authenticated sessions only

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[:glob ✱]

Bugzilla should support redirecting to https for authentication only.

This would take strain off the server, and allow for scripts that access
bugzilla bugs (bots, etc) that don't/can't support the ssl redirect.

Comment 1

[:glob ✱]

we'll also have to force ssl for secure bugs, and buglists that contain secure bugs.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

... plus private attachments and comments, and probably all pages for logged in
users (to secure the login token, even though IP spoofing is hard, so this is a
relatively minor concern).  I think we're better off securing everything than
writing and maintaining complex code for determining what needs securing and
running the risk of bugs in that code or missing something.  I recommend we
don't implement this feature (i.e. mark it WONTFIX).

Comment 3

[:glob ✱]

how about allowing anonymous http access, but once you log in, require https?

that'll cover everything that should be secured with simple logic.

Comment 4

[Stephen Lee]

I would agree with comment 3. It is trivial to determine whether or not a user 
is logged in.

Changes could be kept local to the login code, which could either redirect or 
simply set the form target to the HTTPS page.

As new login cookies would only be issued over SSL, the browser would 
presumably only submit them via SSL, and the only additional action needed on 
the server would be to delete any old cookies that had been used on non-secure 
HTTP previously.

Comment 5

[:glob ✱]

Attachment: force sessions to https

Comment 6

[:glob ✱]

Attachment: force sessions to https v2

use $cgi->self_url

Comment 7

[:glob ✱]

Comment on attachment 160342 [details] [diff] [review]
force sessions to https v2

thinking about this last night, if you enable this parameter and don't have
https configured, you won't be able to log in again to disable ssl.

i'll add a sanity check when you enable the parameter.

Comment 8

[:glob ✱]

Attachment: force sessions to https v3

adds a sanity check to ensure that port 443 is open before setting the
parameter.

Comment 9

[Vlad Dascalu]

Comment on attachment 160428 [details] [diff] [review]
force sessions to https v3

+	 my $host = $ENV{SERVER_NAME} || 'localhost';
+	 trick_taint($host);

This is highly unprobable, but $ENV{SERVER_NAME} could be something like
javascript:alert() or something like that. Since we're displaying it later in
the code, I guess it's good practice to validate it againest some pattern or
something.

+   desc => 'If this is on, Bugzilla will force authenticated sessions to ' .
+	    'use https instead of http.',

Some installations have Bugzilla changed to IssueZilla or something similar.
Since we don't have access to template vars in the param descriptions, we could
rephrase that to something that excludes the Bugzilla word, like:

"If this is on, authenticated sessions will be forced to use https instead of
http."


Anyway, both of these are nits. The reason for review- is that IIS sets the
HTTPS env variable to "off" instead of null. I'm not sure if we support IIS out
of the box now, but to add the case where $ENV{'HTTPS'} is off instead of null
would be trivial. See http://www.geocities.com/herong_yang/perl/cgi.html for an
example of IIS ENV variables, including HTTPS.

Comment 10

[:glob ✱]

Attachment: force sessions to https v4

thanks for the review vlad.

this revision..

html_quote's $host, which will protect us from cross site scripting.  i started
looking at validation of the host, but i wasn't sure about how to deal with
idn's.

changes description to not include "bugzilla" (although almost every other
parameter's description does) :)

checks for $ENV{HTTPS} eq 'off'

Comment 11

[Vlad Dascalu]

Comment on attachment 160448 [details] [diff] [review]
force sessions to https v4

Sorry for not catching this the first time. Something bugged me but I couldn't
have put my finger on it.

It's better to write defensive code. There is an assumption implied in the
patch, which might not always be true. More specifically, we assume that a null
or "off" value for $ENV{'HTTPS'} implies disabled SSL. But there might be
webservers out there where this might not be true. Setting the URL to "https"
doesn't guarantee in 100% cases that the $ENV variable will get an "on" value.
There might be let's say 1% webservers out there  with this property. Then the
code would cause an infinite redirect loop, because each time starting with the
second time, the code would redirect to the same URL (that starts with https).

So the problem is the assumption that there is a correlation between the URL
starting with https and the ENV variable. The code would be more bullet-proof
if this assumption wouldn't be implied and the code wouldn't cause infinite
loops when the assumption is not true.

One easy way to fix this is to perform the redirect only if an actual change in
the $url takes place. Something like:

+	     if ($url =~ s/^http:/https:/i) {
+		 print $cgi->redirect(-location => $url);
+		 exit;
+	     }

That way, if the URL already starts with https (but the $ENV variable still
doesn't reflect the SSL mode), then the infinte loop doesn't happen.

Comment 12

[Vlad Dascalu]

Looks like v5 will be ready to go. By the way, have you tested this?

Comment 13

[:glob ✱]

Attachment: force sessions to https v5

here's v5 :)

i ended up going with ($cgi->protocol ne 'https') as i think it makes the
intent of the code clearer.

for what it's worth, $cgi->protocol basically does this:

  return 'https' if uc($ENV{HTTPS}) eq 'ON'; 
  return 'https' if $ENV{'SERVER_PORT'} == 443;
  my($protocol,$version) = split('/', $ENV{'SERVER_PROTOCOL'}); # eg HTTP/1.0
  return lc($protocol);

i also realised that if you enable ssl, any existing login cookies against http
will not be redirected to https, so there's a new check in
Bugzilla::Auth::Login::WWW.


yes, i've tested this.

Comment 14

[Vlad Dascalu]

r=vladd if you put the redundant code inside one sub and call that sub. Or you
can wait a little and probably someone else will review+ the current version :)

Thanks for working on this!

Comment 15

[:glob ✱]

Attachment: force sessions to https v6

wasn't sure where the best place was for the sub, in the end Bugzilla::CGI.pm
won.

Comment 16

[Stephen Lee]

+   name => 'ssl',
+   desc => 'If this is on, authenticated sessions will be forced to use ' .
+           'https instead of http.',

...

+        my $url = $self->self_url;
+        $url =~ s/^http:/https:/i;

What if the server is accessible by more than one name, and SSL cert only 
applies to one of these?

Could something be done with urlbase? - e.g. if urlbase begins with https, and 
the server is also accessible by http, then this behaviour might be invoked?

Comment 17

[:glob ✱]

hrm, good point.

perhaps we need 'sslbase'.

Comment 18

[:glob ✱]

Attachment: force sessions to https v7

this revision:

  - adds "sslbase" parameter
  - changes "ssl" parameter from boolean to list, with following options:
      "never" ( same as off previously )
      "authenticated sessions" ( same as on previously )
      "always" ( non-authenticated session require ssl )

Comment 19

[Dave Miller [:justdave]]

I'm not sure I follow the Socket code...  what are we doing with Socket?

Comment 20

[:glob ✱]

it checks that we can connect to port 443 on the specified host.

otherwise if they enter an invalid hostname, once the the param has been saved,
they won't be able to return to the parameters screen to correct.

Comment 21

[Vlad Dascalu]

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

Comment 22

[Vlad Dascalu]

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.141; previous revision: 1.140
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/Auth/Login/WWW.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW.pm,v  <--  WWW.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/Auth/Login/WWW/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW/CGI.pm,v  <--  CGI.pm
new revision: 1.5; previous revision: 1.4
done