Host-side third party token validation

Host-side third party token validation

This creates a TokenValidator implementation on the host, that upon receiving a token:
Signs the token with its private key.
Uses URLFetcher to request the exchange of the token for a secret from the Token Validation URL.
On receiving a reply, checks that the scope in the reply matches the one required for this connection.
Uses the callback to send the shared_token back to the authentication layer.

(The server will authenticate the host by checking that the token signature matches the host public key that the client included in the token request)

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

[rmsousa, 2013-02-23 03:43:05]

Another piece of the token authentication CL. This is the host side of the token
authentication - just the pieces to read the policies, and the code to validate
the token.

[Wez, 2013-03-06 00:45:29]

On 2013/02/23 03:43:05, rmsousa wrote:
> Another piece of the token authentication CL. This is the host side of the
token
> authentication - just the pieces to read the policies, and the code to
validate
> the token.

You need a bug number, and the description should briefly summarise the changes
involved. :)

[Wez, 2013-03-06 00:47:01]

+sergeyu for a second pair of eyes. :)

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
File remoting/host/host_token_validator_factory.h (right):

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.h:17: class HostTokenValidatorFactory
nit: Do we need "host" in the class name? It's not very descriptive and the
class is already in the "host" directory.

Perhaps just TokenValidatorFactory, for now?

