The default bug view has changed. See this FAQ.

Factor out TransportSecurityInfo superclass from nsNSSSocketInfo

RESOLVED FIXED in mozilla15

Status

()

Core
Security: PSM
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
Blocks: 615342
Depends on: 480745
Created attachment 609375 [details] [diff] [review]
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.
Blocks: 660749
Created attachment 609377 [details] [diff] [review]
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.
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.
Created attachment 609531 [details] [diff] [review]
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo
Attachment #609375 - Attachment is obsolete: true
Created attachment 609532 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo
Attachment #609377 - Attachment is obsolete: true
Created attachment 610040 [details] [diff] [review]
Split nsNSSSocketInfo into TransportSecurityInfo and nsNSSSocketInfo

Honza, please see the description of the patch in the previous comments.
Attachment #610040 - Flags: review?(honzab.moz)
Created attachment 610041 [details] [diff] [review]
Make SSLServerCertVerification use TransportSecurityInfo instead of nsNSSSocketInfo

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();

private?
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]:
-----------------------------------------------------------------

r=honzab

Sorry for the delay.
Attachment #610041 - Flags: review?(honzab.moz) → review+
Updated patches due to bitrot.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a432c2d1b41
https://hg.mozilla.org/integration/mozilla-inbound/rev/57cc1577f951
Target Milestone: --- → mozilla15
(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.
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.
Whiteboard: [Inbound: THREE changesets in TWO pushes]
https://hg.mozilla.org/mozilla-central/rev/4a432c2d1b41
https://hg.mozilla.org/mozilla-central/rev/57cc1577f951
https://hg.mozilla.org/mozilla-central/rev/29914c0fb85e
Status: NEW → RESOLVED
Last Resolved: 5 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.