Skip to content
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

Added production-grade nginx and Docker environment #1209

Merged
merged 32 commits into from
Feb 14, 2024

Conversation

colinmollenhour
Copy link
Member

@colinmollenhour colinmollenhour commented Sep 16, 2020

Description

This pull request implements a far more secure web server configuration than the common Apache one. Apache is still fully supported but I would encourage the community to consider adopting this as a standard config long-term.

Why?

This nginx-based config naturally implements a much more secure design which is to allow only what is specified rather than allow all and then deny specific files. For example, right now the LICENSE.txt file is publicly accessible because we forgot to block access to it.

In general the design here is to only load PHP through endpoints with the PHP file name hard-coded and only load static files outside of the pub subdirectories with the locations that are allowed.

Advantages:

  • Frontend and admin access is split to separate domain names and containers
    • Frontend does not allow Admin or API access
    • Easily configure special security features for the admin domain such as IP whitelists
  • Cannot load unauthorized PHP files, only the ones specifically designated in the nginx config. Many RCE attacks would be made impossible since only index.php, get.php and api.php can be invoked from the web
  • Cannot load other static files (composer.json, app/etc/local.xml, etc.) - security doesn't depend on not accidentally (or maliciously?) deleting .htaccess files that are scattered throughout. Many data leaks would be made impossible (e.g. I accidentally deleted var/.htaccess once.. oops!)
  • nginx+php-pfm is arguably better than Apache anyway (lower memory usage, more efficient at serving static assets, easy to add things like rate limiting)
  • Provides a clear way to support multiple stores on one installation (separate "pub" directory per store with store code defined in nginx config, no need for symlinks!)

Bonus 1: Multi-store automatic SSL

I added automatic-SSL support with easy multi-store configuration using Caddy. Basically you just create a new subdirectory of "pub" for each store code and edit the Caddyfile to map your domains to store codes and you're done!

Bonus 2: Fine-grained rate limiting

The rate limiting is setup in nginx with separate zones for frontend, media, admin, api and post requests. Burst and delay are configured as appropriate. There is no rate limit set for static file requests but it could easily be added.

Fixed Issues (if relevant)

  1. References discussion from introduce a /pub/ directory analog to Magento2 #571 - not necessarily a complete fix
  2. Many HackerOne reports have to do with brute force attacks - this addresses those with built-in rate limiting

Manual testing scenarios (*)

Run dev/openmage/install.sh and test frontend and admin sites for common security flaws like exposed files/directories.

Rate limiting can be tested with a tool like hey:

hey http://openmage-7f000001.nip.io/about-magento-demo-store/

image

hey -c 30 -n 30 -m POST -d 'form_key=nlUYY6fIw5jXbl8y&login%5Busername%5D=adm
inasd%40asd.com&login%5Bpassword%5D=asdasd&send=' http://openmage-admin-7f000001.nip.io:81/index.php/admin/index/index/key/4a754406966e9b2d4be39a15e76e88d4/

image

Questions or comments

While this exact config has not been specifically battle-tested in production yet, it was derived from many years of production experience and is meant to be production-ready from a security standpoint.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@colinmollenhour colinmollenhour mentioned this pull request Sep 28, 2020
kkrieger85
kkrieger85 previously approved these changes Oct 22, 2020
@colinmollenhour
Copy link
Member Author

colinmollenhour commented Jan 22, 2021

Anyone else have any feedback or objections to making this the default?

@addison74
Copy link
Contributor

I did not find any noticeable differences in the use of Apache versus Nginx, but I must say that I had high-performance servers. I had an attempt to implement the Varnish cache and collaborated for a while on the Turpentine project, but the people interest waned. Although in the beginning I tried to implement Magento 1 with Nginx, HHVM, in the end I ended up leaving it with Apache because they came with version 2.4, PHP went to 7.0. Many users are familiar with Apache directives, .htaccess, .htpasswd and few panels install Nginx.

The fact that you wanted to bring extra security to the project is to be appreciated but I would not default it immediately. I would create two versions of OpenMage, one classic to remain based on Apache and another for Nginx. Within the time limit I can help you testing this project even though I am not an Nginx fan :)

@colinmollenhour
Copy link
Member Author

Thanks for your feedback @addison74.

The reason to favor nginx over Apache is the security of the configuration, I don't know that there is any appreciable performance benefit as Apache is quite fast.

The intent is also just to provide a good model for out-of-the box hosting, not to remove support for Apache. Without such a model everyone does it their own way and some are probably very good and some probably very bad.. So not to say this is the best (though with help it could be) but the goal is to provide a very solid model for hosting OpenMage securely that is the "official" right way even if it isn't the only way.

