Authenticate user/password with PAM in BeginSessionRequest()

remoting/host/user_auth_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_auth_pam.h"
+
+#include <security/pam_appl.h>
+#include <stdlib.h>
+#include <string>
+
+namespace remoting {
+
+// Private Implementation pattern to avoid leaking PAM types, and the
+// conversation function, into the header file.
+class UserAuthPamPimpl {
+ public:
+  explicit UserAuthPamPimpl(UserAuthPam* user_auth_pam);
+  ~UserAuthPamPimpl();
+  bool Authenticate(const std::string& username,
+                    const std::string& password);
+
+ private:
+  // Conversation function passed to PAM as a callback.
+  static int ConvFunction(int num_msg,
+                          const struct pam_message** msg,
+                          struct pam_response** resp,
+                          void* appdata_ptr);
+
+  UserAuthPam* user_auth_pam_;
+  pam_handle_t* pam_handle_;
+
+  // Store these for the PAM conversation function.
+  std::string username_;
+  std::string password_;
+};
+
+UserAuthPamPimpl::UserAuthPamPimpl(UserAuthPam* user_auth_pam)
+    : user_auth_pam_(user_auth_pam) {
+}
+
+UserAuthPamPimpl::~UserAuthPamPimpl() {
+}
+
+bool UserAuthPamPimpl::Authenticate(const std::string& username,
+                                    const std::string& password) {
+  username_ = username;
+  password_ = password;
+  struct pam_conv c;
+  c.conv = ConvFunction;
+  c.appdata_ptr = static_cast<void*>(this);
+  // TODO(lambroslambrou): Allow PAM service name to be configurable.
+  if (pam_start("chromoting", username_.c_str(), &c, &pam_handle_) !=
+      PAM_SUCCESS) {
+    return false;
+  }
+
+  // TODO(lambroslambrou): Move to separate thread.
+  int pam_status = pam_authenticate(pam_handle_, 0);
+  pam_end(pam_handle_, pam_status);
+  return pam_status == PAM_SUCCESS;
+}
+
+// static
+int UserAuthPamPimpl::ConvFunction(int num_msg,
+                                   const struct pam_message** msg,

[awong, 2011/02/15 01:05:28]

Since this is c++, we don't need to explicitly tag types with "struct".  Remove
here and below?

[Lambros, 2011/02/15 15:16:35]

Done.

+                                   struct pam_response** resp,
+                                   void* appdata_ptr) {
+  if (num_msg <= 0)
+    return PAM_CONV_ERR;
+  UserAuthPamPimpl* user_auth =
+      reinterpret_cast<UserAuthPamPimpl*>(appdata_ptr);
+  // Must allocate with malloc(), as the calling PAM module will
+  // release the memory with free().
+  struct pam_response* resp_tmp = reinterpret_cast<struct pam_response*>(

[awong, 2011/02/15 01:05:28]

Use a scoped_ptr_malloc<> to hold the result?

[Lambros, 2011/02/15 15:16:35]

I had a go at this, but I found it more trouble than it's worth.
The main problem is that the smart ptr class can only do half the cleanup job -
it cannot clean up the strdup()ed strings, so we still need some dirty cleanup
code anyway.  Then it makes sense to have the cleanup in one place, rather than
hiding bits of it away inside a C++ destructor.
Although the smart ptr saves you from forgetting a "free()" in the cleanup code,
it introduces another thing you have to remember: calling release() before
handing it back to PAM.  Also, it doesn't really work as an array with []
syntax.  So I don't think it fits this situation.

+      malloc(num_msg * sizeof(struct pam_response)));
+  if (resp_tmp == NULL)
+    return PAM_CONV_ERR;
+  bool raise_error = false;
+  // On exit from the loop, 'count' will hold the number of initialised items
+  // that the cleanup code needs to look at, in case of error.
+  int count;
+  for (count = 0; count < num_msg; count++) {
+    // Alias for readability.
+    struct pam_response& r = resp_tmp[count];

[awong, 2011/02/15 01:05:28]

Please avoid single-letter variable names.

Also, people generally avoid non-const references in Chromium code.

[Lambros, 2011/02/15 15:16:35]

Done.

+    r.resp_retcode = 0;
+    r.resp = NULL;
+    switch (msg[count]->msg_style) {
+      case PAM_PROMPT_ECHO_ON:
+        r.resp = strdup(user_auth->username_.c_str());
+        if (r.resp == NULL)
+          raise_error = true;
+        break;
+      case PAM_PROMPT_ECHO_OFF:
+        r.resp = strdup(user_auth->password_.c_str());
+        if (r.resp == NULL)
+          raise_error = true;
+        break;
+      case PAM_TEXT_INFO:
+        // No response needed, continue with next prompt.

[awong, 2011/02/15 01:05:28]

Explain why no response is needed...

[Lambros, 2011/02/15 15:16:35]

Done.  I've added a little bit more to the comment.

+        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);
+      }
+    }
+    free(resp_tmp);
+    return PAM_CONV_ERR;
+  } else {
+    *resp = resp_tmp;
+    return PAM_SUCCESS;
+  }
+}
+
+UserAuthPam::UserAuthPam() : pimpl_(new UserAuthPamPimpl(this)) {
+}
+
+UserAuthPam::~UserAuthPam() {
+}
+
+bool UserAuthPam::Authenticate(const std::string& username,
+                               const std::string& password) {
+  return pimpl_->Authenticate(username, password);
+}
+
+}  // namespace remoting