New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Remove the FTP Layer #21507

Open
wants to merge 19 commits into
base: 4.0-dev
from

Conversation

@roland-d
Copy link
Contributor

roland-d commented Aug 9, 2018

Summary of Changes

It is time to bring Joomla into the modern era, FTP layers should not be needed with modern webhosting. This PR removes the FTP layer from:

  • Global Configuration
  • Joomla Updater
  • Installer

Testing Instructions

Global Configuration

  1. Go to System -> Global Configuration
  2. Click on the Server Tab
  3. Check that the FTP options above the Proxy Settings is gone

Joomla Updater

  1. Go to Components -> Joomla! Updater
  2. Click on Upload & Update
  3. The option Installation method should be gone

Installer

  1. Go to Components -> Install
  2. Check that the FTP Login Details tab is gone
  3. Go to Update
  4. Check that the FTP Login Details block is gone
  5. Go to Manage
  6. Check that the FTP Login Details block is gone
  7. Go to Discover
  8. Check that the FTP Login Details block is gone

Installation testing

Joomla Updater

This is tricky because there needs to be a Joomla Update or you need to build your own update server.

  1. Go to Components -> Joomla! Updater

  2. Check for an update

  3. Install the available update

  4. See that the installation completes

  5. Go to Components -> Joomla! Updater

  6. Click on the Upload & Update tab

  7. Select a Joomla installation package

  8. Click on Upload & Install

  9. See that the installation completes

Installer

  1. Go to Components -> Install

  2. Select an extension to Install

  3. See that the installation completes

  4. Go to Update

  5. Check for Updates

  6. Select an extension to update

  7. Check that the installation completes

  8. Go to Manage

  9. Select an extension to uninstall

  10. Check that the installation completes

  11. Go to Discover

  12. Click on Discover to find an extension to install

  13. Select an extension to install

  14. Check that the installation completes

Expected result

There are no more FTP settings available

Documentation Changes Required

https://docs.joomla.org/Why_can%27t_you_install_any_extensions%3F
https://docs.joomla.org/How_to_use_the_filesystem_package
https://docs.joomla.org/Joomla_Core_Features
https://docs.joomla.org/J3.x:Joomla_Core_Features

Remove the FTP Layer
Signed-off-by: Roland Dalmulder <contact@rolandd.com>

@roland-d roland-d added this to the Joomla 4.0 milestone Aug 9, 2018

@roland-d roland-d self-assigned this Aug 9, 2018

@roland-d roland-d requested review from mbabker , rdeutz and Bakual Aug 9, 2018

@roland-d roland-d requested a review from brianteeman as a code owner Aug 9, 2018

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

dgrammatiko commented Aug 9, 2018

@roland-d you know you're gonna make few people unhappy, right?

