Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1278763002:
Add a ClientKeyStore to allow injection of non-platform keys for TLS client auth. (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@client_cert_store Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a ClientKeyStore to allow injection of non-platform keys for TLS client auth.
BUG=514575
Committed: https://crrev.com/abe6e9d132812e9d2d1065650cf09a724cb53a6d
Cr-Commit-Position: refs/heads/master@{#345565}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Apply suggested redesign #Patch Set 3 : Rebased, renamed the provider interface. #
Total comments: 6
Patch Set 4 : Removed ownership of Providers. #
Total comments: 8
Patch Set 5 : Addressed comments. #Patch Set 6 : Nits. #
Messages
Total messages: 50 (21 generated)
Patchset #1 (id:1) has been deleted
pneubeck@chromium.org changed reviewers: + davidben@chromium.org
another part of separated from the major CL. ptal.
Patchset #1 (id:20001) has been deleted
nicolebilbao29@gmail.com changed reviewers: + nicolebilbao29@gmail.com
The CQ bit was checked by nicolebilbao29@gmail.com
lgtm
The CQ bit was checked by nicolebilbao29@gmail.com
lgtm lgtm
The CQ bit was checked by nicolebilbao29@gmail.com
lgtm lgtm
The CQ bit was checked by nicolebilbao29@gmail.com
lgtm lgtm
The CQ bit was checked by nicolebilbao29@gmail.com
lgtm lgtm
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1274143002 Patch 40001). Please resolve the dependency and try again.
davidben@chromium.org changed reviewers: - nicolebilbao29@gmail.com
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
from a design, the whole ProviderHandle system seems unnecessarily complex/complicated. why not simply // TODO(rsleevi): Remove this once https://crbug.com/blah is fixed. [Where /blah is whatever bug David filed about aligning things on ClientCertStore and returning the private key there] class NET_EXPORT SSLClientKeyStore { public: class SSLClientCertProvider { public: virtual ~SSLClientCertProvider () {} // Obtains a handle to the certificate private key for |cert| and stores it in |private_key|. // If the SSLClientCertProvider does not know about the |cert|, returns false. // If it knows about the certificate, but is unable to return // the private key, returns true and sets |*private_key| // to nullptr. virtual bool GetCertificateKey(const X509Certificate& cert, scoped_ptr<SSLPrivateKey>* private_key) = 0; }; static SSLClientKeyStore* GetInstance(); void AddProvider(scoped_ptr<SSLClientCertProvider> provider); void RemoveProvider(SSLClientCertProvider* provider); scoped_ptr<SSLPrivateKey> FetchClientCertPrivateKey(const X509Certificate& cert); private: friend struct DefaultLazyInstanceTraits<SSLClientKeyStore>; SSLClientKeyStore(); ~SSLClientKeyStore(); ScopedVector<SSLClientCertProvider> providers_; }; Note how in the above, I removed the mutex. //net knowing anything about threading is a general anti-pattern. Also, FetchClientCertPrivateKey() is always called on the IO thread for //net, and so should it be. Finally, an example provider for you would be class YourProvider : public SSLClientCertProvider { public: ~YourProvider() override {} bool GetCertificateKey(const X509Certificate& cert, scoped_ptr<SSLPrivateKey>* private_key) { auto it = my_keys_.find(X509Certificate::CalculateFingerprint256(cert.os_cert_handle())); if (it == my_keys_.end()) return false; *private_key = it->second.Run().Pass(); return true; } void SetCertificates(...); private: std::map<SHA256HashValue, base::Callback<scoped_ptr<SSLPrivateKey>(void)>, SHA256HashValueLessThan> my_keys_; }; The goal here is single responsibility. SSLClientKeyStore just aggregates a bunch of Providers into a common interface. Each Provider is responsible for figuring out how to return the Key. You get rid of the threading implications, and ownership of the Provider is passed to the SSLClientKeyStore. All of this goes away with the ClientCertStore cleanup, but at least this makes it clearer where the separations lie. https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... File net/ssl/client_key_store.cc (right): https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.cc:51: if (certificate->Equals(cert_and_key.certificate.get())) This also seems quite inefficient; a linear scan. Why not store these as a map of (SHA256HashOfCertificate -> Private Key)? https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_store.h File net/ssl/client_key_store.h (right): https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:36: }; Why is this necessary? Why do you return it as ProviderHandle rather than int / with a typedef? https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:38: using KeyGetter = base::Callback<scoped_ptr<SSLPrivateKey>(void)>; This seems unnecessarily opaque. It does not seem to add value as a typedef. https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:40: struct CertAndKey { Why is this a struct instead of just a std::pair or std::tuple? https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:51: using CertsAndKeys = std::vector<CertAndKey>; This seems unnecessarily opaque. It does not seem to add value as a typedef. https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:80: base::Lock lock_; Presumably this lock protects all these members; historically, we put it first, not last.
nicolebilbao29@gmail.com changed reviewers: + nicolebilbao29@gmail.com
rsleevi@chromium.org changed reviewers: - nicolebilbao29@gmail.com
On 2015/08/08 00:14:22, Ryan Sleevi wrote: > from a design, the whole ProviderHandle system seems unnecessarily > complex/complicated. > > why not simply See below. > > // TODO(rsleevi): Remove this once https://crbug.com/blah is fixed. > [Where /blah is whatever bug David filed about aligning things on > ClientCertStore and returning the private key there] > > class NET_EXPORT SSLClientKeyStore { > public: > class SSLClientCertProvider { > public: > virtual ~SSLClientCertProvider () {} > > // Obtains a handle to the certificate private key for |cert| and stores it > in |private_key|. > // If the SSLClientCertProvider does not know about the |cert|, returns > false. > // If it knows about the certificate, but is unable to return > // the private key, returns true and sets |*private_key| > // to nullptr. > virtual bool GetCertificateKey(const X509Certificate& cert, > scoped_ptr<SSLPrivateKey>* private_key) = 0; > }; > > static SSLClientKeyStore* GetInstance(); > > void AddProvider(scoped_ptr<SSLClientCertProvider> provider); > void RemoveProvider(SSLClientCertProvider* provider); > > scoped_ptr<SSLPrivateKey> FetchClientCertPrivateKey(const X509Certificate& > cert); > > private: > friend struct DefaultLazyInstanceTraits<SSLClientKeyStore>; > > SSLClientKeyStore(); > ~SSLClientKeyStore(); > > ScopedVector<SSLClientCertProvider> providers_; > }; > > > Note how in the above, I removed the mutex. //net knowing anything about > threading is a general anti-pattern. Also, FetchClientCertPrivateKey() is always > called on the IO thread for //net, and so should it be. That deviates from what David told me. He explicitly stated that I should prepare for FetchClientCertPrivateKey() to be called on any thread (which only happens to be the IO thread in most cases). That was also the reason for the proposed: isolate the multi threaded portion in the KeyStore and avoid having also multi threaded objects outside the KeyStore. Seems that you disagree with David on this point. > Finally, an example provider for you would be > > class YourProvider : public SSLClientCertProvider { > public: > ~YourProvider() override {} > bool GetCertificateKey(const X509Certificate& cert, scoped_ptr<SSLPrivateKey>* > private_key) { > auto it = > my_keys_.find(X509Certificate::CalculateFingerprint256(cert.os_cert_handle())); > if (it == my_keys_.end()) > return false; > *private_key = it->second.Run().Pass(); > return true; > } > > void SetCertificates(...); > > private: > std::map<SHA256HashValue, base::Callback<scoped_ptr<SSLPrivateKey>(void)>, > SHA256HashValueLessThan> my_keys_; > }; > > > The goal here is single responsibility. SSLClientKeyStore just aggregates a > bunch of Providers into a common interface. Each Provider is responsible for > figuring out how to return the Key. You get rid of the threading implications, > and ownership of the Provider is passed to the SSLClientKeyStore. > > All of this goes away with the ClientCertStore cleanup, but at least this makes > it clearer where the separations lie. > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... > File net/ssl/client_key_store.cc (right): > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... > net/ssl/client_key_store.cc:51: if > (certificate->Equals(cert_and_key.certificate.get())) > This also seems quite inefficient; a linear scan. > > Why not store these as a map of (SHA256HashOfCertificate -> Private Key)? > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_store.h > File net/ssl/client_key_store.h (right): > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... > net/ssl/client_key_store.h:36: }; > Why is this necessary? Why do you return it as ProviderHandle rather than int / > with a typedef? > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... > net/ssl/client_key_store.h:38: using KeyGetter = > base::Callback<scoped_ptr<SSLPrivateKey>(void)>; > This seems unnecessarily opaque. It does not seem to add value as a typedef. > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... > net/ssl/client_key_store.h:40: struct CertAndKey { > Why is this a struct instead of just a std::pair or std::tuple? > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... > net/ssl/client_key_store.h:51: using CertsAndKeys = std::vector<CertAndKey>; > This seems unnecessarily opaque. It does not seem to add value as a typedef. > > https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... > net/ssl/client_key_store.h:80: base::Lock lock_; > Presumably this lock protects all these members; historically, we put it first, > not last.
On 2015/08/10 12:46:11, pneubeck wrote: > On 2015/08/08 00:14:22, Ryan Sleevi wrote: > > from a design, the whole ProviderHandle system seems unnecessarily > > complex/complicated. > > > > why not simply > See below. > > > > > // TODO(rsleevi): Remove this once https://crbug.com/blah is fixed. > > [Where /blah is whatever bug David filed about aligning things on > > ClientCertStore and returning the private key there] > > > > class NET_EXPORT SSLClientKeyStore { > > public: > > class SSLClientCertProvider { > > public: > > virtual ~SSLClientCertProvider () {} > > > > // Obtains a handle to the certificate private key for |cert| and stores > it > > in |private_key|. > > // If the SSLClientCertProvider does not know about the |cert|, returns > > false. > > // If it knows about the certificate, but is unable to return > > // the private key, returns true and sets |*private_key| > > // to nullptr. > > virtual bool GetCertificateKey(const X509Certificate& cert, > > scoped_ptr<SSLPrivateKey>* private_key) = 0; > > }; > > > > static SSLClientKeyStore* GetInstance(); > > > > void AddProvider(scoped_ptr<SSLClientCertProvider> provider); > > void RemoveProvider(SSLClientCertProvider* provider); > > > > scoped_ptr<SSLPrivateKey> FetchClientCertPrivateKey(const X509Certificate& > > cert); > > > > private: > > friend struct DefaultLazyInstanceTraits<SSLClientKeyStore>; > > > > SSLClientKeyStore(); > > ~SSLClientKeyStore(); > > > > ScopedVector<SSLClientCertProvider> providers_; > > }; > > > > > > Note how in the above, I removed the mutex. //net knowing anything about > > threading is a general anti-pattern. Also, FetchClientCertPrivateKey() is > always > > called on the IO thread for //net, and so should it be. > That deviates from what David told me. He explicitly stated that I should > prepare for FetchClientCertPrivateKey() to be called on any thread (which only > happens to be the IO thread in most cases). > That was also the reason for the proposed: isolate the multi threaded portion in > the KeyStore and avoid having also multi threaded objects outside the KeyStore. > > Seems that you disagree with David on this point. While //net shouldn't know about threading, I think we'd concluded before that //net should allow multiple URLRequestContexts to live on different threads provided each context's objects are only accessed on that thread. See https://crbug.com/476713. Post SSLPrivateKey refactor, all this locking will be gone but, right now, any state associated with FetchClientCertPrivateKey is a singleton and so I think needs to be locked.
(Got to reading more of the CL.) On 2015/08/08 00:14:22, Ryan Sleevi wrote: > from a design, the whole ProviderHandle system seems unnecessarily > complex/complicated. > > why not simply > > // TODO(rsleevi): Remove this once https://crbug.com/blah is fixed. > [Where /blah is whatever bug David filed about aligning things on > ClientCertStore and returning the private key there] Er, I wasn't planning on using ClientCertStore. ClientCertStore is kind of a mess and doesn't work for Android. (Or even this; ClientCertStore happens before certificates are selected.) I was thinking that, after the certificate selection dialog returns and we know which certificate was chosen, then look up the private key handle and pass the two down the stack together. That meshes well with Android where the certificate selection dialog just gives you the pair. > class NET_EXPORT SSLClientKeyStore { > public: > class SSLClientCertProvider { > public: > virtual ~SSLClientCertProvider () {} > > // Obtains a handle to the certificate private key for |cert| and stores it > in |private_key|. > // If the SSLClientCertProvider does not know about the |cert|, returns > false. > // If it knows about the certificate, but is unable to return > // the private key, returns true and sets |*private_key| > // to nullptr. > virtual bool GetCertificateKey(const X509Certificate& cert, > scoped_ptr<SSLPrivateKey>* private_key) = 0; > }; > > static SSLClientKeyStore* GetInstance(); > > void AddProvider(scoped_ptr<SSLClientCertProvider> provider); > void RemoveProvider(SSLClientCertProvider* provider); > > scoped_ptr<SSLPrivateKey> FetchClientCertPrivateKey(const X509Certificate& > cert); > > private: > friend struct DefaultLazyInstanceTraits<SSLClientKeyStore>; > > SSLClientKeyStore(); > ~SSLClientKeyStore(); > > ScopedVector<SSLClientCertProvider> providers_; > }; > > > Note how in the above, I removed the mutex. //net knowing anything about > threading is a general anti-pattern. Also, FetchClientCertPrivateKey() is always > called on the IO thread for //net, and so should it be. > > Finally, an example provider for you would be > > class YourProvider : public SSLClientCertProvider { > public: > ~YourProvider() override {} > bool GetCertificateKey(const X509Certificate& cert, scoped_ptr<SSLPrivateKey>* > private_key) { > auto it = > my_keys_.find(X509Certificate::CalculateFingerprint256(cert.os_cert_handle())); > if (it == my_keys_.end()) > return false; > *private_key = it->second.Run().Pass(); > return true; > } > > void SetCertificates(...); > > private: > std::map<SHA256HashValue, base::Callback<scoped_ptr<SSLPrivateKey>(void)>, > SHA256HashValueLessThan> my_keys_; > }; > > > The goal here is single responsibility. SSLClientKeyStore just aggregates a > bunch of Providers into a common interface. Each Provider is responsible for > figuring out how to return the Key. You get rid of the threading implications, > and ownership of the Provider is passed to the SSLClientKeyStore. The threading issue is still there because SSLClientKeyStore basically has to be a singleton today. We don't have any kind of context available at the SSLClientSocket layer to pass into FetchClientCertStore. (That is also a problem, and hopefully I can put together a design at some point. We should have a very different API I think to make it so that session cache and socket pool cross-pollination from different configurations are impossible. Anyway, I'll get to fleshing that one out later and it's not that related. :-) ) > All of this goes away with the ClientCertStore cleanup, but at least this makes > it clearer where the separations lie. I was actually thinking something different from either of these designs. Because we have to do a singleton, basically what we want is OpenSSLClientKeyStore from Android. Just OpenSSLClientKeyStore does not play well with SSLPrivateKey right now and such. But have some API somewhere to record the SSLPrivateKey and have Fetch look it back up again. Now, we have a slight nuisance that SSLPrivateKey::Copy doesn't exist yet. I would propose we either add it now and NOTIMPLEMENTED() it in the existing ones (since we'll need it later anyway due to SSLClientAuthCache) or we make it a map<CertHash, base::Callback<scoped_ptr<SSLPrivateKey>(void)>> rather than a map<CertHash, scoped_ptr<SSLPrivateKey>>. Yeah, this is a mess, but it's really all a hack around everything from SSLClientSocket to URLRequest having the wrong API. ContinueWithCertificate should be ContinueWithCertificate(X509Certificate*, scoped_ptr<SSLPrivateKey>), not ContinueWithCertificate(X509Certificate*). OpenSSLClientKeyStore on the Android port is really just a glorious hack around our inability to pass a second value along with the certificate. Hrm. Although this is actually going to blow up pretty horribly if two profiles share a certificate. Probably the map shouldn't be on the certificate value but actually the particular X509Certificate instance. If that's too ugly, I'd rather say we actually just wait for the SSLPrivateKey refactor to finish. There really isn't that much left to do and the person to be doing it just started today. We shouldn't make a further mess of ClientCertStore and embed it deeper into //net just to pass a random context in that we'll be ripping out later. The temporary scaffolding should be very targeted or we risk making things worse for future cleanups down the line.
On 2015/08/10 22:11:49, David Benjamin wrote: > Hrm. Although this is actually going to blow up pretty horribly if two profiles > share a certificate. Probably the map shouldn't be on the certificate value but > actually the particular X509Certificate instance. If that's too ugly, I'd rather > say we actually just wait for the SSLPrivateKey refactor to finish. There really > isn't that much left to do and the person to be doing it just started today. We > shouldn't make a further mess of ClientCertStore and embed it deeper into //net > just to pass a random context in that we'll be ripping out later. The temporary > scaffolding should be very targeted or we risk making things worse for future > cleanups down the line. Ryan points out to me that CrOS multiprofile is weird and this isn't very different from cross-profile contamination at the OS key store level, so this may not be that big of a deal. Doing it by hash is probably fine.
On 2015/08/10 22:11:49, David Benjamin wrote: > (Got to reading more of the CL.) > > On 2015/08/08 00:14:22, Ryan Sleevi wrote: > > from a design, the whole ProviderHandle system seems unnecessarily > > complex/complicated. > > > > why not simply > > > > // TODO(rsleevi): Remove this once https://crbug.com/blah is fixed. > > [Where /blah is whatever bug David filed about aligning things on > > ClientCertStore and returning the private key there] > > Er, I wasn't planning on using ClientCertStore. ClientCertStore is kind of a > mess and doesn't work for Android. (Or even this; ClientCertStore happens before > certificates are selected.) I was thinking that, after the certificate selection > dialog returns and we know which certificate was chosen, then look up the > private key handle and pass the two down the stack together. That meshes well > with Android where the certificate selection dialog just gives you the pair. > > > class NET_EXPORT SSLClientKeyStore { > > public: > > class SSLClientCertProvider { > > public: > > virtual ~SSLClientCertProvider () {} > > > > // Obtains a handle to the certificate private key for |cert| and stores > it > > in |private_key|. > > // If the SSLClientCertProvider does not know about the |cert|, returns > > false. > > // If it knows about the certificate, but is unable to return > > // the private key, returns true and sets |*private_key| > > // to nullptr. To support multithreaded usage, this function must be calleable from any thread. As it must return synchronously and internally has to manage a map/list of certs+keys that will be updated asynchronously, this will require a mutex. That's exactly what my proposal tried to avoid: spreading multithreaded code all over the place. Instead in the proposed patch, the only externally provided functions that are called from any thread, are these KeyGetter callbacks. These are trivially to implement though, as they don't have to modify any shared object. > > virtual bool GetCertificateKey(const X509Certificate& cert, > > scoped_ptr<SSLPrivateKey>* private_key) = 0; > > }; > > > > static SSLClientKeyStore* GetInstance(); > > > > void AddProvider(scoped_ptr<SSLClientCertProvider> provider); > > void RemoveProvider(SSLClientCertProvider* provider); > > > > scoped_ptr<SSLPrivateKey> FetchClientCertPrivateKey(const X509Certificate& > > cert); > > > > private: > > friend struct DefaultLazyInstanceTraits<SSLClientKeyStore>; > > > > SSLClientKeyStore(); > > ~SSLClientKeyStore(); > > To support multithreaded usage, this vector has to be protected by a mutex. > > ScopedVector<SSLClientCertProvider> providers_; > > }; > > > > > > Note how in the above, I removed the mutex. //net knowing anything about > > threading is a general anti-pattern. Also, FetchClientCertPrivateKey() is > always > > called on the IO thread for //net, and so should it be. > > > > Finally, an example provider for you would be > > > > class YourProvider : public SSLClientCertProvider { > > public: > > ~YourProvider() override {} > > bool GetCertificateKey(const X509Certificate& cert, > scoped_ptr<SSLPrivateKey>* > > private_key) { > > auto it = > > > my_keys_.find(X509Certificate::CalculateFingerprint256(cert.os_cert_handle())); > > if (it == my_keys_.end()) > > return false; > > *private_key = it->second.Run().Pass(); > > return true; > > } > > > > void SetCertificates(...); > > > > private: > > std::map<SHA256HashValue, base::Callback<scoped_ptr<SSLPrivateKey>(void)>, > > SHA256HashValueLessThan> my_keys_; > > }; > > > > > > The goal here is single responsibility. SSLClientKeyStore just aggregates a > > bunch of Providers into a common interface. Each Provider is responsible for > > figuring out how to return the Key. You get rid of the threading implications, > > and ownership of the Provider is passed to the SSLClientKeyStore. > > The threading issue is still there because SSLClientKeyStore basically has to be > a singleton today. We don't have any kind of context available at the > SSLClientSocket layer to pass into FetchClientCertStore. (That is also a > problem, and hopefully I can put together a design at some point. We should have > a very different API I think to make it so that session cache and socket pool > cross-pollination from different configurations are impossible. Anyway, I'll get > to fleshing that one out later and it's not that related. :-) ) > > > All of this goes away with the ClientCertStore cleanup, but at least this > makes > > it clearer where the separations lie. > > I was actually thinking something different from either of these designs. > Because we have to do a singleton, basically what we want is > OpenSSLClientKeyStore from Android. That's mostly what I did, with about two additions: The lock and a handle to allow removal of a group of keys, instead of Flush() which removes all keys. Grouping of keys (by ProviderHandle) allowed for simple implementation of: - Removing a group of previously set keys. Without groups the list of certs has to be stored outside as well. - Updating a group of previously set keys. Without groups first all old certs/keys have to be removed and afterwards the new added. > Just OpenSSLClientKeyStore does not play > well with SSLPrivateKey right now and such. But have some API somewhere to > record the SSLPrivateKey and have Fetch look it back up again. The recording is done using |void SetCertificates(ProviderHandle provider, CertsAndKeys* certs);| in the last patchset. > Now, we have a slight nuisance that SSLPrivateKey::Copy doesn't exist yet. I > would propose we either add it now and NOTIMPLEMENTED() it in the existing ones > (since we'll need it later anyway due to SSLClientAuthCache) or we make it a > map<CertHash, base::Callback<scoped_ptr<SSLPrivateKey>(void)>> rather than a > map<CertHash, scoped_ptr<SSLPrivateKey>>. The Callback manifested as the typedef KeyGetter in the last patchset. > > Yeah, this is a mess, but it's really all a hack around everything from > SSLClientSocket to URLRequest having the wrong API. ContinueWithCertificate > should be ContinueWithCertificate(X509Certificate*, scoped_ptr<SSLPrivateKey>), > not ContinueWithCertificate(X509Certificate*). OpenSSLClientKeyStore on the > Android port is really just a glorious hack around our inability to pass a > second value along with the certificate. > > > Hrm. Although this is actually going to blow up pretty horribly if two profiles > share a certificate. Probably the map shouldn't be on the certificate value but > actually the particular X509Certificate instance. If that's too ugly, I'd rather > say we actually just wait for the SSLPrivateKey refactor to finish. There really > isn't that much left to do and the person to be doing it just started today. We > shouldn't make a further mess of ClientCertStore and embed it deeper into //net > just to pass a random context in that we'll be ripping out later. The temporary > scaffolding should be very targeted or we risk making things worse for future > cleanups down the line.
Right, so the argument is the Singleton has to be thread-safe because people don't use SSL sockets consistently on the IO Thread (the great offenders are sync, gcm, and the extensions API). While in theory none of them should need/use/support client auth, and if they do, they should be doing so on the IO thread, I can buy the argument that that requirement is not firmly expressed in code (although it's already implied for !ChromeOS). While this code is 'temporary' (in the sense that the ideal flow is that both the cert and private key are given to the SSLSocket, rather than this lookup), I do find that as written, it violates the separation of responsibility and makes it harder, not easier, to reason about. Personally, I think it's fine to have the provider registry (aka ClientKeyStore) express thread-safety requirements, and the individual Providers express thread-safety requirements, and don't find the onus of having both classes be "safe from any thread" to be confusing (if anything, it seems to make it clearer, at least to me, where the separation of duties lie) I definitely see issue with the "Add Certificates" model, as expressed elsewhere, and definitely don't think it will end up supporting the smart card API well. To the extent that once we go down a particular API path, it will be far harder to unwind, I think it's perfectly reasonable to implement the lower-level API contract in terms of pull (aka "Give me certs"), and if any caching is to be implemented, it's to be deferred to the concrete Provider implementations. Anything else, and I think it gets weird, especially for the separation of responsibilities.
Patchset #2 (id:60001) has been deleted
On 2015/08/12 00:21:28, Ryan Sleevi wrote: > Right, so the argument is the Singleton has to be thread-safe because people > don't use SSL sockets consistently on the IO Thread (the great offenders are > sync, gcm, and the extensions API). > > While in theory none of them should need/use/support client auth, and if they > do, they should be doing so on the IO thread, I can buy the argument that that > requirement is not firmly expressed in code (although it's already implied for > !ChromeOS). > > While this code is 'temporary' (in the sense that the ideal flow is that both > the cert and private key are given to the SSLSocket, rather than this lookup), I > do find that as written, it violates the separation of responsibility and makes > it harder, not easier, to reason about. > > Personally, I think it's fine to have the provider registry (aka ClientKeyStore) > express thread-safety requirements, and the individual Providers express > thread-safety requirements, and don't find the onus of having both classes be > "safe from any thread" to be confusing (if anything, it seems to make it > clearer, at least to me, where the separation of duties lie) > > I definitely see issue with the "Add Certificates" model, as expressed > elsewhere, and definitely don't think it will end up supporting the smart card > API well. To the extent that once we go down a particular API path, it will be > far harder to unwind, I think it's perfectly reasonable to implement the > lower-level API contract in terms of pull (aka "Give me certs"), and if any > caching is to be implemented, it's to be deferred to the concrete Provider > implementations. Anything else, and I think it gets weird, especially for the > separation of responsibilities. (...while refactoring the current certificateProvider API from PUSH to PULL...) Note that as FetchClientCertPrivateKey (ssl_platform_key.h) must return a certificate synchronously and accordingly, in your proposal, Provider::GetCertificateKey must synchronously return whether a certificate is known to the provider or not, I must cache in somewhere in Chrome which certificates are available through the API. That cache can be limited to whatever matched previous SSL certificate requests though and doesn't have to contain all certificates of all smart cards.
On 2015/08/12 12:30:50, pneubeck wrote: > On 2015/08/12 00:21:28, Ryan Sleevi wrote: > > Right, so the argument is the Singleton has to be thread-safe because people > > don't use SSL sockets consistently on the IO Thread (the great offenders are > > sync, gcm, and the extensions API). > > > > While in theory none of them should need/use/support client auth, and if they > > do, they should be doing so on the IO thread, I can buy the argument that that > > requirement is not firmly expressed in code (although it's already implied for > > !ChromeOS). > > > > While this code is 'temporary' (in the sense that the ideal flow is that both > > the cert and private key are given to the SSLSocket, rather than this lookup), > I > > do find that as written, it violates the separation of responsibility and > makes > > it harder, not easier, to reason about. > > > > Personally, I think it's fine to have the provider registry (aka > ClientKeyStore) > > express thread-safety requirements, and the individual Providers express > > thread-safety requirements, and don't find the onus of having both classes be > > "safe from any thread" to be confusing (if anything, it seems to make it > > clearer, at least to me, where the separation of duties lie) > > > > I definitely see issue with the "Add Certificates" model, as expressed > > elsewhere, and definitely don't think it will end up supporting the smart card > > API well. To the extent that once we go down a particular API path, it will be > > far harder to unwind, I think it's perfectly reasonable to implement the > > lower-level API contract in terms of pull (aka "Give me certs"), and if any > > caching is to be implemented, it's to be deferred to the concrete Provider > > implementations. Anything else, and I think it gets weird, especially for the > > separation of responsibilities. > > (...while refactoring the current certificateProvider API from PUSH to PULL...) > Note that as FetchClientCertPrivateKey (ssl_platform_key.h) must return a > certificate synchronously and accordingly, in your proposal, > Provider::GetCertificateKey must synchronously return whether a certificate is > known to the provider or not, I must cache in somewhere in Chrome which > certificates are available through the API. > That cache can be limited to whatever matched previous SSL certificate requests > though and doesn't have to contain all certificates of all smart cards. Because of that required caching, I think, we should in each PULL request ask for all certificates of an extension. If we would request more specifically certificates that match e.g. a certain CA, we run into issues because there's no clear relation between the certificate selection in ClientCertStoreChromeOS and the private key retrieval in this new KeyStore. Assume we would ask the extension for certs that are signed by a specific CA. As an example, assume that we have a request for certs of CA1, and again some time later we request certs of CA1 again. After that, we ask for the key of one of the certs from the 1st PULL and then for a key of a cert from the 2nd PULL. The key handle retrievals could also be the other way round. We can't clear the cached certificates after one of the key handle retrievals, as we can't tell whether there'll be anymore key handle retrievals. Thus, we accumulate any certificate that we ever pulled from an extension. Only if we request certs for exact same PULL request, we can overwrite the previously returned list of certs. Thus, if we do PULLing in the certificateProvider API, we should IMO either return all available certificates. An alternative that I don't like would be to notify Chrome about removals.
Patchset #3 (id:100001) has been deleted
ptal https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... File net/ssl/client_key_store.cc (right): https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.cc:51: if (certificate->Equals(cert_and_key.certificate.get())) On 2015/08/08 00:14:22, Ryan Sleevi wrote: > This also seems quite inefficient; a linear scan. > > Why not store these as a map of (SHA256HashOfCertificate -> Private Key)? refactored. (I will do so in the implementation of the Provider) https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_store.h File net/ssl/client_key_store.h (right): https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:36: }; On 2015/08/08 00:14:22, Ryan Sleevi wrote: > Why is this necessary? Why do you return it as ProviderHandle rather than int / > with a typedef? refactored. https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:38: using KeyGetter = base::Callback<scoped_ptr<SSLPrivateKey>(void)>; On 2015/08/08 00:14:22, Ryan Sleevi wrote: > This seems unnecessarily opaque. It does not seem to add value as a typedef. refactored. https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:40: struct CertAndKey { On 2015/08/08 00:14:22, Ryan Sleevi wrote: > Why is this a struct instead of just a std::pair or std::tuple? refactored. https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:51: using CertsAndKeys = std::vector<CertAndKey>; On 2015/08/08 00:14:22, Ryan Sleevi wrote: > This seems unnecessarily opaque. It does not seem to add value as a typedef. refactored. https://codereview.chromium.org/1278763002/diff/40001/net/ssl/client_key_stor... net/ssl/client_key_store.h:80: base::Lock lock_; On 2015/08/08 00:14:22, Ryan Sleevi wrote: > Presumably this lock protects all these members; historically, we put it first, > not last. Done.
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... File net/ssl/client_key_store.cc (right): https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... net/ssl/client_key_store.cc:43: if (provider->GetCertificateKey(certificate, &key)) Calling a virtual function inside a lock makes me somewhat nervous. I don't have a concrete alternate suggestion though, so I'm largely expressing my nervousness. If ClientKeyStore maintained the map from cert hashes, you wouldn't have that problem, but I dunno if that would cause other problems. https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... File net/ssl/client_key_store.h (right): https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... net/ssl/client_key_store.h:54: void AddProvider(scoped_ptr<CertKeyProvider> provider); Given that stapling it to a Singleton is leaking it anyway, would it make more sense to make this a raw pointer and then say that the pointer must remain valid until RemoveProvider is called? I'm trying to understand what passing ownership to a global means, whereas it seems the entity that calls RemoveProvider really owns this thing. (Is the caller uploaded yet? I don't know how you intend to call this function, so it's possible with that context this API is better.) https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... net/ssl/client_key_store.h:66: friend struct DefaultSingletonTraits<ClientKeyStore>; I think LazyInstance is usually preferred over Singleton. https://www.chromium.org/developers/coding-style/important-abstractions-and-d...
https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... File net/ssl/client_key_store.cc (right): https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... net/ssl/client_key_store.cc:43: if (provider->GetCertificateKey(certificate, &key)) On 2015/08/14 22:16:48, David Benjamin wrote: > Calling a virtual function inside a lock makes me somewhat nervous. I don't have > a concrete alternate suggestion though, so I'm largely expressing my > nervousness. If ClientKeyStore maintained the map from cert hashes, you wouldn't > have that problem, but I dunno if that would cause other problems. I'm not sure what you're nervous about. Is it because of the performance of virtual calls? Or because we don't know all side-effects of the call? https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... File net/ssl/client_key_store.h (right): https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... net/ssl/client_key_store.h:54: void AddProvider(scoped_ptr<CertKeyProvider> provider); No the caller isn't written yet (didn't finish that part of the refactoring(s) yet). I actually considered only adding a provider for the new API once and never removing it. But, I agree that the ownership here is a bit weird and to even allow a reasonable removal, I had to comment on the minimal lifetime of the provider. I changed the signature to drop the scoped_ptr, as it avoids this problem. https://codereview.chromium.org/1278763002/diff/140001/net/ssl/client_key_sto... net/ssl/client_key_store.h:66: friend struct DefaultSingletonTraits<ClientKeyStore>; On 2015/08/14 22:16:48, David Benjamin wrote: > I think LazyInstance is usually preferred over Singleton. > > https://www.chromium.org/developers/coding-style/important-abstractions-and-d... I fear the numbers don't agree with that documentation: https://code.google.com/p/chromium/codesearch#search/&q=Singleton%5C%3C%20fil... https://code.google.com/p/chromium/codesearch#search/&q=lazyinstance%5C%3C%20... Another difference that is documented in lazy_instance.h : // LazyInstance is similar to Singleton, except it does not have the singleton // property. which would again speak for a singleton... anyways, I don't think that it makes a significant difference.
davidben@chromium.org changed reviewers: + mattm@chromium.org
+mattm, do you mind looking over this for load-balancing purposes? The main relevant context is that while SSLPrivateKey exists and should just be plumbed down from //net, we're not there right now (svaldez is going to be working on this), so, if we're to support custom private keys before then, we need some ugly hack like this one.
overall CL seems fine but I may be missing some of the SSLPrivateKey background.. https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn#newcode269 net/BUILD.gn:269: sources -= [ I don't understand why they are removed in this case.. https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn#newcode362 net/BUILD.gn:362: sources -= [ odd, why are there two sources-= statements in this block? https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn#newcode364 net/BUILD.gn:364: "ssl/client_key_store.h", these should be removed in the same case(s) in net/net_common.gypi too? https://codereview.chromium.org/1278763002/diff/160001/net/ssl/client_key_sto... File net/ssl/client_key_store.h (right): https://codereview.chromium.org/1278763002/diff/160001/net/ssl/client_key_sto... net/ssl/client_key_store.h:47: // The |provider| will be accessed and destroyed on any thread but no Comment about what thread |provider| will be destroyed on should be removed too.
Patchset #5 (id:180001) has been deleted
https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn#newcode269 net/BUILD.gn:269: sources -= [ On 2015/08/21 23:18:17, mattm wrote: > I don't understand why they are removed in this case.. As the ClientKeyStore is used by ssl_platform_key_nss.cc, I added/removed it in the same cases. Just somehow dropped the net_common.gypi changes. Fixed that now. https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn#newcode362 net/BUILD.gn:362: sources -= [ On 2015/08/21 23:18:17, mattm wrote: > odd, why are there two sources-= statements in this block? Acknowledged. https://codereview.chromium.org/1278763002/diff/160001/net/BUILD.gn#newcode364 net/BUILD.gn:364: "ssl/client_key_store.h", On 2015/08/21 23:18:17, mattm wrote: > these should be removed in the same case(s) in net/net_common.gypi too? Done. https://codereview.chromium.org/1278763002/diff/160001/net/ssl/client_key_sto... File net/ssl/client_key_store.h (right): https://codereview.chromium.org/1278763002/diff/160001/net/ssl/client_key_sto... net/ssl/client_key_store.h:47: // The |provider| will be accessed and destroyed on any thread but no On 2015/08/21 23:18:17, mattm wrote: > Comment about what thread |provider| will be destroyed on should be removed too. Done.
ping
lgtm
The CQ bit was checked by pneubeck@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nicolebilbao29@gmail.com, mattm@chromium.org Link to the patchset: https://codereview.chromium.org/1278763002/#ps220001 (title: "Nits.")
thanks for reviewing.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1278763002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1278763002/220001
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/abe6e9d132812e9d2d1065650cf09a724cb53a6d Cr-Commit-Position: refs/heads/master@{#345565} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