@addison74
Copy link
Contributor

You are welcome. I remember when I did the first tests with Nginx I had to look for a lot of information resources to make Magento work with it. They didn't even exist much at that time. There were enough settings and I always had a piece of paper to find them when needed. Most people prefer Apache because once uploaded Magento starts out of the box without much efforts. As speed there is a winner LiteSpeed and Apache/Nginx on the same level. But with the new hardware resources throwing into play like NVMe the processing speed has improved radically.

If it is up to me I would improve the content of the .htaccess file in Magento root. There are certain paths that should be blocked because they are periodically checked by bots. Also, when the webserver is behind a load balancer. In Apache case a directive in .htaccess saves you from installing modules.

@colinmollenhour
Copy link
Member Author

I agree, making Magento work on nginx can be a lot of work, that's why I want to make this easy for people by including it here.

There are certain paths that should be blocked ...

That is my point exactly, rather than have to keep up with blocking dozens of paths the default should be that only the files that should be public are public and everything else is blocked. The safest way to guarantee this is to make each PHP file explicitly routed in nginx config so that you can't just drop PHP files in directories and execute them. I'm sure it's possible to do this in Apache somehow but it is much easier to do with nginx. The result is that you completely mitigate a lot of possible attacks or human errors by design.

@justinbeaty
Copy link
Contributor

justinbeaty commented May 13, 2021

@colinmollenhour While I haven't tested your nginx config, I wanted to add a note on a small bug I just found running Magento behind nginx.

In the admin section, go to a page like Manage Products, then hit Reset Filter, and try to use the pagination buttons. In my case, the pagination stopped working.

Here's what happens. Before hitting Reset Filter the AJAX calls are like this:

https://example.com/index.php/admin/catalog_product/grid/key/xxx/page/2/?ajax=true&isAjax=true

After hitting Reset Filter, the AJAX calls are like this:

https://example.com/index.php/admin/catalog_product/grid/key/xxx/product_filter//page/3/?ajax=true&isAjax=true

Notice the product_filter// part, which should be interpreted by Magento as product_filter=''. But nginx was stripping double slashes, so product_filter='page' and other screwy things.

The simple solution is to add merge_slashes off; in the nginx server block.

Hopefully this saves someone time, it lead me down a rabbit hole trying to figure out which commit in magento-lts broke this, which of course there was none.

Edit: This can actually cause a security problem, as https://example.com//index.php downloads the index.php file. Some more research will be needed. In my case the directive is only present in the admin.example.com server block, which has an htpasswd on the whole subdomain. It seems to work, but I'm not sure about this caveat in the docs:

If the directive is specified on the server level, its value is only used if a server is a default one. The value specified also applies to all virtual servers listening on the same address and port.

@fballiano fballiano changed the base branch from 20.0 to main April 4, 2023 17:30
@addison74
Copy link
Contributor

In my opinion, DDEV is the fastest way for someone to get a test environment, regardless of whether the operating system is Windows or Linux. By editing the configuration file, you can set the Docker containers needed to run OpenMage. In the case of the webserver, you can choose for Nginx or Apache. I tested both of them and they worked immediately without any issues.

I believe that Apache should remain as the default webserver version for OpenMage. For Nginx users, a tutorial should be created (e.g. Nginx.md) with all the necessary information and settings, some of them are in this PR.

@colinmollenhour
Copy link
Member Author

@addison74 The goal of this PR is not just for dev but also to provide a blueprint for a secure production environment. I like ddev as well, but I think a complex project like this should provide more out of the box in the way of server config since it is not easy for juinor admins and very easy to mess up even for senior admins. Currently we only provide "not ready for production!" environments so everyone is left to figure out production on their own.

dev/openmage/README.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed Component: ConfigurableSwatches Relates to Mage_ConfigurableSwatches Component: GoogleAnalytics Relates to Mage_GoogleAnalytics Component: Install Relates to Mage_Install Component: PaypalUk Relates to Mage_PaypalUk Component: lib/* Relates to lib/* composer Relates to composer.json phpstan phpcs php-cs-fixer phpunit labels Oct 26, 2023
@colinmollenhour
Copy link
Member Author

@addison74 I merged the latest main and the errors are resolved now..

@colinmollenhour
Copy link
Member Author

vendor folder should be blocked ? it's safe if we not use pub as root configuration?

I don't understand the question.. All files that are not in pub are inherently blocked with this configuration.

@empiricompany
Copy link
Contributor

vendor folder should be blocked ? it's safe if we not use pub as root configuration?

I don't understand the question.. All files that are not in pub are inherently blocked with this configuration.

Sorry, i remembered incorrectly that the root in "pub" was optional, but looking at the configurations, I realized I was mistaken.
I have blocked the vendor folder in old configurations that I had.

@kiatng kiatng mentioned this pull request Jan 27, 2024
3 tasks
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the idea of having more favicons (which, btw, I've replaced in this PR because there was one that too little and too complicated) and robots.txt, but this PR should be merged since it stayed waiting forever.

@colinmollenhour
Copy link
Member Author

I don't love the idea of having more favicons (which, btw, I've replaced in this PR because there was one that too little and too complicated) and robots.txt, but this PR should be merged since it stayed waiting forever.

Not because it's awesome? 😉

@fballiano
Copy link
Contributor

heheheh it was awesome from the beginning ;-)

@fballiano fballiano changed the title Production-grade nginx and Docker environment Added production-grade nginx and Docker environment Feb 14, 2024
@fballiano fballiano merged commit 42f3c19 into OpenMage:main Feb 14, 2024
17 checks passed
@fballiano
Copy link
Contributor

boom, merged per 2gray+1green thanks @colinmollenhour!

@kiatng
Copy link
Contributor

kiatng commented May 17, 2024

@colinmollenhour With nginx-admin.conf, I'm not sure how to configure the admin subdomain URLs in backend > System > Configuration > Web > sections Unsecure and Secure. There is no admin store to configure the URLs. Not sure if I understand this correctly:

Normal URL: www.example.com/admin
With nginx-admin.conf URL: admin.example.com

Normal local.xml

    <admin>
        <routers>
            <adminhtml>
                <args>
                    <frontName><![CDATA[admin]]></frontName>
                </args>
            </adminhtml>
        </routers>
    </admin>

With nginx-admin.conf, unnecessary to set <frontName>? [edit] It's not required to set <frontName> in local.xml or custom admin path (see screenshot below).

[EDIT] I get it, this is where to set the admin URL: backend > System > Configuration > Admin:
image

@colinmollenhour
Copy link
Member Author

Yes, currently "Use Custom Admin Path" is not supported in this config - it would mostly work but there are a few nginx routes that need to be updated to support it.

@kiatng
Copy link
Contributor

kiatng commented Sep 12, 2024

Trying to make use of nginx-admin.conf with URL like admin.example.com, which is set in backend > System > Configuration > Admin:
image
But can't get it to work, the page redirects to default store. When I hardcode $mageRunCode ='admin'; in index.php, I get 404. Is there a guide on how to set this up?

[edit]
I cannot find any reference of 'default/admin/url/custom' in the core. The only references are for the admin path Mage_Adminhtml_Helper_Data::XML_PATH_USE_CUSTOM_ADMIN_PATH and Mage_Adminhtml_Helper_Data::XML_PATH_CUSTOM_ADMIN_PATH. Ref the following, it's not possible to have an empty admin path.

/**
* Check if requested path starts with one of the admin front names
*
* @param Zend_Controller_Request_Http $request
* @return bool
*/
protected function _isAdminFrontNameMatched($request)
{
$useCustomAdminPath = (bool)(string)Mage::getConfig()
->getNode(Mage_Adminhtml_Helper_Data::XML_PATH_USE_CUSTOM_ADMIN_PATH);
$customAdminPath = (string)Mage::getConfig()->getNode(Mage_Adminhtml_Helper_Data::XML_PATH_CUSTOM_ADMIN_PATH);
$adminPath = ($useCustomAdminPath) ? $customAdminPath : null;
if (!$adminPath) {
$adminPath = (string)Mage::getConfig()
->getNode(Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_ROUTER_FRONTNAME);
}
$adminFrontNames = [$adminPath];
// Check for other modules that can use admin router (a lot of Magento extensions do that)
$adminFrontNameNodes = Mage::getConfig()->getNode('admin/routers')
->xpath('*[not(self::adminhtml) and use = "admin"]/args/frontName');
if (is_array($adminFrontNameNodes)) {
foreach ($adminFrontNameNodes as $frontNameNode) {
$adminFrontNames[] = (string)$frontNameNode;
}
}
$pathPrefix = ltrim($request->getPathInfo(), '/');
$urlDelimiterPos = strpos($pathPrefix, '/');
if ($urlDelimiterPos) {
$pathPrefix = substr($pathPrefix, 0, $urlDelimiterPos);
}
return in_array($pathPrefix, $adminFrontNames);
}

Meaning, it's not possible to have a admin URL like admin.example.com, it needs the admin path: admin.example.com/admin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PayPal Relates to Mage_Paypal documentation environment JavaScript Relates to js/* Mage.php Relates to app/Mage.php translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants