428659 - Setting SSL param to 'authenticated sessions' only protects logins and param doesn't protect WebService calls at all

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Bradley Baetz (:bbaetz)]

To reproduce:

Set 'ssl' to 'authenticated sessions'
Log in
Go to http://foo/bugzilla/show_bug.cgi?id=1

No redirection to https

Filing as security but remove if you disagree.

Also, it doesn't protect the webservice User.login call....

Broken in 3.0; haven't checked further back

Comment 1

[Marc Schumann [:Wurblzap]]

Instead, it unnecessarily redirects http pages with the login form to their https sisters...

Comment 2

[Marc Schumann [:Wurblzap]]

I didn't mean to remove the sec flag. This seems to be a Mylyn bug.

Comment 3

[Frédéric Buclin]

Not a security bug.

Comment 4

[Frédéric Buclin]

Byron, as I understand it, as soon as you are logged in, all pages should use sslbase instead of urlbase when ssl eq 'authenticated sessions', but your code in Bugzilla::CGI::new() doesn't enforce this:

    # Redirect to SSL if required
    if (Bugzilla->params->{'sslbase'} ne ''
        && Bugzilla->params->{'ssl'} eq 'always'
        && i_am_cgi())
    {
        $self->require_https(Bugzilla->params->{'sslbase'});
    }

It only enforces https if ssl eq 'always'. Maybe it should be

 ssl eq 'always' || (ssl eq 'authenticated sessions' && Bugzilla->user->id)

to really do what it's supposed to do. What do you think?

Comment 5

[:glob ✱]

frédéric's fix looks correct, except i'm not sure if it'll cover the rpc calls.

Comment 6

[Frédéric Buclin]

OK, setting blocking+ so that we are sure the correct behavior is discussed before we release 3.2. justdave, mkanat, myk, what would you expect as being the correct behavior? I think my comment 4 makes sense, i.e. enforce to redirect to https: if the user is logged in.

Byron, what's wrong with the RPC calls? They do not call templates at all.

Comment 7

[Max Kanat-Alexander]

  Anything that sends a cookie or authentication information should be protected by "authenticated sessions", yes. If something isn't currently being protected and it sends a cookie or auth info, that needs to be fixed.

Comment 8

[Bradley Baetz (:bbaetz)]

The xml rpc wrapper sub doesn't check for this param at the moment.

Comment 9

[:glob ✱]

(In reply to comment #8)
> The xml rpc wrapper sub doesn't check for this param at the moment.

yes, that's what i meant.  if this param is set, i'd expect xml-rpc calls to also require ssl.

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v1)

Here is a first crack at solving this issue. I am overidding handle() in Bugzilla::WebService::XMLRPC::Transport::HTTP::CGI and checking for SERVER_PORT != 443 and if ssl is being enforced and then it issues a redirect to https://<sslbase>/xmlrpc.cgi.

Works so far for me in my testing.

Please take a look.
Dave

Comment 11

[Max Kanat-Alexander]

Comment on attachment 320054 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v1)

