diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle index 405897da0..3ef942d18 100644 --- a/fiat-api/fiat-api.gradle +++ b/fiat-api/fiat-api.gradle @@ -21,6 +21,7 @@ dependencies { compileOnly spinnaker.dependency("frigga") compileOnly spinnaker.dependency("korkSecurity") compileOnly spinnaker.dependency("korkWeb") + compileOnly spinnaker.dependency("kork") compileOnly spinnaker.dependency("lombok") compileOnly spinnaker.dependency("okHttp") compileOnly spinnaker.dependency("okHttpUrlconnection") @@ -39,6 +40,7 @@ dependencies { testCompile spinnaker.dependency("slf4jApi") testCompile spinnaker.dependency("frigga") testCompile spinnaker.dependency("korkSecurity") + testCompile spinnaker.dependency("kork") testCompile spinnaker.dependency("bootAutoConfigure") testCompile spinnaker.dependency("spectatorApi") } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java index 830bd80eb..20303d737 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java @@ -26,18 +26,17 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; +import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.context.properties.EnableConfigurationProperties; -import org.springframework.boot.web.servlet.FilterRegistrationBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.ComponentScan; import org.springframework.context.annotation.Configuration; -import org.springframework.core.Ordered; import org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; +import org.springframework.security.web.context.SecurityContextPersistenceFilter; import retrofit.Endpoints; import retrofit.RestAdapter; import retrofit.client.Client; @@ -77,25 +76,36 @@ public FiatService fiatService(FiatClientConfigurationProperties fiatConfigurati } @Bean - FilterRegistrationBean fiatFilterRegistrationBean(FiatAuthenticationFilter filter) { - val frb = new FilterRegistrationBean(filter); - frb.setOrder(Ordered.HIGHEST_PRECEDENCE + 1); - frb.addUrlPatterns("/*"); - return frb; + FiatWebSecurityConfigurerAdapter fiatSecurityConfig(@Value("${services.fiat.enabled:false}") boolean isFiatEnabled) { + return new FiatWebSecurityConfigurerAdapter(isFiatEnabled); } - @ConditionalOnExpression("!${services.fiat.enabled:false}") - @Bean - AnonymousConfig anonymousConfig() { - return new AnonymousConfig(); - } + private class FiatWebSecurityConfigurerAdapter extends WebSecurityConfigurerAdapter { + private final boolean isFiatEnabled; + + private FiatWebSecurityConfigurerAdapter(boolean isFiatEnabled) { + this.isFiatEnabled = isFiatEnabled; + } - private class AnonymousConfig extends WebSecurityConfigurerAdapter { @Override protected void configure(HttpSecurity http) throws Exception { - // TODO(ttomsu): Make management endpoints non-sensitive? - log.info("Fiat service is disabled. Setting Spring Security to allow all traffic."); - http.authorizeRequests().anyRequest().permitAll().and().csrf().disable(); + if (isFiatEnabled) { + /* + * Having `FiatAuthenticationFilter` prior to `SecurityContextPersistenceFilter` results in the + * `SecurityContextHolder` being overridden with a null value. + * + * The null value then causes the `AnonymousAuthenticationFilter` to inject an "anonymousUser" which when + * passed over the wire to fiat is promptly rejected. + * + * This behavior is triggered when `management.security.enabled` is `false`. + */ + http + .csrf().disable() + .addFilterAfter(new FiatAuthenticationFilter(), SecurityContextPersistenceFilter.class); + } else { + log.info("Fiat service is disabled. Setting Spring Security to allow all traffic."); + http.authorizeRequests().anyRequest().permitAll().and().csrf().disable(); + } } } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationFilter.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationFilter.java index feee4a11d..ef91b2271 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationFilter.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationFilter.java @@ -24,14 +24,12 @@ import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; -import org.springframework.stereotype.Component; import javax.servlet.*; import java.io.IOException; import java.util.ArrayList; @Slf4j -@Component public class FiatAuthenticationFilter implements Filter { @Override diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java index 5ba3a1b6b..3fbcb2187 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java @@ -26,6 +26,7 @@ import com.netflix.spinnaker.fiat.model.UserPermission; import com.netflix.spinnaker.fiat.model.resources.Authorizable; import com.netflix.spinnaker.fiat.model.resources.ResourceType; +import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService; import com.netflix.spinnaker.security.AuthenticatedRequest; import com.netflix.spinnaker.security.User; import lombok.extern.slf4j.Slf4j; @@ -37,6 +38,7 @@ import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Component; +import java.io.File; import java.io.Serializable; import java.util.Set; import java.util.concurrent.ExecutionException; @@ -47,7 +49,7 @@ @Component @Slf4j public class FiatPermissionEvaluator implements PermissionEvaluator { - + private final DynamicConfigService dynamicConfigService; private final Registry registry; private final FiatService fiatService; private final FiatClientConfigurationProperties configProps; @@ -57,9 +59,11 @@ public class FiatPermissionEvaluator implements PermissionEvaluator { private final Id getPermissionCounterId; @Autowired - public FiatPermissionEvaluator(Registry registry, + public FiatPermissionEvaluator(DynamicConfigService dynamicConfigService, + Registry registry, FiatService fiatService, FiatClientConfigurationProperties configProps) { + this.dynamicConfigService = dynamicConfigService; this.registry = registry; this.fiatService = fiatService; this.configProps = configProps; @@ -86,7 +90,7 @@ public boolean hasPermission(Authentication authentication, Serializable resourceName, String resourceType, Object authorization) { - if (!configProps.isEnabled()) { + if (!isEnabled()) { return true; } if (resourceName == null || resourceType == null || authorization == null) { @@ -168,7 +172,7 @@ public UserPermission.View getPermission(String username) { @SuppressWarnings("unused") @Deprecated public boolean storeWholePermission() { - if (!configProps.isEnabled()) { + if (!isEnabled()) { return true; } @@ -210,4 +214,8 @@ private boolean permissionContains(UserPermission.View permission, public boolean isAdmin() { return true; // TODO(ttomsu): Chosen by fair dice roll. Guaranteed to be random. } + + private boolean isEnabled() { + return dynamicConfigService.isEnabled("fiat", configProps.isEnabled()); + } } diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy index bfa51f7ae..3e9c9d559 100644 --- a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy @@ -25,6 +25,7 @@ import com.netflix.spinnaker.fiat.model.resources.Permissions import com.netflix.spinnaker.fiat.model.resources.ResourceType import com.netflix.spinnaker.fiat.model.resources.Role import com.netflix.spinnaker.fiat.model.resources.ServiceAccount +import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService import org.springframework.security.core.Authentication import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken import spock.lang.Specification @@ -32,11 +33,15 @@ import spock.lang.Subject import spock.lang.Unroll class FiatPermissionEvaluatorSpec extends Specification { + DynamicConfigService dynamicConfigService = Mock(DynamicConfigService) { + _ * isEnabled('fiat', true) >> { return true } + } FiatService fiatService = Mock(FiatService) Registry registry = new NoopRegistry(); @Subject FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator( + dynamicConfigService, registry, fiatService, buildConfigurationProperties() diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java index f57081001..61aebe100 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/controllers/AuthorizeController.java @@ -16,6 +16,8 @@ package com.netflix.spinnaker.fiat.controllers; +import com.netflix.spectator.api.Id; +import com.netflix.spectator.api.Registry; import com.netflix.spinnaker.fiat.config.FiatServerConfigurationProperties; import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig; import com.netflix.spinnaker.fiat.model.Authorization; @@ -48,16 +50,22 @@ @RequestMapping("/authorize") public class AuthorizeController { + private final Registry registry; private final PermissionsRepository permissionsRepository; private final FiatServerConfigurationProperties configProps; + private final Id getUserPermissionCounterId; + @Autowired - public AuthorizeController(PermissionsRepository permissionsRepository, + public AuthorizeController(Registry registry, + PermissionsRepository permissionsRepository, FiatServerConfigurationProperties configProps) { + this.registry = registry; this.permissionsRepository = permissionsRepository; this.configProps = configProps; - } + this.getUserPermissionCounterId = registry.createId("fiat.getUserPermissionCounterId"); + } @ApiOperation(value = "Used mostly for testing. Not really any real value to the rest of " + "the system. Disabled by default.") @@ -207,6 +215,11 @@ private Optional getUserPermissionOrDefault(String userId) { ).orElse(null); if (userPermission != null || !configProps.isDefaultToUnrestrictedUser()) { + registry.counter( + getUserPermissionCounterId + .withTag("success", userPermission != null) + .withTag("fallback", false) + ).increment(); return Optional.ofNullable(userPermission); } @@ -223,6 +236,12 @@ private Optional getUserPermissionOrDefault(String userId) { .orElse(null); } + registry.counter( + getUserPermissionCounterId + .withTag("success", userPermission != null) + .withTag("fallback", true) + ).increment(); + return Optional.ofNullable(userPermission); } } diff --git a/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy index a82052d2b..43ecffdd3 100644 --- a/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy +++ b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/AuthorizeControllerSpec.groovy @@ -18,6 +18,8 @@ package com.netflix.spinnaker.fiat.controllers import com.fasterxml.jackson.databind.ObjectMapper import com.netflix.hystrix.strategy.HystrixPlugins +import com.netflix.spectator.api.NoopRegistry +import com.netflix.spectator.api.Registry import com.netflix.spinnaker.config.FiatSystemTest import com.netflix.spinnaker.config.TestUserRoleProviderConfig import com.netflix.spinnaker.fiat.config.FiatServerConfigurationProperties @@ -33,6 +35,7 @@ import org.springframework.test.web.servlet.MockMvc import org.springframework.test.web.servlet.setup.MockMvcBuilders import org.springframework.web.context.WebApplicationContext import spock.lang.AutoCleanup +import spock.lang.Shared import spock.lang.Specification import spock.lang.Unroll @@ -42,6 +45,8 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers. @FiatSystemTest class AuthorizeControllerSpec extends Specification { + @Shared + Registry registry = new NoopRegistry() @Autowired WebApplicationContext wac @@ -146,7 +151,7 @@ class AuthorizeControllerSpec extends Specification { def "should get user from repo"() { setup: PermissionsRepository repository = Mock(PermissionsRepository) - AuthorizeController controller = new AuthorizeController(repository, fiatServerConfigurationProperties) + AuthorizeController controller = new AuthorizeController(registry, repository, fiatServerConfigurationProperties) def foo = new UserPermission().setId("foo@batman.com") @@ -168,7 +173,7 @@ class AuthorizeControllerSpec extends Specification { def "should get user's accounts from repo"() { setup: PermissionsRepository repository = Mock(PermissionsRepository) - AuthorizeController controller = new AuthorizeController(repository, fiatServerConfigurationProperties) + AuthorizeController controller = new AuthorizeController(registry, repository, fiatServerConfigurationProperties) def bar = new Account().setName("bar") def foo = new UserPermission().setId("foo").setAccounts([bar] as Set) @@ -243,6 +248,7 @@ class AuthorizeControllerSpec extends Specification { def "should fallback to unrestricted user if no session available"() { given: def authorizeController = new AuthorizeController( + registry, permissionsRepository, new FiatServerConfigurationProperties(defaultToUnrestrictedUser: defaultToUnrestrictedUser) )