Sign CertificateVerify messages on a background thread.

net/ssl/ssl_private_key.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_SSL_SSL_PRIVATE_KEY_H_
+#define NET_SSL_SSL_PRIVATE_KEY_H_
+
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/string_piece.h"
+#include "net/base/net_errors.h"
+
+namespace net {
+
+// An interface for a private key for use with SSL client authentication.
+class SSLPrivateKey {

[Ryan Sleevi, 2015/06/12 23:37:20]

It's unclear why you split this and the threaded implementation apart? Could you
explain more the thinking/reasoning behind that? It may be fine, but it just
seems like non-trivial abstraction.

[davidben, 2015/06/15 21:28:25]

SSLClientSocketOpenSSL doesn't care that the operation is on a background
thread, just that it's asynchronous. I'm assuming the CHAPS code would
eventually just talk to chapsd and we'd have no need to burn a thread for it.

+ public:
+  using SignCallback = base::Callback<void(Error, const std::vector<uint8_t>&)>;

[Ryan Sleevi, 2015/06/12 23:37:20]

Blergh; I generally hate typedefs like these; I find it makes things less
readable rather than more. It's not strictly needed, is it?

[davidben, 2015/06/15 21:28:25]

It's used a medium-ish amount of times between this header and it being
mentioned a couple more times in threaded_ssl_private_key.{cc,h}. But the full
callback is actually kinda long, so we have to wrap things awkwardly:

  void SignDigest(
      Hash hash,
      const base::StringPiece& input,
      const base::Callback<void(Error, const std::vector<uint8_t>&)>& callback)
      override;

  virtual void SignDigest(
      Hash hash,
      const base::StringPiece& input,
      const base::Callback<void(Error, const std::vector<uint8_t>&)>&
          callback) = 0;

I think I mildly prefer having the callback name. There's only the one
operation, so I don't think it'd be that confusing. But I don't insist on it.

[Ryan Sleevi, 2015/06/15 22:35:27]

I guess it depends on whether or not you're using the enum class for the ability
to forward declare (based on your previous reply to forward declarations, I
suspect it's not likely, in which case, an enum class isn't terribly useful but
I'm ambivalent). The typedef would also prevent any form of forward declaration

[davidben, 2015/06/15 22:41:20]

Hrm? I'm not sure I follow. Is there any consumer of SignedCallback that doesn't
also implement SSLPrivateKey? For those, we need to include ssl_private_key.h
regardless.

(I used enum class for the static checking on switch-case. That didn't really
work, but I figured the namespacing and losing implicit conversions was
marginally better. And then you pointed out the MSVC thing, so now we have the
static checking too.)

The problematic enum in ssl_client_socket_openssl.h is enum
ssl_private_key_result_t which can't be an enum class because that's from C.

[Ryan Sleevi, 2015/06/15 23:02:06]

Well, strictly speaking, callbacks are meant to break the dependency graph of
needing to explicitly know some type (commonly, the Delegate* foo pattern). A
callback can be specified in the form of a general signature, and then
base::Bind() does the type-erasure for it.

I guess the previous comment wasn't clearer that this is "OK here, for several
reasons, but generally an anti-pattern" (as somewhat discussed in the chat).
You don't need the enum class for that though. That's provided by the compiler
regardless for enums.

+
+  enum class Type {
+    RSA,
+    ECDSA,
+  };
+
+  enum class Hash {
+    MD5_SHA1,
+    MD5,
+    SHA1,
+    SHA224,
+    SHA256,
+    SHA384,
+    SHA512,
+  };
+
+  SSLPrivateKey() {}
+  virtual ~SSLPrivateKey() {}
+
+  // Returns whether the key is an RSA key or an ECDSA key.
+  virtual Type GetType() = 0;

[Ryan Sleevi, 2015/06/12 23:37:20]

:/

This seems to violate
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Run-Time_Type...
and turns this into a form of poor-man's RTTI. Having only looked at this
interface first, I'm not sure what concrete suggestions to make.

[davidben, 2015/06/15 21:28:24]

Per out-of-band discussion, added a comment to GetType explaining why this is
needed.

+
+  // Returns true if the key supports signing hashes of type |hash|.
+  virtual bool SupportsHash(Hash hash) = 0;
+
+  // Returns the maximum size of a signature. For an RSA key, this must be the
+  // size of the modulus in bytes.
+  virtual size_t GetMaxSignatureLength() = 0;

[Ryan Sleevi, 2015/06/12 23:37:20]

I suppose this API works because we assume asymetric keys will always go up to
at least the key size (possibly +1) ?

[davidben, 2015/06/15 21:28:25]

I'm not sure what you mean. It's the caller's job to figure out the maximum.
ECDSA keys are NOT sized up to the key size (which isn't really a well-defined
notion) and rather the maximum size of some DER thing.

OpenSSL generally expects to size buffers itself and wants to know the size
synchronously. I only need an upper bound that's tight enough to size a buffer
sanely.

+
+  // Asynchronously signs an |input| which was computed with the hash |hash|. On
+  // completion, it calls |callback| with the signature or an error code if the
+  // operation failed. For an RSA key, the signature is a PKCS#1 signature. The
+  // SSLPrivateKey implementation is responsible for prepending the DigestInfo
+  // prefix and adding PKCS#1 padding.
+  virtual void SignDigest(Hash hash,
+                          const base::StringPiece& input,
+                          const SignCallback& callback) = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SSLPrivateKey);
+};
+
+}  // namespace net
+
+#endif  // NET_SSL_SSL_PRIVATE_KEY_H_