Last Comment Bug 713934 - Update PSM to use SSL_AuthCertificateComplete instead of SSL_RestartHandshakeAfterAuthCertificate
: Update PSM to use SSL_AuthCertificateComplete instead of SSL_RestartHandshake...
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Other Branch
: All All
: P1 normal (vote)
: mozilla13
Assigned To: Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
:
Mentors:
Depends on: 542832
Blocks: 713936
  Show dependency treegraph
 
Reported: 2011-12-28 13:21 PST by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-02-22 11:46 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
+
fixed
+
fixed


Attachments
Part 1, version 1: Add new patches to NSS (70.51 KB, patch)
2011-12-29 15:18 PST, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review+
Details | Diff | Splinter Review
Part 2, version 1: Update SetCertVerificationResult to use SSL_AuthCertificateComplete (1.88 KB, patch)
2011-12-29 15:19 PST, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
no flags Details | Diff | Splinter Review
Part 2: Update SetCertVerificationResult to use SSL_AuthCertificateComplete [v2] (1.89 KB, patch)
2012-01-21 22:33 PST, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
honzab.moz: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-28 13:21:50 PST
In the latest patches to bug 542832, I changed the signature of SSL_RestartHandshakeAfterAuthCertificate and renamed it to SSL_AuthCertificateComplete. Unless the NSS team disagrees with implementing those patches, SSL_AuthCertificateComplete will replace SSL_RestartHandshakeAfterAuthCertificate, so Gecko must switch to using SSL_AuthCertificateComplete.

Note that currently, we have a private-to-mozilla-central/-aurora patch that implements SSL_RestartHandshakeAfterAuthCertificate. We need to do this to update to the NSS 3.12.2 release, which we *must* do for mozilla-aurora as well as mozilla-central.
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-29 15:18:36 PST
Created attachment 584868 [details] [diff] [review]
Part 1, version 1: Add new patches to NSS
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-29 15:19:15 PST
Created attachment 584869 [details] [diff] [review]
Part 2, version 1: Update SetCertVerificationResult to use SSL_AuthCertificateComplete
Comment 3 Honza Bambas (:mayhemer) 2012-01-05 10:51:44 PST
Comment on attachment 584869 [details] [diff] [review]
Part 2, version 1: Update SetCertVerificationResult to use SSL_AuthCertificateComplete

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

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +982,5 @@
> +          if (errorCode == 0) {
> +            NS_ERROR("SSL_AuthCertificateComplete didn't set error code");
> +            errorCode = PR_INVALID_STATE_ERROR;
> +          }
> +          SetCanceled(errorCode, PlainErrorMessage);

Wrong indention (and if the code where right, see bellow, blank line before SetCanceled when after closing brace, please).

And SetCanceled should be called even errorCode was not 0, right?
Comment 4 Honza Bambas (:mayhemer) 2012-01-06 02:31:01 PST
Comment on attachment 584868 [details] [diff] [review]
Part 1, version 1: Add new patches to NSS

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

r=honzab of the mozilla-central changes.  Landing this patch is conditioned by reviews of related patches in bug 542832:

ssl-restart-3-alerts-v1.patch = "Rename SSL_RHAAC to SSL_AuthCertificateComplete; send alert when async cert. authentication fails"
ssl-restart-4-tstclnt-alerts-v2.patch = "Update tstclnt to use the new function name and to send alerts when cert authentication fails [v2]"
ssl-restart-5-disable-false-start-v1.patch = "Disable False Start when async cert authentication is used until they work safely together"
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-21 22:33:04 PST
Created attachment 590543 [details] [diff] [review]
Part 2: Update SetCertVerificationResult to use SSL_AuthCertificateComplete [v2]
Comment 6 Honza Bambas (:mayhemer) 2012-01-22 13:36:45 PST
Comment on attachment 590543 [details] [diff] [review]
Part 2: Update SetCertVerificationResult to use SSL_AuthCertificateComplete [v2]

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

r=honzab
Comment 7 Marco Bonardo [::mak] 2012-02-14 02:39:11 PST
https://hg.mozilla.org/mozilla-central/rev/8365c736e4f3
Comment 8 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-16 11:52:17 PST
Comment on attachment 590543 [details] [diff] [review]
Part 2: Update SetCertVerificationResult to use SSL_AuthCertificateComplete [v2]

[Approval Request Comment]
Regression caused by (bug #): bug 713936
User impact if declined: Firefox stops working on Linux distributions that use system NSS 3.13.2 or later
Testing completed (on m-c, etc.): Landed on mozilla-central yesterday
Risk to taking this patch (and alternatives if risky): Low. This is not a change in logic, it is just rearranging the existing logic to accommodate the change to the NSS function that changed signatures between NSS 3.13.2 beta1 and NSS 3.13.2 RTM.
String changes made by this patch: None
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-16 11:53:33 PST
Note that this must land at the same time as bug 713936, because bug 713936 will change the NSS API in a way that breaks Firefox without this patch.
Comment 10 Alex Keybl [:akeybl] 2012-02-16 14:46:42 PST
Comment on attachment 590543 [details] [diff] [review]
Part 2: Update SetCertVerificationResult to use SSL_AuthCertificateComplete [v2]

[Triage Comment]
Approved for Aurora 12 and Beta 11 given the need to switch to the final NSS version.

Note You need to log in before you can comment on or make changes to this bug.