From 88605e4cb6e39fb781ddc49ab26bf8a224962d80 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 20 Mar 2018 12:46:30 +0100 Subject: [PATCH] Rotate non-renewable leases. We now support rotation of non-renewable leases in the sense that we expire the lease first and then obtain secrets from Vault again. Previously, non-renewable secrets were not rotated at all as rotation was tied to the renewal only and without renewal there was no rotation. See gh-215. --- .../core/lease/SecretLeaseContainer.java | 92 +++++++++++++------ .../lease/SecretLeaseContainerUnitTests.java | 46 +++++++++- 2 files changed, 106 insertions(+), 32 deletions(-) diff --git a/spring-vault-core/src/main/java/org/springframework/vault/core/lease/SecretLeaseContainer.java b/spring-vault-core/src/main/java/org/springframework/vault/core/lease/SecretLeaseContainer.java index 0d433660..253ecd01 100644 --- a/spring-vault-core/src/main/java/org/springframework/vault/core/lease/SecretLeaseContainer.java +++ b/spring-vault-core/src/main/java/org/springframework/vault/core/lease/SecretLeaseContainer.java @@ -69,7 +69,7 @@ import org.springframework.web.client.HttpStatusCodeException; * SecretLeaseContainer container = new SecretLeaseContainer(vaultOperations, * taskScheduler); * - * final RequestedSecret requestedSecret = container + * RequestedSecret requestedSecret = container * .requestRotatingSecret("mysql/creds/my-role"); * container.addLeaseListener(new LeaseListenerAdapter() { * @Override @@ -373,7 +373,13 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements lease = Lease.none(); } - potentiallyScheduleLeaseRenewal(requestedSecret, lease, renewalScheduler); + if (renewalScheduler.isLeaseRenewable(lease, requestedSecret)) { + scheduleLeaseRenewal(requestedSecret, lease, renewalScheduler); + } + else if (renewalScheduler.isLeaseRotateOnly(lease, requestedSecret)) { + scheduleLeaseRotation(requestedSecret, lease, renewalScheduler); + } + onSecretsObtained(requestedSecret, lease, secrets.getRequiredData()); } } @@ -470,34 +476,18 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements } } - void potentiallyScheduleLeaseRenewal(final RequestedSecret requestedSecret, - final Lease lease, final LeaseRenewalScheduler leaseRenewal) { + void scheduleLeaseRenewal(RequestedSecret requestedSecret, Lease lease, + LeaseRenewalScheduler leaseRenewal) { - if (!leaseRenewal.isLeaseRenewable(lease, requestedSecret)) { - return; - } + logRenewalCandidate(requestedSecret, lease, "renewal"); - if (log.isDebugEnabled()) { + leaseRenewal.scheduleRenewal(requestedSecret, leaseToRenew -> { - if (lease.hasLeaseId()) { - log.debug(String.format("Secret %s with Lease %s qualified for renewal", - requestedSecret.getPath(), lease.getLeaseId())); - } - else { - log.debug(String.format( - "Secret %s with cache hint is qualified for renewal", - requestedSecret.getPath())); - } - - } - - leaseRenewal.scheduleRenewal(requestedSecret, lease1 -> { - - Lease newLease = doRenewLease(requestedSecret, lease1); + Lease newLease = doRenewLease(requestedSecret, leaseToRenew); if (!Lease.none().equals(newLease)) { - potentiallyScheduleLeaseRenewal(requestedSecret, newLease, leaseRenewal); + scheduleLeaseRenewal(requestedSecret, newLease, leaseRenewal); onAfterLeaseRenewed(requestedSecret, newLease); } @@ -507,6 +497,35 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements } + void scheduleLeaseRotation(RequestedSecret requestedSecret, Lease lease, + LeaseRenewalScheduler leaseRenewal) { + + logRenewalCandidate(requestedSecret, lease, "rotation"); + + leaseRenewal.scheduleRenewal(requestedSecret, leaseToRotate -> { + + onLeaseExpired(requestedSecret, leaseToRotate); + + return Lease.none(); // rotation creates a new lease. + }, lease, getMinRenewal(), getExpiryThreshold()); + } + + private static void logRenewalCandidate(RequestedSecret requestedSecret, Lease lease, + String action) { + + if (log.isDebugEnabled()) { + + if (lease.hasLeaseId()) { + log.debug(String.format("Secret %s with Lease %s qualified for %s", + requestedSecret.getPath(), lease.getLeaseId(), action)); + } + else { + log.debug(String.format("Secret %s with cache hint is qualified for %s", + requestedSecret.getPath(), action)); + } + } + } + // ------------------------------------------------------------------------- // Implementation hooks and helper methods // ------------------------------------------------------------------------- @@ -539,7 +558,7 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements * @param lease the lease. * @return the new lease or {@literal null} if expired/secret cannot be rotated. */ - protected Lease doRenewLease(final RequestedSecret requestedSecret, final Lease lease) { + protected Lease doRenewLease(RequestedSecret requestedSecret, Lease lease) { try { Lease renewed = lease.hasLeaseId() ? renew(lease) : lease; @@ -572,7 +591,7 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements } @SuppressWarnings("unchecked") - private Lease renew(final Lease lease) { + private Lease renew(Lease lease) { ResponseEntity> entity = operations .doWithSession(restOperations -> (ResponseEntity) restOperations @@ -615,7 +634,7 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements * @param lease must not be {@literal null}. */ @SuppressWarnings("unchecked") - protected void doRevokeLease(RequestedSecret requestedSecret, final Lease lease) { + protected void doRevokeLease(RequestedSecret requestedSecret, Lease lease) { try { @@ -673,9 +692,8 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements * prevent too many renewals in a very short timeframe. * @param expiryThreshold duration to renew before {@link Lease}. */ - void scheduleRenewal(final RequestedSecret requestedSecret, - final RenewLease renewLease, final Lease lease, - final Duration minRenewal, final Duration expiryThreshold) { + void scheduleRenewal(RequestedSecret requestedSecret, RenewLease renewLease, + Lease lease, Duration minRenewal, Duration expiryThreshold) { if (log.isDebugEnabled()) { if (lease.hasLeaseId()) { @@ -723,6 +741,10 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements } try { + + // Renew lease may call scheduleRenewal(…) with a different lease + // Id to alter set up its own renewal schedule. If it's the old + // lease, then renewLease() outcome controls the current LeaseId. currentLeaseRef .compareAndSet(lease, renewLease.renewLease(lease)); } @@ -797,6 +819,16 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements public Lease getLease() { return currentLeaseRef.get(); } + + private boolean isLeaseRotateOnly(Lease lease, RequestedSecret requestedSecret) { + + if (lease == null) { + return false; + } + + return lease.hasLeaseId() && !lease.isRenewable() + && requestedSecret.getMode() == Mode.ROTATE; + } } /** diff --git a/spring-vault-core/src/test/java/org/springframework/vault/core/lease/SecretLeaseContainerUnitTests.java b/spring-vault-core/src/test/java/org/springframework/vault/core/lease/SecretLeaseContainerUnitTests.java index 50ce9670..4145cfab 100644 --- a/spring-vault-core/src/test/java/org/springframework/vault/core/lease/SecretLeaseContainerUnitTests.java +++ b/spring-vault-core/src/test/java/org/springframework/vault/core/lease/SecretLeaseContainerUnitTests.java @@ -16,6 +16,7 @@ package org.springframework.vault.core.lease; import java.time.Duration; +import java.util.ArrayList; import java.util.Collections; import java.util.Date; import java.util.HashMap; @@ -228,6 +229,43 @@ public class SecretLeaseContainerUnitTests { verify(taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class)); } + @Test + public void shouldRotateNonRenewableLease() { + + final List events = new ArrayList(); + when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn( + scheduledFuture); + + when(vaultOperations.read(requestedSecret.getPath())).thenReturn( + createSecrets("key", "value", false), + createSecrets("key", "value2", false)); + + secretLeaseContainer.addRequestedSecret(RequestedSecret.rotating(requestedSecret + .getPath())); + secretLeaseContainer.addLeaseListener(new LeaseListenerAdapter() { + @Override + public void onLeaseEvent(SecretLeaseEvent leaseEvent) { + events.add(leaseEvent); + } + }); + + secretLeaseContainer.start(); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Runnable.class); + verify(taskScheduler).schedule(captor.capture(), any(Trigger.class)); + + captor.getValue().run(); + verify(taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class)); + + assertThat(events).hasSize(3); + assertThat(events.get(0)).isInstanceOf(SecretLeaseCreatedEvent.class); + assertThat(events.get(1)).isInstanceOf(SecretLeaseExpiredEvent.class); + assertThat(events.get(2)).isInstanceOf(SecretLeaseCreatedEvent.class); + + SecretLeaseCreatedEvent rotated = (SecretLeaseCreatedEvent) events.get(2); + assertThat(rotated.getSecrets()).containsEntry("key", "value2"); + } + @Test public void shouldRotateGenericSecret() { @@ -534,13 +572,17 @@ public class SecretLeaseContainerUnitTests { } private VaultResponse createSecrets() { + return createSecrets("key", "value", true); + } + + private VaultResponse createSecrets(String key, String value, boolean renewable) { VaultResponse secrets = new VaultResponse(); secrets.setLeaseId("lease"); - secrets.setRenewable(true); + secrets.setRenewable(renewable); secrets.setLeaseDuration(100); - secrets.setData(Collections.singletonMap("key", (Object) "value")); + secrets.setData(Collections.singletonMap(key, (Object) value)); return secrets; }