From c6b03778ffa2f7ca92c4a198be5da59a15da5d28 Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Tue, 30 Apr 2024 11:32:12 -0400 Subject: [PATCH] mgr/smb: remove embedded join source and users and groups types Drop the embedded versions of the join source and usrers and groups configs from the cluster resource type. This means that potentially sensitive information is *always* stored in a separate resource from the less-sensitive general cluster config. Future changes can then make security related choices about those specific resources without worrying about having embedded variants inside the cluster. For convenience I added a new "emtpy" users and groups source type that can be used for testing. It defines *no* users and groups (an empty set basically). This wouldn't be useful in a production scenario today as it would deploy an smbd that you couldn't log into. But for unit testing it's extremely handy, and maybe it can be used for integration testing too. This is a large change because lots of unit tests relied on embedded join auths or users-and-groups. I tried to handle all the breaking changes in one go for bisect-ability. Signed-off-by: John Mulligan --- src/pybind/mgr/smb/enums.py | 5 +- src/pybind/mgr/smb/handler.py | 7 +- src/pybind/mgr/smb/resources.py | 42 ++----- src/pybind/mgr/smb/tests/test_enums.py | 2 - src/pybind/mgr/smb/tests/test_handler.py | 119 ++++++------------- src/pybind/mgr/smb/tests/test_resources.py | 127 +-------------------- src/pybind/mgr/smb/tests/test_smb.py | 46 +++----- 7 files changed, 73 insertions(+), 275 deletions(-) diff --git a/src/pybind/mgr/smb/enums.py b/src/pybind/mgr/smb/enums.py index 6e19c882dad..92b8705ebba 100644 --- a/src/pybind/mgr/smb/enums.py +++ b/src/pybind/mgr/smb/enums.py @@ -41,15 +41,12 @@ class AuthMode(_StrEnum): class JoinSourceType(_StrEnum): - PASSWORD = 'password' - HTTP_URI = 'http_uri' RESOURCE = 'resource' class UserGroupSourceType(_StrEnum): - INLINE = 'inline' - HTTP_URI = 'http_uri' RESOURCE = 'resource' + EMPTY = 'empty' class ConfigNS(_StrEnum): diff --git a/src/pybind/mgr/smb/handler.py b/src/pybind/mgr/smb/handler.py index ab97bc1248f..9931fe0a688 100644 --- a/src/pybind/mgr/smb/handler.py +++ b/src/pybind/mgr/smb/handler.py @@ -1099,8 +1099,6 @@ def _save_pending_join_auths( for idx, src in enumerate(checked(cluster.domain_settings).join_sources): if src.source_type == JoinSourceType.RESOURCE: javalues = checked(arefs[src.ref].auth) - elif src.source_type == JoinSourceType.PASSWORD: - javalues = checked(src.auth) else: raise ValueError( f'unsupported join source type: {src.source_type}' @@ -1124,9 +1122,8 @@ def _save_pending_users_and_groups( if ugsv.source_type == UserGroupSourceType.RESOURCE: ugvalues = augs[ugsv.ref].values assert ugvalues - elif ugsv.source_type == UserGroupSourceType.INLINE: - ugvalues = ugsv.values - assert ugvalues + elif ugsv.source_type == UserGroupSourceType.EMPTY: + continue else: raise ValueError( f'unsupported users/groups source type: {ugsv.source_type}' diff --git a/src/pybind/mgr/smb/resources.py b/src/pybind/mgr/smb/resources.py index bd5ca878121..1a40076538e 100644 --- a/src/pybind/mgr/smb/resources.py +++ b/src/pybind/mgr/smb/resources.py @@ -162,18 +162,17 @@ class JoinAuthValues(_RBase): class JoinSource(_RBase): """Represents data that can be used to join a system to Active Directory.""" - source_type: JoinSourceType - auth: Optional[JoinAuthValues] = None - uri: str = '' + source_type: JoinSourceType = JoinSourceType.RESOURCE ref: str = '' def validate(self) -> None: - if self.ref: + if not self.ref: + raise ValueError('reference value must be specified') + else: validation.check_id(self.ref) @resourcelib.customize def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource: - rc.uri.quiet = True rc.ref.quiet = True return rc @@ -190,40 +189,21 @@ class UserGroupSettings(_RBase): class UserGroupSource(_RBase): """Represents data used to set up user/group settings for an instance.""" - source_type: UserGroupSourceType - values: Optional[UserGroupSettings] = None - uri: str = '' + source_type: UserGroupSourceType = UserGroupSourceType.RESOURCE ref: str = '' def validate(self) -> None: - if self.source_type == UserGroupSourceType.INLINE: - pfx = 'inline User/Group configuration' - if self.values is None: - raise ValueError(pfx + ' requires values') - if self.uri: - raise ValueError(pfx + ' does not take a uri') - if self.ref: - raise ValueError(pfx + ' does not take a ref value') - if self.source_type == UserGroupSourceType.HTTP_URI: - pfx = 'http User/Group configuration' - if not self.uri: - raise ValueError(pfx + ' requires a uri') - if self.values: - raise ValueError(pfx + ' does not take inline values') - if self.ref: - raise ValueError(pfx + ' does not take a ref value') if self.source_type == UserGroupSourceType.RESOURCE: - pfx = 'resource reference User/Group configuration' if not self.ref: - raise ValueError(pfx + ' requires a ref value') - if self.uri: - raise ValueError(pfx + ' does not take a uri') - if self.values: - raise ValueError(pfx + ' does not take inline values') + raise ValueError('reference value must be specified') + else: + validation.check_id(self.ref) + else: + if self.ref: + raise ValueError('ref may not be specified') @resourcelib.customize def _customize_resource(rc: resourcelib.Resource) -> resourcelib.Resource: - rc.uri.quiet = True rc.ref.quiet = True return rc diff --git a/src/pybind/mgr/smb/tests/test_enums.py b/src/pybind/mgr/smb/tests/test_enums.py index f3f0f4eeb8b..ef0edf87acb 100644 --- a/src/pybind/mgr/smb/tests/test_enums.py +++ b/src/pybind/mgr/smb/tests/test_enums.py @@ -18,8 +18,6 @@ import smb.enums (smb.enums.State.UPDATED, "updated"), (smb.enums.AuthMode.USER, "user"), (smb.enums.AuthMode.ACTIVE_DIRECTORY, "active-directory"), - (smb.enums.JoinSourceType.PASSWORD, "password"), - (smb.enums.UserGroupSourceType.INLINE, "inline"), ], ) def test_stringified(value, strval): diff --git a/src/pybind/mgr/smb/tests/test_handler.py b/src/pybind/mgr/smb/tests/test_handler.py index 1a22dbb6e15..ceaf044744d 100644 --- a/src/pybind/mgr/smb/tests/test_handler.py +++ b/src/pybind/mgr/smb/tests/test_handler.py @@ -31,11 +31,7 @@ def test_internal_apply_cluster(thandler): auth_mode=smb.enums.AuthMode.USER, user_group_settings=[ smb.resources.UserGroupSource( - source_type=smb.resources.UserGroupSourceType.INLINE, - values=smb.resources.UserGroupSettings( - users=[], - groups=[], - ), + source_type=smb.resources.UserGroupSourceType.EMPTY, ), ], ) @@ -50,11 +46,7 @@ def test_cluster_add(thandler): auth_mode=smb.enums.AuthMode.USER, user_group_settings=[ smb.resources.UserGroupSource( - source_type=smb.resources.UserGroupSourceType.INLINE, - values=smb.resources.UserGroupSettings( - users=[], - groups=[], - ), + source_type=smb.resources.UserGroupSourceType.EMPTY, ), ], ) @@ -72,11 +64,7 @@ def test_internal_apply_cluster_and_share(thandler): auth_mode=smb.enums.AuthMode.USER, user_group_settings=[ smb.resources.UserGroupSource( - source_type=smb.resources.UserGroupSourceType.INLINE, - values=smb.resources.UserGroupSettings( - users=[], - groups=[], - ), + source_type=smb.resources.UserGroupSourceType.EMPTY, ), ], ) @@ -109,8 +97,7 @@ def test_internal_apply_remove_cluster(thandler): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], } @@ -141,8 +128,7 @@ def test_internal_apply_remove_shares(thandler): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], }, @@ -222,8 +208,7 @@ def test_internal_apply_add_joinauth(thandler): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], } @@ -254,8 +239,7 @@ def test_internal_apply_add_usergroups(thandler): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], } @@ -286,8 +270,7 @@ def test_generate_config_basic(thandler): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], }, @@ -338,15 +321,21 @@ def test_generate_config_ad(thandler): 'realm': 'dom1.example.com', 'join_sources': [ { - 'source_type': 'password', - 'auth': { - 'username': 'testadmin', - 'password': 'Passw0rd', - }, + 'source_type': 'resource', + 'ref': 'foo1', } ], }, }, + 'join_auths.foo1': { + 'resource_type': 'ceph.smb.join.auth', + 'auth_id': 'foo1', + 'intent': 'present', + 'auth': { + 'username': 'testadmin', + 'password': 'Passw0rd', + }, + }, 'shares.foo.s1': { 'resource_type': 'ceph.smb.share', 'cluster_id': 'foo', @@ -566,52 +555,6 @@ def test_apply_update_password(thandler): assert jdata == {'username': 'testadmin', 'password': 'Zm9vYmFyCg'} -def test_apply_update_cluster_inline_pw(thandler): - test_apply_full_cluster_create(thandler) - to_apply = [ - smb.resources.Cluster( - cluster_id='mycluster1', - auth_mode=smb.enums.AuthMode.ACTIVE_DIRECTORY, - domain_settings=smb.resources.DomainSettings( - realm='MYDOMAIN.EXAMPLE.ORG', - join_sources=[ - smb.resources.JoinSource( - source_type=smb.enums.JoinSourceType.RESOURCE, - ref='join1', - ), - smb.resources.JoinSource( - source_type=smb.enums.JoinSourceType.PASSWORD, - auth=smb.resources.JoinAuthValues( - username='Jimmy', - password='j4mb0ree!', - ), - ), - ], - ), - ), - ] - - results = thandler.apply(to_apply) - assert results.success, results.to_simplified() - assert len(list(results)) == 1 - - assert 'mycluster1' in thandler.public_store.namespaces() - ekeys = list(thandler.public_store.contents('mycluster1')) - assert len(ekeys) == 5 - assert 'cluster-info' in ekeys - assert 'config.smb' in ekeys - assert 'spec.smb' in ekeys - assert 'join.0.json' in ekeys - assert 'join.1.json' in ekeys - - # we changed the password value. the store should reflect that - jdata = thandler.public_store['mycluster1', 'join.0.json'].get() - assert jdata == {'username': 'testadmin', 'password': 'Passw0rd'} - # we changed the password value. the store should reflect that - jdata2 = thandler.public_store['mycluster1', 'join.1.json'].get() - assert jdata2 == {'username': 'Jimmy', 'password': 'j4mb0ree!'} - - def test_apply_add_second_cluster(thandler): test_apply_full_cluster_create(thandler) to_apply = [ @@ -622,15 +565,20 @@ def test_apply_add_second_cluster(thandler): realm='YOURDOMAIN.EXAMPLE.ORG', join_sources=[ smb.resources.JoinSource( - source_type=smb.enums.JoinSourceType.PASSWORD, - auth=smb.resources.JoinAuthValues( - username='Jimmy', - password='j4mb0ree!', - ), + source_type=smb.enums.JoinSourceType.RESOURCE, + ref='coolcluster', ), ], ), ), + smb.resources.JoinAuth( + auth_id='coolcluster', + auth=smb.resources.JoinAuthValues( + username='Jimmy', + password='j4mb0ree!', + ), + linked_to_cluster='coolcluster', + ), smb.resources.Share( cluster_id='coolcluster', share_id='images', @@ -643,7 +591,7 @@ def test_apply_add_second_cluster(thandler): results = thandler.apply(to_apply) assert results.success, results.to_simplified() - assert len(list(results)) == 2 + assert len(list(results)) == 3 assert 'mycluster1' in thandler.public_store.namespaces() ekeys = list(thandler.public_store.contents('mycluster1')) @@ -865,13 +813,14 @@ def test_apply_remove_all_clusters(thandler): def test_all_resources(thandler): test_apply_add_second_cluster(thandler) rall = thandler.all_resources() - assert len(rall) == 6 + assert len(rall) == 7 assert rall[0].resource_type == 'ceph.smb.cluster' assert rall[1].resource_type == 'ceph.smb.share' assert rall[2].resource_type == 'ceph.smb.share' assert rall[3].resource_type == 'ceph.smb.cluster' assert rall[4].resource_type == 'ceph.smb.share' assert rall[5].resource_type == 'ceph.smb.join.auth' + assert rall[6].resource_type == 'ceph.smb.join.auth' @pytest.mark.parametrize( @@ -962,6 +911,10 @@ def test_all_resources(thandler): 'resource_type': 'ceph.smb.join.auth', 'auth_id': 'join1', }, + { + 'resource_type': 'ceph.smb.join.auth', + 'auth_id': 'coolcluster', + }, ], ), # cluster with id diff --git a/src/pybind/mgr/smb/tests/test_resources.py b/src/pybind/mgr/smb/tests/test_resources.py index 6fce09c2698..82446876a7c 100644 --- a/src/pybind/mgr/smb/tests/test_resources.py +++ b/src/pybind/mgr/smb/tests/test_resources.py @@ -117,10 +117,6 @@ domain_settings: join_sources: - source_type: resource ref: bob - - source_type: password - auth: - username: Administrator - password: fallb4kP4ssw0rd --- resource_type: ceph.smb.share cluster_id: chacha @@ -168,13 +164,10 @@ def test_load_yaml_resource_yaml1(): assert cluster.intent == enums.Intent.PRESENT assert cluster.auth_mode == enums.AuthMode.ACTIVE_DIRECTORY assert cluster.domain_settings.realm == 'CEPH.SINK.TEST' - assert len(cluster.domain_settings.join_sources) == 2 + assert len(cluster.domain_settings.join_sources) == 1 jsrc = cluster.domain_settings.join_sources assert jsrc[0].source_type == enums.JoinSourceType.RESOURCE assert jsrc[0].ref == 'bob' - assert jsrc[1].source_type == enums.JoinSourceType.PASSWORD - assert jsrc[1].auth.username == 'Administrator' - assert jsrc[1].auth.password == 'fallb4kP4ssw0rd' assert isinstance(loaded[1], smb.resources.Share) assert isinstance(loaded[2], smb.resources.Share) @@ -427,7 +420,7 @@ domain_settings: "exc_type": ValueError, "error": "not supported", }, - # u/g inline missing + # u/g empty with extra ref { "yaml": """ resource_type: ceph.smb.cluster @@ -435,89 +428,11 @@ cluster_id: randolph intent: present auth_mode: user user_group_settings: - - source_type: inline -""", - "exc_type": ValueError, - "error": "requires values", - }, - # u/g inline extra uri - { - "yaml": """ -resource_type: ceph.smb.cluster -cluster_id: randolph -intent: present -auth_mode: user -user_group_settings: - - source_type: inline - values: - users: [] - groups: [] - uri: http://foo.bar.example.com/baz.txt -""", - "exc_type": ValueError, - "error": "does not take", - }, - # u/g inline extra ref - { - "yaml": """ -resource_type: ceph.smb.cluster -cluster_id: randolph -intent: present -auth_mode: user -user_group_settings: - - source_type: inline - values: - users: [] - groups: [] + - source_type: empty ref: xyz """, "exc_type": ValueError, - "error": "does not take", - }, - # u/g uri missing - { - "yaml": """ -resource_type: ceph.smb.cluster -cluster_id: randolph -intent: present -auth_mode: user -user_group_settings: - - source_type: http_uri -""", - "exc_type": ValueError, - "error": "requires", - }, - # u/g uri extra values - { - "yaml": """ -resource_type: ceph.smb.cluster -cluster_id: randolph -intent: present -auth_mode: user -user_group_settings: - - source_type: http_uri - values: - users: [] - groups: [] - uri: http://foo.bar.example.com/baz.txt -""", - "exc_type": ValueError, - "error": "does not take", - }, - # u/g uri extra ref - { - "yaml": """ -resource_type: ceph.smb.cluster -cluster_id: randolph -intent: present -auth_mode: user -user_group_settings: - - source_type: http_uri - uri: http://boop.example.net - ref: xyz -""", - "exc_type": ValueError, - "error": "does not take", + "error": "ref may not be", }, # u/g resource missing { @@ -530,39 +445,7 @@ user_group_settings: - source_type: resource """, "exc_type": ValueError, - "error": "requires", - }, - # u/g resource extra values - { - "yaml": """ -resource_type: ceph.smb.cluster -cluster_id: randolph -intent: present -auth_mode: user -user_group_settings: - - source_type: resource - ref: xyz - uri: http://example.net/foo -""", - "exc_type": ValueError, - "error": "does not take", - }, - # u/g resource extra resource - { - "yaml": """ -resource_type: ceph.smb.cluster -cluster_id: randolph -intent: present -auth_mode: user -user_group_settings: - - source_type: resource - ref: xyz - values: - users: [] - groups: [] -""", - "exc_type": ValueError, - "error": "does not take", + "error": "reference value must be", }, ], ) diff --git a/src/pybind/mgr/smb/tests/test_smb.py b/src/pybind/mgr/smb/tests/test_smb.py index 09380cad869..429a5cdec00 100644 --- a/src/pybind/mgr/smb/tests/test_smb.py +++ b/src/pybind/mgr/smb/tests/test_smb.py @@ -39,11 +39,7 @@ def test_internal_apply_cluster(tmodule): auth_mode=smb.enums.AuthMode.USER, user_group_settings=[ smb.resources.UserGroupSource( - source_type=smb.resources.UserGroupSourceType.INLINE, - values=smb.resources.UserGroupSettings( - users=[], - groups=[], - ), + source_type=smb.resources.UserGroupSourceType.EMPTY, ), ], ) @@ -58,11 +54,7 @@ def test_cluster_add_cluster_ls(tmodule): auth_mode=smb.enums.AuthMode.USER, user_group_settings=[ smb.resources.UserGroupSource( - source_type=smb.resources.UserGroupSourceType.INLINE, - values=smb.resources.UserGroupSettings( - users=[], - groups=[], - ), + source_type=smb.resources.UserGroupSourceType.EMPTY, ), ], ) @@ -80,11 +72,7 @@ def test_internal_apply_cluster_and_share(tmodule): auth_mode=smb.enums.AuthMode.USER, user_group_settings=[ smb.resources.UserGroupSource( - source_type=smb.resources.UserGroupSourceType.INLINE, - values=smb.resources.UserGroupSettings( - users=[], - groups=[], - ), + source_type=smb.resources.UserGroupSourceType.EMPTY, ), ], ) @@ -117,8 +105,7 @@ def test_internal_apply_remove_cluster(tmodule): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], } @@ -149,8 +136,7 @@ def test_internal_apply_remove_shares(tmodule): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], }, @@ -230,8 +216,7 @@ def test_internal_apply_add_joinauth(tmodule): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], } @@ -262,8 +247,7 @@ def test_internal_apply_add_usergroups(tmodule): 'intent': 'present', 'user_group_settings': [ { - 'source_type': 'inline', - 'values': {'users': [], 'groups': []}, + 'source_type': 'empty', } ], } @@ -296,15 +280,21 @@ def _example_cfg_1(tmodule): 'realm': 'dom1.example.com', 'join_sources': [ { - 'source_type': 'password', - 'auth': { - 'username': 'testadmin', - 'password': 'Passw0rd', - }, + 'source_type': 'resource', + 'ref': 'foo', } ], }, }, + 'join_auths.foo': { + 'resource_type': 'ceph.smb.join.auth', + 'auth_id': 'foo', + 'intent': 'present', + 'auth': { + 'username': 'testadmin', + 'password': 'Passw0rd', + }, + }, 'shares.foo.s1': { 'resource_type': 'ceph.smb.share', 'cluster_id': 'foo', -- 2.39.5