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

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

Open
6 tasks
yanosz opened this issue Jun 21, 2022 · 3 comments
Open
6 tasks

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

yanosz opened this issue Jun 21, 2022 · 3 comments

Comments

@yanosz
Copy link

yanosz commented Jun 21, 2022

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.

@pmlopes
Copy link
Contributor

pmlopes commented Jun 21, 2022

@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.

@yanosz
Copy link
Author

yanosz commented Jun 21, 2022

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

@pmlopes
Copy link
Contributor

pmlopes commented Jun 21, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants