More secure chown fallback with run-as-real user jobs.

Previously it would fallback to making the working directory world writable. @dctrud pointed out this is not ideal from a security perspective and should not be the default - http://dev.list.galaxyproject.org/DRMAA-chmod-to-world-writable-if-to-user-chown-fails-should-not-be-default-td4666869.html - I agree. The former behavior can be restored by enabling the new external_chown_insecure_fallback option - but this commit does disable that option by default and this may be a breaking change under certain circumstances. I'll document this option after #94 is merged.
galaxyproject · Apr 8, 2015 · 87e19aa · 87e19aa
1 parent 8683145
commit 87e19aa
Show file tree

Hide file tree

Showing 2 changed files with 7 additions and 3 deletions.
diff --git a/lib/galaxy/config.py b/lib/galaxy/config.py
@@ -247,6 +247,7 @@ def __init__( self, **kwargs ):
         self.drmaa_external_runjob_script = kwargs.get('drmaa_external_runjob_script', None )
         self.drmaa_external_killjob_script = kwargs.get('drmaa_external_killjob_script', None)
         self.external_chown_script = kwargs.get('external_chown_script', None)
+        self.external_chown_insecure_fallback = string_as_bool( kwargs.get('external_chown_insecure_fallback', False) )
         self.environment_setup_file = kwargs.get( 'environment_setup_file', None )
         self.use_heartbeat = string_as_bool( kwargs.get( 'use_heartbeat', 'False' ) )
         self.log_actions = string_as_bool( kwargs.get( 'log_actions', 'False' ) )

diff --git a/lib/galaxy/jobs/__init__.py b/lib/galaxy/jobs/__init__.py
@@ -1589,9 +1589,12 @@ def change_ownership_for_run( self ):
         if self.app.config.external_chown_script and job.user is not None:
             try:
                 self._change_ownership( self.user_system_pwent[0], str( self.user_system_pwent[3] ) )
-            except:
-                log.exception( '(%s) Failed to change ownership of %s, making world-writable instead' % ( job.id, self.working_directory ) )
-                os.chmod( self.working_directory, 0777 )
+            except Exception:
+                if self.app.config.external_chown_insecure_fallback:
+                    log.exception( '(%s) Failed to change ownership of %s, making world-writable instead' % ( job.id, self.working_directory ) )
+                    os.chmod( self.working_directory, 0777 )
+                else:
+                    log.exception( '(%s) Failed to change ownership of %s, job will likely fail' % ( job.id, self.working_directory ) )
 
     def reclaim_ownership( self ):
         job = self.get_job()