OpenAPI: implement oidc discovery support

[pmlopes]

Currently, there are a couple of limitations known to openapi module.

It is not possible to combine oauth2 config with oidc discovery
Scopes required per route are not checked.

This issue is a tracking issue to address this issues and more.

This is a follow up to: https://github.com/vert-x3/vertx-auth/issues/450

[thced]

I have a bit of a struggle to understand how I could be able to add a SockJSHandler (really, the bridge), when using the Vert.x v4 RouterBuilder from vertx-web-openapi module. The SockJSHandler is "deprecated" and I am not allowed to call Router::handleContext on the routerBuilder's operation. Are there any thoughts about adding the possibility to add a Router to an operation, or some other means of supporting this scenario?
In my scenario we have one endpoint which we would like to be covered under OpenAPI to be able to specify and document the events we send. This endpoint is also covered by our oauth handler (needs to be disabled, since sockjs does not allow us to send Authorization header, and the oauth handler does not take a query param into account anyway).

[slinkydeveloper]

@thced more than adding a Router, you can add more than one handler to each operation.

[slinkydeveloper]

Is there any action to take here @pmlopes ?

[thced]

@thced more than adding a Router, you can add more than one handler to each operation.

@slinkydeveloper Yes, I understand the handler chaining, but the operation does not take a Router instance. In essence, it does not allow you to mount a subRouter to an operation.
This might be all fine, and my use-case might be inappropriate.
We ended up mounting a handler to the operation, that returns a Link header instead, with a generated URI outside OpenAPI (this is a one-shot URI), which is used to initiate the SockJS communication from the client.
Not ideal, but I think we will manage.. would have been nice to be able to just have an option to mount the SockJS bridge directly on the operation, and be able to go through OAuth2 (assuming we are authenticated) and opening up the socket. Maybe that is not possible.. :)

[pmlopes]

@slinkydeveloper I think we need an overload to allow sub routers like we allow handlers, the tricky part is getting the order right and see if the subrouter doesn't shadow the remaining of the router.

Alternatively we may need to define a specific sockjs support .

[pmlopes]

Initial proposal to address the security limitations:

/cc @slinkydeveloper @vietj @photomorre

RouteBuilder::createRouter()

This method should become asynchronous. Currently the method assumes that the user handlers and missing security handlers are created in a blocking fashion. For OIDC (security) this isn't true, for example, we need to execute the discovery function that will do a network download to configure itself.

RouteBuilder::securityHandlerFactory(...)

<T> RouterBuilder securityHandlerFactory(
    String securitySchemaName,
    Function<T, Future<AuthenticationHandler>> securityHandlerFactory);

With this in mind we should create a auth factory that is asynchronous and returns a Future

The securityHandlerFactory should be a simple interface:

(OAuth2Options securitySchemaOptions) -> Future.succeeded(result);

This factory should be called for all types: oauth2 and openid. The options object should be initialized with the schema yaml config data from components.securitySchemes.

It shall not be the responsibility of the builder to call the discovery asynchronous method.

Reasons:

1. OpenAPI explicitly does not define clientid, client-secret, etc... so the user will have to provide those
2. The flow may be explicitly defined by the user for example OBO.

In the end such a usage would look like:

