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

Secure Cell passphrase API: ObjCThemis #609

Merged
merged 5 commits into from
Mar 27, 2020
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Mar 23, 2020

Add support of Secure Cell passphrase API to ObjCThemis, powering Objective-C and Swift interfaces. The API is described in RFC 3.5 (with extensions).

User notes

Passphrase-based interface of Secure Cell allows you to use short and memorable passphrases to secure your data. While symmetric keys are more secure, they are also longer and much harder for humans to remember.

Swift API

Here is how you can use passphrases with Secure Cell in Swift:

import themis

let cell = TSCellSeal(passphrase: "secret")!

let message = "precious message".data(using: .utf8)!

let encrypted = try cell.encrypt(message)
let decrypted = try cell.decrypt(encrypted)

assert(decrypted == message)

Passphrase API accepts passphrases as relatively short strings, suitable for human memory. Master key API uses randomly generated, long binary keys, which are more suitable for machines to remember. However, they are also more efficient and generally more secure due to considerable length. You should prefer to use keys over passphrases if there are no humans involved. The interface is almost the same:

// Generate a new key if you don't have one:
let masterKey = TSGenerateSymmetricKey()
// Or use an existing value that you store somewhere:
let masterKey = Data(base64Encoded: "b0gyNlM4LTFKRDI5anFIRGJ4SmQyLGE7MXN5YWUzR2U=")!

let cell = TSCellSeal(key: masterKey)!

let message = "precious message".data(using: .utf8)!

let encrypted = try cell.encrypt(message)
let decrypted = try cell.decrypt(encrypted)

assert(decrypted == message)

Objective-C

The passphrase examples above are equally simple in Objective-C too:

#import <objcthemis/objcthemis.h>

TSCellSeal *cell = [[TSCellSeal alloc] initWithPassphrase:@"secret"];

NSData *message = [@"precious message" dataUsingEncoding:NSUTF8StringEncoding];

NSData *encrypted = [cell encrypt:message];
NSData *decrypted = [cell decrypt:encrypted];

XCTAssertTrue([decrypted isEqualToData:message]);

with master key API staying the same:

// Generate a new key if you don't have one:
NSData *masterKey = TSGenerateSymmetricKey();
// Or use an existing value that you store somewhere:
NSData *masterKey = [[NSData alloc] initWithBase64EncodedString:@"b0gyNlM4LTFKRDI5anFIRGJ4SmQyLGE7MXN5YWUzR2U="];

TSCellSeal *cell = [[TSCellSeal alloc] initWithKey:masterKey];

Technical notes

There are no significant deviations from RFC 3.5 this time.

Then only change is the addition of initWithPassphrase:usingEncoding: method based on previous experience with PyThemis and RbThemis. EDIT: which ultimately was removed because of Swift compatibility.

Unfortunately, this method cannot be made available to Swift because the automatic type translation cannot convert NSStringEncoding into String.Encoding. We cannnot use NS_REFINED_FOR_SWIFT either to correct this, so Swift users can only encode strings explicitly themselves. Since this is a compatiblity API, no significant usability issues are expected.

Note the initializers. This is not how you do class clusters in Objective-C, but unfortunately this is the only way to get a sane Swift API without actually writing Swift code. We cannot include Swift code in ObjCThemis because Swift lacks ABI stability and Xcode projects require a particular version of Swift to be set.

Swift 5 supports stable ABI but we cannot do much about this while we have to support Swift 4 (which we will probably do until it is available in Xcode).

Current implementation is usable as is but it is currently impossible for users to inherit from TSCellSealWithPassphrase. This should not be necessary, it seems, but there are legitimate cases when this might be requested (i.e., common base64-encoded encryption). We'll deal with that later based on user feedback. It is possible to export the private subclass to allow subclassing it, but initialization via TSCellSeal is still much more convenient.

Checklist

  • Change is covered by automated tests
  • Benchmark results are attached (no benches for Obj-C specifically)
  • The coding guidelines are followed
  • Public API has proper documentation
  • Example projects and code samples are up-to-date (will do after 0.13 release)
  • Changelog is updated

Add passphrase API support for Objective-C and Swift. The new private
subclass TSCellSealWithPassphrase implements encryption and decryption
with passphrases while TSCellSeal provides initializers.

This is not how you do class clusters in Objective-C, but unfortunately
this is the only way to get a sane Swift API without actually writing
Swift code. We cannot include Swift code in ObjCThemis because Swift
lacks ABI stability and Xcode projects require a particular version of
Swift to be set.

Swift 5 supports stable ABI but we cannot do much about this while we
have to support Swift 4 (which we will probably do until it is available
in Xcode).

Current implementation is usable as is but it is currently impossible
for users to inherit from TSCellSealWithPassphrase. This should not be
necessary, it seems, but there are legitimate cases when this might be
requested (i.e., common base64-encoded encryption). We'll deal with that
later based on user feedback. It is possible to export the private
subclass to allow subclassing it, but initialization via TSCellSeal is
still much more convenient.
@ilammy ilammy added W-SwiftThemis 🔶 Wrapper: SwiftThemis, Swift API W-ObjCThemis 🎨 Wrapper: ObjCThemis, Objective-C API labels Mar 23, 2020
@ilammy ilammy requested a review from vixentael March 23, 2020 17:40
@ilammy ilammy requested a review from Lagovas as a code owner March 23, 2020 17:40
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

i like PRs where there're more tests than code

// We cannot use NS_REFINED_FOR_SWIFT because we cannot ship Swift code
// in our framework without ABI stability guarantees. Sorry, Swift users,
// but you'll have to encode your passphrases explicitly for now.
NS_SWIFT_UNAVAILABLE("cannot export for Swift directly");
Copy link
Contributor

Choose a reason for hiding this comment

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

lol poor users :(

Copy link
Contributor

Choose a reason for hiding this comment

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

but won't it confuse to have this interface in objc, but not to have in swift? maybe we should remove it completely, as most users use swift anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dunno... Normally, the users would not want to use this interface anyway, even in Objective-C, and if they really need to use a different encoding, they can always use the one that takes NSData. So by removing this interface we do not remove any features, only lower usability.

On the other hand, once we are able to ship Swift code with ObjCThemis (I imagine this should be possible when Swift 4 becomes obsolete), we can forward this interface to Swift with an extension. So we can leave it in Objective-C with this in mind, if this interface is useful.

“Help me, @Lagovas. You're my only hope.” Please be a tie-breaker.

The default decisions will be to remove it as we can extend the API in a future version, but we won't be able to remove it once it's published.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that removing this interface will lead to lower usability, but I do think that having interface-that-you-can't-use leads to frustration :)

My vote is to kill it 🔥

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

kill it 🔥

Done and done.


- (nullable instancetype)initWithPassphraseData:(NSData *)passphrase
{
self = [TSCellSealWithPassphrase alloc];
Copy link
Contributor

Choose a reason for hiding this comment

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

why we do this as two lines?

self = [[TSCellSealWithPassphrase alloc] initWithPassphraseData:passphrase]];

won't it work like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uh... I forgot why. Just tested it, it seems I did this to suppress Xcode warning about the initializer not calling an init method on "self". With this code

self = [[TSCellSealWithPassphrase alloc] initWithPassphraseData:passphrase]];

the newly allocated object is not self (yet) so it triggers the warnings. Writing it like that keeps Xcode satisfied.

Should I add a comment in the code about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please put a code comment.

something deeply triggers me here, but i don't really understand why and i don't remember all the specifics of ObjC coding 🤔

{
themis_status_t res = THEMIS_FAIL;

size_t decryptedLengh = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
size_t decryptedLengh = 0;
size_t decryptedLenght = 0;

and below

Copy link
Contributor

Choose a reason for hiding this comment

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

(you see, i'm actually reading your PRs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm actually reading your PRs

Awww... uwu

patting Korbo

-decryptedLenght
+decryptedLength

XCTAssert([decryptedMessage isEqualToString:message]);
}

- (void)testDataLengthExtension
Copy link
Contributor

Choose a reason for hiding this comment

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

smart!


- (void)testKeyIncompatibility
{
// Passphrases are not keys. Keys are not passphrases.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

- (void)testEncodingDefault
{
// Passphrases are encoded in UTF-8 by default.
NSString *secret = @"暗号";
Copy link
Contributor

Choose a reason for hiding this comment

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

encryption!

let cell = TSCellSeal(passphrase: "secret")!
let message = "All your base are belong to us!".data(using: .utf8)!
let shortContext = ".".data(using: .utf8)!
let longContext = "You have no chance to survive make your time. Ha ha ha ha ...".data(using: .utf8)!
Copy link
Contributor

Choose a reason for hiding this comment

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

O_o

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes sense... in context:

Operator: Main screen turn on.

Captain: It's you !!

CATS: How are you gentlemen !!

CATS: All your base are belong to us.

CATS: You are on the way to destruction.

Captain: What you say !!

CATS: You have no chance to survive make your time.

CATS: Ha ha ha ha ....

Operator: Captain !!

Since it's not possible to bridge this method into Swift, let's drop it
completely. Replace the test with the same as in Swift which checks that
we are able to decrypt data with UTF-16BE passphrase.
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

Let’s merge this baby

@ilammy ilammy merged commit e1f14b3 into cossacklabs:master Mar 27, 2020
@ilammy ilammy deleted the kdf/obj-c branch March 27, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-ObjCThemis 🎨 Wrapper: ObjCThemis, Objective-C API W-SwiftThemis 🔶 Wrapper: SwiftThemis, Swift API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants