|
|
Created:
4 years, 5 months ago by rolandshoemaker Modified:
3 years, 8 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, estark, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtend the webRequest.onCompleted event details object with TLS/SSL information
If a request has a valid ssl_info field a new object, sslInfo, is added to the
details object passed back to the onCompleted event listener. This object
contains information about the underlying transport that was established
including both the sent and built certificate chains. If a response is loaded
from the cache it will not contain the sent chain since it is not stored
along with the response (as far as I can tell).
I'm slightly concerned that adding the raw bodies of the sent and built certificate
chains may have a negative performance impact since they are relatively large byte
buffers but this is a required feature for HTTPS Everywhere. For a while I toyed
with the idea of adding a JS method to the returned sslInfo object that would return
the raw bodies but looking at the existing code I couldn't figure out how this would
be done (or if it is even possible).
Variable naming in the sslInfo object generally reflects the names used internally for
the corresponding variable but there are some probably a few places where better names
could be chosen for interoperability.
BUG=628819
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove questionably useful fields & add feature switch #
Total comments: 26
Patch Set 3 : Rebase cleanup #Patch Set 4 : Trim more fields and use composed cipher suite name #
Total comments: 1
Patch Set 5 : Use BoringSSL's SSL_CIPHER_get_rfc_name #
Total comments: 8
Patch Set 6 : Consistently use key constants for dict fields and simplify validation error reporting #
Total comments: 18
Messages
Total messages: 34 (5 generated)
Thanks for doing this! Based on https://dev.chromium.org/developers/design-documents/extensions/proposed-chan... I'd suggest emailing apps-dev@chromium.org to get approval for the API change. (They'll probably also have thoughts on the performance impact.) https://codereview.chromium.org/2156763003/diff/1/extensions/browser/api/web_... File extensions/browser/api/web_request/web_request_api_constants.h (right): https://codereview.chromium.org/2156763003/diff/1/extensions/browser/api/web_... extensions/browser/api/web_request/web_request_api_constants.h:54: extern const char kSSLInfoKey[]; Is it necessary to include all of the fields below? I would lean towards not including fields unless there is a use case for them.
On 2016/07/18 23:05:31, estark wrote: > Thanks for doing this! Based on > https://dev.chromium.org/developers/design-documents/extensions/proposed-chan... > I'd suggest emailing mailto:apps-dev@chromium.org to get approval for the API change. > (They'll probably also have thoughts on the performance impact.) Whoops! Somehow I completely missed that page the entire time I was working on this. I should have a proposal doc done soonish and will send it off to the list. > > https://codereview.chromium.org/2156763003/diff/1/extensions/browser/api/web_... > File extensions/browser/api/web_request/web_request_api_constants.h (right): > > https://codereview.chromium.org/2156763003/diff/1/extensions/browser/api/web_... > extensions/browser/api/web_request/web_request_api_constants.h:54: extern const > char kSSLInfoKey[]; > Is it necessary to include all of the fields below? I would lean towards not > including fields unless there is a use case for them.
https://codereview.chromium.org/2156763003/diff/1/extensions/browser/api/web_... File extensions/browser/api/web_request/web_request_api_constants.h (right): https://codereview.chromium.org/2156763003/diff/1/extensions/browser/api/web_... extensions/browser/api/web_request/web_request_api_constants.h:54: extern const char kSSLInfoKey[]; On 2016/07/18 23:05:31, estark wrote: > Is it necessary to include all of the fields below? I would lean towards not > including fields unless there is a use case for them. I think there is a good bit that can be axed. After getting to test this for a while there are a number of things that ended up not being as super informative as I original thought they would be.
palmer@chromium.org changed reviewers: + meacer@chromium.org, palmer@chromium.org, rdevlin.cronin@chromium.org
meacer or rdevlin.cronin, are you good people to review this?
On 2016/08/04 23:56:13, palmer (OOO thru August)) wrote: > meacer or rdevlin.cronin, are you good people to review this? Did the API proposal ever go out?
On 2016/08/09 23:39:35, Devlin wrote: > On 2016/08/04 23:56:13, palmer (OOO thru August)) wrote: > > meacer or rdevlin.cronin, are you good people to review this? > > Did the API proposal ever go out? We'll have a better time to comment on the proposal, but a few high-level thoughts. - This is a bunch of extra stuff. I'd echo estark's question about whether all of these (even the trimmed down list) are really necessary. We can always add more stuff later - removing it is much harder. So starting small is typically best. :) - Given the extra amount of work this can do, it might almost be worthwhile to introduce an option for whether or not these are generated, so that extensions that don't need the information don't get the extra performance hit. - If we go forward with this, we probably don't need the feature switch (but it may depend on if the implementation grows any more complex). Cheers!
This looks super-cool to me. I believe this would also enable one of the extensions that some people had asked about building-- a PetName or Gravatar-style visualization of the target site's certificate that would allow super-savvy users to notice if a cert chain had changed. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... File extensions/common/api/web_request.json (right): https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:157: "enum": ["UNKNOWN", "SSL 2.0", "SSL 3.0", "TLS 1.0", "TLS 1.1", "TLS 1.2", "QUIC"] Does it make sense to add "TLS 1.3" now, given that prototype implementations are underway? Is there any way to make an SSL2 connection in Chrome? Do we need version numbers for QUIC? https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:244: "description": "The subject of the certificate" Other descriptions end with a period. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:249: "description": "The issuer subject of the certificate" Other descriptions end with a period. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:257: "description": "notBefore time of the certificate" Other descriptions end with a period. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:261: "description": "notAfter time of the certificate" Other descriptions end with a period. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:283: "description" :"Name of cipher used" Other descriptions end with a period. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:294: "deflateCompression": { Is there any way to enable (unsafe) deflateCompression any longer? https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:326: "description": "Array of Certificates that was sent by the server.", Is this the list of exactly what the server sent? Or the path we built? Servers can send certs that aren't in the actual path, can't they? https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:342: "description": "True if the leaf certificate issued by known root." I think this is "True if the leaf certificate has a valid path to a known root." ? https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:350: "description": "True if reovcation checking for certificates in the chain is enabled." Typo: s/reovcation/revocation https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:368: "description": "True if a certificate in the chain uses a SHA1 signature." Excluding the root, right? https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:761: "sslInfo": {"$ref": "SSLInfo", "optional": true, "description": "Optional information about the underlying SSL/TLS transport, if one was used."} Is sslInfo the best name, given that the protocol will almost never be SSL?
davidben@chromium.org changed reviewers: + davidben@chromium.org
https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... File extensions/common/api/web_request.json (right): https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:157: "enum": ["UNKNOWN", "SSL 2.0", "SSL 3.0", "TLS 1.0", "TLS 1.1", "TLS 1.2", "QUIC"] On 2016/08/10 14:46:47, elawrence wrote: > Does it make sense to add "TLS 1.3" now, given that prototype implementations > are underway? Is there any way to make an SSL2 connection in Chrome? Do we need > version numbers for QUIC? Indeed a very early stages TLS 1.3 implementation exists behind an about:flags toggle already! https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:162: "enum": ["UNKNOWN", "NULL", "RC4_40", "RC4_128", "RC2_CBC_40", "IDEA_CBC", "DES40_CBC", "DES_CBC", "3DES_EDE_CBC", "AES_128_CBC", "AES_256_CBC", "CAMELLIA_128_CBC", "CAMELLIA_256_CBC", "SEED_CBC", "AES_128_GCM", "AES_256_GCM", "CAMELLIA_128_GCM", "CAMELLIA_256_GCM", "CHACHA20_POLY1305"] (Most of these are things we do not and will never support, FWIW.) https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:167: "enum": ["UNKNOWN", "NULL", "RSA", "RSA_EXPORT", "DH_DSS_EXPORT", "DH_DSS", "DH_RSA_EXPORT", "DH_RSA", "DHE_DSS_EXPORT", "DHE_DSS", "DHE_RSA_EXPORT", "DHE_RSA", "DH_anon_EXPORT", "DH_anon", "ECDH_ECDSA", "ECDHE_ECDSA", "ECDH_RSA", "ECDHE_RSA", "ECDH_anon"] How will this work with TLS 1.3 which is expected to (change still in a PR) look totally different here? Instead what used to be called NamedCurve in TLS 1.2 (a sub-negotiated part of ECDHE) is going to be morally the key exchange with values like secp256r1, x25519, ffdhe2048, etc. The _RSA and _ECDSA half for signing-based exchanges is no longer negotiated as part of the cipher at all. Instead it is, like in TLS 1.2 but not 1.1 and below, part of a signature algorithm negotiation which gives back signature algorithms like rsa_pkcs1_sha256 or rsa_pss_sha256 or ecdsa_p256_sha256. DevTools will need a change here too, but especially if we're doing something where we're committing to an API, this needs some care. If we're decomposing, it should be patterned after TLS 1.3 which actually decomposes. If not patterned after TLS 1.3, it should not try to decompose at all and just give you TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 which will turn into TLS_AES_128_GCM_SHA256 in TLS 1.3 with additional fields for the things on the side. This currently does neither and decomposes yet does not match any true decomposition in any version of the protocol. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:172: "enum": ["UNKNOWN", "NULL", "HMAC-MD5", "HMAC-SHA1", "HMAC-SHA256", "HMAC-SHA384"] What is returned for modern ciphers like AEADs? Best practices these days are *NOT* split up encryption and integrity and use one pre-composed primitive. To that end, it might be better to report values like: - AES_256_GCM - AES_128_GCM - CHACHA20_POLY1305 - AES_128_CBC_SHA - RC4_MD5 (no longer supported) This also aligns with BoringSSL's implementation which pretends everything is vaguely AEAD-shaped. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:294: "deflateCompression": { On 2016/08/10 14:46:47, elawrence wrote: > Is there any way to enable (unsafe) deflateCompression any longer? Nope. BoringSSL does not even implement it. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:357: "$ref": "ValidationErrors" [Anything certificate-related like this should not ship without rsleevi's approval. He would know better which things we should and should not commit to indefinitely by way of an extensions API.] https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:365: "sha1SignaturePresent": { This sort of thing is a temporary (albeit very very long-lived) deprecation thing. It is probably better not to burn this kind of thing into the extensions API. That's something the consumer can extract from the certificate chain if it wants to. But, again, rsleevi's preferences are definitive here.
https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... File extensions/common/api/web_request.json (right): https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:157: "enum": ["UNKNOWN", "SSL 2.0", "SSL 3.0", "TLS 1.0", "TLS 1.1", "TLS 1.2", "QUIC"] On 2016/08/10 14:46:47, elawrence wrote: > Does it make sense to add "TLS 1.3" now, given that prototype implementations > are underway? Is there any way to make an SSL2 connection in Chrome? Do we need > version numbers for QUIC? Acknowledged. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:167: "enum": ["UNKNOWN", "NULL", "RSA", "RSA_EXPORT", "DH_DSS_EXPORT", "DH_DSS", "DH_RSA_EXPORT", "DH_RSA", "DHE_DSS_EXPORT", "DHE_DSS", "DHE_RSA_EXPORT", "DHE_RSA", "DH_anon_EXPORT", "DH_anon", "ECDH_ECDSA", "ECDHE_ECDSA", "ECDH_RSA", "ECDHE_RSA", "ECDH_anon"] On 2016/08/10 19:42:14, davidben wrote: > How will this work with TLS 1.3 which is expected to (change still in a PR) look > totally different here? > > Instead what used to be called NamedCurve in TLS 1.2 (a sub-negotiated part of > ECDHE) is going to be morally the key exchange with values like secp256r1, > x25519, ffdhe2048, etc. > > The _RSA and _ECDSA half for signing-based exchanges is no longer negotiated as > part of the cipher at all. Instead it is, like in TLS 1.2 but not 1.1 and below, > part of a signature algorithm negotiation which gives back signature algorithms > like rsa_pkcs1_sha256 or rsa_pss_sha256 or ecdsa_p256_sha256. > > DevTools will need a change here too, but especially if we're doing something > where we're committing to an API, this needs some care. > > If we're decomposing, it should be patterned after TLS 1.3 which actually > decomposes. If not patterned after TLS 1.3, it should not try to decompose at > all and just give you TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 which will turn into > TLS_AES_128_GCM_SHA256 in TLS 1.3 with additional fields for the things on the > side. > > This currently does neither and decomposes yet does not match any true > decomposition in any version of the protocol. The 'CipherNames', 'KeyExchangeNames', and 'MACNames' fields are extracted from the SSLInfo.connection_status bitmap using net::SSLCipherSuiteToStrings. Currently if the cipher is AEAD the 'MACName' will be, grossly, be set to 'NULL'. Given that the strings produced by this function seem to be the standard format here, and as far as I can currently tell the DevTools/Certificate info UI, I'm not really sure what the best path forward is. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:294: "deflateCompression": { On 2016/08/10 14:46:47, elawrence wrote: > Is there any way to enable (unsafe) deflateCompression any longer? Acknowledged. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:326: "description": "Array of Certificates that was sent by the server.", On 2016/08/10 14:46:47, elawrence wrote: > Is this the list of exactly what the server sent? Or the path we built? Servers > can send certs that aren't in the actual path, can't they? This is pulled from SSLInfo.unverified_cert which is documented as being '[t]he SSL certificate as received by the client' and is differentiated from the chain built during validation. https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:761: "sslInfo": {"$ref": "SSLInfo", "optional": true, "description": "Optional information about the underlying SSL/TLS transport, if one was used."} On 2016/08/10 14:46:47, elawrence wrote: > Is sslInfo the best name, given that the protocol will almost never be SSL? Good point, I originally thought about just using tlsInfo but ended up using what is already used internally, do you think that would be better?
https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... File extensions/common/api/web_request.json (right): https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:167: "enum": ["UNKNOWN", "NULL", "RSA", "RSA_EXPORT", "DH_DSS_EXPORT", "DH_DSS", "DH_RSA_EXPORT", "DH_RSA", "DHE_DSS_EXPORT", "DHE_DSS", "DHE_RSA_EXPORT", "DHE_RSA", "DH_anon_EXPORT", "DH_anon", "ECDH_ECDSA", "ECDHE_ECDSA", "ECDH_RSA", "ECDHE_RSA", "ECDH_anon"] On 2016/08/15 03:38:33, rolandshoemaker wrote: > On 2016/08/10 19:42:14, davidben wrote: > > How will this work with TLS 1.3 which is expected to (change still in a PR) > look > > totally different here? > > > > Instead what used to be called NamedCurve in TLS 1.2 (a sub-negotiated part of > > ECDHE) is going to be morally the key exchange with values like secp256r1, > > x25519, ffdhe2048, etc. > > > > The _RSA and _ECDSA half for signing-based exchanges is no longer negotiated > as > > part of the cipher at all. Instead it is, like in TLS 1.2 but not 1.1 and > below, > > part of a signature algorithm negotiation which gives back signature > algorithms > > like rsa_pkcs1_sha256 or rsa_pss_sha256 or ecdsa_p256_sha256. > > > > DevTools will need a change here too, but especially if we're doing something > > where we're committing to an API, this needs some care. > > > > If we're decomposing, it should be patterned after TLS 1.3 which actually > > decomposes. If not patterned after TLS 1.3, it should not try to decompose at > > all and just give you TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 which will turn > into > > TLS_AES_128_GCM_SHA256 in TLS 1.3 with additional fields for the things on the > > side. > > > > This currently does neither and decomposes yet does not match any true > > decomposition in any version of the protocol. > > The 'CipherNames', 'KeyExchangeNames', and 'MACNames' fields are extracted from > the SSLInfo.connection_status bitmap using net::SSLCipherSuiteToStrings. > Currently if the cipher is AEAD the 'MACName' will be, grossly, be set to > 'NULL'. > > Given that the strings produced by this function seem to be the standard format > here, and as far as I can currently tell the DevTools/Certificate info UI, I'm > not really sure what the best path forward is. They're not really standard format. They're used in one part of the UI which is going to need changing for TLS 1.3. UI is easy because we can change it. Extensions APIs are forever, so it is extremely important that we get that right. I do not think it would be acceptable to ship this API as it stands.
alex.gaynor@gmail.com changed reviewers: + alex.gaynor@gmail.com
https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... File extensions/common/api/web_request.json (right): https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... extensions/common/api/web_request.json:167: "enum": ["UNKNOWN", "NULL", "RSA", "RSA_EXPORT", "DH_DSS_EXPORT", "DH_DSS", "DH_RSA_EXPORT", "DH_RSA", "DHE_DSS_EXPORT", "DHE_DSS", "DHE_RSA_EXPORT", "DHE_RSA", "DH_anon_EXPORT", "DH_anon", "ECDH_ECDSA", "ECDHE_ECDSA", "ECDH_RSA", "ECDHE_RSA", "ECDH_anon"] As one of the folks interested in this API, I think exposing simply the "fully composed" version is sufficient for my needs.
On 2016/08/19 15:53:36, alex.gaynor wrote: > https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... > File extensions/common/api/web_request.json (right): > > https://codereview.chromium.org/2156763003/diff/20001/extensions/common/api/w... > extensions/common/api/web_request.json:167: "enum": ["UNKNOWN", "NULL", "RSA", > "RSA_EXPORT", "DH_DSS_EXPORT", "DH_DSS", "DH_RSA_EXPORT", "DH_RSA", > "DHE_DSS_EXPORT", "DHE_DSS", "DHE_RSA_EXPORT", "DHE_RSA", "DH_anon_EXPORT", > "DH_anon", "ECDH_ECDSA", "ECDHE_ECDSA", "ECDH_RSA", "ECDHE_RSA", "ECDH_anon"] > As one of the folks interested in this API, I think exposing simply the "fully > composed" version is sufficient for my needs. Sorry for the lag, my day job has required my attention. If there is consensus that using the fully composed format (presumably using the IANA designated names) would be acceptable I'd be happy to move forward with that solution. davidben could you point me to where I can find the specific suites that are implemented in BoringSSL so I can only implement for those?
Friendly ping. I'm back and available to review, if that helps. :)
Sorry for the three month pause, I've updated and paired back some of the excess fields. I've also implemented BoringSSL style composed cipher suite names instead of partially decomposing them.
https://codereview.chromium.org/2156763003/diff/60001/net/ssl/ssl_cipher_suit... File net/ssl/ssl_cipher_suite_names.cc (right): https://codereview.chromium.org/2156763003/diff/60001/net/ssl/ssl_cipher_suit... net/ssl/ssl_cipher_suite_names.cc:427: } I haven't looked at the rest yet, but the whole point of using the composed names was to match the spec, which means adding a bunch of code to assemble it manually rather defeats the purpose. BoringSSL provides an SSL_CIPHER_get_rfc_name. https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S...
On 2016/12/07 15:31:45, davidben wrote: > https://codereview.chromium.org/2156763003/diff/60001/net/ssl/ssl_cipher_suit... > File net/ssl/ssl_cipher_suite_names.cc (right): > > https://codereview.chromium.org/2156763003/diff/60001/net/ssl/ssl_cipher_suit... > net/ssl/ssl_cipher_suite_names.cc:427: } > I haven't looked at the rest yet, but the whole point of using the composed > names was to match the spec, which means adding a bunch of code to assemble it > manually rather defeats the purpose. BoringSSL provides an > SSL_CIPHER_get_rfc_name. > > https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#S... D'oh, yup that makes much more sense. I _think_ I properly added the required DEPS file to allow the third_party/boringssl include but I'm not 100%.
Some comments. Also, update the commit message to remove the "first time" commentary. :) https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1285: base::DictionaryValue* dn_dict = new base::DictionaryValue(); Nit/Note: In unambiguous situations like this, you can use auto: auto* dn_dict = new base::DictionaryValue(); but it's a matter of personal taste. https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1323: info->Set("subject", ExtractDN(cert->subject())); Is it better to use your keys::kFooKey constants for these? https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1327: std::vector<std::string>* ip_addrs = new std::vector<std::string>; You leak these pointers — there's no "delete dns_names;" at the end of the function. The norm is: std::vector foo; // stuff return ...; When |foo| goes out of scope, its destructor is called, and that involves releasing any internally-managed storage. Since you need pointers for cert->GetSubjectAltName, use the unary & operator: std::vector dns_names; std::vector ip_addresses; cert->GetSubjectAltName(&dns_names, &ip_addresses); https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1355: base::ListValue* chain = new base::ListValue(); auto here too, if you like. https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1371: static std::unordered_map<net::CertStatus, int> status_to_error_map = { This creates a static initializer, which we don't like: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables. A common idiom you'll see in Chromium code for things like this is: BarEnum FooEnumToBarEnum(FooEnum foo) { switch (foo) { case BLAH: return BLORG; // ... default: NOTREACHED(); return BAD; } } https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1395: for (auto const& error : status_to_error_map) { I think the style guide calls for "const auto& error". Search other code to be sure. https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.h (right): https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.h:354: // Returns a list of SSL errors contained within |status|. Ownership is passed I'd say "X.509 validation errors" rather than "SSL" (or "TLS"), since they are not TLS protocol errors. (And, indeed, it might (?) be useful to expose those someday...?) https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_event_details.h (right): https://codereview.chromium.org/2156763003/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_event_details.h:81: // - sslInfo I'd name it by keys::kWhatever instead.
Description was changed from ========== Extend the webRequest.onCompleted event details object with TLS/SSL information If a request has a valid ssl_info field a new object, sslInfo, is added to the details object passed back to the onCompleted event listener. This object contains information about the underlying transport that was established including both the sent and built certificate chains. If a response is loaded from the cache it will not contain the sent chain since it is not stored along with the response (as far as I can tell). This is my first time working on Chromium and with C++ so I'm pretty sure there are things I'm doing that I definitely shouldn't so please point all of the silly things out! I'm slightly concerned that adding the raw bodies of the sent and built certificate chains may have a negative performance impact since they are relatively large byte buffers but this is a required feature for HTTPS Everywhere. For a while I toyed with the idea of adding a JS method to the returned sslInfo object that would return the raw bodies but looking at the existing code I couldn't figure out how this would be done (or if it is even possible). Variable naming in the sslInfo object generally reflects the names used internally for the corresponding variable but there are some probably a few places where better names could be chosen for interoperability. BUG=628819 ========== to ========== Extend the webRequest.onCompleted event details object with TLS/SSL information If a request has a valid ssl_info field a new object, sslInfo, is added to the details object passed back to the onCompleted event listener. This object contains information about the underlying transport that was established including both the sent and built certificate chains. If a response is loaded from the cache it will not contain the sent chain since it is not stored along with the response (as far as I can tell). I'm slightly concerned that adding the raw bodies of the sent and built certificate chains may have a negative performance impact since they are relatively large byte buffers but this is a required feature for HTTPS Everywhere. For a while I toyed with the idea of adding a JS method to the returned sslInfo object that would return the raw bodies but looking at the existing code I couldn't figure out how this would be done (or if it is even possible). Variable naming in the sslInfo object generally reflects the names used internally for the corresponding variable but there are some probably a few places where better names could be chosen for interoperability. BUG=628819 ==========
rolandshoemaker@gmail.com changed reviewers: + rsleevi@chromium.org
Getting closer! Some nits and comments. The next thing to figure out is who, among Chromium committers, is going to own this going forward (even if Roland continues to tend to it)? meacer or rdevlin.cronin, any interest? If not, I will. I'm probably the least good choice, but I'm happy to if there is nobody else. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1284: static base::DictionaryValue* ExtractDN(const net::CertPrincipal& dn) { Are |ExtractDN| and |ExtractCertificateInfo| part of the public API? (Judging from the .h file, it seems like not?) If not, put them in the anonymous namespace block at the top of this file. That's the equivalent of declaring them static in C, allowing the compiler to easily see the functions are private to this file. Various optimizations may ensue. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1300: base::ListValue* addrs = new base::ListValue(); I'd say it's best to be consistent about using/not using auto for declarations where it's appropriate. Since you use it on line 1285 now, I'd suggest using it here and on line 1305 and wherever else. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1317: std::unique_ptr<base::DictionaryValue> ExtractCertificateInfo( I'm not sure if it's correct to use smart pointers here? I see the 2 call sites of this function are used to put a value in a ListValue. At that point, the ListValue ('s owner) owns the pointers, and will free the memory when the ListValue goes away. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1330: base::ListValue* names = new base::ListValue(); Could use auto here, too, and elsewhere. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1348: der_holder.c_str(), der_holder.size())); Is this formatting the result of `git cl format`? (Maybe it is, I dunno.) Make sure to run that before submitting the CL, in any case.
I'm more uncomfortable with pursuing this than I did in the past, so I'm sorry to push back so much on this. As this particularly represents a more stable API contract than the //net team currently has bandwidth or desire to support (that is, by exposing to extensions, it makes such changes now need to more thoroughly consider any extension-related breakage), it would do really good to capture an explainer about why this belongs as an extension API, what is only possible with it being exposed as part of an extension API (as opposed to, say, simply not being part of the platform), and how those compatibility concerns are less than the benefits that could be received if this is exposed. It also seems we should approach the API surface here the same as we do the Web Platform (c.f. "Extensible Web Manifesto"), and make sure we justify each bit of API surface by establishing that there is either an ample cowpath (not relevant here, arguably) demonstrating a particular idiom, or establishing that a given piece of information can *only* be provided by the browser. Given that DevTools API exposes many of these details now, it's also a fundamental question about whether "Extensions" are the right solution (for whatever problem you're trying to solve), versus other forms such as Headless/DevTools-based (such as WebPageTest/HTTPArchive do) https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:962: request->ssl_info().is_valid()) Braces https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api_constants.cc (right): https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_constants.cc:49: const char kCertificateIssuedByKnownRootKey[] = "issuedByKnownRoot"; This should not be exposed https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1284: static base::DictionaryValue* ExtractDN(const net::CertPrincipal& dn) { I'm not supportive of exposing this information at all 1) It's accessible via JS for those implementations that need (e.g. asn1js), thus indicating its polyfillability does not require a specific platform exposure 2) It introduces a dependency on net::CertPrincipal, which we (//net) folks don't want. 3) It exposes names 'as if' theres only one possible value for a given field. So this would nuke the entire field https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1348: der_holder.c_str(), der_holder.size())); The only field I'm supportive of exposing is the raw DER. Everything else can/should be left up to JS/extensions handling this as appropriate, but we shouldn't impose a particular format or parsing cost on them. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.cc:1365: chain->Append(ExtractCertificateInfo(interCert)); This is a pattern that we've explicitly tried to discourage //net consumers from - creating net::X509Certificates from intermediates. In light of the above comments, this is hopefully entirely unnecessary. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api_helpers.h (right): https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.h:22: #include "net/cert/x509_certificate.h" You're forward declaring this; you don't need to include it. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.h:25: #include "net/ssl/ssl_info.h" This is not used https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.h:351: base::ListValue* ExtractCertificateChain( Because "ownership is passed to the caller", you should be encapsulating this in a std::unique_ptr std::unique_ptr<base::ListValue> ExtractCertificateChain(...) https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api_helpers.h:352: scoped_refptr<net::X509Certificate> cert); Because you're *not* passing ownership (or holding a reference to this), you should be passing this as a naked pointer. net::X509Certificate* cert https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_event_details.cc (right): https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_event_details.cc:264: ssl_info.is_issued_by_known_root); I'm very concerned about exposing this (or any other non-issued by public roots) to extensions, and think that will necessarily merit a very careful review from privacy folks about exposing this to extensions and whether our current permissions accurately reflect the sensitivity of this. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_event_details.cc:270: ssl_info.is_valid() && !net::IsCertStatusError(ssl_info.cert_status)); I'm uncomfortable with us surfacing this as if it was an endorsement for extensions to use. I'm also concerned because I don't believe we do surface information about *invalid* certificates to extensions today. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_event_details.cc:274: built_dict->SetString(keys::kErrorKey, error); This is an explicit non-goal; we do not want the error strings to be part of any API surface, which necessarily this becomes part of. We can/will change error strings, so exposing this detail represents a stronger API contract than we want to expose from //net to extensions. https://codereview.chromium.org/2156763003/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_event_details.cc:278: (ssl_info.cert_status & net::CERT_STATUS_IS_EV)); Can you explain why this is necessary? This represents an API surface that we'd have to maintain, when it's strongly desirable that this not be.
(sorry getting back to this has taken so long, we had some internal back and forth about what we thought would actually be useful) Based on a number of out-of-band conversations I've been convinced that a lot of what I initially suggested exposing is not necessarily that useful and would put a significant burden on the ongoing maintenance efforts of the extensions and net teams. After looking over the initial suggestions again and taking to a handful of other extension devs who are interested in the subject I've boiled down my original proposal to six items that I think would provide the most value and cannot be easily (or at all) extracted from the chain itself or by a third-party service/crawler (the first four have the strongest arguments in my opinion while the last two are slightly less important but would expose useful/interesting information) without putting an unwieldy maintenance or stability burden on Chromium teams going forward. Providing this data via the extensions API would allow the development of: a. plugins that feed third-party telemetry systems like EFF's DSO b. plugins that want to replicate functionality of things like SSL Labs security scoring c. plugins that want to provide lower level/more prominent cert chain introspection than provided by Chromium (or other browsers) existing cert UIs EFF is willing to take the brunt of the maintenance cost of these changes going forward and provide the required engineering time to take weight of the various Chromium teams this will impact where possible. ## Properties to expose: * Built + sent chain (raw DER) Both built and sent chain are useful for knowing which path the validation engine considers canonical + for knowing if the server is sending superfluous certificates in the chain for both third-party telemetry gathering and PageSpeed style extensions. No further processing needs to be done beyond providing the raw DER as these can be trivially parsed by extensions and third-party services * Built chain validity Only way to know if a user clicked through a cert warning, useful for third-party telemetry where third-party doesn't want to attempt to reconstruct behavior of specific validation engine (+ hard to replicate via crawling). Fine with this just being a binary var instead of providing reasoning as to _why_ the chain is considered invalid. * Ciphersuite No way of extracting from chain, useful for determining feature deployment + understanding which suite a server/browser decided on, no real way of replicating via crawling without replicating the various suite lists provided by browsing on an ongoing basis. * TLS version Again no way of extracting from chain, useful for determining version deployment and hard to determine with crawling without replicating the same protocol version support that the browser has on an ongoing basis. There is some complexity in how this would be exposed since version numbers for things like draft vs. final TLS 1.3 and QUIC are kind of fluid. In order to not complicate API stability perhaps just the version indicator ints defined in the various RFCs could be used and no attempt should be made to map these to a human readable string. * OCSP/SCT provided in handshake Useful for determining feature deployment and for determining security level of a TLS session and cannot be extracted from the chain (expect I guess in the case of the leaf containing MustStaple and the chain being considered not valid?). Can be extracted via crawling/third-party services but that becomes somewhat complicated if Chromium/other browsers choose to deploy some kind of expect-ct. * HSTS/HPKP status Cannot be extracted from the chain and useful for measuring feature deployment and security scoring. Hard to replicate via crawling without replicating internal preload lists etc and often relies on state cached by the browser in question which cannot really be replicated.
My specific, immediate, use-cases would be addressed by ciphersuite and TLS version information you listed here, so that all sounds great to me. (In the future, I suspect I'll come up with other things to care about, e.g. X25519 vs. P-256 for ECDH, but those are such distant questions that it's not even worth considering them). It was suggested that it'd be helpful if I articulated my use case. My use case is attempting to collect _data_ to prioritze my complaints to website operators about lack of TLS, and poor TLS quality. I wrote about this recently: https://alexgaynor.net/2017/mar/26/year-of-tracking-http/
Thanks! I think this is super-useful for continued discussion! On 2017/03/29 21:24:28, rolandshoemaker wrote: > ## Properties to expose: > > * Built + sent chain (raw DER) > > Both built and sent chain are useful for knowing which path the validation > engine considers canonical + for knowing if the server is sending superfluous > certificates in the chain for both third-party telemetry gathering and PageSpeed > style extensions. No further processing needs to be done beyond providing the > raw DER as these can be trivially parsed by extensions and third-party services Sounds great, and I agree (tools like asn1.js are fine here) There's a question from a privacy/permissions side about whether or not adding the server sent chain represents a greater privacy risk. We'll need to get this in front of Chrome privacy. I'm unclear, palmer@ - are you able to help nudge and drive that discussion on Roland's behalf? I think it's just a matter of double-checking our permissions and privacy guarantees around webRequest For example, a Chrome App has access to the webRequest API in the <webview>, independent of any special App permissions. If we included the server-sent chain, and there was an enterprise MITM present, that would end up revealing the enterprise MITM to the app, without requiring special permissions. Similarly, does our WebRequest set of permissions (which includes all data on all sites) _also_ presume to include that potentially personally-identifying information? One mitigation to this would be to use is_issued_by_known_root and only surface those chains that were issued by known roots, but then that still acts as a signal to the presence of such a MITM device. I don't _believe_ it's possible today to extract that information as part of the Web Platform. Even features keyed off is_issued_by_known_root (such as HPKP or HSTS-error-blocking) wouldn't be able to set their initial config if a MITM device was present. (Thinking more out loud - it does seem that the WoSign/StartCom deprecation or the Symantec-CT-requirement can be used on the client side to determine if a MITM is present, by using the _success_ of such a certificate to load in Chrome as evidence of a MITM) Anyways, that hopefully articulates why the "sent" chain is more complex/complicated and would need some privacy/security sign-off. > * Built chain validity Yeah, this is already web-observable (by virtue of the load onError event), so there should be no issues here. > * Ciphersuite I think this one represents a flexibility risk. For example, QUIC ciphersuites do not (presently) align with TLS ciphersuites. While work is being done to align the two (replacing QUIC crypto with TLS 1.3 crypto), it's an element of divergence that will be present for "the near term". This is similar to the protocol remarks I raised on the Network Info spec. I _think_ we could probably mitigate this by adding protocol-and-ciphersuite, so that it can be disambiguated. What the 'protocol' value should be (for QUIC drafts and such) would need to be determined, probably by rch@ > * TLS version Same issue as above. Requests won't necessarily use TLS even if "https" :) (Whether or not it's a good thing that Chrome experimented this way is worth considering, but it was safe to do at the time precisely because we didn't have to worry about dependencies like this that relied on the observable behaviour) rch@ would influence API shape. > * OCSP/SCT provided in handshake Can you expand a bit more on this? It sounds like this is because it'd be useful to have a Chrome extension that collects data from users and reports it to a central server. I'm not sure, but that may be against Chrome store policies. Perhaps I've misunderstood though. > * HSTS/HPKP status Define "status"? The server would send the header (and you'd have that data). Alternatively, is this an orthogonal feature set (part of the general "expose API for managing HSTS/HPKP config")?
Oh, and as for how to provide useful next steps: I think the privacy aspect (of sent/built chains) and the API shape (related to ciphersuite and version) may mean it's worth putting up something on GitHub sketching the explainer/shape, just so we can get folks commenting. I would say "Google Docs", but docs can be a poor medium for tracking some of the longer-term design decisions and evolutions for code, so the GH with markdown approach might offer a low-friction way to solicit the feedback from the relevant folks, and also double as a way to get broader engagement. WDYT?
I think there's a relatively simple solution to the TLS version and protocol solutions: they're both strings whose exact contents may change, and we should rename the keys to reflect the multiprotocol nature: perhaps "protocolVersion" and "ciphersuite" (ok, that one still works). I'm not sure if QUIC has a versioning scheme, but "protocolVersion": "QUIC" seems fair. An open question is whether TLS1.3 draft versions should be reflected there, or just "protocolVersion": "TLS1.3". For ciphersuite, we can use the RFC names for TLS; a quick skim of the QUIC crypto-doc suggests it doesn't use named ciphersuites the way TLS does, and instead has a set of negotiated parameters. I'm not sure offhand what the best way to represent that would be.
On 2017/04/01 20:36:13, alex.gaynor wrote: > I think there's a relatively simple solution to the TLS version and protocol > solutions: they're both strings whose exact contents may change, and we should > rename the keys to reflect the multiprotocol nature: perhaps "protocolVersion" > and "ciphersuite" (ok, that one still works). > > I'm not sure if QUIC has a versioning scheme, but "protocolVersion": "QUIC" > seems fair. An open question is whether TLS1.3 draft versions should be > reflected there, or just "protocolVersion": "TLS1.3". > > For ciphersuite, we can use the RFC names for TLS; a quick skim of the QUIC > crypto-doc suggests it doesn't use named ciphersuites the way TLS does, and > instead has a set of negotiated parameters. I'm not sure offhand what the best > way to represent that would be. Hey Alex, Let's take that discussion off the bug review. There's a lot of developer ergonomics at play here, and these issues, like I tried to reference in the previous remark, are ones other specs are addressing. We should strive for compatibility in the platform.
I'm sorry to have been so absent from this. I got mired in several million other things... But yeah, the Github + discussion on one of the Chromium mailing lists would be good. Maybe https://groups.google.com/a/chromium.org/forum/#!forum/privacy and/or chromium-dev?
Throw up a Github, let's small-group iterate it ala incubation, figure out the unknowns, and try to explicitly reach out to The Right Folks for feedback. That'll presumably involve folks from extensions and privacy, but may also need to rope in some of the folks doing some of the other (Web platform-y) features to see how things like this are being tackled in NetInfo. After that, we'll circulate it broadly to chromium-dev@ (where we try to circulate 'design docs', but this is more a design doc one pager, which is just the right level for broader chromium-dev) and begin implementing in parallel. On Mon, Apr 3, 2017 at 7:00 PM, <palmer@chromium.org> wrote: > I'm sorry to have been so absent from this. I got mired in several million > other > things... > > But yeah, the Github + discussion on one of the Chromium mailing lists > would be > good. Maybe https://groups.google.com/a/chromium.org/forum/#!forum/privacy > and/or chromium-dev? > > https://codereview.chromium.org/2156763003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've thrown up a slightly re-formatted version of my arguments above with key names and types for the suggest fields. It could still use a little cleanup and a bit more reasoning about the problem this solves/risks it presents but I thought I'd send out the link now so that we can get the discussions rolling in a slightly better forum as soon as possible. See you over on GitHub! https://github.com/EFForg/webrequest-tlsinfo-api/blob/master/proposal.md On 2017/04/03 23:07:44, Ryan Sleevi wrote: > Throw up a Github, let's small-group iterate it ala incubation, figure out > the unknowns, and try to explicitly reach out to The Right Folks for > feedback. That'll presumably involve folks from extensions and privacy, but > may also need to rope in some of the folks doing some of the other (Web > platform-y) features to see how things like this are being tackled in > NetInfo. > > After that, we'll circulate it broadly to chromium-dev@ (where we try to > circulate 'design docs', but this is more a design doc one pager, which is > just the right level for broader chromium-dev) and begin implementing in > parallel. > > On Mon, Apr 3, 2017 at 7:00 PM, <mailto:palmer@chromium.org> wrote: > > > I'm sorry to have been so absent from this. I got mired in several million > > other > > things... > > > > But yeah, the Github + discussion on one of the Chromium mailing lists > > would be > > good. Maybe https://groups.google.com/a/chromium.org/forum/#!forum/privacy > > and/or chromium-dev? > > > > https://codereview.chromium.org/2156763003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. |