>Index: Bugzilla/WebService.pm
>+    # Redirect to SSL if required
>+    if ($ENV{SERVER_PORT} != 443

  Theoretically people can run SSL on other ports.

>+            && Bugzilla->params->{'sslbase'} ne '' 
>+                && Bugzilla->params->{'ssl'} eq 'always' 
>+                    || (Bugzilla->params->{'ssl'} eq 'authenticated sessions' && Bugzilla->user->id)) {
>+       
>+        my $headers = HTTP::Headers->new( Status => 301, 
>+                                          Location => Bugzilla->params->{'sslbase'} . "xmlrpc.cgi", 
>+                                          NPH => 1 );
>+        binmode(STDOUT);
>+        print STDOUT $headers->as_string . "\015\012\015\012";

  You could just require Bugzilla::CGI inside of here and do the redirect that way, or just call enforce_ssl on the $cgi object or something (if we have a method like that--we probably should if we don't).

Comment 12

[David Lawrence [:dkl]]

(In reply to comment #11)
> >+    if ($ENV{SERVER_PORT} != 443
> 
>   Theoretically people can run SSL on other ports.
> 

There is also $ENV{SERVER_URI} which is the full hostname.

if ($ENV{SERVER_URI} =~ /^https/) { }

Does that look like a better solution?

> >+        my $headers = HTTP::Headers->new( Status => 301, 
> >+                                          Location => Bugzilla->params->{'sslbase'} . "xmlrpc.cgi", 
> >+                                          NPH => 1 );
> >+        binmode(STDOUT);
> >+        print STDOUT $headers->as_string . "\015\012\015\012";
> 
>   You could just require Bugzilla::CGI inside of here and do the redirect that
> way, or just call enforce_ssl on the $cgi object or something (if we have a
> method like that--we probably should if we don't).

I will submit a revised patch using Bugzilla::CGI.

Dave

Comment 13

[:glob ✱]

if (uc($ENV{HTTPS}) eq 'ON' || $ENV{'SERVER_PORT'} == 443) { }

that said, see if you can do
  Bugzilla->cgi->require_https(Bugzilla->params->{'sslbase'});

perhaps the params check should go into require_https()?

Comment 14

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v2)

Revised patch to redirect SSL for XMLRPC requests.

Bugzilla::WebService::handle() is now calling Bugzilla::CGI::require_https() instead of printing the headers directly. require_https() had to be revised
to allow overriding the query and status values since XMLRPC requests need it
to be handled differently than standard CGI requests. I also had to update
Bugzilla::Util::i_am_cgi() to return 0 if SCRIPT_NAME =~ xmlrpc.cgi since we do not want Bugzilla::CGI::new to redirect automatically.

Please let me know if this looks better.

Dave

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v3)

I have created a revised patch that no longer looks for xmlrpc.cgi in Bugzilla::Util::i_am_cgi() and instead looks at Bugzilla->usage(). This was suggested by mkanat in IRC. I also no longer pass in args to Bugzilla::CGI::require_https() and instead also look at Bugzilla->usage() to determine status code and whether to form a proper query URL. 

Another thing mkanat suggested was embedding the https check in the on_action() sub in xmlrpc.cgi. I was unable to get it to work properly using that method and may need someone to help me debug why since it requires digging into the SOAP::Lite internals. I was able to get it to work properly overriding handle() in Bugzilla::WebService.pm. Also the on_action() is more for modifying SOAP headers rather than use for XMLRPC according to the comments, so maybe the reason it doesn't work well.

Please take a look at this new version and comment.

Dave

Comment 16

[Frédéric Buclin]

Comment on attachment 325412 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v3)

Sorry, but you don't fix the primary issue at all, see my comment 4.

Comment 17

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v4)

Revised patch to address primary issue in comment 4 in additional to redirect for XMLRPC requests.

Please review
Thanks
Dave

Comment 18

[Max Kanat-Alexander]

Comment on attachment 326965 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v4)

  There's one thing I'm concerned about:

> sub i_am_cgi {
>     # I use SERVER_SOFTWARE because it's required to be
>     # defined for all requests in the CGI spec.
>-    return exists $ENV{'SERVER_SOFTWARE'} ? 1 : 0;
>+    return exists $ENV{'SERVER_SOFTWARE'} && Bugzilla->usage_mode != USAGE_MODE_WEBSERVICE ? 1 : 0;

  i_am_cgi is used in Bugzilla::init_page, before Bugzilla->usage_mode could ever possibly be set, which is very confusing. I don't think this modification should be made here.

Comment 19

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v5)

New patch:

1. Changed Bugzilla::Util::i_am_cgi() back to before since checking for Bugzilla->usage_mode will not work in Bugzilla::init_page.

2. No longer call Bugzilla->cgi->require_https() directly in Bugzilla::WebService::handle() and instead create an instance of Bugzilla->cgi which will do the redirecto automatically in the new() constructor.

Tested and works as expected.
Please review.

Dave

Comment 20

[Max Kanat-Alexander]

Comment on attachment 327419 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v5)

That looks good to me!

Comment 21

[Max Kanat-Alexander]

Comment on attachment 327419 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v5)

Oh, I just realized that we also need to protect plaintext calls to User.login. Although their login info will be transmitted in the clear before the call, at least their cookies will come back on an encrypted connection.

Comment 22

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v6)

Similar to v5 of patch except now WebService::User::login() will throw an error if it is called over non-SSL connection when server is requiring SSL always or for authenticated_sessions.

Please review
Thanks
Dave

Comment 23

[Max Kanat-Alexander]

Comment on attachment 327840 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v6)

Two things:

We've got that code all over the place now for checking if we're in https, so let's centralize it somewhere.

The error can be more explicit--this installation requires you to call User.login using an SSL connection.

Comment 24

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v7)

New patch that creates new Util.pm functions in_ssl_connection() and require_ssl(). 

1. in_ssl_connection() simply checks to see if current request is over secure connection.
2. require_ssl() checks to see if we should be using a secure connection for the request based on current server settings and also is user is logged in or not. It takes an extra parameter that says to check for user is not logged in if coming from methods such as User.login. Otherwise it checks is user is logged in if 'ssl' == 'authenticated_sessions'.

Note: I am pretty certain that the (Bugzilla->params->{ssl} eq 'authenticated_sessions' && Bugzilla->user->id) will always be false since the on_action() subroutine Bugzilla::WebService::handle_login() is running after we do the ssl check in Bugzilla::WebService::handle(). A different bug?

Max noted that he wants as little duplication of code as possible so I may attach a new patch tomorrow that combines in_ssl_connection() and require_ssl() into a single function that then redirects if needed or issues the 'ssl_require' error if coming from User.login. But will wait to see what people think so far first.

Please review
Dave

Comment 25

[Max Kanat-Alexander]

Comment on attachment 327864 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v7)

This is missing the actual implementation of in_ssl_connection and require_ssl.

Since we always use both of those functions together, let's merge them into one function called ssl_required() or ssl_redirect_required().

Comment 26

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v8)

Okay, let's try this one more time.

New patch for review.

1. Created Bugzilla::Util::ssl_require_redirect() that will return true/false based on if a redirect is required. I do not do the redirect in the function itself as this caused deep recursion in the Bugzilla::CGI::new constructor.
Also it takes a optional argument which is the XMLRPC method name when using under USAGE_MODE_WEBSERVICE. It will return true it the case the method matches User.login() and the ssl eq 'authenticated_sessions' and the user is not logged in already. This alleviates the need to throw a ssl_required error. Although I can add that back in if this is not satisfactory.

2. I created a new on_action handler called Bugzilla::WebService::handle_redirect() that is called after Bugzilla::WebService::handle_login() so that Bugzilla->user->id has time to be set. Also the on_action handlers know the method name being called so it is easier to do the User.login check in ssl_require_redirect().

3. I had to add $| = 1 to xmlrpc.cgi as when doing a redirect in a on_action handler it will generate a SOAP error instead of the proper redirect if buffering is enabled which causes it to happen too late. Hopefully the change will not cause any issues and has worked fine for me so far in my testing.

4. I have updated in other places in the code that need to check ssl_require_redirect() instead of checking the values of 'ssl' directly.

Please let me know if this is a better solution.

Thanks
Dave

Comment 27

[Max Kanat-Alexander]

Comment on attachment 328362 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v8)

  Yes, I like this much better! A few comments:

>+# Disable buffering
>+$|++;

  I would rather see that changed more locally to where it needs to be changed. I don't want every xmlrpc call to be unbuffered. You're free to call flush on STDOUT if you need to, too.

>+                    Bugzilla::WebService::handle_redirect($uri . "." . $method);

  Wouldn't it be simpler to pass them in as two separate variables?

