eclipse-vertx/vertx-auth

Towards a more secure API design (for JWT at least)

yanosz opened this issue · 3 comments

I've recently started working on vert.x - however, I was surprised about the JWT-API and its documentation.

I'd suggest re-considering its design in a more usable and secure way, hence addressing usable security from an API perspective.

I'd like to suggest these improvements. These ideas resulted from working with the JWT-API for a few hours - not a review or audit.

  • Use arrays (byte arrays, char arrays) instead of passwords. Many Java APIs do so (for instance https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/security/KeyStore.html#getInstance(java.io.File,char%5B%5D) - whereas Vert.x does not (for instance: https://vertx.io/docs/apidocs/io/vertx/ext/auth/KeyStoreOptions.html#setPassword-java.lang.String-) . This is a security thing: Arrays are mutable and can be wiped out of memory, whereas Strings cannot. Hence, it's widespread practice to do so.

  • Document best-practices for using the JWT integration. This may be obvious for an experienced programmer, but dangerous for a not-so crypto savvy one.

    • Looking at https://vertx.io/docs/vertx-auth-jwt/java/ - secret key material is encoded in java source code. This is broken and insecure. Please provide a best-practice example for using this API in a secure manner - please avoid insecure examples without outlining their danger.

    • https://vertx.io/docs/vertx-web/java/#_jwt_authentication - encodes the password in plain-text. At least, it says that this is insecure. However, it would be better to provide a secure example that can be used as best practice - i.e., how would one use an authentication provider

    • Be more explicit about the algorithmic recommendations in https://vertx.io/docs/vertx-auth-jwt/java/. For instance, I do not yet see why one should generate a 2048-Bit key for a HS256 using keytool. 2048-Bit is typically used in asymmetric schemes, whereas HS256 is a symmetric one.

  • Be explicit on encodings - for instance, by try and error I noticed that HashingStrategy requires the salt to be base64-encoded, whereas Java (PBEKeySpec) requires a non-encoded array. This is dangerous. For the compiler, strings are strings. But from a crypto-perspective, the byte-entropy of a base64-encoded string (i.e. printable ascii characters) is 6-bit. In result, confusing encoded and non-encoded data could result in weak passwords, salts and keys.

@yanosz thanks for starting this discussion. The reason why you see String in KeyStoreOptions is because in vert.x option objects are annotated with @DataObject which means the compiler will generate a converter from and to JSON so the requirements are that the attributes of this simple POJO are within the JSON permitted types.

Similar rules also apply to most of vert.x public APIs, so you will not see char/byte or their array variants.

I do agree we need to improve the examples and documentation. We could also update the APIs where base64 are expected to expect a Buffer type instead of String which would already signal that there's binary data expected and we can discard it from memory, instead of being hold on the string pool.

Thanks for your reply. Regarding :

The reason why you see String in KeyStoreOptions is because in vert.x option objects [...] requirements are that the attributes of his simple POJO are within the JSON permitted types."

I do not yet see how this kind of architecture is needed for keystore-options :-) (however, that does not mean much, because I'm only starting with vert.x). Intuitively, I would expect keystore-configuration to be independent of actual data that is processed - i.e. I can hardly think of a situation in which a passphrase or even a private-key for a key-store option is transmitted with a JSON-based wire-protocol in a vert.x setting

It's not a requirement for keystore options, it's a requirement because vert.x started as a "polyglot" runtime, so in order to support other languages, say for example, javascript, ruby when we used to support nashorn and jruby we had to make a decision on limiting the allowed types to public APIs (note, internally there are no restrictions but configurations and public interfaces, follow these rules).

With vert.x 5 we may start removing these restrictions as, actually, we dropped nashorn and have a graalvm alternative that supports pretty much any JVM type.

Let me illustrate how this used to work. This is the same code for both Java and JavaScript:

JWT jwt = new JWT()
  .addJWK(
    new JWK(new PubSecKeyOptions()
      .setAlgorithm("HS256")
       // we're providing a secret as a base64 string
      .setBuffer("qnscAdgRlkIhAUPY44oiexBKtQbGY0orf7OV1I50")));

In JavaScript this used to be like this:

JWT jwt = new JWT()
  .addJWK(
    new JWK({
      algorithm: "HS256",
       // we're providing a secret as a base64 string
      buffer: "qnscAdgRlkIhAUPY44oiexBKtQbGY0orf7OV1I50"
    });

Options would "magically" be converted to/from JSON which would impair the choice of types in the API.

With graalvm, we don't have this "magic" conversion anymore, which means we can have richer types. I think we should start documenting a good refactoring for vert.x 5.0.0 where:

  1. we use Buffer where binary data is required
  2. we use char[] where passwords are to be provided

This would fix the ambiguity in some APIs where by the type String we don't know directly if a base64 string or really a char array.

This change would also require that we relax the codegen rules, which is used to perform API validation and used by all modules and downstream projects.