[Chromoting] Factor out common code for pin hashing.

[Chromoting] Factor out common code for pin hashing.


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

[simonmorris, 2012-04-27 18:44:55]

ptal

[simonmorris, 2012-04-30 23:01:44]

On 2012/04/27 18:44:55, simonmorris wrote:
> ptal

ping

[Wez, 2012-04-30 23:13:56]

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.cc
File remoting/host/pin_hash.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.cc#newc...
remoting/host/pin_hash.cc:18: bool base64_result = base::Base64Encode(hash,
&hash_base64);
nit: Test base::Base64Encode()'s result directly rather than saving into a local
variable.

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.h
File remoting/host/pin_hash.h (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.h#newco...
remoting/host/pin_hash.h:12: // Hashes a host ID and a PIN.
Can we reword to indicate that it specifically produces a hash in the form used
by Me2Me hosts?

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.h#newco...
remoting/host/pin_hash.h:13: std::string GetPinHash(const std::string& host_id,
const std::string& pin);
nit: GetPinHash -> MakePinHash() / MakeHostPinHash() / MakeMe2MeHostHash()?

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash_unittes...
File remoting/host/pin_hash_unittest.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash_unittes...
remoting/host/pin_hash_unittest.cc:26: ASSERT_EQ(4, hash_set.size());
I think it'd be preferable to have a list of (host-Id, PIN, hash) triples that
can be verified against one another?

http://codereview.chromium.org/10243011/diff/1/remoting/host/plugin/host_scri...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:618: bool
HostNPScriptObject::GetPinHash(const NPVariant* args,
As for the underlying method, we should have a better name for this, e.g.
MakePinHash().

http://codereview.chromium.org/10243011/diff/1/remoting/host/verify_config_wi...
File remoting/host/verify_config_window_win.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/verify_config_wi...
remoting/host/verify_config_window_win.cc:122: return (GetPinHash(host_id_, pin)
== host_secret_hash_);
IsPinValid() in
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference...
does this with a SharedSecretHash, which parses the host secret hash into the
hash method and the data, with the advantage that the verify code will work with
hosts configured with other methods in future.

It might make more sense for SharedSecretHash to have a VerifyHash() method?

[simonmorris, 2012-05-01 00:25:55]

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.cc
File remoting/host/pin_hash.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.cc#newc...
remoting/host/pin_hash.cc:18: bool base64_result = base::Base64Encode(hash,
&hash_base64);
On 2012/04/30 23:13:57, Wez wrote:
> nit: Test base::Base64Encode()'s result directly rather than saving into a
local
> variable.

Done.

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.h
File remoting/host/pin_hash.h (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.h#newco...
remoting/host/pin_hash.h:12: // Hashes a host ID and a PIN.
On 2012/04/30 23:13:57, Wez wrote:
> Can we reword to indicate that it specifically produces a hash in the form
used
> by Me2Me hosts?

Done.

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash.h#newco...
remoting/host/pin_hash.h:13: std::string GetPinHash(const std::string& host_id,
const std::string& pin);
On 2012/04/30 23:13:57, Wez wrote:
> nit: GetPinHash -> MakePinHash() / MakeHostPinHash() / MakeMe2MeHostHash()?

Done.

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash_unittes...
File remoting/host/pin_hash_unittest.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/pin_hash_unittes...
remoting/host/pin_hash_unittest.cc:26: ASSERT_EQ(4, hash_set.size());
On 2012/04/30 23:13:57, Wez wrote:
> I think it'd be preferable to have a list of (host-Id, PIN, hash) triples that
> can be verified against one another?

Done.

http://codereview.chromium.org/10243011/diff/1/remoting/host/plugin/host_scri...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:618: bool
HostNPScriptObject::GetPinHash(const NPVariant* args,
On 2012/04/30 23:13:57, Wez wrote:
> As for the underlying method, we should have a better name for this, e.g.
> MakePinHash().

I think that's out of scope for this CL.

http://codereview.chromium.org/10243011/diff/1/remoting/host/verify_config_wi...
File remoting/host/verify_config_window_win.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/verify_config_wi...
remoting/host/verify_config_window_win.cc:122: return (GetPinHash(host_id_, pin)
== host_secret_hash_);
On 2012/04/30 23:13:57, Wez wrote:
> IsPinValid() in
>
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference...
> does this with a SharedSecretHash, which parses the host secret hash into the
> hash method and the data, with the advantage that the verify code will work
with
> hosts configured with other methods in future.
> 
> It might make more sense for SharedSecretHash to have a VerifyHash() method?

Done.

[Wez, 2012-05-01 01:02:36]

lgtm

http://codereview.chromium.org/10243011/diff/1/remoting/host/plugin/host_scri...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/10243011/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:618: bool
HostNPScriptObject::GetPinHash(const NPVariant* args,
On 2012/05/01 00:25:55, simonmorris wrote:
> On 2012/04/30 23:13:57, Wez wrote:
> > As for the underlying method, we should have a better name for this, e.g.
> > MakePinHash().
> 
> I think that's out of scope for this CL.

My thinking was that since the API return value is changing, it's name also can,
but of course it's bundled, so version breaks don't apply.

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash.h
File remoting/host/pin_hash.h (right):

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash.h#ne...
remoting/host/pin_hash.h:13: // A Me2Me host uses this to hash a shared secret.
nit: Suggest "Create a Me2Me shared-secret hash, consisting of the hash method
and the hashed PIN."

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash_unit...
File remoting/host/pin_hash_unittest.cc (right):

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash_unit...
remoting/host/pin_hash_unittest.cc:24:
ASSERT_TRUE(VerifyHostPinHash(MakeHostPinHash(host_id, pin), host_id, pin));
Verify that it returns false if you supply different pins or hosts to the two
functions?

[simonmorris, 2012-05-01 01:11:54]

fyi

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash.h
File remoting/host/pin_hash.h (right):

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash.h#ne...
remoting/host/pin_hash.h:13: // A Me2Me host uses this to hash a shared secret.
On 2012/05/01 01:02:36, Wez wrote:
> nit: Suggest "Create a Me2Me shared-secret hash, consisting of the hash method
> and the hashed PIN."

Done.

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash_unit...
File remoting/host/pin_hash_unittest.cc (right):

http://codereview.chromium.org/10243011/diff/2003/remoting/host/pin_hash_unit...
remoting/host/pin_hash_unittest.cc:24:
ASSERT_TRUE(VerifyHostPinHash(MakeHostPinHash(host_id, pin), host_id, pin));
On 2012/05/01 01:02:36, Wez wrote:
> Verify that it returns false if you supply different pins or hosts to the two
> functions?

Done.

[commit-bot: I haz the power, 2012-05-01 16:46:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10243011/9003

[commit-bot: I haz the power, 2012-05-01 18:17:52]

Change committed as 134725