From fef018e394b05f525359ace31cff225ea55e8022 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Wed, 3 Jan 2024 12:12:03 -0600 Subject: [PATCH 1/3] fix(api): Use consistent anonymous principal Spring Security uses an anonymous principal of "anonymousUser" by default which does not match the rest of Spinnaker where the anonymous user is expected to have the username "anonymous". This ensures that fiat-api code consistently uses the same anonymous principal. Related to https://github.com/spinnaker/spinnaker/issues/6918 --- ...ticatedRequestAuthenticationConverter.java | 9 +++++---- .../fiat/shared/FiatAuthenticationConfig.java | 3 +++ .../shared/FiatAuthenticationConverter.java | 5 ++++- fiat-core/fiat-core.gradle | 2 +- .../fiat/model/SpinnakerAuthorities.java | 19 +++---------------- 5 files changed, 16 insertions(+), 22 deletions(-) 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..050cfff0e 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 @@ -17,11 +17,12 @@ package com.netflix.spinnaker.fiat.shared; import com.netflix.spinnaker.security.AuthenticatedRequest; +import com.netflix.spinnaker.security.SpinnakerAuthorities; +import com.netflix.spinnaker.security.SpinnakerUsers; 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; @@ -41,8 +42,8 @@ public Authentication convert(HttpServletRequest request) { .orElseGet( () -> new AnonymousAuthenticationToken( - "anonymous", - "anonymous", - AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS"))); + SpinnakerUsers.ANONYMOUS, + SpinnakerUsers.ANONYMOUS, + List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY))); } } 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..994777a09 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 @@ -28,6 +28,7 @@ import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator; import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor; import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger; +import com.netflix.spinnaker.security.SpinnakerUsers; import lombok.Setter; import lombok.val; import okhttp3.OkHttpClient; @@ -143,6 +144,8 @@ protected void configure(HttpSecurity http) throws Exception { .exceptionHandling() .and() .anonymous() + // match the same anonymous userid as expected elsewhere + .principal(SpinnakerUsers.ANONYMOUS) .and() .addFilterBefore( new FiatAuthenticationFilter(fiatStatus, authenticationConverter), 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..e6ee515c5 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,6 +19,7 @@ import com.netflix.spinnaker.fiat.model.SpinnakerAuthorities; import com.netflix.spinnaker.fiat.model.UserPermission; import com.netflix.spinnaker.kork.common.Header; +import com.netflix.spinnaker.security.SpinnakerUsers; import java.util.List; import javax.servlet.http.HttpServletRequest; import lombok.RequiredArgsConstructor; @@ -47,6 +48,8 @@ public Authentication convert(HttpServletRequest request) { } } return new AnonymousAuthenticationToken( - "anonymous", "anonymous", List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY)); + SpinnakerUsers.ANONYMOUS, + SpinnakerUsers.ANONYMOUS, + List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY)); } } 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/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)); - } } From 08ec1ef2f0d6b9f77d258cfb4a5ac12f9d7f7b6d Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Wed, 3 Jan 2024 12:20:02 -0600 Subject: [PATCH 2/3] refactor(core): Use kork-security abstractions Related to https://github.com/spinnaker/spinnaker/issues/6911 --- .../fiat/shared/FiatPermissionEvaluator.java | 77 +++++-------------- .../shared/FiatPermissionEvaluatorSpec.groovy | 28 +++++++ .../PermissionsControlledResource.groovy | 24 ++++++ .../spinnaker/fiat/model/Authorization.java | 27 ++++++- .../model/AuthorizationMapControlled.java | 33 ++++++++ 5 files changed, 127 insertions(+), 62 deletions(-) create mode 100644 fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/PermissionsControlledResource.groovy create mode 100644 fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/AuthorizationMapControlled.java 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/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/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); + } +} From df292770d499a539a94b9e8afbcb5f5d3de4db4f Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Mon, 8 Jan 2024 12:57:41 -0600 Subject: [PATCH 3/3] refactor(api): Extract common anonymous auth This extracts some constants for use in configuring anonymous authentication. --- fiat-api/fiat-api.gradle | 4 +- ...ticatedRequestAuthenticationConverter.java | 10 +-- .../fiat/shared/FiatAuthenticationConfig.java | 37 ----------- .../shared/FiatAuthenticationConverter.java | 8 +-- .../FiatWebSecurityConfigurerAdapter.java | 64 +++++++++++++++++++ 5 files changed, 69 insertions(+), 54 deletions(-) create mode 100644 fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatWebSecurityConfigurerAdapter.java 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 050cfff0e..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 @@ -17,11 +17,8 @@ package com.netflix.spinnaker.fiat.shared; import com.netflix.spinnaker.security.AuthenticatedRequest; -import com.netflix.spinnaker.security.SpinnakerAuthorities; -import com.netflix.spinnaker.security.SpinnakerUsers; 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.web.authentication.AuthenticationConverter; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; @@ -39,11 +36,6 @@ public Authentication convert(HttpServletRequest request) { .map( user -> (Authentication) new PreAuthenticatedAuthenticationToken(user, "N/A", List.of())) - .orElseGet( - () -> - new AnonymousAuthenticationToken( - SpinnakerUsers.ANONYMOUS, - SpinnakerUsers.ANONYMOUS, - List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY))); + .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 994777a09..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 @@ -28,7 +28,6 @@ import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator; import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor; import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger; -import com.netflix.spinnaker.security.SpinnakerUsers; import lombok.Setter; import lombok.val; import okhttp3.OkHttpClient; @@ -42,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; @@ -113,43 +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() - // match the same anonymous userid as expected elsewhere - .principal(SpinnakerUsers.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 e6ee515c5..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,11 +19,8 @@ import com.netflix.spinnaker.fiat.model.SpinnakerAuthorities; import com.netflix.spinnaker.fiat.model.UserPermission; import com.netflix.spinnaker.kork.common.Header; -import com.netflix.spinnaker.security.SpinnakerUsers; -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; @@ -47,9 +44,6 @@ public Authentication convert(HttpServletRequest request) { user, "N/A", permission.toGrantedAuthorities()); } } - return new AnonymousAuthenticationToken( - SpinnakerUsers.ANONYMOUS, - SpinnakerUsers.ANONYMOUS, - List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY)); + return FiatWebSecurityConfigurerAdapter.ANONYMOUS; } } 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); + } +}