Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1274143002:
ClientCertStoreChromeOS: support additional non-platform certs. (Closed)
|
Created:
5 years, 4 months ago by pneubeck (no reviews) Modified:
5 years, 4 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. |
DescriptionClientCertStoreChromeOS: support additional non-platform certs.
To reduce complexity, this change removes the inheritance of ClientCertStoreChromeOS from net::ClientCertStoreNSS and instead exposes the required functions of ClientCertStoreNSS as static methods.
BUG=514575
Committed: https://crrev.com/385704ecd579795bccfc03079191256700000daa
Cr-Commit-Position: refs/heads/master@{#345288}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Remove inefficient filtering #
Total comments: 2
Patch Set 3 : Refactored to suggested pulling of certificates. #
Total comments: 6
Patch Set 4 : Refactored ClientCertStoreNSS. #
Total comments: 13
Patch Set 5 : Addressed nits. #Patch Set 6 : Rebased. #
Total comments: 5
Patch Set 7 : Addressed Steven's comments. #Messages
Total messages: 41 (18 generated)
Patchset #1 (id:1) has been deleted
pneubeck@chromium.org changed reviewers: + davidben@chromium.org
ptal. I separated this part from the larger CL for better reviewability.
Patchset #1 (id:20001) has been deleted
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos.cc:87: selected_certs->end()); This seems highly inefficient - the operator() operation goes from O(N) [linear scan] to O(N*M), where M is the number of certs, since you iterate through the entirity of |additional_certs| every single pass. It seems you really should do two passes - create a temp CERT_CertList, call ClientCertStoreNSS::GetClientCertsImpl() on it, get filtered_additional_certs. Then call GetClientCertsImpl on cert_list, get selected_certs. Run selected_certs through the predicate. Then selected_certs->insert(selected_certs.end(), filtered_additional_certs.begin(), filtered_additional_certs.end()); This gives you O(N) + O(M) performance. https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos.h (right): https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos.h:40: const CertificateList& additional_certs, Are you sure this is the right pattern? I don't believe it is. It forces your ClientCertStoreChromeOS to know all of the certificates at load time, and prevents you from ever changing them. That certainly doesn't match the NSS guarantees, nor necessarily that of smart cards (e.g. where authenticating to a hostile card may reveal additional certificates). Are you sure it's what you want?
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos.cc:87: selected_certs->end()); On 2015/08/07 23:51:09, Ryan Sleevi wrote: > This seems highly inefficient - the operator() operation goes from O(N) [linear > scan] to O(N*M), where M is the number of certs, since you iterate through the > entirity of |additional_certs| every single pass. > > It seems you really should do two passes - create a temp CERT_CertList, call > ClientCertStoreNSS::GetClientCertsImpl() on it, get filtered_additional_certs. > Then call GetClientCertsImpl on cert_list, get selected_certs. Run > selected_certs through the predicate. Then > > selected_certs->insert(selected_certs.end(), filtered_additional_certs.begin(), > filtered_additional_certs.end()); > > This gives you O(N) + O(M) performance. Done. https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos.h (right): https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos.h:40: const CertificateList& additional_certs, On 2015/08/07 23:51:09, Ryan Sleevi wrote: > Are you sure this is the right pattern? I don't believe it is. > > It forces your ClientCertStoreChromeOS to know all of the certificates at load > time, and prevents you from ever changing them. That certainly doesn't match the > NSS guarantees, nor necessarily that of smart cards (e.g. where authenticating > to a hostile card may reveal additional certificates). Are you sure it's what > you want? Yeah, I thought about that point as well. As far as I understand it, the (only) call chain is URLRequestHttpJob::OnStartCompleted -> URLRequestJob::NotifyCertificateRequested -> URLRequest::NotifyCertificateRequested -> ResourceLoader::OnCertificateRequested -> ResourceContext::CreateClientCertStore (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... ) which is executed everytime for the server requesting a client cert. Of course, there's the chance that a certificate becomes available after the cert store is created and before the certificates are retrieved from the cert store. But that chance always exists. We can at most try to make the window as small as possible. My expectation would be that it is already rather small with the current implementation. Is there reason to expect otherwise? On the other hand, to retrieve the cert list in GetClientCerts will end up in more callback complexity again, that we should avoid if there's no clear benefit.
https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos.h (right): https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos.h:40: const CertificateList& additional_certs, On 2015/08/10 12:09:56, pneubeck wrote: > On 2015/08/07 23:51:09, Ryan Sleevi wrote: > > Are you sure this is the right pattern? I don't believe it is. > > > > It forces your ClientCertStoreChromeOS to know all of the certificates at load > > time, and prevents you from ever changing them. That certainly doesn't match > the > > NSS guarantees, nor necessarily that of smart cards (e.g. where authenticating > > to a hostile card may reveal additional certificates). Are you sure it's what > > you want? > > Yeah, I thought about that point as well. > As far as I understand it, the (only) call chain is > URLRequestHttpJob::OnStartCompleted -> > URLRequestJob::NotifyCertificateRequested -> > URLRequest::NotifyCertificateRequested -> > ResourceLoader::OnCertificateRequested -> > ResourceContext::CreateClientCertStore > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > ) > > which is executed everytime for the server requesting a client cert. > Of course, there's the chance that a certificate becomes available after the > cert store is created and before the certificates are retrieved from the cert > store. But that chance always exists. We can at most try to make the window as > small as possible. > My expectation would be that it is already rather small with the current > implementation. > Is there reason to expect otherwise? > > On the other hand, to retrieve the cert list in GetClientCerts will end up in > more callback complexity again, that we should avoid if there's no clear > benefit. Honestly, this whole ClientCertStore thing is pretty weird. :-) It shouldn't be in //net and it's really confusing that it's actually a single query, and not a handle to the actual store, as it should be. Fixing all that right now is probably not reasonable. That said, there is still the root issue of whether the certificate list is synchronously available. This leaks all the way up to the extension API itself, so we probably need to get that right right now. Should the API be certificateProvider.pubishClientCertificates or should the certificate provider provide an asynchronous API to query the smartcard as-needed? Certainly the latter matches how smartcards typically work. The former would require that loading the driver query the smartcard on startup and that Chrome maintain a list in-memory to mirror that list. And perhaps the smartcard doesn't have any way to get notified when a new certificate is installed (Ryan, is that a capability that smartcards typically have?) and would have to poll just in case we add a new one. This seems poor. If we do go with an asynchronous API in the certificate provider, I would propose there be a CertFilter::GetAdditionalCertificates that returns a CertificateList asynchronously. Or perhaps we go ahead and move ClientCertStoreChromeOS into //chrome now---really ClientCertStore shouldn't be in //net at all---and then you can avoid this ClientCertStore / CertFilter split. https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:945: net::CertificateList(), // no additional certificates Nit: I think this is two spaces?
On 2015/08/10 21:54:00, David Benjamin wrote: > https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... > File net/ssl/client_cert_store_chromeos.h (right): > > https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... > net/ssl/client_cert_store_chromeos.h:40: const CertificateList& > additional_certs, > On 2015/08/10 12:09:56, pneubeck wrote: > > On 2015/08/07 23:51:09, Ryan Sleevi wrote: > > > Are you sure this is the right pattern? I don't believe it is. > > > > > > It forces your ClientCertStoreChromeOS to know all of the certificates at > load > > > time, and prevents you from ever changing them. That certainly doesn't match > > the > > > NSS guarantees, nor necessarily that of smart cards (e.g. where > authenticating > > > to a hostile card may reveal additional certificates). Are you sure it's > what > > > you want? > > > > Yeah, I thought about that point as well. > > As far as I understand it, the (only) call chain is > > URLRequestHttpJob::OnStartCompleted -> > > URLRequestJob::NotifyCertificateRequested -> > > URLRequest::NotifyCertificateRequested -> > > ResourceLoader::OnCertificateRequested -> > > ResourceContext::CreateClientCertStore > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > ) > > > > which is executed everytime for the server requesting a client cert. > > Of course, there's the chance that a certificate becomes available after the > > cert store is created and before the certificates are retrieved from the cert > > store. But that chance always exists. We can at most try to make the window as > > small as possible. > > My expectation would be that it is already rather small with the current > > implementation. > > Is there reason to expect otherwise? > > > > On the other hand, to retrieve the cert list in GetClientCerts will end up in > > more callback complexity again, that we should avoid if there's no clear > > benefit. > > Honestly, this whole ClientCertStore thing is pretty weird. :-) It shouldn't be > in //net and it's really confusing that it's actually a single query, and not a > handle to the actual store, as it should be. > > Fixing all that right now is probably not reasonable. That said, there is still > the root issue of whether the certificate list is synchronously available. This > leaks all the way up to the extension API itself, so we probably need to get > that right right now. > > Should the API be certificateProvider.pubishClientCertificates or should the > certificate provider provide an asynchronous API to query the smartcard > as-needed? A polling API for sure has more implications. E.g. we would leak information about which https pages with client auth the user visited. Looking at the PKCS#11 interface, I still don't see why enumerating all certificates using C_FindObjects wouldn't work. If enumerating is protected by a PIN, then we'll have to ask the user for a PIN immediately after inserting the smart card. In that case, polling will not help much either: The user will instead have to enter the PIN on the first TLS client auth, no matter whether the smart card contains a matching cert or not. > Certainly the latter matches how smartcards typically work. The > former would require that loading the driver query the smartcard on startup and > that Chrome maintain a list in-memory to mirror that list. And perhaps the > smartcard doesn't have any way to get notified when a new certificate is > installed (Ryan, is that a capability that smartcards typically have?) and would > have to poll just in case we add a new one. This seems poor. We don't care about modification of the smart card. The only change of the smart card's state that we have to expect are session changes after PIN entries. As the PIN entry is known to the extension, it can publish again all certificates (if that list changed at all), AFAIU. > If we do go with an asynchronous API in the certificate provider, I would > propose there be a CertFilter::GetAdditionalCertificates that returns a > CertificateList asynchronously. For sure, passing in a CertificateList is the simpler interface than passing in an asynchronous getter. Without benefit, we should avoid that. Please explain what benefit you see by using an asynchronous getter. > Or perhaps we go ahead and move > ClientCertStoreChromeOS into //chrome now---really ClientCertStore shouldn't be > in //net at all---and then you can avoid this ClientCertStore / CertFilter > split. Independently of the support for additional certs, sure, I can move that class and merge it with CertFilter. > > https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... > File chrome/browser/profiles/profile_io_data.cc (right): > > https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... > chrome/browser/profiles/profile_io_data.cc:945: net::CertificateList(), // no > additional certificates > Nit: I think this is two spaces?
Another argument against polling: It's not feasible to poll the smart card itself per certificate request, as smart cards are terribly slow (I was told consistently). To avoid extreme delays, somewhere in the stack the certificates should be cached: either in the driver or in Chrome. So, having the driver publish the certs doesn't add new assumptions/restrictions than what is practically required already. On 2015/08/11 09:36:23, pneubeck wrote: > On 2015/08/10 21:54:00, David Benjamin wrote: > > > https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... > > File net/ssl/client_cert_store_chromeos.h (right): > > > > > https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... > > net/ssl/client_cert_store_chromeos.h:40: const CertificateList& > > additional_certs, > > On 2015/08/10 12:09:56, pneubeck wrote: > > > On 2015/08/07 23:51:09, Ryan Sleevi wrote: > > > > Are you sure this is the right pattern? I don't believe it is. > > > > > > > > It forces your ClientCertStoreChromeOS to know all of the certificates at > > load > > > > time, and prevents you from ever changing them. That certainly doesn't > match > > > the > > > > NSS guarantees, nor necessarily that of smart cards (e.g. where > > authenticating > > > > to a hostile card may reveal additional certificates). Are you sure it's > > what > > > > you want? > > > > > > Yeah, I thought about that point as well. > > > As far as I understand it, the (only) call chain is > > > URLRequestHttpJob::OnStartCompleted -> > > > URLRequestJob::NotifyCertificateRequested -> > > > URLRequest::NotifyCertificateRequested -> > > > ResourceLoader::OnCertificateRequested -> > > > ResourceContext::CreateClientCertStore > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > > ) > > > > > > which is executed everytime for the server requesting a client cert. > > > Of course, there's the chance that a certificate becomes available after the > > > cert store is created and before the certificates are retrieved from the > cert > > > store. But that chance always exists. We can at most try to make the window > as > > > small as possible. > > > My expectation would be that it is already rather small with the current > > > implementation. > > > Is there reason to expect otherwise? > > > > > > On the other hand, to retrieve the cert list in GetClientCerts will end up > in > > > more callback complexity again, that we should avoid if there's no clear > > > benefit. > > > > Honestly, this whole ClientCertStore thing is pretty weird. :-) It shouldn't > be > > in //net and it's really confusing that it's actually a single query, and not > a > > handle to the actual store, as it should be. > > > > Fixing all that right now is probably not reasonable. That said, there is > still > > the root issue of whether the certificate list is synchronously available. > This > > leaks all the way up to the extension API itself, so we probably need to get > > that right right now. > > > > Should the API be certificateProvider.pubishClientCertificates or should the > > certificate provider provide an asynchronous API to query the smartcard > > as-needed? > A polling API for sure has more implications. E.g. we would leak information > about which https pages with client auth the user visited. > Looking at the PKCS#11 interface, I still don't see why enumerating all > certificates using C_FindObjects wouldn't work. > If enumerating is protected by a PIN, then we'll have to ask the user for a PIN > immediately after inserting the smart card. > In that case, polling will not help much either: The user will instead have to > enter the PIN on the first TLS client auth, no matter whether the smart card > contains a matching cert or not. > > > Certainly the latter matches how smartcards typically work. The > > former would require that loading the driver query the smartcard on startup > and > > that Chrome maintain a list in-memory to mirror that list. And perhaps the > > smartcard doesn't have any way to get notified when a new certificate is > > installed (Ryan, is that a capability that smartcards typically have?) and > would > > have to poll just in case we add a new one. This seems poor. > We don't care about modification of the smart card. > The only change of the smart card's state that we have to expect are session > changes after PIN entries. > As the PIN entry is known to the extension, it can publish again all > certificates (if that list changed at all), AFAIU. > > > If we do go with an asynchronous API in the certificate provider, I would > > propose there be a CertFilter::GetAdditionalCertificates that returns a > > CertificateList asynchronously. > For sure, passing in a CertificateList is the simpler interface than passing in > an asynchronous getter. > Without benefit, we should avoid that. > Please explain what benefit you see by using an asynchronous getter. > > > Or perhaps we go ahead and move > > ClientCertStoreChromeOS into //chrome now---really ClientCertStore shouldn't > be > > in //net at all---and then you can avoid this ClientCertStore / CertFilter > > split. > Independently of the support for additional certs, sure, I can move that class > and merge it with CertFilter. > > > > > > https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... > > File chrome/browser/profiles/profile_io_data.cc (right): > > > > > https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... > > chrome/browser/profiles/profile_io_data.cc:945: net::CertificateList(), // no > > additional certificates > > Nit: I think this is two spaces?
On 2015/08/11 10:02:58, pneubeck wrote: > Another argument against polling: > It's not feasible to poll the smart card itself per certificate request, as > smart cards are terribly slow (I was told consistently). > To avoid extreme delays, somewhere in the stack the certificates should be > cached: either in the driver or in Chrome. > So, having the driver publish the certs doesn't add new assumptions/restrictions > than what is practically required already. Yes, but I think if caching is to be implemented, it should be as close to the data as possible. I don't know why you said that if the user inserts a hostile smart card, we'd need to immediately prompt for the PIN. We don't do that on Linux platforms - we do defer until we're actually querying the card and we see it is hostile (we used to require the user to actively go to the certificate browser to view and then get the hostile-pin, since the cert browser had the effect of enumerating all client certs). I definitely think a PULL-based API, rather than PUSH, is the best long-term solution, and matches the other APIs (whether C_FindObjects, MSFT's CSP/KSPs, or Apple's TokenD) that developers are familiar with. The PULL may return a cache, but that things are cached isn't a concern of the //net layer - that's deferred to the concrete classes. As for CreateClientCertStore being called, yeah, that was supposed to be cleaned up and fully hosted into the //content embedder as a single object, but alas, that team disbanded and the cleanup work never got done. Now you see why I have trouble trusting people to clean things up after the fact ;)
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
ptal https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... File net/ssl/client_cert_store_chromeos.h (right): https://codereview.chromium.org/1274143002/diff/40001/net/ssl/client_cert_sto... net/ssl/client_cert_store_chromeos.h:40: const CertificateList& additional_certs, On 2015/08/10 21:54:00, David Benjamin wrote: > On 2015/08/10 12:09:56, pneubeck wrote: > > On 2015/08/07 23:51:09, Ryan Sleevi wrote: > > > Are you sure this is the right pattern? I don't believe it is. > > > > > > It forces your ClientCertStoreChromeOS to know all of the certificates at > load > > > time, and prevents you from ever changing them. That certainly doesn't match > > the > > > NSS guarantees, nor necessarily that of smart cards (e.g. where > authenticating > > > to a hostile card may reveal additional certificates). Are you sure it's > what > > > you want? > > > > Yeah, I thought about that point as well. > > As far as I understand it, the (only) call chain is > > URLRequestHttpJob::OnStartCompleted -> > > URLRequestJob::NotifyCertificateRequested -> > > URLRequest::NotifyCertificateRequested -> > > ResourceLoader::OnCertificateRequested -> > > ResourceContext::CreateClientCertStore > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > ) > > > > which is executed everytime for the server requesting a client cert. > > Of course, there's the chance that a certificate becomes available after the > > cert store is created and before the certificates are retrieved from the cert > > store. But that chance always exists. We can at most try to make the window as > > small as possible. > > My expectation would be that it is already rather small with the current > > implementation. > > Is there reason to expect otherwise? > > > > On the other hand, to retrieve the cert list in GetClientCerts will end up in > > more callback complexity again, that we should avoid if there's no clear > > benefit. > > Honestly, this whole ClientCertStore thing is pretty weird. :-) It shouldn't be > in //net and it's really confusing that it's actually a single query, and not a > handle to the actual store, as it should be. > > Fixing all that right now is probably not reasonable. That said, there is still > the root issue of whether the certificate list is synchronously available. This > leaks all the way up to the extension API itself, so we probably need to get > that right right now. > > Should the API be certificateProvider.pubishClientCertificates or should the > certificate provider provide an asynchronous API to query the smartcard > as-needed? Certainly the latter matches how smartcards typically work. The > former would require that loading the driver query the smartcard on startup and > that Chrome maintain a list in-memory to mirror that list. And perhaps the > smartcard doesn't have any way to get notified when a new certificate is > installed (Ryan, is that a capability that smartcards typically have?) and would > have to poll just in case we add a new one. This seems poor. > > If we do go with an asynchronous API in the certificate provider, I would > propose there be a CertFilter::GetAdditionalCertificates that returns a > CertificateList asynchronously. Or perhaps we go ahead and move > ClientCertStoreChromeOS into //chrome now---really ClientCertStore shouldn't be > in //net at all---and then you can avoid this ClientCertStore / CertFilter > split. Done. https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1274143002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:945: net::CertificateList(), // no additional certificates On 2015/08/10 21:54:00, David Benjamin wrote: > Nit: I think this is two spaces? Done.
https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:104: additional_certs_ = certs; Hrm. Although we never actually call GetClientCerts more than once and this interface is a mess, it is sort of weird to implement it this way, making it stateful. (What if we have two GetClientCerts going on in parallel?) Actually, why not do this: 1. GetClientCertsAfterInit takes a net::CertificateList parameter. 2. Use net::ClientCertStoreNSS::GetClientCertsImpl to filter that additional list. 3. Call net::ClientCertStoreNSS::GetClientCerts with a different callback which concatenates your lists together and calls the real callback. In fact, if you do this (or even as they are now), it starts to seem like this whole subclassing thing is kind of unnecessary to begin with. Instead you could: 1. Split net::ClientCertStoreNSS::GetClientCertsImpl into a random utility function you can just call. FilterCertificateList or something. 2. ClientCertStoreChromeOS implements ClientCertStore directly and contains a ClientCertStoreNSS. 3. ClientCertStoreChromeOS::GetClientCerts does: a. Look up certificates from ClientCertStoreNSS. -- async -- b. Call additional CertNotAllowedPredicate on that list. c. Look up certificates from CertificateProvider. -- async -- d. Call FilterCertificateList on that list. e. Call the callback with the concatenation of those two. Composition and all that good stuff. :-) https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right): https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:555: nullptr, // no additional certs "No additional provider." perhaps? https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:944: nullptr, // no additional certs "No additional provider." perhaps?
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
If you agree that the PasswordDelegate isn't required in ChromeOS, I can simplify the ClientCertStoreChromeOS further. https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:104: additional_certs_ = certs; On 2015/08/14 21:49:16, David Benjamin wrote: > Hrm. Although we never actually call GetClientCerts more than once and this > interface is a mess, it is sort of weird to implement it this way, making it > stateful. (What if we have two GetClientCerts going on in parallel?) > > Actually, why not do this: > > 1. GetClientCertsAfterInit takes a net::CertificateList parameter. > 2. Use net::ClientCertStoreNSS::GetClientCertsImpl to filter that additional > list. > 3. Call net::ClientCertStoreNSS::GetClientCerts with a different callback which > concatenates your lists together and calls the real callback. > > In fact, if you do this (or even as they are now), it starts to seem like this > whole subclassing thing is kind of unnecessary to begin with. Instead you could: > > 1. Split net::ClientCertStoreNSS::GetClientCertsImpl into a random utility > function you can just call. FilterCertificateList or something. > 2. ClientCertStoreChromeOS implements ClientCertStore directly and contains a > ClientCertStoreNSS. > 3. ClientCertStoreChromeOS::GetClientCerts does: > a. Look up certificates from ClientCertStoreNSS. > -- async -- > b. Call additional CertNotAllowedPredicate on that list. > c. Look up certificates from CertificateProvider. > -- async -- > d. Call FilterCertificateList on that list. > e. Call the callback with the concatenation of those two. > > Composition and all that good stuff. :-) Not as easy because a lot of the NSS stuff was executed on the worker pool. What everyone overlooked so far, is that the complete list of matching certs must be sorted. I fixed that in the last patch set. I went ahead an broke the ClientCertStoreNSS into static functions and removed any inheritance. That should make this class much simpler now. There's some duplication now to post to the worker pool. But IMO that's acceptable and avoiding this duplication will mean more callbacks/virtuals... https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right): https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:555: nullptr, // no additional certs On 2015/08/14 21:49:16, David Benjamin wrote: > "No additional provider." perhaps? Done. https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1274143002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:944: nullptr, // no additional certs On 2015/08/14 21:49:16, David Benjamin wrote: > "No additional provider." perhaps? Done.
lgtm. Sorry about the delay, and thanks for the cleanup. That was somewhat easier to follow I think. (The whole mess with const SSLCertRequestInfo* vs & and where the CertificateList is secretly a field on the SSLCertRequestInfo is all kinds of absurd, but it's an existing issue and you're not making it any worse, so meh. :-) ) https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider.h (right): https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider.h:15: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; Per https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket..., don't forward-declare this type. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:34: const ClientCertStoreChromeOS::CertFilter& filter_; Nit: I'd probably have made this a pointer I think? I don't know if we save const references into structures much. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:109: << pre_size << " platform certs"; Optional: Has this and the corresponding NSS DVLOG ever been useful? Seems we may as well not bother with either I think. I dunno, the only time I've ever debugged an issue with existing logging is when we don't preserve all details of some error (notably Windows error codes when some CNG or CAPI operation fails). https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/client_cert_store_chromeos.h (right): https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.h:18: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; Ditto. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.h:50: const net::ClientCertStoreNSS::PasswordDelegateFactory& Optional: If you want to keep the type inside ClientCertStoreChromeOS so the consumers don't have to change, I think a using PasswordDelegateFactory = net::ClientCertStoreNSS::PasswordDelegateFactory; would also be fine. I suppose doing that somewhat abstracts net::ClientCertStoreNSS from the consumer, which isn't a bad idea? I dunno. I don't really have preferences, so your call. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.h:50: const net::ClientCertStoreNSS::PasswordDelegateFactory& > If you agree that the PasswordDelegate isn't required in ChromeOS, I can > simplify the ClientCertStoreChromeOS further ProfileIOData seems to supply one. Whether it's actually possible for it to do anything, I'm not sure. I would think not since you guys don't support random PKCS#11 modules, but I'm not confident in my understanding of any of that. Ryan might know more definitively. https://codereview.chromium.org/1274143002/diff/220001/net/ssl/client_cert_st... File net/ssl/client_cert_store_nss.h (right): https://codereview.chromium.org/1274143002/diff/220001/net/ssl/client_cert_st... net/ssl/client_cert_store_nss.h:51: static void GetPlatformCerts( Nit: A little odd that one is called BlahOnWorkerThread and the other isn't. Probably should be consistently one or the other.
davidben@chromium.org changed reviewers: + mattm@chromium.org
Actually, I should probably +mattm who is (a) arguably a better reviewer for this particular CL and (b) I *badly* need to load-balance my reviews. Matt, do you mind looking at this and at least giving an initial pass over the subsequent ones? I kind of need to declare code review bankruptcy again and focus tomorrow on fixing this bug assigned to me. :-)
https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider.h (right): https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider.h:15: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; On 2015/08/20 00:40:34, David Benjamin (slow) wrote: > Per > https://codereview.chromium.org/994743003/diff/1/net/socket/ssl_server_socket..., > don't forward-declare this type. Done. I think we should fix all such existing forward declarations, as they give the wrong impression to new developers that this is the correct way of doing it. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:34: const ClientCertStoreChromeOS::CertFilter& filter_; On 2015/08/20 00:40:34, David Benjamin (slow) wrote: > Nit: I'd probably have made this a pointer I think? I don't know if we save > const references into structures much. Done. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:109: << pre_size << " platform certs"; On 2015/08/20 00:40:34, David Benjamin (slow) wrote: > Optional: Has this and the corresponding NSS DVLOG ever been useful? Seems we > may as well not bother with either I think. I dunno, the only time I've ever > debugged an issue with existing logging is when we don't preserve all details of > some error (notably Windows error codes when some CNG or CAPI operation fails). Removed. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/client_cert_store_chromeos.h (right): https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.h:18: typedef std::vector<scoped_refptr<X509Certificate>> CertificateList; On 2015/08/20 00:40:34, David Benjamin (slow) wrote: > Ditto. Done. https://codereview.chromium.org/1274143002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.h:50: const net::ClientCertStoreNSS::PasswordDelegateFactory& On 2015/08/20 00:40:34, David Benjamin (slow) wrote: > Optional: If you want to keep the type inside ClientCertStoreChromeOS so the > consumers don't have to change, I think a > > using PasswordDelegateFactory = > net::ClientCertStoreNSS::PasswordDelegateFactory; > > would also be fine. I suppose doing that somewhat abstracts > net::ClientCertStoreNSS from the consumer, which isn't a bad idea? I dunno. I > don't really have preferences, so your call. I'm rather sure that we can remove it, but let's defer the removal until Ryan is back. In general there's always the totally unknown of another project using chromeos=1 builds for... something and they will not run the binary in a chromeos environment. https://codereview.chromium.org/1274143002/diff/220001/net/ssl/client_cert_st... File net/ssl/client_cert_store_nss.h (right): https://codereview.chromium.org/1274143002/diff/220001/net/ssl/client_cert_st... net/ssl/client_cert_store_nss.h:51: static void GetPlatformCerts( On 2015/08/20 00:40:34, David Benjamin (slow) wrote: > Nit: A little odd that one is called BlahOnWorkerThread and the other isn't. > Probably should be consistently one or the other. Done. also added a comment to each.
sorry for delay, lgtm!
pneubeck@chromium.org changed reviewers: + mmenke@chromium.org, stevenjb@chromium.org
Steven: ptal at the new OWNERShip. mmenke@: ptal at profile_io_data.cc
On 2015/08/22 08:30:03, pneubeck wrote: > Steven: ptal at the new OWNERShip. > mmenke@: ptal at profile_io_data.cc profile_io_data LGTM
https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:117: #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h" If you're just passing in a NULL certificate provider here, this isn't actually needed.
https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:117: #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h" On 2015/08/22 17:21:54, mmenke wrote: > If you're just passing in a NULL certificate provider here, this isn't actually > needed. The argument is a scoped_ptr<T>, that's why the full definition of T was required.
The CQ bit was checked by pneubeck@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1274143002/#ps260001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274143002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274143002/260001
The CQ bit was unchecked by pneubeck@chromium.org
owner lgtm https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/certificate_provider/certificate_provider.h (right): https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/certificate_provider/certificate_provider.h:18: CertificateProvider() {} nit: not needed? https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right): https://codereview.chromium.org/1274143002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/net/client_cert_store_chromeos.cc:86: callback, true)) { nit: I think this would be a little more clear if the if were inverted and an early exit was used. A comment would also be nice. https://codereview.chromium.org/1274143002/diff/260001/net/ssl/client_cert_st... File net/ssl/client_cert_store_nss.cc (right): https://codereview.chromium.org/1274143002/diff/260001/net/ssl/client_cert_st... net/ssl/client_cert_store_nss.cc:44: callback, true)) { nit: I liked the early exit better.
Patchset #7 (id:280001) has been deleted
The CQ bit was checked by pneubeck@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org, stevenjb@chromium.org, mmenke@chromium.org, mattm@chromium.org Link to the patchset: https://codereview.chromium.org/1274143002/#ps300001 (title: "Addressed Steven's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274143002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274143002/300001
Message was sent while issue was closed.
Committed patchset #7 (id:300001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/385704ecd579795bccfc03079191256700000daa Cr-Commit-Position: refs/heads/master@{#345288} |
