<< Back to previous view

[GLASSFISH-20286] SAM not able to handle wrapped requests Created: 11/Apr/13  Updated: 03/Apr/14  Resolved: 03/Apr/14

Status: Resolved
Project: glassfish
Component/s: security
Affects Version/s: 4.0_b83
Fix Version/s: 4.0.1

Type: Bug Priority: Major
Reporter: myfear Assignee: Nithya Ramakrishnan
Resolution: Fixed Votes: 4
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

File Attachments: Java Source File HttpRequestWrapper_diff.java     Java Source File RequestFacadeWrapper.java    
Tags: fishcat 4_0_1-review
Participants: arjan tijms, JeffTancill, kithouna, monzillo, myfear, Nithya Ramakrishnan, quang.dang, shreedhar_ganapathy and Tim Quinn

 Description   

Wrapping a HttpResonse in SAM isn't possible.
According to the JASPIC spec it should.

refer to this one:
http://java.net/jira/browse/JASPIC_SPEC-8

  • M


 Comments   
Comment by kithouna [ 11/Apr/13 02:11 PM ]

Without this being fixed it's really hard to use JASPIC for any real scenario.

Comment by kithouna [ 15/Apr/13 10:27 AM ]

If a TCK test for this existed, then it would have to be fixed for 4.0, wouldn't it?

Comment by shreedhar_ganapathy [ 19/Apr/13 05:26 PM ]

Hi Tim
Could you look into this issue and add an evaluation ASAP?

Comment by Tim Quinn [ 19/Apr/13 05:49 PM ]

Shreedhar,

Jeff is aware that I won't be able to work on this. I'm reassigning it back to the security team.

Comment by JeffTancill [ 26/Apr/13 08:23 PM ]

Wrapping of a response is not a critical area of the spec and difficult to make work in all cases. Not germain to most servlet scenarios.

Comment by arjan tijms [ 26/Apr/13 10:42 PM ]

Wrapping of a response is not a critical area of the spec and difficult to make work in all cases. Not germain to most servlet scenarios.

While wrapping the response might be relatively rare indeed, please note that the issue is not only about wrapping the response, but also about wrapping the request. In fact, wrapping the request is the most important scenario.

The very frequently used FORM based authentication from the Servlet spec needs to restore the original request (make the original POST parameters and the original cookies, headers, etc available to the resource to be invoked), after the user has authenticated.

This means that without wrapping the request, as fas as I know, it's not possible to implement this kind of authentication with JASPIC in a portable way.

JBoss for instance uses proprietary API to achieve this effect, see HTTPFormServerAuthModule#restoreRequest

Spring Security wraps the request in order to restore an original request after authentication has taken place, see RequestCacheAwareFilter#doFilter

The description of this issue may have inadvertently put some emphasis on the response wrapping, but the title mentions request wrapping and the linked SPEC issue gives request wrapping as the prime use case.

Comment by quang.dang [ 09/May/13 06:16 PM ]

Arjan, could you please verify if this issue has been fixed with the change in https://java.net/jira/browse/GLASSFISH-20453 ? thanks.

Comment by arjan tijms [ 09/May/13 11:06 PM ]

Arjan, could you please verify if this issue has been fixed with the change in https://java.net/jira/browse/GLASSFISH-20453 ? thanks.

I checked using the code as mentioned in JASPIC_SPEC-8 on GlassFish 4.0 b88. It now gets much further, almost there, but at the very last moment just before invoking the test Servlet it looses the wrapped request again.

Specifically, in RealmAdapter the following code sets a com.sun.web.security.HttpRequestWrapper as a note, which contains both the original request facade (com.sun.enterprise.web.pwc.connector.coyote.PwcCoyoteRequest) and the new request that was put into the map by the SAM:

HttpServletRequest newRequest = (HttpServletRequest) messageInfo.getRequestMessage();
if (newRequest != req) {
    request.setNote(Globals.WRAPPED_REQUEST, new HttpRequestWrapper(request, newRequest));
}

In StandardPipline after the fix made in revision 61790, the following code now correctly retrieves the HttpRequestWrapper that was previously set as a note:

Request req = request;
Response resp = response;
if (chaining) { // (chaining is now true)
    req = getRequest(request); // (req becomes the correct HttpRequestWrapper instance)
    resp = getResponse(request, response);
}
basic.invoke(req, resp);

A bit down the line in org.apache.catalina.core.StandardWrapperValve.invoke(Request, Response), the original request (the one com.sun.enterprise.web.pwc.connector.coyote.PwcCoyoteRequest was encapsulating) is retrieved from the com.sun.web.security.HttpRequestWrapper instance:

RequestFacade hreq = (RequestFacade) request.getRequest(true);

And with that hreq variable, which is the original request and not the one provided by the SAM, the Servlet (or any Filter incase there is one) is called:

if (servlet != null) {
    if (filterChain != null) {
        filterChain.setWrapper(wrapper);
        filterChain.doFilter(hreq, hres);
    } else {
        wrapper.service(hreq, hres, servlet);
    }
}

Incidentally, this hreq will be the same instance that the SAM sees when originally calling messageInfo.getRequestMessage().

Specifically for this case com.sun.web.security.HttpRequestWrapper#getRequest should have returned the instance that came from the SAM, but what it currently does is calling through to the original facade that it is also wrapping:

public ServletRequest getRequest(boolean maskDefaultContextMapping) {
    return httpRequest.getRequest(maskDefaultContextMapping);
}

Since com.sun.web.security.HttpRequestWrapper has package visibility, it think it's safe to assume that it's only used by RealmAdapter.

