643041 - Merge nsIX509Cert2 and nsIX509Cert3 into nsIX509Cert

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Zack Weinberg (:zwol)]

There is no longer any good reason for nsIX509Cert2 and ...3 to be separate interfaces from nsIX509Cert, now that we don't freeze interfaces.

Comment 1

[Zack Weinberg (:zwol)]

... and we should fix bug 235230 at the same time, assuming there is still a bug there.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: patch: merge cert interfaces, fix type of windowTitle attribute

Fixes bug 235230 and bug 234856 as side effects.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: add'l patch: remove ignoreOcsp argument from GetUsagesArray/GetUsagesString

While developing the patch above I noticed that all callers of GetUsagesArray and GetUsagesString pass FALSE to their ignoreOcsp argument, and the async version of GetUsagesArray doesn't even *have* that argument; so this additional patch removes it.  It is a separate patch for approval's sake -- maybe there's a good reason to do the merge but keep this argument around -- but, on the theory that it would be landed together with the other one, does *not* bump the IID on nsIX509Cert, since we just did that.

I checked comm-central for additional uses of ignoreOcsp but did not find any.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 520458 [details] [diff] [review]
patch: merge cert interfaces, fix type of windowTitle attribute

+++ b/security/manager/ssl/src/nsNSSCertificate.cpp
+      nsMemory::Free(commonName);

NS_Free, please. Although I doubt this is the right allocator for NSS. http://mxr.mozilla.org/mozilla/source/security/nss/lib/certdb/cert.h#804 says to use PORT_Free.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: merge cert interfaces, revised

Uses PORT_Free instead of nsMemory::Free as requested; also fixes a silly mistake caught in the process of writing an automated test for bug 234856.

Comment 6

[Kaspar]

Shouldn't the Component be "Security: PSM"? This mostly touches stuff under security/manager/... (and nothing under netwerk/, actually).

Comment 7

[Zack Weinberg (:zwol)]

Security:PSM has a component description that makes it sound like it's for some weird piece of NSS that Firefox doesn't actually use.  I've since been told it is the place for this sort of bug, yeah.

Comment 8

[Zack Weinberg (:zwol)]

Comment on attachment 520461 [details] [diff] [review]
add'l patch: remove ignoreOcsp argument from GetUsagesArray/GetUsagesString

This should maybe get a super-review.

Comment 9

[Daniel Veditz [:dveditz]]

What happens when code written with the ignoreOcsp argument calls the new function? Hopefully xpconnect throws an exception? or does it try to interpret the args and come up with something mysterious?

Looks like a few addons use this and are going to break. Will they notice what's wrong? Is it at all possible for them to write their addon to work with both the new form and old form (not everyone will upgrade immediately)? Check properties on the cert they get before they QI to Cert2 or Cert3 forms and see if it has the merged methods or not? In JS add-on code no one is using the IID so changing it doesn't help them (but helps C++ callers).

Comment 10

[Daniel Veditz [:dveditz]]

These methods are used (with ignoreOcsp always false) by the "GMail S/MIME", "Key Manager", and "XML Digital Signature Procesing Tool" addons.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 521083 [details] [diff] [review]
merge cert interfaces, revised

biesi is the wrong reviewer for PSM, adding a review request.

I'm ok with the merging in theory, will wait for kai's r+ before looking in depth.

Comment 12

[Zack Weinberg (:zwol)]

(In reply to comment #9)
> What happens when code written with the ignoreOcsp argument calls the new
> function? Hopefully xpconnect throws an exception? or does it try to interpret
> the args and come up with something mysterious?

There are no in-tree JS uses of these functions, and I'm not sure how to call an IDL-declared function with non-[retval] out parameters from JS, so I don't know how to do an experiment here.

> Looks like a few addons use this and are going to break.

How did you search add-on source code?

> Will they notice
> what's wrong? Is it at all possible for them to write their addon to work with
> both the new form and old form (not everyone will upgrade immediately)? Check
> properties on the cert they get before they QI to Cert2 or Cert3 forms and see
> if it has the merged methods or not?

I was able to do an experiment for the merge-CertN patch (not the ignoreOcsp patch) by enhancing the test case for bug 234856; both Cert2 and Cert3 add a read-only attribute that's easy to look at.  I was hoping that XPConnect objects were duck typed from JS's perspective, so if you had an object with nominal type nsIX509Cert you could call Cert2/Cert3 methods on it without bothering to call QueryInterface first, but it is not so.  Pre-these patches, if you don't do

    cert.QueryInterface(Ci.nsIX509Cert3);

then Cert2/Cert3 properties are undefined.  But post-these patches, that QI call throws an exception.  If you want to write code that works with both you have to do

    if("nsIX509Cert3" in Ci)
      cert.QueryInterface(Ci.nsIX509Cert3);

which naturally no one will have bothered to do yet.  I'd be okay with adding back stub nsIX509Cert2 and Cert3 interfaces that don't do anything but make those QI calls succeed.

Comment 13

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

Zach, there are other incompatible changes to these interfaces I want to make--most notably, deprecating and/or removing the methods that verify certificate chains, because certificate chain validation must not happen on the main thread, because it blocks the calling thread too long.

As all these changes will affect addon compatibility, I think we should do them all at the same addon-breaking time. I also want to give advance notice to addon developers about the breaking changes.

Comment 14

[Zack Weinberg (:zwol)]

That would be fine with me.  Do you have a timeframe?

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 520461 [details] [diff] [review]
add'l patch: remove ignoreOcsp argument from GetUsagesArray/GetUsagesString

This interface change will break the "GMail S/MIME" and "Key Manager" addons at least, of the AMO addons I was able to grep. Those authors need to be notified if this goes through.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 521083 [details] [diff] [review]
merge cert interfaces, revised

In addition to "Key Manager", nsIX509Cert3 is used by "Cert Viewer Plus" and "Skip Cert Error" (a successor to MitM Me).

nsIX509Cert2 is used by Cipherfox, Cert Viewer Plus, XML Digital Signature Procesing Tool, and Key Manager.

I don't know how much of AMO my corpus contains, I may be missing a significant number of addons. I also would not have found use of these interfaces from binary components in AMO-hosted addons. I have no visibility whatsoever into non-AMO-hosted addons.

If bsmith is planning breaking changes to these interfaces we should definitely combine them. Removing sr request until after Kai has reviewed.

Comment 17

[Zack Weinberg (:zwol)]

(In reply to Daniel Veditz from comment #15)
> Comment on attachment 520461 [details] [diff] [review]
> add'l patch: remove ignoreOcsp argument from GetUsagesArray/GetUsagesString
> 
> This interface change will break the "GMail S/MIME" and "Key Manager" addons
> at least, of the AMO addons I was able to grep. Those authors need to be
> notified if this goes through.

Do you think it would be better to preserve the argument for interface compatibility's sake, but have it throw NS_ERROR_NOT_IMPLEMENTED if it were ever passed as 'true'?  I could do that.

(In reply to Daniel Veditz from comment #16)
> 
> In addition to "Key Manager", nsIX509Cert3 is used by "Cert Viewer Plus" and
> "Skip Cert Error" (a successor to MitM Me).
> 
> nsIX509Cert2 is used by Cipherfox, Cert Viewer Plus, XML Digital Signature
> Procesing Tool, and Key Manager.

What do you think of preserving them as empty interfaces so that QueryInterface succeeds?  Inheriting, so one may still call nsIX509Cert methods on an nsIX509Cert3*.  Would that reduce the compat burden?

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 521083 [details] [diff] [review]
merge cert interfaces, revised

I apologize, but I don't have time for nice-to-have cleanup patches.

Furthermore I'm worried you will unnecessarily break add-ons that make use of this separation. In order to keep things compatible, you should consider tohave dummy derived interfaces, that are empty, allowing compatibility with add-ons because using the old names will still work.

I give you module owner approval, but please make very sure this doesn't cause any trouble, when in doubt, I'd prefer wontfix.

Comment 19

[Zack Weinberg (:zwol)]

Comment on attachment 521083 [details] [diff] [review]
merge cert interfaces, revised

I'll generate a new version that preserves stub interfaces for compatibility.  This probably won't happen before September.  Clearing review requests.

Comment 20

[Zack Weinberg (:zwol)]

Comment on attachment 520461 [details] [diff] [review]
add'l patch: remove ignoreOcsp argument from GetUsagesArray/GetUsagesString

># HG changeset patch
># Parent 9ffdeecd301bdc0466b5a2093656a68ca3d1ccc5
># User Zack Weinberg <[e-mail]>
>Bug 643041 tangential: remove the always-false ignoreOCSP argument to getUsagesArray and getUsagesString.
>
>diff --git a/security/manager/ssl/public/nsIX509Cert.idl b/security/manager/ssl/public/nsIX509Cert.idl
>--- a/security/manager/ssl/public/nsIX509Cert.idl
>+++ b/security/manager/ssl/public/nsIX509Cert.idl
>@@ -233,43 +233,39 @@ interface nsIX509Cert : nsISupports {
>    *  @return The chain of certifficates including the issuers.
>    */
>   nsIArray getChain();
> 
>   /**
>    *  Obtain an array of human readable strings describing
>    *  the certificate's certified usages.
>    *
>-   *  @param ignoreOcsp Do not use OCSP even if it is currently activated.
>    *  @param verified The certificate verification result, see constants.
>    *  @param count The number of human readable usages returned.
>    *  @param usages The array of human readable usages.
>    */
>-  void getUsagesArray(in boolean ignoreOcsp,
>-                      out PRUint32 verified,
>+  void getUsagesArray(out PRUint32 verified,
>                       out PRUint32 count,
>                       [array, size_is(count)] out wstring usages);
> 
>   /**
>    *  Async version of getUsagesArray().
>    *  The nsICertVerificationListener object will be notified when
>    *  data is available.  See below.
>    */
>   void requestUsagesArrayAsync(in nsICertVerificationListener cvl);
> 
>   /**
>    *  Obtain a single comma separated human readable string describing
>    *  the certificate's certified usages.
>    *
>-   *  @param ignoreOcsp Do not use OCSP even if it is currently activated.
>    *  @param verified The certificate verification result, see constants.
>    *  @param purposes The string listing the usages.
>    */
>-  void getUsagesString(in boolean ignoreOcsp, out PRUint32 verified,
>-                       out AString usages);
>+  void getUsagesString(out PRUint32 verified, out AString usages);
> 
>   /**
>    *  Verify the certificate for a particular usage.
>    *
>    *  @return The certificate verification result, see constants.
>    */
>    unsigned long verifyForUsage(in unsigned long usage);
> 
>diff --git a/security/manager/ssl/src/nsCertTree.cpp b/security/manager/ssl/src/nsCertTree.cpp
>--- a/security/manager/ssl/src/nsCertTree.cpp
>+++ b/security/manager/ssl/src/nsCertTree.cpp
>@@ -1188,17 +1188,17 @@ nsCertTree::GetCellText(PRInt32 row, nsI
>   } else if (NS_LITERAL_STRING("tokencol").Equals(colID) && cert) {
>     rv = cert->GetTokenName(_retval);
>   } else if (NS_LITERAL_STRING("emailcol").Equals(colID) && cert) {
>     rv = cert->GetEmailAddress(_retval);
>   } else if (NS_LITERAL_STRING("purposecol").Equals(colID) && mNSSComponent && cert) {
>     PRUint32 verified;
> 
>     nsAutoString theUsages;
>-    rv = cert->GetUsagesString(PR_FALSE, &verified, theUsages); // allow OCSP
>+    rv = cert->GetUsagesString(&verified, theUsages);
>     if (NS_FAILED(rv)) {
>       verified = nsIX509Cert::NOT_VERIFIED_UNKNOWN;
>     }
> 
>     switch (verified) {
>       case nsIX509Cert::VERIFIED_OK:
>         _retval = theUsages;
>         break;
>diff --git a/security/manager/ssl/src/nsCertVerificationThread.cpp b/security/manager/ssl/src/nsCertVerificationThread.cpp
>--- a/security/manager/ssl/src/nsCertVerificationThread.cpp
>+++ b/security/manager/ssl/src/nsCertVerificationThread.cpp
>@@ -52,20 +52,17 @@ void nsCertVerificationJob::Run()
>   PRUint32 verified;
>   PRUint32 count;
>   PRUnichar **usages;
> 
>   nsCOMPtr<nsICertVerificationResult> ires;
>   nsRefPtr<nsCertVerificationResult> vres = new nsCertVerificationResult;
>   if (vres)
>   {
>-    nsresult rv = mCert->GetUsagesArray(PR_FALSE, // do not ignore OCSP
>-                                        &verified,
>-                                        &count,
>-                                        &usages);
>+    nsresult rv = mCert->GetUsagesArray(&verified, &count, &usages);
>     vres->mRV = rv;
>     if (NS_SUCCEEDED(rv))
>     {
>       vres->mVerified = verified;
>       vres->mCount = count;
>       vres->mUsages = usages;
>     }
> 
>diff --git a/security/manager/ssl/src/nsNSSCertificate.cpp b/security/manager/ssl/src/nsNSSCertificate.cpp
>--- a/security/manager/ssl/src/nsNSSCertificate.cpp
>+++ b/security/manager/ssl/src/nsNSSCertificate.cpp
>@@ -468,17 +468,18 @@ nsNSSCertificate::FormatUIStrings(const 
>           details.Append(temp1);
>         }
> 
>         details.Append(PRUnichar('\n'));
>       }
>     }
> 
>     PRUint32 tempInt = 0;
>-    if (NS_SUCCEEDED(x509Proxy->GetUsagesString(PR_FALSE, &tempInt, temp1)) && !temp1.IsEmpty()) {
>+    if (NS_SUCCEEDED(x509Proxy->GetUsagesString(&tempInt, temp1))
>+        && !temp1.IsEmpty()) {
>       details.AppendLiteral("  ");
>       if (NS_SUCCEEDED(nssComponent->GetPIPNSSBundleString("CertInfoPurposes", info))) {
>         details.Append(info);
>         details.AppendLiteral(": ");
>       }
>       details.Append(temp1);
>       details.Append(PRUnichar('\n'));
>     }
>@@ -1402,32 +1403,31 @@ nsNSSCertificate::VerifyForUsage(PRUint3
>     }
>   }
>   
>   return NS_OK;  
> }
> 
> 
> NS_IMETHODIMP
>-nsNSSCertificate::GetUsagesArray(PRBool ignoreOcsp,
>-                                 PRUint32 *_verified,
>+nsNSSCertificate::GetUsagesArray(PRUint32 *_verified,
>                                  PRUint32 *_count,
>                                  PRUnichar ***_usages)
> {
>   nsNSSShutDownPreventionLock locker;
>   if (isAlreadyShutDown())
>     return NS_ERROR_NOT_AVAILABLE;
> 
>   nsresult rv;
>   const int max_usages = 13;
>   PRUnichar *tmpUsages[max_usages];
>   const char *suffix = "";
>   PRUint32 tmpCount;
>   nsUsageArrayHelper uah(mCert);
>-  rv = uah.GetUsagesArray(suffix, ignoreOcsp, max_usages, _verified, &tmpCount, tmpUsages);
>+  rv = uah.GetUsagesArray(suffix, max_usages, _verified, &tmpCount, tmpUsages);
>   NS_ENSURE_SUCCESS(rv,rv);
>   if (tmpCount > 0) {
>     *_usages = (PRUnichar **)nsMemory::Alloc(sizeof(PRUnichar *) * tmpCount);
>     if (!*_usages)
>       return NS_ERROR_OUT_OF_MEMORY;
>     for (PRUint32 i=0; i<tmpCount; i++) {
>       (*_usages)[i] = tmpUsages[i];
>     }
>@@ -1457,31 +1457,29 @@ nsNSSCertificate::RequestUsagesArrayAsyn
>   nsresult rv = nsCertVerificationThread::addJob(job);
>   if (NS_FAILED(rv))
>     delete job;
> 
>   return rv;
> }
> 
> NS_IMETHODIMP
>-nsNSSCertificate::GetUsagesString(PRBool ignoreOcsp,
>-                                  PRUint32   *_verified,
>-                                  nsAString &_usages)
>+nsNSSCertificate::GetUsagesString(PRUint32   *_verified, nsAString &_usages)
> {
>   nsNSSShutDownPreventionLock locker;
>   if (isAlreadyShutDown())
>     return NS_ERROR_NOT_AVAILABLE;
> 
>   nsresult rv;
>   const int max_usages = 13;
>   PRUnichar *tmpUsages[max_usages];
>   const char *suffix = "_p";
>   PRUint32 tmpCount;
>   nsUsageArrayHelper uah(mCert);
>-  rv = uah.GetUsagesArray(suffix, ignoreOcsp, max_usages, _verified, &tmpCount, tmpUsages);
>+  rv = uah.GetUsagesArray(suffix, max_usages, _verified, &tmpCount, tmpUsages);
>   NS_ENSURE_SUCCESS(rv,rv);
>   _usages.Truncate();
>   for (PRUint32 i=0; i<tmpCount; i++) {
>     if (i>0) _usages.AppendLiteral(",");
>     _usages.Append(tmpUsages[i]);
>     nsMemory::Free(tmpUsages[i]);
>   }
>   return NS_OK;
>diff --git a/security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp b/security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
>--- a/security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
>+++ b/security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
>@@ -277,36 +277,34 @@ nsNSSCertificateFakeTransport::GetValidi
> NS_IMETHODIMP
> nsNSSCertificateFakeTransport::VerifyForUsage(PRUint32 usage, PRUint32 *verificationResult)
> {
>   NS_NOTREACHED("Unimplemented on content process");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
> NS_IMETHODIMP
>-nsNSSCertificateFakeTransport::GetUsagesArray(PRBool ignoreOcsp,
>-                                 PRUint32 *_verified,
>-                                 PRUint32 *_count,
>-                                 PRUnichar ***_usages)
>+nsNSSCertificateFakeTransport::GetUsagesArray(PRUint32 *_verified,
>+                                              PRUint32 *_count,
>+                                              PRUnichar ***_usages)
> {
>   NS_NOTREACHED("Unimplemented on content process");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
> NS_IMETHODIMP
> nsNSSCertificateFakeTransport::RequestUsagesArrayAsync(nsICertVerificationListener *aResultListener)
> {
>   NS_NOTREACHED("Unimplemented on content process");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
> NS_IMETHODIMP
>-nsNSSCertificateFakeTransport::GetUsagesString(PRBool ignoreOcsp,
>-                                  PRUint32   *_verified,
>-                                  nsAString &_usages)
>+nsNSSCertificateFakeTransport::GetUsagesString(PRUint32   *_verified,
>+                                               nsAString &_usages)
> {
>   NS_NOTREACHED("Unimplemented on content process");
>   return NS_ERROR_NOT_IMPLEMENTED;
> }
> 
> /* readonly attribute nsIASN1Object ASN1Structure; */
> NS_IMETHODIMP
> nsNSSCertificateFakeTransport::GetASN1Structure(nsIASN1Object * *aASN1Structure)
>diff --git a/security/manager/ssl/src/nsUsageArrayHelper.cpp b/security/manager/ssl/src/nsUsageArrayHelper.cpp
>--- a/security/manager/ssl/src/nsUsageArrayHelper.cpp
>+++ b/security/manager/ssl/src/nsUsageArrayHelper.cpp
>@@ -147,60 +147,46 @@ nsUsageArrayHelper::verifyFailed(PRUint3
>     // this means, no verification result has ever been received
>   default:
>     *_verified = nsNSSCertificate::NOT_VERIFIED_UNKNOWN; break;
>   }
> }
> 
> nsresult
> nsUsageArrayHelper::GetUsagesArray(const char *suffix,
>-                      PRBool ignoreOcsp,
>                       PRUint32 outArraySize,
>                       PRUint32 *_verified,
>                       PRUint32 *_count,
>                       PRUnichar **outUsages)
> {
>   nsNSSShutDownPreventionLock locker;
>   if (NS_FAILED(m_rv))
>     return m_rv;
> 
>   if (outArraySize < max_returned_out_array_size)
>     return NS_ERROR_FAILURE;
> 
>-  nsCOMPtr<nsINSSComponent> nssComponent;
>-
>-  if (ignoreOcsp) {
>-    nsresult rv;
>-    nssComponent = do_GetService(kNSSComponentCID, &rv);
>-    if (NS_FAILED(rv))
>-      return rv;
>-    
>-    if (nssComponent) {
>-      nssComponent->SkipOcsp();
>-    }
>-  }
>-
>   PRUint32 &count = *_count;
>   count = 0;
>   SECCertificateUsage usages;
>-  
>-  CERT_VerifyCertificateNow(defaultcertdb, mCert, PR_TRUE, 
>+
>+  CERT_VerifyCertificateNow(defaultcertdb, mCert, PR_TRUE,
> 			    certificateUsageSSLClient |
> 			    certificateUsageSSLServer |
> 			    certificateUsageSSLServerWithStepUp |
> 			    certificateUsageEmailSigner |
> 			    certificateUsageEmailRecipient |
> 			    certificateUsageObjectSigner |
> 			    certificateUsageSSLCA |
> 			    certificateUsageStatusResponder,
> 			    NULL, &usages);
>   int err = PR_GetError();
> 
>   // The following list of checks must be < max_returned_out_array_size
>-  
>+
>   check(suffix, usages & certificateUsageSSLClient, count, outUsages);
>   check(suffix, usages & certificateUsageSSLServer, count, outUsages);
>   check(suffix, usages & certificateUsageSSLServerWithStepUp, count, outUsages);
>   check(suffix, usages & certificateUsageEmailSigner, count, outUsages);
>   check(suffix, usages & certificateUsageEmailRecipient, count, outUsages);
>   check(suffix, usages & certificateUsageObjectSigner, count, outUsages);
> #if 0
>   check(suffix, usages & certificateUsageProtectedObjectSigner, count, outUsages);
>@@ -210,19 +196,15 @@ nsUsageArrayHelper::GetUsagesArray(const
> #if 0
>   check(suffix, usages & certificateUsageVerifyCA, count, outUsages);
> #endif
>   check(suffix, usages & certificateUsageStatusResponder, count, outUsages);
> #if 0
>   check(suffix, usages & certificateUsageAnyCA, count, outUsages);
> #endif
> 
>-  if (ignoreOcsp && nssComponent) {
>-    nssComponent->SkipOcspOff();
>-  }
>-
>   if (count == 0) {
>     verifyFailed(_verified, err);
>   } else {
>     *_verified = nsNSSCertificate::VERIFIED_OK;
>   }
>   return NS_OK;
> }
>diff --git a/security/manager/ssl/src/nsUsageArrayHelper.h b/security/manager/ssl/src/nsUsageArrayHelper.h
>--- a/security/manager/ssl/src/nsUsageArrayHelper.h
>+++ b/security/manager/ssl/src/nsUsageArrayHelper.h
>@@ -43,17 +43,16 @@
> #include "nsNSSComponent.h"
> 
> class nsUsageArrayHelper
> {
> public:
>   nsUsageArrayHelper(CERTCertificate *aCert);
> 
>   nsresult GetUsagesArray(const char *suffix,
>-               PRBool ignoreOcsp,
>                PRUint32 outArraySize,
>                PRUint32 *_verified,
>                PRUint32 *_count,
>                PRUnichar **tmpUsages);
> 
>   enum { max_returned_out_array_size = 12 };
> 
> private:

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Zack, are you still interested in working on this? This is something I would like to see happen.

Comment 22

[Zack Weinberg (:zwol)]

I would also like to see this happen, but I don't know when I'm going to have time to do Firefox work again.  The research is being very time consuming right now.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ok - no worries. I'll go ahead and unassign you in case anyone else is interested.

Comment 24

[Dana Keeler (she/her) (use needinfo) [:keeler]]

For anyone looking to do this, we should also merge nsIX509CertDB2 into nsIX509CertDB at the same time.

Comment 25

[hpathak]

Attachment: bug_643041_merge_interfaces.diff

Comment 26

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8446332 [details] [diff] [review]
bug_643041_merge_interfaces.diff

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

Well done so far. There are a few issues:
- Any interface that changes needs to have its uuid updated (see comment below)
- Watch out for trailing whitespace on lines touched by this patch or too much vertical space. Generally multiple blank lines in a row are unnecessary.
- Make sure &/* indicators are next to the type name: 'const nsAString& foo' on lines touched by this patch
- Let's not make changes unrelated to merging these interfaces (e.g. the style nits in nsNSSCertificateDB.cpp)

Upon thinking about this more, we should just go ahead and remove the unneeded interfaces. The changes required of addons that use these interfaces are completely mechanical and won't be too much of a hardship. It would be necessary to send an email to the appropriate newsgroups when we're ready to land this, though.

::: dom/wifi/WifiWorker.js
@@ +3293,5 @@
>        this._sendMessage(message, false, "Wifi is disabled", msg);
>        return;
>      }
>  
>      let certDB2 = Cc["@mozilla.org/security/x509certdb;1"]

nit: we should probably rename this to "certDB"

::: security/manager/pki/resources/content/viewCertDetails.js
@@ +94,5 @@
>    DisplayGeneralDataFromCert(cert);
>    BuildPrettyPrint(cert);
>    
> +  cert.requestUsagesArrayAsync(new listener());
> +  

nit: trailing whitespace

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +22,5 @@
>    /* Note: To distinguish between 
>     *         "unstrusted because missing or untrusted issuer"
>     *       and 
>     *         "untrusted because self signed"
> +   *       query nsIX509Cert::isSelfSigned 

nit: trailing space

::: security/manager/ssl/public/nsIX509Cert.idl
@@ +19,5 @@
>  
>  /**
>   * This represents a X.509 certificate.
>   */
>  [scriptable, uuid(891d2009-b9ba-4a0d-bebe-6b3a30e33191)]

Any changes to interfaces in idl files means their uuids need to be updated - try 'uuidgen' or '/mgs firebot uuid' on irc.

@@ +144,5 @@
>    const unsigned long CA_CERT      = 1 << 0;
>    const unsigned long USER_CERT    = 1 << 1;
>    const unsigned long EMAIL_CERT   = 1 << 2;
>    const unsigned long SERVER_CERT  = 1 << 3;
> +  const unsigned long ANY_CERT  = 0xffff;

nit: might as well align this like the previous lines

@@ +147,5 @@
>    const unsigned long SERVER_CERT  = 1 << 3;
> +  const unsigned long ANY_CERT  = 0xffff;
> +
> +  /**
> +  * Type of this certificate 

nit: trailing space

@@ +152,5 @@
> +  */
> +  readonly attribute unsigned long certType;
> +
> +/**
> +*  True if the certificate is self-signed.  CA issued 

nit: trailing space

@@ +196,5 @@
> +   */
> +  const unsigned long CMS_CHAIN_MODE_CertOnly = 1;
> +  const unsigned long CMS_CHAIN_MODE_CertChain = 2;
> +  const unsigned long CMS_CHAIN_MODE_CertChainWithRoot = 3;
> +

nit: there doesn't need to be this much blank vertical space

@@ +223,5 @@
>                        out uint32_t verified,
>                        out uint32_t count, 
>                        [array, size_is(count)] out wstring usages);
>  
> +

nit: there should be only one blank line between the previous declaration and the next one

@@ +229,5 @@
> +   *  Async version of nsIX509Cert::getUsagesArray()
> +   *
> +   *  Will not block, will request results asynchronously,
> +   *  availability of results will be notified on the main thread.
> +   */ 

nit: trailing space

::: security/manager/ssl/public/nsIX509Cert2.idl
@@ +9,3 @@
>  
>  /**
> + * Dummy interface

Maybe add "for temporary addon compatibility. This will be removed in the future."

::: security/manager/ssl/public/nsIX509Cert3.idl
@@ +7,2 @@
>  /**
>   * Extending nsIX509Cert

Have the same comment here that's in nsIX509Cert2

::: security/manager/ssl/public/nsIX509CertDB2.idl
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  

Have the same comment here as in nsIX509Cert2

@@ -4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
>  

Something strange happened with this patch - all changes to nsIX509CertDB2.idl should be in the same place.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +538,1 @@
>      bool isSelfSigned;

nit: fix indentation in this block

::: security/manager/ssl/src/nsCertOverrideService.cpp
@@ +435,5 @@
>    return GetCertFingerprintByDottedOidString(nsscert.get(), dottedOid, fp);
>  }
>  
>  NS_IMETHODIMP
> +nsCertOverrideService::RememberValidityOverride(const nsACString & aHostName, 

nit: as long as you're changing this line, move the & a space to the left

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1122,5 @@
>    PRFileDesc *fd = nullptr;
>  
>    rv = aFile->OpenNSPRFileDesc(PR_RDONLY, 0, &fd);
>  
> +  if (NS_FAILED(rv)) {

these changes are unrelated - let's leave this section as it is for now

@@ +1287,5 @@
>  }
>  
>  /* nsIX509Cert getDefaultEmailEncryptionCert (); */
>  NS_IMETHODIMP
> +nsNSSCertificateDB::FindEmailEncryptionCert(const nsAString &aNickname, 

nit: const nsAString& aNickname

@@ +1288,5 @@
>  
>  /* nsIX509Cert getDefaultEmailEncryptionCert (); */
>  NS_IMETHODIMP
> +nsNSSCertificateDB::FindEmailEncryptionCert(const nsAString &aNickname, 
> +                                            nsIX509Cert **_retval)

nit: nsIX509Cert** _retval

@@ +1326,5 @@
>  
>  /* nsIX509Cert getDefaultEmailSigningCert (); */
>  NS_IMETHODIMP
> +nsNSSCertificateDB::FindEmailSigningCert(const nsAString &aNickname, 
> +                                         nsIX509Cert **_retval)

same idea

@@ +1590,5 @@
>        if (dummycert) {
>  	/*
>  	 * Make sure the subject names are different.
>  	 */ 
> +	        if (CERT_CompareName(&cert->subject, &dummycert->subject) == SECEqual)

the indentation on this block is too much

::: security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
@@ +390,5 @@
> +nsNSSCertificateFakeTransport::GetCert()
> +{
> +  CERTCertificate* cert = nullptr; 
> +  NS_NOTREACHED("Unimplemented on content process");
> +  return cert;

maybe just return nullptr?

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +386,5 @@
>    // like to join this connection
>  
>    ScopedCERTCertificate nssCert;
>  
> +  nsCOMPtr<nsIX509Cert> cert2 = do_QueryInterface(SSLStatus()->mServerCert);

nit: rename to "cert"

::: security/manager/ssl/tests/unit/test_getchain.js
@@ +7,5 @@
>  
>  do_get_profile(); // must be called before getting nsIX509CertDB
>  const certdb  = Cc["@mozilla.org/security/x509certdb;1"]
>                    .getService(Ci.nsIX509CertDB);
> +

nit: just remove this blank line

Comment 27

[hpathak]

Attachment: bug_643041_merge_interfaces.diff

Comment 28

[hpathak]

Attachment: bug_643041_merge_interfaces.diff

Comment 29

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8448020 [details] [diff] [review]
bug_643041_merge_interfaces.diff

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

Looking great! Just a few more nits to address.

::: dom/wifi/WifiWorker.js
@@ +3294,5 @@
>        return;
>      }
>  
> +    let certDB = Cc["@mozilla.org/security/x509certdb;1"]
> +                  .getService(Ci.nsIX509CertDB);

nit: don't change the indentation on files not in security/

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +18,5 @@
>  
>    readonly attribute boolean isDomainMismatch;
>    readonly attribute boolean isNotValidAtThisTime;
>  
> +  /* Note: To distinguish between

nit: let's not touch style issues in files where other changes aren't also necessary. If you feel like it, have an additional patch with whitespace-only changes. If it only touches files in security/manager, I can review it (otherwise, I really can't give it r+).

::: security/manager/ssl/public/nsIX509Cert.idl
@@ +19,5 @@
>  
>  /**
>   * This represents a X.509 certificate.
>   */
> +[scriptable, uuid(043DF3DE-E103-4C13-99BE-AF841F8A2A9C)]

nit: lower-case hex, please

@@ +147,5 @@
>    const unsigned long SERVER_CERT  = 1 << 3;
> +  const unsigned long ANY_CERT     = 0xffff;
> +
> +  /**
> +  * Type of this certificate

nit: line up the first * in each line of comments

@@ +153,5 @@
> +  readonly attribute unsigned long certType;
> +
> +/**
> +*  True if the certificate is self-signed. CA issued
> +*  certificates are always self-signed.

nit: this comment needs to be indented

@@ +285,5 @@
> +                   [retval, array, size_is(length)] out octet data);
> +
> +  /**
> +   * Retrieves the NSS certificate object wrapped by this interface
> +   **/

nit: just one * here

@@ +303,5 @@
> +                       tokenNames);
> +
> + /**
> +  * Either delete the certificate from all cert databases, or mark it as untrusted.
> +  **/

nit: just one * Here

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +19,5 @@
>  %}
>  
>  typedef uint32_t AppTrustedRoot;
>  
> +[scriptable, function, uuid(8F4CB051-A0A2-4322-BA28-2CF037A27133)]

nit: lowercase hex, please

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ -704,5 @@
> -  cert2 = do_QueryInterface(cert);
> -  if (!cert2) {
> -    NS_NOTREACHED("every nsSSLStatus must have a cert"
> -                  "that implements nsIX509Cert2");
> -    PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);

Actually, here we should probably do something like:

if (!cert) {
  NS_NOTREACHED("every nsSSLStatus... nsIX509Cert");
  PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0);
  return SECFailure;
}

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +540,1 @@
>          && isSelfSigned) {

nit: this can probably fit all on one line

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1317,5 @@
>  
>  /* nsIX509Cert getDefaultEmailSigningCert (); */
>  NS_IMETHODIMP
> +nsNSSCertificateDB::FindEmailSigningCert(const nsAString& aNickname,
> +                                          nsIX509Cert** _retval)

nit: one less space of indentation

@@ +1587,5 @@
>  	  dummycert = nullptr;
>  	}
>        }
>      }
> +    if (!dummycert)

nit: as long as you're changing this line, add {}

@@ +1595,5 @@
>  }
>  
> +NS_IMETHODIMP nsNSSCertificateDB::AddCertFromBase64(const char *aBase64,
> +                                                    const char *aTrust,
> +                                                    const char *aName)

nit: const char*

@@ +1761,5 @@
>  #endif
>  
> +  ScopedCERTCertificate nssCert(aCert->GetCert());
> +  if (!nssCert) {
> +  return NS_ERROR_INVALID_ARG;

nit: indentation

::: security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
@@ +357,5 @@
>    return NS_OK;
>  }
> +
> +NS_IMETHODIMP
> +nsNSSCertificateFakeTransport::GetCertType(unsigned int *type)

nit: unsigned int*
Also, I would just leave out the parameter name, since it's unused

@@ +364,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +nsNSSCertificateFakeTransport::GetIsSelfSigned(bool *selfSigned)

same

@@ +371,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +nsNSSCertificateFakeTransport::RequestUsagesArrayAsync(nsICertVerificationListener *listener)

same

@@ +379,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsNSSCertificateFakeTransport::GetAllTokenNames(unsigned int* length,
> +                                                char16_t*** tokenNames)

Just leave out the unused parameter names

@@ +395,5 @@
> +
> +NS_IMETHODIMP
> +nsNSSCertificateFakeTransport::ExportAsCMS(unsigned int chainMode,
> +                                           unsigned int* length,
> +                                           unsigned char** data)

same

::: security/manager/tools/genHPKPStaticPins.js
@@ +24,5 @@
>  let { FileUtils } = Cu.import("resource://gre/modules/FileUtils.jsm", {});
>  let { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
>  
>  let gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
> +                 .getService(Ci.nsIX509CertDB);

nit: this should be indented one less space

Comment 30

[hpathak]

Attachment: bug_643041_merge_interfaces.diff

Comment 31

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8448497 [details] [diff] [review]
bug_643041_merge_interfaces.diff

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

Looks great! r=me with nits addressed. Be sure to post to dev.platform (and maybe an appropriate addons mailing list) about this change. The try run for this should probably be as comprehensive as possible.

::: security/manager/ssl/public/nsISSLStatus.idl
@@ +22,5 @@
>    /* Note: To distinguish between 
>     *         "unstrusted because missing or untrusted issuer"
>     *       and 
>     *         "untrusted because self signed"
> +   *       query nsIX509Cert::isSelfSigned

The change to this file still only consists of whitespace-only changes.

::: security/manager/ssl/public/nsIX509Cert.idl
@@ +302,5 @@
> +                       [retval, array, size_is(length)] out wstring
> +                       tokenNames);
> +
> +  /**
> +   * Either delete the certificate from all cert databases, or mark it as untrusted.

looks like this line is >80 characters - wrap it to two lines.

::: security/manager/ssl/src/nsCertTree.cpp
@@ +808,5 @@
>              // user is trying to delete a perm trusted cert,
>              // although there are still overrides stored,
>              // so, we keep the cert, but remove the trust
>  
> +            mozilla::pkix::ScopedCERTCertificate nsscert (cert->GetCert());

nit: 'nsscert(cert->GetCert());'

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +386,5 @@
>    // like to join this connection
>  
>    ScopedCERTCertificate nssCert;
>  
> +  nsCOMPtr<nsIX509Cert> cert = do_QueryInterface(SSLStatus()->mServerCert);

Actually, do_QueryInterface shouldn't be necessary here - just do:
nsCOMPtr<nsIX509Cert> cert(SSLStatus()->mServerCert);

::: security/manager/tools/genHPKPStaticPins.js
@@ +24,5 @@
>  let { FileUtils } = Cu.import("resource://gre/modules/FileUtils.jsm", {});
>  let { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
>  
>  let gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
> +              .getService(Ci.nsIX509CertDB);

the . should line up with the [ on the previous line here

Comment 32

[hpathak]

Attachment: bug_643041_merge_interfaces.diff

https://tbpl.mozilla.org/?tree=Try&rev=c12449794c29

Comment 33

[Carsten Book [:Tomcat]]

Hi,

this patch failed to apply:

applying bug_643041_merge_interfaces.diff
patching file security/apps/AppSignatureVerification.cpp
Hunk #1 FAILED at 622
1 out of 3 hunks FAILED -- saving rejects to file security/apps/AppSignatureVerification.cpp.rej
patching file security/manager/ssl/public/moz.build
Hunk #1 succeeded at 35 with fuzz 1 (offset -1 lines).
patching file security/manager/ssl/src/nsNSSCertificateDB.h
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file security/manager/ssl/src/nsNSSCertificateDB.h.rej
patching file security/manager/tools/genHPKPStaticPins.js
Hunk #1 succeeded at 21 with fuzz 1 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug_643041_merge_interfaces.diff


could you maybe try to rebase this patch ?

Comment 34

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/240a0b085725

Good work!

(In reply to Carsten Book [:Tomcat] from comment #33)
> could you maybe try to rebase this patch ?

I'd already done that in my local tree, and I pushed the rebased version.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/240a0b085725