Skip to content

Commit

Permalink
Make some JavaThemis exceptions unchecked (cossacklabs#563)
Browse files Browse the repository at this point in the history
* Make some JavaThemis exceptions unchecked

Checked exceptions are commonly considered a design mistake in Java
standard library [1]. They have some limited uses, but generally they
do not provide safe and ergonomic interface for the users because they
are often misunderstood. Is is common to use checked exceptions in
situations where unchecked exceptions are more approriate. This leads
to exceptions that cannot be handled by the application and it is really
common to ignore ("fix") them by adding "throws Exception" specifier
because you can't be bothered to enumerate a dozen of concrete classes
of internal library errors. Either way it is not better than using
unchecked exceptions in the first place.

Unchecked exceptions are intended for system faults and programming
errors which are generally not recoverable by the application [2].
For example, using a method incorrectly, not checking for "null" when
it should be checked for, providing invalid argument values, etc.
These are generally not under contol of the application, in the sense
that the user cannot do something to avoid this failure other than
fixing application logic (or, well, circumenting the faulty code path).

Checked exceptions are intended to be used for recoverable errors [2].
That is, something that the application can control and influence.
For example, invalid user input can cause a checked exception because
this is an expected situation (the user can make mistakes and this is
not something that application can generally prevent in code). However,
the application can control the user and instruct them to give correct
input, or at least tell the user that theinput is incorrect. That is,
the application is expected to always handle checked exception in some
way, and there should be a catch clause somewhere in the middle, not
only on the top level for possible run-time exceptions.

[1]: http://literatejava.com/exceptions/checked-exceptions-javas-biggest-mistake/
[2]: https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html

With this in mind, convert the following com.cossacklabs.themis
exceptions into runtime, unchecked variants:

- NullArgumentException - it is used to indicate that the programmer
  passed a null value where it should not be passed, and this will
  cause a NullPointerException later. In fact, NullArgumentException
  is now a subclass of NullPointerException as it indicates misuse
  of null values.

- InvalidArgumentException - it is used to indicate that the programmer
  has passed an invalid argument to the method: an unknown constant,
  an empty buffer that must not be empty, or some other violation of
  the method contract. It is a subclass of IllegalArgumentException now.

- KeyGenerationException - it is used to indicate internal failure in
  Themis key generation routines. This is not something under user
  control and key generation does not have any input that can change
  its behavior. This error can be caused either by a bug in Themis Core
  or by a misconfigured cryptographic backed. Both situations cannot be
  recovered by the application.

Note, however, that SecureCellException, SecureMessageWrapException,
SecureSessionException, and SecureCompareException are still checked.
These exceptions are used to indicate issues with user-provided data:
key material, data messages, etc. These are expected failures and should
be handled by the application somehow.

* Throw RuntimeException on UTF-16 conversion errors

Convert UnsupportedEncodingException into RuntimeException in interfaces
that expect Strings and unconditionally serialize them as UTF-16 bytes.
If the implementation does not support UTF-16 -- which is highly
unlikely, given that Java pioneered Unicode and UTF-16 specifically --
then the user application cannot do anything about it (other that
rewrite JavaThemis code). This is not an error with user input. Themis
does not give the user an option to select encoding and always uses
UTF-16, so the user application cannot recover from this error.

* Remove "throws" specs for unchecked exceptions

After these adjustments, update the "throws" clauses in Themis code and
remove all unchecked exceptions from them. This allows user applications
to not handle these exceptions immediately. (Though they can still leave
"throws" specifications in their code if they wish to. Our tests do,
for example, because I can't be bothered to update them without doing
a complete rewrite for readability.)

* Wrap some checked exceptions into runtime exceptions

Drop more "throws" specs from Secure Session and Secure Comparator
interfaces by wrapping some of the exceptions into appropriate runtime
exceptions.

Secure Session and Secure Comparator throw checked exceptions in many
places where user input cannot cause failures. For example, the user
cannot recover from a failure to create Secure Session object. These
errors should cause unchecked, runtime exceptions.

However, in some places checked exceptions are warranted: such as when
wrapping and unwrapping Secure Session messages, or when Secure
Comparator protocol exchange is processed. Here we leave checked
exceptions and indicate them with "throws" specs.

* Avoid throwing UnsupportedEncodingException

Use a different overload of String.getBytes() that does not involve
charset lookup (and possible "unknown encoding" error).
  • Loading branch information
ilammy committed Dec 6, 2019
1 parent 06d27e6 commit 5f886e1
Show file tree
Hide file tree
Showing 10 changed files with 132 additions and 113 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ _Code:_
- JDK location is now detected automatically in most cases, you should not need to set JAVA_HOME or JDK_INCLUDE_PATH manually ([#551](https://github.com/cossacklabs/themis/pull/551)).
- JNI libraries are now available as `libthemis-jni` packages for supported Linux systems ([#552](https://github.com/cossacklabs/themis/pull/552), [#553](https://github.com/cossacklabs/themis/pull/553)).
- Fixed a NullPointerException bug in `SecureSocket` initialisation ([#557](https://github.com/cossacklabs/themis/pull/557)).
- Some Themis exceptions have been converted from checked `Exception` to _unchecked_ `RuntimeException`, relaxing requirements for `throws` specifiers ([#563](https://github.com/cossacklabs/themis/pull/563)).

- **Python**

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package com.cossacklabs.themis;

public class InvalidArgumentException extends Exception {
public class InvalidArgumentException extends IllegalArgumentException {

public InvalidArgumentException(String message) {
super("Invalid argument: " + message);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@

package com.cossacklabs.themis;

public class KeyGenerationException extends Exception {
public class KeyGenerationException extends RuntimeException {

}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ private KeypairGenerator() {
* @return new EC Keypair
* @throws KeyGenerationException when cannot generate a keypair
*/
public static Keypair generateKeypair() throws KeyGenerationException {
public static Keypair generateKeypair() {
try {
return generateKeypair(AsymmetricKey.KEYTYPE_EC);
} catch (InvalidArgumentException e) {
Expand All @@ -51,8 +51,8 @@ public static Keypair generateKeypair() throws KeyGenerationException {
* @throws KeyGenerationException when cannot generate a keypair
* @throws InvalidArgumentException when keyType is invalid
*/
public static Keypair generateKeypair(int keyType) throws KeyGenerationException, InvalidArgumentException {
public static Keypair generateKeypair(int keyType) {

if ((keyType != AsymmetricKey.KEYTYPE_EC) && (keyType != AsymmetricKey.KEYTYPE_RSA)) {
throw new InvalidArgumentException("keyType is invalid");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

package com.cossacklabs.themis;

public class NullArgumentException extends Exception {
public class NullArgumentException extends NullPointerException {

public NullArgumentException() {
super();
}
Expand Down
49 changes: 22 additions & 27 deletions src/wrappers/themis/java/com/cossacklabs/themis/SecureCell.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@

package com.cossacklabs.themis;

import java.io.UnsupportedEncodingException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;

/**
* Themis secure cell
Expand All @@ -32,8 +33,8 @@ public class SecureCell {
* @param SecureCell mode
* @throws InvalidArgumentException when unsupported mode is specified
*/
public SecureCell(int mode) throws InvalidArgumentException {
public SecureCell(int mode) {

if (mode < MODE_SEAL || mode > MODE_CONTEXT_IMPRINT) {
throw new InvalidArgumentException("invalid mode");
}
Expand All @@ -55,46 +56,44 @@ public SecureCell(byte[] key) {
* @param SecureCell mode
* @throws InvalidArgumentException when unsupported mode is specified
*/
public SecureCell(byte[] key, int mode) throws InvalidArgumentException {
public SecureCell(byte[] key, int mode) {
this(mode);
this.key = key;
}

/**
* Creates new SecureCell with default master password in SEAL mode
* @param default master password
* @throws UnsupportedEncodingException when UTF-16 decoding is not supported
*/
public SecureCell(String password) throws UnsupportedEncodingException {
public SecureCell(String password) {
this(password.getBytes(CHARSET));
}

/**
* Creates new SecureCell with default master password in specified mode
* @param default master password
* @param SecureCell mode
* @throws UnsupportedEncodingException when UTF-16 decoding is not supported
* @throws InvalidArgumentException when unsupported mode is specified
*/
public SecureCell(String password, int mode) throws UnsupportedEncodingException, InvalidArgumentException {
public SecureCell(String password, int mode) {
this(mode);
this.key = password.getBytes(CHARSET);
}

int mode = MODE_SEAL;
byte[] key;

static final String CHARSET = "UTF-16";
static final Charset CHARSET = StandardCharsets.UTF_16;

public static final int MODE_SEAL = 0;
public static final int MODE_TOKEN_PROTECT = 1;
public static final int MODE_CONTEXT_IMPRINT = 2;

static native byte[][] encrypt(byte[] key, byte[] context, byte[] data, int mode);
static native byte[] decrypt(byte[] key, byte[] context, byte[][] protectedData, int mode);
static SecureCellData protect(byte[] key, byte[] context, byte[] data, int mode) throws NullArgumentException, SecureCellException {

static SecureCellData protect(byte[] key, byte[] context, byte[] data, int mode) throws SecureCellException {

if (null == key) {
throw new NullArgumentException("Master key was not provided");
}
Expand All @@ -118,9 +117,9 @@ static SecureCellData protect(byte[] key, byte[] context, byte[] data, int mode)

return new SecureCellData(protectedData[0], protectedData[1]);
}
static byte[] unprotect(byte[] key, byte[] context, SecureCellData protectedData, int mode) throws NullArgumentException, SecureCellException, InvalidArgumentException {

static byte[] unprotect(byte[] key, byte[] context, SecureCellData protectedData, int mode) throws SecureCellException {

if (null == key) {
throw new NullArgumentException("Master key was not provided");
}
Expand Down Expand Up @@ -163,7 +162,7 @@ static byte[] unprotect(byte[] key, byte[] context, SecureCellData protectedData
* @throws NullArgumentException when key or data is null
* @throws SecureCellException when cannot protect the data
*/
public SecureCellData protect(byte[] key, byte[] context, byte[] data) throws NullArgumentException, SecureCellException {
public SecureCellData protect(byte[] key, byte[] context, byte[] data) throws SecureCellException {
return protect(key, context, data, this.mode);
}

Expand All @@ -175,7 +174,7 @@ public SecureCellData protect(byte[] key, byte[] context, byte[] data) throws Nu
* @throws NullArgumentException when default master key or data is null
* @throws SecureCellException when cannot protect the data
*/
public SecureCellData protect(byte[] context, byte[] data) throws NullArgumentException, SecureCellException {
public SecureCellData protect(byte[] context, byte[] data) throws SecureCellException {
return this.protect(this.key, context, data);
}

Expand All @@ -187,9 +186,8 @@ public SecureCellData protect(byte[] context, byte[] data) throws NullArgumentEx
* @return SecureCellData with protected data
* @throws NullArgumentException when key or data is null
* @throws SecureCellException when cannot protect the data
* @throws UnsupportedEncodingException when UTF-16 decoding is not supported
*/
public SecureCellData protect(String password, String context, byte[] data) throws UnsupportedEncodingException, NullArgumentException, SecureCellException {
public SecureCellData protect(String password, String context, byte[] data) throws SecureCellException {
return this.protect(password.getBytes(CHARSET), context.getBytes(CHARSET), data);
}

Expand All @@ -200,9 +198,8 @@ public SecureCellData protect(String password, String context, byte[] data) thro
* @return SecureCellData with protected data
* @throws NullArgumentException when key or data is null
* @throws SecureCellException when cannot protect the data
* @throws UnsupportedEncodingException when UTF-16 decoding is not supported
*/
public SecureCellData protect(String context, byte[] data) throws UnsupportedEncodingException, NullArgumentException, SecureCellException {
public SecureCellData protect(String context, byte[] data) throws SecureCellException {
return this.protect(this.key, context.getBytes(CHARSET), data);
}

Expand All @@ -216,7 +213,7 @@ public SecureCellData protect(String context, byte[] data) throws UnsupportedEnc
* @throws SecureCellException when cannot decrypt protectedData
* @throws InvalidArgumentException when protectedData is incorrect
*/
public byte[] unprotect(byte[] key, byte[] context, SecureCellData protectedData) throws NullArgumentException, SecureCellException, InvalidArgumentException {
public byte[] unprotect(byte[] key, byte[] context, SecureCellData protectedData) throws SecureCellException {
return unprotect(key, context, protectedData, this.mode);
}

Expand All @@ -229,7 +226,7 @@ public byte[] unprotect(byte[] key, byte[] context, SecureCellData protectedData
* @throws SecureCellException when cannot decrypt protectedData
* @throws InvalidArgumentException when protectedData is incorrect
*/
public byte[] unprotect(byte[] context, SecureCellData protectedData) throws NullArgumentException, SecureCellException, InvalidArgumentException {
public byte[] unprotect(byte[] context, SecureCellData protectedData) throws SecureCellException {
return this.unprotect(this.key, context, protectedData);
}

Expand All @@ -242,9 +239,8 @@ public byte[] unprotect(byte[] context, SecureCellData protectedData) throws Nul
* @throws NullArgumentException when key or protectedData is null
* @throws SecureCellException when cannot decrypt protectedData
* @throws InvalidArgumentException when protectedData is incorrect
* @throws UnsupportedEncodingException when UTF-16 decoding is not supported
*/
public byte[] unprotect(String password, String context, SecureCellData protectedData) throws UnsupportedEncodingException, NullArgumentException, SecureCellException, InvalidArgumentException {
public byte[] unprotect(String password, String context, SecureCellData protectedData) throws SecureCellException {
return this.unprotect(password.getBytes(CHARSET), context.getBytes(CHARSET), protectedData);
}

Expand All @@ -256,9 +252,8 @@ public byte[] unprotect(String password, String context, SecureCellData protecte
* @throws NullArgumentException when key or protectedData is null
* @throws SecureCellException when cannot decrypt protectedData
* @throws InvalidArgumentException when protectedData is incorrect
* @throws UnsupportedEncodingException when UTF-16 decoding is not supported
*/
public byte[] unprotect(String context, SecureCellData protectedData) throws UnsupportedEncodingException, NullArgumentException, SecureCellException, InvalidArgumentException {
public byte[] unprotect(String context, SecureCellData protectedData) throws SecureCellException {
return this.unprotect(this.key, context.getBytes(CHARSET), protectedData);
}
}
54 changes: 31 additions & 23 deletions src/wrappers/themis/java/com/cossacklabs/themis/SecureCompare.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.cossacklabs.themis;

import java.io.UnsupportedEncodingException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;

public class SecureCompare {

Expand All @@ -19,30 +21,30 @@ public enum ProtocolResult {
SEND_TO_PEER
}

static final String CHARSET = "UTF-16";
static final Charset CHARSET = StandardCharsets.UTF_16;

private long nativeCtx = 0;

native long create();
native void destroy();
public SecureCompare() throws SecureCompareException {

public SecureCompare() {

nativeCtx = create();

if (0 == nativeCtx) {
throw new SecureCompareException();
throw new RuntimeException("failed to create Secure Comparator", new SecureCompareException());
}
}
public SecureCompare(byte[] secret) throws SecureCompareException {

public SecureCompare(byte[] secret) {

this();
appendSecret(secret);

}
public SecureCompare(String password) throws UnsupportedEncodingException, SecureCompareException {

public SecureCompare(String password) {
this(password.getBytes(CHARSET));
}

Expand All @@ -58,49 +60,55 @@ protected void finalize() {
}

native int jniAppend(byte[] secret);

public void appendSecret(byte[] secretData) throws SecureCompareException {

public void appendSecret(byte[] secretData) {
if (secretData == null) {
throw new NullArgumentException("secret cannot be null");
}
if (secretData.length == 0) {
throw new InvalidArgumentException("secret cannot be empty");
}
if (0 != jniAppend(secretData)) {
throw new SecureCompareException();
throw new RuntimeException("failed to append secret data", new SecureCompareException());
}
}

native byte[] jniBegin();
CompareResult parseResult(int result) throws SecureCompareException {

CompareResult parseResult(int result) {
if (result == scompareNotReady()) {
return CompareResult.NOT_READY;
}else if (result == scompareNoMatch()){
return CompareResult.NO_MATCH;
}else if (result == scompareMatch()){
return CompareResult.MATCH;
}
throw new SecureCompareException();
throw new RuntimeException("unexpected comparison result: " + result, new SecureCompareException());
}
public byte[] begin() throws SecureCompareException {

public byte[] begin() {
byte[] compareData = jniBegin();

if (null == compareData) {
throw new SecureCompareException();
throw new RuntimeException("failed to begin comparison", new SecureCompareException());
}

return compareData;
}

native byte[] jniProceed(byte[] compareData);

public byte[] proceed(byte[] compareData) throws SecureCompareException {
return jniProceed(compareData);
}

native int jniGetResult();
public CompareResult getResult() throws SecureCompareException {

public CompareResult getResult() {
return parseResult(jniGetResult());
}

native int scompareMatch();
native int scompareNoMatch();
native int scompareNotReady();
}
}
Loading

0 comments on commit 5f886e1

Please sign in to comment.