Add net/android/keystore.h

net/android/java/src/org/chromium/net/AndroidKeyStore.java

Index: net/android/java/src/org/chromium/net/AndroidKeyStore.java
diff --git a/net/android/java/src/org/chromium/net/AndroidKeyStore.java b/net/android/java/src/org/chromium/net/AndroidKeyStore.java
new file mode 100644
index 0000000000000000000000000000000000000000..d4c2ba6ba904b83acf8118e6738338345bddc39d
--- /dev/null
+++ b/net/android/java/src/org/chromium/net/AndroidKeyStore.java
@@ -0,0 +1,126 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[palmer, 2013/01/26 02:14:20]

NIT: 2013. :)

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.net;
+
+import android.util.Log;
+
+import java.security.interfaces.DSAPrivateKey;
+import java.security.interfaces.ECPrivateKey;
+import java.security.interfaces.RSAPrivateKey;
+import java.security.PrivateKey;
+import java.security.Signature;
+
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+import org.chromium.net.SSLClientCertType;
+
+@JNINamespace("net::android")
+public class AndroidKeyStore {
+
+    private static final String TAG = AndroidKeyStore.class.getName();
+
+    ////////////////////////////////////////////////////////////////////
+    //
+    // Message signing support.
+    //
+
+    /**
+     * Called from native code to sign a given message with an RSA-based
+     * PrivateKey object.

[Ryan Sleevi, 2013/01/26 01:51:57]

Comment is wrong. It's not just RSA keys you're handling

+     * Assumes that initSignatureForKey() was called previously on the

[palmer, 2013/01/26 02:14:20]

Eliminate the need for this assumption by having this function call
initSignatureForKey (if necessary; i.e. if sFooSignature is null), and then you
can make initSignatureForKey private and not bother callers with needing to know
about it.

[digit1, 2013/01/28 10:16:30]

I should have documented this better when refactoring this code. It was more
obvious in previous versions of the patch which included the client certificate
request through an AsyncTask.

The main issue is that Signature.getInstance() is a potentially blocking
operation (at least on vanilla Java, it can touch on-disk databases the first
time it is called, due to the way the crypto API works), so the plan is for
initSignatureForKey() to be called in a background thread (the same one that
called  KeyStore.getPrivateKey(), which can also block). Note that the function
is only used to perform lazy initialization of a static global variable.

signWithPrivateKey() itself will always be called from the I/O thread,
technically it needs to be synchronized with initSignatureForKey() to avoid
memory-ordering issues on multi-core devices (the object will always be created
before signWithPrivateKey() is called, but without synchronization of explicit
memory barriers, the I/O thread might see the sRsaSignature reference before it
observes the full initialization of the object). There are no real concerns
about synchronizing several concurrent signWithPrivateKey() however.

I'll have a look on the Android implementation to see if they have the same
problem. If it can be guaranteed that the function never blocks, it's probably
ok to call it directly instead. In the meantime, I'll update the comments.

+     * same privateKey handle.
+     * @param privateKey The PrivateKey handle.
+     * @param message The message to sign.
+     * @return signature as a byte buffer.
+     */
+    @CalledByNative
+    public static byte[] signWithPrivateKey(PrivateKey privateKey,
+                                            byte[] message) {
+        synchronized (TAG) {

[Ryan Sleevi, 2013/01/26 01:51:57]

Performing the signing entirely in a synchronized section? o_O Is it necessary,
or is it just because you're relying on
s/RsaSignature/sDsaSignature/sEcdsaSignature?

Can you not exit the synchronized section once you've acquired the reference?

[palmer, 2013/01/26 02:14:20]

Seems odd to synchronize on an unrelated object; why not |this|? Or, why not
declare the method synchronized? (Since the whole body already is.)

And, yes, I agree with sleevi; it'd be nice not to need to synchronize this
method. Maybe only initSignatureForKey needs to be synchronized, and once it has
been called for our signature type, we can assume our signature object will
never become null. Right? Since this is a lazy allocation of singletons thing,
and not a constantly-updating-these-fields thing.

[digit1, 2013/01/28 10:16:30]

This is a static method, using |this| cannot work.
Moreover, both signWithPrivateKey() and initSignatureForPrivateKey() use the
same mutable static members, and thus need to use the same lock. That's why a
sycnrhonized function cannot work here.
Nope, because it's initialized in a different thread, we need some way to
enforce memory ordering. The simplest is to use synchronization. An alternative
is to use volatile variables and be very cautious, but that's always 

Very frankly, the synchronization here doesn't impact performance in any
meaningful way (compared to signing), since all signing will happen in the same
thread anyway (i.e. you won't run this in parallel).

 Since this is a lazy allocation of singletons thing,
Technically, the state of the static members is modified in each
signWithPrivateKey() invokation. I.e. this is not lazy-initialization of
immutable objects.

[digit1, 2013/01/28 10:16:30]

See comment above, it's because initSignatureForPrivateKey() will be called from
a different thread.
The method modifies the state of the static Signature objects. Technically, it
is possible to do that out of the synchronized block because we know that this
function will only be used from the I/O thread (i.e. it will never run in
parallel). However, this wouldn't change anything about its performance, and
this introduces a potential bug later if this assumption changes in the future,
so I'm not really sure it's such a great idea.

The radical "fix" would be to remove initSignatureForPrivateKey() entirely, i.e.
consider that blocking the first time Signature.getInstance() is called is
simply ok,  or verifying with the platform that this will never happen
otherwise. I'd be interested in your opinion on this.

+            // Get the Signature singleton for this key.
+            Signature signature = null;
+            if (privateKey instanceof RSAPrivateKey) {
+                signature = sRsaSignature;
+            } else if (privateKey instanceof DSAPrivateKey) {
+                signature = sDsaSignature;
+            } else if (privateKey instanceof ECPrivateKey) {
+                signature = sEcdsaSignature;
+            }
+            if (signature == null) {
+                Log.e(TAG, "Unsupported private key algorithm: " + privateKey.getAlgorithm());
+                return null;
+            }
+
+            // Sign the message.
+            try {
+                signature.initSign(privateKey);
+                signature.update(message);
+                return signature.sign();
+            } catch (Exception e) {
+                Log.e(TAG, "Exception while signing message with " + privateKey.getAlgorithm() +
+                           " private key: " + e);
+                return null;
+            }
+        }
+    }
+
+    /**
+     * Called from native code to return the type of a given PrivateKey
+     * object. This is an integer that maps to one of the values defined
+     * by org.chromium.net.SSLClientCertType, which is itself
+     * auto-generated from net/base/ssl_client_cert_type_list.h
+     * @param privateKey The PrivateKey handle
+     * @return key type, or SSLClientCertType.INVALID_TYPE if unknown.
+     */
+    @CalledByNative
+    public static int getPrivateKeySigningType(PrivateKey privateKey) {
+        if (privateKey instanceof RSAPrivateKey)
+            return SSLClientCertType.RSA_SIGN;
+        if (privateKey instanceof DSAPrivateKey)
+            return SSLClientCertType.DSS_SIGN;
+        if (privateKey instanceof ECPrivateKey)
+            return SSLClientCertType.ECDSA_SIGN;
+        else
+            return SSLClientCertType.INVALID_TYPE;
+    }
+
+    // Single signature instances. Used to perform signing with private
+    // keys. To avoid increasing Chromium's startup time, these are
+    // created lazily by calling initSignatureForKey below.
+    private static Signature sRsaSignature;
+    private static Signature sDsaSignature;
+    private static Signature sEcdsaSignature;
+
+    /**
+     * Called to ensure that the global Signature object corresponding
+     * to a given private key is initialized before a call to signWithPrivateKey.
+     * Note that this is a potentially blocking operation.
+     * @param key A PrivateKey handle.
+     */
+    public static void initSignatureForKey(PrivateKey key) {
+        String algorithm = key.getAlgorithm();
+        synchronized (TAG) {

[palmer, 2013/01/26 02:14:20]

Same comment as above.

+            try {
+                // Hint: Algorithm names come from:
+                // http://docs.oracle.com/javase/6/docs/technotes/guides/security/StandardNames.html
+                if (algorithm.equals("RSA") && sRsaSignature == null) {
+                    // IMPORTANT: Due to what looks like a platform bug, this will
+                    // throw NoSuchAlgorithmException on Android 4.0.x and 4.1.x, Fixed in 4.2

[palmer, 2013/01/26 02:14:20]

NIT: End with period, not comma.

+                    // and higher. See https://android-review.googlesource.com/#/c/40352/

[palmer, 2013/01/26 02:14:20]

Anonymous internet users can't read this; that would be OK except this file is
open source. Refer instead to a public bug.

[digit1, 2013/01/28 10:16:30]

Really? This is a link to the public AOSP gerrit instance. It works pretty well
in an incognito window on Chrome and Firefox, so seems definitely visible. There
is no public bug for this issue (though the linked page gives the number of the
internal buganizer entry for it).

+                    sRsaSignature = Signature.getInstance("NONEwithRSA");
+                } else if (algorithm.equals("DSA") && sDsaSignature == null) {
+                    sDsaSignature = Signature.getInstance("NONEwithDSA");
+                } else if (algorithm.equals("EC") && sEcdsaSignature == null) {
+                    // The documentation mentions that NONEwithECDSA is ambiguous
+                    // and that SHA1withECDSA, its synonym, should be used instead.

[Ryan Sleevi, 2013/01/26 01:51:57]

No, you misread. It's talking about the algorithm name "ECDSA", not
"NONEwithECDSA". You can just remove this comment.

[digit1, 2013/01/28 10:16:30]

Oh right, thanks, I'll remove this comment.

+                    sEcdsaSignature = Signature.getInstance("NONEwithECDSA");
+                }

[palmer, 2013/01/26 02:14:20]

Should there be an else clause that warns the caller they asked for something
that doesn't exist?

[digit1, 2013/01/28 10:16:30]

This is later handled in signWithPrivateKey().

+            } catch (Exception e) {
+                Log.w(TAG, "Could not create " + algorithm + " Signature singleton:" + e);
+            }
+        }
+    }
+
+}