Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1403863002:
Enable exporting SSLKEYLOGFILE on Android w/ command line arguments (Closed)
|
Created:
5 years, 2 months ago by Zhongyi Shi Modified:
5 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable exporting SSLKEYLOGFILE on Android w/ command line arguments
This moves handling of the SSLKEYLOGFILE environment variable from
//net to //chrome, and enables a command-line flag to support
Android builds. This has the side-effect of disabling the
SSLKEYLOGFILE support for //content-embedders, which must now
use SSLClientSocket::SetSSLKeyLogFile
Note that SSLClientSocket::SetSSLKeyLogFile is a temporary
solution until the SSLClientSocket can have its global context
broken up; in an ideal world, this would be a parameter off the
HttpNetworkSession::Params, and embedders should ideally handle
this environment variable similar to such params.
BUG=535184
Committed: https://crrev.com/81f85c6d903cd698fafbf1fbfe4e6dc101536070
Cr-Commit-Position: refs/heads/master@{#354562}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Avoid setting env variable, use static method #Patch Set 3 : linker support for android #Patch Set 4 : openssl envrironment #
Total comments: 7
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : fix windows #Patch Set 8 : handle env in io_thread #
Total comments: 9
Patch Set 9 : #
Total comments: 11
Patch Set 10 : wrapping up #
Total comments: 10
Patch Set 11 : #
Total comments: 2
Patch Set 12 : #
Messages
Total messages: 48 (13 generated)
zhongyi@chromium.org changed reviewers: + bmcquade@chromium.org, davidben@chromium.org
Hi guys, This CL tried to set env variable with cmd line args to export SSLkey files, though I thought this method is not efficient and reliable as env variable change should be explicitly set by the user. However, at least we are able to export SSLkey files w/o explicit export env vars in linux. Like David suggested in the original issue discussion, better way to do that is to call SSLClientSocketOpenSSL::SetKeyLogFile somewhere in //chrome. I am still working on figuring out the best spot in //chrome to call and export files. Any suggestion or insight is appreciated. Thanks, Cherie
zhongyi@chromium.org changed reviewers: + rch@chromium.org
https://codereview.chromium.org/1403863002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:562: env->SetVar("SSLKEYLOGFILE", switch_value); Thanks for putting this patch together! I don't know this code well, but it seems like it might be better to not use the base::Environment instance as a means for passing this down. davidben had suggested: "ideally this would work as some parameter hanging off the HttpNetworkSession or something else that gets routed into the SSL_CTX from BoringSSL". It's not clear to me how to push this information down to where it's needed though. The logic to set up the SSL_CTX is in the SSLClientSocketOpenSSL::SSLContext singleton which is private to the SSLClientSocketOpenSSL implementation. The most direct way might be to add a 'static void SetSslKeyLogFile(const char* filename);' to SSLClientSocketOpenSSL which could push this into the singleton SSLClientSocketOpenSSL::SSLContext instance. We could also add a 'string ssl_key_log_file' param to HttpNetworkSession::Params, and then push down the HttpNetworkSession or SSLClientSocketOpenSSL::SSLContext::Params to a static method on SSLClientSocketOpenSSL responsible for pushing the relevant config info into SSLClientSocketOpenSSL::SSLContext. I'm not familiar enough with the code to know if this is desirable or if there's a good way to do that. David, can you give guidance here? What's the best way to plumb this string value down to the SSLContext singleton instance?
https://codereview.chromium.org/1403863002/diff/1/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/1/chrome/browser/io_thread.cc... chrome/browser/io_thread.cc:562: env->SetVar("SSLKEYLOGFILE", switch_value); On 2015/10/13 14:33:36, Bryan McQuade wrote: > Thanks for putting this patch together! > > I don't know this code well, but it seems like it might be better to not use the > base::Environment instance as a means for passing this down. > > davidben had suggested: "ideally this would work as some parameter hanging off > the HttpNetworkSession or something else that gets routed into the SSL_CTX from > BoringSSL". > > It's not clear to me how to push this information down to where it's needed > though. The logic to set up the SSL_CTX is in the > SSLClientSocketOpenSSL::SSLContext singleton which is private to the > SSLClientSocketOpenSSL implementation. > > The most direct way might be to add a 'static void SetSslKeyLogFile(const char* > filename);' to SSLClientSocketOpenSSL which could push this into the singleton > SSLClientSocketOpenSSL::SSLContext instance. > > We could also add a 'string ssl_key_log_file' param to > HttpNetworkSession::Params, and then push down the HttpNetworkSession or > SSLClientSocketOpenSSL::SSLContext::Params to a static method on > SSLClientSocketOpenSSL responsible for pushing the relevant config info into > SSLClientSocketOpenSSL::SSLContext. I'm not familiar enough with the code to > know if this is desirable or if there's a good way to do that. > > David, can you give guidance here? What's the best way to plumb this string > value down to the SSLContext singleton instance? I think something on HttpNetworkSession::Params is, long-term, desirable. (Probably not a string because we want all out HttpNetworkSessions to write to one file, so some kind of SSLKeyLogger object.) But we can't do that while SSL_CTX is on a singleton. I mean to fix that at some point but haven't yet. So in the meantime, I think the static method on SSLClientSocketOpenSSL is the way to go. https://codereview.chromium.org/1403863002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1403863002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:979: const char kSSLKeyLogFile[] = "sslkey-log-file"; Nit: I'd probably call this ssl-key-log-file
Just a couple additional comments. Looks very good overall. https://codereview.chromium.org/1403863002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:560: std::string switch_value = command_line.GetSwitchValueNative( consider calling this ssl_key_log_file instead of switch_value. https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.cc (right): https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.cc:477: scoped_ptr<base::Environment> env(base::Environment::Create()); I think there's some benefit to centralizing the logic that decides what filename to use here, and moving the env lookup to the code you've added in io_thread.cc (and also removing the env lookup logic in the SSLContext constructor). The logic in io_thread.cc could do something like: 1. check to see if env variable is set. if so, use it (pass its value to SSLClientSocketOpenSSL::SetSSLKeyLogFile) 2. if env variable not set, check to see if command line flag is set. if so, use it. (and we may wish to log a warning if both the env variable and command line flag are set to different values) https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:58: static void SetSslKeyLogFile(std::string ssl_keylog_file); nit: since other names use 'SSL' in all caps, let's do the same here (Ssl -> SSL). https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:58: static void SetSslKeyLogFile(std::string ssl_keylog_file); if there's not a strong need to use std::string here, might as well use const char* or base::StringPiece instead.
On 2015/10/14 00:38:54, Bryan McQuade wrote: > Just a couple additional comments. Looks very good overall. > > https://codereview.chromium.org/1403863002/diff/60001/chrome/browser/io_threa... > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/1403863002/diff/60001/chrome/browser/io_threa... > chrome/browser/io_thread.cc:560: std::string switch_value = > command_line.GetSwitchValueNative( > consider calling this ssl_key_log_file instead of switch_value. > > https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_openssl.cc (right): > > https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_openssl.cc:477: scoped_ptr<base::Environment> > env(base::Environment::Create()); > I think there's some benefit to centralizing the logic that decides what > filename to use here, and moving the env lookup to the code you've added in > io_thread.cc (and also removing the env lookup logic in the SSLContext > constructor). The logic in io_thread.cc could do something like: > > 1. check to see if env variable is set. if so, use it (pass its value to > SSLClientSocketOpenSSL::SetSSLKeyLogFile) > 2. if env variable not set, check to see if command line flag is set. if so, use > it. > (and we may wish to log a warning if both the env variable and command line flag > are set to different values) > > https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... > File net/socket/ssl_client_socket_openssl.h (right): > > https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_openssl.h:58: static void > SetSslKeyLogFile(std::string ssl_keylog_file); > nit: since other names use 'SSL' in all caps, let's do the same here (Ssl -> > SSL). > > https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... > net/socket/ssl_client_socket_openssl.h:58: static void > SetSslKeyLogFile(std::string ssl_keylog_file); > if there's not a strong need to use std::string here, might as well use const > char* or base::StringPiece instead. Thanks for the feedback. Working on the compile and linker issues for the time being. Should be able to wrapped it up tomorrow. Might need someone else to patch it and do a quick verify whether it works for chrome on Android.
Patchset #5 (id:80001) has been deleted
So I tested the code on Linux environment, and it works with the command line option --ssl-key-log-file=[absolute path for the key log file]. However, it happens to me there are some adb issues with android devices. Can anyone patch the cl and do a quick verification on Android? Otherwise, I can do that tomorrow when I bring the other Android device to the office. https://codereview.chromium.org/1403863002/diff/1/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1403863002/diff/1/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:979: const char kSSLKeyLogFile[] = "sslkey-log-file"; On 2015/10/13 16:07:39, David Benjamin wrote: > Nit: I'd probably call this ssl-key-log-file Done. https://codereview.chromium.org/1403863002/diff/60001/chrome/browser/io_threa... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/60001/chrome/browser/io_threa... chrome/browser/io_thread.cc:560: std::string switch_value = command_line.GetSwitchValueNative( On 2015/10/14 00:38:54, Bryan McQuade wrote: > consider calling this ssl_key_log_file instead of switch_value. Done. https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:58: static void SetSslKeyLogFile(std::string ssl_keylog_file); On 2015/10/14 00:38:54, Bryan McQuade wrote: > nit: since other names use 'SSL' in all caps, let's do the same here (Ssl -> > SSL). Done. https://codereview.chromium.org/1403863002/diff/60001/net/socket/ssl_client_s... net/socket/ssl_client_socket_openssl.h:58: static void SetSslKeyLogFile(std::string ssl_keylog_file); On 2015/10/14 00:38:54, Bryan McQuade wrote: > if there's not a strong need to use std::string here, might as well use const > char* or base::StringPiece instead. Just to keep consistent with the usage in this file, when getting switch value, we always use std::string.
Thanks! This basically looks good to me with these comments addressed, though davidben is the real owner/expert here, so I defer to him for final review. https://codereview.chromium.org/1403863002/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:566: if (env->GetVar("SSLKEYLOGFILE", &env_keylog_file) && let's avoid looking up the env var twice. how about pulling this lookup up right below the base::Environment::Create() line so we can avoid doing the lookup 2x. something like: scoped_ptr<base::Environment> env(base::Environment::Create()); std::string env_keylog_file; env->GetVar("SSLKEYLOGFILE", &env_keylog_file); std::string flag_keylog_file; if (command_line.HasSwitch(switches::kSSLKeyLogFile)) { flag_keylog_file = command_line.GetSwitchValueASCII(...); if (flag_keylog_file.empty()) LOG(WARNING) << "..."; } if (!env_keylog_file.empty() && !flag_keylog_file.empty()) LOG(WARNING) << "..."; const std::string& keylog_file = !flag_keylog_file.empty() ? flag_keylog_file : env_keylog_file; if (!keylog_file.empty()) { #if defined(USE_OPENSSL) ... } https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... net/socket/ssl_client_socket.h:117: static void SetSSLKeyLogFile(std::string ssl_keylog_file); suggest using 'const std::string&' rather than 'std::string' to avoid the unnecessary copy. https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... net/socket/ssl_client_socket.h:117: static void SetSSLKeyLogFile(std::string ssl_keylog_file); i'm wondering if we should #if defined(USE_OPENSSL) the entire function definition, in order to avoid allowing anyone to mistakenly call the method when they are on a platform that doesn't have openssl. Will defer to davidben to decide. https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:40: class NET_EXPORT SSLClientSocketOpenSSL : public SSLClientSocket { do you still need to NET_EXPORT now that this gets invoked only by SSLClientSocket? I'm new to this codebase, so I'm not sure, but think we should avoid NET_EXPORT here if we can. https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:58: static void SetSSLKeyLogFile(std::string ssl_keylog_file); same - 'const std::string&' rather than 'std::string'
https://codereview.chromium.org/1403863002/diff/160001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/160001/chrome/browser/io_thre... chrome/browser/io_thread.cc:566: if (env->GetVar("SSLKEYLOGFILE", &env_keylog_file) && On 2015/10/15 01:05:26, Bryan McQuade wrote: > let's avoid looking up the env var twice. how about pulling this lookup up right > below the base::Environment::Create() line so we can avoid doing the lookup 2x. > > something like: > scoped_ptr<base::Environment> env(base::Environment::Create()); > std::string env_keylog_file; > env->GetVar("SSLKEYLOGFILE", &env_keylog_file); > > std::string flag_keylog_file; > if (command_line.HasSwitch(switches::kSSLKeyLogFile)) { > flag_keylog_file = command_line.GetSwitchValueASCII(...); > if (flag_keylog_file.empty()) LOG(WARNING) << "..."; > } > > if (!env_keylog_file.empty() && !flag_keylog_file.empty()) > LOG(WARNING) << "..."; > > const std::string& keylog_file = > !flag_keylog_file.empty() ? flag_keylog_file : env_keylog_file; > if (!keylog_file.empty()) { > #if defined(USE_OPENSSL) > ... > } Done. https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... net/socket/ssl_client_socket.h:117: static void SetSSLKeyLogFile(std::string ssl_keylog_file); On 2015/10/15 01:05:26, Bryan McQuade wrote: > suggest using 'const std::string&' rather than 'std::string' to avoid the > unnecessary copy. Done. https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:40: class NET_EXPORT SSLClientSocketOpenSSL : public SSLClientSocket { On 2015/10/15 01:05:26, Bryan McQuade wrote: > do you still need to NET_EXPORT now that this gets invoked only by > SSLClientSocket? I'm new to this codebase, so I'm not sure, but think we should > avoid NET_EXPORT here if we can. We no looger need the NET_EXPORT, as the it's not used outside the //net stack. https://codereview.chromium.org/1403863002/diff/160001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:58: static void SetSSLKeyLogFile(std::string ssl_keylog_file); On 2015/10/15 01:05:26, Bryan McQuade wrote: > same - 'const std::string&' rather than 'std::string' Done.
lgtm. thanks! https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:576: const std::string ssl_keylog_file = if possible, this should be const std::string& to avoid the copy.
On 2015/10/15 02:09:47, Bryan McQuade wrote: > lgtm. thanks! > > https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... > chrome/browser/io_thread.cc:576: const std::string ssl_keylog_file = > if possible, this should be const std::string& to avoid the copy. Sorry for missing the const std::string&, will update that based on feedback from David. :P
Thanks! Just minor nits. Hopefully I can find the time soon to smash SSLClientSocket::SSLContext to pieces so this is less messy. It's clearly a problem. :-) https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:557: Optional: I probably wouldn't bother trying to log when both are set and stuff. You probably can make this logic simpler with a helper std::string GetSSLKeyLogFile(const base::CommandLine& command_line) and then you can just early return out of there. https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:580: #if defined(USE_OPENSSL) You can remove this. I'm pretty sure anything using io_thread.cc can assume USE_OPENSSL at this point. https://codereview.chromium.org/1403863002/diff/180001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1403863002/diff/180001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:978: // Command line flag exporting SSL key to the file path provided. Perhaps: // Causes SSL key material to be logged to the specified file for debugging purposes. See https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format for the format. https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:90: LOG(WARNING) << "No openSSL support"; I'd just use NOTIMPLEMENTED(); https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:56: // Export ssl key log files if env variable is not set. This comment's no longer accurate I think. Comment should also mention that it must be called before SSLClientSockets are created. (Also worth mentioning on the ssl_client_socket.h wrapper since that's the one //chrome uses anyway.)
https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:557: On 2015/10/15 16:46:00, David Benjamin wrote: > Optional: I probably wouldn't bother trying to log when both are set and stuff. > You probably can make this logic simpler with a helper > > std::string GetSSLKeyLogFile(const base::CommandLine& command_line) > > and then you can just early return out of there. Done. https://codereview.chromium.org/1403863002/diff/180001/chrome/browser/io_thre... chrome/browser/io_thread.cc:580: #if defined(USE_OPENSSL) On 2015/10/15 16:46:00, David Benjamin wrote: > You can remove this. I'm pretty sure anything using io_thread.cc can assume > USE_OPENSSL at this point. Done. https://codereview.chromium.org/1403863002/diff/180001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/1403863002/diff/180001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:978: // Command line flag exporting SSL key to the file path provided. On 2015/10/15 16:46:00, David Benjamin wrote: > Perhaps: > > // Causes SSL key material to be logged to the specified file for debugging > purposes. See > https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format for > the format. Done. https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket.cc (right): https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket.cc:90: LOG(WARNING) << "No openSSL support"; On 2015/10/15 16:46:00, David Benjamin wrote: > I'd just use > NOTIMPLEMENTED(); Done. https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... File net/socket/ssl_client_socket_openssl.h (right): https://codereview.chromium.org/1403863002/diff/180001/net/socket/ssl_client_... net/socket/ssl_client_socket_openssl.h:56: // Export ssl key log files if env variable is not set. On 2015/10/15 16:46:00, David Benjamin wrote: > This comment's no longer accurate I think. Comment should also mention that it > must be called before SSLClientSockets are created. (Also worth mentioning on > the ssl_client_socket.h wrapper since that's the one //chrome uses anyway.) Done.
One last set of style nits. :-) https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.cc:538: void IOThread::GetSSLKeyLogFile(const base::CommandLine& command_line, This can just be a function defined after line 299 up in the anonymous namespace and then you don't need to mention it in the header. https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.cc:539: std::string& ssl_keylog_file){ Style guide prohibits non-const reference parameters. This should either be a pointer or just have it return std::string since you don't have any other outputs. https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.cc:576: if (!ssl_keylog_file.empty()) { Nit: I actually really dislike this rule and prefer curly braces everywhere, but Chromium prefers to omit them. :-( https://codereview.chromium.org/1403863002/diff/200001/net/socket/ssl_client_... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/1403863002/diff/200001/net/socket/ssl_client_... net/socket/ssl_client_socket.h:118: // supports OPENSSL. Must be called before SSLClientSockets are created. Nit: supports OPENSSL -> uses OpenSSL
https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.cc:538: void IOThread::GetSSLKeyLogFile(const base::CommandLine& command_line, if this method doesn't depend any IOThread class state, you can move it to the anonymous namespace at the top of the file rather than making it part of the IOThread class. this also allows you to omit it from the header file, which keeps the header shorter. https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.h:272: void GetSSLKeyLogFile(const base::CommandLine& command_line, this should return std::string rather than using an out parameter. see https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... for more info.
Patchset #11 (id:220001) has been deleted
Patchset #11 (id:240001) has been deleted
https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.cc:538: void IOThread::GetSSLKeyLogFile(const base::CommandLine& command_line, On 2015/10/15 19:11:12, Bryan McQuade wrote: > if this method doesn't depend any IOThread class state, you can move it to the > anonymous namespace at the top of the file rather than making it part of the > IOThread class. this also allows you to omit it from the header file, which > keeps the header shorter. Done. https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.cc:539: std::string& ssl_keylog_file){ On 2015/10/15 19:08:23, David Benjamin wrote: > Style guide prohibits non-const reference parameters. This should either be a > pointer or just have it return std::string since you don't have any other > outputs. Done. https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1403863002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.h:272: void GetSSLKeyLogFile(const base::CommandLine& command_line, On 2015/10/15 19:11:13, Bryan McQuade wrote: > this should return std::string rather than using an out parameter. see > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... > for more info. Done. https://codereview.chromium.org/1403863002/diff/200001/net/socket/ssl_client_... File net/socket/ssl_client_socket.h (right): https://codereview.chromium.org/1403863002/diff/200001/net/socket/ssl_client_... net/socket/ssl_client_socket.h:118: // supports OPENSSL. Must be called before SSLClientSockets are created. On 2015/10/15 19:08:23, David Benjamin wrote: > Nit: supports OPENSSL -> uses OpenSSL Done.
Oh no, I forgot! This will... Nah, lgtm. :-) Thanks!
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/1403863002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403863002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403863002/260001
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...)
The CQ bit was checked by bmcquade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403863002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403863002/260001
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...)
On 2015/10/16 00:59:03, commit-bot: I haz the power wrote: > 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...) Whoops, lack of coverage on io_thread.cc. Can someone has ownership of that file do review for me? Thanks a lot!
It'd be nice if there were a unit test for this, but that would be exceedingly painful. Instead, have you verified that this works correctly by building chrome? Otherwise, minor formatting for chrome/browser/io_thread and then it'll be good to do. https://codereview.chromium.org/1403863002/diff/260001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/260001/chrome/browser/io_thre... chrome/browser/io_thread.cc:181: } Nit: I would format this slightly differently. (In particular, no else after a return from an if, ssl_keylog_file => file and some minor cosmetics. std::string GetSSLKeyLogFile(const base::CommandLine& command_line){ if (command_line.HasSwitch(switches::kSSLKeyLogFile)) { std::string file = command_line.GetSwitchValueASCII(switches::kSSLKeyLogFile); if (!file.empty()) return file; LOG(WARNING) << "ssl-key-log-file argument missing"; } scoped_ptr<base::Environment> env(base::Environment::Create()); std::string file; env->GetVar("SSLKEYLOGFILE", &file); return file; }
On 2015/10/16 02:37:30, Ryan Hamilton wrote: > It'd be nice if there were a unit test for this, but that would be exceedingly > painful. Instead, have you verified that this works correctly by building > chrome? > > Otherwise, minor formatting for chrome/browser/io_thread and then it'll be good > to do. > > https://codereview.chromium.org/1403863002/diff/260001/chrome/browser/io_thre... > File chrome/browser/io_thread.cc (right): > > https://codereview.chromium.org/1403863002/diff/260001/chrome/browser/io_thre... > chrome/browser/io_thread.cc:181: } > Nit: I would format this slightly differently. (In particular, no else after a > return from an if, ssl_keylog_file => file and some minor cosmetics. > > std::string GetSSLKeyLogFile(const base::CommandLine& command_line){ > if (command_line.HasSwitch(switches::kSSLKeyLogFile)) { > std::string file = > command_line.GetSwitchValueASCII(switches::kSSLKeyLogFile); > if (!file.empty()) > return file; > > LOG(WARNING) << "ssl-key-log-file argument missing"; > } > scoped_ptr<base::Environment> env(base::Environment::Create()); > std::string file; > env->GetVar("SSLKEYLOGFILE", &file); > return file; > } have built that on my machine and it works.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
This is a late drive by, but is there a reason this is in chrome_switches and not content_switches? I believe we want this to affect content-using applications, including Opera, the same way the environment variable does. As Android is also a content-using application, isn't that the right layer?
On 2015/10/16 16:34:28, Ryan Sleevi (OOO) wrote: > This is a late drive by, but is there a reason this is in chrome_switches and > not content_switches? > > I believe we want this to affect content-using applications, including Opera, > the same way the environment variable does. As Android is also a content-using > application, isn't that the right layer? Android uses much of //chrome too, so this'll still affect them. No clue about Opera. I don't have terribly strong opinions one way or the other w.r.t. chrome_switches vs content_switches. There is some messiness that, until we make SSLClientSocketOpenSSL less broken about SSL_CTX usage, we have to run this very early and //content isn't the one creating URLRequestContexts so we have to be a little fussy about it happening at the right point in startup at a layer that knows enough of what's going on globally. Actually, once we fix SSLClientSocketOpenSSL, this'll be an HttpNetworkSession or whatever parameter, and //content doesn't create URLRequestContexts. So I think //chrome is correct.
On 2015/10/16 16:37:44, David Benjamin wrote: > Actually, once we fix SSLClientSocketOpenSSL, this'll be an HttpNetworkSession > or whatever parameter, and //content doesn't create URLRequestContexts. So I > think //chrome is correct. So this breaks behaviour for Opera and CEF, so I don't think that's a good solution :)
On 2015/10/16 16:45:31, Ryan Sleevi (OOO) wrote: > On 2015/10/16 16:37:44, David Benjamin wrote: > > Actually, once we fix SSLClientSocketOpenSSL, this'll be an HttpNetworkSession > > or whatever parameter, and //content doesn't create URLRequestContexts. So I > > think //chrome is correct. > > So this breaks behaviour for Opera and CEF, so I don't think that's a good > solution :) Do you have an alternative solution that would work? I think Opera and CEF can just turn it on if they want it. This shouldn't be in //net for the same reason that BoringSSL interpreting the environment variable internally would be wrong. //content vs //chrome is a different matter since //content already deals with files and command-line arguments. But the current //content //chrome split, for better or worse (worse in many ways, IMO), makes the embedder create URLRequestContexts. That part is a mess, but I do not think this change should block on this cleanup. Our startup process is extremely messy and will take someone (almost certainly me or Matt) to take a couple uninterrupted weeks to just figure out how it should look like, much less do the fixing.
On 2015/10/16 16:50:48, David Benjamin wrote: > On 2015/10/16 16:45:31, Ryan Sleevi (OOO) wrote: > > On 2015/10/16 16:37:44, David Benjamin wrote: > > > Actually, once we fix SSLClientSocketOpenSSL, this'll be an > HttpNetworkSession > > > or whatever parameter, and //content doesn't create URLRequestContexts. So I > > > think //chrome is correct. > > > > So this breaks behaviour for Opera and CEF, so I don't think that's a good > > solution :) > > Do you have an alternative solution that would work? > > I think Opera and CEF can just turn it on if they want it. This shouldn't be in > //net for the same reason that BoringSSL interpreting the environment variable > internally would be wrong. //content vs //chrome is a different matter since > //content already deals with files and command-line arguments. But the current > //content //chrome split, for better or worse (worse in many ways, IMO), makes > the embedder create URLRequestContexts. That part is a mess, but I do not think > this change should block on this cleanup. Our startup process is extremely messy > and will take someone (almost certainly me or Matt) to take a couple > uninterrupted weeks to just figure out how it should look like, much less do the > fixing. (Taking this CL's approach and just putting it someone in //content's startup also isn't okay because that cannot be the end state. It means we have globals everywhere, threading problems, and calling the function before SSLClientSockets are created is a problem. The only reason I'm stamped this version right now is because SSLClientSocketOpenSSL uses SSL_CTX wrong and I haven't had the time yet to fix it.)
https://codereview.chromium.org/1403863002/diff/260001/chrome/browser/io_thre... File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/1403863002/diff/260001/chrome/browser/io_thre... chrome/browser/io_thread.cc:181: } On 2015/10/16 02:37:30, Ryan Hamilton wrote: > Nit: I would format this slightly differently. (In particular, no else after a > return from an if, ssl_keylog_file => file and some minor cosmetics. > > std::string GetSSLKeyLogFile(const base::CommandLine& command_line){ > if (command_line.HasSwitch(switches::kSSLKeyLogFile)) { > std::string file = > command_line.GetSwitchValueASCII(switches::kSSLKeyLogFile); > if (!file.empty()) > return file; > > LOG(WARNING) << "ssl-key-log-file argument missing"; > } > scoped_ptr<base::Environment> env(base::Environment::Create()); > std::string file; > env->GetVar("SSLKEYLOGFILE", &file); > return file; > } Done.
chrome/browser/io_thread.{h,cc} LGTM
I've not reviewed the rest of the code but others have.
I reworded the description here to better reflect the discussion on this CL, as well as adding the BUG= to link it to the associated bug. I don't want to hold up this CL on figuring out the layering, so LGTM; I've added Opera engineers to the CL as a friendly 'heads up' to them that we'll end up breaking this. My gut is that we should figure out how to be plumbing this through //content, but David and I can chat about that.
On 2015/10/16 18:26:03, Ryan Sleevi (OOO) wrote: > I reworded the description here to better reflect the discussion on this CL, as > well as adding the BUG= to link it to the associated bug. > > I don't want to hold up this CL on figuring out the layering, so LGTM; I've > added Opera engineers to the CL as a friendly 'heads up' to them that we'll end > up breaking this. > > My gut is that we should figure out how to be plumbing this through //content, > but David and I can chat about that. Thanks for updating and taking care of the cl!
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1403863002/#ps280001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1403863002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1403863002/280001
Message was sent while issue was closed.
Committed patchset #12 (id:280001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/81f85c6d903cd698fafbf1fbfe4e6dc101536070 Cr-Commit-Position: refs/heads/master@{#354562} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
