NSS: remove cipher policy framework.

NSS: remove cipher policy framework.

Export policies are so last century.

[wtc, 2013-08-08 01:03:10]

agl: just a note that I didn't forget this CL. I will review
it tomorrow.

[wtc, 2013-08-08 21:26:28]

Review comments on patch set 2:

This generally looks good. Just some nits.

Is it OK that I just check this in in the NSS upstream
repository?  Or do you have other Chromium-private NSS
patches that depend on this patch?

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:256: * order to maintain ABI compatibility. */

It would be nice to document that SSL_CipherPolicyGet returns.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:352: /* Values for "policy" argument to
SSL_PolicySet */

Pre-existing typo: SSL_PolicySet => SSL_CipherPolicySet

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:912: SSL_IMPORT SECStatus
NSS_SetFrancePolicy(void);

I think the comments for these three NSS_SetXXXPolicy functions should all be
removed, and we should add a comment to say they are deprecated and have no
effect. All supported ciphers are now allowed.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:91: /*      cipher_suite                   
enabled is_present*/

Nit: add a space before "enabled"

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4780: return SECFailure;	/*
ssl3_cipher_suites_test_presence has set error code. */

Nit: wrap this comment line?

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7560: /* Double check that the cached cipher
suite is still enabled, and

Nit: remove the comma before "and".

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7588: /* Look for a matching cipher suite. */

Nit: a matching => an available?

"matching" seems to refer to the original function name of "config_match".

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslcon.c (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslcon.c:115: /* bit-masks, showing which SSLv2 suites
are allowed.

masks => mask

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslcon.c:120: /* bit values for the above two bit masks
*/

two bit masks => bit mask

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:1777: /* Internal config function so SSL2 can
initialize the present state of

"SSL2" seems like a typo of "SSL3".

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:1779: extern int
ssl3_cipher_suites_test_presence(sslSocket *);

test => init?

to stress the fact that this function is used as an initialization function,
which must be called before cipher_suite_available can be used.

Another good name for this function would be ssl3_cipher_suite_available_init,
to match the pattern of the original names ssl3_config_match_init vs.
config_match.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:1097: * HandshakeLock to protect this. 
Probably want a global lock.

I think lines 1096-1097 (or perhaps the entire comment) should be removed.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:1114: *oPolicy = 0;

I think we should set *oPolicy to SSL_ALLOWED.

[agl, 2013-08-09 15:53:49]

I think that this is good to go upstream if you wish!

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:256: * order to maintain ABI compatibility. */
On 2013/08/08 21:26:28, wtc wrote:
> 
> It would be nice to document that SSL_CipherPolicyGet returns.

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:352: /* Values for "policy" argument to
SSL_PolicySet */
On 2013/08/08 21:26:28, wtc wrote:
> 
> Pre-existing typo: SSL_PolicySet => SSL_CipherPolicySet

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:912: SSL_IMPORT SECStatus
NSS_SetFrancePolicy(void);
On 2013/08/08 21:26:28, wtc wrote:
> 
> I think the comments for these three NSS_SetXXXPolicy functions should all be
> removed, and we should add a comment to say they are deprecated and have no
> effect. All supported ciphers are now allowed.

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:91: /*      cipher_suite                   
enabled is_present*/
On 2013/08/08 21:26:28, wtc wrote:
> 
> Nit: add a space before "enabled"

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:4780: return SECFailure;	/*
ssl3_cipher_suites_test_presence has set error code. */
On 2013/08/08 21:26:28, wtc wrote:
> 
> Nit: wrap this comment line?

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7560: /* Double check that the cached cipher
suite is still enabled, and
On 2013/08/08 21:26:28, wtc wrote:
> 
> Nit: remove the comma before "and".

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7588: /* Look for a matching cipher suite. */
On 2013/08/08 21:26:28, wtc wrote:
> 
> Nit: a matching => an available?
> 
> "matching" seems to refer to the original function name of "config_match".

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslcon.c (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslcon.c:115: /* bit-masks, showing which SSLv2 suites
are allowed.
On 2013/08/08 21:26:28, wtc wrote:
> 
> masks => mask

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslcon.c:120: /* bit values for the above two bit masks
*/
On 2013/08/08 21:26:28, wtc wrote:
> 
> two bit masks => bit mask

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:1777: /* Internal config function so SSL2 can
initialize the present state of
On 2013/08/08 21:26:28, wtc wrote:
> 
> "SSL2" seems like a typo of "SSL3".

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:1779: extern int
ssl3_cipher_suites_test_presence(sslSocket *);
On 2013/08/08 21:26:28, wtc wrote:
> Another good name for this function would be ssl3_cipher_suite_available_init,
> to match the pattern of the original names ssl3_config_match_init vs.
> config_match.

Have switched to this name.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslsock.c (right):

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:1097: * HandshakeLock to protect this. 
Probably want a global lock.
On 2013/08/08 21:26:28, wtc wrote:
> I think lines 1096-1097 (or perhaps the entire comment) should be removed.

Done.

https://codereview.chromium.org/21564003/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslsock.c:1114: *oPolicy = 0;
On 2013/08/08 21:26:28, wtc wrote:
> 
> I think we should set *oPolicy to SSL_ALLOWED.

Done.

[wtc, 2013-08-09 19:28:19]

Patch set 3 LGTM. I suggest some minor tweaks.

I will commit this patch in the upstream NSS hg repository.
I don't think this patch needs to go into the Chromium
source tree.

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4781: error code. */

Add a '*' before "error code".

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7589: /* Look for a matching cipher suite. */

Also change this "a matching" to "an available".

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7632: /* Look for a matching compression
algorithm. */

Let's change this comment to "Select a compression algorithm."
because we aren't checking the present state of
compression algorithms.

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslimpl.h (left):

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslimpl.h:926: 

Resurrect this blank line.

[agl, 2013-08-12 11:29:12]

Not planning on landing in Chrome, but have updated the patch file.

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:4781: error code. */
On 2013/08/09 19:28:19, wtc wrote:
> 
> Add a '*' before "error code".

Done.

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7589: /* Look for a matching cipher suite. */
On 2013/08/09 19:28:19, wtc wrote:
> 
> Also change this "a matching" to "an available".

Done.

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:7632: /* Look for a matching compression
algorithm. */
On 2013/08/09 19:28:19, wtc wrote:
> 
> Let's change this comment to "Select a compression algorithm."
> because we aren't checking the present state of
> compression algorithms.

Done.

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/sslimpl.h (left):

https://codereview.chromium.org/21564003/diff/35001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/sslimpl.h:926: 
On 2013/08/09 19:28:19, wtc wrote:
> 
> Resurrect this blank line.

Done.

[wtc, 2013-08-12 18:47:07]

I landed this patch in the NSS upstream repository:
https://hg.mozilla.org/projects/nss/rev/fee4f3677439