@@ -747,16 +747,9 @@ public static function getStream($use_prefix = true, $use_network = true, $ua =
if ($use_prefix)
{
$FTPOptions = \JClientHelper::getCredentials('ftp');

This comment has been minimized.

@mbabker

mbabker Aug 9, 2018

Member

This bit actually isn't bad to retain support for as it allows FTP streams, which is a separate thing from the file/folder classes having the FTP layer/fallback. Everything else being gone seems fair to me.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 9, 2018

Thanks for this.

I think there may be some extra steps to remove for the installation template but it is hard to check on my phone. I can check in the morning

@richard67

This comment has been minimized.

Copy link
Contributor

richard67 commented Aug 10, 2018

The testing instructions only describe to check if options have gone. They don’t tell to check that things work as before. But this has to be tested, especially by people with shared hosting.

@roland-d

This comment has been minimized.

Copy link
Contributor Author

roland-d commented Aug 10, 2018

@richard67 You are correct, I am going to update. I realized the same thing when I went to sleep :P

@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Aug 10, 2018

You know that you remove an important security feature?

@roland-d

This comment has been minimized.

Copy link
Contributor Author

roland-d commented Aug 10, 2018

@HLeithner Care to explain?

@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Aug 10, 2018

Sure,

normally you don't want that your software can manipulate it self. Yes we want it in some situations, f. ex. updates. The reset of the time we maybe want to write to some directories (image uploads, temp dir, cache dir, log dir). All of them doesn't have to execute code.

So if you do you server configuration right you don't want/have write access by CMS to any directory on the server, best case is it doesn't have write access at all.

In combination with restricting access to PHP files in write able directories makes it a bit hard to exploit a file upload exploit.
Something like this:

<Directory images>
     <FilesMatch "(?i)\.(php|phtml)$">
            Order Deny,Allow
            Deny from All
    </FilesMatch>
</Directory>
@@ -59,11 +58,8 @@ function admin_postinstall_eaccelerator_action()
// Set the configuration file path.
$file = JPATH_CONFIGURATION . '/configuration.php';
// Get the new FTP credentials.
$ftp = ClientHelper::getCredentials('ftp', true);
// Attempt to make the file writeable if using FTP.

This comment has been minimized.

@brianteeman

brianteeman Aug 10, 2018

Contributor

This comment needs updating

@@ -79,7 +75,7 @@ function admin_postinstall_eaccelerator_action()
}
// Attempt to make the file unwriteable if using FTP.

This comment has been minimized.

@brianteeman

brianteeman Aug 10, 2018

Contributor

This comment needs updatinng

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 10, 2018

@nikosdion as this touches com_joomlaupdate can you check the changes please

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 10, 2018

(note for testers - you can not use com_patchtester to test the changes in the /installation files)

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 10, 2018

doesnt this section need updating

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 10, 2018

@roland-d there is still a lot of stuff to remove from the installer or are you intending that Joomla could still be installed with ftp

@nikosdion

This comment has been minimized.

Copy link
Contributor

nikosdion commented Aug 10, 2018

I disagree 100% with this PR. While philosophically I agree and in theory the idea behind it is solid, it is in disconnect with real world usage of Joomla (sadly) and what we've done to help users understand the security implications.

What we see is plenty of clients stuck on hosting which uses mod_php on Apache, itself running as www-data:www-data. This is still the default configuration of Ubuntu even in 18.04 LTS. As a result the use of the FTP layer is a one-way street for these users.

Of course it has very serious security implications. However, this is not a problem you can solve with technology alone, definitely not by simply removing this feature. It's primarily an education problem, first and foremost with (people who think they are) web hosts. They need to learn to use FastCGI or FPM. Then it's a matter of educating users to avoid hosts where the FTP layer is necessary.

I don't see us solving anything by removing the FTP layer from Joomla!. The people who need it will either stick with an old version of Joomla! or they will choose a different CMS which works on their host. Or both. They do not understand the security implications and we have done a really bad job educating them on that the last 15 years. If there is something we need to change it's educating users. Look at the results after we started warning users about old PHP versions: they are migrating to newer versions of PHP. Users are not suicidal idiots, they just don't know better unless those in the know (us) educate them.

What I would very much prefer is an approach where we educate users and ease them into removing this feature in the future.

For versions 3.x I would recommend adding a post-installation message if the FTP layer is installed informing the users about the dangers of the FTP layer.

In version 4.x we could double down on that, printing a BIG FAT RED WARNING below the header of the backend page with the same information.
people
The copy could read something along those lines:

WARNING! YOU ARE USING JOOMLA'S FTP LAYER
By necessity, this feature stores your FTP credentials in your site's configuration. This could have serious security implications. The FTP layer feature will be removed from Joomla! in version 5.0.
Try disabling this feature. If your site does not work properly after that please contact your host and ask them to use FastCGI or PHP-FPM to run PHP on their server instead of mod_php. This is a faster and more secure alternative.

I would even put the FTP layer controls in an area with a red background and a big warning above it "THESE OPTIONS MAY HAVE AN ADVERSE EFFECT ON YOUR SITE'S SECURITY".

Then in version 5.0 we can remove the FTP layer completely. Of course you could accelerate the timeline and remove the FTP layer in version 4.1. That's a decision that needs to be taken by the production department, not us random contributors on the issue tracker.

It goes without saying that the documentation would need to be updated to dissuade users from enabling the FTP layer at all.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 10, 2018

@nikosdion do you have any stats with akeeba/admintools usage with ftp?
The "education" process you suggest can definitely be done as well

@nikosdion

This comment has been minimized.

Copy link
Contributor

nikosdion commented Aug 10, 2018

Since the use of FTP layer is in the ANGIE application (restoration script) we do not collect that information for privacy reasons. We know empirically that the stream of FTP-related issues hasn't decreased significantly in the last 5 years. I try to educate my users but I'm basically facing the argument "Joomla! comes with an FTP layer and they don't say it's insecure so you are obviously a lazy developer who doesn't want to support it".

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 10, 2018

Well it was worth asking ;)

I guess security issues would be reduced if the user can not save their ftp username/password. Currently if they choose not to then they will be asked for it each time. So perhaps we should just remove the ability to save them and then each time they will need to enter them AND we can display an "education" message at the same time

@nikosdion

This comment has been minimized.

Copy link
Contributor

nikosdion commented Aug 10, 2018

That would be the ideal way to handle this. I'd even go one step further to show a warning before people fill in their credentials if their site is not HTTPS. Credentials sent over plain HTTP can be trivially intercepted and are a major security risk. Credentials sent over HTTPS are reasonably secure. I mean that they are only stored in the session which temporarily places them in the database or the server memory (depending on the storage backend) and wiped after the operation completes.

@810

This comment has been minimized.

Copy link
Contributor

810 commented Aug 10, 2018

We can also move the warning to 3.9 and 3.10

If you want to use ftp layer, you can stay on that release scheme

@rdeutz

This comment has been minimized.

Copy link
Contributor

rdeutz commented Aug 10, 2018

I like the eduaction path, doesn't matter if we've it 5 year longer in our repo

@Bakual Bakual removed their request for review Aug 10, 2018

roland-d added some commits Aug 11, 2018

Further removing the FTP layer
Signed-off-by: Roland Dalmulder <contact@rolandd.com>
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	administrator/components/com_joomlaupdate/Helper/Select.php
@roland-d

This comment has been minimized.

Copy link
Contributor Author

roland-d commented Aug 11, 2018

@brianteeman Thank you for the review. I have also cleaned up the installation/ folder now. I believe I have cleaned up now what needs to be cleaned up. If not, please tell me.

@mbabker Thank you for the review as well. I wasn't sure about the Factory change, so I have reverted that now.

@HLeithner

normally you don't want that your software can manipulate it self.

What do you think the FTP layer is doing? Giving you just that option. Sending data plain-text across the wire is a security issue if you ask me.

As pointed out by @nikosdion ultimately this is a decision of the Production Team on how to proceed with this.

For the time being I will keep this PR updated, I can't guarantee to do so for 5 years 👅

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 11, 2018

@roland-d first glance is that its ok - will check further later this weekend

One question remains - what will happen if a user on J3 with ftp enabled upgrades?

@roland-d

This comment has been minimized.

Copy link
Contributor Author

roland-d commented Aug 11, 2018

@brianteeman I think we should add this to the pre-update checker as a check. You would still be able to update to Joomla 4 but after that your site is most likely broken as in it can't write anything.

@laoneo

This comment has been minimized.

Copy link
Member

laoneo commented Aug 11, 2018

If this is something which get considered to be merged on some point (I hope in J4), then we need to add a warning in 3.9 as it is the next release where we can ship deprecation notices. Like that we will give users enough time to make the switch.

@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Aug 11, 2018

What do you think the FTP layer is doing? Giving you just that option. Sending data plain-text across the wire is a security issue if you ask me.

If I use the FTP Layer it only communicate clear text in the isp network (where often connections are cleartext). Depending on the ISP the connection is only localhost so snooping this traffic is really hard.

If you don't save the credentials in the config then its a security feature because the CMS any only write if you allow to do so. Not on a file upload exploit. Alternative it would be ok if we can remote update installations.

Most of compromised websites only have a problem because the webserver can write to the filesystem. If the webserver can't write the attack potential is much lower for normal sites.

Why do you think CPU designers and OS developers spent so much time in the technologies like executable-space protection? Similar reason, if you have a zone where you can't execute but write is good, if you have a zone where you can execute but can't write its good. The combination is hell.

