Support for SSL and corresponding options - patch request

ambisoft commented 12 years ago

Hello,

I found myself in a need of having ability to use the plugin while also enabling the SSL Connector in the Jetty. At first tried to accomplish the result with using a custom configuration, but this requires providing the whole Jetty config and I still could not make it run the way I needed.

So instead I forked the plugin and added support for SSL; with the following options exposed:

sslPort in container.Configuration := 8081

sslKeystore in container.Configuration := "<path_to>/ssl.keystore"

sslPassword in container.Configuration := "keystore password"

The fork is available at:

https://github.com/ambisoft/xsbt-web-plugin

Could you please check it out and, if it looks good, incorporate it into the project?

best regards Mark

aolshevskiy commented 12 years ago

This plugin is solely for testing your app. SSL support can be injected through custom configuration. I don't think it's necesary to host SSL support in code for sake of simplicity.

Objections?

ambisoft commented 12 years ago

Well, the main reason I thought it would be good to make it part of the core is that:

SSL support is a pretty common requirement, and
it can't really be injected through custom config, custom config requires providing a full Jetty configuration. In particular, it requires defining Context Handlers, if one wants to have support for the hot deployment.

The whole thing behind using SBT for development is being able to re-deploy the changes instantly/in-memory (as opposed to rebuilding the whole .war and redeploying it). I could not find an easy way to do it through custom configuration and I didn't find any examples online either.

So maybe a better idea would be to introduce a way to really inject custom options using XML, but without requiring a full config - so using a default connector and application loader/deployer with ability to add things from an XML snippet or file?

best Mark

aolshevskiy commented 12 years ago

At first go and clean up your code out of this Java idioms :) I left some comments for you.

ambisoft commented 12 years ago

Hey!

Thanks for checking it out and the comments. I changed SslSettings to be immutable case class and made the keystore and password Option[String]. I'm not sure what 'reasonable defaults' would be for these two, because keystore is a file which can be anywhere and password - hard to suggest anything ;) I made both None by default.

The 'keytool' command line utility uses .keystore file which is supposed to be in the user's home dir, but I think in practice xsbt-web-plugin when used in development, is running in the project's dir with the .war and related files generated automatically using maven/sbt (and with arbitrary keystore filename), so I don't quite feel that would be a good default.

Can you please take a look and tell if it's good now?

thanks a lot Mark

aolshevskiy commented 12 years ago

Sorry for offense, but actually your code is still bad. Go find at least 10 people in the community who need SSL support except of you and I'll implement this support myself.

Here are some notes for further clean up:

You ignored my comment about qualified import
Try too make ssl settings a one object(instead of a collection of options) since they're coupled as you say. This way you can just set it to none by default allowing the users to populate all settings or none at all. Exactly what you want I believe. Also this would allow to simplify jetty programmatic configuration code.
Create a pull request(I'm too lazy to checkout both reps and do manual comparison :) )
Since you're adding new startup configuration then you might want to merge startup settings like port, ssl settings and conf xml into one object to prevent start task argument list growing even further :)

aolshevskiy commented 12 years ago

Create a pull request(I'm too lazy to checkout both reps and do manual comparison :) )

Ahh, https://github.com/ambisoft/xsbt-web-plugin/compare/caf97184df26dc5e9ea4a8b98747c065fc768b55...master

ambisoft commented 12 years ago

Hey,

I made ssl a single setting, but probably 'there is more than one way to do it', so ... At first I exposed SslSettings as plugin's SBT key, but then decided using a tuple gives much nicer (shorter) syntax. Right now the usage is:

ssl in container.Configuration := Some(ssl_port, "path_to_keystore", "keystore_password", "key_password")

Should I create a pull request?

thanks Mark

hgoldwire commented 12 years ago

Hi guys. I too need this functionality, so that's 1 down 9 to go.

Does this code need more cleanup? If so, I'm happy to donate some labor to get this into the main project (and hopefully some day a widely-available maven repo).

Best, Henry

aolshevskiy commented 12 years ago

Should I create a pull request?

Sure.

At first I exposed SslSettings as plugin's SBT key, but then decided using a tuple gives much nicer (shorter) syntax. Right now the usage is:

Tuples are not descriptive from my point of view.

dimensia commented 12 years ago

We're trying to get SSL working locally as well too. Code that we're working with communicate using secure cookies and so we cannot test locally without SSL.

I'd love to see sbt have built-in support for SSL.

shindig commented 12 years ago

We would really like to have this also. It is incredibly convenient to be able to test SSL locally during development in SBT.

ambisoft commented 12 years ago

Ok, looks like 7 more people needed ;) btw. we have been using the forked version for a few weeks now and our team (www.parkio.com) is happy about it, it's a huge productivity booster. If you don't want to wait for the 'official' version, please check https://github.com/ambisoft/xsbt-web-plugin

rm -rf $HOME/.ivy2/cache/com.github.siasia
rm -rf $HOME/.ivy2/local/com.github.siasia
git clone https://github.com/ambisoft/xsbt-web-plugin.git
cd xsbt-web-plugin
sh <path_to_sbt>/sbt publish-local (builds a plugin and publishes locally in the ivy repository)

To use this version of the plugin, make the following change in the project/plugins.sbt

libraryDependencies <+= sbtVersion(v => "com.github.siasia" %% "xsbt-web-plugin" % (v+"-0.2.10.1"))

(i.e. the version is 0.2.10.1 instead of 0.2.10)

Config settings:

ssl in container.Configuration := Some(ssl_port, "path_to_keystore", "keystore_password", "key_password")

That's it. Up and running.

Still hoping it will make it into the main release

dimensia commented 12 years ago

ambisoft: Thanks! I'll check it out right now.

dimensia commented 12 years ago

It works great for us, thanks again!

For others reading this, we had to add:

import com.github.siasia.PluginKeys._ import com.github.siasia.WebPlugin._

Though that is not specific to SSL, and is just general xsbt-web-plugin configuration as far as I can tell.

ambisoft commented 12 years ago

dimensia: awesome :)

