If the video filter man in the middle XSS (eg via evil DNS) is classified as a security issue, shouldn’t the current update status process (eg via admin/reports/updates/update) be marked a security issue as well?

As far as I can see there are no authenticity checks whatsoever on the release data and download.

This can be used to

1 - prevent reception of update alerts via the update status channel
2 - entice admins to install modules from untrusted servers

(this has been cleared by the secteam for public discussion, the suggestion was the Infra queue, but it would need Core support as well).

CommentFileSizeAuthor
#101 update_status_does_not-1538118-101.patch1.46 KBsanduhrs
#101 update_status_does_not-1538118-101.interdiff.txt1.46 KBsanduhrs
#91 increment-1538118-91.txt1.67 KBpwolanin
#91 1538118-91.patch11.81 KBpwolanin
#85 increment-1538118-85.txt2.86 KBpwolanin
#85 https-updates-1538118-85.patch10.14 KBpwolanin
#82 update_status_does_not-1538118-82.patch8.3 KBYogesh Pawar
#78 1538118-78.update-https-drupal-org.interdiff.txt582 bytesdww
#78 1538118-78.update-https-drupal-org.patch9.05 KBdww
#76 1538118-75.patch9.11 KBdawehner
#75 1538118-75.patch56.65 KBdawehner
#73 interdiff-1538118.txt1.56 KBdawehner
#73 1538118-72.patch7.52 KBdawehner
1538118-72.patch66 bytesmlhess
#35 1538118-35.patch7.55 KBswentel
#35 interdiff.txt2.32 KBswentel
#31 1538118-31.patch5.24 KBmgifford
#2 updates.d.o.-https-1538118-2.patch701 bytesWim Leers
#23 1538118-23.patch1.22 KBswentel
#23 1538118-23.patch5.78 KBswentel
#26 interdiff.txt554 bytesswentel
#26 1538118-26.patch5.78 KBswentel
#39 interdiff-39.patch788 bytesswentel
#39 1538118-39.patch7.7 KBswentel
#103 update_status_does_not-1538118-101.interdiff.txt1.46 KBsanduhrs
#103 update_status_does_not-1538118-101.patch11.77 KBsanduhrs
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers’s picture

Priority: Normal » Major
Issue summary: View changes

This applies to both 7 and 8.

In 8's \Drupal\update\UpdateFetcher:

  /**
   * URL to check for updates, if a given project doesn't define its own.
   */
  const UPDATE_DEFAULT_URL = 'http://updates.drupal.org/release-history';

IMHO that using HTTP instead of HTTPS, even though it's perfectly accessible over HTTPS, is a gross oversight.

Wim Leers’s picture

Status: Active » Needs review
FileSize
701 bytes

i.e. why not just do this?

Dave Reid’s picture

Issue tags: +Security improvements
David_Rothstein’s picture

Issue tags: +needs backport to D7

In addition to using https I assume we need to verify the cert. Drupal 8 may handle that already though. See also: #1081192: Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase)

I would think this should be classified as a critical issue (given that the above issue already is).

Wim Leers’s picture

In addition to using https I assume we need to verify the cert.

Oh wow, I'd think that happens automatically? :O

I would think this should be classified as a critical issue (given that the above issue already is).

I was thinking the same.

cilefen’s picture

Title: Update (status) MiTM » Update status does not verify the identity or authenticity of the release history URL

I added the words "identity" and "authenticity" to the title because I think we need both.

moshe weitzman’s picture

FYI Drush already uses SSL for updates.drupal.org. Would be good to double check with infra team before making this change.

cilefen’s picture

What about a gpg signature that goes alongside the xml file?

drumm’s picture

Status: Needs review » Reviewed & tested by the community

Would be good to double check with infra team before making this change.

Drupal.org's infra is ready for this change.

Heine’s picture

This issue is about Drupal core not about politics or publications. Please keep it on topic. I've deleted OT comments.

drumm’s picture

Version: 8.0.x-dev » 8.1.x-dev

Maybe 8.1 is the right place.

alexpott’s picture

Version: 8.1.x-dev » 8.0.x-dev

To me this is not an API change and would eligible to be included in a patch release. I'd be surprised if any further fixes that need to be made here will result in an API change so setting back to 8.0.x.

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +8.0.3 release notes

I think we should mention this change in the release notes as some setups I know of use an outbound proxy to manage what their Drupal site can connect to.

Committed d7fb0d1 and pushed to 8.0.x and 8.1.x. Thanks!

Heine’s picture

Status: Patch (to be ported) » Needs review

I've tested on Ubuntu 14.04 on Drupal 8.

Updates can be fetched normally.

With updates.drupal.org pointing to another ip with a certificate I get the expected exception / error:

Via manual checking I saw the error: "Failed to get available update data for one project."
In the log I found the error:

GuzzleHttp\Exception\RequestException: cURL error 51: SSL: no alternative certificate subject name matches target host name 'updates.drupal.org' (see http://curl.haxx.se/libcurl/c/libcurl-errors.html) in GuzzleHttp\Handler\CurlFactory::createRejection() (line 187 of /var/www/drupal8/htdocs/vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php).

Two concerns:

- is guzzle able to verify certificates on Windows systems?
- curl_setopt documentation on http://php.net/manual/en/function.curl-setopt.php refers to defaults on curl 7.10. If we rely on defaults, can we be sure all versions of PHP matching the requirements have these defaults? Or in other words, should we set verification explicitly?

alexpott’s picture

Version: 7.x-dev » 8.0.x-dev

Given #16 I'm going to revert the commit and wait for clarification.

[edit] I never pushed so all is good in the world :)

Heine’s picture

I've found the following statement on old php documentation:

PHP 5.0.0 requires a libcurl version 7.10.5 or greater.

Combined with the statement on the verify_peer default on 7.10 being TRUE, this means the defaults are appropriate for our use on all supported versions of PHP. A Windows test remains to be done.

swentel’s picture

Should we do the same for LOCALE_TRANSLATION_DEFAULT_SERVER_PATTERN in locale.module ?

drumm’s picture

Yes, all updates.drupal.org URLs can be updated.

swentel’s picture

Checked the whole codebase for URL's pointing to *.drupal.org.
(edit - ignore the first patch here)

swentel’s picture

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
@@ -33823,7 +33823,7 @@
-  'value' => 's:41:"http://updates.drupal.org/release-history";',
+  'value' => 's:41:"https://updates.drupal.org/release-history";',

the string length also needs to be updated in this serialized string.

swentel’s picture

Status: Needs work » Needs review
FileSize
5.78 KB
554 bytes

Right

The last submitted patch, 23: 1538118-23.patch, failed testing.

The last submitted patch, 23: 1538118-23.patch, failed testing.

alexpott’s picture

+++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
@@ -33823,7 +33823,7 @@
-  'value' => 's:41:"http://updates.drupal.org/release-history";',
+  'value' => 's:42:"https://updates.drupal.org/release-history";',

Let's not change this... it is a Drupal 6 value. However what this does highlight is that we need to convert the http value to a https value during an update...

dstol’s picture

@Heine, Guzzle on a Windows 10 system is able to validate certificates. I was using DevDesktop with Drupal 8.0.1, PHP 5.6.14 and 5.5.30. I worked with basic` and drumm against updates.staging.devdrupal.org.

mgifford’s picture

I don't know why we shouldn't change the update_fetch_url variable within drupal6.php, but happy to remove it from the patch. I'm wondering if we should add documentation though if we aren't going to include this.

This should work, right? https://updates.drupal.org/release-history

Just trying to nudge this along.

Status: Needs review » Needs work

The last submitted patch, 31: 1538118-31.patch, failed testing.

klausi’s picture

The test is now failing as expected because we need to implement a migration for the variable value in D6 and D7. If the value is the untouched "http://updates.drupal.org/release-history" then turn it into "https://updates.drupal.org/release-history".

From a quick look I could not figure out how to modify core/modules/update/migration_templates/update_settings.yml. We probably will have to write a migration class that extends Drupal\migrate_drupal\Plugin\migrate\source\Variable and overrides a method to do the alteration. But which method should it be? values()? prepareRow()? initializeIterator()? Something else?

Any pointer where we do something similar in core already - modifying a variable value during a migration?

swentel’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
2.32 KB

This should do it

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/src/Plugin/migrate/source/UpdateSettings.php
@@ -0,0 +1,29 @@
+    $values['update_fetch_url'] = 'https://updates.drupal.org/release-history';

+++ b/core/modules/update/src/UpdateFetcher.php
@@ -22,7 +22,7 @@ class UpdateFetcher implements UpdateFetcherInterface {
-  const UPDATE_DEFAULT_URL = 'http://updates.drupal.org/release-history';

Of course this begs the question what should we do if the Drupal 6 has a custom update_fetch_url variable? Ie. one not pointing to http://updates.drupal.org/release-history

swentel’s picture

Yeah, crossed my mind as well, do strpos on the value to see if it points to drupal.org then I guess ?

alexpott’s picture

@swentel yeah I guess so

swentel’s picture

Done. I also added a check in case the value is empty, because in reality I think the chance that the variable will be in the variable table is pretty slim.

The last submitted patch, 39: interdiff-39.patch, failed testing.

The last submitted patch, 39: 1538118-39.patch, failed testing.

hass’s picture

hass’s picture

David_Rothstein’s picture

Issue tags: -8.0.3 release notes

Removing tag since per above this did not wind up going into 8.0.3.

mgifford’s picture

Ok, so what more needs to happen for this to be RTBC?

Do we need to test it in production sites? Kinda sad that after the media embarrassments around this we didn't get it in 8.0.3

This looks like a pretty trivial fix. I think for most of us, we use Drush so this isn't a big deal. Still, it's functionality that we've brought into Core and need to support.

swentel’s picture

I think someone needs to RTBC it so committers can have a look at it, don't really think anything is missing.

greggles’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

hass’s picture

Status: Reviewed & tested by the community » Postponed

I think this need to wait for the two issues I linked too.

drumm’s picture

Status: Postponed » Reviewed & tested by the community
hass’s picture

Than we still have the issue that nearly every curl installation has outdated root certificates embedded and this will cause fetch fails. See https://www.drupal.org/node/2481341

David_Rothstein’s picture

In addition to what @hass said, https://groups.drupal.org/node/506128#comment-1141666 suggests that the Update module might need to add a requirement on openssl as a result of this issue. (I'm not sure if that's relevant for Drupal 7 only, or also Drupal 8?)

We should proceed carefully here - although the patch is a security improvement, the protection it adds is for a not-very-likely scenario. But breaking the ability of some sites to contact updates.drupal.org at all could be an actual security problem.

drumm’s picture

Could we trap and warn about errors, then fall back to http?

ianthomas_uk’s picture

You'd need to be very careful catching errors and falling back, because you wouldn't want to catch any errors that could be caused by an attacker.

You could maybe catch the error and allow the user to make the decision, while leaving a visible error on their status page that openssl is misconfigured.

David_Rothstein’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work

We should add a hook_requirements() warning at least.

The download could possibly be a hard fail since there are workarounds, although even then hard failing seems worse than someone not installing an actual security update given the relative likelihood.

Update checking itself there's a limited amount of damage that could be done so it'd be better to fall back with some kind of warning than not checking at all - much more likely that a module needs a security update than some kind of phishing attack with the update status report.Could also drupal_set_message()/trigger_error() when doing the fallback for those cases.

Definitely need something for the openssl requirement so bumping back to needs work.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pwolanin’s picture

Priority: Major » Critical

We indicate here openssl is required - is that page ahead of the actual code?
https://www.drupal.org/requirements/php#openssl

Drupal 8 is using a Drupal\Core\Http\ClientFactory which could probably return anything implementing the \GuzzleHttp\ClientInterface?

Are we sure that openssl is always going to be the required SSL library? vs gnuTLS or something else? So I guess the idea is just to set a requirement error or message if the checks fail consistently over HTTPS?

For update checking I could tell you there are no updates so your site is insecure and open to later attack? It seems like this is important to also secure.

An alternative to forcing HTTPS would be layering openssl_sign() and openssl_verify() into the process, but that would mean infra changes on d.o to publish a cert or CA and cert and sign each file.

alexpott’s picture

With regards to falling back #2367319-82: Implement automatic background updates for highly critical security issues is relevant - basically wordpress is falling back - I think

Perhaps the correct question to ask is not "is the automatic update mechanism totally bulletproof & locked down like Fort Knox" but rather "is the risk of unpatched vulnerabilities in the CMS greater than the risk introduced by the automatic update mechanism?"

Is pertinent - the risk of not falling back is that the sites might not find out they have to update - and that risk is proven since we know people install without access to up-to-date certs.

David_Rothstein’s picture

I agree with the idea of trying https, then falling back on http if it fails (but with some kind of warning message) - it seems like a good plan which would be significantly better than what we have now.

(To actually force everyone to use a secure method in the long run would probably require something more like some of the ideas in the issue @cilefen linked to above: #509010: Build a Package signing system for d.o. so that people can securely and simply download and verify packages.)

We indicate here openssl is required - is that page ahead of the actual code?
https://www.drupal.org/requirements/php#openssl

Seems like it. This was added in https://www.drupal.org/node/1783062/revisions/view/9242254/9339198, probably when it looked like things were moving towards all-SSL faster than they actually did. I edited it now to say openssl is recommended rather than required: https://www.drupal.org/node/1783062/revisions/view/9859061/9882701

cilefen’s picture

effulgentsia’s picture

Issue tags: +Triaged D8 critical

Discussed with @xjm, @alexpott, @webchick and @Cottser, and we agree this is Critical. For many Drupal users, Update status is the primary channel by which they are notified of security updates and/or by which they apply them. As such, it is pretty essential for the module itself to be as secure as possible, within reason.

dsnopek’s picture

This may be unique to Drupal 6 sites (since they are much more likely to be running out-of-date servers) but I just wanted to give some input from our experience with the mydropwizard module (a replacement for the core update status module in Drupal 6, for Drupal 6 Long-Term Support).

By default, our module gets release data from an HTTPS end-point (https://updates.mydropwizard.com), using what we believe to be a basically "best practices" configuration (see an SSL test) that shouldn't be problematic or unusual. However, there were enough users having issues with their server connecting to the HTTPS end-point that we couldn't solve, that we had to provide an HTTP end-point (http://updates-insecure.mydropwizard.com) and instructions to switch from the default: #2699493: No longer connects to the update service & #2741987: problem determining the status of available updates

Unfortunately, none of the sites with issues were our direct customers, so we weren't able to debug directly on any of these servers - we just had the issue queue.

Anyway, all that just to give some evidence for needing some kind of fallback or option to use HTTP when a user's server just can't seem to connect via HTTPS :-)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

See #606592-94: Allow updating core with the update manager about Wordpress's potentially catastrophic auto-update vulnerability as this pertains to that issue.

cilefen’s picture

cilefen’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dww’s picture

Yeah, when update manager was added to Drupal, the d.o infra still didn't support https. :/ Glad we're finally resolving this.

drumm’s picture

With 8.3.0 coming up, can we be more-agressive in getting this updated?

If Guzzle can’t make the https request, that's a sign something is not set up well on the server. We can make sure this failing is sufficiently noisy and worry about falling back to http, if necessary, in a separate issue.

drumm’s picture

Hiding a few files from the issue summary. #39 looks like the patch to review, which I sent in for a test with 8.3.x.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.52 KB
1.56 KB

I started to implement that fallback.

Note: https://badssl.com/ is a great site.

Status: Needs review » Needs work

The last submitted patch, 73: 1538118-72.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
56.65 KB

This time even with the missing files.

dawehner’s picture

Sorry for the spam :(

colan’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/src/UpdateFetcher.php
@@ -67,6 +86,11 @@ public function fetchProjectData(array $project, $site_key = '') {
+        $url = str_replace('https://', 'http', $url);

Looks like this will drop the "://" when switching to HTTP.

dww’s picture

Here's a reroll to fix #77, thanks for catching it!

I grepped through all of core and there are a few other references to http://*.drupal.org. However, they're non-critical scope creep in here, since none of them are related to updates.d.o.

I reviewed the rest of this patch and it all looks good. Assuming the testbot is happy, I think this is RTBC. But I guess I shouldn't do that to "my own" patch (even though my change is trivial). But +1 for committing this ASAP.

Thanks,
-Derek

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

Status: Needs review » Needs work

I have some nits to pick, but this looks great.

  1. +++ b/core/modules/update/src/Plugin/migrate/source/UpdateSettings.php
    @@ -0,0 +1,31 @@
    +/**
    + * @file
    + * Contains \Drupal\update\Plugin\migrate\source\UpdateSettings.
    + */
    

    This docblock should be removed, we don't do @file docblocks for classes anymore.

  2. +++ b/core/modules/update/src/UpdateFetcher.php
    @@ -59,6 +59,25 @@ public function __construct(ConfigFactoryInterface $config_factory, ClientInterf
    +   * This method falls back to not https in case there was some certificate
    +   * problem.
    

    /s/not https/http/ to make it more explicit what's happening?

  3. +++ b/core/modules/update/src/UpdateFetcher.php
    @@ -59,6 +59,25 @@ public function __construct(ConfigFactoryInterface $config_factory, ClientInterf
    +   * @param bool $with_non_ssl_fallback
    +   *   Should the function callback to NON ssl.
    

    /s/NON ssl/http/? At the very least, we should probably use either non https or non ssl instead of mixing them in the same docblock. This might mean that we have to rename the variable as well?

Yogesh Pawar’s picture

Assigned: Unassigned » Yogesh Pawar
Yogesh Pawar’s picture

Assigned: Yogesh Pawar » Unassigned
Status: Needs work » Needs review
FileSize
8.3 KB

Changes made as per comment #80 & previous patch failed to apply on 8.4.x branch

cilefen’s picture

There is an issue open against packagist to employ a Chronicle ledger.

pwolanin’s picture

Status: Needs review » Needs work

Discussing in person with David Strauss.

I don't think it makes sense to have an automatic fallback in terms of the security goals here - let's require a setting.php setting to allow the fallback if the PHP install in not able to handle https?

We also (or in a follow-up) could add log messages or DSMs with details about the https failure so we can get data to look for e.g. a openssl version or lack of it that cause the fail and we could check for locally in PHP rather than responding to a network failure that could be engineered by an attacker?

pwolanin’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
FileSize
10.14 KB
2.86 KB

Here's a quick update to add a setting to control the fallback behavior.

David_Rothstein’s picture

+    $this->withHttpFallback = $settings->get('update_fetch_with_http_fallback', FALSE);

I think the fallback has to be automatic (i.e. the default should not be FALSE here), otherwise we risk killing update status information for lots of sites, which would be worse for security overall.

But as discussed above, we could display a warning to the administrator in the case where the http:// fallback wound up being used.

David_Rothstein’s picture

+   * @param bool $with_http_fallback
+   *   Should the function fall back to http.
+   *
+   * @return string
+   */
+  protected function doRequestWithHTTPFallback($url, array $options, $with_http_fallback) {

It also seems a little weird to me that a function called "doRequestWithHTTPFallback" has an option to do the request without the HTTP fallback. Probably needs a different function name here :)

Status: Needs review » Needs work

The last submitted patch, 85: https-updates-1538118-85.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

David Strauss’s picture

I discussed automatic fallback with Peter as well. What I have to insist on for automatic fallback is that it happen on local criteria only. It should not allow an attacker to tamper with TLS negotiation and cause a fallback.

Can we detect -- by only investigating locally -- whether fallback is necessary? Alternatively, can we at least lock in that succeeding once with HTTPS causes it to be required on subsequent requests?

pwolanin’s picture

Looks like I forgot to include default.settings.php changes in the patch, and looks like I need to update a couple unit tests for the changed constructor.

@David Strauss - since this patch will move us from few people using https to most people using https, and requires a settings op-in to use the fallback I'm worried about adding more complexity there.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
11.81 KB
1.67 KB

This adds the missing change to default.settings.php and fixes the test I think

David Strauss’s picture

The new patch looks good. I only shared my thoughts on automated fallback because we would have to do so very carefully. I prefer being explicit about it the way it is in #91.

drumm’s picture

Status: Needs review » Reviewed & tested by the community

Tested a fresh install with this and it looks good to me.

The last submitted patch, 1538118-72.patch, failed testing. View results

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

I agree on the 'if you want fallback you have to opt-in approach'

David's comment from #87 hasn't been addressed yet.

+++ b/sites/default/default.settings.php
@@ -321,6 +321,17 @@
+ * Fallback to HTTP for update status.
+ *
+ * If your Drupal site fails to connect to drupal.org using HTTPS to fetch
+ * Drupal core and module update status, you may uncomment this setting
+ * and set to TRUE to allow an insecure fallback to HTTP.
+ *
+ * @see \Drupal\update\UpdateFetcher
+ */
+# $settings['update_fetch_with_http_fallback'] = FALSE;
+

I think the language should be stronger here.

Something along the lines of 'note that you are opening your site up to a potential man in the middle attack and you should consider instead attempting <a href="https://drupal.org/link/to/docs/about/certfiles">resolve the issues</a> before enabling this option'?

Even a 'Caution: this may void your warranty' type message ala Firefox' about:config would probably suffice.

I note Bolt CMS has a whole docs page on how to configure https://docs.bolt.cm/3.2/howto/curl-ca-certificates - its pretty decent

larowlan’s picture

Happy to bump the docs change to a follow-up as getting this in sooner rather than later is important.

But I think David's comment needs addressing

larowlan’s picture

suggest doRequestWithOptionalHTTPFallback for the name to address #87?

pwolanin’s picture

How about just method name doRequest() ?

larowlan’s picture

+1

borisson_’s picture

+1 for #98.

sanduhrs’s picture

The last submitted patch, 101: update_status_does_not-1538118-101.patch, failed testing. View results

sanduhrs’s picture

David_Rothstein’s picture

Having the fallback off by default still seems very problematic to me. We know for a fact that this will kill Update Status for lots of sites, because it has happened before (in addition to some of the above comments you can also read through https://groups.drupal.org/node/506128 and #2646894: Redirect http://updates.drupal.org and http://ftp.drupal.org to https version (roll back the updates.drupal.org part?)). And those comments are just from people who figured out there was a problem and bothered to research it. Probably many other sites simply didn't even know that Update Status stopped working. If we enshrine this in core, they'll simply stop getting Update Status data, which is worse for security than the problem this issue is trying to fix.

Why can't we have the fallback on by default, but with a warning in the UI when the data came from using it (or, perhaps, also consider David Strauss's suggestion of turning the fallback off as soon as an HTTPS request succeeds the first time)?

Also, what happened to the hook_requirements()/openssl discussions above?

And:

     catch (RequestException $exception) {
       watchdog_exception('update', $exception);
+
+      if ($with_http_fallback && strpos($exception->getMessage(), 'SSL certificate problem: Invalid') !== FALSE) {

Are we sure that this particular exception message is the only one we want to use the fallback for? Also not sure that watchdog-ing it in the same way for both scenarios is correct - we do want to record the failure somehow, but when the fallback is about to be attempted it seems like it would be useful to indicate that in the log message.

David_Rothstein’s picture

If people are really adamant about having the fallback off by default, maybe one solution would be to warn a lot more loudly in the case of an Update Status failure (for example, right now I think it's only a REQUIREMENT_WARNING which won't even trigger an email to be sent out). This is probably a good idea regardless.

But it still would leave site owners needing to hand-edit settings.php to fix the issue, with no real guidance anywhere on how to do that or any obvious indication that that would fix their problem.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.