Implement exponential backoff for failed Me2Me authentication attempts

Implement exponential backoff for failed Me2Me authentication attempts

BUG=113273
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129104

[Sergey Ulanov, 2012-03-23 23:03:33]

I'll resurrect ChromotingHost unittests and write unittest for backoff logic in
a separate CL.

[Jói, 2012-03-26 12:31:51]

Quick drive-by:

http://codereview.chromium.org/9836062/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9836062/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:287: login_backoff_.InformOfRequest(false);
It's confusing to me that this is unconditionally false (i.e. considered an
error event for the BackoffEntry).  Is this correct?  If it is, I would suggest
adding a comment to explain.

I guess perhaps the idea is to assume an error here until you get into
OnSessionAuthenticated at which point you Reset(), but this isn't immediately
obvious since the semantics of an InformOfRequest(false) call would normally be
understood to indicate an actual error rather than an
assumed-error-until-proven-otherwise error.

[Sergey Ulanov, 2012-03-26 17:54:14]

Jamie, this CL has some client-side changes, so it's better if we can land it
today ASAP. Can you please take a look?

[Jamie, 2012-03-26 21:08:22]

lgtm

http://codereview.chromium.org/9836062/diff/2001/remoting/webapp/_locales/en/...
File remoting/webapp/_locales/en/messages.json (right):

http://codereview.chromium.org/9836062/diff/2001/remoting/webapp/_locales/en/...
remoting/webapp/_locales/en/messages.json:142: "message": "Connections to the
remote computer are temporarily blocked because somebody was trying to connect
to it with invalid PIN.",
We usually try to give remedy instructions in our error messages. In this case
"Please try again later" might be appropriate.

[Sergey Ulanov, 2012-03-26 23:00:31]

http://codereview.chromium.org/9836062/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9836062/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:287: login_backoff_.InformOfRequest(false);
On 2012/03/26 12:31:56, Jói wrote:
> It's confusing to me that this is unconditionally false (i.e. considered an
> error event for the BackoffEntry).  Is this correct?  If it is, I would
suggest
> adding a comment to explain.
> 
> I guess perhaps the idea is to assume an error here until you get into
> OnSessionAuthenticated at which point you Reset(), but this isn't immediately
> obvious since the semantics of an InformOfRequest(false) call would normally
be
> understood to indicate an actual error rather than an
> assumed-error-until-proven-otherwise error.

Yes, that's the idea. This method is called when a new client is connected but
not authenticated yet. We want to prevent attacks when multiple connections are
made simultaneously and then all try to authenticate. Added comments about it.

http://codereview.chromium.org/9836062/diff/2001/remoting/webapp/_locales/en/...
File remoting/webapp/_locales/en/messages.json (right):

http://codereview.chromium.org/9836062/diff/2001/remoting/webapp/_locales/en/...
remoting/webapp/_locales/en/messages.json:142: "message": "Connections to the
remote computer are temporarily blocked because somebody was trying to connect
to it with invalid PIN.",
On 2012/03/26 21:08:22, Jamie wrote:
> We usually try to give remedy instructions in our error messages. In this case
> "Please try again later" might be appropriate.

Done.

[commit-bot: I haz the power, 2012-03-27 01:20:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9836062/11001

[Jói, 2012-03-27 10:44:45]

LGTM, thanks for adding the comments Sergey.

On Tue, Mar 27, 2012 at 1:20 AM,  <commit-bot@chromium.org> wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9836062/11001
>
>
> http://codereview.chromium.org/9836062/

[Wez, 2012-03-27 16:23:56]

http://codereview.chromium.org/9836062/diff/11001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9836062/diff/11001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:290: // them try to authenticate
simultaneously.
nit: Suggest rewording:
"We treat each incoming connection as a failure to authenticate, and clear the
backoff when a connection successfully authenticates. This allows the backoff to
protect from parallel connection attempts as well as sequential ones."

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/jingle_me...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:41: CANCEL,
Why is this called "CANCEL"?

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
File remoting/protocol/session_manager.h (right):

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
remoting/protocol/session_manager.h:121: DISABLED,
nit: "DISABLED" seems like a bad name for this, since that's the term we
generally use when the host is stopped (i.e. not online).  "OVERLOAD" or
"UNAVAILABLE" might be better terms.

[Wez, 2012-03-27 16:25:19]

Late-to-the-party LGTM, but please see my comments re naming, below.

On 2012/03/27 16:23:56, Wez wrote:
>
http://codereview.chromium.org/9836062/diff/11001/remoting/host/chromoting_ho...
> File remoting/host/chromoting_host.cc (right):
> 
>
http://codereview.chromium.org/9836062/diff/11001/remoting/host/chromoting_ho...
> remoting/host/chromoting_host.cc:290: // them try to authenticate
> simultaneously.
> nit: Suggest rewording:
> "We treat each incoming connection as a failure to authenticate, and clear the
> backoff when a connection successfully authenticates. This allows the backoff
to
> protect from parallel connection attempts as well as sequential ones."
> 
>
http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/jingle_me...
> File remoting/protocol/jingle_messages.h (right):
> 
>
http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/jingle_me...
> remoting/protocol/jingle_messages.h:41: CANCEL,
> Why is this called "CANCEL"?
> 
>
http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
> File remoting/protocol/session_manager.h (right):
> 
>
http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
> remoting/protocol/session_manager.h:121: DISABLED,
> nit: "DISABLED" seems like a bad name for this, since that's the term we
> generally use when the host is stopped (i.e. not online).  "OVERLOAD" or
> "UNAVAILABLE" might be better terms.

[Sergey Ulanov, 2012-03-27 18:00:51]

Addressed the comments in a new CL: http://codereview.chromium.org/9860045/

http://codereview.chromium.org/9836062/diff/11001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9836062/diff/11001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:290: // them try to authenticate
simultaneously.
On 2012/03/27 16:23:56, Wez wrote:
> nit: Suggest rewording:
> "We treat each incoming connection as a failure to authenticate, and clear the
> backoff when a connection successfully authenticates. This allows the backoff
to
> protect from parallel connection attempts as well as sequential ones."

Done.

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/jingle_me...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:41: CANCEL,
On 2012/03/27 16:23:56, Wez wrote:
> Why is this called "CANCEL"?

One of the reason values defined in the Jingle standard:
http://xmpp.org/extensions/xep-0166.html#def-reason

Alternatively we could use <general-error/> and then specify
application-specific error, but that would complicate the parsing code and
<cancel/> should work just fine.

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
File remoting/protocol/session_manager.h (right):

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
remoting/protocol/session_manager.h:121: DISABLED,
On 2012/03/27 16:23:56, Wez wrote:
> nit: "DISABLED" seems like a bad name for this, since that's the term we
> generally use when the host is stopped (i.e. not online). "OVERLOAD" or
"UNAVAILABLE" might be better terms.

There is HOST_IS_OFFLINE error code that we use to indicate that the host is
offline, so I don't think HOST_IS_DISABLED can be confusing. IMHO distinction
between UNAVAILABLE and OFFLINE is even less clear. 
I've renamed this particular value to UNAVAILABLE, but kept HOST_IS_DISABLED
error code  - changing it now would require merging to the M19 branch as we use
these names for interaction between the client plugin and webapp. We can still
do if you feel strongly about it.

[Wez, 2012-03-27 19:49:41]

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
File remoting/protocol/session_manager.h (right):

http://codereview.chromium.org/9836062/diff/11001/remoting/protocol/session_m...
remoting/protocol/session_manager.h:121: DISABLED,
On 2012/03/27 18:00:52, sergeyu wrote:
> On 2012/03/27 16:23:56, Wez wrote:
> > nit: "DISABLED" seems like a bad name for this, since that's the term we
> > generally use when the host is stopped (i.e. not online). "OVERLOAD" or
> "UNAVAILABLE" might be better terms.
> 
> There is HOST_IS_OFFLINE error code that we use to indicate that the host is
> offline, so I don't think HOST_IS_DISABLED can be confusing.

If we have "offline" and "disabled" then it's natural for a reader to assume
that DISABLED means the user explicitly disabled the host, and that OFFLINE
means that the host is enabled but not online for some reason.

> IMHO distinction between UNAVAILABLE and OFFLINE is even less clear.

I'd be fine with OVERLOAD[ED]. ;)

"unavailable" and "offline" have overlapping meanings, but "offline" is pretty
specific - it's saying the host is actually offline.  "unavailable" is more
specific, meaning it's online but not currently available - that might be
re-usable to mean that the host is in-use if we ever wanted to provide
floor-control for access to hosts.

> I've renamed this particular value to UNAVAILABLE, but kept HOST_IS_DISABLED
> error code  - changing it now would require merging to the M19 branch as we
use
> these names for interaction between the client plugin and webapp. We can still
> do if you feel strongly about it.

I do think that HOST_IS_DISABLED is a very confusing name - it suggests that the
host is installed & configured, but not enabled right now, rather than that it
has automatically stopped accepting connections.