[PATCH 0/5] CERN - Proposed distcc Modifications.

[Ian....@cern.ch]

Here are the latest set of patches for the CERN modifications. We are
currently running a pilot service and I wanted to see if CERN user
feedback necessitated any changes, hence the delay.

I have implemented all the changes that resulted from distcc list feedback,
but there are two new modifications that will need to be reviewed.

Firstly, GSS-API authentication is no longer specified via an environment
variable, but is now a per host option. As a result hosts.c and lock.c
have been modified.

Secondly, the resolution for the log file timestamps has been increased to
include microseconds.

With regard to the python tests, instead of having a separate class to test
for the presence of the GSS-API and an underlying security mechanism, I have
overridden the startDaemon method to handle GSS-API errors for those tests
that need them.

To answer an outstanding question:

On Thu, 5 Mar 2009, Fergus Henderson wrote:
>
> BTW, what happens if you specify --show-principle without --auth? Does that
> work?
>

--show-principal will show the current GSS-API principal and exit, and
therefore should be used on its own.

Thanks

Ian

[Ian....@cern.ch]

[Ian....@cern.ch]

[Fergus Henderson]

That all sounds good.  I await the patches...

--
Fergus Henderson <fer...@google.com>

[Fergus Henderson]

On Thu, May 7, 2009 at 7:50 AM, <Ian....@cern.ch> wrote:

This patch includes integration tests for CERN based modifications.
These include testing for the correct behaviour of both client and
server for all command line options, mutual authentication, and
blacklist/whitelist functionality.

Tests will only be performed if distcc/distccd where built with
GSS-API support and there is an underlying security mechanism.

Index: quilt-distcc-3.1/distcc-3.1/test/testdistcc.py
===================================================================
--- quilt-distcc-3.1.orig/distcc-3.1/test/testdistcc.py
+++ quilt-distcc-3.1/distcc-3.1/test/testdistcc.py

...

+class AuthEnabledDaemon_Case(SimpleDistCC_Case):
+    """Test handling of --auth daemon option.
+    The daemon may fail to run if valid credentials cannot
+    be obtained, however, the daemon should still attempt to
+    acquire credentials which should show up in the log."""
+
+    def runtest(self):
+        result, out, err = self.runcmd_unchecked("distccd --version")
+        if not re.search("Built with GSS-API support for mutual authentication.", out):
+            raise comfychair.NotRunError("distcc/distccd not built with GSS-API support.")
+
+        self.runcmd_unchecked(self.distccd() +
+                "--auth --verbose --lifetime=10 --daemon "

Why is lifetime hardcoded to "10" here, rather than using self._daemon_lifetime as is done elsewhere?
 

+class MutualAuthentication_Case(CompileHello_Case):
+    """Test mutual authentication between client and daemon.
+    This will only complete successfully if both the client and
+    daemon can obtain credentials."""
+
+    hostname = ""
+
+    def setup(self):
+        CompileHello_Case.setup(self)

Since this class inherits from CompileHello_Case, and since this method does nothing except call the inherited method, you don't need to define this method in this class.

--
Fergus Henderson <fergus.h...@gmail.com>

[Fergus Henderson]

I'm going to address the two minor issues below myself, and then I'll apply this patch.

--
Fergus Henderson <fer...@google.com>

[Fergus Henderson]

Hi Craig and Ian,

Could you please review the relative diff below,
which shows the changes that I made to Ian Baker's patch?

Cheers,
  Fergus.

Revision: 708
Author: fergus.henderson
Date: Fri Jul 24 13:15:43 2009
Log:
Apply patch from Ian....@cern.ch:

Integration tests for CERN modifications.

This patch includes integration tests for CERN based modifications.
These include testing for the correct behaviour of both client and
server for all command line options, mutual authentication, and
blacklist/whitelist functionality.

Tests will only be performed if distcc/distccd where built with
GSS-API support and there is an underlying security mechanism.

Revised by me (Fergus Henderson) to address some review comments
and to fix failing tests:
 - ensure that the tests don't fail when DISTCC_PRINCIPAL and
   DISTCCD_PRINCIPAL aren't set
 - fix a problem where the Concurrent_Case test was failing
   due to some unwaited-for processes left behind by these
   new tests of authentication
 - the name of the per host option is now ",auth", not ",gssapi"
 - delete unnecessary definitions of setup().


http://code.google.com/p/distcc/source/detail?r=708

Modified:
 /changes/fergus.henderson/t3w4/trunk/Makefile.in
Replaced:
 /changes/fergus.henderson/t3w4/trunk
 /changes/fergus.henderson/t3w4/trunk/test/testdistcc.py

=======================================
--- /changes/fergus.henderson/t3w4/trunk/Makefile.in    Fri Jul 24 09:31:07 2009
+++ /changes/fergus.henderson/t3w4/trunk/Makefile.in    Fri Jul 24 13:15:43 2009
@@ -303,7 +303,7 @@
 h_compile_obj = src/h_compile.o $(common_obj) src/compile.o src/timefile.o \
                src/backoff.o src/emaillog.o src/remote.o src/clinet.o \
               src/clirpc.o src/include_server_if.o src/state.o src/where.o \
-               src/ssh.o src/strip.o src/cpp.o
+               src/ssh.o src/strip.o src/cpp.o @AUTH_DISTCC_OBJS@
 h_getline_obj = src/h_getline.o $(common_obj)

 # All source files, for the purposes of building the distribution
=======================================
--- /changes/fergus.henderson/t3w4/trunk/test/testdistcc.py     Fri Jul 24 10:39:01 2009
+++ /changes/fergus.henderson/t3w4/trunk/test/testdistcc.py     Fri Jul 24 13:15:43 2009
@@ -1672,10 +1672,18 @@
 class Concurrent_Case(CompileHello_Case):
    """Try many compilations at the same time"""
    def daemon_lifetime(self):
+        # may take about a minute or so
        return 120

    def runtest(self):
-        # may take about a minute or so
+        # First, make sure there are no unwaited-for child processes
+        # hanging around from previous tests.
+        while True:
+          try:
+            os.wait()    # Will eventually fail with ECHILD.
+          except OSError, e:
+            break
+        # Now run this test...
        pids = {}
        for unused_i in xrange(50):
            kid = self.runcmd_background(self.distcc() +
@@ -2147,9 +2155,13 @@

        if not re.search("Built with GSS-API support for mutual authentication.", out):

            raise comfychair.NotRunError("distcc/distccd not built with GSS-API support.")

+        os.environ['DISTCC_PRINCIPAL'] = 'foo'
        out, err = self.runcmd("distcc --show-principal")
-        self.assert_re_search('Principal', out)
-
+        self.assert_re_search('Principal is\t: foo', out)
+
+        del os.environ['DISTCC_PRINCIPAL']
+        out, err = self.runcmd("distcc --show-principal")
+        self.assert_re_search('Principal\t: Not Set', out)

 class ShowPrincipalDaemon_Case(SimpleDistCC_Case):
    """Test handling of --show-principal daemon option."""
@@ -2159,9 +2171,13 @@

        if not re.search("Built with GSS-API support for mutual authentication.", out):

            raise comfychair.NotRunError("distcc/distccd not built with GSS-API support.")

+        os.environ['DISTCCD_PRINCIPAL'] = 'foo'
        out, err = self.runcmd("distccd --show-principal")
-        self.assert_re_search('Principal is', out)
-
+        self.assert_re_search('Principal is\t: foo', out)
+
+        del os.environ['DISTCCD_PRINCIPAL']
+        out, err = self.runcmd("distccd --show-principal")
+        self.assert_re_search('Principal\t: Not Set', out)

 class AuthEnabledClient_Case(CompileHello_Case):
    """Test handling of the gssapi per host option.
@@ -2169,12 +2185,9 @@
    authenticate, however, the client should still attempt
    authentication which should show up in the log."""

-    def setup(self):
-        CompileHello_Case.setup(self)
-
    """Override the setupEnv method to use the gssapi per host option."""
    def setupEnv(self):
-        os.environ['DISTCC_HOSTS'] = ('127.0.0.1:%d%s,gssapi' %
+        os.environ['DISTCC_HOSTS'] = ('127.0.0.1:%d%s,auth' %
          (self.server_port, _server_options))
        os.environ['DISTCC_LOG'] = os.path.join(os.getcwd(), 'distcc.log')
        os.environ['DISTCC_VERBOSE'] = '1'
@@ -2211,7 +2224,10 @@
                "--auth --verbose --lifetime=10 --daemon "
                "--log-file=distccd.log --allow 127.0.0.1")
        log = open("distccd.log").read()
-        self.assert_re_search('Acquiring credentials.', log)
+        self.assert_re_search('(Acquiring credentials'
+                              '|\\(dcc_gssapi_acquire_credentials\\) '
+                                  'ERROR: No principal name specified'
+                              ')', log)


 class UseBlacklist_Case(WithDaemon_Case):
@@ -2285,9 +2301,6 @@

    hostname = ""

-    def setup(self):
-        CompileHello_Case.setup(self)
-
    def getHostName(self):
        if not self.hostname:
            stdin, stdout, stderr = os.popen3("hostname -f")
@@ -2302,7 +2315,7 @@
    variable and to add the hostname to DISTCC_HOSTS to allow for
    proper authentication."""
    def setupEnv(self):
-        os.environ['DISTCC_HOSTS'] = ('%s:%d%s,gssapi' %
+        os.environ['DISTCC_HOSTS'] = ('%s:%d%s,auth' %
          (self.getHostName(), self.server_port, _server_options))
        os.environ['DISTCC_LOG'] = os.path.join(os.getcwd(), 'distcc.log')
        os.environ['DISTCC_VERBOSE'] = '1'

On Fri, Jul 24, 2009 at 1:51 PM, Fergus Henderson <fer...@google.com> wrote:

I'm going to address the two minor issues below myself, and then I'll apply this patch.

On Sun, Jun 28, 2009 at 1:44 PM, Fergus Henderson <fergus.h...@gmail.com> wrote:

On Thu, May 7, 2009 at 7:50 AM, <Ian....@cern.ch> wrote:

This patch includes integration tests for CERN based modifications.
These include testing for the correct behaviour of both client and
server for all command line options, mutual authentication, and
blacklist/whitelist functionality.

Tests will only be performed if distcc/distccd where built with
GSS-API support and there is an underlying security mechanism.

--
Fergus Henderson <fer...@google.com>

[Fergus Henderson]

On Sun, Jun 28, 2009 at 1:44 PM, Fergus Henderson <fergus.h...@gmail.com> wrote:

To answer my own question: because (a) this class doesn't inherit from WithDaemon_Case, so it doesn't have the daemon_lifetime member,
and (b) the shorter value (10 rather than 60) is fine here because this is not a long-running test; we just need the daemon to start attempting to acquire credentials.

--
Fergus Henderson <fer...@google.com>

[Craig Silverstein]

} Could you please review the relative diff below, which shows the
} changes that I made to Ian Baker's patch?

It's plausible to me, but best if Ian chimes in as well.

craig