713934 - Update PSM to use SSL_AuthCertificateComplete instead of SSL_RestartHandshakeAfterAuthCertificate

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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?)]

Attachment: Part 1, version 1: Add new patches to NSS

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2, version 1: Update SetCertVerificationResult to use SSL_AuthCertificateComplete

Comment 3

[Honza Bambas (:mayhemer)]

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)]

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?)]

Attachment: Part 2: Update SetCertVerificationResult to use SSL_AuthCertificateComplete [v2]

Comment 6

[Honza Bambas (:mayhemer)]

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]]

https://hg.mozilla.org/mozilla-central/rev/8365c736e4f3

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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?)]

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]]

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.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f234a7c7cbd0
https://hg.mozilla.org/releases/mozilla-beta/rev/72570745fef2