221329 - need to add nsIX509Cert::isPerm and nsIX509CertDB::addCert methods

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[David :Bienvenu]

For 4.x compatibility, we need to add nsIX509Cert::isPerm and
nsIX509CertDB::addCert methods so autoconfig js can add certs. Patch upcoming.

Comment 1

[David :Bienvenu]

Attachment: proposed fix

proposed fix - this patch seems to work fine, at least for root certs.

Comment 2

[David :Bienvenu]

this patch also contains the patch for bug 220816, although that fix is not
neccesary. It is, however, needed if GetRawDER is ever to be called from js,
because js expects the result to be copied. Please let me know if you'd rather I
removed those diffs from the patch.

Comment 3

[David :Bienvenu]

Comment on attachment 132707 [details] [diff] [review]
proposed fix

requesting reviews

Comment 4

[David :Bienvenu]

cc'ing Kaie in case he has time to review this.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 132707 [details] [diff] [review]
proposed fix

I emailed my comments directly to David.

Comment 6

[David :Bienvenu]

Attachment: fix addressing Nelson's comments

fix memory leak, clean up return value

Comment 7

[Kai Engert [:KaiE:]]

Comment on attachment 132891 [details] [diff] [review]
fix addressing Nelson's comments

I'm sorry, but both interfaces you are changing are marked @status frozen :-(

I assume we don't want to change frozen interfaces.

Comment 8

[David :Bienvenu]

well, cripes, that's really unfortunate. Excuse my ignorance - we've never
frozen any mailnews interfaces so I don't know the policy. I assume frozen means
frozen but it doesn't hurt to ask:

1. Does anyone care if these interfaces are frozen? Is there anyone out there
actually using them, or would we know if there were? If there were, would adding
new methods at the end break them?

2. I suppose my alternative is to create two new interfaces, one each with the
methods I need, and make the underlying objects implement those interfaces, and
QI for those interfaces. Or, roll my two methods into one new method on the new
interface nsIX509CertDB2:

 void addX509CertFromBase64(in string aBase64, in string aTrust, in string aName);

It would check if the cert already existed in the db, and if not, add it to the
db, and set the trust appropriately. 

Is that approach going to be acceptable?

thx, - David

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 132891 [details] [diff] [review]
fix addressing Nelson's comments

Random nits first:

>@@ -850,12 +850,24 @@
>     return NS_ERROR_NOT_AVAILABLE;
> 
>   if (mCert) {
>-    *aArray = (PRUint8 *)mCert->derCert.data;
>+    *aArray = (PRUint8 *) PR_Malloc(mCert->derCert.len);
>+    if (*aArray)
>+      memcpy(*aArray, (PRUint8 *)mCert->derCert.data, mCert->derCert.len);
>+    else
>+      return NS_ERROR_OUT_OF_MEMORY;
>     *aLength = mCert->derCert.len;
>     return NS_OK;

This control flow is arbitrarily overcomplicated -- why not return NS_ERROR_OOM
right away if (! *aArray)?  Then the memcpy is not overindented and separated
from the *aLength setting and return NS_OK.

>   }
>   *aLength = 0;
>   return NS_ERROR_FAILURE;

That nit suggests inverting the if (mCert) test to return NS_ERROR_FAILURE
early.	Also, why is *aLength zeroed only for this failure return, not for the
NS_ERROR_NOT_AVAILABLE in the first line of context?  Are any callers actually
counting on valid out params after a failed call?

>+}
>+
>+NS_IMETHODIMP
>+nsNSSCertificate::GetIsPerm(PRBool *aIsPerm)
>+{
>+  NS_ENSURE_ARG_POINTER(aIsPerm);
>+  *aIsPerm = (mCert) ? mCert->isperm : PR_FALSE;

  *aIsPerm = mCert && mCert->isperm;

is better than using ?:PR_FALSE.

Another nit, possibly a stupid question on my part: is isPerm the best name for
this attribute?  Should it be an readonly attribute, or an explicit method?

Unfortunately, FROZEN means "don't ever change this."  See bug 168452 for some
of the history leading up to the freeze.  It may be that no one actually uses
these frozen interfaces yet.  If that can be determined, adding at the end is
ok.  The trick will be determining that no one counts on this now.  I suspect
some embedders do, though perhaps not the original embedder driving the effort
in  bug 168452.

/be

Comment 10

[David :Bienvenu]

Attachment: add a new interface to accomplish this

this is the other approach, since the interfaces are frozen. I backed out the
getrawder changes because if we have embedders using that, their code will
start leaking, which takes care of the nits :-)

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

+  // need to calculate the trust bits from the aTrust string.
+  rv = CERT_DecodeTrustString(trust.GetTrust(), /* this is const, but not
declared that way */(char *) aTrust);
+  trust.SetValidCA();

what if CERT_DecodeTrustString returns an error?

Comment 12

[David :Bienvenu]

Attachment: patch addressing Nelson's comment

I've moved things around so that we error out with a bad trust string, and also
added an error check for creating the x509 cert from base64.

Comment 13

[David :Bienvenu]

Comment on attachment 133266 [details] [diff] [review]
patch addressing Nelson's comment

requesting review for patch addressing Nelson's comments.

Comment 14

[David :Bienvenu]

Comment on attachment 132964 [details] [diff] [review]
add a new interface to accomplish this

obsoleting old patch

Comment 15

[David :Bienvenu]

I realize everyone's very busy, but can any security module owner review the
last patch? It adds a new interface, so it can't break any existing clients...

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 133266 [details] [diff] [review]
patch addressing Nelson's comment

David,	
I looked over your patch and I believe it should do what you expect it to do
for users whose crypto software is not running in 'FIPS mode".	r=MisterSSL.  

My remaining concerns at this point are whether these interfaces, which are 
more-or-less modelled on the old C4.x ones, are right for modern mozilla.  
Old C4.x had one central cert DB.  Now, mozilla has potentially numerous 
PKCS11 crypto modules, including hardware and software modules.  The cert 
interfaces should be able to acccess (and store) certs into any module.  
The single central certDB paradigm allows access to only a limited subset 
of the underlying capability of modern mozilla.

In a perfect world, I'd say we should review all the existing mozilla 
crypto JS interfaces, and not create any more JS interfaces that would limit
the functionality (to the old paradigm), and instead devise new interfaces
that match mozilla's capabilities.  

OTOH, I think there will always be a software cert store, and having an
interface that can manipulate (only) that store is not necessarily a bad thing.

Comment 17

[David :Bienvenu]

Comment on attachment 133266 [details] [diff] [review]
patch addressing Nelson's comment

great, thx, Nelson.

Comment 18

[(not reading, please use [e-mail] instead)]

Comment on attachment 133266 [details] [diff] [review]
patch addressing Nelson's comment

sr=sspitzer

Comment 19

[David :Bienvenu]

fixed, thx.

Comment 20

[Matthias Versen [:Matti]]

Comment on attachment 132707 [details] [diff] [review]
proposed fix

removing obsolete review request