fix(encryption): Refactor EncryptionWrapper, fix zero-byte size, add S3 tests#57279
Open
cuppett wants to merge 17 commits intonextcloud:masterfrom
Open
fix(encryption): Refactor EncryptionWrapper, fix zero-byte size, add S3 tests#57279cuppett wants to merge 17 commits intonextcloud:masterfrom
cuppett wants to merge 17 commits intonextcloud:masterfrom
Conversation
cuppett
requested review from
leftybournes,
salmart-dev,
sorbaugh and
yemkareems
and removed request for
a team
cuppett
force-pushed
the
fix/s3-primary-storage-encryption
branch
2 times, most recently
from
0a01f11 to
fd39e51
Compare
cuppett
force-pushed
the
fix/s3-primary-storage-encryption
branch
12 times, most recently
from
f4f2d21 to
1a81edd
Compare
Member
|
Failing test seems related. e.g. |
Member
This is not true, the home storage is mounted at |
cuppett
force-pushed
the
fix/s3-primary-storage-encryption
branch
from
fcadd6f to
c0c3ae6
Compare
cuppett
changed the title
cuppett
force-pushed
the
fix/s3-primary-storage-encryption
branch
2 times, most recently
from
30903a9 to
4dc1def
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cuppett
force-pushed
the
fix/s3-primary-storage-encryption
branch
2 times, most recently
from
ebc2843 to
0c4e10b
Compare
Contributor
Author
|
@salmart-dev @sorbaugh @leftybournes I think I'm ready here (after a slog). Can a couple of you review? I whittled this back down. I think the main things are:
|
cuppett
force-pushed
the
fix/s3-primary-storage-encryption
branch
2 times, most recently
from
f723818 to
1e1aeaa
Compare
miaulalala
requested changes
Apr 23, 2026
…y object storage (S3) This simplifies the encryption wrapper to clarify the logic and setting checks. Fix: - Now checks encryptHomeStorage app setting for home mount point - When enabled (default), encryption wrapper is applied to primary storage - Maintains backward compatibility with existing behaviors Testing: - Comprehensive test suite with 23 tests covering 1KB to 110MB files - Validates size consistency across database, S3, and actual content - Tests multipart uploads, seeking, partial reads, and streaming - All tests passing on real AWS S3 with encryption enabled - Migration test suite (3 tests) for upgrade scenarios - Updates to EncryptionWrapperTest mock objects Verified: - Files are encrypted in S3 (AES-256-CTR with HMAC signatures) - Size tracking accurate (DB stores unencrypted size) - No corruption at any file size (including 16MB, 64MB, 100MB) - Multipart uploads work correctly with encryption - Storage overhead: ~1.2% for files > 1MB Migration Path: - Users with unencrypted files can run encryption:encrypt-all - Command safely handles mixed encrypted/unencrypted content - Skips already-encrypted files (checks database flag) This validates in tests long-standing GitHub issues where users reported encryption not working with S3 primary storage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Stephen Cuppett <steve@cuppett.com>
This fixes a bug where zero-byte encrypted files were incorrectly reported as 8192 bytes (the encryption header size) instead of 0 bytes. Root Cause: - CacheEntry::getUnencryptedSize() and FileInfo::getSize() checked if unencrypted_size > 0 before using it - For zero-byte files, this condition fails (0 > 0 is false) - Falls back to encrypted size (8192 bytes) instead of 0 Impact: - Zero-byte encrypted files showed wrong size in UI and API - Database stored correct unencrypted_size=0, but getSize() returned 8192 - Affected quota calculations, file listings, and size comparisons Fix: - Remove the > 0 check from both methods - Now checks only isset() to distinguish between missing and zero values - Zero-byte files correctly report size as 0 Testing: - Added zero-byte files to all encryption test scenarios - testSizeConsistencyAcrossSources with 0 bytes - PASS - testEncryptedFileRoundTrip with 0 bytes - PASS - testEncryptedFileIntegrity with 0 bytes - PASS - All 26 tests passing including edge cases Files Modified: - lib/private/Files/Cache/CacheEntry.php - getUnencryptedSize() - lib/private/Files/FileInfo.php - getSize() - tests/lib/Files/ObjectStore/S3EncryptionTest.php - Added 0-byte test cases fix(encryption): Fix unencrypted size calculation for cached zero values When the zero-byte fix removed the `> 0` check from getUnencryptedSize(), it exposed a gap in verifyUnencryptedSize() that didn't recalculate the size when unencrypted_size=0 but the encrypted file has content. This caused Behat tests to fail because: 1. Newly encrypted files had unencrypted_size=0 in cache 2. getUnencryptedSize() returned 0 (instead of falling back to size) 3. verifyUnencryptedSize() didn't recalculate (conditions didn't match) 4. Encryption stream received 0 and returned empty content Added condition to trigger recalculation when cache reports 0 bytes but the physical encrypted file has content (size > 0). Fixes 21 Behat test failures: - files_features/encryption.feature (1 scenario) - files_features/transfer-ownership.feature (20 scenarios) fix(encryption): Fix type error for unencryptedSize property The Stream\Encryption::$unencryptedSize property was typed as ?int, but fixUnencryptedSize() returns int|float for large files. This caused TypeErrors when the verifyUnencryptedSize() fix triggered recalculation. Changed property type from ?int to int|float|null to match the return type of fixUnencryptedSize(). Fixes: - EncryptionMasterKeyUploadTest::testUploadOverWrite - EncryptionUploadTest::testUploadOverWrite 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Fixes 375 PHPUnit errors in php8.2-s3-minio CI caused by encryption tests not cleaning up their state, polluting subsequent tests. Root Cause: - 5 tests using EncryptionTrait enable encryption but have NO tearDown - After tests complete, encryption_enabled remains 'yes' - EncryptionWrapper sees encryption enabled for ALL subsequent tests - Tests without encryption keys fail with "multikeyencryption failed" Tests Fixed: 1. apps/encryption/tests/EncryptedStorageTest.php 2. apps/encryption/tests/Command/FixEncryptedVersionTest.php 3. apps/files_sharing/tests/EncryptedSizePropagationTest.php 4. apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionUploadTest.php 5. apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionMasterKeyUploadTest.php Fix: Add tearDown() method to each test that calls tearDownEncryptionTrait(), which restores encryption_enabled to its previous value. Also Fixed: - tests/lib/Encryption/EncryptionWrapperTest.php: Mock isEnabled() for tests that expect wrapper to be applied Testing: - Local full suite run: 0 encryption errors (vs 375 in CI) - Encryption state properly restored after tests - ViewTest passes after encryption tests run Impact: - Prevents test state pollution in CI - Each test properly cleans up encryption configuration - No functional changes to production code fix(tests): Add global encryption state cleanup to prevent test pollution Fixes 255 PHPUnit errors in php8.2-s3-minio CI caused by encryption state pollution. Tests that enable encryption (encryption_enabled='yes') without proper cleanup now have automatic cleanup in base TestCase. Root Cause: - EncryptionWrapper.wrapStorage() now checks Manager::isEnabled() - This check is new (not in master) and makes wrapper sensitive to encryption_enabled app config value - Tests leaving encryption_enabled='yes' cause subsequent tests to fail - HomeMountPoints get encryption wrapper but no user keys exist - Results in MultiKeyEncryptException in 255+ tests Previous Fix Attempt (c0c3ae6): - Added tearDown to 5 specific test files - Reduced errors from 375 to 255 - But pollution source remained unknown This Fix: - Add encryption cleanup to TestCase.tearDown() base method - Runs after all trait tearDown methods - Automatically resets encryption state if enabled - Applies to ALL 6000+ tests extending TestCase - No need to track down individual pollution sources Impact: - Prevents all encryption state pollution - Minimal performance impact (only resets if enabled) - Safe (try-catch prevents breaking tests) - Comprehensive (applies to entire test suite) Testing: - Verified cleanup runs: manually set encryption_enabled='yes' - Ran ViewTest: failed (expected - no keys) - After test: encryption_enabled reset to 'no' (cleanup worked) Fixes: https://github.com/nextcloud/server/actions/runs/20576563150 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Stephen Cuppett <steve@cuppett.com>
This fixes a bug where encrypted files uploaded via stream (e.g., using `occ files:put` or `Folder::newFile($path, $stream)`) had their `unencrypted_size` incorrectly set to 0 in the database, even though the files were properly encrypted and the encryption wrapper provided the correct size. Root Cause: The Scanner class had overly restrictive logic (from commit 649bed5) that prevented `unencrypted_size` from being updated in two scenarios: 1. **For existing files with cached unencrypted_size=0:** - Scanner would get correct value from encryption wrapper - But would remove it because cached value was 0 - Result: Cache never updated from incorrect 0 to correct value 2. **For new file uploads:** - Encryption wrapper provided correct unencrypted_size in metadata - But Scanner unconditionally removed it for new files - Result: Cache entry created with unencrypted_size=0 Impact Before Fix: - Files uploaded via `occ files:put` had unencrypted_size=0 - `info:file` reported all files as "size: 0 B" - Required running `files:scan` to fix the values - The workaround in verifyUnencryptedSize() fixed it on access/scan, but didn't prevent the incorrect initial value Impact After Fix: - Files uploaded via any method have correct unencrypted_size immediately - No scan required to fix database values - Both zero-byte and non-zero encrypted files work correctly The Fix: 1. **For existing files (line 200-207):** - Only skip update if BOTH cached AND new values are 0 - Allows updating from incorrect cached 0 to correct non-zero value - Avoids unnecessary updates when both are legitimately 0 2. **For new files (line 223-228):** - Only remove unencrypted_size if file is not encrypted - Or if unencrypted_size is 0/not set (unencrypted file) - Preserves correct unencrypted_size for encrypted files Testing: - Verified 0B, 1MB, 100MB files upload with correct unencrypted_size - Database values correct immediately after upload (no scan needed) - Zero-byte files still report size as 0 (not 8192) - Local and S3 storage both work correctly Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Fixes AppConfigTypeConflictException when setting encryption configuration values. The deprecated setAppValue()/getAppValue() methods default to MIXED type, which conflicts with existing STRING type values in the database. Changes: - core/Command/Encryption/Enable: Use setValueString/getValueString - core/Command/Encryption/Disable: Use setValueString/getValueString - tests/lib/Traits/EncryptionTrait: Use typed IAppConfig methods This resolves all 29 test failures in the PRIMARY-s3 PHPUnit test group where encryption setup was throwing type conflict exceptions. Co-authored-by: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Update unit tests for Enable and Disable commands to mock the new IAppConfig dependency that was added in the previous commit. Changes: - tests/Core/Command/Encryption/EnableTest: Add IAppConfig mock - tests/Core/Command/Encryption/DisableTest: Add IAppConfig mock - Update test expectations to use getValueString/setValueString Fixes 7 test failures in the NODB test suite. Co-authored-by: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>
… errors Add 19 pre-existing Psalm errors to the baseline that are not caused by this branch's encryption fixes: - 14 Imagick-related UndefinedClass/UndefinedConstant errors in apps/theming/lib/IconBuilder.php, apps/theming/lib/ImageManager.php, lib/private/Preview/Bitmap.php, lib/private/Preview/HEIC.php, and lib/private/Preview/SVG.php. These stem from the imagick.php stub at build/stubs/imagick.php missing ImagickPixel, ImagickDraw, ImagickException classes and several constants. - 1 DeprecatedMethod error in tests/lib/TestCase.php for SimpleContainer::offsetGet, introduced when that file was added to psalm.xml project files by upstream commit a430702. None of these errors originate from this branch's changes. Signed-off-by: Stephen Cuppett <steve@cuppett.com>
…lm baseline - Remove unrelated Imagick entries from psalm-baseline.xml that caused static-code-analysis CI failure; these are pre-existing stubs resolved independently of this PR's changes - Fix Scanner.php: preserve valid unencrypted_size === 0 for encrypted new files by dropping the erroneous === 0 check in the new-file branch - Align S3EncryptionTest/S3EncryptionMigrationTest setUp/tearDown with EncryptionTrait: remove duplicate IConfig state snapshot (taken after encryption was already enabled, which overwrote the trait's snapshot of the pre-test state), call tearDownEncryptionTrait() instead - Remove permanently-skipped testEncryptAllHandlesMixedContent and its unused createUnencryptedFileInS3 helper - Fix testDetectUnencryptedFilesQuery: replace LIKE on numeric storage column with proper two-step query using IN + PARAM_INT_ARRAY - Reduce peak memory in large-file tests: hash before unset(data), use incremental hash_init/hash_update/hash_final for streaming integrity; gate 64 MB / 100 MB data-provider cases behind RUN_HEAVY_S3_TESTS - Skip 110 MB multipart test behind same env gate; fix misleading "6MB file" comment (actual size is 110 MiB) - Drop unused IConfig constructor param from Enable/Disable commands and their tests; commands already use IAppConfig exclusively Signed-off-by: Stephen Cuppett <steve@cuppett.com>
…n writes Setup::setupSystem() is gated by a local 'keys-validated' cache and can no-op even on a fresh S3 bucket where the keys have never been written. This caused every write in S3EncryptionTest to throw MultiKeyEncryptException because getPublicMasterKey() returned an empty string. Call validateMasterKey() and validateShareKey() directly in setUp() — matching the pattern already used by apps/encryption/tests/EncryptedStorageTest.php — to guarantee system keys exist in the S3 keystore before any file operations. Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Replace deprecated IConfig::getAppValue() with the typed IAppConfig::getValueString() for the encryptHomeStorage check, consistent with the rest of this PR's config migration. Signed-off-by: Stephen Cuppett <steve@cuppett.com>
- Guard $config['class'] with null coalescing in setUpBeforeClass() to avoid PHP notice when objectstore is configured without a 'class' key - Remove unused $rootStorage property and unwrapping loop in setUp() (was assigned but never referenced in any test method) - Chain previous exception in getObjectUrn() throw to preserve stack trace Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Contributor
Author
|
Working through propagating getBoolVal everywhere. |
Contributor
|
Your tests fail bc the IAppConfig methods are called before the Nextcloud install is finished. The old code handled that gracefully, but the current code does not. Please change your code to wrap the IAppConfig calls in |
…meStorage to bool Add RetypeEncryptionConfigKeys IRepairStep to convert these two appconfig keys from VALUE_STRING to VALUE_BOOL on upgrade. IAppConfig::getValueBool() and setValueBool() throw AppConfigTypeConflictException against STRING-typed rows; this migration reads the current string value, deletes the row, and rewrites it as a bool so callers can safely use the typed API. Handles 'yes/no/1/0/true/false/on' truthy parsing (mirrors AppConfig internals), is idempotent (no-op when already bool or absent), and is registered first in Repair::getRepairSteps() so it runs before any later code path reads these keys as bool. Signed-off-by: Stephen Cuppett <steve@cuppett.com>
…n keys Switch all reads and writes of core.encryption_enabled and encryption.encryptHomeStorage from string-based IConfig/IAppConfig getValueString/setValueString to typed IAppConfig getValueBool/setValueBool, per PR review feedback. The preceding commit adds a Repair step that re-types any existing STRING-typed rows to BOOL before these callers run. Production changes: - core/Command/Encryption/DecryptAll: getValueBool / setValueBool - core/Command/Encryption/Enable: getValueBool / setValueBool - core/Command/Encryption/Disable: getValueBool / setValueBool - lib/private/Encryption/Manager: inject IAppConfig, getValueBool for isEnabled() - lib/private/Encryption/EncryptionWrapper: getValueBool for encryptHomeStorage - lib/private/Server: pass IAppConfig to Manager factory - apps/encryption/lib/Util: inject IAppConfig, getValueBool/setValueBool - apps/encryption/lib/Settings/Admin: pass IAppConfig to new Util(...) - apps/provisioning_api/lib/Controller/AppConfigController: expand encryption_enabled guard to accept yes/1/true/on (case-insensitive) Test changes: - All mock expectations updated to getValueBool/setValueBool - EncryptionTrait: bool property, getValueBool/setValueBool - TestCase tearDown: getValueBool/setValueBool - AppConfigControllerTest: extended data providers for new truthy spellings - Psalm baseline: remove 2 stale DeprecatedMethod entries resolved by this change Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Manager::isEnabled() and EncryptionWrapper::wrap() are both called during
OC::init() bootstrap, before occ upgrade has had a chance to run the
RetypeEncryptionConfigKeys repair step. If encryption_enabled or
encryptHomeStorage were previously stored as VALUE_STRING, the new
getValueBool() calls throw AppConfigTypeConflictException and prevent the
server from booting at all - making occ upgrade unrunnable.
Add a catch(AppConfigTypeConflictException) fallback to both callers that
reads the raw string value and interprets it the legacy way ('yes'/'1' →
true). This fallback only fires in the upgrade window between extracting
new code and running occ upgrade; once the repair step has re-typed the
rows it is never reached again.
Also strengthen RetypeEncryptionConfigKeysTest setUp() to delete both
keys before each test, so real-DB residue from occ invocations in the
same process cannot pollute the absence-check test.
Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Injecting IAppConfig eagerly via the Manager constructor caused AppConfig to be resolved during registerEncryptionWrapperAndHooks() — earlier in the boot sequence than upstream master. This changed the timing of the first loadConfig() call in AppConfig relative to when oc_appconfig is accessible, producing a crash via Server::boot() → HookConnector → new View() → ILockingProvider → Memcache::createLocking() → getGlobalPrefix() → getAppInstalledVersions() during php -f tests/enable_all.php in CI. Fix: remove IAppConfig from the Manager constructor and resolve it lazily via Server::get(IAppConfig::class) inside isEnabled() only, guarded by the existing installed=false early return. This mirrors the pattern in EncryptionWrapper::shouldEncryptHomeStorage() and ensures AppConfig is first resolved after the DB schema is fully ready. The AppConfigTypeConflictException and \Throwable catches are preserved for upgrade-window and DB-not-ready safety respectively. Update ManagerTest::testManagerIsEnabled() to use the real AppConfig singleton (now that the constructor no longer accepts a mock) and mark it @group DB since it requires oc_appconfig to exist. Signed-off-by: Stephen Cuppett <steve@cuppett.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments 1 & 2 (Enable/Disable commands): wrap getValueBool/setValueBool calls with AppConfigTypeConflictException catches so occ encryption:enable and occ encryption:disable don't crash on pre-upgrade installs where core.encryption_enabled is still VALUE_STRING typed. Comment 3 (EncryptionWrapper): the legacy string fallback in shouldEncryptHomeStorage() was only treating '1' as truthy, inconsistent with RetypeEncryptionConfigKeys which accepts '1', 'true', 'yes', 'on'. Extract parseLegacyBoolString() helper and use the full truthy set. Apply the same fix to Manager::isEnabled()'s fallback which was only checking === 'yes'. DecryptAll: same AppConfigTypeConflictException pattern on its four setValueBool calls, extracted to a writeEncryptionEnabled() helper to avoid repetition. Comment 4 (S3EncryptionTest): the testEncryptedMultipartUpload heavy test was calling file_get_contents() on a 110MiB file to verify the hash. Switch to streaming fread/hash_update loop to keep peak memory bounded. Comment 5 (S3EncryptionMigrationTest): testDetectUnencryptedFilesQuery only asserted the count was numeric. Add a direct filecache row check to verify the encrypted test file is actually marked encrypted=1, so the test catches regressions in the detection logic. Comment 6 (AdminTest.php willReturnMap): NOT applied — the reviewer's suggestion to use 4 elements is incorrect. Admin.php calls getValueBool with 2 explicit args, so the 3-element map entry (2 args + return value) is correct and the test passes. Signed-off-by: Stephen Cuppett <steve@cuppett.com>
The migration test writes files that are intentionally unencrypted (simulating pre-migration state), so asserting encrypted != 0 was wrong. Replace with a weaker but still meaningful assertion: verify the file actually landed in filecache (proving the write and query target the same table), plus the existing numeric-result sanity check. Signed-off-by: Stephen Cuppett <steve@cuppett.com>
Contributor
Author
|
@miaulalala Okay, I'm back! Did the things to convert to a boolean throughout for that app config setting. |
Contributor
PR is looking good, tests pass. @artonge or @CarlSchwan can either of you review? |
miaulalala
approved these changes
Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR makes several improvements to Nextcloud's encryption infrastructure:
encryptHomeStoragesetting for home storage mountsChanges
Code Fixes
lib/private/Encryption/EncryptionWrapper.phpRefactored storage wrapper logic:
encryptHomeStoragesetting on HomeMountPoint mountslib/private/Files/Cache/CacheEntry.php&lib/private/Files/FileInfo.phpFixed zero-byte file size reporting:
> 0check that caused 0-byte encrypted files to report as 8192 bytes (encryption header size)Test Infrastructure
New:
tests/lib/Files/ObjectStore/S3EncryptionTest.php(493 lines)Comprehensive S3 encryption test suite covering:
New:
tests/lib/Files/ObjectStore/S3EncryptionMigrationTest.php(270 lines)Migration scenario tests:
encryption:encrypt-alltests/lib/TestCase.php+ 5 encryption test filesFixed test pollution:
MultiKeyEncryptExceptionfailures from state leakage between testsFiles Changed
lib/private/Encryption/EncryptionWrapper.phplib/private/Files/Cache/CacheEntry.phplib/private/Files/FileInfo.phptests/lib/TestCase.phptests/lib/Encryption/EncryptionWrapperTest.phptests/lib/Files/ObjectStore/S3EncryptionTest.phptests/lib/Files/ObjectStore/S3EncryptionMigrationTest.phpapps/Backward Compatibility
✅ Fully backward compatible
encryptHomeStoragesetting defaults to enabled ('1')Testing
Run the S3 encryption tests: