Use Jetty's ProxyServlet implementation #2753

watou · 2017-01-05T04:21:15Z

The Jetty proxy servlet is a more thorough implementation of a proxy, and likely fixes #2692 (not closing connections) and header mistakes (#2734 (comment)).

It is configurable via OSGi ConfigAdminManager using pid org.eclipse.smarthome.proxy. Any of the properties documented here can be passed to the servlet.

"Assumed" Basic authentication behaves the same as previous, but required Digest authentication is not addressed (see this report for current issues with authentication). To be clear, the authentication behavior should be identical to the previous version.

A follow-on PR will address which URLs are allowed to be proxied by maintaining a whitelist:

All sitemap-derived URLs are automatically whitelisted and will be proxied
If the target url= host:port is on a whitelist configured for the servlet, it will be proxied. If not, the servlet will respond with a 302 redirect to the given URL.

Signed-off-by: John Cocula john@cocula.com

Made configurable via OSGi Signed-off-by: John Cocula <john@cocula.com>

kaikreuzer

Lgtm in general.
One concern I have: Up to know, ESH is compatible to javax.servlet 2.4 (see C.2, afaik, OSGi 4.2 uses servlet spec 2.4) - the Jetty ProxyServlet breaks this (I found a dependency to at least 2.6, which corresponds to Servlet 3.0 spec).

kaikreuzer · 2017-02-07T13:29:53Z

