From 26a511495e6777698efa827981e1a5f7e0a7aef6 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Tue, 25 Nov 2014 11:47:37 +0000 Subject: [PATCH] Allow the user to opt-out of anonymous access restrictions for /health By default, when /health is accessed anonymously, the details are stripped, i.e. the response will only indicate UP or DOWN. Furthermore the response is cached for a configurable period to prevent a denial of service attack. This commit adds a configuration property, endpoints.health.restrict-anonymous-access, that can be set to false to allow full anonymous access to /health. When full access is allowed, the details will be included in the response and the response will not be cached. Closes gh-1977 --- .../boot/actuate/endpoint/HealthEndpoint.java | 35 +++++++---- .../endpoint/mvc/HealthMvcEndpoint.java | 16 +++-- .../endpoint/mvc/HealthMvcEndpointTests.java | 59 ++++++++++++++++++- .../appendix-application-properties.adoc | 1 + 4 files changed, 92 insertions(+), 19 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/HealthEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/HealthEndpoint.java index b6d6fd857f3..1e32bd77209 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/HealthEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/HealthEndpoint.java @@ -30,6 +30,7 @@ import org.springframework.util.Assert; * * @author Dave Syer * @author Christian Dupuis + * @author Andy Wilkinson */ @ConfigurationProperties(prefix = "endpoints.health", ignoreUnknownFields = true) public class HealthEndpoint extends AbstractEndpoint { @@ -38,18 +39,7 @@ public class HealthEndpoint extends AbstractEndpoint { private long timeToLive = 1000; - /** - * Time to live for cached result. If accessed anonymously, we might need to cache the - * result of this endpoint to prevent a DOS attack. - * @return time to live in milliseconds (default 1000) - */ - public long getTimeToLive() { - return this.timeToLive; - } - - public void setTimeToLive(long ttl) { - this.timeToLive = ttl; - } + private boolean restrictAnonymousAccess = true; /** * Create a new {@link HealthIndicator} instance. @@ -69,6 +59,27 @@ public class HealthEndpoint extends AbstractEndpoint { this.healthIndicator = healthIndicator; } + /** + * Time to live for cached result. If accessed anonymously, we might need to cache the + * result of this endpoint to prevent a DOS attack. + * @return time to live in milliseconds (default 1000) + */ + public long getTimeToLive() { + return this.timeToLive; + } + + public void setTimeToLive(long ttl) { + this.timeToLive = ttl; + } + + public boolean isRestrictAnonymousAccess() { + return this.restrictAnonymousAccess; + } + + public void setRestrictAnonymousAccess(boolean restrictAnonymousAccess) { + this.restrictAnonymousAccess = restrictAnonymousAccess; + } + /** * Invoke all {@link HealthIndicator} delegates and collect their health information. */ diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java index 1f89c16123f..9d43e06be26 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpoint.java @@ -36,6 +36,7 @@ import org.springframework.web.bind.annotation.ResponseBody; * * @author Christian Dupuis * @author Dave Syer + * @author Andy Wilkinson * @since 1.1.0 */ public class HealthMvcEndpoint implements MvcEndpoint { @@ -121,7 +122,7 @@ public class HealthMvcEndpoint implements MvcEndpoint { // Not too worried about concurrent access here, the worst that can happen is the // odd extra call to delegate.invoke() this.cached = health; - if (!secure(principal)) { + if (this.delegate.isRestrictAnonymousAccess() && !secure(principal)) { // If not secure we only expose the status health = Health.status(health.getStatus()).build(); } @@ -133,15 +134,20 @@ public class HealthMvcEndpoint implements MvcEndpoint { } private boolean useCachedValue(Principal principal) { - long currentAccess = System.currentTimeMillis(); - if (this.cached == null || secure(principal) - || (currentAccess - this.lastAccess) > this.delegate.getTimeToLive()) { - this.lastAccess = currentAccess; + long accessTime = System.currentTimeMillis(); + if (cacheIsStale(accessTime) || secure(principal) + || !this.delegate.isRestrictAnonymousAccess()) { + this.lastAccess = accessTime; return false; } return this.cached != null; } + private boolean cacheIsStale(long accessTime) { + return this.cached == null + || (accessTime - this.lastAccess) > this.delegate.getTimeToLive(); + } + @Override public String getPath() { return "/" + this.delegate.getId(); diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java index db4f96165be..99cf91dac7f 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/mvc/HealthMvcEndpointTests.java @@ -28,7 +28,10 @@ import org.springframework.http.ResponseEntity; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.authority.AuthorityUtils; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -93,6 +96,7 @@ public class HealthMvcEndpointTests { public void secure() { given(this.endpoint.invoke()).willReturn( new Health.Builder().up().withDetail("foo", "bar").build()); + given(this.endpoint.isRestrictAnonymousAccess()).willReturn(true); Object result = this.mvc.invoke(this.user); assertTrue(result instanceof Health); assertTrue(((Health) result).getStatus() == Status.UP); @@ -102,6 +106,7 @@ public class HealthMvcEndpointTests { @Test public void secureNotCached() { given(this.endpoint.getTimeToLive()).willReturn(10000L); + given(this.endpoint.isRestrictAnonymousAccess()).willReturn(true); given(this.endpoint.invoke()).willReturn( new Health.Builder().up().withDetail("foo", "bar").build()); Object result = this.mvc.invoke(this.user); @@ -117,16 +122,66 @@ public class HealthMvcEndpointTests { @Test public void unsecureCached() { given(this.endpoint.getTimeToLive()).willReturn(10000L); + given(this.endpoint.isRestrictAnonymousAccess()).willReturn(true); given(this.endpoint.invoke()).willReturn( new Health.Builder().up().withDetail("foo", "bar").build()); Object result = this.mvc.invoke(this.user); assertTrue(result instanceof Health); - assertTrue(((Health) result).getStatus() == Status.UP); + Health health = (Health) result; + assertTrue(health.getStatus() == Status.UP); + assertThat(health.getDetails().size(), is(equalTo(1))); + assertThat(health.getDetails().get("foo"), is(equalTo((Object) "bar"))); given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build()); result = this.mvc.invoke(null); // insecure now - Health health = (Health) result; + assertTrue(result instanceof Health); + health = (Health) result; // so the result is cached assertTrue(health.getStatus() == Status.UP); + // but the details are hidden + assertThat(health.getDetails().size(), is(equalTo(0))); } + @Test + public void unsecureAnonymousAccessUnrestricted() { + given(this.endpoint.invoke()).willReturn( + new Health.Builder().up().withDetail("foo", "bar").build()); + given(this.endpoint.isRestrictAnonymousAccess()).willReturn(false); + Object result = this.mvc.invoke(null); + assertTrue(result instanceof Health); + assertTrue(((Health) result).getStatus() == Status.UP); + assertEquals("bar", ((Health) result).getDetails().get("foo")); + } + + @Test + public void unsecureIsNotCachedWhenAnonymousAccessIsUnrestricted() { + given(this.endpoint.getTimeToLive()).willReturn(10000L); + given(this.endpoint.isRestrictAnonymousAccess()).willReturn(false); + given(this.endpoint.invoke()).willReturn( + new Health.Builder().up().withDetail("foo", "bar").build()); + Object result = this.mvc.invoke(null); + assertTrue(result instanceof Health); + assertTrue(((Health) result).getStatus() == Status.UP); + given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build()); + result = this.mvc.invoke(null); + @SuppressWarnings("unchecked") + Health health = ((ResponseEntity) result).getBody(); + assertTrue(health.getStatus() == Status.DOWN); + } + + @Test + public void newValueIsReturnedOnceTtlExpires() throws InterruptedException { + given(this.endpoint.getTimeToLive()).willReturn(50L); + given(this.endpoint.isRestrictAnonymousAccess()).willReturn(true); + given(this.endpoint.invoke()).willReturn( + new Health.Builder().up().withDetail("foo", "bar").build()); + Object result = this.mvc.invoke(null); + assertTrue(result instanceof Health); + assertTrue(((Health) result).getStatus() == Status.UP); + Thread.sleep(100); + given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build()); + result = this.mvc.invoke(null); + @SuppressWarnings("unchecked") + Health health = ((ResponseEntity) result).getBody(); + assertTrue(health.getStatus() == Status.DOWN); + } } diff --git a/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc b/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc index 443942cf8bb..bb36cb4f660 100644 --- a/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc +++ b/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc @@ -404,6 +404,7 @@ content into your application; rather pick only the properties that you need. endpoints.health.id=health endpoints.health.sensitive=false endpoints.health.enabled=true + endpoints.health.restrict-anonymous-access=true endpoints.health.time-to-live=1000 endpoints.info.id=info endpoints.info.sensitive=false