Implement an openssl version of a QUIC ProofSource.

Implement an openssl version of a QUIC ProofSource.

BUG=525144, 514472
Committed: https://crrev.com/9f6d72eecc98aec5c085b0e995413182bf3718a9
Cr-Commit-Position: refs/heads/master@{#347831}

[Ryan Hamilton, 2015-08-24 21:39:18]

rch@chromium.org changed reviewers:
+ davidben@chromium.org, rtenneti@chromium.org

[Ryan Hamilton, 2015-08-24 21:39:18]

davidben: Ok, here's my attempt to do an OpenSSL (boringSSL?) only version of a
QUIC ProofSource. I'm basically cutting/pasting from SignatureCreator, but it
seems to work :>
rtenneti: Can you look at the QUIC changes and see if they look plausible to
you?

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

lgtm

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
File net/quic/crypto/proof_source_chromium_openssl.cc (right):

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:42:
X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_encoded_cert);
overly nit: should we consider checking the return value from
X509Certificate::GetDEREncoded? Should we return false if it returns false?

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:54:
private_key_.reset(crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
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).

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:79: if
(!EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING)) {
overly nit: 1 != EVP_PKEY_CTX_set_rsa_padding ala internal code. Not necessary
to make the change though.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:92: DLOG(ERROR) << "Unable to
sign lable.";
overly nit: lable -> label?

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

Here's an initial set of comments. I haven't looked at it too careful yet,
though I'm probably mostly relevant for the BoringSSL expertise.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
File net/quic/crypto/proof_source_chromium_openssl.cc (right):

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:70: if (!digest) {
Doesn't happen.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:76: private_key_->key())) {
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.)

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:79: if
(!EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING)) {
On 2015/08/24 22:31:30, ramant wrote:
> overly nit: 1 != EVP_PKEY_CTX_set_rsa_padding ala internal code. Not necessary
> to make the change though.

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.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:84: if (1 !=
EVP_PKEY_CTX_set_rsa_pss_saltlen(pkey_ctx, -1)) {
This can be ! as well.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:109: std::vector<uint8>
signature(len);
Nit: uint8_t

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:114: }
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]

Thanks for the comments!

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
File net/quic/crypto/proof_source_chromium_openssl.cc (right):

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:42:
X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_encoded_cert);
On 2015/08/24 22:31:30, ramant wrote:
> overly nit: should we consider checking the return value from
> X509Certificate::GetDEREncoded? Should we return false if it returns false?

Done.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:54:
private_key_.reset(crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
On 2015/08/24 22:31:30, ramant wrote:
> 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).

Done.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:70: if (!digest) {
On 2015/08/24 22:50:24, David Benjamin wrote:
> Doesn't happen.

Done.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:76: private_key_->key())) {
On 2015/08/24 22:50:24, David Benjamin wrote:
> 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.)

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.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:79: if
(!EVP_PKEY_CTX_set_rsa_padding(pkey_ctx, RSA_PKCS1_PSS_PADDING)) {
On 2015/08/24 22:31:30, ramant wrote:
> overly nit: 1 != EVP_PKEY_CTX_set_rsa_padding ala internal code. Not necessary
> to make the change though.

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

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:84: if (1 !=
EVP_PKEY_CTX_set_rsa_pss_saltlen(pkey_ctx, -1)) {
On 2015/08/24 22:50:24, David Benjamin wrote:
> This can be ! as well.

Whoops, done!

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:92: DLOG(ERROR) << "Unable to
sign lable.";
On 2015/08/24 22:31:30, ramant wrote:
> overly nit: lable -> label?

Done.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:109: std::vector<uint8>
signature(len);
On 2015/08/24 22:50:24, David Benjamin wrote:
> Nit: uint8_t

Done.

https://codereview.chromium.org/1309813003/diff/20001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:114: }
On 2015/08/24 22:50:24, David Benjamin wrote:
> 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.

Done.

[ramant (doing other things), 2015-08-24 23:32:25]

Thanks for fixing the long a TODO thing.

LGTM.

[davidben, 2015-08-25 19:33:03]

lgtm

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
File net/quic/crypto/proof_source_chromium_openssl.cc (right):

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:6: 
This should include:

#include <openssl/digest.h>
#include <openssl/evp.h>
#include <openssl/rsa.h>

(For EVP_sha256, EVP_DigestSign*, and RSA_PKCS1_PSS_PADDING, respectively.)

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:75: DCHECK(digest);
Nit: I wouldn't even bother with that. :-) This function is literally:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...

You can just inline the call or something.

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:78: if
(!EVP_DigestSignInit(sign_context.get(), &pkey_ctx, digest, NULL,
Nit: nullptr

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:93: if
(!EVP_DigestSignFinal(sign_context.get(), NULL, &len)) {
Nit: nullptr

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:94: DLOG(ERROR) << "Unable to
finalize signature.";
Nit: I wouldn't bother. :-) *shrug*

https://codereview.chromium.org/1309813003/diff/60001/net/quic/test_tools/cry...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/1309813003/diff/60001/net/quic/test_tools/cry...
net/quic/test_tools/crypto_test_utils_chromium.cc:129: cert_verifier, new
TransportSecurityState, "quic_root.crt");
[Not for this CL, but TestProofVerifierChromium should probably take its
CertVerifier as a scoped_ptr, so it's clear it takes ownership.]

[Ryan Sleevi, 2015-08-25 21:19:19]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-08-25 21:19:20]

Could you add a BUG to this description, to clearly indicate what's being added
to Chrome ;)

[Ryan Hamilton, 2015-08-26 18:52:02]

On 2015/08/25 21:19:20, Ryan Sleevi wrote:
> Could you add a BUG to this description, to clearly indicate what's being
added
> to Chrome ;)

Done

[Ryan Hamilton, 2015-08-26 18:52:14]

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
File net/quic/crypto/proof_source_chromium_openssl.cc (right):

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:6: 
On 2015/08/25 19:33:02, David Benjamin wrote:
> This should include:
> 
> #include <openssl/digest.h>
> #include <openssl/evp.h>
> #include <openssl/rsa.h>
> 
> (For EVP_sha256, EVP_DigestSign*, and RSA_PKCS1_PSS_PADDING, respectively.)

Done.

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:75: DCHECK(digest);
On 2015/08/25 19:33:02, David Benjamin wrote:
> Nit: I wouldn't even bother with that. :-) This function is literally:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/boring...
> 
> You can just inline the call or something.

Done.

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:78: if
(!EVP_DigestSignInit(sign_context.get(), &pkey_ctx, digest, NULL,
On 2015/08/25 19:33:02, David Benjamin wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:93: if
(!EVP_DigestSignFinal(sign_context.get(), NULL, &len)) {
On 2015/08/25 19:33:02, David Benjamin wrote:
> Nit: nullptr

Done.

https://codereview.chromium.org/1309813003/diff/60001/net/quic/crypto/proof_s...
net/quic/crypto/proof_source_chromium_openssl.cc:94: DLOG(ERROR) << "Unable to
finalize signature.";
On 2015/08/25 19:33:02, David Benjamin wrote:
> Nit: I wouldn't bother. :-) *shrug*

Done. Because it can't fail?

https://codereview.chromium.org/1309813003/diff/60001/net/quic/test_tools/cry...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/1309813003/diff/60001/net/quic/test_tools/cry...
net/quic/test_tools/crypto_test_utils_chromium.cc:129: cert_verifier, new
TransportSecurityState, "quic_root.crt");
On 2015/08/25 19:33:02, David Benjamin wrote:
> [Not for this CL, but TestProofVerifierChromium should probably take its
> CertVerifier as a scoped_ptr, so it's clear it takes ownership.]

TODO added. Thanks.

[Ryan Hamilton, 2015-08-26 18:54:07]

Ok, so I get the sense that davidben likes this approach, but sleevi prefers the
no-footgun approach of https://codereview.chromium.org/1305183005/. I'm inclined
to go the //crypto/ route, because I'm so unfamiliar with Boring/OpenSSL APIs
that I'm basically just a dog scientist here. But I'm *totally* happy to go
whichever route y'all agree with. Just let me know who wins the cage match! :>

[davidben, 2015-08-26 19:19:07]

On 2015/08/26 18:54:07, Ryan Hamilton wrote:
> Ok, so I get the sense that davidben likes this approach, but sleevi prefers
the
> no-footgun approach of https://codereview.chromium.org/1305183005/. I'm
inclined
> to go the //crypto/ route, because I'm so unfamiliar with Boring/OpenSSL APIs
> that I'm basically just a dog scientist here. But I'm *totally* happy to go
> whichever route y'all agree with. Just let me know who wins the cage match! :>

I will defer to sleevi because I don't think I care enough to jump into a cage
match. :-)

[Ryan Hamilton, 2015-08-27 19:54:33]

On 2015/08/26 19:19:07, David Benjamin wrote:
> On 2015/08/26 18:54:07, Ryan Hamilton wrote:
> > Ok, so I get the sense that davidben likes this approach, but sleevi prefers
> the
> > no-footgun approach of https://codereview.chromium.org/1305183005/. I'm
> inclined
> > to go the //crypto/ route, because I'm so unfamiliar with Boring/OpenSSL
APIs
> > that I'm basically just a dog scientist here. But I'm *totally* happy to go
> > whichever route y'all agree with. Just let me know who wins the cage match!
:>
> 
> I will defer to sleevi because I don't think I care enough to jump into a cage
> match. :-)

Heh. Discussed offline with sleevi and he endorses landing this CL, and not the
//crypto variant. I'll do that forthwith.

[Ryan Hamilton, 2015-09-08 21:32:12]

Patchset #19 (id:360001) has been deleted

[Ryan Hamilton, 2015-09-08 21:32:21]

Patchset #18 (id:340001) has been deleted

[Ryan Hamilton, 2015-09-08 23:15:18]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2015-09-08 23:15:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
rtenneti@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1309813003/#ps400001
(title: "More iOS fix")

[commit-bot: I haz the power, 2015-09-08 23:15:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309813003/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309813003/400001

[commit-bot: I haz the power, 2015-09-08 23:45:08]

Committed patchset #19 (id:400001)

[commit-bot: I haz the power, 2015-09-08 23:45:49]

Patchset 19 (id:??) landed as
https://crrev.com/9f6d72eecc98aec5c085b0e995413182bf3718a9
Cr-Commit-Position: refs/heads/master@{#347831}