net: disable ECDSA ciphersuites on platforms where we can't support it.

net: disable ECDSA ciphersuites on platforms where we can't support it.


BUG=142782

[agl, 2012-08-15 00:24:26]

If the NSS change looks ok, I'll send to Mark for the base/ change.

[Ryan Sleevi, 2012-08-15 00:40:27]

http://codereview.chromium.org/10830326/diff/1/net/socket/ssl_client_socket_n...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10830326/diff/1/net/socket/ssl_client_socket_n...
net/socket/ssl_client_socket_nss.cc:3128: #endif
Seems like you can move this into NSSSSLInitSingleton (nss_ssl_util.cc), and
only have to set it once per-process rather than once-per-socket, and then
update the comment in net/base/ssl_config_service.h

[agl, 2012-08-15 01:06:20]

http://codereview.chromium.org/10830326/diff/1/net/socket/ssl_client_socket_n...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/10830326/diff/1/net/socket/ssl_client_socket_n...
net/socket/ssl_client_socket_nss.cc:3128: #endif
On 2012/08/15 00:40:27, Ryan Sleevi wrote:
> Seems like you can move this into NSSSSLInitSingleton (nss_ssl_util.cc), and
> only have to set it once per-process rather than once-per-socket, and then
> update the comment in net/base/ssl_config_service.h

Good point, there's a SetDefault function too. Done.

[Ryan Sleevi, 2012-08-15 01:12:07]

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:70: // On some platforms we cannot verify ECDSA
certificates.
nit: Suggested re-word to drop the "we" (a pet nit of Mark's that I've
inherited), but also provide context why this is, since normally the two
statements would be unrelated.

// On platforms that do not support ECDSA signed certificates,
// disable ECDSA cipher suites, as servers may use the presence
// of such ciphersuites as a hint to send an ECDSA certificate.

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:100: break;
break or continue?

If continue, you can probably just rewrite
if (rv == SECSuccess && info.authAlgorithm == ...)
  SSL_CipherPrefSetDefault(..., PR_FALSE);

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:104: }
nit: lose the braces for the one-line if here

[agl, 2012-08-15 01:36:29]

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:70: // On some platforms we cannot verify ECDSA
certificates.
On 2012/08/15 01:12:07, Ryan Sleevi wrote:
> nit: Suggested re-word to drop the "we" (a pet nit of Mark's that I've
> inherited), but also provide context why this is, since normally the two
> statements would be unrelated.
> 
> // On platforms that do not support ECDSA signed certificates,
> // disable ECDSA cipher suites, as servers may use the presence
> // of such ciphersuites as a hint to send an ECDSA certificate.

Done. (And have reworked the ssl_config_service.h comment similarly.)

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:100: break;
On 2012/08/15 01:12:07, Ryan Sleevi wrote:
> break or continue?
> 
> If continue, you can probably just rewrite
> if (rv == SECSuccess && info.authAlgorithm == ...)
>   SSL_CipherPrefSetDefault(..., PR_FALSE);

I did mean break, but less code is better.

http://codereview.chromium.org/10830326/diff/2002/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:104: }
On 2012/08/15 01:12:07, Ryan Sleevi wrote:
> nit: lose the braces for the one-line if here

So many different micro-styles... :)

[agl, 2012-08-15 01:40:29]

Mark: can you take a look at the base/mac/ changes? I need that function only to
bake on Canary and then merge back to 21. It can be removed after that.

(I'll actually land as two different CLs so that the base/ changes doesn't
conflict with the M21 version of mac_util.[h|cc].)

[Ryan Sleevi, 2012-08-15 01:45:35]

net/ lgtm

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:74: if (base::win::GetVersion() <
base::win::VERSION_VISTA) {
nit on the braces here ;)

[Mark Mentovai, 2012-08-15 02:19:22]

Karen is the TPM in charge of Chrome 21. If she gives you a hard time, I’ll talk
to her.

http://codereview.chromium.org/10830326/diff/7007/base/mac/mac_util.h
File base/mac/mac_util.h (right):

http://codereview.chromium.org/10830326/diff/7007/base/mac/mac_util.h#newcode131
base/mac/mac_util.h:131: BASE_EXPORT bool IsOSSnowLeopardOrLater();
Nope. This is a tautology on the trunk.

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:78: if (!base::mac::IsOSSnowLeopardOrLater()) {
We’ve removed all 10.5-specific code on the trunk. You’ll need to leave the
Mac-specific parts out of this patch. For the merge to the 21.0.1180 branch, you
will need to add this code here back in. IsOSSnowLeopardOrLater still does exist
on that branch.

[Mark Mentovai, 2012-08-15 02:33:48]

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:30: 
Alternative B, if you’re intent on landing this on the trunk as-is first and
then merging it straight over, is to use the strategy you proposed earlier by
landing IsOSSnowLeopardOrLater on the trunk temporarily in a separate
not-for-merge change independent of the “meat” of this one, except scoping it to
this file only, not landing it in base. Since its return value on the trunk is a
constant anyway, you could just write it right here:

namespace {
namespace base {
namespace mac {
// see, told ya it was a tautology
bool IsOSSnowLeopardOrLater() { return true; }
}  // namespace mac
}  // namespace base
}  // namespace

Obviously you’d need to keep the mac_util.h #include in the “meat” of your
for-merge change to make sure it works on the branch, which won’t have this
version of the function.

[wtc, 2012-08-15 02:38:42]

Review comments on patch set 5:

http://codereview.chromium.org/10830326/diff/7007/base/mac/mac_util.h
File base/mac/mac_util.h (right):

http://codereview.chromium.org/10830326/diff/7007/base/mac/mac_util.h#newcode155
base/mac/mac_util.h:155: #define BASE_MAC_MAC_UTIL_H_INLINED_GE_10_7

Since you didn't define an inline version of IsOSSnowLeopardOrLater
here, the definition of IsOSSnowLeopardOrLater()
in mac_util.mm should not be inside an
  #if !defined(BASE_MAC_MAC_UTIL_H_INLINED_GE_10_7)
block.

http://codereview.chromium.org/10830326/diff/7007/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

http://codereview.chromium.org/10830326/diff/7007/base/mac/mac_util.mm#newcod...
base/mac/mac_util.mm:576: #if !defined(BASE_MAC_MAC_UTIL_H_INLINED_GE_10_7)

This may be incorrect.  See my comment in mac_util.h.

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

http://codereview.chromium.org/10830326/diff/7007/net/socket/nss_ssl_util.cc#...
net/socket/nss_ssl_util.cc:98: for (unsigned i = 0; i < num_ciphersuites; i++) {

You should merge this for loop with the existing for loop
on lines 58-65.  Pwehaps I missed something?

[agl, 2012-08-16 04:24:12]

On Tue, Aug 14, 2012 at 7:38 PM,  <wtc@chromium.org> wrote:
> You should merge this for loop with the existing for loop
> on lines 58-65.  Pwehaps I missed something?

I ended up landing this as Mark suggested, without any changes in base/.

For the merge CL I didn't want to use the existing loop, but in the
cleanup CL on trunk, I have done so.


Cheers

AGL