search

twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.78k stars 1.45k forks source link

Make SslServerSessionVerifier return an asynchronous result #745

Open acidghost opened 5 years ago

acidghost commented 5 years ago

In cases where the SslServerSessionVerifier needs the result of a Future before returning a Boolean, the user is required to block (Await).

Expected behavior

Ideally SslServerSessionVerifier.apply should return a Future[Boolean] so that the user is not required to Await for a future value and the netty thread doesn't block.

Actual behavior

Suppose SslServerSessionVerifier.apply makes a call to an external service (e.g. a finagle HTTP client) and needs data from the reply in order to verify the SSL session. In this case the user is required to Await.result over that future in order to return a plain Boolean.

Steps to reproduce the behavior

Here's a toy project: https://github.com/acidghost/microservices-finagle/blob/56f9e50fb24b9a7a81f28ef9d177112df5ec5c2f/users/src/main/scala/users/Main.scala#L47. In the code, consul.authorize parses the peer certificate with BouncyCastle and then makes an HTTP request to the local consul agent (https://www.consul.io/docs/connect/native.html) using a finagle HTTP client.

mosesn commented 5 years ago

@acidghost this seems pretty reasonable to me. let me see if I can pull in our local expert too. would you be interested in making this PR if we approve it?

acidghost commented 5 years ago

@mosesn for sure I can try to tackle it! I already have an idea (or at least I think I do) of the parts that need to be changed (com.twitter.finagle.netty4.ssl.server.SslServerVerificationHandler and com.twitter.finagle.netty3.ssl.server.SslServerConnectHandler).

ryanoneill commented 5 years ago

Hi @acidghost. This is a very reasonable change, and I think it improves the existing functionality. We didn't make it Future-based originally because we didn't need the functionality based on how our credentials and authorization metadata are distributed internally.

My only request here would be to try to do the client side as well at the same time. I would like the two APIs to stay in sync. Thanks.

mosesn commented 5 years ago

@acidghost alright, go for it!