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

Authentication / security enhancements #1167

Closed
adamw opened this issue Apr 14, 2021 · 20 comments · Fixed by #1600
Closed

Authentication / security enhancements #1167

adamw opened this issue Apr 14, 2021 · 20 comments · Fixed by #1600
Labels
api help wanted Extra attention is needed server

Comments

@adamw
Copy link
Member

adamw commented Apr 14, 2021

Current state

Tapir currently has some support for authentication / security:

  • inputs which extract tokens used for authentication can be created using auth.apiKey[String], auth.bearer[Token] etc. These are then marked as authentication inputs in OpenAPI docs, but otherwise extract raw auth tokens, such as a string api key, just as normal inputs do
  • an extendable base endpoint with server-side authentication logic can be defined, using the .serverLogicForCurrent method. Such an endpoint has logic for transforming e.g. a String token into a User instance built-in, and can be extended with additional inputs/outputs (error outputs are fixed)
  • alternatively, server logic can be provided in parts, given a complete endpoint definition, using .serverLogicPart. That way, the same authentication function can be used in multiple endpoints, without the need for manual composition

Missing features

Still, some functionalities are missing. Some examples of features that would be nice to have:

  • ensuring that for an endpoint with authentication inputs defined, authentication logic is always run - even if the authenticated e.g. User instance isn't used
  • specifying common authentication logic for all secure endpoints, without the need to specify it for each one
  • checking authentication before parsing the body - or even before decoding any other inputs
  • performing authorization - e.g. checking roles extracted from a JWT token, comparing them to what the endpoint supports

How to improve Tapir's support for authentication / security? I don't have a definite answer, but I would surely appreciate feedback on what people use right now, and what are their experiences.

Survey

Please fill this short survey: https://forms.gle/WWzEP78vpmQdkHyp8 (I'll post anonymous results here in some time), or if you prefer leave a comment beneath:

  1. How do you perform authentication with tapir? Do you use only .serverLogic, or .serverLogicForCurrent, .serverLogicPart?
  2. Do you use .serverLogicForCurrent or .serverLogicPart for other purposes?
  3. What is your main pain point with authentication/authorization in Tapir? What kind of features would you like to see added?
  4. Would you use DefferedServerEndpoint as described below?

Thank you!

Possible solutions

What are some possible solutions?

A. It might be tempting, to define an interceptor which would perform authentication, and possibly authorisation as well. An interceptor could inspect the endpoint, and if there are supported authentication inputs, run the authentication logic. Moreover, if in the endpoints tags (a yet-non-existent, but easy to add field Endpoint.tags: Map[String, Any]) contain required roles, we could perform authorisation here as well. Interceptors could be extended with a callback allowing them to run when the endpoint is determined to match the method/path, but before the body is decoded.

There are some major problems here, however. How to pass the result of authentication to the endpoint's logic? This would have to be somehow attached to the request (again to an untyped tags map), and then extracted by an appropriate input on the endpoint (using extractFromRequest). Doable, but not a clean & typesafe solution. Also, the "supported" authentication inputs would have to be passed exactly as defined in the endpoint, which would open the door to yet another possible runtime error.

B. Create server endpoints with part of the logic deferred. This is an extension (or maybe replacement?) to what is currently possible using the two partial server logic variants. Having an Endpoint[(A, I), E, O, R] - note that the input is intentionally written down as a tuple containing authentication inputs A and other inputs I - we could define a method:

val d = (e: Endpoint[(A, I), E, O, R]).deferredServerLogic[A, User] { (u: User, i: I) => sth: F[Either[E, O]] }

d: DefferedServerEndpoint[A, User, I, E, O, R]

Hence we would provide the server logic for the endpoint, except providing the authentication part. Then, we could complete the server logic, giving the authentication logic once, in bulk:

// assuming that all deferred endpoints have the same A, User types, 
// and a common E supertype - that parameter would need to be covariant
val dses: List[DefferedServerEndpoint[A, User, _, E, _, R]] = ... 

val ses = l.completeServerLogic { (a: A) => sth: User }
ses: List[ServerEndpoint[_, _, _, R]]

// or using the interpreter
Http4sServerInterpreter.toRoutes(dses, (a: A) => sth: User): Route

What do you think? Maybe you have some alternative ideas? What other security-related features are missing from tapir? Let us know - thanks :)

@adamw adamw added api help wanted Extra attention is needed server labels Apr 14, 2021
@markarasev
Copy link
Contributor

markarasev commented Apr 16, 2021

Not sure if intentional but to me a user parameter is missing to the function passed to the deferredServerLogic example:

val d = (e: Endpoint[(A, I), E, O, R]).deferredServerLogic[A, User] { (user: User, i: I) => sth: F[Either[E, O]] }

d: DefferedServerEndpoint[A, User, I, E, O, R]

The authenticated user may be necessary to perform user-related operations for instance.

@adamw
Copy link
Member Author

adamw commented Apr 16, 2021

@markarasev Yes of course, thanks for pointing this out! Fixed :)

@adamw
Copy link
Member Author

adamw commented Apr 16, 2021

Btw.: suggestions on alternate syntax, improvements to the existing methods or ideas for new ones welcome - even if not fully fleshed out and maybe impossible to implement :)

@markarasev
Copy link
Contributor

markarasev commented Apr 16, 2021

I find that .serverLogicForCurrent feels really natural for sharing common behavior but the need to fully define the error type upfront is a show stopper to me:

I have several endpoints sharing the same authentication method and logic but each endpoint has specific errors. I don't want to use a too generic error type, that would imply an endpoint could return some concrete error although it cannot and should never do so (a "Not Found" error defined for getting an entity would also be allowed when PUTting an entity at an existing path for instance).

At the moment, I'm fine with duplicating authentication logic (which can be factorized to a single method call) but it feels a little bit odd. Actually, I don't understand this restriction on error types while some other types can be refined further, but I did not have a closer look to the code to find the reason.

If .serverLogicForCurrent could be "fixed" by, for instance, allowing the error type to be replaced later by a super-type of the previous one, it would be ideal. It could look like:

sealed trait GetEntityError
sealed trait Endpoint2Error
case class EntityNotFound(id: String) extends GetEntityError
case object ConcreteEndpoint2Error extends Endpoint2Error
case class Unauthorized(token: String) extends GetEntityError with Endpoint2Error

case class User(name: String)
def auth(token: String): Future[Either[Unauthorized, User]] = Future {
  if (token == "secret") Right(User("Spock"))
  else Left(Unauthorized(token))    // shared authorization error
}

val secureEndpoint: PartialServerEndpoint[String, User, Unit, Unauthorized, Unit, Any, Future] = endpoint
  .in(header[String]("X-AUTH-TOKEN"))
  .errorOut(jsonBody[Unauthorized])
  .serverLogicForCurrent(auth)

val entities = Seq(...)
  
val getEntityEndpoint = secureEndpoint.get
  .in("entity")
  .in(path[String]))
  .out(stringBody)
  .errorOut(jsonBody[GetEntityError])    // GetEntityError is a supertype of Unauthorized
  .serverLogic { case (user, entityId) =>
    val result = entities.find(_.id == entityId) match {
      case Some(entity) => Right(entity.toString)
      case None => Left(EntityNotFound(entityId))     // Here I can use an endpoint-specific error
    }
    Future.successful[Either[GetEntityError, String]](result)
  }
  
val endpoint2 = secureEndpoint.errorOut(jsonBody[Endpoint2Error])    // Endpoint2Error is a supertype of Unauthorized
// ...

Also, an alternative could be to provide a mapping function from the previous error type to the new one when redefining .errorOut.

While I understand the idea behind .deferredServerLogic, it seems much less straightforward than .serverLogicForCurrent to me. I would be able to discover the latter using autocompletion and use it right away, while I would need to read some docs about .deferredServerLogic to understand and use it.

@pishen
Copy link

pishen commented Sep 6, 2021

Not sure if it's possible, but it may be more useful if the output of PartialServerEndpoint could be available in the next call of serverLogicForCurrent, for example:

val groupEndpoint = endpoint
  .in(header[String]("X-AUTH-TOKEN"))
  .in("group" / path[Int]("group-id"))
  .errorOut(jsonBody[Error])
  .serverLogicForCurrent { case (token, groupId) =>
    val res = for {
      user <- EitherT(auth(token))
      _ <- userInGroup(user, groupId)
    } yield {
      (user, groupId)
    }
    res.value
  }

val itemEndpoint = groupEndpoint
  .in("item" / path[Int]("item-id"))
  .serverLogicForCurrent { case ((user, groupId), itemId) =>
    EitherT(itemInGroup(itemId, groupId)) // need groupId again here
      .map(_ => itemId)
      .value
  }

val finalEndpoint = itemEndpoint
  .in("buy")
  .serverLogic { case ((user, groupId), itemId) =>
    ...
  }

@adamw
Copy link
Member Author

adamw commented Oct 23, 2021

Thanks for the feedback so far! I'd like to share an updated proposal for security-related features in the upcoming versions of Tapir. As always, more feedback welcome :)

