correctly load and store the public key in mod_sso; fixes issue #20

src/mod_sso/mod_sso.cc

@@ -39,17 +39,20 @@
 
 extern "C" module AP_MODULE_DECLARE_DATA sso_module;
 
+using namespace std;
+
 typedef struct {
-   const char *login_server;
-   const char *domain;
-   const char *service;
-   char *public_key;
+  const char *login_server;
+  const char *domain;
+  const char *service;
+
+  // Note: public_key is a binary buffer (non zero-terminated).
+  const char *public_key;
+  int public_key_len;
 } modsso_config;
 
 typedef const char *(*CMD_HAND_TYPE) ();
 
-using namespace std;
-
 /**
  * Create a modsso_config structure.
  *
@@ -63,13 +66,14 @@ static void *create_modsso_config(apr_pool_t *p, char *s)
   modsso_config *newcfg;
 
   // Allocate memory from the provided pool.
-  newcfg = (modsso_config *) apr_pcalloc(p, sizeof(modsso_config));
+  newcfg = (modsso_config *)apr_palloc(p, sizeof(modsso_config));
 
   // Set default values.
   newcfg->login_server = NULL;
   newcfg->service = NULL;
   newcfg->domain = NULL;
   newcfg->public_key = NULL;
+  newcfg->public_key_len = 0;
 
   // Return the created configuration struct.
   return (void *)newcfg;
@@ -85,7 +89,14 @@ static void *merge_modsso_config(apr_pool_t *p, void *base, void *add)
   newcfg->login_server = cadd->login_server ? cadd->login_server : cbase->login_server;
   newcfg->service = cadd->service ? cadd->service : cbase->service;
   newcfg->domain = cadd->domain ? cadd->domain : cbase->domain;
-  newcfg->public_key = cadd->public_key ? cadd->public_key : cbase->public_key;
+
+  newcfg->public_key = cbase->public_key;
+  newcfg->public_key_len = cbase->public_key_len;
+  if (cadd->public_key) {
+    newcfg->public_key = cadd->public_key;
+    newcfg->public_key_len = cadd->public_key_len;
+  }    
+
   return (void *)newcfg;
 }
 
@@ -93,29 +104,29 @@ static const char *set_modsso_login_server(cmd_parms *parms,
 					   void *mconfig, const char *arg)
 {
   modsso_config *s_cfg = (modsso_config *)mconfig;
-  s_cfg->login_server = (char *) arg;
+  s_cfg->login_server = arg;
   return NULL;
 }
 
 static const char *set_modsso_service(cmd_parms *parms, void *mconfig, const char *arg)
 {
   modsso_config *s_cfg = (modsso_config *)mconfig;
-  s_cfg->service = (char *) arg;
+  s_cfg->service = arg;
   return NULL;
 }
 
 static const char *set_modsso_domain(cmd_parms *parms, void *mconfig, const char *arg)
 {
   modsso_config *s_cfg = (modsso_config *)mconfig;
-  s_cfg->domain = (char *) arg;
+  s_cfg->domain = arg;
   return NULL;
 }
 
 static const char *set_modsso_public_key_file(cmd_parms *parms, void *mconfig, const char *arg)
 {
   modsso_config *s_cfg = (modsso_config *)mconfig;
-  char buf[4096];
-  apr_size_t n = sizeof(buf) - 1;
+  char buf[128];
+  apr_size_t n = sizeof(buf);
   apr_file_t *file;
   if (apr_file_open(&file, arg, APR_FOPEN_READ, 0, parms->pool) != APR_SUCCESS) {
     return "Could not open SSOPublicKeyFile";
@@ -123,14 +134,16 @@ static const char *set_modsso_public_key_file(cmd_parms *parms, void *mconfig, c
   if (apr_file_read(file, (void *)buf, &n) != APR_SUCCESS) {
     return "Could not read contents of SSOPublicKeyFile";
   }
-  s_cfg->public_key = (char *)malloc(n + 1);
-  memcpy(s_cfg->public_key, buf, n);
-  s_cfg->public_key[n] = 0;
   apr_file_close(file);
+
+  char *key = (char *)malloc(n);
+  memcpy(key, buf, n);
+  s_cfg->public_key = key;
+  s_cfg->public_key_len = n;
+
   return NULL;
 }
 
-
 static const command_rec mod_sso_cmds[] =
 {
   AP_INIT_TAKE1("SSOLoginServer", (CMD_HAND_TYPE) set_modsso_login_server, 
@@ -148,7 +161,6 @@ static const command_rec mod_sso_cmds[] =
   {NULL, NULL, NULL, 0, cmd_how(0), NULL}
 };
 
-
 /**
  * Send the given text to the client.
  *
@@ -514,7 +526,8 @@ static int mod_sso_authenticate_user(request_rec *r)
   // Test for valid cookie
   string sso_cookie = get_cookie(r, sso_cookie_name);
   if (!sso_cookie.empty()) {
-    sso::Verifier verifier(s_cfg->public_key, s_cfg->service,
+    string pkey(s_cfg->public_key, s_cfg->public_key_len);
+    sso::Verifier verifier(pkey, s_cfg->service,
                            s_cfg->domain, req_groups);
 
     try {

src/mod_sso/test/httpd_integration_test.py

@@ -10,6 +10,76 @@ import urllib
 sys.path.append("../../python")
 import sso
 
+# Allow overriding the apache2 executable location from the environment.
+APACHE_BIN = os.getenv('APACHE_BIN', '/usr/sbin/apache2')
+if not os.path.exists(APACHE_BIN):
+    raise Exception('Apache2 could not be found in %s, this test cannot run' % 
+                    APACHE_BIN)
+
+
+def _start_httpd(public_key):
+    with open('public.key', 'w') as fd:
+        fd.write(public_key)
+    cmd = ["env", "TESTROOT=%s" % os.getcwd(),
+           APACHE_BIN, "-f", os.path.join(os.getcwd(), "test-httpd.conf"), "-X"]
+    httpd = subprocess.Popen(cmd)
+    print 'httpd pid:', httpd.pid
+    time.sleep(1)
+    return httpd
+
+
+def _stop_httpd(httpd):
+    os.kill(httpd.pid, 15)
+    time.sleep(1)
+    try:
+        os.kill(httpd.pid, 9)
+    except OSError:
+        pass
+    try:
+        os.remove("test-httpd.pid")
+    except OSError:
+        pass
+
+
+def _query(url, host=None, cookie=None):
+    conn = httplib.HTTPConnection("127.0.0.1", 33000)
+    headers = {"Host": host or "localhost"}
+    if cookie:
+        headers["Cookie"] = cookie
+    conn.request("GET", url, headers=headers)
+    resp = conn.getresponse()
+    location = None
+    body = None
+    if resp.status in (301, 302):
+        location = resp.getheader("Location")
+    elif resp.status == 200:
+        body = resp.read()
+    conn.close()
+    return (resp.status, body, location)
+
+
+class HttpdKeyLoadingTest(unittest.TestCase):
+
+    def testKeyWithNullByte(self):
+
+        has_null_byte = lambda s: s.find('\0') >= 0
+
+        public = ''
+        while not has_null_byte(public):
+            public, secret = sso.generate_keys()
+        print "public key:", ['%02x' % ord(x) for x in public]
+
+        httpd = _start_httpd(public)
+        try:
+            signer = sso.Signer(secret)
+            t = sso.Ticket("testuser", "service.example.com/", "example.com",
+                           set(["group1"]))
+            ts = signer.sign(t)
+            status, body, location = _query("/index.html", 
+                                            cookie="SSO_test=%s" % ts)
+            self.assertEquals(200, status)
+        finally:
+            _stop_httpd(httpd)
 
 
 class HttpdIntegrationTest(unittest.TestCase):
@@ -17,25 +87,10 @@ class HttpdIntegrationTest(unittest.TestCase):
     def setUp(self):
         self.public, self.secret = sso.generate_keys()
         self.signer = sso.Signer(self.secret)
-        with open('public.key', 'w') as fd:
-            fd.write(self.public)
-        cmd = ["env", "TESTROOT=%s" % os.getcwd(),
-               "/usr/sbin/apache2", "-f", os.path.join(os.getcwd(), "test-httpd.conf"), "-X"]
-        self.httpd = subprocess.Popen(cmd)
-        time.sleep(1)
-        print 'httpd pid:', self.httpd.pid
+        self.httpd = _start_httpd(self.public)
 
     def tearDown(self):
-        os.kill(self.httpd.pid, 15)
-        time.sleep(1)
-        try:
-            os.kill(self.httpd.pid, 9)
-        except OSError:
-            pass
-        try:
-	    os.remove("test-httpd.pid")
-        except OSError:
-            pass
+        _stop_httpd(self.httpd)
 
     def _ticket(self, user="testuser", group="group1"):
         t = sso.Ticket(user, "service.example.com/", "example.com", set([group]))
@@ -43,21 +98,15 @@ class HttpdIntegrationTest(unittest.TestCase):
         print 'ticket:', signedt
         return signedt
 
-    def _query(self, url, host=None, cookie=None):
-        conn = httplib.HTTPConnection("127.0.0.1", 33000)
-        headers = {"Host": host or "localhost"}
-        if cookie:
-            headers["Cookie"] = cookie
-        conn.request("GET", url, headers=headers)
-        resp = conn.getresponse()
-        location = None
-        body = None
-        if resp.status in (301, 302):
-            location = resp.getheader("Location")
-        elif resp.status == 200:
-            body = resp.read()
-        conn.close()
-        return (resp.status, body, location)
+    def testManyRequests(self):
+        n = 100
+        errors = 0
+        for i in xrange(n):
+            cookie = 'SSO_test=%s' % self._ticket()
+            status, body, location = _query("/index.html", cookie=cookie)
+            if status != 200:
+                errors += 1
+        self.assertEquals(0, errors)
 
     def testRedirectionUrls(self):
 
@@ -117,9 +166,9 @@ class HttpdIntegrationTest(unittest.TestCase):
               "location": "https://login.example.com/?s=testhost.example.com%2Fother-service%2F&d=https%3A%2F%2Ftesthost.example.com%2Fother-service%2Findex.html"}),
             ]
         for name, check in checks:
-            status, body, location = self._query(check["url"],
-                                                 host=check.get("http_host"),
-                                                 cookie=check.get("cookie"))
+            status, body, location = _query(check["url"],
+                                            host=check.get("http_host"),
+                                            cookie=check.get("cookie"))
             self.assertEquals(
                 check["status"], status,
                 "test: '%s'\nunexpected HTTP status for %s (got %d, exp %d)" % (
@@ -136,8 +185,8 @@ class HttpdIntegrationTest(unittest.TestCase):
                         name, check["url"], location, check["location"]))
 
         # test that environment variables are correctly set
-        status, body, location = self._query("/cgi/env.cgi",
-                                             cookie=mkcookie(self._ticket()))
+        status, body, location = _query("/cgi/env.cgi",
+                                        cookie=mkcookie(self._ticket()))
         self.assertEquals(200, status)
         self.assertTrue(body)
         self.assertTrue("REMOTE_USER=testuser" in body)