Implements Signature Creator & Verifier for openssl

Implements Signature Creator & Verifier for openssl
Also adds a little more infrastructure to assist in openssl error handling.

BUG=None
TEST=base_unittests RSA* and Sign*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66622

[joth, 2010-11-17 13:24:13]

Slowly getting there...!

[bulach, 2010-11-17 14:35:09]

LGTM

few suggestion, it'd be great to have some openssl expert to look at it as
well...

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_creato...
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_creato...
base/crypto/signature_creator_openssl.cc:19: scoped_ptr<SignatureCreator>
self(new SignatureCreator);
s/self/new_signature/

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_creato...
base/crypto/signature_creator_openssl.cc:31: EVP_MD_CTX_destroy(sign_context_);
scoped?

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
File base/crypto/signature_verifier_openssl.cc (right):

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
base/crypto/signature_verifier_openssl.cc:41: OpenSSLErrStackTracer
err_tracer("VerifyInit");
maybe: SignatureVerifier::VerifyInit

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
base/crypto/signature_verifier_openssl.cc:75: OpenSSLErrStackTracer
err_tracer("VerifyUpdate");
ditto

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
base/crypto/signature_verifier_openssl.cc:83: OpenSSLErrStackTracer
err_tracer("VerifyFinal");
ditto

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h#newcode90
base/openssl_util.h:90: // we be included in any error dump.
s/we be/will be/
but perhaps even more direct:
If |message| is not null it'll be included in any error dump.

(and similarly above..)

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h#newcode93
base/openssl_util.h:93: OpenSSLErrStackTracer(const char* message) :
message_(message) {}
explicit

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h#newcode97
base/openssl_util.h:97: const char* message_;
const char* const

[bulach, 2010-11-17 14:35:09]

LGTM

few suggestion, it'd be great to have some openssl expert to look at it as
well...

[joth, 2010-11-17 14:49:31]

Cheers.

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_creato...
File base/crypto/signature_creator_openssl.cc (right):

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_creato...
base/crypto/signature_creator_openssl.cc:19: scoped_ptr<SignatureCreator>
self(new SignatureCreator);
On 2010/11/17 14:35:09, bulach wrote:
> s/self/new_signature/

Done. (=>result)

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_creato...
base/crypto/signature_creator_openssl.cc:31: EVP_MD_CTX_destroy(sign_context_);
On 2010/11/17 14:35:09, bulach wrote:
> scoped?

Prefer not to, for same reason as RSAPrivateKey.

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
File base/crypto/signature_verifier_openssl.cc (right):

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
base/crypto/signature_verifier_openssl.cc:41: OpenSSLErrStackTracer
err_tracer("VerifyInit");
On 2010/11/17 14:35:09, bulach wrote:
> maybe: SignatureVerifier::VerifyInit

Done.

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
base/crypto/signature_verifier_openssl.cc:75: OpenSSLErrStackTracer
err_tracer("VerifyUpdate");
On 2010/11/17 14:35:09, bulach wrote:
> ditto

Done.

http://codereview.chromium.org/5105003/diff/2001/base/crypto/signature_verifi...
base/crypto/signature_verifier_openssl.cc:83: OpenSSLErrStackTracer
err_tracer("VerifyFinal");
On 2010/11/17 14:35:09, bulach wrote:
> ditto

Done.

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h#newcode90
base/openssl_util.h:90: // we be included in any error dump.
On 2010/11/17 14:35:09, bulach wrote:
> s/we be/will be/
> but perhaps even more direct:
> If |message| is not null it'll be included in any error dump.
> 
> (and similarly above..)

Done.

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h#newcode93
base/openssl_util.h:93: OpenSSLErrStackTracer(const char* message) :
message_(message) {}
On 2010/11/17 14:35:09, bulach wrote:
> explicit

Done.

http://codereview.chromium.org/5105003/diff/2001/base/openssl_util.h#newcode97
base/openssl_util.h:97: const char* message_;
On 2010/11/17 14:35:09, bulach wrote:
> const char* const

Done.
(Although, this rule is mostly useful for globals/statics)

[wtc, 2010-11-17 20:30:48]

I only reviewed the .h files, and skimmed the .cc files.
LGTM (please consider this a drive-by review).

http://codereview.chromium.org/5105003/diff/12001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/5105003/diff/12001/base/openssl_util.h#newcode85
base/openssl_util.h:85: void ClearOpenSSLERRStack(const char* message);
Please consult the Style Guide's recommendations on
function overloading and optional arguments.  I remember
they're discouraged in general but allowed in some cases,
but I don't remember the details.

[joth, 2010-11-18 12:52:47]

New snapshot uploaded pulls in a few more files due to ClearOpenSSLStack knock
on. Would you mind taking another quick look marcus?
(New additions to the patch: base/crypto/encryptor_openssl.cc
base/crypto/rsa_private_key_openssl.cc base/crypto/symmetric_key_openssl.cc
net/base/cert_test_util.cc )

http://codereview.chromium.org/5105003/diff/12001/base/openssl_util.h
File base/openssl_util.h (right):

http://codereview.chromium.org/5105003/diff/12001/base/openssl_util.h#newcode85
base/openssl_util.h:85: void ClearOpenSSLERRStack(const char* message);
On 2010/11/17 20:30:48, wtc wrote:
> Please consult the Style Guide's recommendations on
> function overloading and optional arguments.  I remember
> they're discouraged in general but allowed in some cases,
> but I don't remember the details.

Done.
It falls under the "overloads must be obvious" subjectiveness test.
I had a better idea though and removed the overload, and updated all callsites
to pass FROM_HERE instead as this seems generally better.

[bulach, 2010-11-18 13:48:25]

LGTM

FROM_HERE certainly looks nicer, thanks!