On Jan 28, 2008 4:40 AM, Daniel Stenberg <daniel_at_haxx.se> wrote:
> On Sun, 27 Jan 2008, Gautam Kachroo wrote:
>
> >> The question is of course what to do about this situation. Do we stop
> >> allowing the method used in test 509 or do we somehow allow it and make
> >> sure that the re-used connection's SSL connection is continued to the
> >> complete state? Or is there any other alternatives?
> >
> > I got the test to work by turning on forking in sws. The problem when sws
> > doesn't fork is that the server is blocked waiting for a request on the
> > first connection. Of course, there is never a request on that connection
> > since the client has initiated a second connection. When connection reuse is
> > forbidden, the first connection is closed which frees up the server for the
> > second request. Otherwise, before this patch, the connection is reused which
> > doesn't cause the server to be hung. With this patch, the first connection
> > lingers keeping the (non-forking) server blocked.
> >
> > I think that means the patch is fine.
>
> I'm torn back and forth on this.
>
> The fact that you need to do a second request for test 509 basically disables
> what test 509 tries to do: a redirect to the correct URL while still
> connected. Because if that's not possible (anymore) to do, the application
> could just as well just figure out the "true" URL in a first request and then
> make a second one on its own. I'm not saying this is entirelly ruled out, but
> it'll practically revert Peter Sylvester's change from January 12th 2004 that
> introduced this ability.

Well, it looks like the prior patch should still work for http urls. I
think it's always been incomplete for https urls, but incomplete in
ways that weren't noticed, see below.
If it's important to re-use the existing connection, then that
connection should be fully initialized when it is re-used, i.e. ensure
that step2 and step3 are called.

> > There may be a concern about keeping a connection to the server lingering,
> > but that's not the worst thing in the world since test 509 seems like a rare
> > case. Of course, Curl_done could close the first connection when it sees
> > that the ssl connection state is not complete. There's no point in having
> > the connection in the connection cache if it willl never be re-used.
>
> Yeah, if there's no code to continue the SSL connection negoation on a
> subsequent re-use, there's no point in keeping it around. However, this makes
> me very puzzled since then how come test 509 works today (without this patch)
> then? Isn't this simply a proof that this patch sets the SSL connection to a
> "complete" state a bit too late and that the connection in 509 should rather
> be seen as negotiated and fine since obviously it can be re-used fine?

I think it's because the connection state is set to ssl_connect_done
in Curl_ossl_connect_step3 which is only used by the curl library.
openssl has finished initializing the connection, therefore the
connection is ready to use for ssl traffic, which is why the test
succeeds before the patch.
But, the code in Curl_ossl_connect_step3 is skipped, i.e. sessionid
not put into sessionid cache, verifyhost not called, cert verification
not checked.
We could mark the connection complete earlier, but then the code in
Curl_ossl_connect_step3 would become "optional" for an SSL connection.

> I'm not prepared to run the sws server in forked mode just yet without further
> investigations that things will work fine with it. The fork option was mostly
> added for when I've done stand-alone controlled tests.
>

I understand, but forked mode is important in this situation since
there are two open connections from the client. The second is the
"real" one, but the server is busy with the first one in non-forked
mode.

thanks,
-gk
Received on 2008-01-28