Problem/Motivation

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).

Steps to reproduce

Enable Update manager

Proposed resolution

  • Try to fetch over https.
  • Print warning messages on the available updates report and the 'Update' form if the site failed to fetch data, warning admins about proper HTTPS config
  • Fallback to the current HTTP URL only if the 'update_fetch_with_http_fallback' setting has been overridden to TRUE (defaults FALSE).
  • Migration to D8/9 sites from D7 and before will re-write the update_fetch_url config from HTTP to HTTPS

E-mailing admins if their site is failing to fetch info is at #3162336: Update manager should email admins when it fails to fetch available update data

Remaining tasks

  1. Create the d.o handbook page documenting the details so we can link to it. Update the @todo URL in the patch. https://www.drupal.org/docs/system-requirements/php-openssl-requirements (see patch #137)
  2. Decide if we need the flexibility of a Twig template for this (#132) or if we can just use some hard-coded stuff via the Messenger service (#134). Yes to a template - see #139 for why.
  3. Decide if we need DI for the UpdateManagerUpdate form (#135a) or can just use \Drupal::service() (#135b) No new DI, it's a template_preprocess() now (see #139).
  4. Decide what (if any) new automated testing we need for the error messages, fallback setting, etc. Kernel tests added in #140.
  5. Needs manual testing
  6. Needs UX review See #136
  7. Maybe needs a product manager review?
  8. Needs a release manager review to decide about backports with all these new strings and other changes
  9. Release note snippet?
  10. CR? Draft CR
  11. RTBC
  12. Commit
  13. https://www.drupal.org/node/3170647/edit to update the version info in the docs about this.
  14. Backport(s)?

User interface changes

New messages if Update Manager fails to fetch available updates (presumably because the site's PHP isn't configured for outbound HTTPS requests to work):

Available updates report

Available updates report when a site can't make outbound HTTPS requests

Update form

Updates form when a site can't make outbound HTTPS request

Cryptic watchdog message

This is why we need a handbook page. ;)
Error message in DB log when site can't make outbound HTTPS request

New setting documented in default.settings.php

/**
 * Fallback to HTTP for Update Manager.
 *
 * If your Drupal site fails to connect to updates.drupal.org using HTTPS to
 * fetch Drupal core, module and theme update status, you may uncomment this
 * setting and set it to TRUE to allow an insecure fallback to HTTP. Note that
 * doing so will open your site up to a potential man-in-the-middle attack. You
 * should instead attempt to resolve the issues before enabling this option.
 * @see https://www.drupal.org/docs/system-requirements/php-requirements#openssl
 * @see https://en.wikipedia.org/wiki/Man-in-the-middle_attack
 * @see \Drupal\update\UpdateFetcher
 */
# $settings['update_fetch_with_http_fallback'] = FALSE;

API changes

  • New Twig template: update-fetch-error-message.html.twig with template_preprocess_update_fetch_error_message()
  • New UpdateSettings migrate source plugin
  • \Drupal\update\UpdateFetcher (the 'update.fetcher' service) now needs the site Settings injected.

Data model changes

N/A

Release notes snippet

The Update Manager now fetches available update data over HTTPS. Your site's PHP must have OpenSSL properly configured. If that's not possible, you can set $settings['update_fetch_with_http_fallback'] = TRUE; in your settings.php file to use the less secure HTTP fallback. See the PHP OpenSSL requirements section of the Drupal.org handbook for more details.

CommentFileSizeAuthor
#170 1538118-170.patch29.93 KBalexpott
#170 157-170-interdiff.txt8.2 KBalexpott
#165 1538118-165.patch25.81 KBayushmishra206
#165 interdiff_162-165.txt2.05 KBayushmishra206
#162 1538118.161_162.interdiff.txt699 bytesdww
#162 1538118-162.patch27.52 KBdww
#161 1538118.157_161.interdiff.txt6.39 KBdww
#161 1538118-161.patch27.53 KBdww
#157 1538118.156_157.interdiff.txt5.78 KBdww
#157 1538118-157.patch29.02 KBdww
#156 1538118.154_156.interdiff.txt5.21 KBdww
#156 1538118-156.patch24 KBdww
#154 1538118.150_154.interdiff.txt4.42 KBdww
#154 1538118-154.patch22.55 KBdww
#150 1538118.142_150.rawdiff.txt1.21 KBdww
#150 1538118.142_150.interdiff.txt2.66 KBdww
#150 1538118-150.patch22.74 KBdww
#142 1538118.140_142.interdiff.txt913 bytesdww
#142 1538118-142.patch22.36 KBdww
#140 1538118.137_140.interdiff.txt8.21 KBdww
#140 1538118-140.patch21.13 KBdww
#138 1538118-138.duplicate-error-from-manual-check.png73.19 KBdww
#138 1538118-138.update-ui.png121.33 KBdww
#138 1538118-138.available-updates.png190.63 KBdww
#137 1538118.135b_137.interdiff.txt826 bytesdww
#137 1538118-137.patch17.74 KBdww
#135 1538118-135.watchdog.png87.33 KBdww
#135 1538118-135.update-ui.png111.71 KBdww
#135 1538118-135.available-updates.png180.18 KBdww
#135 1538118.135a_135b.interdiff.txt2.15 KBdww
#135 1538118.134_135b.interdiff.txt4.53 KBdww
#135 1538118.134_135a.interdiff.txt6.45 KBdww
#135 1538118-135b.patch17.74 KBdww
#135 1538118-135a.patch19.26 KBdww
#134 1538118.133_134.interdiff.txt1.17 KBdww
#134 1538118-134.9_x.patch15.18 KBdww
#133 1538118.132_133.9_x.interdiff.txt7.31 KBdww
#133 1538118.131_133.9_x.interdiff.txt3.28 KBdww
#133 1538118-133.9_x.patch15.22 KBdww
#132 1538118-132.update-report-with-messenger-error.png164.37 KBdww
#132 1538118-132.status-report-existing-error.png35.92 KBdww
#132 1538118-132.update-report-with-new-error.png268.43 KBdww
#132 1538118.131_132.interdiff.txt4.29 KBdww
#132 1538118-132.8_9_x.patch16.55 KBdww
#132 1538118-132.9_x.patch16.57 KBdww
#131 1538118-131.8_9_x.patch11.79 KBdww
#131 1538118-131.9_x.patch11.81 KBdww
#127 1538118-127.broken-php-default-setting.png232.02 KBdww
#127 1538118.124_127.9_x.interdiff.txt932 bytesdww
#127 1538118-127.8_9_x.patch11.62 KBdww
#127 1538118-127.9_x.patch11.64 KBdww
#125 1538118.121_124.8_9_x.interdiff.txt2.72 KBdww
#125 1538118.121_124.9_x.interdiff.txt2.72 KBdww
#125 1538118-124.8_9_x.patch12.23 KBdww
#125 1538118-124.9_x.patch12.26 KBdww
#121 1538118.119_121.interdiff.txt677 bytesdww
#121 1538118-121.8_9_x.patch9.99 KBdww
#121 1538118-121.9_x.patch10.01 KBdww
#119 1538118.101_118.rawdiff.txt5.18 KBdww
#119 1538118.118_119.interdiff.txt2.07 KBdww
#119 1538118-119.8_9_x.patch9.97 KBdww
#119 1538118-119.9_x.patch9.99 KBdww
#118 1538118-118.8_9_x.patch9.66 KBdww
#118 1538118-118.9_1_x.patch9.68 KBdww
#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 KByogeshmpawar
#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
Support from Acquia helps fund testing for Drupal Acquia logo

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?

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » 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

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.

DamienMcKenna’s picture

Would having a documented configuration option (not exposed through the UI) to use doRequestWithHTTPFallback() help for sites that are unfortunately stuck on servers with broken/outdated SSL systems?

moshe weitzman’s picture

@Damien - is that different from the option thats been discussed since #84?

colan’s picture

If we started bundling Certainty, would that eliminate the need for a fallback mechanism?

DamienMcKenna’s picture

@moshe: Apologies, I missed that part of the discussion.

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

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

samuel.mortenson’s picture

I also don't think we should support the HTTP fallback. If the HTTPS request fails we should display a message that looks similar to normal insecure messages, something to the effect of "Update status check failed, your site may be insecure. Please verify your openssl.cafile or openssl.capath settings in php.ini.".

David Strauss’s picture

Using Certainty would be overkill for our update infrastructure. If we want to add something to Drupal to cryptographically validate update metadata, we can ship a public key or specific CA roots without shipping a comprehensive set (via Certainty).

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

chrisck’s picture

Ran into this issue today with Drupal 9.0.2 using a proxy server. Update manager couldn't check for updates until Line 20 in UpdateFetcher.php was changed to include https protocol:

  const UPDATE_DEFAULT_URL = 'http://updates.drupal.org/release-history';

to

  const UPDATE_DEFAULT_URL = 'https://updates.drupal.org/release-history';
dww’s picture

FileSize
9.68 KB
9.66 KB

Ugh, sorry this totally fell off my radar. :( Here's a re-roll / port to modern core.

Some of this already happened in #2907863: Prefer HTTPS protocol over HTTP when fetching translations

These patches also fixes this:

1) Drupal\Tests\update\Kernel\Migrate\d6\MigrateUpdateConfigsTest::testUpdateSettings
Drupal\migrate\Plugin\Exception\BadPluginDefinitionException: The update_settings plugin must define the source_module property.

Interdiff can't hang, so here's raw diff of this vs. #101.

The "9_1_x" patch also applies cleanly to 9.0.x branch.

#95 still needs help, but let's see what the bot thinks of these.

dww’s picture

This mostly resolves #95 (IMHO). However, tagging for 'Needs documentation' for the proposed handbook page that's currently @todo in here. ;)

Also fix some other comments to be a bit more accurate, and use HTTP not http.

Forgot to attach 1538118.101_118.rawdiff.txt to my last comment, so here it is.

chrisck’s picture

I should also mention that my earlier comment #117 only fixed Update Manager to check contrib module updates. Drupal core still wasn't getting updates until deleting entries in "key_value" table from comment #7 No available releases found. Update Manager is working great and happy to see all green colours.

dww’s picture

Found what I think is a promising home for these docs:

https://www.drupal.org/docs/system-requirements/php-requirements#openssl

That already references this bug. ;) So I think we can just flesh that out.

Canceled the testbot on #119, since this is closer to RTBC.

Leaving the 'Needs documentation' tag since that section still needs to be expanded.

chrisck’s picture

Thank you for doing that. I hope the patch gets merged into Drupal core soon.

The last submitted patch, 118: 1538118-118.9_1_x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 118: 1538118-118.8_9_x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dww’s picture

I wonder why the bot didn't change the status. Maybe it got confused by me canceling the later test runs...

This should fix all the test failures and the 2 code standards violations.

I'm not 100% sure I'm handling the migrate UI test changes correctly. The deal is that the patch is enabling Update status in the D6 site, so the attempted migration was confused about that. Instead of just changing the test to ignore Update (status|manager), I think I'm making the appropriate change to indicate that we now have a migration for the Update status settings to move them to modern Update manager settings. Would love to get confirmation from someone who understands Migrate Drupal UI and its tests better.

Tagging for Bug Smash, too. ;)

quietone’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal/migrations/state/migrate_drupal.migrate_drupal.yml
@@ -46,6 +46,7 @@ finished:
+    update: core

Since the update module has a migration this can be removed. The state file in migrate_drupal only exists for modules that do not have migrations. There is already a state file, core/modules/update/migrations/state/update.migrate_drupal.yml declaring that the migration for the update module is finished.

