Differential D536

remote runner: better escape commands, don't consume errors
ClosedPublic
Actions

Authored by kparal on Aug 26 2015, 11:56 AM.

Details

Reviewers
lbrabec
jskladan
Commits
rLTRN2f56c2fb2bc6: remote runner: escape commands, don't consume errors
Summary

If cd fails, the whole command should fail. Better escape taskdir
path.

properly escape arguments when using paramiko

Tested with

runtask -i 'aa;touch /tmp/hacked;echo ' -t koji_build ../task-rpmlint/rpmlint.yml --libvirt --debug
Test Plan

manually ran a test, works

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kparal retitled this revision from to remote runner: better escape commands, don't consume errors.Aug 26 2015, 11:56 AM
kparal updated this object.
kparal edited the test plan for this revision. (Show Details)
kparal added reviewers: lbrabec, jskladan.
jskladan added a comment.Aug 27 2015, 7:48 AM

taskdir is not getting escaped, if you really, really wanted to escape it, use subprocess.list2cmdline instead of ' ' .join().

But at least half of your patch does what you wanted, so...

Macro freddie: don't stop me now from acking the patch

jskladan accepted this revision.Aug 27 2015, 7:48 AM
This revision is now accepted and ready to land.Aug 27 2015, 7:48 AM
lbrabec requested changes to this revision.Aug 27 2015, 7:53 AM

pipes.quote(str) should do the trick

This revision now requires changes to proceed.Aug 27 2015, 7:53 AM
kparal added a comment.Aug 27 2015, 8:02 AM
In D536#10079, @jskladan wrote:

taskdir is not getting escaped, if you really, really wanted to escape it, use subprocess.list2cmdline instead of ' ' .join().

But at least half of your patch does what you wanted, so...

Haha, you're at least half right, subprocess.list2cmdline seems to be used for MS shell escaping ;-)

Thanks, though!

kparal updated this revision to Diff 1400.Aug 27 2015, 8:30 AM
kparal updated this object.
  • properly escape arguments when using paramiko

    Tested with ` runtask -i 'aa;touch /tmp/hacked;echo ' -t koji_build ../task-rpmlint/rpmlint.yml --libvirt --debug `
lbrabec accepted this revision.Aug 27 2015, 8:32 AM

LGTM

This revision is now accepted and ready to land.Aug 27 2015, 8:32 AM
jskladan added a comment.Aug 27 2015, 8:42 AM
In D536#10087, @kparal wrote:

Haha, you're at least half right, subprocess.list2cmdline seems to be used for MS shell escaping ;-)

You kids, and your cheapo-UNIX-clones...

Closed by commit rLTRN2f56c2fb2bc6: remote runner: escape commands, don't consume errors (authored by kparal). · Explain WhyAug 27 2015, 8:44 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mlibtaskotron/runner.py (13 lines)

Diff 1401

View Options

libtaskotron/runner.py

Show First 20 Lines • Show All 368 Lines • ▼ Show 20 Line(s)343 def prepare_task(self):
369 369​
370 # need to have default_flow_style false to get valid yaml w/ nested dicts 370​ # need to have default_flow_style false to get valid yaml w/ nested dicts
371 self.ssh.write_file(os.path.join(self.taskdir, 371​ self.ssh.write_file(os.path.join(self.taskdir,
372 os.path.basename(self.arg_data['taskfile'])), 372​ os.path.basename(self.arg_data['taskfile'])),
373 yaml.dump(self.task_data, default_flow_style=False)) 373​ yaml.dump(self.task_data, default_flow_style=False))
374 374​
375 def run(self): 375​ def run(self):
376 # execute task 376​ # execute task
377 task_cmd = ['cd %s;' % self.taskdir, 377​ task_cmd = ['cd', pipes.quote(self.taskdir), '&&',
378 'runtask', 378​ 'runtask',
379 '-t', 379​ '-t', pipes.quote(self.arg_data['type']),
380 self.arg_data['type'], 380​ '-i', pipes.quote(self.arg_data['item']),
381 '-i', 381​ '--uuid', pipes.quote(self.arg_data['uuid']),
382 self.arg_data['item'], # XXX what if item is "; rm -rf /;"? ​
383 '--uuid', ​
384 self.arg_data['uuid'], ​
385 '--local', 382​ '--local',
386 os.path.basename(self.arg_data['taskfile'])] 383​ pipes.quote(os.path.basename(self.arg_data['taskfile']))]
387 384​
388 self.exitcode = self.ssh.cmd(' '.join(task_cmd)) 385​ self.exitcode = self.ssh.cmd(' '.join(task_cmd))
389 386​
390 def get_output(self): 387​ def get_output(self):
391 '''copy artifacts from vm''' 388​ '''copy artifacts from vm'''
392 try: 389​ try:
393 self.ssh.get_dir(self.arg_data['artifactsdir'], self.arg_data['artifactsdir']) 390​ self.ssh.get_dir(self.arg_data['artifactsdir'], self.arg_data['artifactsdir'])
394 except exc.TaskotronRemoteError, e: 391​ except exc.TaskotronRemoteError, e:
▲ Show 20 Lines • Show All 146 Lines • Show Last 20 Lines