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

Issue 12316083: Move HostKeyPair into protocol::KeyPair. (Closed)

Created:
7 years, 7 months ago by rmsousa
Modified:
7 years, 6 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@master
Visibility:
Public.

Description

Move HostKeyPair into protocol::KeyPair. This makes the RSAPrivateKey dependency encapsulated inside KeyPair, and makes it a refcounted object, rather than copying the private key on every authenticator constructor. It also allows authenticators to use the KeyPair methods (such as GetSignature() or GetPublicKey()). BUG=115899 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186709

Patch Set 1 : Move HostKeyPair into protocol::KeyPair #

Total comments: 2

Patch Set 2 : Add missing files, move TestKeyPair. #

Total comments: 47

Patch Set 3 : Review comments #

Patch Set 4 : Move KeyPair to base/RsaKeyPair - make it refcounted #

Patch Set 5 : Rename key_pair_ to local_key_pair_ #

Total comments: 36

Patch Set 6 : Reviewer comments #

Total comments: 16

Patch Set 7 : Fix reviewer comments #

Patch Set 8 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+330 lines, -447 lines) Patch
A remoting/base/rsa_key_pair.h View 1 2 3 4 5 6 1 chunk +55 lines, -0 lines 0 comments Download
A + remoting/base/rsa_key_pair.cc View 1 2 3 4 5 6 4 chunks +29 lines, -37 lines 0 comments Download
A remoting/base/rsa_key_pair_unittest.cc View 1 2 3 4 5 1 chunk +98 lines, -0 lines 0 comments Download
A + remoting/base/test_rsa_key_pair.h View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M remoting/host/chromoting_host.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M remoting/host/heartbeat_sender.h View 1 2 3 4 chunks +5 lines, -5 lines 0 comments Download
M remoting/host/heartbeat_sender.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M remoting/host/heartbeat_sender_unittest.cc View 1 2 3 4 5 5 chunks +9 lines, -8 lines 0 comments Download
D remoting/host/host_key_pair.h View 1 chunk +0 lines, -51 lines 0 comments Download
D remoting/host/host_key_pair.cc View 1 chunk +0 lines, -118 lines 0 comments Download
M remoting/host/host_key_pair_unittest.cc View 1 chunk +0 lines, -70 lines 0 comments Download
M remoting/host/plugin/host_script_object.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M remoting/host/plugin/host_script_object.cc View 1 2 3 4 5 7 chunks +9 lines, -10 lines 0 comments Download
M remoting/host/register_support_host_request.h View 1 2 3 3 chunks +3 lines, -3 lines 0 comments Download
M remoting/host/register_support_host_request.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M remoting/host/register_support_host_request_unittest.cc View 1 2 3 4 5 5 chunks +10 lines, -8 lines 0 comments Download
M remoting/host/remoting_me2me_host.cc View 1 2 3 4 5 5 chunks +14 lines, -5 lines 0 comments Download
M remoting/host/setup/host_starter.h View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M remoting/host/setup/host_starter.cc View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
D remoting/host/test_key_pair.h View 1 1 chunk +0 lines, -30 lines 0 comments Download
M remoting/protocol/authenticator_test_base.h View 1 2 3 4 5 2 chunks +5 lines, -5 lines 0 comments Download
M remoting/protocol/authenticator_test_base.cc View 1 2 3 4 5 6 2 chunks +6 lines, -7 lines 0 comments Download
M remoting/protocol/it2me_host_authenticator_factory.h View 1 2 3 3 chunks +6 lines, -6 lines 0 comments Download
M remoting/protocol/it2me_host_authenticator_factory.cc View 1 2 3 3 chunks +4 lines, -4 lines 0 comments Download
M remoting/protocol/me2me_host_authenticator_factory.h View 1 2 3 2 chunks +6 lines, -7 lines 0 comments Download
M remoting/protocol/me2me_host_authenticator_factory.cc View 1 2 3 4 5 3 chunks +4 lines, -4 lines 0 comments Download
M remoting/protocol/negotiating_authenticator.h View 1 2 3 4 5 6 3 chunks +8 lines, -8 lines 0 comments Download
M remoting/protocol/negotiating_authenticator.cc View 1 2 3 4 5 6 7 4 chunks +8 lines, -11 lines 0 comments Download
M remoting/protocol/negotiating_authenticator_unittest.cc View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator.h View 1 2 3 4 5 6 4 chunks +5 lines, -6 lines 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator.cc View 1 2 3 4 5 6 4 chunks +6 lines, -6 lines 0 comments Download
M remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc View 1 2 3 4 5 6 chunks +9 lines, -9 lines 0 comments Download
M remoting/protocol/v2_authenticator.h View 1 2 3 4 5 3 chunks +5 lines, -6 lines 0 comments Download
M remoting/protocol/v2_authenticator.cc View 1 2 3 4 5 4 chunks +5 lines, -5 lines 0 comments Download
M remoting/protocol/v2_authenticator_unittest.cc View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M remoting/remoting.gyp View 1 2 3 4 5 6 7 5 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
rmsousa
Hi Wez, This refactor is a prerequisite for the token authentication CL (the host authenticator ...
7 years, 7 months ago (2013-02-23 00:48:51 UTC) #1
Wez
sergeyu: Can you take a look as well, please? rmsousa: Mostly this looks pretty good. ...
7 years, 7 months ago (2013-02-23 03:43:19 UTC) #2
Wez
BTW, please add a BUG to the CL description. If you make the scoped_ptr<> changes ...
7 years, 7 months ago (2013-02-23 03:45:06 UTC) #3
Sergey Ulanov
I don't like that we copy KeyPair objects everywhere. Problem is that crypto::RSAPrivateKey stores system-specific ...
7 years, 7 months ago (2013-02-26 00:14:13 UTC) #4
Sergey Ulanov
Also if we do copy the keys, it might be better to pass them as ...
7 years, 7 months ago (2013-02-26 00:34:42 UTC) #5
Wez
On 2013/02/26 00:34:42, sergeyu wrote: > Also if we do copy the keys, it might ...
7 years, 7 months ago (2013-02-26 01:42:35 UTC) #6
rmsousa
On 2013/02/26 00:14:13, sergeyu wrote: > I don't like that we copy KeyPair objects everywhere. ...
7 years, 7 months ago (2013-02-26 02:38:52 UTC) #7
rmsousa
ping?
7 years, 6 months ago (2013-03-05 23:47:17 UTC) #8
Wez
Looking pretty good. https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair.h File remoting/base/rsa_key_pair.h (right): https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair.h#newcode23 remoting/base/rsa_key_pair.h:23: // Generates a new (random) private ...
7 years, 6 months ago (2013-03-06 00:43:25 UTC) #9
rmsousa
ptal https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair.h File remoting/base/rsa_key_pair.h (right): https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair.h#newcode23 remoting/base/rsa_key_pair.h:23: // Generates a new (random) private key from ...
7 years, 6 months ago (2013-03-06 04:36:49 UTC) #10
Sergey Ulanov
LGTM once my nits are addressed. https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pair.cc File remoting/base/rsa_key_pair.cc (right): https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pair.cc#newcode21 remoting/base/rsa_key_pair.cc:21: RsaKeyPair::RsaKeyPair() { } ...
7 years, 6 months ago (2013-03-06 20:51:34 UTC) #11
rmsousa
https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pair.cc File remoting/base/rsa_key_pair.cc (right): https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pair.cc#newcode21 remoting/base/rsa_key_pair.cc:21: RsaKeyPair::RsaKeyPair() { } On 2013/03/06 20:51:34, sergeyu wrote: > ...
7 years, 6 months ago (2013-03-07 03:27:44 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12316083/33001
7 years, 6 months ago (2013-03-07 03:28:20 UTC) #13
commit-bot: I haz the power
Failed to apply patch for remoting/protocol/negotiating_authenticator.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 6 months ago (2013-03-07 03:28:31 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12316083/37001
7 years, 6 months ago (2013-03-07 05:16:13 UTC) #15
commit-bot: I haz the power
7 years, 6 months ago (2013-03-07 13:43:15 UTC) #16
Message was sent while issue was closed.
Change committed as 186709

Powered by Google App Engine
This is Rietveld 408576698