feat(api): Support running with management.security.enabled: false (#…

…232)

This PR re-orders how the `FiatAuthenticationFilter` is inserted into
the http chain.

Previously when management security was disabled, the caller was
redirected to `/login`.

- introduce a metric to track how often the unregistered user fallback is hit
- support dynamically disabling the permission evaluator

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")
 }

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();
+      }
     }
   }
 

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

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());
+  }
 }

fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy

@@ -25,18 +25,23 @@ 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
 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()

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<UserPermission> 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<UserPermission> getUserPermissionOrDefault(String userId) {
           .orElse(null);
     }
 
+    registry.counter(
+        getUserPermissionCounterId
+            .withTag("success", userPermission != null)
+            .withTag("fallback", true)
+    ).increment();
+
     return Optional.ofNullable(userPermission);
   }
 }

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("[email protected]")
 
@@ -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)
     )