Sustainsys / Saml2

Saml2 Authentication services for ASP.NET
Other
961 stars 604 forks source link

Content-Length: 0 with Owin middleware #295

Closed Peperud closed 9 years ago

Peperud commented 9 years ago

Post binding works with IdentityServer3 v1.6.2, but doesn't with the currently released v2.0. Response comes with no body and response header "Content-Length: 0"

AndersAbel commented 9 years ago

Please see #259 too, which contains a proposed fix for this issue.

AndersAbel commented 9 years ago

This has been reported before, in the form of a pull request, but let's keep this issue open as I prefer to have an issue that is separate from the PR.

I've been looking into this. I've tested by running the sample owin application (on IIS Express) and downloading the metadata. Using Fiddler I can see that a Content-Length header with a correct value is sent. I've also updated all the Katana (Microsoft.Owin.*) packages to 3.0.1 and I still see a correct Content-Length

I would like some more information on the environment you are using, to be able to reproduce this. Especially I'm interested in how you're hosting the application - IIS Express? IIS? Owin Self Host?

Having to set Content-Length manually just feels wrong. I think it should be handled by the framework, so before doing any updates I want to be sure that we really have to.

Peperud commented 9 years ago

System that works - Windows 8.1, IIS hosted, IdSrv3 v1.6.x
System that does not - Windows 2012 R2, IIS hosted, IdSrv3 v2

I agree, setting the context length explicitly is asking for trouble.

On the system that works, the response comes with Transfer-Encoding: chunked (and of course no Content-Length).

JoeBrockhaus commented 9 years ago

I'm guessing the fixation on the ContentLength being set was simply a red herring from the extra writer being used, perhaps causing the framework to set it when it was producing the issue. I agree setting that length was a tad fishy, but I wasn't quite sure what the intent with the original extra writer, using UTF8, was supposed to be, so wanted to make it as consistent as possible.

I've just tested the change on our previously-affected code and it seems everything is working fine. FWIW, ContentLength IS being sent in the Response, and it matches the actual length of that content received by the browser.

As for the environment we experienced the issue in, we were hosting in IIS on 8.1 at the time. And I believe we also tested hosting in IISExpress at the time (might've been the thing that prompted us to switch to IIS to troubleshoot). We've since switched back to using Express, which is how I just tested this.

AndersAbel commented 9 years ago

@Peperud Can you try with a build of the current master, which contains the updated Owin writing (without a separate writer) and see if all works with that?

If it works, I suggest that we close this issue as fixed.

Peperud commented 9 years ago

@AndersAbel Issue is still there with the current master (2012 R2, IIS, IdSrv3 v2). Explicitly setting the content-length header value (in Kentor.AuthServices.Owin/CommandResultExtensions/Apply) makes it work.

@JoeBrockhaus The 3 bytes you mention in #259 are most likely UTF-8 BOM being emitted by the StreamWriter used in the previous code. .

AndersAbel commented 9 years ago

I ask a Q on Stack Overflow on what the correct way is to handle this, hope I'll get an authoritative answer. http://stackoverflow.com/questions/32154294/content-length-0-with-owin

AndersAbel commented 9 years ago

I got an answer on Stack Overflow from @tratcher aka Mr Katana. We shouldn't have to set Content-Length.

@Peperud When you see Content-Length: 0, is the content still present in the body? Would be great if you could copy the actual, raw response from Fiddler and add it as a comment here. That would make it easier to find out if this is a Katana bug or something else.

albinsunnanbo commented 9 years ago

Random thought: Is it possible to figure out when the Content-Length header is set? Could you insert inspection/debugging middlewares before and after Authservices and see if it is set anywhere? Most likely it should not be set at all during the owin part, but later by IIS. If it is set during the owin middleware execution it would be a clue.

Peperud commented 9 years ago

@AndersAbel Yes, there's content in the body and Fiddler rightfully shows a warning: content-length-zero

Here's the raw response:

HTTP/1.1 200 OK
Cache-Control: private
Content-Length: 0
Content-Type: text/html
Server: Microsoft-IIS/8.5
X-AspNet-Version: 4.0.30319
X-Powered-By: ASP.NET
Date: Sun, 23 Aug 2015 21:14:15 GMT

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
<body onload="document.forms[0].submit()">
<noscript>
<p>
<strong>Note:</strong> Since your browser does not support JavaScript, 
you must press the Continue button once to proceed.
</p>
</noscript>
<form action="https://sso.connect.pingidentity.com/sso/idp/SSO.saml2?idpid=5a07d61f-4afe-43a0-857a-3e65568458ae" 
method="post">
<div>
<input type="hidden" name="SAMLRequest" 
value="PHNhbWwycDpBdXRoblJlcXVlc3QgeG1sbnM6c2FtbDJwPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6cHJvdG9jb2wiIHhtbG5zOnNhbWwyPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXNzZXJ0aW9uIiBJRD0iaWQyZTBmMzM5ZTZjNzg0MTNhOWNhZDU4MjQ3MTYzYzNjNSIgVmVyc2lvbj0iMi4wIiBJc3N1ZUluc3RhbnQ9IjIwMTUtMDgtMjNUMjE6MTQ6MTVaIiBEZXN0aW5hdGlvbj0iaHR0cHM6Ly9zc28uY29ubmVjdC5waW5naWRlbnRpdHkuY29tL3Nzby9pZHAvU1NPLnNhbWwyP2lkcGlkPTVhMDdkNjFmLTRhZmUtNDNhMC04NTdhLTNlNjU1Njg0NThhZSIgQXNzZXJ0aW9uQ29uc3VtZXJTZXJ2aWNlVVJMPSJodHRwczovL2l2YW4tdGVzdDEueXNpLnlhcmRpLmNvbS95QXV0aC9pZGVudGl0eS9BdXRoU2VydmljZXMvQWNzIj4NCiAgPHNhbWwyOklzc3Vlcj5odHRwOi8vaXZhbi10ZXN0MS55c2kueWFyZGkuY29tL3lBdXRoL3RydXN0PC9zYW1sMjpJc3N1ZXI+DQo8L3NhbWwycDpBdXRoblJlcXVlc3Q+"/>
</div>
<noscript>
<div>
<input type="submit" value="Continue"/>
</div>
</noscript>
</form>
</body>
</html>

Browsers show no body content, but that's understandable if they stop the moment they parse the zero content-length header.

AndersAbel commented 9 years ago

Thanks @Peperud,

With this info, and the answer on Stack Overflow, I have a suggestion:

  1. We'll add the explicit setting of the Content-Length header, as originally suggested by @JoeBrockhaus to work around the problem.
  2. We'll file a bug on codeplex with the fiddler trace above (ping: @tratcher).
Tratcher commented 9 years ago

FYI: I'm pretty sure this isn't coming from Katana, it doesn't manage content-length.

AndersAbel commented 9 years ago

@Tratcher Thanks for that clarification. Looks like we'll have to settle with setting the content-length manually then.

Peperud commented 9 years ago

Thanks @Tratcher, I guess the real question - who is responsible for managing the content-length header? Glancing at the Katana source (I might have missed something, but), the only authentication middleware that writes directly to the response appears to be the Oath one and then it does set the content-length. What are the rules for writing to the response body from an authentication handler?

@AndersAbel Noticed something when attached a debugger to Kentor middleware :

  1. “KentorAuthServicesAuthenticationHandler.ApplyResponseChallengeAsync” is called with 401 challenge.
  2. As soon as the “Apply” extension method writes to the response (and before the Apply method actually completes), the “ApplyResponseChallengeAsync” is called again (it doesn't execute the body, because the status now is the newly set 200, but it’s called).
  3. Then “Apply” completes and “ApplyResponseChallengeAsync” is called yet again (status 200).

This could be completely normal...

I'm not familiar with the way the response stream is wired up by Katana from IIS to the middleware, but could it be that the it somehow affects the buffering, potentially preventing a "gatekeeper" at the end to inspect the stream and set/replace the content-length?

Tratcher commented 9 years ago

Oh yikes, you're trying to write to the response body from an auth middleware? That explains a few things. Rule of thumb: middleware should write to the response body OR call the next middleware, never do both.

AndersAbel commented 9 years ago

@Tratcher When we're writing to the body, we do not call the next middleware.

@Peperud The call series you list surely looks strange, we should look into it.

Peperud commented 9 years ago

@AndersAbel Looks like by the time the 401 response is processed in Kentor's auth handler, there's already a zero Content-length in the headers. From this point there are two things that I can do to make it work: Either

  1. Calculate and set actual content length. Or
  2. Remove the content-length header from the response.

content-length-zero-set-already

content-length-zero-set-already2

I had breakpoints on all of the IdSrv3 entries in the above call stack and none of them had the content-length set.

@Tratcher I'm not entirely sure if the [External Code] entry in the stack before the auth handler is only Katana (what else?), but perhaps it is not entirely unreasonable to speculate at this point that the content-length did get changed somehow in Katana before it called the auth handler override?

albinsunnanbo commented 9 years ago

@Peperud you can right click in the call stack window and select "Show External Code" to expand the external code if you want to be sure.

Peperud commented 9 years ago

@albinsunnanbo Good call :+1: There it is. content-length-zero-set-already3

AndersAbel commented 9 years ago

@Peperud If removing content-length works, I think we should use that as the fix.

@Tratcher I was a bit fast when replying earlier. The middleware IS indeed writing a body, after having called next. It is a very specific case: After returning from the call to next, if there is a AuthenticationChallengeRequest and our middleware is configured to use the SAML2P POST binding a body is written.

Tratcher commented 9 years ago

@AndersAbel Yeah, that's not supported by the authentication middleware infrastructure. It causes all sorts of problems as you have discovered.

AndersAbel commented 9 years ago

@Tratcher I see. I assume that means that if we can handle the AuthenticationChallengeRequest by creating a redirect that is fine, but if we need to write to the body we should first perform an extra redirect.

In the code, that means we'd have to check if there is a body generated in the CommandResult and in that case redirect to ~/AuthServices/SignIn instead of applying the command result to the response. When doing that we would have to make sure that any authentication properties are properly preserved.

AndersAbel commented 9 years ago

I've made the proposed fix by removing Content-Length if present in PR #318. I can't really test it though as I've not experienced the problem msyelf. @Peperud and @JoeBrockhaus would you be able to verify if this fix works on the systems where you've experienced the errors?

Peperud commented 9 years ago

@AndersAbel Doesn't compile right now. Will test later.

albinsunnanbo commented 9 years ago

You need C#6 / VS2015 to compile. See #314

akiander commented 9 years ago

I am hitting this issue on a Windows Server 2012 R2 web server running under IIS. The issue does not occur when I run locally on my Windows 7 laptop via Visual Studio and IIS Express.

I tried pull request 318 but it did not solve the issue.

I did not know this issue was already in progress so I opened a separate issue but will close now that since this is exactly the issue I'm facing.

I'm open to any and all suggestions on how to proceed.

akiander commented 9 years ago

I have this working by reversing the order inside the Apply method on CommandResultExtensions.

If I first call this:

 context.Response.ContentLength = null;

and then call this:

 context.Response.Write(commandResult.Content);

then it works. But the pull request above has it the other way around and it doesn't work for me. I went back and forth enough times that I'm fairly convinced that should be the fix.

Peperud commented 9 years ago

I can confirm - it does not work the way it is in the pull request. Reversing the order makes it work. This is how I had it locally before and it works with the recent sources too.

            if (commandResult.Content != null)
            {
                if (context.Response.ContentLength.HasValue)
                {
                    // Remove value set by other middleware and let the host calculate
                    // a new value. See issue #295 for discussion on this.
                    context.Response.ContentLength = null;
                }
                context.Response.Write(commandResult.Content);
            }
JoeBrockhaus commented 9 years ago

Indeed, ContentLength must be set before the response is written to. This was something we learned early on in our investigation. If you debug this, you'll see the client receives the response immediately after .Write(commandResult.Content)

AndersAbel commented 9 years ago

I switched the order and now reset ContentLength first. I also added tests to ensure that nobody switches order again without noticing.

The PR #318 is updated with the new code. Is it now ready to be merged?

akiander commented 9 years ago

The PR #318 is ready. Thanks for addressing this, I think it will be helpful to have this in place going forward.