Support client side encryption via SEGCrypto protocol #592
Conversation
part of SEGAnalyticsConfiguration
|
Tests are here. But unfortunately in Swift, and it's out of scope to setup swift tests in master. They do pass of course :) |
|
|
||
| #import <Foundation/Foundation.h> | ||
|
|
||
| @protocol SEGCrypto <NSObject> |
There was a problem hiding this comment.
This would be public so users can supply their own implementation, yeah?
There was a problem hiding this comment.
Cool so let's move it out of the Internal folder?
|
We should definitely port the tests here either in Swift or just with what's already setup https://github.com/segmentio/analytics-ios/blob/master/Example/Tests/SEGHTTPClientTests.m. |
|
also the PR should be against the |
|
@f2prateek changed based to |
|
@tonyxiao makes sense about testing in Swift, I don't think this should be released without adding some tests so we can just write them in objective c for now. |
|
|
||
| @interface SEGUserDefaultsStorage : NSObject <SEGStorage> | ||
|
|
||
| @property (nonatomic, strong, nullable) id<SEGCrypto> crypto; |
There was a problem hiding this comment.
It seems a bit leaky that the the storage mechanism knows about encryption. Also causes fair bit of duplicated code b/w the two storage implementations to deal with crypto.
I think the encryption could happen before call the storage methods, and decryption after.
The usage could be like this:
NSData *encrypted = [crypto encrypt:data];
[storage setData:encrypted forKey:key];
NSData *encrypted = [storage dataForKey:key];
NSData *decrypted = [crypto decrypt:encrypted];
There was a problem hiding this comment.
This makes it difficult to be backward compatible with old storage mechanisms. In particular for user default based storage we don't always storage NSData objects, we directly store NSString and etc.
There was a problem hiding this comment.
Hmmm... I see. I think that's ok, the user defaults code is only for tvOS and is only been available for a single release, and it gives us a window to change some of the implementation and we should use it now rather than be stuck with a legacy implementation.
There was a problem hiding this comment.
Although I think this shouldn't be a blocker for the change.
There was a problem hiding this comment.
@f2prateek are we ok not worrying about migration on tvOS since last version?
There was a problem hiding this comment.
@tonyxiao Possibly, given support has been very recently added (just since the last release) I'm not too worried about migrating it cleanly for tvOS, if it helps us implementing this.
But as I said, doesn't need to be a blocker, should be ok as is and we can refine it later since the FileStorage class is internal.
|
Is it worth the effort to port the tests and duplicate code considering we'll actually have real Swift setup soon? Most of the current code also does not contain test coverage. |
|
I think it definitely is worth the effort of having tests for new features. It doesn't have to be a full end to end test, but at least have unit tests like https://github.com/segmentio/analytics-ios/blob/master/Example/Tests/SEGHTTPClientTests.m. We haven't had time to bring back the removed tests, but should at least keep adding tests for new components. |
|
Hmm between re-writing tests in Objc and setting up tests in swift I'd rather just set up tests in Swift so no effort is wasted. Maybe I should get that in first or part of this PR. |
|
I don't think it should be that much work to port the tests, I'm happy to do it myself once this branch is ready (it might already be) |
|
It's all good. I'll take a stab at getting the Swift tests running in this branch, shouldn't to too bad. (After all, we have it already elsewhere) |
|
@f2prateek all comments addressed. |
| // Convenient shorthand. Will randomly generate salt and iv. | ||
| - (instancetype _Nonnull)initWithPassword:(NSString * _Nonnull)password; | ||
|
|
||
| + (NSData * _Nonnull)randomDataOfLength:(size_t)length; |
There was a problem hiding this comment.
Does this need to be public?
There was a problem hiding this comment.
Yes, it's a convenience method for people to generate salt and iv and possibly password.
|
LGTM |
* Adding SEGStorage and SEGCrypto classes * Adding SEGUserDefaultsStorage * Route all disk access through SEGStorage classes and expose SEGCrypto as part of SEGAnalyticsConfiguration * Run pod install again * Fix anonymousId loading. Removing no longer used anonymousIdURL * Removing unused SEGAnalyticsURLForFilename * Adding CryptoTest, FileStorageTest and UserDefaultsStorageTest from redux branch * Adding changes to Podfile
https://segment.atlassian.net/browse/INT-587
Architecture here: https://paper.dropbox.com/doc/Pluggable-Client-Side-Encryption-Architecture-Review-z7spaYemPF4IU5vVlppQL
In order to make this happen we needed to route all disk access through
SEGStorage protocol. There are two concrete implementations, one for file based
and another for user defaults based, depending on OS.
cc @f2prateek @jgershen