response.setStatus(...) causes C# client to respond with invalid token

[yamvcgz]

In waffle/servlet/spi/NegotiateSecurityFilterProvider.java, the following code snipet is the same in both 1.3 and 1.5, and the call to "response.setStatus(...)" in the code never works with C# client (only works with web browsers). It always causes the next token sent back from the C# client to be invalid (why?):
if (securityContext.getContinue() || ntlmPost) {
response.setHeader("Connection", "keep-alive");
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); //<-------- setStatus( ) cause C# client to respond with invalid token -----
response.flushBuffer();
return null;
}

However, in 1.3, there was an additional package "waffle.apache" and I was using waffle/apache/NegotiateAuthenticator.java instead (but it is no longer available in 1.5). C# client works fine with NegotiateAuthenticator.java because sendError() is called, not setStatus():

            if (securityContext.getContinue() || ntlmPost) {
                response.setHeader("Connection", "keep-alive");
                response.sendError(HttpServletResponse.SC_UNAUTHORIZED); //<------ C# client responds correctly to sendError() -------
                response.flushBuffer();
                return false;
            }

After I changed waffle/servlet/spi/NegotiateSecurityFilterProvider.java to call sendError(), the problem is gone. I think any C# client can reproduce the problem.

[yamvcgz]

More test revealed that what really matters is calling setContentLength(). sendError() only happens to set content length when my Tomcat server returns an error page. So the fix is to add the following after setStatus():

if (securityContext.isContinue() || ntlmPost) {

        response.setHeader("Connection", "keep-alive");
        response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);

/******** fix start _/
String data = "dummy";
response.getWriter().print(data);
response.setContentLength(data.length());
/_*** fix end ******/
response.flushBuffer();
return null;
}

[hazendaz]

Is this something that was intended to be moved into master code? I see the code only on branch.

[dblock]

I am not convinced that this is a right fix. I think I was hoping that we could get an actual test for this that would reliably say "it works with a C# client" before merging, that would fail without this code. I don't mean a Java test, I mean a piece of C# code that runs against a test server. Want to give it a shot?

[hazendaz]

@dblock After looking at this again and rebasing against our master, something occurred to me. The javadoc of setStatus which we have in a few places (spi, shiro, and spring security) is stated to only be used with valid non error responses. It states that attempting to use it with errors will not invoke the error page logic as defined in web.xml. That would in turn affect the situation this reported stated. The client usage should be irrelavent.

Now going a bit further, after looking at what was stated, it's clear that keeping setStatus or even changing tomcat ones as the POC did for this is not correct. That simply treats all as success then tries to write to the print writer the error which is probably bad for business anyways (at least I don't think we should be doing so). So I think the real fix is simply correcting to use sendError anytime we are physically stating the connection is Unauthorized as I stated in those three areas. The reporter did state that tomcat was fine with sendError. So that further supports that is the fix. What I think the reporter was missing was the fact that it was not getting content length but that occurs if the error page is entirely skipped (I'm guessing a bit but that seems logical answer and hense why sendError worked for reporter on apache side).

Now as another note, when I rebased against master, all tomcat tests in this area start failing as it forces character encoding internally now requiring coyote response and without some additional changes to testing which now smells funny, the fix as is won't work from build perspective and probably worse from runtime (untested).

OK - aside from all this, I think these classes need to use sendError instead of setStatus.

Source\JNA\waffle-jna\src\mainjava\waffle\servlet\spi\NegotiateSecurityFilterProvider.java
Source\JNA\waffle-shiro\src\main\java\waffle\shiro\negotiate\NegotiateAuthenticationFilter.java
Source\JNA\waffle-spring-security4\java\waffle\spring\NegotiateSecurityFilterEntryPoint.java

Then I think we can get rid of setStatus out of all our mocks. Basically this is all theory at this point but wanted to get your opinion if I happen to be onto something here.

For reference, the core code changes would all essentially be only this.

response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);  -> old
response.sendError(HttpServletResponse.SC_UNAUTHORIZED); -> new

Please chime in on this in case I'm totally off base. I'd hate to go down this path if it's not correct. In the end I'd like to get rif of our temporary branch for this. In the meantime, I'll push up what you had originally fixed rebased along with separate commit with what I'm thinking.