Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1312363004:
Add histogram for key exchange strength (Closed)
DescriptionAdd histogram for key exchange strength
BUG=525957
Committed: https://crrev.com/66456a28179acfaaac34fa0e5c38d571ffe78145
Cr-Commit-Position: refs/heads/master@{#349644}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove Non-ephemeral methods #
Total comments: 4
Patch Set 3 : Update comment #
Total comments: 8
Patch Set 4 : Stricter on what is logged #Messages
Total messages: 27 (7 generated)
sigbjorn@opera.com changed reviewers: + agl@chromium.org, asvitkine@chromium.org, jar@chromium.org, rsleevi@chromium.org
PTAL
On 2015/08/26 15:25:53, sigbjorn wrote: > PTAL Requires https://codereview.chromium.org/1313363003/, this is the first implementation of that API.
https://codereview.chromium.org/1312363004/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/1312363004/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:383: // UMA_HISTOGRAM_... macros cache the Histogram instance and thus only work What decision are we going to make with this data? Also, we don't support DH or ECDH cipher suites.
https://codereview.chromium.org/1312363004/diff/1/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/1312363004/diff/1/net/socket/ssl_client_socke... net/socket/ssl_client_socket_pool.cc:383: // UMA_HISTOGRAM_... macros cache the Histogram instance and thus only work On 2015/08/26 17:28:02, agl wrote: > What decision are we going to make with this data? For now, as long as DH is still supported (for however long that may be), it will help make decisions about how to treat DH, in particular DH 1024. It will also give data points which may be used in determining when and how to deprecate DH. For RSA, the information is available elsewhere, and already histogrammed. I don't know what RSA information is used for, but presumably there is a need for it. This information is currently not available elsewhere for ECDH. The distribution of curves is not equal, knowing which are used is interesting information, even if no actions are yet planned based on the knowledge. In general, this is raw data, available for use in whichever way is fit. > Also, we don't support DH or ECDH cipher suites. Removed
histograms lgtm, but leaving it up to owners to make the call on this
not lgtm Actually, have a question after looking at this deeper. https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24622: + Key exchange information/strength, by connection method. Recorded for each Actually, looking at the comment for key_exchange_info: * DHE: the size, in bits, of the multiplicative group. * RSA: the size, in bits, of the modulus. * ECDHE: the TLS id for the curve. The last point kind of worries me, but I don't know too much about SSL. Is there anything sensitive in the TLS id?
https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24622: + Key exchange information/strength, by connection method. Recorded for each On 2015/08/28 20:39:07, Alexei Svitkine wrote: > Actually, looking at the comment for key_exchange_info: > > * DHE: the size, in bits, of the multiplicative group. > * RSA: the size, in bits, of the modulus. > * ECDHE: the TLS id for the curve. > > The last point kind of worries me, but I don't know too much about SSL. Is there > anything sensitive in the TLS id? No. It's a public list - https://tools.ietf.org/html/rfc4492#section-5.1.1 / http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-param... - that is akin to identifying the (well-known) curve employed.
lgtm https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24622: + Key exchange information/strength, by connection method. Recorded for each On 2015/08/28 20:52:20, Ryan Sleevi wrote: > On 2015/08/28 20:39:07, Alexei Svitkine wrote: > > Actually, looking at the comment for key_exchange_info: > > > > * DHE: the size, in bits, of the multiplicative group. > > * RSA: the size, in bits, of the modulus. > > * ECDHE: the TLS id for the curve. > > > > The last point kind of worries me, but I don't know too much about SSL. Is > there > > anything sensitive in the TLS id? > > No. It's a public list - https://tools.ietf.org/html/rfc4492#section-5.1.1 / > http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-param... > - that is akin to identifying the (well-known) curve employed. Ok, thanks! Maybe worth expanding the comment to have the same info as the bullet points I quoted from the code.
https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1312363004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24622: + Key exchange information/strength, by connection method. Recorded for each > Maybe worth expanding the comment to have the same info as the bullet points I > quoted from the code. Added a pointer to where to find the information. Personally, I prefer to have such information in just one place, to avoid it becoming out of date.
lgtm
davidben@chromium.org changed reviewers: + davidben@chromium.org
Drive-by comment as this was linked from the other CL's bug. https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:385: if (strncmp(str, "RSA", 3) == 0) { I think you an exact match (strcmp) here. https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:388: } else if (strncmp(str, "DHE", 3) == 0) { Nit: I'd do DHE_ and ECDHE_ to make sure you get the separator. https://codereview.chromium.org/1312363004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1312363004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75444: + SSL/TLS connection in the socket pool where Connect() succeeds. Given that these are all totally incomparable things, it doesn't seem there's much point in grouping them together. You could just make them separate histograms, which would also allow you to, for ECDHE, add a enum definition. Likewise, there doesn't seem any sense in logging "Unknown" because values for different key exchanges don't align. NOTREACHED() seems more appropriate.
https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:385: if (strncmp(str, "RSA", 3) == 0) { On 2015/09/15 14:35:35, David Benjamin wrote: > I think you an exact match (strcmp) here. Right now all comparisons use strncmp against the first part of the name only. I find this easy to relate to, without needing to look up the details of the names. I can use strcmp here, and include the underscores below, the code might be more accurate, but presumably also more confusing. E.g. "RSA_EXPORT" is still a valid cipher name, so one would have to do quite some digging to find out that all RSA ciphers are included if doing strcmp. Anyone hunting down a bug (from elsewhere) might suspect the discrepancies between the ciphers here instead. I have no strong opinion though, will change both here and below if you prefer :)
https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:385: if (strncmp(str, "RSA", 3) == 0) { On 2015/09/15 15:35:05, sigbjorn wrote: > On 2015/09/15 14:35:35, David Benjamin wrote: > > I think you an exact match (strcmp) here. > > Right now all comparisons use strncmp against the first part of the name only. I > find this easy to relate to, without needing to look up the details of the > names. > > I can use strcmp here, and include the underscores below, the code might be more > accurate, but presumably also more confusing. E.g. "RSA_EXPORT" is still a valid > cipher name, so one would have to do quite some digging to find out that all RSA > ciphers are included if doing strcmp. Anyone hunting down a bug (from elsewhere) > might suspect the discrepancies between the ciphers here instead. > > I have no strong opinion though, will change both here and below if you prefer > :) We do not support RSA_EXPORT and I can quite confidently say we never will. :-P This logic is already making assumptions about what cipher suites we support due to it omitting some of the crazier things like ECDH_RSA and ECDH_anon. In so far as parsing the names is sound at all, I think it's better to include the separator and/or end-of-string in the comparison, so that you're actually looking at an individual component. For instance, what if ECDHE happened to be called "DHECE" for Diffie-Hellman, Elliptic Curve, Ephemeral (kind of a weird acronym, but we did already end up with one modifier at the end)? Then this code would be broken.
https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_pool.cc (right): https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:385: if (strncmp(str, "RSA", 3) == 0) { On 2015/09/15 15:41:54, David Benjamin wrote: > On 2015/09/15 15:35:05, sigbjorn wrote: > > On 2015/09/15 14:35:35, David Benjamin wrote: > > > I think you an exact match (strcmp) here. > > > > Right now all comparisons use strncmp against the first part of the name only. > I > > find this easy to relate to, without needing to look up the details of the > > names. > > > > I can use strcmp here, and include the underscores below, the code might be > more > > accurate, but presumably also more confusing. E.g. "RSA_EXPORT" is still a > valid > > cipher name, so one would have to do quite some digging to find out that all > RSA > > ciphers are included if doing strcmp. Anyone hunting down a bug (from > elsewhere) > > might suspect the discrepancies between the ciphers here instead. > > > > I have no strong opinion though, will change both here and below if you prefer > > :) > > We do not support RSA_EXPORT and I can quite confidently say we never will. :-P > This logic is already making assumptions about what cipher suites we support due > to it omitting some of the crazier things like ECDH_RSA and ECDH_anon. > > In so far as parsing the names is sound at all, I think it's better to include > the separator and/or end-of-string in the comparison, so that you're actually > looking at an individual component. > > For instance, what if ECDHE happened to be called "DHECE" for Diffie-Hellman, > Elliptic Curve, Ephemeral (kind of a weird acronym, but we did already end up > with one modifier at the end)? Then this code would be broken. Done. https://codereview.chromium.org/1312363004/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_pool.cc:388: } else if (strncmp(str, "DHE", 3) == 0) { On 2015/09/15 14:35:35, David Benjamin wrote: > Nit: I'd do DHE_ and ECDHE_ to make sure you get the separator. Done. https://codereview.chromium.org/1312363004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1312363004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:75444: + SSL/TLS connection in the socket pool where Connect() succeeds. On 2015/09/15 14:35:35, David Benjamin wrote: > Given that these are all totally incomparable things, it doesn't seem there's > much point in grouping them together. You could just make them separate > histograms, which would also allow you to, for ECDHE, add a enum definition. > > Likewise, there doesn't seem any sense in logging "Unknown" because values for > different key exchanges don't align. NOTREACHED() seems more appropriate. Done.
The CQ bit was checked by sigbjorn@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, agl@chromium.org Link to the patchset: https://codereview.chromium.org/1312363004/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312363004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312363004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Accidentally uploaded the rebase for Issue 1313363003 to here instead. Will fix once that is in.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by sigbjorn@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312363004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312363004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/66456a28179acfaaac34fa0e5c38d571ffe78145 Cr-Commit-Position: refs/heads/master@{#349644} |
