Do not use HTTP/2 without adequate security.

Do not use HTTP/2 without adequate security.

Stop using HTTP/2 in case TLS 1.2 is not supported, connection has been
downgraded to below TLS 1.2, or AES-GCM cipher required by HTTP/2 draft
specification is not supported.

BUG=436835
Committed: https://crrev.com/1e7575035b6a712eae7972304dc7b44af329626e
Cr-Commit-Position: refs/heads/master@{#308226}

[Bence, 2014-12-04 16:11:51]

bnc@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Bence, 2014-12-04 16:11:51]

Ryan: PTAL.  In particular, please check that I have followed everything
important in https://code.google.com/p/chromium/issues/detail?id=436835#c18.

Is it possible that not blacklisted ciphersuites get disabled by
disabled_cipher_suites?  Do we really need to hardcode the blacklist here and
make sure that we advertise non-blacklisted ones?  I mean, servers must be smart
enough not to negotiate HTTP/2 when the negotiated ciphersuites are not
suitable, and there is no guarantee that if we offer non blacklisted ones that
those will actually be chosen anyway.

[Bence, 2014-12-08 12:51:17]

Ryan: ping

[Bence, 2014-12-08 17:00:19]

bnc@chromium.org changed reviewers:
+ rch@chromium.org
- rsleevi@chromium.org

[Ryan Sleevi, 2014-12-08 17:05:11]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2014-12-08 17:05:12]

Deferring to RCH here.

I'm not fond of exposing static methods on the SSLClientSocket, because that
doesn't take into consideration any flags policy that might be in effect that
would disable the necessary ciphersuites (the SSLConfig's disabled
ciphersuites). Yes, there are organizations who want to disable the more secure
ciphersuites, the same as those that want to disable TLS 1.2

In order to determine whether or not to negotiate SPDY / HTTP/2, you need to
figure out from the socket what the effective set of enabled ciphersuites would
be / is, and then see if any of those intersect with
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_cipher...
(IsSecureTLSCipherSuite)

Note I would STRONGLY encourage that this method _not_ be a static method on
SSLClientSocket, because that means duplicating information that might be
per-state in the socket.

[Ryan Hamilton, 2014-12-08 20:09:50]

On 2014/12/08 17:05:12, Ryan Sleevi wrote:
> Deferring to RCH here.
> 
> I'm not fond of exposing static methods on the SSLClientSocket, because that
> doesn't take into consideration any flags policy that might be in effect that
> would disable the necessary ciphersuites (the SSLConfig's disabled
> ciphersuites). Yes, there are organizations who want to disable the more
secure
> ciphersuites, the same as those that want to disable TLS 1.2
> 
> In order to determine whether or not to negotiate SPDY / HTTP/2, you need to
> figure out from the socket what the effective set of enabled ciphersuites
would
> be / is, and then see if any of those intersect with
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_cipher...
> (IsSecureTLSCipherSuite)
> 
> Note I would STRONGLY encourage that this method _not_ be a static method on
> SSLClientSocket, because that means duplicating information that might be
> per-state in the socket.

Ok, after talking with sleevi about this it sounds like there is a bit of
dynamic cipher suite state in the SSLClientSocket. It seems *ideal* to expose
this via a per-socket method. bnc how inconvenient would it be to perform the
next_proto mutation after you have the socket? (As opposed to taking the static
method route)

[Bence, 2014-12-09 15:22:30]

rch:

1.
> Ok, after talking with sleevi about this it sounds like there is a bit of
> dynamic cipher suite state in the SSLClientSocket. It seems *ideal* to expose
> this via a per-socket method. bnc how inconvenient would it be to perform the
> next_proto mutation after you have the socket? (As opposed to taking the
static
> method route)

I don't have a pointer to the relevant SSLClientSocket object in
HttpNetworkTransaction where I need to call GetNextProtos.  How do I get one?

Also, GetNextProtos is called at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...
and at
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_log_u...,
how do I get a handle of SSLClientSocket there?

2. I had to put ShouldAdvertiseHTTP2 in ssl_cipher_suite_names.cc, because
that's the only place to access kCipherSuites[], but I'd much rather see this
method in SSLConfig or in SSLClientSocket.  Can I move kCipherSuites[] to
ssl_cipher_suite_names.cc?

Thanks.

[Bence, 2014-12-10 12:48:12]

Patchset #5 (id:80001) has been deleted

[Bence, 2014-12-10 13:09:36]

Patchset #5 (id:100001) has been deleted

[Bence, 2014-12-10 18:05:20]

bnc@chromium.org changed reviewers:
+ davidben@chromium.org

[Bence, 2014-12-10 18:05:21]

rch: PTAL.
davidben: PTAL at ssl_client_socket_nss.cc.

Thank you both.

https://codereview.chromium.org/757033004/diff/160001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/757033004/diff/160001/net/http/http_network_s...
net/http/http_network_session.cc:29: #include "net/socket/ssl_client_socket.h"
Used in line 169:
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...

https://codereview.chromium.org/757033004/diff/180001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/180001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:976: //EnsureNSSSSLInit();
davidben: do I need EnsureNSSSSLInit() here, or is the one in line 3143 executed
before this?

[Ryan Hamilton, 2014-12-10 21:11:41]

This looks promising to me.

Very curious to get davidben's feedback.

https://codereview.chromium.org/757033004/diff/200001/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/nss_ssl_util...
net/socket/nss_ssl_util.cc:140: }
nit: net style says to not use {}s on 1 line if statements.

(Or is this clang-formatted?

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:244: for (std::vector<uint16>::const_iterator it
= cipher_suites.begin();
for (uint16 cipher : cipher_suites) {
}

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:246: if (IsSecureTLSCipherSuite(*it)) {
Just to confirm, this is the right check, yes?

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:272: proto <= kProtoSPDY4MaximumVersion) {
consider adding an IsHttp2NextProto(NextProto next_proto) method since this
logic is now in two places and it's a bit opaque.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.h:213: // that is up to Section 9.2 of the HTTP/2
specification.  Note that the
s/that is up to/that satisfies/

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.h:221: // adequate cipher suites and TLS are also
advertised.
Instead of talking about adequate cipher suites and TLS versions, I would just
refer to |advertise_http2|. Or possibly consider reversing the polarity and
making it |exclude_http2|. As currently written, if advertise_http2 = true, I
might expect that that *forces* http2 (which it doesn't do obviously).

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:983: it != cipher_suites.end(); ++it) {
for (uint16 cipher : cipher_suites) {
}

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:986: *it) ==
ssl_config_.disabled_cipher_suites.end()) {
Hm, I wonder if this logic would be better done down in
SSLClientSocketNSS::InitializeSSLOptions where we currently handle
disabled_cipher_suites. (I guess we can't do that because we need to serialize
next protos already?)

In that case, I wonder if we could move that code up here? perhaps the SSL folks
can comment.

[davidben, 2014-12-10 21:29:57]

Taking disabled_cipher_suites into account for NSS seems to be quite messy (see
comments below). I do wonder how worth it it is.

https://codereview.chromium.org/757033004/diff/180001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/180001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:976: //EnsureNSSSSLInit();
On 2014/12/10 18:05:21, Bence wrote:
> davidben: do I need EnsureNSSSSLInit() here, or is the one in line 3143
executed
> before this?

NSS is initialized at this point.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:269: if (!advertise_http2) {
I'm mildly unhappy that the SSL layer needs to know whether about HTTP/2's
cipher suite requirements (rather than thinking ALPN tokens are opaque strings
and have a higher-level layer deal), but checking cipher suites at any other
layer would be extremely messy. So this is fine.

If we were to care about that, I'd say to not bother with checking
disabled_cipher_suites. That only ever gets set via command-line flag or
enterprise policy it seems.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:978: const std::vector<uint16> cipher_suites
= GetNSSEnabledCipherSuites();
I don't think this excludes AES-GCM when unimplemented.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:980: // suites, because NSS does not know
about them quite yet.
Is this true? Core::Init is called from line 3304, after disabled_cipher_suites
is processed. There's SSL_CipherPrefGet, though that does a linear search. I
wonder if we want to optimize that or expose another function from libssl.
(That's bundled so we can patch it freely.)

For more fun, there's apparently a separate "policy" flag to each cipher suite
which is ANOTHER way to enable/disable them. But we never set it, so let's
ignore it.
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/ns...

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:992: IsSecurityAdequateForHTTP2(ssl_config_,
enabled_cipher_suites));
There's actually yet another case, but I don't think we can do much about it.

IF the server have previously negotiated TLS 1.1, say, because of the fallback.
Then we'll have a session cached for that server. NSS then clamps the
client_version to the session's version when offering a resumption.

(BoringSSL doesn't do this. It's dumb. NSS cites this recommendation in the spec
which I think was flat-out wrong.)

[Ryan Sleevi, 2014-12-10 22:53:06]

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:246: if (IsSecureTLSCipherSuite(*it)) {
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> Just to confirm, this is the right check, yes?

Yes

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:269: if (!advertise_http2) {
On 2014/12/10 21:29:57, David Benjamin wrote:
> I'm mildly unhappy that the SSL layer needs to know whether about HTTP/2's
> cipher suite requirements (rather than thinking ALPN tokens are opaque strings
> and have a higher-level layer deal), but checking cipher suites at any other
> layer would be extremely messy. So this is fine.

It's almost like you were at the last IETF meeting ;)

On a more practical matter, does it make sense to have the SSLClientSocket
provide some form of callback after this socket has been configured, to let
applications choose the ALPN strings then? It seems like it'd be a little weird
to implement given the ConnectJob layering (for example, AFAIK, the
SSLClientSocket connect job won't have been bound yet to the HTTP
ConnectJob/Request layer, since the pools are in the way, right?), but it might
allow setting policy elsewhere.

Alternatively, having the higher layer pass in a policy selector that can choose
the ALPN strings for a given socket. For now, we might just give the policy
selector the effective set of enabled ciphersuites and TLS versions, in the
future we might pass additional details.

WDYT?

> 
> If we were to care about that, I'd say to not bother with checking
> disabled_cipher_suites. That only ever gets set via command-line flag or
> enterprise policy it seems.

Yes, although it still feels weird to request something from the client we know
we won't support. This is the same argument MSFT has made on the server side.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:978: const std::vector<uint16> cipher_suites
= GetNSSEnabledCipherSuites();
On 2014/12/10 21:29:57, David Benjamin wrote:
> I don't think this excludes AES-GCM when unimplemented.

Correct. NumImplemented returns what NSS is aware of, but it doesn't do the run
time detection until later. This is just a static list of all it's theoretically
capable of.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:992: IsSecurityAdequateForHTTP2(ssl_config_,
enabled_cipher_suites));
On 2014/12/10 21:29:57, David Benjamin wrote:
> There's actually yet another case, but I don't think we can do much about it.
> 
> IF the server have previously negotiated TLS 1.1, say, because of the
fallback.
> Then we'll have a session cached for that server. NSS then clamps the
> client_version to the session's version when offering a resumption.
> 
> (BoringSSL doesn't do this. It's dumb. NSS cites this recommendation in the
spec
> which I think was flat-out wrong.)

File an NSS bug?

Then again, when BoringSSL tries TLS 1.3, where the master secret negotiation
has changed, BoringSSL will be the wrong one ;)

[Bence, 2014-12-11 15:51:40]

Patchset #13 (id:280001) has been deleted

[Bence, 2014-12-11 16:50:50]

Ryans: PTAL.  David: what's up with the linux_chromium_gn_dbg failure that
started with Patch Set 11?  Thanks.

https://codereview.chromium.org/757033004/diff/200001/net/socket/nss_ssl_util.cc
File net/socket/nss_ssl_util.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/nss_ssl_util...
net/socket/nss_ssl_util.cc:140: }
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> nit: net style says to not use {}s on 1 line if statements.
> 
> (Or is this clang-formatted?

This is formatted by git cl format, which does not enforce this net style rule.

Done.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:244: for (std::vector<uint16>::const_iterator it
= cipher_suites.begin();
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> for (uint16 cipher : cipher_suites) {
> }

Done.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:272: proto <= kProtoSPDY4MaximumVersion) {
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> consider adding an IsHttp2NextProto(NextProto next_proto) method since this
> logic is now in two places and it's a bit opaque.

It will not be in two places after rebasing on https://crrev.com/794563003,
please revisit if you still prefer factoring into its own function.  Also note
that once HTTP/2 is finalized, there will be a singe |proto == kProtoSPDY4|
here.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.h:213: // that is up to Section 9.2 of the HTTP/2
specification.  Note that the
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> s/that is up to/that satisfies/

Done.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.h:221: // adequate cipher suites and TLS are also
advertised.
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> Instead of talking about adequate cipher suites and TLS versions, I would just
> refer to |advertise_http2|. Or possibly consider reversing the polarity and
> making it |exclude_http2|. As currently written, if advertise_http2 = true, I
> might expect that that *forces* http2 (which it doesn't do obviously).

I decided against reversing polarity, because I do not want a negation like
SerializeNextProtos(!IsSecurityAdequate), not do I like reversing the polarity
of IsSecurityAdequate.

Done.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:978: const std::vector<uint16> cipher_suites
= GetNSSEnabledCipherSuites();
On 2014/12/10 21:29:57, David Benjamin wrote:
> I don't think this excludes AES-GCM when unimplemented.

Done.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:980: // suites, because NSS does not know
about them quite yet.
On 2014/12/10 21:29:57, David Benjamin wrote:
> Is this true? Core::Init is called from line 3304, after
disabled_cipher_suites
> is processed. There's SSL_CipherPrefGet, though that does a linear search. I
> wonder if we want to optimize that or expose another function from libssl.
> (That's bundled so we can patch it freely.)
> 
> For more fun, there's apparently a separate "policy" flag to each cipher suite
> which is ANOTHER way to enable/disable them. But we never set it, so let's
> ignore it.
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/ns...

Done.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:983: it != cipher_suites.end(); ++it) {
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> for (uint16 cipher : cipher_suites) {
> }

Moot.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:986: *it) ==
ssl_config_.disabled_cipher_suites.end()) {
On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> Hm, I wonder if this logic would be better done down in
> SSLClientSocketNSS::InitializeSSLOptions where we currently handle
> disabled_cipher_suites. (I guess we can't do that because we need to serialize
> next protos already?)
> 
> In that case, I wonder if we could move that code up here? perhaps the SSL
folks
> can comment.

For the time being, I decided to use SSL_CipherPrefGet, and I think this is a
correct solution.  Yes, I agree that moving lines 3227--3233 here would make
sense.  Can we do it on a separate CL?

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:992: IsSecurityAdequateForHTTP2(ssl_config_,
enabled_cipher_suites));
On 2014/12/10 21:29:57, David Benjamin wrote:
> There's actually yet another case, but I don't think we can do much about it.
> 
> IF the server have previously negotiated TLS 1.1, say, because of the
fallback.
> Then we'll have a session cached for that server. NSS then clamps the
> client_version to the session's version when offering a resumption.
> 
> (BoringSSL doesn't do this. It's dumb. NSS cites this recommendation in the
spec
> which I think was flat-out wrong.)

Acknowledged.

https://codereview.chromium.org/757033004/diff/240001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/757033004/diff/240001/net/net.gypi#newcode157
net/net.gypi:157: 'ssl/ssl_cipher_suite_names.h',
davidben: linux_chromium_gn_dbg started failing since I added these lines with a
relevant linker error: "multiple definition of [function defined in
ssl_cipher_suite_names.h]".

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:259: next_proto <= kProtoSPDY4MaximumVersion) {
Ryan: do I need to use curly braces if the statement is a one liner but the
condition is not?  Could you send me a link to the net style guide?

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:984: enabled) {
Do I need curly braces in case of a multi-line condition and one line statement?

[Ryan Hamilton, 2014-12-11 19:59:05]

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:269: if (!advertise_http2) {
On 2014/12/10 22:53:06, Ryan Sleevi wrote:
> On 2014/12/10 21:29:57, David Benjamin wrote:
> > I'm mildly unhappy that the SSL layer needs to know whether about HTTP/2's
> > cipher suite requirements (rather than thinking ALPN tokens are opaque
strings
> > and have a higher-level layer deal), but checking cipher suites at any other
> > layer would be extremely messy. So this is fine.
> 
> It's almost like you were at the last IETF meeting ;)
> 
> On a more practical matter, does it make sense to have the SSLClientSocket
> provide some form of callback after this socket has been configured, to let
> applications choose the ALPN strings then? It seems like it'd be a little
weird
> to implement given the ConnectJob layering (for example, AFAIK, the
> SSLClientSocket connect job won't have been bound yet to the HTTP
> ConnectJob/Request layer, since the pools are in the way, right?), but it
might
> allow setting policy elsewhere.
> 
> Alternatively, having the higher layer pass in a policy selector that can
choose
> the ALPN strings for a given socket. For now, we might just give the policy
> selector the effective set of enabled ciphersuites and TLS versions, in the
> future we might pass additional details.
> 
> WDYT?

We thought about this when bnc and I were brainstorming this. I'm totally with
you that it seems like that would be a "better" layering, but as you say, I'm
not at all sure what we would callback into? I guess we could add some sort of
NextProtoVectorSanitizerCallback to the SSLSocketParams which could implement
this logic? I'm not sure that the cleaner layering would be worth the extra
plumbing, but I could be convinced otherwise. What do you think?

[Ryan Hamilton, 2014-12-11 20:06:10]

Looking good.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:986: *it) ==
ssl_config_.disabled_cipher_suites.end()) {
On 2014/12/11 16:50:49, Bence wrote:
> On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> > Hm, I wonder if this logic would be better done down in
> > SSLClientSocketNSS::InitializeSSLOptions where we currently handle
> > disabled_cipher_suites. (I guess we can't do that because we need to
serialize
> > next protos already?)
> > 
> > In that case, I wonder if we could move that code up here? perhaps the SSL
> folks
> > can comment.
> 
> For the time being, I decided to use SSL_CipherPrefGet, and I think this is a
> correct solution.  Yes, I agree that moving lines 3227--3233 here would make
> sense.  Can we do it on a separate CL?

Sure, just add a TODO.

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:259: next_proto <= kProtoSPDY4MaximumVersion) {
On 2014/12/11 16:50:49, Bence wrote:
> Ryan: do I need to use curly braces if the statement is a one liner but the
> condition is not?  Could you send me a link to the net style guide?

Yes, that's the convention. I'm not sure this is officially written down
anywhere but it's pretty much the received wisedom. That being said, if you run
your cl through git cl format we're all supposed to be happy with the format so
feel free to do that.

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:984: enabled) {
On 2014/12/11 16:50:50, Bence wrote:
> Do I need curly braces in case of a multi-line condition and one line
statement?

Yes, that's the convention.

https://codereview.chromium.org/757033004/diff/320001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/757033004/diff/320001/net/net.gypi#newcode157
net/net.gypi:157: 'ssl/ssl_cipher_suite_names.h',
These files appear to already be in net.gypi. Do they really need to be in it
twice?

[Bence, 2014-12-12 15:49:25]

Ryan: PTAL.  David: PTAL at ssl_client_socket_nss.cc.  BTW I figured out the
NaCl duplicate definition build issue.

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:986: *it) ==
ssl_config_.disabled_cipher_suites.end()) {
On 2014/12/11 20:06:09, Ryan Hamilton wrote:
> On 2014/12/11 16:50:49, Bence wrote:
> > On 2014/12/10 21:11:41, Ryan Hamilton wrote:
> > > Hm, I wonder if this logic would be better done down in
> > > SSLClientSocketNSS::InitializeSSLOptions where we currently handle
> > > disabled_cipher_suites. (I guess we can't do that because we need to
> serialize
> > > next protos already?)
> > > 
> > > In that case, I wonder if we could move that code up here? perhaps the SSL
> > folks
> > > can comment.
> > 
> > For the time being, I decided to use SSL_CipherPrefGet, and I think this is
a
> > correct solution.  Yes, I agree that moving lines 3227--3233 here would make
> > sense.  Can we do it on a separate CL?
> 
> Sure, just add a TODO.

Done.

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:259: next_proto <= kProtoSPDY4MaximumVersion) {
On 2014/12/11 20:06:09, Ryan Hamilton wrote:
> On 2014/12/11 16:50:49, Bence wrote:
> > Ryan: do I need to use curly braces if the statement is a one liner but the
> > condition is not?  Could you send me a link to the net style guide?
> 
> Yes, that's the convention. I'm not sure this is officially written down
> anywhere but it's pretty much the received wisedom. That being said, if you
run
> your cl through git cl format we're all supposed to be happy with the format
so
> feel free to do that.

Acknowledged.

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/300001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:984: enabled) {
On 2014/12/11 20:06:10, Ryan Hamilton wrote:
> On 2014/12/11 16:50:50, Bence wrote:
> > Do I need curly braces in case of a multi-line condition and one line
> statement?
> 
> Yes, that's the convention.

Acknowledged.

https://codereview.chromium.org/757033004/diff/320001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/757033004/diff/320001/net/net.gypi#newcode157
net/net.gypi:157: 'ssl/ssl_cipher_suite_names.h',
On 2014/12/11 20:06:10, Ryan Hamilton wrote:
> These files appear to already be in net.gypi. Do they really need to be in it
> twice?

You are right, I should have moved it instead of copying it.

Done.

[Ryan Hamilton, 2014-12-12 18:56:44]

LGTM, but please wait for David's approval before landing.

Thanks for doing this CL!

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:258: if (kProtoSPDY4MinimumVersion <= next_proto
&&
nit: might as well make this a single if statement with 3 conditions.

[davidben, 2014-12-12 19:45:36]

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:986: if (PK11_TokenExists(cipher) &&
We talked about this out-of-band, but to publish this comment: PK11_TokenExists
takes a CK_MECHANISM_TYPE, not a cipher suite. NSS has to do other work to
translate between a cipher suite to the mechanisms it needs. Unfortunately, that
seems to happen pretty late.

Probably simplest to only check PK11_TokenExists for AES-GCM. That's the only
case that varies that we care about.

[Bence, 2014-12-12 20:21:34]

David: PTAL.

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:258: if (kProtoSPDY4MinimumVersion <= next_proto
&&
On 2014/12/12 18:56:44, Ryan Hamilton wrote:
> nit: might as well make this a single if statement with 3 conditions.

Done.

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/350001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:986: if (PK11_TokenExists(cipher) &&
On 2014/12/12 19:45:36, David Benjamin wrote:
> We talked about this out-of-band, but to publish this comment:
PK11_TokenExists
> takes a CK_MECHANISM_TYPE, not a cipher suite. NSS has to do other work to
> translate between a cipher suite to the mechanisms it needs. Unfortunately,
that
> seems to happen pretty late.
> 
> Probably simplest to only check PK11_TokenExists for AES-GCM. That's the only
> case that varies that we care about.

Done.

[Ryan Sleevi, 2014-12-12 21:33:29]

LGTM, but two concerns below:

https://codereview.chromium.org/757033004/diff/420001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/757033004/diff/420001/net/http/http_network_s...
net/http/http_network_session.cc:29: #include "net/socket/ssl_client_socket.h"
unnecessary?

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:977: #endif
Don't mix #defines in bodies like this. This block should go on line 151 -
immediately after the headers, before any code - because this is effectively a
"control for older headers" check.

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:985: ssl_config_.next_protos,
PK11_TokenExists(CKM_AES_GCM));
I'm fine with the simplified form - the death of NSS will make this easier.

However, I think we can make this a little more robust for the edge cases:

For bulk encryption:
(PK11_TokenExists(CKM_AES_GCM) || PK11_TokenExists(CKM_NSS_CHACHA20_POLY1305))

Will need to ifdef dance to CKM_NSS_CHACHA20_POLY1305 to (CKM_NSS + 26)

For key agreement:
(PK11_TokenExists(CKM_DH_PKCS_DERIVE) || PK11_TokenExists(CKM_ECDH1_DERIVE))

Shouldn't need to #ifdef dance this

The reason for also checking key agreement is that some distros (Fedora, Red
Hat, and, IIRC, Debian) also disable ECC. If that's the case, then they're
leaving the key agreement alg to DH, which some disable by policy.

The word you're looking for is "yay"

Put together:

((PK11_TokenExists(CKM_AES_GCM) || PK11_TokenExists(CKM_NSS_CHACHA20_POLY1305))
&& (PK11_TokenExists(CKM_DH_PKCS_DERIVE) || PK11_TokenExists(CKM_ECDH1_DERIVE)))

Finally, it doesn't seem like you're checking for TLS1.2 support here. 
IsSecurityAdequateForHTTP2 does, but you don't call that here, so you should
also be testing that (in the ssl_config_, presumably)

[davidben, 2014-12-12 21:56:22]

(Patch set 21 seems to have compile problems.)

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:985: ssl_config_.next_protos,
PK11_TokenExists(CKM_AES_GCM));
On 2014/12/12 21:33:28, Ryan Sleevi wrote:
> The reason for also checking key agreement is that some distros (Fedora, Red
> Hat, and, IIRC, Debian) also disable ECC. If that's the case, then they're
> leaving the key agreement alg to DH, which some disable by policy.

Wait, seriously, we can't do ECDH on Fedora, Red Hat, and Debian?


> ((PK11_TokenExists(CKM_AES_GCM) ||
PK11_TokenExists(CKM_NSS_CHACHA20_POLY1305))
> && (PK11_TokenExists(CKM_DH_PKCS_DERIVE) ||
PK11_TokenExists(CKM_ECDH1_DERIVE)))
> 
> Finally, it doesn't seem like you're checking for TLS1.2 support here. 
> IsSecurityAdequateForHTTP2 does, but you don't call that here, so you should
> also be testing that (in the ssl_config_, presumably)

Note that this still doesn't absolve us of assuming the server implements 9.2.2.
Google servers will speak RSA and ECDHE_RSA, but not DHE_RSA.

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.h:215: static bool IsCipherAdequateForHTTP2(
Nit: IsCipherAdequateForHTTP2 -> HasCipherAdequateForHTTP2 or maybe
HasHTTP2CompatibleCipher.

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:792: "!aECDH:!AESGCM+AES256");
For more fun, BoringSSL also has some bits where the configured cipher suite
list is filtered based on other parameters. I believe the only case you care
about is the PSK ones, so add an !aPSK.

Feel free to drop !IDEA, !FZA, and !SRP if you like. Those were deleted from
BoringSSL.

(Or just not bother with the disabled_cipher_suites bit because that's only
controlled by a command-line flag anyway. On BoringSSL, unlike NSS, we don't
have any supported variability of this sort in what ciphers we do and don't
support.)

[Bence, 2014-12-12 22:08:44]

Sorry for the frequent patches and broken compiles, by local compile was not
done yet since the rebase.

https://codereview.chromium.org/757033004/diff/420001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/757033004/diff/420001/net/http/http_network_s...
net/http/http_network_session.cc:29: #include "net/socket/ssl_client_socket.h"
On 2014/12/12 21:33:28, Ryan Sleevi wrote:
> unnecessary?

Used in line 169, see comment in Patch Set 7.

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:977: #endif
On 2014/12/12 21:33:28, Ryan Sleevi wrote:
> Don't mix #defines in bodies like this. This block should go on line 151 -
> immediately after the headers, before any code - because this is effectively a
> "control for older headers" check.

Done.

https://codereview.chromium.org/757033004/diff/420001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:985: ssl_config_.next_protos,
PK11_TokenExists(CKM_AES_GCM));
On 2014/12/12 21:33:28, Ryan Sleevi wrote:
> I'm fine with the simplified form - the death of NSS will make this easier.
> 
> However, I think we can make this a little more robust for the edge cases:
> 
> For bulk encryption:
> (PK11_TokenExists(CKM_AES_GCM) || PK11_TokenExists(CKM_NSS_CHACHA20_POLY1305))
> 
> Will need to ifdef dance to CKM_NSS_CHACHA20_POLY1305 to (CKM_NSS + 26)
> 
> For key agreement:
> (PK11_TokenExists(CKM_DH_PKCS_DERIVE) || PK11_TokenExists(CKM_ECDH1_DERIVE))
> 
> Shouldn't need to #ifdef dance this
> 
> The reason for also checking key agreement is that some distros (Fedora, Red
> Hat, and, IIRC, Debian) also disable ECC. If that's the case, then they're
> leaving the key agreement alg to DH, which some disable by policy.
> 
> The word you're looking for is "yay"
> 
> Put together:
> 
> ((PK11_TokenExists(CKM_AES_GCM) ||
PK11_TokenExists(CKM_NSS_CHACHA20_POLY1305))
> && (PK11_TokenExists(CKM_DH_PKCS_DERIVE) ||
PK11_TokenExists(CKM_ECDH1_DERIVE)))
> 
> Finally, it doesn't seem like you're checking for TLS1.2 support here. 
> IsSecurityAdequateForHTTP2 does, but you don't call that here, so you should
> also be testing that (in the ssl_config_, presumably)

Done.

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.h:215: static bool IsCipherAdequateForHTTP2(
On 2014/12/12 21:56:22, David Benjamin wrote:
> Nit: IsCipherAdequateForHTTP2 -> HasCipherAdequateForHTTP2 or maybe
> HasHTTP2CompatibleCipher.

Done.

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/757033004/diff/440001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:792: "!aECDH:!AESGCM+AES256");
On 2014/12/12 21:56:22, David Benjamin wrote:
> For more fun, BoringSSL also has some bits where the configured cipher suite
> list is filtered based on other parameters. I believe the only case you care
> about is the PSK ones, so add an !aPSK.
> 
> Feel free to drop !IDEA, !FZA, and !SRP if you like. Those were deleted from
> BoringSSL.
> 
> (Or just not bother with the disabled_cipher_suites bit because that's only
> controlled by a command-line flag anyway. On BoringSSL, unlike NSS, we don't
> have any supported variability of this sort in what ciphers we do and don't
> support.)

Done.

[Bence, 2014-12-13 00:09:31]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-12-13 00:11:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/757033004/520001

[commit-bot: I haz the power, 2014-12-13 02:20:25]

Committed patchset #25 (id:520001)

[commit-bot: I haz the power, 2014-12-13 02:21:28]

Patchset 25 (id:??) landed as
https://crrev.com/1e7575035b6a712eae7972304dc7b44af329626e
Cr-Commit-Position: refs/heads/master@{#308226}