Comment by arjan tijms [ 17/Aug/13 10:36 PM ]

I've recently created a kind of Acid test for JASPIC at https://github.com/arjantijms/jaspic-capabilities-test

This includes a test for the request/response wrapping where this issue is about. As it appears GlassFish 4 passes all tests that I've created so far, except for the request wrapping one:

     [exec] [INFO] jaspic-capabilities-test .......................... SUCCESS [1.583s]
     [exec] [INFO] jaspic-capabilities-test-common ................... SUCCESS [1.886s]
     [exec] [INFO] jaspic-capabilities-test-basic-authentication ..... SUCCESS [25.576s]
     [exec] [INFO] jaspic-capabilities-test-lifecycle ................ SUCCESS [20.078s]
     [exec] [INFO] jaspic-capabilities-test-wrapping ................. FAILURE [19.948s]
     [exec] [INFO] jaspic-capabilities-test-register-session-simple .. SUCCESS [20.878s]
     [exec] [INFO] jaspic-capabilities-test-ejb-propagation .......... SUCCESS [24.103s]

Interestingly, it DOES passes the response wrapping test, but (as per the code analysis from 09/may) not the request wrapping one:

    [exec] Starting test: testRequestWrapping
     [exec] request isWrapped: null
     [exec] response isWrapped: true
     [exec] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 17.661 sec <<< FAILURE! - in org.omnifaces.jaspictest.WrappingIT
     [exec] testRequestWrapping(org.omnifaces.jaspictest.WrappingIT)  Time elapsed: 0.031 sec  <<< FAILURE!
     [exec] java.lang.AssertionError: Request wrapped by SAM did not arrive in Servlet.
     [exec] 	at org.junit.Assert.fail(Assert.java:88)
     [exec] 	at org.junit.Assert.assertTrue(Assert.java:41)
     [exec] 	at org.omnifaces.jaspictest.WrappingIT.testRequestWrapping(WrappingIT.java:45)

As I look at the GlassFish code again then it really looks like it's just a tiny fix that needs to be done. The wrapped request almost makes it all the way to the Servlet.

Hopefully the tests can make it easier to work on this issue.

Comment by kithouna [ 26/Sep/13 01:08 PM ]

Sorry to ask again, but any updates here? Seems the solution is really close now!

Comment by monzillo [ 25/Oct/13 07:06 PM ]

I had spent some time on this just before the release of the EE7 RI, but at least at that time,
a solution did not seem to be a simple thing. At that time I had sought someone expert in the role of
the RequestFacade as described in StandardWrapperValve (which is the point where a wrapper returned
from a sam is being unwrapped) ...

/*

  • Create a request facade such that if the request was received
  • at the root context, and the root context is mapped to a
  • default-web-module, the default-web-module mapping is masked from
  • the application code to which the request facade is being passed.
  • For example, the request.facade's getContextPath() method will
  • return "/", rather than the context root of the default-web-module,
  • in this case.
    */
    RequestFacade hreq = (RequestFacade) request.getRequest(true);

It would certainly help to get an informed opinion as to whether it is indeed feasible for a wrapped facade to
be passed through the pipeline. (2) I will attach some diffs that do that, and that seem to restore the ability of
a sam to wrap the request, but I am not sure of all the consequences, so that is the reason why we have not checked this in.

(2) Another potential resolutuon would be to delay the point at which the wrapper installed by the sam, is used to replace the facade, such that that occurs, just prior to leaving what I views as the internal extent of the pipeline, and where the servlet filters and application resources begin. this would appear to occur at some point after the above call (in StandardWrapperValve) and perhaps could occur at about the following point:

313: if (servlet != null) {
if (filterChain != null) { filterChain.setWrapper(wrapper); filterChain.doFilter(hreq, hres); } else { wrapper.service(hreq, hres, servlet); }
}

the wrapper (if one is returened) is currently being detected and sent down the pipeline, by the code in StandardPipeline.doInvoke(,,true) where you see the following

728: else if (basic != null) {
Request req = request;
Response resp = response;
if (chaining) { req = getRequest(request); resp = getResponse(request, response); }
basic.invoke(req, resp);
basic.postInvoke(req, resp);
}

Moving the point at which a wrapper returned by the sam would be sent into the pipeline, would of course mean that the wrapper returned by the sam (and this the security effects embodies in the sam) would not be seen within the internal pipeline (other than by valves that know how to access the attribute containing the wrapped request)

I will attach a diff to HttpRequestWrapper and a new file RequestFacadeWrapper that together are an implementation of approach 2.

hopefully there will be folks who can help resolve this.

Comment by monzillo [ 25/Oct/13 07:11 PM ]

changes to implement approach 1 in 4.0 code base (as described in my comment). I also describe a second perhaps simpler approach, based on moving/delaying the point at which the wrapped request would enter the pipeline.

Comment by monzillo [ 25/Oct/13 07:17 PM ]

(2 atachements embody) changes to implement approach 1 (as described in my comment above) in 4.0 code base . In also describe a second perhaps simpler approach (also in my above comment), based on moving/delaying the point at which the wrapped request would be sent down the pipeline.

Comment by Nithya Ramakrishnan [ 03/Apr/14 06:57 PM ]

Committed the changes proposed by Ron - introduce a RequestFacadeWrapper to handle wrapped requests in SAM.

In the https://github.com/arjantijms/jaspic-capabilities-test framework, the jaspic-capabilities-test-wrapping test passes with these changes

Transmitting file data ..
Committed revision 63174.

Generated at Sat Apr 19 10:26:13 UTC 2014 using JIRA 4.0.2#472.