1050518 - remove nsICertificatePrincipal

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

nsICertificatePrincipal is a confusingly-named unnecessary wrapper around nsIX509Cert. We do not need it.

Comment 1

[Mike Hoye [:mhoye]]

Can you flesh out this bug a bit with a DXR link and line or two about what a successful patch looks like?

Comment 2

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

Maybe this is a good next bug, instead. In any case, here's the uses of nsICertificatePrincipal:

http://dxr.mozilla.org/mozilla-central/search?q=nsICertificatePrincipal&case=true&redirect=true

In the case of the interface file and implementation (nsICertificatePrincipal.idl, nsCertificatePrincipal.h, nsCertificatePrincipal.cpp), they can be removed altogether. The other uses will basically be replaced by nsIX509Cert. Method/field names may have to be updated to reflect this (for example, nsIZipReader.getCertificatePrincipal should probably be nsIZipReader.getSigningCert or similar).
Uses of nsIZipReader.getCertificatePrincipal will have to be changed as well to reflect that an nsIX509Cert is returned instead: http://dxr.mozilla.org/mozilla-central/search?q=getCertificatePrincipal&case=true

Things to remember:
* When removing a file, remove its entry from the appropriate moz.build file
* When changing an interface, update its uuid with uuidgen or similar

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Remove nsICertificatePrincipal

I took a stab at this, it is my first bug in this module :-). David, can you take a look? Thanks!

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

Comment 4

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

Comment on attachment 8486966 [details] [diff] [review]
Remove nsICertificatePrincipal

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

This looks great! Just a couple of comments.
One general comment is that it might be nice to add braces around any conditional statement that gets touched. Then again, some of those lines are part of multi-line if/else if blocks, so this patch would end up changing a lot more than is strictly necessary. Your call. (If you do decide to do that, I should probably have one last look before this gets checked in.)

::: modules/libjar/nsJAR.cpp
@@ +322,5 @@
>    return rv;
>  }
>  
>  NS_IMETHODIMP
> +nsJAR::GetSigningCert(const nsACString &aFilename, nsIX509Cert** aSigningCert)

nit: while we're here, let's move the '&' to the left: 'const nsACString& aFilename' (this is what the style should be, but old code often doesn't follow the style guidelines)

::: modules/libjar/test/chrome/test_bug386153.html
@@ +46,5 @@
>  // Gets the pretty name from the signing cert or null if the zip is unsigned.
>  function getSigner(zip) {
> +  var signingCert = zip.getSigningCert(null);
> +  if (signingCert)
> +    return signingCert.organization

nit: this line should have a trailing ';'

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +5135,5 @@
>            this.certificate = x509;
>          if (this.certificate && this.certificate.commonName.length > 0)
>            this.certName = this.certificate.commonName;
>          else
> +          this.certName = this.certificate.organization;

I think we can clean this section up a little. First, x509 will always be a Ci.nsIX509, so we can get rid of the conditional there. Then, this.certificate will be set, so no need to test for that. In the end, it will look something like this:

this.certificate = x509;
if (this.certificate.commonName.length > 0) {
  this.certName = this.certificate.commonName;
} else {
  this.certName = this.certificate.organization;
}

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Remove nsICertificatePrincipal v2

Thanks for the quick review David! I addressed your comments and added a few braces in the if-else blocks my patch was directly touching. Thanks!

Comment 6

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

Comment on attachment 8487668 [details] [diff] [review]
Remove nsICertificatePrincipal v2

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

Great!

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

Thanks!

Win Try: https://tbpl.mozilla.org/?tree=Try&rev=89b871a59eca

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14febaa4c101

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14febaa4c101