Skip to content

SQLCipher support #1162

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

Closed
pgawlowski wants to merge 2 commits into from
Closed

SQLCipher support #1162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c78ce6
SQLCipher support
Apr 17, 2020
c869e0a
Update Apollo.podspec
pgawlowski Apr 17, 2020
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
8 changes: 8 additions & 0 deletions Apollo.podspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,14 @@ Pod::Spec.new do |s|
ss.dependency 'Apollo/Core'
ss.dependency 'SQLite.swift', '~>0.12.2'
end

# Cache persistance storage mechanism extended with encryption mechanism
s.subspec 'SQLiteCipher' do |ss|
ss.source_files = 'Sources/ApolloSQLite/*.swift'
ss.dependency 'Apollo/Core'
ss.dependency 'SQLCipher', '~>4.2.0'
ss.dependency 'SQLite.swift/SQLCipher', '> 0.12.0'
end
Copy link
Copy Markdown
Contributor

@designatednerd designatednerd Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not actually using anything from SQLite cipher inside our library, so I don't think this is necessary (and would add a couple of dependencies that could cause conflicts.

Copy link
Copy Markdown
Author

@pgawlowski pgawlowski Apr 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem is - it seems to be impossible to use it together with Apollo/SQLite.
SQLite.swift/SQLCipher - requires only this particular SQLite dependency.
Mixing it together withss.dependency 'SQLite.swift', '~>0.12.2' results in not working SQLCipher layer. It is not going to cause any error, crash or compilation warning. Just DB is not going to encrypt at all.

That is why I prepared separate dependency.
Please check this comment and the answer:
stephencelis/SQLite.swift#570 (comment)
This is the exact situation.

I was already trying to use
pod Apollo/SQLite
'pod 'SQLCipher', '~>4.2.0'
'pod 'SQLite.swift/SQLCipher', '> 0.12.0'

together with this modified init

    self.shouldVacuumOnClear = shouldVacuumOnClear
    self.db = db
    try self.createTableIfNeeded()
  }

and DB is not encrypting.

@edit:
To avoid this dependency we would have to introduce SQLitenormalizedCache ... without SQLite in apollo dependencies to provide it from the outside with

pod 'SQLCipher', '~>4.2.0'
pod 'SQLite.swift/SQLCipher', '> 0.12.0'

Any thoughts how to resolve?

Copy link
Copy Markdown
Contributor

@designatednerd designatednerd Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, looking at the SQLitePodspec it looks like there's some additional xcconfigs that get set up in order to make that work. Looking at the README for FMDB, another iOS SQLite lib, I see a similar caveat about how to set up the podspec.

Looking at the tutorial on SQLCipher, there's a fairly blunt caveat about only using their SQLite 3:

Screen Shot 2020-04-17 at 3 27 05 PM

Neither SQLite.swift nor FMDB support using this through anything other than CocoaPods, which is a big old red flag for me in terms of maintaining support on the Apollo side, which would be necessary if we were to add this to the library.

Honestly, at this point, it feels like we're pretzeling ourselves to make this work in a way that just doesn't make sense for the overwhelming majority of users. This is a major reason why NormalizedCache is a protocol: so that you can implement your own version of it if the one we provide is insufficient.


# Websocket and subscription support based on Starscream
s.subspec 'WebSocket' do |ss|
Expand Down
6 changes: 6 additions & 0 deletions Sources/ApolloSQLite/SQLiteNormalizedCache.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ public final class SQLiteNormalizedCache {
try self.createTableIfNeeded()
}

public init(db: Connection, shouldVacuumOnClear: Bool = false) throws {
self.shouldVacuumOnClear = shouldVacuumOnClear
self.db = db
try self.createTableIfNeeded()
}
Copy link
Copy Markdown
Contributor

@designatednerd designatednerd Apr 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a reasonable addition in terms of allowing people to use SQLCipher or other extensions to SQLite.swift, though, so I do think we should keep this. Can you add some documentation to this initializer please?


private func recordCacheKey(forFieldCacheKey fieldCacheKey: CacheKey) -> CacheKey {
let components = fieldCacheKey.components(separatedBy: ".")
var updatedComponents = [String]()
Expand Down
⚡