|
|
Created:
11 years, 3 months ago by Jaime Soriano Modified:
9 years, 4 months ago CC:
chromium-reviews_googlegroups.com, darin (slow to review) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
Description[Replaced by http://codereview.chromium.org/276037 ]
Provides a certificate for SSL client authentication on NSS sockets.
GUI is still missing, so certificates and private keys have to be
stored manually, p.e.:
$ pk12util -d sql:$HOME/.pki/nssdb -i PKCS12_file.p12
Adds --auto-ssl-client-auth command-line option to enable this feature.
BUG=16830
TEST=Try to connect to a web page that requires SSL authentication and confirm that it connects if and only if a valid certificate is stored in .pki/nssdb database.
Patch Set 1 #Patch Set 2 : '' #
Total comments: 6
Patch Set 3 : '' #
Total comments: 19
Patch Set 4 : '' #
Total comments: 3
Messages
Total messages: 17 (0 generated)
Great! I will look at the patch tomorrow or on Monday. (I'm busy today.) Thanks a lot for the patch.
I took a quick look. The main issue with your patch is that it does not ask the user for approval of sending his client certificate to the server. Please see http://dev.chromium.org/developers/design-documents/ssl-client-authentication for the reason why we need to ask the user for approval. We don't have a cert selection dialog for Linux yet. As a workaround, we can resurrect the --auto-ssl-client-auth command-line option for Linux. See r18819 and r19456.
http://codereview.chromium.org/220009/diff/6001/7002 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/6001/7002#newcode70 Line 70: #include "chrome/common/chrome_switches.h" The net module can't use functions from the chrome module. I will describe the right solution below. http://codereview.chromium.org/220009/diff/6001/7002#newcode717 Line 717: status = NSS_GetClientAuthData(NULL, socket, caNames, pRetCert, pRetKey); We should consider NSS_GetClientAuthData as a reference implementation of the auth cert handler. Its limitation is that it returns only one cert, even if the user has multiple certs that meet the server's criteria: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/auth... Note the "break" statement in that for loop. Our auth cert handler can be based on the NSS_GetClientAuthData algorithm, with the following differences: The first time, it finds all the certs that meet the server's criteria, saves the certs, and then returns SECFailure to make the handshake fail. We need to make sure some other code sets the ERR_SSL_CLIENT_AUTH_CERT_NEEDED error. This error code will cause higher-level code to retrieves the list of client certs from this object, and ask the user to select one. Since we don't have a cert selection dialog for Linux yet, in resource_dispatcher_host.cc, we check CommandLine::ForCurrentProcess()->HasSwitch(switches::kAutoSSLClientAuth). If true, we pick the first cert in the list automatically. If false, we return NULL. If a cert is selected (automatically), the higher-level code will redo the handshake, specifying the chosen client cert for us to use. The second time this auth cert handler is called, the higher-level code should have specified a client cert, chosen by the user, for us to use. Then we just return that cert.
http://codereview.chromium.org/220009/diff/6001/7003 File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/220009/diff/6001/7003#newcode95 Line 95: #if defined(USE_NSS) This test should be #if defined(OS_LINUX) because the issue is that we don't have a cert selection dialog for Linux.
http://codereview.chromium.org/220009/diff/6001/7002 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/6001/7002#newcode717 Line 717: status = NSS_GetClientAuthData(NULL, socket, caNames, pRetCert, pRetKey); On 2009/09/25 15:59:44, wtc wrote: > We should consider NSS_GetClientAuthData as a reference > implementation of the auth cert handler. Please replace "auth cert handler" by "client auth data handler" throughout my comment. Sorry about the confusion. Here is the client auth data handler in Mozilla/Firefox: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSI... You can study its algorithm. Note that it does the UI right in the client auth data handler. We can't do that in Chromium.
http://codereview.chromium.org/220009/diff/6001/7003 File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/220009/diff/6001/7003#newcode95 Line 95: #if defined(USE_NSS) On 2009/09/25 16:01:07, wtc wrote: > This test should be #if defined(OS_LINUX) > because the issue is that we don't have a cert selection > dialog for Linux. I used USE_NSS because my implementation is for NSS sockets. http://codereview.chromium.org/220009/diff/6001/7002 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/6001/7002#newcode717 Line 717: status = NSS_GetClientAuthData(NULL, socket, caNames, pRetCert, pRetKey); On 2009/09/25 15:59:44, wtc wrote: > We should consider NSS_GetClientAuthData as a reference > implementation of the auth cert handler. Its limitation > is that it returns only one cert, even if the user has > multiple certs that meet the server's criteria: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/auth... > > Note the "break" statement in that for loop. > Yes, I used it as initial implementation and it's enough for our needs, but as you say, it's not how the final implementation has to be done. > Our auth cert handler can be based on the > NSS_GetClientAuthData algorithm, with the following > differences: > The first time, it finds all the certs that meet the > server's criteria, saves the certs, and then returns > SECFailure to make the handshake fail. We need to > make sure some other code sets the ERR_SSL_CLIENT_AUTH_CERT_NEEDED error. > This error code will cause higher-level code to > retrieves the list of client certs from this object, > and ask the user to select one. > > Since we don't have a cert selection dialog for Linux > yet, in resource_dispatcher_host.cc, we check > CommandLine::ForCurrentProcess()->HasSwitch(switches::kAutoSSLClientAuth). > If true, we pick the first cert in the list automatically. > If false, we return NULL. > I tried to set the ERR_SSL_CLIENT_AUTH_CERT_NEEDED (see the first patch set), but it seems that ResourceDispatcherHost::OnCertificateRequested and SSLClientSocketNSS::GetSSLCertRequestInfo were never called. What was I doing wrong? As I didn't manage to do it, I included the Chrome switches here instead of in the resource dispatcher in this temporal implementation. > If a cert is selected (automatically), the higher-level > code will redo the handshake, specifying the chosen > client cert for us to use. > > The second time this auth cert handler is called, the > higher-level code should have specified a client cert, > chosen by the user, for us to use. Then we just return > that cert. Ok, I'll try to do it.
http://codereview.chromium.org/220009/diff/8004/8006 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/8004/8006#newcode727 Line 727: if (cert && (privkey = PK11_FindKeyByAnyCert(cert, proto_win))) { PK11_FindKeyByAnyCert provokes segmentation fault sometimes when refreshing a SSL-client-authenticated site: #0 0xb725b8aa in memcpy () from /lib/tls/i686/cmov/libc.so.6 #1 0x09eeeb58 in sqlite3VdbeMemSetStr (pMem=0xb16ff368, z=0x31617a6f <Address 0x31617a6f out of bounds>, n=101658641, enc=0 '\0', xDel=0xffffffff) at /home/kronoss/src/chromium/src/third_party/sqlite/src/vdbemem.c:715 #2 0x09ee9640 in bindText (pStmt=0xb1634528, i=1, zData=0x31617a6f, nData=101658641, xDel=0xffffffff, encoding=0 '\0') at /home/kronoss/src/chromium/src/third_party/sqlite/src/vdbeapi.c:937 #3 0x09ee96f0 in sqlite3_bind_blob (pStmt=0xb1634528, i=1, zData=0x31617a6f, nData=101658641, xDel=0xffffffff) at /home/kronoss/src/chromium/src/third_party/sqlite/src/vdbeapi.c:960 #4 0xb256685d in sdb_FindObjectsInit (sdb=0xb5023d08, template=0xb5a95370, count=1, find=0xb5a952a8) at sdb.c:716 #5 0xb2568814 in sftkdb_FindObjectsInit (handle=0xb242caa8, template=0xb5a95370, count=1, find=0xb5a952a8) at sftkdb.c:1219 #6 0xb25501d7 in sftk_searchDatabase (handle=0xb242caa8, search=0xb162f6e8, pTemplate=0xb5a95370, ulCount=1) at pkcs11.c:4138 #7 0xb2552113 in NSC_FindObjectsInit (hSession=16777217, pTemplate=0xb5a95370, ulCount=1) at pkcs11.c:4263 #8 0xb76731ae in pk11_FindObjectByTemplate (slot=0xb242d5e8, theTemplate=0xb5a95370, tsize=1) at pk11obj.c:1477 #9 0xb765eb6a in PK11_FindObjectForCert (cert=0xb2489d98, wincx=0x0, pSlot=0xb5a953b8) at pk11cert.c:1172 #10 0xb765ecbc in PK11_FindKeyByAnyCert (cert=0xb2489d98, wincx=0x0) at pk11cert.c:1650 #11 0x09c774b8 in net::SSLClientSocketNSS::ClientAuthHandler (arg=0xb1625838, socket=0xb1620998, caNames=0xb5a955b4, pRetCert=0xb16f7874, pRetKey=0xb16f7878) at /home/kronoss/src/chromium/src/net/socket/ssl_client_socket_nss.cc:728 ...
I will try to take a look at this tomorrow. I have been swamped with work. Sorry.
On 2009/10/07 02:41:09, wtc wrote: > I will try to take a look at this tomorrow. I have been swamped > with work. Sorry. Ok, don't worry, thanks!
http://codereview.chromium.org/220009/diff/8004/8006 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/8004/8006#newcode759 Line 759: if ( PK11_FindKeyByAnyCert(cert, proto_win) ) { This key has to be freed
Jaime, Finally I was able to spend some time on this changelist this afternoon. I tested it against https://www.openid.com/, and it works. https://www.openid.com/ does SSL client authentication during a renegotiation handshake, so I needed to add some code to support that. I had a test server that did SSL client authentication during a regular handshake, but I had to reuse that server for some other testing, so I can't test regular SSL client auth today. I edited the description of your CL slightly. One question: since SSL_ClearSessionCache returns void, what do you mean by "SSL_ClearSessionCache fails"? Do you mean it crashes? I suggested some changes below. Depending on your interest in contributing to Chromium, you are welcome to make those changes, or I can fix them for you. Of course, I hope you will continue to contribute to Chromium! Please let me know if you have questions about renegotiation handshake. That's what SSL_ReHandshake does, by the way. Thanks a lot for your help. Could you add a TODO comment about this issue? http://code.google.com/p/chromium/issues/detail?id=13934 Although NSS doesn't have the API limitation of Schannel, we are using NSS as if it had that limitation, so we will also be sending a client cert before we verify the server cert on Linux. I will look into how to fix that. http://codereview.chromium.org/220009/diff/8004/8009 File chrome/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/220009/diff/8004/8009#newcode1060 Line 1060: #else The #else block should include all of the original code in this function, in particular lines 1045-1051. http://codereview.chromium.org/220009/diff/8004/8006 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/8004/8006#newcode369 Line 369: server_cert_verify_result_.Reset(); Let's also reset client_auth_cert_needed_ to false here. http://codereview.chromium.org/220009/diff/8004/8006#newcode498 Line 498: EnterFunction(""); Let's remove the EnterFunction/LeaveFunction unless they are useful to you. (I don't use them.) To match our Windows implementation in ssl_client_socket_win.cc, ClientAuthHandler should simply remember the caNames argument in a member variable, and we look up the client certs (the code in lines 739-769) in this method. http://codereview.chromium.org/220009/diff/8004/8006#newcode707 Line 707: void * arg, The parameters should be indented by 4 spaces. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla... http://codereview.chromium.org/220009/diff/8004/8006#newcode726 Line 726: cert = that->ssl_config_.client_cert->os_cert_handle(); I think we need to acquire a reference to 'cert' with a CERT_DupCertificate(cert) call. http://codereview.chromium.org/220009/diff/8004/8006#newcode759 Line 759: if ( PK11_FindKeyByAnyCert(cert, proto_win) ) { On 2009/10/07 09:58:26, Jaime Soriano wrote: > This key has to be freed Yes, I noticed this issue, too. http://codereview.chromium.org/220009/diff/8004/8006#newcode792 Line 792: if (SECSuccess != SSL_ReHandshake(nss_fd_, PR_TRUE)) { I believe this SSL_ReHandshake call should be removed. I removed it, and the code still works. Why do you need this SSL_ReHandshake call? http://codereview.chromium.org/220009/diff/8004/8006#newcode900 Line 900: int SSLClientSocketNSS::DoPayloadRead() { To support a server-initiated renegotiation handshake, I added the following code here: int SSLClientSocketNSS::DoPayloadRead() { EnterFunction(user_buf_len_); + if (client_auth_cert_needed_) { + user_buf_ = NULL; + LeaveFunction(""); + return ERR_SSL_CLIENT_AUTH_CERT_NEEDED; + } int rv = PR_Read(nss_fd_, user_buf_->data(), user_buf_len_); if (rv >= 0) { LogData(user_buf_->data(), rv); I'm not sure if that's the best solution, but it works. http://codereview.chromium.org/220009/diff/8004/8005 File net/socket/ssl_client_socket_nss.h (right): http://codereview.chromium.org/220009/diff/8004/8005#newcode15 Line 15: #include <keyt.h> Please move <keyt.h> up, right below #undef Lock, and then add a blank line before <string>. The blank line separates the C system headers from C++ system headers. See the example at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... http://codereview.chromium.org/220009/diff/8004/8005#newcode82 Line 82: // NSS calls this when client authentication is required. Nit: change "required" to "requested". It's possible for a web server to request but not require a client cert, so using "requested" is more accurate here. http://codereview.chromium.org/220009/diff/8004/8005#newcode84 Line 84: PRFileDesc *socket, The 2nd - 4th parameters should be aligned with the first parameter. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla... We should rename the function parameters following our coding style. Also in our C++ code we put the * next to the type rather than the parameter/variable. I can fix these nits for you when I check this in.
On 2009/10/10 00:13:34, wtc wrote: > I edited the description of your CL slightly. One question: > since SSL_ClearSessionCache returns void, what do you mean > by "SSL_ClearSessionCache fails"? Do you mean it crashes? > Yes, segmentation fault. > I suggested some changes below. Depending on your interest > in contributing to Chromium, you are welcome to make those > changes, or I can fix them for you. Of course, I hope you > will continue to contribute to Chromium! > Sure, I can continue fixing it :) > Could you add a TODO comment about this issue? > http://code.google.com/p/chromium/issues/detail?id=13934 > Ok, I'll do it. > Although NSS doesn't have the API limitation of Schannel, > we are using NSS as if it had that limitation, so we will > also be sending a client cert before we verify the server > cert on Linux. I will look into how to fix that. > Thanks for your suggestions, I'll continue the work when I have some time, maybe today later or on Monday.
http://codereview.chromium.org/220009/diff/8004/8009 File chrome/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/220009/diff/8004/8009#newcode1060 Line 1060: #else On 2009/10/10 00:13:34, wtc wrote: > The #else block should include all of the original code > in this function, in particular lines 1045-1051. Shouldn't we check if the client certs list is empty in both cases? http://codereview.chromium.org/220009/diff/8004/8006 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/8004/8006#newcode726 Line 726: cert = that->ssl_config_.client_cert->os_cert_handle(); On 2009/10/10 00:13:34, wtc wrote: > I think we need to acquire a reference to 'cert' with a > CERT_DupCertificate(cert) call. mmm, yes, it's possible, I debugged this a bit more, and it seems that cert fields are empty on the next line, what very probably provokes the failures. I'll check with your suggestion, good point. http://codereview.chromium.org/220009/diff/8004/8006#newcode792 Line 792: if (SECSuccess != SSL_ReHandshake(nss_fd_, PR_TRUE)) { On 2009/10/10 00:13:34, wtc wrote: > I believe this SSL_ReHandshake call should be removed. > I removed it, and the code still works. Why do you need > this SSL_ReHandshake call? Without doing that, the client authentication wasn't requested on the "second pass", anyway I'll try again. http://codereview.chromium.org/220009/diff/8004/8006#newcode900 Line 900: int SSLClientSocketNSS::DoPayloadRead() { On 2009/10/10 00:13:34, wtc wrote: > To support a server-initiated renegotiation handshake, I > added the following code here: > > int SSLClientSocketNSS::DoPayloadRead() { > EnterFunction(user_buf_len_); > + if (client_auth_cert_needed_) { > + user_buf_ = NULL; > + LeaveFunction(""); > + return ERR_SSL_CLIENT_AUTH_CERT_NEEDED; > + } > int rv = PR_Read(nss_fd_, user_buf_->data(), user_buf_len_); > if (rv >= 0) { > LogData(user_buf_->data(), rv); > > I'm not sure if that's the best solution, but it works. Ok, I'll add it to the patch. http://codereview.chromium.org/220009/diff/8004/8005 File net/socket/ssl_client_socket_nss.h (right): http://codereview.chromium.org/220009/diff/8004/8005#newcode15 Line 15: #include <keyt.h> On 2009/10/10 00:13:34, wtc wrote: > Please move <keyt.h> up, right below #undef Lock, and > then add a blank line before <string>. > > The blank line separates the C system headers from C++ > system headers. See the example at > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... > I have to take a deeper look to the guidelines :)
I didn't manage to change the way I do the handshake renegotiation. http://codereview.chromium.org/220009/diff/18001/19002 File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/220009/diff/18001/19002#newcode374 Line 374: client_auth_ca_names_ = NULL; If client_auth_ca_names_ is freed here with CERT_FreeDistNames, something fails later double-freeing it. http://codereview.chromium.org/220009/diff/18001/19002#newcode801 Line 801: that->client_auth_ca_names_ = ca_names_copy; I'm not sure if this is the correct way to duplicate a CERTDistNames. It works, but I cannot explicitelly free it. http://codereview.chromium.org/220009/diff/18001/19002#newcode824 Line 824: if (SECSuccess != SSL_ReHandshake(nss_fd_, PR_TRUE)) { I tried to remove this, but then it didn't request the client authentication on the second pass. I also tried with this lines by wtc: > int SSLClientSocketNSS::DoPayloadRead() { > EnterFunction(user_buf_len_); > + if (client_auth_cert_needed_) { > + user_buf_ = NULL; > + LeaveFunction(""); > + return ERR_SSL_CLIENT_AUTH_CERT_NEEDED; > + } > int rv = PR_Read(nss_fd_, user_buf_->data(), user_buf_len_); > if (rv >= 0) {
Jaime, Thanks for the new Patch Set. I didn't have time to review it today. I will try to review and test it tomorrow.
Jaime, I have started to review your new patch set. It has been moved to http://codereview.chromium.org/276037. http://codereview.chromium.org/220009/diff/8004/8009 File chrome/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/220009/diff/8004/8009#newcode1060 Line 1060: #else On 2009/10/10 14:31:50, Jaime Soriano wrote: > > Shouldn't we check if the client certs list is empty in both cases? Yes, we should. In the OS_LINUX case, we check if the client certs list is empty in the ? : expression.
ncluded in the email sent (if any). |