Skip to content

feat(objectstore): S3 conditional writes to prevent silent overwrites #62473

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.

Sign up for GitHub

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

Open
solracsf wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(objectstore): S3 conditional writes to prevent silent overwrites #62473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions config/config.sample.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2116,6 +2116,27 @@
// using Amazon S3 (or any other implementation that supports it) we recommend enabling it by using "when_supported".
'request_checksum_calculation' => 'when_required',
'response_checksum_validation' => 'when_required',
// optional: Use S3 conditional writes (the "If-None-Match" header) so that
// writing a newly created file can never silently overwrite an object that
// already exists at its target key. This protects against data loss when the
// file cache and the bucket get out of sync, for example after restoring the
// database from a backup, when two instances accidentally share one bucket,
// or when file ids are duplicated. In those cases the upload fails loudly
// instead of destroying the existing object.
//
// Disabled by default (opt-in) so upgrades keep their existing behaviour.
// Requires Signature v4 (the default) and a store that enforces the header
// (Amazon S3, current MinIO, Cloudflare R2 and others; support varies for
// other S3-compatible stores). Valid values:
// false (default) disabled; keep the previous behaviour.
// 'auto' probe the bucket once and enable the feature only if the store
// actually enforces the header (recommended way to turn it on);
// true force it on (only set this if you know your store supports it).
//
// Note: do NOT add a bucket policy that requires "If-None-Match" for every
// write. Nextcloud intentionally overwrites the same object key whenever a
// file is updated, so such a policy would break all file modifications.
'conditional_writes' => false,
],
],

Expand Down
2 changes: 2 additions & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,8 +516,10 @@
'OCP\\Files\\Notify\\IRenameChange' => $baseDir . '/lib/public/Files/Notify/IRenameChange.php',
'OCP\\Files\\ObjectStore\\Events\\BucketCreatedEvent' => $baseDir . '/lib/public/Files/ObjectStore/Events/BucketCreatedEvent.php',
'OCP\\Files\\ObjectStore\\IObjectStore' => $baseDir . '/lib/public/Files/ObjectStore/IObjectStore.php',
'OCP\\Files\\ObjectStore\\IObjectStoreConditionalWrite' => $baseDir . '/lib/public/Files/ObjectStore/IObjectStoreConditionalWrite.php',
'OCP\\Files\\ObjectStore\\IObjectStoreMetaData' => $baseDir . '/lib/public/Files/ObjectStore/IObjectStoreMetaData.php',
'OCP\\Files\\ObjectStore\\IObjectStoreMultiPartUpload' => $baseDir . '/lib/public/Files/ObjectStore/IObjectStoreMultiPartUpload.php',
'OCP\\Files\\ObjectStore\\ObjectAlreadyExistsException' => $baseDir . '/lib/public/Files/ObjectStore/ObjectAlreadyExistsException.php',
'OCP\\Files\\ReservedWordException' => $baseDir . '/lib/public/Files/ReservedWordException.php',
'OCP\\Files\\Search\\ISearchBinaryOperator' => $baseDir . '/lib/public/Files/Search/ISearchBinaryOperator.php',
'OCP\\Files\\Search\\ISearchComparison' => $baseDir . '/lib/public/Files/Search/ISearchComparison.php',
Expand Down
2 changes: 2 additions & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,8 +557,10 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\Files\\Notify\\IRenameChange' => __DIR__ . '/../../..' . '/lib/public/Files/Notify/IRenameChange.php',
'OCP\\Files\\ObjectStore\\Events\\BucketCreatedEvent' => __DIR__ . '/../../..' . '/lib/public/Files/ObjectStore/Events/BucketCreatedEvent.php',
'OCP\\Files\\ObjectStore\\IObjectStore' => __DIR__ . '/../../..' . '/lib/public/Files/ObjectStore/IObjectStore.php',
'OCP\\Files\\ObjectStore\\IObjectStoreConditionalWrite' => __DIR__ . '/../../..' . '/lib/public/Files/ObjectStore/IObjectStoreConditionalWrite.php',
'OCP\\Files\\ObjectStore\\IObjectStoreMetaData' => __DIR__ . '/../../..' . '/lib/public/Files/ObjectStore/IObjectStoreMetaData.php',
'OCP\\Files\\ObjectStore\\IObjectStoreMultiPartUpload' => __DIR__ . '/../../..' . '/lib/public/Files/ObjectStore/IObjectStoreMultiPartUpload.php',
'OCP\\Files\\ObjectStore\\ObjectAlreadyExistsException' => __DIR__ . '/../../..' . '/lib/public/Files/ObjectStore/ObjectAlreadyExistsException.php',
'OCP\\Files\\ReservedWordException' => __DIR__ . '/../../..' . '/lib/public/Files/ReservedWordException.php',
'OCP\\Files\\Search\\ISearchBinaryOperator' => __DIR__ . '/../../..' . '/lib/public/Files/Search/ISearchBinaryOperator.php',
'OCP\\Files\\Search\\ISearchComparison' => __DIR__ . '/../../..' . '/lib/public/Files/Search/ISearchComparison.php',
Expand Down
31 changes: 30 additions & 1 deletion lib/private/Files/ObjectStore/ObjectStoreStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,10 @@
use OCP\Files\IMimeTypeDetector;
use OCP\Files\NotFoundException;
use OCP\Files\ObjectStore\IObjectStore;
use OCP\Files\ObjectStore\IObjectStoreConditionalWrite;
use OCP\Files\ObjectStore\IObjectStoreMetaData;
use OCP\Files\ObjectStore\IObjectStoreMultiPartUpload;
use OCP\Files\ObjectStore\ObjectAlreadyExistsException;
use OCP\Files\Storage\IChunkedFileWrite;
use OCP\Files\Storage\IStorage;
use OCP\IDBConnection;
Expand Down Expand Up @@ -538,7 +540,15 @@ public function writeStream(string $path, $stream, ?int $size = null): int {
$totalWritten = $writtenSize;
});

if ($this->objectStore instanceof IObjectStoreMetaData) {
if (!$exists
&& $this->objectStore instanceof IObjectStoreConditionalWrite
&& $this->objectStore->supportsConditionalWrites()) {
// A newly created file targets a fresh, never-used object urn, so the
// object must not exist yet. Refuse to overwrite anything already there:
// its presence means the file cache and the object store are out of sync
// and an unconditional write would silently destroy existing data.
$this->objectStore->writeObjectIfNotExists($urn, $countStream, $metadata);
} elseif ($this->objectStore instanceof IObjectStoreMetaData) {
$this->objectStore->writeObjectWithMetaData($urn, $countStream, $metadata);
} else {
$this->objectStore->writeObject($urn, $countStream, $metadata['mimetype']);
Expand All @@ -548,6 +558,25 @@ public function writeStream(string $path, $stream, ?int $size = null): int {
}

$stat['size'] = $totalWritten;
} catch (ObjectAlreadyExistsException $ex) {
// Only reachable for newly created files (conditional writes are not used
// when overwriting). The target object already exists, so the file cache
// and the object store are out of sync; refuse the write instead of
// destroying the existing object.
if (!$exists) {
$this->getCache()->remove($uploadPath);
}
$this->logger->critical(
'Refusing to overwrite existing object ' . $urn . ' for newly created file ' . $path . '. '
. 'The file cache and the object store are out of sync (possible causes: database restored from '
. 'backup, multiple instances sharing this bucket, or duplicated file ids). No data was overwritten; '
. 'resolve the desynchronisation before retrying. See the admin documentation on object storage.',
[
'app' => 'objectstore',
'exception' => $ex,
]
);
throw new GenericFileException('Object already exists in object store', 0, $ex);
} catch (\Exception $ex) {
if (!$exists) {
/*
Expand Down
3 changes: 2 additions & 1 deletion lib/private/Files/ObjectStore/S3.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@
use Aws\Result;
use Exception;
use OCP\Files\ObjectStore\IObjectStore;
use OCP\Files\ObjectStore\IObjectStoreConditionalWrite;
use OCP\Files\ObjectStore\IObjectStoreMetaData;
use OCP\Files\ObjectStore\IObjectStoreMultiPartUpload;

class S3 implements IObjectStore, IObjectStoreMultiPartUpload, IObjectStoreMetaData {
class S3 implements IObjectStore, IObjectStoreMultiPartUpload, IObjectStoreMetaData, IObjectStoreConditionalWrite {
use S3ConnectionTrait;
use S3ObjectTrait;

Expand Down
7 changes: 7 additions & 0 deletions lib/private/Files/ObjectStore/S3ConfigTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,11 @@ trait S3ConfigTrait {
private bool $useMultipartCopy = true;

protected int $retriesMaxAttempts;

/**
* Conditional write mode: true (force on), false (off, the default) or 'auto'
* (probe the bucket once and cache the result). Opt-in to preserve existing
* behaviour on upgrade.
*/
protected bool|string $conditionalWrites = false;
}
30 changes: 30 additions & 0 deletions lib/private/Files/ObjectStore/S3ConnectionTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ protected function parseParams($params) {
$this->copySizeLimit = $params['copySizeLimit'] ?? 5242880000;
$this->useMultipartCopy = (bool)($params['useMultipartCopy'] ?? true);
$this->retriesMaxAttempts = $params['retriesMaxAttempts'] ?? 5;
$this->conditionalWrites = $this->parseConditionalWritesMode($params['conditional_writes'] ?? false);
$params['region'] = empty($params['region']) ? 'eu-west-1' : $params['region'];
$params['hostname'] = empty($params['hostname']) ? 's3.' . $params['region'] . '.amazonaws.com' : $params['hostname'];
$params['s3-accelerate'] = $params['hostname'] === 's3-accelerate.amazonaws.com' || $params['hostname'] === 's3-accelerate.dualstack.amazonaws.com';
Expand All @@ -77,6 +78,35 @@ protected function parseParams($params) {
$this->params = $params;
}

/**
* Normalize the configured conditional-write mode to true, false or 'auto'.
*
* Accepts booleans as well as the string forms that env-templated configs tend
* to produce; anything unrecognized falls back to 'auto' with a warning.
*/
private function parseConditionalWritesMode(mixed $value): bool|string {
if (is_bool($value)) {
return $value;
}
switch (strtolower((string)$value)) {
case '':
case 'auto':
return 'auto';
case 'true':
case '1':
return true;
case 'false':
case '0':
return false;
default:
Server::get(LoggerInterface::class)->warning(
'Invalid "conditional_writes" value "' . (string)$value . '" for S3 object store; falling back to "auto".',
['app' => 'objectstore'],
);
return 'auto';
}
}

public function getBucket() {
return $this->bucket;
}
Expand Down
Loading
Loading