improve error handling in the auth_client library, and fix the SSL options:...

improve error handling in the auth_client library, and fix the SSL options: test passes with libcurl-openssl

pam/auth_client.c

@@ -6,9 +6,14 @@
 #ifdef HAVE_MEMORY_H
 #include <memory.h>
 #endif
+#include <sys/stat.h>
 #include <curl/curl.h>
 #include "auth_client.h"
 
+#define CURL_CHECK(x) { \
+  int _err = (x); if (_err != CURLE_OK) { return auth_client_err_from_curl(_err); } \
+}
+
 static const char *kAuthApiPath = "/api/1/auth";
 
 struct auth_client {
@@ -17,10 +22,11 @@ struct auth_client {
   const char *server;
 };
 
-static void auth_client_set_proto(auth_client_t ac, const char *proto) {
+static int auth_client_set_proto(auth_client_t ac, const char *proto) {
   char url[strlen(ac->server) + 32];
   sprintf(url, "%s://%s%s", proto, ac->server, kAuthApiPath);
-  curl_easy_setopt(ac->c, CURLOPT_URL, url);
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_URL, url));
+  return AC_OK;
 }
 
 auth_client_t auth_client_new(const char *service, const char *server) {
@@ -35,18 +41,34 @@ auth_client_t auth_client_new(const char *service, const char *server) {
   return ac;
 }
 
-void auth_client_set_certificate(auth_client_t ac,
-                                 const char *ca_file,
-                                 const char *crt_file,
-                                 const char *key_file) {
-  curl_easy_setopt(ac->c, CURLOPT_CAPATH, ca_file);
-  curl_easy_setopt(ac->c, CURLOPT_SSLCERTTYPE, "PEM");
-  curl_easy_setopt(ac->c, CURLOPT_SSLCERT, crt_file);
-  curl_easy_setopt(ac->c, CURLOPT_SSLKEYTYPE, "PEM");
-  curl_easy_setopt(ac->c, CURLOPT_SSLKEY, key_file);
-  curl_easy_setopt(ac->c, CURLOPT_SSL_VERIFYPEER, 1);
-  curl_easy_setopt(ac->c, CURLOPT_SSL_VERIFYHOST, 0);
-  auth_client_set_proto(ac, "https");
+static int file_exists(const char *path) {
+  struct stat stbuf;
+  if (stat(path, &stbuf) < 0) {
+    return 0;
+  }
+  return 1;
+}
+
+void auth_client_set_verbose(auth_client_t ac, int verbose) {
+  curl_easy_setopt(ac->c, CURLOPT_VERBOSE, verbose);
+}
+
+int auth_client_set_certificate(auth_client_t ac,
+                                const char *ca_file,
+                                const char *crt_file,
+                                const char *key_file) {
+  if (!file_exists(ca_file) || !file_exists(crt_file) || !file_exists(key_file)) {
+    return AC_ERR_FILE_NOT_FOUND;
+  }
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_SSLCERTTYPE, "PEM"));
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_SSLCERT, crt_file));
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_SSLKEYTYPE, "PEM"));
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_SSLKEY, key_file));
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_CAINFO, ca_file));
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_SSL_VERIFYPEER, 2));
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_SSL_VERIFYHOST, 0));
+  CURL_CHECK(curl_easy_setopt(ac->c, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1));
+  return auth_client_set_proto(ac, "https");
 }
 
 void auth_client_free(auth_client_t ac) {
@@ -63,6 +85,10 @@ const char *auth_client_strerror(int err) {
     return "Authentication failure";
   case AC_ERR_OTP_REQUIRED:
     return "OTP required";
+  case AC_ERR_BAD_RESPONSE:
+    return "Bad server response";
+  case AC_ERR_FILE_NOT_FOUND:
+    return "Certificate or CA file not found";
   default:
     return "Unknown error";
   }
@@ -144,7 +170,7 @@ static void post_field_add(struct cbuf *form_data, const char *key, const char *
     cbuf_append(form_data, "&", 1);
   }
   cbuf_append(form_data, quoted_key, strlen(quoted_key));
-  cbuf_append(form_data, "&", 1);
+  cbuf_append(form_data, "=", 1);
   cbuf_append(form_data, quoted_value, strlen(quoted_value));
   free(quoted_key);
   free(quoted_value);

pam/auth_client.h

@@ -10,6 +10,7 @@ typedef struct auth_client* auth_client_t;
 #define AC_ERR_AUTHENTICATION_FAILURE  -1
 #define AC_ERR_OTP_REQUIRED            -2
 #define AC_ERR_BAD_RESPONSE            -3
+#define AC_ERR_FILE_NOT_FOUND          -4
 #define AC_ERR_CURL_BASE             -100
 #define auth_client_err_to_curl(e)     (-(e)+(AC_ERR_CURL_BASE))
 #define auth_client_err_from_curl(e)   ((AC_ERR_CURL_BASE)-(e))
@@ -17,10 +18,11 @@ typedef struct auth_client* auth_client_t;
 auth_client_t auth_client_new(const char *service, const char *server);
 void auth_client_free(auth_client_t ac);
 const char *auth_client_strerror(int err);
-void auth_client_set_certificate(auth_client_t ac,
-                                 const char *ca_file,
-                                 const char *crt_file,
-                                 const char *key_file);
+void auth_client_set_verbose(auth_client_t ac, int verbose);
+int auth_client_set_certificate(auth_client_t ac,
+                                const char *ca_file,
+                                const char *crt_file,
+                                const char *key_file);
 int auth_client_authenticate(auth_client_t ac,
                              const char *username,
                              const char *password,

pam/auth_client_test.cc

@@ -8,8 +8,8 @@ extern "C" {
 
 static const char *server = NULL;
 
-static const char *ssl_ca = "../authserv/test/testca/public/ca.pem";
-static const char *ssl_cert = "../authserv/test/testca/public/certs/client.pem";
+static const char *ssl_ca = "../authserv/test/testca/ca.pem";
+static const char *ssl_cert = "../authserv/test/testca/certs/client.pem";
 static const char *ssl_key = "../authserv/test/testca/private/client.key";
 
 TEST(AuthClientCurlInterface, ErrorConversion) {
@@ -22,7 +22,21 @@ TEST(AuthClientCurlInterface, ErrorConversion) {
 TEST(AuthClient, NewAndFree) {
   auth_client_t ac;
   ac = auth_client_new("service", server);
-  EXPECT_TRUE(ac != NULL);
+  ASSERT_TRUE(ac != NULL);
+
+  auth_client_free(ac);
+}
+
+TEST(AuthClient, CertSetupFailsWithoutCA) {
+  auth_client_t ac = auth_client_new("service", server);
+  ASSERT_TRUE(ac != NULL);
+
+  EXPECT_NE(AC_OK,
+            auth_client_set_certificate(ac, "nonexisting.pem", ssl_cert, ssl_key));
+  EXPECT_NE(AC_OK,
+            auth_client_set_certificate(ac, ssl_ca, "nonexisting.pem", ssl_key));
+  EXPECT_NE(AC_OK,
+            auth_client_set_certificate(ac, ssl_ca, ssl_cert, "nonexisting.key"));
 
   auth_client_free(ac);
 }
@@ -31,13 +45,17 @@ TEST(AuthClient, AuthOK) {
   auth_client_t ac;
   int result;
 
-  printf("Connecting to %s...\n", server);
   ac = auth_client_new("service", server);
-  EXPECT_TRUE(ac != NULL);
+  ASSERT_TRUE(ac != NULL);
+
+  auth_client_set_verbose(ac, 1);
+
+  result = auth_client_set_certificate(ac, ssl_ca, ssl_cert, ssl_key);
+  EXPECT_EQ(AC_OK, result) << "set_certificate() error: " << auth_client_strerror(result);
 
-  auth_client_set_certificate(ac, ssl_ca, ssl_cert, ssl_key);
   result = auth_client_authenticate(ac, "user", "pass", NULL, "127.0.0.1");
-  EXPECT_EQ(AC_OK, result) << "Got error: " << auth_client_strerror(result);
+  EXPECT_EQ(AC_OK, result) << "authenticate() error: " << auth_client_strerror(result)
+                           << ", server=" << server;
 
   auth_client_free(ac);
 }

pam/pam_authclient.c

@@ -140,14 +140,18 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh,
     password = resp->resp;
   }
 
+  retval = PAM_AUTH_ERR;
+
   // Create the auth client request.
   ac = auth_client_new(service, cfg.auth_server);
   if (cfg.ssl_crt && cfg.ssl_key && cfg.ca_file) {
-    auth_client_set_certificate(ac, cfg.ca_file, cfg.ssl_crt, cfg.ssl_key);
+    err = auth_client_set_certificate(ac, cfg.ca_file, cfg.ssl_crt, cfg.ssl_key);
+    if (err != AC_OK) {
+      D(("auth_client_set_certificate() error: %s", auth_client_strerror(err)));
+      goto error;
+    }
   }
 
-  retval = PAM_AUTH_ERR;
-
   // Allow two authentication attempts in case we receive an
   // OTP_REQUIRED response from the server.
   for (i = 0; i < 2; i++) {
@@ -183,6 +187,7 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh,
     break;
   }
 
+ error:
   auth_client_free(ac);
   return retval;
 }