diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle index 1c1b8f51b..b9fffe5a6 100644 --- a/fiat-api/fiat-api.gradle +++ b/fiat-api/fiat-api.gradle @@ -16,6 +16,8 @@ dependencies { api "io.spinnaker.kork:kork-api" + api "io.spinnaker.kork:kork-secrets" + api "io.spinnaker.kork:kork-security" implementation project(":fiat-core") @@ -25,7 +27,6 @@ dependencies { implementation "org.springframework.security:spring-security-web" implementation "com.netflix.spectator:spectator-api" implementation "io.spinnaker.kork:kork-core" - implementation "io.spinnaker.kork:kork-security" implementation "io.spinnaker.kork:kork-telemetry" implementation "io.spinnaker.kork:kork-web" implementation "io.spinnaker.kork:kork-retrofit" @@ -39,4 +40,5 @@ dependencies { implementation "com.github.ben-manes.caffeine:caffeine" testImplementation "org.slf4j:slf4j-api" + testImplementation "org.springframework.security:spring-security-test" } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/AuthenticatedRequestAuthenticationConverter.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/AuthenticatedRequestAuthenticationConverter.java index 56f837705..03e6534a7 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/AuthenticatedRequestAuthenticationConverter.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/AuthenticatedRequestAuthenticationConverter.java @@ -19,9 +19,7 @@ import com.netflix.spinnaker.security.AuthenticatedRequest; import java.util.List; import javax.servlet.http.HttpServletRequest; -import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.core.Authentication; -import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.web.authentication.AuthenticationConverter; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; @@ -38,11 +36,6 @@ public Authentication convert(HttpServletRequest request) { .map( user -> (Authentication) new PreAuthenticatedAuthenticationToken(user, "N/A", List.of())) - .orElseGet( - () -> - new AnonymousAuthenticationToken( - "anonymous", - "anonymous", - AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS"))); + .orElse(FiatWebSecurityConfigurerAdapter.ANONYMOUS); } } 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 c4d0a6cd0..b8a2edcca 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 @@ -41,10 +41,7 @@ import org.springframework.context.annotation.Import; import org.springframework.core.annotation.Order; 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.authentication.AnonymousAuthenticationFilter; import org.springframework.security.web.authentication.AuthenticationConverter; import retrofit.Endpoints; import retrofit.RestAdapter; @@ -112,41 +109,10 @@ AuthenticationConverter defaultAuthenticationConverter() { return new AuthenticatedRequestAuthenticationConverter(); } - @Bean - FiatWebSecurityConfigurerAdapter fiatSecurityConfig( - FiatStatus fiatStatus, AuthenticationConverter authenticationConverter) { - return new FiatWebSecurityConfigurerAdapter(fiatStatus, authenticationConverter); - } - @Bean @Order(HIGHEST_PRECEDENCE) FiatAccessDeniedExceptionHandler fiatAccessDeniedExceptionHandler( ExceptionMessageDecorator exceptionMessageDecorator) { return new FiatAccessDeniedExceptionHandler(exceptionMessageDecorator); } - - private static class FiatWebSecurityConfigurerAdapter extends WebSecurityConfigurerAdapter { - private final FiatStatus fiatStatus; - private final AuthenticationConverter authenticationConverter; - - private FiatWebSecurityConfigurerAdapter( - FiatStatus fiatStatus, AuthenticationConverter authenticationConverter) { - super(true); - this.fiatStatus = fiatStatus; - this.authenticationConverter = authenticationConverter; - } - - @Override - protected void configure(HttpSecurity http) throws Exception { - http.servletApi() - .and() - .exceptionHandling() - .and() - .anonymous() - .and() - .addFilterBefore( - new FiatAuthenticationFilter(fiatStatus, authenticationConverter), - AnonymousAuthenticationFilter.class); - } - } } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConverter.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConverter.java index 92d30494e..42d716081 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConverter.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConverter.java @@ -19,10 +19,8 @@ import com.netflix.spinnaker.fiat.model.SpinnakerAuthorities; import com.netflix.spinnaker.fiat.model.UserPermission; import com.netflix.spinnaker.kork.common.Header; -import java.util.List; import javax.servlet.http.HttpServletRequest; import lombok.RequiredArgsConstructor; -import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.core.Authentication; import org.springframework.security.web.authentication.AuthenticationConverter; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; @@ -46,7 +44,6 @@ public Authentication convert(HttpServletRequest request) { user, "N/A", permission.toGrantedAuthorities()); } } - return new AnonymousAuthenticationToken( - "anonymous", "anonymous", List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY)); + return FiatWebSecurityConfigurerAdapter.ANONYMOUS; } } 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 46f1ab8db..af942c684 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 @@ -21,15 +21,15 @@ import com.netflix.spectator.api.Id; import com.netflix.spectator.api.Registry; import com.netflix.spinnaker.fiat.model.Authorization; -import com.netflix.spinnaker.fiat.model.SpinnakerAuthorities; import com.netflix.spinnaker.fiat.model.UserPermission; import com.netflix.spinnaker.fiat.model.resources.Account; import com.netflix.spinnaker.fiat.model.resources.Authorizable; import com.netflix.spinnaker.fiat.model.resources.ResourceType; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; import com.netflix.spinnaker.kork.telemetry.caffeine.CaffeineStatsCounter; -import com.netflix.spinnaker.security.AccessControlled; +import com.netflix.spinnaker.security.AbstractPermissionEvaluator; import com.netflix.spinnaker.security.AuthenticatedRequest; +import com.netflix.spinnaker.security.SpinnakerUsers; import java.io.Serializable; import java.util.Arrays; import java.util.Collections; @@ -48,17 +48,14 @@ import org.apache.commons.lang3.StringUtils; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.HttpStatus; -import org.springframework.security.access.PermissionEvaluator; import org.springframework.security.core.Authentication; -import org.springframework.security.core.context.SecurityContextHolder; -import org.springframework.security.core.userdetails.UserDetails; import org.springframework.stereotype.Component; import org.springframework.util.backoff.BackOffExecution; import org.springframework.util.backoff.ExponentialBackOff; @Component @Slf4j -public class FiatPermissionEvaluator implements PermissionEvaluator { +public class FiatPermissionEvaluator extends AbstractPermissionEvaluator { private static final ThreadLocal authorizationFailure = new ThreadLocal<>(); private final Registry registry; @@ -146,37 +143,26 @@ private static RetryHandler buildRetryHandler( this.getPermissionCounterId = registry.createId("fiat.getPermission"); } + @Override + protected boolean isDisabled() { + return !fiatStatus.isEnabled(); + } + @Override public boolean hasPermission( Authentication authentication, Object resource, Object authorization) { if (!fiatStatus.isGrantedAuthoritiesEnabled()) { return false; } - if (!fiatStatus.isEnabled()) { - return true; - } - if (authentication == null || resource == null) { - log.warn( - "Permission denied because at least one of the required arguments was null. authentication={}, resource={}", - authentication, - resource); - return false; - } - if (authentication.getAuthorities().contains(SpinnakerAuthorities.ADMIN_AUTHORITY)) { - return true; - } - if (resource instanceof AccessControlled) { - return ((AccessControlled) resource).isAuthorized(authentication, authorization); - } - return false; + return super.hasPermission(authentication, resource, authorization); } public boolean canCreate(String resourceType, Object resource) { - if (!fiatStatus.isEnabled()) { + if (isDisabled()) { return true; } - String username = getUsername(SecurityContextHolder.getContext().getAuthentication()); + String username = SpinnakerUsers.getCurrentUserId(); try { return AuthenticatedRequest.propagate( @@ -208,7 +194,7 @@ public boolean canCreate(String resourceType, Object resource) { */ @SuppressWarnings("unused") public boolean hasCachedPermission(String username) { - if (!fiatStatus.isEnabled()) { + if (isDisabled()) { return true; } @@ -217,7 +203,7 @@ public boolean hasCachedPermission(String username) { public boolean hasPermission( String username, Serializable resourceName, String resourceType, Object authorization) { - if (!fiatStatus.isEnabled()) { + if (isDisabled()) { return true; } if (resourceName == null || resourceType == null || authorization == null) { @@ -266,18 +252,6 @@ public boolean hasPermission( return hasPermission; } - @Override - public boolean hasPermission( - Authentication authentication, - Serializable resourceName, - String resourceType, - Object authorization) { - if (!fiatStatus.isEnabled()) { - return true; - } - return hasPermission(getUsername(authentication), resourceName, resourceType, authorization); - } - /** * Invalidates the cached permissions for a user. * @@ -370,12 +344,12 @@ public UserPermission.View getPermission(String username) { @SuppressWarnings("unused") @Deprecated public boolean storeWholePermission() { - if (!fiatStatus.isEnabled()) { + if (isDisabled()) { return true; } - val authentication = SecurityContextHolder.getContext().getAuthentication(); - val permission = getPermission(getUsername(authentication)); + var user = SpinnakerUsers.getCurrentUserId(); + var permission = getPermission(user); return permission != null; } @@ -383,21 +357,6 @@ public static Optional getAuthorizationFailure() { return Optional.ofNullable(authorizationFailure.get()); } - private String getUsername(Authentication authentication) { - String username = "anonymous"; - if (authentication != null - && authentication.isAuthenticated() - && authentication.getPrincipal() != null) { - Object principal = authentication.getPrincipal(); - if (principal instanceof UserDetails) { - username = ((UserDetails) principal).getUsername(); - } else if (StringUtils.isNotEmpty(principal.toString())) { - username = principal.toString(); - } - } - return username; - } - private boolean permissionContains( UserPermission.View permission, String resourceName, @@ -500,10 +459,10 @@ public boolean isAdmin() { } public boolean isAdmin(Authentication authentication) { - if (!fiatStatus.isEnabled()) { + if (isDisabled()) { return true; } - UserPermission.View permission = getPermission(getUsername(authentication)); + UserPermission.View permission = getPermission(SpinnakerUsers.getUserId(authentication)); return permission != null && permission.isAdmin(); } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatWebSecurityConfigurerAdapter.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatWebSecurityConfigurerAdapter.java new file mode 100644 index 000000000..9b461165b --- /dev/null +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatWebSecurityConfigurerAdapter.java @@ -0,0 +1,64 @@ +/* + * Copyright 2024 Apple, Inc. + * + * 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 com.netflix.spinnaker.fiat.shared; + +import static org.springframework.security.config.Customizer.withDefaults; + +import com.netflix.spinnaker.security.SpinnakerAuthorities; +import com.netflix.spinnaker.security.SpinnakerUsers; +import java.util.List; +import org.springframework.security.authentication.AnonymousAuthenticationToken; +import org.springframework.security.config.annotation.web.builders.HttpSecurity; +import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.web.authentication.AnonymousAuthenticationFilter; +import org.springframework.security.web.authentication.AuthenticationConverter; +import org.springframework.stereotype.Component; + +@Component +public class FiatWebSecurityConfigurerAdapter extends WebSecurityConfigurerAdapter { + private static final String KEY = "spinnaker-anonymous"; + private static final Object PRINCIPAL = SpinnakerUsers.ANONYMOUS; + private static final List AUTHORITIES = + List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY); + static final AnonymousAuthenticationToken ANONYMOUS = + new AnonymousAuthenticationToken(KEY, PRINCIPAL, AUTHORITIES); + + private final FiatStatus fiatStatus; + private final AuthenticationConverter authenticationConverter; + + public FiatWebSecurityConfigurerAdapter( + FiatStatus fiatStatus, AuthenticationConverter authenticationConverter) { + super(true); + this.fiatStatus = fiatStatus; + this.authenticationConverter = authenticationConverter; + } + + @Override + protected void configure(HttpSecurity http) throws Exception { + http.servletApi(withDefaults()) + .exceptionHandling(withDefaults()) + .anonymous( + anonymous -> + // https://github.com/spinnaker/spinnaker/issues/6918 + // match the same anonymous userid as expected elsewhere + anonymous.principal(PRINCIPAL).key(KEY).authorities(AUTHORITIES)) + .addFilterBefore( + new FiatAuthenticationFilter(fiatStatus, authenticationConverter), + AnonymousAuthenticationFilter.class); + } +} 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 cf823a19a..981b88ab5 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 @@ -323,4 +323,32 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { 'WRITE' | false 'EXECUTE' | false } + + def "should evaluate permissions for AuthorizationMapControlled objects"() { + given: + def resource = new PermissionsControlledResource() + with(resource.permissions) { + add(Authorization.READ, 'integration group') + add(Authorization.WRITE, 'test group') + add(Authorization.EXECUTE, 'test group') + } + + when: + def hasPermission = evaluator.hasPermission(authentication, resource, authorization) + + then: + hasPermission == expectedHasPermission + + where: + authorization | expectedHasPermission + 'execute' | true + "execute" | true + 'EXECUTE' | true + "EXECUTE" | true + Authorization.EXECUTE | true + 'write' | true + 'WRITE' | true + 'read' | false + Authorization.READ | false + } } diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/PermissionsControlledResource.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/PermissionsControlledResource.groovy new file mode 100644 index 000000000..dd7ce2ddc --- /dev/null +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/PermissionsControlledResource.groovy @@ -0,0 +1,24 @@ +/* + * Copyright 2024 Apple, Inc. + * + * 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 com.netflix.spinnaker.fiat.shared + +import com.netflix.spinnaker.fiat.model.AuthorizationMapControlled +import com.netflix.spinnaker.fiat.model.resources.Permissions + +class PermissionsControlledResource implements AuthorizationMapControlled { + Permissions.Builder permissions = new Permissions.Builder() +} diff --git a/fiat-core/fiat-core.gradle b/fiat-core/fiat-core.gradle index 4559abafb..1ecb0f6ae 100644 --- a/fiat-core/fiat-core.gradle +++ b/fiat-core/fiat-core.gradle @@ -1,6 +1,6 @@ dependencies { - api "org.springframework.security:spring-security-core" + api "io.spinnaker.kork:kork-security:$korkVersion" implementation "com.fasterxml.jackson.core:jackson-annotations" implementation "com.google.code.findbugs:jsr305" diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java index 19a812180..5fdd8e59b 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java @@ -19,7 +19,9 @@ import java.util.Collections; import java.util.EnumSet; import java.util.Locale; +import java.util.Map; import java.util.Set; +import javax.annotation.CheckForNull; import javax.annotation.Nullable; public enum Authorization { @@ -28,14 +30,33 @@ public enum Authorization { EXECUTE, CREATE; + private static final Map + KORK_TO_FIAT = + Map.of( + com.netflix.spinnaker.security.Authorization.READ, READ, + com.netflix.spinnaker.security.Authorization.WRITE, WRITE, + com.netflix.spinnaker.security.Authorization.EXECUTE, EXECUTE, + com.netflix.spinnaker.security.Authorization.CREATE, CREATE); + public static final Set ALL = Collections.unmodifiableSet(EnumSet.allOf(Authorization.class)); - @Nullable - public static Authorization parse(Object o) { + @CheckForNull + public static Authorization parse(@Nullable Object o) { + if (o == null) { + return null; + } if (o instanceof Authorization) { return (Authorization) o; } - return o != null ? valueOf(o.toString().toUpperCase(Locale.ROOT)) : null; + if (o instanceof com.netflix.spinnaker.security.Authorization) { + return KORK_TO_FIAT.get(o); + } + var string = o.toString().toUpperCase(Locale.ROOT); + try { + return valueOf(string); + } catch (IllegalArgumentException ignored) { + return null; + } } } diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/AuthorizationMapControlled.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/AuthorizationMapControlled.java new file mode 100644 index 000000000..8c39aef39 --- /dev/null +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/AuthorizationMapControlled.java @@ -0,0 +1,33 @@ +/* + * Copyright 2023 Apple, Inc. + * + * 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 com.netflix.spinnaker.fiat.model; + +import com.netflix.spinnaker.security.PermissionMapControlled; +import javax.annotation.Nullable; + +/** + * Common interface for access-controlled classes which use a permission map of {@link + * Authorization} enums. + */ +public interface AuthorizationMapControlled extends PermissionMapControlled { + @Nullable + @Override + default Authorization valueOf(@Nullable Object authorization) { + return Authorization.parse(authorization); + } +} diff --git a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/SpinnakerAuthorities.java b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/SpinnakerAuthorities.java index a934cd97f..95b1f37df 100644 --- a/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/SpinnakerAuthorities.java +++ b/fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/SpinnakerAuthorities.java @@ -20,25 +20,12 @@ import org.springframework.security.core.authority.SimpleGrantedAuthority; /** - * Constants and utilities for working with Spring Security GrantedAuthority objects specific to - * Spinnaker and Fiat. Spinnaker-specific roles such as admin and account manager are represented - * here as granted authorities. + * Migrated to {@link com.netflix.spinnaker.security.SpinnakerAuthorities} in {@code kork-security}. + * This is left for backward compatibility. */ -public class SpinnakerAuthorities { - public static final String ADMIN = "SPINNAKER_ADMIN"; - /** Granted authority for Spinnaker administrators. */ - public static final GrantedAuthority ADMIN_AUTHORITY = new SimpleGrantedAuthority(ADMIN); - +public class SpinnakerAuthorities extends com.netflix.spinnaker.security.SpinnakerAuthorities { public static final String ACCOUNT_MANAGER = "SPINNAKER_ACCOUNT_MANAGER"; /** Granted authority for Spinnaker account managers. */ public static final GrantedAuthority ACCOUNT_MANAGER_AUTHORITY = new SimpleGrantedAuthority(ACCOUNT_MANAGER); - - /** Granted authority for anonymous users. */ - public static final GrantedAuthority ANONYMOUS_AUTHORITY = forRoleName("ANONYMOUS"); - - /** Creates a granted authority corresponding to the provided name of a role. */ - public static GrantedAuthority forRoleName(String role) { - return new SimpleGrantedAuthority(String.format("ROLE_%s", role)); - } }