fix(artifacts): authenticate against AuthenticatedRequest.getSpinnake…

…rUser 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 #1178 demonstrates.
spinnaker · Apr 28, 2024 · 97ea0d3 · 97ea0d3
1 parent 0bd4ff8
commit 97ea0d3
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 35 deletions.
diff --git 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/...a/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreConfiguration.java b/...a/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> permissionEvaluator,
+      Optional<UserPermissionEvaluator> 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/...ain/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetter.java b/...ain/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/...java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreGetterTest.java b/...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"));
   }
 }