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 e9c722e0..e2f68143 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 @@ -20,8 +20,8 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.Set; +import java.util.Map.Entry; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.ScheduledFuture; @@ -291,13 +291,6 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements } } - private static boolean isRotatingGenericSecret(RequestedSecret requestedSecret, - VaultResponseSupport> secrets) { - return Mode.ROTATE.equals(requestedSecret.getMode()) && !secrets.isRenewable() - && secrets.getLeaseDuration() > 0 - && "".equals(secrets.getLeaseId()); - } - private void start(RequestedSecret requestedSecret, LeaseRenewalScheduler renewalScheduler) { @@ -306,12 +299,15 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements if (secrets != null) { Lease lease; + if (StringUtils.hasText(secrets.getLeaseId())) { lease = Lease.of(secrets.getLeaseId(), secrets.getLeaseDuration(), secrets.isRenewable()); - } else if (isRotatingGenericSecret(requestedSecret, secrets)) { - lease = Lease.of(secrets.getLeaseDuration()); - } else { + } + else if (isRotatingGenericSecret(requestedSecret, secrets)) { + lease = Lease.fromTimeToLive(secrets.getLeaseDuration()); + } + else { lease = Lease.none(); } @@ -320,6 +316,13 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements } } + private static boolean isRotatingGenericSecret(RequestedSecret requestedSecret, + VaultResponseSupport> secrets) { + + return Mode.ROTATE.equals(requestedSecret.getMode()) && !secrets.isRenewable() + && secrets.getLeaseDuration() > 0; + } + /** * Stop the {@link SecretLeaseContainer}. Stopping the container will stop lease * renewal, secrets rotation and event publishing. Active leases are not expired. @@ -389,7 +392,7 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements Lease lease = entry.getValue().getLease(); entry.getValue().disableScheduleRenewal(); - if (lease != null && !lease.isRotatingGenericLease()) { + if (lease != null && lease.hasLeaseId()) { doRevokeLease(entry.getKey(), lease); } } @@ -408,33 +411,42 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements void potentiallyScheduleLeaseRenewal(final RequestedSecret requestedSecret, final Lease lease, final LeaseRenewalScheduler leaseRenewal) { - if (leaseRenewal.isLeaseRenewable(lease)) { + if (!leaseRenewal.isLeaseRenewable(lease, requestedSecret)) { + return; + } - if (log.isDebugEnabled()) { - log.debug(String.format("Lease %s qualified for renewal", - lease.getLeaseId())); + if (log.isDebugEnabled()) { + + 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(new RenewLease() { + } - @Override - public Lease renewLease(Lease lease) { + leaseRenewal.scheduleRenewal(requestedSecret, new RenewLease() { - Lease newLease = doRenewLease(requestedSecret, lease); + @Override + public Lease renewLease(Lease lease) { - if (newLease == null) { - return null; - } + Lease newLease = doRenewLease(requestedSecret, lease); + + if (!Lease.none().equals(newLease)) { potentiallyScheduleLeaseRenewal(requestedSecret, newLease, leaseRenewal); onAfterLeaseRenewed(requestedSecret, newLease); - - return newLease; } - }, lease, getMinRenewalSeconds(), getExpiryThresholdSeconds()); - } + + return newLease; + } + }, lease, getMinRenewalSeconds(), getExpiryThresholdSeconds()); } // ------------------------------------------------------------------------- @@ -468,43 +480,20 @@ 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(final RequestedSecret requestedSecret, final Lease lease) { try { - ResponseEntity> entity = operations - .doWithSession(new RestOperationsCallback>>() { - @Override - @SuppressWarnings("unchecked") - public ResponseEntity> doWithRestOperations( - RestOperations restOperations) { - if (lease.isRotatingGenericLease()) { - return (ResponseEntity) restOperations.exchange( - requestedSecret.getPath(), HttpMethod.GET, - null, Map.class, (Object) null); - } else { - return (ResponseEntity) restOperations.exchange( - "/sys/renew/{leaseId}", HttpMethod.PUT, null, Map.class, lease.getLeaseId()); - } - } - }); + Lease renewed = lease.hasLeaseId() ? renew(lease) : lease; - Map body = entity.getBody(); - String leaseId = (String) body.get("lease_id"); - Number leaseDuration = (Number) body.get("lease_duration"); - boolean renewable = (Boolean) body.get("renewable"); + if (!renewed.hasLeaseId() || renewed.getLeaseDuration() == 0 + || renewed.getLeaseDuration() < minRenewalSeconds) { - if (leaseDuration == null || leaseDuration.intValue() < minRenewalSeconds) { onLeaseExpired(requestedSecret, lease); - return null; + return Lease.none(); } - Lease results = Lease.of(leaseId, leaseDuration.longValue(), renewable); - if (results.isRotatingGenericLease()) { - onSecretsObtained(requestedSecret, results, body); - } - return results; + return renewed; } catch (HttpStatusCodeException e) { @@ -521,7 +510,31 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements onError(requestedSecret, lease, e); } - return null; + return Lease.none(); + } + + private Lease renew(final Lease lease) { + + ResponseEntity> entity = operations + .doWithSession(new RestOperationsCallback>>() { + + @Override + @SuppressWarnings("unchecked") + public ResponseEntity> doWithRestOperations( + RestOperations restOperations) { + return (ResponseEntity) restOperations.exchange( + "/sys/renew/{leaseId}", HttpMethod.PUT, null, Map.class, + lease.getLeaseId()); + } + }); + + Map body = entity.getBody(); + String leaseId = (String) body.get("lease_id"); + Number leaseDuration = (Number) body.get("lease_duration"); + boolean renewable = (Boolean) body.get("renewable"); + + return Lease.of(leaseId, leaseDuration != null ? leaseDuration.longValue() : 0, + renewable); } /** @@ -553,8 +566,8 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements onBeforeLeaseRevocation(requestedSecret, lease); - operations.doWithSession( - new RestOperationsCallback>>() { + operations + .doWithSession(new RestOperationsCallback>>() { @Override @SuppressWarnings("unchecked") @@ -605,20 +618,29 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements /** * Schedule {@link Lease} renewal. Previously registered renewal tasks are * canceled to prevent renewal of stale {@link Lease}s. + * @param requestedSecret the requested secret. * @param renewLease strategy to renew a {@link Lease}. * @param lease the current {@link Lease}. * @param minRenewalSeconds minimum number of seconds before renewing a * {@link Lease}. This is to prevent too many renewals in a very short timeframe. * @param expiryThresholdSeconds number of seconds to renew before {@link Lease}. - * expires. */ - void scheduleRenewal(final RenewLease renewLease, final Lease lease, + void scheduleRenewal(final RequestedSecret requestedSecret, + final RenewLease renewLease, final Lease lease, final int minRenewalSeconds, final int expiryThresholdSeconds) { if (log.isDebugEnabled()) { - log.debug(String.format( - "Scheduling renewal for lease %s, lease duration %d", - lease.getLeaseId(), lease.getLeaseDuration())); + if (lease.hasLeaseId()) { + log.debug(String + .format("Scheduling renewal for secret %s with lease %s, lease duration %d", + requestedSecret.getPath(), lease.getLeaseId(), + lease.getLeaseDuration())); + } + else { + log.debug(String + .format("Scheduling renewal for secret %s, with cache hint duration %d", + requestedSecret.getPath(), lease.getLeaseDuration())); + } } Lease currentLease = this.currentLeaseRef.get(); @@ -628,36 +650,42 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements cancelSchedule(currentLease); } - ScheduledFuture scheduledFuture = taskScheduler.schedule( - new Runnable() { + Runnable task = new Runnable() { - @Override - public void run() { + @Override + public void run() { - try { + schedules.remove(lease); - schedules.remove(lease); + if (currentLeaseRef.get() != lease) { + log.debug("Current lease has changed. Skipping renewal"); + return; + } - if (currentLeaseRef.get() != lease) { - log.debug("Current lease has changed. Skipping renewal"); - return; - } - - if (log.isDebugEnabled()) { - log.debug(String.format("Renewing lease %s", - lease.getLeaseId())); - } - - currentLeaseRef.compareAndSet(lease, - renewLease.renewLease(lease)); - } - catch (Exception e) { - log.error( - String.format("Cannot renew lease %s", - lease.getLeaseId()), e); - } + if (log.isDebugEnabled()) { + if (lease.hasLeaseId()) { + log.debug(String.format("Renewing lease %s for secret %s", + lease.getLeaseId(), requestedSecret.getPath())); } - }, + else { + log.debug(String.format("Renewing secret without lease %s", + requestedSecret.getPath())); + } + } + + try { + currentLeaseRef + .compareAndSet(lease, renewLease.renewLease(lease)); + } + catch (Exception e) { + log.error(String.format("Cannot renew lease %s", + lease.getLeaseId()), e); + } + } + }; + + ScheduledFuture scheduledFuture = taskScheduler.schedule( + task, new OneShotTrigger(getRenewalSeconds(lease, minRenewalSeconds, expiryThresholdSeconds))); @@ -699,9 +727,21 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements - expiryThresholdSeconds); } - private boolean isLeaseRenewable(Lease lease) { - return lease != null - && (lease.isRenewable() || lease.isRotatingGenericLease()); + private boolean isLeaseRenewable(Lease lease, RequestedSecret requestedSecret) { + + if (lease == null) { + return false; + } + + if (lease.isRenewable()) { + return true; + } + + if (!lease.hasLeaseId() && requestedSecret.getMode() == Mode.ROTATE) { + return true; + } + + return false; } public Lease getLease() { @@ -751,7 +791,7 @@ public class SecretLeaseContainer extends SecretLeaseEventPublisher implements * Renew a lease. * * @param lease must not be {@literal null}. - * @return the new lease + * @return the new lease. * @throws VaultException if lease renewal runs into problems */ Lease renewLease(Lease lease) throws VaultException; diff --git a/spring-vault-core/src/main/java/org/springframework/vault/core/lease/domain/Lease.java b/spring-vault-core/src/main/java/org/springframework/vault/core/lease/domain/Lease.java index 3dd312b2..1a764fd4 100644 --- a/spring-vault-core/src/main/java/org/springframework/vault/core/lease/domain/Lease.java +++ b/spring-vault-core/src/main/java/org/springframework/vault/core/lease/domain/Lease.java @@ -16,7 +16,6 @@ package org.springframework.vault.core.lease.domain; import org.springframework.util.Assert; -import org.springframework.util.StringUtils; /** * A lease abstracting the lease Id, duration and its renewability. @@ -52,19 +51,20 @@ public class Lease { public static Lease of(String leaseId, long leaseDuration, boolean renewable) { Assert.hasText(leaseId, "LeaseId must not be empty"); + return new Lease(leaseId, leaseDuration, renewable); } /** - * Create a new non-renewable {@link Lease}, with an empty lease ID and - * specified duration. + * Create a new non-renewable {@link Lease}, without a {@code leaseId} and specified + * duration. * - * @param leaseDuration the lease duration in seconds + * @param leaseDuration the lease duration in seconds. * @return the created {@link Lease} + * @since 1.1 */ - public static Lease of(long leaseDuration) { - - return new Lease("", leaseDuration, false); + public static Lease fromTimeToLive(long leaseDuration) { + return new Lease(null, leaseDuration, false); } /** @@ -76,6 +76,14 @@ public class Lease { return NONE; } + /** + * @return {@literal true} is the lease is associated with a {@code leaseId}. + * @since 1.1 + */ + public boolean hasLeaseId() { + return leaseId != null; + } + /** * @return the lease Id */ @@ -91,21 +99,12 @@ public class Lease { } /** - * * @return {@literal true} if the lease is renewable. */ public boolean isRenewable() { return renewable; } - /** - * - * @return {@literal true} if the lease represents a rotating generic secret. - */ - public boolean isRotatingGenericLease() { - return !renewable && leaseDuration > 0 && StringUtils.isEmpty(leaseId); - } - @Override public boolean equals(Object o) { if (this == o) 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 1f5addd4..90be8c13 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 @@ -187,8 +187,8 @@ public class SecretLeaseContainerUnitTests { @SuppressWarnings("unchecked") public void shouldAcceptSecretsWithRenewableLease() { - when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))) - .thenReturn(scheduledFuture); + when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn( + scheduledFuture); when(vaultOperations.read(requestedSecret.getPath())).thenReturn(createSecrets()); @@ -215,6 +215,43 @@ public class SecretLeaseContainerUnitTests { verify(taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class)); } + @Test + public void shouldRotateGenericSecret() { + + when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn( + scheduledFuture); + + when(vaultOperations.read(rotatingGenericSecret.getPath())).thenReturn( + createGenericSecrets(Collections.singletonMap("key", (Object) "value")), + createGenericSecrets(Collections.singletonMap("foo", (Object) "bar"))); + + secretLeaseContainer.addRequestedSecret(rotatingGenericSecret); + + secretLeaseContainer.start(); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Runnable.class); + verify(taskScheduler).schedule(captor.capture(), any(Trigger.class)); + + captor.getValue().run(); + verifyZeroInteractions(scheduledFuture); + verify(taskScheduler, times(2)).schedule(captor.capture(), any(Trigger.class)); + + ArgumentCaptor createdEvents = ArgumentCaptor + .forClass(SecretLeaseCreatedEvent.class); + verify(leaseListenerAdapter, times(3)).onLeaseEvent(createdEvents.capture()); + + List events = createdEvents.getAllValues(); + + assertThat(events).hasSize(3); + assertThat(events.get(0)).isInstanceOf(SecretLeaseCreatedEvent.class); + assertThat(events.get(0).getSecrets()).containsOnlyKeys("key"); + + assertThat(events.get(1)).isInstanceOf(SecretLeaseExpiredEvent.class); + + assertThat(events.get(2)).isInstanceOf(SecretLeaseCreatedEvent.class); + assertThat(events.get(2).getSecrets()).containsOnlyKeys("foo"); + } + @Test public void shouldNotRenewExpiringLease() { @@ -237,8 +274,8 @@ public class SecretLeaseContainerUnitTests { @Test public void shouldNotRotateExpiringLease() { - when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))) - .thenReturn(scheduledFuture); + when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn( + scheduledFuture); VaultResponse first = createSecrets(); VaultResponse second = createSecrets(); @@ -294,8 +331,8 @@ public class SecretLeaseContainerUnitTests { public void shouldPublishRenewalErrors() { prepareRenewal(); - when(vaultOperations.doWithSession(any(RestOperationsCallback.class))) - .thenThrow(new HttpClientErrorException(HttpStatus.I_AM_A_TEAPOT)); + when(vaultOperations.doWithSession(any(RestOperationsCallback.class))).thenThrow( + new HttpClientErrorException(HttpStatus.I_AM_A_TEAPOT)); secretLeaseContainer.start(); @@ -406,10 +443,10 @@ public class SecretLeaseContainerUnitTests { verify(vaultOperations).doWithSession(any(RestOperationsCallback.class)); verify(scheduledFuture).cancel(false); verify(leaseListenerAdapter).onLeaseEvent(any(SecretLeaseCreatedEvent.class)); - verify(leaseListenerAdapter) - .onLeaseEvent(any(BeforeSecretLeaseRevocationEvent.class)); - verify(leaseListenerAdapter) - .onLeaseEvent(any(AfterSecretLeaseRevocationEvent.class)); + verify(leaseListenerAdapter).onLeaseEvent( + any(BeforeSecretLeaseRevocationEvent.class)); + verify(leaseListenerAdapter).onLeaseEvent( + any(AfterSecretLeaseRevocationEvent.class)); } @Test @@ -427,25 +464,20 @@ public class SecretLeaseContainerUnitTests { verifyZeroInteractions(taskScheduler); - verify(leaseListenerAdapter, never()) - .onLeaseEvent(any(BeforeSecretLeaseRevocationEvent.class)); - verify(leaseListenerAdapter, never()) - .onLeaseEvent(any(AfterSecretLeaseRevocationEvent.class)); + verify(leaseListenerAdapter, never()).onLeaseEvent( + any(BeforeSecretLeaseRevocationEvent.class)); + verify(leaseListenerAdapter, never()).onLeaseEvent( + any(AfterSecretLeaseRevocationEvent.class)); } @Test - public void shouldRequestRotatingGenericSecrets() throws Exception { + public void shouldRequestRotatingGenericSecrets() { - when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))) - .thenReturn(scheduledFuture); + when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn( + scheduledFuture); - VaultResponse secrets = new VaultResponse(); - secrets.setLeaseId(""); - secrets.setRenewable(false); - secrets.setLeaseDuration(60); - secrets.setData(Collections.singletonMap("key", (Object) "value")); - - when(vaultOperations.read(rotatingGenericSecret.getPath())).thenReturn(secrets); + when(vaultOperations.read(rotatingGenericSecret.getPath())).thenReturn( + createGenericSecrets()); secretLeaseContainer.addRequestedSecret(rotatingGenericSecret); secretLeaseContainer.start(); @@ -463,8 +495,8 @@ public class SecretLeaseContainerUnitTests { @SuppressWarnings("unchecked") private void prepareRenewal() { - when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))) - .thenReturn(scheduledFuture); + when(taskScheduler.schedule(any(Runnable.class), any(Trigger.class))).thenReturn( + scheduledFuture); when(vaultOperations.read(requestedSecret.getPath())).thenReturn(createSecrets()); @@ -499,4 +531,19 @@ public class SecretLeaseContainerUnitTests { return secrets; } + + private VaultResponse createGenericSecrets() { + return createGenericSecrets(Collections.singletonMap("key", (Object) "value")); + } + + private VaultResponse createGenericSecrets(Map data) { + + VaultResponse secrets = new VaultResponse(); + + secrets.setRenewable(false); + secrets.setLeaseDuration(100); + secrets.setData(data); + + return secrets; + } } diff --git a/src/main/asciidoc/reference/getting-started.adoc b/src/main/asciidoc/reference/getting-started.adoc index 43baacc9..2c93217d 100644 --- a/src/main/asciidoc/reference/getting-started.adoc +++ b/src/main/asciidoc/reference/getting-started.adoc @@ -483,7 +483,7 @@ public class AppConfig { ---- ==== -NOTE: Secrets obtained from `generic` secret backends are associated with a TTL (`refresh_interval`) but not a lease Id. Spring Vault's ``PropertySource`` is not refreshing/flushing these secrets once the TTL expires despite the requested `Renewal` mode. +NOTE: Secrets obtained from `generic` secret backends are associated with a TTL (`refresh_interval`) but not a lease Id. Spring Vault's ``PropertySource`` rotates generic secrets when reaching its TTL. In certain situations, it may not be possible or practical to tightly control property source ordering when using `@VaultPropertySource` annotations.