From 07627d4d3958c34d317037cdd62b88b3ad750392 Mon Sep 17 00:00:00 2001 From: Michael Still Date: Mon, 1 Apr 2019 20:58:54 +0000 Subject: [PATCH] Hacking N362: Don't abbrev/alias privsep import As noted in [1]: We always import privsep modules like this: import nova.privsep.libvirt Not like this: from nova.privsep import libvirt This is because it makes it obvious at the caller that a priviledged operation is occuring: nova.privsep.libvirt.destroy_root_filesystem() Not just: libvirt.destroy_root_filesystem() This is especially true when the imported module is called "libvirt", which is a very common term in the codebase and super hard to grep for specific uses of. This commit introduces hacking rule N362 to enforce the above. Change-Id: I9b6aefa015acbf28e49a9ff1713a8bb544586579 Co-Authored-By: Eric Fried --- HACKING.rst | 3 +++ nova/hacking/checks.py | 32 +++++++++++++++++++++++++++++++ nova/tests/fixtures.py | 4 ++-- nova/tests/unit/test_hacking.py | 34 +++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 2 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 480b79d1bbb7..16664f02fd88 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -70,6 +70,9 @@ Nova Specific Commandments - [N360] Yield must always be followed by a space when yielding a value. - [N361] Check for usage of deprecated assertRegexpMatches and assertNotRegexpMatches +- [N362] Imports for privsep modules should be specific. Use "import nova.privsep.path", + not "from nova.privsep import path". This ensures callers know that the method they're + calling is using priviledge escalation. Creating Unit Tests ------------------- diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 65f81ac34d7b..9f05ab512dfc 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -102,6 +102,9 @@ redundant_import_alias_re = re.compile(r"import (?:.*\.)?(.+) as \1$") yield_not_followed_by_space = re.compile(r"^\s*yield(?:\(|{|\[|\"|').*$") asse_regexpmatches = re.compile( r"(assertRegexpMatches|assertNotRegexpMatches)\(") +privsep_file_re = re.compile('^nova/privsep[./]') +privsep_import_re = re.compile( + r"^(?:import|from).*\bprivsep\b") class BaseASTChecker(ast.NodeVisitor): @@ -851,6 +854,34 @@ def assert_regexpmatches(logical_line): "of assertRegexpMatches/assertNotRegexpMatches.") +def privsep_imports_not_aliased(logical_line, filename): + """Do not abbreviate or alias privsep module imports. + + When accessing symbols under nova.privsep in code or tests, the full module + path (e.g. nova.privsep.linux_net.delete_bridge(...)) should be used + explicitly rather than importing and using an alias/abbreviation such as: + + from nova.privsep import linux_net + ... + linux_net.delete_bridge(...) + + See Ief177dbcb018da6fbad13bb0ff153fc47292d5b9. + + N362 + """ + if ( + # Give modules under nova.privsep a pass + not privsep_file_re.match(filename) and + # Any style of import of privsep... + privsep_import_re.match(logical_line) and + # ...that isn't 'import nova.privsep[.foo...]' + logical_line.count(' ') > 1): + yield (0, "N362: always import privsep modules so that the use of " + "escalated permissions is obvious to callers. For example, " + "use 'import nova.privsep.path' instead of " + "'from nova.privsep import path'.") + + def factory(register): register(import_no_db_in_virt) register(no_db_session_in_public_api) @@ -895,3 +926,4 @@ def factory(register): register(no_redundant_import_alias) register(yield_followed_by_space) register(assert_regexpmatches) + register(privsep_imports_not_aliased) diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index 297056ae0016..bc69d5dc3876 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -56,7 +56,7 @@ from nova.network.neutronv2 import constants as neutron_constants from nova import objects from nova.objects import base as obj_base from nova.objects import service as service_obj -from nova import privsep +import nova.privsep from nova import quota as nova_quota from nova import rpc from nova import service @@ -2047,7 +2047,7 @@ class PrivsepFixture(fixtures.Fixture): def setUp(self): super(PrivsepFixture, self).setUp() self.useFixture(fixtures.MockPatchObject( - privsep.sys_admin_pctxt, 'client_mode', False)) + nova.privsep.sys_admin_pctxt, 'client_mode', False)) class NoopQuotaDriverFixture(fixtures.Fixture): diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index 4f0b3ea83560..fd4bb676e100 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -860,3 +860,37 @@ class HackingTestCase(test.NoDBTestCase): self.assertNotRegexpMatchesbar("Notmatch", output) """ self._assert_has_no_errors(code, checks.assert_regexpmatches) + + def test_import_alias_privsep(self): + code = """ + from nova import privsep + import nova.privsep as nova_privsep + from nova.privsep import linux_net + import nova.privsep.linux_net as privsep_linux_net + """ + errors = [(x + 1, 0, 'N362') for x in range(4)] + bad_filenames = ('nova/foo/bar.py', + 'nova/foo/privsep.py', + 'nova/privsep_foo/bar.py') + for filename in bad_filenames: + self._assert_has_errors( + code, checks.privsep_imports_not_aliased, + expected_errors=errors, + filename=filename) + good_filenames = ('nova/privsep.py', + 'nova/privsep/__init__.py', + 'nova/privsep/foo.py') + for filename in good_filenames: + self._assert_has_no_errors( + code, checks.privsep_imports_not_aliased, filename=filename) + code = """ + import nova.privsep + import nova.privsep.foo + import nova.privsep.foo.bar + import nova.foo.privsep + import nova.foo.privsep.bar + import nova.tests.unit.whatever + """ + for filename in (good_filenames + bad_filenames): + self._assert_has_no_errors( + code, checks.privsep_imports_not_aliased, filename=filename)