RouterBuilder.create(vertx, yaml_location)
  .onSuccess(routerBuilder -> {

    routerBuilder.setOptions(FACTORY_OPTIONS);

    routerBuilder.operation("candy")
      .handler(rc -> rc.end("Chocolate"));

    // here the new API
    routerBuilder.securityHandlerFactory(
      // id:
      "oauth2",
      // factory:
      (oauth2options) -> {
        return OpenIDConnectAuth.discover(
          vertx,
          // enhance the options if needed
          oauth2options
          // Azure OpenId does not return the same url where the request was sent to
          .setValidateIssuer(false)
          .setSite(site)
          .setJWTOptions(new JWTOptions()
            .setNonceAlgorithm("SHA-256")
            .addAudience(config.getClientID()))
          .setExtraParameters(extraParameters))
      });
      
  // since the createRouter() is not async because it may call the factory above
  // the usage will become:
  
  routerBuilder.createRouter()
    .onSuccess(router -> {
      // use the generated router now!
      ...      

We will need to issue a deprecation to the blocking createRouter() ASAP to inform that we will replace it with the async version. So we may need to discuss with the upstream to see how we should address this with the minimum impact.

We may avoid having issues with the current authenticationHandler() as a breaking change by deprecating and replacing with:

/**
 * @deprecated use {@link #securityHandlerFactory} instead.
 */
@deprecated
default RouterBuilder securityHandler(String securitySchemaName, AuthenticationHandler handler) {
  return securityHandlerFactory(securitySchemaName, options -> Future.succeededFuture(handler));
}

[slinkydeveloper]

This method should become asynchronous. Currently the method assumes that the user handlers and missing security handlers are created in a blocking fashion. For OIDC (security) this isn't true, for example, we need to execute the discovery function that will do a network download to configure itself.

That is a huge breaking change 😄 Can this be done before the user starts dealing with openapi at all? Can we move this somehow to create where the openapi loader does some async things to load the contract? I could just go through the whole contract and figure out the required parameters...

Also can you show a sample contract for that?

It shall not be the responsibility of the builder to call the discovery asynchronous method.

I guess that answers my above proposal... Can we give flow and parameters in the create options? Although it doesn't sound correct abstraction wise

[pmlopes]

Following offsite discussion to avoid breaking the API we may achieve the same result by adding an extra static factory:

RouterBuilder.create(
  vertx,
  "charlie_chocolate_factory.yaml",
  loaderOptions,
  // here a new argument to provide AuthenticationHandler's
  (securitySchemaName, oauth2options) -> {
    switch(securitySchemaName) {
    case "openid":
    case "oauth2":
        return OpenIDConnectAuth.discover(
          vertx,
          // enhance the options if needed
          oauth2options
          // Azure OpenId does not return the same url where the request was sent to
          .setValidateIssuer(false)
          .setSite(site)
          .setJWTOptions(new JWTOptions()
            .setNonceAlgorithm("SHA-256")
            .addAudience(config.getClientID()))
          .setExtraParameters(extraParameters));
      default:
        return null;
    }
  }).onSuccess(routerBuilder -> {

    routerBuilder.setOptions(FACTORY_OPTIONS);

    routerBuilder.operation("candy")
      .handler(rc -> rc.end("Chocolate"));

  Router result = routerBuilder.createRouter();

Now the switch does feel unnatural here, so we could introduce a new intermedia type to "provide" the AuthenticationHandler:

@VertxGen
public interface AuthenticationHandlerProvider {
  @Fluent
  <T> AuthenticationHandlerProvider add(
    String securitySchemaName,
    Function<T, Future<AuthenticationHandler>> securityHandlerFactory);
}

Which would be usable as:

RouterBuilder.create(
  vertx,
  "charlie_chocolate_factory.yaml",
  loaderOptions,
  // here a new argument to provide AuthenticationHandler's
  AuthenticationHandlerProvider.create()
    .add("openid", oauth2options ->
        OpenIDConnectAuth.discover(
          vertx,
          // enhance the options if needed
          oauth2options
          // Azure OpenId does not return the same url where the request was sent to
          .setValidateIssuer(false)
          .setSite(site)
          .setJWTOptions(new JWTOptions()
            .setNonceAlgorithm("SHA-256")
            .addAudience(config.getClientID()))
          .setExtraParameters(extraParameters)))
  
).onSuccess(routerBuilder -> {

    routerBuilder.setOptions(FACTORY_OPTIONS);

    routerBuilder.operation("candy")
      .handler(rc -> rc.end("Chocolate"));

  Router result = routerBuilder.createRouter();

This option would allow adding an arbitrary number of providers, avoiding the switch statement. Making it probably more readable and extractable to a external variable if the list is long, etc...

[pmlopes]

Renamed the issue to reflect the discussion in the comments.

This issue shall track the implementation of oidc support to openapi. When this value is present, it shall be used to configure the oauth2 handler

[pmlopes]

@thced I've repurposed this issue to track OpenID Connect support, perhaps we should create a different one for sockjs bridge.

Maybe we could define a template openapi document with all the sockjs endpoints? and have some way to mount a sub router instead of a handler? However mounting a subrouter is tricky because it can shade the generated router so I'm inclined to say we need to carefully see how to handle this.

[pmlopes]

I've been reviewing the OpenAPI module regarding security, currently we lack support for openIdConnect. After reading the spec and going over the forums and support ticket, I think I understand the way it is supposed to work.

When a API document defines:

components:
  securitySchemes:
    openId:
      type: openIdConnect
      openIdConnectUrl: https://example.com/.well-known/openid-configuration

The router generator should perform a "discovery" call to configure the OAuth2Handler. There are a few things missing here. In order to perform such call we still need at least 2 configuration values:

1. client-id
2. client-secret

It is assumed that openIdConnect works on authentication_code or implicit flow. The spec ignores this because for server side applications, only authentication_code should be supported. However, users may need to define other flows, for example on-behalf-of. This means that during the creating of the OpenAPI object we need to have some sort of asynchronous supplier of the handler.

RouterBuilder.create(
  vertx,
  "some_api.yaml",
  loaderOptions,
  // here a new argument to provide AuthenticationProviders's
  AuthenticationProviderFactory.create()
    .add("openid", config ->
        OpenIDConnectAuth.discover(
          vertx,
          // enhance the config if needed
          // received config: {openIdConnectUrl: "https://example.com"}
          new Oauth2Options()
            .setSite(config.getString("openIdConnectUrl"))

This ensures that the creation of AuthenticationProviders will work. However when the router is generated, we need to have a handler, luckily the handler creation is a synchronous process so we can quickly wrap it as:

OAuth2Handler.create(authProvider, "http://localhost/callback")

It is interesting to see that the handler requires an optional second argument which isn't defined anywhere. So we may need to change the original idea to create an asynchronous Oauth2Handler instead.

RouterBuilder.create(
  vertx,
  "some_api.yaml",
  loaderOptions,
  // here a new argument to provide AuthenticationHandler's
  AuthenticationHandlerFactory.create()
    .add("openid", config ->
        OpenIDConnectAuth.discover(
          vertx,
          // enhance the config if needed
          new Oauth2Options()
            .setSite(config.getString("openIdConnectUrl"))
          .compose(provider -> Future.succeeded(Oauth2Handler.create(provider, "http://localhost/callback")

We could try to get the server value from the document server's array so the config would be:

servers:
  - url: http://localhost

Then the config json passed would contain 2 values:

{openIdConnectUrl: "https://example.com", servers: [{url: "http://localhost"}]}

Which would reduce the duplication on configuration:

RouterBuilder.create(
  vertx,
  "some_api.yaml",
  loaderOptions,
  // here a new argument to provide AuthenticationHandler's
  AuthenticationHandlerFactory.create()
    .add("openid", config ->
        OpenIDConnectAuth.discover(
          vertx,
          // enhance the config if needed
          new Oauth2Options()
            .setSite(config.getString("openIdConnectUrl"))
          .compose(provider -> Future.succeeded(
            Oauth2Handler.create(
              provider,
              config.getJsonArray("servers").get(0).getString("url") + "/callback")

This would solve the creation of the handler. However we havent' dealt with request scopes. An api document can contain:

# top level global security
security:
  - openId:
      - pets_read
      - pets_write
      - admin

Which assumes that the token request should ask for those scopes at least, so we probably should also pass the global scopes to the factory config

{
  openIdConnectUrl: "https://example.com",
  servers: [{url: "http://localhost"]},
  scopes: ["pets_read", "pets_write", "admin"]
}

Note that the global ones should be filtered by security identity, so we don't need to have it as an object as we return the openId ones only.

With this the user may choose to use them or not in the factory:

RouterBuilder.create(
  vertx,
  "some_api.yaml",
  loaderOptions,
  // here a new argument to provide AuthenticationHandler's
  AuthenticationHandlerFactory.create()
    .add("openid", config ->
        OpenIDConnectAuth.discover(
          vertx,
          // enhance the config if needed
          new Oauth2Options()
            .setSite(config.getString("openIdConnectUrl"))
          .compose(provider -> Future.succeeded(
            Oauth2Handler.create(
              provider,
              config.getJsonArray("servers").get(0).getString("url") + "/callback")
              .withScope(config.getJsonArray("scopes").getString(0)
              ...

Now the handler is fully complete to be used in the router. It is important to notice that since some API's require security and other not the handler is only mounted before the user handler requiring it.

There's just one flaw here. Web users won't be able to login ever, because we didn't handle the Oauth2 "callback" route.

This means that the route builder need to mount as soon as possible (say after the static handler, session handler, etc...) the callbacks for each constructed handler:

for (Oauth2Handler h : securityHandlers) {
  Route callbackRoute = router.route();
  h.setupCallback(r);
}

This will ensure that the login handshake will work. We should only setupCallback if a handler has a callback URL, so this means that we may need to reverse the control at the handler itself to receive the router instead of route so it will mount only if needed.

Have I missed anything here @slinkydeveloper ?

PS: The same proposal should also work for security type oauth2. In this case the config json should contain the api document https://swagger.io/specification/#oauth-flow-object which allows the developer to extract the urls + flow from it avoiding double configuration.

[pmlopes]

Task breakout to accomplish this feature:

• Create the interface AuthenticationHandlerFactory which should be annotated with @VertxGen
• The interface should have a factory static method create() to follow the vert.x conventions
• The interface should have a method add(String id, Function<JsonObject, Future<AuthenticationHandler> provider)
• The implementation should store a map of the functions to be queried internally
• At the end of RouterBuilder.create a compose operation (or similar) should be used to bind the handlers to the factory
• Loop all the document components.securitySchemes and for each lookup a function by id and invoke it, the result should then be stored internally using the existing method: RouteBuilder.securityHandler(String, AuthenticationHandler)
• Extra loops are required to collect the extra config needed, for example the global security scopes and servers if present
• In RouteBuilder.createRouter() before looping the operations we need to mount the oauth2 callbacks for each handler of type Oauth2Handler

[thced]

@thced I've repurposed this issue to track OpenID Connect support, perhaps we should create a different one for sockjs bridge.

Maybe we could define a template openapi document with all the sockjs endpoints? and have some way to mount a sub router instead of a handler? However mounting a subrouter is tricky because it can shade the generated router so I'm inclined to say we need to carefully see how to handle this.

That is perfectly fine; to be honest, we can wait till this issue arise again, and then take it to the drawing board.

[pmlopes]

To avoid OpenAPI to handle the clone of OAauth2Handler's because of different scopes see: #1905

[pmlopes]

This issue should also be compatible with: JWTAuth as used here:

https://github.com/heremaps/xyz-hub/blob/80f93330d447c2ff7610407026494058a211f199/xyz-hub-service/src/main/java/com/here/xyz/hub/XYZHubRESTVerticle.java

Reference:

https://swagger.io/docs/specification/authentication/bearer-authentication/

    openapi: 3.0.0
    ...
    # 1) Define the security scheme type (HTTP bearer)
    components:
      securitySchemes:
        bearerAuth:            # arbitrary name for the security scheme
          type: http
          scheme: bearer
          bearerFormat: JWT    # optional, arbitrary value for documentation purposes
    # 2) Apply the security globally to all operations
    security:
      - bearerAuth: []         # use the same name as above

    paths:
      /something:
        get:
          security:
            - bearerAuth: []

From the document it seems that the security scopes here are expected to always be an empty list as scopes are an Oauth2/OIDC feature

[pmlopes]

OpenAPI 3.1.0 also states:

http://spec.openapis.org/oas/v3.1.0#patterned-fields-2

For other security scheme types, the array MAY contain a list of role names which are required for the execution, but are not otherwise defined or exchanged in-band.

This means that JWT auth should also consider validating the claims. We need to add an AuthorizationHandler for security related tasks.

[pmlopes]

OIDC + JWT scopes have been merged to master.