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 toTRUE
(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
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)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.Decide if we need DI for theNo new DI, it's a template_preprocess() now (see #139).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.Kernel tests added in #140.- Needs manual testing
Needs UX reviewSee #136- Maybe needs a product manager review?
- Needs a release manager review to decide about backports with all these new strings and other changes
Release note snippet?CR?Draft CR- RTBC
- Commit
- https://www.drupal.org/node/3170647/edit to update the version info in the docs about this.
- 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
Update form
Cryptic watchdog message
This is why we need a handbook page. ;)
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
withtemplate_preprocess_update_fetch_error_message()
- New
UpdateSettings
migrate source plugin \Drupal\update\UpdateFetcher
(the 'update.fetcher' service) now needs the siteSettings
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.
Comment | File | Size | Author |
---|---|---|---|
#170 | 1538118-170.patch | 29.93 KB | alexpott |
#170 | 157-170-interdiff.txt | 8.2 KB | alexpott |
#165 | 1538118-165.patch | 25.81 KB | ayushmishra206 |
#165 | interdiff_162-165.txt | 2.05 KB | ayushmishra206 |
#162 | 1538118-162.patch | 27.52 KB | dww |
Comments
Comment #1
Wim LeersThis applies to both 7 and 8.
In 8's
\Drupal\update\UpdateFetcher
:IMHO that using HTTP instead of HTTPS, even though it's perfectly accessible over HTTPS, is a gross oversight.
Comment #2
Wim Leersi.e. why not just do this?
Comment #3
Dave ReidComment #4
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIn 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).
Comment #5
Wim LeersOh wow, I'd think that happens automatically? :O
I was thinking the same.
Comment #6
cilefen CreditAttribution: cilefen commentedI added the words "identity" and "authenticity" to the title because I think we need both.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedFYI Drush already uses SSL for updates.drupal.org. Would be good to double check with infra team before making this change.
Comment #9
cilefen CreditAttribution: cilefen commentedWhat about a gpg signature that goes alongside the xml file?
Comment #11
drummDrupal.org's infra is ready for this change.
Comment #12
Heine CreditAttribution: Heine commentedThis issue is about Drupal core not about politics or publications. Please keep it on topic. I've deleted OT comments.
Comment #13
drummMaybe 8.1 is the right place.
Comment #14
alexpottTo 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.
Comment #15
alexpottI 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!
Comment #16
Heine CreditAttribution: Heine commentedI'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:
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?
Comment #17
alexpottGiven #16 I'm going to revert the commit and wait for clarification.
[edit] I never pushed so all is good in the world :)
Comment #18
Heine CreditAttribution: Heine commentedI've found the following statement on old php documentation:
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.
Comment #21
swentel CreditAttribution: swentel commentedShould we do the same for LOCALE_TRANSLATION_DEFAULT_SERVER_PATTERN in locale.module ?
Comment #22
drummYes, all updates.drupal.org URLs can be updated.
Comment #23
swentel CreditAttribution: swentel commentedChecked the whole codebase for URL's pointing to *.drupal.org.
(edit - ignore the first patch here)
Comment #24
swentel CreditAttribution: swentel commentedComment #25
klausithe string length also needs to be updated in this serialized string.
Comment #26
swentel CreditAttribution: swentel commentedRight
Comment #29
alexpottLet'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...
Comment #30
dstol@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.
Comment #31
mgiffordI 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.
Comment #33
klausiThe 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?
Comment #34
cilefen CreditAttribution: cilefen commented#509010: Build a Package signing system for d.o. so that people can securely and simply download and verify packages
Comment #35
swentel CreditAttribution: swentel commentedThis should do it
Comment #36
alexpottOf 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
Comment #37
swentel CreditAttribution: swentel commentedYeah, crossed my mind as well, do strpos on the value to see if it points to drupal.org then I guess ?
Comment #38
alexpott@swentel yeah I guess so
Comment #39
swentel CreditAttribution: swentel commentedDone. 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.
Comment #42
hass CreditAttribution: hass commentedAdding blockers
Comment #43
hass CreditAttribution: hass commentedComment #44
cilefen CreditAttribution: cilefen commented#2654474: Getting update info results in RequestException : cURL error 60: SSL certificate problem: unable to get local issuer certificate
Comment #45
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRemoving tag since per above this did not wind up going into 8.0.3.
Comment #46
mgiffordOk, 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.
Comment #47
swentel CreditAttribution: swentel commentedI think someone needs to RTBC it so committers can have a look at it, don't really think anything is missing.
Comment #48
gregglesLooks good to me.
Comment #49
hass CreditAttribution: hass commentedI think this need to wait for the two issues I linked too.
Comment #50
drumm#1879970: drupal_http_request fails when remote server is using openssl v1.0.0 and #924498: Proxy https support for drupal_http_request() are 7.x issues. They should not hold up 8.x.
Comment #51
hass CreditAttribution: hass commentedThan 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
Comment #52
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIn 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.
Comment #53
drummCould we trap and warn about errors, then fall back to http?
Comment #54
ianthomas_ukYou'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.
Comment #55
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #56
catchWe 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.
Comment #58
pwolanin CreditAttribution: pwolanin as a volunteer commentedWe 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.
Comment #59
alexpottWith regards to falling back #2367319-82: Implement automatic background updates for highly critical security issues is relevant - basically wordpress is falling back - I think
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.
Comment #60
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.)
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
Comment #61
cilefen CreditAttribution: cilefen as a volunteer commentedI saw this: https://theupdateframework.github.io
Comment #62
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDiscussed 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.
Comment #63
dsnopekThis 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 :-)
Comment #65
cilefen CreditAttribution: cilefen as a volunteer commentedSee #606592-94: Allow updating core with the update manager about Wordpress's potentially catastrophic auto-update vulnerability as this pertains to that issue.
Comment #66
cilefen CreditAttribution: cilefen as a volunteer commentedWordPress to get secure, cryptographic updates
Comment #67
cilefen CreditAttribution: cilefen as a volunteer commentedComment #69
dwwYeah, when update manager was added to Drupal, the d.o infra still didn't support https. :/ Glad we're finally resolving this.
Comment #70
drummWith 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.
Comment #71
drummHiding 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.
Comment #73
dawehnerI started to implement that fallback.
Note: https://badssl.com/ is a great site.
Comment #75
dawehnerThis time even with the missing files.
Comment #76
dawehnerSorry for the spam :(
Comment #77
colanLooks like this will drop the "://" when switching to HTTP.
Comment #78
dwwHere'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
Comment #80
borisson_I have some nits to pick, but this looks great.
This docblock should be removed, we don't do @file docblocks for classes anymore.
/s/not https/http/ to make it more explicit what's happening?
/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?
Comment #81
yogeshmpawarComment #82
yogeshmpawarChanges made as per comment #80 & previous patch failed to apply on 8.4.x branch
Comment #83
cilefen CreditAttribution: cilefen as a volunteer commentedThere is an issue open against packagist to employ a Chronicle ledger.
Comment #84
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedDiscussing 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?
Comment #85
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedHere's a quick update to add a setting to control the fallback behavior.
Comment #86
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.Comment #87
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt 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 :)
Comment #89
David StraussI 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?
Comment #90
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedLooks 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.
Comment #91
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedThis adds the missing change to default.settings.php and fixes the test I think
Comment #92
David StraussThe 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.
Comment #93
drummTested a fresh install with this and it looks good to me.
Comment #95
larowlanI agree on the 'if you want fallback you have to opt-in approach'
David's comment from #87 hasn't been addressed yet.
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
Comment #96
larowlanHappy 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
Comment #97
larowlansuggest
doRequestWithOptionalHTTPFallback
for the name to address #87?Comment #98
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedHow about just method name
doRequest()
?Comment #99
larowlan+1
Comment #100
borisson_+1 for #98.
Comment #101
sanduhrsPatch against 8.5.x attached, addressing #87
Comment #103
sanduhrsAttach corrected patch.
Comment #104
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHaving 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:
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.
Comment #105
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIf 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.
Comment #107
DamienMcKennaWould 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?
Comment #108
moshe weitzman CreditAttribution: moshe weitzman commented@Damien - is that different from the option thats been discussed since #84?
Comment #109
colanIf we started bundling Certainty, would that eliminate the need for a fallback mechanism?
Comment #110
DamienMcKenna@moshe: Apologies, I missed that part of the discussion.
Comment #112
samuel.mortensonI 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.".
Comment #113
David StraussUsing 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).
Comment #117
chrisck CreditAttribution: chrisck commentedRan 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:
to
Comment #118
dwwUgh, 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:
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.
Comment #119
dwwThis 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.
Comment #120
chrisck CreditAttribution: chrisck commentedI 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.
Comment #121
dwwFound 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.
Comment #122
chrisck CreditAttribution: chrisck commentedThank you for doing that. I hope the patch gets merged into Drupal core soon.
Comment #125
dwwI 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. ;)
Comment #126
quietone CreditAttribution: quietone as a volunteer commentedSince 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.
Comment #127
dwwSweet, 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:
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" :/
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. :(
Comment #128
larowlanMy thoughts:
Comment #131
dwwWeird, 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:
So only this needs work:
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:
Comment #132
dwwUpon 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:
Some good news is that the status report already has a warning for this case:
Open questions:
Messenger
service for this? Seemed really sketchy and wrong for a template preprocess function to be invoking that. :/ Perhaps it could go directly intoUpdateController::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(). ;)Comment #133
dwwHere'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.
Comment #134
dwwHow 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.
Comment #135
dwwIf 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 hassleStill @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.
Comment #136
benjifisherWe discussed this issue at the #3168518: Drupal Usability Meeting 2020-09-08.
dblog
module is not enabled?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).
Comment #137
dwwThanks for reviewing this, @benjifisher and UX team!
Exactly.
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 yoursettings.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.
That entire bullet is conditional on if dblog is enabled:
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:
So, next steps:
UpdateManagerUpdate
form (#135a) or can just use\Drupal::service()
(#135b)Thanks!
-Derek
Comment #138
dwwNew 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 fromupdate_fetch_data_finished()
? Or maybe that'd be better overall UX?Thoughts?
Thanks,
-Derek
Comment #139
larowlanAddressing 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
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.
Comment #140
dwwThanks for the feedback, @larowlan!
Next steps:
Thanks,
-Derek
Comment #142
dwwWhoops. Forgot about Stable9TemplateOverrideTest. Adding the new template to stable(9?) themes.
Comment #143
dwwI "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.
Comment #144
dwwShould 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
Comment #145
colanCan 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.
Comment #146
dww@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.
Comment #147
benjifisherTypo: "now", not "how" in the Release notes snippet.
Comment #148
benjifisherWe 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:
dblog
module is not enabled, then still show the second bullet point, but without a link./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.
Comment #149
colan👍
Comment #150
dwwRe: #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
Comment #151
dww#148.2: Doc edits: https://www.drupal.org/node/3170647/revisions/view/12067127/12084142
#148.3: Follow-up: #3175086: Add a status report check that the site can connect to the public Internet
Still needs review, sign-off on the test coverage, etc...
Thanks!
-Derek
Comment #152
tedbowwow, this is so close! this would be a great improvement.
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.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?
Do we need to make the new argument to the constructor optional?
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.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?
We can add the type for $url and $with_http_fallback and the return type.
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 usinghttp:
we don't need to make the request.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 callingtemplate_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 newupdate_theme
entry and the preprocess are setup correctly together.should we have
'variables' => ['error_message' => []]
here?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.Comment #154
dww@tedbow - Thanks for the review!
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
Comment #155
tedbow@dww thanks for addressing my review.
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.|null
for the@param
for$renderer
= NULL
for the parameter to make actually optional and update the @param commentComment #156
dww@tedbow thanks again!
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.
Comment #157
dwwBased 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. ;)Comment #158
tedbowtemplate_preprocess_update_fetch_error_message()
directly but I guess having both is more complete test coverage. Looks good.Comment #159
alexpott... fwiw the messenger supports render arrays.
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.
Comment #160
dwwTIL! Wow, that makes most of the API changes in this patch unnecessary. Re-working now...
Comment #161
dwwOkay, maybe not 'all' of the API changes. ;) But certainly some of them. Updated the summary and CR accordingly.
Also fixing these from self-review:
s/Fetcherr/Fetcher/
Although we're not totally consistent, it seems that we generally don't want the space before the : in return type definitions.
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 serviceComment #162
dwwSorry, not sure how I missed the CS violations. Fixed here.
Comment #163
alexpott@dww I'm so sorry... I meant to remove
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.
Comment #164
alexpottThis is not necessary now that #3151118: Include bootstrap.inc using composer has landed.
Comment #165
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade the changes suggested in #164.
Comment #166
alexpottNow we don;t have two namespaces in this file we don't need to enclose the other namespace...
Plus #163 needs addressing.
Comment #168
paulocsComment #169
paulocsComment #170
alexpottHere'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.
Comment #171
dww@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
Comment #172
tedbowReview this again #170 does have fixes from #161(minsus removal of the renderer service) and #163
🎉
Comment #173
larowlanSaving 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.
Comment #175
larowlanCommitted dcd4464 and pushed to 9.2.x. Thanks!
Published change record.
Marked for backport to 9.1.x after freeze is over.
Comment #176
larowlanNote to self: the change notice went out with 9.2.x but will need updating if backported
Comment #177
xjmThis 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.Comment #179
larowlanBackported per #177
Added link to CR to the release note
Congrats all
Comment #180
dwwYay!! 🎉🎉 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
Comment #181
dwwRe: #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
Comment #182
larowlanI'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?
Comment #183
dwwThanks 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
Comment #185
longwaveOpened #3293648: [D7 backport] Update status does not verify the identity or authenticity of the release history URL for D7 backport.