aolshevskiy commented 12 years ago

@ambisoft you could at least create a pull request for my convenience instead of blaming as you say 'official' version for lagging behind your 'precious' creation.

aolshevskiy commented 12 years ago

Oh, and thanks for unit tests I asked for. You didn't wrote em. It's awesome.

aolshevskiy commented 12 years ago

And also, you think you're too cool to not use deprecated methods, hah:

[warn] /home/siasia/projects/xsbt-web-plugin/target/templates/1/JettyRunner.scala:73: method setKeystore in class SslSelectChannelConnector is deprecated: see corresponding Javadoc for more information.
[warn]      conn.setKeystore(ssl.keystore)
[warn]           ^
[warn] /home/siasia/projects/xsbt-web-plugin/target/templates/1/JettyRunner.scala:74: method setPassword in class SslSelectChannelConnector is deprecated: see corresponding Javadoc for more information.
[warn]      conn.setPassword(ssl.password)
[warn]           ^
[warn] /home/siasia/projects/xsbt-web-plugin/target/templates/1/JettyRunner.scala:75: method setKeyPassword in class SslSelectChannelConnector is deprecated: see corresponding Javadoc for more information.
[warn]      conn.setKeyPassword(ssl.keyPassword)
[warn]           ^

ambisoft commented 12 years ago

I thought it's ok, because the plugin already used deprecated methods in other places:

[warn] /Users/mark/programming/scala/ambisoft/xsbt-web-plugin/src/main/scala/com/github/siasia/Container.scala:26: method evaluateTask in object Project is deprecated: This method does not apply state changes requested during task execution.  Use 'runTask' instead, which does.
[warn]              Project.evaluateTask(key, state) getOrElse
[warn]                      ^
[warn] /Users/mark/programming/scala/ambisoft/xsbt-web-plugin/src/main/scala/com/github/siasia/Container.scala:30: type ScopedTask in package sbt is deprecated: Use TaskKey, which is a drop-in replacement.
[warn]      private implicit def keyToResult[T](key: ScopedTask[T])(implicit state: State): T = eval(key)
[warn]                                               ^
[warn] /Users/mark/programming/scala/ambisoft/xsbt-web-plugin/src/main/scala/com/github/siasia/Container.scala:68: object TaskData in package sbt is deprecated: Superseded by task state system.
[warn]      discoveredContexts <<= TaskData.write(apps map discoverContexts) triggeredBy start,
[warn]                             ^
[warn] /Users/mark/programming/scala/ambisoft/xsbt-web-plugin/src/main/scala/com/github/siasia/Container.scala:96: object TaskData in package sbt is deprecated: Superseded by task state system.
[warn]      InputTask(TaskData(discoveredContexts)(reloadParser)(Nil)) {
[warn]

Looks like your unit tests didn't catch these...

shindig commented 12 years ago

Ambisoft, thanks for the contribution. It works great!

ambisoft commented 12 years ago

@shindig - great, may I ask what version of Jetty you're using? I haven't really tested it at all with 7

One weird thing I noticed was that google does not return much for "xsbt-web-plugin ssl" or "xsbt-web-plugin https"; as if nobody had this issue at all, or as if most people addressed it differently (by using nginx one level higher?) ; notheless having it 'just working' simplifies things a lot

aolshevskiy commented 12 years ago

Unit tests has nothing to do about deprecation. This an incorrect argument, you know. Anyway this is merged in and published into central as a version 0.2.11. Go on and check manually if it works for you folks since @ambisoft didn't provided any tests for me.

aolshevskiy commented 12 years ago

I haven't really tested it at all with 7

You are so kind, man.

dimensia commented 12 years ago

Regarding Jetty 7, I'm using Jetty 7 at the moment (we're planning on moving to Jetty 8 sometime soon hopefully) and it works for me.

ambisoft commented 12 years ago

@dimensia - sorry for making you the tester then :)

dimensia commented 12 years ago

I updated to 0.2.11 and the SSL changes are still working great! Thanks @ambisoft and @siasia!

dimensia commented 12 years ago

Also, we should get this documented in the wiki. I'm willing to help out with that if others are busy.

aolshevskiy commented 12 years ago

You're welcome :)

ambisoft commented 12 years ago

Thanks @siasia and @dimensia AND @shindig AND @hgoldwire :)

hgoldwire commented 12 years ago

Thanks guys! It works great!

joaosa commented 12 years ago

I also needed this functionality and it works perfectly. Thank you all!

earldouglas / xsbt-web-plugin

Support for SSL and corresponding options - patch request #65