[Chromoting] Let the Windows daemon controller read the unprivileged part of the config without ele…

[Chromoting] Let the Windows daemon controller read the unprivileged part of the config without elevating permissions.

The elevated controller writes the unprivileged part of the config to a file
with an ACL that lets the daemon controller read it.

BUG=121518
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133964

[simonmorris, 2012-04-23 17:11:23]

ptal

[alexeypa (please no reviews), 2012-04-23 17:35:10]

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:49: const char* const
kUnprivilegedConfigKeys[] = { kHostId, kXmppLogin };
kReadonlyKeys and kUnprivilegedConfigKeys strangely match. It does not seem to
be a coincidence.

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:53: HRESULT ReadConfig(const FilePath&
filename,
Is ReadConfig used now, when GetCOnfig is implemented in the plugin?

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:105: const char16* descriptor =
privileged ?
I somehow feel that passing ACL and filename to this function is better than
passing a flag.

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:154: if
(!MoveFileExW(tempname.value().c_str(),
Both MoveFileEx should the last thing to happen (unless you want to use NTFS
transactions). I.e. the sequence should be CreateTempFile1, CreateTempFile2,
MoveFileEx, MoveFileEx.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:8: #include <sddl.h>
I don't think you need to include sddl.h here

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:28: #include
"remoting/host/daemon_controller_common_win.h"
Is it a new file? I don't see it in the CL.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:316:
callback.Run(scoped_ptr<base::DictionaryValue>(ReadConfig()));
Why is a separate private method? DoXxx methods are the worker methods and
ReadConfig's code naturally belongs to DoGetConfig.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:432: FilePath filename =
dir.Append(kUnprivilegedConfigFileName);
This is a leaking abstraction. The COM component is supposed to hide the details
like the name of configuration file and so on. Let's talk offline to figure out
a better packaging/abstraction for this code.

http://codereview.chromium.org/10191007/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10191007/diff/1/remoting/remoting.gyp#newcode305
remoting/remoting.gyp:305: 'host/daemon_controller_common_win.h',
There files are missing from the CL.

[simonmorris, 2012-04-23 19:39:49]

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:49: const char* const
kUnprivilegedConfigKeys[] = { kHostId, kXmppLogin };
On 2012/04/23 17:35:10, alexeypa wrote:
> kReadonlyKeys and kUnprivilegedConfigKeys strangely match. It does not seem to
> be a coincidence.  

Yes, but is it clear that the two sets will always be equal, even if we later
add new keys?

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:53: HRESULT ReadConfig(const FilePath&
filename,
On 2012/04/23 17:35:10, alexeypa wrote:
> Is ReadConfig used now, when GetCOnfig is implemented in the plugin?

No, but there's no need to use this CL to remove it.

I've added a TODO.

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:105: const char16* descriptor =
privileged ?
On 2012/04/23 17:35:10, alexeypa wrote:
> I somehow feel that passing ACL and filename to this function is better than
> passing a flag.

Done.

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:154: if
(!MoveFileExW(tempname.value().c_str(),
On 2012/04/23 17:35:10, alexeypa wrote:
> Both MoveFileEx should the last thing to happen (unless you want to use NTFS
> transactions). I.e. the sequence should be CreateTempFile1, CreateTempFile2,
> MoveFileEx, MoveFileEx.

I'll make this change when we're sure that the unprivileged config file should
be written here.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:8: #include <sddl.h>
On 2012/04/23 17:35:10, alexeypa wrote:
> I don't think you need to include sddl.h here

Done.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:28: #include
"remoting/host/daemon_controller_common_win.h"
On 2012/04/23 17:35:10, alexeypa wrote:
> Is it a new file? I don't see it in the CL.

Done.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:316:
callback.Run(scoped_ptr<base::DictionaryValue>(ReadConfig()));
On 2012/04/23 17:35:10, alexeypa wrote:
> Why is a separate private method? DoXxx methods are the worker methods and
> ReadConfig's code naturally belongs to DoGetConfig.

Only because ReadConfig forms a pair with WriteConfig. I don't mind merging
ReadConfig into DoGetConfig, if we're sure that WriteConfig doesn't belong in
this file.

http://codereview.chromium.org/10191007/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/10191007/diff/1/remoting/remoting.gyp#newcode305
remoting/remoting.gyp:305: 'host/daemon_controller_common_win.h',
On 2012/04/23 17:35:10, alexeypa wrote:
> There files are missing from the CL.

Done.

[simonmorris, 2012-04-24 16:26:45]

ping

[alexeypa (please no reviews), 2012-04-24 16:54:56]

It should be good to go as soon as the remaining comments are addressed.

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:49: const char* const
kUnprivilegedConfigKeys[] = { kHostId, kXmppLogin };
On 2012/04/23 19:39:49, simonmorris wrote:
> On 2012/04/23 17:35:10, alexeypa wrote:
> > kReadonlyKeys and kUnprivilegedConfigKeys strangely match. It does not seem
to
> > be a coincidence.  
> Yes, but is it clear that the two sets will always be equal, even if we later
> add new keys?

As per our discussion these lists are not the same. Both are while lists. Please
file a bug so we will not forget to make sure it is correctly documented and
documented in the code.

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:53: HRESULT ReadConfig(const FilePath&
filename,
On 2012/04/23 19:39:49, simonmorris wrote:
> On 2012/04/23 17:35:10, alexeypa wrote:
> > Is ReadConfig used now, when GetCOnfig is implemented in the plugin?
> 
> No, but there's no need to use this CL to remove it.
> 
> I've added a TODO.

Please include this into the COM interfaces cleanup bug and assign it to me.

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:154: if
(!MoveFileExW(tempname.value().c_str(),
On 2012/04/23 19:39:49, simonmorris wrote:
> On 2012/04/23 17:35:10, alexeypa wrote:
> > Both MoveFileEx should the last thing to happen (unless you want to use NTFS
> > transactions). I.e. the sequence should be CreateTempFile1, CreateTempFile2,
> > MoveFileEx, MoveFileEx.
> 
> I'll make this change when we're sure that the unprivileged config file should
> be written here.

Just to document what we discussed. Both files will be written by the privileged
code. The name of the unprivileged file will be a part of the interface as a
temporary solution. We will file a bug (the COM interface cleanup one) to
address it later.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:316:
callback.Run(scoped_ptr<base::DictionaryValue>(ReadConfig()));
On 2012/04/23 19:39:49, simonmorris wrote:
> On 2012/04/23 17:35:10, alexeypa wrote:
> > Why is a separate private method? DoXxx methods are the worker methods and
> > ReadConfig's code naturally belongs to DoGetConfig.
> 
> Only because ReadConfig forms a pair with WriteConfig. I don't mind merging
> ReadConfig into DoGetConfig, if we're sure that WriteConfig doesn't belong in
> this file.

WriteConfig belongs to the privileged code so I guess you can move the code back
to DoGetConfig

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:432: FilePath filename =
dir.Append(kUnprivilegedConfigFileName);
On 2012/04/23 17:35:10, alexeypa wrote:
> This is a leaking abstraction. The COM component is supposed to hide the
details
> like the name of configuration file and so on. Let's talk offline to figure
out
> a better packaging/abstraction for this code.

We have decided to file a bug to tracking fixing it. We will fix it in the
future if it cause enough pain.

http://codereview.chromium.org/10191007/diff/9001/remoting/host/daemon_contro...
File remoting/host/daemon_controller_common_win.h (right):

http://codereview.chromium.org/10191007/diff/9001/remoting/host/daemon_contro...
remoting/host/daemon_controller_common_win.h:15: // The unprivileged
configuration file name.
nit: Document the location of the file as well.

[simonmorris, 2012-04-25 00:33:32]

ptal

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:49: const char* const
kUnprivilegedConfigKeys[] = { kHostId, kXmppLogin };
On 2012/04/24 16:54:56, alexeypa wrote:
> On 2012/04/23 19:39:49, simonmorris wrote:
> > On 2012/04/23 17:35:10, alexeypa wrote:
> > > kReadonlyKeys and kUnprivilegedConfigKeys strangely match. It does not
seem
> to
> > > be a coincidence.  
> > Yes, but is it clear that the two sets will always be equal, even if we
later
> > add new keys?
> 
> As per our discussion these lists are not the same. Both are while lists.
Please
> file a bug so we will not forget to make sure it is correctly documented and
> documented in the code.

crbug.com/124825

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:53: HRESULT ReadConfig(const FilePath&
filename,
On 2012/04/24 16:54:56, alexeypa wrote:
> On 2012/04/23 19:39:49, simonmorris wrote:
> > On 2012/04/23 17:35:10, alexeypa wrote:
> > > Is ReadConfig used now, when GetCOnfig is implemented in the plugin?
> > 
> > No, but there's no need to use this CL to remove it.
> > 
> > I've added a TODO.
> 
> Please include this into the COM interfaces cleanup bug and assign it to me.

crbug.com/124937

http://codereview.chromium.org/10191007/diff/1/remoting/host/elevated_control...
remoting/host/elevated_controller_win.cc:154: if
(!MoveFileExW(tempname.value().c_str(),
On 2012/04/23 17:35:10, alexeypa wrote:
> Both MoveFileEx should the last thing to happen (unless you want to use NTFS
> transactions). I.e. the sequence should be CreateTempFile1, CreateTempFile2,
> MoveFileEx, MoveFileEx.

Done.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:316:
callback.Run(scoped_ptr<base::DictionaryValue>(ReadConfig()));
On 2012/04/24 16:54:56, alexeypa wrote:
> On 2012/04/23 19:39:49, simonmorris wrote:
> > On 2012/04/23 17:35:10, alexeypa wrote:
> > > Why is a separate private method? DoXxx methods are the worker methods and
> > > ReadConfig's code naturally belongs to DoGetConfig.
> > 
> > Only because ReadConfig forms a pair with WriteConfig. I don't mind merging
> > ReadConfig into DoGetConfig, if we're sure that WriteConfig doesn't belong
in
> > this file.
> 
> WriteConfig belongs to the privileged code so I guess you can move the code
back
> to DoGetConfig
> 

Done.

http://codereview.chromium.org/10191007/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:432: FilePath filename =
dir.Append(kUnprivilegedConfigFileName);
On 2012/04/24 16:54:56, alexeypa wrote:
> On 2012/04/23 17:35:10, alexeypa wrote:
> > This is a leaking abstraction. The COM component is supposed to hide the
> details
> > like the name of configuration file and so on. Let's talk offline to figure
> out
> > a better packaging/abstraction for this code.
> 
> We have decided to file a bug to tracking fixing it. We will fix it in the
> future if it cause enough pain.

crbug.com/124937

http://codereview.chromium.org/10191007/diff/9001/remoting/host/daemon_contro...
File remoting/host/daemon_controller_common_win.h (right):

http://codereview.chromium.org/10191007/diff/9001/remoting/host/daemon_contro...
remoting/host/daemon_controller_common_win.h:15: // The unprivileged
configuration file name.
On 2012/04/24 16:54:56, alexeypa wrote:
> nit: Document the location of the file as well.

Done.

[alexeypa (please no reviews), 2012-04-25 16:36:56]

LGTM

http://codereview.chromium.org/10191007/diff/15001/remoting/host/daemon_contr...
File remoting/host/daemon_controller_common_win.h (right):

http://codereview.chromium.org/10191007/diff/15001/remoting/host/daemon_contr...
remoting/host/daemon_controller_common_win.h:16: // remoting::GetConfigDir().
nit: ... is returned by remoting::GetConfigDir(), maybe?

http://codereview.chromium.org/10191007/diff/15001/remoting/host/elevated_con...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/15001/remoting/host/elevated_con...
remoting/host/elevated_controller_win.cc:129: {
nit: This scope is not needed any more. It was needed to close the file handle
before calling MoveFile()

[simonmorris, 2012-04-25 17:08:44]

fyi

http://codereview.chromium.org/10191007/diff/15001/remoting/host/daemon_contr...
File remoting/host/daemon_controller_common_win.h (right):

http://codereview.chromium.org/10191007/diff/15001/remoting/host/daemon_contr...
remoting/host/daemon_controller_common_win.h:16: // remoting::GetConfigDir().
On 2012/04/25 16:36:56, alexeypa wrote:
> nit: ... is returned by remoting::GetConfigDir(), maybe?

Done.

http://codereview.chromium.org/10191007/diff/15001/remoting/host/elevated_con...
File remoting/host/elevated_controller_win.cc (right):

http://codereview.chromium.org/10191007/diff/15001/remoting/host/elevated_con...
remoting/host/elevated_controller_win.cc:129: {
On 2012/04/25 16:36:56, alexeypa wrote:
> nit: This scope is not needed any more. It was needed to close the file handle
> before calling MoveFile()

Done.

[commit-bot: I haz the power, 2012-04-25 18:05:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10191007/5

[commit-bot: I haz the power, 2012-04-25 20:13:36]

Change committed as 133964