eclipse-vertx / vertx-auth

Apache License 2.0
166 stars 156 forks source link

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

Open yanosz opened 2 years ago

yanosz commented 2 years ago

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.

pmlopes commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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.