fix(artifacts): authenticate against AuthenticatedRequest.getSpinnake…

…rUser 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 spinnaker#1178 demonstrates.
dbyron-sf · Apr 27, 2024 · 1a3d826 · 1a3d826
1 parent 74bda00
commit 1a3d826
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 35 deletions.
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"));
   }
 }