514913 - Eliminate ssl="authenticated sessions"

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Max Kanat-Alexander]

As discussed on the mailing list, we're going to eliminate the "authenticated sessions" and just have a single ssl_redirect parameter that redirects users to the sslbase if it's on.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, this seems to work all right in my testing. One of the nice things about this change is that redirects will happen much faster. Under mod_cgi, they will happen before anything else is compiled, and under mod_perl they'll happen in the StartHandler (or whatever it's called, before the response handler).

Comment 2

[Max Kanat-Alexander]

Oh, the reason that I asked dkl for review is that I'm having a problem when trying to access the http version of the webservice from the SOAP::Lite XMLRPCsh.pl client--it seems that the re-POST'ing isn't working, somehow, and I'm not sure why. I know that you worked on the xmlrpc SSL stuff--do you have any ideas?

Comment 3

[David Lawrence [:dkl]]

I can look at this patch. As for the broken redirect I have had to do the following in the past to fix the issue:

delete $ENV{CONTENT_TYPE};

I will try it in your patch and let you know how it goes.

Dave

Comment 4

[David Lawrence [:dkl]]

Ok, I wanted to try and find a workable solution before I commented fully on this. And now that I have I will comment on what I found out. 

Redirecting POSTed data without confirmation is not really a good thing to do normally and is a special exception for Bugzilla XMLRPC in that we need to do it transparently.

Reason:

"""
http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html:

If the 301 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued.

Note: When automatically redirecting a POST request after
receiving a 301 status code, some existing HTTP/1.0 user agents
will erroneously change it into a GET request.
"""

This is why we need to check for USAGE_MODE_XMLRPC in Bugzilla::CGI::require_https and set query => 0 otherwise CGI.pm will convert the POSTed data from STDIN to POSTDATA= URL in the redirect headers.

An issue with your patch is that the first time that require_https() is called in Bugzilla::init_page(), the usage mode is not set yet and there for query => 1. In my debugging, I manually set query => 0 and got a little farther in that 
CGI.pm not longer created the POSTDATA= URL string.

After that I was still getting failure since CGI.pm is still stomping on STDIN before SOAP::Lite could get a chance to read it for itself. So I would get the invalid XML error message back to the client. So the only way I have found so far to make this work in both the web browser and xmlrpc.cgi was in Bugzilla::init_page() I changed to the following:

    # If we are running in a web server, and ssl_redirect is on, we need to
    # do the redirect before doing anything else, and this is the ideal time.
    if (i_am_cgi() and basename($0) ne 'xmlrpc.cgi' and Bugzilla->params->{'ssl_redirect'}) {
        Bugzilla->cgi->require_https;
    } 

and then in Bugzilla::WebService::Server::handle_login() I added another redirect there:

sub handle_login {
    my ($self, $class, $method, $full_method) = @_;

    # If we are running in a web server, and ssl_redirect is on, we need to
    # do the redirect before doing anything else, and this is the ideal time.
    if (Bugzilla->params->{'ssl_redirect'}) {
        Bugzilla->cgi->require_https;
    }

    eval "require $class";
    return if $class->login_exempt($method);
    Bugzilla->login();
}

This now works in my testing with the above changes. xmlrpc.cgi will set Bugzilla->usage_mode to USAGE_MODE_XMLRPC before handle_login() is called so the require_https() call will set query => 0 properly.

So two problems:

1. Bugzilla->usage_mode() is not set when the first require_https() is called.
2. Bugzilla::CGI->new cannot be called before SOAP::Lite has had a chance to read in the POSTed data.

My solution above is ok but requires separate calls to require_https(). If that is an ok compromise we could leave it that way.

Dave

Comment 5

[Max Kanat-Alexander]

Attachment: v2

Okay, I did some looking into all of this, and it turns out that the problem was being caused by constructing a CGI object too early--I think it was messing with the POSTDATA or something and then SOAP::Lite didn't get things in the format it was expecting. So I've re-worked it to only create a CGI object if we're actually doing the redirect, and also not passing along ?POSTDATA= in the URL.

Comment 6

[David Lawrence [:dkl]]

(In reply to comment #5)
> Created an attachment (id=405002) [details]
> v2
> 
> Okay, I did some looking into all of this, and it turns out that the problem
> was being caused by constructing a CGI object too early--I think it was messing
> with the POSTDATA or something and then SOAP::Lite didn't get things in the
> format it was expecting. So I've re-worked it to only create a CGI object if
> we're actually doing the redirect, and also not passing along ?POSTDATA= in the
> URL.

Which is similar to my explanation in different words from comment #4 ;)

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Which is similar to my explanation in different words from comment #4 ;)

  Ah, so it is! :-) I read mostly only the suggested implementation and didn't see the details at the end. :-)

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 405002 [details] [diff] [review]
v2

Good solution and works as expected via UI and XMLRPC.
r=dkl

Comment 9

[Max Kanat-Alexander]

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.78; previous revision: 1.77
done
Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.29; previous revision: 1.28
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.65; previous revision: 1.64
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.49; previous revision: 1.48
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.79; previous revision: 1.78
done
Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.91; previous revision: 1.90
done
Checking in Bugzilla/Auth/Login/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/CGI.pm,v  <--  CGI.pm
new revision: 1.13; previous revision: 1.12
done
Checking in Bugzilla/Auth/Persist/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v  <--  Cookie.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/Config/Core.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Core.pm,v  <--  Core.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/WebService/Server.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server.pm,v  <--  Server.pm
new revision: 1.2; previous revision: 1.1
done
Checking in docs/en/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/administration.xml,v  <--  administration.xml
new revision: 1.96; previous revision: 1.95
done
Checking in template/en/default/account/auth/login-small.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/auth/login-small.html.tmpl,v  <--  login-small.html.tmpl
new revision: 1.21; previous revision: 1.20
done
Checking in template/en/default/admin/params/core.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/core.html.tmpl,v  <--  core.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.167; previous revision: 1.166
done

Comment 10

[Max Kanat-Alexander]

The second time you ran checksetup (and all later times), checksetup threw the following warning:

  Use of uninitialized value in string ne at Bugzilla/Config.pm line 198.

I've corrected with an additional checkin:

Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.80; previous revision: 1.79
done

Comment 11

[Max Kanat-Alexander]

Added to the release notes in bug 547466.