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

Issue 12313085: Host-side third party token validation (Closed)

Created:
7 years, 7 months ago by rmsousa
Modified:
7 years, 5 months ago
Reviewers:
Sergey Ulanov, Wez
CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@third_party_auth_protocol
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 48

Patch Set 2 : Reviewer comments #

Patch Set 3 : Reviewer comments #

Total comments: 1

Patch Set 4 : Make TokenValidatorFactory global #

Patch Set 5 : Add missing parameters #

Total comments: 28

Patch Set 6 : Reviewer comments #

Total comments: 19

Patch Set 7 : Reviewer comments #

Total comments: 1

Patch Set 8 : Add TODO comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+457 lines, -31 lines) Patch
M remoting/host/policy_hack/policy_watcher.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M remoting/host/policy_hack/policy_watcher.cc View 1 2 3 4 5 2 chunks +10 lines, -2 lines 0 comments Download
M remoting/host/policy_hack/policy_watcher_unittest.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M remoting/host/remoting_me2me_host.cc View 1 2 3 4 5 6 7 7 chunks +58 lines, -3 lines 0 comments Download
A remoting/host/token_validator_factory_impl.h View 1 2 3 4 5 6 1 chunk +50 lines, -0 lines 0 comments Download
A remoting/host/token_validator_factory_impl.cc View 1 2 3 4 5 6 1 chunk +184 lines, -0 lines 0 comments Download
M remoting/protocol/it2me_host_authenticator_factory.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M remoting/protocol/me2me_host_authenticator_factory.h View 1 2 3 4 5 6 3 chunks +21 lines, -2 lines 0 comments Download
M remoting/protocol/me2me_host_authenticator_factory.cc View 1 2 3 4 5 6 2 chunks +49 lines, -8 lines 0 comments Download
M remoting/protocol/negotiating_authenticator_unittest.cc View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M remoting/protocol/negotiating_host_authenticator.h View 1 2 3 4 5 6 4 chunks +18 lines, -2 lines 0 comments Download
M remoting/protocol/negotiating_host_authenticator.cc View 1 2 3 4 5 6 3 chunks +42 lines, -10 lines 0 comments Download
M remoting/protocol/third_party_host_authenticator.h View 1 2 3 4 5 6 2 chunks +13 lines, -0 lines 0 comments Download
M remoting/remoting.gyp View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
rmsousa
Another piece of the token authentication CL. This is the host side of the token ...
7 years, 7 months ago (2013-02-23 03:43:05 UTC) #1
Wez
On 2013/02/23 03:43:05, rmsousa wrote: > Another piece of the token authentication CL. This is ...
7 years, 6 months ago (2013-03-06 00:45:29 UTC) #2
Wez
+sergeyu for a second pair of eyes. :) https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_validator_factory.h File remoting/host/host_token_validator_factory.h (right): https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_validator_factory.h#newcode17 remoting/host/host_token_validator_factory.h:17: class ...
7 years, 6 months ago (2013-03-06 00:47:01 UTC) #3
Wez
https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_validator_factory.cc File remoting/host/host_token_validator_factory.cc (right): https://codereview.chromium.org/12313085/diff/1/remoting/host/host_token_validator_factory.cc#newcode15 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_validator_factory.cc#newcode16 remoting/host/host_token_validator_factory.cc:16: #include ...
7 years, 6 months ago (2013-03-06 01:01:08 UTC) #4
rmsousa
FYI, although the code that is already here will likely stay, there's also some glue ...
7 years, 6 months ago (2013-03-06 01:02:07 UTC) #5
rmsousa
PTAL. I'm interested in ideas for the lifetime/ownership of the TokenValidatorFactory - I'm not really ...
7 years, 6 months ago (2013-03-25 22:45:58 UTC) #6
Sergey Ulanov
https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack/policy_watcher.cc File remoting/host/policy_hack/policy_watcher.cc (right): https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack/policy_watcher.cc#newcode118 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_token_validator_factory.cc ...
7 years, 6 months ago (2013-03-28 22:34:53 UTC) #7
rmsousa
https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher_token_validator_factory.cc File remoting/host/url_fetcher_token_validator_factory.cc (right): https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher_token_validator_factory.cc#newcode77 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 ...
7 years, 6 months ago (2013-03-28 23:12:49 UTC) #8
Sergey Ulanov
https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher_token_validator_factory.cc File remoting/host/url_fetcher_token_validator_factory.cc (right): https://codereview.chromium.org/12313085/diff/40001/remoting/host/url_fetcher_token_validator_factory.cc#newcode77 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 ...
7 years, 6 months ago (2013-03-28 23:39:39 UTC) #9
rmsousa
https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack/policy_watcher.cc File remoting/host/policy_hack/policy_watcher.cc (right): https://codereview.chromium.org/12313085/diff/40001/remoting/host/policy_hack/policy_watcher.cc#newcode118 remoting/host/policy_hack/policy_watcher.cc:118: default_values_->SetString(kHostTokenUrlPolicyName, ""); On 2013/03/28 22:34:54, sergeyu wrote: > please ...
7 years, 5 months ago (2013-04-04 22:13:43 UTC) #10
Sergey Ulanov
Looks mostly good. The only issue is that the case when policies are not configured ...
7 years, 5 months ago (2013-04-05 20:28:33 UTC) #11
Wez
Look's good! https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher_token_validator_factory.cc File remoting/host/url_fetcher_token_validator_factory.cc (right): https://codereview.chromium.org/12313085/diff/53001/remoting/host/url_fetcher_token_validator_factory.cc#newcode185 remoting/host/url_fetcher_token_validator_factory.cc:185: return token_url_.is_valid() && token_validation_url_.is_valid() && key_pair_; On ...
7 years, 5 months ago (2013-04-05 22:46:12 UTC) #12
Wez
On 2013/04/05 22:46:12, Wez wrote: > Look's good! *Looks...
7 years, 5 months ago (2013-04-05 22:51:36 UTC) #13
rmsousa
https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me2me_host.cc File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/12313085/diff/53001/remoting/host/remoting_me2me_host.cc#newcode760 remoting/host/remoting_me2me_host.cc:760: if (policies->GetString( On 2013/04/05 20:28:34, sergeyu wrote: > not ...
7 years, 5 months ago (2013-04-06 00:37:25 UTC) #14
Wez
LGTM as soon as Sergey's happy. :)
7 years, 5 months ago (2013-04-06 00:38:40 UTC) #15
Sergey Ulanov
lgtm https://codereview.chromium.org/12313085/diff/63003/remoting/host/remoting_me2me_host.cc File remoting/host/remoting_me2me_host.cc (right): https://codereview.chromium.org/12313085/diff/63003/remoting/host/remoting_me2me_host.cc#newcode507 remoting/host/remoting_me2me_host.cc:507: factory = protocol::Me2MeHostAuthenticatorFactory::CreateRejecting(); It would be better if ...
7 years, 5 months ago (2013-04-06 00:56:31 UTC) #16
Wez
On 2013/04/06 00:56:31, sergeyu wrote: > lgtm > > https://codereview.chromium.org/12313085/diff/63003/remoting/host/remoting_me2me_host.cc > File remoting/host/remoting_me2me_host.cc (right): > ...
7 years, 5 months ago (2013-04-06 01:02:58 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12313085/69001
7 years, 5 months ago (2013-04-06 01:35:40 UTC) #18
commit-bot: I haz the power
7 years, 5 months ago (2013-04-06 04:50:47 UTC) #19
Message was sent while issue was closed.
Change committed as 192701

Powered by Google App Engine
This is Rietveld 408576698