Closed
Bug 221329
Opened 21 years ago
Closed 21 years ago
need to add nsIX509Cert::isPerm and nsIX509CertDB::addCert methods
Categories
(Core Graveyard :: Security: UI, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: Bienvenu, Assigned: Bienvenu)
Details
Attachments
(1 file, 3 obsolete files)
6.39 KB,
patch
|
nelson
:
review+
sspitzer
:
superreview+
|
Details | Diff | Splinter Review |
For 4.x compatibility, we need to add nsIX509Cert::isPerm and nsIX509CertDB::addCert methods so autoconfig js can add certs. Patch upcoming.
Assignee | ||
Comment 1•21 years ago
|
||
proposed fix - this patch seems to work fine, at least for root certs.
Assignee | ||
Comment 2•21 years ago
|
||
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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•21 years ago
|
||
Comment on attachment 132707 [details] [diff] [review] proposed fix requesting reviews
Attachment #132707 -
Flags: superreview?(scott)
Attachment #132707 -
Flags: review?(MisterSSL)
Assignee | ||
Comment 4•21 years ago
|
||
cc'ing Kaie in case he has time to review this.
Comment 5•21 years ago
|
||
Comment on attachment 132707 [details] [diff] [review] proposed fix I emailed my comments directly to David.
Attachment #132707 -
Flags: review?(MisterSSL) → review-
Assignee | ||
Comment 6•21 years ago
|
||
fix memory leak, clean up return value
Attachment #132707 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #132891 -
Flags: review?(MisterSSL)
Comment 7•21 years ago
|
||
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.
Attachment #132891 -
Flags: review?(MisterSSL) → review-
Assignee | ||
Comment 8•21 years ago
|
||
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•21 years ago
|
||
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
Assignee | ||
Comment 10•21 years ago
|
||
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•21 years ago
|
||
+ // 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?
Assignee | ||
Comment 12•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #132891 -
Attachment is obsolete: true
Assignee | ||
Comment 13•21 years ago
|
||
Comment on attachment 133266 [details] [diff] [review] patch addressing Nelson's comment requesting review for patch addressing Nelson's comments.
Attachment #133266 -
Flags: review?(MisterSSL)
Assignee | ||
Comment 14•21 years ago
|
||
Comment on attachment 132964 [details] [diff] [review] add a new interface to accomplish this obsoleting old patch
Attachment #132964 -
Attachment is obsolete: true
Assignee | ||
Comment 15•21 years ago
|
||
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•21 years ago
|
||
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.
Attachment #133266 -
Flags: review?(MisterSSL) → review+
Assignee | ||
Comment 17•21 years ago
|
||
Comment on attachment 133266 [details] [diff] [review] patch addressing Nelson's comment great, thx, Nelson.
Attachment #133266 -
Flags: superreview?(scott)
Comment 18•21 years ago
|
||
Comment on attachment 133266 [details] [diff] [review] patch addressing Nelson's comment sr=sspitzer
Attachment #133266 -
Flags: superreview?(scott) → superreview+
Assignee | ||
Comment 19•21 years ago
|
||
fixed, thx.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 20•21 years ago
|
||
Comment on attachment 132707 [details] [diff] [review] proposed fix removing obsolete review request
Attachment #132707 -
Flags: superreview?(scott)
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•