From 823461cd25884a8edcb37af82ef0bc700537141a Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 17 Feb 2023 10:52:44 -0300 Subject: [PATCH 01/17] Implement and connect the AdJoiner AD controller offers the blockign endpoint. AD Joiner creates a task when the AD Controller join_domain() is called. Install controller requests that in the end of the post install method. AD Controller calls itself on dry run for testing purposes TODO: - refine tests; - do the real join. --- subiquity/common/apidef.py | 4 +++ subiquity/common/types.py | 6 ++++ subiquity/server/ad_joiner.py | 41 +++++++++++++++++++++++++ subiquity/server/controllers/ad.py | 21 +++++++++++++ subiquity/server/controllers/install.py | 2 ++ subiquity/tests/api/test_api.py | 3 ++ 6 files changed, 77 insertions(+) create mode 100644 subiquity/server/ad_joiner.py diff --git a/subiquity/common/apidef.py b/subiquity/common/apidef.py index e75da3c4..f0f0d0ab 100644 --- a/subiquity/common/apidef.py +++ b/subiquity/common/apidef.py @@ -28,6 +28,7 @@ from subiquity.common.types import ( ADConnectionInfo, AdAdminNameValidation, AdDomainNameValidation, + AdJoinResult, AdPasswordValidation, AddPartitionV2, AnyStep, @@ -434,6 +435,9 @@ class API: class check_password: def POST(password: Payload[str]) -> AdPasswordValidation: ... + class join_result: + def GET(wait: bool = True) -> AdJoinResult: ... + class LinkAction(enum.Enum): NEW = enum.auto() diff --git a/subiquity/common/types.py b/subiquity/common/types.py index 732408b1..e33cfc7f 100644 --- a/subiquity/common/types.py +++ b/subiquity/common/types.py @@ -801,3 +801,9 @@ class AdDomainNameValidation(enum.Enum): class AdPasswordValidation(enum.Enum): OK = 'OK' EMPTY = 'Empty' + + +class AdJoinResult(enum.Enum): + OK = 'OK' + JOIN_ERROR = 'Failed to join' + UNKNOWN = "Didn't attempt to join yet" diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py new file mode 100644 index 00000000..2801c0f2 --- /dev/null +++ b/subiquity/server/ad_joiner.py @@ -0,0 +1,41 @@ +# Copyright 2023 Canonical, Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import asyncio +from subiquity.common.types import ( + ADConnectionInfo, + AdJoinResult, +) + + +class AdJoiner(): + def __init__(self): + self._result = AdJoinResult.UNKNOWN + self.join_task = None + + async def join_domain(self, info: ADConnectionInfo) -> AdJoinResult: + self.join_task = asyncio.create_task(self.async_join(info)) + self._result = await self.join_task + return self._result + + async def async_join(self, info: ADConnectionInfo) -> AdJoinResult: + # TODO: Join. + return await asyncio.sleep(3, result=AdJoinResult.JOIN_ERROR) + + async def join_result(self): + if self.join_task is None: + return AdJoinResult.UNKNOWN + + return await self.join_task diff --git a/subiquity/server/controllers/ad.py b/subiquity/server/controllers/ad.py index 346f6210..89f2e8d6 100644 --- a/subiquity/server/controllers/ad.py +++ b/subiquity/server/controllers/ad.py @@ -25,8 +25,10 @@ from subiquity.common.types import ( ADConnectionInfo, AdAdminNameValidation, AdDomainNameValidation, + AdJoinResult, AdPasswordValidation ) +from subiquity.server.ad_joiner import AdJoiner from subiquity.server.controller import SubiquityController log = logging.getLogger('subiquity.server.controllers.ad') @@ -95,6 +97,8 @@ class ADController(SubiquityController): def __init__(self, app): super().__init__(app) + self.ad_joiner = None + self.join_result = AdJoinResult.UNKNOWN if self.app.opts.dry_run: self.ping_strgy = StubDcPingStrategy() else: @@ -141,6 +145,23 @@ class ADController(SubiquityController): to configure AD are present in the live system.""" return self.ping_strgy.has_support() + async def join_result_GET(self, wait: bool = True) -> AdJoinResult: + # Enables testing the API without the need for the install controller + if self.app.opts.dry_run and self.ad_joiner is None: + await self.join_domain() + + if wait and self.ad_joiner: + self.join_result = await self.ad_joiner.join_result() + + return self.join_result + + async def join_domain(self) -> None: + """To be called from the install controller if joining was requested""" + if self.ad_joiner is None: + self.ad_joiner = AdJoiner() + + await self.ad_joiner.join_domain(self.model.conn_info) + # Helper out-of-class functions grouped. class AdValidators: diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 8f99322f..55fbabb1 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -406,6 +406,8 @@ class InstallController(SubiquityController): policy = self.model.updates.updates await self.run_unattended_upgrades(context=context, policy=policy) await self.restore_apt_config(context=context) + if self.model.ad.do_join: + await self.app.controllers.Ad.join_domain() @with_context(description="configuring cloud-init") async def configure_cloud_init(self, context): diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 77c2d4a0..b501e7a1 100644 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -1696,3 +1696,6 @@ class TestActiveDirectory(TestAPI): result = await instance.post(endpoint + '/check_admin_name', data='$Ubuntu') self.assertEqual('OK', result) + # Attempts to join with the info supplied above. + join_result = await instance.get(endpoint + '/join_result') + self.assertEqual('JOIN_ERROR', join_result) From fc6facbc018575d51a38d697c3d734118fb11caf Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 17 Feb 2023 12:13:28 -0300 Subject: [PATCH 02/17] Split strategies for live and dry run. --- subiquity/server/ad_joiner.py | 38 +++++++++++++++++++++++++++--- subiquity/server/controllers/ad.py | 2 +- subiquity/tests/api/test_api.py | 7 ++++++ 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index 2801c0f2..a5601097 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -20,10 +20,43 @@ from subiquity.common.types import ( ) +class AdJoinStrategy(): + cmd = "/usr/sbin/realm" + args = ["join"] + + async def do_join(self, info: ADConnectionInfo) -> AdJoinResult: + # Now what? + # TODO: Join. + result = AdJoinResult.JOIN_ERROR + return await asyncio.sleep(3, result=result) + + +class StubStrategy(AdJoinStrategy): + async def do_join(self, info: ADConnectionInfo, hostname: str, context) \ + -> AdJoinResult: + """ Enables testing without real join. The result depends on the + domain name initial character, such that if it is: + - p or P: returns PAM_ERROR. + - j or J: returns JOIN_ERROR. + - returns OK otherwise. """ + initial = info.domain_name[0] + if initial in ('j', 'J'): + return AdJoinResult.JOIN_ERROR + + if initial in ('p', 'P'): + return AdJoinResult.PAM_ERROR + + return AdJoinResult.OK + + class AdJoiner(): - def __init__(self): + def __init__(self, dry_run: bool): self._result = AdJoinResult.UNKNOWN self.join_task = None + if dry_run: + self.strategy = StubStrategy() + else: + self.strategy = AdJoinStrategy() async def join_domain(self, info: ADConnectionInfo) -> AdJoinResult: self.join_task = asyncio.create_task(self.async_join(info)) @@ -31,8 +64,7 @@ class AdJoiner(): return self._result async def async_join(self, info: ADConnectionInfo) -> AdJoinResult: - # TODO: Join. - return await asyncio.sleep(3, result=AdJoinResult.JOIN_ERROR) + return await self.strategy.do_join(info) async def join_result(self): if self.join_task is None: diff --git a/subiquity/server/controllers/ad.py b/subiquity/server/controllers/ad.py index 89f2e8d6..2afa1530 100644 --- a/subiquity/server/controllers/ad.py +++ b/subiquity/server/controllers/ad.py @@ -158,7 +158,7 @@ class ADController(SubiquityController): async def join_domain(self) -> None: """To be called from the install controller if joining was requested""" if self.ad_joiner is None: - self.ad_joiner = AdJoiner() + self.ad_joiner = AdJoiner(self.app.opts.dry_run) await self.ad_joiner.join_domain(self.model.conn_info) diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index b501e7a1..62672827 100644 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -1697,5 +1697,12 @@ class TestActiveDirectory(TestAPI): data='$Ubuntu') self.assertEqual('OK', result) # Attempts to join with the info supplied above. + ad_dict = { + 'admin_name': 'Ubuntu', + 'domain_name': 'jubuntu.com', + 'password': 'u', + } + result = await instance.post(endpoint, ad_dict) + self.assertIsNone(result) join_result = await instance.get(endpoint + '/join_result') self.assertEqual('JOIN_ERROR', join_result) From 2cedf0f3f080cc22ab2a221ef718ef0d5959ea88 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 17 Feb 2023 17:00:41 -0300 Subject: [PATCH 03/17] Attempts to join the target through curtin --- subiquity/common/types.py | 2 + subiquity/server/ad_joiner.py | 82 +++++++++++++++++++------ subiquity/server/controllers/ad.py | 22 +++---- subiquity/server/controllers/install.py | 7 ++- 4 files changed, 81 insertions(+), 32 deletions(-) diff --git a/subiquity/common/types.py b/subiquity/common/types.py index e33cfc7f..4875f97b 100644 --- a/subiquity/common/types.py +++ b/subiquity/common/types.py @@ -806,4 +806,6 @@ class AdPasswordValidation(enum.Enum): class AdJoinResult(enum.Enum): OK = 'OK' JOIN_ERROR = 'Failed to join' + EMPTY_HOSTNAME = 'Target hostname cannot be empty' + PAM_ERROR = 'Failed to update pam-auth' UNKNOWN = "Didn't attempt to join yet" diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index a5601097..8df3b37b 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -14,21 +14,65 @@ # along with this program. If not, see . import asyncio +import logging +from socket import gethostname +from subiquitycore.utils import arun_command +from subiquity.server.curtin import run_curtin_command from subiquity.common.types import ( ADConnectionInfo, AdJoinResult, ) +log = logging.getLogger('subiquity.server.ad_joiner') + class AdJoinStrategy(): cmd = "/usr/sbin/realm" args = ["join"] - async def do_join(self, info: ADConnectionInfo) -> AdJoinResult: - # Now what? - # TODO: Join. + def __init__(self, app): + self.app = app + + async def do_join(self, info: ADConnectionInfo, hostname: str, context) \ + -> AdJoinResult: + """ This method changes the hostname and perform a real AD join, thus + should only run in a live session. """ result = AdJoinResult.JOIN_ERROR - return await asyncio.sleep(3, result=result) + hostname_current = gethostname() + # Set hostname for AD to determine FQDN (no FQDN option in realm join, + # only adcli, which only understands the live system, but not chroot) + cp = await arun_command(['hostname', hostname]) + if cp.returncode: + log.info("Failed to set live session hostname for adcli") + return result + + root_dir = self.app.root + cp = await run_curtin_command( + self.app, context, "in-target", "-t", root_dir, + "--", "realm", "join", "--install", root_dir, "--user", + info.admin_name, "--computer-name", hostname, "--unattended", + info.domain_name, private_mounts=True, input=info.password, + timeout=60) + + if not cp.returncode: + # Restoring the live session hostname. + # Enable pam_mkhomedir + cp = await run_curtin_command(self.app, context, "in-target", + "-t", root_dir, "--", + "pam-auth-update", "--package", + "--enable", "mkhomedir", + private_mounts=True) + + if cp.returncode: + result = AdJoinResult.PAM_ERROR + else: + result = AdJoinResult.OK + + cp = await arun_command(['hostname', hostname_current]) + if cp.returncode: + log.info("Failed to restore live session hostname") + + return result class StubStrategy(AdJoinStrategy): @@ -50,24 +94,24 @@ class StubStrategy(AdJoinStrategy): class AdJoiner(): - def __init__(self, dry_run: bool): + def __init__(self, app): self._result = AdJoinResult.UNKNOWN - self.join_task = None - if dry_run: - self.strategy = StubStrategy() + self._completion_event = asyncio.Event() + if app.opts.dry_run: + self.strategy = StubStrategy(app) else: - self.strategy = AdJoinStrategy() + self.strategy = AdJoinStrategy(app) - async def join_domain(self, info: ADConnectionInfo) -> AdJoinResult: - self.join_task = asyncio.create_task(self.async_join(info)) - self._result = await self.join_task + async def join_domain(self, info: ADConnectionInfo, hostname: str, + context) -> AdJoinResult: + if hostname: + self._result = await self.strategy.do_join(info, hostname, context) + else: + self._result = AdJoinResult.EMPTY_HOSTNAME + + self._completion_event.set() return self._result - async def async_join(self, info: ADConnectionInfo) -> AdJoinResult: - return await self.strategy.do_join(info) - async def join_result(self): - if self.join_task is None: - return AdJoinResult.UNKNOWN - - return await self.join_task + await self._completion_event.wait() + return self._result diff --git a/subiquity/server/controllers/ad.py b/subiquity/server/controllers/ad.py index 2afa1530..fc102c5e 100644 --- a/subiquity/server/controllers/ad.py +++ b/subiquity/server/controllers/ad.py @@ -97,7 +97,7 @@ class ADController(SubiquityController): def __init__(self, app): super().__init__(app) - self.ad_joiner = None + self.ad_joiner = AdJoiner(self.app) self.join_result = AdJoinResult.UNKNOWN if self.app.opts.dry_run: self.ping_strgy = StubDcPingStrategy() @@ -146,21 +146,19 @@ class ADController(SubiquityController): return self.ping_strgy.has_support() async def join_result_GET(self, wait: bool = True) -> AdJoinResult: - # Enables testing the API without the need for the install controller - if self.app.opts.dry_run and self.ad_joiner is None: - await self.join_domain() - - if wait and self.ad_joiner: + """ If [wait] is True, this method blocks until an attempt to + join a domain completes. Otherwise returns the current known state. + Most likely it will be AdJoinResult.UNKNOWN. """ + if wait: self.join_result = await self.ad_joiner.join_result() return self.join_result - async def join_domain(self) -> None: - """To be called from the install controller if joining was requested""" - if self.ad_joiner is None: - self.ad_joiner = AdJoiner(self.app.opts.dry_run) - - await self.ad_joiner.join_domain(self.model.conn_info) + async def join_domain(self, hostname: str, context) -> None: + """ To be called from the install controller if the user requested + joining an AD domain """ + await self.ad_joiner.join_domain(self.model.conn_info, hostname, + context) # Helper out-of-class functions grouped. diff --git a/subiquity/server/controllers/install.py b/subiquity/server/controllers/install.py index 55fbabb1..bc37ca0a 100644 --- a/subiquity/server/controllers/install.py +++ b/subiquity/server/controllers/install.py @@ -407,7 +407,12 @@ class InstallController(SubiquityController): await self.run_unattended_upgrades(context=context, policy=policy) await self.restore_apt_config(context=context) if self.model.ad.do_join: - await self.app.controllers.Ad.join_domain() + hostname = self.model.identity.hostname + if not hostname: + with open(self.tpath('etc/hostname'), 'r') as f: + hostname = f.read().strip() + + await self.app.controllers.AD.join_domain(hostname, context) @with_context(description="configuring cloud-init") async def configure_cloud_init(self, context): From af6c7f747eb0c2cfd260bba1c58fca2a7fd352c2 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Wed, 22 Feb 2023 09:29:29 -0300 Subject: [PATCH 04/17] Blocking API test --- subiquity/tests/api/test_api.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 62672827..4de8f7bf 100644 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -1704,5 +1704,12 @@ class TestActiveDirectory(TestAPI): } result = await instance.post(endpoint, ad_dict) self.assertIsNone(result) - join_result = await instance.get(endpoint + '/join_result') - self.assertEqual('JOIN_ERROR', join_result) + join_result_ep = endpoint + '/join_result' + # Without wait this shouldn't block but the result is unknown until + # the install controller runs. + join_result = await instance.get(join_result_ep, wait=False) + self.assertEqual('UNKNOWN', join_result) + # And without the installer controller running, a blocking call + # should timeout since joining never happens. + with self.assertRaises(asyncio.exceptions.CancelledError): + join_result = await instance.get(join_result_ep, wait=True) From ee1ac0493be05207996f8fd821006f8b29722d8b Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Wed, 22 Feb 2023 09:54:19 -0300 Subject: [PATCH 05/17] Use the command class fields --- subiquity/server/ad_joiner.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index 8df3b37b..48c90e28 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -27,8 +27,8 @@ log = logging.getLogger('subiquity.server.ad_joiner') class AdJoinStrategy(): - cmd = "/usr/sbin/realm" - args = ["join"] + realm = "/usr/sbin/realm" + pam = "/usr/sbin/path-auth-update" def __init__(self, app): self.app = app @@ -49,7 +49,7 @@ class AdJoinStrategy(): root_dir = self.app.root cp = await run_curtin_command( self.app, context, "in-target", "-t", root_dir, - "--", "realm", "join", "--install", root_dir, "--user", + "--", self.realm, "join", "--install", root_dir, "--user", info.admin_name, "--computer-name", hostname, "--unattended", info.domain_name, private_mounts=True, input=info.password, timeout=60) @@ -59,7 +59,7 @@ class AdJoinStrategy(): # Enable pam_mkhomedir cp = await run_curtin_command(self.app, context, "in-target", "-t", root_dir, "--", - "pam-auth-update", "--package", + self.pam, "--package", "--enable", "mkhomedir", private_mounts=True) From 7f5466229bd34e185ef1526254d532da54fe1d66 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Wed, 22 Feb 2023 10:19:04 -0300 Subject: [PATCH 06/17] Only block if the model is set for joining Also, the model.do_join field was never set. Now it is. --- subiquity/models/ad.py | 4 ++++ subiquity/server/controllers/ad.py | 9 +++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/subiquity/models/ad.py b/subiquity/models/ad.py index 9cf8c24b..1816b8a4 100644 --- a/subiquity/models/ad.py +++ b/subiquity/models/ad.py @@ -27,6 +27,10 @@ class ADModel: self.do_join = False self.conn_info: Optional[ADConnectionInfo] = None + def set(self, info: ADConnectionInfo): + self.conn_info = info + self.do_join = True + def set_domain(self, domain: str): if not domain: return diff --git a/subiquity/server/controllers/ad.py b/subiquity/server/controllers/ad.py index fc102c5e..605c825f 100644 --- a/subiquity/server/controllers/ad.py +++ b/subiquity/server/controllers/ad.py @@ -120,7 +120,7 @@ class ADController(SubiquityController): async def POST(self, data: ADConnectionInfo) -> None: """ Configures this controller with the supplied info. Clients are required to validate the info before POST'ing """ - self.model.conn_info = data + self.model.set(data) await self.configured() async def check_admin_name_POST(self, admin_name: str) \ @@ -146,10 +146,11 @@ class ADController(SubiquityController): return self.ping_strgy.has_support() async def join_result_GET(self, wait: bool = True) -> AdJoinResult: - """ If [wait] is True, this method blocks until an attempt to - join a domain completes. Otherwise returns the current known state. + """ If [wait] is True and the model is set for joining, this method + blocks until an attempt to join a domain completes. + Otherwise returns the current known state. Most likely it will be AdJoinResult.UNKNOWN. """ - if wait: + if wait and self.model.do_join: self.join_result = await self.ad_joiner.join_result() return self.join_result From c756b1518333917023274700d87e1cd32f2252ab Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Wed, 22 Feb 2023 12:39:32 -0300 Subject: [PATCH 07/17] Tests some async invariants There is a quite non-straightforward sequencing between the AD controller and Joiner classes. A client may do a blocking call to join_result_GET at any moment. It should only be blocking if the ADModel is set. Curtin commands are not tested here, though. --- subiquity/server/tests/test_ad.py | 68 +++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 subiquity/server/tests/test_ad.py diff --git a/subiquity/server/tests/test_ad.py b/subiquity/server/tests/test_ad.py new file mode 100644 index 00000000..174191be --- /dev/null +++ b/subiquity/server/tests/test_ad.py @@ -0,0 +1,68 @@ +# Copyright 2023 Canonical, Ltd. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as +# published by the Free Software Foundation, either version 3 of the +# License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +import unittest +from subiquity.common.types import ADConnectionInfo, AdJoinResult +from subiquity.server.controllers.ad import ADController +from subiquity.models.ad import ADModel +from subiquitycore.tests.mocks import make_app + + +class TestAdJoin(unittest.IsolatedAsyncioTestCase): + def setUp(self): + self.app = make_app() + self.controller = ADController(self.app) + self.controller.model = ADModel() + + async def test_never_join(self): + # Calling join_result_GET has no effect if the model is not set. + result = await self.controller.join_result_GET(wait=True) + self.assertEqual(result, AdJoinResult.UNKNOWN) + + async def test_join_Unknown(self): + # Result remains UNKNOWN while ADController.join_domain is not called. + self.controller.model.set(ADConnectionInfo(domain_name='ubuntu.com', + admin_name='Helper', + password='1234')) + + result = await self.controller.join_result_GET(wait=False) + self.assertEqual(result, AdJoinResult.UNKNOWN) + + async def test_join_OK(self): + # The equivalent of a successful POST + self.controller.model.set(ADConnectionInfo(domain_name='ubuntu.com', + admin_name='Helper', + password='1234')) + # Mimics a client requesting the join result. Blocking by default. + result = self.controller.join_result_GET() + # Mimics a calling from the install controller. + await self.controller.join_domain('this', 'AD Join') + self.assertEqual(await result, AdJoinResult.OK) + + async def test_join_Join_Error(self): + self.controller.model.set(ADConnectionInfo(domain_name='jubuntu.com', + admin_name='Helper', + password='1234')) + await self.controller.join_domain('this', 'AD Join') + result = await self.controller.join_result_GET(wait=True) + self.assertEqual(result, AdJoinResult.JOIN_ERROR) + + async def test_join_Pam_Error(self): + self.controller.model.set(ADConnectionInfo(domain_name='pubuntu.com', + admin_name='Helper', + password='1234')) + await self.controller.join_domain('this', 'AD Join') + result = await self.controller.join_result_GET(wait=True) + self.assertEqual(result, AdJoinResult.PAM_ERROR) From b8ca8af99fa22f88d32345813a4d825f63c134be Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Wed, 22 Feb 2023 13:20:33 -0300 Subject: [PATCH 08/17] Fix command name typo --- subiquity/server/ad_joiner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index 48c90e28..f8dea630 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -28,7 +28,7 @@ log = logging.getLogger('subiquity.server.ad_joiner') class AdJoinStrategy(): realm = "/usr/sbin/realm" - pam = "/usr/sbin/path-auth-update" + pam = "/usr/sbin/pam-auth-update" def __init__(self, app): self.app = app From 3a9fb2b132a956def112f3a82b5a2c5d51192a42 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Thu, 23 Feb 2023 10:06:37 -0300 Subject: [PATCH 09/17] Fix misplaced comment about the live hostname --- subiquity/server/ad_joiner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index f8dea630..becf8ff6 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -55,7 +55,6 @@ class AdJoinStrategy(): timeout=60) if not cp.returncode: - # Restoring the live session hostname. # Enable pam_mkhomedir cp = await run_curtin_command(self.app, context, "in-target", "-t", root_dir, "--", @@ -68,6 +67,7 @@ class AdJoinStrategy(): else: result = AdJoinResult.OK + # Restoring the live session hostname. cp = await arun_command(['hostname', hostname_current]) if cp.returncode: log.info("Failed to restore live session hostname") From a5478feb8cadc7851e2930072cc99fd15fb17a64 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 24 Feb 2023 08:32:59 -0300 Subject: [PATCH 10/17] Merges unit tests into the controllers directory --- subiquity/server/controllers/tests/test_ad.py | 62 ++++++++++++++++- subiquity/server/tests/test_ad.py | 68 ------------------- 2 files changed, 60 insertions(+), 70 deletions(-) delete mode 100644 subiquity/server/tests/test_ad.py diff --git a/subiquity/server/controllers/tests/test_ad.py b/subiquity/server/controllers/tests/test_ad.py index bd6c95e9..3c70c6f7 100644 --- a/subiquity/server/controllers/tests/test_ad.py +++ b/subiquity/server/controllers/tests/test_ad.py @@ -13,13 +13,23 @@ # You should have received a copy of the GNU Affero General Public License # along with this program. If not, see . -from unittest import TestCase +from unittest import ( + TestCase, + IsolatedAsyncioTestCase, +) from subiquity.common.types import ( AdAdminNameValidation, + ADConnectionInfo, AdDomainNameValidation, + AdJoinResult, AdPasswordValidation, ) -from subiquity.server.controllers.ad import AdValidators +from subiquity.server.controllers.ad import ( + ADController, + AdValidators, +) +from subiquity.models.ad import ADModel +from subiquitycore.tests.mocks import make_app class TestADValidation(TestCase): @@ -119,3 +129,51 @@ class TestADValidation(TestCase): admin = r'$Ubuntu{}' result = AdValidators.admin_user_name(admin) self.assertEqual(AdAdminNameValidation.OK, result) + + +class TestAdJoin(IsolatedAsyncioTestCase): + def setUp(self): + self.app = make_app() + self.controller = ADController(self.app) + self.controller.model = ADModel() + + async def test_never_join(self): + # Calling join_result_GET has no effect if the model is not set. + result = await self.controller.join_result_GET(wait=True) + self.assertEqual(result, AdJoinResult.UNKNOWN) + + async def test_join_Unknown(self): + # Result remains UNKNOWN while ADController.join_domain is not called. + self.controller.model.set(ADConnectionInfo(domain_name='ubuntu.com', + admin_name='Helper', + password='1234')) + + result = await self.controller.join_result_GET(wait=False) + self.assertEqual(result, AdJoinResult.UNKNOWN) + + async def test_join_OK(self): + # The equivalent of a successful POST + self.controller.model.set(ADConnectionInfo(domain_name='ubuntu.com', + admin_name='Helper', + password='1234')) + # Mimics a client requesting the join result. Blocking by default. + result = self.controller.join_result_GET() + # Mimics a calling from the install controller. + await self.controller.join_domain('this', 'AD Join') + self.assertEqual(await result, AdJoinResult.OK) + + async def test_join_Join_Error(self): + self.controller.model.set(ADConnectionInfo(domain_name='jubuntu.com', + admin_name='Helper', + password='1234')) + await self.controller.join_domain('this', 'AD Join') + result = await self.controller.join_result_GET(wait=True) + self.assertEqual(result, AdJoinResult.JOIN_ERROR) + + async def test_join_Pam_Error(self): + self.controller.model.set(ADConnectionInfo(domain_name='pubuntu.com', + admin_name='Helper', + password='1234')) + await self.controller.join_domain('this', 'AD Join') + result = await self.controller.join_result_GET(wait=True) + self.assertEqual(result, AdJoinResult.PAM_ERROR) diff --git a/subiquity/server/tests/test_ad.py b/subiquity/server/tests/test_ad.py deleted file mode 100644 index 174191be..00000000 --- a/subiquity/server/tests/test_ad.py +++ /dev/null @@ -1,68 +0,0 @@ -# Copyright 2023 Canonical, Ltd. -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU Affero General Public License as -# published by the Free Software Foundation, either version 3 of the -# License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU Affero General Public License for more details. -# -# You should have received a copy of the GNU Affero General Public License -# along with this program. If not, see . - -import unittest -from subiquity.common.types import ADConnectionInfo, AdJoinResult -from subiquity.server.controllers.ad import ADController -from subiquity.models.ad import ADModel -from subiquitycore.tests.mocks import make_app - - -class TestAdJoin(unittest.IsolatedAsyncioTestCase): - def setUp(self): - self.app = make_app() - self.controller = ADController(self.app) - self.controller.model = ADModel() - - async def test_never_join(self): - # Calling join_result_GET has no effect if the model is not set. - result = await self.controller.join_result_GET(wait=True) - self.assertEqual(result, AdJoinResult.UNKNOWN) - - async def test_join_Unknown(self): - # Result remains UNKNOWN while ADController.join_domain is not called. - self.controller.model.set(ADConnectionInfo(domain_name='ubuntu.com', - admin_name='Helper', - password='1234')) - - result = await self.controller.join_result_GET(wait=False) - self.assertEqual(result, AdJoinResult.UNKNOWN) - - async def test_join_OK(self): - # The equivalent of a successful POST - self.controller.model.set(ADConnectionInfo(domain_name='ubuntu.com', - admin_name='Helper', - password='1234')) - # Mimics a client requesting the join result. Blocking by default. - result = self.controller.join_result_GET() - # Mimics a calling from the install controller. - await self.controller.join_domain('this', 'AD Join') - self.assertEqual(await result, AdJoinResult.OK) - - async def test_join_Join_Error(self): - self.controller.model.set(ADConnectionInfo(domain_name='jubuntu.com', - admin_name='Helper', - password='1234')) - await self.controller.join_domain('this', 'AD Join') - result = await self.controller.join_result_GET(wait=True) - self.assertEqual(result, AdJoinResult.JOIN_ERROR) - - async def test_join_Pam_Error(self): - self.controller.model.set(ADConnectionInfo(domain_name='pubuntu.com', - admin_name='Helper', - password='1234')) - await self.controller.join_domain('this', 'AD Join') - result = await self.controller.join_result_GET(wait=True) - self.assertEqual(result, AdJoinResult.PAM_ERROR) From 706359ea6f335f9ab0c05cb0c4cce8799c38de7e Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 24 Feb 2023 08:39:43 -0300 Subject: [PATCH 11/17] Local-scope timeout in AD API tests Timeout was triggered by the test timeout. This puts a timeout on the single call that must timeout. Potentially speeds up the test scenario. --- subiquity/tests/api/test_api.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/subiquity/tests/api/test_api.py b/subiquity/tests/api/test_api.py index 4de8f7bf..294c3e48 100644 --- a/subiquity/tests/api/test_api.py +++ b/subiquity/tests/api/test_api.py @@ -1711,5 +1711,6 @@ class TestActiveDirectory(TestAPI): self.assertEqual('UNKNOWN', join_result) # And without the installer controller running, a blocking call # should timeout since joining never happens. - with self.assertRaises(asyncio.exceptions.CancelledError): - join_result = await instance.get(join_result_ep, wait=True) + with self.assertRaises(asyncio.exceptions.TimeoutError): + join_result = instance.get(join_result_ep, wait=True) + await asyncio.wait_for(join_result, timeout=1.5) From 5c059dd6ab67529d9a212a468c6716c252f2edc8 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 24 Feb 2023 08:55:52 -0300 Subject: [PATCH 12/17] Wraps the hostname setting into a context manager To have more deterministic set/reset behavior. --- subiquity/server/ad_joiner.py | 67 +++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index becf8ff6..cb952252 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -14,9 +14,10 @@ # along with this program. If not, see . import asyncio +from contextlib import contextmanager import logging from socket import gethostname -from subiquitycore.utils import arun_command +from subiquitycore.utils import run_command from subiquity.server.curtin import run_curtin_command from subiquity.common.types import ( ADConnectionInfo, @@ -26,6 +27,19 @@ from subiquity.common.types import ( log = logging.getLogger('subiquity.server.ad_joiner') +@contextmanager +def hostname_context(hostname: str): + """ Temporarily adjusts the host name to [hostname] and restores it + back in the end of the caller scope. """ + hostname_current = gethostname() + hostname_process = run_command(['hostname', hostname]) + yield hostname_process + # Restoring the live session hostname. + hostname_process = run_command(['hostname', hostname_current]) + if hostname_process.returncode: + log.info("Failed to restore live session hostname") + + class AdJoinStrategy(): realm = "/usr/sbin/realm" pam = "/usr/sbin/pam-auth-update" @@ -37,42 +51,35 @@ class AdJoinStrategy(): -> AdJoinResult: """ This method changes the hostname and perform a real AD join, thus should only run in a live session. """ - result = AdJoinResult.JOIN_ERROR - hostname_current = gethostname() # Set hostname for AD to determine FQDN (no FQDN option in realm join, # only adcli, which only understands the live system, but not chroot) - cp = await arun_command(['hostname', hostname]) - if cp.returncode: - log.info("Failed to set live session hostname for adcli") - return result + with hostname_context(hostname) as host_process: + if host_process.returncode: + log.info("Failed to set live session hostname for adcli") + return AdJoinResult.JOIN_ERROR - root_dir = self.app.root - cp = await run_curtin_command( - self.app, context, "in-target", "-t", root_dir, - "--", self.realm, "join", "--install", root_dir, "--user", - info.admin_name, "--computer-name", hostname, "--unattended", - info.domain_name, private_mounts=True, input=info.password, - timeout=60) + root_dir = self.app.root + cp = await run_curtin_command( + self.app, context, "in-target", "-t", root_dir, + "--", self.realm, "join", "--install", root_dir, "--user", + info.admin_name, "--computer-name", hostname, "--unattended", + info.domain_name, private_mounts=True, input=info.password, + timeout=60) - if not cp.returncode: - # Enable pam_mkhomedir - cp = await run_curtin_command(self.app, context, "in-target", - "-t", root_dir, "--", - self.pam, "--package", - "--enable", "mkhomedir", - private_mounts=True) + if not cp.returncode: + # Enable pam_mkhomedir + cp = await run_curtin_command(self.app, context, "in-target", + "-t", root_dir, "--", + self.pam, "--package", + "--enable", "mkhomedir", + private_mounts=True) - if cp.returncode: - result = AdJoinResult.PAM_ERROR + if cp.returncode: + return AdJoinResult.PAM_ERROR else: - result = AdJoinResult.OK + return AdJoinResult.OK - # Restoring the live session hostname. - cp = await arun_command(['hostname', hostname_current]) - if cp.returncode: - log.info("Failed to restore live session hostname") - - return result + return AdJoinResult.JOIN_ERROR class StubStrategy(AdJoinStrategy): From 03c80eb8a6b3321860c05ea32c52a2cd09843ab8 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 24 Feb 2023 09:08:29 -0300 Subject: [PATCH 13/17] Realm doesn't need curtin It turns out that the `realm --install /target` does a chroot. So curtin in target command is not necessary for that. The same doesn't hold for pam-auth-update. Setting hostnames is still a requirement, because realm calls adcli under the hood, which doesn't go through chroot. --- subiquity/server/ad_joiner.py | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index cb952252..b5858f5e 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -17,7 +17,8 @@ import asyncio from contextlib import contextmanager import logging from socket import gethostname -from subiquitycore.utils import run_command +from subprocess import CalledProcessError +from subiquitycore.utils import arun_command, run_command from subiquity.server.curtin import run_curtin_command from subiquity.common.types import ( ADConnectionInfo, @@ -59,25 +60,28 @@ class AdJoinStrategy(): return AdJoinResult.JOIN_ERROR root_dir = self.app.root - cp = await run_curtin_command( - self.app, context, "in-target", "-t", root_dir, - "--", self.realm, "join", "--install", root_dir, "--user", - info.admin_name, "--computer-name", hostname, "--unattended", - info.domain_name, private_mounts=True, input=info.password, - timeout=60) + cp = await arun_command([self.realm, "join", "--install", root_dir, + "--user", info.admin_name, + "--computer-name", hostname, + "--unattended", info.domain_name], + input=info.password, timeout=60) if not cp.returncode: # Enable pam_mkhomedir - cp = await run_curtin_command(self.app, context, "in-target", - "-t", root_dir, "--", - self.pam, "--package", - "--enable", "mkhomedir", - private_mounts=True) + try: + cp = await run_curtin_command(self.app, context, + "in-target", "-t", root_dir, + "--", self.pam, "--package", + "--enable", "mkhomedir", + private_mounts=False) - if cp.returncode: + return AdJoinResult.OK + except CalledProcessError: + # The app command runner doesn't give us output in case of + # failure in the wait() method, which is called by + # run_curtin_command + log.info("Failed to update pam-auth") return AdJoinResult.PAM_ERROR - else: - return AdJoinResult.OK return AdJoinResult.JOIN_ERROR From 0bd9e0723c3dc504e8d94d4b1e37ff51ff68ae5c Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 24 Feb 2023 10:19:22 -0300 Subject: [PATCH 14/17] No timeouts in arun_command --- subiquity/server/ad_joiner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index b5858f5e..149579df 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -64,7 +64,7 @@ class AdJoinStrategy(): "--user", info.admin_name, "--computer-name", hostname, "--unattended", info.domain_name], - input=info.password, timeout=60) + input=info.password) if not cp.returncode: # Enable pam_mkhomedir From 6914674e599dfebf2b87a65749f6cf923a8c9057 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Fri, 24 Feb 2023 14:51:38 -0300 Subject: [PATCH 15/17] Attempts different ways to join Even though AD joining is not that critical. I noticed in lab higher success rates when not specifying the computer name option in realm join CLI. But I'm not convident enough to say that this would behave better than Ubiquity's original implementation, which does use the computer name option. --- subiquity/server/ad_joiner.py | 50 ++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index 149579df..b59a21f3 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -66,22 +66,42 @@ class AdJoinStrategy(): "--unattended", info.domain_name], input=info.password) - if not cp.returncode: - # Enable pam_mkhomedir - try: - cp = await run_curtin_command(self.app, context, - "in-target", "-t", root_dir, - "--", self.pam, "--package", - "--enable", "mkhomedir", - private_mounts=False) + if cp.returncode: + # Try again without the computer name. Lab tests shown more + # success in this setup, but I'm still not sure if we should + # drop the computer name attempt, since that's the way Ubiquity + # has been doing for ages. + log.debug("Joining operation failed:") + log.debug(cp.stderr) + log.debug(cp.stdout) + log.debug("Trying again without overriding the computer name:") + cp = await arun_command([self.realm, "join", "--install", + root_dir, "--user", info.admin_name, + "--unattended", info.domain_name], + input=info.password) - return AdJoinResult.OK - except CalledProcessError: - # The app command runner doesn't give us output in case of - # failure in the wait() method, which is called by - # run_curtin_command - log.info("Failed to update pam-auth") - return AdJoinResult.PAM_ERROR + if cp.returncode: + log.debug("Joining operation failed:") + log.debug(cp.stderr) + log.debug(cp.stdout) + return AdJoinResult.JOIN_ERROR + + # Enable pam_mkhomedir + try: + # The function raises if the process fail. + await run_curtin_command(self.app, context, + "in-target", "-t", root_dir, + "--", self.pam, "--package", + "--enable", "mkhomedir", + private_mounts=False) + + return AdJoinResult.OK + except CalledProcessError: + # The app command runner doesn't give us output in case of + # failure in the wait() method, which is called by + # run_curtin_command + log.info("Failed to update pam-auth") + return AdJoinResult.PAM_ERROR return AdJoinResult.JOIN_ERROR From babf0255b90c7346fce93e4a4bedbb5d86611b3f Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Sat, 25 Feb 2023 17:15:34 -0300 Subject: [PATCH 16/17] Add the AD model into the POSTINSTALL_MODEL_NAMES AD is by nature an optional feature. Yet, we need to make its model part of the POSTINSTALL_MODEL_NAMES set. That would turn this into a required controller. Thus, explicitly mark AD as configured For as long as TUI doesn't have a matching controller. --- subiquity/client/client.py | 2 ++ subiquity/server/server.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/subiquity/client/client.py b/subiquity/client/client.py index f177a986..a3f5a0a6 100644 --- a/subiquity/client/client.py +++ b/subiquity/client/client.py @@ -461,6 +461,8 @@ class SubiquityClient(TuiApplication): "marking additional endpoints as configured: %s", needed) await self.client.meta.mark_configured.POST(list(needed)) + # TODO: remove this when TUI gets an Active Directory screen: + await self.client.meta.mark_configured.POST(['active_directory']) await self.client.meta.confirm.POST(self.our_tty) def add_global_overlay(self, overlay): diff --git a/subiquity/server/server.py b/subiquity/server/server.py index 9dd2de00..07b8c545 100644 --- a/subiquity/server/server.py +++ b/subiquity/server/server.py @@ -215,7 +215,7 @@ POSTINSTALL_MODEL_NAMES = ModelNames({ "ubuntu_pro", "userdata", }, - desktop={"timezone", "codecs"}) + desktop={"timezone", "codecs", "ad"}) class SubiquityServer(Application): From 33597e3c7df075e2efc705c55db7b6a5bd040c55 Mon Sep 17 00:00:00 2001 From: Carlos Nihelton Date: Wed, 1 Mar 2023 09:44:36 -0300 Subject: [PATCH 17/17] Exception safe context manager Without the try/finally the cleanup could be skipped by an exception being raised. --- subiquity/server/ad_joiner.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/subiquity/server/ad_joiner.py b/subiquity/server/ad_joiner.py index b59a21f3..22ad2ac7 100644 --- a/subiquity/server/ad_joiner.py +++ b/subiquity/server/ad_joiner.py @@ -34,11 +34,13 @@ def hostname_context(hostname: str): back in the end of the caller scope. """ hostname_current = gethostname() hostname_process = run_command(['hostname', hostname]) - yield hostname_process - # Restoring the live session hostname. - hostname_process = run_command(['hostname', hostname_current]) - if hostname_process.returncode: - log.info("Failed to restore live session hostname") + try: + yield hostname_process + finally: + # Restoring the live session hostname. + hostname_process = run_command(['hostname', hostname_current]) + if hostname_process.returncode: + log.info("Failed to restore live session hostname") class AdJoinStrategy():