From d577cf396303f8324f29165b9959625f30cdc412 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:31:08 +0200 Subject: [PATCH] feat(objectstore): S3 conditional writes to prevent silent overwrites Object keys are derived from the file-cache auto-increment id, so a newly created file always targets a key that must not exist yet. When the cache and the bucket drift apart - a database restored from backup, two instances sharing one bucket, duplicated file ids - the current unconditional write silently overwrites and destroys the existing object. Send "If-None-Match: *" for newly created files so the store refuses the write atomically, turning silent data loss into a loud, data-preserving failure. - S3: If-None-Match on single-PUT and multipart creates, typed 412 exception, bounded 409 retry with backoff, abort-safe multipart completion. - ObjectStoreStorage::writeStream uses it only for creates; on refusal it removes the cache entry and logs an actionable critical message. - Per-bucket "conditional_writes" config: false (default), 'auto' (one cached bucket probe, enable only if enforced) or true. Self-disables on SigV2; an unrecognized value is reported and treated as false. Overwrites and the disabled path emit byte-identical S3 requests to before, and there is no DB schema change. Chunked uploads create their target object before filling it and therefore cannot use the header; they keep the previous behaviour, as documented in config.sample.php. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- config/config.sample.php | 28 ++ lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + .../Files/ObjectStore/ObjectStoreStorage.php | 32 ++- lib/private/Files/ObjectStore/S3.php | 3 +- .../Files/ObjectStore/S3ConfigTrait.php | 7 + .../Files/ObjectStore/S3ConnectionTrait.php | 31 +++ .../Files/ObjectStore/S3ObjectTrait.php | 239 +++++++++++++++++- .../IObjectStoreConditionalWrite.php | 51 ++++ .../ObjectAlreadyExistsException.php | 37 +++ .../ConditionalWriteObjectStore.php | 88 +++++++ ...ObjectStoreStorageConditionalWriteTest.php | 147 +++++++++++ tests/lib/Files/ObjectStore/S3Test.php | 106 ++++++++ 13 files changed, 762 insertions(+), 11 deletions(-) create mode 100644 lib/public/Files/ObjectStore/IObjectStoreConditionalWrite.php create mode 100644 lib/public/Files/ObjectStore/ObjectAlreadyExistsException.php create mode 100644 tests/lib/Files/ObjectStore/ConditionalWriteObjectStore.php create mode 100644 tests/lib/Files/ObjectStore/ObjectStoreStorageConditionalWriteTest.php diff --git a/config/config.sample.php b/config/config.sample.php index 061d081d65581..ecf26c1ef229e 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -2116,6 +2116,34 @@ // 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). + // An unrecognized value is reported and treated as false. + // + // Scope: this covers regular uploads. Chunked uploads create their target + // object before filling it and therefore cannot use the header, so they keep + // the previous behaviour. The 'auto' probe also only exercises a plain PUT: a + // store that enforces the header there but not on multipart completion is + // detected as supported, and its multipart creates stay unprotected. + // + // 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, ], ], diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index beafb4700c69d..8d352545d8c31 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -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', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index e024c1fb44b9f..28bb9db39247d 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -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', diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index d776c2a59c559..2b536b2a8570d 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -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; @@ -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']); @@ -548,6 +558,26 @@ 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, duplicated file ids, or a stale ".part" entry ' + . 'left by an interrupted upload). 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) { /* diff --git a/lib/private/Files/ObjectStore/S3.php b/lib/private/Files/ObjectStore/S3.php index eeacfa3fca620..88dabf6523afa 100644 --- a/lib/private/Files/ObjectStore/S3.php +++ b/lib/private/Files/ObjectStore/S3.php @@ -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; diff --git a/lib/private/Files/ObjectStore/S3ConfigTrait.php b/lib/private/Files/ObjectStore/S3ConfigTrait.php index 1c6eaf437ae2f..e3ca0aa2a7a58 100644 --- a/lib/private/Files/ObjectStore/S3ConfigTrait.php +++ b/lib/private/Files/ObjectStore/S3ConfigTrait.php @@ -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; } diff --git a/lib/private/Files/ObjectStore/S3ConnectionTrait.php b/lib/private/Files/ObjectStore/S3ConnectionTrait.php index 3837f0b869f8b..5273ac02b7fec 100644 --- a/lib/private/Files/ObjectStore/S3ConnectionTrait.php +++ b/lib/private/Files/ObjectStore/S3ConnectionTrait.php @@ -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'; @@ -77,6 +78,36 @@ 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 the disabled default with a + * warning, so a typo can never silently turn the feature on. + */ + private function parseConditionalWritesMode(mixed $value): bool|string { + if (is_bool($value)) { + return $value; + } + switch (strtolower((string)$value)) { + case 'auto': + return 'auto'; + case 'true': + case '1': + return true; + case '': + 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 disabled.', + ['app' => 'objectstore'], + ); + return false; + } + } + public function getBucket() { return $this->bucket; } diff --git a/lib/private/Files/ObjectStore/S3ObjectTrait.php b/lib/private/Files/ObjectStore/S3ObjectTrait.php index d2f43ca65f737..85e9d4d302729 100644 --- a/lib/private/Files/ObjectStore/S3ObjectTrait.php +++ b/lib/private/Files/ObjectStore/S3ObjectTrait.php @@ -19,11 +19,30 @@ use GuzzleHttp\Psr7\Utils; use OC\Files\Stream\SeekableHttpStream; use OCA\DAV\Connector\Sabre\Exception\BadGateway; +use OCP\Files\ObjectStore\ObjectAlreadyExistsException; +use OCP\ICacheFactory; +use OCP\Server; use Psr\Http\Message\StreamInterface; +use Psr\Log\LoggerInterface; trait S3ObjectTrait { use S3ConfigTrait; + /** Object key prefix used to probe whether the store enforces conditional writes. */ + private const CONDITIONAL_WRITE_PROBE_KEY = 'nextcloud-conditional-write-probe'; + /** How long a probe result is cached, in seconds. */ + private const CONDITIONAL_WRITE_PROBE_TTL = 604800; // 7 days + + /** Resolved conditional write support, memoized for the lifetime of this instance. */ + private ?bool $conditionalWritesSupported = null; + + /** + * Process-level memo of resolved probe results, keyed by "hostname::bucket", so a + * long-lived worker probes at most once even when no shared cache is configured. + * @var array + */ + private static array $conditionalWritesProbeCache = []; + /** * Returns the connection * @@ -104,7 +123,7 @@ private function buildS3Metadata(array $metadata): array { * @param array $metaData the metadata to set for the object * @throws \Exception when something goes wrong, message will be logged */ - protected function writeSingle(string $urn, StreamInterface $stream, array $metaData): void { + protected function writeSingle(string $urn, StreamInterface $stream, array $metaData, bool $ifNoneMatch = false): void { $mimetype = $metaData['mimetype'] ?? null; unset($metaData['mimetype']); unset($metaData['size']); @@ -123,7 +142,33 @@ protected function writeSingle(string $urn, StreamInterface $stream, array $meta $args['ContentLength'] = $size; } - $this->getConnection()->putObject($args); + if (!$ifNoneMatch) { + $this->getConnection()->putObject($args); + return; + } + + // Refuse to overwrite an existing object. A concurrent delete may race the + // write and yield a 409 Conflict; AWS allows retrying PutObject in that case, + // but only when the body can be rewound to resend it intact. + $args['IfNoneMatch'] = '*'; + $attempts = 0; + while (true) { + try { + $this->getConnection()->putObject($args); + return; + } catch (S3Exception $e) { + if ($this->isPreconditionFailed($e)) { + throw new ObjectAlreadyExistsException($urn, previous: $e); + } + if ($this->isConditionalConflict($e) && $stream->isSeekable() && $attempts < $this->retriesMaxAttempts) { + $attempts++; + usleep(100 * 1000 * $attempts); + $stream->rewind(); + continue; + } + throw $e; + } + } } /** @@ -134,7 +179,7 @@ protected function writeSingle(string $urn, StreamInterface $stream, array $meta * @param array $metaData the metadata to set for the object * @throws \Exception when something goes wrong, message will be logged */ - protected function writeMultiPart(string $urn, StreamInterface $stream, array $metaData): void { + protected function writeMultiPart(string $urn, StreamInterface $stream, array $metaData, bool $ifNoneMatch = false): void { $mimetype = $metaData['mimetype'] ?? null; unset($metaData['mimetype']); unset($metaData['size']); @@ -146,6 +191,7 @@ protected function writeMultiPart(string $urn, StreamInterface $stream, array $m $state = null; $size = $stream->getSize(); $totalWritten = 0; + $preconditionFailed = false; // retry multipart upload once with concurrency at half on failure while (!$uploaded && $attempts <= 1) { @@ -163,11 +209,17 @@ protected function writeMultiPart(string $urn, StreamInterface $stream, array $m 'before_upload' => function (Command $command) use (&$totalWritten): void { $totalWritten += $command['ContentLength']; }, - 'before_complete' => function ($_command) use (&$totalWritten, $size, &$uploader, &$attempts): void { + 'before_complete' => function (Command $command) use (&$totalWritten, $size, &$uploader, $ifNoneMatch): void { if ($size !== null && $totalWritten !== $size) { $e = new \Exception('Incomplete multi part upload, expected ' . $size . ' bytes, wrote ' . $totalWritten); throw new MultipartUploadException($uploader->getState(), $e); } + // Refuse to overwrite an object that already exists. In-progress + // multipart uploads are invisible to this check server-side, so it + // only guards against a fully written object at the same key. + if ($ifNoneMatch) { + $command['IfNoneMatch'] = '*'; + } }, ]); @@ -176,6 +228,14 @@ protected function writeMultiPart(string $urn, StreamInterface $stream, array $m $uploaded = true; } catch (S3MultipartUploadException $e) { $exception = $e; + + // A precondition failure means an object already exists at the key. + // Retrying the completion cannot succeed, so stop and report it. + if ($ifNoneMatch && $this->findPreconditionFailure($e)) { + $preconditionFailed = true; + break; + } + $attempts++; if ($concurrency > 1) { @@ -196,7 +256,21 @@ protected function writeMultiPart(string $urn, StreamInterface $stream, array $m // slow down s3 bucket with orphaned fragments $uploadInfo = $exception->getState()->getId(); if ($exception->getState()->isInitiated() && (array_key_exists('UploadId', $uploadInfo))) { - $this->getConnection()->abortMultipartUpload($uploadInfo); + try { + $this->getConnection()->abortMultipartUpload($uploadInfo); + } catch (\Throwable $abortException) { + // Best-effort cleanup: never let an abort failure mask the real error + // reported below. Orphaned fragments should be reaped by a bucket + // lifecycle rule for incomplete multipart uploads. + Server::get(LoggerInterface::class)->debug( + 'Could not abort multipart upload after a failed write to "' . $urn . '"', + ['app' => 'objectstore', 'exception' => $abortException], + ); + } + } + + if ($preconditionFailed) { + throw new ObjectAlreadyExistsException($urn, previous: $exception); } throw new BadGateway('Error while uploading to S3 bucket', 0, $exception); @@ -212,6 +286,18 @@ public function writeObject($urn, $stream, ?string $mimetype = null) { } public function writeObjectWithMetaData(string $urn, $stream, array $metaData): void { + $this->writeObjectWithCondition($urn, $stream, $metaData, false); + } + + public function writeObjectIfNotExists(string $urn, $stream, array $metaData = []): void { + $this->writeObjectWithCondition($urn, $stream, $metaData, true); + } + + /** + * @param resource $stream + * @param bool $ifNoneMatch write only if no object exists at $urn yet + */ + private function writeObjectWithCondition(string $urn, $stream, array $metaData, bool $ifNoneMatch): void { $canSeek = fseek($stream, 0, SEEK_CUR) === 0; $psrStream = Utils::streamFor($stream, [ 'size' => $metaData['size'] ?? null, @@ -227,7 +313,7 @@ public function writeObjectWithMetaData(string $urn, $stream, array $metaData): $buffer->seek(0); if ($buffer->getSize() < $this->putSizeLimit) { // buffer is fully seekable, so use it directly for the small upload - $this->writeSingle($urn, $buffer, $metaData); + $this->writeSingle($urn, $buffer, $metaData, $ifNoneMatch); } else { if ($psrStream->isSeekable()) { // If the body is seekable, just rewind the body. @@ -242,18 +328,153 @@ public function writeObjectWithMetaData(string $urn, $stream, array $metaData): $loadStream = new Psr7\AppendStream([$buffer, $psrStream]); } - $this->writeMultiPart($urn, $loadStream, $metaData); + $this->writeMultiPart($urn, $loadStream, $metaData, $ifNoneMatch); } } else { if ($size < $this->putSizeLimit) { - $this->writeSingle($urn, $psrStream, $metaData); + $this->writeSingle($urn, $psrStream, $metaData, $ifNoneMatch); } else { - $this->writeMultiPart($urn, $psrStream, $metaData); + $this->writeMultiPart($urn, $psrStream, $metaData, $ifNoneMatch); } } $psrStream->close(); } + public function supportsConditionalWrites(): bool { + if ($this->conditionalWritesSupported !== null) { + return $this->conditionalWritesSupported; + } + + if ($this->conditionalWrites === false) { + return $this->conditionalWritesSupported = false; + } + + // Conditional writes require AWS Signature v4; the legacy v2 signer cannot sign them. + if (!empty($this->params['legacy_auth'])) { + Server::get(LoggerInterface::class)->warning( + 'Conditional writes disabled for S3 object store "' . $this->bucket . '": legacy (Signature v2) authentication cannot sign them.', + ['app' => 'objectstore'], + ); + return $this->conditionalWritesSupported = false; + } + + if ($this->conditionalWrites === true) { + return $this->conditionalWritesSupported = true; + } + + // 'auto': probe the bucket once and cache the outcome. A null result is a probe + // failure (transient error, missing permissions, ...); it is memoized on this + // instance but neither cached nor shared, so a persistently failing probe costs + // one attempt per request instead of one per created file, and a later request + // still retries it. + return $this->conditionalWritesSupported = $this->probeConditionalWrites() ?? false; + } + + /** + * Actively probe whether the store enforces the If-None-Match header. + * + * A store that silently ignores the header cannot be detected any other way, + * so we write a marker object twice: once unconditionally, then again with the + * condition. A compliant store rejects the second write with 412; a store that + * accepts it ignores the header and is treated as unsupported. + * + * @return bool|null true or false once determined, or null when the probe could + * not run (transient error) and should be retried later + */ + private function probeConditionalWrites(): ?bool { + $cacheKey = $this->params['hostname'] . '::' . $this->bucket; + + // Process-level memo first: survives across requests in a long-lived worker even + // when no shared cache is configured, so the probe runs at most once per worker. + if (isset(self::$conditionalWritesProbeCache[$cacheKey])) { + return self::$conditionalWritesProbeCache[$cacheKey]; + } + + $cache = Server::get(ICacheFactory::class)->createDistributed('s3-conditional-write-cache'); + $cached = $cache->get($cacheKey); + if ($cached !== null) { + return self::$conditionalWritesProbeCache[$cacheKey] = ($cached === 1); + } + + $logger = Server::get(LoggerInterface::class); + // A unique key per probe run so that two concurrent probes cannot delete each + // other's marker and race to a wrong "ignores the header" conclusion. + $probeKey = self::CONDITIONAL_WRITE_PROBE_KEY . '-' . bin2hex(random_bytes(8)); + $probeArgs = [ + 'Bucket' => $this->bucket, + 'Key' => $probeKey, + 'Body' => '1', + // Mirror the real single-object write so a bucket policy that mandates an + // ACL or storage class does not reject the probe and wrongly disable the feature. + 'ACL' => 'private', + 'StorageClass' => $this->storageClass, + ] + $this->getServerSideEncryptionParameters(); + + $supported = false; + try { + $connection = $this->getConnection(); + // Make sure the marker object exists so the conditional write has something to conflict with. + $connection->putObject($probeArgs); + + try { + $connection->putObject($probeArgs + ['IfNoneMatch' => '*']); + // The store accepted a write that should have been rejected: it silently + // ignores the header and would give a false sense of safety. + $logger->warning( + 'S3 object store "' . $this->bucket . '" ignores the If-None-Match header; conditional writes disabled.', + ['app' => 'objectstore'], + ); + } catch (S3Exception $e) { + if ($this->isPreconditionFailed($e)) { + $supported = true; + } else { + $logger->info( + 'S3 object store "' . $this->bucket . '" does not support conditional writes (' . ($e->getAwsErrorCode() ?: $e->getStatusCode()) . '); disabled.', + ['app' => 'objectstore'], + ); + } + } + } catch (\Throwable $e) { + // The probe itself failed (transient error, missing permissions, ...). + // Return null so the result is neither cached nor memoized and can be retried. + $logger->debug('Could not probe S3 conditional write support for "' . $this->bucket . '"', ['app' => 'objectstore', 'exception' => $e]); + $this->deleteConditionalWriteProbe($probeKey); + return null; + } + + $this->deleteConditionalWriteProbe($probeKey); + $cache->set($cacheKey, $supported ? 1 : 0, self::CONDITIONAL_WRITE_PROBE_TTL); + return self::$conditionalWritesProbeCache[$cacheKey] = $supported; + } + + private function deleteConditionalWriteProbe(string $probeKey): void { + try { + $this->getConnection()->deleteObject([ + 'Bucket' => $this->bucket, + 'Key' => $probeKey, + ]); + } catch (\Throwable $e) { + // best-effort cleanup, a stray marker object is harmless + } + } + + private function isPreconditionFailed(AwsException $e): bool { + return $e->getStatusCode() === 412 || $e->getAwsErrorCode() === 'PreconditionFailed'; + } + + private function isConditionalConflict(AwsException $e): bool { + return $e->getStatusCode() === 409 || $e->getAwsErrorCode() === 'ConditionalRequestConflict'; + } + + private function findPreconditionFailure(\Throwable $e): bool { + for ($cursor = $e; $cursor !== null; $cursor = $cursor->getPrevious()) { + if ($cursor instanceof AwsException && $this->isPreconditionFailed($cursor)) { + return true; + } + } + return false; + } + /** * @param string $urn the unified resource name used to identify the object * @return void diff --git a/lib/public/Files/ObjectStore/IObjectStoreConditionalWrite.php b/lib/public/Files/ObjectStore/IObjectStoreConditionalWrite.php new file mode 100644 index 0000000000000..67a2fb2aa9c7c --- /dev/null +++ b/lib/public/Files/ObjectStore/IObjectStoreConditionalWrite.php @@ -0,0 +1,51 @@ +urn; + } +} diff --git a/tests/lib/Files/ObjectStore/ConditionalWriteObjectStore.php b/tests/lib/Files/ObjectStore/ConditionalWriteObjectStore.php new file mode 100644 index 0000000000000..00d8d6585ea5f --- /dev/null +++ b/tests/lib/Files/ObjectStore/ConditionalWriteObjectStore.php @@ -0,0 +1,88 @@ + the write methods invoked, in order */ + public array $writeCalls = []; + + public function __construct( + private IObjectStore $wrapped, + private bool $supported = true, + private bool $simulateExistingObject = false, + ) { + } + + public function setSupported(bool $supported): void { + $this->supported = $supported; + } + + public function setSimulateExistingObject(bool $simulate): void { + $this->simulateExistingObject = $simulate; + } + + #[\Override] + public function supportsConditionalWrites(): bool { + return $this->supported; + } + + #[\Override] + public function writeObjectIfNotExists(string $urn, $stream, array $metaData = []): void { + $this->writeCalls[] = 'writeObjectIfNotExists'; + if ($this->simulateExistingObject || $this->wrapped->objectExists($urn)) { + throw new ObjectAlreadyExistsException($urn); + } + $this->wrapped->writeObject($urn, $stream, $metaData['mimetype'] ?? null); + } + + #[\Override] + public function writeObject($urn, $stream, ?string $mimetype = null) { + $this->writeCalls[] = 'writeObject'; + $this->wrapped->writeObject($urn, $stream, $mimetype); + } + + #[\Override] + public function getStorageId() { + return $this->wrapped->getStorageId(); + } + + #[\Override] + public function readObject($urn) { + return $this->wrapped->readObject($urn); + } + + #[\Override] + public function deleteObject($urn) { + $this->wrapped->deleteObject($urn); + } + + #[\Override] + public function objectExists($urn) { + return $this->wrapped->objectExists($urn); + } + + #[\Override] + public function copyObject($from, $to) { + $this->wrapped->copyObject($from, $to); + } + + #[\Override] + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } +} diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStorageConditionalWriteTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStorageConditionalWriteTest.php new file mode 100644 index 0000000000000..1ccc097d77ec7 --- /dev/null +++ b/tests/lib/Files/ObjectStore/ObjectStoreStorageConditionalWriteTest.php @@ -0,0 +1,147 @@ +objectStore = new ConditionalWriteObjectStore(new StorageObjectStore(new Temporary())); + $this->storage = new ObjectStoreStorageOverwrite(['objectstore' => $this->objectStore]); + } + + #[\Override] + protected function tearDown(): void { + if (isset($this->storage)) { + $this->storage->getCache()->clear(); + } + parent::tearDown(); + } + + public function testNewFileUsesConditionalWrite(): void { + $this->storage->file_put_contents('/new.txt', 'hello'); + + self::assertSame(['writeObjectIfNotExists'], $this->objectStore->writeCalls); + self::assertSame('hello', $this->storage->file_get_contents('/new.txt')); + } + + public function testOverwriteUsesUnconditionalWrite(): void { + $this->storage->file_put_contents('/file.txt', 'first'); + $this->objectStore->writeCalls = []; + + $this->storage->file_put_contents('/file.txt', 'second'); + + self::assertSame(['writeObject'], $this->objectStore->writeCalls); + self::assertSame('second', $this->storage->file_get_contents('/file.txt')); + } + + public function testUnsupportedStoreUsesUnconditionalWrite(): void { + $this->objectStore->setSupported(false); + + $this->storage->file_put_contents('/new.txt', 'hello'); + + self::assertSame(['writeObject'], $this->objectStore->writeCalls); + self::assertSame('hello', $this->storage->file_get_contents('/new.txt')); + } + + public function testExistingObjectOnCreateIsRefusedWithoutDataLoss(): void { + // Simulate the file cache and bucket being out of sync: an object already + // occupies the urn that the newly created file will target. + $this->objectStore->setSimulateExistingObject(true); + + $threw = false; + try { + $this->storage->file_put_contents('/ghost.txt', 'payload'); + } catch (GenericFileException) { + $threw = true; + } + + self::assertTrue($threw, 'A create onto an already occupied urn must fail'); + self::assertSame(['writeObjectIfNotExists'], $this->objectStore->writeCalls); + // The failed create must not leave the file (or a stray .part entry) behind. + self::assertFalse($this->storage->file_exists('/ghost.txt')); + self::assertFalse($this->storage->getCache()->inCache('ghost.txt')); + self::assertFalse($this->storage->getCache()->inCache('ghost.txt.part')); + } + + public function testStoreWithoutConditionalInterfaceUsesUnconditionalWrite(): void { + // A backend that does not advertise IObjectStoreConditionalWrite must keep the + // previous behaviour: a plain write, no conditional dispatch. + $recorder = new class(new StorageObjectStore(new Temporary())) implements IObjectStore { + /** @var list */ + public array $writeCalls = []; + + public function __construct( + private IObjectStore $wrapped, + ) { + } + + #[\Override] + public function getStorageId() { + return $this->wrapped->getStorageId(); + } + + #[\Override] + public function readObject($urn) { + return $this->wrapped->readObject($urn); + } + + #[\Override] + public function writeObject($urn, $stream, ?string $mimetype = null) { + $this->writeCalls[] = 'writeObject'; + $this->wrapped->writeObject($urn, $stream, $mimetype); + } + + #[\Override] + public function deleteObject($urn) { + $this->wrapped->deleteObject($urn); + } + + #[\Override] + public function objectExists($urn) { + return $this->wrapped->objectExists($urn); + } + + #[\Override] + public function copyObject($from, $to) { + $this->wrapped->copyObject($from, $to); + } + + #[\Override] + public function preSignedUrl(string $urn, \DateTimeInterface $expiration): ?string { + return null; + } + }; + + $storage = new ObjectStoreStorageOverwrite(['objectstore' => $recorder]); + try { + $storage->file_put_contents('/plain.txt', 'hello'); + + self::assertSame(['writeObject'], $recorder->writeCalls); + self::assertSame('hello', $storage->file_get_contents('/plain.txt')); + } finally { + $storage->getCache()->clear(); + } + } +} diff --git a/tests/lib/Files/ObjectStore/S3Test.php b/tests/lib/Files/ObjectStore/S3Test.php index 94719484f24a8..656a2b8e69ce7 100644 --- a/tests/lib/Files/ObjectStore/S3Test.php +++ b/tests/lib/Files/ObjectStore/S3Test.php @@ -9,6 +9,9 @@ use Icewind\Streams\Wrapper; use OC\Files\ObjectStore\S3; +use OC\Memcache\ArrayCache; +use OCP\Files\ObjectStore\ObjectAlreadyExistsException; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\Server; @@ -190,4 +193,107 @@ public function testFileSizes($size): void { $this->assertNoUpload('testfilesizes'); } + + private function getConfiguredArguments(): array { + $config = Server::get(IConfig::class)->getSystemValue('objectstore'); + if (!is_array($config) || $config['class'] !== S3::class) { + $this->markTestSkipped('objectstore not configured for s3'); + } + // Conditional writes are opt-in (default off); enable them for these tests. + return ['conditional_writes' => 'auto'] + $config['arguments']; + } + + public function testConditionalWriteRejectsOverwrite(): void { + $this->cleanupAfter('conditional-write'); + $s3 = new S3($this->getConfiguredArguments()); + if (!$s3->supportsConditionalWrites()) { + $this->markTestSkipped('the configured object store does not enforce conditional writes'); + } + + $s3->writeObjectIfNotExists('conditional-write', $this->stringToStream('first')); + self::assertSame('first', stream_get_contents($s3->readObject('conditional-write'))); + + $thrown = false; + try { + $s3->writeObjectIfNotExists('conditional-write', $this->stringToStream('second')); + } catch (ObjectAlreadyExistsException) { + $thrown = true; + } + + self::assertTrue($thrown, 'A conditional write to an existing key must be refused'); + // The original data must be preserved. + self::assertSame('first', stream_get_contents($s3->readObject('conditional-write'))); + } + + public function testConditionalWriteRejectsOverwriteMultipart(): void { + $this->cleanupAfter('conditional-write-mpu'); + $arguments = $this->getConfiguredArguments(); + // Force the multipart-upload path even for tiny objects. + $arguments['putSizeLimit'] = 1; + $arguments['uploadPartSize'] = 5 * 1024 * 1024; + $s3 = new S3($arguments); + if (!$s3->supportsConditionalWrites()) { + $this->markTestSkipped('the configured object store does not enforce conditional writes'); + } + + $s3->writeObjectIfNotExists('conditional-write-mpu', $this->stringToStream('first')); + self::assertSame('first', stream_get_contents($s3->readObject('conditional-write-mpu'))); + + $thrown = false; + try { + $s3->writeObjectIfNotExists('conditional-write-mpu', $this->stringToStream('second')); + } catch (ObjectAlreadyExistsException) { + $thrown = true; + } + + self::assertTrue($thrown, 'A conditional multipart write to an existing key must be refused'); + self::assertSame('first', stream_get_contents($s3->readObject('conditional-write-mpu'))); + } + + public function testConditionalWritesDisabledByConfig(): void { + $arguments = $this->getConfiguredArguments(); + $arguments['conditional_writes'] = false; + $s3 = new S3($arguments); + self::assertFalse($s3->supportsConditionalWrites()); + } + + public function testConditionalWriteSupportUsesCachedResult(): void { + $arguments = $this->getConfiguredArguments(); + + // Back the cache with a single shared in-memory instance so a stored probe + // result is observable. In production the distributed cache (e.g. Redis) is a + // shared backend; the phpunit bootstrap otherwise hands out a fresh, isolated + // ArrayCache per createDistributed() call, which cannot be observed across calls. + $cache = new ArrayCache(''); + $factory = $this->createMock(ICacheFactory::class); + $factory->method('createDistributed')->willReturn($cache); + $factory->method('createLocal')->willReturn($cache); + $this->overwriteService(ICacheFactory::class, $factory); + + // Distinct buckets so the process-level probe memo does not carry the first + // result over into the second assertion, and a pinned hostname so the cache key + // matches without depending on the configured arguments carrying one. + $hostname = 'conditional-writes.test'; + $negative = ['bucket' => $arguments['bucket'] . '-cw-neg', 'hostname' => $hostname] + $arguments; + $positive = ['bucket' => $arguments['bucket'] . '-cw-pos', 'hostname' => $hostname] + $arguments; + + try { + // A cached negative result is honoured (in 'auto' mode) without probing the store. + $cache->set($hostname . '::' . $negative['bucket'], 0); + self::assertFalse((new S3($negative))->supportsConditionalWrites()); + + // A cached positive result is likewise reused. + $cache->set($hostname . '::' . $positive['bucket'], 1); + self::assertTrue((new S3($positive))->supportsConditionalWrites()); + + // An empty or unrecognized mode falls back to disabled, never to 'auto': it + // must not pick up that positive result, nor probe the store at all. + foreach (['', 'off'] as $mode) { + $invalid = ['conditional_writes' => $mode] + $positive; + self::assertFalse((new S3($invalid))->supportsConditionalWrites(), 'mode: ' . var_export($mode, true)); + } + } finally { + $this->restoreService(ICacheFactory::class); + } + } }