>Index: Bugzilla/CGI.pm
>+    if (i_am_cgi() && Bugzilla->usage_mode != USAGE_MODE_WEBSERVICE 
>+        && ssl_require_redirect()) {

  Nit: opening brace goes on the next line aligned with "if"

>+    # Do not create query string if data submitted via XMLRPC
>+    my $query = Bugzilla->usage_mode == USAGE_MODE_WEBSERVICE ? 0 : 1;
>+    # XMLRPC clients (SOAP::Lite at least) requires 301 to redirect properly
>+    my $status = Bugzilla->usage_mode == USAGE_MODE_WEBSERVICE ? 301 : 302;
>+    if (defined $url) {

  Nit: Those vars could be inside of the defined $url block, no?

>Index: Bugzilla/Util.pm
>+sub ssl_require_redirect {
>+    my $method = shift;
>+
>+    # Redirect to SSL if required.
>+    if (!(uc($ENV{HTTPS}) eq 'ON' || $ENV{'SERVER_PORT'} == 443)
>+        && Bugzilla->params->{'sslbase'} ne '') {

  Nit: brace

>+        if (Bugzilla->params->{'ssl'} eq 'always'
>+            || (Bugzilla->params->{'ssl'} eq 'authenticated_sessions'
>+                && Bugzilla->user->id)
>+            || (Bugzilla->params->{'ssl'} eq 'authenticated_sessions'
>+                && !Bugzilla->user->id && $method eq 'User.login')) {

  Nit: brace

Comment 28

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v9)

New patch with Max's suggested changes. Removed the $|++ buffering fix in xmlrpc.cgi and made it local to the Bugzilla::WebService::handle_redirect() function.

Please review
Dave

Comment 29

[David Lawrence [:dkl]]

Ok. May be my lack of understanding about some mod_perl'isms but some of the following methods work and some do not as for flushing the buffer.

Works:

use IO::Handle;
STDOUT->autoflush(1); # In Bugzilla::WebService::handle_redirect() and Bugzilla::CGI::require_https()

$| = 1; # In the top of xmlrpc.cgi

local $| = 1; # In the top of xmlrpc.cgi

$| = 1; # In Bugzilla::WebService::handle_redirect()

$| = 1; # In Bugzilla::CGI::require_https();

select((select(STDOUT), $| = 1)[0]); # Works in either Bugzilla::WebService::handle_redirect() or Bugzilla::CGI::require_https()


No work:

local $| = 1; # No work in Bugzilla::WebService::handle_redirect() or Bugzilla::CGI::require_https()

STDOUT->printflush($self->redirect()); # In Bugzilla::CGI::require_https()
Doesn't seem to print anything by itself.

STDOUT->flush(); # Anywhere, no effect whatsoever. Looking in the code for IO::Handle, I do not even see it implemented. Maybe I am missing something.

Max mentioned trying to use mod_perl functions when running under MOD_PERL so I may try some of those. 

Anyone have any clues? 

Dave

Comment 30

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v10)

Interestingly, the redirect works properly when adding a newline character after the print statement in Bugzilla::CGI::require_https(). I know that Perl will autoflush a printed line when it gets a new line character when writing to a terminal but also seems to fix the issue here as well. no $|=1 required inside the functions.

Please let me know if this seems sane to you. All my test cases seem to work with the simple new line addition.

Dave

Comment 31

[Max Kanat-Alexander]

Comment on attachment 328440 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v10)

Yeah, that sounds great, but this patch doesn't have that! You have a local $| in this one.

Comment 32

[David Lawrence [:dkl]]

(In reply to comment #31)
> (From update of attachment 328440 [details] [diff] [review])
> Yeah, that sounds great, but this patch doesn't have that! You have a local $|
> in this one.
> 

Oops, wrong patch. Right one coming.

Comment 33

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v11)

Ok, once more. This time the local $| statement is removed and the new line is added after redirect. Please test if you can.

Dave

Comment 34

[Max Kanat-Alexander]

Comment on attachment 328545 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v11)

If this works, it all looks fine to me.

Comment 35

[David Lawrence [:dkl]]

Attachment: Patch to redirect XMLRPC requests to https if ssl is enforced (v12)

Fixed typo error with last patch, checks for 'authenticated_sessions' changed to 'authentication sessions'. The '_' character was mistakenly added to my last patch. After fixing everything worked properly again.

Carrying over prior r=mkanat and checking in this patch.

Dave

Comment 36

[David Lawrence [:dkl]]

3.2:
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.24.2.1; previous revision: 1.24
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.54.2.1; previous revision: 1.54
done
Checking in xmlrpc.cgi;
/cvsroot/mozilla/webtools/bugzilla/xmlrpc.cgi,v  <--  xmlrpc.cgi
new revision: 1.6.2.1; previous revision: 1.6
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.36.2.1; previous revision: 1.36
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.69.2.1; previous revision: 1.69
done
Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.9.2.1; previous revision: 1.9
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.8.2.1; previous revision: 1.8
done

tip:
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.25; previous revision: 1.24
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.55; previous revision: 1.54
done
Checking in xmlrpc.cgi;
/cvsroot/mozilla/webtools/bugzilla/xmlrpc.cgi,v  <--  xmlrpc.cgi
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.37; previous revision: 1.36
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.70; previous revision: 1.69
done
Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.9; previous revision: 1.8
done

Comment 37

[Frédéric Buclin]

I think we should take the minimal fix suggested in comment 4 for 3.0.5; what do you think?

Comment 38

[Max Kanat-Alexander]

(In reply to comment #37)
> I think we should take the minimal fix suggested in comment 4 for 3.0.5; what
> do you think?

  Yeah, that sounds good.

Comment 39

[David Lawrence [:dkl]]

Attachment: Patch to redirect authenticated sessions to https if ssl is enforced for 3.0.5 (v1)

Patch for 3.0.5 that just addresses request from comment #4 which will cause a redirect if ssl equals 'authenticated sessions' and user is logged in.

Please review
Dave

Comment 40

[Frédéric Buclin]

The patch on tip doesn't work. I'm visiting admin pages using http:// despite I'm logged in and ssl = authenticated sessions. It doesn't try to redirect me to https://.

Comment 41

[Frédéric Buclin]

Even worse, index.cgi no longer redirects to https://. dkl thinks that's because the web browser is unable to see the cookie. I'm going to back out this patch is a fix is not found quickly as the index.cgi bug is a regression.

Comment 42

[Frédéric Buclin]

Comment on attachment 328819 [details] [diff] [review]
Patch to redirect XMLRPC requests to https if ssl is enforced (v12)

>Index: index.cgi

>-if (Bugzilla->params->{'sslbase'} ne '' and Bugzilla->params->{'ssl'} ne 'never') {
>-    $cgi->require_https(Bugzilla->params->{'sslbase'});
>-}
>+$cgi->require_https(Bugzilla->params->{'sslbase'}) 
>+    if ssl_require_redirect();

Wait! This cannot be correct. At that point, the user may be logged out, ssl set to "authentication sessions", but we still want the page to use https://, because that's a login page and we don't want the credentials to be passed unencrypted.



>Index: token.cgi

>-    if (Bugzilla->params->{'sslbase'} ne ''
>-        && Bugzilla->params->{'ssl'} ne 'never')
>-    {
>-        $cgi->require_https(Bugzilla->params->{'sslbase'});
>-    }
>+    Bugzilla->cgi->require_https(Bugzilla->params->{'sslbase'}) 
>+        if ssl_require_redirect();

Same problem here!


As there are several unrelated problems, this patch should be backed out, both on tip and 3.2. dkl, can you do it asap?

Comment 43

[David Lawrence [:dkl]]

Backing out these patches as they cause a regression.

New file revision information:

3.2:
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.24.2.2; previous revision: 1.24.2.1
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.54.2.2; previous revision: 1.54.2.1
done
Checking in xmlrpc.cgi;
/cvsroot/mozilla/webtools/bugzilla/xmlrpc.cgi,v  <--  xmlrpc.cgi
new revision: 1.6.2.2; previous revision: 1.6.2.1
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.36.2.3; previous revision: 1.36.2.2
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.69.2.2; previous revision: 1.69.2.1
done
Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.9.2.2; previous revision: 1.9.2.1
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.8.2.2; previous revision: 1.8.2.1
done

tip:
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.26; previous revision: 1.25
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.56; previous revision: 1.55
done
Checking in xmlrpc.cgi;
/cvsroot/mozilla/webtools/bugzilla/xmlrpc.cgi,v  <--  xmlrpc.cgi
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.39; previous revision: 1.38
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.71; previous revision: 1.70
done
Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.10; previous revision: 1.9
done

Comment 44

[David Lawrence [:dkl]]

After discussion with LpSolit on IRC, I'm going to need to update my patch :)

During my initial testing, I must have had multiple cookies (secure and
non-secure set) which gave false positives when testing for redirect when ssl
eq 'authenticated sessions'. 

After more discussion this is not going to work for authenticated sessions as
Bugzilla->user->id is not set before Bugzilla->login() is called. 

So this will need to be updated to move the redirect to the end of login() in
Bugzilla.pm. This will make sure that Bugzilla->user->id is properly set and
then make the choice to redirect or not.

Also LpSolit brought up that index.cgi needs to always redirect if ssl ne
'never' since it has a login form in the page which would allow the
username/password to be passed unprotected. We need to do the same for any page
that is called due to the GoAheadAndLogIn variable being set to true as well.

I will not likely be able to work on this til next week due to our changeover
this weekend to 3.2 but will see what I can do to get an updated patch in for
review.

Dave

Comment 45

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v13)

New modified patch to address issues pointed out during current discussions which caused a backing out of the previous patch.

1. The redirect check has been moved from Bugzilla::CGI->new() to Bugzilla->login() so we can make the decision to redirect after we have validated the user's credentials.
2. index.cgi, token.cgi, and Bugzilla/Auth/Login/CGI.pm have been reverted back to previous behaviour of redirecting if 'ssl' not equal to 'never' reqardless if we are in an authenticated session or not.
3. Added in the mod_perl fix from bug 445104 which was a bug in this patch originally anyway. Will close that bug when this is checked in.

Please review.
Dave

Comment 46

[Frédéric Buclin]

Comment on attachment 331650 [details] [diff] [review]
Patch to redirect requests to https if ssl is enforced (v13)

>Index: Bugzilla/Util.pm

>+sub ssl_require_redirect {

>+            || (defined $method && $method eq 'User.login'
>+                && Bugzilla->params->{'ssl'} eq 'authenticated sessions'
>+                    && !Bugzilla->user->id)) 

Why do you check !Bugzilla->user->id? Shouldn't we redirect even if the user is already logged in?

Comment 47

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v14)

Yes you are right. Checking for !Bugzilla->user->id when $method eq 'User.login' is not needed since we will need to redirect regardless when 'ssl' equals 'authenticated sessions'.

Attaching updated patch.
Dave

Comment 48

[Max Kanat-Alexander]

Quick question--if you're moving this to Bugzilla->login, why not put the 'always' check inside of Bugzilla::init_page?

Comment 49

[David Lawrence [:dkl]]

(In reply to comment #48)
> Quick question--if you're moving this to Bugzilla->login, why not put the
> 'always' check inside of Bugzilla::init_page?
> 

Because this will break XMLRPC redirects when ssl equal always as CGI.pm will stomp on STDIN. Also Bugzilla->usage_mode is not set by xmlrpc.cgi yet when init_page() runs which means we can't skip the redirect if USAGE_MODE_WEBSERVICE.

Comment 50

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v15)

Updated patch:

1. Updated Bugzilla::Util::ssl_require_redirect to be more readable.
2. No longer need extra on_action in xmlrpc.cgi as we can do the redirect in Bugzilla::WebService::handle_login() now.

So basically it works this way:

1. Special cases, index.cgi, token.cgi, GoAheadAndLogIn will always do not call ssl_require_redirect() and will redirect if ssl is not equal to 'never'.
2. All others will redirect in Bugzilla->login() when it is called.
   Will redirect when:
   1. ssl equals 'always'
   2. ssl equals 'authenticated sessions' and user is logged in either with     cookies or with a username/password pair.
   3. For XMLRPC additionally it will redirect when ssl equals 'authenticated sessions' and method called is 'User.login()' regardless if the user is logged in or not.

Please review, hopefully this is it ;)

Dave

Comment 51

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v16)

Sigh, variable typo error in Bugzilla::WebService::handle_login(). Attaching a fixed patch.

/me drinks more coffee

Comment 52

[Frédéric Buclin]

Comment on attachment 331743 [details] [diff] [review]
Patch to redirect requests to https if ssl is enforced (v16)

>Index: Bugzilla/CGI.pm

>-    # Redirect to SSL if required

The only thing which annoys me here is that if you never call Bugzilla->login in the page (xml.cgi, showattachment.cgi and long_list.cgi), Bugzilla->cgi will no longer redirect you if ssl eq 'always'. Maybe that's not too critical as they all three redirect to show_bug.cgi or attachment.cgi, which both call Bugzilla->login. So redirection to https:// would happen anyway.



>Index: Bugzilla/Util.pm

>+sub ssl_require_redirect {

>+    if (!(uc($ENV{HTTPS}) eq 'ON' || $ENV{'SERVER_PORT'} == 443)) {
>+        if (Bugzilla->params->{'sslbase'} ne '') {

Nit: this would be more readable as:

    if (uc($ENV{HTTPS}) ne 'ON'
        && $ENV{'SERVER_PORT'} != 443
        && Bugzilla->params->{'sslbase'} ne '')
    {


>+            # Return true if ssl set to 'always'
>+            return 1
>+                if Bugzilla->params->{'ssl'} eq 'always';

Nit: you could add:

    return 0 if Bugzilla->params->{'ssl'} eq 'never';

before this line. This would make the code below cleaner:

>+            return 1
>+                if Bugzilla->params->{'ssl'} eq 'authenticated sessions'
>+                    && Bugzilla->user->id;
>+
>+            return 1
>+                if Bugzilla->params->{'ssl'} eq 'authenticated sessions'
>+                    && defined $method && $method eq 'User.login';

With my suggested change above, both could be combined into a single:

    return 1 if (Bugzilla->user->id || ($method && $method eq 'User.login'));

because you already know that Bugzilla->params->{'ssl'} eq 'authenticated sessions' if you came here.



>Index: Bugzilla/WebService.pm

>+    my $method = $uri . "." . $method;

$method is already defined, so |my| must go away.


Otherwise, it looks great! I didn't test your patch, so I'm not setting the flag to review+ (yet! Tell me if you want me to test your patch, maybe after mkanat's review).

Comment 53

[Frédéric Buclin]

Comment on attachment 331743 [details] [diff] [review]
Patch to redirect requests to https if ssl is enforced (v16)

Oh, wait! Where is the change in Bugzilla.pm to do the redirect when Bugzilla->login is called?

Comment 54

[Frédéric Buclin]

Comment on attachment 331192 [details] [diff] [review]
Patch to redirect authenticated sessions to https if ssl is enforced for 3.0.5 (v1)

As discussed on IRC, this won't work as the user ID is not yet available (Bugzilla->login has not been called yet).

Comment 55

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v17)

Let's try this again :)

New patch attached with LpSolits requested changes.

(In reply to comment #52)
> (From update of attachment 331743 [details] [diff] [review])
> >Index: Bugzilla/CGI.pm
> 
> >-    # Redirect to SSL if required
> 
> The only thing which annoys me here is that if you never call Bugzilla->login
> in the page (xml.cgi, showattachment.cgi and long_list.cgi), Bugzilla->cgi will
> no longer redirect you if ssl eq 'always'. Maybe that's not too critical as
> they all three redirect to show_bug.cgi or attachment.cgi, which both call
> Bugzilla->login. So redirection to https:// would happen anyway.

I agree that it is not necessary as you will redirect to another page that does the ssl check. If they didn't we could add the same lines from index.cgi to them as well I suppose.


Please review

Dave

Comment 56

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v18)

Updated comments in certain places to better describe what is happening. 

Please review.
Dave

Comment 57

[Max Kanat-Alexander]

Comment on attachment 331831 [details] [diff] [review]
Patch to redirect requests to https if ssl is enforced (v18)

This is fine, but a note on the comments:

You never need comments to explain *what* the code is doing, unless it's REALLY not obvious. The "return true if" comments just duplicate what the code says exactly below them. The "Redirect to SSL if" comments are the same.

Comments should say *why*, not *what*. So the "why" parts of the comments are all good, but the "what" parts are all unnecessary.

Comment 58

[Max Kanat-Alexander]

Holding approval until:

1) I'm sure this has been tested adequately.
2) LpSolit says whether he wants it before or after QA for RC1.

Comment 59

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v19)

Yeah good comments have never been my strong point unfortunately ;) I have attached an updated patch that hopefully explains things somewhat better then before. Please take a look.

Thanks
Dave

Comment 60

[Max Kanat-Alexander]

Comment on attachment 331962 [details] [diff] [review]
Patch to redirect requests to https if ssl is enforced (v19)

>Index: Bugzilla.pm
>+    # Redirect to SSL if we are cgi script and 
>+    # Bugzilla->params->{ssl} is not set to never.

 These lines can be deleted.

>+    # We run after the login has completed since
>+    # some after the checks in ssl_require_redirect

  "some after"?

>Index: token.cgi
>+    # Redirect to SSL if Bugzilla->params->{ssl} not equal to never.

  Unnecessary.

  (Same comments throughout.)

Comment 61

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v20)

Applied Max's suggested changes. Please review.

Dave

Comment 62

[Frédéric Buclin]

(In reply to comment #43)
> Backing out these patches as they cause a regression.

Technical question: how did you do the backout?

cvs diff -r 1.36 -r 1.36.2.3 Bugzilla/CGI.pm shows that CGI.pm has not been reverted correctly. The indentation is off by one character between both versions of the file. The current file has a wrong indentation in require_https(): 5 characters instead of 4!
cvs diff -r 1.36 -r 1.39 Bugzilla/CGI.pm shows no difference, though, so the problem described above only affects 3.2 RC1. And this also explains why your patch doesn't apply cleanly on both tip and 3.2.

Comment 63

[Frédéric Buclin]

Comment on attachment 333290 [details] [diff] [review]
Patch to redirect requests to https if ssl is enforced (v20)

>+if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
>+    && Bugzilla->params->{'ssl'} ne 'never')

