Implement an openssl version of a QUIC ProofSource.

net/quic/crypto/proof_source_chromium_openssl.cc

+// Copyright 2013 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.
+
+#include "net/quic/crypto/proof_source_chromium.h"
+
+#include "base/strings/string_number_conversions.h"
+#include "crypto/openssl_util.h"
+#include "net/quic/crypto/crypto_protocol.h"
+#include "net/ssl/scoped_openssl_types.h"
+
+using std::string;
+using std::vector;
+
+namespace net {
+
+ProofSourceChromium::ProofSourceChromium() {}
+
+ProofSourceChromium::~ProofSourceChromium() {}
+
+bool ProofSourceChromium::Initialize(const base::FilePath& cert_path,
+                                     const base::FilePath& key_path) {
+  crypto::EnsureOpenSSLInit();
+
+  std::string cert_data;
+  if (!base::ReadFileToString(cert_path, &cert_data)) {
+    DLOG(FATAL) << "Unable to read certificates";
+    return false;
+  }
+
+  CertificateList certs_in_file =
+      X509Certificate::CreateCertificateListFromBytes(
+          cert_data.data(), cert_data.size(), X509Certificate::FORMAT_AUTO);
+
+  if (certs_in_file.empty()) {
+    DLOG(FATAL) << "No certificates";
+    return false;
+  }
+
+  for (const scoped_refptr<X509Certificate>& cert : certs_in_file) {
+    std::string der_encoded_cert;
+    X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_encoded_cert);

[ramant (doing other things), 2015/08/24 22:31:30]

overly nit: should we consider checking the return value from
X509Certificate::GetDEREncoded? Should we return false if it returns false?

[Ryan Hamilton, 2015/08/24 23:29:55]

Done.

+    certificates_.push_back(der_encoded_cert);
+  }
+
+  std::string key_data;
+  if (!base::ReadFileToString(key_path, &key_data)) {
+    DLOG(FATAL) << "Unable to read key";
+    return false;
+  }
+
+  const uint8_t* p = reinterpret_cast<const uint8_t*>(key_data.data());
+  std::vector<uint8_t> input(p, p + key_data.size());
+  private_key_.reset(crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));

[ramant (doing other things), 2015/08/24 22:31:30]

overly nit:
Consider adding after DCHECK (ala gigest check at line#69). Your call.
  if (!private_key_.get()) {
    return false;
  }
 
Guessing GetProof() will fail if private_key is NULL, thus it is ok to return
true even if private_key_.get() is NULL (in release mode).

[Ryan Hamilton, 2015/08/24 23:29:55]

Done.

+  DCHECK(private_key_.get()) << " this: " << this;
+  return true;
+}
+
+bool ProofSourceChromium::GetProof(const IPAddressNumber& server_ip,
+                                   const string& hostname,
+                                   const string& server_config,
+                                   bool ecdsa_ok,
+                                   const vector<string>** out_certs,
+                                   string* out_signature) {
+  DCHECK(private_key_.get()) << " this: " << this;
+
+  crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
+  const EVP_MD* const digest = EVP_sha256();
+  DCHECK(digest);
+  if (!digest) {

[davidben, 2015/08/24 22:50:24]

Doesn't happen.

[Ryan Hamilton, 2015/08/24 23:29:55]

Done.

+    return false;
+  }
+  crypto::ScopedEVP_MD_CTX sign_context(EVP_MD_CTX_create());
+  EVP_PKEY_CTX* pkey_ctx;
+  if (!EVP_DigestSignInit(sign_context.get(), &pkey_ctx, digest, NULL,
+                          private_key_->key())) {

[davidben, 2015/08/24 22:50:24]

If I were you, I'd just chain all these with ||s. I'm not sure the individual
distinct LOGs are that valuable. EVP_DigestSignInit and EVP_DigestSignFinal can
only fail on malloc failure and the others only fail on programmer error or
never.

Otherwise, yeah, this is way more of a pain than SignatureCreator. :-)

crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);

crypto::ScopedEVP_MD_CTX sign_context(EVP_MD_CTX_create());
EVP_PKEY_CTX* pkey_ctx;
size_t len;
if (!EVP_DigestSignInit(sign_context.get(), &pkey_ctx, EVP_sha256(), nullptr,
                        private_key->key()) ||
    !EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING) ||
    // -1 sets the salt length to the digest length.
    !EVP_PKEY_CTX_set_rsa_pss_saltlen(pkey_ctx, -1) ||
    !EVP_DigestSignUpdate(...) ||
    !EVP_DigestSignUpdate(...) ||
    !EVP_DigestSignFinal(sign_context.get(), nullptr, &len)) {
  return false;
}
std::vector<uint8_t> signature(len);
if (!EVP_DigestSignFinal(sign_context.get(), vector_as_array(&signature),
                         &len)) {
  return false;
}
signature.resize(len);

(Though BoringSSL style is to very aggressively chain things with ||s like that.
For the sake of defining len closing to its first use, you could argue that
EVP_DigestSignFinal should be in its own thing so len can be above it, I dunno.)

[Ryan Hamilton, 2015/08/24 23:29:55]

Ah, great, thanks! I think I did what you proposed. Your comment about keeping
len near where it was used resonated with me, so I chose to break out the
SignFinal calls.

+    return false;
+  }
+  if (!EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING)) {

[ramant (doing other things), 2015/08/24 22:31:30]

overly nit: 1 != EVP_PKEY_CTX_set_rsa_padding ala internal code. Not necessary
to make the change though.

[davidben, 2015/08/24 22:50:24]

It's an OpenSSL vs BoringSSL thing. If this code will only be used in Chromium,
we can assume BoringSSL where this is actually a boolean. OpenSSL is obnoxious
and will return -1, 0, 1. Sometimes it returns -2 as well if it really hates
you.

[Ryan Hamilton, 2015/08/24 23:29:55]

Ah, actually davidben said that boringssl has fixed the need to do != 1.

+    LOG(FATAL) << "EVP_PKEY_CTX_set_rsa_padding";
+    return false;
+  }
+  // -1 sets the salt length to the digest length.
+  if (1 != EVP_PKEY_CTX_set_rsa_pss_saltlen(pkey_ctx, -1)) {

[davidben, 2015/08/24 22:50:24]

This can be ! as well.

[Ryan Hamilton, 2015/08/24 23:29:55]

Whoops, done!

+    LOG(FATAL) << "EVP_PKEY_CTX_set_rsa_pss_saltlen";
+    return false;
+  }
+
+  if (!EVP_DigestSignUpdate(sign_context.get(), reinterpret_cast<const uint8*>(
+                                                    kProofSignatureLabel),
+                            sizeof(kProofSignatureLabel))) {
+    DLOG(ERROR) << "Unable to sign lable.";

[ramant (doing other things), 2015/08/24 22:31:30]

overly nit: lable -> label?

[Ryan Hamilton, 2015/08/24 23:29:55]

Done.

+    return false;
+  }
+
+  if (!EVP_DigestSignUpdate(sign_context.get(), reinterpret_cast<const uint8*>(
+                                                    server_config.data()),
+                            server_config.size())) {
+    DLOG(ERROR) << "Unable to sign server config.";
+    return false;
+  }
+
+  // Determine the maximum length of the signature.
+  size_t len = 0;
+  if (!EVP_DigestSignFinal(sign_context.get(), NULL, &len)) {
+    DLOG(ERROR) << "Unable to finalize signature.";
+    return false;
+  }
+  std::vector<uint8> signature(len);

[davidben, 2015/08/24 22:50:24]

Nit: uint8_t

[Ryan Hamilton, 2015/08/24 23:29:55]

Done.

+  // Sign it.
+  if (!EVP_DigestSignFinal(sign_context.get(), vector_as_array(&signature),
+                           &len)) {
+    return false;
+  }

[davidben, 2015/08/24 22:50:24]

This should have a:
  signature.resize(len);

EVP_DigestSignFinal only gives you the maximum size of the signaure. Notably for
ECDSA, it may be a little smaller.

[Ryan Hamilton, 2015/08/24 23:29:55]

Done.

+
+  out_signature->assign(reinterpret_cast<const char*>(&signature[0]),
+                        signature.size());
+  *out_certs = &certificates_;
+  VLOG(1) << "signature: "
+          << base::HexEncode(out_signature->data(), out_signature->size());
+  return true;
+}
+
+}  // namespace net