From 97ea0d3025f9d903cc3e6310ed0e3811ad9fce65 Mon Sep 17 00:00:00 2001 From: David Byron <82477955+dbyron-sf@users.noreply.github.com> Date: Sat, 27 Apr 2024 19:48:10 -0700 Subject: [PATCH] fix(artifacts): authenticate against AuthenticatedRequest.getSpinnakerUser in S3ArtifactStoreGetter (#1180) * chore(build): give local gradle invocations more memory The same amount that github actions uses, to avoid errors like: Expiring Daemon because JVM heap space is exhausted Expiring Daemon because JVM heap space is exhausted FAILURE: Build failed with an exception. * What went wrong: Gradle build daemon has been stopped: JVM garbage collector thrashing and after running out of JVM memory and after running out of JVM memory * fix(artifacts): authenticate against AuthenticatedRequest.getSpinnakerUser in S3ArtifactStoreGetter instead of SecurityContextHolder.getContext() which might be null. Previously hasAuthorization would only user userId for logging. Now it's used for authentication too. This fixes the bug that https://github.com/spinnaker/kork/pull/1178 demonstrates. --- gradle.properties | 2 ++ .../s3/S3ArtifactStoreConfiguration.java | 10 +++---- .../s3/S3ArtifactStoreGetter.java | 16 +++++----- .../s3/S3ArtifactStoreGetterTest.java | 30 ++++++------------- 4 files changed, 23 insertions(+), 35 deletions(-) diff --git a/gradle.properties b/gradle.properties index 7385599a3..9bfc28da7 100644 --- a/gradle.properties +++ b/gradle.properties @@ -3,3 +3,5 @@ org.gradle.parallel=true spinnakerGradleVersion=8.32.1 targetJava11=true includeRuntimes=actuator,core,eureka,retrofit,secrets-aws,secrets-gcp,stackdriver,swagger,tomcat,web + +org.gradle.jvmargs=-Xmx2g -Xms2g diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java index e792d0c13..14ebcc73d 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java @@ -19,6 +19,7 @@ import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreGetter; import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreStorer; import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreURIBuilder; +import com.netflix.spinnaker.security.UserPermissionEvaluator; import java.net.URI; import java.util.Optional; import lombok.extern.log4j.Log4j2; @@ -27,7 +28,6 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.security.access.PermissionEvaluator; import software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; @@ -57,18 +57,18 @@ public ArtifactStoreStorer artifactStoreStorer( @Bean public ArtifactStoreGetter artifactStoreGetter( - Optional permissionEvaluator, + Optional userPermissionEvaluator, ArtifactStoreConfigurationProperties properties, @Qualifier("artifactS3Client") S3Client s3Client) { - if (permissionEvaluator.isEmpty()) { + if (userPermissionEvaluator.isEmpty()) { log.warn( - "PermissionEvaluator is not present. This means anyone will be able to access any artifact in the store."); + "UserPermissionEvaluator is not present. This means anyone will be able to access any artifact in the store."); } String bucket = properties.getS3().getBucket(); - return new S3ArtifactStoreGetter(s3Client, permissionEvaluator.orElse(null), bucket); + return new S3ArtifactStoreGetter(s3Client, userPermissionEvaluator.orElse(null), bucket); } @Bean diff --git a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java index 778c46528..5c31b6b43 100644 --- a/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java +++ b/kork-artifacts/src/main/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java @@ -23,13 +23,11 @@ import com.netflix.spinnaker.kork.artifacts.artifactstore.ArtifactStoreGetter; import com.netflix.spinnaker.kork.artifacts.model.Artifact; import com.netflix.spinnaker.security.AuthenticatedRequest; +import com.netflix.spinnaker.security.UserPermissionEvaluator; import java.util.Base64; import java.util.NoSuchElementException; import lombok.extern.log4j.Log4j2; -import org.springframework.security.access.PermissionEvaluator; import org.springframework.security.authentication.AuthenticationServiceException; -import org.springframework.security.core.Authentication; -import org.springframework.security.core.context.SecurityContextHolder; import software.amazon.awssdk.core.ResponseBytes; import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.model.GetObjectRequest; @@ -42,14 +40,14 @@ @Log4j2 public class S3ArtifactStoreGetter implements ArtifactStoreGetter { private final S3Client s3Client; - private final PermissionEvaluator permissionEvaluator; + private final UserPermissionEvaluator userPermissionEvaluator; private final String bucket; public S3ArtifactStoreGetter( - S3Client s3Client, PermissionEvaluator permissionEvaluator, String bucket) { + S3Client s3Client, UserPermissionEvaluator userPermissionEvaluator, String bucket) { this.s3Client = s3Client; this.bucket = bucket; - this.permissionEvaluator = permissionEvaluator; + this.userPermissionEvaluator = userPermissionEvaluator; } /** @@ -99,11 +97,11 @@ private void hasAuthorization(ArtifactReferenceURI uri, String userId) { .filter(t -> t.key().equals(ENFORCE_PERMS_KEY)) .findFirst() .orElse(null); - Authentication auth = SecurityContextHolder.getContext().getAuthentication(); if (tag == null - || (permissionEvaluator != null - && !permissionEvaluator.hasPermission(auth, tag.value(), "application", "READ"))) { + || (userPermissionEvaluator != null + && !userPermissionEvaluator.hasPermission( + userId, tag.value(), "application", "READ"))) { log.error( "Could not authenticate to retrieve artifact user={} applicationOfStoredArtifact={}", userId, diff --git a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetterTest.java b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetterTest.java index 06bd43ee1..848f7498a 100644 --- a/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetterTest.java +++ b/kork-artifacts/src/test/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetterTest.java @@ -20,7 +20,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -29,9 +28,9 @@ import com.netflix.spinnaker.kork.artifacts.model.Artifact; import com.netflix.spinnaker.kork.common.Header; import com.netflix.spinnaker.security.AuthenticatedRequest; +import com.netflix.spinnaker.security.UserPermissionEvaluator; import java.util.List; import org.junit.jupiter.api.Test; -import org.springframework.security.access.PermissionEvaluator; import software.amazon.awssdk.core.ResponseBytes; import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.model.GetObjectRequest; @@ -46,16 +45,11 @@ public class S3ArtifactStoreGetterTest { public void testGetAuthenticatedWithUser() { // Verify that having a user set is sufficient to authenticate against that // user. - // - // Currently, S3ArtifactStoreGetter.hasAuthorization uses - // SecurityContextHolder.getContext().getAuthentication(). In this test - // that's null. In orca at least, there are some calls to - // get/hasAuthorization where this is also true, but there is a user - // available in AuthenticatedRequest. // given: String application = "my-application"; - AuthenticatedRequest.set(Header.USER, "my-user"); + String user = "my-user"; + AuthenticatedRequest.set(Header.USER, user); S3Client client = mock(S3Client.class); @@ -72,20 +66,16 @@ public void testGetAuthenticatedWithUser() { when(client.getObjectTagging(any(GetObjectTaggingRequest.class))) .thenReturn(getObjectTaggingResponse); - PermissionEvaluator permissionEvaluator = mock(PermissionEvaluator.class); + UserPermissionEvaluator userPermissionEvaluator = mock(UserPermissionEvaluator.class); - // FIXME: The current behavior is to call permissionEvaluator.hasPermission - // with a null Authentication object. The correct behavior is to - // authenticate against AuthenticatedRequest.getSpinnakerUser(). - // // It's arbitrary whether to give permission or not (i.e. return true or // false). Choose true since there are then no exceptions to deal with. - when(permissionEvaluator.hasPermission( - isNull(), eq(application), eq("application"), eq("READ"))) + when(userPermissionEvaluator.hasPermission( + eq(user), eq(application), eq("application"), eq("READ"))) .thenReturn(true); S3ArtifactStoreGetter artifactStoreGetter = - new S3ArtifactStoreGetter(client, permissionEvaluator, "my-bucket"); + new S3ArtifactStoreGetter(client, userPermissionEvaluator, "my-bucket"); ArtifactReferenceURI uri = mock(ArtifactReferenceURI.class); @@ -95,9 +85,7 @@ public void testGetAuthenticatedWithUser() { // then assertThat(artifact).isNotNull(); - // FIXME: Again, the correct behavior is to authenticate against - // AuthenticatedRequest.getSpinnakerUser(). - verify(permissionEvaluator) - .hasPermission(isNull(), eq(application), eq("application"), eq("READ")); + verify(userPermissionEvaluator) + .hasPermission(eq(user), eq(application), eq("application"), eq("READ")); } }