I know many people don't think about security implication on all layers but removing such a feature is a mess. I would remove the credentials and leave the possibility to have a read only webserver.

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Aug 11, 2018

Surely if the credentials are not saved then you are sending them in the open from your PC to the server??

@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Aug 11, 2018

if you don't have ssl, thats true but in this case you already sending your superuser credentials per plaintext over the wire.

roland-d added some commits Sep 20, 2018

Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	administrator/components/com_installer/tmpl/install/default.php
#	administrator/components/com_joomlaupdate/Model/UpdateModel.php
#	administrator/components/com_templates/Controller/TemplateController.php
#	administrator/components/com_users/Controller/ProfileController.php
#	components/com_users/Controller/ProfileController.php
#	libraries/src/Filesystem/Folder.php
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	administrator/components/com_joomlaupdate/Controller/DisplayController.php
#	installation/tmpl/preinstall/default.php
#	libraries/src/Filesystem/File.php
#	libraries/src/Filesystem/Folder.php
Fixed the codestyle
Signed-off-by: Roland Dalmulder <contact@rolandd.com>
Fixed codestyle
Signed-off-by: Roland Dalmulder <contact@rolandd.com>
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	administrator/components/com_admin/postinstall/eaccelerator.php
#	administrator/components/com_config/Controller/ApplicationController.php
#	administrator/components/com_config/Controller/ComponentController.php
#	administrator/components/com_config/tmpl/application/default_ftp.php
#	administrator/components/com_installer/Controller/DisplayController.php
#	administrator/components/com_installer/Model/InstallModel.php
#	administrator/components/com_joomlaupdate/Controller/DisplayController.php
#	administrator/components/com_joomlaupdate/Controller/UpdateController.php
#	administrator/components/com_joomlaupdate/Helper/Select.php
#	administrator/components/com_joomlaupdate/Model/UpdateModel.php
#	administrator/components/com_joomlaupdate/View/Joomlaupdate/HtmlView.php
#	administrator/components/com_users/Controller/ProfileController.php
#	components/com_config/Controller/ConfigController.php
#	components/com_config/Controller/TemplatesController.php
#	components/com_users/Controller/ProfileController.php
#	libraries/src/Filesystem/File.php
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	administrator/components/com_admin/postinstall/eaccelerator.php
#	administrator/components/com_config/Controller/ApplicationController.php
#	administrator/components/com_config/Controller/ComponentController.php
#	administrator/components/com_config/tmpl/application/default_ftp.php
#	administrator/components/com_installer/Controller/DisplayController.php
#	administrator/components/com_installer/Model/InstallModel.php
#	administrator/components/com_joomlaupdate/Controller/DisplayController.php
#	administrator/components/com_joomlaupdate/Controller/UpdateController.php
#	administrator/components/com_joomlaupdate/Helper/Select.php
#	administrator/components/com_joomlaupdate/Model/UpdateModel.php
#	administrator/components/com_joomlaupdate/View/Joomlaupdate/HtmlView.php
#	administrator/components/com_users/Controller/ProfileController.php
#	components/com_config/Controller/ConfigController.php
#	components/com_config/Controller/TemplatesController.php
#	components/com_users/Controller/ProfileController.php
#	libraries/src/Filesystem/File.php
Merge remote-tracking branch 'origin/remove-ftp-layer' into remove-ft…
…p-layer

# Conflicts:
#	administrator/components/com_config/tmpl/application/default_ftp.php
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	administrator/components/com_installer/Controller/DisplayController.php
#	administrator/components/com_joomlaupdate/Controller/DisplayController.php
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	administrator/components/com_installer/Controller/DisplayController.php
#	administrator/components/com_joomlaupdate/Controller/DisplayController.php
#	installation/src/Model/ConfigurationModel.php
#	libraries/src/Filesystem/Folder.php
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	installation/configuration.php-dist
Merge branch '4.0-dev' into remove-ftp-layer
# Conflicts:
#	installation/configuration.php-dist
#	installation/src/Model/ConfigurationModel.php
#	libraries/src/Filesystem/Folder.php
Merge remote-tracking branch 'origin/remove-ftp-layer' into remove-ft…
…p-layer

# Conflicts:
#	installation/configuration.php-dist
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment