From 24e71e868f0599eb041b80d32f7cd3e17977c897 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Mon, 27 Oct 2014 13:11:13 +0000 Subject: [PATCH] Allow /health and /info to authenticate anonymously Then we can optionally find a non-anonymous principal if there is one. If the user is anonymous then the health result is cached up to endpoints.health.ttl (default 1000ms) to prevent a DOS attack. Fixes gh-1353 --- .../ManagementSecurityAutoConfiguration.java | 32 +++++---- ...ConfigurationPropertiesReportEndpoint.java | 1 + .../boot/actuate/endpoint/HealthEndpoint.java | 16 +++++ .../endpoint/mvc/EndpointHandlerMapping.java | 20 ++++-- .../endpoint/mvc/EnvironmentMvcEndpoint.java | 1 + .../endpoint/mvc/HealthMvcEndpoint.java | 67 +++++++++++++++++-- .../endpoint/mvc/MetricsMvcEndpoint.java | 1 + ...agementSecurityAutoConfigurationTests.java | 6 +- .../endpoint/mvc/HealthMvcEndpointTests.java | 63 ++++++++++++++--- .../actuator/SampleActuatorApplication.java | 9 ++- .../src/main/resources/application.properties | 2 +- .../SampleActuatorApplicationTests.java | 11 +++ 12 files changed, 191 insertions(+), 38 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java index 3addb3832e..eb87a13d32 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfiguration.java @@ -141,11 +141,9 @@ public class ManagementSecurityAutoConfiguration { // add them back. List ignored = SpringBootWebSecurityConfiguration .getIgnored(this.security); - ignored.addAll(Arrays.asList(getEndpointPaths(this.endpointHandlerMapping, - false))); if (!this.management.getSecurity().isEnabled()) { ignored.addAll(Arrays.asList(getEndpointPaths( - this.endpointHandlerMapping, true))); + this.endpointHandlerMapping))); } if (ignored.contains("none")) { ignored.remove("none"); @@ -220,7 +218,7 @@ public class ManagementSecurityAutoConfiguration { protected void configure(HttpSecurity http) throws Exception { // secure endpoints - String[] paths = getEndpointPaths(this.endpointHandlerMapping, true); + String[] paths = getEndpointPaths(this.endpointHandlerMapping); if (paths.length > 0 && this.management.getSecurity().isEnabled()) { // Always protect them if present if (this.security.isRequireSsl()) { @@ -229,10 +227,12 @@ public class ManagementSecurityAutoConfiguration { http.exceptionHandling().authenticationEntryPoint(entryPoint()); paths = this.server.getPathsArray(paths); http.requestMatchers().antMatchers(paths); - http.authorizeRequests().anyRequest() - .hasRole(this.management.getSecurity().getRole()) // - .and().httpBasic() // - .and().anonymous().disable(); + // @formatter:off + http.authorizeRequests() + .antMatchers(this.server.getPathsArray(getEndpointPaths(this.endpointHandlerMapping, false))).access("permitAll()") + .anyRequest().hasRole(this.management.getSecurity().getRole()); + // @formatter:on + http.httpBasic(); // No cookies for management endpoints by default http.csrf().disable(); @@ -254,6 +254,12 @@ public class ManagementSecurityAutoConfiguration { } + private static String[] getEndpointPaths(EndpointHandlerMapping endpointHandlerMapping) { + return StringUtils.mergeStringArrays( + getEndpointPaths(endpointHandlerMapping, false), + getEndpointPaths(endpointHandlerMapping, true)); + } + private static String[] getEndpointPaths( EndpointHandlerMapping endpointHandlerMapping, boolean secure) { if (endpointHandlerMapping == null) { @@ -264,13 +270,11 @@ public class ManagementSecurityAutoConfiguration { List paths = new ArrayList(endpoints.size()); for (MvcEndpoint endpoint : endpoints) { if (endpoint.isSensitive() == secure) { - String path = endpointHandlerMapping.getPrefix() + endpoint.getPath(); + String path = endpointHandlerMapping.getPath(endpoint.getPath()); paths.add(path); - if (secure) { - // Add Spring MVC-generated additional paths - paths.add(path + "/"); - paths.add(path + ".*"); - } + // Add Spring MVC-generated additional paths + paths.add(path + "/"); + paths.add(path + ".*"); } } return paths.toArray(new String[paths.size()]); diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java index 13ebbe25de..050c1a44b0 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/ConfigurationPropertiesReportEndpoint.java @@ -202,6 +202,7 @@ public class ConfigurationPropertiesReportEndpoint extends * Extension to {@link JacksonAnnotationIntrospector} to suppress CGLIB generated bean * properties. */ + @SuppressWarnings("serial") private static class CglibAnnotationIntrospector extends JacksonAnnotationIntrospector { 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 e50db28900..de0952f284 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 @@ -36,6 +36,22 @@ public class HealthEndpoint extends AbstractEndpoint { private final HealthIndicator healthIndicator; + private long ttl = 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 getTtl() { + return ttl; + } + + public void setTtl(long ttl) { + this.ttl = ttl; + } + /** * Create a new {@link HealthIndicator} instance. */ diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EndpointHandlerMapping.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EndpointHandlerMapping.java index de6e463602..6c4584ec2f 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EndpointHandlerMapping.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EndpointHandlerMapping.java @@ -18,7 +18,9 @@ package org.springframework.boot.actuate.endpoint.mvc; import java.lang.reflect.Method; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; import org.springframework.boot.actuate.endpoint.Endpoint; @@ -50,7 +52,7 @@ import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandl public class EndpointHandlerMapping extends RequestMappingHandlerMapping implements ApplicationContextAware { - private final Set endpoints; + private final Map endpoints = new HashMap(); private String prefix = ""; @@ -62,7 +64,10 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme * @param endpoints */ public EndpointHandlerMapping(Collection endpoints) { - this.endpoints = new HashSet(endpoints); + HashMap map = (HashMap) this.endpoints; + for (MvcEndpoint endpoint : endpoints) { + map.put(endpoint.getPath(), endpoint); + } // By default the static resource handler mapping is LOWEST_PRECEDENCE - 1 // and the RequestMappingHandlerMapping is 0 (we ideally want to be before both) setOrder(-100); @@ -72,7 +77,7 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme public void afterPropertiesSet() { super.afterPropertiesSet(); if (!this.disabled) { - for (MvcEndpoint endpoint : this.endpoints) { + for (MvcEndpoint endpoint : this.endpoints.values()) { detectHandlerMethods(endpoint); } } @@ -146,6 +151,13 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme return this.prefix; } + /** + * @return the path used in mappings + */ + public String getPath(String endpoint) { + return this.prefix + endpoint; + } + /** * Sets if this mapping is disabled. */ @@ -164,7 +176,7 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme * Return the endpoints */ public Set getEndpoints() { - return this.endpoints; + return new HashSet(this.endpoints.values()); } } diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EnvironmentMvcEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EnvironmentMvcEndpoint.java index 268939ec78..28e34f0803 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EnvironmentMvcEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/EnvironmentMvcEndpoint.java @@ -56,6 +56,7 @@ public class EnvironmentMvcEndpoint extends EndpointMvcAdapter implements this.environment = environment; } + @SuppressWarnings("serial") @ResponseStatus(value = HttpStatus.NOT_FOUND, reason = "No such property") public static class NoSuchPropertyException extends RuntimeException { 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 210c9b2068..5a7971ff31 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 @@ -16,10 +16,12 @@ package org.springframework.boot.actuate.endpoint.mvc; +import java.security.Principal; import java.util.Collections; import java.util.HashMap; import java.util.Map; +import org.springframework.boot.actuate.endpoint.Endpoint; import org.springframework.boot.actuate.endpoint.HealthEndpoint; import org.springframework.boot.actuate.health.Health; import org.springframework.boot.actuate.health.Status; @@ -33,14 +35,21 @@ import org.springframework.web.bind.annotation.ResponseBody; * Adapter to expose {@link HealthEndpoint} as an {@link MvcEndpoint}. * * @author Christian Dupuis + * @author Dave Syer * @since 1.1.0 */ -public class HealthMvcEndpoint extends EndpointMvcAdapter { +public class HealthMvcEndpoint implements MvcEndpoint { private Map statusMapping = new HashMap(); + private HealthEndpoint delegate; + + private long lastAccess = 0; + + private Health cached; + public HealthMvcEndpoint(HealthEndpoint delegate) { - super(delegate); + this.delegate = delegate; setupDefaultStatusMapping(); } @@ -91,21 +100,65 @@ public class HealthMvcEndpoint extends EndpointMvcAdapter { @RequestMapping @ResponseBody - @Override - public Object invoke() { - if (!this.getDelegate().isEnabled()) { - // Shouldn't happen + public Object invoke(Principal principal) { + + if (!delegate.isEnabled()) { + // Shouldn't happen because the request mapping should not be registered return new ResponseEntity>(Collections.singletonMap( "message", "This endpoint is disabled"), HttpStatus.NOT_FOUND); } - Health health = (Health) getDelegate().invoke(); + Health health = getHealth(principal); Status status = health.getStatus(); if (this.statusMapping.containsKey(status.getCode())) { return new ResponseEntity(health, this.statusMapping.get(status .getCode())); } + + return health; + + } + + private Health getHealth(Principal principal) { + Health health = useCachedValue(principal) ? cached : (Health) delegate.invoke(); + // Not too worried about concurrent access here, the worst that can happen is the + // odd extra call to delegate.invoke() + cached = health; + if (!secure(principal)) { + // If not secure we only expose the status + health = Health.status(health.getStatus()).build(); + } return health; } + private boolean secure(Principal principal) { + return principal != null && !principal.getClass().getName().contains("Anonymous"); + } + + private boolean useCachedValue(Principal principal) { + long currentAccess = System.currentTimeMillis(); + if (cached == null || secure(principal) + || currentAccess - lastAccess > delegate.getTtl()) { + lastAccess = currentAccess; + return false; + } + return cached != null; + } + + @Override + public String getPath() { + return "/" + this.delegate.getId(); + } + + @Override + public boolean isSensitive() { + return this.delegate.isSensitive(); + } + + @Override + @SuppressWarnings("rawtypes") + public Class getEndpointType() { + return this.delegate.getClass(); + } + } diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpoint.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpoint.java index 496b4811bd..eb9b951daa 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpoint.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/mvc/MetricsMvcEndpoint.java @@ -48,6 +48,7 @@ public class MetricsMvcEndpoint extends EndpointMvcAdapter { return value; } + @SuppressWarnings("serial") @ResponseStatus(value = HttpStatus.NOT_FOUND, reason = "No such metric") public static class NoSuchMetricException extends RuntimeException { diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfigurationTests.java index 5874b0afda..1d225236b2 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfigurationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/ManagementSecurityAutoConfigurationTests.java @@ -73,8 +73,8 @@ public class ManagementSecurityAutoConfigurationTests { PropertyPlaceholderAutoConfiguration.class); this.context.refresh(); assertNotNull(this.context.getBean(AuthenticationManagerBuilder.class)); - // 6 for static resources, one for management endpoints and one for the rest - assertEquals(8, this.context.getBean(FilterChainProxy.class).getFilterChains() + // 4 for static resources, one for management endpoints and one for the rest + assertEquals(6, this.context.getBean(FilterChainProxy.class).getFilterChains() .size()); } @@ -144,7 +144,7 @@ public class ManagementSecurityAutoConfigurationTests { this.context.refresh(); // Just the management endpoints (one filter) and ignores now plus the backup // filter on app endpoints - assertEquals(8, this.context.getBean(FilterChainProxy.class).getFilterChains() + assertEquals(6, this.context.getBean(FilterChainProxy.class).getFilterChains() .size()); } 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 08c7873eb4..a712c30a3a 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 @@ -16,6 +16,11 @@ package org.springframework.boot.actuate.endpoint.mvc; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; + import java.util.Collections; import org.junit.Before; @@ -25,11 +30,8 @@ import org.springframework.boot.actuate.health.Health; import org.springframework.boot.actuate.health.Status; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import static org.mockito.BDDMockito.given; -import static org.mockito.Mockito.mock; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.authority.AuthorityUtils; /** * Tests for {@link HealthMvcEndpoint}. @@ -42,6 +44,10 @@ public class HealthMvcEndpointTests { private HealthMvcEndpoint mvc = null; + private UsernamePasswordAuthenticationToken user = new UsernamePasswordAuthenticationToken( + "user", "password", + AuthorityUtils.commaSeparatedStringToAuthorityList("ROLE_USER")); + @Before public void init() { this.endpoint = mock(HealthEndpoint.class); @@ -52,7 +58,7 @@ public class HealthMvcEndpointTests { @Test public void up() { given(this.endpoint.invoke()).willReturn(new Health.Builder().up().build()); - Object result = this.mvc.invoke(); + Object result = this.mvc.invoke(null); assertTrue(result instanceof Health); assertTrue(((Health) result).getStatus() == Status.UP); } @@ -61,7 +67,7 @@ public class HealthMvcEndpointTests { @Test public void down() { given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build()); - Object result = this.mvc.invoke(); + Object result = this.mvc.invoke(null); assertTrue(result instanceof ResponseEntity); ResponseEntity response = (ResponseEntity) result; assertTrue(response.getBody().getStatus() == Status.DOWN); @@ -75,10 +81,51 @@ public class HealthMvcEndpointTests { new Health.Builder().status("OK").build()); this.mvc.setStatusMapping(Collections.singletonMap("OK", HttpStatus.INTERNAL_SERVER_ERROR)); - Object result = this.mvc.invoke(); + Object result = this.mvc.invoke(null); assertTrue(result instanceof ResponseEntity); ResponseEntity response = (ResponseEntity) result; assertTrue(response.getBody().getStatus().equals(new Status("OK"))); assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, response.getStatusCode()); } + + @Test + public void secure() { + given(this.endpoint.invoke()).willReturn( + new Health.Builder().up().withDetail("foo", "bar").build()); + Object result = this.mvc.invoke(user); + assertTrue(result instanceof Health); + assertTrue(((Health) result).getStatus() == Status.UP); + assertEquals("bar", ((Health) result).getDetails().get("foo")); + } + + @Test + public void secureNotCached() { + given(this.endpoint.getTtl()).willReturn(10000L); + given(this.endpoint.invoke()).willReturn( + new Health.Builder().up().withDetail("foo", "bar").build()); + Object result = this.mvc.invoke(user); + assertTrue(result instanceof Health); + assertTrue(((Health) result).getStatus() == Status.UP); + given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build()); + result = this.mvc.invoke(user); + @SuppressWarnings("unchecked") + Health health = (Health) ((ResponseEntity) result).getBody(); + assertTrue(health.getStatus() == Status.DOWN); + } + + @Test + public void unsecureCached() { + given(this.endpoint.getTtl()).willReturn(10000L); + given(this.endpoint.invoke()).willReturn( + new Health.Builder().up().withDetail("foo", "bar").build()); + Object result = this.mvc.invoke(user); + 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); // insecure now + Health health = (Health) result; + // so the result is cached + assertTrue(health.getStatus() == Status.UP); + } + } diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleActuatorApplication.java b/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleActuatorApplication.java index ac8387a841..10f8309a37 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleActuatorApplication.java +++ b/spring-boot-samples/spring-boot-sample-actuator/src/main/java/sample/actuator/SampleActuatorApplication.java @@ -17,6 +17,8 @@ package sample.actuator; import org.springframework.boot.SpringApplication; +import org.springframework.boot.actuate.health.Health; +import org.springframework.boot.actuate.health.HealthIndicator; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.ComponentScan; @@ -26,10 +28,15 @@ import org.springframework.context.annotation.Configuration; @EnableAutoConfiguration @EnableConfigurationProperties @ComponentScan -public class SampleActuatorApplication { +public class SampleActuatorApplication implements HealthIndicator { public static void main(String[] args) throws Exception { SpringApplication.run(SampleActuatorApplication.class, args); } + @Override + public Health health() { + return Health.up().withDetail("hello", "world").build(); + } + } diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/main/resources/application.properties b/spring-boot-samples/spring-boot-sample-actuator/src/main/resources/application.properties index d36c067f87..21d52efbf3 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/src/main/resources/application.properties +++ b/spring-boot-samples/spring-boot-sample-actuator/src/main/resources/application.properties @@ -1,5 +1,5 @@ logging.file: /tmp/logs/app.log -logging.level.org.springframework.security: INFO +logging.level.org.springframework.security: DEBUG management.address: 127.0.0.1 #management.port: 8181 endpoints.shutdown.enabled: true diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/SampleActuatorApplicationTests.java b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/SampleActuatorApplicationTests.java index 3007733f60..bab3aeb793 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/SampleActuatorApplicationTests.java +++ b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/SampleActuatorApplicationTests.java @@ -132,6 +132,17 @@ public class SampleActuatorApplicationTests { assertEquals(HttpStatus.OK, entity.getStatusCode()); assertTrue("Wrong body: " + entity.getBody(), entity.getBody().contains("\"status\":\"UP\"")); + assertFalse("Wrong body: " + entity.getBody(), + entity.getBody().contains("\"hello\":\"1\"")); + } + + @Test + public void testSecureHealth() throws Exception { + ResponseEntity entity = new TestRestTemplate("user", getPassword()).getForEntity( + "http://localhost:" + this.port + "/health", String.class); + assertEquals(HttpStatus.OK, entity.getStatusCode()); + assertTrue("Wrong body: " + entity.getBody(), + entity.getBody().contains("\"hello\":1")); } @Test