Shouldn't you define a function for this check? This code appears 3 times, so it would be better to have it in a central place. Something similar to ssl_require_redirect. Not sure if the function should be in CGI.pm or Util.pm. Probably CGI.pm as it's related to it.



>Index: Bugzilla/CGI.pm

>+     # When using XML-RPC with mod_perl, we need the headers sent immediately.
>+     # We used to do this by appending a newline to $self->redirect, but
>+     # that breaks normal web browser redirects.

I don't understand this comment at all. Why "We used to"? As you backed out the patch, this is no longer relevant. I think the last 2 lines should go away. Nobody will ever understand what this newline is about.



> This routine checks if the current page is being served over https, and
> redirects to the https protocol if required, retaining QUERY_STRING.

This description in the POD of require_https is no longer true. This routine no longer checks anything, redirect to HTTPS in *all cases* (rather than "if required") and only retain QUERY_STRING when not called from XML-RPC.


I will test your patch in a few hours, but the code itself looks good.

Comment 64

[Frédéric Buclin]

Comment on attachment 333290 [details] [diff] [review]
Patch to redirect requests to https if ssl is enforced (v20)

I tested your patch on 3.2 RC1 and it seems to work fine, both from a web browser and when using XML-RPC. r=LpSolit (with the POD fixed as written in my previous comment).

Unrelated to this patch, but do you also get your web server log full of:

[error] unusably short session_id provided (0 bytes)
[client 127.0.0.1] xmlrpc.cgi: Use of uninitialized value in pattern match (m//) at /usr/lib/perl5/vendor_perl/5.8.8/SOAP/Transport/HTTP.pm line 411. ?

Comment 65

[David Lawrence [:dkl]]

(In reply to comment #63)
> (From update of attachment 333290 [details] [diff] [review])
> >+if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
> >+    && Bugzilla->params->{'ssl'} ne 'never')
> 
> Shouldn't you define a function for this check? This code appears 3 times, so
> it would be better to have it in a central place. Something similar to
> ssl_require_redirect. Not sure if the function should be in CGI.pm or Util.pm.
> Probably CGI.pm as it's related to it.

Possibly, but I request that we address this in a separate bug as this is not really different from what we were doing before. I agree it should somehow be optimized though later.
 
> 
> >+     # When using XML-RPC with mod_perl, we need the headers sent immediately.
> >+     # We used to do this by appending a newline to $self->redirect, but
> >+     # that breaks normal web browser redirects.
> 
> I don't understand this comment at all. Why "We used to"? As you backed out the
> patch, this is no longer relevant. I think the last 2 lines should go away.
> Nobody will ever understand what this newline is about.
> 

Fixed.

> 
> 
> > This routine checks if the current page is being served over https, and
> > redirects to the https protocol if required, retaining QUERY_STRING.
> 
> This description in the POD of require_https is no longer true. This routine no
> longer checks anything, redirect to HTTPS in *all cases* (rather than "if
> required") and only retain QUERY_STRING when not called from XML-RPC.
> 
> 

Fixed.

Comment 66

[David Lawrence [:dkl]]

Attachment: Patch to redirect requests to https if ssl is enforced (v21)

Patch committed with POD fixes. Review carried over from v20. Checking in.

Comment 67

[David Lawrence [:dkl]]

3.2:
Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.65.2.1; previous revision: 1.65
done
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.24.2.3; previous revision: 1.24.2.2
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.54.2.3; previous revision: 1.54.2.2
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.36.2.4; previous revision: 1.36.2.3
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.69.2.3; previous revision: 1.69.2.2
done
Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.9.2.3; previous revision: 1.9.2.2
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.8.2.3; previous revision: 1.8.2.2
done

tip:
Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.66; previous revision: 1.65
done
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.27; previous revision: 1.26
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.57; previous revision: 1.56
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.40; previous revision: 1.39
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.72; previous revision: 1.71
done
Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.13; previous revision: 1.12
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.11; previous revision: 1.10
done

Comment 68

[Frédéric Buclin]

Added to release notes for 3.2 in bug 463244.