Simplify HealthMvcEndpoint security

Expose full health details if management security is disabled or
management role is present.

Fixes gh-7604
Closes gh-7852
This commit is contained in:
Madhura Bhave 2017-01-03 16:07:39 -08:00 committed by Phillip Webb
parent f8a53cf775
commit 530c3cd3be
6 changed files with 81 additions and 180 deletions

View File

@ -54,7 +54,6 @@ import org.springframework.context.annotation.ConditionContext;
import org.springframework.context.annotation.Conditional;
import org.springframework.core.env.Environment;
import org.springframework.core.type.AnnotatedTypeMetadata;
import org.springframework.util.ClassUtils;
import org.springframework.util.CollectionUtils;
import org.springframework.util.StringUtils;
import org.springframework.web.cors.CorsConfiguration;
@ -162,7 +161,7 @@ public class EndpointWebMvcManagementContextConfiguration {
@ConditionalOnEnabledEndpoint("health")
public HealthMvcEndpoint healthMvcEndpoint(HealthEndpoint delegate) {
HealthMvcEndpoint healthMvcEndpoint = new HealthMvcEndpoint(delegate,
isHealthSecure());
this.managementServerProperties.getSecurity().isEnabled());
if (this.healthMvcEndpointProperties.getMapping() != null) {
healthMvcEndpoint
.addStatusMapping(this.healthMvcEndpointProperties.getMapping());
@ -206,17 +205,6 @@ public class EndpointWebMvcManagementContextConfiguration {
return new AuditEventsMvcEndpoint(auditEventRepository);
}
private boolean isHealthSecure() {
return isSpringSecurityAvailable()
&& this.managementServerProperties.getSecurity().isEnabled();
}
private boolean isSpringSecurityAvailable() {
return ClassUtils.isPresent(
"org.springframework.security.config.annotation.web.WebSecurityConfigurer",
getClass().getClassLoader());
}
private static class LogFileCondition extends SpringBootCondition {
@Override

View File

@ -16,7 +16,7 @@
package org.springframework.boot.actuate.cloudfoundry;
import java.security.Principal;
import javax.servlet.http.HttpServletRequest;
import org.springframework.boot.actuate.endpoint.HealthEndpoint;
import org.springframework.boot.actuate.endpoint.mvc.HealthMvcEndpoint;
@ -36,7 +36,7 @@ class CloudFoundryHealthMvcEndpoint extends HealthMvcEndpoint {
}
@Override
protected boolean exposeHealthDetails(Principal principal) {
protected boolean exposeHealthDetails(HttpServletRequest request) {
return true;
}

View File

@ -16,12 +16,11 @@
package org.springframework.boot.actuate.endpoint.mvc;
import java.security.Principal;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.servlet.http.HttpServletRequest;
import org.springframework.boot.actuate.endpoint.HealthEndpoint;
import org.springframework.boot.actuate.health.Health;
import org.springframework.boot.actuate.health.Status;
@ -33,10 +32,7 @@ import org.springframework.core.env.Environment;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.GrantedAuthority;
import org.springframework.util.Assert;
import org.springframework.util.ClassUtils;
import org.springframework.util.StringUtils;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.ResponseBody;
@ -49,6 +45,7 @@ import org.springframework.web.bind.annotation.ResponseBody;
* @author Andy Wilkinson
* @author Phillip Webb
* @author Eddú Meléndez
* @author Madhura Bhave
* @since 1.1.0
*/
@ConfigurationProperties(prefix = "endpoints.health")
@ -59,11 +56,7 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
private Map<String, HttpStatus> statusMapping = new HashMap<String, HttpStatus>();
private RelaxedPropertyResolver healthPropertyResolver;
private RelaxedPropertyResolver endpointPropertyResolver;
private RelaxedPropertyResolver roleResolver;
private RelaxedPropertyResolver securityPropertyResolver;
private long lastAccess = 0;
@ -86,11 +79,7 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
@Override
public void setEnvironment(Environment environment) {
this.healthPropertyResolver = new RelaxedPropertyResolver(environment,
"endpoints.health.");
this.endpointPropertyResolver = new RelaxedPropertyResolver(environment,
"endpoints.");
this.roleResolver = new RelaxedPropertyResolver(environment,
this.securityPropertyResolver = new RelaxedPropertyResolver(environment,
"management.security.");
}
@ -136,12 +125,12 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
@RequestMapping(produces = MediaType.APPLICATION_JSON_VALUE)
@ResponseBody
public Object invoke(Principal principal) {
public Object invoke(HttpServletRequest request) {
if (!getDelegate().isEnabled()) {
// Shouldn't happen because the request mapping should not be registered
return getDisabledResponse();
}
Health health = getHealth(principal);
Health health = getHealth(request);
HttpStatus status = getStatus(health);
if (status != null) {
return new ResponseEntity<Health>(health, status);
@ -163,13 +152,13 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
return null;
}
private Health getHealth(Principal principal) {
private Health getHealth(HttpServletRequest request) {
long accessTime = System.currentTimeMillis();
if (isCacheStale(accessTime)) {
this.lastAccess = accessTime;
this.cached = getDelegate().invoke();
}
if (exposeHealthDetails(principal)) {
if (exposeHealthDetails(request)) {
return this.cached;
}
return Health.status(this.cached.getStatus()).build();
@ -182,44 +171,19 @@ public class HealthMvcEndpoint extends AbstractEndpointMvcAdapter<HealthEndpoint
return (accessTime - this.lastAccess) >= getDelegate().getTimeToLive();
}
protected boolean exposeHealthDetails(Principal principal) {
return isSecure(principal) || isUnrestricted();
}
private boolean isSecure(Principal principal) {
if (principal == null || principal.getClass().getName().contains("Anonymous")) {
return false;
protected boolean exposeHealthDetails(HttpServletRequest request) {
if (!this.secure) {
return true;
}
if (isSpringSecurityAuthentication(principal)) {
Authentication authentication = (Authentication) principal;
List<String> roles = Arrays.asList(StringUtils
.trimArrayElements(StringUtils.commaDelimitedListToStringArray(
this.roleResolver.getProperty("roles", "ROLE_ACTUATOR"))));
for (GrantedAuthority authority : authentication.getAuthorities()) {
String name = authority.getAuthority();
for (String role : roles) {
if (role.equals(name) || ("ROLE_" + role).equals(name)) {
return true;
}
}
String[] roles = StringUtils.commaDelimitedListToStringArray(
this.securityPropertyResolver.getProperty("roles", "ROLE_ACTUATOR"));
roles = StringUtils.trimArrayElements(roles);
for (String role : roles) {
if (request.isUserInRole(role) || request.isUserInRole("ROLE_" + role)) {
return true;
}
}
return false;
}
private boolean isSpringSecurityAuthentication(Principal principal) {
return ClassUtils.isPresent("org.springframework.security.core.Authentication",
null) && (principal instanceof Authentication);
}
private boolean isUnrestricted() {
Boolean sensitive = this.healthPropertyResolver.getProperty("sensitive",
Boolean.class);
if (sensitive == null) {
sensitive = this.endpointPropertyResolver.getProperty("sensitive",
Boolean.class);
}
return !this.secure && !Boolean.TRUE.equals(sensitive);
}
}

View File

@ -32,6 +32,7 @@ import org.springframework.boot.autoconfigure.web.WebMvcAutoConfiguration;
import org.springframework.boot.test.util.EnvironmentTestUtils;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockServletContext;
import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
@ -60,8 +61,9 @@ public class HealthMvcEndpointAutoConfigurationTests {
this.context.setServletContext(new MockServletContext());
this.context.register(TestConfiguration.class);
this.context.refresh();
MockHttpServletRequest request = new MockHttpServletRequest();
Health health = (Health) this.context.getBean(HealthMvcEndpoint.class)
.invoke(null);
.invoke(request);
assertThat(health.getStatus()).isEqualTo(Status.UP);
assertThat(health.getDetails().get("foo")).isNull();
}

View File

@ -18,20 +18,21 @@ package org.springframework.boot.actuate.endpoint.mvc;
import java.util.Collections;
import javax.servlet.http.HttpServletRequest;
import org.junit.Before;
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.boot.test.util.EnvironmentTestUtils;
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;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockServletContext;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.BDDMockito.given;
@ -44,36 +45,36 @@ import static org.mockito.Mockito.mock;
* @author Dave Syer
* @author Andy Wilkinson
* @author Eddú Meléndez
* @author Madhura Bhave
*/
public class HealthMvcEndpointTests {
private static final PropertySource<?> NON_SENSITIVE = new MapPropertySource("test",
Collections.<String, Object>singletonMap("endpoints.health.sensitive",
"false"));
private static final PropertySource<?> SECURITY_ROLES = new MapPropertySource("test",
Collections.<String, Object>singletonMap("management.security.roles",
"HERO, USER"));
private HttpServletRequest request = new MockHttpServletRequest();
private HealthEndpoint endpoint = null;
private HealthMvcEndpoint mvc = null;
private MockEnvironment environment;
private UsernamePasswordAuthenticationToken user = createAuthenticationToken(
private HttpServletRequest user = createAuthenticationToken(
"ROLE_USER");
private UsernamePasswordAuthenticationToken actuator = createAuthenticationToken(
private HttpServletRequest actuator = createAuthenticationToken(
"ROLE_ACTUATOR");
private UsernamePasswordAuthenticationToken hero = createAuthenticationToken(
private HttpServletRequest hero = createAuthenticationToken(
"ROLE_HERO");
private UsernamePasswordAuthenticationToken createAuthenticationToken(
String authority) {
return new UsernamePasswordAuthenticationToken("user", "password",
AuthorityUtils.commaSeparatedStringToAuthorityList(authority));
private HttpServletRequest createAuthenticationToken(
String role) {
MockServletContext servletContext = new MockServletContext();
servletContext.declareRoles(role);
return new MockHttpServletRequest(servletContext);
}
@Before
@ -88,7 +89,7 @@ public class HealthMvcEndpointTests {
@Test
public void up() {
given(this.endpoint.invoke()).willReturn(new Health.Builder().up().build());
Object result = this.mvc.invoke(null);
Object result = this.mvc.invoke(this.request);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
}
@ -97,7 +98,7 @@ public class HealthMvcEndpointTests {
@Test
public void down() {
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
Object result = this.mvc.invoke(null);
Object result = this.mvc.invoke(this.request);
assertThat(result instanceof ResponseEntity).isTrue();
ResponseEntity<Health> response = (ResponseEntity<Health>) result;
assertThat(response.getBody().getStatus() == Status.DOWN).isTrue();
@ -111,7 +112,7 @@ public class HealthMvcEndpointTests {
.willReturn(new Health.Builder().status("OK").build());
this.mvc.setStatusMapping(
Collections.singletonMap("OK", HttpStatus.INTERNAL_SERVER_ERROR));
Object result = this.mvc.invoke(null);
Object result = this.mvc.invoke(this.request);
assertThat(result instanceof ResponseEntity).isTrue();
ResponseEntity<Health> response = (ResponseEntity<Health>) result;
assertThat(response.getBody().getStatus().equals(new Status("OK"))).isTrue();
@ -125,7 +126,7 @@ public class HealthMvcEndpointTests {
.willReturn(new Health.Builder().outOfService().build());
this.mvc.setStatusMapping(Collections.singletonMap("out-of-service",
HttpStatus.INTERNAL_SERVER_ERROR));
Object result = this.mvc.invoke(null);
Object result = this.mvc.invoke(this.request);
assertThat(result instanceof ResponseEntity).isTrue();
ResponseEntity<Health> response = (ResponseEntity<Health>) result;
assertThat(response.getBody().getStatus().equals(Status.OUT_OF_SERVICE)).isTrue();
@ -133,10 +134,9 @@ public class HealthMvcEndpointTests {
}
@Test
public void secureEvenWhenNotSensitive() {
public void presenceOfRightRoleShouldExposeDetails() {
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
given(this.endpoint.isSensitive()).willReturn(false);
Object result = this.mvc.invoke(this.actuator);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
@ -144,7 +144,18 @@ public class HealthMvcEndpointTests {
}
@Test
public void secureNonAdmin() {
public void managementSecurityDisabledShouldExposeDetails() throws Exception {
this.mvc = new HealthMvcEndpoint(this.endpoint, false);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(this.user);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
}
@Test
public void rightRoleNotPresentShouldNotExposeDetails() {
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(this.user);
@ -154,7 +165,7 @@ public class HealthMvcEndpointTests {
}
@Test
public void secureCustomRole() {
public void customRolePresentShouldExposeDetails() {
this.environment.getPropertySources().addLast(SECURITY_ROLES);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
@ -165,7 +176,7 @@ public class HealthMvcEndpointTests {
}
@Test
public void secureCustomRoleNoAccess() {
public void customRoleShouldNotExposeDetailsForDefaultRole() {
this.environment.getPropertySources().addLast(SECURITY_ROLES);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
@ -178,7 +189,6 @@ public class HealthMvcEndpointTests {
@Test
public void healthIsCached() {
given(this.endpoint.getTimeToLive()).willReturn(10000L);
given(this.endpoint.isSensitive()).willReturn(true);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(this.actuator);
@ -188,7 +198,7 @@ public class HealthMvcEndpointTests {
assertThat(health.getDetails()).hasSize(1);
assertThat(health.getDetails().get("foo")).isEqualTo("bar");
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
result = this.mvc.invoke(null); // insecure now
result = this.mvc.invoke(this.request); // insecure now
assertThat(result instanceof Health).isTrue();
health = (Health) result;
// so the result is cached
@ -197,52 +207,16 @@ public class HealthMvcEndpointTests {
assertThat(health.getDetails()).isEmpty();
}
@Test
public void insecureAnonymousAccessUnrestricted() {
this.mvc = new HealthMvcEndpoint(this.endpoint, false);
this.mvc.setEnvironment(this.environment);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(null);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
}
@Test
public void insensitiveAnonymousAccessRestricted() {
this.environment.getPropertySources().addLast(NON_SENSITIVE);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(null);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
assertThat(((Health) result).getDetails().get("foo")).isNull();
}
@Test
public void insecureInsensitiveAnonymousAccessUnrestricted() {
this.mvc = new HealthMvcEndpoint(this.endpoint, false);
this.mvc.setEnvironment(this.environment);
this.environment.getPropertySources().addLast(NON_SENSITIVE);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(null);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
assertThat(((Health) result).getDetails().get("foo")).isEqualTo("bar");
}
@Test
public void noCachingWhenTimeToLiveIsZero() {
given(this.endpoint.getTimeToLive()).willReturn(0L);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(null);
Object result = this.mvc.invoke(this.request);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
result = this.mvc.invoke(null);
result = this.mvc.invoke(this.request);
@SuppressWarnings("unchecked")
Health health = ((ResponseEntity<Health>) result).getBody();
assertThat(health.getStatus() == Status.DOWN).isTrue();
@ -251,59 +225,16 @@ public class HealthMvcEndpointTests {
@Test
public void newValueIsReturnedOnceTtlExpires() throws InterruptedException {
given(this.endpoint.getTimeToLive()).willReturn(50L);
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);
Object result = this.mvc.invoke(this.request);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
Thread.sleep(100);
given(this.endpoint.invoke()).willReturn(new Health.Builder().down().build());
result = this.mvc.invoke(null);
result = this.mvc.invoke(this.request);
@SuppressWarnings("unchecked")
Health health = ((ResponseEntity<Health>) result).getBody();
assertThat(health.getStatus() == Status.DOWN).isTrue();
}
@Test
public void detailIsHiddenWhenAllEndpointsAreSensitive() {
EnvironmentTestUtils.addEnvironment(this.environment, "endpoints.sensitive:true");
this.mvc = new HealthMvcEndpoint(this.endpoint, false);
this.mvc.setEnvironment(this.environment);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(null);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
assertThat(((Health) result).getDetails().get("foo")).isNull();
}
@Test
public void detailIsHiddenWhenHealthEndpointIsSensitive() {
EnvironmentTestUtils.addEnvironment(this.environment,
"endpoints.health.sensitive:true");
this.mvc = new HealthMvcEndpoint(this.endpoint, false);
this.mvc.setEnvironment(this.environment);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(null);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
assertThat(((Health) result).getDetails().get("foo")).isNull();
}
@Test
public void detailIsHiddenWhenOnlyHealthEndpointIsSensitive() {
EnvironmentTestUtils.addEnvironment(this.environment,
"endpoints.health.sensitive:true", "endpoints.sensitive:false");
this.mvc = new HealthMvcEndpoint(this.endpoint, false);
this.mvc.setEnvironment(this.environment);
given(this.endpoint.invoke())
.willReturn(new Health.Builder().up().withDetail("foo", "bar").build());
Object result = this.mvc.invoke(null);
assertThat(result instanceof Health).isTrue();
assertThat(((Health) result).getStatus() == Status.UP).isTrue();
assertThat(((Health) result).getDetails().get("foo")).isNull();
}
}

View File

@ -32,6 +32,7 @@ import org.springframework.boot.autoconfigure.web.HttpMessageConvertersAutoConfi
import org.springframework.boot.autoconfigure.web.WebMvcAutoConfiguration;
import org.springframework.boot.junit.runner.classpath.ClassPathExclusions;
import org.springframework.boot.junit.runner.classpath.ModifiedClassPathRunner;
import org.springframework.boot.test.util.EnvironmentTestUtils;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.mock.web.MockServletContext;
@ -48,6 +49,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.
* Integration tests for the health endpoint when Spring Security is not available.
*
* @author Andy Wilkinson
* @author Madhura Bhave
*/
@RunWith(ModifiedClassPathRunner.class)
@ClassPathExclusions("spring-security-*.jar")
@ -61,14 +63,28 @@ public class NoSpringSecurityHealthMvcEndpointIntegrationTests {
}
@Test
public void healthDetailIsPresent() throws Exception {
public void healthDetailNotPresent() throws Exception {
this.context = new AnnotationConfigWebApplicationContext();
this.context.setServletContext(new MockServletContext());
this.context.register(TestConfiguration.class);
this.context.refresh();
MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.context).build();
mockMvc.perform(get("/health")).andExpect(status().isOk())
.andExpect(content().string(containsString("\"hello\":\"world\"")));
.andExpect(content().string(containsString("\"status\":\"UP\"")));
}
@Test
public void healthDetailPresent() throws Exception {
this.context = new AnnotationConfigWebApplicationContext();
this.context.setServletContext(new MockServletContext());
this.context.register(TestConfiguration.class);
EnvironmentTestUtils.addEnvironment(this.context,
"management.security.enabled:false");
this.context.refresh();
MockMvc mockMvc = MockMvcBuilders.webAppContextSetup(this.context).build();
mockMvc.perform(get("/health")).andExpect(status().isOk())
.andExpect(content().string(containsString(
"\"status\":\"UP\",\"test\":{\"status\":\"UP\",\"hello\":\"world\"}")));
}
@ImportAutoConfiguration({ JacksonAutoConfiguration.class,