Update JavaThemis Secure Cell API #634
Conversation
Introduce new, interface-based API of Secure Cell. Current API has a number of issues described in RFC 3.9, notably the run-time mode setting and not-quite-secure support of String inputs which interferes with passphrase API. That's why we introduce a new interface which should rectify these issues. The users will deal with *interfaces* of the base SecureCell class: SecureCell.Seal, TokenProtect, ContextImprint. They will take into account differences in inputs and outputs of these modes. They also allow to introduce passphrase API which actually has the same API after the initialization. Using interfaces as opposed to classes allows us to hide unnecessary implementation details and do not expose the users to a number of classes that they don't actually need to interact with. This is also a nice point to start paying more attention to JavaDocs, with better descriptions of APIs and versioning.
Add implementation of SecureCell interfaces. These are package-private classes constructed by static factory methods of the base class. They provide improved API with the similar behavior. The differences include naming and the fact that encryption code path no longer uses checked SecureCellException, instead wrapping it into unchecked RuntimeException. This is because encryption code path is unlikely to fail due to business errors as opposed to programming errors, so there is no need to enforce immediate error checking with checked exception. This is not true for decryption path. Note that native methods do not throw exceptions on their own right now. Instead they indicate errors by returning "null". This would be nice to get right later.
Mark new interface methods with @NotNull and @nullable annotations where appropriate. This improves IDE experience and Kotlin integration. Contracts enable even more warnings in IDE. For example, all Secure Cell methods are pure (i.e., do not change the state of the cell itself). This allows IDE to warn about unused results of Secure Cell calls. Since we do not support Java 6 & 7, we can go with annotation library that requires Java 8+. Jetbrains have a compatbility version which supports earlier JVMs.
Port Swift test suite to Java to check the new API. There are also tests for compatibility between the new and old API. These tests have uncovered a couple of warts in Themis Core implementation. We'll deal with them later.
We require Java 8 for building with embedded Gradle wrapper, however Android still targets Java 7 by default at the moment. It is possible to build for Java 8, but that changes the requirements to JVM. We'd like to avoid cutting support unless we don't really need it. Make it explicit that we're building for Java 7 for Android, and build desktop Java code for Java 7 too. We use Java 7 features in the code so we cannot build with Java 6. That train is now gone. Also, change the annotation library to use the compatibility version which supports Java 7.
Unfortunately, we cannot use "assertThrows" which is so convenient. First of all, we can't use lambdas from Java 8. However, even if we use Runnable instead, JUnit wrapper on Android uses older JUnit which does not have "assertThrows" support in any form. Thus we fall back to ridiculously ugly, but working approach recommended by JUnit authors [1]. [1]: https://github.com/junit-team/junit4/wiki/Exception-testing Maybe when we bump the requirements to Java 8+ we could use better testing library API. But not today.
These new tests are a pinata of crashes. They have discovered a serious bug in JNI code which crashes the process on Android if a corrupted Secure Cell is decrypted. Disable these tests for now so that we can proceed with other updates. They will be reenabled later, once the bug is fixed.
ilammy
added
O-Android 🤖
It turns out that not all Android API levels support java.util.Base64 which appeared in Java 8. Since we build for Java 1.7, it's kinda wise to not rely on that API. Android has its own API for decoding base64, but we cannot use that for desktop Java. So choose neither and provide a polyfill with is API-compatible with java.util.Base64, implemented using an obscure Java API for processing XML which include base64 support as well.
Unfortunately, DatatypeConverter is not available on Android too, so we have to use an external library.
|
Oh ffs, Android! It fails on CircleCI with a specific older API level: But it's fine on my machines, obviously. These unicorns seem to include Now I'm tempted to either remove base64 tests, or just implement the decoder myself. Yes, it will be 36th base64 decoder implementation in the ecosystem. I guess all prior attempts were caused by similar frustration. Unfortunately, Java does not make conditional compilation easy. Otherwise we might have used |
(ノಠ益ಠ)ノ彡┻━┻Unfortunately, we cannot use the latest version of the codec library because some Android systems (e.g., API 24) already include an older version of some of the Commons' classes. They are loaded before our dependencies fetched by Gradle and break the party. Investigation of Android source code shows that they're using what appears to be version 1.2 of the library (the current version is 1.14). Okay, so that limits us to the API provided by 1.2. Fine *sigh*
| * @since JavaThemis 0.13 | ||
| */ | ||
| @NotNull | ||
| @Contract(value = "_ -> new", pure = true) |
There was a problem hiding this comment.
oh... java doesn't provide some named constants for pre-defined values for contracts? to avoid using hardcoded strings
There was a problem hiding this comment.
java doesn't provide some named constants for pre-defined values for contracts?
Nope, consider it source code.
| */ | ||
| @NotNull | ||
| @Contract(pure = true) | ||
| byte[] decrypt(byte[] data, byte[] token, @Nullable byte[] context) throws SecureCellException; |
There was a problem hiding this comment.
why not accept SecureCellData instead of two separate arguments with the same types? It adds new way to make mistake and pass args with incorrect order who not familiar with themis or other libraries and with common order of parameters? If we just add new alias for byte[] for token arg it will force users to think which of byte array they should convert to TokenType before decryption.
There was a problem hiding this comment.
why not accept SecureCellData instead of two separate arguments with the same types?
First of all, to avoid constructing SecureCellData on each decryption. The users typically already have two separate byte[] arrays retrieved from the storage or network so it's not necessary to fuse them into SecureCellData.
Encryption returns SecureCellData because Java does not have multiple return values or tuples, we have no choice here. However, we'll make it so that it looks like a tuple return in Kotlin at least:
val encrypted, token = cell.encrypt(message)(Not in this PR, it will be done later.)
And then, using SecureCellData does not make it much easier since its constructor also takes two byte[] arguments. If you don't know the order, there is no real difference:
byte[] decrypted = cell.decrypt(token, message)
byte[] decrypted = cell.decrypt(new SecureCellData(token, message))(Both are incorrect, of course.)
The only way to make it explicit is to use a builder or setters which is very verbose:
SecureCellData data = new SecureCellData();
data.setEncrypted(message);
data.setToken(token);
byte[] decrypted = cell.decrypt(token);However, Java devs typically use IDE which will provide contextual suggestions based on parameter names. Furthermore, the tuple return in Kotlin will also suggest the correct order. If everything fails, the developers will get an exception on the first attempt to decrypt data and then they'll learn the correct order.
So I think it's more readable to accept these arguments separately, without an intermediate SecureCellData.
| @@ -25,10 +31,10 @@ archivesBaseName = 'java-themis' | |||
| version = javaThemisVersion | |||
| group = 'com.cossacklabs' | |||
|
|
|||
| // Compile for Java 8. | |||
| // Compile for Java 7. | |||
There was a problem hiding this comment.
Java 7?
Modern Android systems support most of Java 8 features, but not quite everything, and there may be some older systems that do not support Java 8. Obviously, Java 9 and later are off limits as well (and will probably remain that way in the nearest future). So we’re effectively limited to Java 7 feature set on Android.
Since desktop Java builds from the same source code, it’s a good idea to limit it to Java 7 as well. it’s likely that given Gradle support for desktop Java, most of the development will happen there, so I don’t want to accidentally use Java 8 features and find out later that Android cannot compile that.
Though, it kinda sucks since Java 8 has so many goodies, like lambdas, default implementations in interfaces, etc. Well, we can live with that since Themis is just a simple wrapper.
| byte[] keyBytes = this.key.key; | ||
| byte[][] encrypted = {data, null}; | ||
| byte[] result = SecureCell.decrypt(keyBytes, context, encrypted, SecureCell.MODE_CONTEXT_IMPRINT); | ||
| // TODO(ilammy, 2020-05-05): teach SecureCell#decrypt to throw SecureCellException (T1605) |
| static final class Decoder { | ||
| byte[] decode(String src) { | ||
| // Modern versions can accept String directly, but Android makes it hard. | ||
| byte[] base64bytes = src.getBytes(StandardCharsets.US_ASCII); |
| public void noDetectTruncatedData() { | ||
| SecureCell.ContextImprint cell = SecureCell.ContextImprintWithKey(new SymmetricKey()); | ||
| byte[] message = "All your base are belong to us!".getBytes(StandardCharsets.UTF_8); | ||
| byte[] context = "We are CATS".getBytes(StandardCharsets.UTF_8); |
This PR makes preparatory changes for new passphrase API by updating existing API of Secure Cell in JavaThemis to more easy to use and similar to other Themis wrappers. The API is described in RFC 3.9.
User notes
SecureCellclass now provides improved Secure Cell interfaces:SecureCell.SealSecureCell.TokenProtectSecureCell.ContextImprintas well as new static factory methods for them, accepting symmetric keys:
SecureCell#SealWithKeySecureCell#TokenProtectWithKeySecureCell#ContextImprintWithKeyAll of this can be used like this in Java:
Optional associated context
All
encryptanddecryptmethods now have an overload without an optional argument for associated context. This makes the code that does not use it more concise. (Of course you can still passnullthere if you must.)Nullability annotations
New interface are properly annotated with
@NotNulland@Nullablewhich improves IDE experience and enables more static checks.SecureCellData usage
Note that with new interfaces you no longer have to construct
SecureCellDataobjects to decrypt data. Just pass yourbyte[]with encrypted data directly todecrypt()methods. In Token Protect mode you pass encrypted data and authentication token as separatebyte[]arrays too.Similarly, Seal and Context Imprint modes return encrypted
byte[]directly. You no longer have togetProtectedData()from the returnedSecureCellDataobject.Token Protect mode still returns
SecureCellDatafrom itsencrypt()method. That's the way it is with Java, which has no multiple return values. This will be improved for Kotlin a bit later.Pending deprecations
This PR does not deprecate the old API, but consider it deprecated.
SecureCellwithnew.protectandunprotectmethods.(This list will be updated when the API is actually deprecated.)
Technical notes
Full rationale may be found in RFC 3.9. Main gripes with the old API were run-time mode-setting with its impact on API, syntactic noise when using more popular Seal mode, and idiosyncratic naming of methods, inconsistent with other wrappers and our own documentation.
New API does not accept
Stringvalues. The old API encoded strings in platform-specific UTF-16 which was quite unique and not portable to other platforms. Furthermore, it suggested that strings can be used as passphrases, which is not secure in typical case.At last, in order to decrypt Sealed data, instead of writing
you can write much more readable code:
And yes, the context now comes after the data, as in all other wrappers.
Last but not least, Secure Cell gets an expanded and updated test suite (translated from whatever was done for SwiftThemis). This will definitely come handy if we are going to reimplement JavaThemis in pure JVM code some day.
Note that JNI API and ABI does not change. We decided to keep compatibility here.
Targeting Java 7
I have discovered that Android compiles its code for Java 7 by default. I wanted to use Java 8 features in tests initially, but given the Android story I think we should use Java 7 even for desktop Java code.
Note that embedded Gradle wrapper still requires Java 8 to run. However, the compiled code will run on Java 7 too.
New TODOs and FIXMEs
This PR adds a bunch of new TODOs in the code. While it's not nice, I don't want to block further development with those issues. They will be resolved later:
nullin Java code.Next tasks
Checklist
Benchmark results are attached(we have no JVM benchmarks)Example projects and code samples are up-to-date(will do later)