dww’s picture

Sweet, thanks for reviewing, @quietone! These patches fix that. Test still passes locally.

Meanwhile, tagging for Manual testing, since there are no automated tests for this (can't be, unless we want to be testing The Internet, which I know we don't want).

However, reviewing some of the more "recent" comments, I'm not totally sure how to best proceed...

#104:

@David_Rothstein is strongly advocating to have the fallback on by default. Also some legit concerns on watchdog in case of initial failure vs fallback.

I think he's also totally right that we shouldn't be testing for only 'SSL certificate problem: Invalid'. That's not the failure mode I was hitting with a bogus openssl.cafile setting (see below), so the fallback wasn't doing anything. So I'm removing that check here and we'll now attempt the fallback (if configured) if the initial HTTPS request fails for any reason.

#105:

Some legit concerns on the UI. For example, if I intentionally break my php.ini in this regard, and try to fetch available updates manually, all I see is this:

Available updates report with PHP's OpenSSL misconfigured, failing to fetch info over HTTPS

If I check watchdog, I see some super cryptic yummy that No One(tm) is going to understand to mean "look in default.settings.php for something called "http fallback" :/

GuzzleHttp\Exception\RequestException: cURL error 77: error setting certificate verify locations: CAfile: /tmp/foo.pem CApath: none (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) in GuzzleHttp\Handler\CurlFactory::createRejection() (line 201 of /.../vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php).

So, uhh, yeah, I think this fallback will be next to useless in the real world without more help in here. :(

#112:

@samuel.mortenson is opposed to having the fallback at all. :(

larowlan’s picture

My thoughts:

  • we need to provide a fallback for low-end hosting where the site owner may not have full-control over their certs file. In my opinion not being notified of updates is worse than fetching them over http
  • I think the fallback needs to be opt-in (defaults to off) - we should ship secure by default (yes I'm looking at you full_html format)
  • Ideally we could use the update email functionality to notify people if updates couldn't be fetched at all, so that 'no updates because peer verification' doesn't just end up in the abyss of a syslog

The last submitted patch, 127: 1538118-127.9_x.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 127: 1538118-127.8_9_x.patch, failed testing. View results

dww’s picture

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

Weird, I thought I saw that test pass locally with the latest patch. :(

Trying to understand this test better, I think I now need to enable Update Manager in that test, so that the core/modules/update/migrations/state/update.migrate_drupal.yml file referenced by @quietone in #126 is actually being used. Confirmed this does pass locally now. 😉 What about the bot? 🤞

Technically still NW for the UI issues from #127/#128. The good news is that:

  • we need to provide a fallback for low-end hosting - Done
  • I think the fallback needs to be opt-in - Done

So only this needs work:

Ideally we could use the update email functionality to notify people if updates couldn't be fetched at all

I'd add that I think if we fail to fetch, we should print more than just what you see in the screenshot in #127. We should have a more robust message that says approximately:

Failed to fetch available update data:
- Check [link to "/admin/reports/dblog?type%5B%5D=update"]your local system log messages[end] for additional error messages.
- See /link/to/d.o for possible reasons this could happen and what you can do to resolve them.

dww’s picture

Upon further reflection, #128's idea on email is sort of a can of worms. I'd rather not delay this issue on sorting all of that out. So I spun it off into a separate issue: #3162336: Update manager should email admins when it fails to fetch available update data

Meanwhile, here's a first attempt at a better UI when the site fails to fetch available updates:

Available updates report when we fail to fetch update data

Some good news is that the status report already has a warning for this case:

Site status report when we fail to fetch available update data

Open questions:

  1. What handbook page should we link to? I guess we need to write something for this, too. Perhaps it should be a whole new page, not just https://www.drupal.org/docs/system-requirements/php-requirements#openssl ?
  2. Do we want this to be as flexible as I've made it with a new twig template for the message? Or should we just hard-code all this? Do we ever want to let site admins alter this?
  3. Would it be better to use the Messenger service for this? Seemed really sketchy and wrong for a template preprocess function to be invoking that. :/ Perhaps it could go directly into UpdateController::updateStatus() instead? When I tried that, I didn't love that the error message ends up above everything else on the page (although maybe that's better)? The nice thing about that is we wouldn't have to touch the existing update-report.html.twig template, which has some more yucky front end BC implications. It would also avoid the yuckiness of all the hard-coded mojo in the new template to make it look/act like an error from addError(). ;)
dww’s picture

Here's the version using the Messenger service directly from UpdateController::updateStatus() by comparison. No new templates. Everything's hard-coded. Definitely seems cleaner and less potential for front-end-BC trouble.

Interdiffs relative to both #131 and #132.

Available updates report when we fail to fetch update data, error message from the Messenger service

dww’s picture

FileSize
15.18 KB
1.17 KB

How convenient, ControllerBase already provides moduleHandler and currentUser services. ;) No new DI needed.
Also fixes a fatal in the previous patch since the version I uploaded forgot to use Url.

Still needs work, but getting closer. No sense testing this one on the bot.

dww’s picture

If we're going to warn site admins that they failed to fetch, we should also do it on the 'Update' form (e.g. /admin/reports/updates/update), not just the 'Available updates' report (/admin/reports/updates).

Therefore, I moved the error message generation into the UpdateManager service.

Here's two versions of that:
a) we jump through all the hoops to properly inject this service into the form
b) just use \Drupal::service() in the form to avoid the hassle

Still @todo for a handbook page. I've asked the #documentation Slack for input on where it should live and what it should be called, but so far, no one has replied. I'll try again.

Otherwise, if we agree to punt email to #3162336: Update manager should email admins when it fails to fetch available update data, and are happy with #134 as the answer to #132.2 + #132.3, this is basically ready for serious review + testing.

Added full summary template, accurate remaining tasks, added screenshots of the new stuff, tagging for UX review and release manager review.

benjifisher’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review +Needs issue summary update

We discussed this issue at the #3168518: Drupal Usability Meeting 2020-09-08.

  • Clarification: the effect on existing sites is that they may (depending on PHP configuration etc.) stop displaying update information. If they do, then they will have to follow the instructions on the to-be-written handbook page to fix the problem or enable the HTTP fallback. Is that right?
  • We need to mention this in the release notes. I am setting the status to NW and adding the tag for an IS update.
  • Question: what does the second message say, and where does the link go, if the dblog module is not enabled?
  • Do not use "the Drupal.org handbook" as the link text. Instead, use the page or section title of the page, something like "See Update Reports in the Drupal.org handbook ..."

Thanks for working on this issue. It is a little embarrassing that we are still using HTTP for the update information (not to mention potentially dangerous).

dww’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
17.74 KB
826 bytes

Thanks for reviewing this, @benjifisher and UX team!

Clarification: the effect on existing sites is that they may (depending on PHP configuration etc.) stop displaying update information. If they do, then they will have to follow the instructions on the to-be-written handbook page to fix the problem or enable the HTTP fallback. Is that right?

Exactly.

We need to mention this in the release notes. I am setting the status to NW and adding the tag for an IS update.

Added this as the release note:

The Update Manager how fetches available update data over HTTPS. Your site's PHP must have OpenSSL properly configured. If that's not possible, you can set $settings['update_fetch_with_http_fallback'] = TRUE; in your settings.php file to use the less secure HTTP fallback. See the PHP OpenSSL requirements section of the Drupal.org handbook for more details.

Crossed that off from remaining tasks and removing 'Needs issue summary update' tag.

Question: what does the second message say, and where does the link go, if the dblog module is not enabled?

That entire bullet is conditional on if dblog is enabled:

    if (\Drupal::moduleHandler()->moduleExists('dblog') && \Drupal::currentUser()->hasPermission('access site reports')) {
      $options = ['query' => ['type' => ['update']]];
      $watchdog_url = Url::fromRoute('dblog.overview', [], $options);
      $message['items']['#items'][] = $this->t('Check <a href="@url">your local system logs</a> for additional error messages.', ['@url' => $watchdog_url->toString()]);
    }
Do not use "the Drupal.org handbook" as the link text. Instead, use the page or section title of the page, something like "See Update Reports in the Drupal.org handbook ..."

Righto. Fixed here. I started the handbook page:

https://www.drupal.org/docs/system-requirements/php-openssl-requirements

Still need to flesh it out a bit, but at least we have a nid to work with and most of the key points are covered (or at least linked to). Crossing that much off the remaining tasks, since now this patch is potentially "final" (pending answers to all the open questions). Leaving the 'Needs documentation' tag until that page is "done".

There's only 1 more @todo in the whole patch, which is an implementation detail we might not want to mess with at all:

      // @todo Consider using 'fetch_failures' from the 'update' collection
      // in the key_value_expire service for this?

So, next steps:

  • Decide if we need the flexibility of a Twig template for this (#132) or if we can just use some hard-coded stuff via the Messenger service (#134).
  • Decide if we need DI for the UpdateManagerUpdate form (#135a) or can just use \Drupal::service() (#135b)
  • Decide what (if any) new automated testing we need for the error messages, fallback setting, etc.
  • Needs manual testing

Thanks!
-Derek

dww’s picture

New screenshots in the summary for the current UI changes from #137.

Note: if you click "check manually", you'll potentially see a duplicate message on the first page load, since the form itself is now always generating the error message, not just the existing error you get from update_fetch_data_finished(). Maybe it's considered scope creep to remove the current message from update_fetch_data_finished()? Or maybe that'd be better overall UX?

Duplicate error messages after clicking 'Check manually' link on available updates page

Thoughts?

Thanks,
-Derek

larowlan’s picture

Addressing the three questions in the 'remaining tasks' - here's my thoughts - I've discussed this with other committers too, there were no objections to my thoughts below, but equally, there was no feedback either - so these represent my personal opinion.

Do we need a twig template for the messages

I'm not aware of any other uses of the messenger where we require a twig-template, so I don't think we need to worry here either but note this comment.

What level of DI is required

+++ b/core/modules/update/src/UpdateManager.php
@@ -234,4 +235,27 @@ public function fetchDataBatch(&$context) {
+  public function getFetchErrorMessage(): array {

if this took both the module handler and the current user as arguments, it could be a static method.

both places this is called from have access to the module handler and the current user already either by ->currentUser()/->moduleHandler() methods or via an existing property.

then the question about DI would go away

yes, that means its no longer something people can sub-class and override, but given both the strings are translatable, modifying the message could be achieved with the translation system

However...

However, this method itself is just to build a render array, so in theory, we could also have a theme hook, and both of those places could use ['#theme' => 'update_update_error_message'] instead of adding the method to the service. So that also resolves the DI issue, as well as the first item. In that scenario the preprocess hook would build the items, and have the logic.

What sort of tests are required

Given we move this to a static method, we could unit test the getErrorMessages method. If we go with the theme approach, we could use a KernelTest
We should be able to use the mock http client to mimic failed results too.

dww’s picture

Issue summary: View changes
FileSize
21.13 KB
8.21 KB

Thanks for the feedback, @larowlan!

  • Converted to a Twig template and #theme
  • Added (initial?) Kernel tests for the error message generation
    • Without dblog
    • With dblog but a user without perms to see the logs
    • With dblog and an admin user
  • Removed some unused use.
  • Update summary API changes and remaining tasks sections.

Next steps:

  • Is this enough test coverage? ;)
  • Does this need a CR?
  • Needs manual testing
  • Maybe needs a product manager review?
  • Needs a release manager review to decide about backports with all these new strings and other changes

Thanks,
-Derek

Status: Needs review » Needs work

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

dww’s picture

Status: Needs work » Needs review
FileSize
22.36 KB
913 bytes

Whoops. Forgot about Stable9TemplateOverrideTest. Adding the new template to stable(9?) themes.

dww’s picture

Issue summary: View changes
Issue tags: -Needs documentation

I "finished" the OpenSSL PHP requirements page:

https://www.drupal.org/node/3170647/revisions/view/12059171/12067114

Removing the 'Needs documentation' tag.

Adding a post-commit remaining task to update that page with the correct version info once we know where this landed.

dww’s picture

Should we have another bullet point in the error message along the lines of "Confirm the site is connected to the public Internet"?

A) Before mucking with SSL certs, this would really be "step 0" for debugging this problem. Probably most likely with local sites, sites living in weirdly configured Docker environments, company intranets, etc. Maybe all of those are enough of an edge case that it's not worth it, but it occurs to me that should really be the first thing folks seeing this error should verify.

B) If the site is *not* connected to the net, the link to the d.o handbook page explaining what to do wouldn't work, either. So it'd be nice to provide some more info directly in the message, instead of assuming folks can get to d.o.

C) It makes it reasonable to always use an #item_list for the message, since there'd always be at least 2 items, even for sites without dblog enabled.

Thoughts?

Thanks,
-Derek

colan’s picture

Can we just check automatically by pinging something to see if there's Internet access? That would be better UX than saying, "Figure it out for yourself." Gnome's NetworkManager does this.

dww’s picture

@colan, re #145: Generally I'd agree with you -- if we can figure out more and tell the admins what we discovered, that'd be better.

However, I'm worried that if the thing we try to ping isn't up, we don't know if that particular server is down, or there's a network problem overall. It seems messy and complicated to have a list of servers to try, logic for retries, etc. Plus, we'd have to test if an http connection works (since we suspect an https failure could be from OpenSSL config, not network config), but then we don't know if we're talking to the server we think we are. All of which makes your suggestion complicated to get right. Which is why I was thinking of the simpler approach of just reminding site admins facing this situation to first check that the server is connected to the rest of the world at all.

But maybe that's not a common enough failure mode to justify making this error any more verbose or complex.

benjifisher’s picture

Issue summary: View changes

Typo: "now", not "how" in the Release notes snippet.

benjifisher’s picture

Status: Needs review » Needs work

We discussed this issue during the #3172419: Drupal Usability Meeting 2020-09-22.

Our main conclusion: the usability issues are minor and should not hold up this issue.

We do have three further suggestions:

  1. If the dblog module is not enabled, then still show the second bullet point, but without a link.
  2. Update the documentation page, but not the error message, to test that the server can access the public internet.
  3. Add a follow-up issue: create a requirements check (for /admin/reports/status) testing whether the server can access the public internet.

I guess that (2) and (3) amount to a recommendation that the suggestion in #144-146 be deferred to the documentation and a follow-up issue so that we can get this issue fixed.

I am not worried about the edge case described in #144(A). Anyone running a local copy of the site is sophisticated enough to read the new documentation page and sort things out. The edge case that I care about is probably very rare: shared hosting running PHP 7 but without OpenSSL.

Re #144(B): Do not confuse who is having trouble accessing external URLs: the server running the Drupal site or the browser viewing it. For example, the server may not have OpenSSL installed, but the browser should still be able to view the documentation on d.o.

#144(C) is a non-issue if you take suggestion (1) above. Personally, I think it is a non-issue even if you do not implement that suggestion.

colan’s picture

👍

dww’s picture

Status: Needs work » Needs review
Issue tags: +Needs followup, +Needs documentation updates
FileSize
22.74 KB
2.66 KB
1.21 KB

Re: #148: Thanks for re-reviewing!

148.1: Fixed

2 and 3 are still todo so tagging for those, but I at least wanted to get a new patch up for code review (especially if the test coverage is sufficient here). Needed a reroll, so interdiff is confused. But here's the local commit that's the only real diff vs #142.

Thanks,
-Derek

dww’s picture

tedbow’s picture

Status: Needs review » Needs work

wow, this is so close! this would be a great improvement.

  1. +++ b/core/assets/scaffold/files/default.settings.php
    @@ -308,6 +308,20 @@
    +# $settings['update_fetch_with_http_fallback'] = FALSE;
    
    diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc
    index 8f8d000..7e12a61 100644
    

    Should this end with TRUE.

    The only reason they would uncomment this line would be to set this to TRUE correct. Having it set to FALSE is that same as leaving commented out, correct?

    Or do we want them to be extra sure?

    I think for other settings such as $settings['reverse_proxy'], $settings['omit_vary_cookie'], , where we have commented out boolean setting we have the value set that would be the non-default setting.

  2. +++ b/core/includes/install.core.inc
    @@ -249,7 +249,7 @@ function install_state_defaults() {
    -    'server_pattern' => 'http://ftp.drupal.org/files/translations/%core/%project/%project-%version.%language.po',
    +    'server_pattern' => 'https://ftp.drupal.org/files/translations/%core/%project/%project-%version.%language.po',
    

    Is this change related here? Is this just left from #2907863: Prefer HTTPS protocol over HTTP when fetching translations ?

    I don't see anything in the summary about this. Could this just be 1 line patch it isn't on issue?

  3. +++ b/core/modules/update/src/Controller/UpdateController.php
    @@ -19,13 +21,23 @@ class UpdateController extends ControllerBase {
    -  public function __construct(UpdateManagerInterface $update_manager) {
    +  public function __construct(UpdateManagerInterface $update_manager, RendererInterface $renderer) {
    

    Do we need to make the new argument to the constructor optional?

  4. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
    @@ -245,6 +251,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      $this->messenger()->addError(\Drupal::service('renderer')->renderPlain($message));
    

    I see there is TBD about whether to use dependency injection with renderer but I am not sure why we won't. Because it is already used in the class without DI? Since this is critical issue I don't think we need change this if that is the reason.

  5. +++ b/core/modules/update/src/UpdateFetcher.php
    @@ -41,17 +42,27 @@ class UpdateFetcher implements UpdateFetcherInterface {
    -  public function __construct(ConfigFactoryInterface $config_factory, ClientInterface $http_client) {
    +  public function __construct(ConfigFactoryInterface $config_factory, ClientInterface $http_client, Settings $settings) {
    

    Don't we usually make new arguments to a constructor optional and a put warning that the argument will be required in the next major? I know I have seen this is controllers but maybe not service classes?

  6. +++ b/core/modules/update/src/UpdateFetcher.php
    @@ -59,6 +70,26 @@ public function __construct(ConfigFactoryInterface $config_factory, ClientInterf
    +  protected function doRequest($url, array $options, $with_http_fallback) {
    

    We can add the type for $url and $with_http_fallback and the return type.

  7. +++ b/core/modules/update/src/UpdateFetcher.php
    @@ -67,6 +98,10 @@ public function fetchProjectData(array $project, $site_key = '') {
    +      if ($with_http_fallback) {
    

    Should we check here if protocol is already 'http://'. Looking at \Drupal\update\UpdateFetcher::getFetchBaseUrl() it looks like there a couple ways the base url could be something custom. If it is already using http: we don't need to make the request.

  8. +++ b/core/modules/update/tests/src/Kernel/UpdateReportTest.php
    @@ -54,4 +57,54 @@ public function providerTemplatePreprocessUpdateReport() {
    +    template_preprocess_update_fetch_error_message($variables);
    ...
    +    template_preprocess_update_fetch_error_message($variables);
    ...
    +    template_preprocess_update_fetch_error_message($variables);
    

    Should we just set up a render array with
    '#theme' => 'update_fetch_error_message', render it and confirm that markup we expect is produced instead of calling template_preprocess_update_fetch_error_message() directly? Would that work in a kernel test?

    That we could be a more complete test. Currently if you changed the update_theme() to have an incorrect entry this test would still pass, correct? If we actually rendered it we would be testing our new update_theme entry and the preprocess are setup correctly together.

  9. +++ b/core/modules/update/update.module
    @@ -161,6 +161,10 @@ function update_theme() {
    +    'update_fetch_error_message' => [
    +      'file' => 'update.report.inc',
    +      'render element' => 'element',
    +    ],
    

    should we have
    'variables' => ['error_message' => []] here?

  10. A couple suggestions for further test coverage

    We could add a method to \Drupal\Tests\update\Unit\UpdateFetcherTest that would test that if we get an exception when request with https then another request is made with the same url but http.

    We could add a new functional test where we use mocked http_client in the container to confirm the that error message is shown if the fallback is not used and there is an exception with https. Then another method confirm that the fallback will be used and no message shown.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

dww’s picture

Status: Needs work » Needs review
FileSize
22.55 KB
4.42 KB

@tedbow - Thanks for the review!

  1. Sure, changed it to TRUE. I thought we left things commented with their default value, but your counter examples are true, too. ;) I did like forcing people to go even more out of their way. But I don't want it to be too confusing and clumsy, either.
  2. Yeah, mostly unrelated. I think that was the result of grep. Moved to #3179318: Always use HTTPS for fetching translations
  3. Done. I opened a CR about it for use in the deprecation message. That said, I believe we're going to want to backport this. When we do, I guess we'll just leave off the deprecation warning, but still make the argument optional.
  4. Yeah, it's already used without DI in the same class. Seems not worth the additional complications and hassles, and further delaying this critical fix.
  5. Yeah, probably wise. ;) Done.
  6. Done.
  7. Good catch, done.
  8. @todo
  9. Yes, done.
  10. @todo

So, we could still expand the test coverage here, but I wanted to get a patch up for everything else.

Thanks again for the review!

Cheers,
-Derek

tedbow’s picture

Status: Needs review » Needs work

@dww thanks for addressing my review.

  1. Sorry i should have pointed out this appears in
    core/assets/scaffold/files/default.settings.php
    sites/default/default.settings.php

    and sites/default/default.settings.php(which hasn't been changed). I think those files should be exactly the same. This is 1 difference now.

  2. 👍
  3. Need to add the |null for the @param for $renderer
  4. 👍, Seems fine not to change this for a critical fix
  5. Still needs the = NULL for the parameter to make actually optional and update the @param comment
  6. 👍
  7. 👍
  8. ok, a todo
  9. 👍
  10. ok, a todo
dww’s picture

@tedbow thanks again!

  1. No worries, I already changed both spots (they're at the opposite ends of the interdiff and patch). We have a test that fails if you don't keep the 2 files in sync, and #154 passed all tests.
  2. Whoops, good catch.
  3. 👍whoops 😉
  4. Yes, rendering in a Kernel test like that works fine. How's this?
  5. Still @todo. Not sure it's needed for this, maybe could be some follow-ups?

Also, noticed that $watchdog_url isn't as good a variable name as $dblog_url in the modern era. ;) That's the other hunk in the interdiff.

RTBC? 🤞🙏

Thanks!
-Derek

p.s. Updating remaining tasks since CR now exists.

dww’s picture

Based on a quick Slack chat with @larowlan, we agreed it was worth adding the Unit test coverage of #152.10, but not the Functional. Now that we've got the Unit test confirming that the http fallback happens as expected, and the Kernel test is confirming the render array looks right, we both think that the Functional test isn't needed. This is based on the approach outlined at https://www.previousnext.com.au/blog/testing-code-makes-http-requests-dr... but modified for Unit testing (that article assumes Kernel tests).

This uncovers yet more yuckiness with watchdog_exception(), so adding #2932518: Deprecate watchdog_exception as related. ;)

tedbow’s picture

Status: Needs review » Reviewed & tested by the community
  1. My bad, it was problem with how I applied the patch locally
  2. looks good
  3. looks good
  4. I had meant to do the render so we won't have to call template_preprocess_update_fetch_error_message() directly but I guess having both is more complete test coverage. Looks good.
  5. yes the unit test works well
alexpott’s picture

... fwiw the messenger supports render arrays.

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -245,6 +251,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    if ($fetch_failed) {
+      $message = ['#theme' => 'update_fetch_error_message'];
+      $this->messenger()->addError(\Drupal::service('renderer')->renderPlain($message));
+    }

You've done the deprecation dance elsewhere but not here - how come? Ah I see because the form already uses \Drupal::service()... the existing usage looks a bit unnecessary. Looks ok to address in a follow-up.

Also re watchdog_exception() - the fix for that is #3151118: Include bootstrap.inc using composer - it's a shame that didn't land already.

dww’s picture

Assigned: Unassigned » dww
Status: Reviewed & tested by the community » Needs work

... fwiw the messenger supports render arrays.

TIL! Wow, that makes most of the API changes in this patch unnecessary. Re-working now...

dww’s picture

Assigned: dww » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
27.53 KB
6.39 KB

Okay, maybe not 'all' of the API changes. ;) But certainly some of them. Updated the summary and CR accordingly.

Also fixing these from self-review:

  1. +++ b/core/modules/update/src/UpdateFetcher.php
    @@ -41,17 +42,31 @@ class UpdateFetcher implements UpdateFetcherInterface {
    +      @trigger_error('The settings service should be passed to UpdateFetcherr::__construct() since 9.1.0. This will be required in Drupal 10.0.0. See https://www.drupal.org/node/3179315', E_USER_DEPRECATED);
    

    s/Fetcherr/Fetcher/

  2. +++ b/core/modules/update/src/UpdateFetcher.php
    @@ -59,6 +74,26 @@ public function __construct(ConfigFactoryInterface $config_factory, ClientInterf
    +  protected function doRequest(string $url, array $options, bool $with_http_fallback) : string {
    

    Although we're not totally consistent, it seems that we generally don't want the space before the : in return type definitions.

  3. +++ b/core/modules/update/tests/src/Unit/UpdateFetcherTest.php
    @@ -97,4 +144,94 @@ public function providerTestUpdateBuildFetchUrl() {
    +      new Response('500', [], 'https failed'),
    ...
    +    // It should have only been an https request.
    ...
    +      new Response('500', [], 'https failed'),
    +      new Response('200', [], 'http worked'),
    ...
    +    // The first should have been https and should have failed.
    ...
    +    // The second should have been the http fallback and should have worked.
    ...
    +    $this->assertEquals('http worked', $data);
    

    Capitalize HTTP(S?) in comments and content.

p.s. Follow-up to remove the other renderer we don't need: #3180382: UpdateManagerUpdate doesn't need to use the renderer service

dww’s picture

FileSize
27.52 KB
699 bytes

Sorry, not sure how I missed the CS violations. Fixed here.

alexpott’s picture

@dww I'm so sorry... I meant to remove

... fwiw the messenger supports render arrays.

It was an aside... and not one that was meant to influence the patch. The "supports" should have been in quotes. It's an undocumented and untested feature. I believe there is an instance somewhere in core where it's used but I'm not sure. Here is a long painful discussion of the issue of render arrays in messages - #2505497: Support render arrays for drupal_set_message().

I think we need to #157 plus any relevant code style fixes from #161 and #162.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/tests/src/Unit/UpdateFetcherTest.php
@@ -97,4 +144,96 @@ public function providerTestUpdateBuildFetchUrl() {
+namespace Drupal\update;
+
+use Drupal\Core\Logger\RfcLogLevel;
+
+/**
+ * Mock version of watchdog_exception().
+ *
+ * This is unfortunate, but UpdateFetcher::doRequest() calls
+ * watchdog_exception(), which is defined in includes/bootstrap.inc, which
+ * cannot be included for Unit tests or we break unit test encapsulation.
+ * Therefore, we define our own copy in the \Drupal\update namespace that
+ * doesn't do anything (we're not trying to test the exception logging).
+ *
+ * @todo Remove in https://www.drupal.org/project/drupal/issues/2932518
+ */
+function watchdog_exception($type, \Exception $exception, $message = NULL, $variables = [], $severity = RfcLogLevel::ERROR, $link = NULL) {
+  // No-op.
 }

This is not necessary now that #3151118: Include bootstrap.inc using composer has landed.

ayushmishra206’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
25.81 KB

Made the changes suggested in #164.

alexpott’s picture

+++ b/core/modules/update/tests/src/Unit/UpdateFetcherTest.php
@@ -218,22 +218,3 @@ public function testUpdateFetcherHttpFallback() {
   }
 
 }
-
-namespace Drupal\update;
-
-use Drupal\Core\Logger\RfcLogLevel;
-
-/**
- * Mock version of watchdog_exception().
- *
- * This is unfortunate, but UpdateFetcher::doRequest() calls
- * watchdog_exception(), which is defined in includes/bootstrap.inc, which
- * cannot be included for Unit tests or we break unit test encapsulation.
- * Therefore, we define our own copy in the \Drupal\update namespace that
- * doesn't do anything (we're not trying to test the exception logging).
- *
- * @todo Remove in https://www.drupal.org/project/drupal/issues/2932518
- */
-function watchdog_exception($type, \Exception $exception, $message = NULL, $variables = [], $severity = RfcLogLevel::ERROR, $link = NULL) {
-  // No-op.
-}

Now we don;t have two namespaces in this file we don't need to enclose the other namespace...

Plus #163 needs addressing.

Status: Needs review » Needs work

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

paulocs’s picture

Assigned: Unassigned » paulocs
paulocs’s picture

Assigned: paulocs » Unassigned
alexpott’s picture

Status: Needs work » Needs review
FileSize
8.2 KB
29.93 KB

Here's a patch based off of #157 that fixes the coding standards and the test to not mock the watchdog_exception function.
Interdiff back to #157.

dww’s picture

@alexpott: Thanks for #170! All looks great to me, but I can't RTBC this. I very much appreciate your efforts to improve this patch, and I love the testing of the error logging, too.

Thanks for the link to #2505497: Support render arrays for drupal_set_message() -- only 1/3 of the way through, but I see it's a sticky mess... ;)

I'll ping Ted to see if they'd be willing to re-RTBC this.

Thanks again,
-Derek

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Review this again #170 does have fixes from #161(minsus removal of the renderer service) and #163

🎉

larowlan’s picture

Saving issue credits and updating tags

I don't think this needs release manager review, there's no major API changes etc.

I discussed possibility of backporting this to 9.1 during beta with Catch and he was on-board with that.

  • larowlan committed dcd4464 on 9.2.x
    Issue #1538118 by dww, swentel, dawehner, pwolanin, sanduhrs, alexpott,...
larowlan’s picture

Title: Update status does not verify the identity or authenticity of the release history URL » [backport] Update status does not verify the identity or authenticity of the release history URL
Version: 9.2.x-dev » 9.1.x-dev

Committed dcd4464 and pushed to 9.2.x. Thanks!

Published change record.

Marked for backport to 9.1.x after freeze is over.

larowlan’s picture

Note to self: the change notice went out with 9.2.x but will need updating if backported

xjm’s picture

This should not be backported after the freeze. It's got API additions, which have a beta deadline. That said, we had to revert something earlier today so @catch and I are comfortable with it being backported during the freeze.

Also, this includes an API addition to default.settings.php, which is one of the things we always mention in release notes. The release note can mention that it's a changed default for new sites and that old sites will have the old behavior, and then link the CR.

  • larowlan committed aedc357 on 9.1.x
    Issue #1538118 by dww, swentel, dawehner, pwolanin, sanduhrs, alexpott,...
larowlan’s picture

Title: [backport] Update status does not verify the identity or authenticity of the release history URL » Update status does not verify the identity or authenticity of the release history URL
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release note

Backported per #177

Added link to CR to the release note

Congrats all

dww’s picture

Yay!! 🎉🎉 Huzzah that this is in. Thanks, everyone!

FWIW, I don't think linking to the CR in the release note is a good idea. That CR is of dubious value for the "API change" of some constructors on classes that probably should be @internal taking new injected services as optional arguments. 😉 The end-user-relevant docs are all in the extensive handbook page that was already linked in the previous release note snippet. I don't want to unilaterally change/revert it, but I'd invite folks to click on both links in this comment and decide for yourself.

Thanks again,
-Derek

dww’s picture

Re: #176 -- I just edited the CR to update the version info. Also added a link to the handbook page for completeness / cross-referencing.

Cheers,
-Derek

larowlan’s picture

Issue summary: View changes

I've removed the link from the release notes, I agree

We have a @todo in the handbook page for a screenshot, I guess we can use the ones here?

dww’s picture

Thanks for the reminder, @larowlan!

Needs documentation updates : https://www.drupal.org/node/3170647/revisions/view/12084142/12118988 ;)

Added the latest screenshot, removed the @todo about versions (now that we know it landed in 9.1.0).

Thanks again,
-Derek

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

longwave’s picture