Closed Bug 528288 (SPDY) Opened 15 years ago Closed 12 years ago

Implement SPDY protocol

Categories

(Core :: Networking, enhancement)

Other Branch
x86
All
enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla11

People

(Reporter: beltzner, Assigned: mcmanus)

References

()

Details

(Whiteboard: [qa!])

Attachments

(21 files, 20 obsolete files)

1.30 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
3.19 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
1.00 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
22.18 KB, patch
mayhemer
: review+
briansmith
: review-
Details | Diff | Splinter Review
14.84 KB, patch
mayhemer
: review+
Biesinger
: superreview+
Details | Diff | Splinter Review
6.44 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
12.90 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
2.30 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
54.85 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
5.26 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
13.32 KB, patch
briansmith
: review-
mcmanus
: superreview+
Details | Diff | Splinter Review
7.99 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
8.54 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
4.14 KB, patch
mcmanus
: review+
mcmanus
: superreview+
Details | Diff | Splinter Review
210.68 KB, patch
mcmanus
: review+
mcmanus
: superreview+
Details | Diff | Splinter Review
1.47 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
16.22 KB, patch
mcmanus
: review+
mcmanus
: superreview+
Details | Diff | Splinter Review
5.89 KB, patch
briansmith
: review+
mayhemer
: superreview+
Details | Diff | Splinter Review
18.15 KB, patch
Details | Diff | Splinter Review
3.90 KB, patch
Details | Diff | Splinter Review
195.88 KB, patch
briansmith
: review+
mcmanus
: review+
mcmanus
: superreview+
Details | Diff | Splinter Review
Should we decide to implement the SPDY protocol created by Google's research group, we will need a bug to track it. Let this be that bug.
Severity: normal → enhancement
Target Milestone: --- → Future
I see Google are now using this live:

"Yes, indeed SPDY is enabled in Chrome and on Google servers for all SSL
traffic at this point.  (Actually, we do 90% on SPDY, with a 10% holdback
for purposes of A/B comparisons). "

http://groups.google.com/group/spdy-dev/msg/dcc9c3a3e50c694f
If Facebook goes both HTTPS only, then at least 1/3rd of US pageviews would be served over HTTPS. SPDY has data to show that it helps.
Last I heard from Google it wasn't "ready for others to implement", but that was a few months ago.  I'll ping again.
The basic ideas in SPDY are great, but it could be better in some details -- it would make sense for the Mozilla community to review and fix these, in conjunction with Google and other interested parties, before creating what will become a de-facto standard.
 
For example, the apparently mandatory use of gzip compression everywhere, unless requested by the client, in the current draft of SPDY seems to me to be less than optimal. Where data has already been efficiently compressed (images, audio, video), it looks sufficiently random that trying to compress it would not only waste CPU resources, it will also wash out any useful cached state in the compressor's history that might be used for compressing other data.
And: given the de-facto standardization that will occur once a critical mass of browsers have implemented a well-defined version of SPDY, it should probably be put into the standards process as a candidate for an official HTTP 2.0.
Google Chrome now supports SPDY and SPDY has been rolled out to all of their web services according to this article:

http://downloadsquad.switched.com/2011/04/11/google-chrome-now-uses-spdy-http-replacement-halves-page-load-t/
Can we do something similar or something faster?
There's no point doing something "similar" as there's web-side work that needs to be done to support it. Google properties are a big win, perhaps big enough to warrant the investment in supporting the protocol, but better still would be knowing if other websites are willing to make the investments in their code that would be required to make it work.
Isn't it a kind of chicken and egg problem? The incentive of websites to invest in supporting the protocol would increase if two major browsers supported the protocol, instead of one.
Maybe a press release or a shout out is needed to figure out what sites are willing to support it?
Yes, it's a chicken and egg problem, as it is with many of the other technologies supported. I was merely responding to the proposal to "do something similar".

It's up to product drivers to decide how much they want to push this protocol forward and determine if web authors are willing to invest. I suggest that this bug isn't the best place for that sort of discussion: please take it to either the dev-planning or dev-platform newsgroups if you want to lead a campaign to try and determine how many websites would be interested. Let's leave this bug for discussion of implementation.
I don't expect we'll see much adoption if there is no good webserver support or atleast good reverse proxy (loadbalancer-like).

Atleast looking at the node.js proxy example didn't seem that complicated:

https://gist.github.com/911761
(In reply to comment #9)
> There's no point doing something "similar" as there's web-side work that needs
> to be done to support it. Google properties are a big win, perhaps big enough
> to warrant the investment in supporting the protocol, but better still would be
> knowing if other websites are willing to make the investments in their code
> that would be required to make it work.

It appears Google is putting some effort into a mod_spdy Apache module[1].  I don't see anything for nginx or lighttpd or IIS (not even sure IIS has the facilities to add something like that).  Apache is ~60% of the market.

I don't think it's up to websites as much as it's up to httpd vendors to offer support.  Apache takes care of ~60% of the web if that becomes stable.  As I understand it SSL cert would still be required, though not sure about a dedicated IP.

It is chicken/egg... but the egg requires a third party [there's a joke here I think].  

1.  http://code.google.com/p/mod-spdy
The dedicated IP wouldn't be needed I would think, because any browser which would add SPDY would be modern enough to also support SNI (virtual hosting for SSL/TLS).

SPDY is used over normal SSL/TLS, right ?

The HTTP-'upgrade' header is only understood/acted up on by browsers which support SPDY.

Although I hear someone at Google/whatever was stupid enough not to add SNI-support to Android phones up to 2.x 2.x, but will be in 3.0 / Honeycomb.

Would they really be stupid enough to add SPDY without SNI ?
Assignee: nobody → mcmanus
Hi,

Google already uses SPDY to serve their web applications - including maps. You can see a huge performance improvement in Google Maps when using SPDY (in Chrome).
(FYI: use chrome://net-internals/#spdy in Chrome to view (live) SPDY information.)
I think one of the blocking reasons SPDY isn't yet in Mozilla browser is that the SPDY specification isn't yet ready yet.

But the latest and probably last specification is ready:

http://mbelshe.github.com/SPDY-Specification/draft-mbelshe-spdy-00.xml

So if you have want to speed up the support of SPDY in Mozilla browsers you should go read it and see if you can find any problems with it and report them on the 'dev-spdy' Google group.
(In reply to comment #17)

> 
> But the latest and probably last specification is ready:
> 
> http://mbelshe.github.com/SPDY-Specification/draft-mbelshe-spdy-00.xml
> 

It's interesting that the authors appear to have made optional, and considering the removal of, the mandatory content compression at the transport layer. This was the main thing which bothered me earlier, since it involves redundant attempts at recompression of things like images and other media; this is a big improvement.

I still think that because SPDY is such a big deal for the web, it shouldn't be standardized solely by Google; it's an obvious candidate for HTTP/2.0. However, the best way to ensure that it is developed into a proper cross-vendor standard is for Mozilla to roll their own implementation, or adapt Google's. 

With that in mind, does anyone know if Google's source code for SPDY is available under a Mozilla-compatible licence? Could Google make it so?
Also: just a thought -- since SPDY already does header compression, and now appears to defer to the underlying content source as to the compression of the content, does it also make sense to transport it over TLS with null compression selected, thus saving CPU time, and, more importantly, server cache footprint?
(In reply to comment #18)
> With that in mind, does anyone know if Google's source code for SPDY is
> available under a Mozilla-compatible licence? Could Google make it so?

Google is going to (or already has) submitted http://mbelshe.github.com/SPDY-Specification/draft-mbelshe-spdy-00.xml as an RFC draft to the IETF. Means, that licensing of the spec itself is not an issue.

Client implementation's LICENSE file (http://src.chromium.org/viewvc/chrome/trunk/src/LICENSE?revision=78928&view=markup) say:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
//
// Redistribution and use in source and binary forms, with or without
// modification, are permitted provided that the following conditions are
// met:
//
//    * Redistributions of source code must retain the above copyright
// notice, this list of conditions and the following disclaimer.
//    * Redistributions in binary form must reproduce the above
// copyright notice, this list of conditions and the following disclaimer
// in the documentation and/or other materials provided with the
// distribution.
//    * Neither the name of Google Inc. nor the names of its
// contributors may be used to endorse or promote products derived from
// this software without specific prior written permission.

Is this contrary to Mozilla license?
It looks to me like a simple BSD-type license, which AFAIK is compatible with the Mozilla tri-license policy. Can anyone confirm if this is the case, in which case the code could be used as a starting point for a Mozilla implementation -- indeed, if it's written cleanly enough, it's just possible that whole files may be capable of being taken in verbatim.
(In reply to comment #20) 
> Is this contrary to Mozilla license?

No, Mozilla is already using Chromium license for other parts.

<about:license#chromium>
Please move all of the discussion to the dev.platform mailing list:
https://lists.mozilla.org/listinfo/dev-platform
After a week or two of subscription to dev-platform, there seems to be very little going on there, and no-one else seems to have taken comment 23's advice, so can I presume in the absence of any activity there that this bug is still the best venue for discussing this issue?

If so, there's an interesting paper from Google at http://assets.en.oreilly.com/1/event/42/Introduction%20to%20SPDY%20Presentation.pdf that goes into the rationale of SPDY's design somewhat further than the previous resources.

There's a strong suggestion in there that SPDY may be, or may be contemplating, using TLS Next Protocol Negotiation extension (  http://tools.ietf.org/html/draft-agl-tls-nextprotoneg-00.html ) to autonegotiate the right protocol.

NPN support is likely to be useful for not only this, but other future protocols. As far as I can see, the Mozilla codebase does not support TLS NPN yet: is this issue worth filing a bug of its own?
Hi Neil, I know SPDY is something I hope to get to in the near future at least for experimentation purposes. We cannot do it with a wholesale import of the Chromium code because it needs to coexist with our HTTP feature stack. But there certainly are lessons to be learned in there.

And yes, it will require an NPN implementation which is why this bug is blocked on bug 547312
Hey Patrick, just curious, have you begun your SPDY implementation yet? We're going to begin implementing draft 3 soon in Chromium.
(In reply to William Chan from comment #26)
> Hey Patrick, just curious, have you begun your SPDY implementation yet?
> We're going to begin implementing draft 3 soon in Chromium.

I have literally just begun - to this point basically just integerated Adam's NSS NPN patch and the work needed in Gecko to do the integration.

Is there a subset of servers available that speak v3? I had been planning on starting with spdy/2 .. do you have an opinion on whether that is a useful step?
Great! No server speaks v3 yet. v3 is, aside from minor tweaks here and there, basically v2 + flow control. So implementing v2 is great and is 95% of the way to a v3 implementation. We'll be updating our test server (http://src.chromium.org/viewvc/chrome/trunk/src/net/tools/flip_server/) to support v3 when we get a chance.
To update my earlier comment #14...

nginx author has stated[1]:
>> i was asking myself: does it really require any change on the server side?

> Yes. A lot of changes.

No word on if they actually plan to start implementing.

The Varnish folks feel it's not very proxy friendly[2].  I'm not sure if that's changed.

Conclusion is that nobody outside of Google seems to actually have a documented production level SPDY implementation.

Cite:
1.  http://forum.nginx.org/read.php?2,22517,22528#msg-22528
2.  http://www.gossamer-threads.com/lists/varnish/misc/19353
(In reply to Robert Accettura [:raccettura] from comment #29)

> The Varnish folks feel it's not very proxy friendly[2].  I'm not sure if
> that's changed.
> 
> Conclusion is that nobody outside of Google seems to actually have a
> documented production level SPDY implementation.
> 

Strangeloop, a vendor of a closed reverse-proxy-accelerator, made minor news in June by adding SPDY<->HTTP gatewaying as part of their server acceleration. SPDY intentionally maps nicely to HTTP/1.1, so this kind of gateway is probably the way a lot of server content will transition (if there is to be a transition). 

Strangeloop lists some pretty big customers: petco, travelocity, visa, O'Reily. I haven't looked to see if there is any spdy enabled content on those sites yet. (it hasn't been long since strangeloop's release so it would be surprising if it was in production..)

SPDY has the potential to solve some pretty significant architectural problems involving parallelism, tcp terminations, and buffer sizes. Those potential of those features, combined with how it forces an all SSL transition, are what really excite me - they will make the web more scalable, not simply faster.

New slides from velocity 2011 are interesting too - particular 41-50.
http://www.slideshare.net/ido-cotendo/from-fast-to-spdy-velocity-2011
Amazon seems to be making a pretty major bet on SPDY in SILK, their web-accel platform for the kindle fire: http://aws.amazon.com/amazonsilk-jobs/
Attached patch patch 0 (obsolete) — Splinter Review
Revision 1 of SPDY support. 

This patch integrates a preffed off early-release implementation of SPDY/2. To enable it set network.http.spdy.enabled to true

There are some significant things still to be done here, but the goal is to get this out of the realm of bugzilla patches and blog posts and into the space of real code in the tree. If that can be done without regressing anything with network.http.spdy.enabled set to false, then Josh and I at least think it is worth proceeding now in order to widen the development and testing community.

I am not aware of any interop bugs - so if those come up definitely file them!

This patch depends on the NPN patch (547312) to NSS. The patch attached to this bug does a horrible job of integrating that into the ssl thread with nasty workarounds. fixing that is a major todo - brian has done a lot of the work of removing the ssl thread in 674147 and I will work to either align those efforts or fix the shortcomings in this patch. Of particular note, this patch can probably regress your battery lifetime even when not visiting spdy resources but ONLY IF you turn the spdy pref on. So at least you know what you are getting into. Obviously that all needs to be fixed before it can be preffed on by default.

This doesn't yet come with any automated testing - that's also a todo and a fairly sizable project.

Performance is anecdotal at this point - and anecdotally about what I would expect. I plan to evaluate this based on a number of types of pages and latencies and across a number of criteria: pageload time, number of connections used, mean time to read first byte of response (perhaps broken down by some content-types where we set priority), bytes transferred, latency of external ping during active session, and IP drop rates in low buffer scenarios.

I also still need to implement the Alternate-Protocol header support. That's basically a way to tell a browser to switch from plain http to ssl (and therefore SPDY) only if it implements a particular protocol (in this case, spdy).

Right now, you will use SPDY if the pref is enabled and if you goto a SSL resource and the SSL handshake indicates via NPN that the server speaks spdy/2. As of today, that is most of the google properties (gmail, g+, picasa, encrypted.google.com search, etc..) along with some stuff from strangeloopnetworks and contendo. The debugabillity of it still needs to be improved. The HTTP logs shows some information as well as the telemetry add on if you have that set up.

The patch has been updated for the great s/PRBool/bool/ migration.
Attachment #563757 - Flags: review?(honzab.moz)
Attachment #563757 - Flags: feedback?(mbelshe)
Attachment #563757 - Flags: feedback?(mbelshe) → feedback?(mike)
Attached patch patch 1 rev 0 (obsolete) — Splinter Review
This is a patch meant to be applied on top of patch 0 (the main spdy patch).

This moves the logic for setting up our NPN request out of Activate and into its own function which can be triggered either from Activate or IsAlive(), whichever is called first. This is because the connection manager can call isalive before activation and the MSG_PEEK done inside the socket transport isalive implementation is capable of starting the SSL state machine. We obviously don't want that to happen without first setting up NPN (it results in https being used when spdy is available).
Attachment #564849 - Flags: review?(honzab.moz)
(In reply to Patrick McManus from comment #32)
> Created attachment 563757 [details] [diff] [review] [diff] [details] [review]
> The patch attached to
> this bug does a horrible job of integrating that into the ssl thread with
> nasty workarounds. fixing that is a major todo - brian has done a lot of the
> work of removing the ssl thread in 674147 and I will work to either align
> those efforts or fix the shortcomings in this patch. 

An update on this. I've done the integration with an inprogress version of 674147 and it does clear up a number of things:

* obviously all of the changes patch 0 makes to sslthread can be removed
* the patch 0 use of nshttpconnection::onsocketreadable and generally using the read path to force the handshake can be totally reverted
* the changes to sockettransportservice made by patch 0 (aka the battery killer) can be reverted because polling for write works reliably now
* the incidence of ssl writes returning would_block has fallen from >50% down to a negligible amount

I look forward to the sslthread going away :)
Attached patch patch 2.0 alternate protocol (obsolete) — Splinter Review
This is an incremental patch, meant to go on top of patch 1.

It implements the Alternate-Protocol HTTP response header which SPDY defines. This can be used as a hint to let the browser know that while the current transaction is being done on plain http the same content is available over spdy too - so future transactions with the same host can be redirected to ssl. It's kind of like STS in that way. This patch only allows for redirections to port 443.

The sec-review will definitely need to talk about alternate-protocol on Oct 17.
Attachment #565265 - Flags: review?(honzab.moz)
This is an incremental patch, meant to go on top of patch 2.

The parser for the settings frame needed a better bounds check.
Attachment #565289 - Flags: review?(honzab.moz)
This is meant to be applied on top of patch 3

Make sure the right error code gets propogated to get the fancy SSL handshake failed page in the case of something like a cert-check failure during the NPN handshake.

When the ssl thread goes away this will too.
Attachment #565539 - Flags: review?(honzab.moz)
This is meant to be applied on top of patch 4.

spdy header names must be lower case by spec. This makes comparisons cheaper, which is nice. It is early enough that we should enforce this property to ensure that entropy doesn't mess it up for us. This patch enforces it on response headers and fixes a case where we screened for TE instead of te on request headers.

Spdy also disallows HTTP chunked responses wrapped inside spdy. This patch checks for that explicitly.
Attachment #565540 - Flags: review?(honzab.moz)
Attached patch patch 6 rev 0 (obsolete) — Splinter Review
This patch is meant to be applied on top of patch 5

It enhances the logging and debugability of SPDY. Specifically all bytes sent or received are logged at trace level 4. We might get to the point where this is not necessary, but it does provide a means of last resort of figuring out most problems. The other routine (but very interesting) information is moved to level 3, so it can be captured separately is necessary.
Attachment #565969 - Flags: review?(honzab.moz)
I'm starting to review this now.  Please don't update the existing patches.  The plan is to first review the integration parts, SPDY portions them self can be landed w/o large fixes as pref'ed off.
Comment on attachment 563757 [details] [diff] [review]
patch 0

Review of attachment 563757 [details] [diff] [review]:
-----------------------------------------------------------------

This is just review of the integration parts with spdy.enable = false, searching mainly for possible performance regressions.  

I don't see any except a small issue with the dead connections timer.


r=honzab for no regressions with SPDY disabled and addressed the timer issue.


If we are OK with pushing the SPDY code it self unreviewed then go forward.
I'll start reviewing this stuff as it would be enabled tomorrow.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +907,5 @@
>              rv = mProxyConnectStream->ReadSegments(ReadFromStream, this,
>                                                        nsIOService::gDefaultSegmentSize,
>                                                        &n);
>          }
> +        else if (!EnsureNPNComplete()) {

Maybe add a comment we won't get here sooner then Activate() has already set the mNPNComplete to true (what bypasses EnsureNPNComplete) when SPDY is disabled, so this branch won't get executed.

@@ +1001,5 @@
>  nsHttpConnection::OnSocketReadable()
>  {
>      LOG(("nsHttpConnection::OnSocketReadable [this=%x]\n", this));
>  
> +    if (!mNPNComplete) {

As well here.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +237,4 @@
>  {
> +    // Leave the timer in place if there are connections that potentially
> +    // need management
> +    if (mNumIdleConns || mNumActiveConns)

We have to stop the timer when there are no idle connections.  Is there any special reason to leave it on when there are active (non-spdy) connections and SPDY is disabled?

If an idle connection is closed because of EOF from the server on it but there is an active connection at that moment, the timer may linger for the time to live of the idle connection previously announced; that could be very long time and after wake up it may cause return from sleep/power reduction ; doesn't necessarily be a big problem, though.


It probably cannot be solved just by adding a condition for !SpdyDisabled() since there may be active spdy connections that have to be monitored.

I think it is better to have a new counter/arrays for just spdy connections and also a separate timer.

I also want to suggest to clean the integration of spdy to our http, using more abstraction and switch from one implementation to another on that level then do |if (mSpdySession) { mSpdySession->Something(); return; }|.

The change I suggest with the timer could be start (even very simple!) of that approach.

Patrick, what do you think?

@@ +1352,5 @@
>      LOG(("nsHttpConnectionMgr::OnMsgPruneDeadConnections\n"));
>  
>      // Reset mTimeOfNextWakeUp so that we can find a new shortest value.
>      mTimeOfNextWakeUp = LL_MAXUINT;
> +    mCT.Enumerate(PruneDeadConnectionsCB, this);

Why this change?  Add a comment please.

I understand it is because the timer also manages SPDY active connections but it's wasting when there are no idle conns and SPDY is disabled.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +197,5 @@
>      , mSendSecureXSiteReferrer(PR_TRUE)
>      , mEnablePersistentHttpsCaching(PR_FALSE)
>      , mDoNotTrackEnabled(PR_FALSE)
> +    , mEnableSpdy(PR_TRUE)
> +    , mCoalesceSpdy(PR_TRUE)

I think all of these (or at least enable-spdy) should initially be rather false, at least for now.

::: netwerk/protocol/http/nsHttpHandler.h
@@ +113,5 @@
>      bool           IsPersistentHttpsCachingEnabled() { return mEnablePersistentHttpsCaching; }
>  
> +    bool           IsSpdyEnabled() {return mEnableSpdy; }
> +    bool           CoalesceSpdy() {return mCoalesceSpdy; }
> +

Space between { and return
Attachment #563757 - Flags: review?(honzab.moz) → review+
Comment on attachment 564849 [details] [diff] [review]
patch 1 rev 0

Review of attachment 564849 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab for spdy.enabled = false.

::: netwerk/protocol/http/nsHttpConnection.h
@@ +217,5 @@
>      bool                            mIdleMonitoring;
>  
>      // SPDY related
>      bool                            mNPNComplete;
> +    bool                            mSetupNPN;

Rather mNPNWasSetup/WasStarted/mSetupNPNCalled ?
Attachment #564849 - Flags: review?(honzab.moz) → review+
Comment on attachment 565265 [details] [diff] [review]
patch 2.0 alternate protocol

Review of attachment 565265 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with spdy.enabled = false

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +664,5 @@
>          
>          LOG(("Connection can be reused [this=%x idle-timeout=%u]\n", this, mIdleTimeout));
>      }
>  
> +    HandleAlternateProtocol(responseHead);

You may not want to do this when connecting to an https proxy (mProxyConnectStream)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +498,5 @@
> +    if (mAlternateProtocolHash.Get(hostPortKey))
> +        return;
> +    
> +    if (mAlternateProtocolHash.Count() > 2000)
> +        mAlternateProtocolHash.Enumerate(TrimAlternateProtocolHash, this);

&nsHttpConnectionMgr::TrimAlternateProtocolHash

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +410,5 @@
>      //
>      nsClassHashtable<nsCStringHashKey, nsConnectionEntry> mCT;
> +
> +    // this table is protected by the monitor
> +    nsDataHashtable<nsCStringHashKey, bool>  mAlternateProtocolHash;

You can use nsCStringHashSet (a hash table that keeps just track of presence).  It's possible to enumerate its mHashTable member directly with PL_DHashTableEnumerate, AFAIK.
Attachment #565265 - Flags: review?(honzab.moz) → review+
Comment on attachment 565289 [details] [diff] [review]
patch 3.0 settings parser

Review of attachment 565289 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with spdy.enabled = false

I didn't actually review the code, with the pref off this will never be executed.
Attachment #565289 - Flags: review?(honzab.moz) → review+
Comment on attachment 565540 [details] [diff] [review]
patch 5 rev 0 SPDY Header compliance enforcement

Review of attachment 565540 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with spdy.enabled = false.
Attachment #565540 - Flags: review?(honzab.moz) → review+
Comment on attachment 565539 [details] [diff] [review]
patch 4 rev 0 - fancy cert failure pages

Patrick please explain (rather just in the bug and not the patch) what this change does and why is exactly needed.  I'm not sure call to checkHandshake is correct like this.
> You can use nsCStringHashSet (a hash table that keeps just track of
> presence).  

another day, another class I wasn't aware of. That's perfect - thanks!
(In reply to Honza Bambas (:mayhemer) from comment #46)
> Comment on attachment 565539 [details] [diff] [review] [diff] [details] [review]
> patch 4 rev 0 - fancy cert failure pages
> 
> Patrick please explain (rather just in the bug and not the patch) what this
> change does and why is exactly needed.  I'm not sure call to checkHandshake
> is correct like this.

Its a SSL thread complication. When an error happens in NSS on the SSL thread the sslthread code normally stuffs the err into mThreadData->mPRErrorCode. It cannot report it directly to the network thread because they are asynchronous. when the network code comes back around to do another read or another write it picks up that code and generally calls PR_SetError() with it while on the netwerk thread. Read/Write are bimodal in the sense that they have the "want_read" and "did_read" modes - the error gets logged in the want_read phase and picked up in the did_read phase.

I didn't make the 2 modes for ssl_handshake because its not the kind of thing that gets intermixed between various states - you do it upfront. Maybe I should have, but the issue goes away with the sslthread so I don't care very much. 

what the patch does is a] record the error in the mThreadData->mPRErrorCode (in Run()), and b] report that error through the usual PR_SetError() approach in ssl_handshake next time that function is called.

This allows the specific error to get handled. The case I was looking at when I wrote the patch was a self-signed cert. Without this patch the cert validation fails (correctly) but you just get a load error, but with this patch the right error is propogated and you get the fancy ssl-error-with-override-button page.
Comment on attachment 565539 [details] [diff] [review]
patch 4 rev 0 - fancy cert failure pages

Review of attachment 565539 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: security/manager/ssl/src/nsSSLThread.cpp
@@ +1216,5 @@
>        else if (nsSSLSocketThreadData::ssl_force_handshake == busy_socket_ssl_state)
>        {
>            if (SSL_ForceHandshake(realFileDesc) != PR_SUCCESS)
>                bstd.mPRErrorCode = PR_GetError();
> +          if (checkHandshake(-1, PR_TRUE, realFileDesc, mBusySocket) < 0)

The change in the first hunk is enough to have the error page.  Error is correctly propagated to the correct thread and gets up the way to doc shell.  I came to the same conclusion while testing locally.

SSL_ForceHandshake also sets the error message on the socket that is another precondition to have the error page.

Call to checkHandshake is needed because of potential TLS intolerance retry.

If we don't call it here, we never call it.  It is called only on the SSL thread after read/write error (we never reach it) or after a handshake timeout (as well, never reached).

I would just call checkHandshake on failure of SSL_ForceHandshake and set mPRErrorCode after call to checkHandshake, probably always.

Please add comments explaining the changes in both hunks.

If you have reason to leave the code as is, then just explain in a patch comment.
Attachment #565539 - Flags: review?(honzab.moz) → review+
Comment on attachment 563757 [details] [diff] [review]
patch 0

Christian can you just have an sr look at the netwerk idl change?
Attachment #563757 - Flags: superreview?(cbiesinger)
Comment on attachment 563757 [details] [diff] [review]
patch 0

Dan, can you have an sr look at the security/ idl change? (bugzilla doesn't seem to want to let me set two sr flags - but that's what I'm requesting here.)
Attachment #563757 - Flags: review?
Comment on attachment 563757 [details] [diff] [review]
patch 0

Review of attachment 563757 [details] [diff] [review]:
-----------------------------------------------------------------

Looked almost only a the IDL changes, looks OK, but see comments below.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +366,5 @@
>  PRIntervalTime
>  nsSocketTransportService::PollTimeout()
>  {
> +    if (mSpdyEnabled) {
> +        // This truly awful hack is because the sslthread force handshake

:(

::: netwerk/socket/nsISSLSocketControl.idl
@@ +53,5 @@
>      attribute nsIInterfaceRequestor     notificationCallbacks;
>  
>      void proxyStartSSL();
>      void StartTLS();
> +    [noscript] void setNPNList(in nsCStringTArrayRef aNPNList);

Please add a comment indicating when this method can be called

@@ +54,5 @@
>  
>      void proxyStartSSL();
>      void StartTLS();
> +    [noscript] void setNPNList(in nsCStringTArrayRef aNPNList);
> +    [noscript] void forceHandshake();

Can you add a comment on what this method does? And why is it noscript?

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +59,5 @@
>     *       query nsIX509Cert3::isSelfSigned 
>     */
>    readonly attribute boolean isUntrusted;
> +
> +  readonly attribute ACString negotiatedNPN;

Please add a comment explaining what happens if no next protocol has been negotiated.

Also, maybe negotiatedNextProtocol would be a better name, because the last N in NPN stands for negotiation, right?
Attachment #563757 - Flags: superreview?(cbiesinger) → superreview+
I forget a general comment: Patrick, please don't use abbreviations.  It makes things hard to read.  80 lines is a low limit, but you can always format the code to fit.
one line fix for one case with an SSL http proxy where we were using NPN (and didn't want to be). Proxies are potentially an interesting case, but I want to get the end to end stuff well understood first.
Attachment #567751 - Flags: review?(honzab.moz)
I've added a pref to control whether or not we honor the alternate-protocol response header. Right now I have defaulted it to true, so no behavior is changed in the patch series. But after the sec review its clear that this is an attribute we should at least be able to control at the pref level.
Attachment #567760 - Flags: review?(honzab.moz)
whoops - that last upload was empty. This is the patch.
Attachment #567760 - Attachment is obsolete: true
Attachment #567765 - Flags: review?(honzab.moz)
Attachment #567760 - Flags: review?(honzab.moz)
Comment on attachment 565969 [details] [diff] [review]
patch 6 rev 0

Review of attachment 565969 [details] [diff] [review]:
-----------------------------------------------------------------

Nice deep logs.  Thanks.

r=honzab with few details fixed.

::: netwerk/protocol/http/SpdySession.cpp
@@ +140,5 @@
>  }
>  
> +void
> +SpdySession::LogIO(SpdySession *self, SpdyStream *stream, char *label,
> +                   const char *data, PRUint32 datalen)

Do you plan to use the |stream| argument?  Otherwise you may want to remove it.

@@ +159,5 @@
> +        *line = 0;
> +        LOG4(("%s", linebuf));
> +      }
> +      line = linebuf;
> +      snprintf (line, 128, "%08X: ", index);

No space after snprintf

@@ +960,5 @@
>      PRUint32 id = (setting[2] << 16) + (setting[1] << 8) + setting[0];
>      PRUint32 flags = setting[3];
>      PRUint32 value =  PR_ntohl(reinterpret_cast<PRUint32 *>(setting)[1]);
>  
> +    LOG3(("Settings ID %d, Flags %X, Value %d",id, flags, value));

space between %d", and id

@@ +1260,5 @@
>    }
>    
>    if (*countRead > 0) {
> +    LOG3(("SpdySession::ReadSegments %p stream=%p generated end of frame %d",
> +          this, *countRead));

Missing formatting argument ! (the stream as 2nd arg).

@@ +1266,5 @@
>      SetWriteCallbacks(stream->Transaction());
>      return rv;
>    }
>    
> +  LOG3(("SpdySession::ReadSegments %p stream=%p stream send complete", this));

As well here.

@@ +1397,5 @@
>        }
>        mFrameDataLast = (mFrameBuffer[4] & kFlag_Data_FIN);
>        Telemetry::Accumulate(Telemetry::SPDY_CHUNK_RECVD, mFrameDataSize >> 10);
> +      LOG3(("Start Processing Data Frame. Session=%p Stream 0x%x Fin=%d Len=%d",
> +            this, streamID, mFrameDataLast, mFrameDataSize));

Not clear whether the stream here is logged by id or by instance pointer

@@ +1537,5 @@
>  SpdySession::CloseTransaction(nsAHttpTransaction *aTransaction,
>                                nsresult aResult)
>  {
>    NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");
> +  LOG3(("spdysession::CloseTransaction %p %p %x", this, aTransaction, aResult));

You should be consistent on case (e.g. spdysession vs SpdySession, I prefer the letter).  grep or any other filtering software may be by default case sensitive, and some lines could be missed from the log this way.

::: netwerk/protocol/http/SpdyStream.cpp
@@ +110,4 @@
>  
>    NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");
> +  LOG3(("SpdyStream %p ReadSegments reader=%p count=%d",
> +        this, reader, count));

Redundant?  Could be merged?

@@ +563,5 @@
>  void
>  SpdyStream::ChangeState(enum stateType newState)
>  {
> +  LOG3(("SpdyStream::ChangeState() %p from %X to %X",
> +        mUpstreamState, newState));

Missing args (1st |this|).
Attachment #565969 - Flags: review?(honzab.moz) → review+
Comment on attachment 567751 [details] [diff] [review]
patch 7 rev 0 . Missed one case where we where using npn with a ssl proxy

Review of attachment 567751 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab for spdy.enabled = false
Attachment #567751 - Flags: review?(honzab.moz) → review+
Comment on attachment 567765 [details] [diff] [review]
patch 8 rev 1 alternate protocol pref

Review of attachment 567765 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +480,5 @@
>  void
>  nsHttpConnectionMgr::ReportSpdyAlternateProtocol(nsHttpConnection *conn)
>  {
> +    // Check network.http.spdy.use-alternate-protocol pref
> +    if(!gHttpHandler->UseAlternateProtocol())

space between if and (!
Attachment #567765 - Flags: review?(honzab.moz) → review+
The spdy host coalescing (aka ip pooling or de-sharding), requires a little bit of cert verification. See https://groups.google.com/forum/#!topic/spdy-dev/UW0_X2GaMSQ

This basically means that if host A and B share the same IP address then the SPDY connection for A can be used for transactions of host B if the cert we are using for host A would be acceptable for host B (and the coalesce feature is enabled, of course). The canonical example here is that A==foo.domain.com and B==bar.domain.com and A's cert is *.domain.com. This is how the google services operate, as a pertinent example. But if A's cert covered just foo.domain.com we don't want to put the requests for bar.domain.com down that pipe.
Attachment #568412 - Flags: superreview?(cbiesinger)
Attachment #568412 - Flags: review?(honzab.moz)
Comment on attachment 568412 [details] [diff] [review]
patch 9 rev 0 coalesce spdy servers requires cert check

Review of attachment 568412 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/public/nsIX509Cert3.idl
@@ +89,5 @@
>                         [retval, array, size_is(length)] out wstring
>                         tokenNames);
> +
> +   /**
> +   * Deterime if the certificate can be verified for specific host name

Determine

@@ +94,5 @@
> +   *
> +   * @param aHostName the hostname to be verified
> +   * @return a boolean successful verification
> +   */
> +    bool isSocketValidForHostname(in string aHostName);

Socket -> Certificate?

Is this the punycode version of the hostname? Please document. Also, if it isn't, the type has to change.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +967,5 @@
> +{
> +  *retval = false;
> +
> +  if (!mCert)
> +      return NS_OK;

Your indentation is inconsistent between this line...

@@ +970,5 @@
> +  if (!mCert)
> +      return NS_OK;
> +    
> +  if (CERT_VerifyCertName(mCert, aHostName) == SECSuccess)
> +    *retval = true;

...and this one
Attachment #568412 - Flags: superreview?(cbiesinger) → superreview+
Thanks Christian!

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #61)

> > +    bool isSocketValidForHostname(in string aHostName);
> 
> Socket -> Certificate?

fwiw I changed it to "isValidForHostname" because it is a method on the cert itself.as I was writing the patch it was a method on an active ssl object but this was obviously a better place for it. so I moved it but forgot to reconsider the name.
Comment on attachment 568412 [details] [diff] [review]
patch 9 rev 0 coalesce spdy servers requires cert check

Review of attachment 568412 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab, but please see the comments, we may need new bugs for some of those.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +453,5 @@
> +
> +    // Lookup preferred directly from the hash instead of using
> +    // GetSpdyPreferred() because we want to avoid the cert double check at
> +    // this point. There can be only one cache key and whether or not the entry
> +    // used can be a runtime decision.

Sorry, the last sentence is not clear to me, could you please rephrase?

@@ +506,5 @@
> +    if (sslStatus && NS_SUCCEEDED(rv))
> +        rv = sslStatus->GetServerCert(getter_AddRefs(cert));
> +
> +    if (cert && NS_SUCCEEDED(rv))
> +        ent->mCert = do_QueryInterface(cert, &rv);

Checking for NS_SUCCEEDED(rv) is mostly redundant here.

There is also overload of do_QueryInterface w/o the second arg.

----------

I think we should not coalesce ssl sessions that clients have previously authenticated with client certificates on ; regardless this is not mentioned in the SPDY spec.  It might lead to privacy leaks/login attacks.

To explain: when the connection (conn) has sent a client certificate, don't allow use of this connection by other requests to the same IP even there would be a match by the cert name, rather use a new one and let client reauth again.

We currently don't track client certificate authentication status in ssl status object.  So it is hard to say if we have sent a client cert or not.  Needs a new API for that.

This probably can be done in a separate patch/bug.  Also we might want to communicate this to google people.

----------

Also, I didn't take a deeper look at the SPDY parts them self yet, but aren't you also coalescing anonymous and non-anonymous connections?  If those are valid for SPDY, but I think those are...

@@ +589,5 @@
> +    nsConnectionEntry *preferred =
> +        mSpdyPreferredHash.Get(aOriginalEntry->mDottedDecimalAddress);
> +
> +    if (preferred == aOriginalEntry)
> +        return preferred;   /* no redirection so no cert check required */

Nit: to make it more obvious at the first look, you may want to return aOriginalEntry here.

@@ +595,5 @@
> +    if (!preferred || !preferred->mCert)
> +        return nsnull;                         /* no ip pooling */
> +
> +    nsresult rv;
> +    bool confirmed = false;

Seems to be unused.
Attachment #568412 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #63)
> 
> I think we should not coalesce ssl sessions that clients have previously
> authenticated with client certificates on ; regardless this is not mentioned
> in the SPDY spec.  It might lead to privacy leaks/login attacks.
> 
> To explain: when the connection (conn) has sent a client certificate, don't
> allow use of this connection by other requests to the same IP even there
> would be a match by the cert name, rather use a new one and let client
> reauth again.

that's an interesting point. If we were to do that then we probably also would not want to pool if we would send a cert in the case we did not pool. (i.e. don't pool if either the original or the target entry used a client side cert). right?

I'll file a bug and raise it on spdy-dev.

> Also, I didn't take a deeper look at the SPDY parts them self yet, but
> aren't you also coalescing anonymous and non-anonymous connections?  If
> those are valid for SPDY, but I think those are...

that's a good point and easy to fix. I'll add another patch for it.

thanks.
Blocks: 696531
as discussed, make the LOAD_ANONYMOUS flag part of the coalescing hash key.
Attachment #568899 - Flags: review?(honzab.moz)
When I thought about this a bit more later in the evening it was clear the port number needs to be part of this key too. The other parts of the conninfo key aren't germane.
Attachment #568899 - Attachment is obsolete: true
Attachment #568946 - Flags: review?(honzab.moz)
Attachment #568899 - Flags: review?(honzab.moz)
Comment on attachment 568412 [details] [diff] [review]
patch 9 rev 0 coalesce spdy servers requires cert check

Review of attachment 568412 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/public/nsIX509Cert3.idl
@@ +94,5 @@
> +   *
> +   * @param aHostName the hostname to be verified
> +   * @return a boolean successful verification
> +   */
> +    bool isSocketValidForHostname(in string aHostName);

Use the name "isValidForHostname". I agree that "Socket" doesn't belong in the cert validation code, but we don't need to repeat "Certificate" either.

Also, if you are going to change an existing interface, change nsIX509Cert instead of nsIX509Cert3; nsIX509Cert2 and nsIX509Cert3 will get merged into nsIX509Cert eventually.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +964,5 @@
>  NS_IMETHODIMP
> +nsNSSCertificate::IsSocketValidForHostname(const char * aHostName,
> +                                           bool *retval)
> +{
> +  *retval = false;

You need to get the nsNSSShutdownPreventionLock and then check that NSS hasn't been shutdown.
Attachment #568412 - Flags: review+
No longer blocks: 696531
Depends on: 696531
Comment on attachment 568412 [details] [diff] [review]
patch 9 rev 0 coalesce spdy servers requires cert check

Review of attachment 568412 [details] [diff] [review]:
-----------------------------------------------------------------

Every place we call CERT_VerifyCertName in security/manager, we need to call it for all hostnames that have previously been coalesced onto this connection and/or have requests queued on this connection. This includes AuthCertificateCallback and nsNSSBadCertHandler in particular.

During a renegotiation, server can send a different certificate than what it sent in previous handshakes, and that cert could be valid for a different set of hosts.
Attachment #568412 - Flags: review+ → review-
(In reply to Brian Smith (:bsmith) from comment #68)
> Every place we call CERT_VerifyCertName in security/manager, we need to call
> it for all hostnames that have previously been coalesced onto this
> connection and/or have requests queued on this connection. This includes
> AuthCertificateCallback and nsNSSBadCertHandler in particular.

There is a simpler way of solving this problem: When we have negotiated SPDY via NPN, fail if the server changes its cert during renegotiation. This check can be added to AuthCertificateCallback. This assumes that we will require NPN for all SPDY (over TLS) connections.
Comment on attachment 568946 [details] [diff] [review]
patch 10.1 colaesce key includes anonymous and port

Review of attachment 568946 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1884,5 @@
> +        if (mEnt->mConnInfo->GetAnonymous())
> +            mEnt->mCoalescingKey.Append("T:");
> +        else
> +            mEnt->mCoalescingKey.Append("F:");
> +        mEnt->mCoalescingKey.AppendInt(mEnt->mConnInfo->Port());

Why not just "A:" and ".:" for anonymous and normal to stick with what we use for connection info keys?

If otherwise, please add a comment.

And also rather append in form "~A:" or alike to avoid any potential conflict with IPv6 addresses?

------

Out of scope of this bug's review: why don't you just pick the peer address from the transport in CONNECTED_TO state event?
Attachment #568946 - Flags: review?(honzab.moz) → review+
> Why not just "A:" and ".:" for anonymous and normal to stick with what we
> use for connection info keys?

I avoided "." beacuse it is used in a dotted decimal string.. but then again I managed to use : which is used in ipv6 as you point out. I don't think either really matters nor do I really care ;)  - so changed it to ~[A.]:

> Out of scope of this bug's review: why don't you just pick the peer address
> from the transport in CONNECTED_TO state event?

That is better - can get it earlier from STATUS_RESOLVED, right?
(In reply to Patrick McManus from comment #71)
 
> That is better - can get it earlier from STATUS_RESOLVED, right?

Ah, now I remember why you said CONNECTED_TO. In general that seems suboptimal, but in this case it doesn't matter (the key isn't interesting until after NPN has completed anyhow, which certainly requires the connection.)
(In reply to Patrick McManus from comment #71)
> > Out of scope of this bug's review: why don't you just pick the peer address
> > from the transport in CONNECTED_TO state event?
> 
> That is better - can get it earlier from STATUS_RESOLVED, right?

Not exactly.  nsSocketTransport::GetPeerAddr returns something only when the transport's state is STATE_TRANSFERRING, there is an artificial check for that.  That is the first thing that breaks it, but there is already a bug to easy this condition (and also add a sync) - can't find the number right now.  But what is more important, socket transport tries to connect all IPs in the list that it obtains in the OnLookupComplete callback.  But not the first one may succeed to connect to.  You should pick the one we actually connect to, not just the first in the list.

I believe there needs to be done more work here, though I didn't check the code so in-depth to say for sure.  The SPDY spec says, that when there is a host whom one of IPs it resolves to equals to one we have an active connection for, then use that existing connection.  I didn't see this in your code, but as I said, I might be mistaken.
based on comment 73
Attachment #569137 - Flags: review?(honzab.moz)
> I believe there needs to be done more work here, though I didn't check the
> code so in-depth to say for sure.  The SPDY spec says, that when there is a
> host whom one of IPs it resolves to equals to one we have an active
> connection for, then use that existing connection.  I didn't see this in
> your code, but as I said, I might be mistaken.

right, it doesn't iterate and do the full n^2 comparisons. But then again, we never iterate the list unless there is a connection error, so its extremely unimportant imo. we can file a followon bug, but I don't see any reason to worry about it at this stage - the "that's the same host I connected to again" test is fine for now.
(In reply to Brian Smith (:bsmith) from comment #67)
> Comment on attachment 568412 [details] [diff] [review] [diff] [details] [review]
> patch 9 rev 0 coalesce spdy servers requires cert check
> 
> Also, if you are going to change an existing interface, change nsIX509Cert
> instead of nsIX509Cert3; nsIX509Cert2 and nsIX509Cert3 will get merged into
> nsIX509Cert eventually.

well, my pov there is that nsix509cert3 is less likely to be used by an addon than nsix509cert and this api addition probably isn't needed by addons either. When they get merged together addon folks can take the hit just once instead of once now and again when they are merged.

if you feel really strongly otherwise, let me know.

> 
> You need to get the nsNSSShutdownPreventionLock and then check that NSS
> hasn't been shutdown.

thanks.
Comment on attachment 569137 [details] [diff] [review]
patch 11 rev 0 coalescing ip from nsisocket instead of dns call

Review of attachment 569137 [details] [diff] [review]:
-----------------------------------------------------------------

Looks nice!

r=honzab

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2033,5 @@
> +        mEnt->mCoalescingKey.IsEmpty() &&
> +        status == nsISocketTransport::STATUS_CONNECTED_TO) {
> +
> +        PRNetAddr addr;
> +        mSocketTransport->GetPeerAddr(&addr);

Check for result here.

@@ +2042,5 @@
> +
> +        if (mEnt->mConnInfo->GetAnonymous())
> +            mEnt->mCoalescingKey.Append("~A:");
> +        else
> +            mEnt->mCoalescingKey.Append("~.:");

AppendLiteral please.

In general, I'd love to have a method on nsConnectionEntry to build the coalescing key inside it.  And also to have an accessor to it, making mCoalescingKey it self private.
Attachment #569137 - Flags: review?(honzab.moz) → review+
Comment on attachment 568412 [details] [diff] [review]
patch 9 rev 0 coalesce spdy servers requires cert check

Review of attachment 568412 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +506,5 @@
> +    if (sslStatus && NS_SUCCEEDED(rv))
> +        rv = sslStatus->GetServerCert(getter_AddRefs(cert));
> +
> +    if (cert && NS_SUCCEEDED(rv))
> +        ent->mCert = do_QueryInterface(cert, &rv);

Here, by saving a reference to the certificate, you seem to be assuming that the server will not change its cert during renegotiations. We should not make that assumption without also prohibiting the server from changing its cert during renegotiations.
(In reply to Brian Smith (:bsmith) from comment #68)
> 
> Every place we call CERT_VerifyCertName in security/manager, we need to call
> it for all hostnames that have previously been coalesced onto this
> connection and/or have requests queued on this connection. This includes
> AuthCertificateCallback and nsNSSBadCertHandler in particular.
> 
> During a renegotiation, server can send a different certificate than what it
> sent in previous handshakes, and that cert could be valid for a different
> set of hosts.

(In reply to Brian Smith (:bsmith) from comment #69)
> 
> There is a simpler way of solving this problem: When we have negotiated SPDY
> via NPN, fail if the server changes its cert during renegotiation. This
> check can be added to AuthCertificateCallback. This assumes that we will
> require NPN for all SPDY (over TLS) connections.

I've got no problem requiring npn and tls for every spdy connection. You seem to be saying that is ok to declare we don't support renegotiation with spdy - I don't feel like I have standing to really make a decision on that. What's renegotiation good for anyhow, assuming we aren't using client certs ?

Your suggestion of doing this in authcertifcatecallback includes, by reference, a whole lot of code I haven't fully internalized. So I want to run by my interpretation of what you suggested here. is this more or less what you mean?


diff --git a/security/manager/ssl/src/nsNSSCallbacks.cpp b/security/manager/ssl/src/nsNSSCallbacks.cpp
--- a/security/manager/ssl/src/nsNSSCallbacks.cpp
+++ b/security/manager/ssl/src/nsNSSCallbacks.cpp
@@ -1170,16 +1170,29 @@ SECStatus PR_CALLBACK AuthCertificateCal
   if (serverCert) {
     nsNSSSocketInfo* infoObject = (nsNSSSocketInfo*) fd->higher->secret;
     nsRefPtr<nsSSLStatus> status = infoObject->SSLStatus();
     nsRefPtr<nsNSSCertificate> nsc;
 
     if (!status || !status->mServerCert) {
       nsc = nsNSSCertificate::Create(serverCert);
     }
+    else if (1 && status && status->mServerCert) {  /* todo if npn==spdy instead of 1*/
+      nsRefPtr<nsNSSCertificate> tempCert = nsNSSCertificate::Create(serverCert);
+      
+      bool sameCert = false;
+      nsresult nsrv = tempCert->Equals(status->mServerCert, &sameCert);
+      if (NS_FAILED(nsrv) || !sameCert) {
+        PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
+               ("AuthCertificateCallback failing "
+                "due to spdy cert renegotiation\n"));
+        fprintf(stderr,"NOT OK RENO\n");
+        rv = SECFailure;
+      }
+    }
 
     CERTCertList *certList = nsnull;
     certList = CERT_GetCertChainFromCert(serverCert, PR_Now(), certUsageSSLCA);
     if (!certList) {
       rv = SECFailure;
     } else {
       PRErrorCode blacklistErrorCode;
       if (rv == SECSuccess) { // PSM_SSL_PKIX_AuthCertificate said "valid cert"
We cannot use the certificate retrieved from nsITransportSecurityInfo or nsISSLStatus to make security-sensitive decisions. The certificate returned from those interfaces might not have been authenticated and/or the integrity of the SSL handshake might not have been verified even when those interfaces return certificates. This is by design; when a cert validation error occurs, we want the site identity block to show the cert that caused the problem.

Here is an example of what could go wrong: We connect to a site www.example.org which ha a self-signed certificate that says it is also valid for *.mozilla.org. Then, we connect to www.mozilla.org. Since we assume an attacker can control the network and DNS is unauthenticated, we assume that an attacker can modify A/AAA records for www.mozilla.org to point to the same IP as www.example.org. AFAICT, with the proposed patch, in this scenerio, we would then try to coalesce traffic for www.mozilla.org on the same connection that we used for www.example.org. The connection would eventually fail, but we never should have gotten this far in the first place.

Also, whenever we use CERT_VerifyCertName, we also need to account for the uesr's certificate error overrides, or explain why we don't need to do so. In fact, it isn't clear to me that we can safely avoid going through the entire AuthCertificateCallback/BadCertHandler procedure before deciding that we can coalesce the connections.

IMO, all of the connection coalescing should be factored out into its own bug and should be enabled/disabled with a separate pref from the main SPDY support, to minimize (schedule) risk.
(In reply to Patrick McManus from comment #79)
> I've got no problem requiring npn and tls for every spdy connection. You
> seem to be saying that is ok to declare we don't support renegotiation with
> spdy - I don't feel like I have standing to really make a decision on that.
> What's renegotiation good for anyhow, assuming we aren't using client certs ?

The most useful reason for doing renegotiation is to protect client certs. But, regardless of what it is good for, many Apache configurations are set up to do renegotiation even without client certificates, because Apache lets you say (stupidly) "I require AES-256 for resources under /some/path even though I allow other ciphers for other parts of the server" and other silly things. So, we have to still support renegotiation.

My suggestion was/is that for now, we still support renegotiations even with SPDY, but that we don't allow renegotiations where the server tries to change its own cert.

> run by my interpretation of what you suggested here. is this more or less
> what you mean?

Yes, this is the right idea. But, you can do this instead:

nsCOMPtr<sIX509Cert> cert;
(void infoObject->GetCert(getter_AddRefs(cert));
nsCOMPtr<nsIX509Cert2> cert2 = do_QueryInterface(cert);

// Prohibit changing the server cert only if we negotiated SPDY,
// in order to support SPDY's cross-origin connection pooling.
if (cert2 && <negoatiated SPDY> &&
    <SPDY cross-origin connection pooling enabled>) {
  CERTCertificate * c = cert2->GetCert();
  NS_ASSERTION(c, "very bad and hopefully impossible state");
  bool sameCert = c == serverCert;
  CERT_DestroyCertificate(c);
  if (!sameCert) {
     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
            ("Refused to allow new cert during renegotiation\n"));
     // probably better to define a new error code instead
     PR_SetError(SSL_ERROR_RENEGOTIATION_NOT_ALLOWED, 0);
     rv = SECFailure;     
  }  
}

Note that you can compare CERTCertificate objects for equality with operator== because NSS ensures that there is only one CERTCertificate instance per distinct cert.

Also, I recommend doing this check at the top of AuthCertificateCallback because then it is clearer that nsNSSSocketInfo::GetCert() will return the cert for the previous handshake.
In reply to Brian Smith (:bsmith) from comment #80)

> Here is an example of what could go wrong: We connect to a site
> www.example.org which ha a self-signed certificate that says it is also
> valid for *.mozilla.org. Then, we connect to www.mozilla.org. Since we
> assume an attacker can control the network and DNS is unauthenticated, we
> assume that an attacker can modify A/AAA records for www.mozilla.org to
> point to the same IP as www.example.org. AFAICT, with the proposed patch, in
> this scenerio, we would then try to coalesce traffic for www.mozilla.org on
> the same connection that we used for www.example.org. The connection would
> eventually fail, but 

My understanding here is that a cert for www.example.org which also calims validity for *.mozilla.org would not valid for establishing a connection (lacking a valid signature). Therefore we won't establish a working connection and therefore we won't call reportspdyconnection(). Its only at that point that the servercert is associated with the connection entry (i.e. to get there it requires a successful handshake where spdy was negotitiated via npn). Is that insufficient?


(In reply to Brian Smith (:bsmith) from comment #80)
> IMO, all of the connection coalescing should be factored out into its own
> bug and should be enabled/disabled with a separate pref from the main SPDY
> support, to minimize (schedule) risk.

There is a separate pref - "network.http.spdy.coalesce-hostnames". I don't see any reason to go to the work of factoring out code that can be preffed off.

But this is an important feature - it goes to the core of spdy's strengths (increasing the number of streams to multiplex and reducing handshakes) while acknowledging the sharding of today's web. We need to be looking for ways to make it work (which we seem to be doing - huzzah!); preferrably from the beginning.

(In reply to Brian Smith (:bsmith) from comment #80)
> Also, whenever we use CERT_VerifyCertName, we also need to account for the
> uesr's certificate error overrides, or explain why we don't need to do so.

we're only ever using an existing connection (accepted under the existing rules) for another name that accepted cert also covers. Can a users's local prefs create a situation where we wouldn't accept that cert again? (I really don't know, I had presumed not.) We can certainly do the full callback rigamarole if necessary, that doesn't seem like a show stopper.
(In reply to Patrick McManus from comment #82)
> My understanding here is that a cert for www.example.org which also calims
> validity for *.mozilla.org would not valid for establishing a connection
> (lacking a valid signature). Therefore we won't establish a working
> connection and therefore we won't call reportspdyconnection(). Its only at
> that point that the servercert is associated with the connection entry (i.e.
> to get there it requires a successful handshake where spdy was negotitiated
> via npn). Is that insufficient?

What happens when the cert is invalid, but the user has added an exception for it to be used for www.example.org? My understanding is that CERT_VerifyCertName(cert, "www.mozilla.org") will return SECSuccess in this case, but that is wrong.

> There is a separate pref - "network.http.spdy.coalesce-hostnames". 

OK, good enough. 

> But this is an important feature - it goes to the core of spdy's strengths
> (increasing the number of streams to multiplex and reducing handshakes)
> while acknowledging the sharding of today's web. We need to be looking for
> ways to make it work (which we seem to be doing - huzzah!); preferrably from
> the beginning.

I agree that it may be useful. But, I don't think it is so important that we would delay landing the rest of the SPDY support if we didn't have it.

> we're only ever using an existing connection (accepted under the existing
> rules) for another name that accepted cert also covers. Can a users's local
> prefs create a situation where we wouldn't accept that cert again? (I really
> don't know, I had presumed not.) We can certainly do the full callback
> rigamarole if necessary, that doesn't seem like a show stopper.

CERT_VerifyCertName (basically) only checks the subjectAltName extensions in the certificate to see if there is one for the given hostname. It only gives valid results when the certificate is valid. Because of the problem I explained above (and possibly more), this is something that would need to be fixed before we can turn the coalescing on. (And, IMO, we shouldn't land the coalescing code until it is correct at least as far as the security aspects go.)
in 696531 brian rightly objects to extending nsISSLStatus when nsITransportSecurityInfo can do the job.

patch 0 in the series on this bug has the same issue with the negotiatedNPN attribute. This patch moves things over to nsITransportSecurityInfo and reverts the changes to nsISSLStatus (I had agreed with honza to keep this as a series of interdiffs as much as possible, so I am making this another patch in the series rather than revising patch 0). 

I want to do this before tackling the issues from comments 79 and 81.
Attachment #569698 - Flags: superreview?(cbiesinger)
Attachment #569698 - Flags: review?(bsmith)
When using spdy on a ssl connection don't allow changes to the cert via re-negotiaton.

Brian, I know your original suggestion was to only ban this when the coalesce feature is on - but a slightly broader approach made more sense to me. The server really doesn't have a way of knowing whether or not the coalescing config is turned on (and therefore can't adjust its behavior accordingly), so it seems best to just say to spdy servers "we don't do that". It can be loosened if necessary later.

This has a dep on patch 12, so I just added it as a new one in the series.

The bit about not extending the stored exception for host a to host b when a's cert covers b is still an outstanding item. I'm pretty sure the right thing to do there is not the full verification chain complete with UI, but rather to just not coalesce that case and make a connection to host b directly (which may or may not generate ui depending on what host b sends as a cert). That is just an optimization loss for people that don't have well trusted certs. I'm not well versed in the relevant interfaces, so I have some research to do.
Attachment #569741 - Flags: review?(bsmith)
(In reply to Patrick McManus from comment #85)
> The bit about not extending the stored exception for host a to host b when
> a's cert covers b is still an outstanding item. I'm pretty sure the right
> thing to do there is not the full verification chain complete with UI, but
> rather to just not coalesce that case and make a connection to host b
> directly (which may or may not generate ui depending on what host b sends as
> a cert).

I agree.

> That is just an optimization loss for people that don't have well
> trusted certs. I'm not well versed in the relevant interfaces, so I have
> some research to do.

I am rewriting it all as part of the SSL thread removal patch, which I am finishing up today. I am still not sure if we need to do a full validation or if we can do a shortcut; doing a full validation means dispatching an event to the PSM certificate verification thread and waiting (potentially, but unlikely) for multiple HTTP transactions to happen as part of OCSP checking.
I just realized that if we implement the Alternate-protocol stuff, that we should make sure that we disable that feature for OCSP/CRL fetches (the requests that are created in nsNSSCallbacks). Otherwise the OCSP responder could redirect us to SPDY-over-TLS which would cause us to need to do OCSP validation for the OCSP responder server, which could have the same certs as the site we are trying to go to, which would cause us to loop forever in OCSP checking.
Comment on attachment 569741 [details] [diff] [review]
patch 13.0 reject server cert changes during renegotiation with spdy

Review of attachment 569741 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1144,5 @@
> +
> +  // Check to see if the cert has actually changed
> +  CERTCertificate * c = cert2->GetCert();
> +  NS_ASSERTION(c, "very bad and hopefully impossible state");
> +  bool sameCert = c == serverCert;

I'm not sure this will always pass.  There is a chance we create a new instance of the certificate with the new negotiation.  There is a function to compare certs CERT_CompareCerts.

@@ +1163,5 @@
>  
>    CERTCertificate *serverCert = SSL_PeerCertificate(fd);
>    CERTCertificateCleaner serverCertCleaner(serverCert);
>  
> +  if (BlockServerCertChangeForSpdy(fd, serverCert) == SECFailure)

More usual is != SECSuccess.
(In reply to Honza Bambas (:mayhemer) from comment #88)
> Comment on attachment 569741 [details] [diff] [review] [diff] [details] [review]
> patch 13.0 reject server cert changes during renegotiation with spdy
> 
> Review of attachment 569741 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/ssl/src/nsNSSCallbacks.cpp
> @@ +1144,5 @@

> > +  bool sameCert = c == serverCert;
> 
> I'm not sure this will always pass.  There is a chance we create a new

that's actually brian's example from comment 81 where he says explicitly its ok. I'm taking his word on that one. Brian?
(In reply to Brian Smith (:bsmith) from comment #87)
> I just realized that if we implement the Alternate-protocol stuff, that we
> should make sure that we disable that feature for OCSP/CRL fetches (the
> requests that are created in nsNSSCallbacks). Otherwise the OCSP responder
> could redirect us to SPDY-over-TLS which would cause us to need to do OCSP
> validation for the OCSP responder server, which could have the same certs as
> the site we are trying to go to, which would cause us to loop forever in
> OCSP checking.

how is this fixed for sts?
(In reply to Patrick McManus from comment #90)

> how is this fixed for sts?

to answer, at least partially, my own question: the sts directive can only be issued across an existing ssl channel.
This is code that should only impact spdy, so it probably makes sense to review it under terms of "no impacts with spdy preffed off". Actually having it in the tree will make it a lot easier to review all the spdy specific code.

I spent some time reading through the coalescing code which was a little bit messy and tried to simplify it somewhat as well as convince myself that we were ok on the "reused cert" front. Attached are the following changes:

* a function to lookup a connection entry for an open nsHttpConnection or queued nsHttpTransaction. The CI hashinfo might not be sufficient given coalescing. While I replaced a bunch of hash lookups with a call to this function, all but the ones dealing with rescheduling priority were doing the right thing (though it was hard to convince yourself for sure).

*removal of the mspdyredir flag which wasn't used for anything.

* only keep a reference to the server cert on preferred (i.e. coalesced to) hosts not ones that are being redirected

* comments and logs

* don't redirect to a host without an active spdy connection. There is no advantage in doing so and the cert can't be relied upon.

* removed a redundant preferred lookup in getconnection - that logic is always done before that function is called
Attachment #570292 - Flags: review?(honzab.moz)
* adds an allowSpdy attribute to nsIHttpChannelInternal (which does not allow you to override the pref)

* nsNSSCallbacks.cpp uses SetAllowSpdy(false) so that the concerns in comment 87 don't result in loops.
Attachment #570293 - Flags: superreview?(cbiesinger)
Attachment #570293 - Flags: review?(honzab.moz)
Comment on attachment 570293 [details] [diff] [review]
patch 15 rev 0 nsihttpinternal no spdy

Review of attachment 570293 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +183,5 @@
>  
> +    /**
> +     * Enable/Disable Spdy negotiation on per channel basis.
> +     */
> +    attribute boolean allowSpdy;

this file needs a new IID
Attachment #570293 - Flags: superreview?(cbiesinger) → superreview+
Attachment #569698 - Flags: superreview?(cbiesinger) → superreview+
as discussed in 696531, disable client certs by policy when using spdy. We can fixup the retry stuff as we go on, but its important to get the policy in at day 1.
Attachment #570339 - Flags: review?(bsmith)
Comment on attachment 569741 [details] [diff] [review]
patch 13.0 reject server cert changes during renegotiation with spdy

Review of attachment 569741 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +1114,5 @@
>    return 0;
>  }
>  
> +// return SECFailure if the new serverCert is different than the old
> +// cert and the socket has negotiated SPDY via NPN

Change comment:

This function assumes that we will only use the SPDY connection coalescing feature on connections where we have negotiated SPDY using NPN. If we ever talk SPDY without having negotiated it with SPDY, this code will give wrong and perhaps unsafe results.

Returns SECSuccess on the initial handshake of all connections, on renegotiations for any connections where we did not negotiate SPDY, or on any SPDY connection where the server's certificate did not change.

@@ +1119,5 @@
> +static SECStatus
> +BlockServerCertChangeForSpdy(PRFileDesc *fd, CERTCertificate *serverCert)
> +{
> +  // Prohibit changing the server cert only if we negotiated SPDY,
> +  // in order to support SPDY's cross-origin connection pooling.

Move this commment up into the comment ahead of the function.

@@ +1132,5 @@
> +    return SECSuccess;
> +  nsCOMPtr<nsIX509Cert2> cert2;
> +  cert2 = do_QueryInterface(cert);
> +  if (!cert2)
> +    return SECSuccess;

Heads up:
Today I will check in a patch that removes nsNSSSocketInfo::GetCert(). You will have to retrieve the certificate like so:

  nsRefPtr<nsSSLStatus> status = infoObject->SSLStatus();
  if (!status) {
    // If we didn't have a status, then this is the
    // first handshake on this connection, not a 
    // renegotiation.
    return SECSuccess;
  }
  status->GetServerCert(getter_AddRefs(cert));
  cert2 = do_QueryInterface(cert);  
  if (!cert2) {
    NS_NOTREACHED("every nsSSLStatus must have a cert"
                  "that implements nsIX509Cert2");
    PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
    return SECFailure; 
  }

@@ +1139,5 @@
> +  nsCAutoString negotiatedNPN;
> +  nsresult rv = infoObject->GetNegotiatedNPN(negotiatedNPN);
> +  if (NS_FAILED(rv) ||
> +      !negotiatedNPN.Equals(NS_LITERAL_CSTRING("spdy/2")))
> +    return SECSuccess;

GetNegotiatedNPN should return NS_OK in the case where nothing was negotiated, and only return an error when it is unable to give a usable result. Then, when NS_FAILED(rv), we should fail the connection because that means we were unable to figure out what protocol, if any, was negotiated.

@@ +1144,5 @@
> +
> +  // Check to see if the cert has actually changed
> +  CERTCertificate * c = cert2->GetCert();
> +  NS_ASSERTION(c, "very bad and hopefully impossible state");
> +  bool sameCert = c == serverCert;

It seems like the author of CERT_CompareCerts disagrees with me that operator== is good enough. Please change this to use CERT_CompareCerts. Sorry.

@@ +1163,5 @@
>  
>    CERTCertificate *serverCert = SSL_PeerCertificate(fd);
>    CERTCertificateCleaner serverCertCleaner(serverCert);
>  
> +  if (BlockServerCertChangeForSpdy(fd, serverCert) == SECFailure)

Yes, please change this to != SECSuccess.
Attachment #569741 - Flags: review?(bsmith) → review+
Comment on attachment 569741 [details] [diff] [review]
patch 13.0 reject server cert changes during renegotiation with spdy

Note that all changes under security/ require sr+ by a PSM peer if the patch was written by somebody who isn't a PSM peer.
Attachment #569741 - Flags: superreview?(honzab.moz)
Comment on attachment 570339 [details] [diff] [review]
patch 16 rev 0 disable client certs

Review of attachment 570339 [details] [diff] [review]:
-----------------------------------------------------------------

I will give you a very quick r+ to the new version.

HEADS UP: today I will land a change to this function for bug 675221. That change will move most of the code in this function to a runnable that runs on the main thread. When we do the merge, we need to put Patrick's additions BEFORE the code that instantiates and dispatches that runnable, not in that runnable.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2838,5 @@
> +  if (NS_SUCCEEDED(rv) && negotiatedNPN.Equals(NS_LITERAL_CSTRING("spdy/2"))) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +           ("[socket=%p] Client cert failed due to spdy\n", socket));
> +    goto loser;
> +  }

Use a C++-style comment. In the comment, include "see BlockServerCertChangeForSpdy for a similar restriction."

info->GetNegotiatedNPN() should only return something other than NS_OK when some error occurs, so if NS_FAILED(rv) then we should PORT_SetError(rv == NS_ERROR_OUT_OF_MEMORY ? SEC_ERROR_NO_MEMORY : SEC_ERROR_LIBRARY_FAILURE); and return SECFailure;

Otherwise, if we negotiated SPDY, we should:

    pRetCert = NULL; 
    pRetKey = NULL;
    return SECSuccess; 

This will send the server a "I won't send you a client certificate" message and continue the connection. (It seems like returning SECFailure will do the same thing, but I think that is a bug in libssl. Regardless, returning SECSuccess makes it clearer that the handshake continues.)
Attachment #570339 - Flags: review?(bsmith) → review-
Comment on attachment 570339 [details] [diff] [review]
patch 16 rev 0 disable client certs

Review of attachment 570339 [details] [diff] [review]:
-----------------------------------------------------------------

Requires sr+ because it is under security/
Attachment #570339 - Flags: superreview?(honzab.moz)
Comment on attachment 570292 [details] [diff] [review]
patch 14 rev 0 spdy coalesce cleanups

Review of attachment 570292 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed only the parts of this patch that use PSM and/or deal with SSL/TLS-specific issues.

Even with the changes I mention below, I am becoming increasingly convinced that this connection coalescing feature is likely to more open to attacks than some may believe. I have sent a preliminary feeler message about this to spdy-dev because I don't have time today to write up the specific problems I see; I am hoping that people will reply saying "we already analyzed that and don't worry because X, Y, and Z." But, if that doesn't happen, and we don't have our own convincing (beyond "Google said so") argument for why the coalescing feature is safe, then we should assume that it isn't safe and also make the following two changes:

1. Set the the coalescing pref off by default. (i.e. require it to be independently enabled independently of enabling SPDY).
2. Add "unsafe" (or whatever we did for WebSockets, if we did something similar) to the name of the pref that controls the coalescing feature.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +552,5 @@
>  
> +        if (cert)
> +            ent->mCert = do_QueryInterface(cert);
> +    }
> +    

nsISSLStatus and nsITransportSecurityInfo are not interfaces to use for making security decisions; they are intended only for displaying information in the UI. AFAICT, no code in netwerk/ (more generally, outside of docshell/ and other UI components) should be using these interfaces at all, other than to pass instances of them between PSM and the UI. (Note that *internally* nsNSSSocketInfo can make decisions based on information in its mSSLStatus member variable, but only because it knows exactly what it put there, when, and why.)

Instead, we should add a new method in nsISSLSocketControl:
    [noscript] boolean joinConnection(
               in nsAString npnProtocol, /* e.g. "spdy/2" */
               in nsAString hostname, 
               in short port);

Note that (npnProtocol, hostname, port) is basically the definition of a web origin, except that here we distinguish between SPDY and HTTP by using the NPN protocol name instead of the scheme.

joinConnection must make AT LEAST the following checks to be safe (if this optimization can be made safe at all; see below):

----- BEGIN -------

If the port is different than the port that the connection is on, or if we haven't negotiated SPDY via NPN yet, return false.

If the hostname is the same as the hostname the connection was already made on then return true.

If we have not yet completed the handshake (set a flag at the end of HandshakeCallback to true), return false. We need to do this because we cannot rely on any information received from the server (including, in particular, the server certificate--even if it was already successfully validated) until the handshake has completed.

If the certificate successfully validated (i.e. SSLStatus().mHaveCertErrorBits) and CERT_VerifyCertName reports that the cert is valid for the given hostname, then return true.

return false.

----- END -------

Some of these checks are likely to already have been done in the code in netwerk/. If that is bothersome, then let's remove the redundant checks in netwerk.

It is likely that in the future we will be able to add code to this method to resolve the issues with client certs (bug 698230) and with server-cert-changing renegotiations (bug 698231).
Attachment #570292 - Flags: review?(honzab.moz) → review-
Comment on attachment 570292 [details] [diff] [review]
patch 14 rev 0 spdy coalesce cleanups

Sorry, I didn't mean to replace the request for Honza's review with my drive-by review.
Attachment #570292 - Flags: review?(honzab.moz)
(In reply to Brian Smith (:bsmith) from comment #101)
> If the certificate successfully validated (i.e.
> SSLStatus().mHaveCertErrorBits)

Instead, we should add to nsSSLStatus :
enum CertStatus { unknown, dv, ev, has_error } mCertStatus (default unknown);
bool IsCertKnownToBeValid() const { return mCertStatus == dv || mCertStatus == ev; }

Do you guys agree that this seems reasonable? If so, I will post a patch on Sunday.
I have noticed in Patrick's patch and in my recommendations that we are waiting a lot for the handshake callback to be called in order to make decisions. In particular we wait for the handshake callback to be called to decide whether to write HTTP or SPDY for a new connection. However, in theory we don't have to wait for the handshake callback to make that determination; if the handshake were tampered with and that caused us to start writing the wrong protocol, then that would be detected by libssl's Finished message checking, and none of our wrong-protocol application data would get sent.

Similarly, when we're trying to decide whether we can add a hostname to an existing connection (coalescing), there are some cases where we can make a yes/no determination much sooner than I described in comment 101. 

In particular, in the psuedocode I mentioned in comment 101, we can check if CERT_VerifCertName rejects the given hostname early (before we check if the handshake callback has been called), and return false ("do not coalesce") right away in that case.

Otherwise, we must wait for the handshake callback to be called before we can return true. But, it would be useful (AFAICT) to return an intermediate  "not yet but probably so try again later" state instead of false like I suggested. But, we shouldn't wait around indefinitely; there must be some kind of timeout. (I have already forgotten whether Patrick's code implements such a timeout. It is too late at night.)
No longer depends on: 696531
Comment on attachment 569698 [details] [diff] [review]
patch 12 negotiatedNPN attribute out of nsIStatus.idl

Review of attachment 569698 [details] [diff] [review]:
-----------------------------------------------------------------

Patrick, if I told you to add this method to nsITransportSecurityInfo, I mis-typed. I meant that this method should be added to nsISSLSocketControl, because nsITransportSecurityInfo is only for UI, not for Necko to use. r+ with the changes below.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +46,5 @@
>  #include "nsISocketTransportService.h"
>  #include "nsISocketTransport.h"
>  #include "nsIServiceManager.h"
>  #include "nsISSLSocketControl.h"
> +#include "nsITransportSecurityInfo.h"

Remove this include.

@@ +170,2 @@
>      nsCOMPtr<nsISupports> sslstatus;
> +    nsCOMPtr<nsITransportSecurityInfo> tsi;

instead:
  nsCOMPtr<nsISSLSocketControl> ssl;

s/tsi/ssl/ below, and remove the declaration of ssl below.

@@ +184,5 @@
>      
>          nsCOMPtr<nsISSLSocketControl> ssl =
>              do_QueryInterface(securityInfo, &rv);
>          if (NS_FAILED(rv))
>              goto npnComplete;

This block can be removed once you make the above changes.

::: netwerk/socket/nsITransportSecurityInfo.idl
@@ +39,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "nsISupports.idl"
>  
> +[scriptable, uuid(e01ffe61-6262-459b-aa6c-e4ef90ca2c2b)]

Do not change the UUID of nsITransportSecurityInfo. Instead, change the UUID of nsISSLSocketControl.

@@ +50,5 @@
> +     * or if the server did not select any protocol choice from that
> +     * list. That also includes the case where the server does not
> +     * implement NPN.
> +     */
> +    readonly attribute ACString negotiatedNPN;

Move this to nsISSLSocketControl. Add a comment that says that this raises NS_ERROR_NOT_CONNECTED if/when the handshake hasn't progressed to where NPN is available.

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +40,5 @@
>  #include "nsISupports.idl"
>  
>  interface nsIX509Cert;
>  
> +[scriptable, uuid(cfede939-def1-49be-81ed-d401b3a07d1c)]

Good, this is the original UUID.

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +248,5 @@
>    nsSSLSocketThreadData *mThreadData;
>  
>    nsresult EnsureDocShellDependentStuffKnown();
>  
> +  // For nsITransportSecurityInfo NPN information

Remove this comment. (With the suggested changes, it wouldn't be nsITransportSecurityInfo but nsISSLSocketControl. But, we can figure that out without the comment.)
Attachment #569698 - Flags: review?(bsmith) → review+
(In reply to Brian Smith (:bsmith) from comment #104)
> However, in
> theory we don't have to wait for the handshake callback to make that
> determination; 

I think that's good! we need the change to nss so that the npnCallback is called even when npn isn't used - yes?
Comment on attachment 569741 [details] [diff] [review]
patch 13.0 reject server cert changes during renegotiation with spdy

Review of attachment 569741 [details] [diff] [review]:
-----------------------------------------------------------------

sr=honzab
Attachment #569741 - Flags: superreview?(honzab.moz) → superreview+
(In reply to Brian Smith (:bsmith) from comment #101)
> Even with the changes I mention below, I am becoming increasingly convinced
> ....
> 1. Set the the coalescing pref off by default. (i.e. require it to be
> independently enabled independently of enabling SPDY).
> 2. Add "unsafe" (or whatever we did for WebSockets, if we did something
> similar) to the name of the pref that controls the coalescing feature.

I would agree.  I also don't have a strong feeling this is going to be either safe or reliable.  My vote for disabling it.
Comment on attachment 569741 [details] [diff] [review]
patch 13.0 reject server cert changes during renegotiation with spdy

Review of attachment 569741 [details] [diff] [review]:
-----------------------------------------------------------------

sr=honzab ...with all the comments addressed first.
> Heads up:
> Today I will check in a patch that removes nsNSSSocketInfo::GetCert(). You
> will have to retrieve the certificate like so:
> 

Brian, thanks for this!
Attached patch patch 0 rev 1 (obsolete) — Splinter Review
Attachment #563757 - Attachment is obsolete: true
Attachment #570805 - Flags: superreview+
Attachment #570805 - Flags: review+
Attachment #563757 - Flags: review?
Attachment #563757 - Flags: feedback?(mike)
Attachment #565265 - Attachment is obsolete: true
Attachment #570814 - Flags: review+
Attachment #565539 - Attachment is obsolete: true
Attachment #570817 - Flags: review+
Attachment #565969 - Attachment is obsolete: true
Attachment #570819 - Flags: review+
Attachment #568412 - Attachment is obsolete: true
Attachment #570825 - Flags: superreview+
Attachment #570825 - Flags: review+
Attachment #567765 - Attachment is obsolete: true
Attachment #568946 - Attachment is obsolete: true
Attachment #570826 - Flags: review+
Attachment #569698 - Attachment is obsolete: true
Attachment #570828 - Flags: superreview+
Attachment #570828 - Flags: review+
Comment on attachment 570292 [details] [diff] [review]
patch 14 rev 0 spdy coalesce cleanups

Review of attachment 570292 [details] [diff] [review]:
-----------------------------------------------------------------

This fixes also bug 666028.

r=honzab with spdy.enabled = false.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +552,5 @@
>  
> +        if (cert)
> +            ent->mCert = do_QueryInterface(cert);
> +    }
> +    

Re Brian's comment on this: good idea.

@@ +1664,5 @@
>          }
>      }
>   
> +    if (ci)
> +        OnMsgProcessPendingQ(NS_OK, ci); // releases |ci|

This breaks parity with the current code.  I understand that not having entry here is something that should never happen, but rather leave the code behavior unchanged.

Fill ci with conn->ConnectionInfo() or ent->mConnInfo, when ent available.  Only after that addref.
Attachment #570292 - Flags: review?(honzab.moz) → review+
Attachment #569741 - Attachment is obsolete: true
Attachment #570834 - Flags: superreview+
Attachment #570834 - Flags: review+
Comment on attachment 570293 [details] [diff] [review]
patch 15 rev 0 nsihttpinternal no spdy

Review of attachment 570293 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab for spdy.enabled = false.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1313,5 @@
>  
> +NS_IMETHODIMP
> +HttpBaseChannel::GetAllowSpdy(bool *aAllowSpdy)
> +{
> +  *aAllowSpdy = mAllowSpdy;

Check on the arg being non-null first.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +516,5 @@
>          }
>      }
>  
> +    if (!mAllowSpdy)
> +        mCaps |=  NS_HTTP_DISALLOW_SPDY;

Two spaces between |= and NS_HTTP_DISALLOW_SPDY

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +181,5 @@
>      void HTTPUpgrade(in ACString aProtocolName,
>                       in nsIHttpUpgradeListener aListener);
>  
> +    /**
> +     * Enable/Disable Spdy negotiation on per channel basis.

Needs a comment how this behaves regarding the preference state.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +158,5 @@
> +  // operation to fufill something like a CRL fetch, which is an
> +  // endless loop.
> +  nsCOMPtr<nsIHttpChannelInternal> internalChannel = do_QueryInterface(chan);
> +  if (internalChannel)
> +    internalChannel->SetAllowSpdy(false);

Check for result.

@@ +164,3 @@
>    nsCOMPtr<nsIHttpChannel> hchan = do_QueryInterface(chan);
>    NS_ENSURE_STATE(hchan);
> +  

Remove this blank line change.
Attachment #570293 - Flags: review?(honzab.moz) → review+
Attachment #570834 - Attachment is patch: true
Comment on attachment 570825 [details] [diff] [review]
patch 9 rev 1coalesce spdy servers requires cert check

Review of attachment 570825 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +593,5 @@
> +    nsresult rv;
> +    bool validCert = false;
> +
> +    rv = preferred->mCert->IsValidForHostname(
> +        aOriginalEntry->mConnInfo->GetHost(), &validCert);

This is still missing the check for against certificate overrides that I mentioned in the previous review.

Assuming that the changes to the coalescing feature (renamed to "unsafe" and default off) are made, we can handle this in a follow-up bug. In that case, you can change my r- to r+.
Attachment #570825 - Flags: review+ → review-
Comment on attachment 570828 [details] [diff] [review]
patch 12 rev 1 dont touch nsisslstatus

Review of attachment 570828 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is basically reverting some of the changes in patch 0. If possible, please merge this patch and patch 0 together before qfinishing them, so that it is clearer in the revision history that nsISSLStatus didn't change.
Comment on attachment 570339 [details] [diff] [review]
patch 16 rev 0 disable client certs

Review of attachment 570339 [details] [diff] [review]:
-----------------------------------------------------------------

Let me check the new version please.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2834,5 @@
>  
> +  /* right now spdy is not allowed to use client certs */
> +  nsCAutoString negotiatedNPN;
> +  nsresult rv = info->GetNegotiatedNPN(negotiatedNPN);
> +  if (NS_SUCCEEDED(rv) && negotiatedNPN.Equals(NS_LITERAL_CSTRING("spdy/2"))) {

I think you can use EqualsLiteral here.

The condition should be if (NS_FAILED(rv) || etc..)
Attachment #570339 - Flags: superreview?(honzab.moz)
Comment on attachment 570817 [details] [diff] [review]
patch 4 rev 1 fancy cert failure pages

Review of attachment 570817 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsSSLThread.cpp
@@ +1224,1 @@
>                bstd.mPRErrorCode = PR_GetError();

Missing the comment, why call to checkHandshare is needed here.
The target for landing the preffed off version of spdy is nov 14, fairly early in the firefox 11 nightly window.
-----
> 
> ::: security/manager/ssl/src/nsSSLThread.cpp
> @@ +1224,1 @@
> >                bstd.mPRErrorCode = PR_GetError();
> 
> Missing the comment, why call to checkHandshare is needed here.

There is a comment there - what would you like it to say?

+          if (SSL_ForceHandshake(realFileDesc) != PR_SUCCESS) {
+              // Possibly retry due to TLS intolerance
+              checkHandshake(-1, PR_TRUE, realFileDesc, mBusySocket);
               bstd.mPRErrorCode = PR_GetError();
+          }
Attachment #570339 - Attachment is obsolete: true
Attachment #570913 - Flags: superreview?(honzab.moz)
Attachment #570913 - Flags: review?(bsmith)
(In reply to Brian Smith (:bsmith) from comment #99)

> HEADS UP: today I will land a change to this function for bug 675221. That
> change will move most of the code in this function to a runnable that runs
> on the main thread. When we do the merge, we need to put Patrick's additions
> BEFORE the code that instantiates and dispatches that runnable, not in that
> runnable.
>

again, very useful. Thanks!
in comment 124 brian asked that patch 12 (also reviewed) be rolled into patch 0. Normally I don't want to do that, but in this case it prevents confusing uuid churn in a idl's history so it makes sense.
Attachment #570805 - Attachment is obsolete: true
Attachment #570828 - Attachment is obsolete: true
Attachment #570918 - Flags: superreview+
Attachment #570918 - Flags: review+
Attachment #570918 - Attachment is patch: true
(In reply to Patrick McManus from comment #129)
> There is a comment there - what would you like it to say?
> 
> +          if (SSL_ForceHandshake(realFileDesc) != PR_SUCCESS) {
> +              // Possibly retry due to TLS intolerance
> +              checkHandshake(-1, PR_TRUE, realFileDesc, mBusySocket);

Total overlook, sorry.  I finally started process of buying new glasses today!  Just in time :)

BTW: would be nice if splinter would have syntax highlighting.
Comment on attachment 570913 [details] [diff] [review]
patch 16 rev 1 disable client certs

Review of attachment 570913 [details] [diff] [review]:
-----------------------------------------------------------------

sr=honzab

Thanks.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2844,5 @@
> +
> +  if (NS_FAILED(rv)) {
> +    PORT_SetError(rv == NS_ERROR_OUT_OF_MEMORY ?
> +                  SEC_ERROR_NO_MEMORY : SEC_ERROR_LIBRARY_FAILURE);
> +     return SECFailure;

Indention.

@@ +2849,5 @@
> +  }
> +
> +  if (negotiatedNPN.EqualsLiteral("spdy/2")) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +           ("[socket=%p] Client cert failed due to spdy\n", socket));

This should better be: "Not returning client cert due to npn=spdy/2"
Attachment #570913 - Flags: superreview?(honzab.moz) → superreview+
In comment 101 brian requested an interface in nsISSLSocketControl to determine whether another potential connection can be coalesced with the existing connection represented by the nsISSLSocketControl object and to use that interface rather than having necko just hold onto the cert and check it directly. I have implemented that in this patch - it means the nshttpconnectionmgr no longer needs a reference to the server's cert.

It also means that we test (via certerrorbits) for stored exceptions before coalescing, not just whether the cert being used covers the alternate hostname which had been an outstanding to do item.

it encompasses comments to both patches 9 and 14 from brian. It made more sense to just create a new patch to deal with the issue rather than muddling the other stuff those patches already did, but I hope with its application it resolves objections to those patches.

Finally, comment 103 suggested an alternate interface might be implemented rather than just checking mHaveCertErrorBits directly. That patch hasn't made an appearance yet, but if it does I'm happy to update this patch to incorporate it.

As for the long list of review requests, I wish it could be shorter :) - but here's what I am asking in partcular wrt this patch:

brian: r? security/*
hozna: r? netwerk/* sr? security/*
biesi: sr? idl change
Attachment #571434 - Flags: superreview?(honzab.moz)
Attachment #571434 - Flags: review?(honzab.moz)
Attachment #571434 - Flags: review?(bsmith)
Comment on attachment 571434 [details] [diff] [review]
patch 17 rev 0  nsisslsocketcontrol::mayjoinconnection()

Christian, bugzilla won't let me set a second sr? flag (even with comma notation), but I'm asking you for one by setting r? for the idl change see comment 135.
Attachment #571434 - Flags: review?(cbiesinger)
Comment on attachment 571434 [details] [diff] [review]
patch 17 rev 0  nsisslsocketcontrol::mayjoinconnection()

Review of attachment 571434 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: netwerk/socket/nsISSLSocketControl.idl
@@ +88,5 @@
> +    /* Determine if a potential SSL connection to hostname:port with
> +     * a desired NPN negotiated protocol of npnProtocol can use use the socket
> +     * associated with this object instead of making a new one.
> +     */
> +    [noscript] boolean mayJoinConnection(

Why make this noscript?

@@ +91,5 @@
> +     */
> +    [noscript] boolean mayJoinConnection(
> +      in ACString npnProtocol, /* e.g. "spdy/2" */
> +      in ACString hostname,
> +      in short port);

Necko generally uses long for ports (e.g. nsISocketTransport), please follow that convention.
Attachment #571434 - Flags: review?(cbiesinger) → review+
the spdy session header decompression code was accidentally growing the buffer on each iteration. That kinda looked like a memory leak because it wouldn't be cleaned up until the long lived session died.
Attachment #574325 - Flags: review?(honzab.moz)
Comment on attachment 574325 [details] [diff] [review]
patch 18 rev 0 decompress buffer sizing

Review of attachment 574325 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab for spdy.enabled = false
Attachment #574325 - Flags: review?(honzab.moz) → review+
Comment on attachment 571434 [details] [diff] [review]
patch 17 rev 0  nsisslsocketcontrol::mayjoinconnection()

Review of attachment 571434 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab for the network parts and spdy.enabled = false.
sr=honzab for the security parts.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +620,2 @@
>              break;
>          }

For should have a block here: for (;;) { if () { } }

@@ +639,3 @@
>  
> +    nsCOMPtr<nsISupports> securityInfo;
> +    nsCOMPtr<nsISSLSocketControl> ssl;

s/ssl/sslSocketControl/ to make it better readable?

::: netwerk/socket/nsISSLSocketControl.idl
@@ +85,5 @@
>       */
>      readonly attribute ACString negotiatedNPN;
> +
> +    /* Determine if a potential SSL connection to hostname:port with
> +     * a desired NPN negotiated protocol of npnProtocol can use use the socket

can use use the - typo

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +982,3 @@
>    }
>  
> +  

Accidental blank line.
Attachment #571434 - Flags: superreview?(honzab.moz)
Attachment #571434 - Flags: superreview+
Attachment #571434 - Flags: review?(honzab.moz)
Attachment #571434 - Flags: review+
Comment on attachment 571434 [details] [diff] [review]
patch 17 rev 0  nsisslsocketcontrol::mayjoinconnection()

Review of attachment 571434 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed only the PSM and PSM-facing changes.

I am not not convinced that we should be doing the connection coalescing this way, but it is good to have an implementation we can experiment with to make an informed decision.

r+ assuming you address the comments. Please post the new patch.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +653,5 @@
> +                                aOriginalEntry->mConnInfo->GetHost(),
> +                                aOriginalEntry->mConnInfo->Port(),
> +                                &isJoinable);
> +
> +    if (NS_FAILED(rv) || !isJoinable) {

Rename "MayJoinConnection" to "JoinConnection" and "isJoinable" to "joined". It should (eventually) have the semantics that, when the result is joined == true, that the connection will be in "joined" mode (i.e. client certs disabled, server cert changes disabled).

Then, we can replace the "negotiated SPDY" tests in nsNSSSocketInfo with "did a previous JoinConnection succeed?" This way, the restrictions we are placing on TLS features will not affect all SPDY servers but instead only ones that are affected by the joining feature.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +579,5 @@
> +  if (!mHandshakeCompleted || !SSLStatus() || !SSLStatus()->mServerCert)
> +    return NS_OK;
> +
> +  // If the cert has error bits (e.g. it is untrusted) then do not join
> +  if (SSLStatus()->mHaveCertErrorBits)

Add a comment that we can only rely on the value of mHaveCertErrorBits because we know that the handshake completed. (Until then, unfortunately, mHaveCertErrorBits may not be set).

Also, add a comment to nsSSLStatus for mHaveCertErrorBits indicating that this is now being relied on for this function.

@@ +589,5 @@
> +  bool validCert = false;
> +  if (!cert3)
> +    return NS_OK;
> +  
> +  nsresult rv = cert3->IsValidForHostname(hostname, &validCert);

I am sorry I didn't catch this before. "Valid" has a specific meaning with certificates and we should avoid using that name. I think with the way the code is organized now (i.e. this is inside PSM), you should just revert the addition of IsValidForHostname to nsIX509Cert3 and call CERT_VerifyCertName directly.
Attachment #571434 - Flags: review?(bsmith) → review+
Comment on attachment 570913 [details] [diff] [review]
patch 16 rev 1 disable client certs

Review of attachment 570913 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2851,5 @@
> +  if (negotiatedNPN.EqualsLiteral("spdy/2")) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +           ("[socket=%p] Client cert failed due to spdy\n", socket));
> +    pRetCert = NULL;
> +    pRetKey = NULL;

*pRetCert = nsnull;
*pRetKey = nsnull;
Comment on attachment 570913 [details] [diff] [review]
patch 16 rev 1 disable client certs

Review of attachment 570913 [details] [diff] [review]:
-----------------------------------------------------------------

r+ after making the fixes below.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2851,5 @@
> +  if (negotiatedNPN.EqualsLiteral("spdy/2")) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +           ("[socket=%p] Client cert failed due to spdy\n", socket));
> +    pRetCert = NULL;
> +    pRetKey = NULL;

*pRetCert = nsnull;
*pRetKey = nsnull;
Attachment #570913 - Flags: review?(bsmith) → review+
(In reply to Brian Smith (:bsmith) from comment #143)

> > +    pRetCert = NULL;
> > +    pRetKey = NULL;
> 
> *pRetCert = nsnull;
> *pRetKey = nsnull;

That's embarrassing - thanks!
(In reply to Brian Smith (:bsmith) from comment #141)
> Comment on attachment 571434 [details] [diff] [review] [diff] [details] [review]
> patch 17 rev 0  nsisslsocketcontrol::mayjoinconnection()

> Then, we can replace the "negotiated SPDY" tests in nsNSSSocketInfo with
> "did a previous JoinConnection succeed?" This way, the restrictions we are
> placing on TLS features will not affect all SPDY servers but instead only
> ones that are affected by the joining feature.
> 

can you clarify if you are expecting this change as part of your review or as future work. Thanks.
(In reply to Patrick McManus from comment #145)
> can you clarify if you are expecting this change as part of your review or
> as future work. Thanks.

Just do the renaming. Loosening the restrictions on client certs and renegotiations requires some thought that I don't have time for right now and I don't want to block your work (any further).
updated with brian's review feedback.

It would be useful if nsix509cert2::getCert contained documentation on the reference implications of getCert(). I think I have the handling right, but please check.

nsix509cert3.idl has been reverted to match mozilla central; honza and I are trying to make this patch queue append only as much as is feasible so I plan to live with the hg churn.
Attachment #571434 - Attachment is obsolete: true
Attachment #575750 - Flags: superreview+
Attachment #575750 - Flags: review+
Rev 1 of this patch regressed test_bug466080.html because GetNegotiatedNPN() requires the handshake to be complete, and the client cert fufillment can be part of the handshake.

Rev 2 changes things to use SSL_GetNextProto which I believe is valid after Server Hello.
Attachment #570913 - Attachment is obsolete: true
Attachment #575765 - Flags: superreview?(honzab.moz)
Attachment #575765 - Flags: review?(bsmith)
Attachment #575750 - Attachment description: nsisslsocketcontrol::joinconnection() → patch 17 rev 1 nsisslsocketcontrol::joinconnection()
Comment on attachment 575765 [details] [diff] [review]
patch 16 rev 2 disable client certs

This is going to be replaced by patch 19
Attachment #575765 - Attachment is obsolete: true
Attachment #575765 - Flags: superreview?(honzab.moz)
Attachment #575765 - Flags: review?(bsmith)
A connection that has sent a client cert is not joinable, and a joined connection may not send a client cert.

For the purposes of this comment a joined connection does not include two connections to the same hostname.
Attachment #575985 - Flags: superreview?(honzab.moz)
Attachment #575985 - Flags: review?(bsmith)
Comment on attachment 575985 [details] [diff] [review]
patch 19 rev 0 restrict client certs and joinability

Review of attachment 575985 [details] [diff] [review]:
-----------------------------------------------------------------

r+. Please address the nits.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +508,5 @@
>    // the handshake completed.
>    if (SSLStatus()->mHaveCertErrorBits)
>      return NS_OK;
>  
> +  // If the connection is using client certificates then do not join

Add:
// because the user decides on whether to send client certs to hosts on a per-domain basis.

@@ +2985,5 @@
>      PR_SetError(SSL_ERROR_NO_CERTIFICATE, 0);
>      return SECFailure;
>    }
>  
> +  if (info->GetJoined()) {

Add the comment:

"We refuse to send a client certificate when there are multiple hostnames joined on this connection, because we only show the user one hostname (mHostName) in the client certificate UI."

(By the way, this suggests we might solve some of this problem by changing the client cert UI, e.g. by adding an "automatically send this certificate for all subdomains of *.hostname.tld" checkbox. Obviously, out of scope for this bug.)

@@ +2987,5 @@
>    }
>  
> +  if (info->GetJoined()) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +           ("[socket=%p] Not returning client cert due to previous join\n", socket));

Normally, at least in PSM, we just use [%p], not [socket=%p]. Using [%p] makes the logs easier to read and filter when we want to see the sequence of events of things on a socket.
Attachment #575985 - Flags: review?(bsmith) → review+
(In reply to Brian Smith (:bsmith) from comment #151)
> Comment on attachment 575985 [details] [diff] [review] [diff] [details] [review]
> patch 19 rev 0 restrict client certs and joinability
> 
> Review of attachment 575985 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> 
Thanks Brian.
TCP connection management is really the focus of SPDY, so I have created a patch to help us analyze through telemetry whether or not we are making progress.

(fun aside - yahoo! just published a paper saying their servers see a median connection with 2.5 requests and just 10KB of data transferred. That's a nightmare).

the patch tracks:

* the number of requests per connection broken down to http/1 vs spdy (and it fixes a typo in the range of the existing spdy telemetry for this)

* the amount of data transferred per connection broken down to http/1 vs spdy

* how often we try and coalesce and how often that succeeds
Attachment #576168 - Flags: review?(honzab.moz)
This is a version (right now fyi) of the base spdy patch (patch 0) that has been integrated with brian's no-sslthread tree.
This is a version (right now fyi) of the patch 13 that has been integrated with brian's no-sslthread tree. The only change is to the filenames as the relevant code moved locations.
Comment on attachment 576199 [details] [diff] [review]
no sslthread version of Patch 0 (base spdy)

Review of attachment 576199 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +178,5 @@
> +
> +    rv = ssl->GetNegotiatedNPN(negotiatedNPN);
> +    if (rv == NS_ERROR_NOT_CONNECTED) {
> +    
> +        rv = ssl->ForceHandshake();

Patrick, can you try replacing this with PR_Send() or PR_Write() with an empty input buffer, to see if you get the correct results? If so, let's do things that way. The NSS implementation of PR_Send/PR_Write has been changed to make it do (AFAICT) everything that SSL_ForceHandshake does, and using PR_Send/PR_Write avoids the issues noted below.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +2347,5 @@
>    return mWarnLevelMissingRFC5746;
>  }
>  
> +NS_IMETHODIMP
> +nsNSSSocketInfo::ForceHandshake()

I think this function should return a SECStatus (as an output parameter, I guess), so that the SECStatus can be interpreted the same way we interpret the result from PR_Send/PR_Write. Otherwise, we end up with two error handling paths at the Necko later.

@@ +2350,5 @@
> +NS_IMETHODIMP
> +nsNSSSocketInfo::ForceHandshake()
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown())

Also, we have to check for logout. See getSocketInfoIfRunning in the SSL thread removal patch.

@@ +2358,5 @@
> +
> +  if (rv == SECSuccess)
> +    return NS_OK;
> +
> +  // consider TLS intolerance

TLS intolerance isn't the only reason for calling checkHandshake. It is also used to set the error code for the socket so that nsNSSSocketInfo's implementation of nsITransportSecurityInfo::GetErrorMessage() returns the correct message for the error page.

@@ +2365,5 @@
> +  if (rv == SECWouldBlock)
> +    return NS_OK;
> +
> +  if (rv == SECFailure && PORT_GetError() == PR_WOULD_BLOCK_ERROR)
> +    return NS_OK;

These two checks above should happen in Necko, like the PR_Send/PR_Write/PR_Read/PR_Recv cases.

AFAICT, this method should just always return NS_OK, and the caller should check both NS_FAILED(rv) and the SECStatus output parameter.
Attachment #576199 - Flags: review-
(In reply to Brian Smith (:bsmith) from comment #156)
> @@ +2350,5 @@
> > +NS_IMETHODIMP
> > +nsNSSSocketInfo::ForceHandshake()
> > +{
> > +  nsNSSShutDownPreventionLock locker;
> > +  if (isAlreadyShutDown())
> 
> Also, we have to check for logout. See getSocketInfoIfRunning in the SSL
> thread removal patch.

We would also have to check that GetErrorCode() != 0; otherwise, we won't detect certificate validation failure and Necko will loop forever calling ForceHandshake.

If we have to keep this ForceHandshake function, then it would be best to just call getSocketInfoIfRunning(mFd) to ensure all the necessary checks are done.
> things that way. The NSS implementation of PR_Send/PR_Write has been changed
> to make it do (AFAICT) everything that SSL_ForceHandshake does, and using
> PR_Send/PR_Write avoids the issues noted below.
> 

That's a big improvement! and indeed, it works fine for test cases of:
* https
* spdy
* cert failure/self signed cert popup

This is going to make the rest of comments 156 and 157 moot so I won't address them in detail.

I'll post an update. I'd appreciate it you'd add your r+ to it.

> We would also have to check that GetErrorCode() != 0; otherwise, we won't
> detect certificate validation failure and Necko will loop forever calling
> ForceHandshake.
> 

fwiw, that wasn't true. Port_getError() was finding the cert validation failure and aborting that loop. But its moot in any event.
Rev 2 of Patch 0 (main spdy patch) based on brian's no-sslthread work (which is not yet landed). Honza, Christian, I've forwarded your earlier r+'s as this is integration work with brian's code but feel free of course to look it over.
Attachment #576199 - Attachment is obsolete: true
Attachment #577291 - Flags: superreview+
Attachment #577291 - Flags: review?(bsmith)
Attachment #577291 - Flags: review+
Comment on attachment 577291 [details] [diff] [review]
no sslthread version of Patch 0 (base spdy) rev1

I reviewed just the interdiff between rev 0 and rev 1, which removed the forceHandshake method from nsISSLSocketControl and nsNSSSocketInfo, and replaced the usage of it with an empty write.
Attachment #577291 - Flags: review?(bsmith) → review+
(In reply to Patrick McManus from comment #159)
> no sslthread version of Patch 0 (base spdy) rev1
> 
> Rev 2 of Patch 0

Note that in one place you say "rev1" and another place you say "rev 2". Make sure you have posted the correct version.

> fwiw, that wasn't true. Port_getError() was finding the cert validation
> failure and aborting that loop. But its moot in any event.

Interesting. I guess this must be because some other PR_* method was called beforehand. I don't think this would have been a reliable method of detecting errors, in any case.
(In reply to Brian Smith (:bsmith) from comment #141)
> Comment on attachment 571434 [details] [diff] [review] [diff] [details] [review]
> patch 17 rev 0  nsisslsocketcontrol::mayjoinconnection()
> 
> Review of attachment 571434 [details] [diff] [review] [diff] [details] [review]:

> Rename "MayJoinConnection" to "JoinConnection" and "isJoinable" to "joined".

I forgot to comment on this:  I'm not a big fun of naming a method as it would be "executable" i.e. as it would do an operation, though it doesn't.

I was much more fun of MayJoinConnection.  Other proposals:  CheckMayJoinConnection (similar with principal's CheckMayLoad()), CanJoinConnection, IsJoinableConnection.

The method just decides if something can be done, it doesn't join the two connections it self.
(In reply to Brian Smith (:bsmith) from comment #161)
>
> 
> Note that in one place you say "rev1" and another place you say "rev 2".
> Make sure you have posted the correct version.
> 

yep it's cool - checked the contents. It is rev 1 starting at rev 0 (which is my normal way, I just slipped up).
Comment on attachment 575985 [details] [diff] [review]
patch 19 rev 0 restrict client certs and joinability

Review of attachment 575985 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +530,5 @@
>        SECSuccess)
>      return NS_OK;
>  
>    // All tests pass - this is joinable
> +  mJoined = true;

What if the consumer of this method's result later decides not to join?

@@ +2985,5 @@
>      PR_SetError(SSL_ERROR_NO_CERTIFICATE, 0);
>      return SECFailure;
>    }
>  
> +  if (info->GetJoined()) {

Not sure 100%, but I think we can get here only on renegotiation.  And that should be disabled for SPDY connections, or not?

@@ +3004,5 @@
>    }
>    
>    if (runnable->mRV != SECSuccess) {
>      PR_SetError(runnable->mErrorCodeToReport, 0);
> +  } else if (*runnable->mPRetCert || *runnable->mPRetKey) {

Maybe the runnable could set the flag it self?  It refers the info object.

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +210,5 @@
>    nsCString mNegotiatedNPN;
>    bool      mNPNCompleted;
>    bool      mHandshakeCompleted;
> +  bool      mJoined;
> +  bool      mSentClientCert;

Rather mClientCertSent ?  Up to you, but this could be misread as mSendClientCert.
Attachment #575985 - Flags: superreview?(honzab.moz) → superreview+
(In reply to Honza Bambas (:mayhemer) from comment #162)
> (In reply to Brian Smith (:bsmith) from comment #141)
> > Rename "MayJoinConnection" to "JoinConnection" and "isJoinable" to "joined".
> 
> I forgot to comment on this:  I'm not a big fun of naming a method as it
> would be "executable" i.e. as it would do an operation, though it doesn't.

> The method just decides if something can be done, it doesn't join the two
> connections it self.

That is exactly why I suggested it be renamed. A name like "MayJoinConnection" implies there aren't any effects. JoinConnection has important effects in that it changes the behavior of client certificates and renegotiations on the connection.

Also, "JoinConnection" implies that an effect will happen, so that it should be called only when we want an effect to occur (i.e. after we've done all the other checks we need to do to decide whether to join the connections). In your review comment, you asked "what if the caller decides not to join," but the idea here is that the caller must decide whether or not to join before calling JoinConnection; afterward, it is too late to change your mind, because the effects have already occurred and we have no way to undo them.
Thanks Honza!

(In reply to Honza Bambas (:mayhemer) from comment #164)

> > +  mJoined = true;
> 
> What if the consumer of this method's result later decides not to join?
> 

Then they get the more conservative (joined behavior). not optimal (but then again neither is calling this method and then not joining) - but safe.

> @@ +2985,5 @@
> >      PR_SetError(SSL_ERROR_NO_CERTIFICATE, 0);
> >      return SECFailure;
> >    }
> >  
> > +  if (info->GetJoined()) {
> 
> Not sure 100%, but I think we can get here only on renegotiation.  And that
> should be disabled for SPDY connections, or not?

We can get here on the original handshake (I learned that in comment 148), and this change satisfies brian's request that we now allow client certs on non-joined connections (and likewise not join connections using client certs). See a mail you were cc'd on from the 21st called "Spdy - patch 16 problem".. it also will allow easier experimentation with OBC.


> 
> Maybe the runnable could set the flag it self?  It refers the info object.

Its 50/50 to me so I coded it the way brian suggested in pseudo code. Let me know if you think it is worth further debate.
(In reply to Brian Smith (:bsmith) from comment #165)
> (In reply to Honza Bambas (:mayhemer) from comment #162)
> > (In reply to Brian Smith (:bsmith) from comment #141)
> > > Rename "MayJoinConnection" to "JoinConnection" and "isJoinable" to "joined".
> > 
> > I forgot to comment on this:  I'm not a big fun of naming a method as it
> > would be "executable" i.e. as it would do an operation, though it doesn't.
> 
> > The method just decides if something can be done, it doesn't join the two
> > connections it self.
> 
> That is exactly why I suggested it be renamed. A name like
> "MayJoinConnection" implies there aren't any effects. JoinConnection has
> important effects in that it changes the behavior of client certificates and
> renegotiations on the connection.
> 
> Also, "JoinConnection" implies that an effect will happen, so that it should
> be called only when we want an effect to occur (i.e. after we've done all
> the other checks we need to do to decide whether to join the connections).
> In your review comment, you asked "what if the caller decides not to join,"
> but the idea here is that the caller must decide whether or not to join
> before calling JoinConnection; afterward, it is too late to change your
> mind, because the effects have already occurred and we have no way to undo
> them.

I don't agree.  I don't see a single "affect" the method does.  It just checks states of things and returns the result of those checks.  Nothing more.  It doesn't change anything.  The name is IMO misleading.
(In reply to Patrick McManus from comment #166)
> Thanks Honza!
> 
> (In reply to Honza Bambas (:mayhemer) from comment #164)
> 
> > > +  mJoined = true;
> > 
> > What if the consumer of this method's result later decides not to join?
> > 
> 
> Then they get the more conservative (joined behavior). not optimal (but then
> again neither is calling this method and then not joining) - but safe.

I think the code architecture as is doesn't allow anything more elegant or safe.  I don't see a different way at the moment.  Probably not that important to deal with now.

> 
> > @@ +2985,5 @@
> > >      PR_SetError(SSL_ERROR_NO_CERTIFICATE, 0);
> > >      return SECFailure;
> > >    }
> > >  
> > > +  if (info->GetJoined()) {
> > 
> > Not sure 100%, but I think we can get here only on renegotiation.  And that
> > should be disabled for SPDY connections, or not?
> 
> We can get here on the original handshake (I learned that in comment 148),
> and this change satisfies brian's request that we now allow client certs on
> non-joined connections (and likewise not join connections using client
> certs). See a mail you were cc'd on from the 21st called "Spdy - patch 16
> problem".. it also will allow easier experimentation with OBC.

I meant this particular code branch: if (info->GetJoined()).  mJoined cannot be set sooner then after the initial handshake has been done.  So, we can execute this code only on renegotiation.  But that is not that important.  Not a review request to investigate on this..

> 
> 
> > 
> > Maybe the runnable could set the flag it self?  It refers the info object.
> 
> Its 50/50 to me so I coded it the way brian suggested in pseudo code. Let me
> know if you think it is worth further debate.

For me it seems to be more encapsulated.  It is a single purpose piece of code, it can be put in that class IMO.  That is not a review request too..
(In reply to Honza Bambas (:mayhemer) from comment #167)
> I don't agree.  I don't see a single "affect" the method does.  It just
> checks states of things and returns the result of those checks.  Nothing
> more.  It doesn't change anything.  The name is IMO misleading.

Currently, it sets mJoined, which results in important effects.

In the future, it will build a set of joined domain names (replacing mHostName), so that SSL-level error messages indicate that the error affects all the domains, and so that we can remove the "server cannot change its certificate during renegotiation for SPDY" restriction.

(Notice that with the current patches, if foo.com is joined onto an existing bar.com connection, SSL-level and probably NSPR-level network errors will give only bar.com as the affected site, which is misleading.)
(In reply to Brian Smith (:bsmith) from comment #169)
> (In reply to Honza Bambas (:mayhemer) from comment #167)
> > I don't agree.  I don't see a single "affect" the method does.  It just
> > checks states of things and returns the result of those checks.  Nothing
> > more.  It doesn't change anything.  The name is IMO misleading.
> 
> Currently, it sets mJoined, which results in important effects.
> 
> In the future, it will build a set of joined domain names (replacing
> mHostName), so that SSL-level error messages indicate that the error affects
> all the domains, and so that we can remove the "server cannot change its
> certificate during renegotiation for SPDY" restriction.
> 
> (Notice that with the current patches, if foo.com is joined onto an existing
> bar.com connection, SSL-level and probably NSPR-level network errors will
> give only bar.com as the affected site, which is misleading.)

OK.  Then the method should throw an exception when it decides not to join and not just return a bool out param.
(In reply to Honza Bambas (:mayhemer) from comment #170)
> OK.  Then the method should throw an exception when it decides not to join
> and not just return a bool out param.

Deciding not to join is not an error. Exceptions (non-NS_OK results) should be reserved for when the function is unable to do what it is supposed to do when it encounters an unexpected problem that may cause it to malfunction. In this case, the function is supposed to mark the connection as joined or tell us that we shouldn't try to join the connection. As long as it is able to do either of those things, it should return NS_OK (not throw an exception).
Comment on attachment 576168 [details] [diff] [review]
patch 20 rev 0 connection telemetry

This patch has been moved to bug 707173
Attachment #576168 - Flags: review?(honzab.moz)
Landed on mozilla-inbound as changesets:

48807fde0339    04afb38d54ee    86e3d2e80614    2cb0358aa68b
9aed66c3a561    0c7ba908f2fd    c80e1a5653cb    56b778efac49
a73317c8e854    a267b3c9d217    842ccf5f5de2    d93829e39b3f
389dc74f60e7    9cf19a023624    3f6e6b127b23    0eb13ad19d08
0bd45ead1676
a reminder that this is landed pref'd off.

To use it right now you'll need to set network.http.spdy.enabled to true in about:config
Blocks: 708415
Backed out (along with the rest of the SPDY landings) in order to stop us hitting the MSVC virtual address limit, so we can reopen the trees (bug 709193).

Sucks, but we don't really have any other choice here :-(

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc48c0992358
Status: RESOLVED → REOPENED
Depends on: 709193
Resolution: FIXED → ---
Target Milestone: mozilla11 → ---
(See the rest of the multiline commit message for the other changesets/bug #s, so you know what needs to reland when this does)
(Note: would have been easier with a compile-time kill switch; I thought that was expected post 2.0?)
For anyone following along, I've written a summary of the current MSVC PGO issues in bug 709193 comment 36.

tl;dr SPDY is not at fault, it's a 'libxul is now too large for MSVC2005 to link on a 32bit OS, even with the 3GB flag' issue, which is going to be resolved by bug 709193 and dependents as soon as possible. As such, two of the largest libxul landings in the last week were chosen by the inbound sheriffs for temporary backout/preffing off (SPDY & Graphite), to confirm our theory & so that non-libxul landings can at least occur in the meantime.
Relanded on mozilla-central :-)

https://hg.mozilla.org/mozilla-central/rev/cf0b31ff2b6d
Status: REOPENED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
No longer depends on: 709193
Blocks: 698229
Is there a bugzilla ID for enabling spdy on by default?
(In reply to mdew from comment #181)
> Is there a bugzilla ID for enabling spdy on by default?

Blocks: 698229
(In reply to mdew from comment #181)
> Is there a bugzilla ID for enabling spdy on by default?

724563
Verified on Firefox 11.0 beta 1 and beta 2 running the testcases http://bit.ly/z4j8KC on Windows XP, Windows 7 x64, Ubuntu 11.10 and Mac 10.6.
Whiteboard: [qa!]
Verified on Firefox 11.0 RC build 2 on Windows XP, Ubuntu 11.10 32-bit and Mac 10.6.8.

BuildID: 20120312181643
Status: RESOLVED → VERIFIED
Version: Trunk → Other Branch
Blocks: 724563
Given SPDY is obsolete and there is bug 1287132, we are not going to document this.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.