Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14793021:
PairingAuthenticator implementation and plumbing. (Closed)
|
Created:
7 years, 7 months ago by Jamie Modified:
7 years, 7 months ago 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 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPairingAuthenticator 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. #Messages
Total messages: 29 (0 generated)
ptal. I'm working on adding some unit tests for this, but I wanted to get the review under way while I'm doing that.
https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; Nit: Please either pass NULL explicitly (and move the comment down), or create an actual (possibly dummy) instance. Passing an implicitly null pointer is a bit harder to read. (Note, not necessarily related to this CL: while looking at whether it was possible to create a dummy instance, I noticed that NotImplementedPairingRegistryDelegate is a bit too literal :) - please take the opportunity to either remove the declaration or add a definition for ::Save). https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:20: const buzz::StaticQName kPairingQName = Nit: We use kPairingTag for tags inside an authenticator. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:20: const buzz::StaticQName kPairingQName = Nit: I'd prefer if these were in a shared place, although there aren't enough commonalities between client and host to justify a PairingAuthenticatorBase, and there isn't anywhere else that seems appropriate... so I suppose leave a comment in each file pointing out that it's in sync with its antipode? https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:42: v2_authenticator_ = V2Authenticator::CreateForClient( There is no need to create this underlying authenticator in the WAITING_MESSAGE state here. It won't be used to send the first message, and it may end up being deleted and recreated when you process the next message - you might as well wait for the next ProcessMessage, and create the appropriate authenticator when you receive the message from the host and actually know which authenticator/state to create (PIN Authenticator in MESSAGE_READY state if it's a failure, SharedSecret Authenticator in WAITING_MESSAGE state if it's a success). https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { This is wrong when the host sends the first message to the (unpaired) client - the host is already using PIN authentication. if you delay the v2_authenticator_ creation, its existence seems like a better check here (and initial_message_sent_ isn't necessary at all). https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:40: // If the client didn't specify an initial message, use the PIN as the shared This implies a stronger protocol requirement (i.e. that should be part of the description in the header): If the client wants to use an existing client_id, it MUST send the initial message in the protocol. If the host selects spake2_pair, it is too late, and the client will have to use the PIN. On the other hand, the server side seems a bit too flexible - the client has the choice of either initiate by sending a blank spake2_pair message (missing-client-id), or send no message and let the host pick this method from the client's supported methods list. I'm slightly bothered by this asymmetry - is there any reason why we don't simply forbid a blank message from the client as PROTOCOL_ERROR? https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:43: if (initial_state_ != WAITING_MESSAGE) { Nit: please DCHECK_EQ(initial_state_, MESSAGE_READY) https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:55: return initial_state_; Please either have an actual state_ variable, or use an implicit state (in this case, if !v2_authenticator_, the state is WAITING_MESSAGE, always). Using initial_state_ anywhere but the constructor is confusing (especially if it's not trivial to determine that the piece of code using initial_state_ will really only be used just after construction). https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:66: const base::Closure& resume_callback) { DCHECK_EQ(state(), WAITING_MESSAGE) https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:69: v2_authenticator_->ProcessMessage(message, resume_callback); please DCHECK the v2_authenticator_ state here. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:101: scoped_ptr<buzz::XmlElement> PairingHostAuthenticator::GetNextMessage() { DCHECK_EQ(state(), MESSAGE_READY) https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:125: local_cert_, key_pair_, shared_secret_, initial_state_); Using initial_state_ is weird here. There are several different flows happening, with different V2Authenticator states on each side: (1) Client sends client_id, host recognizes: C -> H client_id H -> C first spake message (...) (2) Client sends client_id, host does not recognize: C -> H client_id H -> C error_message C -> H first spake message (...) (3) Client does not send a client_id: C -> H blank H -> C first spake message And then on the client there is the opposite mapping, of processing or not processing the first host message based on initial_state_. The host always has all the information required to send teh first SPAKE message - It seems simpler to determine that it always does so. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.h:25: // for authentication instead. In either case, the V2Authenticator is used Please elaborate on how the V2Authenticator is used (i.e. which side sends the first V2 message, and when, for either case). Also, please describe how it works when the client does not have a client_id. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.h:25: // for authentication instead. In either case, the V2Authenticator is used V2Authenticator with HMAC_SHA256 hashing.
ptal. Note that I've added unit tests in addition to addressing your comments. This needed some changes to the existing unit test framework to expose some of the state I needed to verify. https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; On 2013/05/15 01:25:16, rmsousa wrote: > Nit: Please either pass NULL explicitly (and move the comment down), or create > an actual (possibly dummy) instance. Passing an implicitly null pointer is a bit > harder to read. > > (Note, not necessarily related to this CL: while looking at whether it was > possible to create a dummy instance, I noticed that > NotImplementedPairingRegistryDelegate is a bit too literal :) - please take the > opportunity to either remove the declaration or add a definition for ::Save). It says quite clearly that it's not implemented. What more do you want? ;) I've actually already fixed this in https://codereview.chromium.org/14995019/. Regarding passing NULL, the reason I'm doing that for now is that I don't want the UI to present the option of pairing until it's fully implemented. The simplest way of doing that is by not creating the registry. It will also make it easy to support a policy to disable pairing. Creating a dummy implementation doesn't really make sense because it's not an interface. Even if I made it into an interface, it would still need something along the lines of an IsImplemented method in order to prevent pairing from being offered. This seems like overkill to me. I've added a comment to NegotiatingHostAuthenticator::CreateWithSharedSecret to explain. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:20: const buzz::StaticQName kPairingQName = On 2013/05/15 01:25:16, rmsousa wrote: > Nit: We use kPairingTag for tags inside an authenticator. Done. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:20: const buzz::StaticQName kPairingQName = On 2013/05/15 01:25:16, rmsousa wrote: > Nit: I'd prefer if these were in a shared place, although there aren't enough > commonalities between client and host to justify a PairingAuthenticatorBase, and > there isn't anywhere else that seems appropriate... so I suppose leave a comment > in each file pointing out that it's in sync with its antipode? Done. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:42: v2_authenticator_ = V2Authenticator::CreateForClient( On 2013/05/15 01:25:16, rmsousa wrote: > There is no need to create this underlying authenticator in the WAITING_MESSAGE > state here. It won't be used to send the first message, and it may end up being > deleted and recreated when you process the next message - you might as well wait > for the next ProcessMessage, and create the appropriate authenticator when you > receive the message from the host and actually know which authenticator/state to > create (PIN Authenticator in MESSAGE_READY state if it's a failure, SharedSecret > Authenticator in WAITING_MESSAGE state if it's a success). Done. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/15 01:25:16, rmsousa wrote: > This is wrong when the host sends the first message to the (unpaired) client - > the host is already using PIN authentication. > > if you delay the v2_authenticator_ creation, its existence seems like a better > check here (and initial_message_sent_ isn't necessary at all). My understanding is that the client always sends the first message--the optimistic client-id payload on the supported-methods tag. Is that not the case? In any case, I've modified this method in light of your previous comment to have an explicit pairing_state_ member, which may address your concerns. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:40: // If the client didn't specify an initial message, use the PIN as the shared On 2013/05/15 01:25:16, rmsousa wrote: > This implies a stronger protocol requirement (i.e. that should be part of the > description in the header): > If the client wants to use an existing client_id, it MUST send the initial > message in the protocol. If the host selects spake2_pair, it is too late, and > the client will have to use the PIN. Do my changes to the header comments address this concern? > On the other hand, the server side seems a bit too flexible - the client has the > choice of either initiate by sending a blank spake2_pair message > (missing-client-id), or send no message and let the host pick this method from > the client's supported methods list. > > I'm slightly bothered by this asymmetry - is there any reason why we don't > simply forbid a blank message from the client as PROTOCOL_ERROR? I think you're right. I've modified ProcessMessage to explicitly flag this as an error. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:43: if (initial_state_ != WAITING_MESSAGE) { On 2013/05/15 01:25:16, rmsousa wrote: > Nit: please DCHECK_EQ(initial_state_, MESSAGE_READY) Done. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:55: return initial_state_; On 2013/05/15 01:25:16, rmsousa wrote: > Please either have an actual state_ variable, or use an implicit state (in this > case, if !v2_authenticator_, the state is WAITING_MESSAGE, always). Using > initial_state_ anywhere but the constructor is confusing (especially if it's not > trivial to determine that the piece of code using initial_state_ will really > only be used just after construction). An explicit state variable makes less sense that it does in the client because the REJECTED state (meaning pairing failed) is either a protocol error or a case of deferring to the underlying authenticator. I've made it implicit and no longer dependent on the initial state as you suggest. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:66: const base::Closure& resume_callback) { On 2013/05/15 01:25:16, rmsousa wrote: > DCHECK_EQ(state(), WAITING_MESSAGE) Done. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:69: v2_authenticator_->ProcessMessage(message, resume_callback); On 2013/05/15 01:25:16, rmsousa wrote: > please DCHECK the v2_authenticator_ state here. Done. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:101: scoped_ptr<buzz::XmlElement> PairingHostAuthenticator::GetNextMessage() { On 2013/05/15 01:25:16, rmsousa wrote: > DCHECK_EQ(state(), MESSAGE_READY) Done. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:125: local_cert_, key_pair_, shared_secret_, initial_state_); On 2013/05/15 01:25:16, rmsousa wrote: > Using initial_state_ is weird here. There are several different flows happening, > with different V2Authenticator states on each side: > > (1) Client sends client_id, host recognizes: > C -> H client_id > H -> C first spake message > (...) > (2) Client sends client_id, host does not recognize: > C -> H client_id > H -> C error_message > C -> H first spake message > (...) > (3) Client does not send a client_id: > C -> H blank > H -> C first spake message > > And then on the client there is the opposite mapping, of processing or not > processing the first host message based on initial_state_. > > The host always has all the information required to send teh first SPAKE message > - It seems simpler to determine that it always does so. It doesn't always send the first message. If the client has specified an unrecognized id (the pairing has been revoked, for example), the host replies with a pairing-specific message to tell it to request the PIN instead. In this case, the client initiates the SPAKE exchange. I've removed the reference to initial_state_ here though. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.h:25: // for authentication instead. In either case, the V2Authenticator is used On 2013/05/15 01:25:16, rmsousa wrote: > Please elaborate on how the V2Authenticator is used (i.e. which side sends the > first V2 message, and when, for either case). > > Also, please describe how it works when the client does not have a client_id. I've spelled out the protocol in more detail. LMKWYT. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.h:25: // for authentication instead. In either case, the V2Authenticator is used On 2013/05/15 01:25:16, rmsousa wrote: > V2Authenticator with HMAC_SHA256 hashing. Done.
https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; On 2013/05/15 23:41:08, Jamie wrote: > On 2013/05/15 01:25:16, rmsousa wrote: > > Nit: Please either pass NULL explicitly (and move the comment down), or create > > an actual (possibly dummy) instance. Passing an implicitly null pointer is a > bit > > harder to read. > > > > (Note, not necessarily related to this CL: while looking at whether it was > > possible to create a dummy instance, I noticed that > > NotImplementedPairingRegistryDelegate is a bit too literal :) - please take > the > > opportunity to either remove the declaration or add a definition for ::Save). > > It says quite clearly that it's not implemented. What more do you want? ;) > > I've actually already fixed this in https://codereview.chromium.org/14995019/. > Regarding passing NULL, the reason I'm doing that for now is that I don't want > the UI to present the option of pairing until it's fully implemented. The > simplest way of doing that is by not creating the registry. It will also make it > easy to support a policy to disable pairing. Creating a dummy implementation > doesn't really make sense because it's not an interface. Even if I made it into > an interface, it would still need something along the lines of an IsImplemented > method in order to prevent pairing from being offered. This seems like overkill > to me. > > I've added a comment to NegotiatingHostAuthenticator::CreateWithSharedSecret to > explain. I see, I agree passing NULL is the right move in this case. But since that's the case, [nit:] I think actually passing NULL in the parameter list makes the intention more readable. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/15 23:41:08, Jamie wrote: > On 2013/05/15 01:25:16, rmsousa wrote: > > This is wrong when the host sends the first message to the (unpaired) client - > > the host is already using PIN authentication. > > > > if you delay the v2_authenticator_ creation, its existence seems like a better > > check here (and initial_message_sent_ isn't necessary at all). > > My understanding is that the client always sends the first message--the > optimistic client-id payload on the supported-methods tag. Is that not the case? No. If the client does not have a client ID, the code on NegotiatingClientAuthenticator does not pick a method or create an authenticator. This will cause the host to pick the spake2_pair method, and create PairingHostAuthenticator in the MESSAGE_READY state (meaning it will send the first message of the PIN-based V2Authenticator). Then the client will receive that message, and create a PairingClientAuthenticator in the WAITING_MESSAGE state. > > In any case, I've modified this method in light of your previous comment to have > an explicit pairing_state_ member, which may address your concerns. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:125: local_cert_, key_pair_, shared_secret_, initial_state_); On 2013/05/15 23:41:08, Jamie wrote: > On 2013/05/15 01:25:16, rmsousa wrote: > > Using initial_state_ is weird here. There are several different flows > happening, > > with different V2Authenticator states on each side: > > > > (1) Client sends client_id, host recognizes: > > C -> H client_id > > H -> C first spake message > > (...) > > (2) Client sends client_id, host does not recognize: > > C -> H client_id > > H -> C error_message > > C -> H first spake message > > (...) > > (3) Client does not send a client_id: > > C -> H blank > > H -> C first spake message > > > > And then on the client there is the opposite mapping, of processing or not > > processing the first host message based on initial_state_. > > > > The host always has all the information required to send teh first SPAKE > message > > - It seems simpler to determine that it always does so. > > It doesn't always send the first message. If the client has specified an > unrecognized id (the pairing has been revoked, for example), the host replies > with a pairing-specific message to tell it to request the PIN instead. In this > case, the client initiates the SPAKE exchange. I've removed the reference to > initial_state_ here though. Yes, I was saying that this variation was confusing. There are two cases where SPAKE is host initiated, and one where it is client initiated. The host already has the information to start the SPAKE negotiation in the pairing rejected case, there's no reason to wait for the client to start it. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/negotiat... File remoting/protocol/negotiating_authenticator_unittest.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/negotiat... remoting/protocol/negotiating_authenticator_unittest.cc:131: NegotiatingClientAuthenticator* client_as_negotiating_authenticator_; Nit: why not just static_cast<NegotiatingAuthenticatorBase>(client_)? https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:62: const base::Closure& resume_callback) { please DCHECK_EQ(state(), WAITING_MESSAGE) https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:75: } else { This could either be a PIN based V2 message or a Shared Secret V2 Message, depending on whether the client has a client ID or not. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:89: // If the initial message has not yet been sent, return it now. Please DCHECK_EQ(state(), MESSAGE_READY) https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:110: const std::string& shared_secret) { Nit: The term "Secret" is a bit ambiguous here, so it isn't clear that we know the pairing state is rejected because this method is only called when we ask the user for a PIN. I think naming it CreateV2AuthenticatorWithPIN would make it a bit clearer (and symmetrical with the host) https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.h:39: // HMAC_SHA256--only the protocol type differs, which the client uses nit: s/protocol type/method name/ https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.cc:64: DCHECK(v2_authenticator_); if (!v2_authenticator_) return PROTOCOL_ERROR https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.cc:83: if (pairing_tag) { Lack of pairing tag should be an error as well. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.h:34: // authenticator behaves exactly like the V2Authenticator with Please clarify who sends the first SPAKE message in this case (the host)
Sorry, missed one comment - only remembered that there was a test that should be failing after I sent the first message. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/negotiat... File remoting/protocol/negotiating_authenticator_unittest.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/negotiat... remoting/protocol/negotiating_authenticator_unittest.cc:190: kNoClientId, kTestSharedSecret, kTestSharedSecret, This is only passing because the same string is being used as client secret and PIN. In this case, teh client is currently using the ClientSecret flow, and the host is using the PIN flow. Please define different strings for ClientSecret and PIN.
Patch set 4 addresses your comments. Patch set 5 adds explicit support the protocol to prompt for the PIN if the shared secret exchange fails. This should be very unlikely (it suggests that one end of the connection has a corrupted registry) but it needs to be handled, otherwise connections between that client and the host would be impossible. https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; On 2013/05/16 00:46:25, rmsousa wrote: > On 2013/05/15 23:41:08, Jamie wrote: > > On 2013/05/15 01:25:16, rmsousa wrote: > > > Nit: Please either pass NULL explicitly (and move the comment down), or > create > > > an actual (possibly dummy) instance. Passing an implicitly null pointer is a > > bit > > > harder to read. > > > > > > (Note, not necessarily related to this CL: while looking at whether it was > > > possible to create a dummy instance, I noticed that > > > NotImplementedPairingRegistryDelegate is a bit too literal :) - please take > > the > > > opportunity to either remove the declaration or add a definition for > ::Save). > > > > It says quite clearly that it's not implemented. What more do you want? ;) > > > > I've actually already fixed this in https://codereview.chromium.org/14995019/. > > Regarding passing NULL, the reason I'm doing that for now is that I don't want > > the UI to present the option of pairing until it's fully implemented. The > > simplest way of doing that is by not creating the registry. It will also make > it > > easy to support a policy to disable pairing. Creating a dummy implementation > > doesn't really make sense because it's not an interface. Even if I made it > into > > an interface, it would still need something along the lines of an > IsImplemented > > method in order to prevent pairing from being offered. This seems like > overkill > > to me. > > > > I've added a comment to NegotiatingHostAuthenticator::CreateWithSharedSecret > to > > explain. > > I see, I agree passing NULL is the right move in this case. But since that's the > case, [nit:] I think actually passing NULL in the parameter list makes the > intention more readable. The reason I made it separate is that the pairing registry is just one of many parameters, so a comment above the CreateWithSharedSecret wouldn't be as clear. If you still think I should change it, I don't mind, but I think this approach is clearer. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 00:46:25, rmsousa wrote: > On 2013/05/15 23:41:08, Jamie wrote: > > On 2013/05/15 01:25:16, rmsousa wrote: > > > This is wrong when the host sends the first message to the (unpaired) client > - > > > the host is already using PIN authentication. > > > > > > if you delay the v2_authenticator_ creation, its existence seems like a > better > > > check here (and initial_message_sent_ isn't necessary at all). > > > > My understanding is that the client always sends the first message--the > > optimistic client-id payload on the supported-methods tag. Is that not the > case? > > No. If the client does not have a client ID, the code on > NegotiatingClientAuthenticator does not pick a method or create an > authenticator. This will cause the host to pick the spake2_pair method, and > create PairingHostAuthenticator in the MESSAGE_READY state (meaning it will send > the first message of the PIN-based V2Authenticator). Then the client will > receive that message, and create a PairingClientAuthenticator in the > WAITING_MESSAGE state. > > > > > In any case, I've modified this method in light of your previous comment to > have > > an explicit pairing_state_ member, which may address your concerns. Okay, I think we're talking at cross-purposes here. I agree that in this case, the host sends the first message in the underlying authentication protocol. However, in this case we won't be using this class at all. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_hos... remoting/protocol/pairing_host_authenticator.cc:125: local_cert_, key_pair_, shared_secret_, initial_state_); On 2013/05/16 00:46:25, rmsousa wrote: > On 2013/05/15 23:41:08, Jamie wrote: > > On 2013/05/15 01:25:16, rmsousa wrote: > > > Using initial_state_ is weird here. There are several different flows > > happening, > > > with different V2Authenticator states on each side: > > > > > > (1) Client sends client_id, host recognizes: > > > C -> H client_id > > > H -> C first spake message > > > (...) > > > (2) Client sends client_id, host does not recognize: > > > C -> H client_id > > > H -> C error_message > > > C -> H first spake message > > > (...) > > > (3) Client does not send a client_id: > > > C -> H blank > > > H -> C first spake message > > > > > > And then on the client there is the opposite mapping, of processing or not > > > processing the first host message based on initial_state_. > > > > > > The host always has all the information required to send teh first SPAKE > > message > > > - It seems simpler to determine that it always does so. > > > > It doesn't always send the first message. If the client has specified an > > unrecognized id (the pairing has been revoked, for example), the host replies > > with a pairing-specific message to tell it to request the PIN instead. In this > > case, the client initiates the SPAKE exchange. I've removed the reference to > > initial_state_ here though. > > Yes, I was saying that this variation was confusing. There are two cases where > SPAKE is host initiated, and one where it is client initiated. The host already > has the information to start the SPAKE negotiation in the pairing rejected case, > there's no reason to wait for the client to start it. Okay, I understand now. I agree, your approach is better. Done. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/negotiat... File remoting/protocol/negotiating_authenticator_unittest.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/negotiat... remoting/protocol/negotiating_authenticator_unittest.cc:131: NegotiatingClientAuthenticator* client_as_negotiating_authenticator_; On 2013/05/16 00:46:25, rmsousa wrote: > Nit: why not just static_cast<NegotiatingAuthenticatorBase>(client_)? It would introduce a hidden, type-unsafe, dependency between the creation and access of client_. It's not a big deal, but this felt like the safer approach. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/negotiat... remoting/protocol/negotiating_authenticator_unittest.cc:190: kNoClientId, kTestSharedSecret, kTestSharedSecret, On 2013/05/16 00:52:39, rmsousa wrote: > This is only passing because the same string is being used as client secret and > PIN. In this case, teh client is currently using the ClientSecret flow, and the > host is using the PIN flow. Please define different strings for ClientSecret and > PIN. Ah yes. I knew there was something I intended to fix up here... Well spotted! Fixed. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:62: const base::Closure& resume_callback) { On 2013/05/16 00:46:25, rmsousa wrote: > please DCHECK_EQ(state(), WAITING_MESSAGE) Done. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:75: } else { On 2013/05/16 00:46:25, rmsousa wrote: > This could either be a PIN based V2 message or a Shared Secret V2 Message, > depending on whether the client has a client ID or not. If the pairing authenticator is in use, then the client must have a client id, otherwise the negotiating authenticator wouldn't have created one. I think part of the confusion there might be that the protocol type will be Spake2Pair, but it will be handled entirely by the V2Authenticator class. I can see how that could be confusing, but I'm not sure how best to resolve it. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:89: // If the initial message has not yet been sent, return it now. On 2013/05/16 00:46:25, rmsousa wrote: > Please DCHECK_EQ(state(), MESSAGE_READY) Done. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:110: const std::string& shared_secret) { On 2013/05/16 00:46:25, rmsousa wrote: > Nit: The term "Secret" is a bit ambiguous here, so it isn't clear that we know > the pairing state is rejected because this method is only called when we ask the > user for a PIN. > I think naming it CreateV2AuthenticatorWithPIN would make it a bit clearer (and > symmetrical with the host) Fair point. I've made the authenticator classes refer to "PIN" vs. "paired secret" to make it explicit which is which. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.h:39: // HMAC_SHA256--only the protocol type differs, which the client uses On 2013/05/16 00:46:25, rmsousa wrote: > nit: s/protocol type/method name/ Done.
https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/host/remoting_me2me_... remoting/host/remoting_me2me_host.cc:471: scoped_refptr<remoting::protocol::PairingRegistry> pairing_registry; On 2013/05/16 19:30:16, Jamie wrote: > On 2013/05/16 00:46:25, rmsousa wrote: > > On 2013/05/15 23:41:08, Jamie wrote: > > > On 2013/05/15 01:25:16, rmsousa wrote: > > > > Nit: Please either pass NULL explicitly (and move the comment down), or > > create > > > > an actual (possibly dummy) instance. Passing an implicitly null pointer is > a > > > bit > > > > harder to read. > > > > > > > > (Note, not necessarily related to this CL: while looking at whether it was > > > > possible to create a dummy instance, I noticed that > > > > NotImplementedPairingRegistryDelegate is a bit too literal :) - please > take > > > the > > > > opportunity to either remove the declaration or add a definition for > > ::Save). > > > > > > It says quite clearly that it's not implemented. What more do you want? ;) > > > > > > I've actually already fixed this in > https://codereview.chromium.org/14995019/. > > > Regarding passing NULL, the reason I'm doing that for now is that I don't > want > > > the UI to present the option of pairing until it's fully implemented. The > > > simplest way of doing that is by not creating the registry. It will also > make > > it > > > easy to support a policy to disable pairing. Creating a dummy implementation > > > doesn't really make sense because it's not an interface. Even if I made it > > into > > > an interface, it would still need something along the lines of an > > IsImplemented > > > method in order to prevent pairing from being offered. This seems like > > overkill > > > to me. > > > > > > I've added a comment to NegotiatingHostAuthenticator::CreateWithSharedSecret > > to > > > explain. > > > > I see, I agree passing NULL is the right move in this case. But since that's > the > > case, [nit:] I think actually passing NULL in the parameter list makes the > > intention more readable. > > The reason I made it separate is that the pairing registry is just one of many > parameters, so a comment above the CreateWithSharedSecret wouldn't be as clear. > If you still think I should change it, I don't mind, but I think this approach > is clearer. I see what you mean, good point. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_client_authenticator.cc:75: } else { On 2013/05/16 19:30:16, Jamie wrote: > On 2013/05/16 00:46:25, rmsousa wrote: > > This could either be a PIN based V2 message or a Shared Secret V2 Message, > > depending on whether the client has a client ID or not. > > If the pairing authenticator is in use, then the client must have a client id, > otherwise the negotiating authenticator wouldn't have created one. I think part > of the confusion there might be that the protocol type will be Spake2Pair, but > it will be handled entirely by the V2Authenticator class. I can see how that > could be confusing, but I'm not sure how best to resolve it. I see. But I think you'll need to use this authenticator for that case as well. See the other comment. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.cc:64: DCHECK(v2_authenticator_); On 2013/05/16 00:46:25, rmsousa wrote: > if (!v2_authenticator_) return PROTOCOL_ERROR Did you miss this one? https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.cc:83: if (pairing_tag) { On 2013/05/16 00:46:25, rmsousa wrote: > Lack of pairing tag should be an error as well. Did you miss this one? https://codereview.chromium.org/14793021/diff/22001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/22001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:65: if (v2_authenticator_) { This isn't really necessary, the else case below already handles this case. If you want to keep this one for readability, you can remove the "if (!v2_authenticator_)" below. https://codereview.chromium.org/14793021/diff/22001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:80: weak_factory_.GetWeakPtr(), message, resume_callback); Strange, I think passing a naked pointer here should have given you a compiler warning/error. In any case: message is owned by the caller, and is deleted as soon as this method returns - by the time the callback comes in, that object is long gone. You need to make a copy of it, owned by the callback: base::Owned(new buzz::XmlElement(*message)) See https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/... for a very similar case.
(argh... this other comment, that I had forgotten to save before sending the first message.) https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 19:30:16, Jamie wrote: > On 2013/05/16 00:46:25, rmsousa wrote: > > On 2013/05/15 23:41:08, Jamie wrote: > > > On 2013/05/15 01:25:16, rmsousa wrote: > > > > This is wrong when the host sends the first message to the (unpaired) > client > > - > > > > the host is already using PIN authentication. > > > > > > > > if you delay the v2_authenticator_ creation, its existence seems like a > > better > > > > check here (and initial_message_sent_ isn't necessary at all). > > > > > > My understanding is that the client always sends the first message--the > > > optimistic client-id payload on the supported-methods tag. Is that not the > > case? > > > > No. If the client does not have a client ID, the code on > > NegotiatingClientAuthenticator does not pick a method or create an > > authenticator. This will cause the host to pick the spake2_pair method, and > > create PairingHostAuthenticator in the MESSAGE_READY state (meaning it will > send > > the first message of the PIN-based V2Authenticator). Then the client will > > receive that message, and create a PairingClientAuthenticator in the > > WAITING_MESSAGE state. > > > > > > > > In any case, I've modified this method in light of your previous comment to > > have > > > an explicit pairing_state_ member, which may address your concerns. > > Okay, I think we're talking at cross-purposes here. I agree that in this case, > the host sends the first message in the underlying authentication protocol. > However, in this case we won't be using this class at all. Oh. I just realized that - CreateAuthenticator will create a V2Authenticator for this method. In that case, you have a different problem - if you use that authenticator, it will use the default pin fetcher (without the checkbox). I think the better way to fix that is to use this authenticator for every case where method = spake2_pair.
ptal https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 20:12:59, rmsousa wrote: > On 2013/05/16 19:30:16, Jamie wrote: > > On 2013/05/16 00:46:25, rmsousa wrote: > > > On 2013/05/15 23:41:08, Jamie wrote: > > > > On 2013/05/15 01:25:16, rmsousa wrote: > > > > > This is wrong when the host sends the first message to the (unpaired) > > client > > > - > > > > > the host is already using PIN authentication. > > > > > > > > > > if you delay the v2_authenticator_ creation, its existence seems like a > > > better > > > > > check here (and initial_message_sent_ isn't necessary at all). > > > > > > > > My understanding is that the client always sends the first message--the > > > > optimistic client-id payload on the supported-methods tag. Is that not the > > > case? > > > > > > No. If the client does not have a client ID, the code on > > > NegotiatingClientAuthenticator does not pick a method or create an > > > authenticator. This will cause the host to pick the spake2_pair method, and > > > create PairingHostAuthenticator in the MESSAGE_READY state (meaning it will > > send > > > the first message of the PIN-based V2Authenticator). Then the client will > > > receive that message, and create a PairingClientAuthenticator in the > > > WAITING_MESSAGE state. > > > > > > > > > > > In any case, I've modified this method in light of your previous comment > to > > > have > > > > an explicit pairing_state_ member, which may address your concerns. > > > > Okay, I think we're talking at cross-purposes here. I agree that in this case, > > the host sends the first message in the underlying authentication protocol. > > However, in this case we won't be using this class at all. > > Oh. I just realized that - CreateAuthenticator will create a V2Authenticator for > this method. In that case, you have a different problem - if you use that > authenticator, it will use the default pin fetcher (without the checkbox). I > think the better way to fix that is to use this authenticator for every case > where method = spake2_pair. I don't think that's true. The PairingSupportedButNotPaired test covers that case, and it verifies that the method name exposed to the client is Spake2Pair. Do you think it's worth changing the implementation anyway to make it clearer that the pairing authenticator is always used for Spake2Pair (even if it's just passing messages through to the v2 authenticator)? I worry that it might make the implementation more complicated than it needs to be, but I haven't tried it yet. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.cc:64: DCHECK(v2_authenticator_); On 2013/05/16 00:46:25, rmsousa wrote: > if (!v2_authenticator_) return PROTOCOL_ERROR I did, but looking at it, it seems redundant given the DCHECK. I appreciate that the DCHECK doesn't do anything in release mode, but I'd prefer not to mask a coding error with a silent failure. A crash with a stack trace is easier for us to fix. I did need to modify it so that if we've flagged an explicit protocol error, then we return it here, but I think that's better than treating absence of the v2_authenticator when this function is called as a protocol error. https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.cc:83: if (pairing_tag) { On 2013/05/16 20:11:02, rmsousa wrote: > On 2013/05/16 00:46:25, rmsousa wrote: > > Lack of pairing tag should be an error as well. > > Did you miss this one? Sorry, yes. Done. https://codereview.chromium.org/14793021/diff/22001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/22001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:65: if (v2_authenticator_) { On 2013/05/16 20:11:02, rmsousa wrote: > This isn't really necessary, the else case below already handles this case. If > you want to keep this one for readability, you can remove the "if > (!v2_authenticator_)" below. I realized the same thing when I was implementing support for corrupt paired secrets. It's gone now. https://codereview.chromium.org/14793021/diff/22001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:80: weak_factory_.GetWeakPtr(), message, resume_callback); On 2013/05/16 20:11:02, rmsousa wrote: > Strange, I think passing a naked pointer here should have given you a compiler > warning/error. > > In any case: message is owned by the caller, and is deleted as soon as this > method returns - by the time the callback comes in, that object is long gone. > You need to make a copy of it, owned by the callback: base::Owned(new > buzz::XmlElement(*message)) > > See > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/... > for a very similar case. Done.
https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:73: weak_factory_.GetWeakPtr(), resume_callback); (Comment for a future CL, when you implement the UI) It might be a good idea to pass the pairing error in the fetch_secret_callback. If I connect to a host to which I'm supposed to be paired, and end up getting a PIN dialog, I'd like to know if that's because my local Chrome isn't paired, the host doesn't recognize my pairing ID, or the host's registry is corrupted. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 21:09:09, Jamie wrote: > On 2013/05/16 20:12:59, rmsousa wrote: > > On 2013/05/16 19:30:16, Jamie wrote: > > > On 2013/05/16 00:46:25, rmsousa wrote: > > > > On 2013/05/15 23:41:08, Jamie wrote: > > > > > On 2013/05/15 01:25:16, rmsousa wrote: > > > > > > This is wrong when the host sends the first message to the (unpaired) > > > client > > > > - > > > > > > the host is already using PIN authentication. > > > > > > > > > > > > if you delay the v2_authenticator_ creation, its existence seems like > a > > > > better > > > > > > check here (and initial_message_sent_ isn't necessary at all). > > > > > > > > > > My understanding is that the client always sends the first message--the > > > > > optimistic client-id payload on the supported-methods tag. Is that not > the > > > > case? > > > > > > > > No. If the client does not have a client ID, the code on > > > > NegotiatingClientAuthenticator does not pick a method or create an > > > > authenticator. This will cause the host to pick the spake2_pair method, > and > > > > create PairingHostAuthenticator in the MESSAGE_READY state (meaning it > will > > > send > > > > the first message of the PIN-based V2Authenticator). Then the client will > > > > receive that message, and create a PairingClientAuthenticator in the > > > > WAITING_MESSAGE state. > > > > > > > > > > > > > > In any case, I've modified this method in light of your previous comment > > to > > > > have > > > > > an explicit pairing_state_ member, which may address your concerns. > > > > > > Okay, I think we're talking at cross-purposes here. I agree that in this > case, > > > the host sends the first message in the underlying authentication protocol. > > > However, in this case we won't be using this class at all. > > > > Oh. I just realized that - CreateAuthenticator will create a V2Authenticator > for > > this method. In that case, you have a different problem - if you use that > > authenticator, it will use the default pin fetcher (without the checkbox). I > > think the better way to fix that is to use this authenticator for every case > > where method = spake2_pair. > > I don't think that's true. The PairingSupportedButNotPaired test covers that > case, and it verifies that the method name exposed to the client is Spake2Pair. > Do you think it's worth changing the implementation anyway to make it clearer > that the pairing authenticator is always used for Spake2Pair (even if it's just > passing messages through to the v2 authenticator)? I worry that it might make > the implementation more complicated than it needs to be, but I haven't tried it > yet. Oh, I think I misunderstood your design. I was assuming that you added current_method() to NegotiatingAuthenticator just for the unit tests. If you were you planning to have the client go back to the negotiating authenticator when it receives a fetch_secret_callback_, to check the current method, and use that to decide whether to show the checkbox, I don't think that would work - the client has no pointer to the authenticator after the session is started (and that is by design). That information will need to be passed as a parameter in the callback itself. As far as putting the logic inside this class: we should keep a coherent mapping of authentication method to authenticator class, and the logic for an authentication method should be entirely contained in its class, without spilling on the negotiating authenticator. We shouldn't have a method where we don't know what authenticator class is currently being used, or where the host may be talking to two different client classes - that makes future backwards compatibility much more difficult. (You may notice that V2Authenticator is an anti-pattern to pretty much everything I just said - there's quite a bit of its logic living on the NegotiatingAuthenticator. But that actually reinforces this point - the only reason why it's so messy is because previous clients/hosts could have ambiguous V2Authenticator states for the same method name, and when we added the async pin flow (which also had to use the same method name to be backwards compatible), we had to deal with all this ambiguity to keep it backwards compatible with older webapps/hosts). https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/8001/remoting/protocol/pairing_... remoting/protocol/pairing_host_authenticator.cc:64: DCHECK(v2_authenticator_); On 2013/05/16 21:09:10, Jamie wrote: > On 2013/05/16 00:46:25, rmsousa wrote: > > if (!v2_authenticator_) return PROTOCOL_ERROR > > I did, but looking at it, it seems redundant given the DCHECK. I appreciate that > the DCHECK doesn't do anything in release mode, but I'd prefer not to mask a > coding error with a silent failure. A crash with a stack trace is easier for us > to fix. > > I did need to modify it so that if we've flagged an explicit protocol error, > then we return it here, but I think that's better than treating absence of the > v2_authenticator when this function is called as a protocol error. Yes, that's what I meant - you needed to handle the protocol_error_ case here, and that happens if and only if v2_authenticator_ is null (this function is only called when state() == REJECTED). You can either implicitly check v2_authenticator_ and do away with the protocol_error_ attribute, or have a separate protocol_error_ boolean and test it explicitly (in which case I agree that you should keep the DCHECK).
I'll follow-up with a version that always uses the pairing authenticator for Spake2Pair, but I wanted to check we're on the same page re: the eventual checkbox test and use of current_method(). https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:73: weak_factory_.GetWeakPtr(), resume_callback); On 2013/05/16 22:16:37, rmsousa wrote: > (Comment for a future CL, when you implement the UI) > It might be a good idea to pass the pairing error in the fetch_secret_callback. > If I connect to a host to which I'm supposed to be paired, and end up getting a > PIN dialog, I'd like to know if that's because my local Chrome isn't paired, the > host doesn't recognize my pairing ID, or the host's registry is corrupted. Good point. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 22:16:37, rmsousa wrote: > On 2013/05/16 21:09:09, Jamie wrote: > > On 2013/05/16 20:12:59, rmsousa wrote: > > > On 2013/05/16 19:30:16, Jamie wrote: > > > > On 2013/05/16 00:46:25, rmsousa wrote: > > > > > On 2013/05/15 23:41:08, Jamie wrote: > > > > > > On 2013/05/15 01:25:16, rmsousa wrote: > > > > > > > This is wrong when the host sends the first message to the > (unpaired) > > > > client > > > > > - > > > > > > > the host is already using PIN authentication. > > > > > > > > > > > > > > if you delay the v2_authenticator_ creation, its existence seems > like > > a > > > > > better > > > > > > > check here (and initial_message_sent_ isn't necessary at all). > > > > > > > > > > > > My understanding is that the client always sends the first > message--the > > > > > > optimistic client-id payload on the supported-methods tag. Is that not > > the > > > > > case? > > > > > > > > > > No. If the client does not have a client ID, the code on > > > > > NegotiatingClientAuthenticator does not pick a method or create an > > > > > authenticator. This will cause the host to pick the spake2_pair method, > > and > > > > > create PairingHostAuthenticator in the MESSAGE_READY state (meaning it > > will > > > > send > > > > > the first message of the PIN-based V2Authenticator). Then the client > will > > > > > receive that message, and create a PairingClientAuthenticator in the > > > > > WAITING_MESSAGE state. > > > > > > > > > > > > > > > > > In any case, I've modified this method in light of your previous > comment > > > to > > > > > have > > > > > > an explicit pairing_state_ member, which may address your concerns. > > > > > > > > Okay, I think we're talking at cross-purposes here. I agree that in this > > case, > > > > the host sends the first message in the underlying authentication > protocol. > > > > However, in this case we won't be using this class at all. > > > > > > Oh. I just realized that - CreateAuthenticator will create a V2Authenticator > > for > > > this method. In that case, you have a different problem - if you use that > > > authenticator, it will use the default pin fetcher (without the checkbox). I > > > think the better way to fix that is to use this authenticator for every case > > > where method = spake2_pair. > > > > I don't think that's true. The PairingSupportedButNotPaired test covers that > > case, and it verifies that the method name exposed to the client is > Spake2Pair. > > Do you think it's worth changing the implementation anyway to make it clearer > > that the pairing authenticator is always used for Spake2Pair (even if it's > just > > passing messages through to the v2 authenticator)? I worry that it might make > > the implementation more complicated than it needs to be, but I haven't tried > it > > yet. > > Oh, I think I misunderstood your design. I was assuming that you added > current_method() to NegotiatingAuthenticator just for the unit tests. That's correct. > If you were you planning to have the client go back to the negotiating > authenticator when it receives a fetch_secret_callback_, to check the current > method, and use that to decide whether to show the checkbox, I don't think that > would work - the client has no pointer to the authenticator after the session is > started (and that is by design). That information will need to be passed as a > parameter in the callback itself. The way this is working in my uncommitted branch right now is that when the negotiating authenticator asks the client to prompt for a PIN, it passes a bool which it sets to (current_method_ == Spake2Pair). In the rare cases when the pairing authenticator prompts directly (when pairing fails), it just passes true. That flag controls whether or not to show the "remember me" checkbox. The way this interacts with the unit tests is that right now I want to be able to verify that the correct protocol was used. Once the callback has a bool parameter, I'll be able to check that directly, and add checks to make sure that the callback isn't called at all when I don't expect it to be. At that point, I can probably remove the current_method accessor again. > As far as putting the logic inside this class: we should keep a coherent mapping > of authentication method to authenticator class, and the logic for an > authentication method should be entirely contained in its class, without > spilling on the negotiating authenticator. We shouldn't have a method where we > don't know what authenticator class is currently being used, or where the host > may be talking to two different client classes - that makes future backwards > compatibility much more difficult. Yes, I think I agree. Let me see what the code looks like if it always uses the pairing authenticators for Spake2Pair. > > (You may notice that V2Authenticator is an anti-pattern to pretty much > everything I just said - there's quite a bit of its logic living on the > NegotiatingAuthenticator. But that actually reinforces this point - the only > reason why it's so messy is because previous clients/hosts could have ambiguous > V2Authenticator states for the same method name, and when we added the async pin > flow (which also had to use the same method name to be backwards compatible), we > had to deal with all this ambiguity to keep it backwards compatible with older > webapps/hosts).
https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 22:57:46, Jamie wrote: > On 2013/05/16 22:16:37, rmsousa wrote: > > On 2013/05/16 21:09:09, Jamie wrote: > > > On 2013/05/16 20:12:59, rmsousa wrote: > > > > On 2013/05/16 19:30:16, Jamie wrote: > > > > > On 2013/05/16 00:46:25, rmsousa wrote: > > > > > > On 2013/05/15 23:41:08, Jamie wrote: > > > > > > > On 2013/05/15 01:25:16, rmsousa wrote: > > > > > > > > This is wrong when the host sends the first message to the > > (unpaired) > > > > > client > > > > > > - > > > > > > > > the host is already using PIN authentication. > > > > > > > > > > > > > > > > if you delay the v2_authenticator_ creation, its existence seems > > like > > > a > > > > > > better > > > > > > > > check here (and initial_message_sent_ isn't necessary at all). > > > > > > > > > > > > > > My understanding is that the client always sends the first > > message--the > > > > > > > optimistic client-id payload on the supported-methods tag. Is that > not > > > the > > > > > > case? > > > > > > > > > > > > No. If the client does not have a client ID, the code on > > > > > > NegotiatingClientAuthenticator does not pick a method or create an > > > > > > authenticator. This will cause the host to pick the spake2_pair > method, > > > and > > > > > > create PairingHostAuthenticator in the MESSAGE_READY state (meaning it > > > will > > > > > send > > > > > > the first message of the PIN-based V2Authenticator). Then the client > > will > > > > > > receive that message, and create a PairingClientAuthenticator in the > > > > > > WAITING_MESSAGE state. > > > > > > > > > > > > > > > > > > > > In any case, I've modified this method in light of your previous > > comment > > > > to > > > > > > have > > > > > > > an explicit pairing_state_ member, which may address your concerns. > > > > > > > > > > Okay, I think we're talking at cross-purposes here. I agree that in this > > > case, > > > > > the host sends the first message in the underlying authentication > > protocol. > > > > > However, in this case we won't be using this class at all. > > > > > > > > Oh. I just realized that - CreateAuthenticator will create a > V2Authenticator > > > for > > > > this method. In that case, you have a different problem - if you use that > > > > authenticator, it will use the default pin fetcher (without the checkbox). > I > > > > think the better way to fix that is to use this authenticator for every > case > > > > where method = spake2_pair. > > > > > > I don't think that's true. The PairingSupportedButNotPaired test covers that > > > case, and it verifies that the method name exposed to the client is > > Spake2Pair. > > > Do you think it's worth changing the implementation anyway to make it > clearer > > > that the pairing authenticator is always used for Spake2Pair (even if it's > > just > > > passing messages through to the v2 authenticator)? I worry that it might > make > > > the implementation more complicated than it needs to be, but I haven't tried > > it > > > yet. > > > > Oh, I think I misunderstood your design. I was assuming that you added > > current_method() to NegotiatingAuthenticator just for the unit tests. > > That's correct. > > > If you were you planning to have the client go back to the negotiating > > authenticator when it receives a fetch_secret_callback_, to check the current > > method, and use that to decide whether to show the checkbox, I don't think > that > > would work - the client has no pointer to the authenticator after the session > is > > started (and that is by design). That information will need to be passed as a > > parameter in the callback itself. > > The way this is working in my uncommitted branch right now is that when the > negotiating authenticator asks the client to prompt for a PIN, it passes a bool > which it sets to (current_method_ == Spake2Pair). In the rare cases when the > pairing authenticator prompts directly (when pairing fails), it just passes > true. That flag controls whether or not to show the "remember me" checkbox. Oh, OK, we're in the same page, then. > The way this interacts with the unit tests is that right now I want to be able > to verify that the correct protocol was used. Once the callback has a bool > parameter, I'll be able to check that directly, and add checks to make sure that > the callback isn't called at all when I don't expect it to be. At that point, I > can probably remove the current_method accessor again. > > > As far as putting the logic inside this class: we should keep a coherent > mapping > > of authentication method to authenticator class, and the logic for an > > authentication method should be entirely contained in its class, without > > spilling on the negotiating authenticator. We shouldn't have a method where we > > don't know what authenticator class is currently being used, or where the host > > may be talking to two different client classes - that makes future backwards > > compatibility much more difficult. > > Yes, I think I agree. Let me see what the code looks like if it always uses the > pairing authenticators for Spake2Pair. If it ends up too ugly, another option to at least keep them symmetric between client and host is to change negotiating_host_authenticator::CreateAuthenticator to create a V2Autehnticator for method=spake2_pair, state=MESSAGE_READY, and a PairingAutehnticator for method=spake2_pair, state = WAITING_MESSAGE. That also cleans up some of the initial_state handling in PairingHostAuthentciator. I'm fine with either approach. > > > > > (You may notice that V2Authenticator is an anti-pattern to pretty much > > everything I just said - there's quite a bit of its logic living on the > > NegotiatingAuthenticator. But that actually reinforces this point - the only > > reason why it's so messy is because previous clients/hosts could have > ambiguous > > V2Authenticator states for the same method name, and when we added the async > pin > > flow (which also had to use the same method name to be backwards compatible), > we > > had to deal with all this ambiguity to keep it backwards compatible with older > > webapps/hosts). >
https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:475: nit: remove this line. https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:483: nit: remove this line. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:27: return AuthenticationMethod(SPAKE2_PAIR, HMAC_SHA256); Do we really need to hash the secret? I think we can use NONE here. It's useful for pin-based when user reuses the same pin for multiple machines. Since pairing secret generated randomly hashing it doesn't really add anything. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:101: case INVALID: This is not necessary - there is DCHECK on top. Maybe remove DCHECK and add NOTREACHED() here. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (left): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:91: nit: please keep this empty line https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:123: } else { I think this should check that current_method_ is seto to spake2(). Also there should be case for pairing. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:137: current_method_ = AuthenticationMethod::Spake2Pair(); Please DCHECK here that pairing auth is in the list of supported auth methods. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:20: // These definitions must be kept in sync with pairing_host_authenticator.cc. For some authenticators (e.g. third-party) we have base class that defines shared logic, including message definitions. Maybe do the same thing here? https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:26: { kChromotingXmlNamespace, "pairing-failed" }; I'm not sure why we need pairing-failed message. Instead of sending this message can the host just choose regular non-paired authenticator? https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:103: return result.Pass(); Can we also include include first message from v2_authenticator_ in the first request? This will reduce number of roundtrips to finish authentication. The host can just ignore that message if it doesn't accept it (e.g. if it didn't recognize client id). https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:107: DCHECK(v2_authenticator_); nit: don't need this. scoped_ptr<> DCHECKs in operator-> anyway. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:13: class RsaKeyPair; nit: Not used. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:19: typedef base::Callback<void(const std::string& secret)> SecretFetchedCallback; These are also declared in negotiating_client_authenticator.h. Maybe move them somewhere to avoid duplication (e.g. authenticator.h). Also I think they should be called AuthSecretFetchedCallback and FetchAuthSecretCallback. Or maybe just put them inside Authenticator interface? https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:42: class PairingClientAuthenticator : public Authenticator { Can you please add some unittests for the new authenticator. It's important that we have unittests for all authenticators. Having many bugs here would be dangerous. I saw that you added unittests for negotiating authenticator, but I think we should also have unittests for this code without the negotiating layer.
rmsousa: ptal. sergeyu: I'm working on your comments. https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/1/remoting/protocol/pairing_cli... remoting/protocol/pairing_client_authenticator.cc:82: if (!initial_message_sent_) { On 2013/05/16 23:22:24, rmsousa wrote: > On 2013/05/16 22:57:46, Jamie wrote: > > On 2013/05/16 22:16:37, rmsousa wrote: > > > On 2013/05/16 21:09:09, Jamie wrote: > > > > On 2013/05/16 20:12:59, rmsousa wrote: > > > > > On 2013/05/16 19:30:16, Jamie wrote: > > > > > > On 2013/05/16 00:46:25, rmsousa wrote: > > > > > > > On 2013/05/15 23:41:08, Jamie wrote: > > > > > > > > On 2013/05/15 01:25:16, rmsousa wrote: > > > > > > > > > This is wrong when the host sends the first message to the > > > (unpaired) > > > > > > client > > > > > > > - > > > > > > > > > the host is already using PIN authentication. > > > > > > > > > > > > > > > > > > if you delay the v2_authenticator_ creation, its existence seems > > > like > > > > a > > > > > > > better > > > > > > > > > check here (and initial_message_sent_ isn't necessary at all). > > > > > > > > > > > > > > > > My understanding is that the client always sends the first > > > message--the > > > > > > > > optimistic client-id payload on the supported-methods tag. Is that > > not > > > > the > > > > > > > case? > > > > > > > > > > > > > > No. If the client does not have a client ID, the code on > > > > > > > NegotiatingClientAuthenticator does not pick a method or create an > > > > > > > authenticator. This will cause the host to pick the spake2_pair > > method, > > > > and > > > > > > > create PairingHostAuthenticator in the MESSAGE_READY state (meaning > it > > > > will > > > > > > send > > > > > > > the first message of the PIN-based V2Authenticator). Then the client > > > will > > > > > > > receive that message, and create a PairingClientAuthenticator in the > > > > > > > WAITING_MESSAGE state. > > > > > > > > > > > > > > > > > > > > > > > In any case, I've modified this method in light of your previous > > > comment > > > > > to > > > > > > > have > > > > > > > > an explicit pairing_state_ member, which may address your > concerns. > > > > > > > > > > > > Okay, I think we're talking at cross-purposes here. I agree that in > this > > > > case, > > > > > > the host sends the first message in the underlying authentication > > > protocol. > > > > > > However, in this case we won't be using this class at all. > > > > > > > > > > Oh. I just realized that - CreateAuthenticator will create a > > V2Authenticator > > > > for > > > > > this method. In that case, you have a different problem - if you use > that > > > > > authenticator, it will use the default pin fetcher (without the > checkbox). > > I > > > > > think the better way to fix that is to use this authenticator for every > > case > > > > > where method = spake2_pair. > > > > > > > > I don't think that's true. The PairingSupportedButNotPaired test covers > that > > > > case, and it verifies that the method name exposed to the client is > > > Spake2Pair. > > > > Do you think it's worth changing the implementation anyway to make it > > clearer > > > > that the pairing authenticator is always used for Spake2Pair (even if it's > > > just > > > > passing messages through to the v2 authenticator)? I worry that it might > > make > > > > the implementation more complicated than it needs to be, but I haven't > tried > > > it > > > > yet. > > > > > > Oh, I think I misunderstood your design. I was assuming that you added > > > current_method() to NegotiatingAuthenticator just for the unit tests. > > > > That's correct. > > > > > If you were you planning to have the client go back to the negotiating > > > authenticator when it receives a fetch_secret_callback_, to check the > current > > > method, and use that to decide whether to show the checkbox, I don't think > > that > > > would work - the client has no pointer to the authenticator after the > session > > is > > > started (and that is by design). That information will need to be passed as > a > > > parameter in the callback itself. > > > > The way this is working in my uncommitted branch right now is that when the > > negotiating authenticator asks the client to prompt for a PIN, it passes a > bool > > which it sets to (current_method_ == Spake2Pair). In the rare cases when the > > pairing authenticator prompts directly (when pairing fails), it just passes > > true. That flag controls whether or not to show the "remember me" checkbox. > > Oh, OK, we're in the same page, then. > > > The way this interacts with the unit tests is that right now I want to be able > > to verify that the correct protocol was used. Once the callback has a bool > > parameter, I'll be able to check that directly, and add checks to make sure > that > > the callback isn't called at all when I don't expect it to be. At that point, > I > > can probably remove the current_method accessor again. > > > > > As far as putting the logic inside this class: we should keep a coherent > > mapping > > > of authentication method to authenticator class, and the logic for an > > > authentication method should be entirely contained in its class, without > > > spilling on the negotiating authenticator. We shouldn't have a method where > we > > > don't know what authenticator class is currently being used, or where the > host > > > may be talking to two different client classes - that makes future backwards > > > compatibility much more difficult. > > > > Yes, I think I agree. Let me see what the code looks like if it always uses > the > > pairing authenticators for Spake2Pair. > > If it ends up too ugly, another option to at least keep them symmetric between > client and host is to change negotiating_host_authenticator::CreateAuthenticator > to create a V2Autehnticator for method=spake2_pair, state=MESSAGE_READY, and a > PairingAutehnticator for method=spake2_pair, state = WAITING_MESSAGE. > That also cleans up some of the initial_state handling in > PairingHostAuthentciator. > > I'm fine with either approach. I tried using the pairing authenticator for the exchange even if the client is not already paired, but as I feared it makes the code a lot more complex and breaks one of your previous points about having the host always initiate the Spake exchange. You second option of making the two sides symmetric is much simpler, so I've gone with that.
https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:27: return AuthenticationMethod(SPAKE2_PAIR, HMAC_SHA256); On 2013/05/17 01:57:17, Sergey Ulanov wrote: > Do we really need to hash the secret? I think we can use NONE here. It's useful > for pin-based when user reuses the same pin for multiple machines. Since pairing > secret generated randomly hashing it doesn't really add anything. There are some cases where this falls back to the PIN. Using HMAC_SHA256 for everything allows this method to have a consistent hashing algorithm. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:123: } else { On 2013/05/17 01:57:17, Sergey Ulanov wrote: > I think this should check that current_method_ is seto to spake2(). Also there > should be case for pairing. It's intentionally using this for the using-pairing-but-unpaired case. But, as I mentioned, a DCHECK here for the cases which are intentionally being handled helps readability. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:137: current_method_ = AuthenticationMethod::Spake2Pair(); On 2013/05/17 01:57:17, Sergey Ulanov wrote: > Please DCHECK here that pairing auth is in the list of supported auth methods. Good point. Not just a DCHECK, IMO that test should be explicit. These are coming from the webapp, we shouldn't implicitly assume that the input from the webapp is consistent. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:26: { kChromotingXmlNamespace, "pairing-failed" }; On 2013/05/17 01:57:17, Sergey Ulanov wrote: > I'm not sure why we need pairing-failed message. Instead of sending this message > can the host just choose regular non-paired authenticator? The failure case needs to use the same method name because: (1) we dont' support falling back to another auhenticator on failure in the negotiating authenticator - if an authenticator rejects the client that's permanent; and (2) the client still needs to know that pairing is *supported* by the host (so that it can show the option to pair in the UI). Since we're using this authenticator for both cases, the client will receive a SPAKE message with either response. The message is needed to tell if that SPAKE message is based on the secret (success case), or the PIN (failure case). Aside from that, we can use the error attribute to show a message to the client when we ask for the PIN. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:19: typedef base::Callback<void(const std::string& secret)> SecretFetchedCallback; On 2013/05/17 01:57:17, Sergey Ulanov wrote: > These are also declared in negotiating_client_authenticator.h. Maybe move them > somewhere to avoid duplication (e.g. authenticator.h). Also I think they should > be called AuthSecretFetchedCallback and FetchAuthSecretCallback. Or maybe just > put them inside Authenticator interface? They are actually the same function as the regular FetchSecretCallback used for the PIN. But I agree they should be imported from a common place in that case (authenticator.h is a good suggestion). https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:123: } else { Nits: Please add a DCHECK that the method is spake or spakepair (that serves as documentation that those (and only those) are intentionally handled by this else). Also, please add a // TODO: add a parameter to fetch_secret_callback for whether pairing is supported.
Drive-by w/ some nits & suggestions. Looking pretty good, though. :) https://codereview.chromium.org/14793021/diff/53001/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:471: // is committed. nit: This comment implies that you're passing in a[n improper] PairingRegistry, but it looks like you're passing in NULL? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_base.h:74: const base::Closure& resume_callback); nit: This indentation looks broken. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_unittest.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_unittest.cc:100: void VerifyRejected(Authenticator::RejectionReason reason) { nit: It may be better to split out VerifyClientRejected and VerifyHostRejected(), so you can be more specific in the asymmetric auth cases below. (You might still want VerifyRejected to cope with the symmetric cases, though). https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_unittest.cc:183: TEST_F(NegotiatingAuthenticatorTest, PairingRequestedButNotSupported) { nit: This test doesn't actually seem to verify that pairing is requested, so call it PairingNotSupported? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (left): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:91: nit: Restore this blank line, please. :) https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:83: DCHECK_EQ(state(), MESSAGE_READY); nit: Blank line between check and comment, plz https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.h:30: // TODO(jamiewalch): Pass ClientConfig instead of separate parameters. nit: SGTM but let's split out the auth-related members to a ClientAuthConfig that ClientConfig can contain. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.h:61: // Sets |current_authenticator_| and |current_method_| iff the client Would it make more sense to add a getter to Authenticator for its AuthenticationMethod, so we don't have to keep a separate current_method_? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.h:77: // Used for both authenticators. nit: This probably needs updating, since there are more than two kinds of authenticator now (or perhaps "both" means "both client and host" here?) https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.cc:82: // If the client did not specify auth method or specified an unknown nit: comma between 'method' and 'or' https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.cc:82: // If the client did not specify auth method or specified an unknown nit: ".. specify a preferred auth method ..." ? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.h:32: // Creates a host authenticator, using a fixed shared secret/PIN hash. nit: Can we updated this e.g. to "... using the supplied PIN / Access Code's |shared_secret_hash|."? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.h:35: // NULL pairing registry. nit: Suggest "If |pairing_registry| is non-NULL then the SpakePair method will be offered, supporting PIN-less authentication." https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:20: // These definitions must be kept in sync with pairing_host_authenticator.cc. nit: Should we split out all the auth tags into a single auth_tags.[h|cc]? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:46: DCHECK(v2_authenticator_); nit: Won't state() already segfault if |v2_authenticator_| is NULL? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:55: DCHECK(v2_authenticator_); nit: Same here. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:79: Either get rid of the else, or the return. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:26: // * If a client device is already paired, it includes a client id in nit: Suggest capitalizing 'Client Id', so that when you refer to it in other comments it stands out as a specific technical term, similarly to 'Access Code', 'Shared Secret', 'Host Secret', etc elsewhere. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:36: // nit: Lose the extra blank comment https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:72: // created and holds the state of the overall auth exchange. This is a little confusing; are you saying that in both the ACCEPTED and REJECTED |pairing_state_| we defer overall to the V2 authenticator's state? Suggest something like: Stores the progress of the pairing exchange. During the pairing exchange this corresponds to the state of the authentication exchange as a whole. Once pairing is ACCEPTED or REJECTED then the underlying V2 authenticator determines the overall state. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:75: std::string client_id_; nit: Please add at least a block comment for these members. :) https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:81: base::WeakPtrFactory<PairingClientAuthenticator> weak_factory_; nit: Suggest a blank line preceding |weak_factory_|, so it stands out from the block of other members. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:60: DCHECK(v2_authenticator_); nit: rejection_reason() below will segfault if |v2_authenticator_| is NULL, so you don't strictly need this DCHECK. Plus, since it's a debug-only check, if there's a bug in the authenticator logic then a client can coax you into segfaulting anyway - to be defensive you could check |v2_authenticator_| first, then DCHECK(protocol_error_) but then return PROTOCOL_ERROR no matter what. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:117: DCHECK(v2_authenticator_); nit: See above - does the DCHECK add anything here? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:133: DCHECK(v2_authenticator_); nit: Ditto. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:138: DCHECK(!v2_authenticator_); nit: Here too https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:146: DCHECK(v2_authenticator_); nit: Ditto. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:148: // If the SPAKE exchange failed due to invalid credentials, and those nit: Blank lines before this comment, and after the code block, please. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:23: // * If a client device is already paired, it includes a client id in nit: See comment in client imp re capitalization. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:58: void CheckForFailedSpakeExchange(const base::Closure& resume_callback); nit: Comments for these members, please. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:60: scoped_refptr<PairingRegistry> pairing_registry_; nit: At least a block comment introducing these members, please. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:66: bool protocol_error_; nit: This name sounds like it stores an error, rather than just being an indicator that one occurred. Do you actually need it, or could you test for |error_message_| being empty? https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:68: base::WeakPtrFactory<PairingHostAuthenticator> weak_factory_; nit: Blank line before this, please.
I think I've addressed everyone's comments here. In order to address Sergey's comment about eliminating a RTT in one case, I had to reimplement a lot of the protocol logic as a common base class with a small number of call-outs to the client- and host-side subclasses, but I think it's cleaner this way. Since I reimplemented much of it, it's possible I've undone some previous reviewer requests (accidentally or otherwise). PTAL. https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:483: On 2013/05/17 01:57:17, Sergey Ulanov wrote: > nit: remove this line. I think it's more readable with these branches separated out a bit. If you feel strongly that it doesn't, then I'll remove the blank lines, but I'd prefer to leave them. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... File remoting/protocol/authentication_method.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:27: return AuthenticationMethod(SPAKE2_PAIR, HMAC_SHA256); On 2013/05/17 20:09:03, rmsousa wrote: > On 2013/05/17 01:57:17, Sergey Ulanov wrote: > > Do we really need to hash the secret? I think we can use NONE here. It's > useful > > for pin-based when user reuses the same pin for multiple machines. Since > pairing > > secret generated randomly hashing it doesn't really add anything. > > There are some cases where this falls back to the PIN. Using HMAC_SHA256 for > everything allows this method to have a consistent hashing algorithm. I've left it as HMAC_SHA256, for the reason Renato mentions. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/authent... remoting/protocol/authentication_method.cc:101: case INVALID: On 2013/05/17 01:57:17, Sergey Ulanov wrote: > This is not necessary - there is DCHECK on top. Maybe remove DCHECK and add > NOTREACHED() here. The compiler complains if it's not handled. NOTREACHED is a good suggestion though. Done. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (left): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:91: On 2013/05/17 01:57:17, Sergey Ulanov wrote: > nit: please keep this empty line Done. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:123: } else { On 2013/05/17 01:57:17, Sergey Ulanov wrote: > I think this should check that current_method_ is seto to spake2(). Also there > should be case for pairing. Please see the comment thread in response to Renato's request for the same thing. Pairing authenticators are only used if the client is already paired. If not, then the authentication protocol is just plain SPAKE2 using the PIN. Only the method name differs because that's how the client knows it should present the "remember me" checkbox (coming soon...) https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:137: current_method_ = AuthenticationMethod::Spake2Pair(); On 2013/05/17 20:09:03, rmsousa wrote: > On 2013/05/17 01:57:17, Sergey Ulanov wrote: > > Please DCHECK here that pairing auth is in the list of supported auth methods. > > Good point. Not just a DCHECK, IMO that test should be explicit. These are > coming from the webapp, we shouldn't implicitly assume that the input from the > webapp is consistent. Agreed. I've made it explicit. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:20: // These definitions must be kept in sync with pairing_host_authenticator.cc. On 2013/05/17 01:57:17, Sergey Ulanov wrote: > For some authenticators (e.g. third-party) we have base class that defines > shared logic, including message definitions. Maybe do the same thing here? In light of one of your other comments, I've had to introduce a base class containing the bulk of the shared logic, so I've moved these there. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:103: return result.Pass(); On 2013/05/17 01:57:17, Sergey Ulanov wrote: > Can we also include include first message from v2_authenticator_ in the first > request? This will reduce number of roundtrips to finish authentication. The > host can just ignore that message if it doesn't accept it (e.g. if it didn't > recognize client id). Great idea, thanks. Unfortunately, it meant a significant refactoring into a common base class, but I think it's worth it. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:107: DCHECK(v2_authenticator_); On 2013/05/17 01:57:17, Sergey Ulanov wrote: > nit: don't need this. scoped_ptr<> DCHECKs in operator-> anyway. Removed, here and elsewhere. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:13: class RsaKeyPair; On 2013/05/17 01:57:17, Sergey Ulanov wrote: > nit: Not used. Done. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:19: typedef base::Callback<void(const std::string& secret)> SecretFetchedCallback; On 2013/05/17 20:09:03, rmsousa wrote: > On 2013/05/17 01:57:17, Sergey Ulanov wrote: > > These are also declared in negotiating_client_authenticator.h. Maybe move > them > > somewhere to avoid duplication (e.g. authenticator.h). Also I think they > should > > be called AuthSecretFetchedCallback and FetchAuthSecretCallback. Or maybe just > > put them inside Authenticator interface? > > They are actually the same function as the regular FetchSecretCallback used for > the PIN. But I agree they should be imported from a common place in that case > (authenticator.h is a good suggestion). I've moved them to authenticator, but I've left the names as they are now to avoid changing more code than I need to in this CL. https://codereview.chromium.org/14793021/diff/42001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:42: class PairingClientAuthenticator : public Authenticator { On 2013/05/17 01:57:17, Sergey Ulanov wrote: > Can you please add some unittests for the new authenticator. It's important that > we have unittests for all authenticators. Having many bugs here would be > dangerous. I saw that you added unittests for negotiating authenticator, but I > think we should also have unittests for this code without the negotiating layer. > The tests in negotiating_authenticator_unittests are actually pretty comprehensive, so I'm not very concerned about coverage. I agree that it would be nice to test the pairing classes independently, but do you mind if I wait until the rest of this feature has landed? https://codereview.chromium.org/14793021/diff/53001/remoting/host/remoting_me... File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/host/remoting_me... remoting/host/remoting_me2me_host.cc:471: // is committed. On 2013/05/18 20:08:36, Wez wrote: > nit: This comment implies that you're passing in a[n improper] PairingRegistry, > but it looks like you're passing in NULL? I consider NULL to be highly improper :) I've reworded it. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_base.h:74: const base::Closure& resume_callback); On 2013/05/18 20:08:36, Wez wrote: > nit: This indentation looks broken. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_unittest.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_unittest.cc:100: void VerifyRejected(Authenticator::RejectionReason reason) { On 2013/05/18 20:08:36, Wez wrote: > nit: It may be better to split out VerifyClientRejected and > VerifyHostRejected(), so you can be more specific in the asymmetric auth cases > below. (You might still want VerifyRejected to cope with the symmetric cases, > though). I haven't changed the semantics of the test (at least, not intentionally), I've just made it easier to debug failures. I'll let Renato weigh on whether or not it's worth separating into separate tests, since he wrote the original test, but I don't think it needs to change for this CL. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_unittest.cc:183: TEST_F(NegotiatingAuthenticatorTest, PairingRequestedButNotSupported) { On 2013/05/18 20:08:36, Wez wrote: > nit: This test doesn't actually seem to verify that pairing is requested, so > call it PairingNotSupported? Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (left): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:91: On 2013/05/18 20:08:36, Wez wrote: > nit: Restore this blank line, please. :) Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:83: DCHECK_EQ(state(), MESSAGE_READY); On 2013/05/18 20:08:36, Wez wrote: > nit: Blank line between check and comment, plz Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.cc:123: } else { On 2013/05/17 20:09:03, rmsousa wrote: > Nits: > Please add a DCHECK that the method is spake or spakepair (that serves as > documentation that those (and only those) are intentionally handled by this > else). > > Also, please add a // TODO: add a parameter to fetch_secret_callback for whether > pairing is supported. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.h:30: // TODO(jamiewalch): Pass ClientConfig instead of separate parameters. On 2013/05/18 20:08:36, Wez wrote: > nit: SGTM but let's split out the auth-related members to a ClientAuthConfig > that ClientConfig can contain. SGTM. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.h:61: // Sets |current_authenticator_| and |current_method_| iff the client On 2013/05/18 20:08:36, Wez wrote: > Would it make more sense to add a getter to Authenticator for its > AuthenticationMethod, so we don't have to keep a separate current_method_? Maybe, but I don't think it needs to be done as part of this CL. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_client_authenticator.h:77: // Used for both authenticators. On 2013/05/18 20:08:36, Wez wrote: > nit: This probably needs updating, since there are more than two kinds of > authenticator now (or perhaps "both" means "both client and host" here?) Updated. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.cc:82: // If the client did not specify auth method or specified an unknown On 2013/05/18 20:08:36, Wez wrote: > nit: comma between 'method' and 'or' Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.cc:82: // If the client did not specify auth method or specified an unknown On 2013/05/18 20:08:36, Wez wrote: > nit: ".. specify a preferred auth method ..." ? Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... File remoting/protocol/negotiating_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.h:32: // Creates a host authenticator, using a fixed shared secret/PIN hash. On 2013/05/18 20:08:36, Wez wrote: > nit: Can we updated this e.g. to "... using the supplied PIN / Access Code's > |shared_secret_hash|."? I don't think that's any clearer than what we have now. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.h:35: // NULL pairing registry. On 2013/05/18 20:08:36, Wez wrote: > nit: Suggest "If |pairing_registry| is non-NULL then the SpakePair method will > be offered, supporting PIN-less authentication." Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:20: // These definitions must be kept in sync with pairing_host_authenticator.cc. On 2013/05/18 20:08:36, Wez wrote: > nit: Should we split out all the auth tags into a single auth_tags.[h|cc]? I've factored common code into a base class, which I think is a better place for them than somewhere common (since they're specific to this protocol). https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:46: DCHECK(v2_authenticator_); On 2013/05/18 20:08:36, Wez wrote: > nit: Won't state() already segfault if |v2_authenticator_| is NULL? operator-> already DCHECKs, so I've removed most of these now. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.cc:79: On 2013/05/18 20:08:36, Wez wrote: > Either get rid of the else, or the return. Done (in the new base class). https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_client_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:26: // * If a client device is already paired, it includes a client id in On 2013/05/18 20:08:36, Wez wrote: > nit: Suggest capitalizing 'Client Id', so that when you refer to it in other > comments it stands out as a specific technical term, similarly to 'Access Code', > 'Shared Secret', 'Host Secret', etc elsewhere. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:36: // On 2013/05/18 20:08:36, Wez wrote: > nit: Lose the extra blank comment Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:72: // created and holds the state of the overall auth exchange. On 2013/05/18 20:08:36, Wez wrote: > This is a little confusing; are you saying that in both the ACCEPTED and > REJECTED |pairing_state_| we defer overall to the V2 authenticator's state? > > Suggest something like: > Stores the progress of the pairing exchange. During the pairing exchange this > corresponds to the state of the authentication exchange as a whole. Once pairing > is ACCEPTED or REJECTED then the underlying V2 authenticator determines the > overall state. I've used a couple of booleans in the new implementation, which I think makes the code more readable. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:75: std::string client_id_; On 2013/05/18 20:08:36, Wez wrote: > nit: Please add at least a block comment for these members. :) Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_client_authenticator.h:81: base::WeakPtrFactory<PairingClientAuthenticator> weak_factory_; On 2013/05/18 20:08:36, Wez wrote: > nit: Suggest a blank line preceding |weak_factory_|, so it stands out from the > block of other members. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:60: DCHECK(v2_authenticator_); On 2013/05/18 20:08:36, Wez wrote: > nit: rejection_reason() below will segfault if |v2_authenticator_| is NULL, so > you don't strictly need this DCHECK. Plus, since it's a debug-only check, if > there's a bug in the authenticator logic then a client can coax you into > segfaulting anyway - to be defensive you could check |v2_authenticator_| first, > then DCHECK(protocol_error_) but then return PROTOCOL_ERROR no matter what. Done (in the base class). https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:117: DCHECK(v2_authenticator_); On 2013/05/18 20:08:36, Wez wrote: > nit: See above - does the DCHECK add anything here? Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:133: DCHECK(v2_authenticator_); On 2013/05/18 20:08:36, Wez wrote: > nit: Ditto. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:138: DCHECK(!v2_authenticator_); On 2013/05/18 20:08:36, Wez wrote: > nit: Here too Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:146: DCHECK(v2_authenticator_); On 2013/05/18 20:08:36, Wez wrote: > nit: Ditto. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.cc:148: // If the SPAKE exchange failed due to invalid credentials, and those On 2013/05/18 20:08:36, Wez wrote: > nit: Blank lines before this comment, and after the code block, please. Done (in the base class). https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:23: // * If a client device is already paired, it includes a client id in On 2013/05/18 20:08:36, Wez wrote: > nit: See comment in client imp re capitalization. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:58: void CheckForFailedSpakeExchange(const base::Closure& resume_callback); On 2013/05/18 20:08:36, Wez wrote: > nit: Comments for these members, please. Done in the base class for the former. The latter is now just one of several small helper functions. I don't think they all need comments. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:60: scoped_refptr<PairingRegistry> pairing_registry_; On 2013/05/18 20:08:36, Wez wrote: > nit: At least a block comment introducing these members, please. Done. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:66: bool protocol_error_; On 2013/05/18 20:08:36, Wez wrote: > nit: This name sounds like it stores an error, rather than just being an > indicator that one occurred. Do you actually need it, or could you test for > |error_message_| being empty? error_message doesn't indicate a fatal error, just a failure to authenticate using paired credentials. protocol_error is fatal. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:68: base::WeakPtrFactory<PairingHostAuthenticator> weak_factory_; On 2013/05/18 20:08:36, Wez wrote: > nit: Blank line before this, please. Done.
Sorry, I messed up the upload last night. PTAL.
lgtm This functionally looks ok, just a bunch of nits and a couple comments about the code organization. lgtm as long as the comments are addressed (particularly the one about moving the host-specific message processing block to the host implementation). https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_base.h:76: const AuthenticationMethod& current_method() const { return current_method_; } Nit: Since this is only used for testing, please name it current_method_for_testing. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_unittest.cc (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_unittest.cc:37: const char kTestPin[] = "1234-1234-5678"; Nit: Since we're fixing this file, might as well use 6 digit PINs here to be consistent with real PINs. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... File remoting/protocol/negotiating_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.cc:179: current_authenticator_ = V2Authenticator::CreateForHost( Nit: Please explicitly DCHECK current_method_ against the methods this else is supposed to handle (useful if we add another method in the future) https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.cc (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:20: const buzz::StaticQName kPairingFailedTag = Nit: I'd make these class members for consistency. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:38: return WAITING_MESSAGE; Nit: can you add a comment explaining why this state is WAITING_MESSAGE? (also, since it's a host-specific case, it seems like this case should be handled in PairingHostAuthenticator::state(), and then PairingAuthenticatorBase::state() could just DCHECK(v2_authenticator_)) https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:45: return PROTOCOL_ERROR; rejection_reason() is only queried if state() is REJECTED, so this is unreachable by the state() definition. You might as well DCHECK(v2_authenticator_). (PairingHostAuthenticator overrides state() to make this possible, but it overrides rejection_reason() as well, so this is still unreachable in that case) https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:70: if (!v2_authenticator_) { This whole block is host only. It should be part of PairingHostAuthenticator::ProcessMessage instead. This can be done before the block above, so PairingHostAuthenticator::Process message can just call ::ProcessMessage at the end). That also removes a NOTREACHED in the client implementation (since CreateV2AuthenticatorFromInitialMessage can be either inlined or private to PairingHostAuthenticator) https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:94: scoped_ptr<buzz::XmlElement> result = v2_authenticator_->GetNextMessage(); Nit: In ThirdPartyAuth the two lines below are encompassed by an implementation-specific AddPairingElements(). For consistency, maybe have an AddPairingElements() whose base implementation is MaybeAddErrorMessages, and the client implementation adds the AmendProtocolMessage code? https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:19: // the initial authentication message. it includes a client id and the first SPAKE message for the client secret. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:21: // Paired Secret and initiates a SPAKE with HMAC_SHA256. processes the incoming spake message. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:32: // to the user. Nit: (see Negotiating{Client,Host}Authenticator::CreateAuthenticator) https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:60: // only called on the host side. In particular, that means that it can be So, should this be in PairingHostAuthentciator? https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:65: // Amend a protocol message, for example to add client- or host-specific Nit: Not sure I'd call it a "protocol message". Authenticator message? https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:67: virtual void AmendProtocolMessage(buzz::XmlElement* message) = 0; Nit: I'd go with AddPairingElements, for consistency with AddTokenElements. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:72: std::string error_message_; Nit: define a few enum-like string constants for the error messages that have meaning.
FYI. Feel free to cancel the CQ if you want to insist on any of the nits I've declined. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_base.h:76: const AuthenticationMethod& current_method() const { return current_method_; } On 2013/05/21 23:17:07, rmsousa wrote: > Nit: Since this is only used for testing, please name it > current_method_for_testing. Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... File remoting/protocol/negotiating_authenticator_unittest.cc (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... remoting/protocol/negotiating_authenticator_unittest.cc:37: const char kTestPin[] = "1234-1234-5678"; On 2013/05/21 23:17:07, rmsousa wrote: > Nit: Since we're fixing this file, might as well use 6 digit PINs here to be > consistent with real PINs. Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... File remoting/protocol/negotiating_host_authenticator.cc (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/negotia... remoting/protocol/negotiating_host_authenticator.cc:179: current_authenticator_ = V2Authenticator::CreateForHost( On 2013/05/21 23:17:07, rmsousa wrote: > Nit: Please explicitly DCHECK current_method_ against the methods this else is > supposed to handle (useful if we add another method in the future) Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.cc (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:20: const buzz::StaticQName kPairingFailedTag = On 2013/05/21 23:17:07, rmsousa wrote: > Nit: I'd make these class members for consistency. The reason I didn't is that they're an implementation detail that derived classes don't need to know about it. I could make them private, but I prefer for them to be hidden entirely. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:38: return WAITING_MESSAGE; On 2013/05/21 23:17:07, rmsousa wrote: > Nit: can you add a comment explaining why this state is WAITING_MESSAGE? > > (also, since it's a host-specific case, it seems like this case should be > handled in PairingHostAuthenticator::state(), and then > PairingAuthenticatorBase::state() could just DCHECK(v2_authenticator_)) Done (without the DCHECK, since operator-> has one already. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:45: return PROTOCOL_ERROR; On 2013/05/21 23:17:07, rmsousa wrote: > rejection_reason() is only queried if state() is REJECTED, so this is > unreachable by the state() definition. You might as well > DCHECK(v2_authenticator_). > > (PairingHostAuthenticator overrides state() to make this possible, but it > overrides rejection_reason() as well, so this is still unreachable in that case) Wez asked me to add this on the basis that it's better to fail gracefully in case of a coding error than to crash the host. LMK if you feel strongly and I'll revert it (I don't have a strong opinion either way). https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:70: if (!v2_authenticator_) { On 2013/05/21 23:17:07, rmsousa wrote: > This whole block is host only. It should be part of > PairingHostAuthenticator::ProcessMessage instead. This can be done before the > block above, so PairingHostAuthenticator::Process message can just call > ::ProcessMessage at the end). > > That also removes a NOTREACHED in the client implementation (since > CreateV2AuthenticatorFromInitialMessage can be either inlined or private to > PairingHostAuthenticator) That's much cleaner, thanks. Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:94: scoped_ptr<buzz::XmlElement> result = v2_authenticator_->GetNextMessage(); On 2013/05/21 23:17:07, rmsousa wrote: > Nit: In ThirdPartyAuth the two lines below are encompassed by an > implementation-specific AddPairingElements(). For consistency, maybe have an > AddPairingElements() whose base implementation is MaybeAddErrorMessages, and the > client implementation adds the AmendProtocolMessage code? I don't want the subclasses to be able to avoid the error element being added, so I prefer to keep them separate. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:19: // the initial authentication message. On 2013/05/21 23:17:07, rmsousa wrote: > it includes a client id and the first SPAKE message for the client secret. Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:21: // Paired Secret and initiates a SPAKE with HMAC_SHA256. On 2013/05/21 23:17:07, rmsousa wrote: > processes the incoming spake message. Done. I've also added a comment to describe the fallback behaviour if the SPAKE exchange fails. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:32: // to the user. On 2013/05/21 23:17:07, rmsousa wrote: > Nit: (see Negotiating{Client,Host}Authenticator::CreateAuthenticator) Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:60: // only called on the host side. In particular, that means that it can be On 2013/05/21 23:17:07, rmsousa wrote: > So, should this be in PairingHostAuthentciator? Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:65: // Amend a protocol message, for example to add client- or host-specific On 2013/05/21 23:17:07, rmsousa wrote: > Nit: Not sure I'd call it a "protocol message". Authenticator message? Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:67: virtual void AmendProtocolMessage(buzz::XmlElement* message) = 0; On 2013/05/21 23:17:07, rmsousa wrote: > Nit: I'd go with AddPairingElements, for consistency with AddTokenElements. Done. https://codereview.chromium.org/14793021/diff/67001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:72: std::string error_message_; On 2013/05/21 23:17:07, rmsousa wrote: > Nit: define a few enum-like string constants for the error messages that have > meaning. I'm not sure there's any value in that, and I don't want to create the mistaken impression that these error strings have any semantic meaning. I've updated the comment on |error_message_| to make that explicit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/75001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:58: void CheckForFailedSpakeExchange(const base::Closure& resume_callback); On 2013/05/21 01:24:34, Jamie wrote: > On 2013/05/18 20:08:36, Wez wrote: > > nit: Comments for these members, please. > > Done in the base class for the former. The latter is now just one of several > small helper functions. I don't think they all need comments. Style guide says otherwise. ;) But yeah, you can probably get away without comments on each, if you instead have some comment e.g. "Helper functions for blah blah blah" for the block of methods. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.cc (right): https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:24: nit: Remove the extra blank line. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:109: // user for the PIN and try again. This comment doesn't describe the code that follows it? https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:147: void PairingAuthenticatorBase::SetAuthenticatorAndProcessMessage( nit: Couldn't you fold this and SetAuthenticator together, with a NULL message for the latter? https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:54: SetAuthenticatorCallback; Does this need to be public? https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:82: static const buzz::StaticQName kClientIdAttribute; These belong at the top of the section, after the SetAuthenticatorCallback typedef. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:93: scoped_ptr<Authenticator> authenticator); nit: Document these members, please.
FYI. https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... File remoting/protocol/pairing_host_authenticator.h (right): https://codereview.chromium.org/14793021/diff/53001/remoting/protocol/pairing... remoting/protocol/pairing_host_authenticator.h:58: void CheckForFailedSpakeExchange(const base::Closure& resume_callback); On 2013/05/22 00:51:45, Wez wrote: > On 2013/05/21 01:24:34, Jamie wrote: > > On 2013/05/18 20:08:36, Wez wrote: > > > nit: Comments for these members, please. > > > > Done in the base class for the former. The latter is now just one of several > > small helper functions. I don't think they all need comments. > > Style guide says otherwise. ;) But yeah, you can probably get away without > comments on each, if you instead have some comment e.g. "Helper functions for > blah blah blah" for the block of methods. Done. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.cc (right): https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:24: On 2013/05/22 00:51:45, Wez wrote: > nit: Remove the extra blank line. Done. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:109: // user for the PIN and try again. On 2013/05/22 00:51:45, Wez wrote: > This comment doesn't describe the code that follows it? It's a remnant from where this code used to live. Deleted. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.cc:147: void PairingAuthenticatorBase::SetAuthenticatorAndProcessMessage( On 2013/05/22 00:51:45, Wez wrote: > nit: Couldn't you fold this and SetAuthenticator together, with a NULL message > for the latter? Done. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... File remoting/protocol/pairing_authenticator_base.h (right): https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:54: SetAuthenticatorCallback; On 2013/05/22 00:51:45, Wez wrote: > Does this need to be public? No, I've changed it to protected. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:82: static const buzz::StaticQName kClientIdAttribute; On 2013/05/22 00:51:45, Wez wrote: > These belong at the top of the section, after the SetAuthenticatorCallback > typedef. Done. https://codereview.chromium.org/14793021/diff/75001/remoting/protocol/pairing... remoting/protocol/pairing_authenticator_base.h:93: scoped_ptr<Authenticator> authenticator); On 2013/05/22 00:51:45, Wez wrote: > nit: Document these members, please. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/110001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/123001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/14793021/123001
Message was sent while issue was closed.
Change committed as 201472 |
