Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(120)

Issue 14793021: PairingAuthenticator implementation and plumbing. (Closed)

Created:
7 years, 4 months ago by Jamie
Modified:
7 years, 4 months ago
Reviewers:
Sergey Ulanov, rmsousa, Wez
CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Visibility:
Public.

Description

PairingAuthenticator implementation and plumbing. This CL introduces the pairing authenticator classes, adds support for them to the negotiating authenticator classes and some minimal plumbing. BUG=156182 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201472

Patch Set 1 #

Total comments: 43

Patch Set 2 : Reviewer comments and unit tests. #

Total comments: 23

Patch Set 3 : Reviewer comments. #

Patch Set 4 : Reviewer feedback. #

Total comments: 4

Patch Set 5 : Added protocol support for handling invalid paired secrets cleanly. #

Patch Set 6 : Reviewer comments. #

Total comments: 31

Patch Set 7 : rmsousa's comments. #

Total comments: 69

Patch Set 8 : Refactored common host- and client-side code into common base. #

Total comments: 30

Patch Set 9 : Reviewer comments. #

Total comments: 12

Patch Set 10 : Reviewer comments. #

Patch Set 11 : Fixed clang errors. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+811 lines, -59 lines) Patch
M remoting/client/chromoting_client.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M remoting/host/remoting_me2me_host.cc View 1 2 3 4 5 6 7 3 chunks +7 lines, -1 line 0 comments Download
M remoting/protocol/authentication_method.h View 2 chunks +2 lines, -0 lines 0 comments Download
M remoting/protocol/authentication_method.cc View 1 2 3 4 5 6 7 3 chunks +25 lines, -9 lines 0 comments Download
M remoting/protocol/authenticator.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M remoting/protocol/it2me_host_authenticator_factory.cc View 1 chunk +1 line, -1 line 0 comments Download
M remoting/protocol/me2me_host_authenticator_factory.h View 2 chunks +9 lines, -1 line 0 comments Download
M remoting/protocol/me2me_host_authenticator_factory.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M remoting/protocol/negotiating_authenticator_base.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -1 line 0 comments Download
M remoting/protocol/negotiating_authenticator_unittest.cc View 1 2 3 4 5 6 7 8 6 chunks +135 lines, -25 lines 0 comments Download
M remoting/protocol/negotiating_client_authenticator.h View 1 2 3 4 5 6 7 3 chunks +12 lines, -8 lines 0 comments Download
M remoting/protocol/negotiating_client_authenticator.cc View 1 2 3 4 5 6 7 4 chunks +23 lines, -5 lines 0 comments Download
M remoting/protocol/negotiating_host_authenticator.h View 1 2 3 4 5 6 7 3 chunks +8 lines, -2 lines 0 comments Download
M remoting/protocol/negotiating_host_authenticator.cc View 1 2 3 4 5 6 7 8 4 chunks +25 lines, -4 lines 0 comments Download
A remoting/protocol/pairing_authenticator_base.h View 1 2 3 4 5 6 7 8 9 1 chunk +106 lines, -0 lines 0 comments Download
A remoting/protocol/pairing_authenticator_base.cc View 1 2 3 4 5 6 7 8 9 1 chunk +154 lines, -0 lines 0 comments Download
A remoting/protocol/pairing_client_authenticator.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +49 lines, -0 lines 0 comments Download
A remoting/protocol/pairing_client_authenticator.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +72 lines, -0 lines 0 comments Download
A remoting/protocol/pairing_host_authenticator.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +56 lines, -0 lines 0 comments Download
A remoting/protocol/pairing_host_authenticator.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +104 lines, -0 lines 0 comments Download
M remoting/remoting.gyp View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
Jamie
ptal. I'm working on adding some unit tests for this, but I wanted to get ...
7 years, 4 months ago (2013-05-14 23:18:37 UTC) #1
rmsousa
https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_host.cc File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_host.cc#newcode471 remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; Nit: Please either pass NULL explicitly (and ...
7 years, 4 months ago (2013-05-15 01:25:15 UTC) #2
Jamie
ptal. Note that I've added unit tests in addition to addressing your comments. This needed ...
7 years, 4 months ago (2013-05-15 23:41:08 UTC) #3
rmsousa
https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_host.cc File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_host.cc#newcode471 remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; On 2013/05/15 23:41:08, Jamie wrote: > On ...
7 years, 4 months ago (2013-05-16 00:46:25 UTC) #4
rmsousa
Sorry, missed one comment - only remembered that there was a test that should be ...
7 years, 4 months ago (2013-05-16 00:52:39 UTC) #5
Jamie
Patch set 4 addresses your comments. Patch set 5 adds explicit support the protocol to ...
7 years, 4 months ago (2013-05-16 19:30:15 UTC) #6
rmsousa
https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_host.cc File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_host.cc#newcode471 remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; On 2013/05/16 19:30:16, Jamie wrote: > On ...
7 years, 4 months ago (2013-05-16 20:11:01 UTC) #7
rmsousa
(argh... this other comment, that I had forgotten to save before sending the first message.) ...
7 years, 4 months ago (2013-05-16 20:12:59 UTC) #8
Jamie
ptal https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc#newcode82 remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 20:12:59, rmsousa wrote: ...
7 years, 4 months ago (2013-05-16 21:09:09 UTC) #9
rmsousa
https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc#newcode73 remoting/protocol/pairing_client_authenticator.cc:73: weak_factory_.GetWeakPtr(), resume_callback); (Comment for a future CL, when you ...
7 years, 4 months ago (2013-05-16 22:16:37 UTC) #10
Jamie
I'll follow-up with a version that always uses the pairing authenticator for Spake2Pair, but I ...
7 years, 4 months ago (2013-05-16 22:57:46 UTC) #11
rmsousa
https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc#newcode82 remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 22:57:46, Jamie wrote: > ...
7 years, 4 months ago (2013-05-16 23:22:24 UTC) #12
Sergey Ulanov
https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me2me_host.cc File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me2me_host.cc#newcode475 remoting/host/remoting_me2me_host.cc:475: nit: remove this line. https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me2me_host.cc#newcode483 remoting/host/remoting_me2me_host.cc:483: nit: remove this ...
7 years, 4 months ago (2013-05-17 01:57:16 UTC) #13
Jamie
rmsousa: ptal. sergeyu: I'm working on your comments. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_client_authenticator.cc#newcode82 remoting/protocol/pairing_client_authenticator.cc:82: if ...
7 years, 4 months ago (2013-05-17 17:55:44 UTC) #14
rmsousa
https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authentication_method.cc File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authentication_method.cc#newcode27 remoting/protocol/authentication_method.cc:27: return AuthenticationMethod(SPAKE2_PAIR, HMAC_SHA256); On 2013/05/17 01:57:17, Sergey Ulanov wrote: ...
7 years, 4 months ago (2013-05-17 20:09:02 UTC) #15
Wez
Drive-by w/ some nits & suggestions. Looking pretty good, though. :) https://codereview.chromium.org/14793021/diff/53001/remoting/host/remoting_me2me_host.cc File remoting/host/remoting_me2me_host.cc (right): ...
7 years, 4 months ago (2013-05-18 20:08:36 UTC) #16
Jamie
I think I've addressed everyone's comments here. In order to address Sergey's comment about eliminating ...
7 years, 4 months ago (2013-05-21 01:24:33 UTC) #17
Jamie
Sorry, I messed up the upload last night. PTAL.
7 years, 4 months ago (2013-05-21 21:51:53 UTC) #18
rmsousa
lgtm This functionally looks ok, just a bunch of nits and a couple comments about ...
7 years, 4 months ago (2013-05-21 23:17:07 UTC) #19
Jamie
FYI. Feel free to cancel the CQ if you want to insist on any of ...
7 years, 4 months ago (2013-05-22 00:19:14 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/75001
7 years, 4 months ago (2013-05-22 00:21:04 UTC) #21
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 4 months ago (2013-05-22 00:40:46 UTC) #22
Wez
https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing_host_authenticator.h File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing_host_authenticator.h#newcode58 remoting/protocol/pairing_host_authenticator.h:58: void CheckForFailedSpakeExchange(const base::Closure& resume_callback); On 2013/05/21 01:24:34, Jamie wrote: ...
7 years, 4 months ago (2013-05-22 00:51:45 UTC) #23
Jamie
FYI. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing_host_authenticator.h File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing_host_authenticator.h#newcode58 remoting/protocol/pairing_host_authenticator.h:58: void CheckForFailedSpakeExchange(const base::Closure& resume_callback); On 2013/05/22 00:51:45, Wez ...
7 years, 4 months ago (2013-05-22 01:16:46 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/110001
7 years, 4 months ago (2013-05-22 01:17:43 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/123001
7 years, 4 months ago (2013-05-22 01:37:18 UTC) #26
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 4 months ago (2013-05-22 01:47:28 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/123001
7 years, 4 months ago (2013-05-22 04:53:06 UTC) #28
commit-bot: I haz the power
7 years, 4 months ago (2013-05-22 09:00:24 UTC) #29
Message was sent while issue was closed.
Change committed as 201472

Powered by Google App Engine
This is Rietveld 408576698