From ef68482a24a20da32dbd19445b2c27db0dba337a Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Mon, 31 Jul 2023 11:52:52 -0500 Subject: [PATCH 1/2] feature(security): Support more principal types This updates `AuthenticatedRequest::getSpinnakerUser` to support more principal types besides `UserDetails` including `AuthenticatedPrincipal` (used by OAuth2 and SAML2 Spring Security libraries) and `Principal` (the generic Java API). Also adds some related utility code for granted authorities. --- .../security/AuthenticatedRequest.java | 15 +++- .../security/SpinnakerAuthorities.java | 90 +++++++++++++++++++ .../security/AuthenticatedRequestSpec.groovy | 17 ++++ 3 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/AuthenticatedRequest.java b/kork-security/src/main/java/com/netflix/spinnaker/security/AuthenticatedRequest.java index 736bffc76..38a674b37 100644 --- a/kork-security/src/main/java/com/netflix/spinnaker/security/AuthenticatedRequest.java +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/AuthenticatedRequest.java @@ -20,6 +20,7 @@ import com.google.common.base.Preconditions; import com.netflix.spinnaker.kork.common.Header; +import java.security.Principal; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -33,6 +34,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.slf4j.MDC; +import org.springframework.security.core.AuthenticatedPrincipal; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.core.userdetails.User; @@ -85,9 +87,16 @@ default Optional getSpinnakerUser() { * @return the user id of the provided principal */ default Optional getSpinnakerUser(Object principal) { - return (principal instanceof UserDetails) - ? Optional.ofNullable(((UserDetails) principal).getUsername()) - : get(Header.USER); + if (principal instanceof UserDetails) { + return Optional.ofNullable(((UserDetails) principal).getUsername()); + } + if (principal instanceof AuthenticatedPrincipal) { + return Optional.ofNullable(((AuthenticatedPrincipal) principal).getName()); + } + if (principal instanceof Principal) { + return Optional.ofNullable(((Principal) principal).getName()); + } + return Optional.ofNullable(principal).map(Object::toString).or(() -> get(Header.USER)); } } diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java b/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java new file mode 100644 index 000000000..75fba07d4 --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/SpinnakerAuthorities.java @@ -0,0 +1,90 @@ +/* + * Copyright 2022 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.security; + +import java.util.Collection; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +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 are represented here as granted authorities with the + * {@code SPINNAKER_} prefix. + */ +public class SpinnakerAuthorities { + private static final String ROLE_PREFIX = "ROLE_"; + + public static final String ADMIN = "SPINNAKER_ADMIN"; + /** Granted authority for Spinnaker administrators. */ + public static final GrantedAuthority ADMIN_AUTHORITY = new SimpleGrantedAuthority(ADMIN); + + /** 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. */ + @Nonnull + public static GrantedAuthority forRoleName(@Nonnull String role) { + return new SimpleGrantedAuthority(ROLE_PREFIX + role); + } + + /** Checks if the given user is a Spinnaker admin. */ + public static boolean isAdmin(@Nullable Authentication authentication) { + return authentication != null + && authentication.getAuthorities().contains(SpinnakerAuthorities.ADMIN_AUTHORITY); + } + + /** Checks if the given user has the provided role. */ + public static boolean hasRole(@Nullable Authentication authentication, @Nonnull String role) { + return authentication != null && streamRoles(authentication).anyMatch(role::equals); + } + + /** Checks if the given user has any of the provided roles. */ + public static boolean hasAnyRole( + @Nullable Authentication authentication, @Nonnull Collection roles) { + return authentication != null && streamRoles(authentication).anyMatch(roles::contains); + } + + /** Gets the list of roles assigned to the given user. */ + @Nonnull + public static List getRoles(@Nullable Authentication authentication) { + if (authentication == null) { + return List.of(); + } + return streamRoles(authentication).distinct().collect(Collectors.toList()); + } + + @Nonnull + private static Stream streamRoles(@Nonnull Authentication authentication) { + return authentication.getAuthorities().stream() + .filter(SpinnakerAuthorities::isRole) + .map(SpinnakerAuthorities::getRole); + } + + private static boolean isRole(@Nonnull GrantedAuthority authority) { + return authority.getAuthority().startsWith(ROLE_PREFIX); + } + + private static String getRole(@Nonnull GrantedAuthority authority) { + return authority.getAuthority().substring(ROLE_PREFIX.length()); + } +} diff --git a/kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthenticatedRequestSpec.groovy b/kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthenticatedRequestSpec.groovy index cc15df260..10f555f45 100644 --- a/kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthenticatedRequestSpec.groovy +++ b/kork-security/src/test/groovy/com/netflix/spinnaker/security/AuthenticatedRequestSpec.groovy @@ -18,6 +18,9 @@ package com.netflix.spinnaker.security import com.netflix.spinnaker.kork.common.Header import org.slf4j.MDC +import org.springframework.security.authentication.TestingAuthenticationToken +import org.springframework.security.core.AuthenticatedPrincipal +import org.springframework.security.core.context.SecurityContextHolder import spock.lang.Specification class AuthenticatedRequestSpec extends Specification { @@ -136,4 +139,18 @@ class AuthenticatedRequestSpec extends Specification { then: closure.run() } + + void "should support AuthenticatedPrincipal"() { + when: + def user = new TestingAuthenticationToken(new TestPrincipal(name: 'foo'), '', + [SpinnakerAuthorities.forRoleName('alpha')]) + SecurityContextHolder.context.authentication = user + + then: + AuthenticatedRequest.spinnakerUser.get() == 'foo' + } + + static class TestPrincipal implements AuthenticatedPrincipal { + String name + } } From 0b730dc50e01f9746d38e6329d150ee195c7197d Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Mon, 31 Jul 2023 11:55:57 -0500 Subject: [PATCH 2/2] feature(web): Update AuthenticatedRequestFilter to support more principals This updates the filter for a few related things: - Support more types of `Authentication` principals - Add `AllowedAccountAuthority` for simpler authority representation of allowed accounts - Use the `SecurityContextRepository` API from Spring Security instead of relying on internal details of its API - Normalize anonymous users into the userid `anonymous` - Add allowed account authorities to `User` authorities - Use the `ROLE_` granted authority prefix for roles as already used in Fiat --- .../security/AllowedAccountAuthority.java | 41 +++++++++ .../com/netflix/spinnaker/security/User.java | 13 +-- .../spinnaker/security/UserSpec.groovy | 13 ++- .../filters/AuthenticatedRequestFilter.groovy | 87 +++++++++++-------- 4 files changed, 108 insertions(+), 46 deletions(-) create mode 100644 kork-security/src/main/java/com/netflix/spinnaker/security/AllowedAccountAuthority.java diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/AllowedAccountAuthority.java b/kork-security/src/main/java/com/netflix/spinnaker/security/AllowedAccountAuthority.java new file mode 100644 index 000000000..e28aa0edb --- /dev/null +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/AllowedAccountAuthority.java @@ -0,0 +1,41 @@ +/* + * 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.security; + +import com.netflix.spinnaker.kork.annotations.NonnullByDefault; +import java.util.Locale; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import org.springframework.security.core.GrantedAuthority; + +/** + * Represents an authorization to use an account. Allowed account authorities are based on an older + * security mechanism used by Spinnaker before other account types besides cloud providers and + * permissions were added. + */ +@Getter +@EqualsAndHashCode(of = "account") +@NonnullByDefault +public class AllowedAccountAuthority implements GrantedAuthority { + private final String account; + private final String authority; + + public AllowedAccountAuthority(String account) { + this.account = account.toLowerCase(Locale.ROOT); + authority = AllowedAccountsAuthorities.PREFIX + this.account; + } +} diff --git a/kork-security/src/main/java/com/netflix/spinnaker/security/User.java b/kork-security/src/main/java/com/netflix/spinnaker/security/User.java index 64697a4f5..3eeddeb7f 100644 --- a/kork-security/src/main/java/com/netflix/spinnaker/security/User.java +++ b/kork-security/src/main/java/com/netflix/spinnaker/security/User.java @@ -18,14 +18,14 @@ import static java.util.Collections.unmodifiableCollection; import static java.util.Collections.unmodifiableList; -import static java.util.stream.Collectors.toList; import com.fasterxml.jackson.annotation.JsonIgnore; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.core.userdetails.UserDetails; import org.springframework.util.StringUtils; @@ -57,10 +57,11 @@ public class User implements UserDetails { @Override public List getAuthorities() { - return roles.stream() - .filter(StringUtils::hasText) - .map(SimpleGrantedAuthority::new) - .collect(toList()); + Stream accountAuthorities = + allowedAccounts.stream().filter(StringUtils::hasText).map(AllowedAccountAuthority::new); + Stream roleAuthorities = + roles.stream().filter(StringUtils::hasText).map(SpinnakerAuthorities::forRoleName); + return Stream.concat(accountAuthorities, roleAuthorities).collect(Collectors.toList()); } /** Not used */ diff --git a/kork-security/src/test/groovy/com/netflix/spinnaker/security/UserSpec.groovy b/kork-security/src/test/groovy/com/netflix/spinnaker/security/UserSpec.groovy index ca98bb327..461f4e1c6 100644 --- a/kork-security/src/test/groovy/com/netflix/spinnaker/security/UserSpec.groovy +++ b/kork-security/src/test/groovy/com/netflix/spinnaker/security/UserSpec.groovy @@ -69,7 +69,16 @@ class UserSpec extends Specification { def "should filter out empty roles"() { expect: new User(roles: [""]).getAuthorities().isEmpty() - new User(roles: ["", "bar"]).getAuthorities()*.getAuthority() == ["bar"] - new User(roles: ["foo", "", "bar"]).getAuthorities()*.getAuthority() == ["foo", "bar"] + new User(roles: ["", "bar"]).getAuthorities()*.getAuthority() == ["ROLE_bar"] + new User(roles: ["foo", "", "bar"]).getAuthorities()*.getAuthority() == ["ROLE_foo", "ROLE_bar"] + } + + def "should translate allowed accounts into granted authorities"() { + setup: + def user = new User(allowedAccounts: ['a', 'b', 'c']) + + expect: + user.getAuthorities()*.getAuthority() == + ['ALLOWED_ACCOUNT_a', 'ALLOWED_ACCOUNT_b', 'ALLOWED_ACCOUNT_c'] } } diff --git a/kork-web/src/main/groovy/com/netflix/spinnaker/filters/AuthenticatedRequestFilter.groovy b/kork-web/src/main/groovy/com/netflix/spinnaker/filters/AuthenticatedRequestFilter.groovy index 53050b8f9..9964a394b 100644 --- a/kork-web/src/main/groovy/com/netflix/spinnaker/filters/AuthenticatedRequestFilter.groovy +++ b/kork-web/src/main/groovy/com/netflix/spinnaker/filters/AuthenticatedRequestFilter.groovy @@ -17,24 +17,24 @@ package com.netflix.spinnaker.filters import com.netflix.spinnaker.kork.common.Header -import com.netflix.spinnaker.security.AllowedAccountsAuthorities +import com.netflix.spinnaker.security.AllowedAccountAuthority import com.netflix.spinnaker.security.AuthenticatedRequest import groovy.util.logging.Slf4j +import org.springframework.security.core.context.SecurityContext import org.springframework.security.core.context.SecurityContextHolder -import org.springframework.security.core.context.SecurityContextImpl -import org.springframework.security.core.userdetails.UserDetails +import org.springframework.security.web.context.HttpRequestResponseHolder +import org.springframework.security.web.context.HttpSessionSecurityContextRepository +import org.springframework.security.web.context.SecurityContextRepository +import org.springframework.web.filter.OncePerRequestFilter -import javax.servlet.Filter import javax.servlet.FilterChain -import javax.servlet.FilterConfig import javax.servlet.ServletException -import javax.servlet.ServletRequest -import javax.servlet.ServletResponse import javax.servlet.http.HttpServletRequest +import javax.servlet.http.HttpServletResponse import java.security.cert.X509Certificate @Slf4j -class AuthenticatedRequestFilter implements Filter { +class AuthenticatedRequestFilter extends OncePerRequestFilter { private static final String X509_CERTIFICATE = "javax.servlet.request.X509Certificate" /* @@ -54,63 +54,77 @@ class AuthenticatedRequestFilter implements Filter { private final boolean extractSpinnakerUserOriginHeader private final boolean forceNewSpinnakerRequestId private final boolean clearAuthenticatedRequestPostFilter + private final SecurityContextRepository securityContextRepository public AuthenticatedRequestFilter(boolean extractSpinnakerHeaders = false, boolean extractSpinnakerUserOriginHeader = false, boolean forceNewSpinnakerRequestId = false, - boolean clearAuthenticatedRequestPostFilter = true) { + boolean clearAuthenticatedRequestPostFilter = true, + SecurityContextRepository securityContextRepository = null) { this.extractSpinnakerHeaders = extractSpinnakerHeaders this.extractSpinnakerUserOriginHeader = extractSpinnakerUserOriginHeader this.forceNewSpinnakerRequestId = forceNewSpinnakerRequestId this.clearAuthenticatedRequestPostFilter = clearAuthenticatedRequestPostFilter + this.securityContextRepository = securityContextRepository ?: new HttpSessionSecurityContextRepository() } @Override - void init(FilterConfig filterConfig) throws ServletException {} - - @Override - void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - def spinnakerUser = null - def spinnakerAccounts = null - HashMap otherSpinnakerHeaders = new HashMap<>() + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws ServletException, IOException { + String spinnakerUser = null + String spinnakerAccounts = null + Map otherSpinnakerHeaders = [:] try { - def session = ((HttpServletRequest) request).getSession(false) - def securityContext = (SecurityContextImpl) session?.getAttribute("SPRING_SECURITY_CONTEXT") - if (!securityContext) { - securityContext = SecurityContextHolder.getContext() + SecurityContext securityContext + // first check if there is a session with a SecurityContext (but don't create the session yet) + if (securityContextRepository.containsContext(request)) { + def holder = new HttpRequestResponseHolder(request, response) + securityContext = securityContextRepository.loadContext(holder) + } else { + // otherwise, try checking SecurityContextHolder as this may be a fresh session + securityContext = SecurityContextHolder.context } - def principal = securityContext?.authentication?.principal - if (principal && principal instanceof UserDetails) { - spinnakerUser = principal.username - spinnakerAccounts = AllowedAccountsAuthorities.getAllowedAccounts(principal).join(",") + // next, if an authenticated user is present, get their username and allowed account authorities + // (when using OAuth2, the principal may be an OAuth2User or OidcUser rather than a UserDetails instance) + def auth = securityContext.authentication + if (auth && auth.authenticated) { + spinnakerUser = auth.name + spinnakerAccounts = auth.authorities + .grep(AllowedAccountAuthority) + .collect { (it as AllowedAccountAuthority).account } + .join(',') } } catch (Exception e) { log.error("Unable to extract spinnaker user and account information", e) } if (extractSpinnakerHeaders) { - def httpServletRequest = (HttpServletRequest) request - spinnakerUser = spinnakerUser ?: httpServletRequest.getHeader(Header.USER.getHeader()) - spinnakerAccounts = spinnakerAccounts ?: httpServletRequest.getHeader(Header.ACCOUNTS.getHeader()) - - Enumeration headers = httpServletRequest.getHeaderNames() + // for backend services using the x-spinnaker-user header for authentication + spinnakerUser = spinnakerUser ?: request.getHeader(Header.USER.header) + spinnakerAccounts = spinnakerAccounts ?: request.getHeader(Header.ACCOUNTS.header) - for (header in headers) { + for (header in request.headerNames) { String headerUpper = header.toUpperCase() if (headerUpper.startsWith(Header.XSpinnakerPrefix)) { - otherSpinnakerHeaders.put(headerUpper, httpServletRequest.getHeader(header)) + otherSpinnakerHeaders[headerUpper] = request.getHeader(header) } } } + // normalize anonymous principal name in case of misconfiguration + // [anonymous] => anonymous (occasional use in Kayenta, potentially used in Orca; shows up in test data) + // anonymousUser => anonymous (default principal in Spring Security's AnonymousAuthenticationFilter) + // __unrestricted_user__ => anonymous (principal used in Fiat for anonymous) + if (spinnakerUser ==~ /\[?anonymous]?User|__unrestricted_user__/) { + spinnakerUser = 'anonymous' + } + if (extractSpinnakerUserOriginHeader) { - otherSpinnakerHeaders.put( - Header.USER_ORIGIN.getHeader(), - "deck".equalsIgnoreCase(((HttpServletRequest) request).getHeader("X-RateLimit-App")) ? "deck" : "api" - ) + def app = request.getHeader('X-RateLimit-App') + def origin = 'deck'.equalsIgnoreCase(app) ? 'deck' : 'api' + otherSpinnakerHeaders[Header.USER_ORIGIN.header] = origin } if (forceNewSpinnakerRequestId) { @@ -151,7 +165,4 @@ class AuthenticatedRequestFilter implements Filter { } } } - - @Override - void destroy() {} }