Skip to content
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.

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

EncryptingFileSystem #1511

Closed
mandrachek opened this issue Oct 19, 2022 · 3 comments
Closed

EncryptingFileSystem #1511

mandrachek opened this issue Oct 19, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@mandrachek
Copy link

See #245
Now that coil has it's own cache, and no longer relies on OkHttp's, theoretically, we should be able to encrypt the cache contents. Has anyone implemented this? It looks like it would require implementing an encrypted FileSystem. (I've done similar with OkHttp, but it's internal FileSystem isn't an okio.FileSystem and seem substantially easier to implement (despite the difficulties with it being internal and having to use reflection to create a Cache with one).

@mandrachek mandrachek added the enhancement New feature or request label Oct 19, 2022
@colinrtwhite
Copy link
Member

I did a quick search and couldn't find one, but I think this should support writing to/reading from files on disk:

class CipherFileSystem(
    private val cipher: Cipher,
    delegate: FileSystem = SYSTEM,
) : ForwardingFileSystem(delegate) {

    override fun sink(file: Path, mustCreate: Boolean): Sink {
        return super.sink(file, mustCreate).buffer().cipherSink(cipher)
    }

    override fun source(file: Path): Source {
        return super.source(file).buffer().cipherSource(cipher)
    }
}

You can set it on your DiskCache using DiskCache.Builder.fileSystem(CipherFileSystem(cipher)). Disclaimer: I haven't tested this.

@mandrachek
Copy link
Author

@colinrtwhite - you'd think it would be that easy, but it never is!

1 - you can't encrypt the journal. it depends on an appendingSink, and you can't really append to an encrypted file. So in the sink and source methods, you need to check for "journal" or "journal.tmp", and pass those through as-is.

2 - The IV. You really should have a separate IV per file. So instead of passing a Cipher, you should instead pass a SecretKey, and for each sink/source, you should get a new Cipher. You then have to store the iv per file.

3 - If you read and write the IV as part of source and sink calls to get your cipher, you also need to take into account that once the download is complete to a .tmp file, the file gets renamed to remove the .tmp extension. (You can override atomicMove() to also move the IV file). You also need to override the delete method to remove your iv files when an encrypted file gets removed from the cache.

I have this approach working - with logging I can see the source getting accessed, files getting written, and the sink reading the file, and image loads work just fine. Pulling files directly from the cache, they are appropriately encrypted, and the IV's are in separate files, per file.

class EncryptingFileSystem(private val key: SecretKey, delegate: FileSystem = SYSTEM) :
    ForwardingFileSystem(delegate) {

    override fun delete(path: Path, mustExist: Boolean) {
        super.delete(path, mustExist)
        deleteIv(path)
    }

    override fun atomicMove(source: Path, target: Path) {
        moveIv(source,target)
        super.atomicMove(source, target)
    }

    override fun sink(file: Path, mustCreate: Boolean): Sink {
        return if (isJournal(file)) {
            super.sink(file)
        } else {
            super.sink(file).cipherSink(getEncryptionCipher(key).also {
                writeIv(getIvFile(file), it.iv)
            })
        }
    }

    override fun source(file: Path): Source {
        return if (isJournal(file)) {
            super.source(file)
        } else {
            readIv(getIvFile(file))?.let { iv ->
                super.source(file).cipherSource(getDecryptionCipher(key, iv))
            } ?: super.source(file)
        }
    }
    
    private fun isJournal(file: Path): Boolean =
        (file.name == "journal" || file.name == "journal.tmp")
    
    // ... implement your own getIvFile, deleteIv, moveIv, writeIv, readIv, getEncryptionCipher, getDecryptionCipher

}

In an attempt to simplify things around dealing with writing a separate IV file, I tried using Jetpack Crypto, and it's EncryptedFile.openFileInput() and EncryptedFile.openFileOutput(), and converting those to Okio Source and SInk via .source() and .sink() respectively. For some reason this doesn't seem to work - no encrypted file gets written - in fact, logging indicates that JetpackEncryptingFileSystem.source() never gets called - and no file other than the journal ever gets written to disk.

This theoretically should work, but doesn't - don't know if anybody can provide any insight:

class JetpackEncryptingFileSystem(
    private val context: Context,
    delegate: FileSystem = SYSTEM
) : ForwardingFileSystem(delegate) {

    private fun isJournal(file: Path): Boolean =
        (file.name == "journal" || file.name == "journal.tmp")

    override fun sink(file: Path, mustCreate: Boolean): Sink {
        return if  (isJournal(file)) {
            delegate.sink(file)
        } else {
            getEncryptedFile(file.toFile()).openFileOutput().sink()
        }
    }

    override fun source(file: Path): Source {
        return if (isJournal(file)) {
            delegate.source(file)
        } else {
            Log.e("CRYPTO","SOURCE CALLED: $file")
            getEncryptedFile(file.toFile()).openFileInput().source()
        }
    }

    private fun getEncryptedFile(file: File): EncryptedFile {

        val masterKey: MasterKey = MasterKey.Builder(context).apply {
            setKeyGenParameterSpec(
                KeyGenParameterSpec.Builder(
                    MasterKey.DEFAULT_MASTER_KEY_ALIAS,
                    KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT
                ).apply {
                    setKeySize(MasterKey.DEFAULT_AES_GCM_MASTER_KEY_SIZE)
                    setBlockModes(KeyProperties.BLOCK_MODE_GCM)
                    setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE)
                }.build()
            )
        }.build()

        return EncryptedFile.Builder(
            context, file, masterKey, EncryptedFile.FileEncryptionScheme.AES256_GCM_HKDF_4KB
        ).build()
    }
}

@mandrachek
Copy link
Author

Ah. I "fixed" this by having sink check for the existence of the file before passing to getEncryptedFile - this lets is get downloaded and encrypted (source gets called, and I can see the file). Unfortunately "tink" doesn't support renaming files, and since it gets renamed from x.number.tmp to x.number, decryption will always fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants