Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1313363003:
Expose OpenSSL's key_exchange_info in the content API (Closed)
DescriptionExpose OpenSSL's key_exchange_info in the content API
The key_exchange_info contains information about the strength of the SSL
key exchange. This information is useful for statistics, user information,
and making trust decisions for connections. This commit makes the information
available in the API.
BUG=525957
Committed: https://crrev.com/79cf372726c52fd12a9009db8735b5098270b6c5
Cr-Commit-Position: refs/heads/master@{#349635}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review fixups: Renumber enum, add tests #
Total comments: 22
Patch Set 3 : Review fixups: Test style, Pickle reordering, ECCurveName() style #
Total comments: 2
Patch Set 4 : Prperly initialize variable #
Total comments: 6
Patch Set 5 : Drop superfluous comment #
Total comments: 4
Patch Set 6 : Comment and DCHECK fixups #Patch Set 7 : Rebase #Patch Set 8 : Fix "unreachable code" warning #Patch Set 9 : Proper #ifdef fix #
Messages
Total messages: 46 (13 generated)
sigbjorn@opera.com changed reviewers: + agl@chromium.org, mmenke@chromium.org, rsleevi@chromium.org, sievers@chromium.org
PTAL. Adds the API, but doesn't use it yet. Planned uses is in statistics (https://codereview.chromium.org/1312363004/ already does this) and in the UI (Opera wants to do this, don't know if Chrome wants to). Depending on what the statistics show, and the deprecation plan for DH, this information might be used to nudge DH 1024 bit out of this world.
https://codereview.chromium.org/1313363003/diff/1/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/1313363003/diff/1/net/ssl/ssl_info.h#newcode66 net/ssl/ssl_info.h:66: // A zero indicates that the value is unknown. Is there a case where for some cipher, an actual value of 0 is possible?
https://codereview.chromium.org/1313363003/diff/1/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/1313363003/diff/1/net/ssl/ssl_info.h#newcode66 net/ssl/ssl_info.h:66: // A zero indicates that the value is unknown. On 2015/08/26 15:28:37, mmenke wrote: > Is there a case where for some cipher, an actual value of 0 is possible? No. It will hold either a bit strength (which necessarily is greater than 0), or an enum value. In theory OpenSSl's internal representation might grow to hold an enum value of 0 in the future, see https://boringssl-review.googlesource.com/#/c/5280/, search for "One question for Adam though". It could still avoid reporting it as 0 to the content layer, even if that was the case.
On 2015/08/26 15:46:15, sigbjorn wrote: > https://codereview.chromium.org/1313363003/diff/1/net/ssl/ssl_info.h > File net/ssl/ssl_info.h (right): > > https://codereview.chromium.org/1313363003/diff/1/net/ssl/ssl_info.h#newcode66 > net/ssl/ssl_info.h:66: // A zero indicates that the value is unknown. > On 2015/08/26 15:28:37, mmenke wrote: > > Is there a case where for some cipher, an actual value of 0 is possible? > > No. It will hold either a bit strength (which necessarily is greater than 0), or > an enum value. > > In theory OpenSSl's internal representation might grow to hold an enum value of > 0 in the future, see https://boringssl-review.googlesource.com/#/c/5280/, search > for "One question for Adam though". It could still avoid reporting it as 0 to > the content layer, even if that was the case. browser/loader LGTM, I'll defer to agl and rsleevi on the rest.
rsleevi@chromium.org changed reviewers: + davidben@chromium.org, estark@chromium.org
At the risk of a pile-on, adding David and Emily, since I believe this affects both of them in ways they may wish to chime in on.
For Chrome, I think I'm much more interested in driving DHE out of the world entirely than trying to slowly raise limits. Raising limits doesn't capture "security level" for DHE anyway. There are other bad groups than just size. On the ECDHE side, we don't advertise very many curves to begin with. It might be useful to histogram those, but some opaque "key_exchange_info" number isn't right for that. (It probably ought to have been that in BoringSSL too, but I'd assumed this would only be something Opera would have in a corner somewhere and got tired of fussing. Otherwise I would have insisted on seeing the Chromium CL first.) (Curves have the additional trouble that we also care about what curves are in the certificate chain, as we discovered when removing P-521. I expect most servers just hard-code P-256 for the key exchange itself anyway.)
https://codereview.chromium.org/1313363003/diff/1/content/common/ssl_status_s... File content/common/ssl_status_serialization.cc (right): https://codereview.chromium.org/1313363003/diff/1/content/common/ssl_status_s... content/common/ssl_status_serialization.cc:36: pickle.WriteInt(ssl_status.key_exchange_info); Can you please update the tests in ssl_status_serialization_unittest.cc to check that this field is serialized and deserialized correctly? (And, if you add a sanity check that key_exchange_info >= 0 when deserializing, please add a unit test for that sanity check too.) https://codereview.chromium.org/1313363003/diff/1/content/common/ssl_status_s... content/common/ssl_status_serialization.cc:77: // Sanity check |security_bits|: the only allowed negative value is -1. Can you do a similar sanity check for |key_exchange_info|? (I assume it should be >=0?)
On 2015/08/26 16:17:10, David Benjamin wrote: > On the ECDHE side, we don't advertise very many curves to begin with. It might > be useful to histogram those, but some opaque "key_exchange_info" number isn't > right for that. (It probably ought to have been that in BoringSSL too, but I'd > assumed this would only be something Opera would have in a corner somewhere and > got tired of fussing. Otherwise I would have insisted on seeing the Chromium CL > first.) If you have another preferred method, it is still not too late to change the boringssl implementation details, I can do that if that is the conclusion here. Whatever comes out of boringssl will still be an opaque enum value for ECDH, which is what will be shown in the histogram too, but possibly via an internal variable with a different name. Histogram: Net.SSL_KeyExchange.ECDHE recorded 10 samples (flags = 0x1) 23 ------------------------------------------------------------------------O (9 = 90.0%) 24 ----------O (1 = 10.0%)
Unrelated, belated email catchup: Can you make sure that changes have bugs :) Helps for Chrome launch tracking, privacy reviews, etc ;)
show-stopper bug: This would break the cache. https://codereview.chromium.org/1313363003/diff/1/net/http/http_response_info.cc File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/1/net/http/http_response_info... net/http/http_response_info.cc:60: RESPONSE_INFO_HAS_KEY_EXCHANGE_INFO = 1 << 10, Renumbering these breaks the cache, since we serialize all of these bits into the state. It needs to go at the end.
https://codereview.chromium.org/1313363003/diff/1/content/common/ssl_status_s... File content/common/ssl_status_serialization.cc (right): https://codereview.chromium.org/1313363003/diff/1/content/common/ssl_status_s... content/common/ssl_status_serialization.cc:36: pickle.WriteInt(ssl_status.key_exchange_info); On 2015/08/26 17:46:41, estark wrote: > Can you please update the tests in ssl_status_serialization_unittest.cc to check > that this field is serialized and deserialized correctly? (And, if you add a > sanity check that key_exchange_info >= 0 when deserializing, please add a unit > test for that sanity check too.) Done. https://codereview.chromium.org/1313363003/diff/1/content/common/ssl_status_s... content/common/ssl_status_serialization.cc:77: // Sanity check |security_bits|: the only allowed negative value is -1. On 2015/08/26 17:46:42, estark wrote: > Can you do a similar sanity check for |key_exchange_info|? (I assume it should > be >=0?) Done. https://codereview.chromium.org/1313363003/diff/1/net/http/http_response_info.cc File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/1/net/http/http_response_info... net/http/http_response_info.cc:60: RESPONSE_INFO_HAS_KEY_EXCHANGE_INFO = 1 << 10, On 2015/08/28 00:25:04, Ryan Sleevi wrote: > Renumbering these breaks the cache, since we serialize all of these bits into > the state. It needs to go at the end. Done.
Ping David/Adam/Ryan. Still waiting for a conclusion if this is wanted in Chromium, or a different implementation is preferred.
If Opera wishes to expose this information in the UI then I think it's reasonable that content should plumb it though. But I don't really review changes in this part of the code any more so I don't have an opinion on the CL itself.
Yeah, I think it's a reasonable request, but there's a number of BUGs here that it may help re-reviewing your code and looking for the gotchas :) https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... File content/common/ssl_status_serialization_unittest.cc (right): https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:25: void VerifyEqual(SSLStatus* const a, SSLStatus* const b) { Both of these should be passed as const-ref, not constant pointers to non-constant objects. https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:25: void VerifyEqual(SSLStatus* const a, SSLStatus* const b) { I'm not sure why you re-invent SSLStatus::Equals. Was it for the printing? If so, just do this in the test: void PrintTo(const SSLStatus& status, ::std::ostream* os) { *os << "Security Style: " << status.security_style; *os << "Cert ID: " << status.cert_id; ... etc } bool SSLStatusAreEqual(const SSLStatus& a, const SSLStatus &b) { return a.Equals(b); } EXPECT_PRED2(SSLStatusAreEqual, status, deserialized); will do all the pretty-printing. https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:43: SSLStatus status, deserialized; Style wise, the original form was more consistent with the style guide SSLStatus status; InitializeDummySSLStatus(&status); std::string serialized = SerializeSecurityInfo(status); SSLStatus deserialized; ASSERT_TRUE(DeserializeSecurityInfo(serialized, &deserialized)); https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:44: InitializeSSLStatus(&status); Naming wise, this might be better renamed, as an SSLStatus (such as status or deserialized) are already de-facto initialized. https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:62: VerifyEqual(&default_ssl_status, &invalid_deserialized); Use of a temp seems unnecessary https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:92: SSLStatus status, default_ssl_status, invalid_deserialized; This sort of rewrite seems inconsistent with the style guide SSLStatus status; InitializeSSLStatus(&status); ... std::string serialized = ... SSLStatus invalid_deserialized; ASSERT_FALSE(DeserializeSecurityInfo(...)); ASSERT_TRUE(SSLStatusAreEqual(SSLStatus(), invalid_deserialized)); https://codereview.chromium.org/1313363003/diff/20001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/20001/net/http/http_response_... net/http/http_response_info.cc:222: } BUG: This is a stable serialization format, as unfortunate as it may be :) As a result, if an older version of Chrome (e.g. Stable) were to read a cache entry from a newer version (e.g. Beta), because they shared the same profile dir, then it wouldn't understand the RESPONSE_INFO_HAS_KEY_EXCHANGE_INFO and would totally explode. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:427: if (!GetCipherProperties(cipher_suite, &key_exchange, &cipher, &mac)) Why do this lookup for non-OpenSSL cases? https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:428: return NULL; nullptr throughout. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:431: case 16: // ECDHE_RSA This seems very brittle; I guess it's contingent upon whether or not the two methods above are auto-generated. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:436: #if defined(USE_OPENSSL) newline between 435/436 for legibility. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (right): https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:64: // is an elliptic curve, and a name is known. Returns NULL otherwise. nullptr ;)
https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... File content/common/ssl_status_serialization_unittest.cc (right): https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:43: SSLStatus status, deserialized; On 2015/09/02 01:37:07, Ryan Sleevi wrote: > Style wise, the original form was more consistent with the style guide > > SSLStatus status; > InitializeDummySSLStatus(&status); > std::string serialized = SerializeSecurityInfo(status); > > SSLStatus deserialized; > ASSERT_TRUE(DeserializeSecurityInfo(serialized, &deserialized)); Done. https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:44: InitializeSSLStatus(&status); On 2015/09/02 01:37:07, Ryan Sleevi wrote: > Naming wise, this might be better renamed, as an SSLStatus (such as status or > deserialized) are already de-facto initialized. Done. https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:62: VerifyEqual(&default_ssl_status, &invalid_deserialized); On 2015/09/02 01:37:07, Ryan Sleevi wrote: > Use of a temp seems unnecessary Done. https://codereview.chromium.org/1313363003/diff/20001/content/common/ssl_stat... content/common/ssl_status_serialization_unittest.cc:92: SSLStatus status, default_ssl_status, invalid_deserialized; On 2015/09/02 01:37:07, Ryan Sleevi wrote: > This sort of rewrite seems inconsistent with the style guide > > SSLStatus status; > InitializeSSLStatus(&status); > ... > std::string serialized = ... > > SSLStatus invalid_deserialized; > ASSERT_FALSE(DeserializeSecurityInfo(...)); > > ASSERT_TRUE(SSLStatusAreEqual(SSLStatus(), invalid_deserialized)); Done. https://codereview.chromium.org/1313363003/diff/20001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/20001/net/http/http_response_... net/http/http_response_info.cc:222: } On 2015/09/02 01:37:07, Ryan Sleevi wrote: > BUG: This is a stable serialization format, as unfortunate as it may be :) > > As a result, if an older version of Chrome (e.g. Stable) were to read a cache > entry from a newer version (e.g. Beta), because they shared the same profile > dir, then it wouldn't understand the RESPONSE_INFO_HAS_KEY_EXCHANGE_INFO and > would totally explode. Moved last, this should hopefully work as the code doesn't seem to have any checks that it reached the end of the Pickle. Please comment if not sufficient. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:427: if (!GetCipherProperties(cipher_suite, &key_exchange, &cipher, &mac)) On 2015/09/02 01:37:07, Ryan Sleevi wrote: > Why do this lookup for non-OpenSSL cases? Done. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:428: return NULL; On 2015/09/02 01:37:07, Ryan Sleevi wrote: > nullptr throughout. Done. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:431: case 16: // ECDHE_RSA On 2015/09/02 01:37:07, Ryan Sleevi wrote: > This seems very brittle; I guess it's contingent upon whether or not the two > methods above are auto-generated. I don't know if the methods above are generated, the mentioned .go file isn't in my checkout, and old versions did not generate any c++ code. To avoid the code duplication I can create an anonymous helper function IsECDHE(), and use that throughout. That would only mess things up if the code is autogenerated though. A solution which is brittle in other places instead, is to skip this check entirely, and always pass |key_exchange_info| to SSL_get_curve_name(). As |key_exchange_info| either contains an enum, or a bit strength (which' value is >> enum values), a nullptr will in practice be returned for non-EC key exchanges anyhow. Up to you. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:436: #if defined(USE_OPENSSL) On 2015/09/02 01:37:07, Ryan Sleevi wrote: > newline between 435/436 for legibility. Code changed. https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.h (right): https://codereview.chromium.org/1313363003/diff/20001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.h:64: // is an elliptic curve, and a name is known. Returns NULL otherwise. On 2015/09/02 01:37:07, Ryan Sleevi wrote: > nullptr ;) Changed to state nullptr, but technically it returns NULL, from Chromium_depot_tools\src\third_party\boringssl\src\ssl\t1_lib.c tls1_ec_curve_id2name
Ryan, care to check the new code?
Still one BUG hiding :) https://codereview.chromium.org/1313363003/diff/40001/content/public/common/s... File content/public/common/ssl_status.h (right): https://codereview.chromium.org/1313363003/diff/40001/content/public/common/s... content/public/common/ssl_status.h:50: int key_exchange_info; BUG: You forgot to initialize this member in the ctor.
https://codereview.chromium.org/1313363003/diff/40001/content/public/common/s... File content/public/common/ssl_status.h (right): https://codereview.chromium.org/1313363003/diff/40001/content/public/common/s... content/public/common/ssl_status.h:50: int key_exchange_info; On 2015/09/08 22:19:49, Ryan Sleevi wrote: > BUG: You forgot to initialize this member in the ctor. Done.
Are there any further issues blocking this?
LGTM. Was just busy swamped with other things. https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... net/http/http_response_info.cc:284: ssl_info.key_exchange_info = key_exchange_info; Is there any validation of |key_exchange_info| that can or should be done after deserialization? https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... net/http/http_response_info.cc:385: // Write key_exchange_info here to provide backwards compatibility Drop this comment; doesn't feel like it's adding much, especially if someone is fresh reading this file. (In particular, ambiguity regarding 'here' compared to....? that knowledge is only clear from the context of this CL, not from the source itself)
https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... net/http/http_response_info.cc:284: ssl_info.key_exchange_info = key_exchange_info; On 2015/09/15 08:23:21, Ryan Sleevi wrote: > Is there any validation of |key_exchange_info| that can or should be done after > deserialization? This is modelled on security_bits above, which is an int with similar characteristics. No further checks are done there. It would be possible to verify it being non-negative in both places. https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... net/http/http_response_info.cc:385: // Write key_exchange_info here to provide backwards compatibility On 2015/09/15 08:23:21, Ryan Sleevi wrote: > Drop this comment; doesn't feel like it's adding much, especially if someone is > fresh reading this file. > > (In particular, ambiguity regarding 'here' compared to....? that knowledge is > only clear from the context of this CL, not from the source itself) Done.
lgtm from me as well, modulo a few nits, despite my very very strong anti-feature bias. :-) Being able to get the curve out isn't crazy and probably worthwhile. It probably would have been helpful as a sanity check when we got rid of P-521 (though the fallout from that did not come from the key exchange curve). For DHE, I doubt anything actionable will come of this information. I still think the only thing useful to do with DHE at this point is to get rid of it altogether. 47 will collide with the holidays and I don't think we want to do anything concurrent with RC4, so I probably won't go down that road until a bit later. But supposedly iOS 9 and OS X 10.11 do this, so we'll see how it goes for them. Initial probes of Alexa top 1m were promising. https://codereview.chromium.org/1313363003/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1313363003/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:126: !request->ssl_info().connection_status); Nit: Existing thing, but this probably ought to be separate DCHECKs and DCHECK_EQ for the two equality ones, so more information is available when it fails. https://codereview.chromium.org/1313363003/diff/80001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/1313363003/diff/80001/net/ssl/ssl_info.h#newc... net/ssl/ssl_info.h:65: // key_exchange_info for more information. Nit: I'd mention this SSL_SESSION from BoringSSL.
https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... net/http/http_response_info.cc:284: ssl_info.key_exchange_info = key_exchange_info; On 2015/09/15 08:34:35, sigbjorn wrote: > On 2015/09/15 08:23:21, Ryan Sleevi wrote: > > Is there any validation of |key_exchange_info| that can or should be done > after > > deserialization? > > This is modelled on security_bits above, which is an int with similar > characteristics. No further checks are done there. It would be possible to > verify it being non-negative in both places. |security_bits| can be -1 and it is actually sanity-checked in DeserializeSecurityInfo: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/ssl...
https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... File net/http/http_response_info.cc (right): https://codereview.chromium.org/1313363003/diff/60001/net/http/http_response_... net/http/http_response_info.cc:284: ssl_info.key_exchange_info = key_exchange_info; On 2015/09/15 14:37:49, estark wrote: > On 2015/09/15 08:34:35, sigbjorn wrote: > > On 2015/09/15 08:23:21, Ryan Sleevi wrote: > > > Is there any validation of |key_exchange_info| that can or should be done > > after > > > deserialization? > > > > This is modelled on security_bits above, which is an int with similar > > characteristics. No further checks are done there. It would be possible to > > verify it being non-negative in both places. > > |security_bits| can be -1 and it is actually sanity-checked in > DeserializeSecurityInfo: > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/ssl... |key_exchange_info| is already sanity-checked in DeserializeSecurityInfo. https://codereview.chromium.org/1313363003/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1313363003/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:126: !request->ssl_info().connection_status); On 2015/09/15 14:33:17, David Benjamin wrote: > Nit: Existing thing, but this probably ought to be separate DCHECKs and > DCHECK_EQ for the two equality ones, so more information is available when it > fails. Done. https://codereview.chromium.org/1313363003/diff/80001/net/ssl/ssl_info.h File net/ssl/ssl_info.h (right): https://codereview.chromium.org/1313363003/diff/80001/net/ssl/ssl_info.h#newc... net/ssl/ssl_info.h:65: // key_exchange_info for more information. On 2015/09/15 14:33:17, David Benjamin wrote: > Nit: I'd mention this SSL_SESSION from BoringSSL. Done.
The CQ bit was checked by sigbjorn@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, rsleevi@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1313363003/#ps100001 (title: "Comment and DCHECK fixups")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313363003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313363003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sigbjorn@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, davidben@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1313363003/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313363003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313363003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by sigbjorn@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, davidben@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1313363003/#ps140001 (title: "Fix "unreachable code" warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313363003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313363003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
The CQ bit was checked by sigbjorn@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, davidben@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1313363003/#ps160001 (title: "Proper #ifdef fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1313363003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1313363003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/79cf372726c52fd12a9009db8735b5098270b6c5 Cr-Commit-Position: refs/heads/master@{#349635} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
