From d0124ff9671aa740f8d5bc7ddc0b7a8f122245a4 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 26 May 2017 08:32:36 +0200 Subject: [PATCH] Polishing. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify code. Add since tags. Introduce Lease.fromTimeToLive(…) factory method in favor of Lease.of(…) with empty lease Id. Refactor Lease.isRotatingGenericLease(…) to Lease.hasLeaseId(…). Adopt SecretLeaseContainer to these changes. Add debug logging for leases without lease Id. Extend unit tests. Adjust reference documentation. Original pull request: gh-95. See gh-68. --- .../core/lease/SecretLeaseContainer.java | 226 +++++++++++------- .../vault/core/lease/domain/Lease.java | 31 ++- .../lease/SecretLeaseContainerUnitTests.java | 99 ++++++-- .../asciidoc/reference/getting-started.adoc | 2 +- 4 files changed, 222 insertions(+), 136 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 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.