Merge ~cjwatson/launchpad:py3-restricted-codehosting-session into launchpad:master
- Git
- lp:~cjwatson/launchpad
- py3-restricted-codehosting-session
- Merge into master
Proposed by
Colin Watson
Status: | Merged |
---|---|
Approved by: | Colin Watson |
Approved revision: | 6eea481b643548f77da39cf87be4d0e39b7d9c3a |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~cjwatson/launchpad:py3-restricted-codehosting-session |
Merge into: | launchpad:master |
Diff against target: |
189 lines (+30/-28) 2 files modified
lib/lp/codehosting/sshserver/session.py (+4/-3) lib/lp/codehosting/sshserver/tests/test_session.py (+26/-25) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Cristian Gonzalez (community) | Approve | ||
Review via email: mp+397325@code.launchpad.net |
Commit message
Fix restricted codehosting session for Python 3
Description of the change
The command to execute is passed to us as bytes, not text.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lp/codehosting/sshserver/session.py b/lib/lp/codehosting/sshserver/session.py | |||
2 | index 482dd4f..28a0a69 100644 | |||
3 | --- a/lib/lp/codehosting/sshserver/session.py | |||
4 | +++ b/lib/lp/codehosting/sshserver/session.py | |||
5 | @@ -14,6 +14,7 @@ import os | |||
6 | 14 | 14 | ||
7 | 15 | from lazr.sshserver.events import AvatarEvent | 15 | from lazr.sshserver.events import AvatarEvent |
8 | 16 | from lazr.sshserver.session import DoNothingSession | 16 | from lazr.sshserver.session import DoNothingSession |
9 | 17 | import six | ||
10 | 17 | from six.moves.urllib.parse import urlparse | 18 | from six.moves.urllib.parse import urlparse |
11 | 18 | from twisted.internet import process | 19 | from twisted.internet import process |
12 | 19 | from twisted.python import log | 20 | from twisted.python import log |
13 | @@ -107,7 +108,7 @@ class ExecOnlySession(DoNothingSession): | |||
14 | 107 | executable and arguments is a sequence of command-line arguments | 108 | executable and arguments is a sequence of command-line arguments |
15 | 108 | with the name of the executable as the first value. | 109 | with the name of the executable as the first value. |
16 | 109 | """ | 110 | """ |
18 | 110 | args = command.split() | 111 | args = six.ensure_binary(command).split() |
19 | 111 | return args[0], args | 112 | return args[0], args |
20 | 112 | 113 | ||
21 | 113 | 114 | ||
22 | @@ -161,8 +162,8 @@ def lookup_command_template(command): | |||
23 | 161 | command_template = python_command + args | 162 | command_template = python_command + args |
24 | 162 | 163 | ||
25 | 163 | if command in ( | 164 | if command in ( |
28 | 164 | 'bzr serve --inet --directory=/ --allow-writes', | 165 | b'bzr serve --inet --directory=/ --allow-writes', |
29 | 165 | 'brz serve --inet --directory=/ --allow-writes'): | 166 | b'brz serve --inet --directory=/ --allow-writes'): |
30 | 166 | return command_template | 167 | return command_template |
31 | 167 | # At the moment, only bzr/brz branch serving is allowed. | 168 | # At the moment, only bzr/brz branch serving is allowed. |
32 | 168 | raise ForbiddenCommand("Not allowed to execute %r." % (command,)) | 169 | raise ForbiddenCommand("Not allowed to execute %r." % (command,)) |
33 | diff --git a/lib/lp/codehosting/sshserver/tests/test_session.py b/lib/lp/codehosting/sshserver/tests/test_session.py | |||
34 | index fb07cea..305c65b 100644 | |||
35 | --- a/lib/lp/codehosting/sshserver/tests/test_session.py | |||
36 | +++ b/lib/lp/codehosting/sshserver/tests/test_session.py | |||
37 | @@ -77,9 +77,9 @@ class MockProcessTransport: | |||
38 | 77 | self.log.append(('childConnectionLost', childFD, reason)) | 77 | self.log.append(('childConnectionLost', childFD, reason)) |
39 | 78 | 78 | ||
40 | 79 | def signalProcess(self, signal): | 79 | def signalProcess(self, signal): |
42 | 80 | if self._executable == 'raise-os-error': | 80 | if self._executable == b'raise-os-error': |
43 | 81 | raise OSError() | 81 | raise OSError() |
45 | 82 | if self._executable == 'already-terminated': | 82 | if self._executable == b'already-terminated': |
46 | 83 | raise ProcessExitedAlready() | 83 | raise ProcessExitedAlready() |
47 | 84 | self.log.append(('signalProcess', signal)) | 84 | self.log.append(('signalProcess', signal)) |
48 | 85 | 85 | ||
49 | @@ -116,7 +116,7 @@ class TestExecOnlySession(AvatarTestCase): | |||
50 | 116 | 116 | ||
51 | 117 | def test_openShellNotImplemented(self): | 117 | def test_openShellNotImplemented(self): |
52 | 118 | # openShell closes the connection. | 118 | # openShell closes the connection. |
54 | 119 | protocol = MockProcessTransport('bash') | 119 | protocol = MockProcessTransport(b'bash') |
55 | 120 | self.session.openShell(protocol) | 120 | self.session.openShell(protocol) |
56 | 121 | self.assertEqual( | 121 | self.assertEqual( |
57 | 122 | [('writeExtended', connection.EXTENDED_DATA_STDERR, | 122 | [('writeExtended', connection.EXTENDED_DATA_STDERR, |
58 | @@ -148,7 +148,7 @@ class TestExecOnlySession(AvatarTestCase): | |||
59 | 148 | # inside, it tells the process transport to end the connection between | 148 | # inside, it tells the process transport to end the connection between |
60 | 149 | # the SSH server and the child process. | 149 | # the SSH server and the child process. |
61 | 150 | protocol = ProcessProtocol() | 150 | protocol = ProcessProtocol() |
63 | 151 | self.session.execCommand(protocol, 'cat /etc/hostname') | 151 | self.session.execCommand(protocol, b'cat /etc/hostname') |
64 | 152 | self.session.closed() | 152 | self.session.closed() |
65 | 153 | self.assertEqual( | 153 | self.assertEqual( |
66 | 154 | [('signalProcess', 'HUP'), ('loseConnection',)], | 154 | [('signalProcess', 'HUP'), ('loseConnection',)], |
67 | @@ -160,7 +160,7 @@ class TestExecOnlySession(AvatarTestCase): | |||
68 | 160 | protocol = ProcessProtocol() | 160 | protocol = ProcessProtocol() |
69 | 161 | # MockTransport will raise an OSError on signalProcess if the executed | 161 | # MockTransport will raise an OSError on signalProcess if the executed |
70 | 162 | # command is 'raise-os-error'. | 162 | # command is 'raise-os-error'. |
72 | 163 | self.session.execCommand(protocol, 'raise-os-error') | 163 | self.session.execCommand(protocol, b'raise-os-error') |
73 | 164 | self.session.closed() | 164 | self.session.closed() |
74 | 165 | self.assertEqual( | 165 | self.assertEqual( |
75 | 166 | [('loseConnection',)], | 166 | [('loseConnection',)], |
76 | @@ -172,22 +172,22 @@ class TestExecOnlySession(AvatarTestCase): | |||
77 | 172 | protocol = ProcessProtocol() | 172 | protocol = ProcessProtocol() |
78 | 173 | # MockTransport will raise a ProcessExitedAlready on signalProcess if | 173 | # MockTransport will raise a ProcessExitedAlready on signalProcess if |
79 | 174 | # the executed command is 'already-terminated'. | 174 | # the executed command is 'already-terminated'. |
81 | 175 | self.session.execCommand(protocol, 'already-terminated') | 175 | self.session.execCommand(protocol, b'already-terminated') |
82 | 176 | self.session.closed() | 176 | self.session.closed() |
83 | 177 | self.assertEqual([('loseConnection',)], self.session._transport.log) | 177 | self.assertEqual([('loseConnection',)], self.session._transport.log) |
84 | 178 | 178 | ||
85 | 179 | def test_getCommandToRunSplitsCommandLine(self): | 179 | def test_getCommandToRunSplitsCommandLine(self): |
86 | 180 | # getCommandToRun takes a command line and splits it into the name of | 180 | # getCommandToRun takes a command line and splits it into the name of |
87 | 181 | # an executable to run and a sequence of arguments. | 181 | # an executable to run and a sequence of arguments. |
89 | 182 | command = 'cat foo bar' | 182 | command = b'cat foo bar' |
90 | 183 | executable, arguments = self.session.getCommandToRun(command) | 183 | executable, arguments = self.session.getCommandToRun(command) |
93 | 184 | self.assertEqual('cat', executable) | 184 | self.assertEqual(b'cat', executable) |
94 | 185 | self.assertEqual(['cat', 'foo', 'bar'], list(arguments)) | 185 | self.assertEqual([b'cat', b'foo', b'bar'], list(arguments)) |
95 | 186 | 186 | ||
96 | 187 | def test_execCommandSpawnsProcess(self): | 187 | def test_execCommandSpawnsProcess(self): |
97 | 188 | # ExecOnlySession.execCommand spawns the appropriate process. | 188 | # ExecOnlySession.execCommand spawns the appropriate process. |
98 | 189 | protocol = ProcessProtocol() | 189 | protocol = ProcessProtocol() |
100 | 190 | command = 'cat /etc/hostname' | 190 | command = b'cat /etc/hostname' |
101 | 191 | self.session.execCommand(protocol, command) | 191 | self.session.execCommand(protocol, command) |
102 | 192 | executable, arguments = self.session.getCommandToRun(command) | 192 | executable, arguments = self.session.getCommandToRun(command) |
103 | 193 | self.assertEqual([(protocol, executable, arguments, None, None, | 193 | self.assertEqual([(protocol, executable, arguments, None, None, |
104 | @@ -204,7 +204,7 @@ class TestExecOnlySession(AvatarTestCase): | |||
105 | 204 | # 'eofReceived' closes standard input when called while a command is | 204 | # 'eofReceived' closes standard input when called while a command is |
106 | 205 | # running. | 205 | # running. |
107 | 206 | protocol = ProcessProtocol() | 206 | protocol = ProcessProtocol() |
109 | 207 | self.session.execCommand(protocol, 'cat /etc/hostname') | 207 | self.session.execCommand(protocol, b'cat /etc/hostname') |
110 | 208 | self.session.eofReceived() | 208 | self.session.eofReceived() |
111 | 209 | self.assertEqual([('closeStdin',)], self.session._transport.log) | 209 | self.assertEqual([('closeStdin',)], self.session._transport.log) |
112 | 210 | 210 | ||
113 | @@ -227,10 +227,10 @@ class TestExecOnlySession(AvatarTestCase): | |||
114 | 227 | session = ExecOnlySession( | 227 | session = ExecOnlySession( |
115 | 228 | self.avatar, self.reactor, environment={'FOO': 'BAR'}) | 228 | self.avatar, self.reactor, environment={'FOO': 'BAR'}) |
116 | 229 | protocol = ProcessProtocol() | 229 | protocol = ProcessProtocol() |
118 | 230 | session.execCommand(protocol, 'yes') | 230 | session.execCommand(protocol, b'yes') |
119 | 231 | self.assertEqual({'FOO': 'BAR'}, session.environment) | 231 | self.assertEqual({'FOO': 'BAR'}, session.environment) |
120 | 232 | self.assertEqual( | 232 | self.assertEqual( |
122 | 233 | [(protocol, 'yes', ['yes'], {'FOO': 'BAR'}, None, None, None, 0, | 233 | [(protocol, b'yes', [b'yes'], {'FOO': 'BAR'}, None, None, None, 0, |
123 | 234 | None)], | 234 | None)], |
124 | 235 | self.reactor.log) | 235 | self.reactor.log) |
125 | 236 | 236 | ||
126 | @@ -289,12 +289,12 @@ class TestRestrictedExecOnlySession(AvatarTestCase): | |||
127 | 289 | # Note that Conch doesn't have a well-defined way of rejecting | 289 | # Note that Conch doesn't have a well-defined way of rejecting |
128 | 290 | # commands. Disconnecting in execCommand will do. We don't raise | 290 | # commands. Disconnecting in execCommand will do. We don't raise |
129 | 291 | # an exception to avoid logging an OOPS. | 291 | # an exception to avoid logging an OOPS. |
131 | 292 | protocol = MockProcessTransport('cat') | 292 | protocol = MockProcessTransport(b'cat') |
132 | 293 | self.assertEqual( | 293 | self.assertEqual( |
134 | 294 | None, self.session.execCommand(protocol, 'cat')) | 294 | None, self.session.execCommand(protocol, b'cat')) |
135 | 295 | self.assertEqual( | 295 | self.assertEqual( |
136 | 296 | [('writeExtended', connection.EXTENDED_DATA_STDERR, | 296 | [('writeExtended', connection.EXTENDED_DATA_STDERR, |
138 | 297 | "Not allowed to execute 'cat'.\r\n"), | 297 | "Not allowed to execute %r.\r\n" % b'cat'), |
139 | 298 | ('loseConnection',)], | 298 | ('loseConnection',)], |
140 | 299 | protocol.log) | 299 | protocol.log) |
141 | 300 | 300 | ||
142 | @@ -303,9 +303,10 @@ class TestRestrictedExecOnlySession(AvatarTestCase): | |||
143 | 303 | # executable and arguments corresponding to the provided executed | 303 | # executable and arguments corresponding to the provided executed |
144 | 304 | # command template. | 304 | # command template. |
145 | 305 | executable, arguments = self.session.getCommandToRun('foo') | 305 | executable, arguments = self.session.getCommandToRun('foo') |
147 | 306 | self.assertEqual('bar', executable) | 306 | self.assertEqual(b'bar', executable) |
148 | 307 | self.assertEqual( | 307 | self.assertEqual( |
150 | 308 | ['bar', 'baz', str(self.avatar.user_id)], list(arguments)) | 308 | [b'bar', b'baz', str(self.avatar.user_id).encode('UTF-8')], |
151 | 309 | list(arguments)) | ||
152 | 309 | 310 | ||
153 | 310 | def test_getAvatarAdapter(self): | 311 | def test_getAvatarAdapter(self): |
154 | 311 | # getAvatarAdapter is a convenience classmethod so that | 312 | # getAvatarAdapter is a convenience classmethod so that |
155 | @@ -357,15 +358,15 @@ class TestSessionIntegration(AvatarTestCase): | |||
156 | 357 | session.environment['BRZ_EMAIL']) | 358 | session.environment['BRZ_EMAIL']) |
157 | 358 | 359 | ||
158 | 359 | executable, arguments = session.getCommandToRun( | 360 | executable, arguments = session.getCommandToRun( |
161 | 360 | 'bzr serve --inet --directory=/ --allow-writes') | 361 | b'bzr serve --inet --directory=/ --allow-writes') |
162 | 361 | interpreter = '%s/bin/py' % config.root | 362 | interpreter = ('%s/bin/py' % config.root).encode('UTF-8') |
163 | 362 | self.assertEqual(interpreter, executable) | 363 | self.assertEqual(interpreter, executable) |
164 | 363 | self.assertEqual( | 364 | self.assertEqual( |
167 | 364 | [interpreter, get_brz_path(), 'lp-serve', | 365 | [interpreter, get_brz_path().encode('UTF-8'), b'lp-serve', |
168 | 365 | '--inet', str(self.avatar.user_id)], | 366 | b'--inet', str(self.avatar.user_id).encode('UTF-8')], |
169 | 366 | list(arguments)) | 367 | list(arguments)) |
170 | 367 | self.assertRaises( | 368 | self.assertRaises( |
172 | 368 | ForbiddenCommand, session.getCommandToRun, 'rm -rf /') | 369 | ForbiddenCommand, session.getCommandToRun, b'rm -rf /') |
173 | 369 | 370 | ||
174 | 370 | 371 | ||
175 | 371 | class TestLookupCommand(TestCase): | 372 | class TestLookupCommand(TestCase): |
176 | @@ -378,11 +379,11 @@ class TestLookupCommand(TestCase): | |||
177 | 378 | config.root + '/bin/py ' + get_brz_path() + | 379 | config.root + '/bin/py ' + get_brz_path() + |
178 | 379 | ' lp-serve --inet %(user_id)s', | 380 | ' lp-serve --inet %(user_id)s', |
179 | 380 | lookup_command_template( | 381 | lookup_command_template( |
181 | 381 | 'bzr serve --inet --directory=/ --allow-writes')) | 382 | b'bzr serve --inet --directory=/ --allow-writes')) |
182 | 382 | 383 | ||
183 | 383 | def test_brz(self): | 384 | def test_brz(self): |
184 | 384 | self.assertEqual( | 385 | self.assertEqual( |
185 | 385 | config.root + '/bin/py ' + get_brz_path() + | 386 | config.root + '/bin/py ' + get_brz_path() + |
186 | 386 | ' lp-serve --inet %(user_id)s', | 387 | ' lp-serve --inet %(user_id)s', |
187 | 387 | lookup_command_template( | 388 | lookup_command_template( |
189 | 388 | 'brz serve --inet --directory=/ --allow-writes')) | 389 | b'brz serve --inet --directory=/ --allow-writes')) |
Looks good!