Abstracts SHA256 context for NSS / OpenSSL.

Abstracts SHA256 context for NSS / OpenSSL.
Stubs out SslServerSocket for OpenSSL.

BUG=none
TEST=Sha256Test.TestContext (and compiles with openssl flag).

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

[bulach, 2011-01-14 17:09:27]

Hi Lei Zheng,

We have an experimental build that uses openssl instead of nss. This change is
needed following yours:
http://codereview.chromium.org/6023006/

instead of depending directly on NSS SHA256, I created a thin abstraction and
implemented on openssl as well.

Would you mind taking a look please?

[bulach, 2011-01-21 12:11:06]

ping? :)

[wtc, 2011-01-21 22:48:50]

LGTM.  I suggest some renaming below.

http://codereview.chromium.org/6276002/diff/6001/base/sha2.h
File base/sha2.h (right):

http://codereview.chromium.org/6276002/diff/6001/base/sha2.h#newcode34
base/sha2.h:34: class SHA256Context {
Please name this class SHA256Hash or SHA256.  (I am worried
that SHA256 may conflict with an existing enum or macro in
a system crypto library.)

Ideally, we should create a Hash or SecureHash class,
whose Create method takes an algorithm enum as input.
Then we can use the class to compute an MD5, SHA-1, or
SHA-256 hash.

http://codereview.chromium.org/6276002/diff/6001/base/sha2.h#newcode41
base/sha2.h:41: virtual void Finish(void* output, size_t len) = 0;
This method should be named Final.

http://codereview.chromium.org/6276002/diff/6001/base/sha2_unittest.cc
File base/sha2_unittest.cc (right):

http://codereview.chromium.org/6276002/diff/6001/base/sha2_unittest.cc#newcod...
base/sha2_unittest.cc:100: TEST(Sha256Test, TestContext) {
You'll need to rename this test "TestUpdate" or "TestStreaming"
to match the SHA256 class name I suggested.

http://codereview.chromium.org/6276002/diff/6001/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_openssl.cc (right):

http://codereview.chromium.org/6276002/diff/6001/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:50: Socket* socket, X509Certificate*
certificate, base::RSAPrivateKey* key,
List one parameter per line.

[lzheng, 2011-01-24 18:46:35]

I got only one nit.

http://codereview.chromium.org/6276002/diff/6001/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_openssl.cc (right):

http://codereview.chromium.org/6276002/diff/6001/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:5: #include
"net/socket/ssl_server_socket.h"
nit: put this behind "base/logging.h"

[bulach, 2011-01-24 20:54:33]

thanks both!

I addressed all comments and renamed / moved things around as appropriate. would
you mind a final look please?

http://codereview.chromium.org/6276002/diff/6001/base/sha2.h
File base/sha2.h (right):

http://codereview.chromium.org/6276002/diff/6001/base/sha2.h#newcode34
base/sha2.h:34: class SHA256Context {
On 2011/01/21 22:48:51, wtc wrote:
> Please name this class SHA256Hash or SHA256.  (I am worried
> that SHA256 may conflict with an existing enum or macro in
> a system crypto library.)
> 
> Ideally, we should create a Hash or SecureHash class,
> whose Create method takes an algorithm enum as input.
> Then we can use the class to compute an MD5, SHA-1, or
> SHA-256 hash.

I liked this idea, seems more flexible.
I extracted this into a SecureHash class and moved things accordingly.

http://codereview.chromium.org/6276002/diff/6001/base/sha2.h#newcode41
base/sha2.h:41: virtual void Finish(void* output, size_t len) = 0;
On 2011/01/21 22:48:51, wtc wrote:
> This method should be named Final.

done, in the new files.

http://codereview.chromium.org/6276002/diff/6001/base/sha2_unittest.cc
File base/sha2_unittest.cc (right):

http://codereview.chromium.org/6276002/diff/6001/base/sha2_unittest.cc#newcod...
base/sha2_unittest.cc:100: TEST(Sha256Test, TestContext) {
On 2011/01/21 22:48:51, wtc wrote:
> You'll need to rename this test "TestUpdate" or "TestStreaming"
> to match the SHA256 class name I suggested.

renamed to TestUpdate (in the new file).

http://codereview.chromium.org/6276002/diff/6001/net/socket/ssl_server_socket...
File net/socket/ssl_server_socket_openssl.cc (right):

http://codereview.chromium.org/6276002/diff/6001/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:5: #include
"net/socket/ssl_server_socket.h"
On 2011/01/24 18:46:35, lzheng wrote:
> nit: put this behind "base/logging.h"

Done.

http://codereview.chromium.org/6276002/diff/6001/net/socket/ssl_server_socket...
net/socket/ssl_server_socket_openssl.cc:50: Socket* socket, X509Certificate*
certificate, base::RSAPrivateKey* key,
On 2011/01/21 22:48:51, wtc wrote:
> List one parameter per line.

Done.

[wtc, 2011-01-25 19:06:42]

LGTM.

High-level comments:

Please add the new secure_hash files to base/crypto instead
of the top-level base directory.

New files should have copyright year 2011.

The SecureHash API looks asymmetric:
  virtual void Update(const std::string& str) = 0;
  virtual void Finish(void* output, size_t len) = 0;

Perhaps Update should take
  const void* input, size_t len
as inputs, or an overloaded Upload with those inputs should
be added.

You may want to move net.gyp and
ssl_server_socket_openssl.cc to a separate CL.  They are
completely unrelated to the SecureHash class.

Low-level comments follow.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h
File base/secure_hash.h (right):

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode12
base/secure_hash.h:12: #include "base/sha2.h"
This file should not need to include the header for any
specific hash algorithm.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode17
base/secure_hash.h:17: // advance.
Remove "when the full string is not known in advance", which
implies it can only be used when the full string is not known
in advance.

Or you can reword it to remove that confusion.  Please change
"full string" to "full input".

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode20
base/secure_hash.h:20: enum Type {
This enum should be named Algorithm.  The constant can be
named ALG_SHA256 or just SHA256.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc
File base/secure_hash_nss.cc (right):

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
base/secure_hash_nss.cc:8: #include "base/stl_util-inl.h"
Remove this header.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
base/secure_hash_nss.cc:37: ::SHA256Context ctx_;
Nit: you can remove :: now.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
base/secure_hash_nss.cc:46: return NULL;
Please use a switch statement to make it easy to add new
hash algorithms in the future.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc
File base/secure_hash_openssl.cc (right):

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc...
base/secure_hash_openssl.cc:12: #include "base/stl_util-inl.h"
Remove this header.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc...
base/secure_hash_openssl.cc:49: return NULL;
Use a switch statement.

http://codereview.chromium.org/6276002/diff/18001/net/base/openssl_memory_pri...
File net/base/openssl_memory_private_key_store.cc (right):

http://codereview.chromium.org/6276002/diff/18001/net/base/openssl_memory_pri...
net/base/openssl_memory_private_key_store.cc:13: #include
"base/synchronization/lock.h"
Nit: list this header after "base/singleton.h".

[bulach, 2011-01-25 20:33:28]

thanks Wan-Teh! all comments addressed.

if you're ok, I'd like to keep the ssl_server_socket_openssl.cc in this same
patch: I understand they're not related to SecureHash itself, but it's necessary
to compile with the openssl flag.. please, let me know if that's reasonable.

On 2011/01/25 19:06:42, wtc wrote:
> LGTM.
> 
> High-level comments:
> 
> Please add the new secure_hash files to base/crypto instead
> of the top-level base directory.
> 
> New files should have copyright year 2011.
> 
> The SecureHash API looks asymmetric:
>   virtual void Update(const std::string& str) = 0;
>   virtual void Finish(void* output, size_t len) = 0;
> 
> Perhaps Update should take
>   const void* input, size_t len
> as inputs, or an overloaded Upload with those inputs should
> be added.
> 
> You may want to move net.gyp and
> ssl_server_socket_openssl.cc to a separate CL.  They are
> completely unrelated to the SecureHash class.
> 
> Low-level comments follow.
> 
> http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h
> File base/secure_hash.h (right):
> 
> http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode12
> base/secure_hash.h:12: #include "base/sha2.h"
> This file should not need to include the header for any
> specific hash algorithm.
> 
> http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode17
> base/secure_hash.h:17: // advance.
> Remove "when the full string is not known in advance", which
> implies it can only be used when the full string is not known
> in advance.
> 
> Or you can reword it to remove that confusion.  Please change
> "full string" to "full input".
> 
> http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode20
> base/secure_hash.h:20: enum Type {
> This enum should be named Algorithm.  The constant can be
> named ALG_SHA256 or just SHA256.
> 
> http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc
> File base/secure_hash_nss.cc (right):
> 
>
http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
> base/secure_hash_nss.cc:8: #include "base/stl_util-inl.h"
> Remove this header.
> 
>
http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
> base/secure_hash_nss.cc:37: ::SHA256Context ctx_;
> Nit: you can remove :: now.
> 
>
http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
> base/secure_hash_nss.cc:46: return NULL;
> Please use a switch statement to make it easy to add new
> hash algorithms in the future.
> 
> http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc
> File base/secure_hash_openssl.cc (right):
> 
>
http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc...
> base/secure_hash_openssl.cc:12: #include "base/stl_util-inl.h"
> Remove this header.
> 
>
http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc...
> base/secure_hash_openssl.cc:49: return NULL;
> Use a switch statement.
> 
>
http://codereview.chromium.org/6276002/diff/18001/net/base/openssl_memory_pri...
> File net/base/openssl_memory_private_key_store.cc (right):
> 
>
http://codereview.chromium.org/6276002/diff/18001/net/base/openssl_memory_pri...
> net/base/openssl_memory_private_key_store.cc:13: #include
> "base/synchronization/lock.h"
> Nit: list this header after "base/singleton.h".

[bulach, 2011-01-25 20:33:41]

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h
File base/secure_hash.h (right):

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode12
base/secure_hash.h:12: #include "base/sha2.h"
On 2011/01/25 19:06:43, wtc wrote:
> This file should not need to include the header for any
> specific hash algorithm.

Done.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode17
base/secure_hash.h:17: // advance.
On 2011/01/25 19:06:43, wtc wrote:
> Remove "when the full string is not known in advance", which
> implies it can only be used when the full string is not known
> in advance.
> 
> Or you can reword it to remove that confusion.  Please change
> "full string" to "full input".

I rewrote the sentence, please let me know if it's any clearer.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash.h#newcode20
base/secure_hash.h:20: enum Type {
On 2011/01/25 19:06:43, wtc wrote:
> This enum should be named Algorithm.  The constant can be
> named ALG_SHA256 or just SHA256.

Done.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc
File base/secure_hash_nss.cc (right):

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
base/secure_hash_nss.cc:8: #include "base/stl_util-inl.h"
On 2011/01/25 19:06:43, wtc wrote:
> Remove this header.

Done.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
base/secure_hash_nss.cc:37: ::SHA256Context ctx_;
On 2011/01/25 19:06:43, wtc wrote:
> Nit: you can remove :: now.

Done.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_nss.cc#new...
base/secure_hash_nss.cc:46: return NULL;
On 2011/01/25 19:06:43, wtc wrote:
> Please use a switch statement to make it easy to add new
> hash algorithms in the future.

Done.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc
File base/secure_hash_openssl.cc (right):

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc...
base/secure_hash_openssl.cc:12: #include "base/stl_util-inl.h"
On 2011/01/25 19:06:43, wtc wrote:
> Remove this header.

Done.

http://codereview.chromium.org/6276002/diff/18001/base/secure_hash_openssl.cc...
base/secure_hash_openssl.cc:49: return NULL;
On 2011/01/25 19:06:43, wtc wrote:
> Use a switch statement.

Done.

http://codereview.chromium.org/6276002/diff/18001/net/base/openssl_memory_pri...
File net/base/openssl_memory_private_key_store.cc (right):

http://codereview.chromium.org/6276002/diff/18001/net/base/openssl_memory_pri...
net/base/openssl_memory_private_key_store.cc:13: #include
"base/synchronization/lock.h"
On 2011/01/25 19:06:43, wtc wrote:
> Nit: list this header after "base/singleton.h".

Done.

[wtc, 2011-01-26 23:39:37]

LGTM.

It's fine to keep ssl_server_socket_openssl.cc in this CL.
Your rewrite of the comment for SecureHash is clearer.  Thanks!

[bulach, 2011-01-27 11:59:29]

thanks! 

there was an issue with this patch, I had to rename secure_hash_nss to
secure_hash, as there's an exclusion list for *_nss on non-linux platforms:
http://codereview.chromium.org/6374015/

if there's any problem with the new name, please let me know and I'm happy to
follow up.

On 2011/01/26 23:39:37, wtc wrote:
> LGTM.
> 
> It's fine to keep ssl_server_socket_openssl.cc in this CL.
> Your rewrite of the comment for SecureHash is clearer.  Thanks!

[joth, 2011-04-06 14:00:06]

(Late comments as I was out at the time)

On 2011/01/25 19:06:42, wtc wrote:
> LGTM.
> 
> High-level comments:
> 
> Please add the new secure_hash files to base/crypto instead
> of the top-level base directory.
> 

I'm not clear on the motivation here. This means we now have base/sha2.h and
base/crypto/secure_hash.h with overlapping functionality. (Illustrated by the
corresponding test files having repeated test case B.3 from FIPS 180-2)

Is the intention to eventually remove the sha2.h file (and required everyone to
use the incremental API)? What about sha1.h?

Cheers!
Joth

[wtc, 2011-04-06 14:12:27]

The current SecureHash class has only incremental methods,
but we can add a one-shot hash method to SecureHash.  The
most interesting feature of SecureHash is that it allows us
to use the same interface for multiple hash algorithms.

When SecureHash is fully implemented, it will be able to
replace sha1.h and sha2.h.  We can keep sha1.h and sha2.h as
convenience functions.

[joth, 2011-04-06 14:51:41]

OK, thanks for the clarification.

On 6 April 2011 15:12, <wtc@chromium.org> wrote:

> The current SecureHash class has only incremental methods,
> but we can add a one-shot hash method to SecureHash.  The
> most interesting feature of SecureHash is that it allows us
> to use the same interface for multiple hash algorithms.
>
> When SecureHash is fully implemented, it will be able to
> replace sha1.h and sha2.h.  We can keep sha1.h and sha2.h as
> convenience functions.
>
>
> http://codereview.chromium.org/6276002/
>