Well, my point was that we maintain the existing separation between the org.apache.cxf.transport.http and org.apache.cxf.transport.https packages.
Moving getTLSSessionInfo() to the JettySslListenerFactory was just a suggestion as to how to achieve this. But if that's problematic, then lets figure out a different means to the same end. Also there's little point in just maintaining the separation for the Jetty5 code, but not the Jetty6 or servlet code. So I've taken the liberty of modifying your latest patch, to move all the various flavours of getTLSSessionInfo() logic to static methods on the org.apache.cxf.transport.https.SSLUtils class. The Destination impls then all just need a single call out to a new SSLUtils.propogateSecureSession() method. I'm going to go ahead with the commit, but I did notice that you updated the JettyHTTPDestinationTest in http2 but not http. Was this deliberate, i.e. have you further test additions in the pipeline? BTW you're preaching to the converted on the issue of the duplication across http and http2. I've been moaning on this list about the obvious maintenance issues since this was first committed, and I've suggested several times that it be removed from the mainline to a branch. I just noticed that this issue has been made even worse recently, when the servlet transport was decoupled from the JAX-WS module (a good thing in itself), but then duplicated to both http and http2 ... Arghhh! In any case, we do have a long-term plan to sort this out: drop support for Jetty5, move http2 to http, start taking advantage of the non-blocking APIs in Jetty6, and consolidate the duplication across the Jetty- and servlet-based Destinations. Cheers, Eoghan > -----Original Message----- > From: Fred Dushin [mailto:[EMAIL PROTECTED] > Sent: 09 March 2007 16:51 > To: [email protected] > Subject: Re: Accessing Connection-based info from the transport > > This is fairly technical, so folks on the list not interested > in the nitty-gritty need not pay close attention... > > So basically, what you're proposing is to extend the (internally > defined) ServerEngine interface, to, say, yield a reference to a > JettyListenerFactory: > > public interface ServerEngine { > ... > JettyListenerFactory getJettyListenerFactory(); > ... > } > > and then extend the JettyListenerFactory interface to, say, > perform the needed operation: > > public interface JettyListenerFactory { > ... > TLSSessionInfo getTLSSessionInfo(HttpServletRequest request); > ... > } > > The implementation of the SSLJettyListenerFactory will then > implement this operation, but of course it will need none of > it's state to do the work -- it should properly be a static > operation (and PMD should really be instrumented to flag > this), for that reason alone, but of course it can't be, > since it's part of an interface implementation: > > class SSLJettyListenerFactory ... { > ... > public final TLSSessionInfo > getTSLSessionInfo(final HttpServletRequest request) { > final HttpConnection httpCon = req.getHttpConnection(); > if (httpCon != null) { > final Object connection = httpCon.getConnection(); > if (connection == null) { > return null; > } > if (connection instanceof SSLSocket) { > final SSLSocket socket = (SSLSocket) connection; > final SSLSession session = socket.getSession(); > Certificate[] certs = null; > try { > certs = session.getPeerCertificates(); > } catch (final SSLPeerUnverifiedException e) { > // peer has not been verified > } > return new TLSSessionInfo(session.getCipherSuite(), > session, certs); > } > } > return null; > } > } > > And the code in the Destination then reads: > > final TLSSessionInfo sessionInfo = > engine.getJettyListenerFactory().getTSLSessionInfo(req); > if (sessionInfo != null) { > inMessage.put(TLSSessionInfo.class, sessionInfo); > } > > Is the only reason to go through all of this just to avoid > references to JSSE types in the Destination? > > The same strategy is not available to the ServletController, > BTW, so that will need some JSSE specific code in it. And of > course in the > http2 code, it looks like the old mortbay httpRequest is now > a HttpServletRequest, in which case the common code for > accessing the TLSSession should properly be in the base > AbstractHTTPDestination type. I realize http2 is in flux > right now, but is it not to be the new http implementation, > or are we planning on having 2 http transport implementations > for the forseeable future? > > -Fred > > > > > Well the only SSL visibility in the org.cxf.transport.http code is > > simply to retrieve the configured SSL{Client|Server}Policy so as to > > pass these on to the org.cxf.transport.https code, which then > > interprets and applies the SSL policies. > > > > As far as the org.cxf.transport.http code is concerned, the > > SSL{Client|Server}Policy types are completely opaque, and the only > > reason these are even visible in this code was a side-effect of how > > the http-conf schema was defined (i.e. including references > to types > > defined in the security schema). > > > > So I would like to maintain this separation between the > http and https > > code. > > > > Cheers, > > Eoghan > > > > > >> Thanks, > >> -Fred > >> > >> On Mar 9, 2007, at 5:37 AM, Fred Dushin wrote: > >> > >>> A new patch has been uploaded. Unfortunately, Jira does > >> not seem to > >>> allow me to remove the old one. > >>> > >>> Most of the changes Eoghan suggested have been incorporated. In > >>> particular > >>> > >>> * Scratched the idea of a ContextInfo type, since no one > took the > >>> bait > >>> * Added a TLSessionInfo struct (or the best Java has) to > carry TLS > >>> Session data > >>> to the org.apache.cxf.security.transport namespace > (API package) > >>> * Supported in the Jetty and servlet http transports (http > >> and http2) > >>> (We gotta fix that!) > >>> * Refactored Message and Exchange interfaces (and Impls) to now > >>> extend > >>> from a common base type -- not strictly needed, but > definitely a > >>> tidy cleanup > >>> in the API > >>> > >>> Patch is off rev 516352. > >>> > >>> If someone could quickly review and install the patch, > I'd be much > >>> obliged. > >>> > >>> I have no ontological commitments to the proposed changes > -- mostly > >>> just the idea, so please feel free to morph the proposal to > >> the idiom > >>> du jour (As long as I can extract the needed information > out of the > >>> transport!). > >>> > >>> Thanks! > >>> -Fred > >>> > >>> Here's a listing of the changes, as seen from my snapshot: > >>> > >>> 05:20:09 spock:~/src/apache/cxf/cxf-445> svn status > >>> M rt/transports/http/src/main/java/org/apache/cxf/transport/ > >>> http/JettyHTTPDestination.java > >>> M rt/transports/http/src/main/java/org/apache/cxf/transport/ > >>> servlet/ServletController.java > >>> M rt/transports/http2/src/test/java/org/apache/cxf/transport/ > >>> http/JettyHTTPDestinationTest.java > >>> M rt/transports/http2/src/main/java/org/apache/cxf/transport/ > >>> http/JettyHTTPDestination.java > >>> M rt/transports/http2/src/main/java/org/apache/cxf/transport/ > >>> servlet/ServletController.java > >>> M api/src/main/java/org/apache/cxf/message/Exchange.java > >>> M api/src/main/java/org/apache/cxf/message/ExchangeImpl.java > >>> M api/src/main/java/org/apache/cxf/message/Message.java > >>> A api/src/main/java/org/apache/cxf/message/StringMap.java > >>> M api/src/main/java/org/apache/cxf/message/MessageImpl.java > >>> A api/src/main/java/org/apache/cxf/message/StringMapImpl.java > >>> A api/src/main/java/org/apache/cxf/security > >>> A api/src/main/java/org/apache/cxf/security/transport > >>> A api/src/main/java/org/apache/cxf/security/transport/ > >>> TLSSessionInfo.java > >> > >> > > > >
