Merge lp:~thedac/charm-helpers/apparmor into lp:charm-helpers
- apparmor
- Merge into devel
Proposed by
David Ames
Status: | Merged |
---|---|
Merged at revision: | 559 |
Proposed branch: | lp:~thedac/charm-helpers/apparmor |
Merge into: | lp:charm-helpers |
Diff against target: |
214 lines (+177/-1) 2 files modified
charmhelpers/contrib/openstack/context.py (+91/-1) tests/contrib/openstack/test_os_contexts.py (+86/-0) |
To merge this branch: | bzr merge lp:~thedac/charm-helpers/apparmor |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
James Page | Approve | ||
Review via email:
|
Commit message
Description of the change
Apparmor class for OpenStack charms
The class will validate aa-profile-mode config settings and either set
the profile to enforce, complain or disable mode.
To post a comment you must log in.
- 556. By David Ames
-
Fix call to manually_
disable_ aa_profile( ) - 557. By David Ames
-
Move mitaka check into base AA class
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alex Kavanagh (ajkavanagh) wrote : | # |
- 558. By David Ames
-
Set self.ctxt as a property
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Alex Kavanagh (ajkavanagh) wrote : | # |
Just a tiny inline comment.
- 559. By David Ames
-
Use self() rather than self.__call__()
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
James Page (james-page) : | # |
review:
Needs Fixing
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
David Ames (thedac) wrote : | # |
I'll address the comments.
- 560. By David Ames
-
Remove python3 setting, add comments for manual disable
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
James Page (james-page) wrote : | # |
LGTM but does need a rebase prior to merge - please land away!
review:
Approve
- 561. By David Ames
-
Merge upstream
Revision history for this message
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
David Ames (thedac) wrote : | # |
Rebased. Merging.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'charmhelpers/contrib/openstack/context.py' | |||
2 | --- charmhelpers/contrib/openstack/context.py 2016-04-01 10:00:31 +0000 | |||
3 | +++ charmhelpers/contrib/openstack/context.py 2016-04-04 18:08:06 +0000 | |||
4 | @@ -20,7 +20,7 @@ | |||
5 | 20 | import re | 20 | import re |
6 | 21 | import time | 21 | import time |
7 | 22 | from base64 import b64decode | 22 | from base64 import b64decode |
9 | 23 | from subprocess import check_call | 23 | from subprocess import check_call, CalledProcessError |
10 | 24 | 24 | ||
11 | 25 | import six | 25 | import six |
12 | 26 | import yaml | 26 | import yaml |
13 | @@ -45,6 +45,7 @@ | |||
14 | 45 | INFO, | 45 | INFO, |
15 | 46 | WARNING, | 46 | WARNING, |
16 | 47 | ERROR, | 47 | ERROR, |
17 | 48 | status_set, | ||
18 | 48 | ) | 49 | ) |
19 | 49 | 50 | ||
20 | 50 | from charmhelpers.core.sysctl import create as sysctl_create | 51 | from charmhelpers.core.sysctl import create as sysctl_create |
21 | @@ -1491,3 +1492,92 @@ | |||
22 | 1491 | """ | 1492 | """ |
23 | 1492 | def __call__(self): | 1493 | def __call__(self): |
24 | 1493 | return {'use_internal_endpoints': config('use-internal-endpoints')} | 1494 | return {'use_internal_endpoints': config('use-internal-endpoints')} |
25 | 1495 | |||
26 | 1496 | |||
27 | 1497 | class AppArmorContext(OSContextGenerator): | ||
28 | 1498 | """Base class for apparmor contexts.""" | ||
29 | 1499 | |||
30 | 1500 | def __init__(self): | ||
31 | 1501 | self._ctxt = None | ||
32 | 1502 | self.aa_profile = None | ||
33 | 1503 | self.aa_utils_packages = ['apparmor-utils'] | ||
34 | 1504 | |||
35 | 1505 | @property | ||
36 | 1506 | def ctxt(self): | ||
37 | 1507 | if self._ctxt is not None: | ||
38 | 1508 | return self._ctxt | ||
39 | 1509 | self._ctxt = self._determine_ctxt() | ||
40 | 1510 | return self._ctxt | ||
41 | 1511 | |||
42 | 1512 | def _determine_ctxt(self): | ||
43 | 1513 | """ | ||
44 | 1514 | Validate aa-profile-mode settings is disable, enforce, or complain. | ||
45 | 1515 | |||
46 | 1516 | :return ctxt: Dictionary of the apparmor profile or None | ||
47 | 1517 | """ | ||
48 | 1518 | if config('aa-profile-mode') in ['disable', 'enforce', 'complain']: | ||
49 | 1519 | ctxt = {'aa-profile-mode': config('aa-profile-mode')} | ||
50 | 1520 | else: | ||
51 | 1521 | ctxt = None | ||
52 | 1522 | return ctxt | ||
53 | 1523 | |||
54 | 1524 | def __call__(self): | ||
55 | 1525 | return self.ctxt | ||
56 | 1526 | |||
57 | 1527 | def install_aa_utils(self): | ||
58 | 1528 | """ | ||
59 | 1529 | Install packages required for apparmor configuration. | ||
60 | 1530 | """ | ||
61 | 1531 | log("Installing apparmor utils.") | ||
62 | 1532 | ensure_packages(self.aa_utils_packages) | ||
63 | 1533 | |||
64 | 1534 | def manually_disable_aa_profile(self): | ||
65 | 1535 | """ | ||
66 | 1536 | Manually disable an apparmor profile. | ||
67 | 1537 | |||
68 | 1538 | If aa-profile-mode is set to disabled (default) this is required as the | ||
69 | 1539 | template has been written but apparmor is yet unaware of the profile | ||
70 | 1540 | and aa-disable aa-profile fails. Without this the profile would kick | ||
71 | 1541 | into enforce mode on the next service restart. | ||
72 | 1542 | |||
73 | 1543 | """ | ||
74 | 1544 | profile_path = '/etc/apparmor.d' | ||
75 | 1545 | disable_path = '/etc/apparmor.d/disable' | ||
76 | 1546 | if not os.path.lexists(os.path.join(disable_path, self.aa_profile)): | ||
77 | 1547 | os.symlink(os.path.join(profile_path, self.aa_profile), | ||
78 | 1548 | os.path.join(disable_path, self.aa_profile)) | ||
79 | 1549 | |||
80 | 1550 | def setup_aa_profile(self): | ||
81 | 1551 | """ | ||
82 | 1552 | Setup an apparmor profile. | ||
83 | 1553 | The ctxt dictionary will contain the apparmor profile mode and | ||
84 | 1554 | the apparmor profile name. | ||
85 | 1555 | Makes calls out to aa-disable, aa-complain, or aa-enforce to setup | ||
86 | 1556 | the apparmor profile. | ||
87 | 1557 | """ | ||
88 | 1558 | self() | ||
89 | 1559 | if not self.ctxt: | ||
90 | 1560 | log("Not enabling apparmor Profile") | ||
91 | 1561 | return | ||
92 | 1562 | self.install_aa_utils() | ||
93 | 1563 | cmd = ['aa-{}'.format(self.ctxt['aa-profile-mode'])] | ||
94 | 1564 | cmd.append(self.ctxt['aa-profile']) | ||
95 | 1565 | log("Setting up the apparmor profile for {} in {} mode." | ||
96 | 1566 | "".format(self.ctxt['aa-profile'], self.ctxt['aa-profile-mode'])) | ||
97 | 1567 | try: | ||
98 | 1568 | check_call(cmd) | ||
99 | 1569 | except CalledProcessError as e: | ||
100 | 1570 | # If aa-profile-mode is set to disabled (default) manual | ||
101 | 1571 | # disabling is required as the template has been written but | ||
102 | 1572 | # apparmor is yet unaware of the profile and aa-disable aa-profile | ||
103 | 1573 | # fails. If aa-disable learns to read profile files first this can | ||
104 | 1574 | # be removed. | ||
105 | 1575 | if self.ctxt['aa-profile-mode'] == 'disable': | ||
106 | 1576 | log("Manually disabling the apparmor profile for {}." | ||
107 | 1577 | "".format(self.ctxt['aa-profile'])) | ||
108 | 1578 | self.manually_disable_aa_profile() | ||
109 | 1579 | return | ||
110 | 1580 | status_set('blocked', "Apparmor profile {} failed to be set to {}." | ||
111 | 1581 | "".format(self.ctxt['aa-profile'], | ||
112 | 1582 | self.ctxt['aa-profile-mode'])) | ||
113 | 1583 | raise e | ||
114 | 1494 | 1584 | ||
115 | === modified file 'tests/contrib/openstack/test_os_contexts.py' | |||
116 | --- tests/contrib/openstack/test_os_contexts.py 2016-04-01 10:15:57 +0000 | |||
117 | +++ tests/contrib/openstack/test_os_contexts.py 2016-04-04 18:08:06 +0000 | |||
118 | @@ -84,6 +84,21 @@ | |||
119 | 84 | return None | 84 | return None |
120 | 85 | return sorted(self.relation_data[relation_id].keys()) | 85 | return sorted(self.relation_data[relation_id].keys()) |
121 | 86 | 86 | ||
122 | 87 | |||
123 | 88 | class FakeAppArmorContext(context.AppArmorContext): | ||
124 | 89 | |||
125 | 90 | def __init__(self): | ||
126 | 91 | super(FakeAppArmorContext, self).__init__() | ||
127 | 92 | self.aa_profile = 'fake-aa-profile' | ||
128 | 93 | |||
129 | 94 | def __call__(self): | ||
130 | 95 | super(FakeAppArmorContext, self).__call__() | ||
131 | 96 | if not self.ctxt: | ||
132 | 97 | return self.ctxt | ||
133 | 98 | self._ctxt.update({'aa-profile': self.aa_profile}) | ||
134 | 99 | return self.ctxt | ||
135 | 100 | |||
136 | 101 | |||
137 | 87 | SHARED_DB_RELATION = { | 102 | SHARED_DB_RELATION = { |
138 | 88 | 'db_host': 'dbserver.local', | 103 | 'db_host': 'dbserver.local', |
139 | 89 | 'password': 'foo' | 104 | 'password': 'foo' |
140 | @@ -2892,3 +2907,74 @@ | |||
141 | 2892 | config = {'use-internal-endpoints': True} | 2907 | config = {'use-internal-endpoints': True} |
142 | 2893 | self.config.side_effect = fake_config(config) | 2908 | self.config.side_effect = fake_config(config) |
143 | 2894 | self.assertTrue(ctxt()['use_internal_endpoints']) | 2909 | self.assertTrue(ctxt()['use_internal_endpoints']) |
144 | 2910 | |||
145 | 2911 | def test_apparmor_context_call_not_valid(self): | ||
146 | 2912 | ''' Tests for the apparmor context''' | ||
147 | 2913 | mock_aa_object = context.AppArmorContext() | ||
148 | 2914 | # Test with invalid config | ||
149 | 2915 | self.config.return_value = 'NOTVALID' | ||
150 | 2916 | self.assertEquals(mock_aa_object.__call__(), None) | ||
151 | 2917 | |||
152 | 2918 | def test_apparmor_context_call_complain(self): | ||
153 | 2919 | ''' Tests for the apparmor context''' | ||
154 | 2920 | mock_aa_object = context.AppArmorContext() | ||
155 | 2921 | # Test complain mode | ||
156 | 2922 | self.config.return_value = 'complain' | ||
157 | 2923 | self.assertEquals(mock_aa_object.__call__(), | ||
158 | 2924 | {'aa-profile-mode': 'complain'}) | ||
159 | 2925 | |||
160 | 2926 | def test_apparmor_context_call_enforce(self): | ||
161 | 2927 | ''' Tests for the apparmor context''' | ||
162 | 2928 | mock_aa_object = context.AppArmorContext() | ||
163 | 2929 | # Test enforce mode | ||
164 | 2930 | self.config.return_value = 'enforce' | ||
165 | 2931 | self.assertEquals(mock_aa_object.__call__(), | ||
166 | 2932 | {'aa-profile-mode': 'enforce'}) | ||
167 | 2933 | |||
168 | 2934 | def test_apparmor_context_call_disable(self): | ||
169 | 2935 | ''' Tests for the apparmor context''' | ||
170 | 2936 | mock_aa_object = context.AppArmorContext() | ||
171 | 2937 | # Test complain mode | ||
172 | 2938 | self.config.return_value = 'disable' | ||
173 | 2939 | self.assertEquals(mock_aa_object.__call__(), | ||
174 | 2940 | {'aa-profile-mode': 'disable'}) | ||
175 | 2941 | |||
176 | 2942 | def test_apparmor_setup_complain(self): | ||
177 | 2943 | ''' Tests for the apparmor setup''' | ||
178 | 2944 | AA = FakeAppArmorContext() | ||
179 | 2945 | AA.install_aa_utils = MagicMock() | ||
180 | 2946 | AA.manually_disable_aa_profile = MagicMock() | ||
181 | 2947 | # Test complain mode | ||
182 | 2948 | self.config.return_value = 'complain' | ||
183 | 2949 | AA.setup_aa_profile() | ||
184 | 2950 | AA.install_aa_utils.assert_called_with() | ||
185 | 2951 | self.check_call.assert_called_with(['aa-complain', 'fake-aa-profile']) | ||
186 | 2952 | self.assertFalse(AA.manually_disable_aa_profile.called) | ||
187 | 2953 | |||
188 | 2954 | def test_apparmor_setup_enforce(self): | ||
189 | 2955 | ''' Tests for the apparmor setup''' | ||
190 | 2956 | AA = FakeAppArmorContext() | ||
191 | 2957 | AA.install_aa_utils = MagicMock() | ||
192 | 2958 | AA.manually_disable_aa_profile = MagicMock() | ||
193 | 2959 | # Test enforce mode | ||
194 | 2960 | self.config.return_value = 'enforce' | ||
195 | 2961 | AA.setup_aa_profile() | ||
196 | 2962 | self.check_call.assert_called_with(['aa-enforce', 'fake-aa-profile']) | ||
197 | 2963 | self.assertFalse(AA.manually_disable_aa_profile.called) | ||
198 | 2964 | |||
199 | 2965 | def test_apparmor_setup_disable(self): | ||
200 | 2966 | ''' Tests for the apparmor setup''' | ||
201 | 2967 | AA = FakeAppArmorContext() | ||
202 | 2968 | AA.install_aa_utils = MagicMock() | ||
203 | 2969 | AA.manually_disable_aa_profile = MagicMock() | ||
204 | 2970 | # Test disable mode | ||
205 | 2971 | self.config.return_value = 'disable' | ||
206 | 2972 | AA.setup_aa_profile() | ||
207 | 2973 | self.check_call.assert_called_with(['aa-disable', 'fake-aa-profile']) | ||
208 | 2974 | self.assertFalse(AA.manually_disable_aa_profile.called) | ||
209 | 2975 | # Test failed to disable | ||
210 | 2976 | from subprocess import CalledProcessError | ||
211 | 2977 | self.check_call.side_effect = CalledProcessError(0, 0, 0) | ||
212 | 2978 | AA.setup_aa_profile() | ||
213 | 2979 | self.check_call.assert_called_with(['aa-disable', 'fake-aa-profile']) | ||
214 | 2980 | AA.manually_disable_aa_profile.assert_called_with() |
See my inline comments. They don't alter functionality, but change an appArmorContext .ctxt to a property that is set only once regardless of whether returned in the __call__() or directly by instance.ctxt. However, it's a style thing only.
Otherwise, good to go.