From f43ac8097beacefcb4ea3b212004cc4e1e2a67e5 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 10 Sep 2024 12:30:08 +1200 Subject: [PATCH] feat: expand the secret ID out to the full URI when only given the ID (#1358) Adjust the rules for getting the 'canonical' form of the Secret URI: * If the passed ID starts with `secret:`, leave it as-is (unchanged) * Otherwise, if the model UUID is unknown, add the `secret:` prefix to end up with the form `secret:id` (unchanged) * Otherwise, change to the full `secret://{model UUID}/{secret ID` form (new) Also adjust the code that calls the canonicalization [sic] method so that the model UUID is always available in regular ops use. If someone is creating `SecretInfo` objects themselves then a `None` value for the UUID is the default, for backwards compatibility (but a warning is issued in that case). Fixes #1312. --- ops/model.py | 35 +++++++++---- ops/testing.py | 28 ++++++++-- test/test_model.py | 122 ++++++++++++++++++++++++++++--------------- test/test_testing.py | 28 ++++++++++ 4 files changed, 159 insertions(+), 54 deletions(-) diff --git a/ops/model.py b/ops/model.py index 9102f30d0..336ef5aaa 100644 --- a/ops/model.py +++ b/ops/model.py @@ -30,6 +30,7 @@ import tempfile import time import typing +import warnings import weakref from abc import ABC, abstractmethod from pathlib import Path, PurePath @@ -295,7 +296,7 @@ def get_secret(self, *, id: Optional[str] = None, label: Optional[str] = None) - raise TypeError('Must provide an id or label, or both') if id is not None: # Canonicalize to "secret:" form for consistency in backend calls. - id = Secret._canonicalize_id(id) + id = Secret._canonicalize_id(id, self.uuid) content = self._backend.secret_get(id=id, label=label) return Secret(self._backend, id=id, label=label, content=content) @@ -1182,8 +1183,17 @@ def __init__( rotation: Optional[SecretRotate], rotates: Optional[datetime.datetime], description: Optional[str] = None, + *, + model_uuid: Optional[str] = None, ): - self.id = Secret._canonicalize_id(id) + if model_uuid is None: + warnings.warn( + '`model_uuid` should always be provided when creating a ' + 'SecretInfo instance, and will be required in the future.', + DeprecationWarning, + stacklevel=2, + ) + self.id = Secret._canonicalize_id(id, model_uuid) self.label = label self.revision = revision self.expires = expires @@ -1192,7 +1202,9 @@ def __init__( self.description = description @classmethod - def from_dict(cls, id: str, d: Dict[str, Any]) -> 'SecretInfo': + def from_dict( + cls, id: str, d: Dict[str, Any], model_uuid: Optional[str] = None + ) -> 'SecretInfo': """Create new SecretInfo object from ID and dict parsed from JSON.""" expires = typing.cast(Optional[str], d.get('expiry')) try: @@ -1208,6 +1220,7 @@ def from_dict(cls, id: str, d: Dict[str, Any]) -> 'SecretInfo': rotation=rotation, rotates=timeconv.parse_rfc3339(rotates) if rotates is not None else None, description=typing.cast(Optional[str], d.get('description')), + model_uuid=model_uuid, ) def __repr__(self): @@ -1249,7 +1262,7 @@ def __init__( if not (id or label): raise TypeError('Must provide an id or label, or both') if id is not None: - id = self._canonicalize_id(id) + id = self._canonicalize_id(id, backend.model_uuid) self._backend = backend self._id = id self._label = label @@ -1264,11 +1277,13 @@ def __repr__(self): return f"" @staticmethod - def _canonicalize_id(id: str) -> str: + def _canonicalize_id(id: str, model_uuid: Optional[str]) -> str: """Return the canonical form of the given secret ID, with the 'secret:' prefix.""" id = id.strip() if not id.startswith('secret:'): - id = f'secret:{id}' # add the prefix if not there already + # Add the prefix and, if provided, model UUID. + id = f'secret:{id}' if model_uuid is None else f'secret://{model_uuid}/{id}' + return id @classmethod @@ -1308,8 +1323,8 @@ def id(self) -> Optional[str]: """Locator ID (URI) for this secret. This has an unfortunate name for historical reasons, as it's not - really a unique identifier, but the secret's locator URI, which may or - may not include the model UUID (for cross-model secrets). + really a unique identifier, but the secret's locator URI, which will + include the model UUID (for cross-model secrets). Charms should treat this as an opaque string for looking up secrets and sharing them via relation data. If a charm-local "name" is needed @@ -3604,7 +3619,9 @@ def secret_info_get( result = self._run_for_secret('secret-info-get', *args, return_output=True, use_json=True) info_dicts = typing.cast(Dict[str, Any], result) id = list(info_dicts)[0] # Juju returns dict of {secret_id: {info}} - return SecretInfo.from_dict(id, typing.cast(Dict[str, Any], info_dicts[id])) + return SecretInfo.from_dict( + id, typing.cast(Dict[str, Any], info_dicts[id]), model_uuid=self.model_uuid + ) def secret_set( self, diff --git a/ops/testing.py b/ops/testing.py index 9c4d7c9a1..dcabdf346 100644 --- a/ops/testing.py +++ b/ops/testing.py @@ -24,6 +24,7 @@ import os import pathlib import random +import re import shutil import signal import tempfile @@ -2702,7 +2703,7 @@ def planned_units(self) -> int: return len(units) + 1 # Account for this unit. def _get_secret(self, id: str) -> Optional[_Secret]: - return next((s for s in self._secrets if s.id == id), None) + return next((s for s in self._secrets if self._secret_ids_are_equal(s.id, id)), None) def _ensure_secret(self, id: str) -> _Secret: secret = self._get_secret(id) @@ -2724,6 +2725,22 @@ def _ensure_secret_id_or_label(self, id: Optional[str], label: Optional[str]): ) return secret + def _secret_ids_are_equal(self, id1: str, id2: str) -> bool: + secret_re = re.compile( + r'^(?:secret:)?(?://)?(?:(?P[a-z0-9-]+)/)?(?P[a-z0-9-]+)$', re.IGNORECASE + ) + mo = secret_re.match(id1) + if not mo: + return False + model_uuid1 = mo.group('uuid') or self.model_uuid + id1 = mo.group('id') + mo = secret_re.match(id2) + if not mo: + return False + model_uuid2 = mo.group('uuid') or self.model_uuid + id2 = mo.group('id') + return model_uuid1 == model_uuid2 and id1 == id2 + def secret_get( self, *, @@ -2816,6 +2833,7 @@ def secret_info_get( rotation=rotation, rotates=rotates, description=secret.description, + model_uuid=self.model_uuid, ) def secret_set( @@ -2899,7 +2917,7 @@ def _secret_add( description=description, ) self._secrets.append(secret) - return id + return id # Note that this is the 'short' ID, not the canonicalised one. def secret_grant(self, id: str, relation_id: int, *, unit: Optional[str] = None) -> None: secret = self._ensure_secret(id) @@ -2936,9 +2954,11 @@ def secret_remove(self, id: str, *, revision: Optional[int] = None) -> None: secret.revisions = revisions else: # Last revision removed, remove entire secret - self._secrets = [s for s in self._secrets if s.id != id] + self._secrets = [ + s for s in self._secrets if not self._secret_ids_are_equal(s.id, id) + ] else: - self._secrets = [s for s in self._secrets if s.id != id] + self._secrets = [s for s in self._secrets if not self._secret_ids_are_equal(s.id, id)] def open_port(self, protocol: str, port: Optional[int] = None): self._check_protocol_and_port(protocol, port) diff --git a/test/test_model.py b/test/test_model.py index c8266dc90..f8af18404 100644 --- a/test/test_model.py +++ b/test/test_model.py @@ -3482,11 +3482,13 @@ def test_get_secret_id(self, fake_script: FakeScript, model: ops.Model): fake_script.write('secret-get', """echo '{"foo": "g"}'""") secret = model.get_secret(id='123') - assert secret.id == 'secret:123' + assert secret.id == f'secret://{model._backend.model_uuid}/123' assert secret.label is None assert secret.get_content() == {'foo': 'g'} - assert fake_script.calls(clear=True) == [['secret-get', 'secret:123', '--format=json']] + assert fake_script.calls(clear=True) == [ + ['secret-get', f'secret://{model._backend.model_uuid}/123', '--format=json'] + ] def test_get_secret_label(self, fake_script: FakeScript, model: ops.Model): fake_script.write('secret-get', """echo '{"foo": "g"}'""") @@ -3502,12 +3504,18 @@ def test_get_secret_id_and_label(self, fake_script: FakeScript, model: ops.Model fake_script.write('secret-get', """echo '{"foo": "h"}'""") secret = model.get_secret(id='123', label='l') - assert secret.id == 'secret:123' + assert secret.id == f'secret://{model._backend.model_uuid}/123' assert secret.label == 'l' assert secret.get_content() == {'foo': 'h'} assert fake_script.calls(clear=True) == [ - ['secret-get', 'secret:123', '--label', 'l', '--format=json'] + [ + 'secret-get', + f'secret://{model._backend.model_uuid}/123', + '--label', + 'l', + '--format=json', + ] ] def test_get_secret_no_args(self, model: ops.Model): @@ -3537,7 +3545,7 @@ def test_secret_unique_identifier(self, fake_script: FakeScript, model: ops.Mode assert secret.unique_identifier is None secret = model.get_secret(id='123') - assert secret.id == 'secret:123' + assert secret.id == f'secret://{model._backend.model_uuid}/123' assert secret.unique_identifier == '123' secret = model.get_secret(id='secret:124') @@ -3550,7 +3558,7 @@ def test_secret_unique_identifier(self, fake_script: FakeScript, model: ops.Mode assert fake_script.calls(clear=True) == [ ['secret-get', '--label', 'lbl', '--format=json'], - ['secret-get', 'secret:123', '--format=json'], + ['secret-get', f'secret://{model._backend.model_uuid}/123', '--format=json'], ['secret-get', 'secret:124', '--format=json'], ['secret-get', 'secret://modeluuid/125', '--format=json'], ] @@ -3566,8 +3574,9 @@ def test_init(self): rotation=ops.SecretRotate.MONTHLY, rotates=datetime.datetime(2023, 1, 9, 14, 10, 0), description='desc', + model_uuid='abcd', ) - assert info.id == 'secret:3' + assert info.id == 'secret://abcd/3' assert info.label == 'lbl' assert info.revision == 7 assert info.expires == datetime.datetime(2022, 12, 9, 14, 10, 0) @@ -3590,6 +3599,7 @@ def test_from_dict(self): 'rotates': '2023-01-09T14:10:00Z', 'description': 'desc', }, + model_uuid='abcd', ) assert info.id == 'secret:4' assert info.label == 'fromdict' @@ -3600,14 +3610,15 @@ def test_from_dict(self): assert info.description == 'desc' info = ops.SecretInfo.from_dict( - 'secret:4', + '4', { 'label': 'fromdict', 'revision': 8, 'rotation': 'badvalue', }, + model_uuid='abcd', ) - assert info.id == 'secret:4' + assert info.id == 'secret://abcd/4' assert info.label == 'fromdict' assert info.revision == 8 assert info.expires is None @@ -3619,11 +3630,15 @@ def test_from_dict(self): assert info.id == 'secret:5' assert info.revision == 9 + info = ops.SecretInfo.from_dict('secret://abcd/6', {'revision': 9}) + assert info.id == 'secret://abcd/6' + assert info.revision == 9 + class TestSecretClass: @pytest.fixture def model(self): - return ops.Model(ops.CharmMeta(), _ModelBackend('myapp/0')) + return ops.Model(ops.CharmMeta(), _ModelBackend('myapp/0', model_uuid='abcd')) def make_secret( self, @@ -3636,11 +3651,11 @@ def make_secret( def test_id_and_label(self, model: ops.Model): secret = self.make_secret(model, id=' abc ', label='lbl') - assert secret.id == 'secret:abc' + assert secret.id == f'secret://{model._backend.model_uuid}/abc' assert secret.label == 'lbl' secret = self.make_secret(model, id='x') - assert secret.id == 'secret:x' + assert secret.id == f'secret://{model._backend.model_uuid}/x' assert secret.label is None secret = self.make_secret(model, label='y') @@ -3664,7 +3679,7 @@ def test_get_content_refresh(self, model: ops.Model, fake_script: FakeScript): assert content == {'foo': 'refreshed'} assert fake_script.calls(clear=True) == [ - ['secret-get', 'secret:y', '--refresh', '--format=json'] + ['secret-get', f'secret://{model._backend.model_uuid}/y', '--refresh', '--format=json'] ] def test_get_content_uncached(self, model: ops.Model, fake_script: FakeScript): @@ -3674,7 +3689,9 @@ def test_get_content_uncached(self, model: ops.Model, fake_script: FakeScript): content = secret.get_content() assert content == {'foo': 'notcached'} - assert fake_script.calls(clear=True) == [['secret-get', 'secret:z', '--format=json']] + assert fake_script.calls(clear=True) == [ + ['secret-get', f'secret://{model._backend.model_uuid}/z', '--format=json'] + ] def test_get_content_copies_dict(self, model: ops.Model, fake_script: FakeScript): fake_script.write('secret-get', """echo '{"foo": "bar"}'""") @@ -3685,7 +3702,9 @@ def test_get_content_copies_dict(self, model: ops.Model, fake_script: FakeScript content['new'] = 'value' assert secret.get_content() == {'foo': 'bar'} - assert fake_script.calls(clear=True) == [['secret-get', 'secret:z', '--format=json']] + assert fake_script.calls(clear=True) == [ + ['secret-get', f'secret://{model._backend.model_uuid}/z', '--format=json'] + ] def test_peek_content(self, model: ops.Model, fake_script: FakeScript): fake_script.write('secret-get', """echo '{"foo": "peeked"}'""") @@ -3695,7 +3714,14 @@ def test_peek_content(self, model: ops.Model, fake_script: FakeScript): assert content == {'foo': 'peeked'} assert fake_script.calls(clear=True) == [ - ['secret-get', 'secret:a', '--label', 'b', '--peek', '--format=json'] + [ + 'secret-get', + f'secret://{model._backend.model_uuid}/a', + '--label', + 'b', + '--peek', + '--format=json', + ] ] def test_get_info(self, model: ops.Model, fake_script: FakeScript): @@ -3704,28 +3730,28 @@ def test_get_info(self, model: ops.Model, fake_script: FakeScript): # Secret with ID only secret = self.make_secret(model, id='x') info = secret.get_info() - assert info.id == 'secret:x' + assert info.id == f'secret://{model._backend.model_uuid}/x' assert info.label == 'y' assert info.revision == 7 # Secret with label only secret = self.make_secret(model, label='y') info = secret.get_info() - assert info.id == 'secret:x' + assert info.id == f'secret://{model._backend.model_uuid}/x' assert info.label == 'y' assert info.revision == 7 # Secret with ID and label secret = self.make_secret(model, id='x', label='y') info = secret.get_info() - assert info.id == 'secret:x' + assert info.id == f'secret://{model._backend.model_uuid}/x' assert info.label == 'y' assert info.revision == 7 assert fake_script.calls(clear=True) == [ - ['secret-info-get', 'secret:x', '--format=json'], + ['secret-info-get', f'secret://{model._backend.model_uuid}/x', '--format=json'], ['secret-info-get', '--label', 'y', '--format=json'], - ['secret-info-get', 'secret:x', '--format=json'], + ['secret-info-get', f'secret://{model._backend.model_uuid}/x', '--format=json'], ] def test_set_content(self, model: ops.Model, fake_script: FakeScript): @@ -3739,15 +3765,15 @@ def test_set_content(self, model: ops.Model, fake_script: FakeScript): secret = self.make_secret(model, label='y') assert secret.id is None secret.set_content({'bar': 'foo'}) - assert secret.id == 'secret:z' + assert secret.id == f'secret://{model._backend.model_uuid}/z' with pytest.raises(ValueError): secret.set_content({'s': 't'}) # ensure it validates content (key too short) assert fake_script.calls(clear=True) == [ - ['secret-set', 'secret:x', ANY], + ['secret-set', f'secret://{model._backend.model_uuid}/x', ANY], ['secret-info-get', '--label', 'y', '--format=json'], - ['secret-set', 'secret:z', ANY], + ['secret-set', f'secret://{model._backend.model_uuid}/z', ANY], ] assert fake_script.secrets() == {'foo': 'bar', 'bar': 'foo'} @@ -3768,12 +3794,12 @@ def test_set_info(self, model: ops.Model, fake_script: FakeScript): secret = self.make_secret(model, label='y') assert secret.id is None secret.set_info(label='lbl') - assert secret.id == 'secret:z' + assert secret.id == f'secret://{model._backend.model_uuid}/z' assert fake_script.calls(clear=True) == [ [ 'secret-set', - 'secret:x', + f'secret://{model._backend.model_uuid}/x', '--label', 'lab', '--description', @@ -3784,7 +3810,7 @@ def test_set_info(self, model: ops.Model, fake_script: FakeScript): 'monthly', ], ['secret-info-get', '--label', 'y', '--format=json'], - ['secret-set', 'secret:z', '--label', 'lbl'], + ['secret-set', f'secret://{model._backend.model_uuid}/z', '--label', 'lbl'], ] with pytest.raises(TypeError): @@ -3811,16 +3837,23 @@ def test_grant(self, model: ops.Model, fake_script: FakeScript): assert secret.id is None rel345 = ops.Relation('test', 345, True, unit, backend, cache) secret.grant(rel345) - assert secret.id == 'secret:z' + assert secret.id == f'secret://{model._backend.model_uuid}/z' assert fake_script.calls(clear=True) == [ ['relation-list', '-r', '123', '--format=json'], ['relation-list', '-r', '234', '--format=json'], - ['secret-grant', 'secret:x', '--relation', '123'], - ['secret-grant', 'secret:x', '--relation', '234', '--unit', 'app/0'], + ['secret-grant', f'secret://{model._backend.model_uuid}/x', '--relation', '123'], + [ + 'secret-grant', + f'secret://{model._backend.model_uuid}/x', + '--relation', + '234', + '--unit', + 'app/0', + ], ['relation-list', '-r', '345', '--format=json'], ['secret-info-get', '--label', 'y', '--format=json'], - ['secret-grant', 'secret:z', '--relation', '345'], + ['secret-grant', f'secret://{model._backend.model_uuid}/z', '--relation', '345'], ] def test_revoke(self, model: ops.Model, fake_script: FakeScript): @@ -3841,16 +3874,23 @@ def test_revoke(self, model: ops.Model, fake_script: FakeScript): assert secret.id is None rel345 = ops.Relation('test', 345, True, unit, model._backend, model._cache) secret.revoke(rel345) - assert secret.id == 'secret:z' + assert secret.id == f'secret://{model._backend.model_uuid}/z' assert fake_script.calls(clear=True) == [ ['relation-list', '-r', '123', '--format=json'], ['relation-list', '-r', '234', '--format=json'], - ['secret-revoke', 'secret:x', '--relation', '123'], - ['secret-revoke', 'secret:x', '--relation', '234', '--unit', 'app/0'], + ['secret-revoke', f'secret://{model._backend.model_uuid}/x', '--relation', '123'], + [ + 'secret-revoke', + f'secret://{model._backend.model_uuid}/x', + '--relation', + '234', + '--unit', + 'app/0', + ], ['relation-list', '-r', '345', '--format=json'], ['secret-info-get', '--label', 'y', '--format=json'], - ['secret-revoke', 'secret:z', '--relation', '345'], + ['secret-revoke', f'secret://{model._backend.model_uuid}/z', '--relation', '345'], ] def test_remove_revision(self, model: ops.Model, fake_script: FakeScript): @@ -3864,12 +3904,12 @@ def test_remove_revision(self, model: ops.Model, fake_script: FakeScript): secret = self.make_secret(model, label='y') assert secret.id is None secret.remove_revision(234) - assert secret.id == 'secret:z' + assert secret.id == f'secret://{model._backend.model_uuid}/z' assert fake_script.calls(clear=True) == [ - ['secret-remove', 'secret:x', '--revision', '123'], + ['secret-remove', f'secret://{model._backend.model_uuid}/x', '--revision', '123'], ['secret-info-get', '--label', 'y', '--format=json'], - ['secret-remove', 'secret:z', '--revision', '234'], + ['secret-remove', f'secret://{model._backend.model_uuid}/z', '--revision', '234'], ] def test_remove_all_revisions(self, model: ops.Model, fake_script: FakeScript): @@ -3883,12 +3923,12 @@ def test_remove_all_revisions(self, model: ops.Model, fake_script: FakeScript): secret = self.make_secret(model, label='y') assert secret.id is None secret.remove_all_revisions() - assert secret.id == 'secret:z' + assert secret.id == f'secret://{model._backend.model_uuid}/z' assert fake_script.calls(clear=True) == [ - ['secret-remove', 'secret:x'], + ['secret-remove', f'secret://{model._backend.model_uuid}/x'], ['secret-info-get', '--label', 'y', '--format=json'], - ['secret-remove', 'secret:z'], + ['secret-remove', f'secret://{model._backend.model_uuid}/z'], ] diff --git a/test/test_testing.py b/test/test_testing.py index d49a64bb6..e9e34f34d 100644 --- a/test/test_testing.py +++ b/test/test_testing.py @@ -6161,6 +6161,34 @@ def test_user_secret_permissions(self, request: pytest.FixtureRequest): with pytest.raises(RuntimeError): secret.remove_all_revisions() + def test_secret_id_variants(self, request: pytest.FixtureRequest): + harness = ops.testing.Harness( + ops.CharmBase, meta='name: webapp\nrequires:\n db:\n interface: database' + ) + request.addfinalizer(harness.cleanup) + harness.add_relation('db', 'database') + harness.begin() + + # Local (app and unit) secrets, secrets that belong to other apps, + # and user secrets should have uniform ID behaviour. + app_secret = harness.model.app.add_secret({'password': '1234'}) + unit_secret = harness.model.unit.add_secret({'password': '5678'}) + remote_secret_id = harness.add_model_secret('database', {'password': 'abcd'}) + harness.grant_secret(remote_secret_id, 'webapp') + remote_secret = harness.model.get_secret(id=remote_secret_id) + user_secret_id = harness.add_user_secret({'password': 'efgh'}) + harness.grant_secret(user_secret_id, 'webapp') + user_secret = harness.model.get_secret(id=user_secret_id) + + # Ensure that all three variants of the secret ID can be used to + # retrieve the secret from Harness. + for secret in (app_secret, unit_secret, remote_secret, user_secret): + id_only = secret.unique_identifier + id_with_prefix = f'secret:{id_only}' + id_with_model = f'secret://{harness.model.uuid}/{id_only}' + for uri in (id_only, id_with_prefix, id_with_model): + assert harness.model.get_secret(id=uri).get_content() == secret.get_content() + class EventRecorder(ops.CharmBase): def __init__(self, framework: ops.Framework):