Issue 12220104: Wire up SSL client authentication for OpenSSL/Android through the net/ stack

digit1

7 years, 10 months ago (2013-02-11 22:54:00 UTC) #1

digit1

Hello Ryan and ppi, This is the first version of this patch. It builds correctly, ...

7 years, 10 months ago (2013-02-11 23:15:22 UTC) #2

Hello Ryan and ppi,

This is the first version of this patch. It builds correctly, passes the Android
unit tests locally, and more importantly, has been manually tested on Android
4.0.3 / 4.0.4 / 4.1.x and 4.2 succesfully with RSA-based client certificates.

There is still a little polish I'd like to perform before submitting it, and
your input will be welcomed:

- First, I added CLIENT_CERT_DSS_SIGN to support DSA-based client certificates,
but I didn't modify the rest of the Chromium code that deals with
net::SSLClientCertType. I plan to do this in a different patch, then rebase this
one of top of it. Ryan, please let me know if there are non-obvious pitfalls
here. I hope this won't be too difficult. Otherwise, I think I'd prefer to
temporarily disable DSA support to land this patch for M26.

- There are no unit tests for the chrome/browser code right now. The platform
APIs cannot be mocked directly, so this would require a completely different
implementation of ShowSSLClientCertificateSelector. I'm not sure this is worth
it. Of course, we should be able to have automated unit tests for the net/ stuff
by calling net::UseOpenSSLClientCertigningPrivateKey() directly in the test
setup.

- Do we really need to implement RecordClientCertPrivateKey() /
FetchClientCertPrivateKey() for the linux_redux build, given that there will
never be  browser-layer code to support it, as far as I know? If so, we could
merge the code in openssl_util.cc into openssl_private_key_store_android.cc.
Please let me know your preference, and I'll add the corresponding missing unit
tests after the change, if any.

- I'd like to merge run-forwarded-test-server.sh and run-test-server.sh into a
single script. The Android case would be enabled by a simple flag (e.g.
--android-device) instead.

- Also need to generate DSA and ECDSA based keys in
generate-client-certififcates.sh, and have run-forwarded-test-server.h use then
with a new flag to select the right files to expand manual coverage.

Voila, I invite you to have a quick pass on the code to give your opinion on it.
I don't expect huge changes, apart from stylistic ones, but if this isn't the
case please let me know asap. Feel free to add other reviewers if you feel it'd
be useful.

Thanks.

Ryan Sleevi

On 2013/02/11 23:15:22, digit1 wrote: > Hello Ryan and ppi, > > This is the ...

7 years, 10 months ago (2013-02-11 23:59:03 UTC) #3

On 2013/02/11 23:15:22, digit1 wrote:
> Hello Ryan and ppi,
> 
> This is the first version of this patch. It builds correctly, passes the
Android
> unit tests locally, and more importantly, has been manually tested on Android
> 4.0.3 / 4.0.4 / 4.1.x and 4.2 succesfully with RSA-based client certificates.
> 
> There is still a little polish I'd like to perform before submitting it, and
> your input will be welcomed:
> 
> - First, I added CLIENT_CERT_DSS_SIGN to support DSA-based client
certificates,
> but I didn't modify the rest of the Chromium code that deals with
> net::SSLClientCertType. I plan to do this in a different patch, then rebase
this
> one of top of it. Ryan, please let me know if there are non-obvious pitfalls
> here. I hope this won't be too difficult. Otherwise, I think I'd prefer to
> temporarily disable DSA support to land this patch for M26.
> 

