Closed Bug 1453778 Opened 2 years ago Closed 2 years ago

bug 1453741 will break comm-central builds by removing nsIX509CertDB.findCertByEmailAddress

Categories

(MailNews Core :: Security: S/MIME, defect)

defect
Not set

Tracking

(thunderbird_esr6062+ fixed)

RESOLVED FIXED
Thunderbird 61.0
Tracking Status
thunderbird_esr60 62+ fixed

People

(Reporter: keeler, Assigned: jorgk)

References

Details

Attachments

(1 file, 1 obsolete file)

The current plan in bug 1453741 is to remove from mozilla-central the API nsIX509CertDB.findCertByEmailAddress, which is only used in comm-central (in mailnews/extensions/smime/src/nsMsgComposeSecure.cpp and mailnews/extensions/smime/src/nsSMimeJSHelper.cpp ). The current implementation of that API is a bit problematic (it blocks the main thread and can cause nested event loop spinning), but the easiest way for comm-central to deal with this is to just re-implement the function in comm-central.
Thanks for the heads-up.

Magnus, can you take a look. The "bustage-fix" plan would be just to copy the removed code, problematic or not :-(
Flags: needinfo?(mkmelin+mozilla)
This moves the function to C-C. I just wanted to see whether I can get it to compile, but no problem. This could to into a better place so callers in nsSMimeJSHelper.cpp can use it, too.
Attachment #8967572 - Flags: feedback?(mkmelin+mozilla)
Attachment #8967572 - Flags: feedback?(dkeeler)
Comment on attachment 8967572 [details] [diff] [review]
1453778-FindCertByEmailAddress.patch - WIP

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

I imagine you could update the style and/or make it more ergonomic to use, but if this compiles then this looks correct to me.

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +893,5 @@
> +      certVerifier->VerifyCert(node->cert, certificateUsageEmailRecipient,
> +                               mozilla::pkix::Now(),
> +                               nullptr /*XXX pinarg*/,
> +                               nullptr /*hostname*/,
> +                               unusedCertChain);

You might consider passing CertVerifier::FLAG_LOCAL_ONLY as the 'flags' argument here (it has a default value so it doesn't appear currently) to prevent the worst of the main-thread-blocking/event loop spinning behavior (in the future gecko may abort in debug builds or return an error in non-debug builds if it's asked to fetch OCSP on the main thread).
Attachment #8967572 - Flags: feedback?(dkeeler) → feedback+
I moved FindCertByEmailAddress() to nsMsgComposeSecure.cpp as a static function and changed the callers accordingly. I've added the flag that David suggested in comment #3.

Richard, you use S/MIME, could you check that everything is working. FindCertByEmailAddress() looks for a certificated for an e-mail address, so I guess that's used when sending encrypted e-mail to someone and you need to use their public key.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #8967823 - Flags: review?(mkmelin+mozilla)
Attachment #8967823 - Flags: feedback?(richard.marti)
Attachment #8967572 - Attachment is obsolete: true
Attachment #8967572 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 8967823 [details] [diff] [review]
1453778-FindCertByEmailAddress.patch (v2)

Sending a signed and encrypted message worked. Also the reading worked.
Attachment #8967823 - Flags: feedback?(richard.marti) → feedback+
Comment on attachment 8967823 [details] [diff] [review]
1453778-FindCertByEmailAddress.patch (v2)

See comment #4.
Attachment #8967823 - Flags: review?(acelists)
Comment on attachment 8967823 [details] [diff] [review]
1453778-FindCertByEmailAddress.patch (v2)

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

LGTM! r=mkmelin
Attachment #8967823 - Flags: review?(mkmelin+mozilla)
Attachment #8967823 - Flags: review?(acelists)
Attachment #8967823 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2847bbcc7d34
port bug 1453741: Move nsIX509CertDB::FindCertByEmailAddress to C-C. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
Comment on attachment 8967823 [details] [diff] [review]
1453778-FindCertByEmailAddress.patch (v2)

This needs uplift to allow uplift of bug 1293378.
Attachment #8967823 - Flags: approval-comm-esr60+
Blocks: 1530099
You need to log in before you can comment on or make changes to this bug.