703834 - Factor out TransportSecurityInfo superclass from nsNSSSocketInfo

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Creating a TransportSecurityInfo class that implements nsITransportSecurityInfo and the other security-UI-related elements of nsNSSSocketInfo will make the code much easier to understand. Instead of passing an nsNSSSocketInfo to certificate validation, we could pass this TransportSecurityInfo instead. This would make it clearer that the certificate validation code isn't directly affecting anything on the socket transport thread.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

This patch splits nsNSSSocketInfo into two classes--a base class TransportSecurityInfo that handles all the certificate-related stuff, and nsNSSSocketInfo that handles all the SSL-connection stuff. The expectation is that we serialize/deserialize TransportSecurityInfo objects from the cache, and that we create nsNSSSocketInfo instances only when we are actually doing SSL connections.

The benefit of doing this split is that we can change SSLServerCertVerification.cpp to use only TransportSecurityInfo; this way, we can be sure that it is not dependent in any way on working on an actual SSL connection. This will make the patch for bug 660749 easier to review.

This is a very big patch. Other than splitting the class and fixing some whitespace issues, there are no significant code changes.

The part that should be reviewed closest is the changes to nsPSMRememberCertErrorsTable. Previously, it was embedded in nsNSSIOLayerHelpers, which belongs in nsNSSIOLayer.cpp. Besides the obvious refactoring to split it out, I attempted to make it thread-safe by protecting it with a mutex.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

This patch is to be applied on top of the previous one. It consists mostly of s/nsNSSSocketInfo/TransportSecurityInfo/ and s/socketInfo/infoObject/ in SSLServerCertVerification.cpp.

It also moves the SPDY cert change check to a SSL-connection-specific code-path that won't be used by the patch for bug 660749.

It also makes the calling of the cert error listener conditional on the socketInfo implementing nsISSLSocketControl--nsNSSSocketInfo does, but TransportSecurityInfo on its own (which will be used for revalidating certs from the cache) doesn't.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Note: When you read the patch in splinter, there are two sections for nsNSSIOLayer.cpp and two sections for nsNSSIOLayer.h. In each case, one section is for the parts that get copied into TransportSecurityInfo.cpp/h and the other section is for the parts that remain in nsNSSIOLayer.cpp/h.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

Honza, please see the description of the patch in the previous comments.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

Again, see the comments in the preceeding comments.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 610040 [details] [diff] [review]
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

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

r=honzab with few comments addressed.

> // XXX: threadsafe?

It's inherited.  I.e. remove the comment.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1115,2 @@
>  {
> +  MutexAutoLock lock(mMutex);

Please don't call any methods of the infoObject while holding this lock.  It is OK now but don't do that as a precaution when something changes.

The same in the Lookup method.  Just protect what you want to protect - members of this class.

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +144,3 @@
>  
>  public:
> +  RememberCertErrorsTable();

private?

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 610041 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

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

r=honzab

Sorry for the delay.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Updated patches due to bitrot.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a432c2d1b41
https://hg.mozilla.org/integration/mozilla-inbound/rev/57cc1577f951

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #11)
> Updated patches due to bitrot.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4a432c2d1b41
> https://hg.mozilla.org/integration/mozilla-inbound/rev/57cc1577f951

... and addressed Honza's review comments.

Thank you for the review, Honza.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29914c0fb85e

When updating the patch, I accidentally left an unused copy of mSSLStatus in nsNSSSocketInfo, which was wrong and also didn't match what Honza reviewed. I corrected that in the checkin above.

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/4a432c2d1b41
https://hg.mozilla.org/mozilla-central/rev/57cc1577f951
https://hg.mozilla.org/mozilla-central/rev/29914c0fb85e