WFM. DSA certs are the white whales of online SSL (eg: I think there's only 6 or
7 public DSA server certs. I'd guess as many client certs)

> - There are no unit tests for the chrome/browser code right now. The platform
> APIs cannot be mocked directly, so this would require a completely different
> implementation of ShowSSLClientCertificateSelector. I'm not sure this is worth
> it. Of course, we should be able to have automated unit tests for the net/
stuff
> by calling net::UseOpenSSLClientCertigningPrivateKey() directly in the test
> setup.

Hrm. This concerns me. I think we absolutely must be getting some degree of test
coverage landed if this is going to ship.

> 
> - Do we really need to implement RecordClientCertPrivateKey() /
> FetchClientCertPrivateKey() for the linux_redux build, given that there will
> never be  browser-layer code to support it, as far as I know? If so, we could
> merge the code in openssl_util.cc into openssl_private_key_store_android.cc.
> Please let me know your preference, and I'll add the corresponding missing
unit
> tests after the change, if any.

I think it's valuable for unit testing, but if you believe the Android device
tests are both publicly visible and reliable to catch regressions, I'd be fine
with this.

> 
> - I'd like to merge run-forwarded-test-server.sh and run-test-server.sh into a
> single script. The Android case would be enabled by a simple flag (e.g.
> --android-device) instead.

I really didn't like this script to begin with - I think we should be using
net::TestServer, which is much more robust. ppi@ indicated this was just for
local testing - but your comments suggest it's for more.

My goal is I don't want to have separate paths for testing client certs - for
Android and "everyting else" - considering all the work invested into the tests
for desktop, unless there's really strong reasons. I don't think the case has
been made here.

> 
> - Also need to generate DSA and ECDSA based keys in
> generate-client-certififcates.sh, and have run-forwarded-test-server.h use
then
> with a new flag to select the right files to expand manual coverage.
> 
> Voila, I invite you to have a quick pass on the code to give your opinion on
it.
> I don't expect huge changes, apart from stylistic ones, but if this isn't the
> case please let me know asap. Feel free to add other reviewers if you feel
it'd
> be useful.
> 
> Thanks.

Ryan Sleevi

I feel like design wise, we're getting into "thousand cuts" territory. I've already raised concerns ...

7 years, 10 months ago (2013-02-12 00:25:16 UTC) #4

I feel like design wise, we're getting into "thousand cuts" territory. I've
already raised concerns re _openssl vs _android before, and in this CL, I'm
trying to understand the differences between:

openssl_key_store
openssl_private_key_store


Wan-Teh and other net/ owners have raised concerns regarding _util files, since
they quickly become 'catchalls' that allow shortcuts for design, rather than
thinking about how things should be structured here. Your
"GetCertificatePublicKeyOpenSSL" highlights this - it's something that seems
much more logically belonging to x509_util_openssl, since it directly deals with
certs - but even more, it seems like it should just be within the implementation
file, and doesn't need to be some generic function.

Regarding this CL, I'd request it be split in two. I think the net/ changes
should be able to land *and be testable* independent of the larger work. We can
then build up the chrome/ pieces. While helpful to see your 'big picture', I
think we want to keep it manageable to prevent CL exhaustion.

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc
(right):

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:75:
namespace browser {
This should be in "namespace chrome {"

The unnamed namespace, in Chromium code, is vastly more common inside the
namespace than outside (eg: "namespace chrome { namespace browser { namespace {"
)

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:144: if
(port <= 0 || port >= 65535) {
BUG: > rather than >= ?

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:164: do {
Chromium code specifically tries to avoid patterns like this (since they're
effectively goto)

You can rewrite it as:

scoped_ptr<SSLClientCertRequest> guard(this);
ScopedClosureRunner callback_guard(base::Bind(callback_, NULL));
....
if (some_error_condition)
  return;

....
// success condition here

closure_guard.Release(); /* rsleevi: This is so our error condition isn't run */
callback_.Run(result);

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.h
(right):

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:6: #define
CHROME_BROWSER_UI_ANDROID_SSL_CLIENT_CERTIFICATE_SELECTOR_ANDROID_H_
Why is the _Android suffix necessary? You're in chrome/browser/ui/android - it
seems unnecessary.

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:20: class
SSLClientCertRequest {
??? This doesn't match the naming of the file - totally surprised to see this
class in here.

Did you mean to call it SSLClientCertificateSelector?

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_key_store.h
File net/base/openssl_key_store.h (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_key_store.h#...
net/base/openssl_key_store.h:67: const X509Certificate& client_certificate,
EVP_PKEY* private_key);
Is it really necessary to create another file for this - particularly one with
the openssl_ prefix rather than suffix?

Can this not fit anywhere else? I am concerned with the growing number of
seemingly related "one-off" SSL functions, and wonder if there is some logical
way that these should be grouped.

For example, should we just be exporting OpenSSLPrivateKeyStore, and let this
method hang off it? Why or why not?

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_memory_priva...
File net/base/openssl_memory_private_key_store.cc (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_memory_priva...
net/base/openssl_memory_private_key_store.cc:27:
OpenSSLMemoryKeyStoreLeakyTraits>::get();
style: indent to the <

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_memory_priva...
net/base/openssl_memory_private_key_store.cc:70:
OpenSSLMemoryKeyStoreLeakyTraits;
Why is this typedef hanging off the class, and not off GetInstance?

Why are you friending DefaultSingletonTraits if you're leaking, by virtue of
LeakyTraits?

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store.h:46: // increment its internal reference
count.
comment nit: That is the very definition of taking ownership in a refcounted
system.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store.h:54: // call EVP_PKEY_free() to free it.
Can you not return a scoped_ptr<EVP_PKEY, EVP_PKEY_free> then?

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_util.cc
File net/base/openssl_util.cc (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_util.cc#newc...
net/base/openssl_util.cc:40: // the caller shall call EVP_PKEY_free() on it.
Why not just take a hash of the public key, to avoid the potentially expensive
comparisons here and have a quick lookup, rather than this search here?

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_util.cc#newc...
net/base/openssl_util.cc:68: }
Why are you duplicating the logic of FindPrivateKey here? Why not just use that?
Worried about lock contention on the EVP_PKEY_dup? Why not just return an
iterator then (in a helper function) that both FindPrivateKey and AddKeyPair can
use?

digit1

On 2013/02/11 23:59:03, Ryan Sleevi wrote: > > WFM. DSA certs are the white whales ...

7 years, 10 months ago (2013-02-12 13:08:44 UTC) #5

On 2013/02/11 23:59:03, Ryan Sleevi wrote:
> 
> WFM. DSA certs are the white whales of online SSL (eg: I think there's only 6
or
> 7 public DSA server certs. I'd guess as many client certs)
> 
Yes, I know, still it'd be nice to have this covered once for all, if this
doesn't pull too much wool. I've uploaded
https://chromiumcodereview.appspot.com/12221136/ to address that. Notice that I
didn't touch the ServerBoundCertificate unit tests.

> > - There are no unit tests for the chrome/browser code right now. The
platform
> > APIs cannot be mocked directly, so this would require a completely different
> > implementation of ShowSSLClientCertificateSelector. I'm not sure this is
worth
> > it. Of course, we should be able to have automated unit tests for the net/
> stuff
> > by calling net::UseOpenSSLClientCertigningPrivateKey() directly in the test
> > setup.
> 
> Hrm. This concerns me. I think we absolutely must be getting some degree of
test coverage landed if this is going to ship.
> 
I'll see what I can do, but the platform APIs themselves can't be easily mocked,
so I'm pretty skeptical. It's probably possible to inject a bypass during
testing in the Java selectClientCertificate() method though. However, I'll focus
first on the other points you mentioned.

> > 
> > - Do we really need to implement RecordClientCertPrivateKey() /
> > FetchClientCertPrivateKey() for the linux_redux build, given that there will
> > never be  browser-layer code to support it, as far as I know? If so, we
could
> > merge the code in openssl_util.cc into openssl_private_key_store_android.cc.
> > Please let me know your preference, and I'll add the corresponding missing
> unit
> > tests after the change, if any.
> 
> I think it's valuable for unit testing, but if you believe the Android device
> tests are both publicly visible and reliable to catch regressions, I'd be fine
> with this.
> 

I'll first start wit the Android unit test and see if they can be useful for the
linux_redux build. I'm concerned by the fact that such common tests might catch
non-existent regressions, since the code paths will never be reached in
'production' linux_redux builds. I.e. such failures would be false positive, and
probably a waste of time, so I'd prefer not spend too much time implementing
them.

> > 
> > - I'd like to merge run-forwarded-test-server.sh and run-test-server.sh into
a
> > single script. The Android case would be enabled by a simple flag (e.g.
> > --android-device) instead.
> 
> I really didn't like this script to begin with - I think we should be using
> net::TestServer, which is much more robust. ppi@ indicated this was just for
> local testing - but your comments suggest it's for more.
> 
> My goal is I don't want to have separate paths for testing client certs - for
> Android and "everyting else" - considering all the work invested into the
tests
> for desktop, unless there's really strong reasons. I don't think the case has
> been made here.
> 

The run-test-server.sh and run-forwarder-test-server.sh are _only_ to perform
manual testing of the feature (which is required to really exercise the real
Android platform API calls). Having these shell scripts is more convenient for
our QA team which doesn't need to have a full Chromium checkout and TestServer
setup.

I was just suggesting merging the two scripts in a single one, since the process
of manual testing with Chrome on Linux is extremely similar (ppi and I actually
used it to verify we didn't break the Linux port).

All other (automated) unit tests will use TestServer() as already discussed,
there is no change of plans here :-)

digit1

On 2013/02/12 00:25:16, Ryan Sleevi wrote: > I feel like design wise, we're getting into ...

7 years, 10 months ago (2013-02-12 14:19:47 UTC) #6

On 2013/02/12 00:25:16, Ryan Sleevi wrote:
> I feel like design wise, we're getting into "thousand cuts" territory. I've
> already raised concerns re _openssl vs _android before, and in this CL, I'm
> trying to understand the differences between:
> 
> openssl_key_store
> openssl_private_key_store
> 

openssl_key_store provides a NET_EXPORT API that must be used by the browser
layer to store the association of a private key "handle" (in this case an
EVP_PKEY*) with a given client certificate.

openssl_private_key_store contains the implementation of this function, as well
as another internal one that is used to retrieve the private key (used by
net/socket/ssl_client_socket_openssl.cc).

It's essentially a way to store the private key out-of-band.

Ideally, we would just store the private key handle inside the
net::X509Certificate object, which would then be passed from the browser layer
to the net one (through content, net/http, etc...).

In other words, you can consider that:

  - net::UseOpenSSLClientCertSigningPrivateKey() is nearly 
    semantically equivalent to calling a new 
    X509Certificate::SetOpenSSLPrivateKeyHandle() method.

  - net::OpenSSLPrivateKeyStore::FetchClientCertPrivateKey()
    is equivalent to X509Certificate::GetOpenSSLPrivateKeyHandle().

  - net::OpenSSLPrivateKeyStore::RecordClientCertPrivateKey()
    is just the implementation for 
    net::UseOpenSSLClientCertSigningPrivateKey().

We're not using the setter/getter method, because you insisted on not adding new
OpenSSL-specific fields to net::X509Certificate, iirc. 

> 
> Wan-Teh and other net/ owners have raised concerns regarding _util files,
since
> they quickly become 'catchalls' that allow shortcuts for design, rather than
> thinking about how things should be structured here. Your
> "GetCertificatePublicKeyOpenSSL" highlights this - it's something that seems
> much more logically belonging to x509_util_openssl, since it directly deals
with
> certs - but even more, it seems like it should just be within the
implementation
> file, and doesn't need to be some generic function.
> 
I agree that moving this to x509_util_openssl makes sense, so I'll do it. It's
implemented as a common function because it is used by two separate sources.
They're both pretty small and didn't want to merge them through an intermediate
class, but don't feel strongly otherwise.


> Regarding this CL, I'd request it be split in two. I think the net/ changes
> should be able to land *and be testable* independent of the larger work. We
can
> then build up the chrome/ pieces. While helpful to see your 'big picture', I
> think we want to keep it manageable to prevent CL exhaustion.
> 
I'm ok with cutting this patch into two. It seems this issue is still confusing
for many, so here's a small refresher of what was previously discussed in issue
166642. I hope this will clarify things for everyone here.

The SSLClientSocket() implementation(s) are based on the following API:
 
  1/ Caller calls ::Connect() after setting up a net::SSLConfig.
     In particular, its |sned_client_cert| field should be false,
     and its |client_cert| field should be null.

  2/ In the case where the server requests client authentication,
     the SSLClientSocket records the request's parameters, then
     the ::Connect() method returns with a specific error code,
     i.e. ERR_SSL_CLIENT_AUTH_CERT_NEEDED.

  3/ It is up to the caller to find the appropriate client
     certificate, then update the |send_client_cert| and
     |client_cert| fields in the SSLConfig being used.

  4/ After this, ::Connect() is called again. This time, it
     resumes the SSL handshake using the client certificate
     passed in the SSLConfig.

  5/ One interesting twist is that completing the handshake
     requires the client to sign a MAC with the certificate's
     private key. All implementations, until now, assume that
     it is possible to perform this operation inside the
     ::Connect() call, with only a handle to the client
     certificate.

Unfortunately, the assumption in 5/, while fundamental to the current design of
SSLClientSocket classes, is invalid on Android, i.e.:

  - There is no Android API that returns a private key
    handle matching a given input client certificate (or
    even merely perform signing with the corresponding private
    key without returning it).

  - There is an API that can return a string 'alias' for a
    client certificate and its private key. However, it can
    only be called from the UI thread.

  - There are also two APIs to return the platform-specific
    private key handle and encoded certificate chain matching
    a given input alias. Unfortunately, they are potentially
    blocking, so we don't want to call them in the UI or
    I/O thread. In the code above, this is performed in a
    background async task started with the first platform
    API call.

The current code solves the problem by doing the following in
chrome::ShowSSLClientCertificateSelector():

  1/ In the browser layer, invoke the platform APIs in the UI
     thread, then an async background task, to get both the
     encoded certificate chain and its private key handle.

  2/ decode the chain into a net::X509Certificate.

  3/ invoke the new net::UseOpenSSLClientCertSigningPrivateKey()
     function to _record_ the association between the client
     certificate and the platform private key handle.

  4/ pass it to the callback, that will store it in the
     SSLConfig's |client_cert|, among other things.

  5/ Later, SSLClientSocket::Connect() is called again, and
     will grab the |client_cert| from the SSLConfig. It then
     uses the internal method
     OpenSSLPrivateKeyStore::FetchClientCertPrivateKey() to
     retrieve the private key handle for the client certificate,
     then proceeds as usual.

The main benefit of this scheme is that it avoids passing the
private key handle, or a string alias in the net::SSLConfig itself. Both have
been considered but are problematic for different reasons:

  - Ryan has expressed several times that he didn't want
    #if defined(USE_OPENSSL) ... #endif blocks in the headers
    defining public classes like net::SSLConfig.

  - There is no common abstraction for private keys in either
    crypto/ or net/, and something is needed to differentiate
    between RSA/DSA/ECDSA based keys.

  - The private key handle is itself highly system-specific. E.g.
    on Android > 4.0.4 it's a global JNI reference to an opaque
    object which correctly refuses to give the application any
    information about the private key bits. On the other hand,
    this means that any operation, like signing, requires
    invoking other platform APIs through JNI.

  - Even on Android, passing a simple string 'alias' in
    net::SSLConfig would mean that the I/O thread would have
    to perform a blocking API call to retrieve the JNI reference
    to the matching platform PrivateKey object. And this is not
    desirable at all.

Another alternative is to store the JNI global reference in the
Android net::X509Certificate itself, but it was rejected by Ryan who didn't want
to see more OpenSSL-specific fields in the class definition.

digit1

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc (right): https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc#newcode75 chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:75: namespace browser { On 2013/02/12 00:25:17, Ryan Sleevi wrote: ...

7 years, 10 months ago (2013-02-12 15:05:25 UTC) #7

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc
(right):

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:75:
namespace browser {
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> This should be in "namespace chrome {"
> 
> The unnamed namespace, in Chromium code, is vastly more common inside the
> namespace than outside (eg: "namespace chrome { namespace browser { namespace
{"
> )

Done.

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:144: if
(port <= 0 || port >= 65535) {
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> BUG: > rather than >= ?

That's embarrassing :) For some reason I've always been convinced that 65535 is
not a valid port number. Looking at all documentation available shows this is
not the case, so I'll fix this. Thanks.

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:164: do {
Thanks for the suggestion, I've applied your rewrite to the code.

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/andr...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.h
(right):

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:6: #define
CHROME_BROWSER_UI_ANDROID_SSL_CLIENT_CERTIFICATE_SELECTOR_ANDROID_H_
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> Why is the _Android suffix necessary? You're in chrome/browser/ui/android - it
> seems unnecessary.

It's not necessary, but some developers prefer to add it nonetheless because it
makes it easier to distinguish between different implementations of the same
files in their favorite editor/IDE.

Given that there are already several files using this convention in this
directory, I just followed the trend.

I really don't have any preference here though, so if you insist on changing
this, please let me know.

https://chromiumcodereview.appspot.com/12220104/diff/1/chrome/browser/ui/andr...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:20: class
SSLClientCertRequest {
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> ??? This doesn't match the naming of the file - totally surprised to see this
> class in here.
> 
> Did you mean to call it SSLClientCertificateSelector?

I really meant SSLClientCertRequest, it's a C++ class that matches its Java
counterpart, as expected by the JNI generator. The main reason why this is here
is because the auto-generated header "jni/SSLClientCertificateSelector_jni.h"
needs the full declaration of the class before the include (that happens in the
.cc file).

I'll move this to its own header/source file to clean this up.

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_key_s...
File net/base/openssl_key_store.h (right):

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_key_s...
net/base/openssl_key_store.h:67: const X509Certificate& client_certificate,
EVP_PKEY* private_key);
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> Is it really necessary to create another file for this - particularly one with
> the openssl_ prefix rather than suffix?
> 
> Can this not fit anywhere else? I am concerned with the growing number of
> seemingly related "one-off" SSL functions, and wonder if there is some logical
> way that these should be grouped.
> 
> For example, should we just be exporting OpenSSLPrivateKeyStore, and let this
> method hang off it? Why or why not?

There is a need for a single NET_EXPORT function that implements this feature,
to be called directly from the browser layer.

No other feature of OpenSSLPrivateKeyStore will ever be used from outside the
net/ layer, and I don't think exposing / exporting it would be useful (in other
comments, I've even hinted at the idea of getting rid of the interface
entirely).

I'm open to your suggestions for a better location though. I'm inclined to use a
separate header here because it's such a specialized usage that it doesn't
really fit the typical scope of other headers like x509_certificate.h or
x509_util_openssl.h.

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_memor...
File net/base/openssl_memory_private_key_store.cc (right):

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_memor...
net/base/openssl_memory_private_key_store.cc:27:
OpenSSLMemoryKeyStoreLeakyTraits>::get();
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> style: indent to the <

Done.

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_memor...
net/base/openssl_memory_private_key_store.cc:70:
OpenSSLMemoryKeyStoreLeakyTraits;
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> Why is this typedef hanging off the class, and not off GetInstance?
> 
> Why are you friending DefaultSingletonTraits if you're leaking, by virtue of
> LeakyTraits?

Most of this code new actually comes from the existing
openssl_private_key_store_android.cc implementation. Looking at the log, it
seems a leaky singleton is needed because the key store is being used from a
non-joinable worker thread that may still be running at shutdown. At least on
Android. I'm not too sure about linux_redux though. I've removed this to get
back to the previous state of the code. Let me know if this isn't correct.

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_priva...
File net/base/openssl_private_key_store.h (right):

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_priva...
net/base/openssl_private_key_store.h:46: // increment its internal reference
count.
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> comment nit: That is the very definition of taking ownership in a refcounted
> system.
I'll rephrase that. I was just paraphrasing the original comment to make it
clearer. Obviously this didn't work as expected :)

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_util.cc
File net/base/openssl_util.cc (right):

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_util....
net/base/openssl_util.cc:40: // the caller shall call EVP_PKEY_free() on it.
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> Why not just take a hash of the public key, to avoid the potentially expensive
> comparisons here and have a quick lookup, rather than this search here?

I really don't think this is a performance-critical function, and the hash
generation you're mentioning seems much more work with little benefit. Unless
you have a a simple proposal that works without adding too much stuff, I'd
prefer to keep this simple.

https://chromiumcodereview.appspot.com/12220104/diff/1/net/base/openssl_util....
net/base/openssl_util.cc:68: }
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> Why are you duplicating the logic of FindPrivateKey here? Why not just use
that?
> Worried about lock contention on the EVP_PKEY_dup? Why not just return an
> iterator then (in a helper function) that both FindPrivateKey and AddKeyPair
can
> use?

Hmmm... because it's only 3 lines of code. I'll fix this though.

Ryan Sleevi

On 2013/02/12 13:08:44, digit1 wrote: > The run-test-server.sh and run-forwarder-test-server.sh are _only_ to perform > ...

7 years, 10 months ago (2013-02-12 18:38:45 UTC) #8

Ryan Sleevi

On 2013/02/12 14:19:47, digit1 wrote: > On 2013/02/12 00:25:16, Ryan Sleevi wrote: > > I ...

7 years, 10 months ago (2013-02-12 18:43:43 UTC) #9

On 2013/02/12 14:19:47, digit1 wrote:
> On 2013/02/12 00:25:16, Ryan Sleevi wrote:
> > I feel like design wise, we're getting into "thousand cuts" territory. I've
> > already raised concerns re _openssl vs _android before, and in this CL, I'm
> > trying to understand the differences between:
> > 
> > openssl_key_store
> > openssl_private_key_store
> > 
> 
> openssl_key_store provides a NET_EXPORT API that must be used by the browser
> layer to store the association of a private key "handle" (in this case an
> EVP_PKEY*) with a given client certificate.
> 
> openssl_private_key_store contains the implementation of this function, as
well
> as another internal one that is used to retrieve the private key (used by
> net/socket/ssl_client_socket_openssl.cc).
> 
> It's essentially a way to store the private key out-of-band.
> 
> Ideally, we would just store the private key handle inside the
> net::X509Certificate object, which would then be passed from the browser layer
> to the net one (through content, net/http, etc...).
> 
> In other words, you can consider that:
> 
>   - net::UseOpenSSLClientCertSigningPrivateKey() is nearly 
>     semantically equivalent to calling a new 
>     X509Certificate::SetOpenSSLPrivateKeyHandle() method.
> 
>   - net::OpenSSLPrivateKeyStore::FetchClientCertPrivateKey()
>     is equivalent to X509Certificate::GetOpenSSLPrivateKeyHandle().
> 
>   - net::OpenSSLPrivateKeyStore::RecordClientCertPrivateKey()
>     is just the implementation for 
>     net::UseOpenSSLClientCertSigningPrivateKey().
> 
> We're not using the setter/getter method, because you insisted on not adding
new
> OpenSSL-specific fields to net::X509Certificate, iirc. 

More importantly than avoiding OpenSSL-specific code is that the entire concept
in Chromium is that X509Certificate is immutable, because they are shared
objects on multiple threads.

From the perspective of net/, this design concerns me. It took me several read
throughs to understand the semantic differences between your three bullet
points, because of how closely they're related. I would rather see the
OpenSSLPrivateKeyStore NET_EXPORTED then having to NET_EXPORT utility functions
that wrap it, simply to keep the key store from being exported. Those layers of
indirection hurt readability from within and external to net/.

> 
> > 
> > Wan-Teh and other net/ owners have raised concerns regarding _util files,
> since
> > they quickly become 'catchalls' that allow shortcuts for design, rather than
> > thinking about how things should be structured here. Your
> > "GetCertificatePublicKeyOpenSSL" highlights this - it's something that seems
> > much more logically belonging to x509_util_openssl, since it directly deals
> with
> > certs - but even more, it seems like it should just be within the
> implementation
> > file, and doesn't need to be some generic function.
> > 
> I agree that moving this to x509_util_openssl makes sense, so I'll do it. It's
> implemented as a common function because it is used by two separate sources.
> They're both pretty small and didn't want to merge them through an
intermediate
> class, but don't feel strongly otherwise.
> 
> 
> > Regarding this CL, I'd request it be split in two. I think the net/ changes
> > should be able to land *and be testable* independent of the larger work. We
> can
> > then build up the chrome/ pieces. While helpful to see your 'big picture', I
> > think we want to keep it manageable to prevent CL exhaustion.
> > 
> I'm ok with cutting this patch into two. It seems this issue is still
confusing
> for many, so here's a small refresher of what was previously discussed in
issue
> 166642. I hope this will clarify things for everyone here.
> 
> The SSLClientSocket() implementation(s) are based on the following API:
>  
>   1/ Caller calls ::Connect() after setting up a net::SSLConfig.
>      In particular, its |sned_client_cert| field should be false,
>      and its |client_cert| field should be null.
> 
>   2/ In the case where the server requests client authentication,
>      the SSLClientSocket records the request's parameters, then
>      the ::Connect() method returns with a specific error code,
>      i.e. ERR_SSL_CLIENT_AUTH_CERT_NEEDED.
> 
>   3/ It is up to the caller to find the appropriate client
>      certificate, then update the |send_client_cert| and
>      |client_cert| fields in the SSLConfig being used.
> 
>   4/ After this, ::Connect() is called again. This time, it
>      resumes the SSL handshake using the client certificate
>      passed in the SSLConfig.
> 
>   5/ One interesting twist is that completing the handshake
>      requires the client to sign a MAC with the certificate's
>      private key. All implementations, until now, assume that
>      it is possible to perform this operation inside the
>      ::Connect() call, with only a handle to the client
>      certificate.
> 
> Unfortunately, the assumption in 5/, while fundamental to the current design
of
> SSLClientSocket classes, is invalid on Android, i.e.:
> 
>   - There is no Android API that returns a private key
>     handle matching a given input client certificate (or
>     even merely perform signing with the corresponding private
>     key without returning it).
> 
>   - There is an API that can return a string 'alias' for a
>     client certificate and its private key. However, it can
>     only be called from the UI thread.
> 
>   - There are also two APIs to return the platform-specific
>     private key handle and encoded certificate chain matching
>     a given input alias. Unfortunately, they are potentially
>     blocking, so we don't want to call them in the UI or
>     I/O thread. In the code above, this is performed in a
>     background async task started with the first platform
>     API call.
> 
> The current code solves the problem by doing the following in
> chrome::ShowSSLClientCertificateSelector():
> 
>   1/ In the browser layer, invoke the platform APIs in the UI
>      thread, then an async background task, to get both the
>      encoded certificate chain and its private key handle.
> 
>   2/ decode the chain into a net::X509Certificate.
> 
>   3/ invoke the new net::UseOpenSSLClientCertSigningPrivateKey()
>      function to _record_ the association between the client
>      certificate and the platform private key handle.
> 
>   4/ pass it to the callback, that will store it in the
>      SSLConfig's |client_cert|, among other things.
> 
>   5/ Later, SSLClientSocket::Connect() is called again, and
>      will grab the |client_cert| from the SSLConfig. It then
>      uses the internal method
>      OpenSSLPrivateKeyStore::FetchClientCertPrivateKey() to
>      retrieve the private key handle for the client certificate,
>      then proceeds as usual.
> 
> The main benefit of this scheme is that it avoids passing the
> private key handle, or a string alias in the net::SSLConfig itself. Both have
> been considered but are problematic for different reasons:
> 
>   - Ryan has expressed several times that he didn't want
>     #if defined(USE_OPENSSL) ... #endif blocks in the headers
>     defining public classes like net::SSLConfig.
> 
>   - There is no common abstraction for private keys in either
>     crypto/ or net/, and something is needed to differentiate
>     between RSA/DSA/ECDSA based keys.
> 
>   - The private key handle is itself highly system-specific. E.g.
>     on Android > 4.0.4 it's a global JNI reference to an opaque
>     object which correctly refuses to give the application any
>     information about the private key bits. On the other hand,
>     this means that any operation, like signing, requires
>     invoking other platform APIs through JNI.
> 
>   - Even on Android, passing a simple string 'alias' in
>     net::SSLConfig would mean that the I/O thread would have
>     to perform a blocking API call to retrieve the JNI reference
>     to the matching platform PrivateKey object. And this is not
>     desirable at all.
> 
> Another alternative is to store the JNI global reference in the
> Android net::X509Certificate itself, but it was rejected by Ryan who didn't
want
> to see more OpenSSL-specific fields in the class definition.

digit1

On 2013/02/12 18:43:43, Ryan Sleevi wrote: > > More importantly than avoiding OpenSSL-specific code is ...

7 years, 10 months ago (2013-02-12 19:31:49 UTC) #10

Ryan Sleevi

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc (right): https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc#newcode167 chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:167: LOG(INFO) << "Client certificate request cancelled"; seems unnecessarily log ...

7 years, 10 months ago (2013-02-12 20:12:58 UTC) #11

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc
(right):

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:167:
LOG(INFO) << "Client certificate request cancelled";
seems unnecessarily log spammy (and throughout)

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:231: //
Client Auth is not implemented on Android yet.
This function isn't correct, is it?

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:237:
DVLOG(1) << __FUNCTION__ << " " << contents;
This seems extra extra spammy. Minimally, why log __FUNCTION__ here? Why isn't
the dvlog, which already contains file and line, sufficient?

Why log the contents pointer?

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.h
(right):

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:18:
namespace browser {
namespace chrome {
namespace browser {

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:20: class
SSLClientCertRequest {
On 2013/02/12 15:05:25, digit1 wrote:
> On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> > ??? This doesn't match the naming of the file - totally surprised to see
this
> > class in here.
> > 
> > Did you mean to call it SSLClientCertificateSelector?
> 
> I really meant SSLClientCertRequest, it's a C++ class that matches its Java
> counterpart, as expected by the JNI generator. The main reason why this is
here
> is because the auto-generated header "jni/SSLClientCertificateSelector_jni.h"
> needs the full declaration of the class before the include (that happens in
the
> .cc file).
> 
> I'll move this to its own header/source file to clean this up.

But then there is no SSLClientCertificateSelectorAndroid, is there?

Or are you seeing this header containing only the one function from line 59/60?

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:26:
callback_(callback) { }
style: indents are wrong (need two additional spaces on line 23 & 24), and
braces should be should be on separate lines for multiline initializer lists.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_key_store.h
File net/base/openssl_key_store.h (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_key_store.h#...
net/base/openssl_key_store.h:67: const X509Certificate& client_certificate,
EVP_PKEY* private_key);
On 2013/02/12 15:05:25, digit1 wrote:
> On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> > Is it really necessary to create another file for this - particularly one
with
> > the openssl_ prefix rather than suffix?
> > 
> > Can this not fit anywhere else? I am concerned with the growing number of
> > seemingly related "one-off" SSL functions, and wonder if there is some
logical
> > way that these should be grouped.
> > 
> > For example, should we just be exporting OpenSSLPrivateKeyStore, and let
this
> > method hang off it? Why or why not?
> 
> There is a need for a single NET_EXPORT function that implements this feature,
> to be called directly from the browser layer.
> 
> No other feature of OpenSSLPrivateKeyStore will ever be used from outside the
> net/ layer, and I don't think exposing / exporting it would be useful (in
other
> comments, I've even hinted at the idea of getting rid of the interface
> entirely).

While I'm amenable to replacing it, I think having both the helper functions and
the store are confusing for readers both within and external to net/. Having
utility functions for utility functions sake tends to hinder
readability/maintainability, so I'd like to avoid it if at all possible.

It's much easier to refactor calls to a single class than to deal with multiple
layers of indirection, especially when in an early phase of bringup.

> 
> I'm open to your suggestions for a better location though. I'm inclined to use
a
> separate header here because it's such a specialized usage that it doesn't
> really fit the typical scope of other headers like x509_certificate.h or
> x509_util_openssl.h.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store.h:48: EVP_PKEY* private_key) = 0;
Nowhere in Chromium do we pass a "const X509Certificate&". It's always treated
as a "const X509Certificate*" or a "X509Certificate*".

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
File net/base/openssl_private_key_store_android.cc (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:25: ~OpenSSLKeyStoreAndroid()
OVERRIDE {}
style: Don't put OVERRIDE on destructor
style: Put virtual on the destructor

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:29:
OpenSSLKeyStoreAndroidLeakyTraits>::get();
nit: indents (align to < >

Ryan Sleevi

On 2013/02/12 19:31:49, digit1 wrote: > On 2013/02/12 18:43:43, Ryan Sleevi wrote: > > > ...

7 years, 10 months ago (2013-02-12 20:15:14 UTC) #12

On 2013/02/12 19:31:49, digit1 wrote:
> On 2013/02/12 18:43:43, Ryan Sleevi wrote:
> > 
> > More importantly than avoiding OpenSSL-specific code is that the entire
> concept
> > in Chromium is that X509Certificate is immutable, because they are shared
> > objects on multiple threads.
> >
> I don't think this would violate this. The private key is immutable too and is
> uniquely associated with the certificate object. I.e. the certificate is
> currently created with CreateFromDERCertChain(). I can imagine a new
> OpenSSL-specific function like CreateFromDERCertChainAndPrivateKey() that
would
> take both the encoded certificate chain and an EVP_PKEY handle.  It would
create
> an immutable certificate object, no need for SetPrivateKey() here. Of course,
a
> GetPrivateKey() accessor would be needed to retrieve the EVP_PKEY and pass it
to
> OpenSSL in the socket code, but that's about it. That would probably require
> less support code than the current patch, can be independently unit tested,
and
> would make global code flow easier to understand.
>  
> What I don't know is if this is going to interact in surprising ways with
> intermediate layers (e.g. the caching performed in net/http/.

I'd like to avoid this, all the still.

1) It couples the notion of private keys directly to the cert, which as you
acknowledge by design, isn't always great
2) The design assumptions of X509Certificate rely on the ability to cache
objects. This was even moreso in the past, when you literally would get the same
X509Certificate* created in earlier operations. This design assumption is baked
into many layers, and would require much more careful evaluation that does not
align with your timeframes.

> 
> > From the perspective of net/, this design concerns me. It took me several
read
> > throughs to understand the semantic differences between your three bullet
> > points, because of how closely they're related. I would rather see the
> > OpenSSLPrivateKeyStore NET_EXPORTED then having to NET_EXPORT utility
> functions
> > that wrap it, simply to keep the key store from being exported. Those layers
> of
> > indirection hurt readability from within and external to net/.
> >
> Ok, I'll export OpenSSLPrivateKeyStore then. Patch coming soon. Thanks

digit1

FYI, I'm uploading a few intermediate patches to test them on the bots. There is ...

7 years, 10 months ago (2013-02-13 17:16:07 UTC) #13

digit1

I've removed all the chrome/ changes from this Issue (to be revived in a later ...

7 years, 10 months ago (2013-02-13 18:24:34 UTC) #14

I've removed all the chrome/ changes from this Issue (to be revived in a later
patch). This now only contains only net/ specific changes, i.e.:

  - net::OpenSSLPrivateKeyStore is NET_EXPORT, no longer abstract, and
     has unit tests.

  - added new SSLClientSocketOpenSSLClientAuth unit tests that run
     automatically against the test server.

  - SSLClientSocketOpenSSL::GetSSLInfo() returns a valid value for
     |client_cert_sent| even if the server rejected the connection (necessary
     for proper testing).

I've tested that this passes the android and linux_redux net_unittests (while
the full linux_redux build is broken, net_unittests do compile and run).

Please have a look, thanks.

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc
(right):

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:167:
LOG(INFO) << "Client certificate request cancelled";
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> seems unnecessarily log spammy (and throughout)

Actually, given the level of platform bugs we've encountered, I'd like to keep
this in the log in order to potentially diagnose problems when this is deployed
in the wild. I expect surprises due to OEM enhancements, so the most messages
corresponding to unexpected use cases, the better, at this point.

Also, it's not really spammy. It can only be displayed when the UI dialog was
cancelled by the user, which will happen rarely in practice (versus, say
connecting to the server, which will reuse cached credentials).

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:237:
DVLOG(1) << __FUNCTION__ << " " << contents;
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> This seems extra extra spammy. Minimally, why log __FUNCTION__ here? Why isn't
> the dvlog, which already contains file and line, sufficient?
> 
> Why log the contents pointer?

This came directly from the version in
chrome/browser/ui/views/ssl_client_certificate_selector.cc. I've removed it
since it's not needed anyway.

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.h
(right):

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:18:
namespace browser {
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> namespace chrome {
> namespace browser {
Actually, this can't be under the chrome namespace due to weird interactions
with the JNI generated headers. To be more exact, this makes
chrome/browser/android/chrome_jni_registrar.cc fail to compile.

Also, the other ports like chrome/browser/ui/views/ don't even use any namespace
to declare their helper classes. I didn't want to do this.

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:20: class
SSLClientCertRequest {
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> On 2013/02/12 15:05:25, digit1 wrote:
> > On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> > > ??? This doesn't match the naming of the file - totally surprised to see
> this
> > > class in here.
> > > 
> > > Did you mean to call it SSLClientCertificateSelector?
> > 
> > I really meant SSLClientCertRequest, it's a C++ class that matches its Java
> > counterpart, as expected by the JNI generator. The main reason why this is
> here
> > is because the auto-generated header
"jni/SSLClientCertificateSelector_jni.h"
> > needs the full declaration of the class before the include (that happens in
> the
> > .cc file).
> > 
> > I'll move this to its own header/source file to clean this up.
> 
> But then there is no SSLClientCertificateSelectorAndroid, is there?
> 
> Or are you seeing this header containing only the one function from line
59/60?

I've removed the chrome/ changes from this review, but if you look at
intermediate ones (e.g. Patch set 4), you'll see the following:

- ssl_client_certificate_selector_android.h is removed.
- ssl_client_certificate_request.h / .cc are added.
- ssl_client_certificate_selector_android.cc implements
chrome::ShowSSLClientCertificateSelector(), by using
SSLClientCertificateRequest.

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.h:26:
callback_(callback) { }
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> style: indents are wrong (need two additional spaces on line 23 & 24), and
> braces should be should be on separate lines for multiline initializer lists.

thanks, I'll fix this in the next patch that revives the chrome changes.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_key_store.h
File net/base/openssl_key_store.h (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_key_store.h#...
net/base/openssl_key_store.h:67: const X509Certificate& client_certificate,
EVP_PKEY* private_key);
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> On 2013/02/12 15:05:25, digit1 wrote:
> > On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> > > Is it really necessary to create another file for this - particularly one
> with
> > > the openssl_ prefix rather than suffix?
> > > 
> > > Can this not fit anywhere else? I am concerned with the growing number of
> > > seemingly related "one-off" SSL functions, and wonder if there is some
> logical
> > > way that these should be grouped.
> > > 
> > > For example, should we just be exporting OpenSSLPrivateKeyStore, and let
> this
> > > method hang off it? Why or why not?
> > 
> > There is a need for a single NET_EXPORT function that implements this
feature,
> > to be called directly from the browser layer.
> > 
> > No other feature of OpenSSLPrivateKeyStore will ever be used from outside
the
> > net/ layer, and I don't think exposing / exporting it would be useful (in
> other
> > comments, I've even hinted at the idea of getting rid of the interface
> > entirely).
> 
> While I'm amenable to replacing it, I think having both the helper functions
and
> the store are confusing for readers both within and external to net/. Having
> utility functions for utility functions sake tends to hinder
> readability/maintainability, so I'd like to avoid it if at all possible.
> 
> It's much easier to refactor calls to a single class than to deal with
multiple
> layers of indirection, especially when in an early phase of bringup.
> 
> > 
> > I'm open to your suggestions for a better location though. I'm inclined to
use
> a
> > separate header here because it's such a specialized usage that it doesn't
> > really fit the typical scope of other headers like x509_certificate.h or
> > x509_util_openssl.h.
> 

openssl_util.* and openssl_key_store.* are now gone, and
net::OpenSSLPrivateKeyStore is now NET_EXPORT. This simplifies many things.
Thanks.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store.h:48: EVP_PKEY* private_key) = 0;
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> Nowhere in Chromium do we pass a "const X509Certificate&". It's always treated
> as a "const X509Certificate*" or a "X509Certificate*".

I think a simple grep would show otherwise. However, that's true for the net/
code, so I've changed it too.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store.h:54: // call EVP_PKEY_free() to free it.
On 2013/02/12 00:25:17, Ryan Sleevi wrote:
> Can you not return a scoped_ptr<EVP_PKEY, EVP_PKEY_free> then?

Done.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
File net/base/openssl_private_key_store_android.cc (right):

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:25: ~OpenSSLKeyStoreAndroid()
OVERRIDE {}
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> style: Don't put OVERRIDE on destructor
> style: Put virtual on the destructor

Done.

https://codereview.chromium.org/12220104/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:29:
OpenSSLKeyStoreAndroidLeakyTraits>::get();
On 2013/02/12 20:12:58, Ryan Sleevi wrote:
> nit: indents (align to < >

Done.

Ryan Sleevi

https://chromiumcodereview.appspot.com/12220104/diff/22002/net/base/cert_database_openssl.cc File net/base/cert_database_openssl.cc (right): https://chromiumcodereview.appspot.com/12220104/diff/22002/net/base/cert_database_openssl.cc#newcode32 net/base/cert_database_openssl.cc:32: cert, &private_key)) indents https://chromiumcodereview.appspot.com/12220104/diff/22002/net/base/openssl_private_key_store.h File net/base/openssl_private_key_store.h (right): https://chromiumcodereview.appspot.com/12220104/diff/22002/net/base/openssl_private_key_store.h#newcode57 net/base/openssl_private_key_store.h:57: ...

7 years, 10 months ago (2013-02-13 23:25:55 UTC) #15

Ryan Sleevi

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc (right): https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc#newcode167 chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:167: LOG(INFO) << "Client certificate request cancelled"; On 2013/02/13 18:24:34, ...

7 years, 10 months ago (2013-02-13 23:32:26 UTC) #16

digit1

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc (right): https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl_client_certificate_selector_android.cc#newcode167 chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:167: LOG(INFO) << "Client certificate request cancelled"; Point taken then, ...

7 years, 10 months ago (2013-02-14 06:23:50 UTC) #17

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
File chrome/browser/ui/android/ssl_client_certificate_selector_android.cc
(right):

https://codereview.chromium.org/12220104/diff/1/chrome/browser/ui/android/ssl...
chrome/browser/ui/android/ssl_client_certificate_selector_android.cc:167:
LOG(INFO) << "Client certificate request cancelled";
Point taken then, I'll remove the line.

https://codereview.chromium.org/12220104/diff/22002/net/base/cert_database_op...
File net/base/cert_database_openssl.cc (right):

https://codereview.chromium.org/12220104/diff/22002/net/base/cert_database_op...
net/base/cert_database_openssl.cc:32: cert, &private_key))
will do. thanks.

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:57: };
ok, I don't think it's reasonable to depend on changes to scoped_ptr at this
point, so which alternative would you prefer here then:

1/ typedef crypto::ScopedOpenSSL<EVP_PKEY, EVP_PKEY_free> ScopedEVP_PKEY;

2
  struct EVP_PKEY_Deleter {
    inline void operator()(EVP_PKEY* ptr) const {
      EVP_PKEY_free(ptr);
    }
  };
  typedef scoped_ptr<EVP_PKEY, EVP_PKEY_deleter> ScopedEVP_PKEY;

Both are equivalent in the sense that they don't require callers to change their
use.

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:93: protected:
The class must be derived by OpenSSLMemoryPrivateKeyStore and
OpenSSLPrivateKeyStoreAndroid in order to override the StoreKeyPair() method.

I think it's possible to move most stuff to private though, except for the
constructor, destructor and AddKeyPair.

I'm not sure about what you want here, can I ask you to clarify?

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
File net/base/openssl_private_key_store_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store_unittest.cc:120: }
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> As mentioned on previous reviews, such scope blocks are unnecessary and
> generally discouraged.

In this case it is intentionally used to release pkey2 and decrease the
reference count on the priv_key, which is later tested after the call to
Flush(). That's why I added a comment on line 119 to make this explicit.

The point is to check that store->Flush() actually releases the reference it
holds on the key. It is possible to keep pkey2 alive until the end of the
function and check that the final reference count is 2, instead of 1, but I
think this version of the code is less ambiguous / more expressive about its
intent. Do you still prefer that I remove the scope block?

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:49: typedef
crypto::ScopedOpenSSL<BIGNUM, BN_free> ScopedBIGNUM;
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> scoped_ptr<> ?

I don't really see the point given that it requires so much more typing, and
this is unit test code, not something exported by the net/ stack.

Slightly off-topic, it would be nice to have a header defining common
declarations for scoped OpenSSL types that can be reused by several source files
(currently, each one has its own set of typedefs like the one above).  Do you
find this idea wortwhile, and if so, do you have a suggestion for this header
(net/base/x509_util.h maybe?). I don't intend to address this point in this
patch, nor decide whether the declarations should use ScopedOpenSSL or
scoped_ptr, but we're discussing a related issue here, so your input is
welcomed.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:61: }
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> This shouldn't be in a unittest file like this.

Can you clarify? This comes straight from
net/socket/ssl_client_socket_unittest.cc btw.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:70: ScopedStdioHandle
file(file_util::OpenFile(filepath, "rb"));
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> Use FileUtil::ReadFileToString, rather than a ScopedStdioHandle. Avoid the
extra
> dependency on FILE* glue.

I'm surprised by this request, given that last week you asked for _exactly_ the
opposite (i.e. I was using file_util::ReadFileToString, but you asked me to use
PEM_read_PrivateKey(), which requires FILE* handles). Are you really sure you
want this changed in the opposite direction?

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:98: bool
CreateRandomPrivateKey(
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> This is not something we want to do in tests, since it makes it significantly
> worse under ASAN/TSAN/Valgrind/any form of runtime inspection.
> 
I'm not sure to understand what you mean here. Can you clarify?

> There's no need to generate a new key every run.
This is the simplest way I found to generate an EVP_PKEY which had the
interesting property of not being rejected by OpenSSL when used with a
non-matching certificate. The tricky part is that this requires a custom
RSA_METHOD, and functions like PEM_read_PrivateKey() cannot be used to load an
EVP_PKEY with said custom method. The alternative is to implement a custom
ENGINE to address this single point, which appears to be a _lot_ more work.

But the point is moot, because the SendBadSignature test is going to be removed,
so let's not lose time over this anyway.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:157: }  // namespace
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> Put the entire file into namespace net { namespace { } }.

This would result in a link error, because there are similarly named functions
in net/socket/ssl_client_socket_unittest.cc (where they come from), which are
already in the net:: namespace.

I didn't want to move them to yet another common header/source file, so putting
them in an anonymous namespace seemed the simpler thing. The alternative is to
rename the functions though. Are you really sure about this point?

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:159:
//-----------------------------------------------------------------------------
Again, this came from net/socket/ssl_client_socket_unittest.cc. I'll remove them
though.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:394: // It should not allow the
connection.
Ok, I'll remove this test, this gets rid of the problematic
CreateRandomPrivateKey() function too.

Ryan Sleevi

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_key_store.h File net/base/openssl_private_key_store.h (right): https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_key_store.h#newcode57 net/base/openssl_private_key_store.h:57: }; On 2013/02/14 06:23:50, digit1 wrote: > ok, I ...

7 years, 10 months ago (2013-02-14 07:15:00 UTC) #18

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:57: };
On 2013/02/14 06:23:50, digit1 wrote:
> ok, I don't think it's reasonable to depend on changes to scoped_ptr at this
> point, so which alternative would you prefer here then:
> 
> 1/ typedef crypto::ScopedOpenSSL<EVP_PKEY, EVP_PKEY_free> ScopedEVP_PKEY;
> 
> 2
>   struct EVP_PKEY_Deleter {
>     inline void operator()(EVP_PKEY* ptr) const {
>       EVP_PKEY_free(ptr);
>     }
>   };
>   typedef scoped_ptr<EVP_PKEY, EVP_PKEY_deleter> ScopedEVP_PKEY;
> 
> Both are equivalent in the sense that they don't require callers to change
their
> use.

I'm not sure I understand your point about "not depend on changes to
scoped_ptr". We've been collectively trying to reduce the custom-scoped-X
classes, following a series of scoped_ptr<> cleanups to bring it into alignment
with unique_ptr<> in the spec. It's already on my radar to fix
crypto/scoped_capi_types and crypto/scoped_nss_types. I'm particularly sensitive
to new introductions of generics and new scoped types, trying to make sure we're
not creating more work for us to fix later.

I think 1/ is the lesser of the two evils when only given those two options, if
only because we've been doing 1/ (unfortunately), but I think we should be
working to entirely rid the codebase of ScopedOpenSSL in favour of using
scoped_ptr<> consistently, which is why you're getting push back. This is the
first CL touching the scoped_x types now that we have them unified, which is why
I'm saying "Hrm, we should clean this up while we're here."

Part of the general "leave the code better then when you got there" philosophy
in Chrome, even if it results in 'more' requests.

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:93: protected:
On 2013/02/14 06:23:50, digit1 wrote:
> The class must be derived by OpenSSLMemoryPrivateKeyStore and
> OpenSSLPrivateKeyStoreAndroid in order to override the StoreKeyPair() method.
> 
> I think it's possible to move most stuff to private though, except for the
> constructor, destructor and AddKeyPair.
> 
> I'm not sure about what you want here, can I ask you to clarify?

I'm asking that you carefully evaluate what needs to be protected, and what
can/should be private. As the style guide says, especially with datamembers,
they should be private.

Ctor, Dtor, AddKeyPair is fine - if that's the minimal interface to expose.
Exposing KeyPair, lock_, and pairs_ is problematic though.

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:123: base::Lock lock_;
Please make sure to fully comment/explain the threading lifetime - perhaps on
24-26 - so that it's clear why this is locked.

You mentioned worker threads before - but that surprises me, as from looking at
the CL, it seems like it should only be the UI thread (on the activity
completion) and on the IO thread (when performing handshakes). In the activity
completion model (in chrome/), it seems much more preferable to simply PostTask
to the BrowserThread::IO to do the modification, so that this object is *only*
accessed on the IO thread, rather than to require this object to implement
thread-safe locking.

The reasoning behind that is that it's easier to reason about the code when it's
single-threaded and has a defined lifetime.

The PostTask model is further simplified when using scoped_ptr<> with custom
deleters, since then you can use base::Passed to pass the EVP_PKEY to the IO
thread in a scoped_ptr<EVP_PKEY, EVP_PKEY_free>. Using a custom type, you can't
use base::Passed(), and you can't use base::Owned() with an custom-deleter.

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
File net/base/openssl_private_key_store_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store_unittest.cc:120: }
On 2013/02/14 06:23:50, digit1 wrote:
> On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> > As mentioned on previous reviews, such scope blocks are unnecessary and
> > generally discouraged.
> 
> In this case it is intentionally used to release pkey2 and decrease the
> reference count on the priv_key, which is later tested after the call to
> Flush(). That's why I added a comment on line 119 to make this explicit.
> 
> The point is to check that store->Flush() actually releases the reference it
> holds on the key. It is possible to keep pkey2 alive until the end of the
> function and check that the final reference count is 2, instead of 1, but I
> think this version of the code is less ambiguous / more expressive about its
> intent. Do you still prefer that I remove the scope block?

I actually find that preferable, in that when reading this, I missed the
significance of line 119 as it relates to the test.

Alternatively, rather than using the scope block, using .release() would also be
at least internally consistent with how such unit tests have historically been
used, as part of the general preference to minimize unnecessary indentation
(such as how error handling is structured).

Something like:

ASSERT_EQ(3, EVP_PKEY_get_refcount(priv_key.get()));
pkey2.release();
ASSERT_EQ(2, EVP_PKEY_get_refcount(priv_key.get()));
// Flush the store ...
store_->Flush();
ASSERT_EQ(1, EVP_PKEY_get_refcount(priv_key.get()));

Making clear the sequences.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:49: typedef
crypto::ScopedOpenSSL<BIGNUM, BN_free> ScopedBIGNUM;
On 2013/02/14 06:23:50, digit1 wrote:
> On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> > scoped_ptr<> ?
> 
> I don't really see the point given that it requires so much more typing, and
> this is unit test code, not something exported by the net/ stack.
> 
> Slightly off-topic, it would be nice to have a header defining common
> declarations for scoped OpenSSL types that can be reused by several source
files
> (currently, each one has its own set of typedefs like the one above).  Do you
> find this idea wortwhile, and if so, do you have a suggestion for this header
> (net/base/x509_util.h maybe?). 

crypto/openssl_util -> crypto/scoped_openssl_types.h ?

If the types are not SSL-specific (eg: SSL/SSL_CTX), it may make more sense
there.

Alternatively, the benefit of explicitly avoiding "_util" headers is that to get
one type you're not forced to pull in 6 other unrelated headers.

> I don't intend to address this point in this
> patch, nor decide whether the declarations should use ScopedOpenSSL or
> scoped_ptr, but we're discussing a related issue here, so your input is
> welcomed.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:61: }
On 2013/02/14 06:23:50, digit1 wrote:
> On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> > This shouldn't be in a unittest file like this.
> 
> Can you clarify? This comes straight from
> net/socket/ssl_client_socket_unittest.cc btw.

Then we should refactor the logic in test_data_directory to hide the OS_ANDROID.
Repeating subtle logic in multiple files like this is a recipe for breakages, so
it'd be better to centralize that logic. Given that GetTestCertsDirectory and
GetTestCertsDirectoryRelative both live in test_data_directory, and given that
GetTestCertsDirectoryRelative is *only* called by ssl_client_socket*, perhaps it
should be GetTestClientCertsDirectory() and have the implementation in
test_data_directory do the #ifdef switch as appropriate.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:70: ScopedStdioHandle
file(file_util::OpenFile(filepath, "rb"));
On 2013/02/14 06:23:50, digit1 wrote:
> On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> > Use FileUtil::ReadFileToString, rather than a ScopedStdioHandle. Avoid the
> extra
> > dependency on FILE* glue.
> 
> I'm surprised by this request, given that last week you asked for _exactly_
the
> opposite (i.e. I was using file_util::ReadFileToString, but you asked me to
use
> PEM_read_PrivateKey(), which requires FILE* handles). Are you really sure you
> want this changed in the opposite direction?

I seem to recall that you were using the _BIO variant to open the file (that is,
not using ReadFileToString, but BIO_new_file/BIO_s_file)

For OpenSSL, we should be handling all File I/O using our API routines, reading
into a buffer, and supplying that directly (to the OpenSSL functions that accept
raw buffers - ie: most). For the API functions that *require* a BIO or FILE (the
PEM variants include this), use BIO_new_mem_buf(some_string.data(),
some_string.size()) to create a non-mutable, read-only BIO that wraps the
string.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:98: bool
CreateRandomPrivateKey(
On 2013/02/14 06:23:50, digit1 wrote:
> On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> > This is not something we want to do in tests, since it makes it
significantly
> > worse under ASAN/TSAN/Valgrind/any form of runtime inspection.
> > 
> I'm not sure to understand what you mean here. Can you clarify?

We don't want to generate random keys in tests. Every test where key generation
is performed is disabled under every one of our memory sanitizers, because
there's no way to efficiently do things like primality tests under the
additional checks.

> 
> > There's no need to generate a new key every run.
> This is the simplest way I found to generate an EVP_PKEY which had the
> interesting property of not being rejected by OpenSSL when used with a
> non-matching certificate. The tricky part is that this requires a custom
> RSA_METHOD, and functions like PEM_read_PrivateKey() cannot be used to load an
> EVP_PKEY with said custom method. The alternative is to implement a custom
> ENGINE to address this single point, which appears to be a _lot_ more work.

If that's the case (and I haven't looked closely at the OpenSSL code to see if
there's any workarounds), then given the constraint, wouldn't it be easier to
just make the server 'lie' about the key being invalid? Are you testing client
behaviour (that signatures are pairwise consistency checked?) or are you
checking server behaviour (that the server will reject the signature?) I didn't
think OpenSSL did any PCTs in non-FIPS mode for signatures, due to performance,
so I'd be very surprised if you were testing client behaviour here, so much as
checking that the client handles when the server *says* it was invalid.

> 
> But the point is moot, because the SendBadSignature test is going to be
removed,
> so let's not lose time over this anyway.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:157: }  // namespace
On 2013/02/14 06:23:50, digit1 wrote:
> On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> > Put the entire file into namespace net { namespace { } }.
> 
> This would result in a link error, because there are similarly named functions
> in net/socket/ssl_client_socket_unittest.cc (where they come from), which are
> already in the net:: namespace.

Not if you put them in the unnamed namespace, like I was suggesting. The point
was to eliminate the need to do net:: everywhere, while still preserving the
unnamed namespace.

> 
> I didn't want to move them to yet another common header/source file, so
putting
> them in an anonymous namespace seemed the simpler thing. The alternative is to
> rename the functions though. Are you really sure about this point?

Yes. Because I'm cleaning up ssl_client_socket_unittest tomorrow to move them
into net::, as this was something that I realized when I was adding a unittest
myself (see https://codereview.chromium.org/12025040/#msg1 ). I want to avoid
the mistakes in the new file :)

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:159:
//-----------------------------------------------------------------------------
On 2013/02/14 06:23:50, digit1 wrote:
> Again, this came from net/socket/ssl_client_socket_unittest.cc. I'll remove
them
> though.

Same cleanup I'm working on there, trying to avoid having to have that persist
in the new file.

Ryan Sleevi

https://codereview.chromium.org/12220104/diff/21041/net/base/cert_database_openssl.cc File net/base/cert_database_openssl.cc (right): https://codereview.chromium.org/12220104/diff/21041/net/base/cert_database_openssl.cc#newcode32 net/base/cert_database_openssl.cc:32: cert, &private_key)) Braces https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_socket_openssl_unittest.cc File net/socket/ssl_client_socket_openssl_unittest.cc (right): https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_socket_openssl_unittest.cc#newcode44 net/socket/ssl_client_socket_openssl_unittest.cc:44: ...

7 years, 10 months ago (2013-02-14 07:54:01 UTC) #19

digit1

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_key_store.h File net/base/openssl_private_key_store.h (right): https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_key_store.h#newcode57 net/base/openssl_private_key_store.h:57: }; On 2013/02/14 07:15:00, Ryan Sleevi wrote: > I'm ...

7 years, 10 months ago (2013-02-14 08:24:38 UTC) #20

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:57: };
On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> I'm not sure I understand your point about "not depend on changes to
> scoped_ptr". We've been collectively trying to reduce the custom-scoped-X
> classes, following a series of scoped_ptr<> cleanups to bring it into
alignment
> with unique_ptr<> in the spec.

My feeling is that changes to core classes and definitions in base/, that
potentially affect the whole codebase in subtle ways, tend to take a very large
time to land. What I meant is to avoid having to wait for scoped_ptr<> to really
be able to support function deleters natively, since it's not a core feature of
this patch.

 It's already on my radar to fix
> crypto/scoped_capi_types and crypto/scoped_nss_types. I'm particularly
sensitive
> to new introductions of generics and new scoped types, trying to make sure
we're
> not creating more work for us to fix later.
> 
> I think 1/ is the lesser of the two evils when only given those two options,
if
> only because we've been doing 1/ (unfortunately), but I think we should be
> working to entirely rid the codebase of ScopedOpenSSL in favour of using
> scoped_ptr<> consistently, which is why you're getting push back. This is the
> first CL touching the scoped_x types now that we have them unified, which is
why
> I'm saying "Hrm, we should clean this up while we're here."
> 
I'm not sure that fixing scoped_ptr<> itself is going to be easy, so I won't
attempt to do it here. I've recently uploaded a new patch that uses the
EVP_PKEY_Deleter approach, but I'll revert this to crypto::ScopedOpenSSL for the
reasons you outlined. It's better to only do that kind of cleanup once, but this
will happen in the future.

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:93: protected:
On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> I'm asking that you carefully evaluate what needs to be protected, and what
> can/should be private. As the style guide says, especially with datamembers,
> they should be private.
> 
> Ctor, Dtor, AddKeyPair is fine - if that's the minimal interface to expose.
> Exposing KeyPair, lock_, and pairs_ is problematic though.

That's exactly what I've already done, so I think we should be good here.
Thanks.

https://codereview.chromium.org/12220104/diff/22002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:123: base::Lock lock_;
On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> Please make sure to fully comment/explain the threading lifetime - perhaps on
> 24-26 - so that it's clear why this is locked.
> 
> You mentioned worker threads before - but that surprises me, as from looking
at
> the CL, it seems like it should only be the UI thread (on the activity
> completion) and on the IO thread (when performing handshakes). 

The worker threads are mentioned in the Android GetInstance() implementation. I
didn't write this code and don't know exactly what it refers to. Apart from
that, your understanding is correct that only the UI thread shall call
RecordClientCertPrivateKey(), all other functions should exclusively be called
from the I/O thread. That's why the initial proposal was to expose this method
through a single NET_EXPORT function, and keep the OpenSSLPrivateKeyStore
internal to net/


> In the activity
> completion model (in chrome/), it seems much more preferable to simply
PostTask
> to the BrowserThread::IO to do the modification, so that this object is *only*
> accessed on the IO thread, rather than to require this object to implement
> thread-safe locking.
> 
> The reasoning behind that is that it's easier to reason about the code when
it's
> single-threaded and has a defined lifetime.
> 
> The PostTask model is further simplified when using scoped_ptr<> with custom
> deleters, since then you can use base::Passed to pass the EVP_PKEY to the IO
> thread in a scoped_ptr<EVP_PKEY, EVP_PKEY_free>. Using a custom type, you
can't
> use base::Passed(), and you can't use base::Owned() with an custom-deleter.

I agree that this sounds a better alternative, but now I'm all confused about
the best implementation for ScopedEVP_PKEY. I'll have to perform some testing to
see if it works in practice though. base::Bind() compilation errors are
essentially undecipherable :)

https://codereview.chromium.org/12220104/diff/22002/net/data/ssl/certificates...
File net/data/ssl/certificates/client_1_root.pem (right):

https://codereview.chromium.org/12220104/diff/22002/net/data/ssl/certificates...
net/data/ssl/certificates/client_1_root.pem:17: -----END CERTIFICATE-----
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> openssl x509 -text -in client_1_root.pem -out client_1_root.pem

Done.

https://codereview.chromium.org/12220104/diff/22002/net/data/ssl/certificates...
File net/data/ssl/certificates/client_2_root.pem (right):

https://codereview.chromium.org/12220104/diff/22002/net/data/ssl/certificates...
net/data/ssl/certificates/client_2_root.pem:17: -----END CERTIFICATE-----
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> openssl x509 -text -in client_2_root.pem -out client_2_root.pem

Done.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:61: }
I see, I'll do that then.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:70: ScopedStdioHandle
file(file_util::OpenFile(filepath, "rb"));
On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> I seem to recall that you were using the _BIO variant to open the file (that
is,
> not using ReadFileToString, but BIO_new_file/BIO_s_file)
>
No, I was using a memory BIO to pass the data read from ReadFileToString(), just
like you recommend below. Found it at
https://chromiumcodereview.appspot.com/11571059/diff/51001/net/android/keysto...
 
> For OpenSSL, we should be handling all File I/O using our API routines,
reading
> into a buffer, and supplying that directly (to the OpenSSL functions that
accept
> raw buffers - ie: most). For the API functions that *require* a BIO or FILE
(the
> PEM variants include this), use BIO_new_mem_buf(some_string.data(),
> some_string.size()) to create a non-mutable, read-only BIO that wraps the
> string.

Sigh. Will do :(

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:98: bool
CreateRandomPrivateKey(
On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> We don't want to generate random keys in tests. Every test where key
generation
> is performed is disabled under every one of our memory sanitizers, because
> there's no way to efficiently do things like primality tests under the
> additional checks.
>
I see, I didn't realize that. Thanks for the clarification.
 
> > 
> > > There's no need to generate a new key every run.
> > This is the simplest way I found to generate an EVP_PKEY which had the
> > interesting property of not being rejected by OpenSSL when used with a
> > non-matching certificate. The tricky part is that this requires a custom
> > RSA_METHOD, and functions like PEM_read_PrivateKey() cannot be used to load
an
> > EVP_PKEY with said custom method. The alternative is to implement a custom
> > ENGINE to address this single point, which appears to be a _lot_ more work.
> 
> If that's the case (and I haven't looked closely at the OpenSSL code to see if
> there's any workarounds), then given the constraint, wouldn't it be easier to
> just make the server 'lie' about the key being invalid? Are you testing client
> behaviour (that signatures are pairwise consistency checked?) or are you
> checking server behaviour (that the server will reject the signature?) I
didn't
> think OpenSSL did any PCTs in non-FIPS mode for signatures, due to
performance,
> so I'd be very surprised if you were testing client behaviour here, so much as
> checking that the client handles when the server *says* it was invalid.
> 
I just wanted to have a test where the server would refuse the connection even
though a certificate is sent by the client socket. Normally SendBadCert would be
enough, but it is disabled because our version of tlslite doesn't verify client
certificate chains (it would accept _any_ client certificate, I looked at the
source code the check this).

SendBadSignature was just a hacky work-around for this issue. I don't think
there is a way to configure the server to lie about the key being invalid, and I
certainly don't want to have to send a patch upstream to add this "feature", or
fix the verification problem.

However, I don't see this issue critical to test SSLClientSocketOpenSSL, so I
prefer to drop this single test entirely.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:157: }  // namespace
turns out that using 'static' is enough to put the function safely in the net
namespace here.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:157: }  // namespace
On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> > > Put the entire file into namespace net { namespace { } }.
> >
Oh, I realize I misread your initial comment. will do.
  
> Yes. Because I'm cleaning up ssl_client_socket_unittest tomorrow to move them
> into net::, as this was something that I realized when I was adding a unittest
> myself (see https://codereview.chromium.org/12025040/#msg1 ). I want to avoid
> the mistakes in the new file :)

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:284:
//-----------------------------------------------------------------------------
On 2013/02/13 23:25:55, Ryan Sleevi wrote:
> Remove this

Done.

Ryan Sleevi

Will finish the review in a few hours. Sorry about the BIO mixup. https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_socket_openssl_unittest.cc File ...

7 years, 10 months ago (2013-02-14 08:51:15 UTC) #21

Will finish the review in a few hours. Sorry about the BIO mixup.

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:70: ScopedStdioHandle
file(file_util::OpenFile(filepath, "rb"));
On 2013/02/14 08:24:39, digit1 wrote:
> On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> > I seem to recall that you were using the _BIO variant to open the file (that
> is,
> > not using ReadFileToString, but BIO_new_file/BIO_s_file)
> >
> No, I was using a memory BIO to pass the data read from ReadFileToString(),
just
> like you recommend below. Found it at
>
https://chromiumcodereview.appspot.com/11571059/diff/51001/net/android/keysto...

Bah! I think at the time I wrote that comment I was thinking PEM_read_PrivateKey
was the char* based version 9that is, wrapping it in BIO_new_mem_buf was
overkill for a pure memory based).

Your original change was correct. Your reviewer was asleep at the wheel.
>  
> > For OpenSSL, we should be handling all File I/O using our API routines,
> reading
> > into a buffer, and supplying that directly (to the OpenSSL functions that
> accept
> > raw buffers - ie: most). For the API functions that *require* a BIO or FILE
> (the
> > PEM variants include this), use BIO_new_mem_buf(some_string.data(),
> > some_string.size()) to create a non-mutable, read-only BIO that wraps the
> > string.
> 
> Sigh. Will do :(
>

https://codereview.chromium.org/12220104/diff/22002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:98: bool
CreateRandomPrivateKey(
On 2013/02/14 08:24:39, digit1 wrote:
> On 2013/02/14 07:15:00, Ryan Sleevi wrote:
> > We don't want to generate random keys in tests. Every test where key
> generation
> > is performed is disabled under every one of our memory sanitizers, because
> > there's no way to efficiently do things like primality tests under the
> > additional checks.
> >
> I see, I didn't realize that. Thanks for the clarification.
>  
> > > 
> > > > There's no need to generate a new key every run.
> > > This is the simplest way I found to generate an EVP_PKEY which had the
> > > interesting property of not being rejected by OpenSSL when used with a
> > > non-matching certificate. The tricky part is that this requires a custom
> > > RSA_METHOD, and functions like PEM_read_PrivateKey() cannot be used to
load
> an
> > > EVP_PKEY with said custom method. The alternative is to implement a custom
> > > ENGINE to address this single point, which appears to be a _lot_ more
work.
> > 
> > If that's the case (and I haven't looked closely at the OpenSSL code to see
if
> > there's any workarounds), then given the constraint, wouldn't it be easier
to
> > just make the server 'lie' about the key being invalid? Are you testing
client
> > behaviour (that signatures are pairwise consistency checked?) or are you
> > checking server behaviour (that the server will reject the signature?) I
> didn't
> > think OpenSSL did any PCTs in non-FIPS mode for signatures, due to
> performance,
> > so I'd be very surprised if you were testing client behaviour here, so much
as
> > checking that the client handles when the server *says* it was invalid.
> > 
> I just wanted to have a test where the server would refuse the connection even
> though a certificate is sent by the client socket. Normally SendBadCert would
be
> enough, but it is disabled because our version of tlslite doesn't verify
client
> certificate chains (it would accept _any_ client certificate, I looked at the
> source code the check this).
> 
> SendBadSignature was just a hacky work-around for this issue. I don't think
> there is a way to configure the server to lie about the key being invalid, and
I
> certainly don't want to have to send a patch upstream to add this "feature",
or
> fix the verification problem.

We're effectively a forked version of TLSlite. I've been talking with Trevor
Perrin, the upstream maintainer, on getting us de-forked (the project wasn't
active for 6 years, so we 'were' effectively upstream for a while). So you can
add it to the Python code and be done.

That said, it's fine since you dropped it.

> 
> However, I don't see this issue critical to test SSLClientSocketOpenSSL, so I
> prefer to drop this single test entirely.
>

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:150: int rv =
transport_->Connect(callback_.callback());
int rv = callback.GetResult(transport_->Connect(callback_.callback());

This allows you to remove lines 151 & 152.

Same below ~lines 186/187 & 177.

ppi

This looks great! I remember the hard time I had reading the OpenSSL key stores ...

7 years, 10 months ago (2013-02-15 19:54:46 UTC) #22

Ryan Sleevi

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_memory_private_key_store.cc File net/base/openssl_memory_private_key_store.cc (right): https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_memory_private_key_store.cc#newcode20 net/base/openssl_memory_private_key_store.cc:20: // This is the linux_redux specific implementation of nit: ...

7 years, 10 months ago (2013-02-15 23:53:26 UTC) #23

digit1

https://codereview.chromium.org/12220104/diff/21041/net/base/cert_database_openssl.cc File net/base/cert_database_openssl.cc (right): https://codereview.chromium.org/12220104/diff/21041/net/base/cert_database_openssl.cc#newcode32 net/base/cert_database_openssl.cc:32: cert, &private_key)) On 2013/02/14 07:54:01, Ryan Sleevi wrote: > ...

7 years, 10 months ago (2013-02-25 14:26:22 UTC) #24

https://codereview.chromium.org/12220104/diff/21041/net/base/cert_database_op...
File net/base/cert_database_openssl.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/base/cert_database_op...
net/base/cert_database_openssl.cc:32: cert, &private_key))
On 2013/02/14 07:54:01, Ryan Sleevi wrote:
> Braces

Done.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_memory_p...
File net/base/openssl_memory_private_key_store.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_memory_p...
net/base/openssl_memory_private_key_store.cc:20: // This is the linux_redux
specific implementation of
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> nit: "linux_redux" is meaningless here. Try to explain exactly what it is
> 
> // OpenSSLMemoryKeyStore is an OpenSSL private key store that
> // does not make use of any Android APIs, making it suitable
> // for use when using OpenSSL on non-Android platforms (eg:
> // for testing purposes).

Done.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_memory_p...
net/base/openssl_memory_private_key_store.cc:22: class OpenSSLMemoryKeyStore :
public OpenSSLPrivateKeyStore {
I've renamed this to OpenSSLPrivateKeyStoreMemory, and renamed the source file
appropriately. Thanks.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
File net/base/openssl_private_key_store.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store.cc:17: typedef
OpenSSLPrivateKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;
Yes, I prefer to use it, and I've fixed line 31 accordinfly.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store.cc:46: //   error: [chromium-style] Complex
constructor has an inlined body.
I've removed it.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store.h:26: // provide it.
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> nit: delete the "The OpenSSL library ..." comment, because it's unclear why
you
> mention that purely in the context of this header file in isolation.
> 
> // OpenSSLPrivateKeyStore provides an interface for storing
> // or locating the associated private key for a given public
> // key. Because OpenSSL does not provide any notion of key
> // or certificate storage, unlike other platforms, this class
> // provides a basic interface to implement these services.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store.h:41: // visiting |url|, to an appropriate
secure system location.
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> Isn't the "appropriate secure system location" misleading - it's an
> implementation detail about both whether it's "secure" or "system".
> 
It must be stored on the system, this is specifically used to implement support
for the <keygen> attribute, and not storing the public/private key pair to the
system store would make this essentially useless.

Apart from that, this is not technically a blocking function on Android, because
all it does it send an intent with the relevant data and return immediately.

Said intent will always display a UI dialog, which will pause the main UI
thread, but this shall not impact the network thread itself. Also, there is
absolutely _no_ way to know if the operation succeeded or not (e.g. the user
could cancel the installation, or the system can reject the data as invalid).
The API is strictly fire-and-forget unfortunately. Also, trying to probe the
store after the fact to check that the key is installed would also force another
UI dialog, so can't be used. Of course this makes unit-testing impossible.

I have just modified this method to better document it and give it a more
meaningful name (StorePrivateKey() is  misleading). If you think this should be
moved elsewhere, I'd prefer you'd file a bug and this be achieved through a
different patch. I don't really think this method belongs here, but I'm not the
one who added it.

I've changed the comments here to better clarify the distinction.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store.h:99: // Returns the index of the keypair for
|public_key|. or -1 if not found.
Yep, I've fixed that, thanks for pointing this out.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
File net/base/openssl_private_key_store_android.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store_android.cc:21: class OpenSSLKeyStoreAndroid :
public OpenSSLPrivateKeyStore {
I've renamed the class.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store_android.cc:30: typedef
LeakySingletonTraits<OpenSSLKeyStoreAndroid>
With the move to single-threaded store implementation, a leaky singleton doesn't
seem necessary anymore, so I've changed this to a simple Singleton.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
File net/base/openssl_private_key_store_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store_unittest.cc:23: }
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> }  // namespace
> 
> You can and should *minimally* tend this namespace to over the fixture (eg: to
> line 40), but realistically, to the whole file.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store_unittest.cc:30: store_ =
net::OpenSSLPrivateKeyStore::GetInstance();
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> you do not need net::

Done.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store_unittest.cc:33: virtual void TearDown()
OVERRIDE {
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> Do not place OVERRIDE on testing:: overrides (SetUp/TearDown). Keep virtual.
> 
> That said, you can and should just use the constructor and destructor here.
> There's no need for this deferred initialization to SetUp/TearDown
> 
> OpenSSLPrivateKeyStoreTest()
>   : store_(OpenSSLPrivateKeyStore::GetInstance()) {
> }
> virtual ~OpenSSLPrivateKeyStoreTest() {
>   if (store_)
>     store_->Flush();
> }

Done.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store_unittest.cc:62: // Retrieve the private key
now. This shall fail.
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> s/now//
> s/shall/should/
> 
> // Retrieve the private key. This should fail because the
> // store was flushed.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_...
net/base/openssl_private_key_store_unittest.cc:77: // Retrieve the private key
now. This shall fail.
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> Same sort of rewording concerns as above (and throughout the rest of this
file).
> 
> It's more important to explain why/how it will fail, rather than "This shall
> fail" (which is clear from the use of ASSERT_FALSE that "this shall fail")

Done.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl.cc:591: // EVP_PKEY_free() on it when the
SSL object is destroyed.
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> // A note about ownership: FetchClientCertPrivateKey() increments
> // the reference count of the EVP_PKEY. Ownership of this
> // referenced is then passed directly to OpenSSL, which will
> // release the reference using EVP_PKEY_free() when the SSL
> // object is destroyed.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:44: typedef
net::OpenSSLPrivateKeyStore::ScopedEVP_PKEY ScopedEVP_PKEY;
On 2013/02/14 07:54:01, Ryan Sleevi wrote:
> When you do the namespace move suggested below, please make sure to remove all
> net:: from this file.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:51: namespace net {
On 2013/02/14 07:54:01, Ryan Sleevi wrote:
> Move this to line 41 - that is
> 
> namespace net {
> namespace {
> 
> As originally proposed. This will also eliminate the need to declare these
> functions static.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:89: // log is an SSL connect
end event. The NSS sockets will cork in an attempt to
This is no longer needed (see comment below), so has been removed.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:117: }
I've changed it to use SetUp/TearDown exclusively.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:137: }
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> This is an unnecessary check, at least following Chromium patterns. This
should
> only happen if OOM - and we intentionally don't check those conditions.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:150: int rv =
transport_->Connect(callback_.callback());
On 2013/02/14 08:51:15, Ryan Sleevi wrote:
> int rv = callback.GetResult(transport_->Connect(callback_.callback());
> 
> This allows you to remove lines 151 & 152.
> 
> Same below ~lines 186/187 & 177.

Good to know, thanks.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:166: bool
CreateAndConnectSSLClientSocket(net::SSLConfig& ssl_config,
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> comment this function.

Done.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:197: if
(!LogContainsSSLConnectEndEvent(entries, -1)) {
On 2013/02/15 23:53:26, Ryan Sleevi wrote:
> Are you sure this needs to be checked?

Not really, this came from ssl_client_socket_unittest.cc, I've removed it.

https://codereview.chromium.org/12220104/diff/21041/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_openssl_unittest.cc:298:
TEST_F(SSLClientSocketOpenSSLClientAuthTest, DISABLED_SendBadCert) {
Ok, I've removed this test.

Ryan Sleevi

Better, although the design of the key store still feels off. Comments bellow. https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_key_store_unittest.cc File ...

7 years, 10 months ago (2013-02-25 19:51:07 UTC) #25

digit1

https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_key_store_unittest.cc File net/base/openssl_private_key_store_unittest.cc (right): https://codereview.chromium.org/12220104/diff/21041/net/base/openssl_private_key_store_unittest.cc#newcode33 net/base/openssl_private_key_store_unittest.cc:33: virtual void TearDown() OVERRIDE { Can you clarify? I've ...

7 years, 10 months ago (2013-02-26 11:03:13 UTC) #26

Ryan Sleevi

Mostly nits, in the home stretch here. https://codereview.chromium.org/12220104/diff/38017/net/base/openssl_client_key_store.cc File net/base/openssl_client_key_store.cc (right): https://codereview.chromium.org/12220104/diff/38017/net/base/openssl_client_key_store.cc#newcode26 net/base/openssl_client_key_store.cc:26: if (key ...

7 years, 10 months ago (2013-02-27 01:08:20 UTC) #27

digit1

https://codereview.chromium.org/12220104/diff/38017/net/base/openssl_client_key_store.cc File net/base/openssl_client_key_store.cc (right): https://codereview.chromium.org/12220104/diff/38017/net/base/openssl_client_key_store.cc#newcode26 net/base/openssl_client_key_store.cc:26: if (key != NULL) On 2013/02/27 01:08:20, Ryan Sleevi ...

7 years, 9 months ago (2013-02-27 10:24:27 UTC) #28

Ryan Sleevi

LGTM with nits. However, if you find yourself changing any other aspects, please wait for ...

7 years, 9 months ago (2013-02-28 00:58:35 UTC) #29

digit1

https://codereview.chromium.org/12220104/diff/43002/net/base/cert_database_openssl.cc File net/base/cert_database_openssl.cc (left): https://codereview.chromium.org/12220104/diff/43002/net/base/cert_database_openssl.cc#oldcode32 net/base/cert_database_openssl.cc:32: return ERR_NO_PRIVATE_KEY_FOR_CERT; On 2013/02/28 00:58:35, Ryan Sleevi wrote: > ...

7 years, 9 months ago (2013-02-28 15:46:56 UTC) #30

https://codereview.chromium.org/12220104/diff/43002/net/base/cert_database_op...
File net/base/cert_database_openssl.cc (left):

https://codereview.chromium.org/12220104/diff/43002/net/base/cert_database_op...
net/base/cert_database_openssl.cc:32: return ERR_NO_PRIVATE_KEY_FOR_CERT;
On 2013/02/28 00:58:35, Ryan Sleevi wrote:
> Can you remind me where you plan to stick this check?
> 
> Can you add a comment here explaining where the 'real' meat lives, and why?

Yes, in short form:

- This method is never called on Android, because its only
  user, except on ChromeOS, is SSLAddCertHandler. For more
  details see chrome/browser/ssl/ssl_add_certificate.cc and
  ssl_add_certificate_android.cc

- For the Linux build, the store will always be empty.
  More precisely, OpenSSLPrivateKeyStore::StoreKeyPair(),
  which can populate it, is only used by
  KeygenHandler::GenKeyAndSignChallenge(), so the function
  would always return ERR_NO_PRIVATE_KEY_FOR_CERT, during
  unit testing, except if said test checks for full
  keygen handling (i.e. key pair generation + installation,
  then download of a server-generated client certificate
  which will trigger a SSLAddCertHandler).

  Another possibility is a unit test that directly calls
  StoreKeyPair() to "install" a test key pair in the
  private store, then triggers a fake certificate download
  through a SSLCertAddHandler. However, such a test would
  be Linux/OpenSSL-specific, and I'm unsure there is any
  real value in implementing it.

Anyway, I've restored the old implementation, changing FetchPrivateKey() to
HasPrivateKey() to make things a bit clearer.

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_client_k...
File net/base/openssl_client_key_store.cc (right):

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_client_k...
net/base/openssl_client_key_store.cc:68: void
OpenSSLClientKeyStore::KeyPair::operator =(const KeyPair& other) {
Ah, sorry about that. Fixed.

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_client_k...
net/base/openssl_client_key_store.cc:135: return
Singleton<OpenSSLClientKeyStore>::get();
On 2013/02/28 00:58:35, Ryan Sleevi wrote:
> style: indent

Done.

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_private_...
File net/base/openssl_private_key_store.h (right):

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:15: #include "base/synchronization/lock.h"
That's correct, they're no longer needed, I've fixed this.

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_private_...
net/base/openssl_private_key_store.h:36: // FetchClientCertPrivateKey() below.
On 2013/02/28 00:58:35, Ryan Sleevi wrote:
> Comment needs updating.

Done.

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_private_...
File net/base/openssl_private_key_store_android.cc (right):

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_private_...
net/base/openssl_private_key_store_android.cc:32: PKCS8_PRIV_KEY_INFO_free>
pkcs8(EVP_PKEY2PKCS8(pkey));
On 2013/02/28 00:58:35, Ryan Sleevi wrote:
> style:
> crypto::ScopedOpenSSL<PKCS8_PRIV_KEY_INFO,
>                       PKCS_PRIV_KEY_INFO_FREE> pkcs8(EVP_PKEY2PKCS8(pkey));
> 
> or
> 
> crypto::ScopedOpenSSL<
>     PKCS8_PRIV_KEY_INFO, PKCS8_PRIV_KEY_INFO_FREE>
pkcs8(EVP_PKEY2PKCS8(pkey));
> 
> or
> 
> crypto::ScopedOpenSSL<
>     PKCS8_PRIV_KEY_INFO,
>     PKCS8_PRIV_KEY_INFO_FREE> pkcs8(EVP_PKEY2PKCS8(pkey));
> 
> depending on which correctly fits under 80. The point is that
> PKCS8_PRIV_KEY_INFO should not be dangling on the first line if you're going
to
> have to four-space wrap.

Done.

https://codereview.chromium.org/12220104/diff/43002/net/base/openssl_private_...
net/base/openssl_private_key_store_android.cc:40: static_cast<const
uint8*>(private_key), private_len);
On 2013/02/28 00:58:35, Ryan Sleevi wrote:
> style: indents - four spaces here.

Done.

wtc

Review comments on patch set 11: Many of the files are missing when I view ...

7 years, 9 months ago (2013-02-28 17:24:24 UTC) #31

digit1

https://codereview.chromium.org/12220104/diff/49001/net/base/openssl_client_key_store.h File net/base/openssl_client_key_store.h (right): https://codereview.chromium.org/12220104/diff/49001/net/base/openssl_client_key_store.h#newcode23 net/base/openssl_client_key_store.h:23: // certificate private keys. Because the platforms where OpenSSL ...

7 years, 9 months ago (2013-03-02 17:55:58 UTC) #32

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/12220104/47006

7 years, 9 months ago (2013-03-02 22:13:34 UTC) #33

Message was sent while issue was closed.

Change committed as 185785

Issue 12220104: Wire up SSL client authentication for OpenSSL/Android through the net/ stack (Closed)

Description

Patch Set 1 #

Patch Set 2 : simplify + add client authentication unit test (net) #

Patch Set 3 : un-inline constructors to appease Clang #

Patch Set 4 : Fix net/net.gyp typo #

Patch Set 5 : remove chrome/browser changes + fix linux_redux build #

Patch Set 6 : address recent nits #

Patch Set 7 : #

Patch Set 8 : Separate OpenSSLClientKeyStore from OpenSSLPrivateKeyStore #

Patch Set 9 : Fix linux_redux build. #

Patch Set 10 : address nits #

Patch Set 11 : restore in-memory public/private in-memory store for Linux/OpenSSL build. #

Patch Set 12 : git cl try #

Messages