From 15996c62d646c436df73370b0c4983011092c0fe Mon Sep 17 00:00:00 2001 From: Aaron Date: Mon, 21 May 2012 15:19:06 -0700 Subject: [PATCH] [FileBackend] Refactored backend access control functions. * Made secure() no longer create the container/directories. * Added a new publish() function, which is the reverse of secure(). It's seems reasonable to be able to reverse secure() if needed. * Added the ability to call prepare() with the secure() parameters. The "securing" will only be done if the container had to be created. This kills a bunch of slow RTTs with setContainerAccess() in Swift. * Also made the Swift doSecureInternal() function respect the arguments and set 'r:*' to properly make containers public. * Consolidated FileRepo directory creation into an initDirectory() function. This uses the new prepare() arguments. Change-Id: Ie16331ebf26c99295f60b266e07a4727228f53f2 --- includes/filerepo/FileRepo.php | 51 ++++++----- includes/filerepo/backend/FSFileBackend.php | 72 ++++++++++++--- includes/filerepo/backend/FileBackend.php | 49 +++++++++-- .../backend/FileBackendMultiWrite.php | 14 +++ .../filerepo/backend/FileBackendStore.php | 40 +++++++++ .../filerepo/backend/SwiftFileBackend.php | 88 +++++++++++++++---- 6 files changed, 254 insertions(+), 60 deletions(-) diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index d4eef8704f..3d2ccf4fd1 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -164,18 +164,6 @@ class FileRepo { return $status; } - /** - * Take all available measures to prevent web accessibility of new deleted - * directories, in case the user has not configured offline storage - * - * @param $dir string - * @return void - */ - protected function initDeletedDir( $dir ) { - $this->backend->secure( // prevent web access & dir listings - array( 'dir' => $dir, 'noAccess' => true, 'noListing' => true ) ); - } - /** * Determine if a string is an mwrepo:// URL * @@ -726,14 +714,10 @@ class FileRepo { $dstPath = "$root/$dstRel"; $dstDir = dirname( $dstPath ); // Create destination directories for this triplet - if ( !$backend->prepare( array( 'dir' => $dstDir ) )->isOK() ) { + if ( !$this->initDirectory( $dstDir )->isOK() ) { return $this->newFatal( 'directorycreateerror', $dstDir ); } - if ( $dstZone == 'deleted' ) { - $this->initDeletedDir( $dstDir ); - } - // Resolve source to a storage path if virtual $srcPath = $this->resolveToStoragePath( $srcPath ); @@ -863,12 +847,13 @@ class FileRepo { $operations = array(); foreach ( $pairs as $pair ) { list ( $src, $dst ) = $pair; + $dst = $this->resolveToStoragePath( $dst ); $operations[] = array( 'op' => 'store', 'src' => $src, - 'dst' => $this->resolveToStoragePath( $dst ) + 'dst' => $dst ); - $this->backend->prepare( array( 'dir' => dirname( $dst ) ) ); + $status->merge( $this->initDirectory( dirname( $dst ) ) ); } $status->merge( $this->backend->doQuickOperations( $operations ) ); @@ -1058,10 +1043,10 @@ class FileRepo { $dstDir = dirname( $dstPath ); $archiveDir = dirname( $archivePath ); // Abort immediately on directory creation errors since they're likely to be repetitive - if ( !$backend->prepare( array( 'dir' => $dstDir ) )->isOK() ) { + if ( !$this->initDirectory( $dstDir )->isOK() ) { return $this->newFatal( 'directorycreateerror', $dstDir ); } - if ( !$backend->prepare( array( 'dir' => $archiveDir ) )->isOK() ) { + if ( !$this->initDirectory($archiveDir )->isOK() ) { return $this->newFatal( 'directorycreateerror', $archiveDir ); } @@ -1126,6 +1111,27 @@ class FileRepo { return $status; } + /** + * Creates a directory with the appropriate zone permissions. + * Callers are responsible for doing read-only and "writable repo" checks. + * + * @param $dir string Virtual URL (or storage path) of directory to clean + * @return Status + */ + protected function initDirectory( $dir ) { + $path = $this->resolveToStoragePath( $dir ); + list( $b, $container, $r ) = FileBackend::splitStoragePath( $path ); + + $params = array( 'dir' => $path ); + if ( $container === $this->zones['deleted']['container'] ) { + # Take all available measures to prevent web accessibility of new deleted + # directories, in case the user has not configured offline storage + $params = array( 'noAccess' => true, 'noListing' => true ) + $params; + } + + return $this->backend->prepare( $params ); + } + /** * Deletes a directory if empty. * @@ -1231,10 +1237,9 @@ class FileRepo { $archiveDir = dirname( $archivePath ); // does not touch FS // Create destination directories - if ( !$backend->prepare( array( 'dir' => $archiveDir ) )->isOK() ) { + if ( !$this->initDirectory( $archiveDir )->isOK() ) { return $this->newFatal( 'directorycreateerror', $archiveDir ); } - $this->initDeletedDir( $archiveDir ); $operations[] = array( 'op' => 'move', diff --git a/includes/filerepo/backend/FSFileBackend.php b/includes/filerepo/backend/FSFileBackend.php index 8157916f87..4c861e4b64 100644 --- a/includes/filerepo/backend/FSFileBackend.php +++ b/includes/filerepo/backend/FSFileBackend.php @@ -474,13 +474,18 @@ class FSFileBackend extends FileBackendStore { list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] ); $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot; + $existed = is_dir( $dir ); // already there? if ( !wfMkdirParents( $dir ) ) { // make directory and its parents - $status->fatal( 'directorycreateerror', $params['dir'] ); + $status->fatal( 'directorycreateerror', $params['dir'] ); // fails on races } elseif ( !is_writable( $dir ) ) { $status->fatal( 'directoryreadonlyerror', $params['dir'] ); } elseif ( !is_readable( $dir ) ) { $status->fatal( 'directorynotreadableerror', $params['dir'] ); } + if ( is_dir( $dir ) && !$existed ) { + // Respect any 'noAccess' or 'noListing' flags... + $status->merge( $this->doSecureInternal( $fullCont, $dirRel, $params ) ); + } return $status; } @@ -495,21 +500,48 @@ class FSFileBackend extends FileBackendStore { $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot; // Seed new directories with a blank index.html, to prevent crawling... if ( !empty( $params['noListing'] ) && !file_exists( "{$dir}/index.html" ) ) { - $bytes = file_put_contents( "{$dir}/index.html", '' ); - if ( !$bytes ) { + $bytes = file_put_contents( "{$dir}/index.html", $this->indexHtmlPrivate() ); + if ( $bytes === false ) { $status->fatal( 'backend-fail-create', $params['dir'] . '/index.html' ); return $status; } } // Add a .htaccess file to the root of the container... - if ( !empty( $params['noAccess'] ) ) { - if ( !file_exists( "{$contRoot}/.htaccess" ) ) { - $bytes = file_put_contents( "{$contRoot}/.htaccess", "Deny from all\n" ); - if ( !$bytes ) { - $storeDir = "mwstore://{$this->name}/{$shortCont}"; - $status->fatal( 'backend-fail-create', "{$storeDir}/.htaccess" ); - return $status; - } + if ( !empty( $params['noAccess'] ) && !file_exists( "{$contRoot}/.htaccess" ) ) { + $bytes = file_put_contents( "{$contRoot}/.htaccess", $this->htaccessPrivate() ); + if ( $bytes === false ) { + $storeDir = "mwstore://{$this->name}/{$shortCont}"; + $status->fatal( 'backend-fail-create', "{$storeDir}/.htaccess" ); + return $status; + } + } + return $status; + } + + /** + * @see FileBackendStore::doPublishInternal() + * @return Status + */ + protected function doPublishInternal( $fullCont, $dirRel, array $params ) { + $status = Status::newGood(); + list( $b, $shortCont, $r ) = FileBackend::splitStoragePath( $params['dir'] ); + $contRoot = $this->containerFSRoot( $shortCont, $fullCont ); // must be valid + $dir = ( $dirRel != '' ) ? "{$contRoot}/{$dirRel}" : $contRoot; + // Unseed new directories with a blank index.html, to allow crawling... + if ( !empty( $params['listing'] ) && is_file( "{$dir}/index.html" ) ) { + $exists = ( file_get_contents( "{$dir}/index.html" ) === $this->indexHtmlPrivate() ); + if ( $exists && !unlink( "{$dir}/index.html" ) ) { // reverse secure() + $status->fatal( 'backend-fail-delete', $params['dir'] . '/index.html' ); + return $status; + } + } + // Remove the .htaccess file from the root of the container... + if ( !empty( $params['access'] ) && is_file( "{$contRoot}/.htaccess" ) ) { + $exists = ( file_get_contents( "{$contRoot}/.htaccess" ) === $this->htaccessPrivate() ); + if ( $exists && !unlink( "{$contRoot}/.htaccess" ) ) { // reverse secure() + $storeDir = "mwstore://{$this->name}/{$shortCont}"; + $status->fatal( 'backend-fail-delete', "{$storeDir}/.htaccess" ); + return $status; } } return $status; @@ -716,6 +748,24 @@ class FSFileBackend extends FileBackendStore { return $ok; } + /** + * Return the text of an index.html file to hide directory listings + * + * @return string + */ + protected function indexHtmlPrivate() { + return ''; + } + + /** + * Return the text of a .htaccess file to make a directory private + * + * @return string + */ + protected function htaccessPrivate() { + return "Deny from all\n"; + } + /** * Clean up directory separators for the given OS * diff --git a/includes/filerepo/backend/FileBackend.php b/includes/filerepo/backend/FileBackend.php index 76816b4b63..9cf8f94876 100644 --- a/includes/filerepo/backend/FileBackend.php +++ b/includes/filerepo/backend/FileBackend.php @@ -568,9 +568,16 @@ abstract class FileBackend { * This will create any required containers and parent directories. * Backends using key/value stores only need to create the container. * - * @param $params Array + * The 'noAccess' and 'noListing' parameters works the same as in secure(), + * except they are only applied *if* the directory/container had to be created. + * These flags should always be set for directories that have private files. + * * $params include: - * - dir : storage directory + * dir : storage directory + * noAccess : try to deny file access (@since 1.20) + * noListing : try to deny file listing (@since 1.20) + * + * @param $params Array * @return Status */ final public function prepare( array $params ) { @@ -588,8 +595,8 @@ abstract class FileBackend { /** * Take measures to block web access to a storage directory and * the container it belongs to. FS backends might add .htaccess - * files whereas key/value store backends might restrict container - * access to the auth user that represents end-users in web request. + * files whereas key/value store backends might revoke container + * access to the storage user representing end-users in web requests. * This is not guaranteed to actually do anything. * * @param $params Array @@ -603,11 +610,7 @@ abstract class FileBackend { if ( $this->isReadOnly() ) { return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly ); } - $status = $this->doPrepare( $params ); // dir must exist to restrict it - if ( $status->isOK() ) { - $status->merge( $this->doSecure( $params ) ); - } - return $status; + return $this->doSecure( $params ); } /** @@ -615,6 +618,34 @@ abstract class FileBackend { */ abstract protected function doSecure( array $params ); + /** + * Remove measures to block web access to a storage directory and + * the container it belongs to. FS backends might remove .htaccess + * files whereas key/value store backends might grant container + * access to the storage user representing end-users in web requests. + * This essentially can undo the result of secure() calls. + * + * $params include: + * dir : storage directory + * access : try to allow file access + * listing : try to allow file listing + * + * @param $params Array + * @return Status + * @since 1.20 + */ + final public function publish( array $params ) { + if ( $this->isReadOnly() ) { + return Status::newFatal( 'backend-fail-readonly', $this->name, $this->readOnly ); + } + return $this->doPublish( $params ); + } + + /** + * @see FileBackend::publish() + */ + abstract protected function doPublish( array $params ); + /** * Delete a storage directory if it is empty. * Backends using key/value stores may do nothing unless the directory diff --git a/includes/filerepo/backend/FileBackendMultiWrite.php b/includes/filerepo/backend/FileBackendMultiWrite.php index efc6053f40..2fc9d8ed3b 100644 --- a/includes/filerepo/backend/FileBackendMultiWrite.php +++ b/includes/filerepo/backend/FileBackendMultiWrite.php @@ -343,6 +343,20 @@ class FileBackendMultiWrite extends FileBackend { return $status; } + /** + * @see FileBackend::doPublish() + * @param $params array + * @return Status + */ + protected function doPublish( array $params ) { + $status = Status::newGood(); + foreach ( $this->backends as $backend ) { + $realParams = $this->substOpPaths( $params, $backend ); + $status->merge( $backend->doPublish( $realParams ) ); + } + return $status; + } + /** * @see FileBackend::doClean() * @param $params array diff --git a/includes/filerepo/backend/FileBackendStore.php b/includes/filerepo/backend/FileBackendStore.php index 5fe33ba8ab..9c713d381c 100644 --- a/includes/filerepo/backend/FileBackendStore.php +++ b/includes/filerepo/backend/FileBackendStore.php @@ -426,6 +426,46 @@ abstract class FileBackendStore extends FileBackend { return Status::newGood(); } + /** + * @see FileBackend::doPublish() + * @return Status + */ + final protected function doPublish( array $params ) { + wfProfileIn( __METHOD__ ); + wfProfileIn( __METHOD__ . '-' . $this->name ); + $status = Status::newGood(); + + list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] ); + if ( $dir === null ) { + $status->fatal( 'backend-fail-invalidpath', $params['dir'] ); + wfProfileOut( __METHOD__ . '-' . $this->name ); + wfProfileOut( __METHOD__ ); + return $status; // invalid storage path + } + + if ( $shard !== null ) { // confined to a single container/shard + $status->merge( $this->doPublishInternal( $fullCont, $dir, $params ) ); + } else { // directory is on several shards + wfDebug( __METHOD__ . ": iterating over all container shards.\n" ); + list( $b, $shortCont, $r ) = self::splitStoragePath( $params['dir'] ); + foreach ( $this->getContainerSuffixes( $shortCont ) as $suffix ) { + $status->merge( $this->doPublishInternal( "{$fullCont}{$suffix}", $dir, $params ) ); + } + } + + wfProfileOut( __METHOD__ . '-' . $this->name ); + wfProfileOut( __METHOD__ ); + return $status; + } + + /** + * @see FileBackendStore::doPublish() + * @return Status + */ + protected function doPublishInternal( $container, $dir, array $params ) { + return Status::newGood(); + } + /** * @see FileBackend::doClean() * @return Status diff --git a/includes/filerepo/backend/SwiftFileBackend.php b/includes/filerepo/backend/SwiftFileBackend.php index 2b10917c83..6bd8f0c113 100644 --- a/includes/filerepo/backend/SwiftFileBackend.php +++ b/includes/filerepo/backend/SwiftFileBackend.php @@ -524,13 +524,12 @@ class SwiftFileBackend extends FileBackendStore { // (b) Create container as needed try { $contObj = $this->createContainer( $fullCont ); - // Make container public to end-users... - if ( $this->swiftAnonUser != '' ) { - $status->merge( $this->setContainerAccess( - $contObj, - array( $this->auth->username, $this->swiftAnonUser ), // read - array( $this->auth->username ) // write - ) ); + if ( !empty( $params['noAccess'] ) ) { + // Make container private to end-users... + $status->merge( $this->doSecureInternal( $fullCont, $dir, $params ) ); + } else { + // Make container public to end-users... + $status->merge( $this->doPublishInternal( $fullCont, $dir, $params ) ); } if ( $this->swiftUseCDN ) { // Rackspace style CDN $contObj->make_public( $this->swiftCDNExpiry ); @@ -551,6 +550,9 @@ class SwiftFileBackend extends FileBackendStore { */ protected function doSecureInternal( $fullCont, $dir, array $params ) { $status = Status::newGood(); + if ( empty( $params['noAccess'] ) ) { + return $status; // nothing to do + } // Restrict container from end-users... try { @@ -560,18 +562,53 @@ class SwiftFileBackend extends FileBackendStore { // NoSuchContainerException not thrown: container must exist // Make container private to end-users... - if ( $this->swiftAnonUser != '' && !isset( $contObj->mw_wasSecured ) ) { + $status->merge( $this->setContainerAccess( + $contObj, + array( $this->auth->username ), // read + array( $this->auth->username ) // write + ) ); + if ( $this->swiftUseCDN && $contObj->is_public() ) { // Rackspace style CDN + $contObj->make_private(); + } + } catch ( CDNNotEnabledException $e ) { + // CDN not enabled; nothing to see here + } catch ( CloudFilesException $e ) { // some other exception? + $this->handleException( $e, $status, __METHOD__, $params ); + } + + return $status; + } + + /** + * @see FileBackendStore::doPublishInternal() + * @return Status + */ + protected function doPublishInternal( $fullCont, $dir, array $params ) { + $status = Status::newGood(); + + // Unrestrict container from end-users... + try { + // doPrepareInternal() should have been called, + // so the Swift container should already exist... + $contObj = $this->getContainer( $fullCont ); // normally a cache hit + // NoSuchContainerException not thrown: container must exist + + // Make container public to end-users... + if ( $this->swiftAnonUser != '' ) { $status->merge( $this->setContainerAccess( $contObj, - array( $this->auth->username ), // read + array( $this->auth->username, $this->swiftAnonUser ), // read + array( $this->auth->username, $this->swiftAnonUser ) // write + ) ); + } else { + $status->merge( $this->setContainerAccess( + $contObj, + array( $this->auth->username, '.r:*' ), // read array( $this->auth->username ) // write ) ); - // @TODO: when php-cloudfiles supports container - // metadata, we can make use of that to avoid RTTs - $contObj->mw_wasSecured = true; // avoid useless RTTs } - if ( $this->swiftUseCDN && $contObj->is_public() ) { // Rackspace style CDN - $contObj->make_private(); + if ( $this->swiftUseCDN && !$contObj->is_public() ) { // Rackspace style CDN + $contObj->make_public(); } } catch ( CDNNotEnabledException $e ) { // CDN not enabled; nothing to see here @@ -1008,11 +1045,28 @@ class SwiftFileBackend extends FileBackendStore { } /** - * Set read/write permissions for a Swift container + * Set read/write permissions for a Swift container. + * + * $readGrps is a list of the possible criteria for a request to have + * access to write to a container. Each item is of the following format: + * account:user - Grants access if the request is by this user + * + * $readGrps is a list of the possible criteria for a request to have + * access to read a container. Each item is one of the following formats: + * .r: - Grants access if the request is from a referrer host that + * matches the expression and the request is not for a listing. + * Setting this to '*' effectively makes a container public. + * .rlistings: - Grants access if the request is from a referrer host that + * matches the expression and the request for a listing. + * + * @see http://swift.openstack.org/misc.html#acls + * + * In general, we don't allow listings to end-users. It's not useful, isn't well-defined + * (lists are truncated to 10000 item with no way to page), and is just a performance risk. * * @param $contObj CF_Container Swift container - * @param $readGrps Array Swift users who can read (account:user) - * @param $writeGrps Array Swift users who can write (account:user) + * @param $readGrps Array List of read access routes + * @param $writeGrps Array List of write access routes * @return Status */ protected function setContainerAccess( -- 2.20.1