Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates for #6911 and #6918 #1131

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Updates for #6911 and #6918 #1131

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fef018e
fix(api): Use consistent anonymous principal
jvz Jan 3, 2024
08ec1ef
refactor(core): Use kork-security abstractions
jvz Jan 3, 2024
026ad25
Merge branch 'master' into issue-6918
jvz Jan 4, 2024
df29277
refactor(api): Extract common anonymous auth
jvz Jan 8, 2024
683d0fa
Merge branch 'master' into issue-6918
jvz Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions ...n/java/com/netflix/spinnaker/fiat/shared/AuthenticatedRequestAuthenticationConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -41,8 +42,8 @@ public Authentication convert(HttpServletRequest request) {
.orElseGet(
() ->
new AnonymousAuthenticationToken(
"anonymous",
"anonymous",
AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS")));
SpinnakerUsers.ANONYMOUS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notes for reviewers -- the new code looks like it does exactly the same thing as the old code, so this is "just" a refactor.

SpinnakerUsers.ANONYMOUS,
List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY)));
}
}
3 changes: 3 additions & 0 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -143,6 +144,8 @@ protected void configure(HttpSecurity http) throws Exception {
.exceptionHandling()
.and()
.anonymous()
// match the same anonymous userid as expected elsewhere
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different from .anonymous() above? Can you write a test that demonstrates this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default settings in this DSL set the principal to the string anonymousUser. The DSL is tricky to test since it's mostly a DSL for setting up beans.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So using the updated Spring Security DSL, this whole method would look more like the following:

      http.servletApi(Customizer.withDefaults())
          .exceptionHandling(Customizer.withDefaults())
          .anonymous(anonymous -> anonymous.principal(SpinnakerUsers.ANONYMOUS))
          .addFilterBefore(
              new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
              AnonymousAuthenticationFilter.class);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see before. .principal(SpinnakerUsers.ANONYMOUS) modifies how .anonymous() works...it's not another mechanism in addition to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll figure out some sort of test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out testing this is futile. I've extracted some constants to make this more consistent.

.principal(SpinnakerUsers.ANONYMOUS)
.and()
.addFilterBefore(
new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
Expand Down
5 changes: 4 additions & 1 deletion fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
}
77 changes: 18 additions & 59 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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> authorizationFailure = new ThreadLocal<>();

private final Registry registry;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -370,34 +344,19 @@ 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;
}

public static Optional<AuthorizationFailure> 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,
Expand Down Expand Up @@ -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();
}

Expand Down
28 changes: 28 additions & 0 deletions ...-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
24 changes: 24 additions & 0 deletions ...pi/src/test/groovy/com/netflix/spinnaker/fiat/shared/PermissionsControlledResource.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
}
2 changes: 1 addition & 1 deletion fiat-core/fiat-core.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down
27 changes: 24 additions & 3 deletions fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/Authorization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -28,14 +30,33 @@ public enum Authorization {
EXECUTE,
CREATE;

private static final Map<com.netflix.spinnaker.security.Authorization, Authorization>
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<Authorization> 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;
}
}
}
33 changes: 33 additions & 0 deletions fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/AuthorizationMapControlled.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<Authorization> {
@Nullable
@Override
default Authorization valueOf(@Nullable Object authorization) {
return Authorization.parse(authorization);
}
}
Loading