From 98135c964ba20b9b93f566911c3d146523024ebb Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Thu, 25 Dec 2014 12:42:45 -0800 Subject: [PATCH] Remove Principal handler logic from security Update ManagementSecurityAutoConfiguration so that MVC Endpoints that have Principal arguments are not treated in any special way. This restores Spring Boot 1.1.x behavior where the 'sensitive' flag is used to determine access rules. The HealthMvcEndpoint still uses the Principal (when available) to determine if full status information can be displayed. It now also explicitly checks the environment for `endpoints.health.sensitive` to determine if the user has opted-out and requires complete health details. The health MVC endpoint should now work as follows: * Default configuration - No login is required, full information is only displayed if a Principal is available. * endpoints.health.sensitive=true - Login is required, full information is displayed. * endpoints.health.sensitive=false - Login is not required, full information is displayed. Fixes gh-2211 --- .../EndpointAutoConfiguration.java | 18 +---- .../EndpointWebMvcAutoConfiguration.java | 5 +- .../ManagementSecurityAutoConfiguration.java | 16 ---- .../endpoint/mvc/EndpointHandlerMapping.java | 40 +--------- .../endpoint/mvc/HealthMvcEndpoint.java | 76 +++++++++++-------- .../endpoint/mvc/HealthMvcEndpointTests.java | 15 +++- ...pertiesSampleActuatorApplicationTests.java | 15 +++- .../actuator/NonSensitiveHealthTests.java | 58 ++++++++++++++ 8 files changed, 137 insertions(+), 106 deletions(-) create mode 100644 spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/NonSensitiveHealthTests.java diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointAutoConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointAutoConfiguration.java index 88a7113ab27..d0960dd5f51 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointAutoConfiguration.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointAutoConfiguration.java @@ -78,9 +78,6 @@ public class EndpointAutoConfiguration { @Autowired private InfoPropertiesConfiguration properties; - @Autowired(required = false) - private ManagementServerProperties management; - @Autowired(required = false) private HealthAggregator healthAggregator = new OrderedHealthAggregator(); @@ -105,20 +102,7 @@ public class EndpointAutoConfiguration { @Bean @ConditionalOnMissingBean public HealthEndpoint healthEndpoint() { - HealthEndpoint endpoint = new HealthEndpoint(this.healthAggregator, - this.healthIndicators); - endpoint.setSensitive(isHealthEndpointSensitive()); - return endpoint; - } - - /** - * The default health endpoint sensitivity depends on whether all the endpoints by - * default are secure or not. User can always override with - * {@literal endpoints.health.sensitive}. - */ - private boolean isHealthEndpointSensitive() { - return (this.management != null) && (this.management.getSecurity() != null) - && this.management.getSecurity().isEnabled(); + return new HealthEndpoint(this.healthAggregator, this.healthIndicators); } @Bean diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfiguration.java index 6491fae7a93..89b16589b72 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfiguration.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/EndpointWebMvcAutoConfiguration.java @@ -33,6 +33,7 @@ import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.actuate.autoconfigure.ManagementServerProperties.Security; import org.springframework.boot.actuate.endpoint.Endpoint; import org.springframework.boot.actuate.endpoint.EnvironmentEndpoint; import org.springframework.boot.actuate.endpoint.HealthEndpoint; @@ -163,7 +164,9 @@ public class EndpointWebMvcAutoConfiguration implements ApplicationContextAware, @ConditionalOnBean(HealthEndpoint.class) @ConditionalOnProperty(prefix = "endpoints.health", name = "enabled", matchIfMissing = true) public HealthMvcEndpoint healthMvcEndpoint(HealthEndpoint delegate) { - HealthMvcEndpoint healthMvcEndpoint = new HealthMvcEndpoint(delegate); + Security security = this.managementServerProperties.getSecurity(); + boolean secure = (security == null || security.isEnabled()); + HealthMvcEndpoint healthMvcEndpoint = new HealthMvcEndpoint(delegate, secure); if (this.healthMvcEndpointProperties.getMapping() != null) { healthMvcEndpoint.addStatusMapping(this.healthMvcEndpointProperties .getMapping()); 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 b7d17a3527c..b6319c82c49 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 @@ -22,7 +22,6 @@ import java.util.List; import java.util.Set; import javax.annotation.PostConstruct; -import javax.servlet.http.HttpServletRequest; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.actuate.endpoint.Endpoint; @@ -62,7 +61,6 @@ import org.springframework.security.config.annotation.web.configuration.WebSecur import org.springframework.security.config.annotation.web.configurers.ExpressionUrlAuthorizationConfigurer; import org.springframework.security.web.AuthenticationEntryPoint; import org.springframework.security.web.authentication.www.BasicAuthenticationEntryPoint; -import org.springframework.security.web.util.matcher.RequestMatcher; import org.springframework.util.StringUtils; /** @@ -256,23 +254,9 @@ public class ManagementSecurityAutoConfiguration { String[] endpointPaths, ExpressionUrlAuthorizationConfigurer.ExpressionInterceptUrlRegistry requests) { requests.antMatchers(endpointPaths).permitAll(); - if (this.endpointHandlerMapping != null) { - requests.requestMatchers(new PrincipalHandlerRequestMatcher()) - .permitAll(); - } requests.anyRequest().hasRole(this.management.getSecurity().getRole()); } - private final class PrincipalHandlerRequestMatcher implements RequestMatcher { - - @Override - public boolean matches(HttpServletRequest request) { - return ManagementWebSecurityConfigurerAdapter.this.endpointHandlerMapping - .isPrincipalHandler(request); - } - - } - } private static String[] getEndpointPaths(EndpointHandlerMapping endpointHandlerMapping) { 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 c3bdfc0fc86..5257d5e2dba 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 @@ -17,22 +17,17 @@ package org.springframework.boot.actuate.endpoint.mvc; import java.lang.reflect.Method; -import java.security.Principal; import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Set; -import javax.servlet.http.HttpServletRequest; - import org.springframework.boot.actuate.endpoint.Endpoint; import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextAware; import org.springframework.util.Assert; import org.springframework.util.StringUtils; -import org.springframework.web.method.HandlerMethod; -import org.springframework.web.servlet.HandlerExecutionChain; import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.servlet.mvc.condition.PatternsRequestCondition; import org.springframework.web.servlet.mvc.method.RequestMappingInfo; @@ -46,8 +41,8 @@ import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandl *

* One of the aims of the mapping is to support endpoints that work as HTTP endpoints but * can still provide useful service interfaces when there is no HTTP server (and no Spring - * MVC on the classpath). Note that any endpoints having method signaturess will break in - * a non-servlet environment. + * MVC on the classpath). Note that any endpoints having method signatures will break in a + * non-servlet environment. * * @author Phillip Webb * @author Christian Dupuis @@ -62,8 +57,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme private boolean disabled = false; - private Set principalHandlers = new HashSet(); - /** * Create a new {@link EndpointHandlerMapping} instance. All {@link Endpoint}s will be * detected from the {@link ApplicationContext}. @@ -102,9 +95,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme return; } String[] patterns = getPatterns(handler, mapping); - if (handlesPrincipal(method)) { - this.principalHandlers.add(new HandlerMethod(handler, method)); - } super.registerHandlerMethod(handler, method, withNewPatterns(mapping, patterns)); } @@ -132,15 +122,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme return ""; } - private boolean handlesPrincipal(Method method) { - for (Class type : method.getParameterTypes()) { - if (Principal.class.equals(type)) { - return true; - } - } - return false; - } - private RequestMappingInfo withNewPatterns(RequestMappingInfo mapping, String[] patternStrings) { PatternsRequestCondition patterns = new PatternsRequestCondition(patternStrings); @@ -150,23 +131,6 @@ public class EndpointHandlerMapping extends RequestMappingHandlerMapping impleme mapping.getCustomCondition()); } - /** - * Returns {@code true} if the given request is mapped to an endpoint and to a method - * that includes a {@link Principal} argument. - * @param request the http request - * @return {@code true} if the request is - */ - public boolean isPrincipalHandler(HttpServletRequest request) { - try { - HandlerExecutionChain handlerChain = getHandler(request); - Object handler = (handlerChain == null ? null : handlerChain.getHandler()); - return (handler != null && this.principalHandlers.contains(handler)); - } - catch (Exception ex) { - return false; - } - } - /** * @param prefix the prefix to set */ 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 2158673cb14..4ce1e184305 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 @@ -25,6 +25,9 @@ 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; +import org.springframework.boot.bind.RelaxedPropertyResolver; +import org.springframework.context.EnvironmentAware; +import org.springframework.core.env.Environment; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.util.Assert; @@ -37,20 +40,31 @@ import org.springframework.web.bind.annotation.ResponseBody; * @author Christian Dupuis * @author Dave Syer * @author Andy Wilkinson + * @author Phillip Webb * @since 1.1.0 */ -public class HealthMvcEndpoint implements MvcEndpoint { +public class HealthMvcEndpoint implements MvcEndpoint, EnvironmentAware { + + private final HealthEndpoint delegate; + + private final boolean secure; private Map statusMapping = new HashMap(); - private HealthEndpoint delegate; + private RelaxedPropertyResolver propertyResolver; private long lastAccess = 0; private Health cached; public HealthMvcEndpoint(HealthEndpoint delegate) { + this(delegate, true); + } + + public HealthMvcEndpoint(HealthEndpoint delegate, boolean secure) { + Assert.notNull(delegate, "Delegate must not be null"); this.delegate = delegate; + this.secure = secure; setupDefaultStatusMapping(); } @@ -59,6 +73,12 @@ public class HealthMvcEndpoint implements MvcEndpoint { addStatusMapping(Status.OUT_OF_SERVICE, HttpStatus.SERVICE_UNAVAILABLE); } + @Override + public void setEnvironment(Environment environment) { + this.propertyResolver = new RelaxedPropertyResolver(environment, + "endpoints.health."); + } + /** * Set specific status mappings. * @param statusMapping a map of status code to {@link HttpStatus} @@ -108,46 +128,42 @@ public class HealthMvcEndpoint implements MvcEndpoint { "message", "This endpoint is disabled"), HttpStatus.NOT_FOUND); } Health health = getHealth(principal); - Status status = health.getStatus(); - if (this.statusMapping.containsKey(status.getCode())) { - return new ResponseEntity(health, this.statusMapping.get(status - .getCode())); + HttpStatus status = this.statusMapping.get(health.getStatus().getCode()); + if (status != null) { + return new ResponseEntity(health, status); } return health; } private Health getHealth(Principal principal) { - Health health = (useCachedValue(principal) ? this.cached : (Health) this.delegate - .invoke()); - // 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) && this.delegate.isSensitive()) { - // If not secure we only expose the status - health = Health.status(health.getStatus()).build(); + long accessTime = System.currentTimeMillis(); + if (isCacheStale(accessTime) || isSecure(principal) || isUnrestricted()) { + this.lastAccess = accessTime; + this.cached = this.delegate.invoke(); } - return health; + if (isSecure(principal) || isUnrestricted()) { + return this.cached; + } + return Health.status(this.cached.getStatus()).build(); } - private boolean secure(Principal principal) { + private boolean isCacheStale(long accessTime) { + if (this.cached == null) { + return true; + } + return (accessTime - this.lastAccess) > this.delegate.getTimeToLive(); + } + + private boolean isUnrestricted() { + Boolean sensitive = this.propertyResolver.getProperty("sensitive", Boolean.class); + return !this.secure || Boolean.FALSE.equals(sensitive); + } + + private boolean isSecure(Principal principal) { return (principal != null && !principal.getClass().getName() .contains("Anonymous")); } - private boolean useCachedValue(Principal principal) { - long accessTime = System.currentTimeMillis(); - if (cacheIsStale(accessTime) || secure(principal) || !this.delegate.isSensitive()) { - 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 49173bfaa44..28a703a0374 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 @@ -23,8 +23,11 @@ import org.junit.Test; import org.springframework.boot.actuate.endpoint.HealthEndpoint; import org.springframework.boot.actuate.health.Health; import org.springframework.boot.actuate.health.Status; +import org.springframework.core.env.MapPropertySource; +import org.springframework.core.env.PropertySource; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.mock.env.MockEnvironment; import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; import org.springframework.security.core.authority.AuthorityUtils; @@ -44,10 +47,16 @@ import static org.mockito.Mockito.mock; */ public class HealthMvcEndpointTests { + private static final PropertySource NON_SENSITIVE = new MapPropertySource("test", + Collections. singletonMap("endpoints.health.sensitive", + "false"));; + private HealthEndpoint endpoint = null; private HealthMvcEndpoint mvc = null; + private MockEnvironment environment; + private UsernamePasswordAuthenticationToken user = new UsernamePasswordAuthenticationToken( "user", "password", AuthorityUtils.commaSeparatedStringToAuthorityList("ROLE_USER")); @@ -57,6 +66,8 @@ public class HealthMvcEndpointTests { this.endpoint = mock(HealthEndpoint.class); given(this.endpoint.isEnabled()).willReturn(true); this.mvc = new HealthMvcEndpoint(this.endpoint); + this.environment = new MockEnvironment(); + this.mvc.setEnvironment(this.environment); } @Test @@ -143,9 +154,9 @@ public class HealthMvcEndpointTests { @Test public void unsecureAnonymousAccessUnrestricted() { + this.environment.getPropertySources().addLast(NON_SENSITIVE); given(this.endpoint.invoke()).willReturn( new Health.Builder().up().withDetail("foo", "bar").build()); - given(this.endpoint.isSensitive()).willReturn(false); Object result = this.mvc.invoke(null); assertTrue(result instanceof Health); assertTrue(((Health) result).getStatus() == Status.UP); @@ -154,8 +165,8 @@ public class HealthMvcEndpointTests { @Test public void unsecureIsNotCachedWhenAnonymousAccessIsUnrestricted() { + this.environment.getPropertySources().addLast(NON_SENSITIVE); given(this.endpoint.getTimeToLive()).willReturn(10000L); - given(this.endpoint.isSensitive()).willReturn(false); given(this.endpoint.invoke()).willReturn( new Health.Builder().up().withDetail("foo", "bar").build()); Object result = this.mvc.invoke(null); diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java index 58eb4eae46e..bdfc5c096c1 100644 --- a/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java +++ b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/EndpointsPropertiesSampleActuatorApplicationTests.java @@ -20,7 +20,9 @@ import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.security.SecurityProperties; import org.springframework.boot.test.IntegrationTest; import org.springframework.boot.test.SpringApplicationConfiguration; import org.springframework.boot.test.TestRestTemplate; @@ -47,6 +49,9 @@ import static org.junit.Assert.assertTrue; @ActiveProfiles("endpoints") public class EndpointsPropertiesSampleActuatorApplicationTests { + @Autowired + private SecurityProperties security; + @Value("${local.server.port}") private int port; @@ -64,8 +69,9 @@ public class EndpointsPropertiesSampleActuatorApplicationTests { @Test public void testCustomContextPath() throws Exception { - ResponseEntity entity = new TestRestTemplate().getForEntity( - "http://localhost:" + this.port + "/admin/health", String.class); + ResponseEntity entity = new TestRestTemplate("user", getPassword()) + .getForEntity("http://localhost:" + this.port + "/admin/health", + String.class); assertEquals(HttpStatus.OK, entity.getStatusCode()); assertTrue("Wrong body: " + entity.getBody(), entity.getBody().contains("\"status\":\"UP\"")); @@ -73,4 +79,9 @@ public class EndpointsPropertiesSampleActuatorApplicationTests { assertTrue("Wrong body: " + entity.getBody(), entity.getBody().contains("\"hello\":\"world\"")); } + + private String getPassword() { + return this.security.getUser().getPassword(); + } + } diff --git a/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/NonSensitiveHealthTests.java b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/NonSensitiveHealthTests.java new file mode 100644 index 00000000000..90c6bf90d1c --- /dev/null +++ b/spring-boot-samples/spring-boot-sample-actuator/src/test/java/sample/actuator/NonSensitiveHealthTests.java @@ -0,0 +1,58 @@ +/* + * Copyright 2012-2014 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package sample.actuator; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.test.IntegrationTest; +import org.springframework.boot.test.SpringApplicationConfiguration; +import org.springframework.boot.test.TestRestTemplate; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.context.web.WebAppConfiguration; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Tests for /health with {@code endpoints.health.sensitive=false}. + * + * @author Phillip Webb + */ +@RunWith(SpringJUnit4ClassRunner.class) +@SpringApplicationConfiguration(classes = SampleActuatorApplication.class) +@WebAppConfiguration +@IntegrationTest({ "server.port=0", "endpoints.health.sensitive=false" }) +@DirtiesContext +public class NonSensitiveHealthTests { + + @Value("${local.server.port}") + private int port; + + @Test + public void testSecureHealth() throws Exception { + ResponseEntity entity = new TestRestTemplate().getForEntity( + "http://localhost:" + this.port + "/health", String.class); + assertEquals(HttpStatus.OK, entity.getStatusCode()); + assertTrue("Wrong body: " + entity.getBody(), + entity.getBody().contains("\"hello\":1")); + } + +}