Authenticate user/password with PAM in BeginSessionRequest()

remoting/host/user_authenticator_pam.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/user_authenticator_pam.h"
+
+#include <security/pam_appl.h>
+#include <stdlib.h>

[awong, 2011/02/15 16:36:06]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...
I think this should be

#include <stdlib.h>

#include <string>

#include <security/pam_appl.h>

[Lambros, 2011/02/15 18:15:51]

Done.

+#include <string>
+
+namespace remoting {
+
+UserAuthenticatorPam::UserAuthenticatorPam() {
+}
+
+UserAuthenticatorPam::~UserAuthenticatorPam() {
+}
+
+bool UserAuthenticatorPam::Authenticate(const std::string& username,
+                                        const std::string& password) {
+  username_ = username;
+  password_ = password;
+  pam_conv conversation;
+  conversation.conv = ConvFunction;
+  conversation.appdata_ptr = static_cast<void*>(this);

[awong, 2011/02/15 16:36:06]

Do you need a static_cast in order to upcast to void*?

[Lambros, 2011/02/15 18:15:51]

You don't need it.  But since void* is a "strange" type, I thought it best to
make the cast explicit here.
I''ll remove it if that's what people prefer.

+  // TODO(lambroslambrou): Allow PAM service name to be configurable.
+  pam_handle_t* pam_h;

[awong, 2011/02/15 16:36:06]

pam_h -> pam_handle.

[Lambros, 2011/02/15 18:15:51]

Strange!  That's the name I originally picked, but I got compilation errors
coming from PAM.  I thought it was because PAM defines a type also called
pam_handle.
But I've made this change now and everything seems OK.

+  if (pam_start("chromoting", username_.c_str(),

[awong, 2011/02/15 16:36:06]

Pull this out into a file constant.  kPamServiceName or something.

[Lambros, 2011/02/15 18:15:51]

Done.

+                &conversation, &pam_h) != PAM_SUCCESS) {
+    return false;
+  }
+
+  // TODO(lambroslambrou): Move to separate thread.
+  int pam_status = pam_authenticate(pam_h, 0);
+  pam_end(pam_h, pam_status);
+  return pam_status == PAM_SUCCESS;
+}
+
+// static
+int UserAuthenticatorPam::ConvFunction(int num_msg,

[awong, 2011/02/15 16:36:06]

num_msg -> num_messages

[Lambros, 2011/02/15 18:15:51]

I didn't pick those names.  They come from a PAM callback interface documented
in "man pam_conv".  I don't know if I should be changing the names to follow our
own conventions, or sticking with the PAM conventions here?  Is it OK to fiddle
with parameter names in third-party interfaces?  If it is, I will happily change
these! :-)

[awong, 2011/02/15 19:45:21]

Ah, I see.  Yeah, in these APIs that interface with external code, the
conventions are a bit mixed.  Let's stick with what you have.

+                                       const pam_message** msg,

[awong, 2011/02/15 16:36:06]

msg -> message

+                                       pam_response** resp,

[awong, 2011/02/15 16:36:06]

resp -> response

+                                       void* appdata_ptr) {

[awong, 2011/02/15 16:36:06]

Usually we don't type-tag things with _ptr.  Just call appdata?

[Lambros, 2011/02/15 18:15:51]

Specifically, "appdata_ptr" is also a field-name of "struct pam_conv", which is
self-documenting where the parameter originates.  So I'm reluctant to change
this particular name.

+  if (num_msg <= 0)
+    return PAM_CONV_ERR;
+  UserAuthenticatorPam* user_auth =
+      static_cast<UserAuthenticatorPam*>(appdata_ptr);
+  // Must allocate with malloc(), as the calling PAM module will
+  // release the memory with free().
+  pam_response* resp_tmp = static_cast<pam_response*>(
+      malloc(num_msg * sizeof(pam_response)));
+  if (resp_tmp == NULL)
+    return PAM_CONV_ERR;

[awong, 2011/02/15 16:36:06]

Add a newline here?  This block of code is a bit too dense, and I think there's
a natural break between setting up the outer "resp" object and its fields.

[Lambros, 2011/02/15 18:15:51]

Done.

+  bool raise_error = false;
+  // On exit from the loop, 'count' will hold the number of initialised items

[awong, 2011/02/15 16:36:06]

initialised -> initialized. 

(kidding...ignore this comment ;))

[Lambros, 2011/02/15 18:15:51]

Hehe, never noticed that one!  I'm just giving the code some international
flavour (sorry, "flavor")!

+  // that the cleanup code needs to look at, in case of error.
+  int count;
+  for (count = 0; count < num_msg; count++) {
+    // Alias for readability.
+    pam_response* resp_item = &resp_tmp[count];
+    resp_item->resp_retcode = 0;
+    resp_item->resp = NULL;
+    switch (msg[count]->msg_style) {
+      case PAM_PROMPT_ECHO_ON:
+        resp_item->resp = strdup(user_auth->username_.c_str());
+        if (resp_item->resp == NULL)
+          raise_error = true;
+        break;
+      case PAM_PROMPT_ECHO_OFF:
+        resp_item->resp = strdup(user_auth->password_.c_str());
+        if (resp_item->resp == NULL)
+          raise_error = true;
+        break;
+      case PAM_TEXT_INFO:
+        // No response needed, as this instructs the PAM client to display
+        // text to the user.  Leave as NULL and continue with next prompt.
+        break;
+      default:
+        // Unexpected style code, so abort.
+        raise_error = true;
+    }
+    if (raise_error)
+      break;
+  }
+
+  if (raise_error) {
+    // Not passing the response back, so free up any memory used.
+    for (int n = 0; n < count; n++) {
+      if (resp_tmp[n].resp) {
+        free(resp_tmp[n].resp);

[awong, 2011/02/15 16:36:06]

Let's NULL out the pointer as well just to be OCD about this.

[Lambros, 2011/02/15 18:15:51]

Is there any point, when we free() the whole resp_tmp block immediately
afterwards?

[awong, 2011/02/15 19:45:21]

No, not really.  You're right.

+      }
+    }
+    free(resp_tmp);
+    return PAM_CONV_ERR;
+  } else {
+    *resp = resp_tmp;
+    return PAM_SUCCESS;
+  }
+}
+
+}  // namespace remoting