diff --git a/changelog/45.fixed.md b/changelog/45.fixed.md new file mode 100644 index 00000000..363cfb87 --- /dev/null +++ b/changelog/45.fixed.md @@ -0,0 +1 @@ +Fixed a crash when renewing/revoking leases that have been revoked on the Vault server early diff --git a/changelog/46.added.md b/changelog/46.added.md new file mode 100644 index 00000000..8f8b298b --- /dev/null +++ b/changelog/46.added.md @@ -0,0 +1 @@ +Added an optional switch for validating cached leases with the Vault server before returning them from the LeaseStore diff --git a/changelog/47.added.md b/changelog/47.added.md new file mode 100644 index 00000000..48637147 --- /dev/null +++ b/changelog/47.added.md @@ -0,0 +1 @@ +Implemented setting per-lease defaults of lifecycle parameters diff --git a/changelog/48.added.md b/changelog/48.added.md new file mode 100644 index 00000000..1452dc0d --- /dev/null +++ b/changelog/48.added.md @@ -0,0 +1 @@ +Implemented caching arbitrary metadata together with a lease and included it in expiry events diff --git a/changelog/49.added.md b/changelog/49.added.md new file mode 100644 index 00000000..9b8eb643 --- /dev/null +++ b/changelog/49.added.md @@ -0,0 +1 @@ +Added a LeaseStore method for listing cached lease information diff --git a/src/saltext/vault/utils/vault/cache.py b/src/saltext/vault/utils/vault/cache.py index ef9a8a8a..767cc29a 100644 --- a/src/saltext/vault/utils/vault/cache.py +++ b/src/saltext/vault/utils/vault/cache.py @@ -286,7 +286,7 @@ def _check_validity(self, lease_data, valid_for=0): log.debug("Using cached lease.") return lease if self.expire_events is not None: - raise VaultLeaseExpired() + raise VaultLeaseExpired(lease) return None @@ -294,6 +294,7 @@ class VaultLeaseCache(LeaseCacheMixin, CommonCache): """ Handles caching of Vault leases. Supports multiple cache keys. Checks whether cached leases are still valid before returning. + Does not enforce for per-lease ``min_ttl``. """ def get(self, ckey, valid_for=0, flush=True): @@ -306,14 +307,16 @@ def get(self, ckey, valid_for=0, flush=True): return data try: ret = self._check_validity(data, valid_for=valid_for) - except VaultLeaseExpired: + except VaultLeaseExpired as err: if self.expire_events is not None: self.expire_events( tag=f"vault/lease/{ckey}/expire", data={ "valid_for_less": valid_for if valid_for is not None - else data.get("min_ttl") or 0, + else err.lease.min_ttl or 0, + "ttl_left": err.lease.ttl_left, + "meta": err.lease.meta, }, ) ret = None diff --git a/src/saltext/vault/utils/vault/exceptions.py b/src/saltext/vault/utils/vault/exceptions.py index 0517e6eb..b439f7a2 100644 --- a/src/saltext/vault/utils/vault/exceptions.py +++ b/src/saltext/vault/utils/vault/exceptions.py @@ -15,6 +15,10 @@ class VaultLeaseExpired(VaultException): Raised when a cached lease is reported to be expired locally. """ + def __init__(self, lease): + super().__init__() + self.lease = lease + class VaultAuthExpired(VaultException): """ diff --git a/src/saltext/vault/utils/vault/leases.py b/src/saltext/vault/utils/vault/leases.py index b048d323..11d34bac 100644 --- a/src/saltext/vault/utils/vault/leases.py +++ b/src/saltext/vault/utils/vault/leases.py @@ -76,6 +76,10 @@ def is_valid_for(self, valid_for=0, blur=0): return True return abs(delta) <= blur + @property + def ttl_left(self): + return max(self.expire_time - round(time.time()), 0) + class UseCountMixin: """ @@ -171,6 +175,26 @@ def to_dict(self): class VaultLease(BaseLease): """ Data object representing a Vault lease. + + Optional parameters in addition to the required``lease_id`` and ``data``: + + min_ttl + When requesting this lease from the LeaseStore, ensure it is + valid for at least this amount of time, even if the + passed ``valid_for`` parameter is less. + + renew_increment + When renewing this lease, instead of the lease's default TTL, + default to this increment. + + revoke_delay + When revoking this lease, instead of the default value of 60, + default to this amount of time before having the Vault server + revoke it. + + meta + Cache arbitrary metadata together with the lease. It will + be included in expiry events. """ def __init__( @@ -185,10 +209,11 @@ def __init__( ): # save lease-associated data self.data = data - # save metadata used by the engine and beacon modules + # lifecycle default parameters self.min_ttl = min_ttl self.renew_increment = renew_increment self.revoke_delay = revoke_delay + # metadata that is included in expiry events self.meta = meta super().__init__(lease_id, **kwargs) @@ -434,17 +459,14 @@ def get( "When renew_increment is set, it must be at least valid_for to make sense" ) - def check_revoke(lease): + def check_revoke(lease, min_valid, validity_override=None): if self.expire_events is not None: - self.expire_events( - tag=f"vault/lease/{ckey}/expire", - data={ - "valid_for_less": valid_for - if valid_for is not None - else (lease.min_ttl or 0), - "meta": lease.meta, - }, - ) + event_data = { + "valid_for_less": round(min_valid), + "ttl": validity_override if validity_override is not None else lease.ttl_left, + "meta": lease.meta, + } + self.expire_events(tag=f"vault/lease/{ckey}/expire", data=event_data) if revoke is None or revoke: self.revoke(lease, delta=revoke) return None @@ -473,29 +495,29 @@ def check_revoke(lease): # TODO: Save the updated info? self.lookup(lease) except VaultNotFoundError: - return check_revoke(lease) + return check_revoke(lease, effective_min_validity, 0) return lease if not renew: - return check_revoke(lease) + return check_revoke(lease, effective_min_validity) try: lease = self.renew(lease, increment=renew_increment, raise_all_errors=False) except VaultNotFoundError: # The cached lease was already revoked - return check_revoke(lease) + return check_revoke(lease, effective_min_validity, 0) if not lease.is_valid_for(effective_min_validity, blur=renew_blur): if renew_increment is not None: # valid_for cannot possibly be respected - return check_revoke(lease) + return check_revoke(lease, effective_min_validity) # Maybe valid_for is greater than the default validity period, so check if # the lease can be renewed by valid_for try: lease = self.renew(lease, increment=effective_min_validity, raise_all_errors=False) except VaultNotFoundError: # The cached lease was already revoked - return check_revoke(lease) + return check_revoke(lease, effective_min_validity, 0) if not lease.is_valid_for(effective_min_validity, blur=renew_blur): - return check_revoke(lease) + return check_revoke(lease, effective_min_validity) return lease def list(self): diff --git a/tests/unit/utils/vault/test_cache.py b/tests/unit/utils/vault/test_cache.py index f7937c44..988e74d8 100644 --- a/tests/unit/utils/vault/test_cache.py +++ b/tests/unit/utils/vault/test_cache.py @@ -614,8 +614,10 @@ def test_store(self, lease): cache.store("ckey", lease_) store.assert_called_once_with("ckey", lease_.to_dict()) + @pytest.mark.parametrize("lease", ({}, {"meta": {"foo": "bar"}}), indirect=True) + @pytest.mark.usefixtures("cached_outdated") def test_expire_events_with_get( - self, events, cached_outdated, cbank, ckey, lease + self, events, lease ): # pylint: disable=unused-argument, disable-msg=too-many-arguments """ Ensure internal flushing is disabled when the object is initialized @@ -624,4 +626,7 @@ def test_expire_events_with_get( cache = vcache.VaultLeaseCache({}, "cbank", expire_events=events) ret = cache.get("ckey", 10) assert ret is None - events.assert_called_once_with(tag="vault/lease/ckey/expire", data={"valid_for_less": 10}) + events.assert_called_once_with( + tag="vault/lease/ckey/expire", + data={"valid_for_less": 10, "ttl_left": 6, "meta": lease["meta"]}, + ) diff --git a/tests/unit/utils/vault/test_leases.py b/tests/unit/utils/vault/test_leases.py index 3d7fa85c..c988eac6 100644 --- a/tests/unit/utils/vault/test_leases.py +++ b/tests/unit/utils/vault/test_leases.py @@ -307,7 +307,10 @@ def test_get_valid_renew_min_ttl_on_lease_undercut( ) ) store_valid.cache.flush.assert_called_once_with("test") - store_valid.expire_events.assert_called_once() + store_valid.expire_events.assert_called_once_with( + tag="vault/lease/test/expire", + data={"valid_for_less": 3000, "ttl": 2000, "meta": lease["meta"]}, + ) @pytest.mark.parametrize("lease", ({"renew_increment": 2005},), indirect=True) def test_get_valid_renew_lease_default_period(self, store_valid, lease): @@ -371,7 +374,7 @@ def test_get_valid_renew_increment_insufficient(self, store_valid, lease): ) store_valid.cache.flush.assert_called_once_with("test") store_valid.expire_events.assert_called_once_with( - tag="vault/lease/test/expire", data={"valid_for_less": 2100, "meta": None} + tag="vault/lease/test/expire", data={"valid_for_less": 2100, "ttl": 2000, "meta": None} ) @pytest.mark.parametrize("lease", ({"min_ttl": 2100},), indirect=True) @@ -442,11 +445,13 @@ def test_get_valid_renew_valid_for( store_valid.cache.store.assert_called_with("test", ret) store_valid.expire_events.assert_not_called() + @pytest.mark.parametrize("lease", ({}, {"meta": {"foo": "bar"}}), indirect=True) def test_get_valid_not_renew(self, store_valid, lease): """ Currently valid leases should not be returned if they undercut valid_for. By default, revocation should be attempted and the cache - should be flushed. If an event factory was passed, an event should be sent. + should be flushed. If an event factory was passed, an event should be sent + which includes the per-lease metadata. """ ret = store_valid.get("test", valid_for=2000, renew=False) assert ret is None @@ -456,7 +461,8 @@ def test_get_valid_not_renew(self, store_valid, lease): ) store_valid.cache.flush.assert_called_once_with("test") store_valid.expire_events.assert_called_once_with( - tag="vault/lease/test/expire", data={"valid_for_less": 2000, "meta": None} + tag="vault/lease/test/expire", + data={"valid_for_less": 2000, "ttl": 1337, "meta": lease["meta"]}, ) @pytest.mark.parametrize( @@ -476,7 +482,10 @@ def test_get_valid_revoke(self, store_valid, lease, revoke): payload={"lease_id": lease["id"], "increment": revoke or lease["revoke_delay"] or 60}, ) store_valid.cache.flush.assert_called_once_with("test") - store_valid.expire_events.assert_called() + store_valid.expire_events.assert_called_once_with( + tag="vault/lease/test/expire", + data={"valid_for_less": 3500, "ttl": 2000, "meta": lease["meta"]}, + ) def test_get_valid_not_flush(self, store_valid): """ @@ -490,7 +499,7 @@ def test_get_valid_not_flush(self, store_valid): store_valid.client.post.assert_not_called() store_valid.cache.store.assert_not_called() store_valid.expire_events.assert_called_once_with( - tag="vault/lease/test/expire", data={"valid_for_less": 2000, "meta": None} + tag="vault/lease/test/expire", data={"valid_for_less": 2000, "ttl": 1337, "meta": None} ) @pytest.mark.parametrize("check_server", (False, True)) @@ -534,7 +543,10 @@ def test_get_check_server( ) ) store_valid.cache.flush.assert_called_once_with("test") - store_valid.expire_events.assert_called() + store_valid.expire_events.assert_called_once_with( + tag="vault/lease/test/expire", + data={"valid_for_less": 1300, "ttl": 0, "meta": lease["meta"]}, + ) else: store_valid.client.post.assert_not_called() store_valid.expire_events.assert_not_called() @@ -554,7 +566,9 @@ def test_get_renew_already_revoked(self, store_valid, on_call, lease_renewed_res ret = store_valid.get("test", valid_for=3000) assert ret is None store_valid.cache.flush.assert_called_once_with("test") - store_valid.expire_events.assert_called() + store_valid.expire_events.assert_called_once_with( + tag="vault/lease/test/expire", data={"valid_for_less": 3000, "ttl": 0, "meta": None} + ) @pytest.mark.parametrize("as_str", (False, True)) def test_revoke_already_revoked(self, store_valid, lease, as_str):