Implemented local login check for PAM (only enabled on Linux for now).

Implemented local login check for PAM (only enabled on Linux for now).

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

[Jamie, 2012-10-25 23:16:03]

ptal

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:30: INVALID_CREDENTIALS :
underlying_->rejection_reason();
This results in "invalid PIN" at the client, which is not very helpful. We might
need a new error code to support this check.

[Wez, 2012-10-26 03:25:43]

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:11: #include
"remoting/protocol/channel_authenticator.h"
nit: Can't you just forward-define ChannelAuthenticator?

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:16: class PamAuthorizer :
public protocol::Authenticator {
nit: Wrap this in an anonymous namespace.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:16: class PamAuthorizer :
public protocol::Authenticator {
nit: Consider moving this class' method implementations out-of-line; tends to
make classes easier to understand at a glance.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:23: virtual State state() const
OVERRIDE {
nit: Precede this with a comment "protocol::Authenticator implementation"

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:25: REJECTED :
underlying_->state();
nit: This would be more readable as an early-exit if and return.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:30: INVALID_CREDENTIALS :
underlying_->rejection_reason();
On 2012/10/25 23:16:03, Jamie wrote:
> This results in "invalid PIN" at the client, which is not very helpful. We
might
> need a new error code to support this check.

Agreed, but this is good enough for starters.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:30: INVALID_CREDENTIALS :
underlying_->rejection_reason();
nit: See comment on state().

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:61: struct pam_conv conv = {
PamConversation, NULL };
nit: Blank line above this, to aid readability, plz.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:69: LOG(INFO) << "Local login
check for " << username
nit: Blank lines above & below LOG line, for readability.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:78: // We don't expect this
function to be called. Since we have no easy way
Is it likely that this be called nonetheless, or could we just CHECK() here?

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:118:
underlying_->CreateAuthenticator(local_jid, remote_jid, first_message));
nit: Can you pass the output of CreateAuthenticator() directly to the
PamAuthorizer() ctor and save a couple of lines?

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.h (right):

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.h:8: #include
"remoting/protocol/authenticator.h"
nit: Blank line between this include and the next.

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

https://codereview.chromium.org/11276040/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:398: factory.reset(new
PamAuthorizationFactory(factory.Pass()));
nit: Precede this with a brief comment explaining what it adds.

https://codereview.chromium.org/11276040/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/11276040/diff/1/remoting/remoting.gyp#newcode...
remoting/remoting.gyp:1578: 'host/pam_authorization_factory_posix.cc',
Move this to host/linux/pam_authorization_factory.*?

[Jamie, 2012-10-26 17:53:30]

PTAL. Something to consider for a follow-up CL is whether or not we need to
install an explicit PAM config for chrome-remote-desktop. On modern systems it
shouldn't be necessary because the default config includes a pam_access check,
but I can envisage a scenario where things would fail due to unusual PAM
configurations.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:11: #include
"remoting/protocol/channel_authenticator.h"
On 2012/10/26 03:25:43, Wez wrote:
> nit: Can't you just forward-define ChannelAuthenticator?

No, it needs to be a complete type for scoped_ptr.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:16: class PamAuthorizer :
public protocol::Authenticator {
On 2012/10/26 03:25:43, Wez wrote:
> nit: Wrap this in an anonymous namespace.

Done.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:16: class PamAuthorizer :
public protocol::Authenticator {
On 2012/10/26 03:25:43, Wez wrote:
> nit: Consider moving this class' method implementations out-of-line; tends to
> make classes easier to understand at a glance.

Done.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:23: virtual State state() const
OVERRIDE {
On 2012/10/26 03:25:43, Wez wrote:
> nit: Precede this with a comment "protocol::Authenticator implementation"

s/implementation/interface done.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:25: REJECTED :
underlying_->state();
On 2012/10/26 03:25:43, Wez wrote:
> nit: This would be more readable as an early-exit if and return.

Done.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:30: INVALID_CREDENTIALS :
underlying_->rejection_reason();
On 2012/10/26 03:25:43, Wez wrote:
> nit: See comment on state().

Done.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:61: struct pam_conv conv = {
PamConversation, NULL };
On 2012/10/26 03:25:43, Wez wrote:
> nit: Blank line above this, to aid readability, plz.

Done.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:69: LOG(INFO) << "Local login
check for " << username
On 2012/10/26 03:25:43, Wez wrote:
> nit: Blank lines above & below LOG line, for readability.

Done.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:78: // We don't expect this
function to be called. Since we have no easy way
On 2012/10/26 03:25:43, Wez wrote:
> Is it likely that this be called nonetheless, or could we just CHECK() here?

I've changed it to a LOG(FATAL) in the case that we're asked for input, since it
simplifies the code a bit, but if we're just being asked to log something, I
think it's better to do so.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:118:
underlying_->CreateAuthenticator(local_jid, remote_jid, first_message));
On 2012/10/26 03:25:43, Wez wrote:
> nit: Can you pass the output of CreateAuthenticator() directly to the
> PamAuthorizer() ctor and save a couple of lines?

I think that would be less readable.

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.h (right):

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.h:8: #include
"remoting/protocol/authenticator.h"
On 2012/10/26 03:25:43, Wez wrote:
> nit: Blank line between this include and the next.

Done.

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

https://codereview.chromium.org/11276040/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:398: factory.reset(new
PamAuthorizationFactory(factory.Pass()));
On 2012/10/26 03:25:43, Wez wrote:
> nit: Precede this with a brief comment explaining what it adds.

Done.

https://codereview.chromium.org/11276040/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/11276040/diff/1/remoting/remoting.gyp#newcode...
remoting/remoting.gyp:1578: 'host/pam_authorization_factory_posix.cc',
On 2012/10/26 03:25:43, Wez wrote:
> Move this to host/linux/pam_authorization_factory.*?

As discussed, this is better left where it is for now. If we decide to move it
in the future, posix/ is a more natural home, since it will probably also be
used on Mac.

[Wez, 2012-10-26 21:52:23]

lgtm

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/11276040/diff/1/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:118:
underlying_->CreateAuthenticator(local_jid, remote_jid, first_message));
On 2012/10/26 17:53:30, Jamie wrote:
> On 2012/10/26 03:25:43, Wez wrote:
> > nit: Can you pass the output of CreateAuthenticator() directly to the
> > PamAuthorizer() ctor and save a couple of lines?
> 
> I think that would be less readable.

Even if you fold the new PamAuthorizer on to the return line itself?  Actually,
I think that'd be one character too long...

https://codereview.chromium.org/11276040/diff/8/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/11276040/diff/8/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:117: // need to be free()-able
zero-initialized memory.
nit: If we're assuming then should we CHECK()?  Will the caller definitely
free() the memory if it called us for some other reason?

https://codereview.chromium.org/11276040/diff/8/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:120: // We don't expect this
function to be called. Since we have no easy way
nit: Blank line before this comment.

[Jamie, 2012-10-26 22:13:53]

fyi

https://codereview.chromium.org/11276040/diff/8/remoting/host/pam_authorizati...
File remoting/host/pam_authorization_factory_posix.cc (right):

https://codereview.chromium.org/11276040/diff/8/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:117: // need to be free()-able
zero-initialized memory.
On 2012/10/26 21:52:23, Wez wrote:
> nit: If we're assuming then should we CHECK()?  Will the caller definitely
> free() the memory if it called us for some other reason?

We do effectively CHECK via the LOG(FATAL) below. The PAM docs state that the
caller will free() the memory unless we return an error code.

https://codereview.chromium.org/11276040/diff/8/remoting/host/pam_authorizati...
remoting/host/pam_authorization_factory_posix.cc:120: // We don't expect this
function to be called. Since we have no easy way
On 2012/10/26 21:52:23, Wez wrote:
> nit: Blank line before this comment.

Done.

[commit-bot: I haz the power, 2012-10-26 22:14:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/11276040/7007

[commit-bot: I haz the power, 2012-10-27 01:08:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/11276040/7007

[commit-bot: I haz the power, 2012-10-27 01:09:18]

Change committed as 164478

[antarus, 2012-10-31 20:16:54]

On 2012/10/27 01:09:18, I haz the power (commit-bot) wrote:
> Change committed as 164478

This LGTM from a PAM point of view. Depending on what you end up doing for Me2Me
functionality you may need to do something more complex. Since IT2Me uses an
existing user session, this is sufficient for that.

[Wez, 2012-10-31 20:20:05]

On 2012/10/31 20:16:54, antarus wrote:
> On 2012/10/27 01:09:18, I haz the power (commit-bot) wrote:
> > Change committed as 164478
> 
> This LGTM from a PAM point of view. Depending on what you end up doing for
Me2Me
> functionality you may need to do something more complex. Since IT2Me uses an
> existing user session, this is sufficient for that.

This change only affects Me2Me, not It2Me.  We run the Me2Me host under the
local user account under which the user registered the host.