sts-sponsors team mailing list archive

[Igor Brovtsin <mp+433629@xxxxxxxxxxxxxxxxxx>]

Igor Brovtsin has proposed merging ~igor-brovtsin/maas:vault-startup-clear-caches-early into maas:master.

Commit message:
fix(vault): Clean Vault caches before fetching RPC shared secret
    
Otherwise, RPC secret will be fetched from the DB after migration to Vault
(until `regiond` process is restarted). Since there is no RPC secret in the DB
after migration, region will generate a new secret, preventing clients access.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~igor-brovtsin/maas/+git/maas/+merge/433629
-- 
Your team MAAS Maintainers is requested to review the proposed merge of ~igor-brovtsin/maas:vault-startup-clear-caches-early into maas:master.

diff --git a/src/maasserver/start_up.py b/src/maasserver/start_up.py
index 135a3a6..c572b1f 100644
--- a/src/maasserver/start_up.py
+++ b/src/maasserver/start_up.py
@@ -120,6 +120,11 @@ def start_up(master=False):
     """
     while True:
         try:
+            # Since start_up now can be called multiple times in a process lifetime,
+            # vault client caches should be cleared in order to re-read the configuration.
+            # This prevents fetching shared secret from DB when Vault is already enabled.
+            clear_vault_client_caches()
+
             # Ensure the shared secret is configured
             secret = yield security.get_shared_secret()
             MAAS_SECRET.set(secret)
@@ -195,9 +200,6 @@ def inner_start_up(master=False):
     # Only perform the following if the master process for the
     # region controller.
     if master:
-        # Since start_up now can be called multiple times in a process lifetime,
-        # vault client caches should be cleared in order to re-read the configuration.
-        clear_vault_client_caches()
         # Migrate DB credentials to Vault and set the flag if Vault client is configured
         client = get_region_vault_client()
         if client is not None:
diff --git a/src/maasserver/tests/test_start_up.py b/src/maasserver/tests/test_start_up.py
index 7391da7..a49c0ac 100644
--- a/src/maasserver/tests/test_start_up.py
+++ b/src/maasserver/tests/test_start_up.py
@@ -7,13 +7,14 @@ from unittest.mock import call, MagicMock
 
 from testtools.matchers import HasLength
 
-from maasserver import deprecations, eventloop, locks, start_up
+from maasserver import deprecations, eventloop, locks, start_up, vault
 from maasserver.config import RegionConfiguration
 from maasserver.models.config import Config
 from maasserver.models.controllerinfo import ControllerInfo
 from maasserver.models.node import RegionController
 from maasserver.models.notification import Notification
 from maasserver.models.signals import bootsources
+from maasserver.secrets import SecretManager
 from maasserver.start_up import migrate_db_credentials_if_necessary
 from maasserver.testing.config import RegionConfigurationFixture
 from maasserver.testing.eventloop import RegionEventLoopFixture
@@ -22,6 +23,7 @@ from maasserver.testing.testcase import (
     MAASServerTestCase,
     MAASTransactionServerTestCase,
 )
+from maasserver.testing.vault import FakeVaultClient
 from maasserver.utils.orm import post_commit_hooks
 from maasserver.vault import UnknownSecretPath, VaultError
 from maastesting import get_testing_timeout
@@ -32,6 +34,7 @@ from maastesting.matchers import (
 )
 from provisioningserver.config import ConfigurationFile
 from provisioningserver.drivers.osystem.ubuntu import UbuntuOS
+from provisioningserver.security import to_hex
 from provisioningserver.utils import ipaddr
 from provisioningserver.utils.deb import DebVersionsInfo
 from provisioningserver.utils.env import (
@@ -96,6 +99,30 @@ class TestStartUp(MAASTransactionServerTestCase):
         # It also slept once, for 3 seconds, between those attempts.
         self.expectThat(start_up.pause, MockCalledOnceWith(3.0))
 
+    def test_start_up_fetches_secret_from_vault_after_migration(self):
+        vault.clear_vault_client_caches()
+        # Prepare fake vault
+        expected_shared_secret = b"EXPECTED"
+        client = FakeVaultClient()
+        self.patch(vault, "_get_region_vault_client").return_value = client
+        SecretManager(client).set_simple_secret(
+            "rpc-shared", to_hex(expected_shared_secret)
+        )
+        # Cache vault being disabled
+        Config.objects.set_config("vault_enabled", False)
+        assert vault.get_region_vault_client_if_enabled() is None
+        # Enable vault as if migration was finished before the call
+        Config.objects.set_config("vault_enabled", True)
+        # No need to do actual startup
+        self.patch(start_up, "inner_start_up")
+        self.patch(start_up, "pause")
+
+        with post_commit_hooks:
+            start_up.start_up()
+
+        print(MAAS_SECRET.get(), expected_shared_secret)
+        assert MAAS_SECRET.get() == expected_shared_secret
+
 
 class TestInnerStartUp(MAASServerTestCase):
     def setUp(self):
@@ -247,12 +274,6 @@ class TestInnerStartUp(MAASServerTestCase):
         self.assertEqual(region.version, "1:3.1.0-1234-g.deadbeef")
         self.assertEqual(region.info.install_type, "deb")
 
-    def test_clears_vault_cache(self):
-        clear_caches_mock = self.patch(start_up, "clear_vault_client_caches")
-        with post_commit_hooks:
-            start_up.inner_start_up(master=True)
-        clear_caches_mock.assert_called_once()
-
     def test_sets_vault_flag_disabled(self):
         self.patch(start_up, "get_region_vault_client").return_value = None
         with post_commit_hooks: