Merge pull request #166 from cryptomator/feature/165-dont-truncate-he…

…ader Share file header across all OpenFile objects and truncate encrypted files only to fileHeader.size() Fixes #165, fixes #168
cryptomator · Apr 11, 2023 · f426f74 · f426f74
2 parents 48a6b88 + ab4a809
commit f426f74
Show file tree

Hide file tree

Showing 11 changed files with 205 additions and 61 deletions.
diff --git a/src/main/java/org/cryptomator/cryptofs/EffectiveOpenOptions.java b/src/main/java/org/cryptomator/cryptofs/EffectiveOpenOptions.java
@@ -182,6 +182,7 @@ public Set<OpenOption> createOpenOptionsForEncryptedFile() {
 		result.add(READ); // also needed during write
 		result.remove(LinkOption.NOFOLLOW_LINKS);
 		result.remove(APPEND);
+		result.remove(TRUNCATE_EXISTING);
 		return result;
 	}
 

diff --git a/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java b/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java
@@ -3,7 +3,6 @@
 import dagger.BindsInstance;
 import dagger.Subcomponent;
 import org.cryptomator.cryptofs.EffectiveOpenOptions;
-import org.cryptomator.cryptolib.api.FileHeader;
 
 import java.nio.channels.FileChannel;
 
@@ -17,8 +16,6 @@ public interface ChannelComponent {
 	interface Factory {
 
 		ChannelComponent create(@BindsInstance FileChannel ciphertextChannel, //
-								@BindsInstance FileHeader fileHeader, //
-								@BindsInstance @MustWriteHeader boolean mustWriteHeader, //
 								@BindsInstance EffectiveOpenOptions options, //
 								@BindsInstance ChannelCloseListener listener); //
 	}

diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java
@@ -7,10 +7,10 @@
 import org.cryptomator.cryptofs.fh.Chunk;
 import org.cryptomator.cryptofs.fh.ChunkCache;
 import org.cryptomator.cryptofs.fh.ExceptionsDuringWrite;
+import org.cryptomator.cryptofs.fh.FileHeaderHolder;
 import org.cryptomator.cryptofs.fh.OpenFileModifiedDate;
 import org.cryptomator.cryptofs.fh.OpenFileSize;
 import org.cryptomator.cryptolib.api.Cryptor;
-import org.cryptomator.cryptolib.api.FileHeader;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -25,7 +25,6 @@
 import java.nio.file.attribute.BasicFileAttributeView;
 import java.nio.file.attribute.FileTime;
 import java.time.Instant;
-import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.ReadWriteLock;
@@ -40,7 +39,7 @@ public class CleartextFileChannel extends AbstractFileChannel {
 	private static final Logger LOG = LoggerFactory.getLogger(CleartextFileChannel.class);
 
 	private final FileChannel ciphertextFileChannel;
-	private final FileHeader fileHeader;
+	private final FileHeaderHolder fileHeaderHolder;
 	private final Cryptor cryptor;
 	private final ChunkCache chunkCache;
 	private final BufferPool bufferPool;
@@ -51,13 +50,12 @@ public class CleartextFileChannel extends AbstractFileChannel {
 	private final ExceptionsDuringWrite exceptionsDuringWrite;
 	private final ChannelCloseListener closeListener;
 	private final CryptoFileSystemStats stats;
-	private final AtomicBoolean mustWriteHeader;
 
 	@Inject
-	public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeader fileHeader, @MustWriteHeader boolean mustWriteHeader, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference<Instant> lastModified, Supplier<BasicFileAttributeView> attrViewProvider, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) {
+	public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder fileHeaderHolder, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference<Instant> lastModified, Supplier<BasicFileAttributeView> attrViewProvider, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) {
 		super(readWriteLock);
 		this.ciphertextFileChannel = ciphertextFileChannel;
-		this.fileHeader = fileHeader;
+		this.fileHeaderHolder = fileHeaderHolder;
 		this.cryptor = cryptor;
 		this.chunkCache = chunkCache;
 		this.bufferPool = bufferPool;
@@ -71,7 +69,6 @@ public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeader fileHe
 		if (options.append()) {
 			position = fileSize.get();
 		}
-		this.mustWriteHeader = new AtomicBoolean(mustWriteHeader);
 		if (options.createNew() || options.create()) {
 			lastModified.compareAndSet(Instant.EPOCH, Instant.now());
 		}
@@ -183,11 +180,12 @@ private long writeLockedInternal(ByteSource src, long position) throws IOExcepti
 	}
 
 	private void writeHeaderIfNeeded() throws IOException {
-		if (mustWriteHeader.getAndSet(false)) {
-			LOG.trace("{} - Writing file header.", this);
-			ByteBuffer encryptedHeader = cryptor.fileHeaderCryptor().encryptHeader(fileHeader);
-			ciphertextFileChannel.write(encryptedHeader, 0);
+		if (fileHeaderHolder.headerIsPersisted().get()) {
+			return;
 		}
+		LOG.trace("{} - Writing file header.", this);
+		ciphertextFileChannel.write(fileHeaderHolder.getEncrypted(), 0);
+		fileHeaderHolder.headerIsPersisted().set(true);
 	}
 
 	@Override

diff --git a/src/main/java/org/cryptomator/cryptofs/ch/MustWriteHeader.java b/src/main/java/org/cryptomator/cryptofs/ch/MustWriteHeader.java
diff --git a/src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java b/src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java
@@ -11,6 +11,7 @@
 import java.nio.ByteBuffer;
 import java.nio.channels.FileChannel;
 import java.nio.file.Path;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 
 @OpenFileScoped
@@ -21,6 +22,8 @@ public class FileHeaderHolder {
 	private final Cryptor cryptor;
 	private final AtomicReference<Path> path;
 	private final AtomicReference<FileHeader> header = new AtomicReference<>();
+	private final AtomicReference<ByteBuffer> encryptedHeader = new AtomicReference<>();
+	private final AtomicBoolean isPersisted = new AtomicBoolean();
 
 	@Inject
 	public FileHeaderHolder(Cryptor cryptor, @CurrentOpenFilePath AtomicReference<Path> path) {
@@ -36,26 +39,48 @@ public FileHeader get() {
 		return result;
 	}
 
-	public FileHeader createNew() {
+	public ByteBuffer getEncrypted() {
+		var result = encryptedHeader.get();
+		if (result == null) {
+			throw new IllegalStateException("Header not set.");
+		}
+		return result;
+	}
+
+	FileHeader createNew() {
 		LOG.trace("Generating file header for {}", path.get());
 		FileHeader newHeader = cryptor.fileHeaderCryptor().create();
+		encryptedHeader.set(cryptor.fileHeaderCryptor().encryptHeader(newHeader).asReadOnlyBuffer()); //to prevent NONCE reuse, we already encrypt the header and cache it
 		header.set(newHeader);
 		return newHeader;
 	}
 
-	public FileHeader loadExisting(FileChannel ch) throws IOException {
+
+	/**
+	 * Reads, decrypts and caches the file header from the given file channel.
+	 *
+	 * @param ch File channel to the encrypted file
+	 * @return {@link FileHeader} of the encrypted file
+	 * @throws IOException if the file header cannot be read or decrypted
+	 */
+	FileHeader loadExisting(FileChannel ch) throws IOException {
 		LOG.trace("Reading file header from {}", path.get());
 		ByteBuffer existingHeaderBuf = ByteBuffer.allocate(cryptor.fileHeaderCryptor().headerSize());
-		int read = ch.read(existingHeaderBuf, 0);
-		assert read == existingHeaderBuf.capacity();
+		ch.read(existingHeaderBuf, 0);
 		existingHeaderBuf.flip();
 		try {
 			FileHeader existingHeader = cryptor.fileHeaderCryptor().decryptHeader(existingHeaderBuf);
+			encryptedHeader.set(existingHeaderBuf.flip().asReadOnlyBuffer()); //for consistency
 			header.set(existingHeader);
+			isPersisted.set(true);
 			return existingHeader;
 		} catch (IllegalArgumentException | CryptoException e) {
 			throw new IOException("Unable to decrypt header of file " + path.get(), e);
 		}
 	}
 
+	public AtomicBoolean headerIsPersisted() {
+		return isPersisted;
+	}
+
 }
diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java
@@ -11,7 +11,6 @@
 import org.cryptomator.cryptofs.EffectiveOpenOptions;
 import org.cryptomator.cryptofs.ch.CleartextFileChannel;
 import org.cryptomator.cryptolib.api.Cryptor;
-import org.cryptomator.cryptolib.api.FileHeader;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -68,26 +67,18 @@ public OpenCryptoFile(FileCloseListener listener, ChunkCache chunkCache, Cryptor
 	public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, FileAttribute<?>... attrs) throws IOException {
 		Path path = currentFilePath.get();
 
-		if (options.truncateExisting()) {
-			chunkCache.invalidateAll();
-		}
-
 		FileChannel ciphertextFileChannel = null;
 		CleartextFileChannel cleartextFileChannel = null;
 		try {
 			ciphertextFileChannel = path.getFileSystem().provider().newFileChannel(path, options.createOpenOptionsForEncryptedFile(), attrs);
-			final FileHeader header;
-			final boolean isNewHeader;
-			if (ciphertextFileChannel.size() == 0l) {
-				header = headerHolder.createNew();
-				isNewHeader = true;
-			} else {
-				header = headerHolder.loadExisting(ciphertextFileChannel);
-				isNewHeader = false;
+			initFileHeader(options, ciphertextFileChannel);
+			if (options.truncateExisting()) {
+				chunkCache.invalidateAll();
+				ciphertextFileChannel.truncate(cryptor.fileHeaderCryptor().headerSize());
 			}
 			initFileSize(ciphertextFileChannel);
 			cleartextFileChannel = component.newChannelComponent() //
-					.create(ciphertextFileChannel, header, isNewHeader, options, this::channelClosed) //
+					.create(ciphertextFileChannel, options, this::channelClosed) //
 					.channel();
 		} finally {
 			if (cleartextFileChannel == null) { // i.e. something didn't work
@@ -105,6 +96,23 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil
 		return cleartextFileChannel;
 	}
 
+	//visible for testing
+	void initFileHeader(EffectiveOpenOptions options, FileChannel ciphertextFileChannel) throws IOException {
+		try {
+			headerHolder.get();
+		} catch (IllegalStateException e) {
+			//first file channel to file
+			if (options.createNew() || (options.create() && ciphertextFileChannel.size() == 0)) {
+				//file did not exist, create new header
+				//file size will never be zero again, once the header is written because we retain on truncation the header
+				headerHolder.createNew();
+			} else {
+				//file must exist, load header from file
+				headerHolder.loadExisting(ciphertextFileChannel);
+			}
+		}
+	}
+
 	private void closeQuietly(Closeable closeable) {
 		if (closeable != null) {
 			try {
@@ -116,7 +124,7 @@ private void closeQuietly(Closeable closeable) {
 	}
 
 	/**
-	 * Called by {@link #newFileChannel(EffectiveOpenOptions)} to determine the fileSize.
+	 * Called by {@link #newFileChannel(EffectiveOpenOptions, FileAttribute[])} to determine the fileSize.
 	 * <p>
 	 * Before the size is initialized (i.e. before a channel has been created), {@link #size()} must not be called.
 	 * <p>
@@ -141,7 +149,7 @@ private void initFileSize(FileChannel ciphertextFileChannel) throws IOException
 	}
 
 	/**
-	 * @return The size of the opened file. Note that the filesize is unknown until a {@link #newFileChannel(EffectiveOpenOptions) file channel is opened}. In this case this method returns an empty optional.
+	 * @return The size of the opened file. Note that the filesize is unknown until a {@link #newFileChannel(EffectiveOpenOptions, FileAttribute[])} is opened. In this case this method returns an empty optional.
 	 */
 	public Optional<Long> size() {
 		long val = fileSize.get();

diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java
@@ -8,7 +8,6 @@
  *******************************************************************************/
 package org.cryptomator.cryptofs;
 
-import com.google.common.jimfs.Configuration;
 import com.google.common.jimfs.Jimfs;
 import org.cryptomator.cryptofs.util.ByteBuffers;
 import org.cryptomator.cryptolib.api.Masterkey;
@@ -251,17 +250,17 @@ public void testWriteFromSecondChannelWhileStillOpen() throws IOException {
 			}
 		}
 
-		// tests https://github.com/cryptomator/cryptofs/issues/160
+		//tests changes made in https://github.com/cryptomator/cryptofs/pull/166
 		@Test
-		@DisplayName("TRUNCATE_EXISTING leads to new file header")
+		@DisplayName("TRUNCATE_EXISTING does not produce invalid ciphertext")
 		public void testNewFileHeaderWhenTruncateExisting() throws IOException {
 			try (var ch1 = FileChannel.open(file, CREATE_NEW, WRITE)) {
 				ch1.write(StandardCharsets.UTF_8.encode("this content will be truncated soon"), 0);
 				ch1.force(true);
-				try (var ch2 = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { // re-roll file header
+				try (var ch2 = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) {
 					ch2.write(StandardCharsets.UTF_8.encode("hello"), 0);
 				}
-				ch1.write(StandardCharsets.UTF_8.encode(" world"), 5); // should use new file key
+				ch1.write(StandardCharsets.UTF_8.encode(" world"), 5);
 			}
 
 			try (var ch3 = FileChannel.open(file, READ)) {

diff --git a/src/test/java/org/cryptomator/cryptofs/EffectiveOpenOptionsTest.java b/src/test/java/org/cryptomator/cryptofs/EffectiveOpenOptionsTest.java
@@ -294,6 +294,24 @@ public void testTruncateExisting() throws IOException {
 		MatcherAssert.assertThat(inTest.createOpenOptionsForEncryptedFile(), containsInAnyOrder(READ));
 	}
 
+	@Test
+	public void testTruncateExistingAndWrite() throws IOException {
+		EffectiveOpenOptions inTest = EffectiveOpenOptions.from(Set.of(TRUNCATE_EXISTING, WRITE), falseReadonlyFlag);
+
+		Assertions.assertFalse(inTest.append());
+		Assertions.assertFalse(inTest.create());
+		Assertions.assertFalse(inTest.createNew());
+		Assertions.assertFalse(inTest.deleteOnClose());
+		Assertions.assertFalse(inTest.noFollowLinks());
+		Assertions.assertFalse(inTest.readable());
+		Assertions.assertFalse(inTest.syncData());
+		Assertions.assertFalse(inTest.syncDataAndMetadata());
+		Assertions.assertTrue(inTest.truncateExisting());
+		Assertions.assertTrue(inTest.writable());
+
+		MatcherAssert.assertThat(inTest.createOpenOptionsForEncryptedFile(), containsInAnyOrder(READ, WRITE));
+	}
+
 	@Test
 	public void testWrite() throws IOException {
 		EffectiveOpenOptions inTest = EffectiveOpenOptions.from(Set.of(WRITE), falseReadonlyFlag);