[Wez, 2013-03-06 01:01:08]

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
File remoting/host/host_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:15: #include
"base/supports_user_data.h"
Is this include required?

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:16: #include
"base/thread_task_runner_handle.h"
Is this include necessary?

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:35: ~HostTokenValidator() {
nit: virtual

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:38: void ValidateThirdPartyToken(
nit: Add comment "TokenValidator interface."

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:38: void ValidateThirdPartyToken(
virtual + OVERRIDE

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:47: DCHECK(!scope.empty());
nit: DCHECK e.g. that on_token_validated_ is non-null, and other parameters are
valid?

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:48: scope_ = scope;
nit: Space after DCHECKS, and after this parameter-saving block.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:64: void OnURLFetchComplete(const
net::URLFetcher* source) {
nit: Add comment "URLFetcherDelegate interface."

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:64: void OnURLFetchComplete(const
net::URLFetcher* source) {
virtual + OVERRIDE

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:68: request_.reset();
This will fail if the callback caused a new validation to be started, or caused
the validator to be torn down - can you reset |request_| before the callback?

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:72: bool ValidateScope(const
std::string& scope) {
nit: IsValidScope()

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:78: std::string shared_secret;
nit: Add a comment e.g. "Verify that we got a success response."

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:88:
source->GetResponseAsString(&data);
nit: Comment e.g. "Decode the JSON data from the response."

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:95: DictionaryValue* dict =
static_cast<DictionaryValue*>(value.get());
nit: Comment e.g. "Fetch the scope and check that it is valid."

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:104:
dict->GetStringWithoutPathExpansion("access_token", &shared_secret);
nit: Comment e.g. "Scope was valid, so return the secret to the caller."

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:111: base::Callback<void(const
std::string& shared_secret)> on_token_validated_;
nit: Blank line before DISALLOW...

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
File remoting/host/host_token_validator_factory.h (right):

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.h:18: : public
protocol::ThirdPartyAuthenticator::TokenValidatorFactory {
Add a comment explaining what properties the validators this factory dispenses
have.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:362: host_token_validator_factory_.reset(
nit: Comment e.g. "Create factory for third-party token authentication."

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:538: host_token_validator_factory_.get()));
Remind me, will this become Pass()?

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:792: // TODO(rmsousa): Read token URL
policies.
Remove this TODO

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:916: token_issue_url !=
token_verification_url) {
I think it's clearer to structure this with a check for either being empty, and
saying "one or other is empty", with an else that displays the two values.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:921: }
nit: Blank line after this.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:923: token_verification_url_ =
token_verification_url;
nit: Blank line after this

[rmsousa, 2013-03-06 01:02:07]

FYI, although the code that is already here will likely stay, there's also some
glue that's moving here from the previous CL. So while reviewing what's already
here will be useful, please be aware that this will be updated with some more
code soon.

[rmsousa, 2013-03-25 22:45:58]

PTAL. I'm interested in ideas for the lifetime/ownership of the
TokenValidatorFactory - I'm not really a fan of either solution I proposed, so
if you see any kind of refactoring where we can avoid that mess, I'll be glad to
hear it.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
File remoting/host/host_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:15: #include
"base/supports_user_data.h"
On 2013/03/06 01:01:08, Wez wrote:
> Is this include required?

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:16: #include
"base/thread_task_runner_handle.h"
On 2013/03/06 01:01:08, Wez wrote:
> Is this include necessary?

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:35: ~HostTokenValidator() {
On 2013/03/06 01:01:08, Wez wrote:
> nit: virtual

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:38: void ValidateThirdPartyToken(
On 2013/03/06 01:01:08, Wez wrote:
> nit: Add comment "TokenValidator interface."

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:38: void ValidateThirdPartyToken(
On 2013/03/06 01:01:08, Wez wrote:
> virtual + OVERRIDE

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:47: DCHECK(!scope.empty());
On 2013/03/06 01:01:08, Wez wrote:
> nit: DCHECK e.g. that on_token_validated_ is non-null, and other parameters
are
> valid?

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:48: scope_ = scope;
On 2013/03/06 01:01:08, Wez wrote:
> nit: Space after DCHECKS, and after this parameter-saving block.

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:64: void OnURLFetchComplete(const
net::URLFetcher* source) {
On 2013/03/06 01:01:08, Wez wrote:
> virtual + OVERRIDE

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:64: void OnURLFetchComplete(const
net::URLFetcher* source) {
On 2013/03/06 01:01:08, Wez wrote:
> nit: Add comment "URLFetcherDelegate interface."

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:68: request_.reset();
On 2013/03/06 01:01:08, Wez wrote:
> This will fail if the callback caused a new validation to be started, or
caused
> the validator to be torn down - can you reset |request_| before the callback?

Good catch. Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:72: bool ValidateScope(const
std::string& scope) {
On 2013/03/06 01:01:08, Wez wrote:
> nit: IsValidScope()

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:78: std::string shared_secret;
On 2013/03/06 01:01:08, Wez wrote:
> nit: Add a comment e.g. "Verify that we got a success response."

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:88:
source->GetResponseAsString(&data);
On 2013/03/06 01:01:08, Wez wrote:
> nit: Comment e.g. "Decode the JSON data from the response."

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:95: DictionaryValue* dict =
static_cast<DictionaryValue*>(value.get());
On 2013/03/06 01:01:08, Wez wrote:
> nit: Comment e.g. "Fetch the scope and check that it is valid."

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:104:
dict->GetStringWithoutPathExpansion("access_token", &shared_secret);
On 2013/03/06 01:01:08, Wez wrote:
> nit: Comment e.g. "Scope was valid, so return the secret to the caller."

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.cc:111: base::Callback<void(const
std::string& shared_secret)> on_token_validated_;
On 2013/03/06 01:01:08, Wez wrote:
> nit: Blank line before DISALLOW...

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
File remoting/host/host_token_validator_factory.h (right):

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.h:17: class HostTokenValidatorFactory
On 2013/03/06 00:47:02, Wez wrote:
> nit: Do we need "host" in the class name? It's not very descriptive and the
> class is already in the "host" directory.
> 
> Perhaps just TokenValidatorFactory, for now?

That's the name of the base class... I followed the <Implementation><Interface>
naming scheme, arriving at UrlFetcherTokenValidatorFactory... ugh...

https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_vali...
remoting/host/host_token_validator_factory.h:18: : public
protocol::ThirdPartyAuthenticator::TokenValidatorFactory {
On 2013/03/06 01:01:08, Wez wrote:
> Add a comment explaining what properties the validators this factory dispenses
> have.

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:362: host_token_validator_factory_.reset(
On 2013/03/06 01:01:08, Wez wrote:
> nit: Comment e.g. "Create factory for third-party token authentication."

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:538: host_token_validator_factory_.get()));
On 2013/03/06 01:01:08, Wez wrote:
> Remind me, will this become Pass()?

"It's complicated". Basically, NegotiatingAuthenticator needs to have a
reference to the factory (because it may need to create more than authenticator,
and each authenticator owns its own validator). Me2MeAuthenticatorFactory
doesn't own the NegotiatingAuthenticator, and is not guaranteed to outlive it
(it may be that it currently does, but it's not explicitly designed to), so
Me2MeAuthenticatorFactory can't own the TokenValidatorFactory and give
NegotiatingAuthenticator a reference, otherwise that reference may be pulled
from the NegotiatingAuthenticator's feet.
So the options are: Make the TokenValidatorFactory a global, stateless object,
and have all the policy/config parameters be passed as parameters to its
CreateValidator() method, or have the TokenValidatorFactory be stateful and
refcounted. We don't seem to like refcounting random objects in our code unless
we need to, so I went with the global/stateless approach (the refcounted
approach is still in this CL's history, see PatchSet #3 if you're curious).

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:792: // TODO(rmsousa): Read token URL
policies.
On 2013/03/06 01:01:08, Wez wrote:
> Remove this TODO

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:916: token_issue_url !=
token_verification_url) {
On 2013/03/06 01:01:08, Wez wrote:
> I think it's clearer to structure this with a check for either being empty,
and
> saying "one or other is empty", with an else that displays the two values.

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:921: }
On 2013/03/06 01:01:08, Wez wrote:
> nit: Blank line after this.

Done.

https://codereview.chromium.org/12313085/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:923: token_verification_url_ =
token_verification_url;
On 2013/03/06 01:01:08, Wez wrote:
> nit: Blank line after this

Done.

https://codereview.chromium.org/12313085/diff/13016/remoting/protocol/negotia...
File remoting/protocol/negotiating_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/13016/remoting/protocol/negotia...
remoting/protocol/negotiating_authenticator.h:90: const std::string& remote_jid,
Yeah, passing all these parameters 3-4 callstacks down is kind of annoying. I'll
move these to {SharedSecret,ThirdParty}HostAuthenticatorParams structs once the
question about the factory lifetime is settled.

[Sergey Ulanov, 2013-03-28 22:34:53]

https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack...
remoting/host/policy_hack/policy_watcher.cc:118:
default_values_->SetString(kHostTokenUrlPolicyName, "");
please use std::string() instead of ""

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:28: const size_t
kNonceLength = 16;  // 128 bits.
indentation

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:77:
request_->SetUploadData("application/x-www-form-urlencoded", post_body);
Can we use json here instead of url-encoding the data?

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:114: std::string
GetSharedSecretFromResponse(const net::URLFetcher* source) {
Maybe call it ProcessResponse?

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:114: std::string
GetSharedSecretFromResponse(const net::URLFetcher* source) {
|source| is a meaningless name in this context, but you don't need this argument
and can use |request_| instead

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:122: LOG(ERROR) <<
nit: wrap << operator

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:124: return shared_secret;
This is confusing. Maybe better to return std::string() here and move
shared_secret declaration to the end.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:136: DictionaryValue* dict
= static_cast<DictionaryValue*>(value.get());
Ouch. Instead of downcasting using static_cast<> use Value::GetAsDictionary().

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:178: local_jid, remote_jid,
request_context_getter_));
indentation

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/negotia...
File remoting/protocol/negotiating_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/negotia...
remoting/protocol/negotiating_authenticator.h:79: static
scoped_ptr<Authenticator> CreateForHostSharedSecret(
maybe CreateForHostWithSharedSecret()?

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
File remoting/protocol/third_party_host_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
remoting/protocol/third_party_host_authenticator.h:66: scoped_refptr<RsaKeyPair>
key_pair,
These three parameters are specific to url-based token validator, so they
shouldn't be part of the generic interface. Can you move them to
UrlFetcherTokenValidatorFactory constructor?

https://codereview.chromium.org/12313085/diff/40001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/remoting.gyp#new...
remoting/remoting.gyp:368: 'host/url_fetcher_token_validator_factory.cc',
alphabet order

[rmsousa, 2013-03-28 23:12:49]

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:77:
request_->SetUploadData("application/x-www-form-urlencoded", post_body);
On 2013/03/28 22:34:54, sergeyu wrote:
> Can we use json here instead of url-encoding the data?

I'm trying to follow the OAuth protocol, which uses an URL-encoded request here
(perhaps because, even if it's a bad idea, one could possibly use GET here).
It's kinda funny that the request is URL encoded and the response is in JSON,
but, well, that's the protocol...

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
File remoting/protocol/third_party_host_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
remoting/protocol/third_party_host_authenticator.h:66: scoped_refptr<RsaKeyPair>
key_pair,
On 2013/03/28 22:34:54, sergeyu wrote:
> These three parameters are specific to url-based token validator, so they
> shouldn't be part of the generic interface. Can you move them to
> UrlFetcherTokenValidatorFactory constructor?

These can change during the lifetime of the host (policy/config update), so they
can't be owned by a global TokenValidatorFactory. If instead of having a global
one we create a TokenValidatorFactory when we create Me2MeAuthenticatorFactory
(see the previous patchset), we can't pass a naked reference to the factory to
NegotiatingAuthenticator, because there's no ownership between
Me2MeAuthenticatorFactory and NegotiatingAuthenticator, so we end up having to
refcount the TokenValidatorFactory.

Again this gets much simpelr if we declare that the host's protocol choice is
final - Me2MeAuthenticatorFactory can just give the NegotiatingAuthenticator a
TokenValidator instance.

[Sergey Ulanov, 2013-03-28 23:39:39]

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:77:
request_->SetUploadData("application/x-www-form-urlencoded", post_body);
On 2013/03/28 23:12:49, rmsousa wrote:
> On 2013/03/28 22:34:54, sergeyu wrote:
> > Can we use json here instead of url-encoding the data?
> 
> I'm trying to follow the OAuth protocol, which uses an URL-encoded request
here
> (perhaps because, even if it's a bad idea, one could possibly use GET here).
> It's kinda funny that the request is URL encoded and the response is in JSON,
> but, well, that's the protocol...

I'm not sure why we need to worry about OAuth here. Will the protocol be
compatible with OAuth? I.e. is it possible to take existing OAuth provider and
use it for host authentication? If not, then I don't see why we need to worry
about protocol compatibility.

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
File remoting/protocol/third_party_host_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
remoting/protocol/third_party_host_authenticator.h:66: scoped_refptr<RsaKeyPair>
key_pair,
On 2013/03/28 23:12:49, rmsousa wrote:
> On 2013/03/28 22:34:54, sergeyu wrote:
> > These three parameters are specific to url-based token validator, so they
> > shouldn't be part of the generic interface. Can you move them to
> > UrlFetcherTokenValidatorFactory constructor?
> 
> These can change during the lifetime of the host (policy/config update), so
they
> can't be owned by a global TokenValidatorFactory. If instead of having a
global
> one we create a TokenValidatorFactory when we create Me2MeAuthenticatorFactory
> (see the previous patchset), we can't pass a naked reference to the factory to
> NegotiatingAuthenticator, because there's no ownership between
> Me2MeAuthenticatorFactory and NegotiatingAuthenticator, so we end up having to
> refcount the TokenValidatorFactory.

For some policies we restart the host when they change. I think we should do it
for these authentication policies too. This is necessary because you need to
make sure that policies apply to all connected clients. If a client is connected
while token_url changes then it means that the client needs to be
reauthenticated using new URL. I don't think we want to support reauthentication
mid-session - it's easier to restart the host in that case.


> 
> Again this gets much simpelr if we declare that the host's protocol choice is
> final - Me2MeAuthenticatorFactory can just give the NegotiatingAuthenticator a
> TokenValidator instance.

[rmsousa, 2013-04-04 22:13:43]

https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack...
File remoting/host/policy_hack/policy_watcher.cc (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack...
remoting/host/policy_hack/policy_watcher.cc:118:
default_values_->SetString(kHostTokenUrlPolicyName, "");
On 2013/03/28 22:34:54, sergeyu wrote:
> please use std::string() instead of ""

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:28: const size_t
kNonceLength = 16;  // 128 bits.
On 2013/03/28 22:34:54, sergeyu wrote:
> indentation

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:77:
request_->SetUploadData("application/x-www-form-urlencoded", post_body);
On 2013/03/28 23:39:39, sergeyu wrote:
> On 2013/03/28 23:12:49, rmsousa wrote:
> > On 2013/03/28 22:34:54, sergeyu wrote:
> > > Can we use json here instead of url-encoding the data?
> > 
> > I'm trying to follow the OAuth protocol, which uses an URL-encoded request
> here
> > (perhaps because, even if it's a bad idea, one could possibly use GET here).
> > It's kinda funny that the request is URL encoded and the response is in
JSON,
> > but, well, that's the protocol...
> 
> I'm not sure why we need to worry about OAuth here. Will the protocol be
> compatible with OAuth? I.e. is it possible to take existing OAuth provider and
> use it for host authentication? If not, then I don't see why we need to worry
> about protocol compatibility.

Yes, the protocol will be OAuth. It's possible to use an OAuth lib to write a
host auth server.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:114: std::string
GetSharedSecretFromResponse(const net::URLFetcher* source) {
On 2013/03/28 22:34:54, sergeyu wrote:
> |source| is a meaningless name in this context, but you don't need this
argument
> and can use |request_| instead

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:114: std::string
GetSharedSecretFromResponse(const net::URLFetcher* source) {
On 2013/03/28 22:34:54, sergeyu wrote:
> Maybe call it ProcessResponse?

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:122: LOG(ERROR) <<
On 2013/03/28 22:34:54, sergeyu wrote:
> nit: wrap << operator

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:124: return shared_secret;
On 2013/03/28 22:34:54, sergeyu wrote:
> This is confusing. Maybe better to return std::string() here and move
> shared_secret declaration to the end.

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:136: DictionaryValue* dict
= static_cast<DictionaryValue*>(value.get());
On 2013/03/28 22:34:54, sergeyu wrote:
> Ouch. Instead of downcasting using static_cast<> use Value::GetAsDictionary().

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:178: local_jid, remote_jid,
request_context_getter_));
On 2013/03/28 22:34:54, sergeyu wrote:
> indentation

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/negotia...
File remoting/protocol/negotiating_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/negotia...
remoting/protocol/negotiating_authenticator.h:79: static
scoped_ptr<Authenticator> CreateForHostSharedSecret(
On 2013/03/28 22:34:54, sergeyu wrote:
> maybe CreateForHostWithSharedSecret()?

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
File remoting/protocol/third_party_host_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/protocol/third_p...
remoting/protocol/third_party_host_authenticator.h:66: scoped_refptr<RsaKeyPair>
key_pair,
On 2013/03/28 23:39:39, sergeyu wrote:
> On 2013/03/28 23:12:49, rmsousa wrote:
> > On 2013/03/28 22:34:54, sergeyu wrote:
> > > These three parameters are specific to url-based token validator, so they
> > > shouldn't be part of the generic interface. Can you move them to
> > > UrlFetcherTokenValidatorFactory constructor?
> > 
> > These can change during the lifetime of the host (policy/config update), so
> they
> > can't be owned by a global TokenValidatorFactory. If instead of having a
> global
> > one we create a TokenValidatorFactory when we create
Me2MeAuthenticatorFactory
> > (see the previous patchset), we can't pass a naked reference to the factory
to
> > NegotiatingAuthenticator, because there's no ownership between
> > Me2MeAuthenticatorFactory and NegotiatingAuthenticator, so we end up having
to
> > refcount the TokenValidatorFactory.
> 
> For some policies we restart the host when they change. I think we should do
it
> for these authentication policies too. This is necessary because you need to
> make sure that policies apply to all connected clients. If a client is
connected
> while token_url changes then it means that the client needs to be
> reauthenticated using new URL. I don't think we want to support
reauthentication
> mid-session - it's easier to restart the host in that case.
> 
> 
> > 
> > Again this gets much simpelr if we declare that the host's protocol choice
is
> > final - Me2MeAuthenticatorFactory can just give the NegotiatingAuthenticator
a
> > TokenValidator instance.
> 

Done.

https://codereview.chromium.org/12313085/diff/40001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/12313085/diff/40001/remoting/remoting.gyp#new...
remoting/remoting.gyp:368: 'host/url_fetcher_token_validator_factory.cc',
On 2013/03/28 22:34:54, sergeyu wrote:
> alphabet order

Done.

[Sergey Ulanov, 2013-04-05 20:28:33]

Looks mostly good. The only issue is that the case when policies are not
configured correctly should be handled more carefully.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me...
remoting/host/remoting_me2me_host.cc:760: if (policies->GetString(
not related to this CL, so not worth fixing here: it's expected that the values
are always in the dictionary, so this if can be replaced with a DCHECK.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me...
remoting/host/remoting_me2me_host.cc:904: << "TokenUrl: " << token_url << ", "
nit: << should be aligned with << on the previous line when wrapped:
  LOG(ERROR) << "foo "
             << x << "bar";

but if it's wrapped because the string is too long, then you don't need the
second <<. In that case it should be indented 4 spaces
  LOG(ERROR) << "foo "
      "bar" << x;

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:185: return
token_url_.is_valid() && token_validation_url_.is_valid() && key_pair_;
If admin configured the policies but made a typo in one of the URL's then we'll
effectively disable third-party auth and use PIN auth instead. I don't think
it's the right thing to do here. When any of the policies is configured, but set
to invalid value we should treat this case as if third-party auth is enabled but
misconfigured. Obviously it will not be possible to connect to the host in that
case, so we should yell loudly about it in the logs.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.h (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.h:38: 
nit: remove empty line

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.h:46:
DISALLOW_COPY_AND_ASSIGN(UrlFetcherTokenValidatorFactory);
not: empty line above this one.

[Wez, 2013-04-05 22:46:12]

Look's good!

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:185: return
token_url_.is_valid() && token_validation_url_.is_valid() && key_pair_;
On 2013/04/05 20:28:34, sergeyu wrote:
> If admin configured the policies but made a typo in one of the URL's then
we'll
> effectively disable third-party auth and use PIN auth instead. I don't think
> it's the right thing to do here. When any of the policies is configured, but
set
> to invalid value we should treat this case as if third-party auth is enabled
but
> misconfigured. Obviously it will not be possible to connect to the host in
that
> case, so we should yell loudly about it in the logs.

Generally speaking we should disable the host entirely if any policy is
malformed, but leave it running & monitoring policy so it can be re-enabled by a
subsequent fix to policies.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.h (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.h:20: class
UrlFetcherTokenValidatorFactory
nit: Could this just be TokenValidatorFactoryImpl, since we have no other
implementations of third-party token auth planned?

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/me2me_h...
File remoting/protocol/me2me_host_authenticator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/me2me_h...
remoting/protocol/me2me_host_authenticator_factory.cc:101: }
nit: blank line after this

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
File remoting/protocol/negotiating_host_authenticator.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
remoting/protocol/negotiating_host_authenticator.cc:149:
DCHECK(current_method_.is_valid());
nit: blank line here and after the end of the else block, please.

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
File remoting/protocol/negotiating_host_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
remoting/protocol/negotiating_host_authenticator.h:69:
scoped_ptr<ThirdPartyHostAuthenticator::TokenValidator> token_validator_;
nit: blank line after this

[Wez, 2013-04-05 22:51:36]

On 2013/04/05 22:46:12, Wez wrote:
> Look's good!

*Looks...

[rmsousa, 2013-04-06 00:37:25]

https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me...
remoting/host/remoting_me2me_host.cc:760: if (policies->GetString(
On 2013/04/05 20:28:34, sergeyu wrote:
> not related to this CL, so not worth fixing here: it's expected that the
values
> are always in the dictionary, so this if can be replaced with a DCHECK.

Done.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me...
remoting/host/remoting_me2me_host.cc:904: << "TokenUrl: " << token_url << ", "
On 2013/04/05 20:28:34, sergeyu wrote:
> nit: << should be aligned with << on the previous line when wrapped:
>   LOG(ERROR) << "foo "
>              << x << "bar";
> 
> but if it's wrapped because the string is too long, then you don't need the
> second <<. In that case it should be indented 4 spaces
>   LOG(ERROR) << "foo "
>       "bar" << x;
> 
> 

Done.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.cc:185: return
token_url_.is_valid() && token_validation_url_.is_valid() && key_pair_;
On 2013/04/05 22:46:12, Wez wrote:
> On 2013/04/05 20:28:34, sergeyu wrote:
> > If admin configured the policies but made a typo in one of the URL's then
> we'll
> > effectively disable third-party auth and use PIN auth instead. I don't think
> > it's the right thing to do here. When any of the policies is configured, but
> set
> > to invalid value we should treat this case as if third-party auth is enabled
> but
> > misconfigured. Obviously it will not be possible to connect to the host in
> that
> > case, so we should yell loudly about it in the logs.
> 
> Generally speaking we should disable the host entirely if any policy is
> malformed, but leave it running & monitoring policy so it can be re-enabled by
a
> subsequent fix to policies.

I moved this logic to the host itself. At the time it's creating the
authenticator factory, the host knows exactly what kind of authentication it'll
use, so it might as well only create the validator if it's going to be used (and
explicitly create a rejecting factory if the policies are inconsistent).

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
File remoting/host/url_fetcher_token_validator_factory.h (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.h:20: class
UrlFetcherTokenValidatorFactory
On 2013/04/05 22:46:12, Wez wrote:
> nit: Could this just be TokenValidatorFactoryImpl, since we have no other
> implementations of third-party token auth planned?

Done.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.h:38: 
On 2013/04/05 20:28:34, sergeyu wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher...
remoting/host/url_fetcher_token_validator_factory.h:46:
DISALLOW_COPY_AND_ASSIGN(UrlFetcherTokenValidatorFactory);
On 2013/04/05 20:28:34, sergeyu wrote:
> not: empty line above this one.

Done.

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/me2me_h...
File remoting/protocol/me2me_host_authenticator_factory.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/me2me_h...
remoting/protocol/me2me_host_authenticator_factory.cc:101: }
On 2013/04/05 22:46:12, Wez wrote:
> nit: blank line after this

Done.

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
File remoting/protocol/negotiating_host_authenticator.cc (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
remoting/protocol/negotiating_host_authenticator.cc:149:
DCHECK(current_method_.is_valid());
On 2013/04/05 22:46:12, Wez wrote:
> nit: blank line here and after the end of the else block, please.

Done.

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
File remoting/protocol/negotiating_host_authenticator.h (right):

https://codereview.chromium.org/12313085/diff/53001/remoting/protocol/negotia...
remoting/protocol/negotiating_host_authenticator.h:69:
scoped_ptr<ThirdPartyHostAuthenticator::TokenValidator> token_validator_;
On 2013/04/05 22:46:12, Wez wrote:
> nit: blank line after this

Done.

[Wez, 2013-04-06 00:38:40]

LGTM as soon as Sergey's happy. :)

[Sergey Ulanov, 2013-04-06 00:56:31]

lgtm

https://codereview.chromium.org/12313085/diff/63003/remoting/host/remoting_me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/12313085/diff/63003/remoting/host/remoting_me...
remoting/host/remoting_me2me_host.cc:507: factory =
protocol::Me2MeHostAuthenticatorFactory::CreateRejecting();
It would be better if we had a mode in which the host process is started but the
host is offline. There are two downside to keeping it offline: (1) it consumes
resources by keeping XMPP connection, (2) confusing UX because the host shown as
Online while there is no way to access it. This is a minor concern because we
don't expect too many hosts to be in that state. Maybe add TODO to fix it.

[Wez, 2013-04-06 01:02:58]

On 2013/04/06 00:56:31, sergeyu wrote:
> lgtm
> 
>
https://codereview.chromium.org/12313085/diff/63003/remoting/host/remoting_me...
> File remoting/host/remoting_me2me_host.cc (right):
> 
>
https://codereview.chromium.org/12313085/diff/63003/remoting/host/remoting_me...
> remoting/host/remoting_me2me_host.cc:507: factory =
> protocol::Me2MeHostAuthenticatorFactory::CreateRejecting();
> It would be better if we had a mode in which the host process is started but
the
> host is offline. There are two downside to keeping it offline: (1) it consumes
> resources by keeping XMPP connection, (2) confusing UX because the host shown
as
> Online while there is no way to access it. This is a minor concern because we
> don't expect too many hosts to be in that state. Maybe add TODO to fix it.

I like the idea of not starting up ChromotingHost if policies are invalid, and
instead staying up and monitoring for them becoming valid. :)

[commit-bot: I haz the power, 2013-04-06 01:35:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12313085/69001

[commit-bot: I haz the power, 2013-04-06 04:50:47]

Change committed as 192701