Describing security inputs

  1. First of all, I'd like to introduce a SecureEndpoint[A, I, E, O, R]. Such an endpoint would behave just as an Endpoint[I, E, O, R], but it would have the auth inputs separated in a dedicated input, with the type tracked by the type parameter A. So we'd have something like endpoint.authIn(bearerToken) or endpoint.securityIn(bearerToken) (suggestions on naming welcome as well!) to add security inputs, in addition to the usual .in/.out/.errorOut methods. An alternative name would be AuthedEndpoint[A, I, E, O, R].

Server logic & security

  1. There would be a similar differentiation with ServerEndpoint, which would become sealed trait with two implementations: PublicServerEndpoint[I, E, O, R, F[_]](Endpoint[I, E, O, R], I => F[Either[E, O]) and SecureServerEndpoint[A, I, E, O, R, U, F[_]](SecureEndpoint[A, I, E, O, R], A => F[Either[E, U]], (I, U) => F[Either[E, O]]). The first function (A => F[E|U]) would perform the authentication (producing a "user" of type U or an error), and the second is the main business logic with the security context in place.

  2. When interpreting a server endpoint (as a server), the interpreter would first decode the A inputs and run the security part of the business logic, and only then decode the I inputs, if a security context/user has been returned. A decode failure or a result of type E would short-circuit the whole process. That way, body decoding would only be attempted if auth is successful (given of course that the body input is part of I, not A :) ).

  3. This means that the the A part of the input should also contain any inputs to be able to respond with a 401/404 in case the security logic returns an E error (or fails). This might include e.g. a path prefix such as /secure.

  4. It would be possible to provide only the security part of the logic to a SecureEndpoint, yielding a PartialSecureServerEndpoint, which would allow extending the inputs/outputs further.

  5. With the generalisation of oneOf to allow nesting, we can now implement broadening the error type once it's been partially defined. So it would be possible to extend the possible error cases of PartialSecureServerEndpoint using e.g. .errorOutOneOf / .errorOutAlternative / .errorOutVariant (naming TBD :) ).

Removals

  1. With the security-specialised inputs available, I think we will be able to remove the current Auth input wrapper, and simply use normal headers for api-keys, and define Http/Oauth as regular inputs. For documentation, any input that is defined in the A security input of an SecureEndpoint would be considered as part of authentication.

  2. Another area where we could simplify is .serverLogicForCurrent. While the .serverLogicForCurrent mechanism is in some ways more powerful than the proposed scheme, as it allows providing the server logic & inputs in multiple chunks, we'd remove it to simplify the overall design. That would mean that you'll have to provide the server logic in at most two chunks (instead of multiple): the security part and the regular part. It would be possible to support multi-level logic chunks and keep the decode-inputs-if-previous-chunk-successful, but it would be quite complex.

  3. Similarly .serverLogicPart would be removed, however here I'd revive the .andThenLogic functions from earlier versions of Tapir which allowed combining I => F[Either[E, O1]] with O1 => F[Either[E, O2]] functions.

I think the proposed solution meets the requirements mentioned here:

  • allows defining a base "authenticated endpoint" with the security inputs and logic defined, with the possibility to extend both the inputs and the errors
  • in the server interpreter, the server body is only parsed if security is successful
  • provides a readable way of defining security for endpoints
  • and finally - is possible to implement (though we'll see what kind of roadblocks await ;) )

@adamw
Copy link
Member Author

adamw commented Oct 26, 2021

There's one more problem unfortunately ;) We now end up with two unrelated endpoint types: Endpoint and SecureEndpoint, while their server counterparts are related (sealed trait ServerEndpoint with Public and Secure implementations - that's needed to create a list of server endpoints).

However, after a closer look, Endpoint/SecureEndpoint need to be related as well. Client and doc interpreters need to accept both types of endpoints, with the docs interpreter accepting a list which can have mixed endpoint types.

One solution is to create a mirror hierarchy: sealed trait Endpoint with two implementations: PublicEndpoint[I, E, O, R] and SecureEndpoint[A, I, E, O, R].

But maybe there's a simple solution: add a type parameter to Endpoint and be done with it ;). That way, we would have two inputs & two outputs sections for all endpoints. By default the security section would be empty, meaning no special checks.

There's one huge drawback - an additional type parameter for all endpoints. I don't like that, but the alternative - having a hierarchy of endpoints - doesn't look like being easier to work with. It would be hard to preserve type-safety, given that the inputs of both types of endpoints differ.

Let me know what you think :)

@adamw
Copy link
Member Author

adamw commented Oct 26, 2021

This would also simplify the ServerEndpoint. So we'd end up with: Endpoint[A, I, E, O, R] and ServerEndpoint[A, U, I, E, O, R, F[_]].

Hmm we'll need some type aliases to make this more user-friendly :) Maybe following the changes proposed in #1157 after all:

Endpoint[A, I, E, O, R]

ServerEndpoint[R, F[_]]
// and
ServerEndpoint.Full[A, U, I, E, O, R, F[_]]
// A, U, I, E, O are type members of `ServerEndpoint`

@adamw
Copy link
Member Author

adamw commented Nov 6, 2021

First portion of the changes is implemented in 0.19.0-M14: https://github.com/softwaremill/tapir/releases/tag/v0.19.0-M14

Feedback welcome :)

@adamw
Copy link
Member Author

adamw commented Nov 11, 2021

Extending the error output in partial endpoints is now possible using .errorOutVariant, see the https://github.com/softwaremill/tapir/releases/tag/v0.19.0-M16 release

@kamilkloch
Copy link
Contributor

kamilkloch commented Nov 17, 2021

First, thank you the recent changes in this area! I am trying to make use of dedicated security input to implement a basic session-based (cookie) authentication, and I am not sure what would be the right way to proceed. Before tapir, I would use tsec for http4s (https://jmcardon.github.io/tsec/docs/http4s/auth-scookie.html), where one would define an authed endpoint as

case request@GET -> Root / "api" asAuthed user =>

and all the plumbing (cookie authentication, session updates etc.) would happen automatically.

Tapir version of the endpoint would look as follows:

endpoint.securityIn(auth.apiKey(cookie[String]("api-session")))

But now, .serverSecurityLogic(String => ....) has to re-implement all the session-related plumbing from tsec-http4s, is that right? Is there a way to reuse the existing http4s-specific (or zio-http / akka-http) authentication logic? Inject authentication middleware with customInterceptors and lose the security channels in tapir endpoints?

@adamw
Copy link
Member Author

adamw commented Nov 17, 2021

@kamilkloch Hm I would have to make some experiments in code (not sure what the types involved are), but on a first quick glance, if I recall correctly, asAuthed is a function similar to Request => F[Option[SecureRequest]]? Optional - as authentication might fail, and SecureRequest is Request + User.

It would probably be challenging to reuse this directly, as here the asAuthed function is split into two: the meta-data component (.securityIn(auth.apiKey(cookie[String]("api-session")))) and the logic component (which doesn't have to know that the data is coming from a cookie): .serverSecurityLogic(String => ...). So you'd have to somehow extract the String => F[Either[AuthenticationError, User]] function from asAuthed and call it there.

It's probably a good idea to look into integration with libraries such as tsec, but that's a second (or third ;) ) step after having the basics in place.

@kamilkloch
Copy link
Contributor

@adamw The logic component is beneath asAuthed - it all happens in the TSecMiddleWare:
https://github.com/jmcardon/tsec/blob/6b163e19e2373c61d9fb4879fb871b30d7a0c5ee/tsec-http4s/src/main/scala/tsec/authentication/SecuredRequestHandler.scala#L23

Now, general question for all http4s middleware, how to retrofit Request[F] => F[Option[Response[F]] into .server(Security)Logic ;)

@adamw
Copy link
Member Author

adamw commented Nov 17, 2021

@kamilkloch In general this won't be possible - as a middleware contains both the metadata on what it reads (which headers etc.) and the logic in a single opaque function. In tapir this is separated.

You could try with an interceptor, though. The shape is similar :). And I guess the type of a middleware is (Request[F] => F[Option[Response[F]]) => Request[F] => F[Option[Response[F]]?

@adamw
Copy link
Member Author

adamw commented Nov 17, 2021

@kamilkloch in fact, maybe create an issue with a feature request to create a tsec integration?

@kamilkloch
Copy link
Contributor

kamilkloch commented Nov 18, 2021

@adamw Yes, spot on with the type of the middleware :)
I very much agree it is a good idea to add more out-of-the-box support for sessions, JWTs etc, now that the endpoint has decicated channels solely for that purpose. I found the following libraries which do the job for one specific backend:

In fact, it looks like a lot of work has been poured into https://github.com/softwaremill/akka-http-session. Perhaps it would be possible to re-use it and abstract over the backend.

@kamilkloch
Copy link
Contributor

@adamw Created #1625.

@dvgica
Copy link
Contributor

dvgica commented Nov 24, 2021

@adamw would just like to say: really, really nice job on the security and error improvements. We were doing some hacks similar to people in this thread, and the 0.19.0 release totally cleans them up and enables better type safety. Could not be happier!

@adamw
Copy link
Member Author

adamw commented Nov 24, 2021

@dvgica Thank you!

@markarasev
Copy link
Contributor

Just refactored my code using the new security features and it seems to work well, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api help wanted Extra attention is needed server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants