Fix bugs in https support and force using https for authorize.php if available

@Jacob: Yes, exactly. If we're going to go through the trouble (and we should), then it should probably be a general solution, not specific to only solving this problem for authorize.php.

Wow, I didn't realize #1577: Mapping privilege separation on non-SSL/SSL had landed. ;) So, this is a lot closer than I thought. Here's a patch that should get us most of the way there... we use https for authorize.php if available, and we warn on authorize.php if we're still using http. I've only been able to test the http case right now, I was trying to get an https-enabled D7 test site up but I ran into some snags at my hosting provider. Don't have the energy to get it working on my laptop right now, either. ;)

@killes: there are various reasons someone might actually want/need to use ssh over http. We discussed this at length in IRC. The FTP vs. SSH is more about what connection methods your server supports. The security of the localhost connection between httpd and the filesystem is pretty boring. What sucks is that your password is going in the clear over http, but that's sucky regardless of if it's an FTP or SSH connection. Hence the big red box. But in some cases that might be your only choice, or it might not matter (maybe you've got a VPN or something, or you're developing locally and don't want to run an ftpd on your laptop). I don't think we should be completely removing the SSH option just because we're over http. People who have SSH access and care about security but don't have https probably won't be using this tool anyway -- if you're comfortable with ssh and your shell, drush is probably a better solution. If they're not security conscious, we can't stop them from emailing their password to their friends, either. ;)

But yeah, I'm torn. I can definitely see the motive behind #9, and a large part of me agrees with it... Maybe the compromise is another non-UI variable that allows SSH over http, which can default to FALSE. But, in the few cases where it's actually okay, someone could add $conf['allow_ssh_authorization_over_http'] = TRUE; to their settings.php...

Regardless, I'm not sure why you set this to "active", we now have 2 viable patches to review. Nor why you unassigned me from this, since I'm trying to get a solution into core...

p.s. If we're not on https, maybe we should use JS to rot13 the password before we send it over the wire. ;) LOL

Here's the compromise -- we unset the SSH option unless 'allow_ssh_authorization_over_http' is TRUE (defaults to FALSE).

So, there are 3 possible approaches when we're at authorize.php over http:

#8: Just warn about sending your password in clear text in all cases.

#9: If SSH is available, remove SSH as a choice and warn them we did so. If there's no SSH, just use the regular warning from #8.

#11: If SSH is available, and the setting is FALSE (the default) remove that option and warn them we did so. Otherwise, fall back to the regular warning from #8.

Wow, our D7 https support is still quite broken. I've got a self-signed cert on my laptop now so I can actually play with this stuff. Running into all sorts of problems:

A) If you're admin'ing the site via http and then we redirect to https://example.com/authorize.php your SESSION doesn't persist since there's no secure session. #1577: Mapping privilege separation on non-SSL/SSL is 98% of the way there, and then balked at a friggin' checkbox to secure the login forms, so you actually still need a contrib module to secure your D7 site (unless you force the entire site to be https via apache). *sigh* So, to actually see this in action, you need the following in a module:

function wtf_form_alter(&$form, $form_state, $form_id) {
  switch ($form_id) {
    case 'user_pass':
    case 'user_login':
    case 'user_login_block':
    case 'user_register_form':
      $form['#https'] = TRUE;
      break;
  }
}

Once that's in place, if you login, core's magic ssid + sid session handling will kick-in, and then you'll actually be able to go back and forth between http and https pages and remain logged in.

B) url()'s handling of https is busted in various ways. First of all, the PHPDoc for the 'https' option says something sane, that if you don't define it, it defaults to leaving the scheme alone (so if you're on https you stay on https, or on http you stay on http). However, the code doesn't work like that, and if you don't define it, it forces https to be FALSE. :( Secondly, you can't set https = TRUE on an external link (which is what chx's cryptic comment in #14 actually means). ;)

C) Setting $form['#https'] causes FormAPI to call menu_path_is_external() which lives in menu.inc. However, this is a poorly named and placed function, since it doesn't actually have anything to do with the menu system at all. It's a 2-line helper function that just does some string comparison. In fact, it's 2 lines that are identically cut+pasted to/from url() (not sure which was first), although that's probably there for a performance optimization to avoid another function invocation inside the all-important url() function (I added a comment to that effect). Anyway, authorize.php hasn't needed to load menu.inc at all until now, and I refuse to load all of that in authorize.php for a 2-line URL helper function that FAPI now depends on. :( So, I moved menu_path_is_external() into common.inc (right below url(), in fact) with a comment that the function should be renamed to "url_is_external()" in D8. For now, I'm assuming we consider this too much of an API break at this point -- it's called by 2 other places in core besides menu.inc and form.inc. Here are all the call sites in core:

includes/form.inc:        !menu_path_is_external($element['#action'])) {
includes/menu.inc:  $item['external'] = (menu_path_is_external($item['link_path'])  || $item['link_path'] == '<front>') ? 1 : 0;
includes/menu.inc:  if ($path == '<front>' || menu_path_is_external($path)) {
modules/menu/menu.admin.inc:  if (!menu_path_is_external($item['link_path'])) {
modules/shortcut/shortcut.module:  return !menu_path_is_external($path) && menu_get_item($path);

I'd be happy to rename this, but not at the expense of a fight about the API freeze. For now, I'm okay just moving it into the right .inc file at least, even though the name is dumb.

Once all of that was fixed, this is now working. ;) Yay. Now all we need is the auto-detection during the installer (like clean URLs) and the UI for specifying what pages/forms you want to protect. That should probably happen as a separate issue, so long as it's a release-blocking critical task.

Sadly, this patch is going to conflict a bit with #610290: system_run_authorized() shouldn't always drupal_goto() so it can be used in submit handlers (I knew it would), but it'll be pretty easy to fix either patch once the other lands -- it's only going to be a minor conflict in system.module.

Forgot to fix status. Also, changing the title to explain what this issue is for. In terms of the UI in core to actually make https work, let's deal with that over at #616744: Add a UI for if the site supports https.

For comparison, here's the same fixes but with just renaming menu_path_is_external() to url_is_external(). Yay for less @todo comments in core.

So, we're having a big debate about the setting, the warnings, etc, in IRC right now, and I'm posting a few screenshots of the current, compromise approach when you visit authorize.php over http, with the setting both off (the default) or on. No warnings appear over https, obviously, so I'm not going to post screenies of those.

The original patch would basically always look like the "setting-on" version here.

In IRC, greggles proposed leaving ssh as an option in the select, but disabled with a note about it... sounds interesting. However, we still have the problem of the people who should be able to do ssh over http, so we'd probably still need the compromise setting.

Attempted summary of the IRC chat:

- DamZ and davereid are opposed to trying to disable choices. We should always warn if you're sending any passwords over the wire, but otherwise, the admin should make the choice themselves. No setting, no special cases.

- greggles says that if we have the setting at all, people will just search, find the killswitch, and disable the checks if they want, so it doesn't actually help make it more secure.

- killes (of course) would prefer the ssh option completely removed on http no matter what, but is willing to live with the no-UI setting that allows this which defaults to FALSE.

- I point out that if we're going to have this setting and special-case handling of "secure" connection types, that really needs to be another attribute returned in the hook_filetransfer_backends() array. If someone adds an sftp backend, we need to be able to do whatever we're doing for that, too, not *just* ssh.

- greggles had suggested an alternative UI at one point. Instead of the "The SSH backend was disabled to protect the integrity of your password." text appearing in the warning message, we could leave the ssh choice in the drop-down, but have it be a disabled choice, and change the label to be something like: "SSH - disabled to protect your password, try https". Sounds like he's retracted that proposal, and now just supports a warning in all cases and no special handling at all -- just leave all the options there, but warn if they're on http.

From the KISS perspective, the just warn approach is obviously the least complicated.

Drat. I was really hoping we'd all agree on something, but greggles had to run, and I think killes might be sleeping by now. :( Not sure how to proceed...

There seems to be a growing majority supporting the approach of my original patch, so for the sake of argument, here's a reroll of #22 (including the rename to url_is_external() which webchick already agreed to) but with just the warning if you're on http, but all options are still available. No killswitch, no 'secure' attribute in the info hook, etc.

p.s. Hopefully it doesn't matter and we don't try to special-case and restrict certain connection types, but the UI greggles proposed can't really be done until #284917: Allow FAPI select, radios, and checkboxes to specify some options as disabled is resolved, and that's blocked on IE stupidity, so it might never happen. So, if we do end up with some special handling for certainly connection types, we're just going to have to remove them from the list and add them to the warning text.

Rerolled now that #610290: system_run_authorized() shouldn't always drupal_goto() so it can be used in submit handlers landed.

The link actually goes to http://drupal.org/https-information (which also doesn't exist). ;) Generally d.o has been trying to avoid the top-level URLs, but maybe for this it's okay. Fixed a few problems in the last patch:

- <a href="@https-link"> instead of <a href="@https-link"/> (that shouldn't try to be a self-closing tag).
- code style: no space between array and (
- Looks like the style for core are "Learn more" links, not "Learn more about this".

New message now reads:

WARNING: You are not using an encrypted connection, so your password will be sent in plain text. Learn more.

API change documented in the module update doc: http://drupal.org/node/224333/revisions/view/763538/764780

No time for a full handbook page right now, but basic summary of what we need to say:

* sending your password in clear text over the wire is dangerous. https encrypts your connection between the browser and the server, http does not.

* how to setup https for your site:
server:
-- need a certificate (many hosting providers set these up for you, either automatically or for a fee)
-- need httpd configured properly (links to docs?)
drupal:
-- either force all connections to https via .htaccess redirect (more secure, but more expensive in terms of server load)
-- or set $conf['https'] = TRUE; in settings.php (less secure, see #575280: Impersonation when an https session exists., but at least your FTP/SSH password wouldn't be sent in plain text)

the Drupal parts are subject to change pending the outcome of #616744: Add a UI for if the site supports https and #575280 ...