On 11/02/2016 12:34, Rémy Maucherat wrote: > 2016-02-11 12:59 GMT+01:00 Mark Thomas <ma...@apache.org>: > >> JASPIC requires that an authentication module can wrap the >> HttpServletRequest and/or HttpServletResponse passed to it and that the >> wrapped instances are passed to the application. This creates a problem >> since the authenticators are implemented as Valves which pass Tomcat's >> o.a.c.connector.[Request|Response] objects. Tomcat's objects implement >> the Servlet spec interfaces so, as long as no wrapping occurs, all is fine. >> > > Tomcat could simply use the already wrapped request/response later when > invoking the filter chain. It would be set on the MessageInfo, right ? In > that case, JASPIC should get Request.getRequest() and > Response.getResponse() as its request/response objects and the wrapped > request/response can go into two new fields in Request/Response. Would that > work ?
That could work yes. I did think of that option but I rejected as I was concerned about having two versions of the request and response. On reflection, the end result is essentially the same for this option or either of options 3 or 4 so maybe this is the way to go. > Honestly, whatever JASPIC would do with its wrapping is likely irrelevant > for the rest of the Cataline pipeline, so as long as the application sees > it it should be fine. > > There is a long standing enhancement request [1] for being able to use >> wrapping with Valves. Rémy is -1 on implementing it. >> >> I'd still like to support JASPIC but I do understand where Rémy is >> coming from w.r.t. the risks of wrapping. With all that in mind I have >> been thinking through different implementation options and I have >> reached the point where I think it makes sense to set out the options as >> I see them for discussion. >> >> 1. Give up on JASPIC. >> Pros: All the design / integration issues go away >> No performance risk >> Cons: We lose the opportunity for a simple OAuth / SAML solution >> >> 2. Don't support wrapping with JASPIC. >> Pros: All the design / integration issues go away >> Cons: Not specification compliant >> We don't know how much implementations rely on this >> >> 3. Switch the Valve interface to use HttpServletRequest and >> HttpServletResponse. >> Pros: Enables the use of the associated Wrapper classes >> Cons: Valves need access to the internals. ValveBase could >> provide utility methods for accessing the wrapped >> o.a.c.connector.[Request|Response] implementations >> Breaks existing custom Valves >> >> 4. Make o.a.c.connector.[Request|Response] interfaces and provide >> separate concrete implementations. >> Pros: No change to Valve interface >> Cons: JASPIC would need additional code to bridge between these >> interfaces and the HttpServlet.*Wrapper classes >> >> 5. Move JASPIC processing to the start of the FilterChain since it >> uses HttpServlet[Request|Response] >> Pros: No / minimal API changes for Tomcat >> Cons: Would result in authentication happening in multiple places. >> Would need to be very careful to ensure requests couldn't >> bypass authentication, particularly during JASPIC provider >> (de)registratiob >> JASPIC would end up duplciating a lot of the authorization >> code in AuthenticatorBase >> >> >> Of all of these, I think 4 holds the most promise. The first step for >> that option would be reducing the current public interface of >> o.a.c.connector.[Request|Response] to the minimum that is required. From >> a design point of view that is a good thing to do anyway so I plan to >> work on that while these options are discussed. Even if we don't go for >> option 4, the work would be entirely pointless. >> >> If we do follow option 4 then the Javadoc will need updating to make it >> very clear what is supported and what isn't. Generally, I think the >> message needs to be "If you wrap these you are messing with Tomcat's >> internals. Tread very, very carefully. Correct operation of Tomcat >> depends on the correct behaviour of these methods. Before providing an >> alternative implementation of any method you should review the standard >> implementation and how the method is used." >> >> Mark >> >> >> [1] https://bz.apache.org/bugzilla/show_bug.cgi?id=45014 >> >> On the other proposed solutions: > 1) Well, we did so much with this junk, it would be a problem to give up. > 2) It should be properly supported. > 3) Meh. OTOH it's probably just some unwrapping. > 4) I think 3 is better. > 5) Servlet 3 did a lot to move some auth to the application layer, so if > there's an auth step at this point, I don't think it is so horrible > conceptually. > > So maybe I like 3 better if my comment at the beginning cannot work. Breaking all the existing Valves was my main concern with 3. I think your earlier comment can work so I'll go that route for now. Thanks for the review. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