...eclipse.smarthome.ui/src/main/java/org/eclipse/smarthome/ui/internal/proxy/ProxyServlet.java

        if (sitemapName == null) {
-            response.sendError(HttpServletResponse.SC_BAD_REQUEST, "Parameter 'sitemap' must be provided!");
+            logger.error("Parameter 'sitemap' must be provided!");


I do not really like such things to be logged as errors in the runtime log. After all, these might be unsolicited requests from remote clients and errors should rather turn up in the client (if on the server, it should rather be some http access/error log).
Is there no way to send an HTTP error back here as it was done before?

…rver.

watou · 2017-02-07T15:55:35Z

Specific errors should be sent back to the client now (more accurate than previous set) instead of being logged.

I will look at javax.servlet 2.4 compatibility issue. Do you have any suggestions?

watou · 2017-02-08T14:50:55Z

I suppose my testing works on OH2 because it is using OSGi Release 5 or 6? Any chance ESH can move to a later OSGi spec, since it's also moving to Java 8 soon?

openhab> bundle:capabilities 20
javax.servlet-api_3.1.0 [20] provides:
--------------------------------------
osgi.wiring.bundle; javax.servlet-api 3.1.0 [UNUSED]
osgi.wiring.host; javax.servlet-api 3.1.0 [UNUSED]
osgi.identity; javax.servlet-api 3.1.0 [UNUSED]
osgi.wiring.package; javax.servlet 3.1.0 required by:
   org.eclipse.jetty.plus_9.2.19.v20160908 [78]
   org.glassfish.jersey.containers.jersey-container-servlet-core_2.22.2 [156]
   org.openhab.ui.dashboard_2.0.0.RC1 [171]
   org.eclipse.smarthome.io.rest.sitemap_0.9.0.b3 [118]
   org.jupnp_2.2.0 [163]
   org.eclipse.smarthome.io.rest_0.9.0.b3 [115]
   org.eclipse.jetty.servlet_9.2.19.v20160908 [82]
   org.eclipse.jetty.websocket.servlet_9.2.19.v20160908 [93]
   org.eclipse.jetty.util_9.2.19.v20160908 [84]
   com.eclipsesource.jaxrs.publisher_5.3.1.201602281253 [11]
   org.eclipse.jetty.continuation_9.2.19.v20160908 [71]
   org.eclipse.smarthome.core.audio_0.9.0.b3 [100]
   org.eclipse.jetty.servlets_9.2.19.v20160908 [83]
   org.glassfish.jersey.containers.jersey-container-servlet_2.22.2 [155]
   org.apache.karaf.http.core_4.0.8 [45]
   org.eclipse.smarthome.io.rest.sse_0.9.0.b3 [119]
   org.eclipse.jetty.websocket.server_9.2.19.v20160908 [92]
   org.eclipse.smarthome.ui_0.9.0.b3 [137]
   org.eclipse.jetty.jaspi_9.2.19.v20160908 [75]
   org.ops4j.pax.web.pax-web-api_4.3.0 [172]
   org.eclipse.jetty.rewrite_9.2.19.v20160908 [79]
   org.eclipse.jetty.webapp_9.2.19.v20160908 [86]
   org.openhab.core_2.0.0.RC1 [167]
   org.ops4j.pax.web.pax-web-jetty_4.3.0 [173]
   org.eclipse.jetty.server_9.2.19.v20160908 [81]
   org.eclipse.smarthome.ui.icon_0.9.0.b3 [138]
   org.ops4j.pax.web.pax-web-runtime_4.3.0 [174]
   org.eclipse.jetty.websocket.javax.websocket.server_9.2.19.v20160908 [91]
   org.eclipse.jetty.security_9.2.19.v20160908 [80]
   org.eclipse.smarthome.ui_0.9.0.201701161004 [9]
   org.ops4j.pax.web.pax-web-spi_4.3.0 [175]
   org.eclipse.smarthome.ui.classic_0.9.0.b3 [190]
   org.openhab.core.compat1x_2.0.0.RC1 [205]
   org.eclipse.smarthome.ui.basic_0.9.0.b3 [189]
   org.openhab.ui.habpanel_2.0.0.RC1 [211]
   org.eclipse.jetty.proxy_9.2.12.v20150709 [213]
osgi.wiring.package; javax.servlet.descriptor 3.1.0 required by:
   org.eclipse.jetty.servlet_9.2.19.v20160908 [82]
   org.eclipse.jetty.webapp_9.2.19.v20160908 [86]
   org.eclipse.jetty.server_9.2.19.v20160908 [81]
osgi.wiring.package; javax.servlet.annotation 3.1.0 required by:
   org.eclipse.jetty.servlet_9.2.19.v20160908 [82]
   org.glassfish.jersey.containers.jersey-container-servlet_2.22.2 [155]
   org.ops4j.pax.web.pax-web-api_4.3.0 [172]
   org.ops4j.pax.web.pax-web-jetty_4.3.0 [173]
   org.eclipse.jetty.websocket.javax.websocket.server_9.2.19.v20160908 [91]
   org.eclipse.jetty.security_9.2.19.v20160908 [80]
osgi.wiring.package; javax.servlet.http 3.1.0 required by:
   org.glassfish.jersey.containers.jersey-container-servlet-core_2.22.2 [156]
   org.openhab.ui.dashboard_2.0.0.RC1 [171]
   org.eclipse.smarthome.io.rest.sitemap_0.9.0.b3 [118]
   org.jupnp_2.2.0 [163]
   org.eclipse.smarthome.io.rest_0.9.0.b3 [115]
   org.eclipse.jetty.servlet_9.2.19.v20160908 [82]
   org.eclipse.jetty.websocket.servlet_9.2.19.v20160908 [93]
   org.eclipse.jetty.util_9.2.19.v20160908 [84]
   com.eclipsesource.jaxrs.publisher_5.3.1.201602281253 [11]
   org.eclipse.smarthome.core.audio_0.9.0.b3 [100]
   org.eclipse.jetty.servlets_9.2.19.v20160908 [83]
   org.glassfish.jersey.containers.jersey-container-servlet_2.22.2 [155]
   org.eclipse.smarthome.io.rest.sse_0.9.0.b3 [119]
   org.eclipse.jetty.websocket.server_9.2.19.v20160908 [92]
   org.eclipse.smarthome.ui_0.9.0.b3 [137]
   org.eclipse.jetty.jaspi_9.2.19.v20160908 [75]
   org.eclipse.jetty.rewrite_9.2.19.v20160908 [79]
   org.eclipse.jetty.webapp_9.2.19.v20160908 [86]
   org.openhab.core_2.0.0.RC1 [167]
   org.ops4j.pax.web.pax-web-jetty_4.3.0 [173]
   org.eclipse.jetty.server_9.2.19.v20160908 [81]
   org.eclipse.smarthome.ui.icon_0.9.0.b3 [138]
   org.ops4j.pax.web.pax-web-runtime_4.3.0 [174]
   org.eclipse.jetty.websocket.javax.websocket.server_9.2.19.v20160908 [91]
   org.eclipse.jetty.security_9.2.19.v20160908 [80]
   org.eclipse.smarthome.ui_0.9.0.201701161004 [9]
   org.openhab.core.compat1x_2.0.0.RC1 [205]
   org.openhab.ui.habpanel_2.0.0.RC1 [211]
   org.eclipse.jetty.proxy_9.2.12.v20150709 [213]

kaikreuzer · 2017-02-09T12:10:54Z

Any chance ESH can move to a later OSGi spec, since it's also moving to Java 8 soon?

Unfortunately not as there are ESH solutions out there that are using OSGi 4.2 (which is still quite widespread in commercial deployments).

I will look at javax.servlet 2.4 compatibility issue. Do you have any suggestions?

Maybe we could do a similar solution as in the SSE support: This checks for servlet3 support and then registers different implementations. So we could keep the old proxy servlet as a fallback, while registering the Jetty proxy servlet on all servlet3 capable environments. Wdyt?

watou · 2017-02-09T12:59:46Z

...checks for servlet3 support and then registers different implementations. So we could keep the old proxy servlet as a fallback, while registering the Jetty proxy servlet on all servlet3 capable environments.

OK, I will add that.

…ailable; otherwise register older blocking servlet, but maintain identical external usage. Signed-off-by: John Cocula <john@cocula.com>

watou · 2017-02-10T23:52:56Z

Added old servlet back when servlet API for async is not available. Tested old and new servlets in OH2 IDE, including Basic Authentication header. Not tested in an actual OSGi R4.2 container.

Signed-off-by: John Cocula <john@cocula.com>

marcelrv · 2017-02-13T10:38:59Z

@watou Would you mind dropping an already build version of the bundle here in the comment
I can't seem to build it properly.

[INFO] Adding repository http://download.eclipse.org/jetty/updates/jetty-bundles-9.x/9.2.9.v20150224
[WARNING] Target location type 'Directory' is not supported
[INFO] Resolving dependencies of MavenProject: org.eclipse.smarthome.ui:org.eclipse.smarthome.ui:0.9.0-SNAPSHOT @ /home/marcel/oh2/smarthome/bundles/ui/org.eclipse.smarthome.ui/pom.xml
[INFO] {osgi.os=linux, osgi.ws=gtk, org.eclipse.update.install.features=true, osgi.arch=x86}
[ERROR] Cannot resolve project dependencies:
[ERROR]   Software being installed: org.eclipse.smarthome.ui 0.9.0.qualifier
[ERROR]   Missing requirement: org.eclipse.smarthome.ui 0.9.0.qualifier requires 'package org.eclipse.jetty.proxy 0.0.0' but it could not be found
[ERROR]
[ERROR] See http://wiki.eclipse.org/Tycho/Dependency_Resolution_Troubleshooting for help.
[ERROR] Cannot resolve dependencies of MavenProject: org.eclipse.smarthome.ui:org.eclipse.smarthome.ui:0.9.0-SNAPSHOT @ /home/marcel/oh2/smarthome/bundles/ui/org.eclipse.smarthome.ui/pom.xml: See log for details -> [Help 1]

watou · 2017-02-13T10:52:40Z

@marcelrv I would if I could (or knew how)! It eludes me how to build with Maven when something more is needed in the target platform. @maggu2810 (I think) added the targetplatform/third-party directory to allow build and test from the Eclipse IDE, but the README.md there says

This mechanism works for the Eclipse IDE only because Tycho does not support the location types "Directory", "Installation", and "Features".

If anyone knows to how to build PRs like this with mvn, please let me know!

kaikreuzer · 2017-02-13T10:58:11Z

I'll take care of all necessary steps. Consider it done by tomorrow EOB latest!

kaikreuzer

lgtm, just one very minor comment

kaikreuzer · 2017-02-13T14:01:16Z

bundles/ui/org.eclipse.smarthome.ui/META-INF/MANIFEST.MF

@@ -18,6 +18,7 @@ Import-Package: javax.imageio,
 org.eclipse.jetty.client.api,
 org.eclipse.jetty.client.util,
 org.eclipse.jetty.http,
+ org.eclipse.jetty.proxy,


if you add optional=true here, we would be fully backward compatible - i.e. the jetty-proxy bundle does not even have to be present, in which case we would automatically fall back to the old implementation.

@kaikreuzer Just to be clear, are you suggesting that I add optional=true now, and that no other code changes are required for this to work in the way you described? Once org.eclipse.jetty.proxy is in the TP (which I think you are going to do), is optional=true needed?

We need to have it in the TP in any case, otherwise the code won't compile, so I will also have to add it to the OH2 IDE TP.
The "optional=true" is relevant at runtime: with it, the smarthome.ui bundle can be resolved and used even if jetty-proxy bundle is NOT available (and I guess on a system which only has servlet 2.4, it simply CANNOT be available anyhow).

Added optional resolution of org.eclipse.jetty.proxy

perfect, thanks.

maggu2810 · 2017-02-13T14:39:18Z

It eludes me how to build with Maven when something more is needed in the target platform.

You could use a modified target platform that contains the bundles you need (that will work for the IDE and for Maven).
You could use the Eclipse IDE to export the plugin as a jar, so no need to use Maven at all to get a jar.
As Tycho is configured to consider pom dependencies you can add a dependency using the correct GAVs to the respective pom temporary
Last but not least you can wait until someone (in this case Kai) changed the upstream target platform.

watou · 2017-02-13T17:46:58Z

@maggu2810 Thank you very much for the information about alternatives. I will find that very useful in the future.

…that cannot support it. Signed-off-by: John Cocula <john@cocula.com>

watou force-pushed the proxy-servlet branch 5 times, most recently from 941a904 to 9fcbd53 Compare January 7, 2017 12:35

watou changed the title ~~[WIP] Use Jetty's ProxyServlet implementation~~ Use Jetty's ProxyServlet implementation Jan 7, 2017

Use Jetty's ProxyServlet implementation

597fde5

Made configurable via OSGi Signed-off-by: John Cocula <john@cocula.com>

watou force-pushed the proxy-servlet branch from 9fcbd53 to 597fde5 Compare January 8, 2017 13:05

This was referenced Jan 17, 2017

Sonos binding: album cover art #2838

Merged

Onkyo Binding refactoring openhab/openhab-addons#1708

Merged

watou mentioned this pull request Jan 29, 2017

Chromecast: the play and say console commands not working openhab/openhab-addons#1784

Closed

kaikreuzer reviewed Feb 7, 2017

View reviewed changes

Send specific errors back to client instead of logging them on the se…

019324b

…rver.

Register Jetty async proxy servlet only if required servlet API is av…

3ce9d4d

…ailable; otherwise register older blocking servlet, but maintain identical external usage. Signed-off-by: John Cocula <john@cocula.com>

Rename impl classes to indicate intended differences

eb93811

Signed-off-by: John Cocula <john@cocula.com>

kaikreuzer reviewed Feb 13, 2017

View reviewed changes

Added optional resolution of org.eclipse.jetty.proxy package for TPs …

23485e6

…that cannot support it. Signed-off-by: John Cocula <john@cocula.com>

kaikreuzer approved these changes Feb 13, 2017

View reviewed changes

kaikreuzer merged commit bf55397 into eclipse-archived:master Feb 13, 2017

kaikreuzer mentioned this pull request Feb 13, 2017

upgraded to new dependency version which includes Jetty Proxy openhab/openhab-distro#422

Merged

kgoderis mentioned this pull request Mar 28, 2017

Proxy time-outs #3184

Closed

kaikreuzer modified the milestone: 0.9.0 Jun 26, 2017

kirantpatil mentioned this pull request Oct 10, 2017

[Feature Request] Accessing local addresses from external location openhab/org.openhab.ui.habpanel#231

Open

watou deleted the proxy-servlet branch October 1, 2018 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Jetty's ProxyServlet implementation #2753

Use Jetty's ProxyServlet implementation #2753

watou commented Jan 5, 2017 •

edited

Loading

kaikreuzer left a comment

kaikreuzer Feb 7, 2017

watou commented Feb 7, 2017

watou commented Feb 8, 2017

kaikreuzer commented Feb 9, 2017

watou commented Feb 9, 2017

watou commented Feb 10, 2017

marcelrv commented Feb 13, 2017

watou commented Feb 13, 2017

kaikreuzer commented Feb 13, 2017

kaikreuzer left a comment

kaikreuzer Feb 13, 2017

watou Feb 13, 2017

kaikreuzer Feb 13, 2017

watou Feb 13, 2017

kaikreuzer Feb 13, 2017

maggu2810 commented Feb 13, 2017

watou commented Feb 13, 2017

Use Jetty's ProxyServlet implementation #2753

Use Jetty's ProxyServlet implementation #2753

Conversation

watou commented Jan 5, 2017 • edited Loading

kaikreuzer left a comment

Choose a reason for hiding this comment

kaikreuzer Feb 7, 2017

Choose a reason for hiding this comment

watou commented Feb 7, 2017

watou commented Feb 8, 2017

kaikreuzer commented Feb 9, 2017

watou commented Feb 9, 2017

watou commented Feb 10, 2017

marcelrv commented Feb 13, 2017

watou commented Feb 13, 2017

kaikreuzer commented Feb 13, 2017

kaikreuzer left a comment

Choose a reason for hiding this comment

kaikreuzer Feb 13, 2017

Choose a reason for hiding this comment

watou Feb 13, 2017

Choose a reason for hiding this comment

kaikreuzer Feb 13, 2017

Choose a reason for hiding this comment

watou Feb 13, 2017

Choose a reason for hiding this comment

kaikreuzer Feb 13, 2017

Choose a reason for hiding this comment

maggu2810 commented Feb 13, 2017

watou commented Feb 13, 2017

watou commented Jan 5, 2017 •

edited

Loading