cxf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Glynn, Eoghan" <eoghan.gl...@iona.com>
Subject RE: Accessing Connection-based info from the transport
Date Mon, 05 Mar 2007 15:52:46 GMT


Thanks Fred,

A few points about this patch ...

1. We've aimed to maintain a clean separation between the plaintext HTTP
support in o.a.c.t.http.JettyHTTPDestination and the HTTPS logic in
o.a.c.t.https.JettySslListenerFactory (similarly for
o.a.c.t.http.HTTPConduit and o.a.c.t.https.HttpsURLConnectionFactory).
Can you move your additions to JettyHTTPDestination to
JettySslListenerFactory so as to avoid breaking this?


2. Unfortunately we have duplication of the JettyHTTPDestination code
across the http and http2 modules. This is obviously badness, and we
plan to address it, but in the meantime all changes to http must be
manually reflected in http2 also.

Similarly for the servlet transport, though instead of a straight copy,
in this case you'd be using a different mechanism for retrieving the
cipher suite and peer cert (e.g. for Tomcat, probably the
org.apache.tomcat.util.net.SSLSupport mechanism, dunno about other
containers).


3. Dunno if you've had a chance to look at Java5 annotations, but now
that I see your TLSSessionContract.*_REQUIRED fields I think you can
achieve what you want here, and a lot more, in a much cleaner way with
annotations.

Instead of the concept of a string "contractID", as a textual
representation of what's supported and required in the "contract", why
not just use the native Java mechanism, i.e. an annotated class or
interface?

For example ...

import java.lang.annotation.*; 

@Retention(RetentionPolicy.RUNTIME)
public @interface Required {
}


import java.security.cert.Certificate;

public interface TLSSessionContext {

    Certificate[] getPeerCertificateChain();

    Certificate getTrustPoint();

    @Required
    String getChiperSuite();
}


Now here's the pay-off ... in the ContextInfo.put(), you could *enforce*
the requirement for a CipherSuite in a very generic way ... of the top
of my head something like:

public class ContextInfo {
  public <T> T put(String key, T value) {
      Method[] methods =  value.getClass().getMethods()
      for (Method m : methods) {
         if (m.getName().startsWith("get")
             && m.isAnnotationPresent(Required.class)) {
             try {
                 if (m.invoke(value, null) == null) {
                     throw new MissingRequiredContext("non-null " +
m.getName() + " required");
                 }
             } catch (InvocationTargetException ite) {
                 // and so on for other exceptions ...
             }
         }
      }
      return value.getClass().cast(map.put(key, value));
  }

  //...
}


4. I see you use the GNU C++ style of:

   returnType
   methodName();

as opposed to the more normal Java style:

   returnType methodName();

It doesn't break the checkstyle/PMD targets, but in the interests of
consistency with 20-ish KLOCs already in the CXF repo, why not just use
the Java style? Especially when changing a pre-existing source file. 

Another minor stylistic point ... we tend to use import as opposed to
fully-qualify classes (i.e. "import java.security.cert.Certificate; ...
public static final Pair<String, Class<Certificate>> TRUST_POINT" as
opposed to "public static final Pair<java.lang.String,
Class<java.security.cert.Certificate>> TRUST_POINT").

BTW, I've absolutely no interest in a my-style-is-better-than-yours
flame war. I've seen far too much time burned on that in other projects.
So I really don't care what style is used, as long as its consistent
through-out the source tree.

Cheers,
Eoghan


> -----Original Message-----
> From: Fred Dushin [mailto:fred@dushin.net] 
> Sent: 05 March 2007 14:23
> To: cxf-dev@incubator.apache.org
> Subject: Re: Accessing Connection-based info from the transport
> 
> Sorry -- under CXF-445.
> 
> https://issues.apache.org/jira/browse/CXF-445
> 
> On Mar 5, 2007, at 7:38 AM, Glynn, Eoghan wrote:
> 
> >
> > Fred,
> >
> > Where did you submit the patch to?
> >
> > /Eoghan
> >
> >> -----Original Message-----
> >> From: Fred Dushin [mailto:fred@dushin.net]
> >> Sent: 05 March 2007 11:30
> >> To: cxf-dev@incubator.apache.org
> >> Subject: Re: Accessing Connection-based info from the transport
> >>
> >> Thanks, Eoghan.
> >>
> >> I can't really speak to the IP Address issue you raise -- 
> right now 
> >> what I'm really concerned about is TLS-related 
> information, which in 
> >> the case of the Jetty implementation, is available on off the 
> >> SSLSession.  The local and peer IP addresses are available for the 
> >> asking there, too, but currently I have no need for them 
> (though I'd 
> >> certainly be open to exposing this information to anyone 
> downstream).
> >>
> >> As for the aesthetic issue about whether it's easier to 
> design a new 
> >> struct to carry this information, for each type of 
> information, I've 
> >> submitted a patch which I think fairly effectively avoids 
> having to 
> >> do this, and I'd argue it's no less usable or readable on 
> the part of 
> >> the producer or consumer of this information.  You'll see patterns 
> >> like
> >>
> >> ContextInfo info = message.get(TLSSessionContract.class);
> >> java.security.cert.Certificate[] peerCerts = info.get 
> >> (TLSSessionContract.PEER_CERTIFICATES);
> >>
> >> and off you go.  This is in contrast to something like:
> >>
> >> TLSSessionInfo info = message.get(TLSSessionInfo.class);
> >> java.security.cert.Certificate[] peerCerts = 
> >> info.getPeerCertificates();
> >>
> >> but it has the advantage that it's slightly more future-proofed.
> >> E.g., in the latter case, if TLSSessionInfo changes, then 
> >> implementations will fail to compile.  And of course the type
> >> (ContextInfo) has general applicability outside of my 
> myopic needs at 
> >> present.
> >>
> >> Of course, I'm not a committer here, so feel free to ignore the 
> >> patch.  If this forum seriously feels the former approach is a 
> >> detriment to the project, I can easily retrofit the 
> approach, but I 
> >> think that would be unfortunate.  The former approach is 
> really a lot 
> >> simpler and cleaner, IMO.
> >>
> >> Please let me know ASAP.
> >>
> >> Thanks,
> >> -Fred
> >>
> >
> 
> 

Mime
View raw message