Closed Bug 703834 Opened 8 years ago Closed 8 years ago

Factor out TransportSecurityInfo superclass from nsNSSSocketInfo


(Core :: Security: PSM, enhancement)

Not set





(Reporter: briansmith, Assigned: briansmith)


(Blocks 2 open bugs)



(2 files, 4 obsolete files)

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.
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.
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.
Duplicate of this bug: 733666
Summary: Factor out TransportSecurityInfo from nsNSSSocketInfo into its own class/object owned by nsNSSSocketInfo → Factor out TransportSecurityInfo superclass from nsNSSSocketInfo
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.
Honza, please see the description of the patch in the previous comments.
Attachment #610040 - Flags: review?(honzab.moz)
Again, see the comments in the preceeding comments.
Attachment #609531 - Attachment is obsolete: true
Attachment #609532 - Attachment is obsolete: true
Attachment #610041 - Flags: review?(honzab.moz)
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();

Attachment #610040 - Flags: review?(honzab.moz) → review+
Comment on attachment 610041 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

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


Sorry for the delay.
Attachment #610041 - Flags: review?(honzab.moz) → review+
(In reply to Brian Smith (:bsmith) from comment #11)
> Updated patches due to bitrot.

... and addressed Honza's review comments.

Thank you for the review, Honza.

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.
Whiteboard: [Inbound: THREE changesets in TWO pushes]
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [Inbound: THREE changesets in TWO pushes]
You need to log in before you can comment on or make changes to this bug.