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

Honor play settings on AkkaHttpServer for client auth in SSL #8642

Merged
merged 3 commits into from Oct 3, 2018

Conversation

Projects
None yet
4 participants
@ignasi35
Member

ignasi35 commented Sep 28, 2018

Fixes

AkkaHttpServer didn't honour some HTTPS-related settings:

play {
  server {
    # HTTPS configuration
    https {
      # Whether JSSE want client auth mode should be used. This means, the server
      # will request a client certificate, but won't fail if one isn't provided.
      wantClientAuth = false

      # Whether JSSE need client auth mode should be used. This means, the server
      # will request a client certificate, and will fail and terminate the session
      # if one isn't provided.
      needClientAuth = false
    }
  }
}
```

@ignasi35 ignasi35 requested review from renatocaval and marcospereira Sep 28, 2018

@ignasi35 ignasi35 self-assigned this Sep 28, 2018

@ignasi35 ignasi35 added this to the Play 2.6.20 milestone Sep 28, 2018

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 28, 2018

I marked it as needs-backport but it actually needs forward-port (we don't have a label for that).

@dwijnand

This comment has been minimized.

Member

dwijnand commented Sep 28, 2018

(we don't have a label for that)

Buy contact lenses.

@dwijnand

Looks reasonable to me!

@@ -160,7 +160,21 @@ class AkkaHttpServer(context: AkkaHttpServer.Context) extends Server {
// factory for creating an SSLEngine, so the user can configure it themselves. However, that means that in
// order to pass an SSLContext, we need to pass our own one that returns the SSLEngine provided by the factory.
val sslContext = mockSslContext()
ConnectionContext.https(sslContext = sslContext)
val playServerConfig = context.config.configuration.get[Configuration]("play.server")

This comment has been minimized.

@marcospereira

marcospereira Sep 28, 2018

Member

Since we made parserSettings and createServerSettings overridable, we should make the same here. I think that what we have for parserSettings (lazy val and create method) is what we should do here.

This comment has been minimized.

@renatocaval

renatocaval Sep 28, 2018

Contributor

parserSettings is not a lazy val

@@ -160,7 +160,21 @@ class AkkaHttpServer(context: AkkaHttpServer.Context) extends Server {
// factory for creating an SSLEngine, so the user can configure it themselves. However, that means that in
// order to pass an SSLContext, we need to pass our own one that returns the SSLEngine provided by the factory.
val sslContext = mockSslContext()
ConnectionContext.https(sslContext = sslContext)
val playServerConfig = context.config.configuration.get[Configuration]("play.server")

This comment has been minimized.

@marcospereira

marcospereira Sep 28, 2018

Member

There is a serverConfig you can reuse here.

This comment has been minimized.

@ignasi35

ignasi35 Sep 28, 2018

Member

Good call. Missed that.

val playServerConfig = context.config.configuration.get[Configuration]("play.server")
val clientAuth: Option[TLSClientAuth] =
if (playServerConfig.get[Boolean]("https.needClientAuth")) {

This comment has been minimized.

@marcospereira

marcospereira Sep 28, 2018

Member

Maybe add a comment saying that need has a precedence over want?

This comment has been minimized.

@ignasi35

ignasi35 Sep 28, 2018

Member

TBH, I'm not entirely sure what is the precedence. The Java API for SSLEngine only indicates that invoking both setNeedClientAuth and setWantClientAuth override each other so last wins. Using an ADT is better but forces us to choose. The implementation in NettyServer also gives need precedence over want (https://github.com/playframework/playframework/blob/master/framework/src/play-netty-server/src/main/scala/play/core/server/NettyServer.scala#L170-L175)

https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLEngine.html#setNeedClientAuth-boolean-

This comment has been minimized.

@dwijnand

dwijnand Sep 29, 2018

Member

Intuitively, in English, need has precedence over want. I need oxygen. I want gelato.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 28, 2018

Buy contact lenses.

puts contact lenses on top of glasses

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 28, 2018

We discussed two choices to test this:

  1. a scripted tests that reuses the reproducer but it's a scripted test that will require some rework or depending on curl and exec
  2. create a purely programmatic test that:
    1. creates a client CA and a client certificate in keystore client.jks
    2. creates a server CA and a server certificate in keystore server.jks
    3. adds each CA as a trusted element on the other keystore client.jks
    4. programmatically start a play app with netty or akka http backends (with SSL enabled and configured to use server.jks. The play app must enable needClientAuth.
    5. programatically start a client configured to use client.jks
      NOTE: in 2. certificate should use the appropriate CN (probably localhost) .
@renatocaval

This comment has been minimized.

Contributor

renatocaval commented Sep 28, 2018

puts contact lenses on top of glasses

try putting it behind

@dwijnand dwijnand merged commit d286269 into playframework:2.6.x Oct 3, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@renatocaval renatocaval removed the in progress label Oct 3, 2018

@renatocaval renatocaval referenced this pull request Oct 4, 2018

Closed

Forward port #8642 #8660

marcospereira added a commit to marcospereira/playframework that referenced this pull request Nov 6, 2018

Honor play settings on AkkaHttpServer for client auth in SSL (playfra…
…mework#8642)

`AkkaHttpServer` didn't honour some HTTPS-related settings:

````
play {
  server {
    # HTTPS configuration
    https {
      # Whether JSSE want client auth mode should be used. This means, the server
      # will request a client certificate, but won't fail if one isn't provided.
      wantClientAuth = false

      # Whether JSSE need client auth mode should be used. This means, the server
      # will request a client certificate, and will fail and terminate the session
      # if one isn't provided.
      needClientAuth = false
    }
  }
}
```

marcospereira added a commit that referenced this pull request Nov 6, 2018

Honor play settings on AkkaHttpServer for client auth in SSL (#8642) (#…
…8774)

`AkkaHttpServer` didn't honour some HTTPS-related settings:

````
play {
  server {
    # HTTPS configuration
    https {
      # Whether JSSE want client auth mode should be used. This means, the server
      # will request a client certificate, but won't fail if one isn't provided.
      wantClientAuth = false

      # Whether JSSE need client auth mode should be used. This means, the server
      # will request a client certificate, and will fail and terminate the session
      # if one isn't provided.
      needClientAuth = false
    }
  }
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment