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

ObjCThemis: Secure Cell API update #606

Merged
merged 8 commits into from
Mar 20, 2020

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Mar 18, 2020

Update Secure Cell API for Objective-C and Swift as a preparation to passphrase support. This has been described in RFC 3.5.

This is a non-functional change, just making the API more consistent and easy to use, with a straightforward way to add passphrase API soon.

User notes

Swift

All modes now use consistent naming of encryption/decryption methods:

let encrypted = try cell.encrypt(message, context: nil)

let decrypted = try cell.decrypt(encrypted)

They accept Data as their first unnamed argument, followed an optional context: argument (except for Context Imprint mode which requires associated context). Processed data is returned as Data. Errors are reported with Swift exceptions.

Token Protect mode improvements

Token Protect mode returns a helper object with encrypted message and authentication token:

let result = try cell.encrypt(message)

print("Encrypted message:    ", result.encrypted)
print("Authentication token: ", result.token)

Encrypted message and token are accepted separately for decryption, no longer requiring to fill TSCellTokenEncryptedData manually:

let decrypted = try cell.decrypt(encrypted, token: token)

New API is bridged with Swift's Data directly, no longer requiring manual conversion from/to NSMutableData.

Objective-C

All modes now use consistent naming of encryption/decryption methods:

NSData *encrypted = [cell encrypt:message];

NSError *error;
NSData *decrypted = [cell decrypt:encrypted
                          context:nil
                            error:&error];

They accept NSData as their first argument, followed an optional context: argument (except for Context Imprint mode which requires associated context), followed by an optional error: out-parameter. Processed data is returned as NSData. Errors are reported as NSError objects, if requested.

There are convenience overloads with omitted context parameters if you don't need them, and omitted error: parameters if you're not interested in error values (nil is returned in case of error).

Token Protect mode improvements

Token Protect mode returns a helper result object with encrypted message and authentication token:

TSCellTokenEncryptedResult *result = [cell encrypt:message];

NSLog(@"Encrypted message:    %@", result.encrypted);
NSLog(@"Authentication token: %@", result.token);

Encrypted message and token are accepted separately for decryption, no longer requiring to fill TSCellTokenEncryptedData manually:

NSData *decrypted = [cell decrypt:encrypted token:token];

Note that new helper object TSCellTokenEncryptedResult represents only encryption result. It is immutable and uses NSData for fields.

Deprecated API

Old encryption/decryption API is declared deprecated and its use is discouraged. There are no plans to remove this API in the future, it will be retained for compatibility (probably until 1.0).

Complete list of deprecated API

Swift:

ModeDeprecationReplacement
TSCellSeal wrap(_:, context:)
wrap
encrypt(_:, context:)
encrypt
unwrapData(_:, context:)
unwrapData
decrypt(_:, context:)
decrypt
TSCellToken wrap(_:, context:)
wrap
encrypt(_:, context:)
encrypt
unwrapData(_:, context:)
unwrapData
decrypt(_:, token:, context:)
decrypt(_:, token:)
TSCellContextImprint wrap(_:, context:)
wrap
encrypt(_:, context:)
encrypt
unwrapData(_:, context:)
unwrapData
decrypt(_:, context:)
decrypt

Objective-C:

ModeDeprecationReplacement
TSCellSeal wrapData:context:error:
wrapData:error:
encrypt:context:error:
encrypt:error:
unwrapData:context:error:
unwrapData:error:
decrypt:context:error:
decrypt:error:
TSCellToken wrapData:context:error:
wrapData:error:
encrypt:context:error:
encrypt:error:
unwrapData:context:error:
unwrapData:error:
decrypt:token:context:error:
decrypt:token:error:
TSCellContextImprint wrapData:context:error:
wrapData:error:
encrypt:context:error:
encrypt:error:
unwrapData:context:error:
unwrapData:error:
decrypt:context:error:
decrypt:error:

Technical notes

This PR includes significant overhaul of unit test suite for ObjCThemis. It improves coverage and debuggability of ObjCThemis via tests. The test suite also includes compatibility tests.

There are a lot of tedious and repeatative changes there. All modes have roughly the same test suite but with slight differences. Objective-C and Swift test suite are identical (Swift one is a translation). However, they are important to pinpoint API details.

There were plans to improve Token Protect encryption API in Swift to return tuples:

let (encrypted, token) = try cell.encrypt(message)

print("Encrypted message:    ", encrypted)
print("Authentication token: ", token)

Unfortunately, this turned out to be impossible at the moment due to lack of stable Swift ABI.

Checklist

  • Change is covered by automated tests
  • The coding guidelines are followed
  • Public API has proper documentation
  • Example projects and code samples are up-to-date (no, will update separately)
  • Changelog is updated

P.S. I need to slow down because @vixentael capacity is extremely limited recently.

P.P.S. Why did I even started this?.. It was a perfectly fine API. Yeah, it was a bit inconvenient to use, but it did work. We all know that worse is better, right? Whatever. Here you go.

Start preparing Secure Cell to passphrase API introduction by updating
encryption/decryption API. It is not directly related to passphrases,
but since we're going to reeducate users about Secure Cell, it's a good
point to improve the API.

One annoyance with (all) Secure Cell APIs is how they are mapped into
Swift. This is somewhat asymmetrical mapping: wrap and unwrapData,
because of some Swift renaming magic. This naming scheme is also
inconsistent with every other Themis wrapper on desktop systems which
all use encrypt end decrypt.

Update the API to use "encrypt" and "decrypt" naming scheme. Also,
provide more overloads for Objective-C to omit optional arguments.
Note that we have to hide some API from Swift in order to get better
error handling conversion. We need to expose only methods with explicit
NSError out-parameters.

The old API is declated deprecated from now on. It is implemented via
the new API as a compatibility shim.

While we're here, take this chance to improve inline API docs which make
for a better IDE experience.
Context Imprint mode needs the same consistency updates as Seal mode.
However, things are much easier because associated context is not
optional in this mode.

If there is anything interesting, note that it's no longer necessary to
explicitly spell out __autoreleasing for NSError out parameters. Modern
Objective-C compilers are able to infer this annotation.
Token Protect mode is the one which gets most updates. Along with the
same API renaming as other modes, there are two important changes:

- decryption API now accepts message and token separately
- encryption API returns a new helper object

Previous decryption API accepts input via TSCellTokenEncryptedData
objects -- the same ones as returned by encryption API. While this
makes it easy to write unit tests, in real usage mesage and token
are retrieved separately. Constructing an intermediate
TSCellTokenEncryptedData object is completely unnecessary.
New API rectifies this issue.

Encryption API returns a new helper object TSCellTokenEncryptedResult
which is almost idential to TSCellTokenEncryptedData, but uses NSData
instead of NSMutableData. This allows for better Swift experience as
it's directly bridged with Data, without any NSMutableData casts.

TSCellTokenEncryptedResult also uses a different name for encrypted
message, consistent with documentation and Android API. The old name
is accessible to make it easier to port existing code that might
operate on TSCellTokenEncryptedData instances.
Overhaul Secure Cell test suite to improve coverage and debuggability.
Giant testing methods are split into multiple individual tests for each
subsystem separately.
Well, this is simply a rewrite of Objective-C test suite into Swift.
@ilammy ilammy added enhancement W-SwiftThemis 🔶 Wrapper: SwiftThemis, Swift API compatibility Backward and forward compatibility, platform interoperability issues, breaking changes W-ObjCThemis 🎨 Wrapper: ObjCThemis, Objective-C API labels Mar 18, 2020
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.

You are API freak :D

Honestly, I can't feel the value of renaming wrap/unwrap into encrypt/decrypt (as many languages follow different patterns), but in general these are positive changes.

@vixentael
Copy link
Contributor

P.P.S. Why did I even started this?.. It was a perfectly fine API.

lololol, i don't know why you started this :D
but i know, that there're other languages that use wrap/unwrap syntax.

———————-

As usual, please make sure that we have tasks for:

  • updating docs on docserver with new API
  • updating example apps with new API

@ilammy
Copy link
Collaborator Author

ilammy commented Mar 19, 2020

You are API freak :D

The RFCs were only a warning! Now I'll be renaming a method every month until you comply! /s

but i know, that there're other languages that use wrap/unwrap syntax.

Well, in fact, that was the reason and the comment is more like an old-man-shouts-at-a-cloud rant.

When I was reviewing Secure Cell API across all platforms I found that Objective-C, Java, and for some reason Go use this idiosyncratic unwrap naming while all other languages including the core library use "encrypt" and "decrypt" (the same goes for documentation).

  • With Go it's somewhat jarring, considering that everywhere across Acra we call outer methods encrypt and decrypt but deep inside them it's "wrap" and "unwrap" because of reasons.\
  • With Java we'll have to deprecate API anyway to discourage passing String as master key, and I don't want the users to see 5 deprecated methods and 1 correct one, with all of them called wrap.
  • With Swift, the name translation turns Objective-C method wrapData:error: into wrap() but unwrapData:error: into unwrapData() because IDK why. It's kinda asymmetric that users have to wrap their data but then unwrapData their data back.
  • With Objective-C (and Swift) I planned to improve Token Protect API to not require an intermediate object and reasons similar to Java it would be cleaner to add a new set of API than keep multiple deprecations in autocomplete list.

So you could say these are pretty silly OCD reasons for renaming (and reeducating the users), but hey, we're now more consistent and the old API is not going anywhere. *cranks up sunk cost fallacy up to 11*


BTW, it seems that Bitrise did not test this PRs. I'm kinda wary to merge it without that, so I'll see how to talk it into testing...

ilammy and others added 2 commits March 20, 2020 15:30
Pull in new Xcode project, maybe this will wake up Bitrise and make it
test the new changes.

Also, resolve merge conflicts in CHANGELOG to include all changes.
Clean up and update Secure Cell class documentation too. Modern Xcode
does not support HeaderDoc format anymore so keep it simple. Update
wording in some places for better style. Add links to documentation
where users can read more about Secure Cell modes.

Co-authored-by: vixentael <[email protected]>
@ilammy
Copy link
Collaborator Author

ilammy commented Mar 20, 2020

@vixentael, I've updated class description docs, cleaned up formatting there, and added the links to documentation that you suggested. Please give it a look.

It seems that modern Xcode has dropped support for HeaderDoc completely. Advanced tags like @link or @ref are not working anymore, unfortunately. Some limited subset is still there, but it seems Apple wants to send a strong message “please migrate to Swift and use Markdown there”.

Also, this push has woken up Bitrise so we'll finally see if the tests pass there with the changes.

@vixentael
Copy link
Contributor

please migrate to Swift

and enjoy failing code with every new version of Swift lol

@ilammy
Copy link
Collaborator Author

ilammy commented Mar 20, 2020

please migrate to Swift

and enjoy failing code with every new version of Swift lol

Well, if you make a slight change – Swift 5 ⟹ Swift 0.5 – then it suddenly makes a lot of sense! Give them some compassion, they've just lost a valuable number and still can't find it.

@ilammy ilammy merged commit 60c4d57 into cossacklabs:master Mar 20, 2020
@ilammy ilammy deleted the objc-scell-update branch March 20, 2020 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Backward and forward compatibility, platform interoperability issues, breaking changes enhancement 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